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 2:
(8 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@125 PS2, Line 125: help="Add mbn header in elf image. '5' or '6'") I think for boards where elf_inp_xbl_sec is False (i.e. IPQ boards), the MBN version is actually 3?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@214 PS2, Line 214: None As per above, this should probably default to 3? Or maybe we should just make mbn_version non-optional and change all the other Makefiles to pass what they need.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@217 PS2, Line 217: is_ext_mbn_v5 I think all this means is that the MBN version is 5, so we should combine it with your new variable rather than adding a new one. So I'd just change this to
if options.elf_inp_xbl_sec and not options.mbn_version: header_version = 5
(or, as mentioned above, force all boards to pass in mbn_version rather than heuristicking it out like this).
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 You need this a lot so maybe better to make a header_size(header_version) helper function to be able to retrieve it where you need it?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@77 PS2, Line 77: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number I think this is the "default" mbn version? If so, we should remove this constant, make sure header_version is initialized for all code paths, and use that instead.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@556 PS2, Line 556: , header_version = None Does this need to be passed separately if we already have it in flash_parti_ver?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@1017 PS2, Line 1017: boot_header.metadata_size = 0 # oem_metadata size Do these last two really need to be guarded? Why not set them unconditionally, and writePackedData will just ignore them if MBN version is too low?
Then you can write the rest as
boot_header.flash_parti_ver = header_versio if header_version >= 5: boot_header.image_src = 0 boot_header.image_dest_ptr = 0
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@2132 PS2, Line 2132: is_sha256_algo, is_sha384_algo Please just have one 'algo' parameter that can be SHA1, SHA256 or SHA384