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 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.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 4: f Cap F, plus three more locations in the document.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 5: premisses I believe premises is the more common spelling.
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. How about differences?
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. It makes it sound like ME also initializes DRAM, but you were trying to draw a parallel between the ARM processors themselves.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 12: not the old "not at the traditional physical address of" This is probably as good a place as any to put add a reference to the family17h.md file.
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 as RAM and FSP-T calls are unsupported". FWIW, in this doc I wouldn't say does not support CAR, but that it's not required. Not supporting and why gets into the weeds of the architecture.
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 the same line and makes it look funny. I'm not sure if you can force a line break in an ordered list.
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 map from the PSP."
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. The FSP driver already contains FSP_M_XIP, so "making" it run directly from memory isn't a big deal.
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.
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. The fsp2_0 driver has certain expectations about the placement of the FSP-reserved memory. That's not changing, and we're going along with what the driver expects.
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.
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. UPD is simply a structure so I'm not sure how the FSP knows what coreboot did. If you're talking about things like UINT8 vs. uint8_t, that's simply with how the provider chooses to generate the header files. That seems to me to mean that Intel has chosen to be bootloader-agnostic with their product, more than there being a functional difference.
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.