Attention is currently required from: Robert Zieba, Jon Murphy, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/66761 )
Change subject: util/amdfwtool/amdfwread: List AMDFW RO binary entries ......................................................................
Patch Set 12:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/66761/comment/0c490301_ee5fd8e8 PS12, Line 19: 0xd92000 Maybe print a header describing the fields?
https://review.coreboot.org/c/coreboot/+/66761/comment/52449c66_297259c7 PS12, Line 55: 0x0 Hrmm, maybe `%#08x` to match the indentation?
File util/amdfwtool/amdfwread.c:
https://review.coreboot.org/c/coreboot/+/66761/comment/279a52fb_e7281a7c PS12, Line 244: != nit: Same as below, make truthy
https://review.coreboot.org/c/coreboot/+/66761/comment/fabf0db4_2863a73d PS12, Line 250: Address Can we print the address?
https://review.coreboot.org/c/coreboot/+/66761/comment/4257d252_d2e2bd9a PS12, Line 263: l2_dir_offset Print the address
https://review.coreboot.org/c/coreboot/+/66761/comment/76aa8d60_6ac69936 PS12, Line 269: amdfw_bios_dir_walk It would be nice if you passed an indentation counter so when you print it shows up as a tree
https://review.coreboot.org/c/coreboot/+/66761/comment/64f7b171_43f5757b PS12, Line 292: if (type != AMD_PSP_FUSE_CHAIN) nit: Swap the blocks and invert condition to make it truthy.
https://review.coreboot.org/c/coreboot/+/66761/comment/69235ac1_45d90ded PS12, Line 298: Flag Not an Address Maybe print out the actual fuse value?
https://review.coreboot.org/c/coreboot/+/66761/comment/80b5cf99_8ceae876 PS12, Line 311: l2_dir_offset Would be nice to see this printed as well. Maybe ` -> %#x`
https://review.coreboot.org/c/coreboot/+/66761/comment/d6be0f8c_f1f94d76 PS12, Line 329: l2_dir_offset Same as above
https://review.coreboot.org/c/coreboot/+/66761/comment/4a069b9d_3cb4a93f PS12, Line 330: break Do you want to break? Don't you want to processes all the entries to verify you don't have dups?
https://review.coreboot.org/c/coreboot/+/66761/comment/dca35771_7c5e7e6d PS12, Line 469: _ro Is this function limited to ro? Or is the `fw_header` pointing to ro?