Date:   Mon, 20 Sep 2021 20:32:31 +0000
From:   Asmaa Mnebhi <asmaa@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        David Thompson <davthompson@...dia.com>
Subject: RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support

> > +	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> > +	val |= BIT(offset);
> > +	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> 
> What exactly does this do? It appears to clear the interrupt, if i understand
> mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is
> correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear
> to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled?
> 
> Asmaa>>
> YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we
> will not receive further interrupts.

> If the interrupt status bit is set, as soon as you unmask the interrupt, the hardware should fire
> the interrupt. At least, that is how interrupt controllers usually work.

> A typical pattern is that the interrupt fires. You mask it, ack it, and then do what is needed to
> actually handle the interrupt. While doing the handling, the hardware can indicate the interrupt
> again. But since it is masked nothing happened. This avoids your interrupt handler going
> recursive.
> Once the handler has finished, the interrupt is unmasked. At this point it actually fires, triggering
> the interrupt handler again.

Asmaa>> mlxbf2_gpio_irq_enable seems to be called only once when the driver is loaded.
And I will actually remove mlxbf2_gpio_irq_ack because it is not being called at all.
After further investigation, that function is called via chained_irq_enter which is itself invoked in
the interrupt handler. It should have looked something like this:

static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr)
{
    chained_irq_enter(gc->irq->chip, desc);
    // rest of the code here
    chained_irq_exit(gc->irq->chip, desc);
}

But in our case, we decided to directly request the irq  instead of passing a flow-handler to
gpiochip_set_chained_irqchip, because the irq has to be marked as shared (IRQF_SHARED).
gpio-mt7621.c does something similar.
Moreover, whenever an interrupt is fired by HW, it is automatically disabled/masked until
it is explicitly cleared by Software. And this line takes care of it in mlxbf2_gpio_irq_handler:
writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);

After a HW reset, all gpio interrupts are disabled by default by HW. The HW will not signal
any gpio interrupt as long as all bits in YU_GPIO_CAUSE_OR_EVTEN0 are 0.
In mlxbf2_gpio_irq_enable, we configure a specific gpio as an interrupt by writing 1 to
YU_GPIO_CAUSE_OR_EVTEN0. I just wanted to make sure there is no trash value in
YU_GPIO_CAUSE_OR_CLRCAUSE before enabling gpio interrupt support.
So pending interrupts in YU_GPIO_CAUSE_OR_CLRCAUSE only matters if
YU_GPIO_CAUSE_OR_EVTEN0 is set accordingly.
Does this answer your question?

> Please also get your email client fixed. I wrap my emails at around 75 characters. Your mailer
> has destroyed it. Your text should also be wrapped at about 75 characters.

Asmaa>> Sorry about that. I wrapped my outlook emails around 75 characters, I hope it works.

Powered by blists - more mailing lists