Change in coreboot[master]: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in...
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 29 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/1
diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c index ba50362..5874e89 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c @@ -13,7 +13,6 @@ /* PEN_POWER_EN - reset */ PAD_GPO(GPIO_5, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -47,7 +46,6 @@ /* EN_PWR_TOUCHPAD_PS2 - reset */ PAD_GPO(GPIO_6, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), 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 01aef2b..2191793 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -13,7 +13,6 @@ /* PEN_POWER_EN - reset */ PAD_GPO(GPIO_5, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -45,7 +44,6 @@ /* EN_PWR_TOUCHPAD_PS2 - reset */ PAD_GPO(GPIO_13, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -82,7 +80,6 @@ /* PEN_POWER_EN - Enabled*/ PAD_GPO(GPIO_5, HIGH), /* FPMCU_INT_L */ - PAD_GPI(GPIO_6, PULL_UP), PAD_WAKE(GPIO_6, PULL_UP, EDGE_LOW, S3_S4_S5), /* I2S_SDIN */ PAD_NF(GPIO_7, ACP_I2S_SDIN, PULL_NONE), diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index d7ac896..a8b11ba 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -21,17 +21,6 @@ return -1; }
-static void mem_read_write32(uint32_t *address, uint32_t value, uint32_t mask) -{ - uint32_t reg32; - - value &= mask; - reg32 = read32(address); - reg32 &= ~mask; - reg32 |= value; - write32(address, reg32); -} - static void program_smi(uint32_t flags, int gevent_num) { uint8_t level; @@ -181,7 +170,6 @@
void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { - uint32_t *gpio_ptr; uint32_t control, control_flags; uint8_t mux, index, gpio; int gevent_num; @@ -213,51 +201,23 @@ iomux_read8(gpio); /* Flush posted write */
soc_gpio_hook(gpio, mux); + gpio_write32(gpio, control);
- gpio_ptr = gpio_ctrl_ptr(gpio); + if (control_flags == 0) + continue;
- if (control_flags & GPIO_FLAG_SPECIAL_MASK) { - gevent_num = get_gpio_gevent(gpio, gev_tbl, gev_items); - if (gevent_num < 0) { - printk(BIOS_WARNING, "Warning: GPIO pin %d has" - " no associated gevent!\n", gpio); - continue; - } - switch (control_flags & GPIO_FLAG_SPECIAL_MASK) { - case GPIO_FLAG_DEBOUNCE: - mem_read_write32(gpio_ptr, control, - GPIO_DEBOUNCE_MASK); - break; - case GPIO_FLAG_WAKE: - mem_read_write32(gpio_ptr, control, - INT_WAKE_MASK); - break; - case GPIO_FLAG_INT: - mem_read_write32(gpio_ptr, control, - AMD_GPIO_CONTROL_MASK); - break; - case GPIO_FLAG_SMI: - mem_read_write32(gpio_ptr, control, - INT_SCI_SMI_MASK); + gevent_num = get_gpio_gevent(gpio, gev_tbl, gev_items); + if (gevent_num < 0) { + printk(BIOS_WARNING, "Warning: GPIO pin %d has" + " no associated gevent!\n", gpio); + continue; + }
- program_smi(control_flags, gevent_num); - break; - case GPIO_FLAG_SCI: - mem_read_write32(gpio_ptr, control, - INT_SCI_SMI_MASK); - - fill_sci_trigger(control_flags, gevent_num, &sci_trigger_cfg); - - soc_route_sci(gevent_num); - break; - default: - printk(BIOS_WARNING, "Error, flags 0x%08x\n", - control_flags); - break; - } - } else { - mem_read_write32(gpio_ptr, control, - AMD_GPIO_CONTROL_MASK); + if (control_flags & GPIO_FLAG_SMI) { + program_smi(control_flags, gevent_num); + } else if (control_flags & GPIO_FLAG_SCI) { + fill_sci_trigger(control_flags, gevent_num, &sci_trigger_cfg); + soc_route_sci(gevent_num); } }
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 96dcc5b..584f0e1 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -116,7 +116,6 @@ #define GPIO_PULL_PULL_DOWN GPIO_PULLDOWN_ENABLE #define GPIO_PULL_PULL_NONE 0
-#define AMD_GPIO_CONTROL_MASK 0x00f4ff00 #define AMD_GPIO_MUX_MASK 0x03
@@ -132,10 +131,6 @@ #define GPIO_FLAG_EVENT_ACTIVE_MASK (1 << 1) #define GPIO_FLAG_SCI (1 << 2) #define GPIO_FLAG_SMI (1 << 3) -#define GPIO_FLAG_DEBOUNCE (1 << 4) -#define GPIO_FLAG_WAKE (1 << 5) -#define GPIO_FLAG_INT (1 << 6) -#define GPIO_FLAG_SPECIAL_MASK (0x1f << 2)
/* Trigger configuration for GPIO SCI/SMI events. */ #define GPIO_FLAG_EVENT_TRIGGER_LEVEL_HIGH (GPIO_FLAG_EVENT_TRIGGER_LEVEL | \ @@ -167,11 +162,6 @@ return (flags & GPIO_FLAG_EVENT_ACTIVE_MASK) == GPIO_FLAG_EVENT_ACTIVE_LOW; }
-#define GPIO_DEBOUNCE_MASK 0x000000ff -#define INT_TRIGGER_MASK 0x00000700 -#define INT_WAKE_MASK 0x0000e700 -#define INT_SCI_SMI_MASK 0x00f40000 - #define DEB_GLITCH_SHIFT 5 #define DEB_GLITCH_LOW 1 #define DEB_GLITCH_HIGH 2 @@ -205,11 +195,8 @@ #define GPIO_WAKE_S3_S4_S5 (GPIO_WAKE_S3 | GPIO_WAKE_S4_S5)
/* - * Several macros are available to declare programming of GPIO pins, and if - * needed, more than 1 macro can be used for any pin. However, some macros - * will have no effect if combined. For example debounce only affects input - * or one of the interrupts. Some macros should not be combined, such as SMI - * and regular interrupt. The defined macros and their parameters are: + * Several macros are available to declare programming of GPIO pins. The defined macros and + * their parameters are: * PAD_NF Define native alternate function for the pin. * pin the pin to be programmed * function the native function @@ -228,19 +215,19 @@ * PAD_SCI The pin is a SCI source * pin the pin to be programmed * pull pull up, pull down or no pull - * trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH + * event trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH * PAD_SMI The pin is a SMI source * pin the pin to be programmed * pull pull up, pull down or no pull - * trigger LEVEL_LOW, LEVEL_HIGH + * event trigger LEVEL_LOW, LEVEL_HIGH * PAD_WAKE The pin can wake, use after PAD_INT or PAD_SCI * pin the pin to be programmed * pull pull up, pull down or no pull * trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH, BOTH_EDGES * type S0i3, S3, S4_S5 or S4_S5 combinations (S0i3_S3 invalid) - * PAD_DEBOUNCE The input or interrupt will be debounced, invalid after - * PAD_NF + * PAD_DEBOUNCE The input or interrupt will be debounced * pin the pin to be programmed + * pull pull up, pull down or no pull * debounce_type preserve low glitch, preserve high glitch, no glitch * debounce_time the debounce time */ @@ -277,29 +264,31 @@ #define PAD_INT(pin, pull, trigger, action) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ PAD_PULL(pull) | PAD_TRIGGER(trigger) | PAD_INT_ENABLE(action), \ - GPIO_FLAG_INT) + 0)
/* SCI pad configuration */ #define PAD_SCI(pin, pull, trigger) \ - PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, PAD_PULL(pull), \ + PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ + PAD_PULL(pull) | PAD_TRIGGER(LEVEL_HIGH), \ PAD_FLAG_EVENT_TRIGGER(trigger) | GPIO_FLAG_SCI)
/* SMI pad configuration */ #define PAD_SMI(pin, pull, trigger) \ - PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, PAD_PULL(pull), \ + PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ + PAD_PULL(pull) | PAD_TRIGGER(LEVEL_HIGH), \ PAD_FLAG_EVENT_TRIGGER(trigger) | GPIO_FLAG_SMI)
/* WAKE pad configuration */ #define PAD_WAKE(pin, pull, trigger, type) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ PAD_PULL(pull) | PAD_TRIGGER(trigger) | PAD_WAKE_ENABLE(type), \ - GPIO_FLAG_WAKE) + 0)
/* pin debounce configuration */ -#define PAD_DEBOUNCE(pin, type, time) \ +#define PAD_DEBOUNCE(pin, pull, type, time) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ - PAD_DEBOUNCE_CONFIG(type) | PAD_DEBOUNCE_CONFIG(time), \ - GPIO_FLAG_DEBOUNCE) + PAD_PULL(pull) | PAD_DEBOUNCE_CONFIG(type) | PAD_DEBOUNCE_CONFIG(time), \ + 0)
typedef uint32_t gpio_t;
Furquan Shaikh has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 29 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 2: Code-Review+1
Hello build bot (Jenkins), Raul Rangel, Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42875
to look at the new patch set (#3).
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 29 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 3: Code-Review+1
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 3: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42875/3/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42875/3/src/soc/amd/common/block/gp... PS3, Line 204: control Actually, doesn't this clear the pad strength?
Hello build bot (Jenkins), Raul Rangel, Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42875
to look at the new patch set (#4).
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 43 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42875/3/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42875/3/src/soc/amd/common/block/gp... PS3, Line 204: control
Actually, doesn't this clear the pad strength?
Reworked to preserve the pad strength bits.
Hello build bot (Jenkins), Raul Rangel, Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42875
to look at the new patch set (#5).
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 44 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42875/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42875/5/src/soc/amd/common/block/gp... PS5, Line 218: " no associated gevent!\n", gpio); make this one line for the format string?
Hello build bot (Jenkins), Raul Rangel, Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42875
to look at the new patch set (#6).
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 44 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/42875/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42875/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42875/5/src/soc/amd/common/block/gp... PS5, Line 218: " no associated gevent!\n", gpio);
make this one line for the format string?
Done.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
Patch Set 6: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42875 )
Change subject: soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table ......................................................................
soc/amd/gpio, mb/{amd,google}: Configure pads using a single entry in GPIO configuration table
Currently, for Stoneyridge and Picasso mainboards, pads that are configured for SCI/SMI/WAKE need to have multiple entries in the configuration table - one for PAD_GPI and other for the special configuration that is required. This requires a very specific ordering of pads within the table and is prone to errors because of conflicting params provided to the different entries for the same pad. This also does not work very well with the concept of override GPIOs where the entry in base table is overridden with the first matched entry from the override table.
This change updates the way GPIO configuration is handled for special routing like SCI/SMI/WAKE/DEBOUNCE by setting the control field of soc_amd_gpio structure in the macros performing these configurations. Also, program_gpios() is updated to perform a write to GPIO control register instead of read-modify-write. This is because mainboard is expected to provide only a single configuration entry for each pad within a given table. Thus, there is no need to preserve earlier configuration.
Mainboards that were providing multiple entries for a single pad are updated accordingly.
BUG=b:159944426
Change-Id: I3364dc2982d66c4e33c2b4e6b0b97641ebea27f0 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42875 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.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 4 files changed, 44 insertions(+), 85 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c index ba50362..5874e89 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c @@ -13,7 +13,6 @@ /* PEN_POWER_EN - reset */ PAD_GPO(GPIO_5, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -47,7 +46,6 @@ /* EN_PWR_TOUCHPAD_PS2 - reset */ PAD_GPO(GPIO_6, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), 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 01aef2b..2191793 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -13,7 +13,6 @@ /* PEN_POWER_EN - reset */ PAD_GPO(GPIO_5, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -45,7 +44,6 @@ /* EN_PWR_TOUCHPAD_PS2 - reset */ PAD_GPO(GPIO_13, LOW), /* EC_FCH_WAKE_L */ - PAD_GPI(GPIO_24, PULL_UP), PAD_WAKE(GPIO_24, PULL_UP, EDGE_LOW, S3_S4_S5), /* PCIE_RST1_L - Variable timings (May remove) */ PAD_NF(GPIO_27, PCIE_RST1_L, PULL_NONE), @@ -82,7 +80,6 @@ /* PEN_POWER_EN - Enabled*/ PAD_GPO(GPIO_5, HIGH), /* FPMCU_INT_L */ - PAD_GPI(GPIO_6, PULL_UP), PAD_WAKE(GPIO_6, PULL_UP, EDGE_LOW, S3_S4_S5), /* I2S_SDIN */ PAD_NF(GPIO_7, ACP_I2S_SDIN, PULL_NONE), diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index caf07c0..883dcfa 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -21,17 +21,6 @@ return -1; }
-static void mem_read_write32(uint32_t *address, uint32_t value, uint32_t mask) -{ - uint32_t reg32; - - value &= mask; - reg32 = read32(address); - reg32 &= ~mask; - reg32 |= value; - write32(address, reg32); -} - static void program_smi(uint32_t flags, int gevent_num) { uint8_t level; @@ -187,7 +176,6 @@
void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { - uint32_t *gpio_ptr; uint32_t control, control_flags; uint8_t mux, index, gpio; int gevent_num; @@ -219,51 +207,23 @@ iomux_read8(gpio); /* Flush posted write */
soc_gpio_hook(gpio, mux); + __gpio_setbits32(gpio, PAD_CFG_MASK, control);
- gpio_ptr = gpio_ctrl_ptr(gpio); + if (control_flags == 0) + continue;
- if (control_flags & GPIO_FLAG_SPECIAL_MASK) { - gevent_num = get_gpio_gevent(gpio, gev_tbl, gev_items); - if (gevent_num < 0) { - printk(BIOS_WARNING, "Warning: GPIO pin %d has" - " no associated gevent!\n", gpio); - continue; - } - switch (control_flags & GPIO_FLAG_SPECIAL_MASK) { - case GPIO_FLAG_DEBOUNCE: - mem_read_write32(gpio_ptr, control, - GPIO_DEBOUNCE_MASK); - break; - case GPIO_FLAG_WAKE: - mem_read_write32(gpio_ptr, control, - INT_WAKE_MASK); - break; - case GPIO_FLAG_INT: - mem_read_write32(gpio_ptr, control, - AMD_GPIO_CONTROL_MASK); - break; - case GPIO_FLAG_SMI: - mem_read_write32(gpio_ptr, control, - INT_SCI_SMI_MASK); + gevent_num = get_gpio_gevent(gpio, gev_tbl, gev_items); + if (gevent_num < 0) { + printk(BIOS_WARNING, "Warning: GPIO pin %d has no associated gevent!\n", + gpio); + continue; + }
- program_smi(control_flags, gevent_num); - break; - case GPIO_FLAG_SCI: - mem_read_write32(gpio_ptr, control, - INT_SCI_SMI_MASK); - - fill_sci_trigger(control_flags, gevent_num, &sci_trigger_cfg); - - soc_route_sci(gevent_num); - break; - default: - printk(BIOS_WARNING, "Error, flags 0x%08x\n", - control_flags); - break; - } - } else { - mem_read_write32(gpio_ptr, control, - AMD_GPIO_CONTROL_MASK); + if (control_flags & GPIO_FLAG_SMI) { + program_smi(control_flags, gevent_num); + } else if (control_flags & GPIO_FLAG_SCI) { + fill_sci_trigger(control_flags, gevent_num, &sci_trigger_cfg); + soc_route_sci(gevent_num); } }
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 546d55a..f02c8e8 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -74,6 +74,7 @@
#define GPIO_INT_STATUS (1 << 28) #define GPIO_WAKE_STATUS (1 << 29) +#define GPIO_STATUS_MASK (3 << 28)
enum { GEVENT_0, @@ -118,7 +119,6 @@ #define GPIO_PULL_PULL_DOWN GPIO_PULLDOWN_ENABLE #define GPIO_PULL_PULL_NONE 0
-#define AMD_GPIO_CONTROL_MASK 0x00f4ff00 #define AMD_GPIO_MUX_MASK 0x03
@@ -134,10 +134,6 @@ #define GPIO_FLAG_EVENT_ACTIVE_MASK (1 << 1) #define GPIO_FLAG_SCI (1 << 2) #define GPIO_FLAG_SMI (1 << 3) -#define GPIO_FLAG_DEBOUNCE (1 << 4) -#define GPIO_FLAG_WAKE (1 << 5) -#define GPIO_FLAG_INT (1 << 6) -#define GPIO_FLAG_SPECIAL_MASK (0x1f << 2)
/* Trigger configuration for GPIO SCI/SMI events. */ #define GPIO_FLAG_EVENT_TRIGGER_LEVEL_HIGH (GPIO_FLAG_EVENT_TRIGGER_LEVEL | \ @@ -169,11 +165,6 @@ return (flags & GPIO_FLAG_EVENT_ACTIVE_MASK) == GPIO_FLAG_EVENT_ACTIVE_LOW; }
-#define GPIO_DEBOUNCE_MASK 0x000000ff -#define INT_TRIGGER_MASK 0x00000700 -#define INT_WAKE_MASK 0x0000e700 -#define INT_SCI_SMI_MASK 0x00f40000 - #define DEB_GLITCH_SHIFT 5 #define DEB_GLITCH_LOW 1 #define DEB_GLITCH_HIGH 2 @@ -200,18 +191,29 @@ #define GPIO_DEB_200mS (13 | GPIO_TIMEBASE_15560uS) #define GPIO_DEB_500mS (8 | GPIO_TIMEBASE_62440uS)
+#define GPIO_DEB_MASK 0xff + #define GPIO_WAKE_S0i3 (1 << 13) #define GPIO_WAKE_S3 (1 << 14) #define GPIO_WAKE_S4_S5 (1 << 15) #define GPIO_WAKE_S0i3_S4_S5 (GPIO_WAKE_S0i3 | GPIO_WAKE_S4_S5) #define GPIO_WAKE_S3_S4_S5 (GPIO_WAKE_S3 | GPIO_WAKE_S4_S5) +#define GPIO_WAKE_MASK (7 << 13)
/* - * Several macros are available to declare programming of GPIO pins, and if - * needed, more than 1 macro can be used for any pin. However, some macros - * will have no effect if combined. For example debounce only affects input - * or one of the interrupts. Some macros should not be combined, such as SMI - * and regular interrupt. The defined macros and their parameters are: + * Mask used to reset bits in GPIO control register when configuring pad using `program_gpios()` + * Bits that are preserved/untouched: + * - Reserved bits + * - Drive strength bits + * - Read only bits + */ +#define PAD_CFG_MASK (GPIO_DEB_MASK | GPIO_TRIGGER_MASK | GPIO_ACTIVE_MASK | \ + GPIO_INT_ENABLE_MASK | GPIO_WAKE_MASK | GPIO_PULL_MASK | \ + GPIO_OUTPUT_MASK | GPIO_STATUS_MASK) + +/* + * Several macros are available to declare programming of GPIO pins. The defined macros and + * their parameters are: * PAD_NF Define native alternate function for the pin. * pin the pin to be programmed * function the native function @@ -230,19 +232,19 @@ * PAD_SCI The pin is a SCI source * pin the pin to be programmed * pull pull up, pull down or no pull - * trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH + * event trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH * PAD_SMI The pin is a SMI source * pin the pin to be programmed * pull pull up, pull down or no pull - * trigger LEVEL_LOW, LEVEL_HIGH + * event trigger LEVEL_LOW, LEVEL_HIGH * PAD_WAKE The pin can wake, use after PAD_INT or PAD_SCI * pin the pin to be programmed * pull pull up, pull down or no pull * trigger LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, EDGE_HIGH, BOTH_EDGES * type S0i3, S3, S4_S5 or S4_S5 combinations (S0i3_S3 invalid) - * PAD_DEBOUNCE The input or interrupt will be debounced, invalid after - * PAD_NF + * PAD_DEBOUNCE The input or interrupt will be debounced * pin the pin to be programmed + * pull pull up, pull down or no pull * debounce_type preserve low glitch, preserve high glitch, no glitch * debounce_time the debounce time */ @@ -279,29 +281,31 @@ #define PAD_INT(pin, pull, trigger, action) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ PAD_PULL(pull) | PAD_TRIGGER(trigger) | PAD_INT_ENABLE(action), \ - GPIO_FLAG_INT) + 0)
/* SCI pad configuration */ #define PAD_SCI(pin, pull, trigger) \ - PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, PAD_PULL(pull), \ + PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ + PAD_PULL(pull) | PAD_TRIGGER(LEVEL_HIGH), \ PAD_FLAG_EVENT_TRIGGER(trigger) | GPIO_FLAG_SCI)
/* SMI pad configuration */ #define PAD_SMI(pin, pull, trigger) \ - PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, PAD_PULL(pull), \ + PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ + PAD_PULL(pull) | PAD_TRIGGER(LEVEL_HIGH), \ PAD_FLAG_EVENT_TRIGGER(trigger) | GPIO_FLAG_SMI)
/* WAKE pad configuration */ #define PAD_WAKE(pin, pull, trigger, type) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ PAD_PULL(pull) | PAD_TRIGGER(trigger) | PAD_WAKE_ENABLE(type), \ - GPIO_FLAG_WAKE) + 0)
/* pin debounce configuration */ -#define PAD_DEBOUNCE(pin, type, time) \ +#define PAD_DEBOUNCE(pin, pull, type, time) \ PAD_CFG_STRUCT(pin, pin ## _IOMUX_GPIOxx, \ - PAD_DEBOUNCE_CONFIG(type) | PAD_DEBOUNCE_CONFIG(time), \ - GPIO_FLAG_DEBOUNCE) + PAD_PULL(pull) | PAD_DEBOUNCE_CONFIG(type) | PAD_DEBOUNCE_CONFIG(time), \ + 0)
typedef uint32_t gpio_t;
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Raul Rangel (Code Review)