Change in coreboot[master]: mb/google/zork: Power off fingerprint sensor on shutdown

List overview All Threads
Download

newer

older

Change in coreboot[master]: [WIP]...

Martin Roth (Code Review)

7 Nov 2020 7 Nov '20

3:31 a.m.

Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

mb/google/zork: Power off fingerprint sensor on shutdown

When the system shuts down, turn the fingerprint sensor off. This sets the GPIOs correctly for the next boot. The fingerprint sensor was previously left on, and was just powering down when the rails went low.

On suspend, the fingerprint sensor stays awake and puts itself in a low powerstate mode based on the SLP_Sx_L pin states.

BUG=b:171837716 TEST=Fingerprint sensor still works after S3, GPIO state on the boot following a shutdown is low. BRANCH=Zork

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I4b292181d67c08c20fa4c473b363cfe2f934bff0

Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47311/1

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 bf27622..389ff5b 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -337,8 +337,25 @@ PAD_GPO(GPIO_76, LOW), };

+static const struct soc_amd_gpio gpio_fp_shutdown_table[] = { + /* NVME_AUX_RESET_L */ + PAD_GPO(GPIO_40, LOW), + /* EN_PWR_CAMERA */ + PAD_GPO(GPIO_76, LOW), + + /* FPMCU_RST_L */ + PAD_GPO(GPIO_11, LOW), + /* EN_PWR_FP */ + PAD_GPO(GPIO_32, LOW), +}; + const __weak struct soc_amd_gpio *variant_sleep_gpio_table(size_t *size, int slp_typ) { + if (slp_typ == SLP_TYP_S5) { + *size = ARRAY_SIZE(gpio_fp_shutdown_table); + return gpio_fp_shutdown_table; + } + *size = ARRAY_SIZE(gpio_sleep_table); return gpio_sleep_table; }

Attachments:

attachment.htm (text/html — 4.3 KB)

Show replies by date

Martin Roth (Code Review)

7 Nov 7 Nov

6:35 a.m.

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47311

to look at the new patch set (#3).

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

mb/google/zork: Power off fingerprint sensor on shutdown

On suspend, the fingerprint sensor stays awake and puts itself in a low powerstate mode based on the SLP_Sx_L pin states.

BUG=b:171837716 TEST=Fingerprint sensor still works after S3, GPIO state on the boot following a shutdown is low. BRANCH=Zork

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47311/3

Furquan Shaikh (Code Review)

9 Nov 9 Nov

12:50 p.m.

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

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) { I think this can be handled on the boot-up path. Currently, you have a check to configure the pads as GPO low when the device reboots. Instead it can be done for all paths which are not resuming from S3. Then you don't need the conditional here.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 3 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 09 Nov 2020 03:50:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Martin Roth (Code Review)

10 Nov 10 Nov

7:09 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

I think this can be handled on the boot-up path. […]

The request from the fingerprint team was to take these low at shutdown. If they decide that's not needed, I'm happy to change it.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 3 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 09 Nov 2020 22:09:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

13 Nov 13 Nov

2:06 a.m.

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

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 12 Nov 2020 17:06:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

2:48 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

The request from the fingerprint team was to take these low at shutdown. […]

The fingerprint team has said that this shutdown is desired, although not required. Since this matches what's being done on the intel board, I think it makes sense to leave it in.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 12 Nov 2020 17:48:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

2:53 a.m.

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

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

The fingerprint team has said that this shutdown is desired, although not required. […]

Quick question: Is there external PD on the GPIO_11 signal? If yes, then you don't need this setting at all. All GPIOs are reset on any kind of SoC reset. It guarantees that the pad state is set to default. So, as long as there is an external strong PD on the EN_PWR_FP pin, this configuration is not really required.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 12 Nov 2020 17:53:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Martin Roth (Code Review)

5:09 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

Quick question: Is there external PD on the GPIO_11 signal? If yes, then you don't need this setting […]

No, there isn't a pulldown. There's actually a pull-up, but it's PP3300_FPMCU

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 12 Nov 2020 20:09:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Martin Roth martinroth@google.com Gerrit-MessageType: comment

Martin Roth (Code Review)

17 Nov 17 Nov

1:18 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

No, there isn't a pulldown. […]

What do you think Furquan?

I agree with you that this patch isn't critical as everything will lose power on a shutdown eventually. Personally, I'm inclined to merge it since the fingerprint guys would like it, but if you'd still rather we didn't, I'm fine with that too.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 16 Nov 2020 16:18:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

5:03 a.m.

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

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:

https://review.coreboot.org/c/coreboot/+/47311/3/src/mainboard/google/zork/v... PS3, Line 354: if (slp_typ == SLP_TYP_S5) {

...

What do you think Furquan? […]

Sorry, I missed posting an update here last week. I talked to Michael and he confirmed that zork devices have external pull-downs to ensure that the signals are driven low in case of reset (where the pads go back to their default state). I don't want to block this change from going in if it is required, but I think it would be good to evaluate if the weak pull down serves the purpose of ensuring that the signal is pulled low on reset. It also gets rid of the requirement of configuring the pads as GPO driven low early in bootblock. Can you please check on that and fix the flow as a follow-up if required.

Marking as resolved to continue landing this patch for now.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 8 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 16 Nov 2020 20:03:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Paul Fagerburg (Code Review)

18 Nov 18 Nov

1:54 a.m.

Paul Fagerburg has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47311 )

Change subject: mb/google/zork: Power off fingerprint sensor on shutdown ......................................................................

mb/google/zork: Power off fingerprint sensor on shutdown

On suspend, the fingerprint sensor stays awake and puts itself in a low powerstate mode based on the SLP_Sx_L pin states.

BUG=b:171837716 TEST=Fingerprint sensor still works after S3, GPIO state on the boot following a shutdown is low. BRANCH=Zork

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47311 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 17 insertions(+), 0 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved

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 9e980db..14b85b0 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -350,8 +350,25 @@ PAD_GPO(GPIO_76, LOW), };

-- To view, visit https://review.coreboot.org/c/coreboot/+/47311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3837b58372d8f4a504535e76bd21c667d68f8995 Gerrit-Change-Number: 47311 Gerrit-PatchSet: 9 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1420

days inactive

1431

days old

coreboot-gerrit@coreboot.org

10 comments

3 participants

tags (0)

participants (3)

Furquan Shaikh (Code Review)
Martin Roth (Code Review)
Paul Fagerburg (Code Review)