Lijian Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32367
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from 3V_TSP_S0_FUSE during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/1
diff --git a/src/mainboard/google/sarien/dsdt.asl b/src/mainboard/google/sarien/dsdt.asl index e5b0cca..09ffd91 100644 --- a/src/mainboard/google/sarien/dsdt.asl +++ b/src/mainboard/google/sarien/dsdt.asl @@ -39,6 +39,8 @@ { #include <soc/intel/cannonlake/acpi/northbridge.asl> #include <soc/intel/cannonlake/acpi/southbridge.asl> + /* Per board variant mainboard hooks. */ + #include <variant/acpi/mainboard.asl> } }
diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl new file mode 100644 index 0000000..2d6260b --- /dev/null +++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl @@ -0,0 +1,25 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define TS_PD GPP_E7 + +/* Method called from _PTS prior to enter sleep state */ +Method(MPTS, 1) { + /* Before enter S5 soft off state */ + If (Arg0 == 5) { + /* Clear touch screen pd pin to avoid leakage */ + CTXS(TS_PD) + } /*S5 soft off state*/ +} diff --git a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl new file mode 100644 index 0000000..2d6260b --- /dev/null +++ b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl @@ -0,0 +1,25 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define TS_PD GPP_E7 + +/* Method called from _PTS prior to enter sleep state */ +Method(MPTS, 1) { + /* Before enter S5 soft off state */ + If (Arg0 == 5) { + /* Clear touch screen pd pin to avoid leakage */ + CTXS(TS_PD) + } /*S5 soft off state*/ +}
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 2:
Any reason why this is not done in mainboard_smi_sleep? e.g. https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/eve/...
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 2:
Patch Set 2:
Any reason why this is not done in mainboard_smi_sleep? e.g. https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/eve/...
I think either way will be okay, make it in DSDT because eventually we want to get rid of smi in coreboot.
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#4).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from 3V_TSP_S0_FUSE during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/4
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 21: 5 I know we've been focusing on S0ix and S5 power, but should this also be done in S3?
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 24: * add a space after the *
Roy Mingi Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 21: 5
I know we've been focusing on S0ix and S5 power, but should this also be done in S3?
S3 is not a POR on both sarien and arcada.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 23: CTXS (TS_PD) Won't you be violating the power down specification by not asserting reset before disabling power?
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#5).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from 3V_TSP_S0_FUSE during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/5
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 21: 5
S3 is not a POR on both sarien and arcada.
S3 still need to be working as fail safe option?
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 23: CTXS (TS_PD)
Won't you be violating the power down specification by not asserting reset before disabling power?
@Roy, would you like to answer here?
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#6).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from 3V_TSP_S0_FUSE during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/6
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#8).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from 3V_TSP_S0_FUSE during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/8
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 24: *
add a space after the *
Done
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 21: 5
S3 still need to be working as fail safe option?
I don't know that it does, I was just curious if we should make it do the same thing just in case.
Roy Mingi Park has uploaded a new patch set (#9) to the change originally created by Lijian Zhao. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from TOUCH_SCREEN_PD# (GPP_E7 which is connected to RESET pin of Wacom controller) during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/9
Roy Mingi Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 9: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 23: CTXS (TS_PD)
@Roy, would you like to answer here?
Actually, TS_PD(GPP_E7) is RESET and leakage power was observed from RESET pin not from 3V_TSP_S0_FUSE(GPP_B21) as I updated the comment.
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#10).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from TOUCH_SCREEN_PD# (GPP_E7 which is connected to RESET pin of Wacom controller) during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/10
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/4/src/mainboard/google/sarien/variants... PS4, Line 23: CTXS (TS_PD)
Actually, TS_PD(GPP_E7) is RESET and leakage power was observed from RESET pin not from 3V_TSP_S0_FU […]
From the devicetree for sarien and arcada, I see only one GPIO being exported GPP_E7: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/sari....
So, probably this is fine.
BTW, I just realized that you already have a power resource exported for the touchscreen device, so the kernel should already be taking care of turning off power resource in S3 and S0ix. For S5, there was a bug and I had pushed a CL for 4.4 kernel but never got a chance to upstream it. Can you try this out without any changes in ASL to see if the touchscreen power is good: crrev.com/c/1577858
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
There is each CL for both arcada and sarien in which both GPP_E7 and GPP_B21 are being controlled in S0iX.
For arcarda, https://review.coreboot.org/c/coreboot/+/32311
For sarien, https://review.coreboot.org/c/coreboot/+/32330
Aah interesting. So, the same power resource would ideally work with crrev.com/c/1577858 in kernel even for S5.
So I shall continue to make the change?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
There is each CL for both arcada and sarien in which both GPP_E7 and GPP_B21 are being controlled in S0iX.
For arcarda, https://review.coreboot.org/c/coreboot/+/32311
For sarien, https://review.coreboot.org/c/coreboot/+/32330
Aah interesting. So, the same power resource would ideally work with crrev.com/c/1577858 in kernel even for S5.
So I shall continue to make the change?
I meant try out the kernel change(crrev.com/c/1577858) without the change in .asl to see if the touchscreen power looks good in S5.
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#12).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from TOUCH_SCREEN_PD# (GPP_E7 which is connected to RESET pin of Wacom controller) during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/dsdt.asl A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/12
Roy Mingi Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 12:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
There is each CL for both arcada and sarien in which both GPP_E7 and GPP_B21 are being controlled in S0iX.
For arcarda, https://review.coreboot.org/c/coreboot/+/32311
For sarien, https://review.coreboot.org/c/coreboot/+/32330
Aah interesting. So, the same power resource would ideally work with crrev.com/c/1577858 in kernel even for S5.
So I shall continue to make the change?
I meant try out the kernel change(crrev.com/c/1577858) without the change in .asl to see if the touchscreen power looks good in S5.
Hi Furquan,
I tested with your kernel change(crrev.com/c/1577858) wo/ the change in .asl. And it also fixed leakage power on RESET (GPP_E7) in S5.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
(1 comment)
There is each CL for both arcada and sarien in which both GPP_E7 and GPP_B21 are being controlled in S0iX.
For arcarda, https://review.coreboot.org/c/coreboot/+/32311
For sarien, https://review.coreboot.org/c/coreboot/+/32330
Aah interesting. So, the same power resource would ideally work with crrev.com/c/1577858 in kernel even for S5.
So I shall continue to make the change?
I meant try out the kernel change(crrev.com/c/1577858) without the change in .asl to see if the touchscreen power looks good in S5.
Hi Furquan,
I tested with your kernel change(crrev.com/c/1577858) wo/ the change in .asl. And it also fixed leakage power on RESET (GPP_E7) in S5.
Thanks for the confirmation Roy. I am preparing the CL for upstream review. I will let you know how it goes. If it gets accepted, we should not require any ASL changes.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
> Patch Set 11: > > (1 comment)
There is each CL for both arcada and sarien in which both GPP_E7 and GPP_B21 are being controlled in S0iX.
For arcarda, https://review.coreboot.org/c/coreboot/+/32311
For sarien, https://review.coreboot.org/c/coreboot/+/32330
Aah interesting. So, the same power resource would ideally work with crrev.com/c/1577858 in kernel even for S5.
So I shall continue to make the change?
I meant try out the kernel change(crrev.com/c/1577858) without the change in .asl to see if the touchscreen power looks good in S5.
Hi Furquan,
I tested with your kernel change(crrev.com/c/1577858) wo/ the change in .asl. And it also fixed leakage power on RESET (GPP_E7) in S5.
Thanks for the confirmation Roy. I am preparing the CL for upstream review. I will let you know how it goes. If it gets accepted, we should not require any ASL changes.
Okay, move the change to WIP first and looking forward to abandon that.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 12:
upstream doesn't seem to want to take this patch yet, can you update this and rebase now that we have MPTS/MWAK in the mainboard.asl?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 12:
Patch Set 12:
upstream doesn't seem to want to take this patch yet
...Furquan's kernel patch that is.
Hello EricR Lai, Roy Mingi Park, Duncan Laurie, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32367
to look at the new patch set (#13).
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from TOUCH_SCREEN_PD# (GPP_E7 which is connected to RESET pin of Wacom controller) during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c --- M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32367/13
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
Patch Set 12:
upstream doesn't seem to want to take this patch yet, can you update this and rebase now that we have MPTS/MWAK in the mainboard.asl?
Done
Roy Mingi Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13: Code-Review+1
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... PS13, Line 36: /* Clear touch screen pd pin to avoid leakage */ Check Arg0=5 first? Make sure it's going S5, otherwise S3 test will cause touch lose function and never come back.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... PS13, Line 36: /* Clear touch screen pd pin to avoid leakage */
Check Arg0=5 first? Make sure it's going S5, otherwise S3 test will cause touch lose function and ne […]
I think S3 path should have already asserted reset by following the _OFF sequence in ACPI.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... PS13, Line 36: /* Clear touch screen pd pin to avoid leakage */
I think S3 path should have already asserted reset by following the _OFF sequence in ACPI.
Oh yes.. I forget there is on/off when s3 be called. okay, never mind my comment.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... File src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl:
https://review.coreboot.org/#/c/32367/13/src/mainboard/google/sarien/variant... PS13, Line 36: /* Clear touch screen pd pin to avoid leakage */
Oh yes.. I forget there is on/off when s3 be called. okay, never mind my comment.
Original patch did check for S5 first, but later we decided to remove that check,
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13:
Patch Set 13:
(1 comment)
:) totally fine in this case. _on method will call in ACPI resume from s3.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
Patch Set 13: Code-Review+2
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32367 )
Change subject: mb/google/sarien: Fix s5 touchscreen power leakage ......................................................................
mb/google/sarien: Fix s5 touchscreen power leakage
Leakage power is observed from TOUCH_SCREEN_PD# (GPP_E7 which is connected to RESET pin of Wacom controller) during S5. To avoid leakage power, GPP_E7 needs to be turned off before S5 entry.
BUG=b:129899315 TEST=Measure leakage power in S5 from both Arcada and Sarien
Signed-off-by: Lijian Zhao lijian.zhao@intel.com Change-Id: Ie4229477b7149c0a75f4a8c6c7c453a37cc1c78c Reviewed-on: https://review.coreboot.org/c/coreboot/+/32367 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Roy Mingi Park roy.mingi.park@intel.com Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 2 files changed, 8 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Roy Mingi Park: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl index bc03696..41121d2 100644 --- a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl +++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl @@ -14,6 +14,7 @@ */
#define CAM_EN GPP_B11 /* Active low */ +#define TS_PD GPP_E7
/* Method called from LPIT prior to enter s0ix state */ Method (MS0X, 1) @@ -31,6 +32,9 @@ Method (MPTS, 1) { _SB.PCI0.LPCB.EC0.PTS (Arg0) + + /* Clear touch screen pd pin to avoid leakage */ + _SB.PCI0.CTXS (TS_PD) }
/* Method called from _WAK prior to wakeup */ diff --git a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl index bc03696..41121d2 100644 --- a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl +++ b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl @@ -14,6 +14,7 @@ */
#define CAM_EN GPP_B11 /* Active low */ +#define TS_PD GPP_E7
/* Method called from LPIT prior to enter s0ix state */ Method (MS0X, 1) @@ -31,6 +32,9 @@ Method (MPTS, 1) { _SB.PCI0.LPCB.EC0.PTS (Arg0) + + /* Clear touch screen pd pin to avoid leakage */ + _SB.PCI0.CTXS (TS_PD) }
/* Method called from _WAK prior to wakeup */
-
Duncan Laurie (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Lijian Zhao (Code Review)
-
Roy Mingi Park (Code Review)