Change in ...coreboot[master]: soc/amd/stoneyridge/gpio: Configure debounce for irq gpios
Hello Daniel Kurtz,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30921
to review the following change.
Change subject: soc/amd/stoneyridge/gpio: Configure debounce for irq gpios ......................................................................
soc/amd/stoneyridge/gpio: Configure debounce for irq gpios
FT4 has a strange property where whenever the debounce registers for any one gpio are changed, the FT4 disables interrupt propagation for ALL gpio irqs for ~4ms.
In other words, if an edge interrupt of one gpio happens exactly during this debounce-irq-off window immediately following the configuration of another gpio, the interrupt will be lost.
It is quite difficult to deal with this in the kernel, since during kernel boot time, drivers & devices are probed asynchronously, meaning it may happen that an already loaded driver may miss an interrupt when some later driver is being probed and configuring its gpio interrupt.
To eliminate this possibility, we pre-configure the debounce registers in ram stage for all gpios that will be used as irqs later by the kernel using the same configuration as used by the kernel, as per this table:
IRQ Debounce Edge Remove Glitch Level High Preserve Low Glitch Level Low Preserve High Glitch
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
BUG=b:113880780 BRANCH=none TEST=Reboot stress test grunt (>100 times); no messages in dmesg like: tpm tpm0: Timeout waiting for TPM ready
Change-Id: I94c7ecfb14e5bb209b3598e10287c80eb19da25b --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 36 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/30921/1
diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index 955cc6a..bca8f5d 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -219,12 +219,41 @@ return gpio; }
+/* + * Returns the debounce type corresponding to a given interrupt type. + * + * This matches what the linux kernel will set during gpio configuration: + * + * Interrupt Debounce + * Edge Remove Glitch + * Level High Preserve Low Glitch + * Level Low Preserve High Glitch + */ +static uint32_t gpio_irq_debounce(uint32_t flag) +{ + uint32_t trigger; + + trigger = flag & FLAGS_TRIGGER_MASK; + switch (trigger) { + case GPIO_TRIGGER_LEVEL_LOW: + return GPIO_IN_PRESERVE_HIGH_GLITCH; + case GPIO_TRIGGER_LEVEL_HIGH: + return GPIO_IN_PRESERVE_LOW_GLITCH; + case GPIO_TRIGGER_EDGE_LOW: + return GPIO_IN_REMOVE_GLITCH; + case GPIO_TRIGGER_EDGE_HIGH: + return GPIO_IN_REMOVE_GLITCH; + default: + return GPIO_IN_NO_DEBOUNCE; + } +} + void sb_program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { uint8_t *mux_ptr; uint32_t *gpio_ptr; uint32_t control, control_flags, edge_level, direction; - uint32_t mask, bit_edge, bit_level; + uint32_t mask, bit_edge, bit_level, debounce; uint8_t mux, index, gpio; int gevent_num;
@@ -273,6 +302,10 @@ case GPIO_SCI_FLAG: mem_read_write32(gpio_ptr, control, INT_SCI_SMI_MASK); + /* Always set debounce type for SCI gpio */ + debounce = gpio_irq_debounce(control_flags); + mem_read_write32(gpio_ptr, debounce, + GPIO_DEBOUNCE_MASK); get_sci_config_bits(control_flags, &bit_edge, &bit_level); edge_level |= bit_edge << gevent_num; diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 04eda49..422ee57 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -450,9 +450,11 @@ #define INT_SCI_SMI_MASK 0x00f40000
#define IN_GLITCH_SHIFT 5 +#define DEBOUNCE_NONE 0 #define GLITCH_LOW 1 #define GLITCH_HIGH 2 #define GLITCH_NONE 3 +#define GPIO_IN_NO_DEBOUNCE (DEBOUNCE_NONE << IN_GLITCH_SHIFT) #define GPIO_IN_PRESERVE_LOW_GLITCH (GLITCH_LOW << IN_GLITCH_SHIFT) #define GPIO_IN_PRESERVE_HIGH_GLITCH (GLITCH_HIGH << IN_GLITCH_SHIFT) #define GPIO_IN_REMOVE_GLITCH (GLITCH_NONE << IN_GLITCH_SHIFT)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30921 )
Change subject: soc/amd/stoneyridge/gpio: Configure debounce for irq gpios ......................................................................
Patch Set 1: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30921 )
Change subject: soc/amd/stoneyridge/gpio: Configure debounce for irq gpios ......................................................................
soc/amd/stoneyridge/gpio: Configure debounce for irq gpios
FT4 has a strange property where whenever the debounce registers for any one gpio are changed, the FT4 disables interrupt propagation for ALL gpio irqs for ~4ms.
In other words, if an edge interrupt of one gpio happens exactly during this debounce-irq-off window immediately following the configuration of another gpio, the interrupt will be lost.
It is quite difficult to deal with this in the kernel, since during kernel boot time, drivers & devices are probed asynchronously, meaning it may happen that an already loaded driver may miss an interrupt when some later driver is being probed and configuring its gpio interrupt.
To eliminate this possibility, we pre-configure the debounce registers in ram stage for all gpios that will be used as irqs later by the kernel using the same configuration as used by the kernel, as per this table:
IRQ Debounce Edge Remove Glitch Level High Preserve Low Glitch Level Low Preserve High Glitch
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
BUG=b:113880780 BRANCH=none TEST=Reboot stress test grunt (>100 times); no messages in dmesg like: tpm tpm0: Timeout waiting for TPM ready
Change-Id: I94c7ecfb14e5bb209b3598e10287c80eb19da25b Reviewed-on: https://review.coreboot.org/c/30921 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 36 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index 955cc6a..bca8f5d 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -219,12 +219,41 @@ return gpio; }
+/* + * Returns the debounce type corresponding to a given interrupt type. + * + * This matches what the linux kernel will set during gpio configuration: + * + * Interrupt Debounce + * Edge Remove Glitch + * Level High Preserve Low Glitch + * Level Low Preserve High Glitch + */ +static uint32_t gpio_irq_debounce(uint32_t flag) +{ + uint32_t trigger; + + trigger = flag & FLAGS_TRIGGER_MASK; + switch (trigger) { + case GPIO_TRIGGER_LEVEL_LOW: + return GPIO_IN_PRESERVE_HIGH_GLITCH; + case GPIO_TRIGGER_LEVEL_HIGH: + return GPIO_IN_PRESERVE_LOW_GLITCH; + case GPIO_TRIGGER_EDGE_LOW: + return GPIO_IN_REMOVE_GLITCH; + case GPIO_TRIGGER_EDGE_HIGH: + return GPIO_IN_REMOVE_GLITCH; + default: + return GPIO_IN_NO_DEBOUNCE; + } +} + void sb_program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { uint8_t *mux_ptr; uint32_t *gpio_ptr; uint32_t control, control_flags, edge_level, direction; - uint32_t mask, bit_edge, bit_level; + uint32_t mask, bit_edge, bit_level, debounce; uint8_t mux, index, gpio; int gevent_num;
@@ -273,6 +302,10 @@ case GPIO_SCI_FLAG: mem_read_write32(gpio_ptr, control, INT_SCI_SMI_MASK); + /* Always set debounce type for SCI gpio */ + debounce = gpio_irq_debounce(control_flags); + mem_read_write32(gpio_ptr, debounce, + GPIO_DEBOUNCE_MASK); get_sci_config_bits(control_flags, &bit_edge, &bit_level); edge_level |= bit_edge << gevent_num; diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 04eda49..422ee57 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -450,9 +450,11 @@ #define INT_SCI_SMI_MASK 0x00f40000
#define IN_GLITCH_SHIFT 5 +#define DEBOUNCE_NONE 0 #define GLITCH_LOW 1 #define GLITCH_HIGH 2 #define GLITCH_NONE 3 +#define GPIO_IN_NO_DEBOUNCE (DEBOUNCE_NONE << IN_GLITCH_SHIFT) #define GPIO_IN_PRESERVE_LOW_GLITCH (GLITCH_LOW << IN_GLITCH_SHIFT) #define GPIO_IN_PRESERVE_HIGH_GLITCH (GLITCH_HIGH << IN_GLITCH_SHIFT) #define GPIO_IN_REMOVE_GLITCH (GLITCH_NONE << IN_GLITCH_SHIFT)
Raul Rangel has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/30921 )
Change subject: soc/amd/stoneyridge/gpio: Configure debounce for irq gpios ......................................................................
-
Daniel Kurtz (Code Review) -
Martin Roth (Code Review)
-
Raul Rangel (Code Review)