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 13:
(10 comments)
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/13//COMMIT_MSG@9 PS13, Line 9: Enable/disable GPIO clock gating when enter/exit s0ix is common : request on CNL/CML. Move it from board level to soc level. This description does not really capture the changes that are being done in this CL.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 171: Store (GPID (Local0), Local1) : Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0)) Can this be simply written as:
Local1 = GPID (Local0) GPMB[Local0] = PCRR (Local1, GPIO_MISCCFG)
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 172: Store (PCRR (Local1, GPIO_MISCCFG), Index(GPMB, Local0)) Aren't you saving the entire GPIO_MISCCFG register here and not just the GPIO PM config bits?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 183: Index(GPMB, Local0) Can this be simply written as "GPMB[Local0]" in ASL2.0 syntax?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 192: /* Store current setting */ : SGPM () Why is store called within a method that is supposed to "Enable GPIO Power Management bits"?
Comment for the method should be updated to clearly reflect what the method is doing i.e. it is not just enabling GPIO PM bits.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: Enable GPIO PM This is not just enable. It also saves the current state of GPIO PM bits.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 79: * space before *
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 80: OGPM I don't understand why we need to have NVS now? Can't we just always follow the policy of saving and enabling GPIO PM bits when entering S0ix and restoring when exiting S0ix?
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/cannonlake/a... PS13, Line 100: OGPM Same question as above.
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/13/src/soc/intel/common/acpi/... PS13, Line 46: * space before *