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)
Do we want let only "coreboot" configure the PM bit without FSP-S, right? If so, gpio_override_pm flag is meaningful. Otherwise, it not so significant like you said.
Actually, gpio_override_pm config means that the mainboard wants to override the GPIO PM config bits. If that is not set, then SoC would by default set the PM bits to MISCCFG_ENABLE_GPIO_PM_CONFIG. Thus, in both cases, we don't care about what FSP is doing. Coreboot itself is doing the GPIO PM bit configuration in SoC code. Either the PM bits are set to MISCCFG_ENABLE_GPIO_PM_CONFIG if mainboard does not want an override. Else, if mainboard wants an override, it can set gpio_override_pm and provide the values of PM bits.
Here is my point, if we let coreboot configure the PM bits, means we will use it in devicetree.cb in all boards come after.
Not necessary. Coreboot would always configure GPIO PM bits. Whether it would be MISCCFG_ENABLE_GPIO_PM_CONFIG or something that mainboard provides depends on the value of gpio_override_pm.
So we don't have to add condition in coreboot but with a choice in the OS level for s0ix/Sx.
Where in the OS? Sorry, I don't think I understand this statement.