Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver/usbloader: rename RKUSBDriver to reflect rkdeveloptool usage #1542

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

a3f
Copy link
Contributor

@a3f a3f commented Nov 12, 2024

To interact with a Rockchip SoC in BootROM mode over USB, Rockchip
offers the rkdeveloptool, which is among others already packaged in
Debian.

We currently support this via the RKUSBDriver, which binds to the
RKUSBLoader resource, which currently lists a single VID/PID pair.

Actually making use of this is where things get confusing:
Labgrid will look up the rk-usb-loader key to find rkdeveloptool and then
fall back to a binary named rk-usb-loader. To my knowledge, no one names
their rkdeveloptool that way and rk-usb-loader has since become the
name of the Rockchip USB loader distributed by barebox.

On systems, like the LXA TAC, this is doubly confusing: There's a
rk-usb-loader binary, which Labgrid would use by default, but it's not
compatible with rkdeveloptool, supports only the RK35xx SoCs barebox
supports and is currently not supported by Labgrid at all.

Given that RKUSBLoader only supports a single SoC, our documentation is
wrong (it references the unrelated barebox' rk-usb-loader) and that
our usage of rkdeveloptool goes beyond the BootROM's USB protocol and
additionally flashes persistent media by talking to a first stage
usb_loader that's uploaded first, I think the best course of action
is to rename both the driver and the tool key to reflect that
rkdeveloptool is actually used.


I have yet to test the first (rename) commit, but posting it anyway to get some feedback if the rename is ok.

@a3f
Copy link
Contributor Author

a3f commented Nov 12, 2024

The support was originally added in #437. Pinging @otavio and @chtavares for their input.

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.5%. Comparing base (0304ec6) to head (203206a).

Files with missing lines Patch % Lines
labgrid/driver/usbloader.py 33.3% 2 Missing ⚠️
labgrid/remote/client.py 0.0% 1 Missing ⚠️
labgrid/resource/udev.py 0.0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1542   +/-   ##
======================================
  Coverage    56.5%   56.5%           
======================================
  Files         168     168           
  Lines       13111   13111           
======================================
  Hits         7417    7417           
  Misses       5694    5694           
Flag Coverage Δ
3.10 56.5% <33.3%> (ø)
3.11 56.5% <33.3%> (ø)
3.12 56.5% <33.3%> (ø)
3.13 56.5% <33.3%> (ø)
3.9 56.6% <33.3%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@a3f a3f force-pushed the rkdeveloptool-rename branch from 25fc0f3 to d278c0b Compare November 12, 2024 10:17
a3f added a commit to a3f/labgrid that referenced this pull request Nov 12, 2024
The match list doesn't yet support the RK3399pro (2207:330c).
Add it along with the rest of the VID/PIDs listed in rkdeveloptool's
udev rule[1] as well as the VID/PID pairs for the newer RK3566
and RK3588[2].

[1]: https://github.com/rockchip-linux/rkdeveloptool/blob/master/99-rk-rockusb.rules
[2]: labgrid-project#1542 (comment)

Signed-off-by: Ahmad Fatoum <[email protected]>
@a3f a3f force-pushed the rkdeveloptool-rename branch from d278c0b to 4aba887 Compare November 12, 2024 10:34
a3f added 2 commits November 12, 2024 11:58
To interact with a Rockchip SoC in BootROM mode over USB, Rockchip
offers the rkdeveloptool, which is among others already packaged in
Debian.

We currently support this via the RKUSBDriver, which binds to the
RKUSBLoader resource, which currently lists a single VID/PID pair.

Actually making use of this is where things get confusing:
Labgrid will look up the rk-usb-loader key to find rkdeveloptool and then
fall back to a binary named `rk-usb-loader`. To my knowledge, no one names
their rkdeveloptool that way and `rk-usb-loader` has since become the
name of the Rockchip USB loader distributed by barebox.

On systems, like the LXA TAC, this is doubly confusing: There's a
rk-usb-loader binary, which Labgrid would use by default, but it's not
compatible with rkdeveloptool, supports only the RK35xx SoCs barebox
supports and is currently not supported by Labgrid at all.

Given that RKUSBLoader only supports a single SoC, our documentation is
wrong (it references the unrelated barebox' rk-usb-loader) and that
our usage of rkdeveloptool goes beyond the BootROM's USB protocol and
additionally flashes persistent media by talking to a first stage
usb_loader that's uploaded first, I think the best course of action
is to rename both the driver and the tool key to reflect that
rkdeveloptool is actually used.

Signed-off-by: Ahmad Fatoum <[email protected]>
The match list doesn't yet support the RK3399pro (2207:330c).
Add it along with the rest of the VID/PIDs listed in rkdeveloptool's
udev rule[1] as well as the VID/PID pairs for the newer RK3566
and RK3588[2].

[1]: https://github.com/rockchip-linux/rkdeveloptool/blob/master/99-rk-rockusb.rules
[2]: labgrid-project#1542 (comment)

Signed-off-by: Ahmad Fatoum <[email protected]>
@a3f a3f force-pushed the rkdeveloptool-rename branch from 4aba887 to 203206a Compare November 12, 2024 10:59
Copy link

@otavio otavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and your changes are very good. I believe that this makes much clear for the user about the intended goal and the tool that is required for make it to work.

I also think that adding extra device support is very good.

@@ -87,7 +87,7 @@ def load(self, filename=None):

@target_factory.reg_driver
@attr.s(eq=False)
class RKUSBDriver(Driver, BootstrapProtocol):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually let the old RKUSBDriver raise a DeprecationWarning and instead use the new driver, i.e. see NetworkUSBStorageDriver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work transparently, because I am changing the name of the tool key as well, but I can do this, so users are nudged into the right direction.

@QSchulz
Copy link

QSchulz commented Nov 13, 2024

Sorry for chiming in with something that is not entirely related to those changes.

So we sell Rockchip-based SoMs and boards for the last 6-7 years and I've taken interest in labgrid but I've hit a few "weirdnesses" (to me) in that process, that may very well be because I misunderstood how labgrid works.

So, my setup is the following:

  • executor on PC-A
  • client on PC-B

RKUSBLoader is only matching devices with vendor id 0x2207 and model id 0x110a. I've no clue what that represents as the RK3399, PX30 and RK3588 I work with aren't matching that model id. For now I've just added mines in there but I was wondering if it would make more sense to have parameters for that Resource, à-la AndroidUSBFastboot for example. Otherwise we can simply add more to the matching list over time, not sure it's interesting to bind labgrid updates with Rockchip SoC support?

The tool to use to interact with the USB loader on Rockchip needs to run on the executor, but this needs to be specified on the Driver configuration side, which is part of the labgrid-device-config yaml file, which is on PC-B while it's going to run on PC-A. My issue is... there are currently three known tools for USB loading on Rockchip devices, rkdeveloptool, rockusb and this rk-usb-loader. I assume we could have some executors with one binary, and some others with another one. Why would the target conf file need to know on which executor something is going to run? Shouldn't this be related to a Place or the Resource itself?

@a3f
Copy link
Contributor Author

a3f commented Nov 13, 2024

RKUSBLoader is only matching devices with vendor id 0x2207 and model id 0x110a. I've no clue what that represents as the RK3399, PX30 and RK3588 I work with aren't matching that model id.

I am not sure either, maybe RV11?

For now I've just added mines in there but I was wondering if it would make more sense to have parameters for that Resource, à-la AndroidUSBFastboot for example.

I think it's useful anyhow to have the well-known VID/PIDs known to Labgrid. If it's possible to override them, it makes sense to have parameters for it, but the default should be that users add the VIDs/PIDs to Labgrid for shared benefit.

Otherwise we can simply add more to the matching list over time, not sure it's interesting to bind labgrid updates with Rockchip SoC support?

I don't find it unreasonable to update Labgrid when you are interfacing with a new SoC.

The tool to use to interact with the USB loader on Rockchip needs to run on the executor, but this needs to be specified on the Driver configuration side, which is part of the labgrid-device-config yaml file, which is on PC-B while it's going to run on PC-A. My issue is... there are currently three known tools for USB loading on Rockchip devices, rkdeveloptool, rockusb and this rk-usb-loader. I assume we could have some executors with one binary, and some others with another one. Why would the target conf file need to know on which executor something is going to run? Shouldn't this be related to a Place or the Resource itself?

I haven't used rockusb so far, but both rkdeveloptool and rk-usb-loader have an incompatile commnad-line interface.

For i.MX, there are multiple tools as well and Labgrid handles this by separating the resource (RKUSBLoader) from the Driver (which will be called RKDevelopToolUSBDriver in future). A future rk-usb-loader driver could then be called RKUSBLoaderDriver and each would implement the CLI of the underlying host tool.

Best case, both rk-usb-loader and rkdeveloptool are installed into $PATH and no one would need to care where exactly they are located, unless it needs to be overridden. This is already the case for other tools and the first patch in this PR has Rockchip USB support follow suit.

What the client needs to know about though is which driver should handle the resource. This isn't something that we can just leave to the exporter, because the tools tend to have their idiosyncrasies, which leak through the abstraction. Once the driver is configured in your environment YAML though (e.g. with the name of the USB loader binary to use), the rest of Labgrid can interface with it generically via the Boostrap protocol, e.g. in the client it's just labgrid-client -c myconfig.yaml bootstrap barebox-rk3399-pro-n10.img

@QSchulz
Copy link

QSchulz commented Nov 13, 2024

RKUSBLoader is only matching devices with vendor id 0x2207 and model id 0x110a. I've no clue what that represents as the RK3399, PX30 and RK3588 I work with aren't matching that model id.

I am not sure either, maybe RV11?

RV1108 apparently :)

For now I've just added mines in there but I was wondering if it would make more sense to have parameters for that Resource, à-la AndroidUSBFastboot for example.

I think it's useful anyhow to have the well-known VID/PIDs known to Labgrid. If it's possible to override them, it makes sense to have parameters for it, but the default should be that users add the VIDs/PIDs to Labgrid for shared benefit.

Otherwise we can simply add more to the matching list over time, not sure it's interesting to bind labgrid updates with Rockchip SoC support?

I don't find it unreasonable to update Labgrid when you are interfacing with a new SoC.

I think it's unnecessary churn for the project and waiting time for people to be able to use labgrid with their yet-unsupported SoC. But that's a perfectly understandable policy, there's such policy for Yocto bbclasses for example, which is implemented in such a way it is very difficult to extend transparently.

The tool to use to interact with the USB loader on Rockchip needs to run on the executor, but this needs to be specified on the Driver configuration side, which is part of the labgrid-device-config yaml file, which is on PC-B while it's going to run on PC-A. My issue is... there are currently three known tools for USB loading on Rockchip devices, rkdeveloptool, rockusb and this rk-usb-loader. I assume we could have some executors with one binary, and some others with another one. Why would the target conf file need to know on which executor something is going to run? Shouldn't this be related to a Place or the Resource itself?

I haven't used rockusb so far, but both rkdeveloptool and rk-usb-loader have an incompatile commnad-line interface.

rockusb[1] implements a third one :)

