Aaron Durbin 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 438: reg = (reg & ~CF9_GLB_RST) | CF9_LOCK; This is still changing the semantics of the previous implementation.
apl was doing the following:
static void soc_final(void *data) {
-------/* Disable global reset, just in case */ -------pmc_global_reset_enable(0); -------/* Make sure payload/OS can't trigger global reset */ -------pmc_global_reset_lock();
}
Now, if we want to combine that into a single function we certainly can. However, I think you should name it accordingly. i.e. you are also clearing global reset request flag as well.
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); : }
with CB:36596 the mmio part would translate to this: […]
How was one going to convey the difference of the location of the register? Add another Kconfig? It still needs:
1. if mmconf need the device and then implicitly use the name soc/pci_devs.h 2. Need definition of ETR offset which chagnes by chipset which is being provided through soc/pm.h.
You can add a Kconfig and then provide a helper function in here to provide the address or ETR. It'd still be a "callback" in the sense one shouldn't be sprinkling the logic of determining those preconditions all over the code.