LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: JeffyChen <jeffy.chen@rock-chips.com>
To: Doug Anderson <dianders@chromium.org>,
	Brian Norris <briannorris@chromium.org>
Cc: 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: Wed, 09 May 2018 10:21:34 +0800	[thread overview]
Message-ID: <5AF25B2E.8080901@rock-chips.com> (raw)
In-Reply-To: <CAD=FV=W_v+aoNT09k-2exVABTFzOp0heY-XWcLi95uPisZhiGQ@mail.gmail.com>

Hi Doug,

On 05/09/2018 03:46 AM, Doug Anderson wrote:
> 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.
>

hmm, but i saw the gpio-dwapb.c actually toggle trigger after handle 
irq, which might avoid the race Brian mentioned:
+               generic_handle_irq(gpio_irq);
+               irq_status &= ~BIT(hwirq);
+
+               if ((irq_get_trigger_type(gpio_irq) & IRQ_TYPE_SENSE_MASK)
+                       == IRQ_TYPE_EDGE_BOTH)
+                       dwapb_toggle_trigger(gpio, hwirq);


and i also saw ./qcom/pinctrl-msm.c do the 
toggle(msm_gpio_update_dual_edge_pos) at the end of 
msm_gpio_irq_set_type and msm_gpio_irq_ack, that seems can avoid the 
polarity races too.

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

right, so we now have 2 cases: rockchip_irq_demux/ rockchip_irq_set_type

if i'm right about the spurious irq(only happen when set rising for a 
high gpio, or set falling for a low gpio), then:

1/ rockchip_irq_demux
it's important to not losing irqs in this case, maybe we can

a) ack irq
b) update polarity for edge both irq

we don't need to disable irq in b), since we would not hit the spurious 
irq cases here(always check gpio level to toggle it)


2/ rockchip_irq_set_type
it's important to not having spurious irqs

so we can disable irq during changing polarity only in these case:
((rising && gpio is heigh) || (falling && gpio is low))

i'm still confirming the spurious irq with IC guys.

>
>
> -Doug
>
>
>

  reply	other threads:[~2018-05-09  2:21 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
2018-05-09  2:21         ` JeffyChen [this message]
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=5AF25B2E.8080901@rock-chips.com \
    --to=jeffy.chen@rock-chips.com \
    --cc=briannorris@chromium.org \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --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).