Change in coreboot[master]: soc/amd/common: Refactor program_gpios() for overrides

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

soc/amd/common: Refactor program_gpios() for overrides

Some GPIO listed in override_cfg may not be programmed.

Change-Id: Ia8e47b2a278a1887db5406c1f863ddafa6a68675
Signed-off-by: Kyösti Mälkki 
---
M src/soc/amd/common/block/gpio_banks/gpio.c
1 file changed, 20 insertions(+), 36 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/43050/1

diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c
index b52d7f1..1019d79 100644
--- a/src/soc/amd/common/block/gpio_banks/gpio.c
+++ b/src/soc/amd/common/block/gpio_banks/gpio.c
@@ -189,7 +189,16 @@

void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size)
 {
-	size_t i;
+	return gpio_configure_pads_with_override(gpio_list_ptr, size, NULL, 0);
+}
+
+void gpio_configure_pads_with_override(const struct soc_amd_gpio *base_cfg,
+					size_t base_num_pads,
+					const struct soc_amd_gpio *override_cfg,
+					size_t override_num_pads)
+{
+	const struct soc_amd_gpio *c;
+	size_t i, j;

/*
     * Disable blocking wake/interrupt status generation while updating
@@ -203,8 +212,16 @@
     */
    master_switch_clr(GPIO_MASK_STS_EN | GPIO_INTERRUPT_EN);

-	for (i = 0; i < size; i++)
-		set_single_gpio(&gpio_list_ptr[i]);
+	for (i = 0; i < base_num_pads; i++) {
+		c = &base_cfg[i];
+		for (j = 0; override_cfg && j < override_num_pads; j++) {
+			if (c->gpio == override_cfg[j].gpio) {
+				c = &override_cfg[j];
+				break;
+			}
+		}
+		set_single_gpio(c);
+	}

/*
     * Re-enable interrupt status generation.
@@ -229,36 +246,3 @@

return 0;
 }
-
-/*
- * This function checks to see if there is an override config present for the
- * provided pad_config. If no override config is present, then the input config
- * is returned. Else, it returns the override config.
- */
-static const struct soc_amd_gpio *gpio_get_config(const struct soc_amd_gpio *c,
-				const struct soc_amd_gpio *override_cfg_table,
-				size_t num)
-{
-	size_t i;
-	if (override_cfg_table == NULL)
-		return c;
-	for (i = 0; i < num; i++) {
-		if (c->gpio == override_cfg_table[i].gpio)
-			return override_cfg_table + i;
-	}
-	return c;
-}
-void gpio_configure_pads_with_override(const struct soc_amd_gpio *base_cfg,
-					size_t base_num_pads,
-					const struct soc_amd_gpio *override_cfg,
-					size_t override_num_pads)
-{
-	size_t i;
-	const struct soc_amd_gpio *c;
-
-	for (i = 0; i < base_num_pads; i++) {
-		c = gpio_get_config(base_cfg + i, override_cfg,
-				override_num_pads);
-		program_gpios(c, 1);
-	}
-}

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

Abandoned

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Raul Rangel, Felix Held.
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

Restored

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Raul Rangel, Felix Held.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3: 
ping

[Felix Held (Code Review)]

Attention is currently required from: Raul Rangel, Kyösti Mälkki.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3:
> ping

finally got around to have a look and I like the general direction this patch, since it avoids unneeded master_switch_set/master_switch_clear sequence between each GPIO configuration in the case of gpio_configure_pads_with_override. I don't think that it solves the possible issue of a GPIO in the override being ignored when there's no corresponding base GPIO configuration. From what I remember it's mostly intended behavior, but it would be good to have at least some function that can check for this and then print some warning. This however can't be done as a part of the gpio_configure_pads_with_override call, since at least in bootblock that call is done before the console_init call. Haven't checked for the later stages.

if you're ok with that, I can take over this patch and port it forward to the current top of tree.

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Raul Rangel, Felix Held.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common: Refactor program_gpios() for overrides
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3:
> finally got around to have a look and I like the general direction this patch, since it avoids unnee […]

Thanks, please take over.

I don't know about the intended behaviour or motivation behind it. If you need a new GPIO implemented for a variant, you need to add entry in the baseboard GPIOs too and thus introduce change in other veriants too.

Change in coreboot[master]: [UNTESTED] soc/amd/common/block/gpio_banks: Rework GPIO pad configura...

[Felix Held (Code Review)]

Attention is currently required from: Raul Rangel, 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/+/43050 )

Change subject: [UNTESTED] soc/amd/common/block/gpio_banks: Rework GPIO pad configuration
......................................................................

[UNTESTED] soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

Before this patch, gpio_configure_pads_with_override called
program_gpios once for each GPIO that needed to be configured which
resulted in base_num_pads - 1 unneeded master_switch_set/
master_switch_clr sequences for the gpio_configure_pads_with_override
call. Instead implement gpio_configure_pads_with_override as the more
generic function and program_gpios as a special case of that which
passes an empty override configuration and override pad number to
gpio_configure_pads_with_override.

Change-Id: Ia8e47b2a278a1887db5406c1f863ddafa6a68675
Signed-off-by: Kyösti Mälkki 
Signed-off-by: Felix Held 
---
M src/soc/amd/common/block/gpio_banks/gpio.c
1 file changed, 23 insertions(+), 39 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/43050/4

Change in coreboot[master]: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

[Felix Held (Code Review)]

Attention is currently required from: Raul Rangel, 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/+/43050 )

Change subject: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration
......................................................................

soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

