Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 4: Code-Review-1
(10 comments)
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@214 PS2, Line 214: None
I cant make --mbn_version non optional as it will break mbn_tools for other targets. […]
Well, you can just change the Makefiles of the other targets to pass --mbn_version as well. We don't have out-of-tree boards in coreboot, all users of this tool are in this repository and can be changed. Or are you also using this same script elsewhere and want the backwards-compatibility there?
I'm not opposed to leaving a heuristic for backwards-compatibility if you want to, though. Although we should maybe still update the Makefiles anyway because it makes them easier to read.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@214 PS4, Line 214: None Didn't we agree this should be 3 if it is not set to anything else?
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@217 PS4, Line 217: is_ext_mbn_v5 = True Again, you should remove this variable because header_version == 5 already means the same thing.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@73 PS2, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header
I will create a function but I will not remove these contants because I dont know weather some older […]
Again, not really sure what chipsets you're talking about. The only coreboot code using any of this are the three Makefiles of SDM845, QCS405 and IPQ40xx. All of them call createxbl.py. Nothing else uses mbn_tools.py.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@77 PS2, Line 77: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number
Removing this and and make header_version initialized for all code path may be risky it will break t […]
Why? If you make sure that header_version is always initialized to some correct default in createxbl.py, you can assume that it is set here. Having this constant is redundant with the header_version option and incorrect whenever that option is not set to 3. It makes the code confusing.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@73 PS4, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header I think both of these constants are unused not and can be removed. You're using the header_size() function instead.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@76 PS4, Line 76: VERSION_6 = 6 # Mbn header_version 6 These constants seem kinda pointless, and you're mostly not using them anyway.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1010 PS4, Line 1010: # Please align comment correctly
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1018 PS4, Line 1018: =0 spacing
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1058 PS4, Line 1058: if (sha_algo is 'SHA384'): Please use == for string comparison