rockusb --help
Usage: rockusb [OPTIONS] <COMMAND>

Commands:
  list
  download-boot
  read
  write
  write-file
  write-bmap
  chip-info
  flash-id
  flash-info
  reset-device
  nbd
  help           Print this message or the help of the given subcommand(s)

Options:
  -d, --device <DEVICE>  Device type specified as <bus>:<address>
  -h, --help             Print help

[1] https://github.com/collabora/rockchiprs/blob/main/rockusb/examples/rockusb.rs

though there is an issue by one of the contributors to add short parameters like rkdeveloptool[2]

[2] collabora/rockchiprs#17

For i.MX, there are multiple tools as well and Labgrid handles this by separating the resource (RKUSBLoader) from the Driver (which will be called RKDevelopToolUSBDriver in future). A future rk-usb-loader driver could then be called RKUSBLoaderDriver and each would implement the CLI of the underlying host tool.

I'm a bit worried by the rename because if we add support back for rk-usb-loader in the future, we'll have releases with "rk-usb-loader support but actually not really, the only way to use it is to point rk-usb-loader to rkdeveloptool with the tools: from the client config", other releases with "no rk-usb-loader support at all, rkdeveloptool is now through rkdeveloptoolusbdriver" and other releases with "actually now rk-usb-loader works, with the same driver as the releases that didn't work before". Which means we are breaking backward compatibility for the config file format (I don't know what's the stance of the project on that though).

[...]

What the client needs to know about though is which driver should handle the resource. This isn't something that we can just leave to the exporter, because the tools tend to have their idiosyncrasies, which leak through the abstraction. Once the driver is configured in your environment YAML though (e.g. with the name of the USB loader binary to use), the rest of Labgrid can interface with it generically via the Boostrap protocol, e.g. in the client it's just labgrid-client -c myconfig.yaml bootstrap barebox-rk3399-pro-n10.img

OK, so this means I need to have a huge myconfig.yaml, with all targets, that I maintain in a versioned repo that I could share with other devs I guess. It's a bit difficult for me to wrap my head around the potential sync issues between a place that is handled by the coordinator (and not versioned) and a client config file which could be defining a resource for a target via a place, those necessarily need to be in sync but by being different files, stored and used on different machines. Anyway, nothing related to this MR :) Thanks for the explanation.

Comment on lines +304 to +309
if match not in [("2207", "110a"), ("2207", "301a"),
("2207", "310c"), ("2207", "320b"),
("2207", "320a"), ("2207", "320c"),
("2207", "330a"), ("2207", "330c"),
("2207", "350a"), ("2207", "350b")
]:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): please comment on the SoC model those represents

This would make it easier to spot what's supported and what's not.

330c is RK3399
350b is RK3588

I also have:

330d for PX30 (but I can add myself, not an issue)

c.f. https://github.com/rockchip-linux/rkdeveloptool/blob/master/99-rk-rockusb.rules for some model ID

https://elixir.bootlin.com/u-boot/v2024.10/source/drivers/usb/gadget/Kconfig#L69 probably could help too :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do that before, because I didn't know what the original VID/PID was for, but I can annotate them now. Thanks!

@a3f
Copy link
Contributor Author

a3f commented Nov 13, 2024

RKUSBLoader is only matching devices with vendor id 0x2207 and model id 0x110a. I've no clue what that represents as the RK3399, PX30 and RK3588 I work with aren't matching that model id.

I am not sure either, maybe RV11?

RV1108 apparently :)

Thanks for finding this out.

For now I've just added mines in there but I was wondering if it would make more sense to have parameters for that Resource, à-la AndroidUSBFastboot for example.

I think it's useful anyhow to have the well-known VID/PIDs known to Labgrid. If it's possible to override them, it makes sense to have parameters for it, but the default should be that users add the VIDs/PIDs to Labgrid for shared benefit.

Otherwise we can simply add more to the matching list over time, not sure it's interesting to bind labgrid updates with Rockchip SoC support?

I don't find it unreasonable to update Labgrid when you are interfacing with a new SoC.

I think it's unnecessary churn for the project and waiting time for people to be able to use labgrid with their yet-unsupported SoC.
But that's a perfectly understandable policy, there's such policy for Yocto bbclasses for example, which is implemented in such a way it is very difficult to extend transparently.

I'll have to defer to the maintainers what they think should be the policy.

The tool to use to interact with the USB loader on Rockchip needs to run on the executor, but this needs to be specified on the Driver configuration side, which is part of the labgrid-device-config yaml file, which is on PC-B while it's going to run on PC-A. My issue is... there are currently three known tools for USB loading on Rockchip devices, rkdeveloptool, rockusb and this rk-usb-loader. I assume we could have some executors with one binary, and some others with another one. Why would the target conf file need to know on which executor something is going to run? Shouldn't this be related to a Place or the Resource itself?

I haven't used rockusb so far, but both rkdeveloptool and rk-usb-loader have an incompatile commnad-line interface.

rockusb[1] implements a third one :)
[1] https://github.com/collabora/rockchiprs/blob/main/rockusb/examples/rockusb.rs

Heh, even more reason to rename the driver to reflect what is supports.

For i.MX, there are multiple tools as well and Labgrid handles this by separating the resource (RKUSBLoader) from the Driver (which will be called RKDevelopToolUSBDriver in future). A future rk-usb-loader driver could then be called RKUSBLoaderDriver and each would implement the CLI of the underlying host tool.

I'm a bit worried by the rename because if we add support back for rk-usb-loader in the future, we'll have releases with "rk-usb-loader support but actually not really, the only way to use it is to point rk-usb-loader to rkdeveloptool with the tools: from the client config",

