Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37685 )
Change subject: soc/intel/cannonlake: Move GPIO PM configuration to soc level ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 79: /* Enable GPIO PM */ : _SB.PCI0.LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
If we think setting MISCCFG_ENABLE_GPIO_PM_CONFIG on the way into S0ix on all systems is OK then could we save the existing value in _PTS and then restore in _WAK instead of needing something in NVS? It isn't quite as flexible but NVS is a bit of a pain..
I actually like this. My initial proposal was to use a single flag in NVS. But, as you said if this policy looks okay:
--> When in S0 use the values provided in devicetree (Configured by coreboot after FSP-S runs) --> When entering any low power state (S0ix, S3, S5, ...), _MPTS and LPIT S0ix entry methods can save the current state of MISCCFG PM bits. --> On resume from low power state, _MWAK and LPIT S0ix exit methods can restore the saved state into MISCCFG PM bits.
then it can actually reduce a lot of NVS complexity.
Why this works well is because: --> MISCCFG PM bits would be retained after any kind of low power mode entry/exit. --> It also gives the flexibility to allow mainboards to have any MISCCFG PM bit settings. --> Additionally, if any entity changes MISCCFG PM bits after coreboot, those would still be honored. --> No need to worry about different TOTAL_GPIO_COMM in nvs.