LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Brian Norris <briannorris@chromium.org>
Cc: JeffyChen <jeffy.chen@rock-chips.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Heiko Stübner" <heiko@sntech.de>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org,
	"Linux ARM" <linux-arm-kernel@lists.infradead.org>
Subject: Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability
Date: Tue, 8 May 2018 12:46:25 -0700	[thread overview]
Message-ID: <CAD=FV=W_v+aoNT09k-2exVABTFzOp0heY-XWcLi95uPisZhiGQ@mail.gmail.com> (raw)
In-Reply-To: <20180508015623.GA61455@rodete-desktop-imager.corp.google.com>

Hi,

On Mon, May 7, 2018 at 6:56 PM, Brian Norris <briannorris@chromium.org> wrote:
> On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:
>> On 05/08/2018 06:15 AM, Brian Norris wrote:
>> > On the other hand...this also implies there may be a race condition
>> > there, where we might lose an interrupt if there is an edge between the
>> > re-configuration of the polarity in rockchip_irq_demux() and the
>> > clearing/handling of the interrupt (handle_edge_irq() ->
>> > chip->irq_ack()). If we have an edge between there, then we might ack
>> > it, but leave the polarity such that we aren't ready for the next
>> > (inverted) edge.
>>
>> if let me guess, the unexpected irq we saw is the hardware trying to avoid
>> losing irq? for example, we set a EDGE_RISING, and the hardware saw the gpio
>> is already high, then though it might lost an irq, so fake one for safe?
>
> I won't pretend to know what the IC designers were doing, but I don't
> think that would resolve the problem I'm talking about. The sequence is
> something like:
> 1. EDGE_BOTH IRQ occurs (e.g., low to high)
> 2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
> 3. continue to handle_edge_irq()
> 4. another HW edge occurs (e.g., high to low)
> 5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
>    rockchip_irq_demux() gets a chance to run and flip the polarity)
> ...
>
> Now the polarity is still low, but the next trigger should be a
> low-to-high edge.
>
>> i'll try to confirm it with IC guys.

One note is that in the case Brian points at (where we need to
simulate EDGE_BOTH by swapping edges) we purposely ignored the TRM and
we needed to do that to avoid losing interrupts.  For details, see
commit 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when
supporting both edges").  We did this because:

1. We believed that the IP block in Rockchip SoCs has nearly the same
logic as "gpio-dwapb.c" and that's what "gpio-dwapb.c" did.

2. We were actually losing real interrupts and this was the only way
we could figure out how to fix it.

When I tested that back in the day I was fairly convinced that we
weren't losing any interrupts in the EDGE_BOTH case after my fix, but
I certainly could have messed up.


For the EDGE_BOTH case it was important not to lose an interrupt
because, as you guys are talking about, we could end up configured the
wrong way.  I think in your case where you're just picking one
polarity losing an interrupt shouldn't matter since it's undefined
exactly if an edge happens while you're in the middle of executing
rockchip_irq_set_type().  Is that right?


-Doug

  parent reply	other threads:[~2018-05-08 19:46 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03  6:55 Jeffy Chen
2018-05-07 22:15 ` Brian Norris
2018-05-08  1:36   ` JeffyChen
2018-05-08  1:56     ` Brian Norris
2018-05-08  2:31       ` JeffyChen
2018-05-08  2:56         ` JeffyChen
2018-05-08 19:46       ` Doug Anderson [this message]
2018-05-09  2:21         ` JeffyChen
2018-05-09  5:18           ` Doug Anderson
2018-05-09  6:41             ` JeffyChen
2018-05-10 22:16             ` Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=W_v+aoNT09k-2exVABTFzOp0heY-XWcLi95uPisZhiGQ@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=briannorris@chromium.org \
    --cc=heiko@sntech.de \
    --cc=jeffy.chen@rock-chips.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --subject='Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it'\''s capability' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).