Marshall Dawson 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/2/Documentation/soc/amd/psp_i... File Documentation/soc/amd/psp_integration.md:
https://review.coreboot.org/c/coreboot/+/37847/2/Documentation/soc/amd/psp_i... PS2, Line 265: | Reset Image | 0x02 | 1 | Boolean value to define the|
Well the quick answer is it's byte 2, bit 0, 1, 2, etc. I could go either way on clarifying it vs. […]
Ack
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?
Done
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 12:
add "on the PSP"?
Done
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 13: .
"on the x86 cores. […]
Done
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?
Done
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 […]
Done
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 lo […]
It's more about how the amdfw.rom artifact is constructed and not about positioning with cbfs. It's built specifically for sitting at one of the 6 locations. With stoneyridge and picasso, we have an index chooser in Kconfig. Looks like that's not in southbridge/amd.
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?
See if the change works any better for you.
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 68:
add a "the"?
Lots of people seem to use "PSP" almost like a proper name. I agree with you, treating like a noun feels more natural.
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 69: for
drop "for"?
Done
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 119: ,
drop ","
Done
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 178: .
"from reset. […]
Done
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?
Done
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
Done
https://review.coreboot.org/c/coreboot/+/37847/3/Documentation/soc/amd/psp_i... PS3, Line 286: SubProgram
one word or space in the middle?
Done
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/d […]
Ack. Kyosti would like for it to change, so I'm not sure how much effort to put into their descriptions now.
amdcompress began as a brand new standalone utility that would simply compress a binary and tack on the header that the PSP expects. Then I added the ability to take an elf file as input and compress only the program portion. So it's only a compressed piece of code with a header on the front. Along the way, there was a suggestion to add the all that to cbfstool vs. a standalone program; very likely because it was borrowing a lot of the elf definitions from cbfsutil.
amdfwutil is the main utility that takes 100% of the PSP blobs (plus the code mentioned above) and creates one single image. It pregenerates all the tables with the correct pointers, the firmwares for the pointers to point to, etc. That artifact is typically called amdfw.rom, then it's simply added into the coreboot image. Once it's added, all the pointers within the tables become correct. (Oh well I was a bit more clear below).
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?
Done
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
Done. Well that's fine, of course.
You wouldn't use a phrase like "passed on the command line" or "passing arguments"?