Attention is currently required from: Shelley Chen, Anjaneya "Reddy" Chagam, Furquan Shaikh, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Morgan Jang, Kyösti Mälkki, Patrick Rudolph, Tim Chu. Hello Shelley Chen, Furquan Shaikh, Kyösti Mälkki,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/58333
to review the following change.
Change subject: [RFC] pci_mmio_cfg: Always use pci_s_* functions ......................................................................
[RFC] pci_mmio_cfg: Always use pci_s_* functions
When MMIO functions are available, the pci_s_* functions do exactly the same. Drop the redundant pci_mmio_* versions.
Change-Id: I1043cbb9a1823ef94bcbb42169cb7edf282f560b Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/include/device/pci_mmio_cfg.h M src/mainboard/intel/cedarisland_crb/bootblock.c M src/mainboard/ocp/tiogapass/bootblock.c M src/soc/intel/xeon_sp/bootblock.c M src/soc/intel/xeon_sp/nb_acpi.c M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h M src/soc/intel/xeon_sp/skx/soc_util.c 7 files changed, 30 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/58333/1
diff --git a/src/include/device/pci_mmio_cfg.h b/src/include/device/pci_mmio_cfg.h index 557adcb..81faa9c 100644 --- a/src/include/device/pci_mmio_cfg.h +++ b/src/include/device/pci_mmio_cfg.h @@ -47,38 +47,44 @@
#endif
+/* + * Avoid name collisions as different stages have different signature + * for these functions. The _s_ stands for simple, fundamental IO or + * MMIO variant. + */ + static __always_inline -uint8_t pci_mmio_read_config8(pci_devfn_t dev, uint16_t reg) +uint8_t pci_s_read_config8(pci_devfn_t dev, uint16_t reg) { return pcicfg(dev)->reg8[reg]; }
static __always_inline -uint16_t pci_mmio_read_config16(pci_devfn_t dev, uint16_t reg) +uint16_t pci_s_read_config16(pci_devfn_t dev, uint16_t reg) { return pcicfg(dev)->reg16[reg / sizeof(uint16_t)]; }
static __always_inline -uint32_t pci_mmio_read_config32(pci_devfn_t dev, uint16_t reg) +uint32_t pci_s_read_config32(pci_devfn_t dev, uint16_t reg) { return pcicfg(dev)->reg32[reg / sizeof(uint32_t)]; }
static __always_inline -void pci_mmio_write_config8(pci_devfn_t dev, uint16_t reg, uint8_t value) +void pci_s_write_config8(pci_devfn_t dev, uint16_t reg, uint8_t value) { pcicfg(dev)->reg8[reg] = value; }
static __always_inline -void pci_mmio_write_config16(pci_devfn_t dev, uint16_t reg, uint16_t value) +void pci_s_write_config16(pci_devfn_t dev, uint16_t reg, uint16_t value) { pcicfg(dev)->reg16[reg / sizeof(uint16_t)] = value; }
static __always_inline -void pci_mmio_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) +void pci_s_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) { pcicfg(dev)->reg32[reg / sizeof(uint32_t)] = value; } @@ -107,45 +113,4 @@ return (uint32_t *)&pcicfg(dev)->reg32[reg / sizeof(uint32_t)]; }
-/* Avoid name collisions as different stages have different signature - * for these functions. The _s_ stands for simple, fundamental IO or - * MMIO variant. - */ - -static __always_inline -uint8_t pci_s_read_config8(pci_devfn_t dev, uint16_t reg) -{ - return pci_mmio_read_config8(dev, reg); -} - -static __always_inline -uint16_t pci_s_read_config16(pci_devfn_t dev, uint16_t reg) -{ - return pci_mmio_read_config16(dev, reg); -} - -static __always_inline -uint32_t pci_s_read_config32(pci_devfn_t dev, uint16_t reg) -{ - return pci_mmio_read_config32(dev, reg); -} - -static __always_inline -void pci_s_write_config8(pci_devfn_t dev, uint16_t reg, uint8_t value) -{ - pci_mmio_write_config8(dev, reg, value); -} - -static __always_inline -void pci_s_write_config16(pci_devfn_t dev, uint16_t reg, uint16_t value) -{ - pci_mmio_write_config16(dev, reg, value); -} - -static __always_inline -void pci_s_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) -{ - pci_mmio_write_config32(dev, reg, value); -} - #endif /* _PCI_MMIO_CFG_H */ diff --git a/src/mainboard/intel/cedarisland_crb/bootblock.c b/src/mainboard/intel/cedarisland_crb/bootblock.c index e51752b..ae8212a 100644 --- a/src/mainboard/intel/cedarisland_crb/bootblock.c +++ b/src/mainboard/intel/cedarisland_crb/bootblock.c @@ -21,7 +21,7 @@ pcr_write32(PID_DMI, 0x2774, 1);
/* Decode for SuperIO (0x2e) and COM1 (0x3f8) */ - pci_mmio_write_config32(PCH_DEV_LPC, 0x80, (1 << 28) | (1 << 16)); + pci_s_write_config32(PCH_DEV_LPC, 0x80, (1 << 28) | (1 << 16));
const pnp_devfn_t serial_dev = PNP_DEV(0x2e, AST2400_SUART1); aspeed_enable_serial(serial_dev, CONFIG_TTYS0_BASE); diff --git a/src/mainboard/ocp/tiogapass/bootblock.c b/src/mainboard/ocp/tiogapass/bootblock.c index b5c2aa1..6bcbfcc 100644 --- a/src/mainboard/ocp/tiogapass/bootblock.c +++ b/src/mainboard/ocp/tiogapass/bootblock.c @@ -30,7 +30,7 @@ pcr_or32(PID_DMI, PCR_DMI_LPCIOE, (1 << 0) | (1 << 1));
/* Enable com1 (0x3f8), com2 (02f8) and superio (0x2e) */ - pci_mmio_write_config32(PCH_DEV_LPC, 0x80, + pci_s_write_config32(PCH_DEV_LPC, 0x80, (1 << 28) | (1 << 16) | (1 << 17) | (0 << 0) | (1 << 4)); }
diff --git a/src/soc/intel/xeon_sp/bootblock.c b/src/soc/intel/xeon_sp/bootblock.c index 1d706fe..5ea09ac 100644 --- a/src/soc/intel/xeon_sp/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock.c @@ -56,9 +56,9 @@ pch_enable_lpc();
/* Set up P2SB BAR. This is needed for PCR to work */ - uint8_t p2sb_cmd = pci_mmio_read_config8(PCH_DEV_P2SB, PCI_COMMAND); - pci_mmio_write_config8(PCH_DEV_P2SB, PCI_COMMAND, p2sb_cmd | PCI_COMMAND_MEMORY); - pci_mmio_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, CONFIG_PCR_BASE_ADDRESS); + uint8_t p2sb_cmd = pci_s_read_config8(PCH_DEV_P2SB, PCI_COMMAND); + pci_s_write_config8(PCH_DEV_P2SB, PCI_COMMAND, p2sb_cmd | PCI_COMMAND_MEMORY); + pci_s_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, CONFIG_PCR_BASE_ADDRESS); }
void bootblock_soc_init(void) diff --git a/src/soc/intel/xeon_sp/nb_acpi.c b/src/soc/intel/xeon_sp/nb_acpi.c index 0c1c5ab..9443966 100644 --- a/src/soc/intel/xeon_sp/nb_acpi.c +++ b/src/soc/intel/xeon_sp/nb_acpi.c @@ -160,7 +160,7 @@ const uint32_t dev = iio_resource->PcieInfo.PortInfo[port].Device; const uint32_t func = iio_resource->PcieInfo.PortInfo[port].Function;
- const uint32_t id = pci_mmio_read_config32(PCI_DEV(bus, dev, func), + const uint32_t id = pci_s_read_config32(PCI_DEV(bus, dev, func), PCI_VENDOR_ID); if (id == 0xffffffff) return 0; diff --git a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h index 01e86e1..4b46ec8 100644 --- a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h +++ b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h @@ -13,13 +13,13 @@ printk(BIOS_SPEW, "%s%x:%x:%x reg: %s (0x%x), data: 0x%x\n", \ fmt, ((uint32_t)dev >> 20) & 0xfff, ((uint32_t)dev >> 15) & 0x1f, \ ((uint32_t)dev >> 12) & 0x07, \ - #reg, reg, pci_mmio_read_config32(dev, reg)) + #reg, reg, pci_s_read_config32(dev, reg))
#define dump_csr64(fmt, dev, reg) \ printk(BIOS_SPEW, "%s%x:%x:%x reg: %s (0x%x), data: 0x%x%x\n", \ fmt, ((uint32_t)dev >> 20) & 0xfff, ((uint32_t)dev >> 15) & 0x1f, \ ((uint32_t)dev >> 12) & 0x07, #reg, reg, \ - pci_mmio_read_config32(dev, reg+4), pci_mmio_read_config32(dev, reg)) + pci_s_read_config32(dev, reg+4), pci_s_read_config32(dev, reg))
#define SAD_ALL_DEV 29 #define SAD_ALL_FUNC 0 diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c index b903249..cc8db64 100644 --- a/src/soc/intel/xeon_sp/skx/soc_util.c +++ b/src/soc/intel/xeon_sp/skx/soc_util.c @@ -79,11 +79,11 @@
/* configure PCU_CR0_FUN csrs */ pci_devfn_t cr0_dev = PCI_DEV(bus, PCU_DEV, PCU_CR0_FUN); - data = pci_mmio_read_config32(cr0_dev, PCU_CR0_P_STATE_LIMITS); + data = pci_s_read_config32(cr0_dev, PCU_CR0_P_STATE_LIMITS); data |= P_STATE_LIMITS_LOCK; - pci_mmio_write_config32(cr0_dev, PCU_CR0_P_STATE_LIMITS, data); + pci_s_write_config32(cr0_dev, PCU_CR0_P_STATE_LIMITS, data);
- plat_info = pci_mmio_read_config32(cr0_dev, PCU_CR0_PLATFORM_INFO); + plat_info = pci_s_read_config32(cr0_dev, PCU_CR0_PLATFORM_INFO); dump_csr64("", cr0_dev, PCU_CR0_PLATFORM_INFO); max_min_turbo_limit_ratio = (plat_info & MAX_NON_TURBO_LIM_RATIO_MASK) >> @@ -94,29 +94,29 @@ /* configure PCU_CR1_FUN csrs */ pci_devfn_t cr1_dev = PCI_DEV(bus, PCU_DEV, PCU_CR1_FUN);
- data = pci_mmio_read_config32(cr1_dev, PCU_CR1_SAPMCTL); + data = pci_s_read_config32(cr1_dev, PCU_CR1_SAPMCTL); /* clear bits 27:31 - FSP sets this with 0x7 which needs to be cleared */ data &= 0x0fffffff; data |= SAPMCTL_LOCK_MASK; - pci_mmio_write_config32(cr1_dev, PCU_CR1_SAPMCTL, data); + pci_s_write_config32(cr1_dev, PCU_CR1_SAPMCTL, data);
/* configure PCU_CR1_FUN csrs */ pci_devfn_t cr2_dev = PCI_DEV(bus, PCU_DEV, PCU_CR2_FUN);
data = PCIE_IN_PKGCSTATE_L1_MASK; - pci_mmio_write_config32(cr2_dev, PCU_CR2_PKG_CST_ENTRY_CRITERIA_MASK, data); + pci_s_write_config32(cr2_dev, PCU_CR2_PKG_CST_ENTRY_CRITERIA_MASK, data);
data = KTI_IN_PKGCSTATE_L1_MASK; - pci_mmio_write_config32(cr2_dev, PCU_CR2_PKG_CST_ENTRY_CRITERIA_MASK2, data); + pci_s_write_config32(cr2_dev, PCU_CR2_PKG_CST_ENTRY_CRITERIA_MASK2, data);
data = PROCHOT_RATIO; printk(BIOS_SPEW, "PCU_CR2_PROCHOT_RESPONSE_RATIO_REG data: 0x%x\n", data); - pci_mmio_write_config32(cr2_dev, PCU_CR2_PROCHOT_RESPONSE_RATIO_REG, data); + pci_s_write_config32(cr2_dev, PCU_CR2_PROCHOT_RESPONSE_RATIO_REG, data); dump_csr("", cr2_dev, PCU_CR2_PROCHOT_RESPONSE_RATIO_REG);
- data = pci_mmio_read_config32(cr2_dev, PCU_CR2_DYNAMIC_PERF_POWER_CTL); + data = pci_s_read_config32(cr2_dev, PCU_CR2_DYNAMIC_PERF_POWER_CTL); data |= UNOCRE_PLIMIT_OVERRIDE_SHIFT; - pci_mmio_write_config32(cr2_dev, PCU_CR2_DYNAMIC_PERF_POWER_CTL, data); + pci_s_write_config32(cr2_dev, PCU_CR2_DYNAMIC_PERF_POWER_CTL, data); } }