Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35506 )
Change subject: trogdor: support mbn_version 6 with python build scripts ......................................................................
Patch Set 17:
(9 comments)
Thanks I think this looks basically good now... just think matching the new header_version variable as a minimum (rather than an exact match) will help make it easier to expand this script in the future.
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/createxbl.py... PS17, Line 269: secure_type = image_header_secflag, header_version = header_version ) nit: on a new line?
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/createxbl.py... PS17, Line 673: if header_version == 5 or header_version == 6: nit: probably makes more sense to check < 4 (since presumably this will stay this way for future versions, unless it explicitly changes again?)
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/createxbl.py... PS17, Line 687: if header_version == 5 or header_version == 6: same
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 74: MBN_HEADER_VERSION_6 = 6 # Mbn header_version 6 nit: not really sure these constants are that useful as opposed to just using the numbers
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 578: if self.flash_parti_ver == MBN_HEADER_VERSION_6: nit: maybe > 5?
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 593: if self.flash_parti_ver == MBN_HEADER_VERSION_6: same
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 1009: if header_version == MBN_HEADER_VERSION_5: This should probably be
if header_version > 4: boot_header.image_src = 0 boot_header.image_dest_ptr = 0
if header_version > 5: boot_header.metadata_size_qti = 0 boot_header.metadata_size = 0
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 1050: global MI_PROG_BOOT_DIGEST_SIZE Can we please change this from a global to a normal local variable (and no ALL_CAPS name)? It's only used inside this function.
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 2134: if header_version == MBN_HEADER_VERSION_6:
5?