Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Nov 11, 2013 at 8:33 PM, Stephen Warren  wrote:
> On 11/11/2013 12:17 PM, Gerlando Falauto wrote:

>> Thanks for your answer. So it should be the parent driver to proactively
>> configure the GPIO as input, active high etc..., when an IRQ for its
>> GPIOs is requested, right?
>
> That was the conclusion of this thread, or a similar thread, yes.

Yepps second that.

And we added an API to that you can flag a GPIO as being
in use by an IRQ using gpio_lock_as_irq() from your
irq_chip callbacks.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Nov 11, 2013 at 8:33 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 11/11/2013 12:17 PM, Gerlando Falauto wrote:

 Thanks for your answer. So it should be the parent driver to proactively
 configure the GPIO as input, active high etc..., when an IRQ for its
 GPIOs is requested, right?

 That was the conclusion of this thread, or a similar thread, yes.

Yepps second that.

And we added an API to that you can flag a GPIO as being
in use by an IRQ using gpio_lock_as_irq() from your
irq_chip callbacks.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

On Monday 11 of November 2013 12:33:20 Stephen Warren wrote:
> On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
> > Hi Stephan,
> > 
> > On 11/11/2013 07:53 PM, Stephen Warren wrote:
> >> On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
> >>> Hi everyone,
> >>>
> >>> [jumping in on an old discussion]
> >>>
> >>> On 09/09/2013 06:19 PM, Mark Brown wrote:
>  On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> > On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> 
> >> The driver that tries to use the GPIO requested by this patch before
> >> HAS to
> >> fail. This is exactly the intention of this patch. We don't want the
> >> GPIO to
> >> be requested any more, if it is used as an interrupt pin.
> 
> > That will break existing drivers. There are drivers that request the
> > same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> > that way.
> 
>  Yes, plus input devices and audio jack detection among others.  This
>  pattern is very common if the GPIO is actually being used as a GPIO, an
>  edge triggered interrupt is used to flag when something happens and the
>  state is determined by reading the GPIO state (often with some
>  debounce).
> >>>
> >>> I actually came across this thread while looking for an answer to the
> >>> following (apparently trivial) question:
> >>>
> >>> If you were to write a new driver & binding, what would be, in general,
> >>> the recommended DT binding for a cascade interrupt controller (or any
> >>> other peripheral, for that matter), which is connected through a GPIO
> >>> (to be used as IRQ)?
> >>>
> >>> a) Through gpios = < N>
> >>> b) through interrupt-parent = <> & interrupts  >>> IRQ_TYPE_LEVEL_LOW>, or
> >>> c) both?
> >>
> >> (b) alone.
> >>
> >>  From the perspective of the child interrupt controller, its output is
> >> purely an interrupt. The fact that the parent interrupt controller could
> >> use that pin as a GPIO in some other context (e.g. on a different board)
> >> isn't something that the child interrupt controller should know/care
> >> about.
> >>
> > 
> > Thanks for your answer. So it should be the parent driver to proactively
> > configure the GPIO as input, active high etc..., when an IRQ for its
> > GPIOs is requested, right?
> 
> That was the conclusion of this thread, or a similar thread, yes.

I would only add that settings, such as pull up/down control that depends
on the chip that drives the interrupt pin (i.e. the child interrupt
controller), are usually configured using pin control bindings.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
> Hi Stephan,
> 
> On 11/11/2013 07:53 PM, Stephen Warren wrote:
>> On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
>>> Hi everyone,
>>>
>>> [jumping in on an old discussion]
>>>
>>> On 09/09/2013 06:19 PM, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> On 09/04/2013 03:05 AM, Lars Poeschel wrote:

>> The driver that tries to use the GPIO requested by this patch before
>> HAS to
>> fail. This is exactly the intention of this patch. We don't want the
>> GPIO to
>> be requested any more, if it is used as an interrupt pin.

> That will break existing drivers. There are drivers that request the
> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> that way.

 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).
>>>
>>> I actually came across this thread while looking for an answer to the
>>> following (apparently trivial) question:
>>>
>>> If you were to write a new driver & binding, what would be, in general,
>>> the recommended DT binding for a cascade interrupt controller (or any
>>> other peripheral, for that matter), which is connected through a GPIO
>>> (to be used as IRQ)?
>>>
>>> a) Through gpios = < N>
>>> b) through interrupt-parent = <> & interrupts >> IRQ_TYPE_LEVEL_LOW>, or
>>> c) both?
>>
>> (b) alone.
>>
>>  From the perspective of the child interrupt controller, its output is
>> purely an interrupt. The fact that the parent interrupt controller could
>> use that pin as a GPIO in some other context (e.g. on a different board)
>> isn't something that the child interrupt controller should know/care
>> about.
>>
> 
> Thanks for your answer. So it should be the parent driver to proactively
> configure the GPIO as input, active high etc..., when an IRQ for its
> GPIOs is requested, right?

That was the conclusion of this thread, or a similar thread, yes.

> Moving to a different target (actually Mark's original point), i.e. a
> generic device which is connected through a GPIO, using the IRQ to
> trigger an event, what would be the recommended way?

Again, the device that's the interrupt source is simply emitting an
interrupt signal, so that's all it should know about.

Now, if you're talking about a device that's emitting just some random
data signal, where the driver wants to be notified of changes in the
signal, then that should be modelled as a GPIO, which the driver can
then convert internally to an IRQ and request. That's because from the
HW perspective, the signal isn't an IRQ output.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi Stephan,

On 11/11/2013 07:53 PM, Stephen Warren wrote:

On 11/11/2013 11:28 AM, Gerlando Falauto wrote:

Hi everyone,

[jumping in on an old discussion]

On 09/09/2013 06:19 PM, Mark Brown wrote:

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:

On 09/04/2013 03:05 AM, Lars Poeschel wrote:



The driver that tries to use the GPIO requested by this patch before
HAS to
fail. This is exactly the intention of this patch. We don't want the
GPIO to
be requested any more, if it is used as an interrupt pin.



That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.


Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


I actually came across this thread while looking for an answer to the
following (apparently trivial) question:

If you were to write a new driver & binding, what would be, in general,
the recommended DT binding for a cascade interrupt controller (or any
other peripheral, for that matter), which is connected through a GPIO
(to be used as IRQ)?

a) Through gpios = < N>
b) through interrupt-parent = <> & interrupts , or
c) both?


(b) alone.

 From the perspective of the child interrupt controller, its output is
purely an interrupt. The fact that the parent interrupt controller could
use that pin as a GPIO in some other context (e.g. on a different board)
isn't something that the child interrupt controller should know/care about.



Thanks for your answer. So it should be the parent driver to proactively 
configure the GPIO as input, active high etc..., when an IRQ for its 
GPIOs is requested, right?


Moving to a different target (actually Mark's original point), i.e. a 
generic device which is connected through a GPIO, using the IRQ to 
trigger an event, what would be the recommended way?


Thanks again!
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
> Hi everyone,
> 
> [jumping in on an old discussion]
> 
> On 09/09/2013 06:19 PM, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>>
 The driver that tries to use the GPIO requested by this patch before
 HAS to
 fail. This is exactly the intention of this patch. We don't want the
 GPIO to
 be requested any more, if it is used as an interrupt pin.
>>
>>> That will break existing drivers. There are drivers that request the
>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>> that way.
>>
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> I actually came across this thread while looking for an answer to the
> following (apparently trivial) question:
> 
> If you were to write a new driver & binding, what would be, in general,
> the recommended DT binding for a cascade interrupt controller (or any
> other peripheral, for that matter), which is connected through a GPIO
> (to be used as IRQ)?
> 
> a) Through gpios = < N>
> b) through interrupt-parent = <> & interrupts  IRQ_TYPE_LEVEL_LOW>, or
> c) both?

(b) alone.

>From the perspective of the child interrupt controller, its output is
purely an interrupt. The fact that the parent interrupt controller could
use that pin as a GPIO in some other context (e.g. on a different board)
isn't something that the child interrupt controller should know/care about.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi everyone,

[jumping in on an old discussion]

On 09/09/2013 06:19 PM, Mark Brown wrote:

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:

On 09/04/2013 03:05 AM, Lars Poeschel wrote:



The driver that tries to use the GPIO requested by this patch before HAS to
fail. This is exactly the intention of this patch. We don't want the GPIO to
be requested any more, if it is used as an interrupt pin.



That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.


Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


I actually came across this thread while looking for an answer to the 
following (apparently trivial) question:


If you were to write a new driver & binding, what would be, in general, 
the recommended DT binding for a cascade interrupt controller (or any 
other peripheral, for that matter), which is connected through a GPIO 
(to be used as IRQ)?


a) Through gpios = < N>
b) through interrupt-parent = <> & interrupts IRQ_TYPE_LEVEL_LOW>, or

c) both?

Thanks!
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi everyone,

[jumping in on an old discussion]

On 09/09/2013 06:19 PM, Mark Brown wrote:

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:

On 09/04/2013 03:05 AM, Lars Poeschel wrote:



The driver that tries to use the GPIO requested by this patch before HAS to
fail. This is exactly the intention of this patch. We don't want the GPIO to
be requested any more, if it is used as an interrupt pin.



That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.


Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


I actually came across this thread while looking for an answer to the 
following (apparently trivial) question:


If you were to write a new driver  binding, what would be, in general, 
the recommended DT binding for a cascade interrupt controller (or any 
other peripheral, for that matter), which is connected through a GPIO 
(to be used as IRQ)?


a) Through gpios = gpio0 N
b) through interrupt-parent = gpio0  interrupts N 
IRQ_TYPE_LEVEL_LOW, or

c) both?

Thanks!
Gerlando
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
 Hi everyone,
 
 [jumping in on an old discussion]
 
 On 09/09/2013 06:19 PM, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
 On 09/04/2013 03:05 AM, Lars Poeschel wrote:

 The driver that tries to use the GPIO requested by this patch before
 HAS to
 fail. This is exactly the intention of this patch. We don't want the
 GPIO to
 be requested any more, if it is used as an interrupt pin.

 That will break existing drivers. There are drivers that request the
 same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
 that way.

 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).
 
 I actually came across this thread while looking for an answer to the
 following (apparently trivial) question:
 
 If you were to write a new driver  binding, what would be, in general,
 the recommended DT binding for a cascade interrupt controller (or any
 other peripheral, for that matter), which is connected through a GPIO
 (to be used as IRQ)?
 
 a) Through gpios = gpio0 N
 b) through interrupt-parent = gpio0  interrupts N
 IRQ_TYPE_LEVEL_LOW, or
 c) both?

(b) alone.

From the perspective of the child interrupt controller, its output is
purely an interrupt. The fact that the parent interrupt controller could
use that pin as a GPIO in some other context (e.g. on a different board)
isn't something that the child interrupt controller should know/care about.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi Stephan,

On 11/11/2013 07:53 PM, Stephen Warren wrote:

On 11/11/2013 11:28 AM, Gerlando Falauto wrote:

Hi everyone,

[jumping in on an old discussion]

On 09/09/2013 06:19 PM, Mark Brown wrote:

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:

On 09/04/2013 03:05 AM, Lars Poeschel wrote:



The driver that tries to use the GPIO requested by this patch before
HAS to
fail. This is exactly the intention of this patch. We don't want the
GPIO to
be requested any more, if it is used as an interrupt pin.



That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.


Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


I actually came across this thread while looking for an answer to the
following (apparently trivial) question:

If you were to write a new driver  binding, what would be, in general,
the recommended DT binding for a cascade interrupt controller (or any
other peripheral, for that matter), which is connected through a GPIO
(to be used as IRQ)?

a) Through gpios = gpio0 N
b) through interrupt-parent = gpio0  interrupts N
IRQ_TYPE_LEVEL_LOW, or
c) both?


(b) alone.

 From the perspective of the child interrupt controller, its output is
purely an interrupt. The fact that the parent interrupt controller could
use that pin as a GPIO in some other context (e.g. on a different board)
isn't something that the child interrupt controller should know/care about.



Thanks for your answer. So it should be the parent driver to proactively 
configure the GPIO as input, active high etc..., when an IRQ for its 
GPIOs is requested, right?


Moving to a different target (actually Mark's original point), i.e. a 
generic device which is connected through a GPIO, using the IRQ to 
trigger an event, what would be the recommended way?


Thanks again!
Gerlando
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
 Hi Stephan,
 
 On 11/11/2013 07:53 PM, Stephen Warren wrote:
 On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
 Hi everyone,

 [jumping in on an old discussion]

 On 09/09/2013 06:19 PM, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
 On 09/04/2013 03:05 AM, Lars Poeschel wrote:

 The driver that tries to use the GPIO requested by this patch before
 HAS to
 fail. This is exactly the intention of this patch. We don't want the
 GPIO to
 be requested any more, if it is used as an interrupt pin.

 That will break existing drivers. There are drivers that request the
 same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
 that way.

 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).

 I actually came across this thread while looking for an answer to the
 following (apparently trivial) question:

 If you were to write a new driver  binding, what would be, in general,
 the recommended DT binding for a cascade interrupt controller (or any
 other peripheral, for that matter), which is connected through a GPIO
 (to be used as IRQ)?

 a) Through gpios = gpio0 N
 b) through interrupt-parent = gpio0  interrupts N
 IRQ_TYPE_LEVEL_LOW, or
 c) both?

 (b) alone.

  From the perspective of the child interrupt controller, its output is
 purely an interrupt. The fact that the parent interrupt controller could
 use that pin as a GPIO in some other context (e.g. on a different board)
 isn't something that the child interrupt controller should know/care
 about.

 
 Thanks for your answer. So it should be the parent driver to proactively
 configure the GPIO as input, active high etc..., when an IRQ for its
 GPIOs is requested, right?

That was the conclusion of this thread, or a similar thread, yes.

 Moving to a different target (actually Mark's original point), i.e. a
 generic device which is connected through a GPIO, using the IRQ to
 trigger an event, what would be the recommended way?

Again, the device that's the interrupt source is simply emitting an
interrupt signal, so that's all it should know about.

Now, if you're talking about a device that's emitting just some random
data signal, where the driver wants to be notified of changes in the
signal, then that should be modelled as a GPIO, which the driver can
then convert internally to an IRQ and request. That's because from the
HW perspective, the signal isn't an IRQ output.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

On Monday 11 of November 2013 12:33:20 Stephen Warren wrote:
 On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
  Hi Stephan,
  
  On 11/11/2013 07:53 PM, Stephen Warren wrote:
  On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
  Hi everyone,
 
  [jumping in on an old discussion]
 
  On 09/09/2013 06:19 PM, Mark Brown wrote:
  On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
  On 09/04/2013 03:05 AM, Lars Poeschel wrote:
 
  The driver that tries to use the GPIO requested by this patch before
  HAS to
  fail. This is exactly the intention of this patch. We don't want the
  GPIO to
  be requested any more, if it is used as an interrupt pin.
 
  That will break existing drivers. There are drivers that request the
  same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
  that way.
 
  Yes, plus input devices and audio jack detection among others.  This
  pattern is very common if the GPIO is actually being used as a GPIO, an
  edge triggered interrupt is used to flag when something happens and the
  state is determined by reading the GPIO state (often with some
  debounce).
 
  I actually came across this thread while looking for an answer to the
  following (apparently trivial) question:
 
  If you were to write a new driver  binding, what would be, in general,
  the recommended DT binding for a cascade interrupt controller (or any
  other peripheral, for that matter), which is connected through a GPIO
  (to be used as IRQ)?
 
  a) Through gpios = gpio0 N
  b) through interrupt-parent = gpio0  interrupts N
  IRQ_TYPE_LEVEL_LOW, or
  c) both?
 
  (b) alone.
 
   From the perspective of the child interrupt controller, its output is
  purely an interrupt. The fact that the parent interrupt controller could
  use that pin as a GPIO in some other context (e.g. on a different board)
  isn't something that the child interrupt controller should know/care
  about.
 
  
  Thanks for your answer. So it should be the parent driver to proactively
  configure the GPIO as input, active high etc..., when an IRQ for its
  GPIOs is requested, right?
 
 That was the conclusion of this thread, or a similar thread, yes.

I would only add that settings, such as pull up/down control that depends
on the chip that drives the interrupt pin (i.e. the child interrupt
controller), are usually configured using pin control bindings.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Sep 24, 2013 at 6:59 PM, Stephen Warren  wrote:
> On 09/24/2013 02:31 AM, Linus Walleij wrote:
>> On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren  
>> wrote:
> ...
>>> Perhaps rather than having the gpio_chip/irq_chip drivers physically
>>> implement a function which calls this common code, they could set some
>>> flags/data/... in the struct gpio_chip/irq_chip indicating that they
>>> desire the core code that implements the error-checking to be enabled.
>>
>> I think it should more be like a function they can call to flag
>> a GPIO as used for IRQ.
>
> For the record, that's pretty much exactly what I meant by implementing
> it in the drivers. The irq_chip driver knows when the IRQ has been
> requested, and calls some gpiolib function to mark the GPIO as
> in-use-as-an-IRQ.

Ah OK we're on the same page, this is progressing :-)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Sep 24, 2013 at 6:59 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 09/24/2013 02:31 AM, Linus Walleij wrote:
 On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 ...
 Perhaps rather than having the gpio_chip/irq_chip drivers physically
 implement a function which calls this common code, they could set some
 flags/data/... in the struct gpio_chip/irq_chip indicating that they
 desire the core code that implements the error-checking to be enabled.

 I think it should more be like a function they can call to flag
 a GPIO as used for IRQ.

 For the record, that's pretty much exactly what I meant by implementing
 it in the drivers. The irq_chip driver knows when the IRQ has been
 requested, and calls some gpiolib function to mark the GPIO as
 in-use-as-an-IRQ.

Ah OK we're on the same page, this is progressing :-)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:31 AM, Linus Walleij wrote:
> On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren  
> wrote:
...
>> Perhaps rather than having the gpio_chip/irq_chip drivers physically
>> implement a function which calls this common code, they could set some
>> flags/data/... in the struct gpio_chip/irq_chip indicating that they
>> desire the core code that implements the error-checking to be enabled.
> 
> I think it should more be like a function they can call to flag
> a GPIO as used for IRQ.

For the record, that's pretty much exactly what I meant by implementing
it in the drivers. The irq_chip driver knows when the IRQ has been
requested, and calls some gpiolib function to mark the GPIO as
in-use-as-an-IRQ.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:26 AM, Linus Walleij wrote:
> On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren  
> wrote:
>> On 09/23/2013 01:53 PM, Linus Walleij wrote:
> 
>>> I think the kernel should prevent such things.
>>
>> It might be nice if it could do that.
>>
>> However, that is 100% unrelated to the problem at hand.
> 
> I don't think it is unrelated when the old OMAP boardfile-based
> code definately prevents such uses by its strict usage
> of gpio_request() for all IRQ-bound GPIOs.
> 
> I think not preventing it for the DT boot path is setting lower
> standards for DT code than for boardfile code which is not
> what we should be doing.

Semantics matter.

In the old board file code, the gpio_request()s were present to work
around the bug in the OMAP driver where request_irq() wouldn't configure
the IRQ signal correctly. That's the primary reason those calls were there.

Now, this had the side-effect of also preventing anything else from
calling gpio_request() on those GPIOs, but that wasn't the primary
motivation; just a convenient effect.

...
> Solving the issue that e.g. two different drivers competing about the
> same resource (as in one driver requesting an IRQ and another one
> requesting a GPIO) is not what I'm after here.
> 
> I'm more after the GPIO subsystem having knowledge of a certain
> GPIO line being requested for IRQ, and denying that line to be set
> as input.

s/input/output/ I assume.

...
> Maybe this can actually be achieved quite easily with
> an additional API? Like gpio_lock_as_irq(gpio) which flags this
> in .flags of struct gpio_desc and prevent such things?
> 
> Alexandre what do you think about this idea?
> 
>> Equally, I am actually not 100% sure we want the core to prevent this.
>> Why shouldn't two different drivers request the same IRQ? Why shouldn't
>> at least one driver, perhaps more, request the pin as a GPIO (assuming
>> it will only read the GPIO value, not flip the pin to output).
> 
> But I have already stated that this is OK?
> 
> Are we talking past each other now?

If all you want to do is prevent gpio_direction_input() on a GPIO that's
in use as a GPIO, then that's probably OK.

However, the interrupt consistency patch that was posted implemented
that restriction by calling gpio_request(), and the wording of most of
what you've written implies to me that implementing the restriction by
calling gpio_request() is what you're after. That approach imposes far
more restrictions than just preventing gpio_direction_input(). Imposing
those additional restrictions is what I'm objecting to.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren  wrote:
> On 09/23/2013 02:01 PM, Linus Walleij wrote:

>> And how to you block the same line from being gpio_request()ed
>> and set as output?
>
> To be honest, I really don't think this problem is terribly likely to
> occur, so I'm really not convinced that it's worth putting a lot of
> effort into solving it.

I have a different opionion, I think this is important. As GPIO
subsystem maintainer I need to be convinced of the integrity
of the system.

> If the problem does occur, it's trivial to see that this has happened by
> looking at /proc/interrupts and /sys/kernel/debug/gpio,

I basically want /sys/kernel/debug/gpio to say if a certain line
is tied for IRQ.

> That driver needs to maintain some state that indicates which of its
> IRQs have been requested. Any time a GPIO request (I mean e.g.
> set_output() not request_gpio)) comes in, the request needs to be
> validated against that IRQ usage state. If there's a conflict, deny the
> GPIO request.

