Re: [PATCH 05/33] gpio: add generic single-register fixed-direction GPIO driver

[Robert Jarzmik <robert.jarzmik@xxxxxxx>]

Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> writes:

> On Fri, Sep 02, 2016 at 07:50:35PM +0200, Robert Jarzmik wrote:
> It should enable the interrupt - the end of that does:
>
>         if (handle != handle_bad_irq && is_chained) {
>                 irq_settings_set_noprobe(desc);
>                 irq_settings_set_norequest(desc);
>                 irq_settings_set_nothread(desc);
>                 desc->action = &chained_action;
>                 irq_startup(desc, true);
>         }
>
> and indeed, I see that it gets enabled on Assabet.  That irq_startup()
> should result in the irqchip's irq_startup, irq_enable, or irq_unmask
> method being called.
In my case, irq_startup() is never called for irq 305, ie. LUBBOCK_SA1111_IRQ.
See deep down the mail for the cause ...

> So, that should result in the IRQ to which the sa1111 is connected
> being unmasked.
Hmm, it never happens to me ...

> Chained interrupts don't appear in /proc/interrupts.
Ah ok.

>> I traced the code in the interrupt controller (pxa_cplds_irqs), the SA1111
>> physical line is not set (even with an AT/2 keyboard connected, and I don't
>> think anybody tried this AT/2 on a lubbock for a long time).
>
> Hmm.  Looking at pxa_cplds_irqs, that's trying to support the CPLD
> interrupts using a very very old and inefficient technique.  The
> whole point of the chained interrupt system is to avoid several
> overheads in the system...  I'm curious why PXA moved away from that.
I think I traced that in the commit message of :
aa8d6b73ea33 ("ARM: pxa: pxa_cplds: add lubbock and mainstone IO")

The concern I had was that the former mechanism was not working anymore as the
chained handler was overwritten in gpio-pxa.c, and I wanted that gpio be
possibly probed at device initcall time.

> One of the problems is that we end up nesting irq_entry()/irq_exit()
> which contains a lot of code, eg trying to run softirqs.  All that
> stuff is pure overhead because we'll never get to run anything like
> that in this path.
>
> I'm also very concerned that the conversion is wrong.  The old code
> has this comment in the irq_unmask function:
>
> -       /* the irq can be acknowledged only if deasserted, so it's done here */
> -       LUB_IRQ_SET_CLR &= ~(1 << lubbock_irq);
>
> In other words, we "acknowledge" (really clear the latched status) of
> the interrupt _after_ we've serviced the peripheral, not before.
> The new code tries to clear down the interrupt when masking it - in
> other words, before the peripheral handler has had a chance to clear
> down its interrupt.

I will check it more carefully. I remember having made tests, and cross-checked
the user manual of the Cotulla Development board, chapter 3.4 "Managing
Peripheral Interrupts", where there is no restriction on when the latched status
can be cleared.

I understand the concern that clearing the status before the interrupt source is
serviced can re-set the status during the peripheral handler, if it's a level
interrupt.

Maybe it worked so far by sheer luck, or because the ethernet card was the only
really tested interrupt.

> I suspect you're seeing about twice as many interrupt counts on your
> ethernet interface because of this - you'll be entering the handler
> once for the real interrupt, but because the clear-down was ineffective,
> you end up re-entering it a second time when hopefully the peripheral
> is no longer asserting the interrupt.
Mmm I will check, but see 2 cat /proc/interrupts in a row :
307:       9384  pxa_cplds   3 Edge      eth0
307:       9417  pxa_cplds   3 Edge      eth0

The difference is not a multiple of 2. I will check anyway.

>> The interrupt is for sure masked, and therefore the SA1111 interrupts are not
>> fired. Even if they would have been enabled, the "pending interrupts register"
>> doesn't show any sa1111 interrupt pending, so there is something else.
>
> Masked where though - in the SA1111 or FPGA?
In the FPGA.

>> As I don't know if "enable_irq()" in sa1111.c would be an option as I have no
>> sight on sa1111.c requirements, I would take any advice here.
>
> It shouldn't be required, as I say above, irq_set_chained_handler_and_data()
> deals with unmasking the IRQ already.
Ah I think I know what happens. The trick is that when this
irq_set_chained_handler_and_data() is called, the irq_desc for irq 305 is not
yet allocated (ie. is NULL). And it is not allocated because
pxa_cplds_irqs.cplds_probe() was not yet called, and had not allocated the irq
descriptors.

I would bet that on neponset the input interrupt to the SA1111 is within
0..NR_IRQS, which is not the case on lubbock anymore since the pxa_cplds_irqs
conversion.

It looks that I have an ordering problem :
 - I want gpio-pxa.probe() to be called at device initcall time
 - pxa_cplds_irqs.probe() cannot complete before gpio-pxa.probe() because it
   needs GPIO0 as its interrupt source
   => it might need to honor -EPROBE_DEFER
 - sa1111.sa1111_probe() cannot complete before pxa_cplds_irqs.probe() because
   it needs IRQ305 as its source
   => it might need to honor -EPROBE_DEFER

I fail to see how the "optimized" irq chained handler can be used in a
device-tree type build, where I don't think the probe ordering is guaranteed.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html