Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/1
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 4f1b842..f8133a9 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -3,12 +3,14 @@ #include <device/mmio.h> #include <device/device.h> #include <console/console.h> +#include <elog.h> #include <gpio.h> #include <amdblocks/acpimmio.h> #include <amdblocks/gpio_banks.h> #include <soc/gpio.h> #include <soc/smi.h> #include <assert.h> +#include <string.h>
static int get_gpio_gevent(uint8_t gpio, const struct soc_amd_event *table, size_t items) @@ -298,3 +300,67 @@ program_gpios(c, 1); } } + +static void check_and_add_wake_gpio(int begin, int end, struct gpio_wake_state *state) +{ + int i; + uint32_t reg; + + for (i = begin; i < end; i++) { + reg = gpio_read32(begin); + if (!(reg & GPIO_WAKE_STATUS)) + continue; + printk(BIOS_INFO, "GPIO %d woke system.\n", i); + if (state->num_valid_wake_gpios >= ARRAY_SIZE(state->wake_gpios)) + continue; + state->wake_gpios[state->num_valid_wake_gpios++] = i; + } +} + +static void check_gpios(uint32_t wake_stat, int bit_limit, int gpio_base, + struct gpio_wake_state *state) +{ + int i; + int gpio_end; + + for (i = 0; i < bit_limit; i++) { + if (!(wake_stat & (1U << i))) + continue; + gpio_base = i * 4; + gpio_end = gpio_base + 4; + /* There is no gpio 63. */ + if (gpio_base == 60) + gpio_end = 63; + check_and_add_wake_gpio(gpio_base, gpio_end, state); + } +} + +void gpio_fill_wake_state(struct gpio_wake_state *state) +{ + /* Turn the wake registers into "gpio" index to conform to existing API. */ + const uint8_t stat0 = GPIO_WAKE_STAT_0 / sizeof(uint32_t); + const uint8_t stat1 = GPIO_WAKE_STAT_1 / sizeof(uint32_t); + const uint8_t master_switch = GPIO_MASTER_SWITCH / sizeof(uint32_t); + + memset(state, 0, sizeof(*state)); + + state->master_switch = gpio_read32(master_switch); + state->wake_stat[0] = gpio_read32(stat0); + state->wake_stat[1] = gpio_read32(stat1); + + printk(BIOS_INFO, "GPIO Master Switch: 0x%08x, Wake Stat 0: 0x%08x, Wake Stat 1: 0x%08x\n", + state->master_switch, state->wake_stat[0], state->wake_stat[1]); + + check_gpios(state->wake_stat[0], 32, 0, state); + check_gpios(state->wake_stat[1], 14, 128, state); +} + +void gpio_add_events(const struct gpio_wake_state *state) +{ + int i; + int end; + + end = MIN(state->num_valid_wake_gpios, ARRAY_SIZE(state->wake_gpios)); + for (i = 0; i < end; i++) + elog_add_event_wake(ELOG_WAKE_SOURCE_GPIO, state->wake_gpios[i]); +} diff --git a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h index fdcd0f8..e020c92 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -20,6 +20,23 @@ #define GPIO_MASTER_SWITCH 0xFC #define GPIO_MASK_STS_EN BIT(28) #define GPIO_INTERRUPT_EN BIT(30) +#define GPIO_WAKE_EN BIT(31) + +#define GPIO_WAKE_STAT_0 0x2F0 +#define GPIO_WAKE_STAT_1 0x2F4 +struct gpio_wake_state { + uint32_t master_switch; + uint32_t wake_stat[2]; + /* Number of wake_gpio with a valid setting. */ + uint32_t num_valid_wake_gpios; + /* GPIO index number that caused a wake. */ + uint8_t wake_gpios[16]; +}; + +/* Fill gpio_wake_state object for future event reporting. */ +void gpio_fill_wake_state(struct gpio_wake_state *state); +/* Add gpio events to the eventlog. */ +void gpio_add_events(const struct gpio_wake_state *state);
#define GPIO_PIN_IN (1 << 0) /* for byte access */ #define GPIO_PIN_OUT (1 << 6) /* for byte access */
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44534/1/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/1/src/soc/amd/common/block/gp... PS1, Line 354: check_gpios(state->wake_stat[0], 32, 0, state); : check_gpios(state->wake_stat[1], 14, 128, state); I'm wondering if I should move this whole patch to be under picasso. I don't think this logic applies to stoney, and I'm not really wanting to try and sort that out. I have a feeling many things are not "common".
Hello Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44534
to look at the new patch set (#2).
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... PS2, Line 310: begin i
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... PS2, Line 329: gpio_base = i * 4; This is overwriting the gpio_base value passed in by the caller.
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44534
to look at the new patch set (#3).
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... PS2, Line 310: begin
i
Done
https://review.coreboot.org/c/coreboot/+/44534/2/src/soc/amd/common/block/gp... PS2, Line 329: gpio_base = i * 4;
This is overwriting the gpio_base value passed in by the caller.
Done
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44534
to look at the new patch set (#4).
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 84 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/4
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44534
to look at the new patch set (#5).
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44534/1/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/1/src/soc/amd/common/block/gp... PS1, Line 354: check_gpios(state->wake_stat[0], 32, 0, state); : check_gpios(state->wake_stat[1], 14, 128, state);
I'm wondering if I should move this whole patch to be under picasso. […]
I added a Picasso check and a comment.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/44534/4/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/4/src/soc/amd/common/block/gp... PS4, Line 328: (1U << i) nit: BIT(i)
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... PS5, Line 352: control_switch Just a note: This isn't really used anywhere except for printing below. Did you intend to check GPIO_WAKE_EN before inspecting the wake stat bits?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44534/4/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/4/src/soc/amd/common/block/gp... PS4, Line 328: (1U << i)
nit: BIT(i)
Done
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... PS5, Line 352: control_switch
Just a note: This isn't really used anywhere except for printing below. […]
I added that because the PPR isn't clear on what bit is blocking the other registers. In my mind, I assumed GpioWakeEn blocks the update of GPIOWakeStatIndex0|1. However, nothing indicates actual behavior. i.e. does it block actual wake or these register fields or both? Because of that I included it in case we needed to conditionally check GpioWakeEn. I can remove it, but I feel like the info may be helpful in the future when we discover the hardware is doing odd things. Let me know what you'd like me to do.
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44534
to look at the new patch set (#6).
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/44534/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/44534/5/src/soc/amd/common/block/gp... PS5, Line 352: control_switch
I added that because the PPR isn't clear on what bit is blocking the other registers. […]
Yeah, I had similar things in mind. I agree that having the information printed out would be helpful when hardware is doing odd things. I just wanted to make sure that it wasn't a miss that the control switch isn't checked anywhere. I think what we have in this patchset looks good.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44534 )
Change subject: soc/amd/common: add gpio subsystem event reporting ......................................................................
soc/amd/common: add gpio subsystem event reporting
In order to log gpio events for wake purposes the state of the gpio subsystem should be snapshotted. Add the ability to capture state of gpio subystem as well as saving up to 16 gpios that indicate their wake status.
Likewise, provide the eventlog additions based on state.
BUG=b:159947207
Signed-off-by: Aaron Durbin adurbin@chromium.org Change-Id: I49fca56c87543aa8aad0eb7da5c5cb570c4349d5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44534 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h 2 files changed, 88 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 4f1b842..7fb6622 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -3,12 +3,14 @@ #include <device/mmio.h> #include <device/device.h> #include <console/console.h> +#include <elog.h> #include <gpio.h> #include <amdblocks/acpimmio.h> #include <amdblocks/gpio_banks.h> #include <soc/gpio.h> #include <soc/smi.h> #include <assert.h> +#include <string.h>
static int get_gpio_gevent(uint8_t gpio, const struct soc_amd_event *table, size_t items) @@ -298,3 +300,72 @@ program_gpios(c, 1); } } + +static void check_and_add_wake_gpio(int begin, int end, struct gpio_wake_state *state) +{ + int i; + uint32_t reg; + + for (i = begin; i < end; i++) { + reg = gpio_read32(i); + if (!(reg & GPIO_WAKE_STATUS)) + continue; + printk(BIOS_INFO, "GPIO %d woke system.\n", i); + if (state->num_valid_wake_gpios >= ARRAY_SIZE(state->wake_gpios)) + continue; + state->wake_gpios[state->num_valid_wake_gpios++] = i; + } +} + +static void check_gpios(uint32_t wake_stat, int bit_limit, int gpio_base, + struct gpio_wake_state *state) +{ + int i; + int begin; + int end; + + for (i = 0; i < bit_limit; i++) { + if (!(wake_stat & BIT(i))) + continue; + begin = gpio_base + i * 4; + end = begin + 4; + /* There is no gpio 63. */ + if (begin == 60) + end = 63; + check_and_add_wake_gpio(begin, end, state); + } +} + +void gpio_fill_wake_state(struct gpio_wake_state *state) +{ + /* Turn the wake registers into "gpio" index to conform to existing API. */ + const uint8_t stat0 = GPIO_WAKE_STAT_0 / sizeof(uint32_t); + const uint8_t stat1 = GPIO_WAKE_STAT_1 / sizeof(uint32_t); + const uint8_t control_switch = GPIO_MASTER_SWITCH / sizeof(uint32_t); + + /* Register fields and gpio availability need to be confirmed on other chipsets. */ + if (!CONFIG(SOC_AMD_PICASSO)) + dead_code(); + + memset(state, 0, sizeof(*state)); + + state->control_switch = gpio_read32(control_switch); + state->wake_stat[0] = gpio_read32(stat0); + state->wake_stat[1] = gpio_read32(stat1); + + printk(BIOS_INFO, "GPIO Control Switch: 0x%08x, Wake Stat 0: 0x%08x, Wake Stat 1: 0x%08x\n", + state->control_switch, state->wake_stat[0], state->wake_stat[1]); + + check_gpios(state->wake_stat[0], 32, 0, state); + check_gpios(state->wake_stat[1], 14, 128, state); +} + +void gpio_add_events(const struct gpio_wake_state *state) +{ + int i; + int end; + + end = MIN(state->num_valid_wake_gpios, ARRAY_SIZE(state->wake_gpios)); + for (i = 0; i < end; i++) + elog_add_event_wake(ELOG_WAKE_SOURCE_GPIO, state->wake_gpios[i]); +} diff --git a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h index fdcd0f8..b1663cf 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -20,6 +20,23 @@ #define GPIO_MASTER_SWITCH 0xFC #define GPIO_MASK_STS_EN BIT(28) #define GPIO_INTERRUPT_EN BIT(30) +#define GPIO_WAKE_EN BIT(31) + +#define GPIO_WAKE_STAT_0 0x2F0 +#define GPIO_WAKE_STAT_1 0x2F4 +struct gpio_wake_state { + uint32_t control_switch; + uint32_t wake_stat[2]; + /* Number of wake_gpio with a valid setting. */ + uint32_t num_valid_wake_gpios; + /* GPIO index number that caused a wake. */ + uint8_t wake_gpios[16]; +}; + +/* Fill gpio_wake_state object for future event reporting. */ +void gpio_fill_wake_state(struct gpio_wake_state *state); +/* Add gpio events to the eventlog. */ +void gpio_add_events(const struct gpio_wake_state *state);
#define GPIO_PIN_IN (1 << 0) /* for byte access */ #define GPIO_PIN_OUT (1 << 6) /* for byte access */
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)