Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34561 )
Change subject: Documentation/soc/amd: Add family 15h ......................................................................
Patch Set 4:
(17 comments)
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... File Documentation/soc/amd/family15h.md:
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 1: Family 15h in
Family 15h [SOCs|Processors]
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 5: Family 15h
Same as above.
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 5: _Excavator_
This is a misleading statement. […]
Ack
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 6: number
numbers
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 8: models **60h-6Fh** (_Merlin Falcon_) and : **70h-7Fh** (_Stoney Ridge_).
Why bold? […]
Ack
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 8: models **60h-6Fh** (_Merlin Falcon_) and : **70h-7Fh** (_Stoney Ridge_).
We also support 15h trinity - the agesa code is in vendorcode/amd/ […]
Ack
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 13: This documentation assumes : familiarity with x86, its reset state and its early initialization : requirements.
I'm not sure the Family 17h warning about x86 reset adds anything to Family 15h. […]
Removed
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 17: AMD®
Don't use (R) after the first instance in any document. For AMD, don't use it at all.
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 23: (_AGESA ™_). _AGESA_ c
Why are these italicized? […]
I wanted to call attention to AGESA, that's why it's italicized.
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 26: : Some functionalit
In the binary PI release used by coreboot, some functionality...
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 27: GPIO setting and
the setting of MTRRS and GPIO configurations along with setting
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 28: (though still available on most _AGESA_
Get rid of this, because who cares?
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 28: _AGESA_
wrap at 72 characters?
Mostly done, though there are 3 or 4 instances of 73 characters.
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 30: reasons
remove "reasons"
Done
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 31: >
Isn't there a better way to format this?
Spelling or the use of a box comment style (>)?
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 38: may be viewed as analogous to the : Intel® Management Engine
Maybe just discuss what it does instead of comparing it to the ME.
I wanted to call attention to the fact that it's a separate processor within the SOC just like Intel's ME. As for discussing its functionality, I would need to read documentation before doing it, so it'll not be in the next patch. Future patch yes, I can do.
https://review.coreboot.org/c/coreboot/+/34561/4/Documentation/soc/amd/famil... PS4, Line 44: https://www.amd.com/system/files/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf
Are you worried about the URL changing? While potentially a risk, it looks like they've named it wi […]
Ack... I'll side with Marshall for the time being.