Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
[WIP] mb/google/kahlee,zork: Remove special GPIO_2 override
Change-Id: I44661f05c8f517ece88714c625603579731d174b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/mainboard/amd/padmelon/gpio.c M src/mainboard/google/kahlee/variants/baseboard/gpio.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/gpio.c M src/soc/amd/stoneyridge/gpio.c 7 files changed, 3 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/43049/1
diff --git a/src/mainboard/amd/padmelon/gpio.c b/src/mainboard/amd/padmelon/gpio.c index 7897d7b..966c69c 100644 --- a/src/mainboard/amd/padmelon/gpio.c +++ b/src/mainboard/amd/padmelon/gpio.c @@ -28,7 +28,7 @@ /* WLAND */ PAD_WAKE(GPIO_137, PULL_UP, LEVEL_LOW, S3), #else - /* PCIE_WAKE */ + /* PCIE_WAKE, SCI */ PAD_GPI(GPIO_2, PULL_DOWN), /* DEVSLP1 - default as GPIO, do not program */
diff --git a/src/mainboard/google/kahlee/variants/baseboard/gpio.c b/src/mainboard/google/kahlee/variants/baseboard/gpio.c index 59d7631..27db3fb 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/gpio.c +++ b/src/mainboard/google/kahlee/variants/baseboard/gpio.c @@ -81,7 +81,7 @@ /* GPIO_1 - SYS_RST_ODL */ PAD_NF(GPIO_1, SYS_RESET_L, PULL_UP),
- /* GPIO_2 - WLAN_PCIE_WAKE_3V3_ODL */ + /* GPIO_2 - WLAN_PCIE_WAKE_3V3_ODL, SCI*/ PAD_NF(GPIO_2, WAKE_L, PULL_UP),
/* GPIO_3 - MEM_VOLT_SEL */ diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c index 2191793..5d5dd11 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -73,7 +73,7 @@ PAD_NF(GPIO_0, PWR_BTN_L, PULL_UP), /* SYS_RESET_L */ PAD_NF(GPIO_1, SYS_RESET_L, PULL_NONE), - /* PCIE_WAKE_L */ + /* PCIE_WAKE_L, SCI */ PAD_NF(GPIO_2, WAKE_L, PULL_UP), /* PEN_DETECT_ODL */ PAD_GPI(GPIO_4, PULL_UP), diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 1bcfc8b..f66921d 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -173,8 +173,6 @@ return gpio; }
-__weak void soc_gpio_hook(uint8_t gpio, uint8_t mux) {} - static void set_single_gpio(const struct soc_amd_gpio *g, struct sci_trigger_regs *sci_cfg) { static const struct soc_amd_event *gev_tbl; @@ -184,8 +182,6 @@ iomux_write8(g->gpio, g->function & AMD_GPIO_MUX_MASK); iomux_read8(g->gpio); /* Flush posted write */
- soc_gpio_hook(g->gpio, g->function); - /* Clear interrupt and wake status (write 1-to-clear bits) */ uint32_t control = g->control | GPIO_INT_STATUS | GPIO_WAKE_STATUS; __gpio_setbits32(g->gpio, PAD_CFG_MASK, control); 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 92eae73..4143e81 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -340,7 +340,5 @@ int gpio_interrupt_status(gpio_t gpio); /* Implemented by soc, provides table of available GPIO mapping to Gevents */ void soc_get_gpio_event_table(const struct soc_amd_event **table, size_t *items); -/* May be implemented by soc to handle special cases */ -void soc_gpio_hook(uint8_t gpio, uint8_t mux);
#endif /* __AMDBLOCK_GPIO_BANKS_H__ */ diff --git a/src/soc/amd/picasso/gpio.c b/src/soc/amd/picasso/gpio.c index 47c005e..78c15fe 100644 --- a/src/soc/amd/picasso/gpio.c +++ b/src/soc/amd/picasso/gpio.c @@ -38,10 +38,3 @@ *table = gpio_event_table; *items = ARRAY_SIZE(gpio_event_table); } - -void soc_gpio_hook(uint8_t gpio, uint8_t mux) -{ - /* Always program Gevent when WAKE_L_AGPIO2 is configured as WAKE_L */ - if ((gpio == 2) && !(mux & AMD_GPIO_MUX_MASK)) - soc_route_sci(GPIO_2_EVENT); -} diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index bd9d1b1..26dec3f 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -38,10 +38,3 @@ *table = gpio_event_table; *items = ARRAY_SIZE(gpio_event_table); } - -void soc_gpio_hook(uint8_t gpio, uint8_t mux) -{ - /* Always program Gevent when WAKE_L_AGPIO2 is configured as WAKE_L */ - if ((gpio == 2) && !(mux & AMD_GPIO_MUX_MASK)) - soc_route_sci(GPIO_2_EVENT); -}
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Abandoned
Attention is currently required from: Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/43049/comment/a132e587_3b620354 PS3, Line 31: , SCI no SCI gets configured here, since the iomux is set to gpio and not to wake pin mode. i have my doubts if this should be in gpio mode though
Attention is currently required from: Kyösti Mälkki. Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Restored
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3: Felix commented in CB:42807
yeah, calling soc_route_sci in soc_gpio_hook is rather unexpected. as far as i've seen the code in soc_gpio_hook was added to make sure that if gpio 2 is configured as wake pin (pin mux setting 0) the corresponding sci gets configured. introducing a PAD_NF_SCI macro and using that instead of the PAD_NF + soc_gpio_hook magic for the wake pin would allow removing the soc_gpio_hook. do you want to look into that or do you want me to open in internal ticket to look into that later?
Attention is currently required from: Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Felix commented in CB:42807 […]
i can take over this patch and implement the solution i outlined in the other patch; the PAD_NF_SCI macro would solve the problem in a clean and clear way and allows us to get rid of soc_gpio_hook
Attention is currently required from: Felix Held. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
i can take over this patch and implement the solution i outlined in the other patch; the PAD_NF_SCI […]
Please do.
Attention is currently required from: Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: [WIP] mb/google/kahlee,zork: Remove special GPIO_2 override ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Please do.
will do; it'll however take some time until i get around to do this. see CB:50373 for what i'm planning to do; it's completely untested at the moment, but i'd say that that will more or less be the way to go to et rid of soc_gpio_hook
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Felix Held has uploaded a new patch set (#4) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
soc/amd: remove special GPIO_2 override soc_gpio_hook
This override was added to have the SCI mapping configured if GPIO was used as WAKE_L pin. This however didn't set up the SCI level and trigger information, so it likely never worked as intended. The soc_gpio_hook is kept for Cezanne, since this SoC needs some GPIO hooks to enable/disable some functionality that somehow doesn't get controlled by the GPIO mux, so when you switch certain pins into GPIO mode, the special functionality doesn't get disabled automatically and additional registers have to be written to behave as intended.
Change-Id: I44661f05c8f517ece88714c625603579731d174b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/cezanne/gpio.c M src/soc/amd/picasso/gpio.c M src/soc/amd/stoneyridge/gpio.c 3 files changed, 0 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/43049/4
Attention is currently required from: Jason Glenesk, Marshall Dawson. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 4:
(1 comment)
File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/43049/comment/904c29c6_f2c37c7f PS3, Line 31: , SCI
no SCI gets configured here, since the iomux is set to gpio and not to wake pin mode. […]
Done
Attention is currently required from: Jason Glenesk, Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4: if you have a better idea to solve what i try to do in CB:51757 please let me know
Attention is currently required from: Jason Glenesk, Marshall Dawson, Kyösti Mälkki, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 4: Code-Review+2
Attention is currently required from: Jason Glenesk, Marshall Dawson, Kyösti Mälkki, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
if you have a better idea to solve what i try to do in CB:51757 please let me know
We can make use of Kconfig or devicetree config to define policy i.e. mainboard can select this to indicate that it wants to disable/enable KBRST functionality and the SoC code can implement the policy -- just like it is done for DISABLE_SPI_FLASH_ROM_SHARING.
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Felix Held has uploaded a new patch set (#5) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
soc/amd: remove special GPIO_2 override soc_gpio_hook
This override was added to have the SCI mapping configured if GPIO was used as WAKE_L pin. This however didn't set up the SCI level and trigger information, so it likely never worked as intended.
Change-Id: I44661f05c8f517ece88714c625603579731d174b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/cezanne/gpio.c M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/gpio.c M src/soc/amd/stoneyridge/gpio.c 5 files changed, 0 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/43049/5
Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS4:
We can make use of Kconfig or devicetree config to define policy i.e. […]
sounds reasonable to me. still not 100% sure what the best solution would be tbh
Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Kyösti Mälkki. Felix Held has uploaded a new patch set (#6) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
soc/amd: remove special GPIO_2 override soc_gpio_hook
This override was added to have the SCI mapping configured if GPIO was used as WAKE_L pin. This however didn't set up the SCI level and trigger information, so it likely never worked as intended.
Change-Id: I44661f05c8f517ece88714c625603579731d174b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/cezanne/gpio.c M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/gpio.c M src/soc/amd/stoneyridge/gpio.c 5 files changed, 0 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/43049/6
Attention is currently required from: Jason Glenesk, Marshall Dawson, Kyösti Mälkki, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
Patch Set 6: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43049 )
Change subject: soc/amd: remove special GPIO_2 override soc_gpio_hook ......................................................................
soc/amd: remove special GPIO_2 override soc_gpio_hook
This override was added to have the SCI mapping configured if GPIO was used as WAKE_L pin. This however didn't set up the SCI level and trigger information, so it likely never worked as intended.
Change-Id: I44661f05c8f517ece88714c625603579731d174b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43049 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/cezanne/gpio.c M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/gpio.c M src/soc/amd/stoneyridge/gpio.c 5 files changed, 0 insertions(+), 36 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/cezanne/gpio.c b/src/soc/amd/cezanne/gpio.c index e1f5046..f2a59e0 100644 --- a/src/soc/amd/cezanne/gpio.c +++ b/src/soc/amd/cezanne/gpio.c @@ -1,10 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <amdblocks/gpio_banks.h> -#include <amdblocks/acpimmio.h> -#include <amdblocks/smi.h> #include <soc/gpio.h> -#include <soc/smi.h> #include <types.h>
/* see the IOMUX function table for the mapping from GPIO number to GEVENT number */ @@ -40,10 +37,3 @@ *table = gpio_event_table; *items = ARRAY_SIZE(gpio_event_table); } - -void soc_gpio_hook(uint8_t gpio, uint8_t mux) -{ - /* Always program Gevent when WAKE_L_AGPIO2 is configured as WAKE_L */ - if ((gpio == 2) && !(mux & AMD_GPIO_MUX_MASK)) - soc_route_sci(GPIO_2_EVENT); -} diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index af948ce..186df50 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -179,8 +179,6 @@ return gpio; }
-__weak void soc_gpio_hook(uint8_t gpio, uint8_t mux) {} - static void set_single_gpio(const struct soc_amd_gpio *g, struct sci_trigger_regs *sci_trigger_cfg) { @@ -193,8 +191,6 @@ iomux_write8(g->gpio, g->function & AMD_GPIO_MUX_MASK); iomux_read8(g->gpio); /* Flush posted write */
- soc_gpio_hook(g->gpio, g->function); - gpio_setbits32(g->gpio, PAD_CFG_MASK, g->control); /* Clear interrupt and wake status (write 1-to-clear bits) */ gpio_or32(g->gpio, GPIO_INT_STATUS | GPIO_WAKE_STATUS); 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 7e6914f..098208b 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -85,7 +85,5 @@ int gpio_interrupt_status(gpio_t gpio); /* Implemented by soc, provides table of available GPIO mapping to Gevents */ void soc_get_gpio_event_table(const struct soc_amd_event **table, size_t *items); -/* May be implemented by soc to handle special cases */ -void soc_gpio_hook(uint8_t gpio, uint8_t mux);
#endif /* AMD_BLOCK_GPIO_BANKS_H */ diff --git a/src/soc/amd/picasso/gpio.c b/src/soc/amd/picasso/gpio.c index b7e1a45..fe7d14b 100644 --- a/src/soc/amd/picasso/gpio.c +++ b/src/soc/amd/picasso/gpio.c @@ -1,10 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <amdblocks/gpio_banks.h> -#include <amdblocks/acpimmio.h> -#include <amdblocks/smi.h> #include <soc/gpio.h> -#include <soc/smi.h> #include <types.h>
static const struct soc_amd_event gpio_event_table[] = { @@ -39,10 +36,3 @@ *table = gpio_event_table; *items = ARRAY_SIZE(gpio_event_table); } - -void soc_gpio_hook(uint8_t gpio, uint8_t mux) -{ - /* Always program Gevent when WAKE_L_AGPIO2 is configured as WAKE_L */ - if ((gpio == 2) && !(mux & AMD_GPIO_MUX_MASK)) - soc_route_sci(GPIO_2_EVENT); -} diff --git a/src/soc/amd/stoneyridge/gpio.c b/src/soc/amd/stoneyridge/gpio.c index d30fa63..90c23a2 100644 --- a/src/soc/amd/stoneyridge/gpio.c +++ b/src/soc/amd/stoneyridge/gpio.c @@ -1,10 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <amdblocks/gpio_banks.h> -#include <amdblocks/acpimmio.h> -#include <amdblocks/smi.h> #include <soc/gpio.h> -#include <soc/smi.h> #include <types.h>
static const struct soc_amd_event gpio_event_table[] = { @@ -39,10 +36,3 @@ *table = gpio_event_table; *items = ARRAY_SIZE(gpio_event_table); } - -void soc_gpio_hook(uint8_t gpio, uint8_t mux) -{ - /* Always program Gevent when WAKE_L_AGPIO2 is configured as WAKE_L */ - if ((gpio == 2) && !(mux & AMD_GPIO_MUX_MASK)) - soc_route_sci(GPIO_2_EVENT); -}
-
Felix Held (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)