[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Neil Armstrong]

> From: Carlo Caione <carlo@endlessm.com>
> 
> In Meson SoCs we have 8 independent GPIO interrupts that can be programmed to
> use any of the GPIOs in the chip as interrupt source.
> 
> These GPIOs are managed by GIC but they can be conditioned (and enabled) by
> some registers external to the GIC.
> 
> GPIOs |--[mux1 or mux2]--[polarity]--[filter]--[edge_select]--> GIC
> 
> The original work has been done by Beniamino. I cleaned it up, fixed a couple
> of bugs, added support for Meson8b and the fix for the .to_irq hook.

Hi Carlo, Linus,

I have another implementation idea about this subject, by using static GPIO-irq
association in DT instead of using a very complex dynamic allocation.

The idea is to add a simple property :
irqs-gpios = <>

To map a GPIO to the irqs, then we can either use the DT irq mapping or use the
gpiolib to_irq() if the gpio is in the table.

The relative drawback is that we will need DT changes to enable routing of a gpio
to an IRQ, but the complete system is based on a DT description anyway.

In this case, we could use gpiochip_irqchip.

Do you think this structure would be acceptable ?

Neil

> Beniamino Galvani (2):
>   pinctrl: meson: Enable GPIO IRQs
>   pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support
> 
>  .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |  12 +
>  drivers/pinctrl/Kconfig                            |   1 +
>  drivers/pinctrl/meson/pinctrl-meson.c              | 254 +++++++++++++++++++++
>  drivers/pinctrl/meson/pinctrl-meson.h              |  18 +-
>  drivers/pinctrl/meson/pinctrl-meson8.c             |  36 +--
>  drivers/pinctrl/meson/pinctrl-meson8b.c            |  36 +--
>  6 files changed, 326 insertions(+), 31 deletions(-)
> 
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Linus Walleij]

On Tue, Jun 28, 2016 at 10:06 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:

> I have another implementation idea about this subject, by using static GPIO-irq
> association in DT instead of using a very complex dynamic allocation.
>
> The idea is to add a simple property :
> irqs-gpios = <>
>
> To map a GPIO to the irqs, then we can either use the DT irq mapping or use the
> gpiolib to_irq() if the gpio is in the table.
>
> The relative drawback is that we will need DT changes to enable routing of a gpio
> to an IRQ, but the complete system is based on a DT description anyway.
>
> In this case, we could use gpiochip_irqchip.
>
> Do you think this structure would be acceptable ?

That depends on:

- A nod from the DT maintainers that this is acceptable from a HW description
  point of view and a simplification that probably all other OS:es
will also want
  to do.

- Rough consensus from the maintainess of the platform that this makes
  sense, do noone appear three months down the road and says "oh wait I
  have this usecase that require us to do this dynamically".

I think the dynamic solution is always more elegant, computers should resolve
complex problems, doing it in DT makes a human do a machine's work which
is as a rule of thumb not a good idea. But there is also the question
of maintenance
cost and what makes sense at a human level so I'm not totally
inflexible on this.

Yours,
Linus Walleij

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Linus Walleij]

On Tue, Jun 28, 2016 at 10:06 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:

> I have another implementation idea about this subject, by using static GPIO-irq
> association in DT instead of using a very complex dynamic allocation.
>
> The idea is to add a simple property :
> irqs-gpios = <>
>
> To map a GPIO to the irqs, then we can either use the DT irq mapping or use the
> gpiolib to_irq() if the gpio is in the table.
>
> The relative drawback is that we will need DT changes to enable routing of a gpio
> to an IRQ, but the complete system is based on a DT description anyway.
>
> In this case, we could use gpiochip_irqchip.
>
> Do you think this structure would be acceptable ?

That depends on:

- A nod from the DT maintainers that this is acceptable from a HW description
  point of view and a simplification that probably all other OS:es
will also want
  to do.

- Rough consensus from the maintainess of the platform that this makes
  sense, do noone appear three months down the road and says "oh wait I
  have this usecase that require us to do this dynamically".

I think the dynamic solution is always more elegant, computers should resolve
complex problems, doing it in DT makes a human do a machine's work which
is as a rule of thumb not a good idea. But there is also the question
of maintenance
cost and what makes sense at a human level so I'm not totally
inflexible on this.

Yours,
Linus Walleij

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Carlo Caione]

On 29/06/16 10:53, Linus Walleij wrote:
> On Tue, Jun 28, 2016 at 10:06 AM, Neil Armstrong
> <narmstrong@baylibre.com> wrote:
> 
> > I have another implementation idea about this subject, by using static GPIO-irq
> > association in DT instead of using a very complex dynamic allocation.
> >
> > The idea is to add a simple property :
> > irqs-gpios = <>
> >
> > To map a GPIO to the irqs, then we can either use the DT irq mapping or use the
> > gpiolib to_irq() if the gpio is in the table.
> >
> > The relative drawback is that we will need DT changes to enable routing of a gpio
> > to an IRQ, but the complete system is based on a DT description anyway.
> >
> > In this case, we could use gpiochip_irqchip.
> >
> > Do you think this structure would be acceptable ?

