Nico Huber 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 9: Code-Review+1
Patch Set 9:
Patch Set 9: Code-Review-1
I must say I do not like to see pci_config_write32() getting repiaced with write32(). But looks like Aaron did not mind it?
It's a tradeoff on where to put complication. Within this one compilation unit or do the binding in the soc code itself. Yes, in this case we needed the current address of the config register to achieve that. In SoC for skl in this use-case we know mmcfg is employed and that code is the best place to make that decision.
Alternatively we put all the complexity in the compilation unit to handle the various cases where the register resides. It requires binding into #defines located in SoC #include files as well as all the conditionals in handling the alternative accesses.
Do you have alternatives?
First off, I think we should just go on here. We can't always have implementations that make everybody happy. I think the problem solved here is not worth to consider all alternatives exhaustively. And if we wanted to make a preceding case for soc/intel/common/ design, it seems a little late. I'd be happy to discuss common-code design some other time, on the ML, though :)
Alternatives? soc_pmc_etr_addr() is only called in two places. Hence we'd only need two if's without that api, see my last comments on CB:35451. As we only want to update ETR, we could also add an API doesn't use pointers, soc_pmc_update_etr(...). But we just can't tell which version works best in the future. Oh, the last top-of-my-head alternative: SKL looks compatible to earlier ICHs and PCHs i.e. `sb/intel/common/`.