Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 10: he Family 17h differences from older AMD and Intel CPU/SOC are: None of these items are relevant to FSP. I think you're only confusing the reader. The exception I see is no CAR.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12: (similar to Intel’s ME) I still don't like mentioning Intel ME here. Don't risk that someone will interpret the ME as also initializing DRAM, but don't cede importance to Intel over AMD.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 13: PSP loads a section of code (a header in the : SPI tells which code and where to load) into RAM and gives control to : it. Why refer to it as a "section" of code? Maybe say the PSP decompresses the BIOS image into DRAM.
Also, the way you've worded this, the PSP gives control to code. I think you should say the PSP starts the x86 execution there.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 17: Reset vector is not the old 0xFFFFFFF0. Don't simply say what it's not -- also say what it is (see comment above). You said before you would reword this and marked it Done without making any changes.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 20: This document I recommend you don't begin back to back sentences with the same phrase
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 27: FSP-T calls are maybe "the FSP-T TempRamInit() call is"
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 32: FSP-M Change to FspMemoryInit() because FSP-M also contains TemRamExit().
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 33: emory sp
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 37: The PSP typically loads the boot vector and associated code into RAM. I don't see what this has to do with "3. FSP-M is loaded into DRAM". It's the x86 that loads FSP-M from cbfs into DRAM.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 42: This is an engineering decision still : yet to be made. Is this true? You were pretty adamant before.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 45: extra line
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 47: 1. Memory fragmentation : : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside. I have no idea what you mean here, so I'm sure the reader won't understand it. Oh, unresolved from PS1. Since this is a work in progress, why not remove it.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 54: don’t doesn't
And if you still stand by this statement, I think you need more details. But I still think they're packed.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 59: can be made Don't talk about what "can be". Only discuss what is and what we're doing.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 64: Similar to AGESA, FSP will call back to platform specific code. This is tbd. If you leave this here I would cite Intel Ice Lake also for MP Services.