Change in ...coreboot[master]: mb/google/hatch: Configure GPIOs again after FSP-S is done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31236

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

mb/google/hatch: Configure GPIOs again after FSP-S is done

FSP-S is currently configuring GPIOs that it should not. This results
in issues like host unable to receive TPM interrupts as the pad for
the interrupt is re-configured as something else.

Until FSP-S is fixed, this change adds a workaround by reconfiguring
GPIOs after FSP-S is run.

BUG=b:123721147
BRANCH=None
TEST=Verified that there are no TPM IRQ timeouts in boot log.

Change-Id: Ib197529a730f0142152c94ddf4a2b005bae123f5
Signed-off-by: Furquan Shaikh 
---
M src/mainboard/google/hatch/ramstage.c
1 file changed, 13 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/31236/1

diff --git a/src/mainboard/google/hatch/ramstage.c b/src/mainboard/google/hatch/ramstage.c
index b8e80e7..e2fe048 100644
--- a/src/mainboard/google/hatch/ramstage.c
+++ b/src/mainboard/google/hatch/ramstage.c
@@ -20,7 +20,7 @@
 #include <variant/gpio.h>
 #include <vendorcode/google/chromeos/chromeos.h>

-void mainboard_silicon_init_params(FSP_S_CONFIG *params)
+static void mainboard_configure_gpios(void)
 {
    const struct pad_config *gpio_table;
    size_t num_gpios;
@@ -29,10 +29,22 @@
    gpio_configure_pads(gpio_table, num_gpios);
 }

+void mainboard_silicon_init_params(FSP_S_CONFIG *params)
+{
+	mainboard_configure_gpios();
+}
+
+static void mainboard_init(struct device *dev)
+{
+	/* Workaround FSP issue by reprogramming GPIOs after FSP-S */
+	mainboard_configure_gpios();
+}
+
 static void mainboard_enable(struct device *dev)
 {
    mainboard_ec_init();

+	dev->ops->init = mainboard_init;
    dev->ops->acpi_inject_dsdt_generator = chromeos_dsdt_generator;
 }

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31236/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31236/1//COMMIT_MSG@8 
PS1, Line 8: 
           : FSP-S
Do you have an exact version?

https://review.coreboot.org/#/c/31236/1/src/mainboard/google/hatch/ramstage.... 
File src/mainboard/google/hatch/ramstage.c:

https://review.coreboot.org/#/c/31236/1/src/mainboard/google/hatch/ramstage.... 
PS1, Line 39: Workaround
Work around

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1:

Should we push this into cnl/cfl ? There are other boards affected.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Should we push this into cnl/cfl ? There are other boards affected.

Other boards currently have the same work around applied. One reason I did not put this in cnl/cfl common code is because it will have to make a copy of the gpio table or add another callback from SoC to mainboard. But I can look into that since I am not sure when exactly this will be fixed in FSP.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Should we push this into cnl/cfl ? There are other boards affected.
>
> Other boards currently have the same work around applied. One reason I did not put this in cnl/cfl common code is because it will have to make a copy of the gpio table or add another callback from SoC to mainboard. But I can look into that since I am not sure when exactly this will be fixed in FSP.

Makes sense. We don't have to make a whole copy. We just need to save the reference and count, but if it's easier to carry the mainboard workaround in a duplicated form we can. Should be marked TODO: this is a hacky workaround that needs to be backed out.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > Should we push this into cnl/cfl ? There are other boards affected.
> >
> > Other boards currently have the same work around applied. One reason I did not put this in cnl/cfl common code is because it will have to make a copy of the gpio table or add another callback from SoC to mainboard. But I can look into that since I am not sure when exactly this will be fixed in FSP.
>
> Makes sense. We don't have to make a whole copy. We just need to save the reference and count, but if it's easier to carry the mainboard workaround in a duplicated form we can. Should be marked TODO: this is a hacky workaround that needs to be backed out.

I have added this change to CNL. Let me know what you think: https://review.coreboot.org/c/coreboot/+/31250/

[Furquan Shaikh (Code Review)]

Furquan Shaikh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31236 )

Change subject: mb/google/hatch: Configure GPIOs again after FSP-S is done
......................................................................

Abandoned

Fixed differently here: https://review.coreboot.org/c/coreboot/+/31250