Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38713 )
Change subject: Documentation/soc/amd/family17: Update to match current design ......................................................................
Patch Set 2:
(11 comments)
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... File Documentation/soc/amd/family17h.md:
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 18: (a.k.a. PSP) in system initialization is addressed here. AMD has
*sigh*, I dislike name changes. Actually this is the first time I've heard "AMD Secure Processor" and everyone still uses "PSP". Haven't found much documentation using the new name either.
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 19: historically required an NDA for access to the PSP
.. AMD continues to require an NDA ..
I don't like the way it sounds, so will make it more direct. I'm still hopeful it'll be open in the future.
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 57: AP
AP and not BSP? I mostly know that acronym from SMP system, where the first processor is the BootStr […]
No, you're right. That mistake comes from working on Chromebooks where Google refers to the EC and the AP. When used in that context, the AP is the entire x86.
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 97: * For the foreseeable future, AGESA source will distributed only
Can you add your expectations or intentions for AMD to give out permissions to distribute custom and […]
I'll add a clarification about debug builds being released, although it's not a favorable position. AFAIK creating and distributing custom, i.e. derivative works, is already permitted through the existing license agreement. (BTW I'm still working to get 3rd parties access to additional NDA source code, including the older binaryPI.)
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 190: enabled
"already available" instead of "enabled"?
Done
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 193: performance, and these regions are marked reserved later in POST.
Can you elaborate about "later in POST"? Reserved only from the OS or excluded from being used in coreboot proper already?
A couple of examples I have in mind: https://review.coreboot.org/c/coreboot/+/38729/1/src/arch/x86/nonxip_rsvd.c or https://review.coreboot.org/c/coreboot/+/38702/1/src/soc/amd/picasso/northbr...
Whether you really need to reserve those depends of how S3 resume path will look like. From what I understand any pre-ramstage rmodule will not be relocatable,...
I was planning on the resume path looking very similar (i.e. if you treat AGESA almost like a black box) to older devices. Load and execute the stages in order... However, it hadn't occurred to me that romstage and FSP could also be cached away for performance.
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 205: amdfw.rom
Mark up with ``?
Done
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 205: image is added into the PSP's amdfw.rom build.
Remove trailing spaces.
Done
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 229: the existing v5 interface impractical.
I believe I know what you mean, but maybe the phrase needs some commas.
Done
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 239: in an Integration Guide.
Did you evaluate FSP rev 2. […]
IIRC the 2.1 spec came out just after we'd started down the 2.0 path. I don't remember precise details now, but I'd experimented with FSP_USES_CB_STACK and it seemed the tianocore IntelFsp2Pkg we were using wasn't yet 2.1-compliant. Internally we recognized the desire to us that but it wasn't a high enough priority to add the necessary code. As it was, we discovered other things missing which needed to be added to make a functioning FSP2.0 implementation.
https://review.coreboot.org/c/coreboot/+/38713/1/Documentation/soc/amd/famil... PS1, Line 250: 5. [https://www.intel.com/content/www/us/en/intelligent-systems/intel-firmware-s...)
please keep the newline at the end
Done