Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47726 )
Change subject: soc/amd/picasso: Add data fabric read helper function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_fa... File src/soc/amd/picasso/data_fabric.c:
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_fa... PS2, Line 180: * requested offset.*/ Remove leading asterisk and add space in front of */ to match the short multi-line style. https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/data_fabric.h:
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... PS2, Line 10: 0xFF Use lower case for hex numbers here, line 31, 37, etc.
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... PS2, Line 28: #define D18F0_DRAM_HOLE_CTL 0x104 Unless I'm mistaken, lines 28-56 are unrelated to the "add data fabric helper". Can you split these definitions into their own patch?
-
Marshall Dawson (Code Review)