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 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 217: is_ext_mbn_v5 = True
This variable is used at many places. I can't see any value add by deleting this variable.
The point is that header_version == 5 means exactly the same as is_ext_mbn_v5 == True now. It is redundant. You're introducing a new variable that can serve all purposes of what this variable did (and more), so please replace existing instances of this variable with that. We don't need the same information X times encoded in X different ways in the code, that just makes things confusing.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1007: if is_ext_mbn_v5 == True:
If I modify this, mbn_tools will not remain backward compatible where some board is using is_ext_mbn […]
I don't follow. is_ext_mbn_v5 is just an implementation detail of this script, you can change it without affecting externally visible behavior. Those boards just pass the --xbl_sec_filepath argument to this script, which used to only set is_ext_mbn_v5 to True, but with your change it also sets header_version to 5. So you can use header_version == 5 as a 100% equivalent check to is_ext_mbn_v5 == True (and therefore get rid of the is_ext_mbn_v5 vairable).
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1009: boot_header.flash_parti_ver = VERSION_5 # version
We have not enforced createxbl to provide header_version.
...but you did? That's why I asked you to initialize header_version to 3 by default (and overwrite it to 5 or 6 where appropriate). So that you will always have a defined header_version you can rely on here and don't need that other variable anymore. header_version should not be optional, it should be required now.