Rishabh Sharma 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 25:
(8 comments)
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?
Done
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 ver […]
Explicitly checking for version 5 and 6 makes much sense, it is more user-friendly.
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/createxbl.py... PS17, Line 687: if header_version == 5 or header_version == 6:
same
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
It is always good to have constants which are pointing to numbers, even SHA256_SIGNATURE_SIZE is also declared same
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?
Currently, 7,8,9 and later are not supported so this will be confusing to add check like this
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
same reply
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 […]
Equality checking here is much more sensible then comparison.
https://review.coreboot.org/c/coreboot/+/35506/17/util/qualcomm/mbn_tools.py... PS17, Line 2134: if header_version == MBN_HEADER_VERSION_6:
All the review comments will be addressed by Rishabh Sharma. He is looking in to it.
same reply as above