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 15:
(22 comments)
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 8: developed\ Why a newline here?
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 9: AMD's AGESA V9 (also UEFI). You could drop "AMD's". You're already established AMD and have also already mentioned it in this sentence. Change to "v5" to match other common usage as well as the rest of this doc. I would drop "(also UEFI)". I don't think the clarification really applies.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 9: which is UEFI I would probably skip the mention of UEFI here, and maybe replace it with tianocore.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: in turn I would drop this phrase.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: differences I wouldn't use "differences" twice in the same sentence. And actually, I don't think it's v5 vs. v9 that's driving the differences. (Although AMD refers to the ABLs on the PSP as AGESA, I won't attempt to convey that here.) Let's just call it the new Family 17h behavior driving it.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: causes "cause", however you might choose a different word like "drive".
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 14: , which will Can you make this a new sentence? Maybe simply See below for details.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 15: "**FSP Differences caused by hardware and AGESA differences**" I think this is just awful when rendered my a markdown viewer. It simply sticks out as bold text.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 16: . Looks like you intended a comma.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 17: AGESA Replace "AGESA does not" with "the cache design cannot"
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 19: to : be placed to place
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 22: As a consequence of the above, reset How about just "The reset vector..."
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 31: support implement
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 32: As a consequence, there’s no need of FSP-T How about "No FSP-T is provided"
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 33: FSP-M is needed (basically reporting memory). Maybe consider a separate item for DRAM and the stripped down FSP-M. Also, I would promote the reporting of the memory configuration to a full sentence from being an afterthought. And no need to say "basically".
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 34: AGESA internal dependencies don't conform to FSP specs I still don't quite understand what this means. If it's related to the stuffing everything into FSP-M in June/July, maybe it just goes away.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 35: Currently all code is being landed in FSP-M due to AGESA dependencies. : However it's intended that some code will land in FSP-S in the future. Please remove.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 48: : ## Current functionality distribution : : * This is a temporary item, to be updated as work progresses. This will be : removed as work ends. : * All AGESA DXE code made PEI. Most code being interfaced through FSP-M, : though some code already being interfaced through FSP-S. This changes almost : weekly, so no point on describing what is currently where. : * Current problems are related to AGESA and hardware dependencies, such as : memory reporting depends on PCI-e initialization. I think you can remove this entire section (and change the name of the one below - maybe "Feature distribution"). "All AGESA DXE code made PEI". So did Intel, so I don't think it's worth mentioning. The dependency you mention has been resolved.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 69: Picasso Since this is for all Family 17h, I'd drop the codename here. You could replace it with something like "proprietary".
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 70: doesn't depend on memory reporti I think your logic is reversed here. Not _required_ for reporting?
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 70: , including devices : we are not yet aware of. Please remove
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 72: I think it's worth mentioning the Notify phases too.