Yes, this is confusing and this PR is rectifying the situation. I hope the impact will be limited as the driver seems not yet widely used evidenced by the single VID/PID match and the wrong documentation. I will add a deprecation warning and fallback as suggested by @Emantor.

other releases with "no rk-usb-loader support at all, rkdeveloptool is now through rkdeveloptoolusbdriver" and other releases with "actually now rk-usb-loader works, with the same driver as the releases that didn't work before". Which means we are breaking backward compatibility for the config file format (I don't know what's the stance of the project on that though).

Even in a future, where we have drivers for all three host utilities, the name RKUSBDriver will not be used again. But yes, a config file with:

    targets:
        drivers:
          RKUSBDriver:
            usb_loader: usb_loader
    tools:
      rk-usb-loader: /bin/rkdeveloptool

will cease to work, but we got to make a tradeoff between complicating things for everyone or breaking it for existing users. The authors of the change, which are the only users I knew of, are fine with it. I hope such breaking changes will be rare in future.

[...]

What the client needs to know about though is which driver should handle the resource. This isn't something that we can just leave to the exporter, because the tools tend to have their idiosyncrasies, which leak through the abstraction. Once the driver is configured in your environment YAML though (e.g. with the name of the USB loader binary to use), the rest of Labgrid can interface with it generically via the Boostrap protocol, e.g. in the client it's just labgrid-client -c myconfig.yaml bootstrap barebox-rk3399-pro-n10.img

OK, so this means I need to have a huge myconfig.yaml, with all targets, that I maintain in a versioned repo that I could share with other devs I guess. It's a bit difficult for me to wrap my head around the potential sync issues between a place that is handled by the coordinator (and not versioned) and a client config file which could be defining a resource for a target via a place, those necessarily need to be in sync but by being different files, stored and used on different machines. Anyway, nothing related to this MR :) Thanks for the explanation.

For test suites, you'll want to check in the environment yaml into the repository alongside the tests anyway. You can customize values with !template $LG_SOME_ENV_VAR if you don't want to hardcode something.

For interactive labgrid-client usage, the client already instantiates drivers by default to match against resources. You still need a config though for the USB loader.

Thinking about it, maybe we should change this in the same go: Other bootstrap drivers don't do persistent flashing of the bootstrapped image, maybe we should change the Rockchip USB driver to load only into RAM by default. That way labgrid-client usage may not need any USB loader binary and it will be possible config-less. Thoughts?

@QSchulz
Copy link

QSchulz commented Nov 13, 2024

[...]

For i.MX, there are multiple tools as well and Labgrid handles this by separating the resource (RKUSBLoader) from the Driver (which will be called RKDevelopToolUSBDriver in future). A future rk-usb-loader driver could then be called RKUSBLoaderDriver and each would implement the CLI of the underlying host tool.

I'm a bit worried by the rename because if we add support back for rk-usb-loader in the future, we'll have releases with "rk-usb-loader support but actually not really, the only way to use it is to point rk-usb-loader to rkdeveloptool with the tools: from the client config",

Yes, this is confusing and this PR is rectifying the situation. I hope the impact will be limited as the driver seems not yet widely used evidenced by the single VID/PID match and the wrong documentation. I will add a deprecation warning and fallback as suggested by @Emantor.

other releases with "no rk-usb-loader support at all, rkdeveloptool is now through rkdeveloptoolusbdriver" and other releases with "actually now rk-usb-loader works, with the same driver as the releases that didn't work before". Which means we are breaking backward compatibility for the config file format (I don't know what's the stance of the project on that though).

Even in a future, where we have drivers for all three host utilities, the name RKUSBDriver will not be used again. But yes, a config file with:

RKUSBLoaderDriver was an awfully good name for a driver for rk-usb-loader though ;)

    targets:
        drivers:
          RKUSBDriver:
            usb_loader: usb_loader
    tools:
      rk-usb-loader: /bin/rkdeveloptool

I think we could simply add support for rkdeveloptool in another driver and keep RKUSBLoaderDriver (and maybe even fix its support?). While we won't support an rkdeveloptool in tools:rk-usb-loader anymore, it should be rather easy to warn users of that via doc update and parsing the config and warn/fail if rk-usb-loader containers rkdeveloptool and say "please use RKDevelopToolDriver instead" or something like that.

Also, I quickly went trhough the rk-usb-loader script and I think it is the equivalent of rkdeveloptool db <file> and that's it. So USB-loading a file.

Currently RKUSBLoaderDriver allows to flash an image after a special binary has been USB loaded through special commands (for rkdeveloptool, that is rkdeveloptool wl <block_offset> <file>. But I don't think we have an equivalent for rk-usb-loader. rockusb supports that though (via different parameters but still).

Also, I just remembered that rkdeveloptool wl (and other arguments) actually use the RKUSB protocol, of which not everything is supported in upstream U-Boot (or even in Rockchip blobs.....). I believe we also have the option of using DFU when USB loading upstream U-Boot, for which dfu-util should be used.

I'm therefore wondering if we shouldn't have clear separation of

  • USB loading a binary
  • interacting with the board which USB loaded the binary

We could very well use rk-usb-loader for the former and rockusb with the latter, or rkdeveloptool + df-util, etc...

will cease to work, but we got to make a tradeoff between complicating things for everyone or breaking it for existing users. The authors of the change, which are the only users I knew of, are fine with it. I hope such breaking changes will be rare in future.

kas is dealing with that with versioning the config format and warning/failing if you're using outdated config format with newer kas. Maybe something to investigate? In any case, if the breaking of backward compatibility is documented somewhere, that's fine to me :)

[...]

What the client needs to know about though is which driver should handle the resource. This isn't something that we can just leave to the exporter, because the tools tend to have their idiosyncrasies, which leak through the abstraction. Once the driver is configured in your environment YAML though (e.g. with the name of the USB loader binary to use), the rest of Labgrid can interface with it generically via the Boostrap protocol, e.g. in the client it's just labgrid-client -c myconfig.yaml bootstrap barebox-rk3399-pro-n10.img

OK, so this means I need to have a huge myconfig.yaml, with all targets, that I maintain in a versioned repo that I could share with other devs I guess. It's a bit difficult for me to wrap my head around the potential sync issues between a place that is handled by the coordinator (and not versioned) and a client config file which could be defining a resource for a target via a place, those necessarily need to be in sync but by being different files, stored and used on different machines. Anyway, nothing related to this MR :) Thanks for the explanation.

For test suites, you'll want to check in the environment yaml into the repository alongside the tests anyway. You can customize values with !template $LG_SOME_ENV_VAR if you don't want to hardcode something.

For interactive labgrid-client usage, the client already instantiates drivers by default to match against resources. You still need a config though for the USB loader.

Thanks, I'll need to finalize some kind of setup, make mistake and learn from them. A bit still too fresh in my head for now.

Thinking about it, maybe we should change this in the same go: Other bootstrap drivers don't do persistent flashing of the bootstrapped image, maybe we should change the Rockchip USB driver to load only into RAM by default. That way labgrid-client usage may not need any USB loader binary and it will be possible config-less. Thoughts?

AFAICT (haven't worked on it), on Rockchip we USB load TPL+SPL but then you have something very limited (proper + TF-A + OP-TEE + ... missing).

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway). Is this what IMXUSBLoader is doing? I vaguely remember that you could USB boot a full bootloader via uuu (but that was ~10 years ago on an i.MX6 so /me shrugs).

@a3f
Copy link
Contributor Author

a3f commented Nov 13, 2024

Even in a future, where we have drivers for all three host utilities, the name RKUSBDriver will not be used again. But yes, a config file with:

RKUSBLoaderDriver was an awfully good name for a driver for rk-usb-loader though ;)

And this name wasn't used before. RKUSBLoader, which is the name of the resource remains as-is. Only RKUSBDriver is being changed.

I think we could simply add support for rkdeveloptool in another driver

That's what I am doing basically

and keep RKUSBLoaderDriver (and maybe even fix its support?).

I don't think adding support anew for rk-usb-loader can be considered a fix. But eventually, we may have a driver for it.

Note that the current name is RKUSBDriver, which is IMO a too generic name, which we should just never use again.

While we won't support an rkdeveloptool in tools:rk-usb-loader anymore, it should be rather easy to warn users of that via doc update and parsing the config and warn/fail if rk-usb-loader containers rkdeveloptool and say "please use RKDevelopToolDriver instead" or something like that.

