Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36570 )
Change subject: soc/intel/common: pmclib: make use of the new ETR address API ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block/... PS4, Line 434: soc_read_pmc_etr_addr(); Check for NULL and unaligned?
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block/... PS4, Line 432: void pmc_global_reset_lock(void) : { : uint32_t *etr = soc_read_pmc_etr_addr(); : uint32_t reg; : : reg = read32(etr); : reg = (reg & ~CF9_GLB_RST) | CF9_LOCK; : write32(etr, reg); : }
besides that, why duplicate code again and again in soc/ when we can have a common function?
If you go through the complexity of implementing a callback to fetch an offset and do sanity check on it, just to clear and set a few bits at that offset it might as well be done per soc. That likely ends up with less code.
if it's PCI config register it is one line: pci_update_config32(..., ETR, ~CF9_GLB_RST, CF9_LOCK);
if it's a MMIO reg it's 2 lines: uint8_t *base = get_base_... write32(base + ETR, (read32(base + ETR) & ~CF9_GLB_RST) | CF9_LOCK);