Felix Held 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:
(7 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 17: pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_CONTROL(reg), : IOMS0_FABRIC_ID << MMIO_DST_FABRIC_ID_SHIFT); : pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_BASE(reg), 0); : pci_write_config32(SOC_DF_F0_DEV, NB_MMIO_LIMIT(reg), 0); out of scope for this patch, but having a data_fabric_write_reg32() might make this code and the code below easier to read
https://review.coreboot.org/c/coreboot/+/47726/2/src/soc/amd/picasso/data_fa... PS2, Line 25: pci_read_config32(SOC_DF_F0_DEV, NB_MMIO_CONTROL(reg)); this could be rewritten to use data_fabric_read_reg32(). I'm ok with doing that in a separate patch to not block this and the ACPI CRAT one
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 masking is required here
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 based, right?
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 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
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.
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