I will look into extending the deprecation warning, so there are messages for both using the old driver name and the old tools key.

Also, I quickly went through the rk-usb-loader script and I think it is the equivalent of rkdeveloptool db <file> and that's it. So USB-loading a file.
Yes.

Currently RKUSBLoaderDriver allows to flash an image after a special binary has been USB loaded through special commands (for rkdeveloptool, that is rkdeveloptool wl <block_offset> <file>. But I don't think we have an equivalent for rk-usb-loader. rockusb supports that though (via different parameters but still).

I know. That's what the driver in Labgrid currently implements.

Also, I just remembered that rkdeveloptool wl (and other arguments) actually use the RKUSB protocol, of which not everything is supported in upstream U-Boot (or even in Rockchip blobs.....). I believe we also have the option of using DFU when USB loading upstream U-Boot, for which dfu-util should be used.

The current driver implements the BootstrapProtocol, whose hallmark is implementing a load function that loads a single file.

Fastboot and DFU go beyond that and they don't implement BootstrapProtocol. labgrid-client has dedicated commands for each, so the user has access to all functionality.

I'm therefore wondering if we shouldn't have clear separation of

* USB loading a binary

* interacting with the board which USB loaded the binary

Yes, I am starting to think a rename alone is insufficient. The proper abstraction would be a fastboot/DFU-esque driver that makes available the flashing, erasing, system reset functionality of the USB loader and BootstrapProtocol::load would just do the equivalent of rkdeveloptool db.

We could very well use rk-usb-loader for the former and rockusb with the latter, or rkdeveloptool + df-util, etc...

barebox only emulates SoC-specific USB gadget protocols if it's needed for the prebootloader to chainload barebox proper. The idea is that the tools in barebox scripts/ take a barebox binary and upload it, so it runs from RAM completely and you reach a shell and then Fastboot/DFU/Network boot whatever is used for persistent flashing. This flow is reflected in other BootstrapProtocols and only rkdeveloptool support seems to differ. So, yes, splitting it up is sensible.

will cease to work, but we got to make a tradeoff between complicating things for everyone or breaking it for existing users. The authors of the change, which are the only users I knew of, are fine with it. I hope such breaking changes will be rare in future.

kas is dealing with that with versioning the config format and warning/failing if you're using outdated config format with newer kas. Maybe something to investigate? In any case, if the breaking of backward compatibility is documented somewhere, that's fine to me :)

It's documented in the changelog and there will be deprecation warnings along with a fallback for a transitory period.

As a counterpoint to kas, docker has made their docker-compose.yml version field obsolete. I don't feel strongly either way, but there are tradeoffs involved and this would need more careful consideration.

Thinking about it, maybe we should change this in the same go: Other bootstrap drivers don't do persistent flashing of the bootstrapped image, maybe we should change the Rockchip USB driver to load only into RAM by default. That way labgrid-client usage may not need any USB loader binary and it will be possible config-less. Thoughts?

AFAICT (haven't worked on it), on Rockchip we USB load TPL+SPL but then you have something very limited (proper + TF-A + OP-TEE + ... missing).

rk-usb-loader only supports what barebox supports upstream, which at this time is RK3568 and RK3588. On both of these, it takes only the barebox image and the system boots to shell. I don't know if this is easily doable for older SoCs or this is just a side-effect of the new boot flow on the RK35xx and the RKNS format.

(The RK3399 I mention in the PR is not supported in barebox. Getting Labgrid to handle the RK3399 properly is part of my Yak shave to eventually upstream out-of-tree barebox RK3399/PX30 support).

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

Personally, I'd always go with Fastboot or DFU, so I am not too motivated to implement a driver for the rockusb protocol in full..

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway).

That's how we use it. Then a script checks that "$bootsource" = "serial" and then barebox starts a composite gadget with fastboot/DFU and ACM in the background and then flashing is done via that.

Is this what IMXUSBLoader is doing? I vaguely remember that you could USB boot a full bootloader via uuu (but that was ~10 years ago on an i.MX6 so /me shrugs).

Yes, IMXUSBLoader boots to shell. uuu can talk both SDP/SDPS (BootROM protocol) and Fastboot with NXP vendor extensions (which of course doesn't use the OEM namespace). Personally, I stay clear of this and use the android-tools fastboot command for Fastboot stuff.

@QSchulz
Copy link

QSchulz commented Nov 13, 2024

Even in a future, where we have drivers for all three host utilities, the name RKUSBDriver will not be used again. But yes, a config file with:

RKUSBLoaderDriver was an awfully good name for a driver for rk-usb-loader though ;)

And this name wasn't used before. RKUSBLoader, which is the name of the resource remains as-is. Only RKUSBDriver is being changed.

Well, if only I had taken the time to double check what I was saying... So I guess that's a +1 for the rename then since we'd probably have a driver named RKUSBLoaderDriver for rk-usb-loader (if anyone wants to implement that :) )?

I think we could simply add support for rkdeveloptool in another driver

That's what I am doing basically

and keep RKUSBLoaderDriver (and maybe even fix its support?).

I don't think adding support anew for rk-usb-loader can be considered a fix. But eventually, we may have a driver for it.

Note that the current name is RKUSBDriver, which is IMO a too generic name, which we should just never use again.

Agreed.

While we won't support an rkdeveloptool in tools:rk-usb-loader anymore, it should be rather easy to warn users of that via doc update and parsing the config and warn/fail if rk-usb-loader containers rkdeveloptool and say "please use RKDevelopToolDriver instead" or something like that.

I will look into extending the deprecation warning, so there are messages for both using the old driver name and the old tools key.

Also, I quickly went through the rk-usb-loader script and I think it is the equivalent of rkdeveloptool db <file> and that's it. So USB-loading a file.
Yes.