I think this should be done by gpiolib, and I think it is trivial
given the right APIs. Putting it in the drivers will just create
an array of similar-look Rube Goldberg-machines and code
duplication. There is no point.

> Now, it's quite possible that the code to maintain this state and
> perform the checks will be similar/common across multiple drivers. If
> so, by all means implement the code somewhere common, and have the
> various irq_chip/gpio_chip drivers call into it.

And this is what we should do in gpiolib.

> The main thing is that all of this has to be driven/controlled/enabled
> by the gpio_chip/irq_chip driver itself, not as some global/blanket
> feature of the GPIO or IRQ subsystems.

Sure.

> Perhaps rather than having the gpio_chip/irq_chip drivers physically
> implement a function which calls this common code, they could set some
> flags/data/... in the struct gpio_chip/irq_chip indicating that they
> desire the core code that implements the error-checking to be enabled.

I think it should more be like a function they can call to flag
a GPIO as used for IRQ.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren  wrote:
> On 09/23/2013 01:53 PM, Linus Walleij wrote:

>> I think the kernel should prevent such things.
>
> It might be nice if it could do that.
>
> However, that is 100% unrelated to the problem at hand.

I don't think it is unrelated when the old OMAP boardfile-based
code definately prevents such uses by its strict usage
of gpio_request() for all IRQ-bound GPIOs.

I think not preventing it for the DT boot path is setting lower
standards for DT code than for boardfile code which is not
what we should be doing.

> A driver which only cares about an IRQ should be able to call just IRQ
> APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
> causes the HW to work should not involve the GPIO subsystem in any way
> at all.

Yes I have bought into that concept now.

> Having the kernel detect when two different drivers both request the
> same resource is entirely another thing. The solution to the first issue
> must not rely on any solution to this second issue.

I understand this stance from a DT point of view - which is about
resource passing and its syntax and semantics.

>From a GPIO subsystem point of view, in keeping resources under
kernel control, I naturally do not agree.

> I'm also not convinced it's possible to solve this second issue given
> the current kernel APIs, since there's not enough semantic information;
> requests of GPIOs and IRQs aren't actually tied to a particular driver
> at present (there's no "struct device *dev" parameter to request_irq or
> gpio_request) and so the subsystems can't actually tell who is
> requesting the GPIO/IRQ, and hence can't detect when the same driver, or
> a different driver, is requesting the same core resource for different
> purposes.

Solving the issue that e.g. two different drivers competing about the
same resource (as in one driver requesting an IRQ and another one
requesting a GPIO) is not what I'm after here.

I'm more after the GPIO subsystem having knowledge of a certain
GPIO line being requested for IRQ, and denying that line to be set
as input.

Maybe this can actually be achieved quite easily with
an additional API? Like gpio_lock_as_irq(gpio) which flags this
in .flags of struct gpio_desc and prevent such things?

Alexandre what do you think about this idea?

> Equally, I am actually not 100% sure we want the core to prevent this.
> Why shouldn't two different drivers request the same IRQ? Why shouldn't
> at least one driver, perhaps more, request the pin as a GPIO (assuming
> it will only read the GPIO value, not flip the pin to output).

But I have already stated that this is OK?

Are we talking past each other now?

> This
> exact situation might happen on some Tegra boards where there's a GPIO
> for VBUS_EN that affects 2 USB ports. It's supposed to be driven
> open-collector. If an external entity forces it low, it means
> over-current.

You are describing a very good reason for the core to be
doing exactly what I described I think?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 09/23/2013 01:53 PM, Linus Walleij wrote:

 I think the kernel should prevent such things.

 It might be nice if it could do that.

 However, that is 100% unrelated to the problem at hand.

I don't think it is unrelated when the old OMAP boardfile-based
code definately prevents such uses by its strict usage
of gpio_request() for all IRQ-bound GPIOs.

I think not preventing it for the DT boot path is setting lower
standards for DT code than for boardfile code which is not
what we should be doing.

 A driver which only cares about an IRQ should be able to call just IRQ
 APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
 causes the HW to work should not involve the GPIO subsystem in any way
 at all.

Yes I have bought into that concept now.

 Having the kernel detect when two different drivers both request the
 same resource is entirely another thing. The solution to the first issue
 must not rely on any solution to this second issue.

I understand this stance from a DT point of view - which is about
resource passing and its syntax and semantics.

From a GPIO subsystem point of view, in keeping resources under
kernel control, I naturally do not agree.

 I'm also not convinced it's possible to solve this second issue given
 the current kernel APIs, since there's not enough semantic information;
 requests of GPIOs and IRQs aren't actually tied to a particular driver
 at present (there's no struct device *dev parameter to request_irq or
 gpio_request) and so the subsystems can't actually tell who is
 requesting the GPIO/IRQ, and hence can't detect when the same driver, or
 a different driver, is requesting the same core resource for different
 purposes.

Solving the issue that e.g. two different drivers competing about the
same resource (as in one driver requesting an IRQ and another one
requesting a GPIO) is not what I'm after here.

I'm more after the GPIO subsystem having knowledge of a certain
GPIO line being requested for IRQ, and denying that line to be set
as input.

Maybe this can actually be achieved quite easily with
an additional API? Like gpio_lock_as_irq(gpio) which flags this
in .flags of struct gpio_desc and prevent such things?

Alexandre what do you think about this idea?

 Equally, I am actually not 100% sure we want the core to prevent this.
 Why shouldn't two different drivers request the same IRQ? Why shouldn't
 at least one driver, perhaps more, request the pin as a GPIO (assuming
 it will only read the GPIO value, not flip the pin to output).

But I have already stated that this is OK?

Are we talking past each other now?

 This
 exact situation might happen on some Tegra boards where there's a GPIO
 for VBUS_EN that affects 2 USB ports. It's supposed to be driven
 open-collector. If an external entity forces it low, it means
 over-current.

You are describing a very good reason for the core to be
doing exactly what I described I think?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 09/23/2013 02:01 PM, Linus Walleij wrote:

 And how to you block the same line from being gpio_request()ed
 and set as output?

 To be honest, I really don't think this problem is terribly likely to
 occur, so I'm really not convinced that it's worth putting a lot of
 effort into solving it.

I have a different opionion, I think this is important. As GPIO
subsystem maintainer I need to be convinced of the integrity
of the system.

 If the problem does occur, it's trivial to see that this has happened by
 looking at /proc/interrupts and /sys/kernel/debug/gpio,

I basically want /sys/kernel/debug/gpio to say if a certain line
is tied for IRQ.

 That driver needs to maintain some state that indicates which of its
 IRQs have been requested. Any time a GPIO request (I mean e.g.
 set_output() not request_gpio)) comes in, the request needs to be
 validated against that IRQ usage state. If there's a conflict, deny the
 GPIO request.

I think this should be done by gpiolib, and I think it is trivial
given the right APIs. Putting it in the drivers will just create
an array of similar-look Rube Goldberg-machines and code
duplication. There is no point.

 Now, it's quite possible that the code to maintain this state and
 perform the checks will be similar/common across multiple drivers. If
 so, by all means implement the code somewhere common, and have the
 various irq_chip/gpio_chip drivers call into it.

And this is what we should do in gpiolib.

 The main thing is that all of this has to be driven/controlled/enabled
 by the gpio_chip/irq_chip driver itself, not as some global/blanket
 feature of the GPIO or IRQ subsystems.

Sure.

 Perhaps rather than having the gpio_chip/irq_chip drivers physically
 implement a function which calls this common code, they could set some
 flags/data/... in the struct gpio_chip/irq_chip indicating that they
 desire the core code that implements the error-checking to be enabled.

I think it should more be like a function they can call to flag
a GPIO as used for IRQ.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:26 AM, Linus Walleij wrote:
 On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 On 09/23/2013 01:53 PM, Linus Walleij wrote:
 
 I think the kernel should prevent such things.

 It might be nice if it could do that.

 However, that is 100% unrelated to the problem at hand.
 
 I don't think it is unrelated when the old OMAP boardfile-based
 code definately prevents such uses by its strict usage
 of gpio_request() for all IRQ-bound GPIOs.
 
 I think not preventing it for the DT boot path is setting lower
 standards for DT code than for boardfile code which is not
 what we should be doing.

Semantics matter.

In the old board file code, the gpio_request()s were present to work
around the bug in the OMAP driver where request_irq() wouldn't configure
the IRQ signal correctly. That's the primary reason those calls were there.

Now, this had the side-effect of also preventing anything else from
calling gpio_request() on those GPIOs, but that wasn't the primary
motivation; just a convenient effect.

...
 Solving the issue that e.g. two different drivers competing about the
 same resource (as in one driver requesting an IRQ and another one
 requesting a GPIO) is not what I'm after here.
 
 I'm more after the GPIO subsystem having knowledge of a certain
 GPIO line being requested for IRQ, and denying that line to be set
 as input.

s/input/output/ I assume.

...
 Maybe this can actually be achieved quite easily with
 an additional API? Like gpio_lock_as_irq(gpio) which flags this
 in .flags of struct gpio_desc and prevent such things?
 
 Alexandre what do you think about this idea?
 
 Equally, I am actually not 100% sure we want the core to prevent this.
 Why shouldn't two different drivers request the same IRQ? Why shouldn't
 at least one driver, perhaps more, request the pin as a GPIO (assuming
 it will only read the GPIO value, not flip the pin to output).
 
 But I have already stated that this is OK?
 
 Are we talking past each other now?

If all you want to do is prevent gpio_direction_input() on a GPIO that's
in use as a GPIO, then that's probably OK.

However, the interrupt consistency patch that was posted implemented
that restriction by calling gpio_request(), and the wording of most of
what you've written implies to me that implementing the restriction by
calling gpio_request() is what you're after. That approach imposes far
more restrictions than just preventing gpio_direction_input(). Imposing
those additional restrictions is what I'm objecting to.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:31 AM, Linus Walleij wrote:
 On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
...
 Perhaps rather than having the gpio_chip/irq_chip drivers physically
 implement a function which calls this common code, they could set some
 flags/data/... in the struct gpio_chip/irq_chip indicating that they
 desire the core code that implements the error-checking to be enabled.
 
 I think it should more be like a function they can call to flag
 a GPIO as used for IRQ.

For the record, that's pretty much exactly what I meant by implementing
it in the drivers. The irq_chip driver knows when the IRQ has been
requested, and calls some gpiolib function to mark the GPIO as
in-use-as-an-IRQ.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 02:01 PM, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren  wrote:
> 
>> Put another way, I don't believe
>> there's any rule when writing DT bindings that states that bindings must
>> not describe the same pin as both a GPIO and an IRQ, although admittedly
>> that may be unusual.
> 
> Actually I think you've won me over here.
> 
> But I do not think that this shall be done at the expense of letting
> DT-based drivers do nasty things like setting up the same GPIO
> line as an IRQ (hammering the hardware to do that) and then
> request the same line to be an output line and drive it, for
> example.
> 
> So the state of the GPIO line has to be tracked: if it is set as
> input and tied to an IRQ it should *not* be possible for the
> other code path to request it and set it as output. But it should
> be possible to set it as input again and read the value.
> 
> Laurent deascribed exactly the latter usecase, which is OK.
> 
>>> I agree with you that it would be the best if the only call would be
>>> request_irq and the chip driver programs the HW appropriately. It would be a
>>> dream, but unfortunately this is not possible at the moment. This is 
>>> something
>>> that Linus pointed out very very early in this whole discussion. The gpio 
>>> and
>>> irq frameworks don't share any information. The irq framework has no chance 
>>> to
>>> program the HW, because it will never find the related gpio.
>>> For this to work the frameworks have to change (and possibly all 
>>> drivers/board
>>> files/whatever using request_irq() and/or request_gpio()) have to change.
>>> That is something that I do not dare to do alone.
>>
>> This is a controller-specific issue, and has nothing to do with the GPIO
>> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
>> needs to program the HW when the IRQ is requested or set up. The Tegra
>> driver already works this way, so there's actual proof that it is
>> possible to do this in practice.
> 
> And how to you block the same line from being gpio_request()ed
> and set as output?

To be honest, I really don't think this problem is terribly likely to
occur, so I'm really not convinced that it's worth putting a lot of
effort into solving it.

If the problem does occur, it's trivial to see that this has happened by
looking at /proc/interrupts and /sys/kernel/debug/gpio, assuming the
system doesn't hang when this happens due to an interrupt storm, and if
it does, it should be pretty easy to track down the problematic register
write.

That said, I'm not outright against the kernel checking for this
situation. I think it'd work as follows:

The only code that knows the correlation between the IRQ and GPIO in
question is the combined gpio_chip/irq_chip driver. Everything /has/ to
be driven through that driver. In some cases, an IRQ may not be related
to any GPIO at all; there are certainly IRQ controllers which take
inputs directly from outside the chip, but have no GPIO functionality at
all on those signals.

That driver needs to maintain some state that indicates which of its
IRQs have been requested. Any time a GPIO request (I mean e.g.
set_output() not request_gpio)) comes in, the request needs to be
validated against that IRQ usage state. If there's a conflict, deny the
GPIO request.

Now, it's quite possible that the code to maintain this state and
perform the checks will be similar/common across multiple drivers. If
so, by all means implement the code somewhere common, and have the
various irq_chip/gpio_chip drivers call into it.

The main thing is that all of this has to be driven/controlled/enabled
by the gpio_chip/irq_chip driver itself, not as some global/blanket
feature of the GPIO or IRQ subsystems.

Perhaps rather than having the gpio_chip/irq_chip drivers physically
implement a function which calls this common code, they could set some
flags/data/... in the struct gpio_chip/irq_chip indicating that they
desire the core code that implements the error-checking to be enabled.
That might save some levels of stack. But the key is still that the
whole scenario is controlled by the end driver, not always assumed to
apply by the core code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 01:53 PM, Linus Walleij wrote:
> On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren  wrote:
> 
>> I believe this situation is exactly what cause the original patch to the
>> OMAP driver to be reverted; that patch should have touched the HW
>> directly to solve the problem when the IRQ was requested, rather than
>> calling into the GPIO subsystem (which also has the side-effect of
>> touching the HW in the same way as desired).
> 
> And that has the side-effect that this line, which is now set up
> in the HW as an input line, can be gpio_request()ed by some other
> code path and set up as output, screwing around with the very
> same registers.
> 
> I think the kernel should prevent such things.

It might be nice if it could do that.

However, that is 100% unrelated to the problem at hand.

A driver which only cares about an IRQ should be able to call just IRQ
APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
causes the HW to work should not involve the GPIO subsystem in any way
at all.

So, the first issue - making a correctly configured system functionally
work without resorting to hacks such as requesting a GPIO to make an IRQ
work, is one thing. This issue should be solved purely inside the
specific HW driver.

Having the kernel detect when two different drivers both request the
same resource is entirely another thing. The solution to the first issue
must not rely on any solution to this second issue.

I'm also not convinced it's possible to solve this second issue given
the current kernel APIs, since there's not enough semantic information;
requests of GPIOs and IRQs aren't actually tied to a particular driver
at present (there's no "struct device *dev" parameter to request_irq or
gpio_request) and so the subsystems can't actually tell who is
requesting the GPIO/IRQ, and hence can't detect when the same driver, or
a different driver, is requesting the same core resource for different
purposes.

Equally, I am actually not 100% sure we want the core to prevent this.
Why shouldn't two different drivers request the same IRQ? Why shouldn't
at least one driver, perhaps more, request the pin as a GPIO (assuming
it will only read the GPIO value, not flip the pin to output). This
exact situation might happen on some Tegra boards where there's a GPIO
for VBUS_EN that affects 2 USB ports. It's supposed to be driven
open-collector. If an external entity forces it low, it means
over-current. Drivers for both ports might want to receive interrupts on
both edges, and even request the GPIO for input so they can both tell
which level the signal is at, in order to report OC/not-OC back up to
higher levels, as soon as any change occurs.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren  wrote:

> Put another way, I don't believe
> there's any rule when writing DT bindings that states that bindings must
> not describe the same pin as both a GPIO and an IRQ, although admittedly
> that may be unusual.

Actually I think you've won me over here.

But I do not think that this shall be done at the expense of letting
DT-based drivers do nasty things like setting up the same GPIO
line as an IRQ (hammering the hardware to do that) and then
request the same line to be an output line and drive it, for
example.

So the state of the GPIO line has to be tracked: if it is set as
input and tied to an IRQ it should *not* be possible for the
other code path to request it and set it as output. But it should
be possible to set it as input again and read the value.

Laurent deascribed exactly the latter usecase, which is OK.

>> I agree with you that it would be the best if the only call would be
>> request_irq and the chip driver programs the HW appropriately. It would be a
>> dream, but unfortunately this is not possible at the moment. This is 
>> something
>> that Linus pointed out very very early in this whole discussion. The gpio and
>> irq frameworks don't share any information. The irq framework has no chance 
>> to
>> program the HW, because it will never find the related gpio.
>> For this to work the frameworks have to change (and possibly all 
>> drivers/board
>> files/whatever using request_irq() and/or request_gpio()) have to change.
>> That is something that I do not dare to do alone.
>
> This is a controller-specific issue, and has nothing to do with the GPIO
> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
> needs to program the HW when the IRQ is requested or set up. The Tegra
> driver already works this way, so there's actual proof that it is
> possible to do this in practice.

And how to you block the same line from being gpio_request()ed
and set as output?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren  wrote:

> I believe this situation is exactly what cause the original patch to the
> OMAP driver to be reverted; that patch should have touched the HW
> directly to solve the problem when the IRQ was requested, rather than
> calling into the GPIO subsystem (which also has the side-effect of
> touching the HW in the same way as desired).

And that has the side-effect that this line, which is now set up
in the HW as an input line, can be gpio_request()ed by some other
code path and set up as output, screwing around with the very
same registers.

I think the kernel should prevent such things.

But I think you're making it sound like the kernel should not prevent
such things, but instead give the drivers enough rope to shoot itself
in the foot if it so desires.

But a third solution is possible: the driver keep track of whether
a line is already requested as IRQ and if that happens, will not
allow gpio_set_direction_output() and other incompatible uses,
but still allow gpio_set_direction_input() and gpio_get_value(),
that would be OK.

In the GPIO descriptor we can flag this, so it would be preferable
to have that tracking in the gpiolib core rather than the driver
though.

> As we discussed on IRC (so mainly for the record in the mailing list
> archive), I believe that if a driver wants to use a pin as an interrupt
> and only an interrupt, even if the pin has the capability in HW to be a
> GPIO (or absolutely anything else at all), then the only call in the
> entire kernel (board code, DT core code, IRQ core, driver, ...) should
> be a single request_irq(), and the IRQ chip driver needs to program the
> HW appropriately to make that work.

I agree with this, but also think that the kernel should prevent the
same resource for being used in an incompatible way.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren  wrote:
> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:

>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
>> double-requests mean that the use-case that you say that have not been 
>> covered
>> by this patch can't actually happen?
>>
>> I mean, if when using board files an explicit call to gpio_request() is made 
>> by
>> platform code then a driver can't call gpio_request() for the same gpio. So 
>> this
>> patch shouldn't cause any regression since is just auto-requesting a GPIO 
>> when
>> is mapped as an IRQ in a DT which basically will be the same that was made by
>> board files before.
>
> I'm not familiar with the board file path; Linus describe this.

Oh um? Not following, that stuff is right above, what is unclear
about this that I need to describe?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:

 I'm a bit confused now. Doesn't the fact that gpio_request() prevents
 double-requests mean that the use-case that you say that have not been 
 covered
 by this patch can't actually happen?

 I mean, if when using board files an explicit call to gpio_request() is made 
 by
 platform code then a driver can't call gpio_request() for the same gpio. So 
 this
 patch shouldn't cause any regression since is just auto-requesting a GPIO 
 when
 is mapped as an IRQ in a DT which basically will be the same that was made by
 board files before.

 I'm not familiar with the board file path; Linus describe this.

Oh um? Not following, that stuff is right above, what is unclear
about this that I need to describe?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren swar...@wwwdotorg.org wrote:

 I believe this situation is exactly what cause the original patch to the
 OMAP driver to be reverted; that patch should have touched the HW
 directly to solve the problem when the IRQ was requested, rather than
 calling into the GPIO subsystem (which also has the side-effect of
 touching the HW in the same way as desired).

And that has the side-effect that this line, which is now set up
in the HW as an input line, can be gpio_request()ed by some other
code path and set up as output, screwing around with the very
same registers.

I think the kernel should prevent such things.

But I think you're making it sound like the kernel should not prevent
such things, but instead give the drivers enough rope to shoot itself
in the foot if it so desires.

But a third solution is possible: the driver keep track of whether
a line is already requested as IRQ and if that happens, will not
allow gpio_set_direction_output() and other incompatible uses,
but still allow gpio_set_direction_input() and gpio_get_value(),
that would be OK.

In the GPIO descriptor we can flag this, so it would be preferable
to have that tracking in the gpiolib core rather than the driver
though.

 As we discussed on IRC (so mainly for the record in the mailing list
 archive), I believe that if a driver wants to use a pin as an interrupt
 and only an interrupt, even if the pin has the capability in HW to be a
 GPIO (or absolutely anything else at all), then the only call in the
 entire kernel (board code, DT core code, IRQ core, driver, ...) should
 be a single request_irq(), and the IRQ chip driver needs to program the
 HW appropriately to make that work.

I agree with this, but also think that the kernel should prevent the
same resource for being used in an incompatible way.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren swar...@wwwdotorg.org wrote:

 Put another way, I don't believe
 there's any rule when writing DT bindings that states that bindings must
 not describe the same pin as both a GPIO and an IRQ, although admittedly
 that may be unusual.

