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 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/11/src/soc/intel/common/acpi/... PS11, Line 22: External(_SB.PCI0.EGPM, MethodObj)
As I understand it, the config in devicetree currently is meant to indicate to common SoC code tha […]
In general, I think it should be okay to just have a policy that all SoCs which support GPIO PM bits will save/restore those bits across S0ix entry/exit. Saves us from having another config and NVS variable when all boards would just want to set it (keeps the implementation simple).
Thinking about it some more -- There are currently two possibilities: a) Mainboard doesn't care about overriding GPIO PM bits. In this case, gpio_override_pm would be set to 0. This will set GPIO PM bits to MISCCFG_ENABLE_GPIO_PM_CONFIG. In this case, doing the save/restore would be simply a NOP.
b) Mainboard sets gpio_override_pm. In this case, GPIO PM bits would be set to something other than MISCCFG_ENABLE_GPIO_PM_CONFIG. As per current implementation, gpio_override_pm is used as an indication from mainboard that PM bits should be saved/restored.
Thus, I don't see a lot of advantage having the added complexity of a new NVS variable and using that to decide whether we should save/restore GPIO PM bits. Instead, doing it unconditionally would be just okay. In the future, if we really come across a board that needs to avoid save/restore of GPIO PM bits, we would anyways have to add a new chip config for that.