Currently RKUSBLoaderDriver allows to flash an image after a special binary has been USB loaded through special commands (for rkdeveloptool, that is rkdeveloptool wl <block_offset> <file>. But I don't think we have an equivalent for rk-usb-loader. rockusb supports that though (via different parameters but still).

I know. That's what the driver in Labgrid currently implements.

Also, I just remembered that rkdeveloptool wl (and other arguments) actually use the RKUSB protocol, of which not everything is supported in upstream U-Boot (or even in Rockchip blobs.....). I believe we also have the option of using DFU when USB loading upstream U-Boot, for which dfu-util should be used.

The current driver implements the BootstrapProtocol, whose hallmark is implementing a load function that loads a single file.

Fastboot and DFU go beyond that and they don't implement BootstrapProtocol. labgrid-client has dedicated commands for each, so the user has access to all functionality.

Thanks for the info. My point being that it depends what one means with 'bootstrapping'. If I'm not mistaken, U-Boot images that are USB loaded only contain TPL+SPL and then is waiting for proper via rockusb/rkusb protocol, fastboot or dfu, no CLI access. I believe it should be possible for fastboot and dfu to upload a file to DRAM and execute it, not sure for rockusb/rkusb though. I seem to recall Collabora is doing that very thing https://gitlab.collabora.com/hardware-enablement/rockchip-3588/notes-for-rockchip-3588/-/blob/main/upstream_uboot.md#how-to-create-a-blob-containing-ddr-init-and-spl-for-rockusb

So the issue here is that depending on the bootloader (I believe, haven't checked myself), we'd reach or not CLI with a simple rkdeveloptool db (Barebox vs U-Boot). This is.... kinda bad? Not sure how to handle that though.

I'm therefore wondering if we shouldn't have clear separation of

* USB loading a binary

* interacting with the board which USB loaded the binary

Yes, I am starting to think a rename alone is insufficient. The proper abstraction would be a fastboot/DFU-esque driver that makes available the flashing, erasing, system reset functionality of the USB loader and BootstrapProtocol::load would just do the equivalent of rkdeveloptool db.

FWIW, I would need to use rkdeveloptool with rockusb support for my board(s) for flashing/erasing/reset since USB gadget isn't working in upstream U-Boot (at the very least on PX30 Ringneck, RK3399 Puma and RK3588 Tiger may be fine), only with the blob, who only supports rockusb commands.

We could very well use rk-usb-loader for the former and rockusb with the latter, or rkdeveloptool + df-util, etc...

barebox only emulates SoC-specific USB gadget protocols if it's needed for the prebootloader to chainload barebox proper. The idea is that the tools in barebox scripts/ take a barebox binary and upload it, so it runs from RAM completely and you reach a shell and then Fastboot/DFU/Network boot whatever is used for persistent flashing. This flow is reflected in other BootstrapProtocols and only rkdeveloptool support seems to differ. So, yes, splitting it up is sensible.

OK. I am not sure this is possible in U-Boot SPL though. And if it is, I doubt it is common on Rockchip boards. I haven't had the need for USB loading upstream U-Boot so far, so have no experience with that.

will cease to work, but we got to make a tradeoff between complicating things for everyone or breaking it for existing users. The authors of the change, which are the only users I knew of, are fine with it. I hope such breaking changes will be rare in future.

kas is dealing with that with versioning the config format and warning/failing if you're using outdated config format with newer kas. Maybe something to investigate? In any case, if the breaking of backward compatibility is documented somewhere, that's fine to me :)

It's documented in the changelog and there will be deprecation warnings along with a fallback for a transitory period.

As a counterpoint to kas, docker has made their docker-compose.yml version field obsolete. I don't feel strongly either way, but there are tradeoffs involved and this would need more careful consideration.

Thanks for the info!

Thinking about it, maybe we should change this in the same go: Other bootstrap drivers don't do persistent flashing of the bootstrapped image, maybe we should change the Rockchip USB driver to load only into RAM by default. That way labgrid-client usage may not need any USB loader binary and it will be possible config-less. Thoughts?

AFAICT (haven't worked on it), on Rockchip we USB load TPL+SPL but then you have something very limited (proper + TF-A + OP-TEE + ... missing).

rk-usb-loader only supports what barebox supports upstream, which at this time is RK3568 and RK3588. On both of these, it takes only the barebox image and the system boots to shell. I don't know if this is easily doable for older SoCs or this is just a side-effect of the new boot flow on the RK35xx and the RKNS format.

Is there some documentation available somewhere for this new boot flow for USB aside from the code in Barebox? I was considering making U-Boot generate a USB bootable (TPL+SPL) image but the source code stopped being released a few years ago and no support for RK35xx in it.

(The RK3399 I mention in the PR is not supported in barebox. Getting Labgrid to handle the RK3399 properly is part of my Yak shave to eventually upstream out-of-tree barebox RK3399/PX30 support).

I have never built nor used Barebox so all I was and am saying is with respect to U-Boot.

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

Personally, I'd always go with Fastboot or DFU, so I am not too motivated to implement a driver for the rockusb protocol in full..

We have some support in upstream U-Boot (though if it is actually properly tested... who knows). FWIW, U-Boot currently doesn't generate a USB bootable image so I doubt many people are generating them by hand using Rockchip host blobs :) (boot_merger IIRC). I also have at least one board which doesn't have upstream support in U-Boot for USB gadget, so I still need to use Rockchip's blob until that is handled (not sure it's going to happen though as it also doesn't work in upstream Linux...).

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway).

That's how we use it. Then a script checks that "$bootsource" = "serial" and then barebox starts a composite gadget with fastboot/DFU and ACM in the background and then flashing is done via that.

IIRC, in U-Boot we check if the boot source is USB and then start USB gadget with different functions (DFU/fastboot/rockusb)? But no CLI at that point, everything done in C code at build time.

@a3f
Copy link
Contributor Author

a3f commented Nov 13, 2024

So the issue here is that depending on the bootloader (I believe, haven't checked myself), we'd reach or not CLI with a simple rkdeveloptool db (Barebox vs U-Boot). This is.... kinda bad? Not sure how to handle that though.

Meh. I guess, we can just kick the can down the road by making usb_loader attribute optional. If it's set, it's used (rkdeveloptool wl), otherwise it's only rkdeveloptool db.

Yes, I am starting to think a rename alone is insufficient. The proper abstraction would be a fastboot/DFU-esque driver that makes available the flashing, erasing, system reset functionality of the USB loader and BootstrapProtocol::load would just do the equivalent of rkdeveloptool db.

FWIW, I would need to use rkdeveloptool with rockusb support for my board(s) for flashing/erasing/reset since USB gadget isn't working in upstream U-Boot (at the very least on PX30 Ringneck, RK3399 Puma and RK3588 Tiger may be fine), only with the blob, who only supports rockusb commands.

I see. Would you like to submit a follow-up PR bringing rockup support in Labgrid to parity with Fastboot? ;)

rk-usb-loader only supports what barebox supports upstream, which at this time is RK3568 and RK3588. On both of these, it takes only the barebox image and the system boots to shell. I don't know if this is easily doable for older SoCs or this is just a side-effect of the new boot flow on the RK35xx and the RKNS format.

Is there some documentation available somewhere for this new boot flow for USB aside from the code in Barebox?

I don't know of anything besides the code, sorry. I assume either BootROM or DDR blob has logic to continue USB boot into DRAM once RAM is setup.

I was considering making U-Boot generate a USB bootable (TPL+SPL) image but the source code stopped being released a few years ago and no support for RK35xx in it.

What does USB-bootable mean? The same barebox image placed on eMMC/SD/SPI can also be loaded via rk-usb-loader.

(The RK3399 I mention in the PR is not supported in barebox. Getting Labgrid to handle the RK3399 properly is part of my Yak shave to eventually upstream out-of-tree barebox RK3399/PX30 support).

I have never built nor used Barebox so all I was and am saying is with respect to U-Boot.

Never too late to start: https://barebox.org/demo ;)

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

Personally, I'd always go with Fastboot or DFU, so I am not too motivated to implement a driver for the rockusb protocol in full..

We have some support in upstream U-Boot (though if it is actually properly tested... who knows). FWIW, U-Boot currently doesn't generate a USB bootable image so I doubt many people are generating them by hand using Rockchip host blobs :) (boot_merger IIRC). I also have at least one board which doesn't have upstream support in U-Boot for USB gadget, so I still need to use Rockchip's blob until that is handled (not sure it's going to happen though as it also doesn't work in upstream Linux...).

I see.

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway).

For me that's the point of bootstrap. Get a full bootloader running, so I can decide what to do next.

That's how we use it. Then a script checks that "$bootsource" = "serial" and then barebox starts a composite gadget with fastboot/DFU and ACM in the background and then flashing is done via that.

IIRC, in U-Boot we check if the boot source is USB and then start USB gadget with different functions (DFU/fastboot/rockusb)? But no CLI at that point, everything done in C code at build time.

I am grateful, I can use a single binary for everything.

@QSchulz
Copy link

QSchulz commented Nov 13, 2024

So the issue here is that depending on the bootloader (I believe, haven't checked myself), we'd reach or not CLI with a simple rkdeveloptool db (Barebox vs U-Boot). This is.... kinda bad? Not sure how to handle that though.

Meh. I guess, we can just kick the can down the road by making usb_loader attribute optional. If it's set, it's used (rkdeveloptool wl), otherwise it's only rkdeveloptool db.

That would force us to use rkdeveloptool with the rockusb protocol, which for example doesn't implement erasing

Yes, I am starting to think a rename alone is insufficient. The proper abstraction would be a fastboot/DFU-esque driver that makes available the flashing, erasing, system reset functionality of the USB loader and BootstrapProtocol::load would just do the equivalent of rkdeveloptool db.

FWIW, I would need to use rkdeveloptool with rockusb support for my board(s) for flashing/erasing/reset since USB gadget isn't working in upstream U-Boot (at the very least on PX30 Ringneck, RK3399 Puma and RK3588 Tiger may be fine), only with the blob, who only supports rockusb commands.

I see. Would you like to submit a follow-up PR bringing rockup support in Labgrid to parity with Fastboot? ;)

What exactly are you suggesting here, I do not understand.

rk-usb-loader only supports what barebox supports upstream, which at this time is RK3568 and RK3588. On both of these, it takes only the barebox image and the system boots to shell. I don't know if this is easily doable for older SoCs or this is just a side-effect of the new boot flow on the RK35xx and the RKNS format.

Is there some documentation available somewhere for this new boot flow for USB aside from the code in Barebox?

I don't know of anything besides the code, sorry. I assume either BootROM or DDR blob has logic to continue USB boot into DRAM once RAM is setup.

I can explain how that works in U-Boot though :) The BootROM finds magic value which starts the expected header. It loads the DRAM init code in SRAM. The DRAM init stage (either through upstream U-Boot TPL with open-source DRAM init or via the blob provided by Rockchip) init the DRAM and then exits back to BootROM which continues reading from the medium according to info in the header and loads the next stage. In U-Boot, that's SPL. Then SPL loads from the same medium (or a fallback one) a fitImage which contains TF-A, OP-TEE and U-Boot proper. Loads TF-A, executes it. TF-A does its thing, changing the EL before executing into U-Boot proper. You'll notice that we don't do a back-to-bootrom step between SPL and TF-A.

I was considering making U-Boot generate a USB bootable (TPL+SPL) image but the source code stopped being released a few years ago and no support for RK35xx in it.

What does USB-bootable mean? The same barebox image placed on eMMC/SD/SPI can also be loaded via rk-usb-loader.

I need to look into Barebox source but I can already tell you that's not the case for U-Boot. We have one image for eMMC/SD and another for SPI. This is required for RK3399 (and older SoCs) because some SoCs have a broken BootROM implementation for the SPI controller which reads only the first half of every sector (what I have been told). I believe this is fine for SoCs with the headerv2 support in BootROM?

Also, we allow different location on storage medium depending on whether it's eMMC/SD or SPI (we use that on RK3399 Puma for example, which has a "small" SPI-NOR).

Considering Rockchip builds that USB loader (the one passed to rkdeveloptool db with boot_merger blob)... it seems odd that the same Barebox binary can be used on all kind of media. I shall investigate :) Thanks for the heads up!

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

Personally, I'd always go with Fastboot or DFU, so I am not too motivated to implement a driver for the rockusb protocol in full..

We have some support in upstream U-Boot (though if it is actually properly tested... who knows). FWIW, U-Boot currently doesn't generate a USB bootable image so I doubt many people are generating them by hand using Rockchip host blobs :) (boot_merger IIRC). I also have at least one board which doesn't have upstream support in U-Boot for USB gadget, so I still need to use Rockchip's blob until that is handled (not sure it's going to happen though as it also doesn't work in upstream Linux...).

I see.

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway).

For me that's the point of bootstrap. Get a full bootloader running, so I can decide what to do next.

Got it.

So, to come clean. I'm currently investigating labgrid for automating U-Boot testing as this is quickly growing out of proportions with everything I need to test. One of the things I need to test is a fallback mechanism for U-Boot itself.

Basically, U-Boot TPL+SPL written to medium A, U-Boot proper written to medium B, U-Boot environment loaded from medium B. I have three storage media (not counting USB booting a whole U-Boot though that would be neat for manufacturing stage), so that makes a decent matrix of tests to run :)

I'm not asking you to support this use-case, just asking you to try to think about not making it impossible (I am not saying you're suggesting something that is making it impossible!)

I've derailed a bit the conversation from the original topic I believe though. Is there something I can help with/needs to be discussed wrt this PR?

@a3f
Copy link
Contributor Author

a3f commented Nov 20, 2024

I talked with Quentin on IRC to clarify some things.

TL;DR: barebox images are more versatile than U-Boot images and have the necessary format and code to be able to be loaded directly into RAM without an intermediate USB loader (which is the only mode rk-usb-loader supports).

U-Boot images aren't there yet and that's why the rkdeveloptool support in Labgrid always flashes the bootloader to persistent media first. This clears up my confusion why the bootstrap code flashes the bootloader instead of just loading it into RAM.

More details below.


So the issue here is that depending on the bootloader (I believe, haven't checked myself), we'd reach or not CLI with a simple rkdeveloptool db (Barebox vs U-Boot). This is.... kinda bad? Not sure how to handle that though.

Meh. I guess, we can just kick the can down the road by making usb_loader attribute optional. If it's set, it's used (rkdeveloptool wl), otherwise it's only rkdeveloptool db.

That would force us to use rkdeveloptool with the rockusb protocol, which for example doesn't implement erasing

I think what's meant here is that without a USB loader we are forced to only talk to the BootROM.
This is ok though, because users that want to talk to a USB loader can keep specifying usb_loader.
Users that just want to load to RAM gain the ability to omit it.

Yes, I am starting to think a rename alone is insufficient. The proper abstraction would be a fastboot/DFU-esque driver that makes available the flashing, erasing, system reset functionality of the USB loader and BootstrapProtocol::load would just do the equivalent of rkdeveloptool db.

I still think so, but now less strongly. Unlike barebox, U-Boot doesn't support being loaded completely in RAM and SPL always searches for U-Boot proper on persistent storage. "RAM boot" is smoething they're interested in, but it's not available yet.

So the bootstrap driver should retain the ability to flash the bootloader.

FWIW, I would need to use rkdeveloptool with rockusb support for my board(s) for flashing/erasing/reset since USB gadget isn't working in upstream U-Boot (at the very least on PX30 Ringneck, RK3399 Puma and RK3588 Tiger may be fine), only with the blob, who only supports rockusb commands.

I see. Would you like to submit a follow-up PR bringing rockup support in Labgrid to parity with Fastboot? ;)

What exactly are you suggesting here, I do not understand.

Fastboot and DFU implement custom functions for erasing/flashing and a proper Labgrid driver could make all this available without limiting users to the BootstrapProtocol::load API.

rk-usb-loader only supports what barebox supports upstream, which at this time is RK3568 and RK3588. On both of these, it takes only the barebox image and the system boots to shell. I don't know if this is easily doable for older SoCs or this is just a side-effect of the new boot flow on the RK35xx and the RKNS format.

Is there some documentation available somewhere for this new boot flow for USB aside from the code in Barebox?

I don't know of anything besides the code, sorry. I assume either BootROM or DDR blob has logic to continue USB boot into DRAM once RAM is setup.

After the discussion, I believe that what rk-usb-loader is doing should be doable for the older SoCs as well. rkdeveloptool itself hasn't changed in 5 years and it didn't have to change to support the newer RK35xxx. So once we add support for RK3399/PX30 to barebox, rk-usb-loader may just work (when adding VID/PID). I'll test this in due time, but it's off-topic here.

I can explain how that works in U-Boot though :) The BootROM finds magic value which starts the expected header. It loads the DRAM init code in SRAM. The DRAM init stage (either through upstream U-Boot TPL with open-source DRAM init or via the blob provided by Rockchip) init the DRAM and then exits back to BootROM which continues reading from the medium according to info in the header and loads the next stage. In U-Boot, that's SPL. Then SPL loads from the same medium (or a fallback one) a fitImage which contains TF-A, OP-TEE and U-Boot proper. Loads TF-A, executes it. TF-A does its thing, changing the EL before executing into U-Boot proper. You'll notice that we don't do a back-to-bootrom step between SPL and TF-A.

I was considering making U-Boot generate a USB bootable (TPL+SPL) image but the source code stopped being released a few years ago and no support for RK35xx in it.

What does USB-bootable mean? The same barebox image placed on eMMC/SD/SPI can also be loaded via rk-usb-loader.

I need to look into Barebox source but I can already tell you that's not the case for U-Boot. We have one image for eMMC/SD and another for SPI. This is required for RK3399 (and older SoCs) because some SoCs have a broken BootROM implementation for the SPI controller which reads only the first half of every sector (what I have been told). I believe this is fine for SoCs with the headerv2 support in BootROM?

It's likely barebox lucked out by not supported these old SoCs and inheriting this quirk.

Also, we allow different location on storage medium depending on whether it's eMMC/SD or SPI (we use that on RK3399 Puma for example, which has a "small" SPI-NOR).

Not a concern for barebox. The image is one piece.

Considering Rockchip builds that USB loader (the one passed to rkdeveloptool db with boot_merger blob)... it seems odd that the same Barebox binary can be used on all kind of media. I shall investigate :) Thanks for the heads up!

:)

You have the option of doing rkusb (or rockusb (the protocol) whatever its actual name) via rkdeveloptool to do stuff (write somewhere for example) or using DFU (or probably also fastboot).

Personally, I'd always go with Fastboot or DFU, so I am not too motivated to implement a driver for the rockusb protocol in full..

We have some support in upstream U-Boot (though if it is actually properly tested... who knows). FWIW, U-Boot currently doesn't generate a USB bootable image so I doubt many people are generating them by hand using Rockchip host blobs :) (boot_merger IIRC). I also have at least one board which doesn't have upstream support in U-Boot for USB gadget, so I still need to use Rockchip's blob until that is handled (not sure it's going to happen though as it also doesn't work in upstream Linux...).

