Thread (9 messages) 9 messages, 3 authors, 2021-09-20

RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support

From: Asmaa Mnebhi <asmaa@nvidia.com>
Date: 2021-09-20 20:34:37
Also in: linux-acpi, linux-gpio, lkml

quoted
+	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.

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help