On Tue, 19 May 2015 22:45:26 +0100
Feng Kan [off-list ref] wrote:

On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier [off-list ref] wrote:

quoted

On Tue, 19 May 2015 09:40:21 +0100
Linus Walleij [off-list ref] wrote:

quoted

On Thu, May 14, 2015 at 10:14 PM, Feng Kan [off-list ref] wrote:

quoted

On Thu, May 14, 2015 at 3:32 AM, Linus Walleij [off-list ref] wrote:

quoted

quoted

But surely the GPIO block has its own status register, so are
you saying that this register is unreliable?

When the GPIO is used as interrupt, the gpio block does not report the
status anymore. Which leaves us stuck with SPISR.

quoted

I can think of a few reasons, like transient IRQs etc but
what is actually causing this?

I won't say the obvious.

Yeah I see your problem now :(

I think it's better to fix the access functions so that you can
cross-call to the GIC driver to get the SPISR flag out though.
Let's see what Marc says.

quoted

quoted

Which GPIO driver is this? Is it upstream?

Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
think that we ought to switch to use some gpio poll driver rather than
using gpio-key.

There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah
that's possible. But honestly I think it's better to deal with this
problem for real because IRQ is more efficient.

So the way I perceive it this is the real problem:

+static int gic_irq_get_irqchip_state(struct irq_data *d,
+                                     enum irqchip_irq_state which, bool *val)
+{
+       switch (which) {
(...)
+       case IRQCHIP_STATE_ACTIVE:
+               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
+               break; case: read
from 0xd04 (SPISR) instead, because that makes more
sense to me, or am I wrong at it?

+       case IRQCHIP_STATE_LINE_LEVEL:
+               *val = gic_peek_irq(d, GIC_DIST_SPISR);
+               break;

And then put a define into <linux/irqchip/arm-gic.h> for
GIC_DIST_SPISR.

What worries me here is that the PENDING state should already give you
the right level of information (this is what the GIC-400 TRM says). The
only reason why SPISR exists is that software can write to the PENDING
register, while SPISR is RO.

If reading the pending state doesn't work, then I'd like to know
exactly *why*. Only then we can add support for LINE_LEVEL using SPISR
(which has to be GIC-400 specific, as it is not architected).

IS_PENDING and IS_ACTIVE works fine for the ISR context. However,
the nature of the register is meant for IRQ handling and not to read
the status of a GPIO. By the time the gpio_key throws a work queue
and check the status of the PENDING register, it is no long relevant.

This is far more subtle that that. Assuming a level-triggered
interrupt, the PENDING state is completely independent of the state of
the interrupt (see the GIC state machine), and will accurately reflect
the input line (an edge-triggered interrupt is of course a different
matter). As outlined in the GIC-400 TRM, both GICD_ICPENDINGRn,
GICD_ISPENDINGRn and GICD_SPISRn are providing the same information
(the only difference being that SPISRn is offering a RO view before the
latches).

SPISRn is essentially a debug register, not something that is meant to
be used in normal code, GPIO or otherwise. So assuming we have an
actual GIC-400 implementation here, I want to know why this isn't
working, and you are in a position to tell me (I assume you can talk to
the HW people that did the integration).

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.