LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: JeffyChen <jeffy.chen@rock-chips.com>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-kernel@vger.kernel.org, heiko@sntech.de,
	linux-rockchip@lists.infradead.org,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Doug Anderson <dianders@chromium.org>
Subject: Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing it's capability
Date: Tue, 08 May 2018 09:36:24 +0800	[thread overview]
Message-ID: <5AF0FF18.1050905@rock-chips.com> (raw)
In-Reply-To: <20180507221511.GA6448@rodete-desktop-imager.corp.google.com>

Hi Brian,

On 05/08/2018 06:15 AM, Brian Norris wrote:
> + Doug
>
> Hi Jeffy,
>
> On Thu, May 03, 2018 at 02:55:53PM +0800, Jeffy Chen wrote:
>> We saw spurious irq when changing irq's trigger type, for example
>> setting gpio-keys's wakeup irq trigger type.
>>
>> And according to the TRM:
>> "Programming the GPIO registers for interrupt capability, edge-sensitive
>> or level-sensitive interrupts, and interrupt polarity should be
>> completed prior to enabling the interrupts on Port A in order to prevent
>> spurious glitches on the interrupt lines to the interrupt controller."
>>
>> Reported-by: Brian Norris <briannorris@google.com>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/pinctrl/pinctrl-rockchip.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
>> index 3924779f55785..7ff45ec8330d1 100644
>> --- a/drivers/pinctrl/pinctrl-rockchip.c
>> +++ b/drivers/pinctrl/pinctrl-rockchip.c
>> @@ -2727,9 +2727,19 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
>>   		return -EINVAL;
>>   	}
>>
>> +	/**
>
> The typical multiline comment style is to use only 1 asterisk -- 2
> asterisks usually denote something more formal, like kerneldoc.

ok, will fix it.

>
>> +	 * According to the TRM, we should keep irq disabled during programming
>> +	 * interrupt capability to prevent spurious glitches on the interrupt
>> +	 * lines to the interrupt controller.
>> +	 */
>
> It's also worth noting that this case may come up in
> rockchip_irq_demux() for EDGE_BOTH triggers:
>
> 		/*
> 		 * Triggering IRQ on both rising and falling edge
> 		 * needs manual intervention.
> 		 */
> 		if (bank->toggle_edge_mode & BIT(irq)) {
> ...
> 				polarity = readl_relaxed(bank->reg_base +
> 							 GPIO_INT_POLARITY);
> 				if (data & BIT(irq))
> 					polarity &= ~BIT(irq);
> 				else
> 					polarity |= BIT(irq);
> 				writel(polarity,
> 				       bank->reg_base + GPIO_INT_POLARITY);
>
> AIUI, this case is not actually a problem, because this polarity
> reconfiguration happens before we call generic_handle_irq(), so the
> extra spurious interrupt is handled and cleared after this point. But it
> seems like this should be noted somewhere in either the commit message,
> the code comments, or both.

just from my testing, the spurious irq only happen when set EDGE_RISING 
to a gpio which is already high level, or set EDGE_FALLING to a gpio 
which is already low level.so the demux / EDGE_BOTH seem ok.

but adding comments as your suggested is a good idea, will do that.

>
> 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'll try to confirm it with IC guys.

>
> I'm not 100% sure about the above, so it might be good to verify it. But
> I also expect this means there's really no way to 100% reliably
> implement both-edge trigger types on hardware like this that doesn't
> directly implement it. So I'm not sure what we'd do with that knowledge.
>
>> +	data = readl(bank->reg_base + GPIO_INTEN);
>> +	writel_relaxed(data & ~mask, gc->reg_base + GPIO_INTEN);
>
> Not sure if this is a needless optimization: but couldn't you only
> disable the interrupt (and make the level and polarity changes) only if
> the level or polarity are actually changing? For example, it's possible
> to end up here when changing from EDGE_BOTH to EDGE_RISING, but we might
> not actually program a new value.

right, i noticed that too, will add a patch for that in v2.
>
> Brian
>
>> +
>>   	writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL);
>>   	writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY);
>>
>> +	writel_relaxed(data, gc->reg_base + GPIO_INTEN);
>> +
>>   	irq_gc_unlock(gc);
>>   	raw_spin_unlock_irqrestore(&bank->slock, flags);
>>   	clk_disable(bank->clk);
>> --
>> 2.11.0
>>
>>
>
>
>

  reply	other threads:[~2018-05-08  1:36 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 [this message]
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
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=5AF0FF18.1050905@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).