EricR Lai 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 16:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37685/15//COMMIT_MSG@13 PS15, Line 13: measure power consumption under s0ix
Can you please confirm the following things: […]
I will add print in the SGPM and RGPM. Feedback to you later. I will find _PS0 method to print as well.
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 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?
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/14/src/soc/intel/cannonlake/a... PS14, Line 192: /* Save current setting and using when resume */
correct the word later. replace using to restore.
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 188: Enable GPIO Power Management bits and save the current setting
nit: Order is not correct ;). […]
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 192: *
nit: space before *
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... File src/soc/intel/cannonlake/acpi/lpit.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/cannonlake/a... PS15, Line 79: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits
nit: Order is reversed.
Done
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)
Oh, I thought Intel put configure gpio PM code in common, but only in CNL. […]
Done
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37685/15/src/soc/intel/common/acpi/... PS15, Line 46: Enable GPIO PM with MISCCFG_ENABLE_GPIO_PM_CONFIG and : * save the current PM bits
nit: order is reversed.
Done