Actually I think you've won me over here.

But I do not think that this shall be done at the expense of letting
DT-based drivers do nasty things like setting up the same GPIO
line as an IRQ (hammering the hardware to do that) and then
request the same line to be an output line and drive it, for
example.

So the state of the GPIO line has to be tracked: if it is set as
input and tied to an IRQ it should *not* be possible for the
other code path to request it and set it as output. But it should
be possible to set it as input again and read the value.

Laurent deascribed exactly the latter usecase, which is OK.

 I agree with you that it would be the best if the only call would be
 request_irq and the chip driver programs the HW appropriately. It would be a
 dream, but unfortunately this is not possible at the moment. This is 
 something
 that Linus pointed out very very early in this whole discussion. The gpio and
 irq frameworks don't share any information. The irq framework has no chance 
 to
 program the HW, because it will never find the related gpio.
 For this to work the frameworks have to change (and possibly all 
 drivers/board
 files/whatever using request_irq() and/or request_gpio()) have to change.
 That is something that I do not dare to do alone.

 This is a controller-specific issue, and has nothing to do with the GPIO
 or IRQ frameworks. The driver for the combined irq/gpio_chip simply
 needs to program the HW when the IRQ is requested or set up. The Tegra
 driver already works this way, so there's actual proof that it is
 possible to do this in practice.

And how to you block the same line from being gpio_request()ed
and set as output?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 01:53 PM, Linus Walleij wrote:
 On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 
 I believe this situation is exactly what cause the original patch to the
 OMAP driver to be reverted; that patch should have touched the HW
 directly to solve the problem when the IRQ was requested, rather than
 calling into the GPIO subsystem (which also has the side-effect of
 touching the HW in the same way as desired).
 
 And that has the side-effect that this line, which is now set up
 in the HW as an input line, can be gpio_request()ed by some other
 code path and set up as output, screwing around with the very
 same registers.
 
 I think the kernel should prevent such things.

It might be nice if it could do that.

However, that is 100% unrelated to the problem at hand.

A driver which only cares about an IRQ should be able to call just IRQ
APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
causes the HW to work should not involve the GPIO subsystem in any way
at all.

So, the first issue - making a correctly configured system functionally
work without resorting to hacks such as requesting a GPIO to make an IRQ
work, is one thing. This issue should be solved purely inside the
specific HW driver.

Having the kernel detect when two different drivers both request the
same resource is entirely another thing. The solution to the first issue
must not rely on any solution to this second issue.

I'm also not convinced it's possible to solve this second issue given
the current kernel APIs, since there's not enough semantic information;
requests of GPIOs and IRQs aren't actually tied to a particular driver
at present (there's no struct device *dev parameter to request_irq or
gpio_request) and so the subsystems can't actually tell who is
requesting the GPIO/IRQ, and hence can't detect when the same driver, or
a different driver, is requesting the same core resource for different
purposes.

Equally, I am actually not 100% sure we want the core to prevent this.
Why shouldn't two different drivers request the same IRQ? Why shouldn't
at least one driver, perhaps more, request the pin as a GPIO (assuming
it will only read the GPIO value, not flip the pin to output). This
exact situation might happen on some Tegra boards where there's a GPIO
for VBUS_EN that affects 2 USB ports. It's supposed to be driven
open-collector. If an external entity forces it low, it means
over-current. Drivers for both ports might want to receive interrupts on
both edges, and even request the GPIO for input so they can both tell
which level the signal is at, in order to report OC/not-OC back up to
higher levels, as soon as any change occurs.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 02:01 PM, Linus Walleij wrote:
 On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 
 Put another way, I don't believe
 there's any rule when writing DT bindings that states that bindings must
 not describe the same pin as both a GPIO and an IRQ, although admittedly
 that may be unusual.
 
 Actually I think you've won me over here.
 
 But I do not think that this shall be done at the expense of letting
 DT-based drivers do nasty things like setting up the same GPIO
 line as an IRQ (hammering the hardware to do that) and then
 request the same line to be an output line and drive it, for
 example.
 
 So the state of the GPIO line has to be tracked: if it is set as
 input and tied to an IRQ it should *not* be possible for the
 other code path to request it and set it as output. But it should
 be possible to set it as input again and read the value.
 
 Laurent deascribed exactly the latter usecase, which is OK.
 
 I agree with you that it would be the best if the only call would be
 request_irq and the chip driver programs the HW appropriately. It would be a
 dream, but unfortunately this is not possible at the moment. This is 
 something
 that Linus pointed out very very early in this whole discussion. The gpio 
 and
 irq frameworks don't share any information. The irq framework has no chance 
 to
 program the HW, because it will never find the related gpio.
 For this to work the frameworks have to change (and possibly all 
 drivers/board
 files/whatever using request_irq() and/or request_gpio()) have to change.
 That is something that I do not dare to do alone.

 This is a controller-specific issue, and has nothing to do with the GPIO
 or IRQ frameworks. The driver for the combined irq/gpio_chip simply
 needs to program the HW when the IRQ is requested or set up. The Tegra
 driver already works this way, so there's actual proof that it is
 possible to do this in practice.
 
 And how to you block the same line from being gpio_request()ed
 and set as output?

To be honest, I really don't think this problem is terribly likely to
occur, so I'm really not convinced that it's worth putting a lot of
effort into solving it.

If the problem does occur, it's trivial to see that this has happened by
looking at /proc/interrupts and /sys/kernel/debug/gpio, assuming the
system doesn't hang when this happens due to an interrupt storm, and if
it does, it should be pretty easy to track down the problematic register
write.

That said, I'm not outright against the kernel checking for this
situation. I think it'd work as follows:

The only code that knows the correlation between the IRQ and GPIO in
question is the combined gpio_chip/irq_chip driver. Everything /has/ to
be driven through that driver. In some cases, an IRQ may not be related
to any GPIO at all; there are certainly IRQ controllers which take
inputs directly from outside the chip, but have no GPIO functionality at
all on those signals.

That driver needs to maintain some state that indicates which of its
IRQs have been requested. Any time a GPIO request (I mean e.g.
set_output() not request_gpio)) comes in, the request needs to be
validated against that IRQ usage state. If there's a conflict, deny the
GPIO request.

Now, it's quite possible that the code to maintain this state and
perform the checks will be similar/common across multiple drivers. If
so, by all means implement the code somewhere common, and have the
various irq_chip/gpio_chip drivers call into it.

The main thing is that all of this has to be driven/controlled/enabled
by the gpio_chip/irq_chip driver itself, not as some global/blanket
feature of the GPIO or IRQ subsystems.

Perhaps rather than having the gpio_chip/irq_chip drivers physically
implement a function which calls this common code, they could set some
flags/data/... in the struct gpio_chip/irq_chip indicating that they
desire the core code that implements the error-checking to be enabled.
That might save some levels of stack. But the key is still that the
whole scenario is controlled by the end driver, not always assumed to
apply by the core code.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/16/2013 07:09 PM, Stephen Warren wrote:
> On 09/16/2013 10:03 AM, Lars Poeschel wrote:
>> On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
>>> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
 On 09/11/2013 12:34 AM, Stephen Warren wrote:
> On 09/10/2013 03:37 PM, Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>> hence prevent the driver's own gpio_request() from succeeding,
>>> since the GPIO is already requested? If this is not a problem, it
>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>> for the GPIO.
>>
>> Or at the very least something that's likely to break in the
>> future.
>
> Looking at the GPIO code, it already prevents double-requests:
>> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
>> 
>> desc_set_label(desc, label ? : "?");
>> status = 0;
>> 
>> } else {
>> 
>> status = -EBUSY;
>> module_put(chip->owner);
>> goto done;
>> 
>> }
>
> And I tested it in practice, and it really does fail.

 I'm a bit confused now. Doesn't the fact that gpio_request() prevents
 double-requests mean that the use-case that you say that have not been
 covered by this patch can't actually happen?

 I mean, if when using board files an explicit call to gpio_request() is
 made by platform code then a driver can't call gpio_request() for the
 same gpio. So this patch shouldn't cause any regression since is just
 auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
 will be the same that was made by board files before.
>>>
>>> I'm not familiar with the board file path; Linus describe this.
>> 
>> It seems Linus is busy, I'll try to help out.
>> 
>>> It sounds like that path is for the case where a driver /only/ cares
>>> about using a pin as an IRQ, and hence the driver only calls
>>> request_irq(). The board file is (earlier) calling gpio_request() in
>>> order to set up that input pin to work correctly as an IRQ. Hence, there
>>> is no double-call to gpio_request().
>> 
>> No, a board file is not a path or something. A board file describes the 
>> wirings 
>> and specifics of an (embedded) computer in C code. The complete knowledge of 
>> how things are connected on a board and which drivers to use is in this 
>> piece 
>> of code. Devicetree replaces legacy board files. These two do pretty much 
>> the 
>> same, but board files have more power, because they are executed and can 
>> contain whatever code is needed to setup a board.
>> But you are right, the driver only calls request_irq(), the board file set 
>> up 
>> the pin before and told the driver which irq to use.
> 
> path == code path, or execution path. I'm well aware of what board files
> are in general.
> 
> I'm just not familiar with board files that employ this particular hack.
> 
>>> The case I said wouldn't work is:
>>>
>>> * This patch calls gpio_request() in order to make the pin work as an IRQ.
>>>
>>> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
>>> gpio_request() and request_irq().
>>>
>>> So, there's a double-call to gpio_request(), which fails, and the driver
>>> fails to probe.
>> 
>> Again, no. In that case you don't define your pin as irq in the device tree, 
>> but only as gpio. The driver knows how to handle gpios and turn them into 
>> irqs 
>> so you have to present it a gpio not an irq. In that case the patch will not 
>> call gpio_request() and there is no double-call to gpio_request().
> 
> That is a way to make this patch work, yes. However, there's no
> guarantee that every driver or DT binding works this way. Forcing
> bindings to work that way is forcing Linux-internal details upon
> bindings, which should not be done. Put another way, I don't believe
> there's any rule when writing DT bindings that states that bindings must
> not describe the same pin as both a GPIO and an IRQ, although admittedly
> that may be unusual.
> 
> ...
>> I agree with you that it would be the best if the only call would be 
>> request_irq and the chip driver programs the HW appropriately. It would be a 
>> dream, but unfortunately this is not possible at the moment. This is 
>> something 
>> that Linus pointed out very very early in this whole discussion. The gpio 
>> and 
>> irq frameworks don't share any information. The irq framework has no chance 
>> to 
>> program the HW, because it will never find the related gpio.
>> For this to work the frameworks have to change (and possibly all 
>> drivers/board 
>> files/whatever using request_irq() and/or request_gpio()) have to change.
>> That is something that I do not dare to do alone.
> 
> This is a controller-specific issue, and 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/16/2013 07:09 PM, Stephen Warren wrote:
 On 09/16/2013 10:03 AM, Lars Poeschel wrote:
 On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
 On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
 On 09/11/2013 12:34 AM, Stephen Warren wrote:
 On 09/10/2013 03:37 PM, Mark Brown wrote:
 On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.

 Or at the very least something that's likely to break in the
 future.

 Looking at the GPIO code, it already prevents double-requests:
 if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
 
 desc_set_label(desc, label ? : ?);
 status = 0;
 
 } else {
 
 status = -EBUSY;
 module_put(chip-owner);
 goto done;
 
 }

 And I tested it in practice, and it really does fail.

 I'm a bit confused now. Doesn't the fact that gpio_request() prevents
 double-requests mean that the use-case that you say that have not been
 covered by this patch can't actually happen?

 I mean, if when using board files an explicit call to gpio_request() is
 made by platform code then a driver can't call gpio_request() for the
 same gpio. So this patch shouldn't cause any regression since is just
 auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
 will be the same that was made by board files before.

 I'm not familiar with the board file path; Linus describe this.
 
 It seems Linus is busy, I'll try to help out.
 
 It sounds like that path is for the case where a driver /only/ cares
 about using a pin as an IRQ, and hence the driver only calls
 request_irq(). The board file is (earlier) calling gpio_request() in
 order to set up that input pin to work correctly as an IRQ. Hence, there
 is no double-call to gpio_request().
 
 No, a board file is not a path or something. A board file describes the 
 wirings 
 and specifics of an (embedded) computer in C code. The complete knowledge of 
 how things are connected on a board and which drivers to use is in this 
 piece 
 of code. Devicetree replaces legacy board files. These two do pretty much 
 the 
 same, but board files have more power, because they are executed and can 
 contain whatever code is needed to setup a board.
 But you are right, the driver only calls request_irq(), the board file set 
 up 
 the pin before and told the driver which irq to use.
 
 path == code path, or execution path. I'm well aware of what board files
 are in general.
 
 I'm just not familiar with board files that employ this particular hack.
 
 The case I said wouldn't work is:

 * This patch calls gpio_request() in order to make the pin work as an IRQ.

 * Driver uses the pin as both a GPIO and an IRQ, and hence calls
 gpio_request() and request_irq().

 So, there's a double-call to gpio_request(), which fails, and the driver
 fails to probe.
 
 Again, no. In that case you don't define your pin as irq in the device tree, 
 but only as gpio. The driver knows how to handle gpios and turn them into 
 irqs 
 so you have to present it a gpio not an irq. In that case the patch will not 
 call gpio_request() and there is no double-call to gpio_request().
 
 That is a way to make this patch work, yes. However, there's no
 guarantee that every driver or DT binding works this way. Forcing
 bindings to work that way is forcing Linux-internal details upon
 bindings, which should not be done. Put another way, I don't believe
 there's any rule when writing DT bindings that states that bindings must
 not describe the same pin as both a GPIO and an IRQ, although admittedly
 that may be unusual.
 
 ...
 I agree with you that it would be the best if the only call would be 
 request_irq and the chip driver programs the HW appropriately. It would be a 
 dream, but unfortunately this is not possible at the moment. This is 
 something 
 that Linus pointed out very very early in this whole discussion. The gpio 
 and 
 irq frameworks don't share any information. The irq framework has no chance 
 to 
 program the HW, because it will never find the related gpio.
 For this to work the frameworks have to change (and possibly all 
 drivers/board 
 files/whatever using request_irq() and/or request_gpio()) have to change.
 That is something that I do not dare to do alone.
 
 This is a controller-specific issue, and has nothing to do with the GPIO
 or IRQ frameworks. The driver for the combined irq/gpio_chip simply
 needs to program the HW when the IRQ is requested or set up. The Tegra
 driver already works this way, so there's actual proof that it is
 possible to do this in practice.
 

Hi Stephen,

I finally had some time to look at this and tried 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/16/2013 10:03 AM, Lars Poeschel wrote:
> On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
>> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
>>> On 09/11/2013 12:34 AM, Stephen Warren wrote:
 On 09/10/2013 03:37 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>> Doesn't this patch call gpio_request() on the GPIO first, and
>> hence prevent the driver's own gpio_request() from succeeding,
>> since the GPIO is already requested? If this is not a problem, it
>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>> for the GPIO.
>
> Or at the very least something that's likely to break in the
> future.

 Looking at the GPIO code, it already prevents double-requests:
> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
> 
> desc_set_label(desc, label ? : "?");
> status = 0;
> 
> } else {
> 
> status = -EBUSY;
> module_put(chip->owner);
> goto done;
> 
> }

 And I tested it in practice, and it really does fail.
>>>
>>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
>>> double-requests mean that the use-case that you say that have not been
>>> covered by this patch can't actually happen?
>>>
>>> I mean, if when using board files an explicit call to gpio_request() is
>>> made by platform code then a driver can't call gpio_request() for the
>>> same gpio. So this patch shouldn't cause any regression since is just
>>> auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
>>> will be the same that was made by board files before.
>>
>> I'm not familiar with the board file path; Linus describe this.
> 
> It seems Linus is busy, I'll try to help out.
> 
>> It sounds like that path is for the case where a driver /only/ cares
>> about using a pin as an IRQ, and hence the driver only calls
>> request_irq(). The board file is (earlier) calling gpio_request() in
>> order to set up that input pin to work correctly as an IRQ. Hence, there
>> is no double-call to gpio_request().
> 
> No, a board file is not a path or something. A board file describes the 
> wirings 
> and specifics of an (embedded) computer in C code. The complete knowledge of 
> how things are connected on a board and which drivers to use is in this piece 
> of code. Devicetree replaces legacy board files. These two do pretty much the 
> same, but board files have more power, because they are executed and can 
> contain whatever code is needed to setup a board.
> But you are right, the driver only calls request_irq(), the board file set up 
> the pin before and told the driver which irq to use.

path == code path, or execution path. I'm well aware of what board files
are in general.

I'm just not familiar with board files that employ this particular hack.

>> The case I said wouldn't work is:
>>
>> * This patch calls gpio_request() in order to make the pin work as an IRQ.
>>
>> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
>> gpio_request() and request_irq().
>>
>> So, there's a double-call to gpio_request(), which fails, and the driver
>> fails to probe.
> 
> Again, no. In that case you don't define your pin as irq in the device tree, 
> but only as gpio. The driver knows how to handle gpios and turn them into 
> irqs 
> so you have to present it a gpio not an irq. In that case the patch will not 
> call gpio_request() and there is no double-call to gpio_request().

That is a way to make this patch work, yes. However, there's no
guarantee that every driver or DT binding works this way. Forcing
bindings to work that way is forcing Linux-internal details upon
bindings, which should not be done. Put another way, I don't believe
there's any rule when writing DT bindings that states that bindings must
not describe the same pin as both a GPIO and an IRQ, although admittedly
that may be unusual.

...
> I agree with you that it would be the best if the only call would be 
> request_irq and the chip driver programs the HW appropriately. It would be a 
> dream, but unfortunately this is not possible at the moment. This is 
> something 
> that Linus pointed out very very early in this whole discussion. The gpio and 
> irq frameworks don't share any information. The irq framework has no chance 
> to 
> program the HW, because it will never find the related gpio.
> For this to work the frameworks have to change (and possibly all 
> drivers/board 
> files/whatever using request_irq() and/or request_gpio()) have to change.
> That is something that I do not dare to do alone.

This is a controller-specific issue, and has nothing to do with the GPIO
or IRQ frameworks. The driver for the combined irq/gpio_chip simply
needs to program the HW when the IRQ is requested or set up. The Tegra
driver already 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
> > On 09/11/2013 12:34 AM, Stephen Warren wrote:
> >> On 09/10/2013 03:37 PM, Mark Brown wrote:
> >>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>  Doesn't this patch call gpio_request() on the GPIO first, and
>  hence prevent the driver's own gpio_request() from succeeding,
>  since the GPIO is already requested? If this is not a problem, it
>  sounds like a bug in gpio_request() not ensuring mutual exclusion
>  for the GPIO.
> >>> 
> >>> Or at the very least something that's likely to break in the
> >>> future.
> >> 
> >> Looking at the GPIO code, it already prevents double-requests:
> >>> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
> >>> 
> >>> desc_set_label(desc, label ? : "?");
> >>> status = 0;
> >>> 
> >>> } else {
> >>> 
> >>> status = -EBUSY;
> >>> module_put(chip->owner);
> >>> goto done;
> >>> 
> >>> }
> >> 
> >> And I tested it in practice, and it really does fail.
> > 
> > I'm a bit confused now. Doesn't the fact that gpio_request() prevents
> > double-requests mean that the use-case that you say that have not been
> > covered by this patch can't actually happen?
> > 
> > I mean, if when using board files an explicit call to gpio_request() is
> > made by platform code then a driver can't call gpio_request() for the
> > same gpio. So this patch shouldn't cause any regression since is just
> > auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
> > will be the same that was made by board files before.
> 
> I'm not familiar with the board file path; Linus describe this.

It seems Linus is busy, I'll try to help out.

> It sounds like that path is for the case where a driver /only/ cares
> about using a pin as an IRQ, and hence the driver only calls
> request_irq(). The board file is (earlier) calling gpio_request() in
> order to set up that input pin to work correctly as an IRQ. Hence, there
> is no double-call to gpio_request().

No, a board file is not a path or something. A board file describes the wirings 
and specifics of an (embedded) computer in C code. The complete knowledge of 
how things are connected on a board and which drivers to use is in this piece 
of code. Devicetree replaces legacy board files. These two do pretty much the 
same, but board files have more power, because they are executed and can 
contain whatever code is needed to setup a board.
But you are right, the driver only calls request_irq(), the board file set up 
the pin before and told the driver which irq to use.
 
> The case I said wouldn't work is:
> 
> * This patch calls gpio_request() in order to make the pin work as an IRQ.
> 
> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
> gpio_request() and request_irq().
> 
> So, there's a double-call to gpio_request(), which fails, and the driver
> fails to probe.

Again, no. In that case you don't define your pin as irq in the device tree, 
but only as gpio. The driver knows how to handle gpios and turn them into irqs 
so you have to present it a gpio not an irq. In that case the patch will not 
call gpio_request() and there is no double-call to gpio_request().
 
> I believe this situation is exactly what cause the original patch to the
> OMAP driver to be reverted; that patch should have touched the HW
> directly to solve the problem when the IRQ was requested, rather than
> calling into the GPIO subsystem (which also has the side-effect of
> touching the HW in the same way as desired).

The situation is different. You can try that out. Test your board/driver with 
this patch and test it with the original patch.

