Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
soc/amd/common: Init interrupts without gevent
Previously, the code wouldn't initialize any of the special flags, including the interrupt flag, if the pin wasn't associated with a GEVENT.
BUG=b:158124527 TEST=Build & boot Trembyle with psp_verstage
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I65efde0a9d347db77c6518c2b988eb29718e778b --- M src/soc/amd/common/block/gpio_banks/gpio.c 1 file changed, 19 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/42809/1
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 2ebef8b..a5f7904 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -173,6 +173,17 @@ return gpio; }
+static int gevent_missing(uint8_t gpio, int gevent_num, const char *string) +{ + if (gevent_num < 0) { + printk(BIOS_WARNING, "Warning: GPIO pin %d has" + " no associated gevent!\n", gpio); + printk (BIOS_WARNING, "Not programming %s\n", string); + return 1; + } + return 0; +} + __weak void soc_gpio_hook(uint8_t gpio, uint8_t mux) {}
void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) @@ -211,14 +222,9 @@ iomux_read8(gpio); /* Flush posted write */
soc_gpio_hook(gpio, mux); - + printk(BIOS_DEBUG, "Setting GPIO: %d\n",gpio); if (control_flags & GPIO_SPECIAL_FLAG) { 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_SPECIAL_MASK) { case GPIO_DEBOUNCE_FLAG: __gpio_setbits32(gpio, GPIO_DEBOUNCE_MASK, control); @@ -233,6 +239,9 @@ /* Can't set SMI flags from PSP */ if (!can_set_smi_flags) break; + if (gevent_missing(gpio, gevent_num, "SMI Flag")) + break; + __gpio_setbits32(gpio, INT_SCI_SMI_MASK, control);
program_smi(control_flags & FLAGS_TRIGGER_MASK, gevent_num); @@ -241,6 +250,8 @@ /* Can't set SCI flags from PSP */ if (!can_set_smi_flags) break; + if (gevent_missing(gpio, gevent_num, "SCI Flag")) + break; __gpio_setbits32(gpio, INT_SCI_SMI_MASK, control);
fill_sci_trigger(control_flags & FLAGS_TRIGGER_MASK, gevent_num, @@ -267,7 +278,8 @@ master_switch_set(GPIO_INTERRUPT_EN);
/* Set all SCI trigger polarity (high/low) and level (edge/level). */ - set_sci_trigger(&sci_trigger_cfg); + if (can_set_smi_flags) + set_sci_trigger(&sci_trigger_cfg); }
int gpio_interrupt_status(gpio_t gpio)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 181: printk (BIOS_WARNING, "Not programming %s\n", string); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 225: printk(BIOS_DEBUG, "Setting GPIO: %d\n",gpio); space required after that ',' (ctx:VxV)
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 242: if (gevent_missing(gpio, gevent_num, "SMI Flag")) : break; We don't need a Gevent for SMIs. Gevent is an ACPI/SCI concept. So I think you can remove this.
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 281: can_set_smi_flags Hrmm, this change should be moved to the CL that adds can_set_smi_flags
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 242: if (gevent_missing(gpio, gevent_num, "SMI Flag")) : break;
We don't need a Gevent for SMIs. Gevent is an ACPI/SCI concept. So I think you can remove this.
Done
https://review.coreboot.org/c/coreboot/+/42809/1/src/soc/amd/common/block/gp... PS1, Line 281: can_set_smi_flags
Hrmm, this change should be moved to the CL that adds can_set_smi_flags
Done
Hello build bot (Jenkins), Raul Rangel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42809
to look at the new patch set (#2).
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
soc/amd/common: Init interrupts without gevent
Previously, the code wouldn't initialize any of the special flags, including the interrupt flag, if the pin wasn't associated with a GEVENT.
BUG=b:158124527 TEST=Build & boot Trembyle with psp_verstage
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I65efde0a9d347db77c6518c2b988eb29718e778b --- M src/soc/amd/common/block/gpio_banks/gpio.c 1 file changed, 13 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/42809/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... PS2, Line 250: if (gevent_missing(gpio, gevent_num, "SCI Flag")) Just move gevent_num = get_gpio_gevent(...) within this case and if < 0 break? No need to do it for every flag any longer? I can clean it up later if you want.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... PS2, Line 250: if (gevent_missing(gpio, gevent_num, "SCI Flag"))
Just move gevent_num = get_gpio_gevent(... […]
I guess I should still program the GPIO. This way a pin will still be configured instead of leaving it in an unknown state.
Hello build bot (Jenkins), Raul Rangel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42809
to look at the new patch set (#3).
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
soc/amd/common: Init interrupts without gevent
Previously, the code wouldn't initialize any of the special flags, including the interrupt flag, if the pin wasn't associated with a GEVENT.
BUG=b:158124527 TEST=Build & boot Trembyle with psp_verstage
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I65efde0a9d347db77c6518c2b988eb29718e778b --- M src/soc/amd/common/block/gpio_banks/gpio.c 1 file changed, 15 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/42809/3
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/2/src/soc/amd/common/block/gp... PS2, Line 244: gevent_num I tried moving the gevent down, but it's needed here, so I restored my earlier code that checks it for both SCI and SMI.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42809/3/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42809/3/src/soc/amd/common/block/gp... PS3, Line 235: gevent_missing I don't think this CL is required anymore. It's only changing the message.
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42809 )
Change subject: soc/amd/common: Init interrupts without gevent ......................................................................
Abandoned
No longer needed.
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Martin Roth (Code Review)
-
Raul Rangel (Code Review)