Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36569 )
Change subject: soc/intel/skylake: add soc implementation for ETR address API ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmuti... File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmuti... PS4, Line 178: pcicfg(PCH_DEVFN_PMC << 12)->reg32[ETR / sizeof(uint32_t)]; : }
this was requested by Aaron
pci ops don't provide an address. If one wanted to not expose some of the PCI hairiness in our code base then one would need to provide read()/write() ETR API.
We can clean this up some by providing equivalent helper functions in pci_mmio_cfg.h and utilizing pci_ops.h. However, I will note we know for a fact that skl has mmconfig and utilizes it so it's not like we need to handle io vs mmio pci config access.
In this function we'd have to do:
return pci_mmio_config32_addr(pcidev_bdf(PCH_DEV_PMC), ETR);
That would inherently do a pci device lookup in the pci device tree and then encode the pci device path into what is needed to match with pci_devfn_t. And then we'd get take the address of the register.
Now, there's plans to remove the duplication of encoding. I have some patches I'm working on as well as Nico and Kyosti. This will likely clean itself up sooner than later.
diff --git a/src/include/device/pci_mmio_cfg.h b/src/include/device/pci_mmio_cfg.h index 5567ed86ae..6e6668e11c 100644 --- a/src/include/device/pci_mmio_cfg.h +++ b/src/include/device/pci_mmio_cfg.h @@ -86,6 +86,24 @@ void pci_mmio_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) pcicfg(dev)->reg32[reg / sizeof(uint32_t)] = value; }
+static __always_inline +uint8_t *pci_mmio_config8_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg8[reg]; +} + +static __always_inline +uint16_t *pci_mmio_config16_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg16[reg / sizeof(uint16_t)]; +} + +static __always_inline +uint32_t *pci_mmio_config32_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg32[reg / sizeof(uint32_t)]; +} + #endif /* !defined(__ROMCC__) */
#if CONFIG(MMCONF_SUPPORT)