Message-ID: <92c61a18-0a1d-099e-4a11-b33a052b4ec2@codeaurora.org>
Date:   Fri, 11 Dec 2020 12:37:30 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>,
        Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Srinivas Ramana <sramana@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when
 remuxing GPIOs

Hi Doug,

On 12/10/2020 6:13 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Dec 8, 2020 at 9:54 PM Maulik Shah <mkshah@...eaurora.org> 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.
Its not a mental model, its how the line is connected to GIC.
GPIO follows the path, TLMM to PDC to GIC.
PDC donot know if the line is muxed away from GPIO to some other 
function, so it can stop forwarding to GIC.

I have slightly modified your test case (see at 
https://crrev.com/c/2584729) which is as per what i used in my testing.

Here is what i am doing, setting GPIO to a fixed function (function 2 here)
Note that function 0 is the GPIO (interrupt mode).

1) Pull up the GPIO in function 2
2) Pull down the GPIO in function 2

Repeat above steps, and you will see fake interrupt every time pull down/up.
This proves that if you mux away from GPIO then still PDC sees the line 
and can latch the interrupt at GIC.

Thanks,
Maulik

>> 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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists