-
Notifications
You must be signed in to change notification settings - Fork 41
Rockchip SoC support #56
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
base: main
Are you sure you want to change the base?
Conversation
Add support for uploading blods to the maskrom memory using Rockchip maskrom protocol. Signed-off-by: Arnaud Patard <[email protected]>
``offset`` is too generic, so rename it to ``class_offset``. Signed-off-by: Arnaud Patard <[email protected]>
That is a very interesting contribution :) thanks a lot! I'll have to take some time to review it thoroughly, but at first glance the overall code organization looks good to me. I'll see if I can get it to work on RK3308 since I have a rockpis lying around. |
fyi, this u-boot series may be interesting: https://patchwork.ozlabs.org/project/uboot/list/?series=445397 as, maybe, we can avoid having to download u-boot proper with DFU. I've yet to test it and check it's working as I understand. |
…in files Add support for reading and sending rockchip binary files, made with boot_merger. Signed-off-by: Arnaud Patard <[email protected]>
fwiw, tested this series, and managed to boot u-boot proper without DFU on rk3399 and rk3588. On rk3588, I've had to disable rc4 encryption so I've updated the PR accordingly |
Hi, I'm starting to review this and there's one thing I've noticed so far: your series is missing documentation on the images which need to be passed to snagrecover. Every SoC family needs a section in docs/fw_binaries.md which describes which firmware binaries are expected by snagrecover and what these binaries contain. The README will also have to be updated to specify that Rockchip SoCs are supported. I'll come back with more details once I've had time to test and review this in depth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good! Apart for the comments in this review, there's also the point about docs/fw_binaries.md that I mentionned before.
I've confirmed that the first stage part works with a RockPiS board but unfortunately, there isn't SPL DFU support for the RK3308 SoC in U-Boot, so I couldn't test the second stage.
Thanks!
logger.error("Missing u-boot.itb DFU partition") | ||
dfu_cmd = dfu.DFU(dev, stm32=False) | ||
dfu_cmd.get_status() | ||
with open(recovery_config["firmware"]["u-boot-fit"]["path"], "rb") as fd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is firmware blob handling so it should really be done in firmware/rk_fw.py. You can take a look at am62x firmware handling which is quite similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to move it to rk_fw.py
. I hope it's better now.
For SPL DFU, I've tested it on rk3399 with patches I've sent on the u-boot mailing list. The same day someone from Radxa sent patches for a bunch of rockchip SoCs to enable it but I've not tested the serie. You may want to test that. Nevertheless, these days, I've been using the ramboot-v1 patches mentioned here and they're working at least on rk3399 and rk3588*. It should work on rk3308 too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Arnaud, thanks very much for your work!
I have tested this on an RK3399 using DFU and successfully booted.
I have not tried the UBoot changes that allow using it without DFU.
I do think more documentation would be helpful for users to be able to use this feature.
logger.info(f"Sleeping {delay}ms") | ||
time.sleep(delay / 1000) | ||
else: | ||
cli_error("Missing xpl or code471/code472 binary configuration") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Missing xPL or code471 ('*_ddr_*.bin')/code472 (*_usbplug_*.bin) binary configuration" would be even more helpful, as this would indicate what binaries "code471" and "code472" are supposed to be, which is not necessarily obvious if you're trying to use the tools without being super familiar with Rockchip internals.
path: rk3399_uboot.bin | ||
u-boot-fit: | ||
# proper u-boot | ||
path: u-boot.itb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested this on RK3399, trying to load u-boot.itb
directly failed for me.
Loading simple-bin.fit.itb
, however, worked perfectly.
This required quite a few patches to U-Boot (including some specific to my board, which has rather poor support upstream).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird. I'll test later if the simple-bin.fit.itb
is working for me too. In this case, I'll update things to use it.
Add support for booting a rockchip bin file, made with boot_merger. It can be: - a file containing the rockchip miniloader, but in this case, it's not useful, as rockusb is not implemented - a file containing uboot tpl and spl. In this case, if u-boot is configured for SPL DFU, the snagrecover configuration file may specify an u-boot FIT image to load into the first DFU slot. This will allow booting u-boot proper and then to fastboot/ums/dfu/... to flash. Signed-off-by: Arnaud Patard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements, there are still a few changes needed, mostly in the doc section. The rest looks good to me.
@@ -13,6 +13,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like these changes should be in the first patch of the series: "Add rockchip maskrom support"
@@ -13,6 +13,7 @@ | |||
|
|||
logger = logging.getLogger("snagrecover") | |||
|
|||
USB_REQUEST_TYPE_VENDOR = (0x02 << 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use usb.TYPE_VENDOR instead
@@ -12,28 +12,29 @@ | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for protocols/rockchip.py, these changes should be in the commit that creates the file, not this one
@@ -289,3 +289,100 @@ extract the FSBL and PMUFW from the complete boot image. | |||
configuration: | |||
* path | |||
|
|||
## For Rockchip devices | |||
|
|||
While it is possible to use the DDR init files and SPL files from Rockchip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really possible to use snagrecover with the prebuilt SPL images from Rockchip? I don't think that's true in the general case, since they don't have DFU support.
it is easier to use the files generated by u-boot as support for the | ||
Rokchip USB protocol is not implemented. | ||
Nevertheless, this means that to boot u-boot proper, u-boot has to be | ||
patched to get support for: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be clear here: using the prebuilt Rockchip SPL images doesn't work with Snagrecover. People have to patch U-Boot and build the images themselves.
The ``tpl/u-boot-tpl-dtb.bin``, ``spl/u-boot-spl-dtb.bin``, ``u-boot.itb`` | ||
files are generated during u-boot's build. Please note has the ``LOADER_OPTION`` | ||
is not handled by snagboot. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also include a specification of snagrecover firmware names and parameters in the same format as for the other SoC families in this documentation. I think it's especially important to describe the "delay" parameter.
when booting. It's due to the ``u-boot-rockchip-usb472.bin`` binary overwriting | ||
the ATAGS provided by the TPL. A possible fix would be to patch u-boot with | ||
https://github.com/u-boot/u-boot/commit/a6e85a35b50ade7df5f32092c1cc05ade303a22a. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
- ``mkimage-in-simple-bin.mkimage-u-boot-tpl`` or ``mkimage-in-simple-bin.mkimage-rockchip-tpl`` | ||
- ``mkimage-in-simple-bin.mkimage-u-boot-spl`` | ||
- ``u-boot.itb`` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Add support for rockchip maskrom mode. It can send files generated with boot_merger and for SoC with older
IDB format, it can generated the blob at run time. For SoCs with IDB v2, some extra work is needed atm.
note: rockusb protocol is not supported, as booting with u-boot will allow using things like dfu / ums / fastboot