| Message ID | 20230418124953.3170028-2-fshao@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix Goodix touchscreen power leakage for MT8186 boards | expand |
Hi, On Tue, Apr 18, 2023 at 5:50 AM Fei Shao <fshao@chromium.org> wrote: > > We observed that on Chromebook device Steelix, if Goodix GT7375P > touchscreen is powered in suspend (because, for example, it connects to > an always-on regulator) and with the reset GPIO asserted, it will > introduce about 14mW power leakage. > > This property is used to indicate that the touchscreen is powered in > suspend. If it's set, the driver will stop asserting the reset GPIO in > power-down, and it will do it in power-up instead to ensure that the > state is always reset after resuming. > > Signed-off-by: Fei Shao <fshao@chromium.org> > --- > > Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > index ce18d7dadae2..942acb286d77 100644 > --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > @@ -43,6 +43,12 @@ properties: > itself as long as it allows the main board to make signals compatible > with what the touchscreen is expecting for its IO rails. > > + powered-in-suspend: > + description: > + This indicates that the touchscreen is powered in suspend, so the driver > + will not assert the reset GPIO in power-down to prevent power leakage. > + type: boolean This seems OK to me. The overall idea is that if we ever turn off the power to the touchscreen we have to assert reset (active low) before doing so, but we don't want to hold reset if we're not actually going to turn the power off because it causes the touchscreen to go into a high power state. This gets complicated if the power rail is always-on or shared with another device. Alternatives I could think of: 1. In the OS, monitor the regulator and see when it's about to go off and then assert reset. This is what I tried to do in previous patches but it got too messy in Linux. It also wasn't perfect since it's (theoretically) possible for a regulator to turn off outside of the scope of the OS (some PMICs will auto turn off rails when the main processor says it's off). 2. In the OS, peek to see if our regulator is marked "always on". This doesn't handle the shared rail case, of course. Also, regulators that are "always on" from the OS perspective could still (theoretically) get turned off at suspend time by the PMIC. 3. Don't even hook up the reset line and just leave the touchscreen out of reset all the time. This has the disadvantage that we can't reset the touchscreen if it gets into a bad state, which could be even more important if the touchscreen is on an always-on or shared rail. Given the above, the solution in ${SUBJECT} patch seems reasonable. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On 18/04/2023 14:49, Fei Shao wrote: > We observed that on Chromebook device Steelix, if Goodix GT7375P > touchscreen is powered in suspend (because, for example, it connects to > an always-on regulator) and with the reset GPIO asserted, it will > introduce about 14mW power leakage. > > This property is used to indicate that the touchscreen is powered in > suspend. If it's set, the driver will stop asserting the reset GPIO in > power-down, and it will do it in power-up instead to ensure that the > state is always reset after resuming. > > Signed-off-by: Fei Shao <fshao@chromium.org> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > > Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > index ce18d7dadae2..942acb286d77 100644 > --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > @@ -43,6 +43,12 @@ properties: > itself as long as it allows the main board to make signals compatible > with what the touchscreen is expecting for its IO rails. > > + powered-in-suspend: > + description: > + This indicates that the touchscreen is powered in suspend, so the driver > + will not assert the reset GPIO in power-down to prevent power leakage. > + type: boolean > + > required: > - compatible > - reg
Hi Fei, On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > We observed that on Chromebook device Steelix, if Goodix GT7375P > touchscreen is powered in suspend (because, for example, it connects to > an always-on regulator) and with the reset GPIO asserted, it will > introduce about 14mW power leakage. > > This property is used to indicate that the touchscreen is powered in > suspend. If it's set, the driver will stop asserting the reset GPIO in > power-down, and it will do it in power-up instead to ensure that the > state is always reset after resuming. > > Signed-off-by: Fei Shao <fshao@chromium.org> > --- This is an interesting problem; were you able to root-cause why the silicon exhibits this behavior? Simply asserting reset should not cause it to draw additional power, let alone 14 mW. This almost sounds like a back-powering problem during suspend. If this is truly expected behavior, is it sufficient to use the always_on constraint of the relevant regulator(s) to make this decision as opposed to introducing a new property? > > Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > index ce18d7dadae2..942acb286d77 100644 > --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > @@ -43,6 +43,12 @@ properties: > itself as long as it allows the main board to make signals compatible > with what the touchscreen is expecting for its IO rails. > > + powered-in-suspend: > + description: > + This indicates that the touchscreen is powered in suspend, so the driver > + will not assert the reset GPIO in power-down to prevent power leakage. > + type: boolean > + > required: > - compatible > - reg > -- > 2.40.0.634.g4ca3ef3211-goog > Kind regards, Jeff LaBundy
Hi Jeff, On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > Hi Fei, > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > touchscreen is powered in suspend (because, for example, it connects to > > an always-on regulator) and with the reset GPIO asserted, it will > > introduce about 14mW power leakage. > > > > This property is used to indicate that the touchscreen is powered in > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > power-down, and it will do it in power-up instead to ensure that the > > state is always reset after resuming. > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > --- > > This is an interesting problem; were you able to root-cause why the silicon > exhibits this behavior? Simply asserting reset should not cause it to draw > additional power, let alone 14 mW. This almost sounds like a back-powering > problem during suspend. > There was a fix for this behavior before so I didn't dig into it on the silicon side. I can ask internally and see if we can have Goodix to confirm this is a known HW erratum. > If this is truly expected behavior, is it sufficient to use the always_on > constraint of the relevant regulator(s) to make this decision as opposed to > introducing a new property? > That sounds good to me. IIUC, for the existing designs, the boards that would set this property would also exclusively set `regulator-always-on` in their supply, so that should suffice. Let me revise the patch. Thanks! Fei > > > > Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > > index ce18d7dadae2..942acb286d77 100644 > > --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > > +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml > > @@ -43,6 +43,12 @@ properties: > > itself as long as it allows the main board to make signals compatible > > with what the touchscreen is expecting for its IO rails. > > > > + powered-in-suspend: > > + description: > > + This indicates that the touchscreen is powered in suspend, so the driver > > + will not assert the reset GPIO in power-down to prevent power leakage. > > + type: boolean > > + > > required: > > - compatible > > - reg > > -- > > 2.40.0.634.g4ca3ef3211-goog > > > > Kind regards, > Jeff LaBundy
Hi, On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote: > > Hi Jeff, > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > Hi Fei, > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > > touchscreen is powered in suspend (because, for example, it connects to > > > an always-on regulator) and with the reset GPIO asserted, it will > > > introduce about 14mW power leakage. > > > > > > This property is used to indicate that the touchscreen is powered in > > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > > power-down, and it will do it in power-up instead to ensure that the > > > state is always reset after resuming. > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > --- > > > > This is an interesting problem; were you able to root-cause why the silicon > > exhibits this behavior? Simply asserting reset should not cause it to draw > > additional power, let alone 14 mW. This almost sounds like a back-powering > > problem during suspend. > > > There was a fix for this behavior before so I didn't dig into it on > the silicon side. > I can ask internally and see if we can have Goodix to confirm this is > a known HW erratum. Certainly it doesn't hurt to check, but it's not really that shocking to me that asserting reset could cause a power draw on some hardware. Reset puts hardware into a default state and that's not necessarily low power. I guess ideally hardware would act like it's "off" when reset is asserted and then then init to the default state on the edge as reset was deasserted, but I not all hardware is designed in an ideal way. > > If this is truly expected behavior, is it sufficient to use the always_on > > constraint of the relevant regulator(s) to make this decision as opposed to > > introducing a new property? > > > That sounds good to me. IIUC, for the existing designs, the boards > that would set this property would also exclusively set > `regulator-always-on` in their supply, so that should suffice. > Let me revise the patch. Thanks! Yeah, I thought about this too and talked about it in my original reply. It doesn't handle the shared-rail case, but then again neither does ${SUBJECT} patch. ...then I guess the only argument against it is my argument that the regulator could be marked "always-on" in the device tree but still turned off by an external entity (PMIC or EC) in S3. In theory this should be specified by "regulator-state-(standby|mem|disk)", but I could believe it being tricky to figure out (what if a parent regulator gets turned off automatically but the child isn't explicit?). Specifically, if a regulator is always-on but somehow gets shut off in suspend then we _do_ want to assert reset (active low) during suspend, otherwise we'll have a power leak through the reset GPIO... :-P ...so I guess I'll continue to assert that I don't think peeking at the regulator's "always-on" property is the best way to go. If everyone else disagrees with me then I won't stand in the way, but IMO the extra property like Fei's patch adds is better. [1] https://lore.kernel.org/r/CAD=FV=V8ZN3969RrPu2-zZYoEE=LDxpi8K_E8EziiDpGOSsq1w@mail.gmail.com
Hi Doug and Fei, Thank you for the informative discussion; I can empathize with the pain these issues bring. On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote: > Hi, > > On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote: > > > > Hi Jeff, > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > > > Hi Fei, > > > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > > > touchscreen is powered in suspend (because, for example, it connects to > > > > an always-on regulator) and with the reset GPIO asserted, it will > > > > introduce about 14mW power leakage. > > > > > > > > This property is used to indicate that the touchscreen is powered in > > > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > > > power-down, and it will do it in power-up instead to ensure that the > > > > state is always reset after resuming. > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > --- > > > > > > This is an interesting problem; were you able to root-cause why the silicon > > > exhibits this behavior? Simply asserting reset should not cause it to draw > > > additional power, let alone 14 mW. This almost sounds like a back-powering > > > problem during suspend. > > > > > There was a fix for this behavior before so I didn't dig into it on > > the silicon side. > > I can ask internally and see if we can have Goodix to confirm this is > > a known HW erratum. > > Certainly it doesn't hurt to check, but it's not really that shocking > to me that asserting reset could cause a power draw on some hardware. > Reset puts hardware into a default state and that's not necessarily > low power. I guess ideally hardware would act like it's "off" when > reset is asserted and then then init to the default state on the edge > as reset was deasserted, but I not all hardware is designed in an > ideal way. While that is true in theory, I have never, ever seen that to be the case when there is not some other underlying problem. What I have seen, however, is that asserting reset actually causes the GPIO to sink current from some other supply and through the IC. I loosely suspect that if you probe the IC's rails and digital I/O during the failure condition, you may find one of them resting at some mid-rail voltage or diode drop. It seems you have a similar suspicion. In that case, it may mean that some other supply in the system should actually be kept on, or that supplies are being brought down out of order. In which case, the solution should actually be a patch to the affected platform(s) dts and not the mainline driver. > > > > > If this is truly expected behavior, is it sufficient to use the always_on > > > constraint of the relevant regulator(s) to make this decision as opposed to > > > introducing a new property? > > > > > That sounds good to me. IIUC, for the existing designs, the boards > > that would set this property would also exclusively set > > `regulator-always-on` in their supply, so that should suffice. > > Let me revise the patch. Thanks! > > Yeah, I thought about this too and talked about it in my original > reply. It doesn't handle the shared-rail case, but then again neither > does ${SUBJECT} patch. ...then I guess the only argument against it is > my argument that the regulator could be marked "always-on" in the > device tree but still turned off by an external entity (PMIC or EC) in > S3. In theory this should be specified by > "regulator-state-(standby|mem|disk)", but I could believe it being > tricky to figure out (what if a parent regulator gets turned off > automatically but the child isn't explicit?). Specifically, if a > regulator is always-on but somehow gets shut off in suspend then we > _do_ want to assert reset (active low) during suspend, otherwise we'll > have a power leak through the reset GPIO... :-P D'oh! Sorry I missed your original reply. My concern is that either solution is a band-aid and does not address the root cause. I would rather see a patch that addresses what seems to be a back-powering problem so that the driver may freely assert reset. That is just my $.02; let me know if I have misunderstood or there are other factors that prevent that path from being viable. > > ...so I guess I'll continue to assert that I don't think peeking at > the regulator's "always-on" property is the best way to go. If > everyone else disagrees with me then I won't stand in the way, but IMO > the extra property like Fei's patch adds is better. > > [1] https://lore.kernel.org/r/CAD=FV=V8ZN3969RrPu2-zZYoEE=LDxpi8K_E8EziiDpGOSsq1w@mail.gmail.com Kind regards, Jeff LaBundy
Hi, On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <jeff@labundy.com> wrote: > > Hi Doug and Fei, > > Thank you for the informative discussion; I can empathize with the pain > these issues bring. > > On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote: > > > > > > Hi Jeff, > > > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > > > > > Hi Fei, > > > > > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > > > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > > > > touchscreen is powered in suspend (because, for example, it connects to > > > > > an always-on regulator) and with the reset GPIO asserted, it will > > > > > introduce about 14mW power leakage. > > > > > > > > > > This property is used to indicate that the touchscreen is powered in > > > > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > > > > power-down, and it will do it in power-up instead to ensure that the > > > > > state is always reset after resuming. > > > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > > --- > > > > > > > > This is an interesting problem; were you able to root-cause why the silicon > > > > exhibits this behavior? Simply asserting reset should not cause it to draw > > > > additional power, let alone 14 mW. This almost sounds like a back-powering > > > > problem during suspend. > > > > > > > There was a fix for this behavior before so I didn't dig into it on > > > the silicon side. > > > I can ask internally and see if we can have Goodix to confirm this is > > > a known HW erratum. > > > > Certainly it doesn't hurt to check, but it's not really that shocking > > to me that asserting reset could cause a power draw on some hardware. > > Reset puts hardware into a default state and that's not necessarily > > low power. I guess ideally hardware would act like it's "off" when > > reset is asserted and then then init to the default state on the edge > > as reset was deasserted, but I not all hardware is designed in an > > ideal way. > > While that is true in theory, I have never, ever seen that to be the case > when there is not some other underlying problem. > > What I have seen, however, is that asserting reset actually causes the GPIO > to sink current from some other supply and through the IC. I loosely suspect > that if you probe the IC's rails and digital I/O during the failure condition, > you may find one of them resting at some mid-rail voltage or diode drop. It > seems you have a similar suspicion. > > In that case, it may mean that some other supply in the system should actually > be kept on, or that supplies are being brought down out of order. In which > case, the solution should actually be a patch to the affected platform(s) dts > and not the mainline driver. I agree that it's a bandaid, but I'm not hopeful that a better solution will be found. Specifically, I'd expect a current leak in the system when you turn a supply off and then assert a GPIO high. That's when the device can start backpowering itself from a GPIO. In this case, it's the opposite. We're keeping the supply on and asserting the (active low) reset GPIO to cause the higher power draw. In another design it was confirmed that the power draw went away when we were able to turn the regulator off (but still keep the active low reset GPIO asserted). We've also confirmed that power is good if we keep the supply on and _don't_ assert the reset GPIO. Both of these two experiments provide some evidence that the system is configured properly and we're not backpowering something. I guess I should revise the above, though. I could believe that there is a current leak but on the touchscreen controller board itself, which is a black box to us. I have certainly been involved in products in the past where the default state of the system at reset caused a minor current leak (I remember an EE telling me that as soon as software started running I should quickly change the direction of a GPIO) and it wouldn't shock me if the touchscreen controller board had a problem like this. If there is a problem like this on the touchscreen controller board there's not much we can do to workaround it. Unfortunately, if the problem ends up needing a hardware change to fix more correctly (which I suspect it does), our hands are tied a bit. This is not prototype hardware but is final hardware. I guess one further note is that, at least on the project I was involved in that had a similar problem, folks in China did a bunch of analysis on this and went as far as adding an extra regulator to the main board schematic to "fix" it. Had the issue just been something where we were misconfiguing GPIOs or leaving a regulator in the wrong state then they (probably) would have identified it rather than spinning the board. -Doug
Hi, On Wed, Apr 19, 2023 at 11:41 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > Hi Doug and Fei, > > > > Thank you for the informative discussion; I can empathize with the pain > > these issues bring. > > > > On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote: > > > > > > > > Hi Jeff, > > > > > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > > > > > > > Hi Fei, > > > > > > > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > > > > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > > > > > touchscreen is powered in suspend (because, for example, it connects to > > > > > > an always-on regulator) and with the reset GPIO asserted, it will > > > > > > introduce about 14mW power leakage. > > > > > > > > > > > > This property is used to indicate that the touchscreen is powered in > > > > > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > > > > > power-down, and it will do it in power-up instead to ensure that the > > > > > > state is always reset after resuming. > > > > > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > > > --- > > > > > > > > > > This is an interesting problem; were you able to root-cause why the silicon > > > > > exhibits this behavior? Simply asserting reset should not cause it to draw > > > > > additional power, let alone 14 mW. This almost sounds like a back-powering > > > > > problem during suspend. > > > > > > > > > There was a fix for this behavior before so I didn't dig into it on > > > > the silicon side. > > > > I can ask internally and see if we can have Goodix to confirm this is > > > > a known HW erratum. > > > > > > Certainly it doesn't hurt to check, but it's not really that shocking > > > to me that asserting reset could cause a power draw on some hardware. > > > Reset puts hardware into a default state and that's not necessarily > > > low power. I guess ideally hardware would act like it's "off" when > > > reset is asserted and then then init to the default state on the edge > > > as reset was deasserted, but I not all hardware is designed in an > > > ideal way. > > > > While that is true in theory, I have never, ever seen that to be the case > > when there is not some other underlying problem. > > > > What I have seen, however, is that asserting reset actually causes the GPIO > > to sink current from some other supply and through the IC. I loosely suspect > > that if you probe the IC's rails and digital I/O during the failure condition, > > you may find one of them resting at some mid-rail voltage or diode drop. It > > seems you have a similar suspicion. > > I reached out to our EE with your thoughts. He said that he understands the concern, but this doesn't apply in our schematics because there's only one supply. Also he simulated a few scenarios that could explain the back-powering, but none of them is possible without having the problematic circuit/rsense layout from within the IC itself. > > In that case, it may mean that some other supply in the system should actually > > be kept on, or that supplies are being brought down out of order. In which > > case, the solution should actually be a patch to the affected platform(s) dts > > and not the mainline driver. > > I agree that it's a bandaid, but I'm not hopeful that a better > solution will be found. > > Specifically, I'd expect a current leak in the system when you turn a > supply off and then assert a GPIO high. That's when the device can > start backpowering itself from a GPIO. In this case, it's the > opposite. We're keeping the supply on and asserting the (active low) > reset GPIO to cause the higher power draw. In another design it was > confirmed that the power draw went away when we were able to turn the > regulator off (but still keep the active low reset GPIO asserted). > We've also confirmed that power is good if we keep the supply on and > _don't_ assert the reset GPIO. Both of these two experiments provide > some evidence that the system is configured properly and we're not > backpowering something. > > I guess I should revise the above, though. I could believe that there > is a current leak but on the touchscreen controller board itself, > which is a black box to us. I have certainly been involved in products > in the past where the default state of the system at reset caused a > minor current leak (I remember an EE telling me that as soon as > software started running I should quickly change the direction of a > GPIO) and it wouldn't shock me if the touchscreen controller board had > a problem like this. If there is a problem like this on the > touchscreen controller board there's not much we can do to workaround > it. > > Unfortunately, if the problem ends up needing a hardware change to fix > more correctly (which I suspect it does), our hands are tied a bit. > This is not prototype hardware but is final hardware. > > I guess one further note is that, at least on the project I was > involved in that had a similar problem, folks in China did a bunch of > analysis on this and went as far as adding an extra regulator to the > main board schematic to "fix" it. Had the issue just been something > where we were misconfiguing GPIOs or leaving a regulator in the wrong > state then they (probably) would have identified it rather than > spinning the board. Thank you Doug for providing the details and explanation, and sorry that I also missed your original reply... I only considered the ideal scenarios for the always_on usage but not the cases you brought up. The concerns make sense to me. I'm still awaiting the response from Goodix, but +1 that if it turns out to be a GT7375P hw issue, we're not able to do much about that except relying on the driver workaround. One more note I want to add is that the first attempt of the fix was added ~2yrs ago, so it's not an one-off on a particular platform, plus `sc7180-trogdor-homestar` and `mt8186-corsola-steelix` are two different designs come from two different teams, but they ended up with the same symptom. With that said, I think we have more confidence to say it's a component misbehavior, and we just fell into the edge case that was not covered or considered by its design. Regards, Fei > > -Doug
Hi Doug and Fei, Thank you for this additional information, and your continued patience with my many questions :) On Thu, Apr 20, 2023 at 04:13:37PM +0800, Fei Shao wrote: > Hi, > > On Wed, Apr 19, 2023 at 11:41 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > > > Hi Doug and Fei, > > > > > > Thank you for the informative discussion; I can empathize with the pain > > > these issues bring. > > > > > > On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote: > > > > > > > > > > Hi Jeff, > > > > > > > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote: > > > > > > > > > > > > Hi Fei, > > > > > > > > > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote: > > > > > > > We observed that on Chromebook device Steelix, if Goodix GT7375P > > > > > > > touchscreen is powered in suspend (because, for example, it connects to > > > > > > > an always-on regulator) and with the reset GPIO asserted, it will > > > > > > > introduce about 14mW power leakage. > > > > > > > > > > > > > > This property is used to indicate that the touchscreen is powered in > > > > > > > suspend. If it's set, the driver will stop asserting the reset GPIO in > > > > > > > power-down, and it will do it in power-up instead to ensure that the > > > > > > > state is always reset after resuming. > > > > > > > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > > > > --- > > > > > > > > > > > > This is an interesting problem; were you able to root-cause why the silicon > > > > > > exhibits this behavior? Simply asserting reset should not cause it to draw > > > > > > additional power, let alone 14 mW. This almost sounds like a back-powering > > > > > > problem during suspend. > > > > > > > > > > > There was a fix for this behavior before so I didn't dig into it on > > > > > the silicon side. > > > > > I can ask internally and see if we can have Goodix to confirm this is > > > > > a known HW erratum. > > > > > > > > Certainly it doesn't hurt to check, but it's not really that shocking > > > > to me that asserting reset could cause a power draw on some hardware. > > > > Reset puts hardware into a default state and that's not necessarily > > > > low power. I guess ideally hardware would act like it's "off" when > > > > reset is asserted and then then init to the default state on the edge > > > > as reset was deasserted, but I not all hardware is designed in an > > > > ideal way. > > > > > > While that is true in theory, I have never, ever seen that to be the case > > > when there is not some other underlying problem. > > > > > > What I have seen, however, is that asserting reset actually causes the GPIO > > > to sink current from some other supply and through the IC. I loosely suspect > > > that if you probe the IC's rails and digital I/O during the failure condition, > > > you may find one of them resting at some mid-rail voltage or diode drop. It > > > seems you have a similar suspicion. > > > > > I reached out to our EE with your thoughts. > He said that he understands the concern, but this doesn't apply in our > schematics because there's only one supply. > Also he simulated a few scenarios that could explain the > back-powering, but none of them is possible without having the > problematic circuit/rsense layout from within the IC itself. > > > > In that case, it may mean that some other supply in the system should actually > > > be kept on, or that supplies are being brought down out of order. In which > > > case, the solution should actually be a patch to the affected platform(s) dts > > > and not the mainline driver. > > > > I agree that it's a bandaid, but I'm not hopeful that a better > > solution will be found. > > > > Specifically, I'd expect a current leak in the system when you turn a > > supply off and then assert a GPIO high. That's when the device can > > start backpowering itself from a GPIO. In this case, it's the > > opposite. We're keeping the supply on and asserting the (active low) > > reset GPIO to cause the higher power draw. In another design it was > > confirmed that the power draw went away when we were able to turn the > > regulator off (but still keep the active low reset GPIO asserted). > > We've also confirmed that power is good if we keep the supply on and > > _don't_ assert the reset GPIO. Both of these two experiments provide > > some evidence that the system is configured properly and we're not > > backpowering something. Back-powering can come in two forms: 1. The one you've described, which is by far the most common. As you mention, it is not the case here since the issue happens only when we drive the GPIO low and not high. 2. Through a forbidden power supply sequence, an input pin of an IC with multiple power supplies becomes a weak power supply itself. Grounding the GPIO then sinks current into the SoC. This problem smelled like (2), especially since asserting the GPIO or powering down the supply alleviates the symptom. Most Goodix controllers I've worked with have two or more supplies, and the datasheet requires them to be enabled or disabled in a specific order. Based on Fei's feedback, however, this IC does not seem to be one of those since there is reportedly only a single rail. I guess either vdd or vddio is tied to a dummy regulator? If so, then perhaps we can scratch this theory. > > > > I guess I should revise the above, though. I could believe that there > > is a current leak but on the touchscreen controller board itself, > > which is a black box to us. I have certainly been involved in products > > in the past where the default state of the system at reset caused a > > minor current leak (I remember an EE telling me that as soon as > > software started running I should quickly change the direction of a > > GPIO) and it wouldn't shock me if the touchscreen controller board had > > a problem like this. If there is a problem like this on the > > touchscreen controller board there's not much we can do to workaround > > it. > > > > Unfortunately, if the problem ends up needing a hardware change to fix > > more correctly (which I suspect it does), our hands are tied a bit. > > This is not prototype hardware but is final hardware. Fair enough, I was simply sketpical that this was the _right_ workaround. Were this an issue of only supply A left on yet the IC datasheet requires supply B to remain on if supply A is on, I would rather see this solved by updating a regulator dts node, trusted FW sequence, etc. > > > > I guess one further note is that, at least on the project I was > > involved in that had a similar problem, folks in China did a bunch of > > analysis on this and went as far as adding an extra regulator to the > > main board schematic to "fix" it. Had the issue just been something > > where we were misconfiguing GPIOs or leaving a regulator in the wrong > > state then they (probably) would have identified it rather than > > spinning the board. > > Thank you Doug for providing the details and explanation, and sorry > that I also missed your original reply... > I only considered the ideal scenarios for the always_on usage but not > the cases you brought up. The concerns make sense to me. > > I'm still awaiting the response from Goodix, but +1 that if it turns > out to be a GT7375P hw issue, we're not able to do much about that > except relying on the driver workaround. > One more note I want to add is that the first attempt of the fix was > added ~2yrs ago, so it's not an one-off on a particular platform, plus > `sc7180-trogdor-homestar` and `mt8186-corsola-steelix` are two > different designs come from two different teams, but they ended up > with the same symptom. > With that said, I think we have more confidence to say it's a > component misbehavior, and we just fell into the edge case that was > not covered or considered by its design. Thanks for your due diligence; if this really is a silicon issue, then the driver should indeed accommodate it. That being said, the name 'powered-in-suspend' seems a bit conflated. We should not be duplicating regulator policy in this driver; the existing naming also may cause confusion for other HW configurations that do leave vdd and vddio on during suspend, but don't have this problem. Here, we actually mean to control the behavior of the reset GPIO through suspend and therefore something like 'goodix,no-reset-during-suspend' is closer to what I understand us to intend to do. I will add more details in patch [2/2]. > > Regards, > Fei > > > > > -Doug Kind regards, Jeff LaBundy
Hi, On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@labundy.com> wrote: > > Back-powering can come in two forms: > > 1. The one you've described, which is by far the most common. As you mention, > it is not the case here since the issue happens only when we drive the GPIO > low and not high. > > 2. Through a forbidden power supply sequence, an input pin of an IC with > multiple power supplies becomes a weak power supply itself. Grounding the > GPIO then sinks current into the SoC. > > This problem smelled like (2), especially since asserting the GPIO or powering > down the supply alleviates the symptom. Most Goodix controllers I've worked > with have two or more supplies, and the datasheet requires them to be enabled > or disabled in a specific order. > > Based on Fei's feedback, however, this IC does not seem to be one of those > since there is reportedly only a single rail. I guess either vdd or vddio is > tied to a dummy regulator? If so, then perhaps we can scratch this theory. There is one rail provided from the main board, but there can be two voltage levels involved depending on stuffings on the touchscreen itself. I talked a bit about this in commit 1d18c1f3b7d9 ("dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply"). > Fair enough, I was simply sketpical that this was the _right_ workaround. > Were this an issue of only supply A left on yet the IC datasheet requires > supply B to remain on if supply A is on, I would rather see this solved by > updating a regulator dts node, trusted FW sequence, etc. Right. That type of stuff was looked at in detail by two separate teams, so I don't think this is the issue. > Thanks for your due diligence; if this really is a silicon issue, then > the driver should indeed accommodate it. > > That being said, the name 'powered-in-suspend' seems a bit conflated. We > should not be duplicating regulator policy in this driver; the existing > naming also may cause confusion for other HW configurations that do leave > vdd and vddio on during suspend, but don't have this problem. > > Here, we actually mean to control the behavior of the reset GPIO through > suspend and therefore something like 'goodix,no-reset-during-suspend' is > closer to what I understand us to intend to do. I will add more details > in patch [2/2]. I'd be fine with that name. ...ah, and adding the "goodix," prefix is a good call. -Doug
On Tue, Apr 25, 2023 at 2:15 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@labundy.com> wrote: > > > > Back-powering can come in two forms: > > > > 1. The one you've described, which is by far the most common. As you mention, > > it is not the case here since the issue happens only when we drive the GPIO > > low and not high. > > > > 2. Through a forbidden power supply sequence, an input pin of an IC with > > multiple power supplies becomes a weak power supply itself. Grounding the > > GPIO then sinks current into the SoC. > > > > This problem smelled like (2), especially since asserting the GPIO or powering > > down the supply alleviates the symptom. Most Goodix controllers I've worked > > with have two or more supplies, and the datasheet requires them to be enabled > > or disabled in a specific order. > > > > Based on Fei's feedback, however, this IC does not seem to be one of those > > since there is reportedly only a single rail. I guess either vdd or vddio is > > tied to a dummy regulator? If so, then perhaps we can scratch this theory. > > There is one rail provided from the main board, but there can be two > voltage levels involved depending on stuffings on the touchscreen > itself. I talked a bit about this in commit 1d18c1f3b7d9 > ("dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply"). > > > > Fair enough, I was simply sketpical that this was the _right_ workaround. > > Were this an issue of only supply A left on yet the IC datasheet requires > > supply B to remain on if supply A is on, I would rather see this solved by > > updating a regulator dts node, trusted FW sequence, etc. > > Right. That type of stuff was looked at in detail by two separate > teams, so I don't think this is the issue. > > > > Thanks for your due diligence; if this really is a silicon issue, then > > the driver should indeed accommodate it. > > > > That being said, the name 'powered-in-suspend' seems a bit conflated. We > > should not be duplicating regulator policy in this driver; the existing > > naming also may cause confusion for other HW configurations that do leave > > vdd and vddio on during suspend, but don't have this problem. > > > > Here, we actually mean to control the behavior of the reset GPIO through > > suspend and therefore something like 'goodix,no-reset-during-suspend' is > > closer to what I understand us to intend to do. I will add more details > > in patch [2/2]. > > I'd be fine with that name. ...ah, and adding the "goodix," prefix is > a good call. > > -Doug That sounds good to me, too. I'll update the name in the next patch. Regards, Fei
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml index ce18d7dadae2..942acb286d77 100644 --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml @@ -43,6 +43,12 @@ properties: itself as long as it allows the main board to make signals compatible with what the touchscreen is expecting for its IO rails. + powered-in-suspend: + description: + This indicates that the touchscreen is powered in suspend, so the driver + will not assert the reset GPIO in power-down to prevent power leakage. + type: boolean + required: - compatible - reg
We observed that on Chromebook device Steelix, if Goodix GT7375P touchscreen is powered in suspend (because, for example, it connects to an always-on regulator) and with the reset GPIO asserted, it will introduce about 14mW power leakage. This property is used to indicate that the touchscreen is powered in suspend. If it's set, the driver will stop asserting the reset GPIO in power-down, and it will do it in power-up instead to ensure that the state is always reset after resuming. Signed-off-by: Fei Shao <fshao@chromium.org> --- Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++ 1 file changed, 6 insertions(+)