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 2:
(16 comments)
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... File Documentation/binaries/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 4: FSP
You need to specify 2.0 somewhere.
FSP 2.0? If so, will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 4: f
Cap F, plus three more locations in the document.
Will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 5: premisses
I believe premises is the more common spelling.
Will double check with error correction... if it accepts, will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 9: deviation
Should be plural. I'm not sure deviation is a good word here. […]
thanks. will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 10: (similar to Intel's ME)
It's very misleading the way that you've included this here. […]
Will try to rephrase it.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 12: not the old
"not at the traditional physical address of" […]
Have not thought about it... will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 19: 1. **No FSP-T** : Because
Did you intend to put these items on the same line? The plugin I'm using to read it puts them on th […]
Will try something on my plugin. If it works, I'll update it and see how it looks in yours. However, if you use gitweb browse, they show up in separate lines.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 20: Because family 17h does not support CAR, there's no FSP-T.
Can you simply make these items more straightforward statements? "Family 17h does not require Cache […]
Ok.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 21: **FSP-M only reports memory**
How about "FSP-M does not initialize memory: Executing FSP-M is still required to obtain the memory […]
Good idea.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 22: inittialized
sp
?
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 24: 3. **FSP-M is loaded to DRAM** : PSP can be made to load a section of the flash into RAM before releasing : the reset, thus FSP-M can be made to run directly from memory.
I would remove this. It seems to imply the PSP is loading FSP-M into DRAM - not true. […]
Ok.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 27: . **FSP-M can be made position independent** : Because it's loaded to memory and does not uses CAR, FSP-M can be made PIC : (Position Independent Code).
As for above, I don't think we've done anything unique here either.
No, but Intel's FSP is highly position dependent. I don't know why Intel forces FSP to be located at a particular address, but it's a difference from our code.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 32: 1. **Memory fragmentation** : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside.
That's a really loaded statement, and I'm really not sure what you're trying to say. […]
Alex provided this statement, I just rephrase it some.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 36: intergers
spelling, and I don't believe this is a correct statement.
Again, Alex provided this statement.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 37: 3. **UPD with no UEFI dependencies** : UPD interface can be made C99 or C11 compatible with no hard dependencies : to UEFI.
I don't understand this. […]
Pretty much all of "engineering decisions" were Alex. I just rephrase his crude statement.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 40: 4. **Platform specific code**
This is also not unique to our implementation.
Alex said so, will investigate. He implied that Intel's does not do such calls.