Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34561 )
Change subject: Documentation/soc/amd: Add Family 15h ......................................................................
Patch Set 12:
(13 comments)
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... File Documentation/soc/amd/family15h.md:
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 5: AMD products How about AMD x86 products?
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 6: of "C of a "Compute...
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 7: (CU) where some parts of the processor are shared between two cores and : some parts are unique for each core. How about "...(CU); a core pair grouping with certain shared resources.
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 10: (see references below). The placement of this after the "Enterprise, Client, and Embedded designs" kind of bugs me. It seems to imply to the reader that each of the references individually must be Enterprise, or Client, etc. This is incorrect, though.
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 14: Initially with CU, 2 x86 cores have separate integer processors but share cache, The first CU designs were 2 x86 cores with...
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 15: A die can have up to : 8 CU ...can contain up to CUs.
I don't specifically recall this limit. You may be correct, but do you have it handy where you found this information?
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 22: Family 15h setups use up to three separate chips How about "Family 15h products range from SOCs to 3-chip solutions."?
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 23: (i.e. even when they're headless) (including headless)
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 26: which deleting this will make it read better.
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 26: Some products Later SOCs
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 27: security related measures I have mixed feelings discussing this here, as no coreboot implementation supports it. BTW, mentioning fTPM seems just as important but also not supported. These features are only in UEFI with no plans to make them available in coreboot.
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 28: SPI SPI flash?
https://review.coreboot.org/c/coreboot/+/34561/12/Documentation/soc/amd/fami... PS12, Line 37: implements You use a form of this word 3 times in 2 sentences. Maybe this one changes to "contains"? Below you could say "Newer products... rely on a more modern..."