Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 7:
(16 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. […]
I don't agree. Maybe I should remove reference to Intel, but the remaining is important to explain why FSP is being implemented this way.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12:
Oh no, trailing spaces
Ack
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. […]
Ack, will remove the offending section.
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. […]
Thanks, will consider it.
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). […]
I guess I misunderstood what you wanted. Will rephrase (and perhaps join with the one above).
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
Will do.
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"
Is there any plan to use FSP-T at all?
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().
I'm going to rewrite this, as Martin explained that FSP-M still does a lot of chip initialization.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 33: emory
sp
Oops
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". […]
Will change some of it. Was trying to save something I misunderstood due to badly phrased information I received from Alex.
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.
From Alex: Looks like he wants to make it position independent, but is having problems to achieve it (don't know why). Will probably remove this until it's solved one way or the other.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 45:
extra line
Ack
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. […]
Will do.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 54: don’t
doesn't […]
Shall we discuss this with Alex? This is his statement.
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.
Will remove.
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.
Not sure on this one. I understood one way from what Alex wrote, than he contradicts me. I'll probably just remove until I know better.