> > To give you an example of an use-case that this patch is trying to solve:
> > 
> > OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used
> > to interface with Pseudo-SRAM devices such as ethernet controllers. So
> > with board files we currently have this
> > (arch/arm/mach-omap2/gpmc-smsc911x.c): ...
> 
> As we discussed on IRC (so mainly for the record in the mailing list
> archive), I believe that if a driver wants to use a pin as an interrupt
> and only an interrupt, even if the pin has the capability in HW to be a
> GPIO (or absolutely anything else at all), then the only call in the
> entire kernel (board code, DT core code, IRQ core, driver, ...) should
> be a single request_irq(), and the IRQ chip driver needs to program the
> HW appropriately to make that work.

I agree with you that it would be the best if the only call would be 
request_irq and the chip driver programs the HW appropriately. It would be a 
dream, but unfortunately this is not possible at the moment. This is something 
that Linus pointed out very very early in this whole discussion. The gpio and 
irq frameworks don't share 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
 On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
  On 09/11/2013 12:34 AM, Stephen Warren wrote:
  On 09/10/2013 03:37 PM, Mark Brown wrote:
  On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
  Doesn't this patch call gpio_request() on the GPIO first, and
  hence prevent the driver's own gpio_request() from succeeding,
  since the GPIO is already requested? If this is not a problem, it
  sounds like a bug in gpio_request() not ensuring mutual exclusion
  for the GPIO.
  
  Or at the very least something that's likely to break in the
  future.
  
  Looking at the GPIO code, it already prevents double-requests:
  if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
  
  desc_set_label(desc, label ? : ?);
  status = 0;
  
  } else {
  
  status = -EBUSY;
  module_put(chip-owner);
  goto done;
  
  }
  
  And I tested it in practice, and it really does fail.
  
  I'm a bit confused now. Doesn't the fact that gpio_request() prevents
  double-requests mean that the use-case that you say that have not been
  covered by this patch can't actually happen?
  
  I mean, if when using board files an explicit call to gpio_request() is
  made by platform code then a driver can't call gpio_request() for the
  same gpio. So this patch shouldn't cause any regression since is just
  auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
  will be the same that was made by board files before.
 
 I'm not familiar with the board file path; Linus describe this.

It seems Linus is busy, I'll try to help out.

 It sounds like that path is for the case where a driver /only/ cares
 about using a pin as an IRQ, and hence the driver only calls
 request_irq(). The board file is (earlier) calling gpio_request() in
 order to set up that input pin to work correctly as an IRQ. Hence, there
 is no double-call to gpio_request().

No, a board file is not a path or something. A board file describes the wirings 
and specifics of an (embedded) computer in C code. The complete knowledge of 
how things are connected on a board and which drivers to use is in this piece 
of code. Devicetree replaces legacy board files. These two do pretty much the 
same, but board files have more power, because they are executed and can 
contain whatever code is needed to setup a board.
But you are right, the driver only calls request_irq(), the board file set up 
the pin before and told the driver which irq to use.
 
 The case I said wouldn't work is:
 
 * This patch calls gpio_request() in order to make the pin work as an IRQ.
 
 * Driver uses the pin as both a GPIO and an IRQ, and hence calls
 gpio_request() and request_irq().
 
 So, there's a double-call to gpio_request(), which fails, and the driver
 fails to probe.

Again, no. In that case you don't define your pin as irq in the device tree, 
but only as gpio. The driver knows how to handle gpios and turn them into irqs 
so you have to present it a gpio not an irq. In that case the patch will not 
call gpio_request() and there is no double-call to gpio_request().
 
 I believe this situation is exactly what cause the original patch to the
 OMAP driver to be reverted; that patch should have touched the HW
 directly to solve the problem when the IRQ was requested, rather than
 calling into the GPIO subsystem (which also has the side-effect of
 touching the HW in the same way as desired).

The situation is different. You can try that out. Test your board/driver with 
this patch and test it with the original patch.

  To give you an example of an use-case that this patch is trying to solve:
  
  OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used
  to interface with Pseudo-SRAM devices such as ethernet controllers. So
  with board files we currently have this
  (arch/arm/mach-omap2/gpmc-smsc911x.c): ...
 
 As we discussed on IRC (so mainly for the record in the mailing list
 archive), I believe that if a driver wants to use a pin as an interrupt
 and only an interrupt, even if the pin has the capability in HW to be a
 GPIO (or absolutely anything else at all), then the only call in the
 entire kernel (board code, DT core code, IRQ core, driver, ...) should
 be a single request_irq(), and the IRQ chip driver needs to program the
 HW appropriately to make that work.

I agree with you that it would be the best if the only call would be 
request_irq and the chip driver programs the HW appropriately. It would be a 
dream, but unfortunately this is not possible at the moment. This is something 
that Linus pointed out very very early in this whole discussion. The gpio and 
irq frameworks don't share any information. The irq framework has no chance to 
program the HW, because it will never find the related gpio.
For this to work the frameworks have to change (and 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/16/2013 10:03 AM, Lars Poeschel wrote:
 On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
 On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
 On 09/11/2013 12:34 AM, Stephen Warren wrote:
 On 09/10/2013 03:37 PM, Mark Brown wrote:
 On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.

 Or at the very least something that's likely to break in the
 future.

 Looking at the GPIO code, it already prevents double-requests:
 if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
 
 desc_set_label(desc, label ? : ?);
 status = 0;
 
 } else {
 
 status = -EBUSY;
 module_put(chip-owner);
 goto done;
 
 }

 And I tested it in practice, and it really does fail.

 I'm a bit confused now. Doesn't the fact that gpio_request() prevents
 double-requests mean that the use-case that you say that have not been
 covered by this patch can't actually happen?

 I mean, if when using board files an explicit call to gpio_request() is
 made by platform code then a driver can't call gpio_request() for the
 same gpio. So this patch shouldn't cause any regression since is just
 auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
 will be the same that was made by board files before.

 I'm not familiar with the board file path; Linus describe this.
 
 It seems Linus is busy, I'll try to help out.
 
 It sounds like that path is for the case where a driver /only/ cares
 about using a pin as an IRQ, and hence the driver only calls
 request_irq(). The board file is (earlier) calling gpio_request() in
 order to set up that input pin to work correctly as an IRQ. Hence, there
 is no double-call to gpio_request().
 
 No, a board file is not a path or something. A board file describes the 
 wirings 
 and specifics of an (embedded) computer in C code. The complete knowledge of 
 how things are connected on a board and which drivers to use is in this piece 
 of code. Devicetree replaces legacy board files. These two do pretty much the 
 same, but board files have more power, because they are executed and can 
 contain whatever code is needed to setup a board.
 But you are right, the driver only calls request_irq(), the board file set up 
 the pin before and told the driver which irq to use.

path == code path, or execution path. I'm well aware of what board files
are in general.

I'm just not familiar with board files that employ this particular hack.

 The case I said wouldn't work is:

 * This patch calls gpio_request() in order to make the pin work as an IRQ.

 * Driver uses the pin as both a GPIO and an IRQ, and hence calls
 gpio_request() and request_irq().

 So, there's a double-call to gpio_request(), which fails, and the driver
 fails to probe.
 
 Again, no. In that case you don't define your pin as irq in the device tree, 
 but only as gpio. The driver knows how to handle gpios and turn them into 
 irqs 
 so you have to present it a gpio not an irq. In that case the patch will not 
 call gpio_request() and there is no double-call to gpio_request().

That is a way to make this patch work, yes. However, there's no
guarantee that every driver or DT binding works this way. Forcing
bindings to work that way is forcing Linux-internal details upon
bindings, which should not be done. Put another way, I don't believe
there's any rule when writing DT bindings that states that bindings must
not describe the same pin as both a GPIO and an IRQ, although admittedly
that may be unusual.

...
 I agree with you that it would be the best if the only call would be 
 request_irq and the chip driver programs the HW appropriately. It would be a 
 dream, but unfortunately this is not possible at the moment. This is 
 something 
 that Linus pointed out very very early in this whole discussion. The gpio and 
 irq frameworks don't share any information. The irq framework has no chance 
 to 
 program the HW, because it will never find the related gpio.
 For this to work the frameworks have to change (and possibly all 
 drivers/board 
 files/whatever using request_irq() and/or request_gpio()) have to change.
 That is something that I do not dare to do alone.

This is a controller-specific issue, and has nothing to do with the GPIO
or IRQ frameworks. The driver for the combined irq/gpio_chip simply
needs to program the HW when the IRQ is requested or set up. The Tegra
driver already works this way, so there's actual proof that it is
possible to do this in practice.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
> On 09/11/2013 12:34 AM, Stephen Warren wrote:
>> On 09/10/2013 03:37 PM, Mark Brown wrote:
>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>>
 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.
>>>
>>> Or at the very least something that's likely to break in the
>>> future.
>>
>> Looking at the GPIO code, it already prevents double-requests:
>>
>>> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
>>> desc_set_label(desc, label ? : "?");
>>> status = 0;
>>> } else {
>>> status = -EBUSY;
>>> module_put(chip->owner);
>>> goto done;
>>> }
>>
>> And I tested it in practice, and it really does fail.
>>
> 
> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
> double-requests mean that the use-case that you say that have not been covered
> by this patch can't actually happen?
> 
> I mean, if when using board files an explicit call to gpio_request() is made 
> by
> platform code then a driver can't call gpio_request() for the same gpio. So 
> this
> patch shouldn't cause any regression since is just auto-requesting a GPIO when
> is mapped as an IRQ in a DT which basically will be the same that was made by
> board files before.

I'm not familiar with the board file path; Linus describe this.

It sounds like that path is for the case where a driver /only/ cares
about using a pin as an IRQ, and hence the driver only calls
request_irq(). The board file is (earlier) calling gpio_request() in
order to set up that input pin to work correctly as an IRQ. Hence, there
is no double-call to gpio_request().

The case I said wouldn't work is:

* This patch calls gpio_request() in order to make the pin work as an IRQ.

* Driver uses the pin as both a GPIO and an IRQ, and hence calls
gpio_request() and request_irq().

So, there's a double-call to gpio_request(), which fails, and the driver
fails to probe.

I believe this situation is exactly what cause the original patch to the
OMAP driver to be reverted; that patch should have touched the HW
directly to solve the problem when the IRQ was requested, rather than
calling into the GPIO subsystem (which also has the side-effect of
touching the HW in the same way as desired).

> To give you an example of an use-case that this patch is trying to solve:
> 
> OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
> interface with Pseudo-SRAM devices such as ethernet controllers. So with board
> files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):
> ...

As we discussed on IRC (so mainly for the record in the mailing list
archive), I believe that if a driver wants to use a pin as an interrupt
and only an interrupt, even if the pin has the capability in HW to be a
GPIO (or absolutely anything else at all), then the only call in the
entire kernel (board code, DT core code, IRQ core, driver, ...) should
be a single request_irq(), and the IRQ chip driver needs to program the
HW appropriately to make that work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
 On 09/11/2013 12:34 AM, Stephen Warren wrote:
 On 09/10/2013 03:37 PM, Mark Brown wrote:
 On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:

 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.

 Or at the very least something that's likely to break in the
 future.

 Looking at the GPIO code, it already prevents double-requests:

 if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
 desc_set_label(desc, label ? : ?);
 status = 0;
 } else {
 status = -EBUSY;
 module_put(chip-owner);
 goto done;
 }

 And I tested it in practice, and it really does fail.

 
 I'm a bit confused now. Doesn't the fact that gpio_request() prevents
 double-requests mean that the use-case that you say that have not been covered
 by this patch can't actually happen?
 
 I mean, if when using board files an explicit call to gpio_request() is made 
 by
 platform code then a driver can't call gpio_request() for the same gpio. So 
 this
 patch shouldn't cause any regression since is just auto-requesting a GPIO when
 is mapped as an IRQ in a DT which basically will be the same that was made by
 board files before.

I'm not familiar with the board file path; Linus describe this.

It sounds like that path is for the case where a driver /only/ cares
about using a pin as an IRQ, and hence the driver only calls
request_irq(). The board file is (earlier) calling gpio_request() in
order to set up that input pin to work correctly as an IRQ. Hence, there
is no double-call to gpio_request().

The case I said wouldn't work is:

* This patch calls gpio_request() in order to make the pin work as an IRQ.

* Driver uses the pin as both a GPIO and an IRQ, and hence calls
gpio_request() and request_irq().

So, there's a double-call to gpio_request(), which fails, and the driver
fails to probe.

I believe this situation is exactly what cause the original patch to the
OMAP driver to be reverted; that patch should have touched the HW
directly to solve the problem when the IRQ was requested, rather than
calling into the GPIO subsystem (which also has the side-effect of
touching the HW in the same way as desired).

 To give you an example of an use-case that this patch is trying to solve:
 
 OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
 interface with Pseudo-SRAM devices such as ethernet controllers. So with board
 files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):
 ...

As we discussed on IRC (so mainly for the record in the mailing list
archive), I believe that if a driver wants to use a pin as an interrupt
and only an interrupt, even if the pin has the capability in HW to be a
GPIO (or absolutely anything else at all), then the only call in the
entire kernel (board code, DT core code, IRQ core, driver, ...) should
be a single request_irq(), and the IRQ chip driver needs to program the
HW appropriately to make that work.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 04:23 PM, Javier Martinez Canillas wrote:
> On 09/10/2013 09:52 PM, Stephen Warren wrote:
>> On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
>> ...
>>> The only thing that this patch tries to solve is when a driver expect to 
>>> request
>>> a IRQ and it doesn't care if is a real IRQ line from an interrupt 
>>> controller or
>>> a GPIO pin mapped as an IRQ.
>>
>> That can be solved in the interrupt chip driver. The fact the previous
>> attempt didn't work doesn't mean that it's impossible.
>>
> 
> Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
> cycle so it was safer to just revert the patches instead of analyzing the
> regression and providing a fix.
> 
> If Linus is fond about taking a local fix for gpio-omap then we can try again
> as a RFC with a better test coverage than before (although the patches had
> several tested and acked-by but no one tested on OMAP1 until the patches were
> merged) getting some TI folks in the loop who have access to those ancient 
> OMAP1
> devices. That way we can repost as a patch just once we are definitely sure 
> that
> it won't cause a regression on any OMAP platform.
> 
>>> With board files we used to explicitly do the GPIO setup
>>> (gpio_{request,direction_input}) but with DT these board files are gone and 
>>> we
>>> need a way to setup a GPIO implicitly when is mapped as an IRQ.
>>
>> Well, that's just an example of hacking around something in a board file
>> that should have been fixed in the GPIO/IRQ controller.
>>
>>> That's the only use case that this patch covers.
>> ...
>>> Also, it would be great if we can find a temporary solution so we can 
>>> finally
>>> have ethernet working with DT on most OMAP2+ boards. Since I expect that 
>>> doing
>>> the mentioned change on gpiolib would take at least a couple of kernel 
>>> releases.
>>
>> Really? I thought this patch was error-checking to make sure that
>> different drivers didn't request a GPIO and an IRQ that are actually the
>> same signal. This patch shouldn't affect any functionality except in
>> cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
>> driver).
>>
> 
> Yes, it doesn't do any error-checking neither prevent a driver to request a 
> GPIO
> and an IRQ that are the same signal (as long as this is supported by the
> GPIO/IRQ controller and its driver). The only thing that Linus patch does is 
> to
> auto request a GPIO for pins that are mapped as IRQ in DT (i.e: 
> interrupt-parent
> = <>).
> 
> The name of the function introduced by Linus is
> of_gpiochip_interrupt_consistency_check() but probably a better name is
> of_gpiochip_autorequest_irq() or something like that.
> 
> A similar behavior could be obtained by let's say calling gpio_request() in
> irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
> instead of doing it in the GPIO chip DT core as Linus does with his patch.
> 
> That's why I don't understand why Linus patch could be an issue for drivers 
> that
> needs to request both the GPIO and the mapped IRQ.
> 
> If this is already supported then nothing will break. If the driver tries to
> request the GPIO twice because this is already made by the DT core then I 
> think
> is a bug in the driver and has to be fixed to support DT since there won't be
> any need to do this manually anymore.

Quick question- Wouldn't this mean that there have to be 2 code paths in the
driver then.. One for DT-case where gpio_request double request is prevented,
and one for non-DT case where you would do the gpio_request followed by
request_irq. I'm not sure if there are drivers today that use DT and need to
converted to prevent the double request? If there are, and such drivers are also
used on non-DT platform, then we would have to fork 2 different code paths while
requesting an IRQ for DT/non-DT in the driver..

Also this path kind of enforces that the driver author must be aware whether
driver is being used on DT or non-DT platform.. Linus mentioned enforcing of
semantics, this is also enforcing semantics no?

Looks like the correct fix for this as discussed in this thread should be to
associate the struct device with the GPIO request, remember it and use the info
during request_irq. This is provided that the old pattern of gpio_request and
request_irq is continued to be used and working. I know this is some time away
so I'm not too opinionated about it.

Regards,

-Joel




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/11/2013 12:34 AM, Stephen Warren wrote:
> On 09/10/2013 03:37 PM, Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>> 
>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>> hence prevent the driver's own gpio_request() from succeeding,
>>> since the GPIO is already requested? If this is not a problem, it
>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>> for the GPIO.
>> 
>> Or at the very least something that's likely to break in the
>> future.
> 
> Looking at the GPIO code, it already prevents double-requests:
> 
>> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
>> desc_set_label(desc, label ? : "?");
>> status = 0;
>> } else {
>> status = -EBUSY;
>> module_put(chip->owner);
>> goto done;
>> }
> 
> And I tested it in practice, and it really does fail.
> 

I'm a bit confused now. Doesn't the fact that gpio_request() prevents
double-requests mean that the use-case that you say that have not been covered
by this patch can't actually happen?

I mean, if when using board files an explicit call to gpio_request() is made by
platform code then a driver can't call gpio_request() for the same gpio. So this
patch shouldn't cause any regression since is just auto-requesting a GPIO when
is mapped as an IRQ in a DT which basically will be the same that was made by
board files before.

To give you an example of an use-case that this patch is trying to solve:

OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
interface with Pseudo-SRAM devices such as ethernet controllers. So with board
files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):

void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
{
   
   if (gpio_request_one(gpmc_cfg->gpio_irq, GPIOF_IN, "smsc911x irq")) {
pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq);
goto free1;
   }
   
   gpmc_smsc911x_resources[1].start = gpio_to_irq(gpmc_cfg->gpio_irq);
   ...
   pdev = platform_device_register_resndata(NULL, "smsc911x", gpmc_cfg->id,
 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
 _smsc911x_config, sizeof(gpmc_smsc911x_config));
   ...
}

and later in the smsc911x ethernet driver probe function:

static int smsc911x_drv_probe(struct platform_device *pdev)
{
   retval = request_irq(dev->irq, smsc911x_irqhandler,
 irq_flags | IRQF_SHARED, dev->name, dev);
   ...
   irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
   ...
   dev->irq = irq_res->start;
   ...
   retval = request_irq(dev->irq, smsc911x_irqhandler,
 irq_flags | IRQF_SHARED, dev->name, dev);
   ...
}

The driver just knows that it has to get the IRQ from a struct resource and it
doesn't care if that is a real IRQ line from an interrupt controller or a GPIO
pin mapped as an IRQ. With linus patch I just can define on a DT (GPMC
properties omitted for simplicity):

ethernet@5,0 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
compatible = "smsc,lan9221", "smsc,lan9115";
reg = <5 0 0xff>;
bank-width = <2>;
interrupt-parent = <>;
interrupts = <16 8>;
vmmc-supply = <>;
vmmc_aux-supply = <>;
reg-io-width = <4>;

smsc,save-mac-address;
};

and it will just work. Without Linus patch the call to request_irq() will fail
because a call to gpio_request() has not been made (and thus the GPIO bank was
not enabled).

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 03:37 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
> 
>> Doesn't this patch call gpio_request() on the GPIO first, and
>> hence prevent the driver's own gpio_request() from succeeding,
>> since the GPIO is already requested? If this is not a problem, it
>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>> for the GPIO.
> 
> Or at the very least something that's likely to break in the
> future.

Looking at the GPIO code, it already prevents double-requests:

> if (test_and_set_bit(FLAG_REQUESTED, >flags) == 0) {
> desc_set_label(desc, label ? : "?");
> status = 0;
> } else {
> status = -EBUSY;
> module_put(chip->owner);
> goto done;
> }

And I tested it in practice, and it really does fail.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:

> Doesn't this patch call gpio_request() on the GPIO first, and hence
> prevent the driver's own gpio_request() from succeeding, since the GPIO
> is already requested? If this is not a problem, it sounds like a bug in
> gpio_request() not ensuring mutual exclusion for the GPIO.

Or at the very least something that's likely to break in the future.


signature.asc
Description: Digital signature

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 09:52 PM, Stephen Warren wrote:
> On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
> ...
>> The only thing that this patch tries to solve is when a driver expect to 
>> request
>> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller 
>> or
>> a GPIO pin mapped as an IRQ.
> 
> That can be solved in the interrupt chip driver. The fact the previous
> attempt didn't work doesn't mean that it's impossible.
>

Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
cycle so it was safer to just revert the patches instead of analyzing the
regression and providing a fix.

If Linus is fond about taking a local fix for gpio-omap then we can try again
as a RFC with a better test coverage than before (although the patches had
several tested and acked-by but no one tested on OMAP1 until the patches were
merged) getting some TI folks in the loop who have access to those ancient OMAP1
devices. That way we can repost as a patch just once we are definitely sure that
it won't cause a regression on any OMAP platform.