Before this patch, gpio_configure_pads_with_override called
program_gpios once for each GPIO that needed to be configured which
resulted in base_num_pads - 1 unneeded master_switch_set/
master_switch_clr sequences for the gpio_configure_pads_with_override
call. Instead implement gpio_configure_pads_with_override as the more
generic function and program_gpios as a special case of that which
passes an empty override configuration and override pad number to
gpio_configure_pads_with_override.

TEST=GPIO configuration and multiplexer register values are the same for
all GPIOs on google/guybrush right before jumping to the payload before
and after the patch.

Change-Id: Ia8e47b2a278a1887db5406c1f863ddafa6a68675
Signed-off-by: Kyösti Mälkki 
Signed-off-by: Felix Held 
---
M src/soc/amd/common/block/gpio_banks/gpio.c
1 file changed, 23 insertions(+), 39 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/43050/5

Change in coreboot[master]: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

[Felix Held (Code Review)]

Attention is currently required from: Raul Rangel, Kyösti Mälkki.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration
......................................................................

Patch Set 5:

(1 comment)

Patchset:

PS3:
> Thanks, please take over. […]

moved the discussion about the intended behavior in regard to GPIO settings being ignored when they are only in the override configuration and not in the base configuration to CB:57789 since this patch does something else

Change in coreboot[master]: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

[Raul Rangel (Code Review)]

Attention is currently required from: Kyösti Mälkki, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43050 )

Change subject: soc/amd/common/block/gpio_banks: Rework GPIO pad configuration
......................................................................

soc/amd/common/block/gpio_banks: Rework GPIO pad configuration

Before this patch, gpio_configure_pads_with_override called
program_gpios once for each GPIO that needed to be configured which
resulted in base_num_pads - 1 unneeded master_switch_set/
master_switch_clr sequences for the gpio_configure_pads_with_override
call. Instead implement gpio_configure_pads_with_override as the more
generic function and program_gpios as a special case of that which
passes an empty override configuration and override pad number to
gpio_configure_pads_with_override.

TEST=GPIO configuration and multiplexer register values are the same for
all GPIOs on google/guybrush right before jumping to the payload before
and after the patch.

Change-Id: Ia8e47b2a278a1887db5406c1f863ddafa6a68675
Signed-off-by: Kyösti Mälkki 
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43050
Tested-by: build bot (Jenkins) 
Reviewed-by: Raul Rangel 
---
M src/soc/amd/common/block/gpio_banks/gpio.c
1 file changed, 23 insertions(+), 39 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Raul Rangel: 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 fa18f61..31b1a52 100644
--- a/src/soc/amd/common/block/gpio_banks/gpio.c
+++ b/src/soc/amd/common/block/gpio_banks/gpio.c
@@ -264,11 +264,15 @@
    }
 }

-void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size)
+void gpio_configure_pads_with_override(const struct soc_amd_gpio *base_cfg,
+					size_t base_num_pads,
+					const struct soc_amd_gpio *override_cfg,
+					size_t override_num_pads)
 {
-	size_t i;
+	const struct soc_amd_gpio *c;
+	size_t i, j;

-	if (!gpio_list_ptr || !size)
+	if (!base_cfg || !base_num_pads)
    	return;

/*
@@ -283,8 +287,17 @@
     */
    master_switch_clr(GPIO_MASK_STS_EN | GPIO_INTERRUPT_EN);

-	for (i = 0; i < size; i++)
-		set_single_gpio(&gpio_list_ptr[i]);
+	for (i = 0; i < base_num_pads; i++) {
+		c = &base_cfg[i];
+		/* Check if override exist for GPIO from the base configuration */
+		for (j = 0; override_cfg && j < override_num_pads; j++) {
+			if (c->gpio == override_cfg[j].gpio) {
+				c = &override_cfg[j];
+				break;
+			}
+		}
+		set_single_gpio(c);
+	}

/*
     * Re-enable interrupt status generation.
@@ -296,6 +309,11 @@
    master_switch_set(GPIO_INTERRUPT_EN);
 }

+void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size)
+{
+	gpio_configure_pads_with_override(gpio_list_ptr, size, NULL, 0);
+}
+
 int gpio_interrupt_status(gpio_t gpio)
 {
    uint32_t reg = gpio_read32(gpio);
@@ -310,40 +328,6 @@
    return 0;
 }

-/*
- * This function checks to see if there is an override config present for the
- * provided pad_config. If no override config is present, then the input config
- * is returned. Else, it returns the override config.
- */
-static const struct soc_amd_gpio *gpio_get_config(const struct soc_amd_gpio *c,
-				const struct soc_amd_gpio *override_cfg_table,
-				size_t num)
-{
-	size_t i;
-	if (override_cfg_table == NULL)
-		return c;
-	for (i = 0; i < num; i++) {
-		if (c->gpio == override_cfg_table[i].gpio)
-			return override_cfg_table + i;
-	}
-	return c;
-}
-
-void gpio_configure_pads_with_override(const struct soc_amd_gpio *base_cfg,
-					size_t base_num_pads,
-					const struct soc_amd_gpio *override_cfg,
-					size_t override_num_pads)
-{
-	size_t i;
-	const struct soc_amd_gpio *c;
-
-	for (i = 0; i < base_num_pads; i++) {
-		c = gpio_get_config(base_cfg + i, override_cfg,
-				override_num_pads);
-		program_gpios(c, 1);
-	}
-}
-
 static void check_and_add_wake_gpio(gpio_t begin, gpio_t end, struct gpio_wake_state *state)
 {
    gpio_t i;