barebox binaries are fine being loaded to start of DRAM and then relocate away from where TF-A needs to be.
In U-Boot this is missing and if I followed linux-rockchip correctly, this needs to be done carefully, so U-Boot doesn't
overwrite parts of itself.

So, "USB loading" via rkdeveloptool db/rk-usb-loader doesn't actually bring your target to a CLI (maybe possible but not sure this is something one wants anyway).

For me that's the point of bootstrap. Get a full bootloader running, so I can decide what to do next.

Got it.

So, to come clean. I'm currently investigating labgrid for automating U-Boot testing as this is quickly growing out of proportions with everything I need to test. One of the things I need to test is a fallback mechanism for U-Boot itself.

Basically, U-Boot TPL+SPL written to medium A, U-Boot proper written to medium B, U-Boot environment loaded from medium B. I have three storage media (not counting USB booting a whole U-Boot though that would be neat for manufacturing stage), so that makes a decent matrix of tests to run :)

I'm not asking you to support this use-case, just asking you to try to think about not making it impossible (I am not saying you're suggesting something that is making it impossible!)

I've derailed a bit the conversation from the original topic I believe though. Is there something I can help with/needs to be discussed wrt this PR?

Thanks, I will incorporate feedback and the takeaways from our discussions. There will be no loss of functionality.

@a3f
Copy link
Contributor Author

a3f commented Nov 20, 2024

A last question remains though @otavio & @QSchulz: The Labgrid driver for the rkdeveloptool doesn't use rkdeveloptool rd to reset the device after flashing. Is there a problem with me adding that?

Other bootstrap drivers boot up the SoC directly after bootstrap finishes, so it would be nice for the Rockchip support to behave the same.

@QSchulz
Copy link

QSchulz commented Nov 20, 2024

Trying to recap here.

Issue

It is currently not possible to have U-Boot reach CLI when loaded through USB. This is an issue as that is what bootstrapping seems to mean in labgrid. We therefore need to flash some persistence storage medium.

Booting into a U-Boot shell (Rockchip official way)

Working with Rockchip-based device traditionally involves the following:

  • <insert step for forcing USB mode in BootROM; see further down>
  • EDIT: <insert step for re-enabling storage medium; see further down> after device appears as USB device on host
  • EDIT: <insert step for "unforcing" USB mode; see further down>
  • rkdeveloptool db <rockchip blob> (or rockusb download-boot <rockchip blob>) which loads the Rockchip blob over USB and executes it. No CLI is reached. The Rockchip blob now expects rockusb protocol commands. Flashing an SPI-NOR, an SPI-NAND or an eMMC require different blobs.
  • EDIT: <insert step for re-enabling storage medium; see further down>
  • <insert SPI erasing step; see further down>
  • Flashing the storage medium is done with rkdeveloptool wl <offset> <binary> (or rockusb write-file <offset> <binary> or rockusb write-bmap <offset> <bmaped binary>).
  • EDIT: <insert step for "unforcing" USB mode; see further down>
  • Reset the device to boot from the newly flashed storage medium, either via a power cycle or with rkdeveloptool rd (or rockusb reset-device).
  • Profit

N.B. on EDIT: moved re-enabling storage medium and unforcing USB mode much earlier in the process as it wasn't required to have it intertwined with rkdeveloptool commands.

Forcing USB mode

On all (to my knowledge) but RK3588 and RK3576, all possible storage media are checked by the BootROM, in a specific order. USB mode for the BootROM is always the last step, if no storage medium has a valid header.

This means if your device has a valid header on a storage medium but you want to not boot from it, you need to disable it on the HW level. This was typically done in older designs by cutting either the clock or one of the data lanes going to the eMMC chip and/or SPI flashes.

This is required to boot into USB mode for the BootROM and is NOT doable via rkdeveloptool. It is also device-specific. For example, on our Haikou devkits we have a BIOS_DISABLE slider which cuts one of the lanes to the non-removable storage media. This BIOS_DISABLE functionality can also be controlled remotely by way of a GPIO on a CP2102. Other devices simply expose two pads/pins one needs to short or have removable eMMC/SPI flashes.

Note that one also needs to remove any existing SD card as since it's a removable storage medium, it's usually not handled by those "disable internal storage media" HW logic. This could be handled via SDMux drivers in labgrid. I could see this as a non-issue for labgrid, forcing you to put the SDMux device into host mode before bootstrapping the device.

For RK3588 and RK3576, the media boot order can be selected by inputting a specific voltage on a specific pin of the SoC's ADC. Wanting to boot from a specific medium at some point, means changing this voltage remotely, so an additional step too is required (unless always ever booting from one method which could be possible only for storage media which are entirely remotely flashable, without any interacting with the device).

Re-enabling storage medium

Only applicable for SoCs whose boot order is not selectable via ADC (so far, RK3588 and RK3576 excluded therefore).

Because on older SoCs to enter the USB mode of the BootROM one needs to cut data lanes on the HW level, those storage media whose data lanes are cut do NOT work until those data lanes are "uncut". This involves the opposite step as described in the previous step.

Unforcing USB mode

Mostly only applicable for SoCs whose boot order is selectable via ADC (so far, RK3588 and RK3576).

Since it is not required to switch back from the USB-only mode in order to flash the storage medium we want to boot from next, the previous step may not be done. If that is the case, then this step needs to update the voltage to the ADC so that the appropriate boot medium is selected after we reset/power cycle the board.

SPI erasing

Can only say for SPI-NOR flashes on RK3399. rkdeveloptool ef does NOT work. This was reported to Rockchip and they said "SoC too old, won't fix" in essence.

Therefore, the following is necessary:

  • Reach any U-Boot CLI (for now, since reaching CLI when loading over USB is not possible with U-Boot, it means following the above steps for flashing the eMMC or booting from SD card, flashed via SDMux device for example)
  • Make sure the mechanism cutting the data lanes to the SPI-NOR flash is off
  • sf probe; sf erase 0 0x400000 in U-Boot CLI
  • Restart from point 1 in Booting into a U-Boot shell (Rockchip official way)

Additional notes

U-Boot has many different binaries for different use cases, to be flashed at different offsets:

  • TPL+SPL at offset X
  • U-Boot proper at offset Y
  • TPL+SPL+proper at offset X

The latter are the u-boot-rockchip*.bin binaries. They are convenience binaries so only one binary needs to be flashed at one offset. However, there may be other things on the storage medium between offset X and offset Y (e.g. U-Boot environment). Also, Y may be much further in the storage medium than X + sizeof(TPL+SPL) meaning we flash zeroes for no reason whatsover.

Offset X is not necessarily the same across devices (the BootROM tries multiple offsets on the same medium), nor it is across flashes on the same device (RK3399 Puma uses offset 0 on SPI-NOR and offset 32K on MMC for example).

Offset Y is not necessarily the same across devices as it's configurable via defconfig (RK3399 Puma and PX30 Ringneck for example differ from the Rockchip default, which is also not stable across all SoCs).

TPL+SPL is not necessarily identical for all storage media (that is the case for SPI-NOR vs MMC for old SoCs such as RK3399 due to an oddity in the BootROM).

N.B.: Here, U-Boot proper is a misnomer as it's actually u-boot.itb which is a fitImage containing U-Boot proper (u-boot.bin) + TF-A (bl31.elf) + OP-TEE (tee.elf)

So, in short, it's a mess.

Questions/TODO

  • How to control internal storage media HW disablement in bootstrap driver? Probably need a new resource for that?
  • For the RKDevelopToolDriver, we need a USB-bootable target binary let's call it usb-boot.bin. This would be the Rockchip blob created with boot_merger tool as of today. We cannot reach U-Boot CLI with it. There may be more than one possible blob to use here (e.g. for SPI-NOR/NAND/eMMC/UFS), so at very least the path to this usb-boot.bin is required, maybe also which storage medium to write to.
  • Then we need a list of images to flash to the storage medium, and their offsets. Or we force the user to have only one big binary (which isn't generated by Rockchip's vendor U-Boot, only upstream, as far as I know). I think it makes sense to allow a list, even if we only support one binary for now, to avoid breaking backward compatibility of the config format if we ever add support for flashing more than one binary for bootstrapping.
  • Then comes the question how to handle the SPI-NOR very broken flashing process right now. This includes controlling the storage media HW disablement multiple times, including also at least two power cycles/device reset.
  • How are we supposed to handle bootstrapping when we have multiple internal storage media? e.g. SPI-NOR and eMMC. To bootstrap without the ability to load the bootloader entirely from USB (e.g. current state for U-Boot), we would need to bootstrap the storage medium among all storage media with a valid header which is checked the earliest by the BootROM or erase that storage medium and go with one which is checked later.