I put some thoughts on this.

Having to statically define the IRQ-GPIO relation in the DT is not the
most elegant solution but this is a really stupid controller and the
whole cascade IRQ controller story revealed to be really a PITA with
this hw when I was trying to write the driver. 

On a side note the driver was working fine but the real problem was on
some misunderstanding on the gpiolib .to_irq hook. My bad in having lost
interest in fixing / clarifying that mess, sorry for that.

The latest respin was https://patchwork.kernel.org/patch/7738781/. Neil,
probably you want also to review that (also privately since after 1 year
I guess it needs to be updated) before someone starts writing the code
for the DT-only solution?

> That depends on:
> 
> - A nod from the DT maintainers that this is acceptable from a HW description
>   point of view and a simplification that probably all other OS:es
> will also want
>   to do.

hardware wise this makes sense at board level and to the best of my
knowledge we are the only one OS using this DT.

> - Rough consensus from the maintainess of the platform that this makes
>   sense, do noone appear three months down the road and says "oh wait I
>   have this usecase that require us to do this dynamically".

this is the real problem I see especially in case someone comes up with
some homemade driver for some expansion board hooked up on the headers.

> I think the dynamic solution is always more elegant, computers should resolve
> complex problems, doing it in DT makes a human do a machine's work which
> is as a rule of thumb not a good idea. But there is also the question
> of maintenance
> cost and what makes sense at a human level so I'm not totally
> inflexible on this.

Totally agree on this.

-- 
Carlo Caione

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Carlo Caione]

On 29/06/16 10:53, Linus Walleij wrote:
> On Tue, Jun 28, 2016 at 10:06 AM, Neil Armstrong
> <narmstrong@baylibre.com> wrote:
> 
> > I have another implementation idea about this subject, by using static GPIO-irq
> > association in DT instead of using a very complex dynamic allocation.
> >
> > The idea is to add a simple property :
> > irqs-gpios = <>
> >
> > To map a GPIO to the irqs, then we can either use the DT irq mapping or use the
> > gpiolib to_irq() if the gpio is in the table.
> >
> > The relative drawback is that we will need DT changes to enable routing of a gpio
> > to an IRQ, but the complete system is based on a DT description anyway.
> >
> > In this case, we could use gpiochip_irqchip.
> >
> > Do you think this structure would be acceptable ?

I put some thoughts on this.

Having to statically define the IRQ-GPIO relation in the DT is not the
most elegant solution but this is a really stupid controller and the
whole cascade IRQ controller story revealed to be really a PITA with
this hw when I was trying to write the driver. 

On a side note the driver was working fine but the real problem was on
some misunderstanding on the gpiolib .to_irq hook. My bad in having lost
interest in fixing / clarifying that mess, sorry for that.

The latest respin was https://patchwork.kernel.org/patch/7738781/. Neil,
probably you want also to review that (also privately since after 1 year
I guess it needs to be updated) before someone starts writing the code
for the DT-only solution?

> That depends on:
> 
> - A nod from the DT maintainers that this is acceptable from a HW description
>   point of view and a simplification that probably all other OS:es
> will also want
>   to do.

hardware wise this makes sense at board level and to the best of my
knowledge we are the only one OS using this DT.

> - Rough consensus from the maintainess of the platform that this makes
>   sense, do noone appear three months down the road and says "oh wait I
>   have this usecase that require us to do this dynamically".

this is the real problem I see especially in case someone comes up with
some homemade driver for some expansion board hooked up on the headers.

> I think the dynamic solution is always more elegant, computers should resolve
> complex problems, doing it in DT makes a human do a machine's work which
> is as a rule of thumb not a good idea. But there is also the question
> of maintenance
> cost and what makes sense at a human level so I'm not totally
> inflexible on this.

Totally agree on this.

-- 
Carlo Caione

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Daniel Drake]

On Wed, Jun 29, 2016 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> I think the dynamic solution is always more elegant, computers should resolve
> complex problems, doing it in DT makes a human do a machine's work which
> is as a rule of thumb not a good idea. But there is also the question
> of maintenance
> cost and what makes sense at a human level so I'm not totally
> inflexible on this.

On this hardware we only have 8 GPIO IRQs available, for 100+ GPIOs.
If you want to monitor for both rising and falling edge of a GPIO then
you have to use 2 GPIO IRQs. 2 are used for the common case of SD card
detect. You can quickly lose several more (2 for audio jack detection,
power button, etc). So these are a very limited resource.

If the allocation is done dynamically I see 2 slight advantages:

 1. If the number of GPIO-IRQ users is greater than the number
available, the distributor can exclude drivers that are not of
interest to him and the corresponding GPIO-IRQs will automatically and
dynamically become available for the drivers that are of interest.

 2. If the device tree encodes these assignments then the management
could be a bit complex, because some will be defined in the SoC dtsi
files and others will be defined in the board dts files (e.g.
Endless's CVBS mode selection switch), we'll have to make sure that
there are not conflicting assignments. Similarly if we end up sharing
dtsi files over different SoC versions then things could get tricky
especially in such a small namespace of 8 GPIO-IRQs. In the dynamic
case this is not an issue.

Daniel

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Daniel Drake]

