Edward Hill has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31278
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ
Use AGPIO 10 as the EC sync interrupt for MKBP events for sensor data.
On this platform, interrupts are routed via the GPIO controller so need to be registered using GpioInt instead of Interrupt.
BUG=b:123750725 BRANCH=grunt TEST=MKBP events still received (with matching EC and kernel changes)
Change-Id: If499d24511bbaa7054207b7e0b98445723332c4f Signed-off-by: Edward Hill ecgh@chromium.org --- M src/ec/google/chromeec/acpi/cros_ec.asl M src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h M src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h M src/mainboard/google/kahlee/variants/baseboard/gpio.c 4 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/31278/1
diff --git a/src/ec/google/chromeec/acpi/cros_ec.asl b/src/ec/google/chromeec/acpi/cros_ec.asl index a5f9202..d41071e 100644 --- a/src/ec/google/chromeec/acpi/cros_ec.asl +++ b/src/ec/google/chromeec/acpi/cros_ec.asl @@ -32,6 +32,17 @@ }) #endif
+#ifdef EC_ENABLE_SYNC_IRQ_GPIO + Name (_CRS, ResourceTemplate () + { + GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, + "\_SB.GPIO", 0x00, ResourceConsumer, ,) + { + EC_SYNC_IRQ + } + }) +#endif + #ifdef EC_ENABLE_MKBP_DEVICE Device (CKSC) { diff --git a/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h b/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h index e90724e..12c6de6 100644 --- a/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h +++ b/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h @@ -15,3 +15,9 @@
/* Enable EC backed Keyboard Backlight in ACPI */ #define EC_ENABLE_KEYBOARD_BACKLIGHT + +/* + * Enable EC sync interrupt via GPIO controller, EC_SYNC_IRQ is defined in + * variant/gpio.h + */ +#define EC_ENABLE_SYNC_IRQ_GPIO diff --git a/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h b/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h index 5a6b540..3ddabb1 100644 --- a/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h +++ b/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h @@ -14,3 +14,6 @@ */
#include <baseboard/gpio.h> + +/* EC sync irq is AGPIO 10 */ +#define EC_SYNC_IRQ 10 diff --git a/src/mainboard/google/kahlee/variants/baseboard/gpio.c b/src/mainboard/google/kahlee/variants/baseboard/gpio.c index e9ae28c..9db26c7 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/gpio.c +++ b/src/mainboard/google/kahlee/variants/baseboard/gpio.c @@ -117,8 +117,8 @@ /* GPIO_8 - DDR_ALERT_3V3_L (currently not used) */ PAD_GPI(GPIO_8, PULL_UP),
- /* GPIO_10 - SLP_S0_L (currently not used) */ - PAD_NF(GPIO_10, S0A3_GPIO, PULL_UP), + /* GPIO_10 - SLP_S0_L, EC_SYNC_IRQ */ + PAD_GPI(GPIO_10, PULL_UP),
/* GPIO_11 - TOUCHSCREEN_INT_3V3_ODL, SCI */ PAD_SCI(GPIO_11, PULL_UP, EDGE_LOW),
Enrico Granata has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31278 )
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... File src/ec/google/chromeec/acpi/cros_ec.asl:
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... PS1, Line 38: GpioInt This might be a good time to ask the question: do we know of a (reasonable) way to make this work using the Interrupt ACPI resource instead of GpioInt?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31278 )
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... File src/ec/google/chromeec/acpi/cros_ec.asl:
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... PS1, Line 38: GpioInt
This might be a good time to ask the question: do we know of a (reasonable) way to make this work us […]
I'm not actually quire sure how all the plumbing works, but I think we could skip the GPIO controller and go directly to the APIC if the GPIO generated an SCI. GPIO10 doesn't have SCI support though. I'm not quite sure how the mappings work though. A GPIO maps to a GEVENT number. I'm assuming that Gevent number then maps to an IRQ from the kernel side somehow, but not really sure.
Enrico Granata has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31278 )
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... File src/ec/google/chromeec/acpi/cros_ec.asl:
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... PS1, Line 38: GpioInt
I'm not actually quire sure how all the plumbing works, but I think we could skip the GPIO controlle […]
Sorry, I should have been more explicit. We do definitely want a direct GPIO line, not an SCI (SCI is what we were using before this change, and it doesn't have good enough latency/jitter properties for this use case).
I was just hoping there would be a way to express this interrupt in ACPI by using an Interrupt resource - because on the kernel side, using a GpioInt requires different API calls (and it's not entirely obvious how to go about "fixing" that).
I wouldn't block this CL on that though, by all means if GpioInt it needs to be, it should be a GpioInt and we will make the kernel work one way or the other.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31278 )
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... File src/ec/google/chromeec/acpi/cros_ec.asl:
https://review.coreboot.org/#/c/31278/1/src/ec/google/chromeec/acpi/cros_ec.... PS1, Line 38: GpioInt
Sorry, I should have been more explicit. […]
It looks like it could be possible using GPIO90, but that would require a hardware change.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31278 )
Change subject: mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ ......................................................................
mb/google/kahlee: Use GPIO_10 for EC_SYNC_IRQ
Use AGPIO 10 as the EC sync interrupt for MKBP events for sensor data.
On this platform, interrupts are routed via the GPIO controller so need to be registered using GpioInt instead of Interrupt.
BUG=b:123750725 BRANCH=grunt TEST=MKBP events still received (with matching EC and kernel changes)
Change-Id: If499d24511bbaa7054207b7e0b98445723332c4f Signed-off-by: Edward Hill ecgh@chromium.org Reviewed-on: https://review.coreboot.org/c/31278 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Enrico Granata egranata@chromium.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Martin Roth martinroth@google.com --- M src/ec/google/chromeec/acpi/cros_ec.asl M src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h M src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h M src/mainboard/google/kahlee/variants/baseboard/gpio.c 4 files changed, 22 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Raul Rangel: Looks good to me, approved Enrico Granata: Looks good to me, but someone else must approve
diff --git a/src/ec/google/chromeec/acpi/cros_ec.asl b/src/ec/google/chromeec/acpi/cros_ec.asl index a5f9202..d41071e 100644 --- a/src/ec/google/chromeec/acpi/cros_ec.asl +++ b/src/ec/google/chromeec/acpi/cros_ec.asl @@ -32,6 +32,17 @@ }) #endif
+#ifdef EC_ENABLE_SYNC_IRQ_GPIO + Name (_CRS, ResourceTemplate () + { + GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, + "\_SB.GPIO", 0x00, ResourceConsumer, ,) + { + EC_SYNC_IRQ + } + }) +#endif + #ifdef EC_ENABLE_MKBP_DEVICE Device (CKSC) { diff --git a/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h b/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h index e90724e..12c6de6 100644 --- a/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h +++ b/src/mainboard/google/kahlee/variants/aleena/include/variant/ec.h @@ -15,3 +15,9 @@
/* Enable EC backed Keyboard Backlight in ACPI */ #define EC_ENABLE_KEYBOARD_BACKLIGHT + +/* + * Enable EC sync interrupt via GPIO controller, EC_SYNC_IRQ is defined in + * variant/gpio.h + */ +#define EC_ENABLE_SYNC_IRQ_GPIO diff --git a/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h b/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h index 5a6b540..3ddabb1 100644 --- a/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h +++ b/src/mainboard/google/kahlee/variants/aleena/include/variant/gpio.h @@ -14,3 +14,6 @@ */
#include <baseboard/gpio.h> + +/* EC sync irq is AGPIO 10 */ +#define EC_SYNC_IRQ 10 diff --git a/src/mainboard/google/kahlee/variants/baseboard/gpio.c b/src/mainboard/google/kahlee/variants/baseboard/gpio.c index e9ae28c..9db26c7 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/gpio.c +++ b/src/mainboard/google/kahlee/variants/baseboard/gpio.c @@ -117,8 +117,8 @@ /* GPIO_8 - DDR_ALERT_3V3_L (currently not used) */ PAD_GPI(GPIO_8, PULL_UP),
- /* GPIO_10 - SLP_S0_L (currently not used) */ - PAD_NF(GPIO_10, S0A3_GPIO, PULL_UP), + /* GPIO_10 - SLP_S0_L, EC_SYNC_IRQ */ + PAD_GPI(GPIO_10, PULL_UP),
/* GPIO_11 - TOUCHSCREEN_INT_3V3_ODL, SCI */ PAD_SCI(GPIO_11, PULL_UP, EDGE_LOW),
-
Edward Hill (Code Review) -
Enrico Granata (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)