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 11:
(25 comments)
https://review.coreboot.org/c/coreboot/+/34662/9/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/9/Documentation/binaries/amd/... PS9, Line 33: 1. AGESA does not support CAR \
If I don't add the backslash, next line will be added at the end of this line. […]
coreboot's online docs are driven by Sphinx (Look at the bottom of https://doc.coreboot.org/). You should attempt to format this doc so that it will display correctly online.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 6: implemented designed
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 6: coreboot compatible hyphenate if you keep this wording
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 7: would take too long to do so This makes it sound like schedule was the primary motivator. It was more to avoid a major rewrite and introducing unsustainability.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 7: would take too long to do so, it was decided to create a wrapper between : AGESA and coreboot that would be developed I would try to rephrase this to discuss what "is" and omit what was decided in the "past" to work on in the "future".
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 9: Said code was reduced to the bare bones This is a very subjective statement, and besides, I'm not sure I agree with it. I believe we're using the majority of the IntelFsp2Pkg. Perhaps discuss instead, of leveraging IntelFsp2Pkg in tianocore to create the AMD implementation.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 10: with internal calls to AGESA meeting UEFI : specifications (as FSP is written using UEFI) I feel like this is stating the obvious, twice. If you want to talk about the internal structure of FSP, that's valid, because there seems to be a bit of an assumption that the reader already knows what FSP source is, how it's constructed, etc. However I'd maybe move that lower in the discussion. This is simply an intro here.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 14: The differences examined here made for different AGESA, which in turn : caused some differences from standard FSP. I don't know what you're getting at in this bullet.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 17: AGESA the cache design
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 19: section of code In PS7 you agreed not to call it a "section of code".
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 21: (also in the header) I'd delete one of these
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 23: already enabled. Richard, I think Kyosti is simply asking you to use the proper terminology. It's common knowledge (by looking at existing open source) that the BIOS Directory Table identifies the compressed image to be placed into DRAM.
I assume util/amdfwtool changes have already published those structures... That said, I believe amdfwtool updated for Family 17h has not yet been released to coreboot...
It's published.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 22: , thus effectively starting the processor with RAM : already enabled. I'd probably delete this. It's somewhat obvious with the way you've ordered the items, and it's repetitive.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 25: whitespace
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 31: HW I would spell this out
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 35: If there’s a need in the : future, FSP-T might be implemented, but currently there’s no plan on : implementing it. Delete this.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 38: romstage This is irrelevant
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 38: However, I wouldn't start an itemized item with "however". #2 should stand alone.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 39: code was moved within AGESA from DXE phase to PEI phase : and will be interfaced through FSP-M. What remains in DXE phase will : be interfaced through FSP-S. This is inaccurate. There is no DXE phase whatsoever. As a result, all functionality available only in DXE needed to be ported to PEI modules. (I'm confident Intel has done the same thing.) Notification phases are after end of PEI has been signaled, however there is never a DXI IPLed.
Don't confuse romstage vs. ramstage, nor FSP-M vs. FSP-S with PEI vs. DXE. It's an easy mistake to make, but they're not analogous.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 46: There’s There are
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 48: does : start starts
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 58: * Currently all functionality is being implemented in FSP-M (see below for : all functionality) Please remove this
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 60: Current problems I would reword this. It sounds like if we break the dependency then we have no problems.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 71: PCI-express PCI Express
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 74: devices : we are not yet aware of. That's a weird statement.