Jason Glenesk 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 3:
(8 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 176: return pci_read_config32(_SOC_DEV(DF_DEV, function), reg);
it might be useful to add a comment that the macros will apply a bit mask to the values, so no bit m […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_fa... PS2, Line 181: reg
reg is the byte address and the two last bits get masked off, since the access is always dword/qword […]
Yes, this is always aligned so they omit the last bits
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.
Done
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". […]
Done
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... PS2, Line 28: #define D18F0_DRAM_HOLE_CTL 0x104 : #define DRAM_HOLE_CTL_VALID BIT(0) : #define DRAM_HOLE_CTL_BASE_SHFT 24 : #define DRAM_HOLE_CTL_BASE (0xFF << DRAM_HOLE_CTL_BASE_SHFT) : : #define D18F0_DRAM_BASE0 0x110 : #define DRAM_BASE_REG_VALID BIT(0) : #define DRAM_BASE_HOLE_EN BIT(1) : #define DRAM_BASE_INTLV_CH_SHFT 4 : #define DRAM_BASE_INTLV_CH (0xF << DRAM_BASE_INTLV_CH_SHFT) : #define DRAM_BASE_INTLV_SEL_SHFT 8 : #define DRAM_BASE_INTLV_SEL (0x7 << DRAM_BASE_INTLV_SEL_SHFT) : #define DRAM_BASE_ADDR_SHFT 12 : #define DRAM_BASE_ADDR (0xFFFFF << DRAM_BASE_ADDR_SHFT) : : #define D18F0_DRAM_LIMIT0 0x114 : #define DRAM_LIMIT_DST_ID_SHFT 0 : #define DRAM_LIMIT_DST_ID (0xFF << DRAM_LIMIT_DST_ID_SHFT) : #define DRAM_LIMIT_INTLV_NUM_SOCK_SHFT 8 : #define DRAM_LIMIT_INTLV_NUM_SOCK (0x1 << DRAM_LIMIT_INTLV_NUM_SOCK_SHFT) : #define DRAM_LIMIT_INTLV_NUM_DIE_SHFT 10 : #define DRAM_LIMIT_INTLV_NUM_DIE (0x3 << DRAM_LIMIT_INTLV_NUM_DIE_SHFT) : #define DRAM_LIMIT_ADDR_SHFT 12 : #define DRAM_LIMIT_ADDR (0xFFFFF << DRAM_LIMIT_ADDR_SHFT) : : #define DF_DRAM_BASE(dram_map_pair) ((dram_map_pair) * 2 * sizeof(uint32_t) \ : + D18F0_DRAM_BASE0) : #define DF_DRAM_LIMIT(dram_map_pair) ((dram_map_pair) * 2 * sizeof(uint32_t) \ : + D18F0_DRAM_LIMIT0)
i'd add those in the next commit, since they aren't needed or used for what this patch adds
Done
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... PS2, Line 62: BIT(0)
i'd prefer (1 << 0) over BIT(0), but both will work. your call.
Done
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/include... PS2, Line 70: DF_IND_CFG_INST_ID
is this a mask? if so, i'dd ad _MASK as suffix
Done
-
Jason Glenesk (Code Review)