Change in ...coreboot[master]: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios"
Hello Daniel Kurtz, build bot (Jenkins), Daniel Kurtz, Martin Roth,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30981
to review the following change.
Change subject: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios" ......................................................................
Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios"
This reverts commit b82afce18aff24b6e5e3b73a67a6533cc4865a26.
Reason for revert: This causes depthcharge to not boot due to TPM timeout errors. Because there is no wait after setting the debounce register, we lose data because the read-modify-write loses the interrupt status bit.
e.g., GPIO 5 sets debounce, without a wait. Then GPIO 9 has it's debounce set. Because the interrupt controller is masking the interrupt enable status bit, the read-modify-write for GPIO9 loses the interrupt enable status bit and it never gets set again. This causes the interrupt to never latch.
We should possibly make depthcharge set the interrupt enable status bit for latched GPIOs.
Change-Id: Idd7259b14b24c441529d64e173be9faec03f4fc8 --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 1 insertion(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/30981/1
diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index bca8f5d..955cc6a 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -219,41 +219,12 @@ 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, debounce; + uint32_t mask, bit_edge, bit_level; uint8_t mux, index, gpio; int gevent_num;
@@ -302,10 +273,6 @@ 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 422ee57..04eda49 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -450,11 +450,9 @@ #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/+/30981 )
Change subject: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios" ......................................................................
Patch Set 1: Code-Review+2
Martin Roth has uploaded a new patch set (#2) to the change originally created by Raul Rangel. ( https://review.coreboot.org/c/coreboot/+/30981 )
Change subject: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios" ......................................................................
Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios"
This reverts commit b82afce18aff24b6e5e3b73a67a6533cc4865a26.
Reason for revert: This causes depthcharge to not boot due to TPM timeout errors. Because there is no wait after setting the debounce register, we lose data because the read-modify-write loses the interrupt status bit.
e.g., GPIO 5 sets debounce, without a wait. Then GPIO 9 has it's debounce set. Because the interrupt controller is masking the interrupt enable status bit, the read-modify-write for GPIO9 loses the interrupt enable status bit and it never gets set again. This causes the interrupt to never latch.
We should possibly make depthcharge set the interrupt enable status bit for latched GPIOs.
Change-Id: Idd7259b14b24c441529d64e173be9faec03f4fc8 Signed-off-by: Raul E Rangel rrangel@chromium.org Signed-off-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, 1 insertion(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/30981/2
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30981 )
Change subject: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios" ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30981 )
Change subject: Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios" ......................................................................
Revert "soc/amd/stoneyridge/gpio: Configure debounce for irq gpios"
This reverts commit b82afce18aff24b6e5e3b73a67a6533cc4865a26.
Reason for revert: This causes depthcharge to not boot due to TPM timeout errors. Because there is no wait after setting the debounce register, we lose data because the read-modify-write loses the interrupt status bit.
e.g., GPIO 5 sets debounce, without a wait. Then GPIO 9 has it's debounce set. Because the interrupt controller is masking the interrupt enable status bit, the read-modify-write for GPIO9 loses the interrupt enable status bit and it never gets set again. This causes the interrupt to never latch.
We should possibly make depthcharge set the interrupt enable status bit for latched GPIOs.
Change-Id: Idd7259b14b24c441529d64e173be9faec03f4fc8 Signed-off-by: Raul E Rangel rrangel@chromium.org Signed-off-by: Martin Roth martinroth@google.com Reviewed-on: https://review.coreboot.org/c/30981 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Daniel Kurtz djkurtz@google.com --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 1 insertion(+), 36 deletions(-)
Approvals: build bot (Jenkins): Verified Daniel Kurtz: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index bca8f5d..955cc6a 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -219,41 +219,12 @@ 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, debounce; + uint32_t mask, bit_edge, bit_level; uint8_t mux, index, gpio; int gevent_num;
@@ -302,10 +273,6 @@ 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 422ee57..04eda49 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -450,11 +450,9 @@ #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)
-
Daniel Kurtz (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)