Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35174 )
Change subject: mb/google/poppy/variant/nocturne: fix EC_SYNC_IRQ definition ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/Kconfig:
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... PS2, Line 197: select SOC_INTEL_COMMON_BLOCK_GPIO_ITSS_POL_CFG
If this is required this should be done at the SoC level. […]
Done
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... PS2, Line 36: EC_SYNC_IRQ
Do you intend to pass the IRQ#, GPIO# or GPE# here?
GPIO#
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/variants/nocturne/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/35174/2/src/mainboard/google/poppy/... PS2, Line 38: GPP_D17_IRQ
Changing this would break the OS level interrupt resource: https://review.coreboot. […]
EC_ENABLE_SYNC_IRQ is not set for nocturne. Should it be? I couldn't find where in the kernel the information exported in the _CRS gets accessed to determine exactly what it is used for. There is also a newer EC_ENABLE_SYNC_GPIO in the top-of-tree branch (but not nocturne branch), but aside from it exporting the EC_SYNC_IRQ using a different header type (EC_ENABLE_SYNC_GPIO defines a "GpioInt" whereas EC_ENABLE_SYNC_IRQ defines an "Interrupt"), I couldn't tell how exactly it was being used by the kernel.