Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Wed, May 13, 2015 at 4:25 AM, Feng Kan <fkan@apm.com> wrote:

> Marc:
>
> Sorry for top posting. It seems with GIC-400, the get_irqstate is not a reliable
> way of accessing the GPIO status. In our case, the gpio interrupt status can
> only be read correctly with the SPISR register. So it seems we have to go
> back to the old way of mapping the SPISR portion of the GIC in our GPIO
> driver. We wanted to ask you if that is acceptable since SPISR in not
> standard in all GIC, perhaps we can treat it differently.

As GPIO maintainer I am curious about what this is...

Can you describe the scenario where your GPIO (!) driver needs
to be poking around in the GIC SPISR?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Wed, May 13, 2015 at 4:58 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 13, 2015 at 4:25 AM, Feng Kan <fkan@apm.com> wrote:
>
>> Marc:
>>
>> Sorry for top posting. It seems with GIC-400, the get_irqstate is not a reliable
>> way of accessing the GPIO status. In our case, the gpio interrupt status can
>> only be read correctly with the SPISR register. So it seems we have to go
>> back to the old way of mapping the SPISR portion of the GIC in our GPIO
>> driver. We wanted to ask you if that is acceptable since SPISR in not
>> standard in all GIC, perhaps we can treat it differently.
>
> As GPIO maintainer I am curious about what this is...
>
> Can you describe the scenario where your GPIO (!) driver needs
> to be poking around in the GIC SPISR?

We are using the gpio driver with gpio_key for power button. The gpio
status can only be retrieved from the GIC register when the gpio is
acting as a interrupt. Marc had provided a patch which allow the
getting of the irq status
using the PENDING_SET register, however it seems it is not reliable.
Another register in the GIC call the SPISR always give the correct
status but it is only available in the GIC-400 and hence not likely to
be incorporated into the general GIC code. We would like to go back
the version 1 of the patch and ioremap the region in the gpio driver
and access the status that way. Please let us know what you think.

>
> Yours,
> Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Wed, May 13, 2015 at 5:44 PM, Feng Kan <fkan@apm.com> wrote:

> We are using the gpio driver with gpio_key for power button. The gpio
> status can only be retrieved from the GIC register when the gpio is
> acting as a interrupt.

OK I understand so much. SPISR is a status register of the state
of the IRQ lines of shared peripherals.

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

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

Techically the GIC would normally be higher up the food
chain, i.e. one IRQ on the GIC is cascaded to sub-IRQs on
the GPIO chip by virtue of its own struct irq_chip and
irqdomain. The exception is typically only systems where
the GPIO block is fused with the IRQ controller so that each
GPIO line has its own unique IRQ line on the primary,
top-level interrupt controller.

Which GPIO driver is this? Is it upstream?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 13, 2015 at 5:44 PM, Feng Kan <fkan@apm.com> wrote:
>
>> We are using the gpio driver with gpio_key for power button. The gpio
>> status can only be retrieved from the GIC register when the gpio is
>> acting as a interrupt.
>
> OK I understand so much. SPISR is a status register of the state
> of the IRQ lines of shared peripherals.
>
> 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.
>
> I can think of a few reasons, like transient IRQs etc but
> what is actually causing this?

I won't say the obvious.
>
> Techically the GIC would normally be higher up the food
> chain, i.e. one IRQ on the GIC is cascaded to sub-IRQs on
> the GPIO chip by virtue of its own struct irq_chip and
> irqdomain. The exception is typically only systems where
> the GPIO block is fused with the IRQ controller so that each
> GPIO line has its own unique IRQ line on the primary,
> top-level interrupt controller.
>
> 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.
>
> Yours,
> Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

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

>> 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;

Here it reads the status from 0x300 where DIST_ACTIVE_SET
is, so if you change GIC_DIST_ACTIVE_SET to
0xd04 (SPISR), does it start working?

I would *guess* that maybe you want to implement and
ask for IRQCHIP_STATE_LINE_LEVEL instead of
IRQCHIP_STATE_ACTIVE, and have that 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.

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

On Tue, 19 May 2015 09:40:21 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> >> 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.
> >>
> >> 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.
> 
> >> 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).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Tue, May 19, 2015 at 12:01 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> 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).

That's a good point. Is it one of these usual cases of hardware
thrown over the wall and the HW engineer mostly yawning or
rolling his eyes if we ask things like this?

Or is there someone who can actually answer the call?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Tue, 19 May 2015 09:40:21 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
>> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> >> 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.
>> >>
>> >> 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.
>>
>> >> 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.



>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

On Tue, 19 May 2015 22:45:26 +0100
Feng Kan <fkan@apm.com> wrote:

> On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On Tue, 19 May 2015 09:40:21 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> >> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> >> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>
> >> >> 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.
> >> >>
> >> >> 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.
> >>
> >> >> 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.