Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31082
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
amd/stoneyridge: Disable GPIO MASK STATUS
MASK_STATUS disables interrupt status generation for the entire GPIO controller when any debounce register is configured. This causes problems when the kernel is loading drivers because we could lose interrupts for previously loaded devices.
sb_program_gpios is also not setup to wait when configuring PAD_DEBOUNCE, so there is a potential that we could lose the interrupt status enable bit for other registers. By disabling MASK_STATUS we avoid that problem.
BUG=b:113880780 BRANCH=none TEST=Ran a reboot stress test that concluded that we are no longer losing TPM interrupts while booting.
Change-Id: Ife1db3b1449f205092509595cbc3eca511bff57a Signed-off-by: Raul E Rangel rrangel@chromium.org --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31082/1
diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index bca8f5d..9b0d2b1 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -251,15 +251,31 @@ 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 *gpio_ptr, *inter_master; uint32_t control, control_flags, edge_level, direction; uint32_t mask, bit_edge, bit_level, debounce; uint8_t mux, index, gpio; int gevent_num;
+ inter_master = (uint32_t *)(uintptr_t)(GPIO_CONTROL_MMIO_BASE + + INTER_MASTER_SWITCH); direction = 0; edge_level = 0; mask = 0; + + /* Disable blocking wake/interrupt status generation while updating + debounce registers. Otherwise when a debounce register is updated + the whole GPIO controller will zero out all interrupt enable status + bits while the delay happens. This could cause us to drop the bits + due to the read-modify-write that happens on each register. + + Additionally disable interrupt generation so we don't get any + spurious interrupts while updating the registers. + */ + mem_read_write32(inter_master, + MASK_STATUS_DISABLE | GPIO_INTER_DISABLE, + MASK_STATUS_MASK | GPIO_INTER_MASK); + for (index = 0; index < size; index++) { gpio = gpio_list_ptr[index].gpio; mux = gpio_list_ptr[index].function; @@ -323,6 +339,12 @@ AMD_GPIO_CONTROL_MASK); } } + + /* Re-enable interrupt status generation. We leave MASK_STATUS disabled + because the kernel will reconfigure the debounce registers while the + drivers load. This will cause interrupts to be missed during boot. */ + mem_read_write32(inter_master, GPIO_INTER_ENABLE, GPIO_INTER_MASK); + /* Set all SCI trigger direction (high/low) */ mem_read_write32((uint32_t *)(uintptr_t)(APU_SMI_BASE + SMI_SCI_TRIG), direction, mask); diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 422ee57..bf2b714 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -41,6 +41,16 @@ uint8_t mux_value; };
+#define INTER_MASTER_SWITCH 0xFC +#define MASK_STATUS_SHIFT 28 +#define MASK_STATUS_MASK (1 << MASK_STATUS_SHIFT) +#define MASK_STATUS_ENABLE (1 << MASK_STATUS_SHIFT) +#define MASK_STATUS_DISABLE 0 +#define GPIO_INTER_SHIFT 30 +#define GPIO_INTER_MASK (1 << GPIO_INTER_SHIFT) +#define GPIO_INTER_ENABLE (1 << GPIO_INTER_SHIFT) +#define GPIO_INTER_DISABLE 0 + #define GPIO_I2C0_SCL BIT(0) #define GPIO_I2C1_SCL BIT(1) #define GPIO_I2C2_SCL BIT(2)
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31082 )
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c File src/soc/amd/stoneyridge/gpio.c:
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c@267 PS1, Line 267: Per https://www.coreboot.org/Developer_Manual (which may be of date?), coreboot uses linux kernel style for which the the preferred style for long (multi-line) comments is:
/* * This is the preferred style for multi-line * comments in the Linux kernel source code. * Please use it consistently. * * Description: A column of asterisks on the left side, * with beginning and ending almost-blank lines. */
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c@344 PS1, Line 344: will s/will/may
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 45: #define MASK_STATUS_SHIFT 28 Hmm, perhaps stick closer to the BKDG names?
#define GPIO_INTERRUPT_EN BIT(30) #define GPIO_MASK_STS_EN BIT(28)
and then in the .c: mem_read_write32(inter_master, GPIO_INTERRUPT_EN | GPIO_MASK_STS_EN, 0); ... mem_read_write32(inter_master, GPIO_INTERRUPT_EN, GPIO_INTERRUPT_EN);
Hello Daniel Kurtz, build bot (Jenkins), Daniel Kurtz, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31082
to look at the new patch set (#2).
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
amd/stoneyridge: Disable GPIO MASK STATUS
MASK_STATUS disables interrupt status generation for the entire GPIO controller when any debounce register is configured. This causes problems when the kernel is loading drivers because we could lose interrupts for previously loaded devices.
sb_program_gpios is also not setup to wait when configuring PAD_DEBOUNCE, so there is a potential that we could lose the interrupt status enable bit for other registers. By disabling MASK_STATUS we avoid that problem.
BUG=b:113880780 BRANCH=none TEST=Ran a reboot stress test that concluded that we are no longer losing TPM interrupts while booting.
Change-Id: Ife1db3b1449f205092509595cbc3eca511bff57a Signed-off-by: Raul E Rangel rrangel@chromium.org --- M src/soc/amd/stoneyridge/gpio.c M src/soc/amd/stoneyridge/include/soc/gpio.h 2 files changed, 30 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31082/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31082 )
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c File src/soc/amd/stoneyridge/gpio.c:
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c@267 PS1, Line 267:
Per https://www.coreboot.org/Developer_Manual (which may be of date?), coreboot uses linux […]
Done
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/gpio.c@344 PS1, Line 344: will
s/will/may
Done
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/31082/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 45: #define MASK_STATUS_SHIFT 28
Hmm, perhaps stick closer to the BKDG names? […]
Done
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31082 )
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31082 )
Change subject: amd/stoneyridge: Disable GPIO MASK STATUS ......................................................................
amd/stoneyridge: Disable GPIO MASK STATUS
MASK_STATUS disables interrupt status generation for the entire GPIO controller when any debounce register is configured. This causes problems when the kernel is loading drivers because we could lose interrupts for previously loaded devices.
sb_program_gpios is also not setup to wait when configuring PAD_DEBOUNCE, so there is a potential that we could lose the interrupt status enable bit for other registers. By disabling MASK_STATUS we avoid that problem.
BUG=b:113880780 BRANCH=none TEST=Ran a reboot stress test that concluded that we are no longer losing TPM interrupts while booting.
Change-Id: Ife1db3b1449f205092509595cbc3eca511bff57a Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://review.coreboot.org/c/31082 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, 30 insertions(+), 1 deletion(-)
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 955cc6a..621aca3 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -222,15 +222,30 @@ 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 *gpio_ptr, *inter_master; uint32_t control, control_flags, edge_level, direction; uint32_t mask, bit_edge, bit_level; uint8_t mux, index, gpio; int gevent_num;
+ inter_master = (uint32_t *)(uintptr_t)(GPIO_CONTROL_MMIO_BASE + + GPIO_MASTER_SWITCH); direction = 0; edge_level = 0; mask = 0; + + /* + * Disable blocking wake/interrupt status generation while updating + * debounce registers. Otherwise when a debounce register is updated + * the whole GPIO controller will zero out all interrupt enable status + * bits while the delay happens. This could cause us to drop the bits + * due to the read-modify-write that happens on each register. + * + * Additionally disable interrupt generation so we don't get any + * spurious interrupts while updating the registers. + */ + mem_read_write32(inter_master, 0, GPIO_MASK_STS_EN | GPIO_INTERRUPT_EN); + for (index = 0; index < size; index++) { gpio = gpio_list_ptr[index].gpio; mux = gpio_list_ptr[index].function; @@ -290,6 +305,16 @@ AMD_GPIO_CONTROL_MASK); } } + + /* + * Re-enable interrupt status generation. + * + * We leave MASK_STATUS disabled because the kernel may reconfigure the + * debounce registers while the drivers load. This will cause interrupts + * to be missed during boot. + */ + mem_read_write32(inter_master, GPIO_INTERRUPT_EN, GPIO_INTERRUPT_EN); + /* Set all SCI trigger direction (high/low) */ mem_read_write32((uint32_t *)(uintptr_t)(APU_SMI_BASE + SMI_SCI_TRIG), direction, mask); diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 47eae84..fe8240f 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -41,6 +41,10 @@ uint8_t mux_value; };
+#define GPIO_MASTER_SWITCH 0xFC +#define GPIO_MASK_STS_EN BIT(28) +#define GPIO_INTERRUPT_EN BIT(30) + #define GPIO_I2C0_SCL BIT(0) #define GPIO_I2C1_SCL BIT(1) #define GPIO_I2C2_SCL BIT(2)
-
Daniel Kurtz (Code Review) -
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)