@QSchulz
Copy link

QSchulz commented Nov 20, 2024

A last question remains though @otavio & @QSchulz: The Labgrid driver for the rkdeveloptool doesn't use rkdeveloptool rd to reset the device after flashing. Is there a problem with me adding that?

Other bootstrap drivers boot up the SoC directly after bootstrap finishes, so it would be nice for the Rockchip support to behave the same.

So... this could be fine but I am not sure it is fool-proof.

On most Rockchip SoCs (not on RK3588 and seemingly not on RK3576), IOs are divided in blocks. Those blocks are powered externally and may have a different voltage applied at runtime than boot time. A typical example would be for SD cards or eMMC where the voltage could be either 1.8V or 3/3.3V depending on the negotiated speed. This is called IO domains in Rockchip world. The issue is that the IO domain needs to be configured accordingly to the power it receives otherwise a) it damages the SoC or b) it simply doesn't work. The second issue is that this power is not controlled by the SoC, but by the PMIC, which I assume is NOT reset when doing a rkdeveloptool rd.

Note that this isn't a theoretical issue, I can tell you for example that PX30 Ringneck cannot currently boot from SD card after an SoC watchdog reset (which doesn't trigger the PMIC) because the power to the IO domain for the SD controller is not matching the one expected by the IO domain as configured by default. I expect that flashing from U-Boot using one of the SD/eMMC speeds which the standard says operates at 1.8V and then using rkdeveloptool rd or reset in U-Boot would probably not work. But YMMV.

@a3f
Copy link
Contributor Author

a3f commented Nov 20, 2024

Thanks for the summary.

How to control internal storage media HW disablement in bootstrap driver? Probably need a new resource for that?

I do it on my RK3399 via GPIOs (DigitalOutputProtocol):

labgrid-client sd-mux host
labgrid-client io high RESET
labgrid-client io high RECOV
labgrid-client io low RESET
labgrid-client io low RECOV
labgrid-client -c ~/rkdeveloptool-rk3399pro.yaml bootstrap /tftpboot/a3f-barebox-rockpi-n10
ssh lxatac -- 'rkdeveloptool rd'
labgrid-client sd-mux dut

For the RKDevelopToolDriver, we need a USB-bootable binary let's call it usb-boot.bin. This would be the Rockchip blob created with boot_merger tool as of today. We cannot reach U-Boot CLI with it. There may be more than one possible blob to use here (e.g. for SPI-NOR/NAND/eMMC/UFS), so at very least the path to this usb-boot.bin is required, maybe also which storage medium to write to.

This is already supported via the RKUSBDriver-specific usb_loader attribute.

Then we need a list of images to flash to the storage medium, and their offsets. Or we force the user to have only one big binary (which isn't generated by Rockchip's vendor U-Boot, only upstream, as far as I know). I think it makes sense to allow a list, even if we only support one binary for now, to avoid breaking backward compatibility of the config format if we ever add support for flashing more than one binary for bootstrapping.

I am not sure we want to add this complexity into a Bootstrap driver. I think bootstrap should just start U-Boot and then a separate labgrid-client rockusb (modelled after existing Fastboot/DFU support) should communicate with it to actually flash the different partitions.

Then comes the question how to handle the SPI-NOR very broken flashing process right now. This includes controlling the storage media HW disablement multiple times, including also at least two power cycles/device reset.

This can always go into the strategy and doesn't need to be handled by Labgrid itself.

How are we supposed to handle bootstrapping when we have multiple internal storage media? e.g. SPI-NOR and eMMC. To bootstrap without the ability to load the bootloader entirely from USB (e.g. current state for U-Boot), we would need to bootstrap the storage medium among all storage media with a valid header which is checked the earliest by the BootROM or erase that storage medium and go with one which is checked later.

I also think that's a strategy's business. Different boards have different requirements.

@QSchulz
Copy link

QSchulz commented Nov 20, 2024

Thanks for the summary.

How to control internal storage media HW disablement in bootstrap driver? Probably need a new resource for that?

I do it on my RK3399 via GPIOs (DigitalOutputProtocol):

labgrid-client sd-mux host
labgrid-client io high RESET
labgrid-client io high RECOV
labgrid-client io low RESET
labgrid-client io low RECOV
labgrid-client -c ~/rkdeveloptool-rk3399pro.yaml bootstrap /tftpboot/a3f-barebox-rockpi-n10
ssh lxatac -- 'rkdeveloptool rd'
labgrid-client sd-mux dut

For the RKDevelopToolDriver, we need a USB-bootable binary let's call it usb-boot.bin. This would be the Rockchip blob created with boot_merger tool as of today. We cannot reach U-Boot CLI with it. There may be more than one possible blob to use here (e.g. for SPI-NOR/NAND/eMMC/UFS), so at very least the path to this usb-boot.bin is required, maybe also which storage medium to write to.

This is already supported via the RKUSBDriver-specific usb_loader attribute.

That's the host tool, I was talking about the target binary. So here I was talking about RKUSBDriver's image property. I updated my previous comment to reflect that.

Then we need a list of images to flash to the storage medium, and their offsets. Or we force the user to have only one big binary (which isn't generated by Rockchip's vendor U-Boot, only upstream, as far as I know). I think it makes sense to allow a list, even if we only support one binary for now, to avoid breaking backward compatibility of the config format if we ever add support for flashing more than one binary for bootstrapping.

I am not sure we want to add this complexity into a Bootstrap driver. I think bootstrap should just start U-Boot and then a separate labgrid-client rockusb (modelled after existing Fastboot/DFU support) should communicate with it to actually flash the different partitions.

Yeah but the issue is "just start U-Boot". We cannot reach the CLI right now with only rkdeveloptool db. So we need to flash it somewhere. We can use a hardcoded offset like we currently do and that would work for eMMC. Then we need to reboot into it.

Forcing USB mode could be done manually before calling labgrid-client bootstrap, but flashing necessarily requires to disable this forced USB mode (for anything but RK3588/RK3576), and this needs to be done between rkdeveloptool db and rkdeveloptool wl. EDIT: this is incorrect. One can put the device in USB mode for the BootROM and then disable the forced USB mode before executing rkdeveloptool db. Which is what the instructions provided by @a3f in their previous post do, if only I had read them.

Or are you suggesting you're simply planning on doing rkdeveloptool db <RKUSBDriver:image> and nothing else? And then the user would always need to do labgrid-client rockusb/dfu-util/fastboot to flash the device?

Essentially, "bootstrap" would simply be "load over USB some bootloader, up to you to know what's next after that"?

If that is the case, wouldn't this driver just be a glorified "run this command remotely with this argument" on a USB device checked to be of the vendorID+productID we define in there?

Considering that essentially nothing but the file path (which is abstracted) and the vendorId+productId are identical, does it really make sense to only have ONE driver for rkdeveloptool, rk-usb-loader and rockusb? Maybe we should rather have three independent drivers which all inherit from a command one which defines the path to the image and vendorId+productId?

[...]

I guess maybe I'm misinterpreting bootstrap as rescue?

E.g. it would be perfectly possible to have a valid header with a completely borked TPL on SPI-NOR and have bootstrap always try to flash eMMC (because that's what's put in the config file). So two things: we expect the user to be able to recover from this on their own by e.g. having another config file with the binaries for flashing/erasing on SPI-NOR, or a specific strategy for that. Or we expect the user to always flash on the storage medium which is looked first by the BootROM and rely on the boot order basically being "USB-only" and "normal mode" to be able to hardcode which one is the medium that is always looked first.

I think it now boils down to the expectations of what this bootstrap should actually do and if it needs to be consistent in behavior across bootloaders, SoCs and boards?

@QSchulz
Copy link

QSchulz commented Nov 21, 2024

So, I'm actually wondering if we shouldn't just remove the ability to flash a bootloader with the bootstrap command and let another command (e.g. rockusb) do that?

If what we want from the bootstrap command is to load some file into the target's DRAM over USB and that's it, then I guess we should move the rkdeveloptool wl part of the logic out into another driver, which wouldn't be required for Barebox (and U-Boot once we have binaries that can reach CLI whenever they are uploaded over USB).

Do you have something particular in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants