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:
(1 comment)
File src/soc/intel/skylake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/61650/comment/3756084a_e87bd572 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.
Well, when the GEN_PMCON_A offset differs, it can be defined in the SoC headers, but the code can still be common code. Same applies to the bit MS4V, which is the only bit offset needed to be known, right?
Denverton uses PMC IA common code but GEN_PMCON_A is not there is soc/pm.h or soc/pmc.h. I ran into issue due to not having macro resolved by all SOC layer. I think there is one more SOC compliant about the same. Moving things common without any guard might need to have all SoC implements the required macro isn't it?
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.
Got it, thanks! Add a short comment for this, please.
There is no harm if you would like to call this more than once.
Please don't 😄
Although EDS says, writing 0 in this bit doesn't take any effect as it's RW/1C. But my comment to ensure we should remove the redundant to keep only one copy.