Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
mb/google/zork: Add finalize GPIO routine
The fingerprint GPIOs need to be brought out of reset at the end of coreboot's boot sequence. This routine allows that to happen.
BUG=b:171837716 TEST=Boot, verify fingerprint works BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I11d14de44caa370f06b8da1277c8d88a7b3437bb
Change-Id: Iee178cc98259b490424c600c2ea6e55a2095670e --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c 6 files changed, 62 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/47310/1
diff --git a/src/mainboard/google/zork/mainboard.c b/src/mainboard/google/zork/mainboard.c index 9503d37..15957af 100644 --- a/src/mainboard/google/zork/mainboard.c +++ b/src/mainboard/google/zork/mainboard.c @@ -247,12 +247,13 @@
gnvs = acpi_get_gnvs();
- if (gnvs) { gnvs->tmps = CTL_TDP_SENSOR_ID; gnvs->tcrt = CRITICAL_TEMPERATURE; gnvs->tpsv = PASSIVE_TEMPERATURE; } + + finalize_gpios(); }
struct chip_operations mainboard_ops = { 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 e538aef..d5f8ed4 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c @@ -291,6 +291,10 @@ wifi_power_reset_configure_pre_v3(); }
+const __weak void finalize_gpios(void) +{ +} + const __weak struct soc_amd_gpio *variant_poweron_gpio_table(size_t *size, int slp_typ) { *size = 0; 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 74e7ed2..bf27622 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -297,6 +297,11 @@ wifi_power_reset_configure_pre_v3(); }
+ +__weak void finalize_gpios(void) +{ +} + static const struct soc_amd_gpio gpio_fingerprint_poweron_table[] = { /* FPMCU_RST_L */ PAD_GPO(GPIO_11, LOW), diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h index 61c8b3c..643fe39 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h @@ -39,6 +39,9 @@ /* Determine whether to init fingerprint sensor or not */ int variant_init_fingerprint(void);
+/* Program any required GPIOs at the finalize phase */ +void finalize_gpios(void); + /* Modify devictree settings during ramstage. */ void variant_devtree_update(void); /* Update audio configuration in devicetree during ramstage. */ diff --git a/src/mainboard/google/zork/variants/berknip/gpio.c b/src/mainboard/google/zork/variants/berknip/gpio.c index ae2d02a..a4d5a64 100644 --- a/src/mainboard/google/zork/variants/berknip/gpio.c +++ b/src/mainboard/google/zork/variants/berknip/gpio.c @@ -3,6 +3,7 @@ #include <baseboard/gpio.h> #include <baseboard/variants.h> #include <boardid.h> +#include <delay.h> #include <gpio.h> #include <soc/gpio.h> #include <ec/google/chromeec/ec.h> @@ -52,6 +53,29 @@ PAD_NC(GPIO_5), };
+void finalize_gpios(void) +{ + uint32_t board_version; + + if (variant_init_fingerprint()) { + + if (google_chromeec_cbi_get_board_version(&board_version)) + board_version = 1; + + /* Berknip board versions prior to v4 need an extra delay here */ + if (board_version <= 3) { + mdelay(550); + } + /* + * Enable the FPMCU by enabling EN_PWR_FP, then bringing it out + * of reset by setting FPMCU_RST_L high 3ms later. + */ + gpio_set(GPIO_32, 1); + mdelay(3); + gpio_set(GPIO_11, 1); + } +} + const struct soc_amd_gpio *variant_override_gpio_table(size_t *size) { uint32_t board_version; diff --git a/src/mainboard/google/zork/variants/morphius/gpio.c b/src/mainboard/google/zork/variants/morphius/gpio.c index 9b36e37..14211b2 100644 --- a/src/mainboard/google/zork/variants/morphius/gpio.c +++ b/src/mainboard/google/zork/variants/morphius/gpio.c @@ -3,6 +3,7 @@ #include <baseboard/gpio.h> #include <baseboard/variants.h> #include <boardid.h> +#include <delay.h> #include <gpio.h> #include <soc/gpio.h> #include <ec/google/chromeec/ec.h> @@ -86,3 +87,26 @@ *size = 0; return NULL; } + +/* Assume all SKUS have a fingerprint sensor */ + +void finalize_gpios(void) +{ + uint32_t board_version; + + if (google_chromeec_cbi_get_board_version(&board_version)) + board_version = 1; + + /* Morphius board versions prior to v5 need an extra delay here */ + if (board_version <= 4) { + mdelay(550); + } + /* + * Enable the FPMCU by enabling EN_PWR_FP, then bringing it out + * of reset by setting FPMCU_RST_L high 3ms later. + */ + gpio_set(GPIO_32, 1); + mdelay(3); + gpio_set(GPIO_11, 1); +} +
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47310/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/1/src/mainboard/google/zork/v... PS1, Line 66: if (board_version <= 3) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/47310/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/1/src/mainboard/google/zork/v... PS1, Line 101: if (board_version <= 4) { braces {} are not necessary for single statement blocks
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47310
to look at the new patch set (#2).
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
mb/google/zork: Add finalize GPIO routine
The fingerprint GPIOs need to be brought out of reset at the end of coreboot's boot sequence. This routine allows that to happen.
BUG=b:171837716 TEST=Boot, verify fingerprint works BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I11d14de44caa370f06b8da1277c8d88a7b3437bb
Change-Id: Iee178cc98259b490424c600c2ea6e55a2095670e --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c 6 files changed, 61 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/47310/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47310/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/2/src/mainboard/google/zork/v... PS2, Line 66: if (board_version <= 3) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/47310/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/2/src/mainboard/google/zork/v... PS2, Line 101: if (board_version <= 4) { braces {} are not necessary for single statement blocks
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47310
to look at the new patch set (#3).
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
mb/google/zork: Add finalize GPIO routine
The fingerprint GPIOs need to be brought out of reset at the end of coreboot's boot sequence. This routine allows that to happen.
BUG=b:171837716 TEST=Boot, verify fingerprint works BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Iee178cc98259b490424c600c2ea6e55a2095670e --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c 6 files changed, 61 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/47310/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... PS3, Line 65: need an extra delay here Why is this delay required?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... PS3, Line 65: need an extra delay here
Why is this delay required?
There was glitch-protection resistor with a value that was too high and kept the signal from going low. I didn't think that information was really needed here because it doesn't really matter to the code, but I can add this to the comment it if you think it's needed. Most of the time we don't really document why hardware changes were required to make changes in the code - that's mostly documented in the bug.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... PS3, Line 65: need an extra delay here
There was glitch-protection resistor with a value that was too high and kept the signal from going l […]
I think the change should be self-sufficient without a need to go back to bug to understand it. It is also helpful when partners add similar support for their own devices or in the future a new board wants to do something similar. Not everyone might have access to the bug.
About the glitch-protection resistor preventing the signal from going low - my reading of the bug is that it is related to not clearing of SRAM for the fingerprint MCU. And it is only for older hardware which will never be shipped. Since this is only for security reasons and does not have any functional impact, I think we don't really need to add the additional delay in coreboot. It simplifies the code path and the sequencing i.e. setting of GPIO_32, GPIO_11 and the 3 ms delay can be added directly in variants/baseboard/gpio_baseboard_tremblye.c without having to do this in each variant supporting fingerprint.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/47310/3/src/mainboard/google/zork/v... PS3, Line 65: need an extra delay here
I think the change should be self-sufficient without a need to go back to bug to understand it. […]
I've posted this comment on the bug. I was specifically asked to make sure we DID reset the fpmcu, even on old boards.
As discussed in the previous commit, I'm going to combine this commit with the previous one. I'm currently leaving this code as it is until it's confirmed which way to go.
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47310 )
Change subject: mb/google/zork: Add finalize GPIO routine ......................................................................
Abandoned
Combined with previous patch
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)