Attention is currently required from: Matt DeVillier, Tim Wawrzynczak, Sridhar Siricilla, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61650 )
Change subject: soc/intel/skylake: Add function to clear PMCON status bits ......................................................................
Patch Set 3:
(2 comments)
File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/94499377_4b25d259 PS3, Line 99: pci_or_config32(dev, GEN_PMCON_A, 0);
well...
This code is not honouring the bit 18 which may not be still relevant for Chrome platform as S4 is not POR but ideally it should.
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/a4eb9c76_9109aef2 PS3, Line 269: void pmc_clear_pmcon_sts(void)
this function is all the same for all platforms using common code, so why exactly do we have to copy-pasta it once again instead of finally moving it to soc/intel/common?
There are few SOCs which eventually uses the IA PMC common code but the SOC EDS does have exact register details. I've tried creating the common code which eventually need to resolve the register definitions from SoC layer as GEN_PMCON_A offset is not same across all SoC. But due to documentation limitation issue, I just avoided pursuing that path.
btw. this probably should be done in soc_pmc_init instead of finalize.
The reason this is being done as part of finalize is to ensure that prior booting to OS, those bits are cleared.
Note: FSP can even request a global reset during FSP notify (which is post soc_pmc_init) hence, after global reset, this corresponding bit would have set. And, we might need to clear it prior to boot. There is no harm if you would like to call this more than once.