Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/AMD_FSP_family_17h.md A Documentation/binaries/index.md 2 files changed, 49 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/1
diff --git a/Documentation/binaries/AMD_FSP_family_17h.md b/Documentation/binaries/AMD_FSP_family_17h.md new file mode 100644 index 0000000..b46beb3 --- /dev/null +++ b/Documentation/binaries/AMD_FSP_family_17h.md @@ -0,0 +1,41 @@ +# FSP implementation differences between Intel and AMD + +## Introduction +Starting with family 17h, AMD is developing an "_As Close As Possible_" FSP +binary. However, some premisses are different for family 17h and beyond, +making it necessary to have some FSP implementation differences. Some other +implementation differences were more of an engineering decision. + +The family 17h deviation from older AMD and Intel CPU/SOC are: +* The memory is initialized by the PSP (similar to Intel's ME) ARM. +* There's _**no support**_ for cache as RAM. +* Reset vector is not the old 0xFFFFFFF0. + +This document is a "work in progress", documenting the differences at a +high level. This document will be updated as more information becomes +available. + +## Differences caused by differences in premisses +1. **No FSP-T** +Because family 17h does not support CAR, there's no FSP-T. +2. **FSP-M only reports memory** +Because memory is inittialized by the PSP, FSP-M only reports the final +memory configuration. +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. +4. **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). + +## Differences by engineering decision +1. **Memory fragmentation** +Though FSP still fragments memory, it has added control for flexibility +of where the chunks will reside. +2. **UPD interface** +UPD interface uses native intergers and don't need to be packed by compiler. +3. **UPD with no UEFI dependencies** +UPD interface can be made C99 or C11 compatible with no hard dependencies +to UEFI. +4. **Platform specific code** +Similar to AGESA, FSP will make call back to platform specific code. diff --git a/Documentation/binaries/index.md b/Documentation/binaries/index.md new file mode 100644 index 0000000..9093bf7 --- /dev/null +++ b/Documentation/binaries/index.md @@ -0,0 +1,8 @@ +# binaries-specific documentation + +This section contains documentation about any binary used by coreboot + +## Video + +## Platform initialization +- [AMD FSP](AMD_FSP_family_17h.md)
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 1:
Initial documentation on FSP.
Hello Alexandru Gagniuc, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#2).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/cavium/index.md A Documentation/binaries/index.md A Documentation/binaries/intel/index.md 5 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/2
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.
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 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.
FSP 2.0? If so, will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 4: f
Cap F, plus three more locations in the document.
Will do.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 5: premisses
I believe premises is the more common spelling.
Will double check with error correction... if it accepts, will do.
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. […]
thanks. will do.
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. […]
Will try to rephrase it.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 12: not the old
"not at the traditional physical address of" […]
Have not thought about it... will do.
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 th […]
Will try something on my plugin. If it works, I'll update it and see how it looks in yours. However, if you use gitweb browse, they show up in separate lines.
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 […]
Ok.
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 […]
Good idea.
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. […]
Ok.
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.
No, but Intel's FSP is highly position dependent. I don't know why Intel forces FSP to be located at a particular address, but it's a difference from our code.
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. […]
Alex provided this statement, I just rephrase it some.
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.
Again, Alex provided this 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. […]
Pretty much all of "engineering decisions" were Alex. I just rephrase his crude statement.
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.
Alex said so, will investigate. He implied that Intel's does not do such calls.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 12: * Reset vector is not the old 0xFFFFFFF0. Expand on this some?
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 25: PSP can be made to load a section of the flash into RAM Maybe "The PSP typically loads the boot vector and associated code into RAM"?
The way it was previously phrased makes it sound like it's a trick.
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 40: 4. **Platform specific code** : Similar to AGESA, FSP will make call back to platform specific code. : Hrm. This is the first I've heard of this, and could create significant problems. This was allowed in AGESA because it was open-sourced. Some people consider this to be "linking" the binaries, and therefore a violation of the GPL.
What are we calling back into coreboot for?
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... PS2, Line 7: ## Platform initialization : - [AMD FSP](AMD_FSP_family_17h.md) What does this have to do with cavium?
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:
(5 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
FSP 2.0? If so, will do.
yes
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 22: inittialized
?
spelling
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).
No, but Intel's FSP is highly position dependent. […]
I would find a way to reword the statement, then, because the Intel FSP can also be loaded to memory.
It may still be a valid point you're trying to make. I looked more closely at the the driver code and it loads it at an address it gets from the header.
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.
Pretty much all of "engineering decisions" were Alex. I just rephrase his crude statement.
Is this from a recent conversation or from some of the early planning documents? Things have been evolving since then and our implementation will be more FSP_2.0-compliant than he originally hoped.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 40: 4. **Platform specific code**
Alex said so, will investigate. He implied that Intel's does not do such calls.
It's not designed into the interface definition, but the Ice Lake implementation can pass in a pointer to a PPI that sits in coreboot. Then I assume the FSP simply installs the PPI. It actually seems way simpler than the AGESA v5 and binaryPI method for callouts.
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 2:
(8 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 22: inittialized
spelling
Ack
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).
I would find a way to reword the statement, then, because the Intel FSP can also be loaded to memory […]
Will try.
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.
Is this from a recent conversation or from some of the early planning documents? Things have been e […]
From a spread sheet he filled up yesterday... you can see what he wrote.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 40: 4. **Platform specific code**
It's not designed into the interface definition, but the Ice Lake implementation can pass in a point […]
Needs more discussion
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 12: * Reset vector is not the old 0xFFFFFFF0.
Expand on this some?
Marshall suggested pointing to family 17h (link), will that suffice?
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 25: PSP can be made to load a section of the flash into RAM
Maybe "The PSP typically loads the boot vector and associated code into RAM"? […]
Will do.
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 40: 4. **Platform specific code** : Similar to AGESA, FSP will make call back to platform specific code. :
Hrm. This is the first I've heard of this, and could create significant problems. […]
I don't know, ask Alex at conference call today. The document he provided me was vague...
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... PS2, Line 7: ## Platform initialization : - [AMD FSP](AMD_FSP_family_17h.md)
What does this have to do with cavium?
Should have been removed when I copy/pasted... will remove.
Hello Alexandru Gagniuc, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#3).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/cavium/index.md A Documentation/binaries/index.md A Documentation/binaries/intel/index.md 5 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/3
Hello Alexandru Gagniuc, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#4).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/cavium/index.md A Documentation/binaries/index.md A Documentation/binaries/intel/index.md 5 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/4
Hello Alexandru Gagniuc, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#5).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/cavium/index.md A Documentation/binaries/index.md A Documentation/binaries/intel/index.md 5 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/5
Hello Alexandru Gagniuc, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#6).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/cavium/index.md A Documentation/binaries/index.md A Documentation/binaries/intel/index.md 5 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 41: intergers inte*r*gers? that's a new datatype :P
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 48: make call back "call back" without the "make" sounds better
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
PS6: Are these placeholders added on purpose?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 6:
(4 comments)
Thank you for writing documentation.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available. Please use the full text width.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 21: > Family 17h does not require Cache as RAM and FSP-T calls are unsupported. I think it does not need to be formatted as citation.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 31: uses use
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 32: a an
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 6:
(15 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
yes
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 4: f
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 5: premisses
Will double check with error correction... if it accepts, will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 9: deviation
thanks. will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 10: (similar to Intel's ME)
Will try to rephrase it.
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 12: not the old
Have not thought about it... will do.
Done
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.
Ok.
Done
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 21: **FSP-M only reports memory**
Good idea.
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available.
Please use the full text width.
It was going beyond 76 characters... What is the limit for documentation?
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 21: > Family 17h does not require Cache as RAM and FSP-T calls are unsupported.
I think it does not need to be formatted as citation.
It was a way to separate context from bullet point... they were being placed on the same line. Will see if I can figure a different way.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 31: uses
use
will do.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 32: a
an
thanks...
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 41: intergers
inte*r*gers? that's a new datatype :P
lol typo.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 48: make call back
"call back" without the "make" sounds better
thanks.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
PS6:
Are these placeholders added on purpose?
yes
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 1: # FSP implementation differences between Intel and AMD Maybe this document should just be about the AMD FSP, with the differences being a subsection?
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available.
It was going beyond 76 characters... […]
72 characters:
https://doc.coreboot.org/getting_started/writing_documentation.html
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP. FSP-M is really being used as an initial call into the FSP and no longer really has anything to do with memory. Right now, it could really be considered "FSP-ROMSTAGE" with FSP-S being "FSP-RAMSTAGE", but even that might change at some point as we're still looking at getting rid of romstage and only running ramstage.
We're also still discussing what parts of AGESA need to go into FSP-M and what needs to be in FSP-S.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
PS6:
yes
You might want to give an explanation as to what the plan is with these placeholders. Are you planning on filling them out? If not, it might just be better not to have them at all, rather than creating the stub.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... File Documentation/binaries/index.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... PS6, Line 2: Are you updating the main documentation page to point to this in a separate commit? Why not just add this as a section to the main page?
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 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 1: # FSP implementation differences between Intel and AMD
Maybe this document should just be about the AMD FSP, with the differences being a subsection?
This would be a change from the initial request on issue tracker. Can be done, would need to rewrite the whole document.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available.
72 characters: […]
So I have to redo it cutting down to 72, not expand as Paul is asking? Also notice that I'm counting columns as it appears to the user, not including markdowns that don't show up to user (example, the link don't show, just the title of the link).
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP.
FSP-M is really being used as an initial call into the FSP and no longer really has anything to do w […]
As I mentioned above, this is a work in progress and as FSP evolves, correction will be needed. Corrections will be separate commits until it's no longer a work in progress, when that message will be removed.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
PS6:
You might want to give an explanation as to what the plan is with these placeholders. […]
Ok, as I'm not planning on filling them I shall remove them.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... File Documentation/binaries/index.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... PS6, Line 2:
Are you updating the main documentation page to point to this in a separate commit? Why not just ad […]
Because I want to leave the opportunity for Intel and Cavium also use the binaries documentation folder. Therefore I have to at least create an AMD folder inside the binaries folder.
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP.
As I mentioned above, this is a work in progress and as FSP evolves, correction will be needed. […]
By the way, does FSP-M currently does anything?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available.
So I have to redo it cutting down to 72, not expand as Paul is asking? Also notice that I'm counting […]
Yes. Find an editor with a reflow function. I use Atom which has reflow and also a markdown preview plugin.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP.
By the way, does FSP-M currently does anything?
Yes, as of this instant, FSP-M handles all the AGESA calls and we're not using FSP-S for anything. By the time you get this document merged, that might have changed.
For now, I'd just say that the functionality in FSP-M and FSP-S is still undetermined and will be decided in the upcoming weeks and months.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... File Documentation/binaries/index.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/inde... PS6, Line 2:
Because I want to leave the opportunity for Intel and Cavium also use the binaries documentation fol […]
Why not just let them do that if/when they decide to add their documentation? I appreciate the thought, I'm just worried that it will lead people to think that there's more documentation than there actually is.
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#7).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the differences between Intel's FSP and the one developed by AMD.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 79 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/7
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 7:
(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 19: 1. **No FSP-T** : Because
Will try something on my plugin. If it works, I'll update it and see how it looks in yours. […]
Done
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.
Ok.
Rewritten.
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).
Will try.
Done
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.
Alex provided this statement, I just rephrase it some.
Asked Alex to change it.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 36: intergers
Again, Alex provided this statement.
Asked Alex to change it.
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 12: * Reset vector is not the old 0xFFFFFFF0.
Marshall suggested pointing to family 17h (link), will that suffice?
Done
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 25: PSP can be made to load a section of the flash into RAM
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 1: # FSP implementation differences between Intel and AMD
This would be a change from the initial request on issue tracker. […]
Partially done.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 17: available.
Yes. Find an editor with a reflow function. […]
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 21: > Family 17h does not require Cache as RAM and FSP-T calls are unsupported.
It was a way to separate context from bullet point... they were being placed on the same line. […]
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP.
Yes, as of this instant, FSP-M handles all the AGESA calls and we're not using FSP-S for anything. […]
For patch set 8.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 31: uses
will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 32: a
thanks...
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 41: intergers
lol typo.
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 48: make call back
thanks.
Done
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
PS6:
Ok, as I'm not planning on filling them I shall remove them.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12: Oh no, trailing spaces
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 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 10: he Family 17h differences from older AMD and Intel CPU/SOC are: None of these items are relevant to FSP. I think you're only confusing the reader. The exception I see is no CAR.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12: (similar to Intel’s ME) I still don't like mentioning Intel ME here. Don't risk that someone will interpret the ME as also initializing DRAM, but don't cede importance to Intel over AMD.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 13: PSP loads a section of code (a header in the : SPI tells which code and where to load) into RAM and gives control to : it. Why refer to it as a "section" of code? Maybe say the PSP decompresses the BIOS image into DRAM.
Also, the way you've worded this, the PSP gives control to code. I think you should say the PSP starts the x86 execution there.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 17: Reset vector is not the old 0xFFFFFFF0. Don't simply say what it's not -- also say what it is (see comment above). You said before you would reword this and marked it Done without making any changes.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 20: This document I recommend you don't begin back to back sentences with the same phrase
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 27: FSP-T calls are maybe "the FSP-T TempRamInit() call is"
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 32: FSP-M Change to FspMemoryInit() because FSP-M also contains TemRamExit().
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 33: emory sp
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 37: The PSP typically loads the boot vector and associated code into RAM. I don't see what this has to do with "3. FSP-M is loaded into DRAM". It's the x86 that loads FSP-M from cbfs into DRAM.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 42: This is an engineering decision still : yet to be made. Is this true? You were pretty adamant before.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 45: extra line
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 47: 1. Memory fragmentation : : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside. I have no idea what you mean here, so I'm sure the reader won't understand it. Oh, unresolved from PS1. Since this is a work in progress, why not remove it.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 54: don’t doesn't
And if you still stand by this statement, I think you need more details. But I still think they're packed.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 59: can be made Don't talk about what "can be". Only discuss what is and what we're doing.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 64: Similar to AGESA, FSP will call back to platform specific code. This is tbd. If you leave this here I would cite Intel Ice Lake also for MP Services.
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 7:
(16 comments)
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 10: he Family 17h differences from older AMD and Intel CPU/SOC are:
None of these items are relevant to FSP. I think you're only confusing the reader. […]
I don't agree. Maybe I should remove reference to Intel, but the remaining is important to explain why FSP is being implemented this way.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12:
Oh no, trailing spaces
Ack
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 12: (similar to Intel’s ME)
I still don't like mentioning Intel ME here. […]
Ack, will remove the offending section.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 13: PSP loads a section of code (a header in the : SPI tells which code and where to load) into RAM and gives control to : it.
Why refer to it as a "section" of code? Maybe say the PSP decompresses the BIOS image into DRAM. […]
Thanks, will consider it.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 17: Reset vector is not the old 0xFFFFFFF0.
Don't simply say what it's not -- also say what it is (see comment above). […]
I guess I misunderstood what you wanted. Will rephrase (and perhaps join with the one above).
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 20: This document
I recommend you don't begin back to back sentences with the same phrase
Will do.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 27: FSP-T calls are
maybe "the FSP-T TempRamInit() call is"
Is there any plan to use FSP-T at all?
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 32: FSP-M
Change to FspMemoryInit() because FSP-M also contains TemRamExit().
I'm going to rewrite this, as Martin explained that FSP-M still does a lot of chip initialization.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 33: emory
sp
Oops
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 37: The PSP typically loads the boot vector and associated code into RAM.
I don't see what this has to do with "3. FSP-M is loaded into DRAM". […]
Will change some of it. Was trying to save something I misunderstood due to badly phrased information I received from Alex.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 42: This is an engineering decision still : yet to be made.
Is this true? You were pretty adamant before.
From Alex: Looks like he wants to make it position independent, but is having problems to achieve it (don't know why). Will probably remove this until it's solved one way or the other.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 45:
extra line
Ack
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 47: 1. Memory fragmentation : : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside.
I have no idea what you mean here, so I'm sure the reader won't understand it. […]
Will do.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 54: don’t
doesn't […]
Shall we discuss this with Alex? This is his statement.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 59: can be made
Don't talk about what "can be". Only discuss what is and what we're doing.
Will remove.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 64: Similar to AGESA, FSP will call back to platform specific code.
This is tbd. If you leave this here I would cite Intel Ice Lake also for MP Services.
Not sure on this one. I understood one way from what Alex wrote, than he contradicts me. I'll probably just remove until I know better.
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 7:
(1 comment)
By the way, I'm not sure how much of a service you're doing by indicating "will do" or "will rewrite". Did you see that notice about All-Comments-Resolved? Because of that new requirement, you will still need to go back and mark each of those Done, Ack, or manually Resolve in a new comment. It would seem more efficient (especially to your reviewers) to negotiate items you disagree on, as you did above, and leave the others until you repush. At that time, you'd just mark those as Done.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 27: FSP-T calls are
Is there any plan to use FSP-T at all?
Not at this time, and I can't think of any reason why we would use it. However, my suggestion was more for plural vs. singular. I believe there is only a single FSP-T call defined in the spec, but you can check.
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 7:
Patch Set 7:
(1 comment)
By the way, I'm not sure how much of a service you're doing by indicating "will do" or "will rewrite". Did you see that notice about All-Comments-Resolved? Because of that new requirement, you will still need to go back and mark each of those Done, Ack, or manually Resolve in a new comment. It would seem more efficient (especially to your reviewers) to negotiate items you disagree on, as you did above, and leave the others until you repush. At that time, you'd just mark those as Done.
I could do that if I was executing the changes immediately... as I'm involved in something else (SPI code) I have to answer the review so that reviewers no I saw it, but have to make it obvious to my self that I have not yet implemented something (locally, not yet pushed).
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 7:
(21 comments)
Patch Set 1:
Initial documentation on FSP.
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 32: 1. **Memory fragmentation** : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside.
Asked Alex to change it.
Just removed this section. Might be returned if Alex provides a good explanation.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 36: intergers
Asked Alex to change it.
Rewritten to better explain this.
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.
From a spread sheet he filled up yesterday... you can see what he wrote.
Re-declared as TBD.
https://review.coreboot.org/c/coreboot/+/34662/1/Documentation/binaries/AMD_... PS1, Line 40: 4. **Platform specific code**
Needs more discussion
Currently removed.
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/amd/... PS2, Line 40: 4. **Platform specific code** : Similar to AGESA, FSP will make call back to platform specific code. :
I don't know, ask Alex at conference call today. The document he provided me was vague...
Removed.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 1: # FSP implementation differences between Intel and AMD
Partially done.
No comparison with Intel now.
https://review.coreboot.org/c/coreboot/+/34662/6/Documentation/binaries/amd/... PS6, Line 25: > obtain the memory map from the PSP.
For patch set 8.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 10: he Family 17h differences from older AMD and Intel CPU/SOC are:
I don't agree. […]
I have rephrased the whole introduction, I do hope you like it better.
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 13: PSP loads a section of code (a header in the : SPI tells which code and where to load) into RAM and gives control to : it.
Thanks, will consider it.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 17: Reset vector is not the old 0xFFFFFFF0.
I guess I misunderstood what you wanted. Will rephrase (and perhaps join with the one above).
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 20: This document
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 27: FSP-T calls are
Not at this time, and I can't think of any reason why we would use it. […]
Ack
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 32: FSP-M
I'm going to rewrite this, as Martin explained that FSP-M still does a lot of chip initialization.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 33: emory
Oops
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 37: The PSP typically loads the boot vector and associated code into RAM.
Will change some of it. […]
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 42: This is an engineering decision still : yet to be made.
From Alex: Looks like he wants to make it position independent, but is having problems to achieve it […]
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 47: 1. Memory fragmentation : : Though FSP still fragments memory, it has added control for flexibility : of where the chunks will reside.
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 54: don’t
Shall we discuss this with Alex? This is his statement.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 59: can be made
Will remove.
Done
https://review.coreboot.org/c/coreboot/+/34662/7/Documentation/binaries/amd/... PS7, Line 64: Similar to AGESA, FSP will call back to platform specific code.
Not sure on this one. I understood one way from what Alex wrote, than he contradicts me. […]
Done
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... File Documentation/binaries/cavium/index.md:
https://review.coreboot.org/c/coreboot/+/34662/2/Documentation/binaries/cavi... PS2, Line 7: ## Platform initialization : - [AMD FSP](AMD_FSP_family_17h.md)
Should have been removed when I copy/pasted... will remove.
Done
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#8).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 5: Starting with Family 17h, AMD is AGESA version 9. However, it is currently \ no need for \ at end of line
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 34: As a consequence, there’s no need of FSP-T. Very little of original \ add 3 spaces at the beginning of each line to place the block inside the option list
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 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 5: Starting with Family 17h, AMD is AGESA version 9. However, it is currently \
no need for \ at end of line
I'm using it to force a line break. However, this is mainly when you want to keep the next line with the same formatting as the previous. I believe there are sections in here, this line included, where you are correct. Will remove it where I can.
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 34: As a consequence, there’s no need of FSP-T. Very little of original \
add 3 spaces at the beginning of each line to place the block inside the option list
Yes, but it would re-arrange the line formatting for the whole frame, thus causing difference with other lines above. Using < > was the only option that I found to work as I needed. I'm trying to keep it around 76 characters once framed. Unless you present another solution that does work for the 76 characters formatting, I'll keep it this way. Three spaces before the line does not work as I need. By the way, this is how an add-on to google docs converted it to mark down.
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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 34: As a consequence, there’s no need of FSP-T. Very little of original \
Yes, but it would re-arrange the line formatting for the whole frame, thus causing difference with o […]
Actually you're correct... with a small variation I'm able to keep the formatting I want. With 4 spaces it works with any reader I have...
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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/8/Documentation/binaries/amd/... PS8, Line 5: Starting with Family 17h, AMD is AGESA version 9. However, it is currently \
I'm using it to force a line break. […]
Done
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#9).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 9: Code-Review+1
(2 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 26: is 0xF000:FFF0. See [Family 17h](https://doc.coreboot.org/soc/amd/family17h.html) for more info. whitespace at end of line
https://review.coreboot.org/c/coreboot/+/34662/9/Documentation/binaries/amd/... PS9, Line 33: 1. AGESA does not support CAR \ The backslash has no effect on sphinx
Hello Alexandru Gagniuc, Angel Pons, Patrick Rudolph, Marshall Dawson, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#10).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 90 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/10
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 10:
(2 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 26: is 0xF000:FFF0. See [Family 17h](https://doc.coreboot.org/soc/amd/family17h.html) for more info.
whitespace at end of line
Done
https://review.coreboot.org/c/coreboot/+/34662/9/Documentation/binaries/amd/... PS9, Line 33: 1. AGESA does not support CAR \
The backslash has no effect on sphinx
If I don't add the backslash, next line will be added at the end of this line. It's needed, even if don't work with a particular markdown reader.
Hello Alexandru Gagniuc, Angel Pons, Patrick Rudolph, Marshall Dawson, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#11).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 90 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/11
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 11:
(1 comment)
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 23: already enabled. Can you refer to 'PSP directory' instead of 'header', if that is the case. I assume util/amdfwtool changes have already published those structures, so while Secure Processor specs may still be under NDA, it should not prevent from documenting this more accurately here.
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:
(1 comment)
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 23: already enabled.
Can you refer to 'PSP directory' instead of 'header', if that is the case. […]
This documentation is about FSP implementation, not the PSP or even Family 17h. If you would like more information on PSP, it should either be at Documentation/soc/amd/family17h.md or its own markdown file (that currently does not exists). There's no place for in depth documentation of PSP on a FSP implementation document. That said, I believe amdfwtool updated for Family 17h has not yet been released to coreboot. I believe it's on the plan, but I also believe it's not yet done.
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.
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.
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Patrick Rudolph, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#12).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 87 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/12
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 12:
(1 comment)
Patch Set 11:
(1 comment)
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 23: already enabled.
Ack
Just make a clear distinction, I think start of romstage would be a natural boundary where FSP documentation begins. Any details you write here about bootblock or PSP directory becomes redundant (and possibly out-of-sync) once the situation about PSP documentation improves.
I'd be happy if you just stated "For these platforms, romstage executes from DRAM. Phases prior to romstage are described in XXXX.md" and remove the bootblock details here.
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 12:
(1 comment)
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 23: already enabled.
Just make a clear distinction, I think start of romstage would be a natural boundary where FSP docum […]
I don't have enough information to write such XXXX.md document. Marshall, can you point me to? By the way, this particular line was removed at PS12, so maybe not needed?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 12:
(6 comments)
Please make sure you're marking comments as resolved when you address them. Patches can no longer be merged until all comments are resolved, and while I don't mind marking one as resolved before merging, there are way too many comments in this patch set for anyone to deal with.
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 13: have has
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 14: v8 I'm not sure what AGESA v8 was. I think the previous version (for stoney) was Arch2008
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 20: tell tells the
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 35: GESA has some DXE code, FSP does not \ : So required code was moved within AGESA from DXE phase to PEI phase. So this is no longer an FSP difference, right?
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 37: urrently 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. If we're initializing PCIe in FSP-S, this is no longer true, correct?
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 45: does remove 'does'
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 12:
(6 comments)
Patch Set 12:
(6 comments)
Please make sure you're marking comments as resolved when you address them. Patches can no longer be merged until all comments are resolved, and while I don't mind marking one as resolved before merging, there are way too many comments in this patch set for anyone to deal with.
I'm doing it so. Currently only PS 11 has 2 comments unresolved... because I asked back clarification and have not yet resolved it. All else is resolved.
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 13: have
has
Done
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 14: v8
I'm not sure what AGESA v8 was. […]
AKA AGESA v8 as I understand it. I could also say "previous AGESA" if you prefer.
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 20: tell
tells the
Done
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 35: GESA has some DXE code, FSP does not \ : So required code was moved within AGESA from DXE phase to PEI phase.
So this is no longer an FSP difference, right?
When you put it this way, I guess you are right... will remove.
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 37: urrently 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.
If we're initializing PCIe in FSP-S, this is no longer true, correct?
Currently we are initializing (not enumeration) PCIe in FSP-M, and that is a major dependency that Alex is having problems to break. He wants it in FSP-S (to conform with Intel FSP), but currently it needs to be done before he can report memory available. I'll convert this in to new #2 item to make it clear that this IS the difference I was talking... not really the DXE/PEI issue.
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 45: does
remove 'does'
Done
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Patrick Rudolph, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#13).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 86 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/13
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 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 14: v8
AKA AGESA v8 as I understand it. I could also say "previous AGESA" if you prefer.
Nope, v5 aka Arch2008
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 37: urrently 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.
Currently we are initializing (not enumeration) PCIe in FSP-M, and that is a major dependency that A […]
We should figure out what's really happening here. IIRC historically AMD has used the knowledge of PCI intentions to determine memory settings (although I can't think of the reason at them moment). We still may want to move PCIe stuff later, if reasonable.
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... PS13, Line 20: BIOS Directory Table ...the BIOS Directory Table...
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... PS13, Line 21: reset vector How about "x86 reset vector"
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Patrick Rudolph, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#14).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 86 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/14
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 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/12/Documentation/binaries/amd... PS12, Line 14: v8
Nope, v5 aka Arch2008
Ack
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... PS13, Line 20: BIOS Directory Table
...the BIOS Directory Table...
Done
https://review.coreboot.org/c/coreboot/+/34662/13/Documentation/binaries/amd... PS13, Line 21: reset vector
How about "x86 reset vector"
Done
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 14:
Ping... review please.
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 14:
(2 comments)
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 7: would take too long to do so, it was decided to create a wrapper between : AGESA and coreboot that would be developed
Not sure I understand what you are saying.
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)
That's what Alex said. […]
rewritten, almost a complete new meaning.
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Patrick Rudolph, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#15).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 86 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/15
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:
Ping... please review.
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.
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.
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:
(3 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 17: AGESA
Originally I was told to not enter in details... blame AGESA instead of HW. I would prefer your way. […]
I disagree with a position of placing "blame" on AGESA. What I'm asking for is already public: https://doc.coreboot.org/soc/amd/family17h.html?highlight=family#problem-sta...
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 18: CAR Add a period to match other bullet items.
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 47: TBD Replace with "will be documented separately for each project using the FSP framework". IIRC Intel calls these Integration Guides but they're separate from the EAS.
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:
(24 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\
Oops, will remove.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 9: which is UEFI
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 9: AMD's AGESA V9 (also UEFI).
You mean v9... will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: in turn
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: causes
Dropping the phrase will make this null.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 13: differences
Will figure out exactly how to do it.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 14: , which will
Sure.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 15: "**FSP Differences caused by hardware and AGESA differences**"
I wanted it to stick out, to mark as a sub-title very visibly. […]
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 16: .
Thanks, will fix.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 17: AGESA
I disagree with a position of placing "blame" on AGESA. […]
Ack
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 18: CAR
Add a period to match other bullet items.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 19: to : be placed
will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 22: As a consequence of the above, reset
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 31: support
Ok.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 32: As a consequence, there’s no need of FSP-T
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 33: FSP-M is needed (basically reporting memory).
Ok
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 34: AGESA internal dependencies don't conform to FSP specs
I guess now it can be removed.
Done
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.
Will do.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 47: TBD
Replace with "will be documented separately for each project using the FSP framework". […]
Done
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.
Ok.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 69: Picasso
Ok.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 70: doesn't depend on memory reporti
Ok.
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 70: , including devices : we are not yet aware of.
Ok
Done
https://review.coreboot.org/c/coreboot/+/34662/15/Documentation/binaries/amd... PS15, Line 72:
Ok.
Done
Hello Alexandru Gagniuc, Angel Pons, Marshall Dawson, Patrick Rudolph, Kerry Brown, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34662
to look at the new patch set (#16).
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Documentation/binaries: Add AMD FSP documentation
Create a document explaining, at a high level, the AMD FSP.
BUG=none. TEST=none.
Change-Id: I59a5d34df93cd0ff647e2ccfdbf8700b4df00a59 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A Documentation/binaries/amd/AMD_FSP_family_17h.md A Documentation/binaries/amd/index.md A Documentation/binaries/index.md 3 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/34662/16
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Patch Set 16: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34662/16/Documentation/binaries/amd... File Documentation/binaries/amd/AMD_FSP_family_17h.md:
https://review.coreboot.org/c/coreboot/+/34662/16/Documentation/binaries/amd... PS16, Line 22: See [Family 17h](https://doc.coreboot.org/soc/amd/family17h.html) for more info. use markdown reference
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 16:
Will someone take this over, or should I abandon it? I'm only available until 1/3/2020, than I'll start on a new job (not coreboot related).
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 16:
I'm OK abandoning it. I believe the real intent behind the task was a concern that the AMD implementation would not be FSP 2.0 compliant. (And the original ideas weren't.) We've worked very hard to stay within the FSP EAS with only small exceptions however, like not having/needing FSP-T.
Once we're completely settled on Picasso's UPD structures, we'll publish an Implementation Guide. That may be the right time to publish an EAS errata doc too. Also, this content will still be in gerrit if we decide to revive it.
Richard Spiegel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34662 )
Change subject: Documentation/binaries: Add AMD FSP documentation ......................................................................
Abandoned
It was never completed, I no longer work for Silverback and no one wants to assume it.
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Richard Spiegel (Code Review)