Michael Niewöhner 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:
(4 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 432: void pmc_global_reset_lock(void)
Header file should be changed to reflect its actual implementation. Comment current indicates: […]
Done
https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block/... PS4, Line 434: soc_read_pmc_etr_addr();
I guess when this was null, we had a bigger problem and wouldn't even reach that code
Done
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. […]
Yup, this is to save one additional read-modify-write cycle; will rename it
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); : }
How was one going to convey the difference of the location of the register? Add another Kconfig? It […]
Well, I just wanted to continue your idea of moving this back to soc/ (which I definitely don't prefer, though). Then no Kconfig would be needed and ETR is already in soc/pm.h