On Wed, Jun 29, 2016 at 2:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> I think the dynamic solution is always more elegant, computers should resolve
> complex problems, doing it in DT makes a human do a machine's work which
> is as a rule of thumb not a good idea. But there is also the question
> of maintenance
> cost and what makes sense at a human level so I'm not totally
> inflexible on this.

On this hardware we only have 8 GPIO IRQs available, for 100+ GPIOs.
If you want to monitor for both rising and falling edge of a GPIO then
you have to use 2 GPIO IRQs. 2 are used for the common case of SD card
detect. You can quickly lose several more (2 for audio jack detection,
power button, etc). So these are a very limited resource.

If the allocation is done dynamically I see 2 slight advantages:

 1. If the number of GPIO-IRQ users is greater than the number
available, the distributor can exclude drivers that are not of
interest to him and the corresponding GPIO-IRQs will automatically and
dynamically become available for the drivers that are of interest.

 2. If the device tree encodes these assignments then the management
could be a bit complex, because some will be defined in the SoC dtsi
files and others will be defined in the board dts files (e.g.
Endless's CVBS mode selection switch), we'll have to make sure that
there are not conflicting assignments. Similarly if we end up sharing
dtsi files over different SoC versions then things could get tricky
especially in such a small namespace of 8 GPIO-IRQs. In the dynamic
case this is not an issue.

Daniel

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Linus Walleij]

On Wed, Jun 29, 2016 at 4:41 PM, Daniel Drake <drake@endlessm.com> wrote:

> On this hardware we only have 8 GPIO IRQs available, for 100+ GPIOs.
> If you want to monitor for both rising and falling edge of a GPIO then
> you have to use 2 GPIO IRQs. 2 are used for the common case of SD card
> detect. You can quickly lose several more (2 for audio jack detection,
> power button, etc). So these are a very limited resource.
>
> If the allocation is done dynamically I see 2 slight advantages:
>
>  1. If the number of GPIO-IRQ users is greater than the number
> available, the distributor can exclude drivers that are not of
> interest to him and the corresponding GPIO-IRQs will automatically and
> dynamically become available for the drivers that are of interest.
>
>  2. If the device tree encodes these assignments then the management
> could be a bit complex, because some will be defined in the SoC dtsi
> files and others will be defined in the board dts files (e.g.
> Endless's CVBS mode selection switch), we'll have to make sure that
> there are not conflicting assignments. Similarly if we end up sharing
> dtsi files over different SoC versions then things could get tricky
> especially in such a small namespace of 8 GPIO-IRQs. In the dynamic
> case this is not an issue.

I think I'm siding with Daniel's analysis.

These IRQs should be dynamically assigned, if practically feasible.

BTW isn't the hierarchical irqdomain invented for exactly this
usecase?

Yours,
Linus Walleij

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Linus Walleij]

On Wed, Jun 29, 2016 at 4:41 PM, Daniel Drake <drake@endlessm.com> wrote:

> On this hardware we only have 8 GPIO IRQs available, for 100+ GPIOs.
> If you want to monitor for both rising and falling edge of a GPIO then
> you have to use 2 GPIO IRQs. 2 are used for the common case of SD card
> detect. You can quickly lose several more (2 for audio jack detection,
> power button, etc). So these are a very limited resource.
>
> If the allocation is done dynamically I see 2 slight advantages:
>
>  1. If the number of GPIO-IRQ users is greater than the number
> available, the distributor can exclude drivers that are not of
> interest to him and the corresponding GPIO-IRQs will automatically and
> dynamically become available for the drivers that are of interest.
>
>  2. If the device tree encodes these assignments then the management
> could be a bit complex, because some will be defined in the SoC dtsi
> files and others will be defined in the board dts files (e.g.
> Endless's CVBS mode selection switch), we'll have to make sure that
> there are not conflicting assignments. Similarly if we end up sharing
> dtsi files over different SoC versions then things could get tricky
> especially in such a small namespace of 8 GPIO-IRQs. In the dynamic
> case this is not an issue.

I think I'm siding with Daniel's analysis.

These IRQs should be dynamically assigned, if practically feasible.

BTW isn't the hierarchical irqdomain invented for exactly this
usecase?

Yours,
Linus Walleij

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

In Meson SoCs we have 8 independent GPIO interrupts that can be programmed to
use any of the GPIOs in the chip as interrupt source.

These GPIOs are managed by GIC but they can be conditioned (and enabled) by
some registers external to the GIC.

GPIOs |--[mux1 or mux2]--[polarity]--[filter]--[edge_select]--> GIC

The original work has been done by Beniamino. I cleaned it up, fixed a couple
of bugs, added support for Meson8b and the fix for the .to_irq hook.

Beniamino Galvani (2):
  pinctrl: meson: Enable GPIO IRQs
  pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |  12 +
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c              | 254 +++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h              |  18 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  36 +--
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  36 +--
 6 files changed, 326 insertions(+), 31 deletions(-)

-- 
1.9.1