From: gregory.clement@free-electrons.com (Gregory CLEMENT)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support
Date: Wed, 26 Apr 2017 15:12:01 +0200 [thread overview]
Message-ID: <87inlr8g9q.fsf@free-electrons.com> (raw)
In-Reply-To: <CACRpkdYYRe4A+Zj+fDpC9SA1pgaVSgewYGB_vCNoWNRXWTtTGQ@mail.gmail.com> (Linus Walleij's message of "Wed, 26 Apr 2017 14:03:15 +0200")
Hi Linus,
On mer., avril 26 2017, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 26, 2017 at 11:23 AM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> On lun., avril 24 2017, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>>> + spin_lock_irqsave(&info->irq_lock, flags);
>>>> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i);
>>>> + /* Manage only the interrupt that was enabled */
>>>> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i);
>>>> + spin_unlock_irqrestore(&info->irq_lock, flags);
>>>> + while (status) {
>>>> + u32 hwirq = ffs(status) - 1;
>>>> + u32 virq = irq_find_mapping(d, hwirq +
>>>> + i * GPIO_PER_REG);
>>>> +
>>>> + generic_handle_irq(virq);
>>>> + status &= ~BIT(hwirq);
>>>> + }
>>>
>>> You hae a problem here is a new IRQ appears while you are inside
>>> of this loop. You need to re-read the status register for each iteration
>>> (and &= with the IRQ_EN I guess).
>>
>> If a new IRQ appears during the loop, then the irq handler will be
>> called again because the cause of this new IRQ won't have been acked
>> yet. So I think we're fine here.
>
> That *might* be true. It is true if the CPU gets a level IRQ from the
> GPIO controller. But hardware dealing with edge IRQs can be very
> quirky here, and just send a pulse on the line to the CPU if the
> CPU-bound IRQ is also just edge triggered. And then that
> pulse would potentially be missed while dealing with the current
> IRQ in this handler. (And exactly this happened to us on other
> hardware.)
OK thanks for sharing your experience, you convinced me, I am going to
send a new version of the patch with this fix.
>
> But anyway: why let the irq handler be called again if you can avoid
> it?
> You would avoid a double context switch by just checking it again
> in the loop before exiting the handler. And that can be really nice
> for latency-sensitive stuff.
I wanted to avoid an uncached access in each loop if it was not
necessary. But as we finally need it, I will do it.
Gregory
>
> Yours,
> Linus Walleij
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
next prev parent reply other threads:[~2017-04-26 13:12 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-05 15:18 [PATCH v4 0/7] Add support for pinctrl/gpio on Armada 37xx Gregory CLEMENT
2017-04-05 15:18 ` [PATCH v4 1/7] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers Gregory CLEMENT
2017-04-10 18:15 ` Rob Herring
2017-04-11 16:09 ` Gregory CLEMENT
2017-04-24 9:29 ` Linus Walleij
2017-04-05 15:18 ` [PATCH v4 2/7] arm64: marvell: enable the Armada 37xx pinctrl driver Gregory CLEMENT
2017-04-26 10:26 ` Gregory CLEMENT
2017-04-05 15:18 ` [PATCH v4 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Gregory CLEMENT
2017-04-05 15:18 ` [PATCH v4 4/7] pinctrl: armada-37xx: Add gpio support Gregory CLEMENT
2017-04-24 11:48 ` Linus Walleij
2017-04-05 15:18 ` [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support Gregory CLEMENT
2017-04-05 16:03 ` Gregory CLEMENT
2017-04-24 12:14 ` Linus Walleij
2017-04-24 12:16 ` Linus Walleij
2017-04-26 9:23 ` Gregory CLEMENT
2017-04-26 12:03 ` Linus Walleij
2017-04-26 13:12 ` Gregory CLEMENT [this message]
2017-04-05 15:18 ` [PATCH v4 6/7] ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 Gregory CLEMENT
2017-04-26 10:27 ` Gregory CLEMENT
2017-04-05 15:18 ` [PATCH v4 7/7] ARM64: dts: marvell: armada37xx: add pinctrl definition Gregory CLEMENT
2017-04-26 10:28 ` Gregory CLEMENT
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=87inlr8g9q.fsf@free-electrons.com \
--to=gregory.clement@free-electrons.com \
--cc=linux-arm-kernel@lists.infradead.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).