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 5:
(7 comments)
This change should be split into three: a) SoC change -- To add support for LOCL or a better named method b) Hatch -- Update to using this LOCL method from SoC c) Drallion - Add support to using LOCL method from SoC.
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/mainboard/google/dralli... PS5, Line 56: MISCCFG_ENABLE_GPIO_PM_CONFIG This is not disabling GPIO PM config. Instead it is being enabled.
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 160: Can you please add a comment to indicate what this method does? and what the arg is?
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 161: LOCL I am trying to remember what LOCL stands for...
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 163: 5 TOTAL_GPIO_COMM
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 165: _SB.PCI0.CGPM Just "CGPM" should work?
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) Why is this being called here? This call is already made from mainboard and that should be sufficient?
https://review.coreboot.org/c/coreboot/+/37685/5/src/soc/intel/cannonlake/ac... PS5, Line 96: /* Disable GPIO PM */ : _SB.PCI0.LOCL (0) Same comment as above.