Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Tue, Dec 8, 2020 at 9:54 PM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> >> but as long as its IRQ is in disabled/masked state it
> >> doesn't matter.
> > ...but there's no requirement that someone would need to disable/mask
> > an interrupt while switching the muxing, is there?  So it does matter.
> >
> >
> >> its only when the GPIO is again set to IRQ mode with set_mux callback,
> >> the phantom IRQ needs clear to start as clean.
> >>
> >> So we should check only for if (i == pctrl->soc->gpio_func) then clear
> >> phantom IRQ.
> >>
> >> The same is case with .direction_output callback, when GPIO is used as
> >> output say as clock, need not clear any phantom IRQ,
> >>
> >> The reason is with every pulse of clock it can latch as pending IRQ in
> >> GIC_ISPEND as long as it stay as output mode/clock.
> >>
> >> its only when switching back GPIO from output direction to input & IRQ
> >> function, need to clear the phantom IRQ.
> >>
> >> so we do not require clear phantom irq in .direction_output callback.
> > I think all the above explanation is with the model that the interrupt
> > detection logic is still happening even when muxed away.  I don't
> > believe that's true.
> Its not the interrupt detection logic that is still happening when muxed
> away, but the GPIO line is routed to GIC from PDC.
> The GPIO line get forwarded when the system is active/out of system
> level low power mode to GIC irrespective of whether GPIO is used as
> interrupt or not.
> Due to this it can still latch the IRQ at GIC after switching to lets
> say Rx mode, whenever the line has any data recive, the line state
> toggles can be latched as error interrupt at GIC.

>From my tests, though, I strongly believe that the pin is only visible
to the PDC if it's muxed as GPIO.  Specifically, in my tests I did
this (with all my patches applied so there were no phantom interrupts
when remuxing):

a) Muxed the pin away from GPIO to special function, but _didn't_ mask
the interrupt.

b) Toggled the line a whole bunch.  These caused no interrupts at all.

c) Muxed back to GPIO.

To me this is quite strong evidence that the muxing is "earlier" in
the path than the connection to the PDC.  In other words, if you
change the mux away from GPIO then the PDC stops seeing it and thus
the GIC also stops seeing it.  The GIC can't latch what it can't see.
This means while you're in "Rx mode" it can't be latched.


OK, so just in case this somehow only happens in S3, I also tried this
(with my patch from https://crrev.com/c/2556012):

a) Muxed away from GPIO ("bogus" pinmux)

b) Enter S3.

c) Toggle the GPIO a whole bunch ("wp enable / wp disable" on Cr50).

d) Wake from S3.

e) Check to see if the interrupt fired a bunch.  It didn't fire at all


In my test code the interrupt is not masked, only muxed away.  That
means that if, somehow, the PDC was still observing it then we'd see
the interrupt fire.  We don't.


Unless I messed up in my tests (always possible, though by this point
I've run them a number of times) then it still feels like your mental
model is wrong, or it's always possible I'm still misunderstanding
your model.  Regardless, rather than trying to re-explain your model
can you please confirm that you've written test code to confirm your
mental model?  If so, can you please provide this test code?  I've
provided several test patches proving out my mental model.

> As the interrupt is in disabled state it won't be sent to CPU.
> Its only when the driver chooses to switch back to interrupt mode we
> want to clear the error interrupt latched to start as clean. same is the
> case when used as output direction.
>
> Hope above is clear.

Unfortunately, it's still not.  :(  Can I convince you to provide a
test patch and a set of steps that will demonstrate the problem you're
worried about?  Specifically:

a) Maybe you're talking about the initial switch from a plain GPIO
input to making it an interrupt for the first time?  Are you worried
about a phantom interrupt in this case?  After patch #1 I think we're
safe because pdc_gic_set_type() will always clear the interrupt,
right?


b) You say "switch back to interrupt mode".  Are you imagining that a
driver does something like this:

request_irq();
...
free_irq();
...
request_irq();

If you're worried about that then we can implement irq_shutdown() for
PDC and then make sure we clear on the first enable after a shutdown,
I guess?


c) Maybe when you say "switch back to interrupt mode" you mean
something else?  If you are talking about muxing away and then muxing
back then I think we already have this covered.  If you are talking
about masking/unmasking then the whole point is that we _do_ want
interrupts latched while masked, right?


OK, I'm going to send out a v3 just to get the already-identified
problems fixed and also to allow landing of patch #1 in the series,
which I think is all agreed upon.  My request to you is that if you
think my code misses a specific case to provide some test patches to
demonstrate that case.


-Doug