Attention is currently required from: David Wu, Krishna P Bhat D, Reka Norman, Stefan Reinauer, Subrata Banik, Tyler Wang.
Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81928?usp=email )
Change subject: util/ifdtool: Add support for checking GPR0 status ......................................................................
Patch Set 11:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81928/comment/42388a6b_96ff6f3b : PS11, Line 34: Start address = 0x00004000 : End address = 0x00322fff I think this should print the actual range, not the enabled range, so these should be 0 (I also left a comment about this below).
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/81928/comment/6bc452b5_db947c0c : PS11, Line 1749: struct fpsba *fpsba, uint32_t gpr0_offset, These arguments are not necessary
https://review.coreboot.org/c/coreboot/+/81928/comment/e0db5437_a0ce6d59 : PS11, Line 1753: enabled_gprd_reg.value = fpsba->pchstrp[gpr0_offset]; This is not necessary (and doesn't do anything since you overwrite all the fields below).
https://review.coreboot.org/c/coreboot/+/81928/comment/cbaf29b1_3ae70d5d : PS11, Line 1786: union gprd enabled_gprd; : enabled_gprd = get_enabled_gprd(fpsba, gpr0_offset, image, size); : nit: combine onto one line
https://review.coreboot.org/c/coreboot/+/81928/comment/8741ba4a_74497d98 : PS11, Line 1807: union gprd enabled_gprd; : enabled_gprd = get_enabled_gprd(fpsba, gpr0_offset, image, size); nit: combine onto one line
https://review.coreboot.org/c/coreboot/+/81928/comment/59c611ec_186a2c65 : PS11, Line 1818: enabled_gprd This should be `fpsba->pchstrp[gpr0_offset]` (print the actual range, not the enabled range).