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 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?
Oops, will remove.
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 mean v9... will do.
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.
Will do.
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".
Dropping the phrase will make this null.
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. […]
Will figure out exactly how to do it.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: in turn
I would drop this phrase.
Will do.
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.
Sure.
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.
I wanted it to stick out, to mark as a sub-title very visibly. Will change
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 16: .
Looks like you intended a comma.
Thanks, will fix.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 17: AGESA
Replace "AGESA does not" with "the cache design cannot"
Originally I was told to not enter in details... blame AGESA instead of HW. I would prefer your way. I'll wait until tomorrow, if no one says anything, will do.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 19: to : be placed
to place
will do.
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... […]
Will do.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 31: support
implement
Ok.
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"
Will do.
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. […]
Ok
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. […]
I guess now it can be removed.
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.
Will do.
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 di […]
Ok.
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. […]
Ok.
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. […]
Ok.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 70: , including devices : we are not yet aware of.
Please remove
Ok
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 72:
I think it's worth mentioning the Notify phases too.
Ok.