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 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 \
coreboot's online docs are driven by Sphinx (Look at the bottom of https://doc.coreboot.org/). […]
If you open the document within coreboot review and then click in browse, it works exactly as I want, and the backslash does not show up. That's enough for me.
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
Done
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 6: coreboot compatible
hyphenate if you keep this wording
Done
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. […]
A major rewrite also takes longer... but I get your point.
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 […]
Not sure I understand what you are saying.
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. […]
Alex wording... will rephrase.
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. […]
Rephrased with above, I believe solved both.
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.
Rephrased.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 17: AGESA
the cache design
True, but I don't want to discuss this here (someone complained before when I did so). Instead I'm saying that AGESA does not support CAR (not explaining why) and thus FSP-T is not needed. It gives the impression (though wrong) that CAR is still possible, just not needed. That's what I was requested to say (to not imply that CAR is no longer supported).
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".
I don't believe the whole code is loaded into memory by the PSP. Anyway, rephrased.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 21: (also in the header)
I'd delete one of these
Ack
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. […]
Ack
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 25:
whitespace
Ack
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 31: HW
I would spell this out
Ack
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.
Done
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.
Sounds strange to start a phrase without capital letter, but coreboot must be all lower caps... Thus I added the "However". Based on below, this will be rephrased.
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 38: romstage
This is irrelevant
Ack
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 46: There’s
There are
Ack
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 48: does : start
starts
Done
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
That's what Alex said. Is it inaccurate?
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.
I'm not talking about UEFI dependencies, but real ones such as you cant report memory before PCIe initialization (not enumeration). Maybe add "hardware"?
https://review.coreboot.org/c/coreboot/+/34662/11/Documentation/binaries/amd... PS11, Line 71: PCI-express
PCI Express
Done
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.
That's from Alex... I agree it's weird. I think maybe he wanted to say "devices he's not yet aware exists inside the SOC". Until explanations from Alex, this stays.