Attention is currently required from: Robert Zieba, Raul Rangel, Jon Murphy, Felix Held.
Karthik Ramasubramanian 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 13:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/66761/comment/ce982f73_fa57c601 PS12, Line 19: 0xd92000
Maybe print a header describing the fields?
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/e3e40fee_7602f444 PS12, Line 55: 0x0
Hrmm, maybe `%#08x` to match the indentation?
Done
File util/amdfwtool/amdfwread.c:
https://review.coreboot.org/c/coreboot/+/66761/comment/b0f6996f_0fa9ed36 PS12, Line 244: !=
nit: Same as below, make truthy
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/19f97c46_71f9f12f PS12, Line 250: Address
Can we print the address?
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/13722c97_41cbbccf PS12, Line 263: l2_dir_offset
Print the address
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/d0b33a6e_7350e0e2 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
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/32f4c364_c98896db PS12, Line 292: if (type != AMD_PSP_FUSE_CHAIN)
nit: Swap the blocks and invert condition to make it truthy.
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/dc9c62e2_5502af3e PS12, Line 298: Flag Not an Address
Maybe print out the actual fuse value?
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/89a3c884_4b58cc15 PS12, Line 311: l2_dir_offset
Would be nice to see this printed as well. […]
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/c39bed36_7dc774ca PS12, Line 329: l2_dir_offset
Same as above
Done
https://review.coreboot.org/c/coreboot/+/66761/comment/d4a78894_d6b46fa6 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?
This is break for the case statement so that we dont fall-through to the next case.
https://review.coreboot.org/c/coreboot/+/66761/comment/56c98b75_fc3e421f PS12, Line 469: _ro
Is this function limited to ro? Or is the `fw_header` pointing to ro?
This is limited to reading only amdfw.rom in RO region. amdfw_[ab].rom is actually a subset of amdfw.rom.
Needs some additional work to list their contents. Can be done as a follow-up change.
-
Karthik Ramasubramanian (Code Review)