Rishabh Sharma 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. […]
Yes correct, if xbl_sec_elf input is not provided then version will be default 3, that why I make this --mbn_version as optional argument
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-option […]
I cant make --mbn_version non optional as it will break mbn_tools for other targets. So according to this logic header_version is optional and there can be following case which will preserve the backward compatibility- 1.) if no input xbl_sec and no --mbn_version then header_version will be None and mbn_tools will add default version 3 to the image 2.) if xbl_sec is given as input and no --mbn_version then header_version will be 5 3.) if --mbn_version is given, this will take precedence over all parameters
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 […]
Cant force all boards to pass mbn_version because it can break mbn_tools invocation for other chipsets
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 […]
Done
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_v […]
Removing this and and make header_version initialized for all code path may be risky it will break the older functionality and backward compatibility
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?
Done
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 w […]
It will be confusion to a newer person who is not aware of the mbn header structure so this condition clarifies the content of the particular header version
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
Done