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 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/6//COMMIT_MSG@11 PS6, Line 11: BUG=?
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 162: Configure GPIO all Community Power Management nit: Configure Power Management bits for all GPIO communities?
https://review.coreboot.org/c/coreboot/+/37685/6/src/soc/intel/cannonlake/ac... PS6, Line 166: GCPM nit: CGPM and GCPM look kind of very similar. Maybe CAPM? [(C)onfigure (A)ll GPIO community (P)ower (M)anagement].
You can also get rid of the two functions and instead move all functionality in CGPM into GCPM.
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)
Ack
I agree that the LOCL() (or GCPM() as you renamed it now) method can be moved to SoC. But until now the GPIO PM bits were set based on mainboard selection. For all Google boards, in general, I would say that we would want to follow the same policy i.e. setting the PM bits to 0 any time we are in S0, but in S0ix they should be set to MISCCFG_ENABLE_GPIO_PM_CONFIG. So, this change works okay. But, I am worried if this really works the same way for other boards. It might not be possible to make that decision right away until we start seeing more boards upstream.
But, I think we should at least be consistent in the way this is being implemented: 1. Mainboard is allowed to set gpio_override_pm and gpio_pm[] bits for each community. Examples: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
2. With the above change, coreboot will configure GPIO MISCCFG as per the gpio_pm[] bits.
3. Once the device enters S0ix, this change will set it to MISCCFG_ENABLE_GPIO_PM_CONFIG which seems okay.
4. But, when it exits S0ix, this change will set it to 0 which might not really be the same as gpio_pm[] bits that were provided in mainboard devicetree.
In general, I think we should provide consistent behavior since the above change can lead to hard to track problems. One thing that we can potentially do (if we really want to go down the route of setting PM bits to 0 or MISCCFG_ENABLE_GPIO_PM_CONFIG only):
--> Get rid of gpio_override_pm and gpio_pm[] from devicetree and chip.h --> Add a new config gpio_pm_s0_disable which basically ensures that GPIO PM configuration is disabled in S0 and enabled only in lower power states. --> SoC code can honor gpio_pm_override_s0 to determine whether it should set MISCCFG registers to 0 for S0 or to leave untouched. --> Additionally, add a new config to GNVS which indicates whether runtime configuration of GPIO PM is required based on the value of gpio_pm_override_s0. --> In this method, now you can check that GNVS variable to decide if the MISCCFG registers should be touched or left as is.
This forces all mainboards to: --> Either stay with default settings of GPIO_MISCCFG or --> Disable all PM bits in S0 and enable all PM bits in lower power states.
Since we do not have enough data on what other mainboards would want, we can start with this since it at least keeps the behavior consistent. In the future, if there are more mainboards with varied requirements, we can always update the behavior. Thoughts?