Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37847 )
Change subject: Documentation/soc/amd: Add PSP integration information ......................................................................
Patch Set 3:
(18 comments)
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... File Documentation/soc/amd/psp_integration.md:
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 3: integration of modules into PSP tables "structure of the PSP tables pointing to the integrated modules" maybe?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 12: add "on the PSP"?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 13: . "on the x86 cores." maybe?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 19: , in order "in the following order" at the end of the sentence?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 21: * 0xfffa0000 : * 0xfff20000 : * 0xffe20000 : * 0xffc20000 : * 0xff820000 : * 0xff020000 those are locations when the spi flash is mapped in the x86's memory space and not the locations in the flash, right? would be useful to add that information
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 28: Most coreboot implementations isn't that common for coreboot and not only "most", since cbfstool can put image parts at defined locations?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 55: n what does the n/nn/nnn mean here?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 68: add a "the"?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 69: for drop "for"?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 119: , drop ","
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 178: . "from reset." maybe?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 181: * Optional image containing a signed whitelist of serial number(s). serial numbers?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 268: add a ", bit 0" here to make things clearer? same for the entries below
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 286: SubProgram one word or space in the middle?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 322: ABLs ABL?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 342: i wonder how amdcompress, cbfstool and amdfwtool work together; would be good if you also describe/document that; could also be done in a follow-up patch
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 347: it will release the x86 for execution "the x86 starts execution when released from reset" maybe?
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 357: passed in "added" maybe? "passed in" sounds a bit weird to me; i'm not a native english speaker though