Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34179 )
Change subject: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL. ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... PS5, Line 122: ByteAcc DWordAcc?
https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... PS5, Line 129: LAnd I guess you want to use And and not LAnd? LAnd is Logical And whereas And is Bitwise AND. Same for Land and Lor in the statements below.
https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... PS5, Line 127: Store (REG, Local0) : /* Ensure only PM bits can be set */ : Store (LAnd (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG), Local1) : /* Clear PM bits */ : Store (LAnd (LNot (MISCCFG_ENABLE_GPIO_PM_CONFIG), Local0), Local0) : Store (LOr (Local1, Local0), Local1) : Store (Local1, REG) : } Just curious. Any reason not to use PCRA and PCRO methods provided by pcr.asl? Then you won't need to define OpRegion, Field here or perform and/or operations. Less code:
Local0 <-- PID_GPIOCOMx
PCRA (Local0, GPIO_MISCCFG, Not (MISCCFG_ENABLE_GPIO_PM_CONFIG)) PCRO (Local0, GPIO_MISCCFG, And (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG))
https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... PS5, Line 169: 16 GPIO_MISCCFG