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 5:
(11 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 342:
My suggestion was to move amdcompress outside util/cbfstool and into util/amdfwtool.
If I skewed your suggestion, that wasn't my intention. I was only trying to indicate that I didn't want to document the current state of the tools any more meticulously, since they're likely to change.
Felix, it looks like I misstated cbfstool's usage above but I think the text below is still accurate. amdcompress is a standalone utility whose source is in the cbfstool directory and shares some files. With that context, cbfstool is used mostly where it always has been.
or does only the boot block get processed by amdcompress into a BIOS reset image and the other coreboot stages still reside in the flash like on other platforms?
It sounds like you've got it. Here's the newly updated bootflow: * Forget any previous conversation or PSs you may have seen about a hybrid romstage, etc. and assume the "normal" bootblock, romstage, ramstage flow. No postcar, and any x86-based verstage implementation is TBD. * The PSP uncompresses the bootblock into memory for us, modifies the CS shadow register to place the reset vector in RAM, then releases reset. Because the PSP does the initial work, and the reset vector isn't 0xfffffff0, an executable bootblock is not required at the top of flash. * bootblock finds/loads/runs romstage (as always), except it's no longer XIP. romstage is loaded to and run from DRAM. Same with FSP. Same with ramstage of course.
All stages and FSP after bootblock are loaded by the x86 -- there has been discussion of trying to combine them into one large compressed image for the PSP, but that's not the case currently (and nobody is working on it). bootblock is currently the only image the PSP deals with. So: * bootblock.elf is fed to amdcompress and the output is amd_biospsp.img. This is the compressed program portion of bootblock.elf. bootblock is linked to match the runtime addressing, and this keys off of CONFIG_X86_RESET_VECTOR. * amd_biospsp.img is then fed, along with all the various PSP blobs to amdfwtool and the output is amdfw.rom. * cbfstool assembles files and stages like normal. ** amdfw.rom is typically added as a cbfs file, however this is not a requirement. We can also shrink cbfs smaller than the flash device and then add amdfw.rom to coreboot.rom manually.
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... File Documentation/soc/amd/psp_integration.md:
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 15: system BIOS.
Reflow on line above?
The rule described here (https://opusdesign.us/wordcount/typographic-widows-orphans/) is how I learned to write, so I don't want to have a single word on the final line. Arguably the entire paragraph could be massaged to look better, but since md rendering doesn't care about line breaks, I'm not sure it's worthwhile.
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 201: Bytes
bytes
Done
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 201: Every firmware binary contains 256 Bytes of a PSP Header, which includes : firmware version
*the* firmware version?
Done
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 239: "Region Type"
Mark it up with ** so no quotes are needed?
These currently match the AMD documentation.
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 292:
please also add the bits for this and the next line
Thanks, missed those.
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 338: **0x66**: x86 microcode patch
Use microcode update patch?
AMD seems to only call it "microcode patch".
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 351: cbfstool/amdcompress
Mark up with ``?
Done
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 354: elf
ELF
Done
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 364: amdfw.rom
Mark up with ``?
Done
https://review.coreboot.org/c/coreboot/+/37847/4/Documentation/soc/amd/psp_i... PS4, Line 368: "AMD Platform Security Processor BIOS Architecture : Design Guide for AMD Family 17h Processors"
Mark up with ** instead of quotes.
Done