>> With board files we used to explicitly do the GPIO setup
>> (gpio_{request,direction_input}) but with DT these board files are gone and 
>> we
>> need a way to setup a GPIO implicitly when is mapped as an IRQ.
> 
> Well, that's just an example of hacking around something in a board file
> that should have been fixed in the GPIO/IRQ controller.
> 
>> That's the only use case that this patch covers.
> ...
>> Also, it would be great if we can find a temporary solution so we can finally
>> have ethernet working with DT on most OMAP2+ boards. Since I expect that 
>> doing
>> the mentioned change on gpiolib would take at least a couple of kernel 
>> releases.
> 
> Really? I thought this patch was error-checking to make sure that
> different drivers didn't request a GPIO and an IRQ that are actually the
> same signal. This patch shouldn't affect any functionality except in
> cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
> driver).
> 

Yes, it doesn't do any error-checking neither prevent a driver to request a GPIO
and an IRQ that are the same signal (as long as this is supported by the
GPIO/IRQ controller and its driver). The only thing that Linus patch does is to
auto request a GPIO for pins that are mapped as IRQ in DT (i.e: interrupt-parent
= <>).

The name of the function introduced by Linus is
of_gpiochip_interrupt_consistency_check() but probably a better name is
of_gpiochip_autorequest_irq() or something like that.

A similar behavior could be obtained by let's say calling gpio_request() in
irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
instead of doing it in the GPIO chip DT core as Linus does with his patch.

That's why I don't understand why Linus patch could be an issue for drivers that
needs to request both the GPIO and the mapped IRQ.

If this is already supported then nothing will break. If the driver tries to
request the GPIO twice because this is already made by the DT core then I think
is a bug in the driver and has to be fixed to support DT since there won't be
any need to do this manually anymore.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 02:47 AM, Lars Poeschel wrote:
> On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
 The driver that tries to use the GPIO requested by this patch before HAS
 to
 fail. This is exactly the intention of this patch. We don't want the
 GPIO to be requested any more, if it is used as an interrupt pin.
>>>
>>> That will break existing drivers. There are drivers that request the
>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>> that way.
>>
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> And I say it again for those coming into the discussion late, like it has 
> been 
> said many many times before: This patch does not break any of this drivers. 
> They simply request their GPIO from DT and turn it into an irq using 
> gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
> parallel. At least this is not a problem.

Doesn't this patch call gpio_request() on the GPIO first, and hence
prevent the driver's own gpio_request() from succeeding, since the GPIO
is already requested? If this is not a problem, it sounds like a bug in
gpio_request() not ensuring mutual exclusion for the GPIO.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
...
> The only thing that this patch tries to solve is when a driver expect to 
> request
> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller 
> or
> a GPIO pin mapped as an IRQ.

That can be solved in the interrupt chip driver. The fact the previous
attempt didn't work doesn't mean that it's impossible.

> With board files we used to explicitly do the GPIO setup
> (gpio_{request,direction_input}) but with DT these board files are gone and we
> need a way to setup a GPIO implicitly when is mapped as an IRQ.

Well, that's just an example of hacking around something in a board file
that should have been fixed in the GPIO/IRQ controller.

> That's the only use case that this patch covers.
...
> Also, it would be great if we can find a temporary solution so we can finally
> have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
> the mentioned change on gpiolib would take at least a couple of kernel 
> releases.

Really? I thought this patch was error-checking to make sure that
different drivers didn't request a GPIO and an IRQ that are actually the
same signal. This patch shouldn't affect any functionality except in
cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
driver).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 10:47 AM, Lars Poeschel wrote:
> On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>> > On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>> > > The driver that tries to use the GPIO requested by this patch before HAS
>> > > to
>> > > fail. This is exactly the intention of this patch. We don't want the
>> > > GPIO to be requested any more, if it is used as an interrupt pin.
>> > 
>> > That will break existing drivers. There are drivers that request the
>> > same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>> > that way.
>> 
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> And I say it again for those coming into the discussion late, like it has 
> been 
> said many many times before: This patch does not break any of this drivers. 
> They simply request their GPIO from DT and turn it into an irq using 
> gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
> parallel. At least this is not a problem.
> 

I agree with Lars and think that we should stop arguing about the limitations of
this patch and how this doesn't solve:


a) The fact that platform code has to call:

gpio_request()
gpio_direction_input()
request_irq()

b) That there are complex use cases where the same driver can request both a
GPIO and the mapped IRQ.

The only thing that this patch tries to solve is when a driver expect to request
a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
a GPIO pin mapped as an IRQ.

With board files we used to explicitly do the GPIO setup
(gpio_{request,direction_input}) but with DT these board files are gone and we
need a way to setup a GPIO implicitly when is mapped as an IRQ.

That's the only use case that this patch covers.

In legacy non-DT booting boards files will continue doing whatever are doing now
to ensure that drivers calling request_irq() will succeed and complex drivers
can just not define the GPIO-IRQ mapping in the DT and do whatever they need to
do manually.

Now, if just solving this use case is not enough and we want a more general
solution then we should start discussing how that solution should look like so
it can be implemented.

The most concrete idea so far is the one proposed by Stephen to pass a struct
device to gpiolib functions so GPIO controller drivers know when the same device
or a different one is requesting a GPIO twice to allow sharing a GPIO or not.

Also, it would be great if we can find a temporary solution so we can finally
have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
the mentioned change on gpiolib would take at least a couple of kernel releases.

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> > On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> > > The driver that tries to use the GPIO requested by this patch before HAS
> > > to
> > > fail. This is exactly the intention of this patch. We don't want the
> > > GPIO to be requested any more, if it is used as an interrupt pin.
> > 
> > That will break existing drivers. There are drivers that request the
> > same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> > that way.
> 
> Yes, plus input devices and audio jack detection among others.  This
> pattern is very common if the GPIO is actually being used as a GPIO, an
> edge triggered interrupt is used to flag when something happens and the
> state is determined by reading the GPIO state (often with some
> debounce).

And I say it again for those coming into the discussion late, like it has been 
said many many times before: This patch does not break any of this drivers. 
They simply request their GPIO from DT and turn it into an irq using 
gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
parallel. At least this is not a problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
  On 09/04/2013 03:05 AM, Lars Poeschel wrote:
   The driver that tries to use the GPIO requested by this patch before HAS
   to
   fail. This is exactly the intention of this patch. We don't want the
   GPIO to be requested any more, if it is used as an interrupt pin.
  
  That will break existing drivers. There are drivers that request the
  same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
  that way.
 
 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).

And I say it again for those coming into the discussion late, like it has been 
said many many times before: This patch does not break any of this drivers. 
They simply request their GPIO from DT and turn it into an irq using 
gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
parallel. At least this is not a problem.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 10:47 AM, Lars Poeschel wrote:
 On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
  On 09/04/2013 03:05 AM, Lars Poeschel wrote:
   The driver that tries to use the GPIO requested by this patch before HAS
   to
   fail. This is exactly the intention of this patch. We don't want the
   GPIO to be requested any more, if it is used as an interrupt pin.
  
  That will break existing drivers. There are drivers that request the
  same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
  that way.
 
 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).
 
 And I say it again for those coming into the discussion late, like it has 
 been 
 said many many times before: This patch does not break any of this drivers. 
 They simply request their GPIO from DT and turn it into an irq using 
 gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
 parallel. At least this is not a problem.
 

I agree with Lars and think that we should stop arguing about the limitations of
this patch and how this doesn't solve:


a) The fact that platform code has to call:

gpio_request()
gpio_direction_input()
request_irq()

b) That there are complex use cases where the same driver can request both a
GPIO and the mapped IRQ.

The only thing that this patch tries to solve is when a driver expect to request
a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
a GPIO pin mapped as an IRQ.

With board files we used to explicitly do the GPIO setup
(gpio_{request,direction_input}) but with DT these board files are gone and we
need a way to setup a GPIO implicitly when is mapped as an IRQ.

That's the only use case that this patch covers.

In legacy non-DT booting boards files will continue doing whatever are doing now
to ensure that drivers calling request_irq() will succeed and complex drivers
can just not define the GPIO-IRQ mapping in the DT and do whatever they need to
do manually.

Now, if just solving this use case is not enough and we want a more general
solution then we should start discussing how that solution should look like so
it can be implemented.

The most concrete idea so far is the one proposed by Stephen to pass a struct
device to gpiolib functions so GPIO controller drivers know when the same device
or a different one is requesting a GPIO twice to allow sharing a GPIO or not.

Also, it would be great if we can find a temporary solution so we can finally
have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
the mentioned change on gpiolib would take at least a couple of kernel releases.

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 02:47 AM, Lars Poeschel wrote:
 On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
 On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
 On 09/04/2013 03:05 AM, Lars Poeschel wrote:
 The driver that tries to use the GPIO requested by this patch before HAS
 to
 fail. This is exactly the intention of this patch. We don't want the
 GPIO to be requested any more, if it is used as an interrupt pin.

 That will break existing drivers. There are drivers that request the
 same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
 that way.

 Yes, plus input devices and audio jack detection among others.  This
 pattern is very common if the GPIO is actually being used as a GPIO, an
 edge triggered interrupt is used to flag when something happens and the
 state is determined by reading the GPIO state (often with some
 debounce).
 
 And I say it again for those coming into the discussion late, like it has 
 been 
 said many many times before: This patch does not break any of this drivers. 
 They simply request their GPIO from DT and turn it into an irq using 
 gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
 parallel. At least this is not a problem.

Doesn't this patch call gpio_request() on the GPIO first, and hence
prevent the driver's own gpio_request() from succeeding, since the GPIO
is already requested? If this is not a problem, it sounds like a bug in
gpio_request() not ensuring mutual exclusion for the GPIO.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
...
 The only thing that this patch tries to solve is when a driver expect to 
 request
 a IRQ and it doesn't care if is a real IRQ line from an interrupt controller 
 or
 a GPIO pin mapped as an IRQ.

That can be solved in the interrupt chip driver. The fact the previous
attempt didn't work doesn't mean that it's impossible.

 With board files we used to explicitly do the GPIO setup
 (gpio_{request,direction_input}) but with DT these board files are gone and we
 need a way to setup a GPIO implicitly when is mapped as an IRQ.

Well, that's just an example of hacking around something in a board file
that should have been fixed in the GPIO/IRQ controller.

 That's the only use case that this patch covers.
...
 Also, it would be great if we can find a temporary solution so we can finally
 have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
 the mentioned change on gpiolib would take at least a couple of kernel 
 releases.

Really? I thought this patch was error-checking to make sure that
different drivers didn't request a GPIO and an IRQ that are actually the
same signal. This patch shouldn't affect any functionality except in
cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
driver).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 09:52 PM, Stephen Warren wrote:
 On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
 ...
 The only thing that this patch tries to solve is when a driver expect to 
 request
 a IRQ and it doesn't care if is a real IRQ line from an interrupt controller 
 or
 a GPIO pin mapped as an IRQ.
 
 That can be solved in the interrupt chip driver. The fact the previous
 attempt didn't work doesn't mean that it's impossible.


Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
cycle so it was safer to just revert the patches instead of analyzing the
regression and providing a fix.

If Linus is fond about taking a local fix for gpio-omap then we can try again
as a RFC with a better test coverage than before (although the patches had
several tested and acked-by but no one tested on OMAP1 until the patches were
merged) getting some TI folks in the loop who have access to those ancient OMAP1
devices. That way we can repost as a patch just once we are definitely sure that
it won't cause a regression on any OMAP platform.

 With board files we used to explicitly do the GPIO setup
 (gpio_{request,direction_input}) but with DT these board files are gone and 
 we
 need a way to setup a GPIO implicitly when is mapped as an IRQ.
 
 Well, that's just an example of hacking around something in a board file
 that should have been fixed in the GPIO/IRQ controller.
 
 That's the only use case that this patch covers.
 ...
 Also, it would be great if we can find a temporary solution so we can finally
 have ethernet working with DT on most OMAP2+ boards. Since I expect that 
 doing
 the mentioned change on gpiolib would take at least a couple of kernel 
 releases.
 
 Really? I thought this patch was error-checking to make sure that
 different drivers didn't request a GPIO and an IRQ that are actually the
 same signal. This patch shouldn't affect any functionality except in
 cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
 driver).
 

Yes, it doesn't do any error-checking neither prevent a driver to request a GPIO
and an IRQ that are the same signal (as long as this is supported by the
GPIO/IRQ controller and its driver). The only thing that Linus patch does is to
auto request a GPIO for pins that are mapped as IRQ in DT (i.e: interrupt-parent
= gpio6).

The name of the function introduced by Linus is
of_gpiochip_interrupt_consistency_check() but probably a better name is
of_gpiochip_autorequest_irq() or something like that.

A similar behavior could be obtained by let's say calling gpio_request() in
irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
instead of doing it in the GPIO chip DT core as Linus does with his patch.

That's why I don't understand why Linus patch could be an issue for drivers that
needs to request both the GPIO and the mapped IRQ.

If this is already supported then nothing will break. If the driver tries to
request the GPIO twice because this is already made by the DT core then I think
is a bug in the driver and has to be fixed to support DT since there won't be
any need to do this manually anymore.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:

 Doesn't this patch call gpio_request() on the GPIO first, and hence
 prevent the driver's own gpio_request() from succeeding, since the GPIO
 is already requested? If this is not a problem, it sounds like a bug in
 gpio_request() not ensuring mutual exclusion for the GPIO.

Or at the very least something that's likely to break in the future.


signature.asc
Description: Digital signature

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 03:37 PM, Mark Brown wrote:
 On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
 
 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.
 
 Or at the very least something that's likely to break in the
 future.

Looking at the GPIO code, it already prevents double-requests:

 if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
 desc_set_label(desc, label ? : ?);
 status = 0;
 } else {
 status = -EBUSY;
 module_put(chip-owner);
 goto done;
 }

And I tested it in practice, and it really does fail.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/11/2013 12:34 AM, Stephen Warren wrote:
 On 09/10/2013 03:37 PM, Mark Brown wrote:
 On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
 
 Doesn't this patch call gpio_request() on the GPIO first, and
 hence prevent the driver's own gpio_request() from succeeding,
 since the GPIO is already requested? If this is not a problem, it
 sounds like a bug in gpio_request() not ensuring mutual exclusion
 for the GPIO.
 
 Or at the very least something that's likely to break in the
 future.
 
 Looking at the GPIO code, it already prevents double-requests:
 
 if (test_and_set_bit(FLAG_REQUESTED, desc-flags) == 0) {
 desc_set_label(desc, label ? : ?);
 status = 0;
 } else {
 status = -EBUSY;
 module_put(chip-owner);
 goto done;
 }
 
 And I tested it in practice, and it really does fail.
 

I'm a bit confused now. Doesn't the fact that gpio_request() prevents
double-requests mean that the use-case that you say that have not been covered
by this patch can't actually happen?

I mean, if when using board files an explicit call to gpio_request() is made by
platform code then a driver can't call gpio_request() for the same gpio. So this
patch shouldn't cause any regression since is just auto-requesting a GPIO when
is mapped as an IRQ in a DT which basically will be the same that was made by
board files before.

To give you an example of an use-case that this patch is trying to solve:

OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
interface with Pseudo-SRAM devices such as ethernet controllers. So with board
files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):

void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
{
   
   if (gpio_request_one(gpmc_cfg-gpio_irq, GPIOF_IN, smsc911x irq)) {
pr_err(Failed to request IRQ GPIO%d\n, gpmc_cfg-gpio_irq);
goto free1;
   }
   
   gpmc_smsc911x_resources[1].start = gpio_to_irq(gpmc_cfg-gpio_irq);
   ...
   pdev = platform_device_register_resndata(NULL, smsc911x, gpmc_cfg-id,
 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
 gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config));
   ...
}

and later in the smsc911x ethernet driver probe function:

static int smsc911x_drv_probe(struct platform_device *pdev)
{
   retval = request_irq(dev-irq, smsc911x_irqhandler,
 irq_flags | IRQF_SHARED, dev-name, dev);
   ...
   irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
   ...
   dev-irq = irq_res-start;
   ...
   retval = request_irq(dev-irq, smsc911x_irqhandler,
 irq_flags | IRQF_SHARED, dev-name, dev);
   ...
}

The driver just knows that it has to get the IRQ from a struct resource and it
doesn't care if that is a real IRQ line from an interrupt controller or a GPIO
pin mapped as an IRQ. With linus patch I just can define on a DT (GPMC
properties omitted for simplicity):

ethernet@5,0 {
pinctrl-names = default;
pinctrl-0 = smsc911x_pins;
compatible = smsc,lan9221, smsc,lan9115;
reg = 5 0 0xff;
bank-width = 2;
interrupt-parent = gpio6;
interrupts = 16 8;
vmmc-supply = vddvario;
vmmc_aux-supply = vdd33a;
reg-io-width = 4;

smsc,save-mac-address;
};

and it will just work. Without Linus patch the call to request_irq() will fail
because a call to gpio_request() has not been made (and thus the GPIO bank was
not enabled).

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 04:23 PM, Javier Martinez Canillas wrote:
 On 09/10/2013 09:52 PM, Stephen Warren wrote:
 On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
 ...
 The only thing that this patch tries to solve is when a driver expect to 
 request
 a IRQ and it doesn't care if is a real IRQ line from an interrupt 
 controller or
 a GPIO pin mapped as an IRQ.

 That can be solved in the interrupt chip driver. The fact the previous
 attempt didn't work doesn't mean that it's impossible.

 
 Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
 cycle so it was safer to just revert the patches instead of analyzing the
 regression and providing a fix.
 
 If Linus is fond about taking a local fix for gpio-omap then we can try again
 as a RFC with a better test coverage than before (although the patches had
 several tested and acked-by but no one tested on OMAP1 until the patches were
 merged) getting some TI folks in the loop who have access to those ancient 
 OMAP1
 devices. That way we can repost as a patch just once we are definitely sure 
 that
 it won't cause a regression on any OMAP platform.
 
 With board files we used to explicitly do the GPIO setup
 (gpio_{request,direction_input}) but with DT these board files are gone and 
 we
 need a way to setup a GPIO implicitly when is mapped as an IRQ.

 Well, that's just an example of hacking around something in a board file
 that should have been fixed in the GPIO/IRQ controller.

 That's the only use case that this patch covers.
 ...
 Also, it would be great if we can find a temporary solution so we can 
 finally
 have ethernet working with DT on most OMAP2+ boards. Since I expect that 
 doing
 the mentioned change on gpiolib would take at least a couple of kernel 
 releases.

 Really? I thought this patch was error-checking to make sure that
 different drivers didn't request a GPIO and an IRQ that are actually the
 same signal. This patch shouldn't affect any functionality except in
 cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
 driver).

 
 Yes, it doesn't do any error-checking neither prevent a driver to request a 
 GPIO
 and an IRQ that are the same signal (as long as this is supported by the
 GPIO/IRQ controller and its driver). The only thing that Linus patch does is 
 to
 auto request a GPIO for pins that are mapped as IRQ in DT (i.e: 
 interrupt-parent
 = gpio6).
 
 The name of the function introduced by Linus is
 of_gpiochip_interrupt_consistency_check() but probably a better name is
 of_gpiochip_autorequest_irq() or something like that.
 
 A similar behavior could be obtained by let's say calling gpio_request() in
 irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
 instead of doing it in the GPIO chip DT core as Linus does with his patch.
 
 That's why I don't understand why Linus patch could be an issue for drivers 
 that
 needs to request both the GPIO and the mapped IRQ.
 
 If this is already supported then nothing will break. If the driver tries to
 request the GPIO twice because this is already made by the DT core then I 
 think
 is a bug in the driver and has to be fixed to support DT since there won't be
 any need to do this manually anymore.

Quick question- Wouldn't this mean that there have to be 2 code paths in the
driver then.. One for DT-case where gpio_request double request is prevented,
and one for non-DT case where you would do the gpio_request followed by
request_irq. I'm not sure if there are drivers today that use DT and need to
converted to prevent the double request? If there are, and such drivers are also
used on non-DT platform, then we would have to fork 2 different code paths while
requesting an IRQ for DT/non-DT in the driver..

Also this path kind of enforces that the driver author must be aware whether
driver is being used on DT or non-DT platform.. Linus mentioned enforcing of
semantics, this is also enforcing semantics no?

Looks like the correct fix for this as discussed in this thread should be to
associate the struct device with the GPIO request, remember it and use the info
during request_irq. This is provided that the old pattern of gpio_request and
request_irq is continued to be used and working. I know this is some time away
so I'm not too opinionated about it.

Regards,

-Joel




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> On 09/04/2013 03:05 AM, Lars Poeschel wrote:

> > The driver that tries to use the GPIO requested by this patch before HAS to 
> > fail. This is exactly the intention of this patch. We don't want the GPIO 
> > to 
> > be requested any more, if it is used as an interrupt pin.

> That will break existing drivers. There are drivers that request the
> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> that way.

Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


signature.asc
Description: Digital signature

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
 On 09/04/2013 03:05 AM, Lars Poeschel wrote:

  The driver that tries to use the GPIO requested by this patch before HAS to 
  fail. This is exactly the intention of this patch. We don't want the GPIO 
  to 
  be requested any more, if it is used as an interrupt pin.

 That will break existing drivers. There are drivers that request the
 same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
 that way.

Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).


signature.asc
Description: Digital signature

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:21 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
>> On 09/02/2013 03:38 AM, Lars Poeschel wrote:
>>> Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
>> ...
>>
 Yet the current patch only addresses a limited set of cases, since it
 doesn't hook the APIs but rather parses the DT. It doesn't cover the
 non-DT case. It should if the feature is useful.
