Attention is currently required from: Bao Zheng, Jason Glenesk, Martin Roth, Marshall Dawson, Zheng Bao. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57747 )
Change subject: amdfwtool: Add ISH header support for A/B recovery layout ......................................................................
Patch Set 24:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57747/comment/521bccd0_5c9dda69 PS24, Line 11: EFS -> PSP L1 0x48 -> PSP L2 A -> ISH A -> BIOS L2 A shouldn't this be: EFS -> PSP L1 0x48 -> ISH A -> PSP L2 A -> BIOS L2 A ? same for the line below
File util/amdfwtool/amdfwtool.h:
https://review.coreboot.org/c/coreboot/+/57747/comment/9fa0d2c9_afb01629 PS24, Line 217: uint32_t glitch_retry_count; document #55758 Rev. 1.13 has one byte for this and 3 reserved bytes after that. since it's little endian, this shouldn't be much of a difference though. when splitting this into an uint8_t and a 3 element uint8_t array for the reserved bytes, it's probably a good idea to mark the struct as packed. also not sure if that document is up to date
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/57747/comment/bf00e831_2895c73e PS24, Line 394: 0x1000 would be good to have a define for this
https://review.coreboot.org/c/coreboot/+/57747/comment/6d8c0c42_42d2b684 PS24, Line 396: 0x1000 would be good to have a define for this
https://review.coreboot.org/c/coreboot/+/57747/comment/85e657d7_3504b369 PS24, Line 662: 0xf the specification i'm looking says that this should be 1
https://review.coreboot.org/c/coreboot/+/57747/comment/26be47f7_e1020627 PS24, Line 670: 0x2 the specification i'm looking says that this should be 1
https://review.coreboot.org/c/coreboot/+/57747/comment/f03be735_1d1bce62 PS24, Line 671: 0xf the specification i'm looking says that this should be 1
https://review.coreboot.org/c/coreboot/+/57747/comment/62d242fd_f732ed3d PS24, Line 686: 0x1000 would be good to have a define for this