Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property

[Jeff LaBundy]

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@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <jeff@xxxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > Hi Jeff,
> > > > >
> > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@xxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > > > > ---
> > > > > >
> > > > > > 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