>>>
>>> As pointed out before, only DT has this problem, that,
>>
>> That's completely false. Both DT and non-DT can represent the exact same
>> HW, and use the exact same drivers. It's equally possible to write a bug
>> in a board file or a DT file (i.e. a typo or incorrect reading of the
>> schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
>> for there to be conflicts. Any solution to this issue needs to address
>> both cases.
> 
> This is again not the point. This is not what this patch is trying to solve. 
> The patch is trying to solve problem A. But you are talking about problem B.
> Sure I can write a bug in board files and I can write a bug in DT files. The 
> patch is not trying to prevent that. This is a completly different thing.

I'm not trying to assert what this current patch was written to solve. I
am asserting what the patch should be (have been) written to solve.

My point is that we shouldn't only solve the problem in case B, but
rather solve the problem in all cases (A, B, and anything else). Doing
anything else isn't useful in my opinion; it's too special-cased.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
>> On 09/02/2013 03:25 AM, Lars Poeschel wrote:
>>> Some leagcy drivers currently do this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>>
>>> In that case request_irq should not fail because the driver is already the
>>> correct owner of this gpio. But if some other entity owns this gpio it
>>> should fail.
>>
>> Yes.
>>
>>> Also if I understand you correct the other way round should also possible:
>>>
>>> request_irq(gpio_to_irq(gpio));
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>>
>>> request_irq() also requests the gpio then but the following request_gpio()
>>> should also not fail.
>>
>> I don't believe that code sequence is currently banned. If we want to
>> ban it, we should document that. Until this is documented as being
>> banned, I think we must fully support that code sequence.
>>
>>> To have it work that way we have to track the owners of all requested
>>> gpios
>>> somewhere. Or am I wrong here?
>>> Where and how to record these owners? Can gpio system achieve this? gpios
>>> are requested without an owning device.
>>
>> Yes. But, I believe we need to fully track every GPIO/IRQ owner already
>> right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
>> uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
>> has already requested them, then we already need to fully track GPIO/IRQ
>> ownership to make sure that the driver's own requests aren't failed
>> because the DT/GPIO core has already requested them on its behalf.
> 
> The driver that tries to use the GPIO requested by this patch before HAS to 
> fail. This is exactly the intention of this patch. We don't want the GPIO to 
> be requested any more, if it is used as an interrupt pin.

That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.

> To be able to implement the way you proposed some mails before, I have to be 
> able to do some checks that to my opinion (and if I understand Linus 
> correctly) aren't possible at the moment.

True.

> So I asked, where and how to record 
> these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
> you please shed some light on this! Thanks!

Alex Courbot's (proposed) new gpiod API has a "dev" parameter to
gpiod_request() which could be used for this purpose. Something similar
could be done for interrupts.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 02:35 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
>> On 09/03/2013 06:35 AM, Linus Walleij wrote:
>>> On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren  
> wrote:
 I still haven't seen an answer to why we really care about this; how
 many times has code actually allocated the same GPIO/IRQ when it
 shouldn't, in a way that it wasn't detectable by some other mechanism,
 i.e. the feature just not working? Why are we even trying to solve this
 issue? I'm not totally convinced it even makes sense to try and solve it.
>>>
>>> We care about this because a number of OMAP boards are not
>>> working properly when booted from device tree, and they have a hard
>>> time figuring out a solution to the problem. Last try exploded. Now
>>> they are looking to create a patch that will fix the actual problem.
>>
>> Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
>> If so, let's just add it.
> 
> That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to 
> be 
> compiled in. Drivers have to work without that requirement.
> That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
> without that.

Drivers will/could work fine without those options enabled and
filesystems mounted. Nothing in this patch series affects that, nor
would not applying the patch series.

However, you only need this patch series if there's some kind of
problematic resource conflict and you want to debug it. If your system
has a problem and you want to debug it, it seems entirely reasonable to
make use of (require making use of) the debugging tools that are already
part of the kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
> On 09/02/2013 03:38 AM, Lars Poeschel wrote:
> > Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
> ...
> 
> >> Yet the current patch only addresses a limited set of cases, since it
> >> doesn't hook the APIs but rather parses the DT. It doesn't cover the
> >> non-DT case. It should if the feature is useful.
> > 
> > As pointed out before, only DT has this problem, that,
> 
> That's completely false. Both DT and non-DT can represent the exact same
> HW, and use the exact same drivers. It's equally possible to write a bug
> in a board file or a DT file (i.e. a typo or incorrect reading of the
> schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
> for there to be conflicts. Any solution to this issue needs to address
> both cases.

This is again not the point. This is not what this patch is trying to solve. 
The patch is trying to solve problem A. But you are talking about problem B.
Sure I can write a bug in board files and I can write a bug in DT files. The 
patch is not trying to prevent that. This is a completly different thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
> On 09/02/2013 03:25 AM, Lars Poeschel wrote:
> > Some leagcy drivers currently do this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> > 
> > In that case request_irq should not fail because the driver is already the
> > correct owner of this gpio. But if some other entity owns this gpio it
> > should fail.
> 
> Yes.
> 
> > Also if I understand you correct the other way round should also possible:
> > 
> > request_irq(gpio_to_irq(gpio));
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > 
> > request_irq() also requests the gpio then but the following request_gpio()
> > should also not fail.
> 
> I don't believe that code sequence is currently banned. If we want to
> ban it, we should document that. Until this is documented as being
> banned, I think we must fully support that code sequence.
> 
> > To have it work that way we have to track the owners of all requested
> > gpios
> > somewhere. Or am I wrong here?
> > Where and how to record these owners? Can gpio system achieve this? gpios
> > are requested without an owning device.
> 
> Yes. But, I believe we need to fully track every GPIO/IRQ owner already
> right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
> uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
> has already requested them, then we already need to fully track GPIO/IRQ
> ownership to make sure that the driver's own requests aren't failed
> because the DT/GPIO core has already requested them on its behalf.

The driver that tries to use the GPIO requested by this patch before HAS to 
fail. This is exactly the intention of this patch. We don't want the GPIO to 
be requested any more, if it is used as an interrupt pin.
To be able to implement the way you proposed some mails before, I have to be 
able to do some checks that to my opinion (and if I understand Linus 
correctly) aren't possible at the moment. So I asked, where and how to record 
these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
you please shed some light on this! Thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
> On 09/03/2013 06:35 AM, Linus Walleij wrote:
> > On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren  
wrote:
> >> I still haven't seen an answer to why we really care about this; how
> >> many times has code actually allocated the same GPIO/IRQ when it
> >> shouldn't, in a way that it wasn't detectable by some other mechanism,
> >> i.e. the feature just not working? Why are we even trying to solve this
> >> issue? I'm not totally convinced it even makes sense to try and solve it.
> > 
> > We care about this because a number of OMAP boards are not
> > working properly when booted from device tree, and they have a hard
> > time figuring out a solution to the problem. Last try exploded. Now
> > they are looking to create a patch that will fix the actual problem.
> 
> Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
> If so, let's just add it.

That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to be 
compiled in. Drivers have to work without that requirement.
That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
without that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
 On 09/03/2013 06:35 AM, Linus Walleij wrote:
  On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren swar...@wwwdotorg.org 
wrote:
  I still haven't seen an answer to why we really care about this; how
  many times has code actually allocated the same GPIO/IRQ when it
  shouldn't, in a way that it wasn't detectable by some other mechanism,
  i.e. the feature just not working? Why are we even trying to solve this
  issue? I'm not totally convinced it even makes sense to try and solve it.
  
  We care about this because a number of OMAP boards are not
  working properly when booted from device tree, and they have a hard
  time figuring out a solution to the problem. Last try exploded. Now
  they are looking to create a patch that will fix the actual problem.
 
 Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
 If so, let's just add it.

That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to be 
compiled in. Drivers have to work without that requirement.
That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
without that.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
 On 09/02/2013 03:25 AM, Lars Poeschel wrote:
  Some leagcy drivers currently do this:
  
  request_gpio(gpio);
  gpio_direction_input(gpio);
  request_irq(gpio_to_irq(gpio));
  
  In that case request_irq should not fail because the driver is already the
  correct owner of this gpio. But if some other entity owns this gpio it
  should fail.
 
 Yes.
 
  Also if I understand you correct the other way round should also possible:
  
  request_irq(gpio_to_irq(gpio));
  request_gpio(gpio);
  gpio_direction_input(gpio);
  
  request_irq() also requests the gpio then but the following request_gpio()
  should also not fail.
 
 I don't believe that code sequence is currently banned. If we want to
 ban it, we should document that. Until this is documented as being
 banned, I think we must fully support that code sequence.
 
  To have it work that way we have to track the owners of all requested
  gpios
  somewhere. Or am I wrong here?
  Where and how to record these owners? Can gpio system achieve this? gpios
  are requested without an owning device.
 
 Yes. But, I believe we need to fully track every GPIO/IRQ owner already
 right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
 uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
 has already requested them, then we already need to fully track GPIO/IRQ
 ownership to make sure that the driver's own requests aren't failed
 because the DT/GPIO core has already requested them on its behalf.

The driver that tries to use the GPIO requested by this patch before HAS to 
fail. This is exactly the intention of this patch. We don't want the GPIO to 
be requested any more, if it is used as an interrupt pin.
To be able to implement the way you proposed some mails before, I have to be 
able to do some checks that to my opinion (and if I understand Linus 
correctly) aren't possible at the moment. So I asked, where and how to record 
these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
you please shed some light on this! Thanks!

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
 On 09/02/2013 03:38 AM, Lars Poeschel wrote:
  Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
 ...
 
  Yet the current patch only addresses a limited set of cases, since it
  doesn't hook the APIs but rather parses the DT. It doesn't cover the
  non-DT case. It should if the feature is useful.
  
  As pointed out before, only DT has this problem, that,
 
 That's completely false. Both DT and non-DT can represent the exact same
 HW, and use the exact same drivers. It's equally possible to write a bug
 in a board file or a DT file (i.e. a typo or incorrect reading of the
 schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
 for there to be conflicts. Any solution to this issue needs to address
 both cases.

This is again not the point. This is not what this patch is trying to solve. 
The patch is trying to solve problem A. But you are talking about problem B.
Sure I can write a bug in board files and I can write a bug in DT files. The 
patch is not trying to prevent that. This is a completly different thing.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 02:35 AM, Lars Poeschel wrote:
 On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
 On 09/03/2013 06:35 AM, Linus Walleij wrote:
 On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 I still haven't seen an answer to why we really care about this; how
 many times has code actually allocated the same GPIO/IRQ when it
 shouldn't, in a way that it wasn't detectable by some other mechanism,
 i.e. the feature just not working? Why are we even trying to solve this
 issue? I'm not totally convinced it even makes sense to try and solve it.

 We care about this because a number of OMAP boards are not
 working properly when booted from device tree, and they have a hard
 time figuring out a solution to the problem. Last try exploded. Now
 they are looking to create a patch that will fix the actual problem.

 Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
 If so, let's just add it.
 
 That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to 
 be 
 compiled in. Drivers have to work without that requirement.
 That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
 without that.

Drivers will/could work fine without those options enabled and
filesystems mounted. Nothing in this patch series affects that, nor
would not applying the patch series.

However, you only need this patch series if there's some kind of
problematic resource conflict and you want to debug it. If your system
has a problem and you want to debug it, it seems entirely reasonable to
make use of (require making use of) the debugging tools that are already
part of the kernel.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:05 AM, Lars Poeschel wrote:
 On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
 On 09/02/2013 03:25 AM, Lars Poeschel wrote:
 Some leagcy drivers currently do this:

 request_gpio(gpio);
 gpio_direction_input(gpio);
 request_irq(gpio_to_irq(gpio));

 In that case request_irq should not fail because the driver is already the
 correct owner of this gpio. But if some other entity owns this gpio it
 should fail.

 Yes.

 Also if I understand you correct the other way round should also possible:

 request_irq(gpio_to_irq(gpio));
 request_gpio(gpio);
 gpio_direction_input(gpio);

 request_irq() also requests the gpio then but the following request_gpio()
 should also not fail.

 I don't believe that code sequence is currently banned. If we want to
 ban it, we should document that. Until this is documented as being
 banned, I think we must fully support that code sequence.

 To have it work that way we have to track the owners of all requested
 gpios
 somewhere. Or am I wrong here?
 Where and how to record these owners? Can gpio system achieve this? gpios
 are requested without an owning device.

 Yes. But, I believe we need to fully track every GPIO/IRQ owner already
 right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
 uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
 has already requested them, then we already need to fully track GPIO/IRQ
 ownership to make sure that the driver's own requests aren't failed
 because the DT/GPIO core has already requested them on its behalf.
 
 The driver that tries to use the GPIO requested by this patch before HAS to 
 fail. This is exactly the intention of this patch. We don't want the GPIO to 
 be requested any more, if it is used as an interrupt pin.

That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.

 To be able to implement the way you proposed some mails before, I have to be 
 able to do some checks that to my opinion (and if I understand Linus 
 correctly) aren't possible at the moment.

True.

 So I asked, where and how to record 
 these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
 you please shed some light on this! Thanks!

Alex Courbot's (proposed) new gpiod API has a dev parameter to
gpiod_request() which could be used for this purpose. Something similar
could be done for interrupts.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:21 AM, Lars Poeschel wrote:
 On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
 On 09/02/2013 03:38 AM, Lars Poeschel wrote:
 Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
 ...

 Yet the current patch only addresses a limited set of cases, since it
 doesn't hook the APIs but rather parses the DT. It doesn't cover the
 non-DT case. It should if the feature is useful.

 As pointed out before, only DT has this problem, that,

 That's completely false. Both DT and non-DT can represent the exact same
 HW, and use the exact same drivers. It's equally possible to write a bug
 in a board file or a DT file (i.e. a typo or incorrect reading of the
 schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
 for there to be conflicts. Any solution to this issue needs to address
 both cases.
 
 This is again not the point. This is not what this patch is trying to solve. 
 The patch is trying to solve problem A. But you are talking about problem B.
 Sure I can write a bug in board files and I can write a bug in DT files. The 
 patch is not trying to prevent that. This is a completly different thing.

I'm not trying to assert what this current patch was written to solve. I
am asserting what the patch should be (have been) written to solve.

My point is that we shouldn't only solve the problem in case B, but
rather solve the problem in all cases (A, B, and anything else). Doing
anything else isn't useful in my opinion; it's too special-cased.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:35 AM, Linus Walleij wrote:
> On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren  wrote:
> 
>> I still haven't seen an answer to why we really care about this; how
>> many times has code actually allocated the same GPIO/IRQ when it
>> shouldn't, in a way that it wasn't detectable by some other mechanism,
>> i.e. the feature just not working? Why are we even trying to solve this
>> issue? I'm not totally convinced it even makes sense to try and solve it.
> 
> We care about this because a number of OMAP boards are not
> working properly when booted from device tree, and they have a hard
> time figuring out a solution to the problem. Last try exploded. Now
> they are looking to create a patch that will fix the actual problem.

Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
If so, let's just add it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:43 AM, Linus Walleij wrote:
> On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren  wrote:
>> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
>> ...
>>> We have been trying to solve this issue for a few months by now and Linus'
>>> approach seems to be the most sensible solution to me.
>>>
>>> Drivers that request an IRQ and assume that platform code will request and 
>>> setup
>>> the GPIO have been broken since the boards using these drivers were 
>>> migrated to
>>> DT (e.g: smsc911x on OMAP2+ boards).
>>
>> That's only true if the driver for the GPIO controller is buggy.
>> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
>> simply needs to set up the pin as an interrupt input, then it doesn't
>> matter which order the driver does things.
> 
> As mentioned it can't do that, because doing that creates a
> restriction on which order the driver does things...

I am not convinced here. Which driver (GPIO/IRQ controller driver, or
the driver which uses GPIOs/IRQs?) Which operations?

> But you mentioned that you wanted an API that would account
> for the case where the *same driver* requested the same resource
> (a GPIO line) to be used for both IRQ and GPIO, through two
> different calls.
> 
> I would be happy to see how we could do that, preferably in a
> generic way.
> 
> Since the gpio_request() does not contain the signature of the
> calling driver I don't see how we could do this without refactoring
> the whole world.
> 
> In that case it would probably be easiest to
> *first* proceed to complete Alexandre's suggested refactorings for
> GPIO descriptors, which tie down GPIOs to be requested like
> clocks and regulators and thus tied to a device, so we can from
> there proceed to implement such a conditional request,
> as we will then have the required information in the GPIO
> subsystem.

Indeed, that does seem necessary if you want the GPIO/IRQ core to be
able to implement this feature.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:38 AM, Lars Poeschel wrote:
> Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
...
>> Yet the current patch only addresses a limited set of cases, since it
>> doesn't hook the APIs but rather parses the DT. It doesn't cover the
>> non-DT case. It should if the feature is useful.
> 
> As pointed out before, only DT has this problem, that,

That's completely false. Both DT and non-DT can represent the exact same
HW, and use the exact same drivers. It's equally possible to write a bug
in a board file or a DT file (i.e. a typo or incorrect reading of the
schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
for there to be conflicts. Any solution to this issue needs to address
both cases.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:25 AM, Lars Poeschel wrote:
> Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
>> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
>> ...
>>
>>> We have been trying to solve this issue for a few months by now and Linus'
>>> approach seems to be the most sensible solution to me.
>>>
>>> Drivers that request an IRQ and assume that platform code will request and
>>> setup the GPIO have been broken since the boards using these drivers were
>>> migrated to DT (e.g: smsc911x on OMAP2+ boards).
>>
>> That's only true if the driver for the GPIO controller is buggy.
>> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
>> simply needs to set up the pin as an interrupt input, then it doesn't
>> matter which order the driver does things.
> 
> Is it really that easy?

Yes.

> request_irq() should request the gpio and set it to input that it needs to 
> fulfill the irq request. That would then be the way to go for new drivers and 
> in the DT case.

Either explicitly request the GPIO, or simply directly program the HW in
whatever way is required for the pin to operate as an IRQ.

> Some leagcy drivers currently do this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
> 
> In that case request_irq should not fail because the driver is already the 
> correct owner of this gpio. But if some other entity owns this gpio it should 
> fail.

Yes.

> Also if I understand you correct the other way round should also possible:
> 
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);
> 
> request_irq() also requests the gpio then but the following request_gpio() 
> should also not fail.

I don't believe that code sequence is currently banned. If we want to
ban it, we should document that. Until this is documented as being
banned, I think we must fully support that code sequence.

> To have it work that way we have to track the owners of all requested gpios 
> somewhere. Or am I wrong here?
> Where and how to record these owners? Can gpio system achieve this? gpios are 
> requested without an owning device.

Yes. But, I believe we need to fully track every GPIO/IRQ owner already
right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
has already requested them, then we already need to fully track GPIO/IRQ
ownership to make sure that the driver's own requests aren't failed
because the DT/GPIO core has already requested them on its behalf.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/31/2013 09:32 AM, anish singh wrote:
> On Sat, Aug 31, 2013 at 1:23 AM, Stephen Warren  > wrote:

> I still haven't seen an answer to why we really care about this; how
> many times has code actually allocated the same GPIO/IRQ when it
> shouldn't, in a way that it wasn't detectable by some other mechanism,
> i.e. the feature just not working? Why are we even trying to solve this
> issue? I'm not totally convinced it even makes sense to try and
> solve it.
> 
>  
> Probably this issue(same gpio/irq being used by multiple drivers) is
> very rare
> but debugging it is bit difficult.

Really? It's easy to just look in /proc/interrupts and
/sys/kernel/debug/gpio.

> This generally happens when we are working
> on latest revised boards where latest gpio number of some driver
> conflicts with the some other driver.

At least in the DT case, which is all that this patch solves, the DT is
describing the HW GPIO/IRQ numbers, so the Linux GPIO numbers are
irrelevant; everything is expressed as raw HW numbers, which are quite
easy to check.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren  wrote:
> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
> ...
>> We have been trying to solve this issue for a few months by now and Linus'
>> approach seems to be the most sensible solution to me.
>>
>> Drivers that request an IRQ and assume that platform code will request and 
>> setup
>> the GPIO have been broken since the boards using these drivers were migrated 
>> to
>> DT (e.g: smsc911x on OMAP2+ boards).
>
> That's only true if the driver for the GPIO controller is buggy.
> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
> simply needs to set up the pin as an interrupt input, then it doesn't
> matter which order the driver does things.

As mentioned it can't do that, because doing that creates a
restriction on which order the driver does things...

But you mentioned that you wanted an API that would account
for the case where the *same driver* requested the same resource
(a GPIO line) to be used for both IRQ and GPIO, through two
different calls.

I would be happy to see how we could do that, preferably in a
generic way.

Since the gpio_request() does not contain the signature of the
calling driver I don't see how we could do this without refactoring
the whole world.

In that case it would probably be easiest to
*first* proceed to complete Alexandre's suggested refactorings for
GPIO descriptors, which tie down GPIOs to be requested like
clocks and regulators and thus tied to a device, so we can from
there proceed to implement such a conditional request,
as we will then have the required information in the GPIO
subsystem.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren  wrote:

> I still haven't seen an answer to why we really care about this; how
> many times has code actually allocated the same GPIO/IRQ when it
> shouldn't, in a way that it wasn't detectable by some other mechanism,
> i.e. the feature just not working? Why are we even trying to solve this
> issue? I'm not totally convinced it even makes sense to try and solve it.

We care about this because a number of OMAP boards are not
working properly when booted from device tree, and they have a hard
time figuring out a solution to the problem. Last try exploded. Now
they are looking to create a patch that will fix the actual problem.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/31/2013 09:32 AM, anish singh wrote:
 On Sat, Aug 31, 2013 at 1:23 AM, Stephen Warren swar...@wwwdotorg.org
 mailto:swar...@wwwdotorg.org wrote:

 I still haven't seen an answer to why we really care about this; how
 many times has code actually allocated the same GPIO/IRQ when it
 shouldn't, in a way that it wasn't detectable by some other mechanism,
 i.e. the feature just not working? Why are we even trying to solve this
 issue? I'm not totally convinced it even makes sense to try and
 solve it.
 
  
 Probably this issue(same gpio/irq being used by multiple drivers) is
 very rare
 but debugging it is bit difficult.

Really? It's easy to just look in /proc/interrupts and
/sys/kernel/debug/gpio.

 This generally happens when we are working
 on latest revised boards where latest gpio number of some driver
 conflicts with the some other driver.

At least in the DT case, which is all that this patch solves, the DT is
describing the HW GPIO/IRQ numbers, so the Linux GPIO numbers are
irrelevant; everything is expressed as raw HW numbers, which are quite
easy to check.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:25 AM, Lars Poeschel wrote:
 Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
 On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
 ...

 We have been trying to solve this issue for a few months by now and Linus'
 approach seems to be the most sensible solution to me.

 Drivers that request an IRQ and assume that platform code will request and
 setup the GPIO have been broken since the boards using these drivers were
 migrated to DT (e.g: smsc911x on OMAP2+ boards).

 That's only true if the driver for the GPIO controller is buggy.
 Whatever request_irq() maps down to in the GPIO/IRQ controller driver
 simply needs to set up the pin as an interrupt input, then it doesn't
 matter which order the driver does things.
 
 Is it really that easy?

Yes.

 request_irq() should request the gpio and set it to input that it needs to 
 fulfill the irq request. That would then be the way to go for new drivers and 
 in the DT case.

Either explicitly request the GPIO, or simply directly program the HW in
whatever way is required for the pin to operate as an IRQ.

 Some leagcy drivers currently do this:
 
 request_gpio(gpio);
 gpio_direction_input(gpio);
 request_irq(gpio_to_irq(gpio));
 
 In that case request_irq should not fail because the driver is already the 
 correct owner of this gpio. But if some other entity owns this gpio it should 
 fail.

Yes.

 Also if I understand you correct the other way round should also possible:
 
 request_irq(gpio_to_irq(gpio));
 request_gpio(gpio);
 gpio_direction_input(gpio);
 
 request_irq() also requests the gpio then but the following request_gpio() 
 should also not fail.

I don't believe that code sequence is currently banned. If we want to
ban it, we should document that. Until this is documented as being
banned, I think we must fully support that code sequence.

 To have it work that way we have to track the owners of all requested gpios 
 somewhere. Or am I wrong here?
 Where and how to record these owners? Can gpio system achieve this? gpios are 
 requested without an owning device.

Yes. But, I believe we need to fully track every GPIO/IRQ owner already
right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
has already requested them, then we already need to fully track GPIO/IRQ
ownership to make sure that the driver's own requests aren't failed
because the DT/GPIO core has already requested them on its behalf.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:38 AM, Lars Poeschel wrote:
 Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
...
 Yet the current patch only addresses a limited set of cases, since it
 doesn't hook the APIs but rather parses the DT. It doesn't cover the
 non-DT case. It should if the feature is useful.
 
 As pointed out before, only DT has this problem, that,

That's completely false. Both DT and non-DT can represent the exact same
HW, and use the exact same drivers. It's equally possible to write a bug
in a board file or a DT file (i.e. a typo or incorrect reading of the
schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
for there to be conflicts. Any solution to this issue needs to address
both cases.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:43 AM, Linus Walleij wrote:
 On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
 ...
 We have been trying to solve this issue for a few months by now and Linus'
 approach seems to be the most sensible solution to me.

 Drivers that request an IRQ and assume that platform code will request and 
 setup
 the GPIO have been broken since the boards using these drivers were 
 migrated to
 DT (e.g: smsc911x on OMAP2+ boards).

 That's only true if the driver for the GPIO controller is buggy.
 Whatever request_irq() maps down to in the GPIO/IRQ controller driver
 simply needs to set up the pin as an interrupt input, then it doesn't
 matter which order the driver does things.
 
 As mentioned it can't do that, because doing that creates a
 restriction on which order the driver does things...

I am not convinced here. Which driver (GPIO/IRQ controller driver, or
the driver which uses GPIOs/IRQs?) Which operations?

 But you mentioned that you wanted an API that would account
 for the case where the *same driver* requested the same resource
 (a GPIO line) to be used for both IRQ and GPIO, through two
 different calls.
 
 I would be happy to see how we could do that, preferably in a
 generic way.
 
 Since the gpio_request() does not contain the signature of the
 calling driver I don't see how we could do this without refactoring
 the whole world.
 
 In that case it would probably be easiest to
 *first* proceed to complete Alexandre's suggested refactorings for
 GPIO descriptors, which tie down GPIOs to be requested like
 clocks and regulators and thus tied to a device, so we can from
 there proceed to implement such a conditional request,
 as we will then have the required information in the GPIO
 subsystem.

Indeed, that does seem necessary if you want the GPIO/IRQ core to be
able to implement this feature.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:35 AM, Linus Walleij wrote:
 On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 
 I still haven't seen an answer to why we really care about this; how
 many times has code actually allocated the same GPIO/IRQ when it
 shouldn't, in a way that it wasn't detectable by some other mechanism,
 i.e. the feature just not working? Why are we even trying to solve this
 issue? I'm not totally convinced it even makes sense to try and solve it.
 
 We care about this because a number of OMAP boards are not
 working properly when booted from device tree, and they have a hard
 time figuring out a solution to the problem. Last try exploded. Now
 they are looking to create a patch that will fix the actual problem.

Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
If so, let's just add it.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren swar...@wwwdotorg.org wrote:

 I still haven't seen an answer to why we really care about this; how
 many times has code actually allocated the same GPIO/IRQ when it
 shouldn't, in a way that it wasn't detectable by some other mechanism,
 i.e. the feature just not working? Why are we even trying to solve this
 issue? I'm not totally convinced it even makes sense to try and solve it.

We care about this because a number of OMAP boards are not
working properly when booted from device tree, and they have a hard
time figuring out a solution to the problem. Last try exploded. Now
they are looking to create a patch that will fix the actual problem.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
 ...
 We have been trying to solve this issue for a few months by now and Linus'
 approach seems to be the most sensible solution to me.

 Drivers that request an IRQ and assume that platform code will request and 
 setup
 the GPIO have been broken since the boards using these drivers were migrated 
 to
 DT (e.g: smsc911x on OMAP2+ boards).

 That's only true if the driver for the GPIO controller is buggy.
 Whatever request_irq() maps down to in the GPIO/IRQ controller driver
 simply needs to set up the pin as an interrupt input, then it doesn't
 matter which order the driver does things.

As mentioned it can't do that, because doing that creates a
restriction on which order the driver does things...

But you mentioned that you wanted an API that would account
for the case where the *same driver* requested the same resource
(a GPIO line) to be used for both IRQ and GPIO, through two
different calls.

I would be happy to see how we could do that, preferably in a
generic way.

Since the gpio_request() does not contain the signature of the
calling driver I don't see how we could do this without refactoring
the whole world.

In that case it would probably be easiest to
*first* proceed to complete Alexandre's suggested refactorings for
GPIO descriptors, which tie down GPIOs to be requested like
clocks and regulators and thus tied to a device, so we can from
there proceed to implement such a conditional request,
as we will then have the required information in the GPIO
subsystem.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
> On 08/29/2013 01:26 PM, Linus Walleij wrote:
> > On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren  
wrote:
> >> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
> >>> Currently the kernel is ambigously treating GPIOs and interrupts
> >>> from a GPIO controller: GPIOs and interrupts are treated as
> >>> orthogonal. This unfortunately makes it unclear how to actually
> >>> retrieve and request a GPIO line or interrupt from a GPIO
> >>> controller in the device tree probe path.
> >> 
> >> I still think that this patch is the wrong approach. Instead, the logic
> >> should be hooked into gpio_request() and request_irq(). This patch only
> >> addresses DT, and ignores anything else, hence doesn't seem like the
> >> right level of abstraction to plug in, since the issue is not related to
> >> DT.> 
> > We tried to do it that way, and it exploded. See commit
> > b4419e1a15905191661ffe75ba2f9e649f5d565e
> > "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> > 
> > Here request_irq() augmented through its irqdomain to
> > issue gpio_request_one().
> > 
> > Why was this patch reverted? It seems this is what has not
> > managed to reach the audience here.
> > 
> > It turns out some drivers were already doing this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> > 
> > Looks perfectly valid right?
> > 
> > Not so: after the above change, we tried to request the
> > GPIO a *second time* in the GPIO driver's irq map function,
> > and of course it failed, as it was already taken.
> > 
> > So saying that it should be done in the request_irq()
> > function is imposing this other semantic on the kernel
> > instead: you must *NOT* request the GPIO with
> > request_gpio() if you're going to use it as an IRQ.
> 
> Surely both request_gpio() and request_irq() must both request the GPIO
> (amongst other things), with the caveat that if the same driver does
> both, this specific "sharing" is allowed.

As explained in my previous mail, I do not see, how this should work.

> If that won't work, then the very core concept behind what this patch is
> attempting to do won't work.

The concept only does not work in the non-DT case, which we do neither try to 
address with this patch.

> > (Also, it force us to implement the same code in each
> > and every driver, but that is a lesser problem.)
> 
> Drivers don't have to do the request; the IRQ/GPIO core can do this,
> with drivers simply providing an irq_to_gpio() (which only returns valid
> data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
> HW).
> 
> > I don't quite understand what is so hard around this.
> > We cannot get away from restricting the semantics in
> > some way, if gpio-controllers shall also be interrupt-controllers,
> > the current patch is the least intrusive I've seen so far.
> 
> Yet the current patch only addresses a limited set of cases, since it
> doesn't hook the APIs but rather parses the DT. It doesn't cover the
> non-DT case. It should if the feature is useful.

As pointed out before, only DT has this problem, that, whatever you do, 
otherwise it has no chance to request a gpio for a gpio-backed irq. Drivers 
can do this and board files also can do this.
I agree with you that it is somewhat odd, that there are two different ways, 
doing the same thing, but I don't see another way yet.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
> ...
> 
> > We have been trying to solve this issue for a few months by now and Linus'
> > approach seems to be the most sensible solution to me.
> > 
> > Drivers that request an IRQ and assume that platform code will request and
> > setup the GPIO have been broken since the boards using these drivers were
> > migrated to DT (e.g: smsc911x on OMAP2+ boards).
> 
> That's only true if the driver for the GPIO controller is buggy.
> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
> simply needs to set up the pin as an interrupt input, then it doesn't
> matter which order the driver does things.

Is it really that easy?
request_irq() should request the gpio and set it to input that it needs to 
fulfill the irq request. That would then be the way to go for new drivers and 
in the DT case.
Some leagcy drivers currently do this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

In that case request_irq should not fail because the driver is already the 
correct owner of this gpio. But if some other entity owns this gpio it should 
fail.

Also if I understand you correct the other way round should also possible:

request_irq(gpio_to_irq(gpio));
request_gpio(gpio);
gpio_direction_input(gpio);

request_irq() also requests the gpio then but the following request_gpio() 
should also not fail.

To have it work that way we have to track the owners of all requested gpios 
somewhere. Or am I wrong here?
Where and how to record these owners? Can gpio system achieve this? gpios are 
requested without an owning device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
 On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
 ...
 
  We have been trying to solve this issue for a few months by now and Linus'
  approach seems to be the most sensible solution to me.
  
  Drivers that request an IRQ and assume that platform code will request and
  setup the GPIO have been broken since the boards using these drivers were
  migrated to DT (e.g: smsc911x on OMAP2+ boards).
 
 That's only true if the driver for the GPIO controller is buggy.
 Whatever request_irq() maps down to in the GPIO/IRQ controller driver
 simply needs to set up the pin as an interrupt input, then it doesn't
 matter which order the driver does things.

Is it really that easy?
request_irq() should request the gpio and set it to input that it needs to 
fulfill the irq request. That would then be the way to go for new drivers and 
in the DT case.
Some leagcy drivers currently do this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

In that case request_irq should not fail because the driver is already the 
correct owner of this gpio. But if some other entity owns this gpio it should 
fail.

Also if I understand you correct the other way round should also possible:

request_irq(gpio_to_irq(gpio));
request_gpio(gpio);
gpio_direction_input(gpio);

request_irq() also requests the gpio then but the following request_gpio() 
should also not fail.

To have it work that way we have to track the owners of all requested gpios 
somewhere. Or am I wrong here?
Where and how to record these owners? Can gpio system achieve this? gpios are 
requested without an owning device.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
 On 08/29/2013 01:26 PM, Linus Walleij wrote:
  On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren swar...@wwwdotorg.org 
wrote:
  On 08/26/2013 08:07 AM, Lars Poeschel wrote:
  Currently the kernel is ambigously treating GPIOs and interrupts
  from a GPIO controller: GPIOs and interrupts are treated as
  orthogonal. This unfortunately makes it unclear how to actually
  retrieve and request a GPIO line or interrupt from a GPIO
  controller in the device tree probe path.
  
  I still think that this patch is the wrong approach. Instead, the logic
  should be hooked into gpio_request() and request_irq(). This patch only
  addresses DT, and ignores anything else, hence doesn't seem like the
  right level of abstraction to plug in, since the issue is not related to
  DT. 
  We tried to do it that way, and it exploded. See commit
  b4419e1a15905191661ffe75ba2f9e649f5d565e
  gpio/omap: auto request GPIO as input if used as IRQ via DT
  
  Here request_irq() augmented through its irqdomain to
  issue gpio_request_one().
  
  Why was this patch reverted? It seems this is what has not
  managed to reach the audience here.
  
  It turns out some drivers were already doing this:
  
  request_gpio(gpio);
  gpio_direction_input(gpio);
  request_irq(gpio_to_irq(gpio));
  
  Looks perfectly valid right?
  
  Not so: after the above change, we tried to request the
  GPIO a *second time* in the GPIO driver's irq map function,
  and of course it failed, as it was already taken.
  
  So saying that it should be done in the request_irq()
  function is imposing this other semantic on the kernel
  instead: you must *NOT* request the GPIO with
  request_gpio() if you're going to use it as an IRQ.
 
 Surely both request_gpio() and request_irq() must both request the GPIO
 (amongst other things), with the caveat that if the same driver does
 both, this specific sharing is allowed.

As explained in my previous mail, I do not see, how this should work.

 If that won't work, then the very core concept behind what this patch is
 attempting to do won't work.

The concept only does not work in the non-DT case, which we do neither try to 
address with this patch.

  (Also, it force us to implement the same code in each
  and every driver, but that is a lesser problem.)
 
 Drivers don't have to do the request; the IRQ/GPIO core can do this,
 with drivers simply providing an irq_to_gpio() (which only returns valid
 data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
 HW).
 
  I don't quite understand what is so hard around this.
  We cannot get away from restricting the semantics in
  some way, if gpio-controllers shall also be interrupt-controllers,
  the current patch is the least intrusive I've seen so far.
 
 Yet the current patch only addresses a limited set of cases, since it
 doesn't hook the APIs but rather parses the DT. It doesn't cover the
 non-DT case. It should if the feature is useful.

As pointed out before, only DT has this problem, that, whatever you do, 
otherwise it has no chance to request a gpio for a gpio-backed irq. Drivers 
can do this and board files also can do this.
I agree with you that it is somewhat odd, that there are two different ways, 
doing the same thing, but I don't see another way yet.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
...
> We have been trying to solve this issue for a few months by now and Linus'
> approach seems to be the most sensible solution to me.
> 
> Drivers that request an IRQ and assume that platform code will request and 
> setup
> the GPIO have been broken since the boards using these drivers were migrated 
> to
> DT (e.g: smsc911x on OMAP2+ boards).

That's only true if the driver for the GPIO controller is buggy.
Whatever request_irq() maps down to in the GPIO/IRQ controller driver
simply needs to set up the pin as an interrupt input, then it doesn't
matter which order the driver does things.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 01:26 PM, Linus Walleij wrote:
> On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren  
> wrote:
>> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>>
>>> Currently the kernel is ambigously treating GPIOs and interrupts
>>> from a GPIO controller: GPIOs and interrupts are treated as
>>> orthogonal. This unfortunately makes it unclear how to actually
>>> retrieve and request a GPIO line or interrupt from a GPIO
>>> controller in the device tree probe path.
>>
>> I still think that this patch is the wrong approach. Instead, the logic
>> should be hooked into gpio_request() and request_irq(). This patch only
>> addresses DT, and ignores anything else, hence doesn't seem like the
>> right level of abstraction to plug in, since the issue is not related to DT.
> 
> We tried to do it that way, and it exploded. See commit
> b4419e1a15905191661ffe75ba2f9e649f5d565e
> "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> 
> Here request_irq() augmented through its irqdomain to
> issue gpio_request_one().
> 
> Why was this patch reverted? It seems this is what has not
> managed to reach the audience here.
> 
> It turns out some drivers were already doing this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
> 
> Looks perfectly valid right?
> 
> Not so: after the above change, we tried to request the
> GPIO a *second time* in the GPIO driver's irq map function,
> and of course it failed, as it was already taken.
> 
> So saying that it should be done in the request_irq()
> function is imposing this other semantic on the kernel
> instead: you must *NOT* request the GPIO with
> request_gpio() if you're going to use it as an IRQ.

Surely both request_gpio() and request_irq() must both request the GPIO
(amongst other things), with the caveat that if the same driver does
both, this specific "sharing" is allowed.

If that won't work, then the very core concept behind what this patch is
attempting to do won't work.

> (Also, it force us to implement the same code in each
> and every driver, but that is a lesser problem.)

Drivers don't have to do the request; the IRQ/GPIO core can do this,
with drivers simply providing an irq_to_gpio() (which only returns valid
data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
HW).

> I don't quite understand what is so hard around this.
> We cannot get away from restricting the semantics in
> some way, if gpio-controllers shall also be interrupt-controllers,
> the current patch is the least intrusive I've seen so far.

Yet the current patch only addresses a limited set of cases, since it
doesn't hook the APIs but rather parses the DT. It doesn't cover the
non-DT case. It should if the feature is useful.

> And I don't even think this is a Linux problem, handing
> out the same resource with two hands is ambigous and is
> going to cause problems with any operating system.
> It is better to restrict this and say in the binding doc that
> the gpio line cannot be < n> assigned when there
> is an interrupt on the same line.

That won't work; there is already HW that allows this and drivers that
assume this.

I still haven't seen an answer to why we really care about this; how
many times has code actually allocated the same GPIO/IRQ when it
shouldn't, in a way that it wasn't detectable by some other mechanism,
i.e. the feature just not working? Why are we even trying to solve this
issue? I'm not totally convinced it even makes sense to try and solve it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 01:26 PM, Linus Walleij wrote:
 On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 On 08/26/2013 08:07 AM, Lars Poeschel wrote:

 Currently the kernel is ambigously treating GPIOs and interrupts
 from a GPIO controller: GPIOs and interrupts are treated as
 orthogonal. This unfortunately makes it unclear how to actually
 retrieve and request a GPIO line or interrupt from a GPIO
 controller in the device tree probe path.

 I still think that this patch is the wrong approach. Instead, the logic
 should be hooked into gpio_request() and request_irq(). This patch only
 addresses DT, and ignores anything else, hence doesn't seem like the
 right level of abstraction to plug in, since the issue is not related to DT.
 
 We tried to do it that way, and it exploded. See commit
 b4419e1a15905191661ffe75ba2f9e649f5d565e
 gpio/omap: auto request GPIO as input if used as IRQ via DT
 
 Here request_irq() augmented through its irqdomain to
 issue gpio_request_one().
 
 Why was this patch reverted? It seems this is what has not
 managed to reach the audience here.
 
 It turns out some drivers were already doing this:
 
 request_gpio(gpio);
 gpio_direction_input(gpio);
 request_irq(gpio_to_irq(gpio));
 
 Looks perfectly valid right?
 
 Not so: after the above change, we tried to request the
 GPIO a *second time* in the GPIO driver's irq map function,
 and of course it failed, as it was already taken.
 
 So saying that it should be done in the request_irq()
 function is imposing this other semantic on the kernel
 instead: you must *NOT* request the GPIO with
 request_gpio() if you're going to use it as an IRQ.

Surely both request_gpio() and request_irq() must both request the GPIO
(amongst other things), with the caveat that if the same driver does
both, this specific sharing is allowed.

If that won't work, then the very core concept behind what this patch is
attempting to do won't work.

 (Also, it force us to implement the same code in each
 and every driver, but that is a lesser problem.)

Drivers don't have to do the request; the IRQ/GPIO core can do this,
with drivers simply providing an irq_to_gpio() (which only returns valid
data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
HW).

 I don't quite understand what is so hard around this.
 We cannot get away from restricting the semantics in
 some way, if gpio-controllers shall also be interrupt-controllers,
 the current patch is the least intrusive I've seen so far.

Yet the current patch only addresses a limited set of cases, since it
doesn't hook the APIs but rather parses the DT. It doesn't cover the
non-DT case. It should if the feature is useful.

 And I don't even think this is a Linux problem, handing
 out the same resource with two hands is ambigous and is
 going to cause problems with any operating system.
 It is better to restrict this and say in the binding doc that
 the gpio line cannot be gpio n assigned when there
 is an interrupt on the same line.

That won't work; there is already HW that allows this and drivers that
assume this.

I still haven't seen an answer to why we really care about this; how
many times has code actually allocated the same GPIO/IRQ when it
shouldn't, in a way that it wasn't detectable by some other mechanism,
i.e. the feature just not working? Why are we even trying to solve this
issue? I'm not totally convinced it even makes sense to try and solve it.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
...
 We have been trying to solve this issue for a few months by now and Linus'
 approach seems to be the most sensible solution to me.
 
 Drivers that request an IRQ and assume that platform code will request and 
 setup
 the GPIO have been broken since the boards using these drivers were migrated 
 to
 DT (e.g: smsc911x on OMAP2+ boards).

That's only true if the driver for the GPIO controller is buggy.
Whatever request_irq() maps down to in the GPIO/IRQ controller driver
simply needs to set up the pin as an interrupt input, then it doesn't
matter which order the driver does things.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 08/29/2013 09:26 PM, Linus Walleij wrote:
> On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren  
> wrote:
>> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>>
>>> Currently the kernel is ambigously treating GPIOs and interrupts
>>> from a GPIO controller: GPIOs and interrupts are treated as
>>> orthogonal. This unfortunately makes it unclear how to actually
>>> retrieve and request a GPIO line or interrupt from a GPIO
>>> controller in the device tree probe path.
>>
>> I still think that this patch is the wrong approach. Instead, the logic
>> should be hooked into gpio_request() and request_irq(). This patch only
>> addresses DT, and ignores anything else, hence doesn't seem like the
>> right level of abstraction to plug in, since the issue is not related to DT.
> 
> We tried to do it that way, and it exploded. See commit
> b4419e1a15905191661ffe75ba2f9e649f5d565e
> "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> 
> Here request_irq() augmented through its irqdomain to
> issue gpio_request_one().
> 
> Why was this patch reverted? It seems this is what has not
> managed to reach the audience here.
>

The mentioned patch also broke some ancient platforms that have not been (and
probably never will) migrated to DT like OMAP1.

So, after trying to add this logic by using a custom irq domain .map function
handler for the OMAP GPIO driver I agree with Linus that is better to provide a
solution at the Device Tree level to not affect platforms that are still using
legacy boot.

Legacy boot don't need a fix since it does not have this issue. Platform code
just request the GPIO and do the setup (gpio_direction_input) before the drivers
call to request_irq().

> It turns out some drivers were already doing this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
>
> Looks perfectly valid right?
>
> Not so: after the above change, we tried to request the
> GPIO a *second time* in the GPIO driver's irq map function,
> and of course it failed, as it was already taken.
>
> So saying that it should be done in the request_irq()
> function is imposing this other semantic on the kernel
> instead: you must *NOT* request the GPIO with
> request_gpio() if you're going to use it as an IRQ.
> 
> (Also, it force us to implement the same code in each
> and every driver, but that is a lesser problem.)
> 
> I don't quite understand what is so hard around this.
> We cannot get away from restricting the semantics in
> some way, if gpio-controllers shall also be interrupt-controllers,
> the current patch is the least intrusive I've seen so far.
> 

We have been trying to solve this issue for a few months by now and Linus'
approach seems to be the most sensible solution to me.

Drivers that request an IRQ and assume that platform code will request and setup
the GPIO have been broken since the boards using these drivers were migrated to
DT (e.g: smsc911x on OMAP2+ boards). I know is really hard to agree on the
better approach to solve this but it would be great if we can find a solution
and fix these broken platforms.

> And I don't even think this is a Linux problem, handing
> out the same resource with two hands is ambigous and is
> going to cause problems with any operating system.
> It is better to restrict this and say in the binding doc that
> the gpio line cannot be < n> assigned when there
> is an interrupt on the same line.
>

Indeed, if a driver wants to manually request a GPIO to be used as an IRQ, then
the DT binding for that driver should not use a gpio as "interrupt-parent" and
should leave the driver to handle this manually.

On the other hand, if a driver just calls request_irq() and it does not know if
the IRQ is a real interrupt line from an interrupt controller or a GPIO line
acting as an IRQ, then the DT binding should just have a required
"interrupt-parent" property to define the phandle for the interrupt controller
used which could or could not be a GPIO controller.

> We can start out by printing warnings when we fail to
> request the corresponding GPIO for an interrupt line
> though, it will be annoying enough and a kind of
> compromise.
>

If the GPIO pin is first requested because it is described to be an IRQ in a DT
and then a driver tries to request the same GPIO pin, then there is a bug in
either the DT or the driver IMHO. The same assumption is made with platform code
right now when using legacy boot, I don't understand why is different for the DT
case.

> Or it has to be the GPIO input hogs.
> 
> Yours,
> Linus Walleij
> 

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren  wrote:
> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
>
> I still think that this patch is the wrong approach. Instead, the logic
> should be hooked into gpio_request() and request_irq(). This patch only
> addresses DT, and ignores anything else, hence doesn't seem like the
> right level of abstraction to plug in, since the issue is not related to DT.

We tried to do it that way, and it exploded. See commit
b4419e1a15905191661ffe75ba2f9e649f5d565e
"gpio/omap: auto request GPIO as input if used as IRQ via DT"

Here request_irq() augmented through its irqdomain to
issue gpio_request_one().

Why was this patch reverted? It seems this is what has not
managed to reach the audience here.

It turns out some drivers were already doing this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

Looks perfectly valid right?

Not so: after the above change, we tried to request the
GPIO a *second time* in the GPIO driver's irq map function,
and of course it failed, as it was already taken.

So saying that it should be done in the request_irq()
function is imposing this other semantic on the kernel
instead: you must *NOT* request the GPIO with
request_gpio() if you're going to use it as an IRQ.

(Also, it force us to implement the same code in each
and every driver, but that is a lesser problem.)

I don't quite understand what is so hard around this.
We cannot get away from restricting the semantics in
some way, if gpio-controllers shall also be interrupt-controllers,
the current patch is the least intrusive I've seen so far.

And I don't even think this is a Linux problem, handing
out the same resource with two hands is ambigous and is
going to cause problems with any operating system.
It is better to restrict this and say in the binding doc that
the gpio line cannot be < n> assigned when there
is an interrupt on the same line.

We can start out by printing warnings when we fail to
request the corresponding GPIO for an interrupt line
though, it will be annoying enough and a kind of
compromise.

Or it has to be the GPIO input hogs.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Strashko, Grygorii]

Hi Lars, Linus,

I've few comments regarding this patch and problem in general.

On 08/26/2013 05:07 PM, Lars Poeschel wrote:
> From: Linus Walleij 
>
> Currently the kernel is ambigously treating GPIOs and interrupts
> from a GPIO controller: GPIOs and interrupts are treated as
> orthogonal. This unfortunately makes it unclear how to actually
> retrieve and request a GPIO line or interrupt from a GPIO
> controller in the device tree probe path.
>
> In the non-DT probe path it is clear that the GPIO number has to
> be passed to the consuming device, and if it is to be used as
> an interrupt, the consumer shall performa a gpio_to_irq() mapping
> and request the resulting IRQ number.
>
> In the DT boot path consumers may have been given one or more
> interrupts from the interrupt-controller side of the GPIO chip
> in an abstract way, such that the driver is not aware of the
> fact that a GPIO chip is backing this interrupt, and the GPIO
> side of the same line is never requested with gpio_request().
> A typical case for this is ethernet chips which just take some
> interrupt line which may be a "hard" interrupt line (such as an
> external interrupt line from an SoC) or a cascaded interrupt
> connected to a GPIO line.
>
> This has the following undesired effects:
>
> - The GPIOlib subsystem is not aware that the line is in use
>and willingly lets some other consumer perform gpio_request()
>on it, leading to a complex resource conflict if it occurs.
>
> - The GPIO debugfs file claims this GPIO line is "free".
>
> - The line direction of the interrupt GPIO line is not
>explicitly set as input, even though it is obvious that such
>a line need to be set up in this way, often making the system
>depend on boot-on defaults for this kind of settings.
>
> To solve this dilemma, perform an interrupt consistency check
> when adding a GPIO chip: if the chip is both gpio-controller and
> interrupt-controller, walk all children of the device tree,
> check if these in turn reference the interrupt-controller, and
> if they do, loop over the interrupts used by that child and
> perform gpio_request() and gpio_direction_input() on these,
> making them unreachable from the GPIO side.
>
> The patch has been devised by Linus Walleij and Lars Poeschel.
>
> Changelog V3:
> - define of_gpiochip_reserve_irq_lines as empty if
>CONFIG_OF_IRQ is not defined. Walking the devicetree simply
>makes no sense in this case and caused a compile time error
>on SPARC.
> - exit of_gpiochip_reserve_irq_lines if no irq_domain can be
>found.
> - convert the irqspec to cpu byte order before invoking the
>irq_domains xlate function.
>
> Changelog V2:
> - To be able to parse custom interrupts properties from the
>device tree, get a reference to the drivers irq_domain
>and use the xlate function to parse the proptery and
>get the irq number. This is tested with
>#interrupt-cells = 1, 2, and 3 and multiple interrupts
>per property.
>
> Cc: devicet...@vger.kernel.org
> Cc: Javier Martinez Canillas 
> Cc: Enric Balletbo i Serra 
> Cc: Grant Likely 
> Cc: Jean-Christophe PLAGNIOL-VILLARD 
> Cc: Santosh Shilimkar 
> Cc: Kevin Hilman 
> Cc: Balaji T K 
> Cc: Tony Lindgren 
> Cc: Jon Hunter 
> Signed-off-by: Lars Poeschel 
> Signed-off-by: Linus Walleij 
> ---
>   drivers/gpio/gpiolib-of.c |  201 
> -
>   1 file changed, 200 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..d328c5d 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -10,7 +10,6 @@
>* the Free Software Foundation; either version 2 of the License, or
>* (at your option) any later version.
>*/
> -
>   #include 
>   #include 
>   #include 
> @@ -19,6 +18,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>
> @@ -126,6 +126,201 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
>   }
>   EXPORT_SYMBOL(of_gpio_simple_xlate);
>
> +#if defined(CONFIG_OF_IRQ)
> +/**
> + * of_gpio_scan_irq_lines() - internal function to recursively scan the 
> device
> + * tree and request or free the GPIOs that are to be used as IRQ lines
> + * @node:node to start the scanning at
> + * @gcn: device node of the GPIO chip
> + * @irq_domain:  the irq_domain for the GPIO chip
> + * @intsize: size of one single interrupt in the device tree for the GPIO
> + *   chip. It is the same as #interrupt-cells.
> + * @gc:  GPIO chip instantiated from same node
> + * @request: wheter the function should request(true) or free(false) the
> + *   irq lines
> + *
> + * This is a internal function that calls itself to recursively scan the 
> device
> + * tree. It scans for uses of the device_node gcn as an interrupt-controller.
> + * If it finds some, it requests the corresponding gpio lines that are to be
> + * used as irq lines and sets them as input.
> + *

RE: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Strashko, Grygorii]

Hi Lars, Linus,

I've few comments regarding this patch and problem in general.

On 08/26/2013 05:07 PM, Lars Poeschel wrote:
 From: Linus Walleij linus.wall...@linaro.org

 Currently the kernel is ambigously treating GPIOs and interrupts
 from a GPIO controller: GPIOs and interrupts are treated as
 orthogonal. This unfortunately makes it unclear how to actually
 retrieve and request a GPIO line or interrupt from a GPIO
 controller in the device tree probe path.

 In the non-DT probe path it is clear that the GPIO number has to
 be passed to the consuming device, and if it is to be used as
 an interrupt, the consumer shall performa a gpio_to_irq() mapping
 and request the resulting IRQ number.

 In the DT boot path consumers may have been given one or more
 interrupts from the interrupt-controller side of the GPIO chip
 in an abstract way, such that the driver is not aware of the
 fact that a GPIO chip is backing this interrupt, and the GPIO
 side of the same line is never requested with gpio_request().
 A typical case for this is ethernet chips which just take some
 interrupt line which may be a hard interrupt line (such as an
 external interrupt line from an SoC) or a cascaded interrupt
 connected to a GPIO line.

 This has the following undesired effects:

 - The GPIOlib subsystem is not aware that the line is in use
and willingly lets some other consumer perform gpio_request()
on it, leading to a complex resource conflict if it occurs.

 - The GPIO debugfs file claims this GPIO line is free.

 - The line direction of the interrupt GPIO line is not
explicitly set as input, even though it is obvious that such
a line need to be set up in this way, often making the system
depend on boot-on defaults for this kind of settings.

 To solve this dilemma, perform an interrupt consistency check
 when adding a GPIO chip: if the chip is both gpio-controller and
 interrupt-controller, walk all children of the device tree,
 check if these in turn reference the interrupt-controller, and
 if they do, loop over the interrupts used by that child and
 perform gpio_request() and gpio_direction_input() on these,
 making them unreachable from the GPIO side.

 The patch has been devised by Linus Walleij and Lars Poeschel.

 Changelog V3:
 - define of_gpiochip_reserve_irq_lines as empty if
CONFIG_OF_IRQ is not defined. Walking the devicetree simply
makes no sense in this case and caused a compile time error
on SPARC.
 - exit of_gpiochip_reserve_irq_lines if no irq_domain can be
found.
 - convert the irqspec to cpu byte order before invoking the
irq_domains xlate function.

 Changelog V2:
 - To be able to parse custom interrupts properties from the
device tree, get a reference to the drivers irq_domain
and use the xlate function to parse the proptery and
get the irq number. This is tested with
#interrupt-cells = 1, 2, and 3 and multiple interrupts
per property.

 Cc: devicet...@vger.kernel.org
 Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
 Cc: Enric Balletbo i Serra eballe...@gmail.com
 Cc: Grant Likely grant.lik...@linaro.org
 Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com
 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Cc: Kevin Hilman khil...@linaro.org
 Cc: Balaji T K balaj...@ti.com
 Cc: Tony Lindgren t...@atomide.com
 Cc: Jon Hunter jgchun...@gmail.com
 Signed-off-by: Lars Poeschel poesc...@lemonage.de
 Signed-off-by: Linus Walleij linus.wall...@linaro.org
 ---
   drivers/gpio/gpiolib-of.c |  201 
 -
   1 file changed, 200 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
 index 665f953..d328c5d 100644
 --- a/drivers/gpio/gpiolib-of.c
 +++ b/drivers/gpio/gpiolib-of.c
 @@ -10,7 +10,6 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
 -
   #include linux/device.h
   #include linux/errno.h
   #include linux/module.h
 @@ -19,6 +18,7 @@
   #include linux/of.h
   #include linux/of_address.h
   #include linux/of_gpio.h
 +#include linux/of_irq.h
   #include linux/pinctrl/pinctrl.h
   #include linux/slab.h

 @@ -126,6 +126,201 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
   }
   EXPORT_SYMBOL(of_gpio_simple_xlate);

 +#if defined(CONFIG_OF_IRQ)
 +/**
 + * of_gpio_scan_irq_lines() - internal function to recursively scan the 
 device
 + * tree and request or free the GPIOs that are to be used as IRQ lines
 + * @node:node to start the scanning at
 + * @gcn: device node of the GPIO chip
 + * @irq_domain:  the irq_domain for the GPIO chip
 + * @intsize: size of one single interrupt in the device tree for the GPIO
 + *   chip. It is the same as #interrupt-cells.
 + * @gc:  GPIO chip instantiated from same node
 + * @request: wheter the function should request(true) or free(false) the
 + *   irq lines
 + *
 + * This is a internal function 

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren swar...@wwwdotorg.org wrote:
 On 08/26/2013 08:07 AM, Lars Poeschel wrote:

 Currently the kernel is ambigously treating GPIOs and interrupts
 from a GPIO controller: GPIOs and interrupts are treated as
 orthogonal. This unfortunately makes it unclear how to actually
 retrieve and request a GPIO line or interrupt from a GPIO
 controller in the device tree probe path.

 I still think that this patch is the wrong approach. Instead, the logic
 should be hooked into gpio_request() and request_irq(). This patch only
 addresses DT, and ignores anything else, hence doesn't seem like the
 right level of abstraction to plug in, since the issue is not related to DT.

We tried to do it that way, and it exploded. See commit
b4419e1a15905191661ffe75ba2f9e649f5d565e
gpio/omap: auto request GPIO as input if used as IRQ via DT

Here request_irq() augmented through its irqdomain to
issue gpio_request_one().

Why was this patch reverted? It seems this is what has not
managed to reach the audience here.

It turns out some drivers were already doing this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

Looks perfectly valid right?

Not so: after the above change, we tried to request the
GPIO a *second time* in the GPIO driver's irq map function,
and of course it failed, as it was already taken.

So saying that it should be done in the request_irq()
function is imposing this other semantic on the kernel
instead: you must *NOT* request the GPIO with
request_gpio() if you're going to use it as an IRQ.

(Also, it force us to implement the same code in each
and every driver, but that is a lesser problem.)

I don't quite understand what is so hard around this.
We cannot get away from restricting the semantics in
some way, if gpio-controllers shall also be interrupt-controllers,
the current patch is the least intrusive I've seen so far.

And I don't even think this is a Linux problem, handing
out the same resource with two hands is ambigous and is
going to cause problems with any operating system.
It is better to restrict this and say in the binding doc that
the gpio line cannot be gpio n assigned when there
is an interrupt on the same line.

We can start out by printing warnings when we fail to
request the corresponding GPIO for an interrupt line
though, it will be annoying enough and a kind of
compromise.

Or it has to be the GPIO input hogs.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 08/29/2013 09:26 PM, Linus Walleij wrote:
 On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren swar...@wwwdotorg.org 
 wrote:
 On 08/26/2013 08:07 AM, Lars Poeschel wrote:

 Currently the kernel is ambigously treating GPIOs and interrupts
 from a GPIO controller: GPIOs and interrupts are treated as
 orthogonal. This unfortunately makes it unclear how to actually
 retrieve and request a GPIO line or interrupt from a GPIO
 controller in the device tree probe path.

 I still think that this patch is the wrong approach. Instead, the logic
 should be hooked into gpio_request() and request_irq(). This patch only
 addresses DT, and ignores anything else, hence doesn't seem like the
 right level of abstraction to plug in, since the issue is not related to DT.
 
 We tried to do it that way, and it exploded. See commit
 b4419e1a15905191661ffe75ba2f9e649f5d565e
 gpio/omap: auto request GPIO as input if used as IRQ via DT
 
 Here request_irq() augmented through its irqdomain to
 issue gpio_request_one().
 
 Why was this patch reverted? It seems this is what has not
 managed to reach the audience here.


The mentioned patch also broke some ancient platforms that have not been (and
probably never will) migrated to DT like OMAP1.

So, after trying to add this logic by using a custom irq domain .map function
handler for the OMAP GPIO driver I agree with Linus that is better to provide a
solution at the Device Tree level to not affect platforms that are still using
legacy boot.

Legacy boot don't need a fix since it does not have this issue. Platform code
just request the GPIO and do the setup (gpio_direction_input) before the drivers
call to request_irq().

 It turns out some drivers were already doing this:
 
 request_gpio(gpio);
 gpio_direction_input(gpio);
 request_irq(gpio_to_irq(gpio));

 Looks perfectly valid right?

 Not so: after the above change, we tried to request the
 GPIO a *second time* in the GPIO driver's irq map function,
 and of course it failed, as it was already taken.

 So saying that it should be done in the request_irq()
 function is imposing this other semantic on the kernel
 instead: you must *NOT* request the GPIO with
 request_gpio() if you're going to use it as an IRQ.
 
 (Also, it force us to implement the same code in each
 and every driver, but that is a lesser problem.)
 
 I don't quite understand what is so hard around this.
 We cannot get away from restricting the semantics in
 some way, if gpio-controllers shall also be interrupt-controllers,
 the current patch is the least intrusive I've seen so far.
 

We have been trying to solve this issue for a few months by now and Linus'
approach seems to be the most sensible solution to me.

Drivers that request an IRQ and assume that platform code will request and setup
the GPIO have been broken since the boards using these drivers were migrated to
DT (e.g: smsc911x on OMAP2+ boards). I know is really hard to agree on the
better approach to solve this but it would be great if we can find a solution
and fix these broken platforms.

 And I don't even think this is a Linux problem, handing
 out the same resource with two hands is ambigous and is
 going to cause problems with any operating system.
 It is better to restrict this and say in the binding doc that
 the gpio line cannot be gpio n assigned when there
 is an interrupt on the same line.


Indeed, if a driver wants to manually request a GPIO to be used as an IRQ, then
the DT binding for that driver should not use a gpio as interrupt-parent and
should leave the driver to handle this manually.

On the other hand, if a driver just calls request_irq() and it does not know if
the IRQ is a real interrupt line from an interrupt controller or a GPIO line
acting as an IRQ, then the DT binding should just have a required
interrupt-parent property to define the phandle for the interrupt controller
used which could or could not be a GPIO controller.

 We can start out by printing warnings when we fail to
 request the corresponding GPIO for an interrupt line
 though, it will be annoying enough and a kind of
 compromise.


If the GPIO pin is first requested because it is described to be an IRQ in a DT
and then a driver tries to request the same GPIO pin, then there is a bug in
either the DT or the driver IMHO. The same assumption is made with platform code
right now when using legacy boot, I don't understand why is different for the DT
case.

 Or it has to be the GPIO input hogs.
 
 Yours,
 Linus Walleij
 

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/