Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
> Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
> > Hi Heiner,
> > 
> > On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
> > > Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
> > > > On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
> > > > > Add support for GPIO interrupts on Amlogic Meson SoC's.
> > > > > 
> > > > > There's a limit of 8 parent interupts which can be used in total.
> > > > > Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
> > > > > one for each edge.
> > > > > 
> > > > > Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > > > > ---
> > > > > v2:
> > > > > - make the GPIO IRQ controller a separate driver
> > > > > - several smaller improvements
> > > > > ---
> > > > >  drivers/pinctrl/Kconfig                   |   1 +
> > > > >  drivers/pinctrl/meson/Makefile            |   2 +-
> > > > >  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
> > > > > ++++++++++++++++++++++++++++++
> > > > 
> > > > Hi Heiner,
> > > > 
> > > > This code has nothing to do with GPIOs on pinmux handling here, what we
> > > > figured with Jerome
> > > > is that this must be an independent IRQ Controller in drivers/irqchip.
> > > > 
> > > 
> > > I'm not convinced and would like to hear more opinions on that. I see it
> > > like this:
> > > The driver implements an irqchip, right. But it's not an interrupt
> > > controller.
> > > Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain
> > > (one for ao
> > > and one for periphs GPIO domain). Therefore the gpio-controller now also
> > > acts as
> > > interrupt-controller. And both gpio (and interrupt) controllers just use
> > > the irqchip
> > > exposed by the new driver.
> > > Last but not least the irqchip can be used with GPIOs only.
> > 
> > In fact it's an interrupt controller since it can mask/unmask and control a
> > singal that
> > can wake up an interrupt for the ARM Core.
> > 
> > Please look at the STM32 EXTI code, the design is quite similar except they
> > don't have a
> > dynamic management of links, but fixed ones.
> > They have a proper independant IRQCHIP driver and a link from the pinctrl
> > driver, and this
> > should be the right design.
> > They have a flaw since they do the mapping from the gpio_to_irq, and Linus
> > won't allow
> > this anymore.
> > 
> 
> At first I involve Rob as he also provided feedback regarding the DT part.
> 
> I had a look at the STM32 EXTI code and it looks very similar to Jerome's
> version.
> Actually I'd assume that the first Meson driver attempt was modeled after
> STM32 EXTI.
Well, no. EXTI has been merged when I already submitted the RFC for this driver,
but that's not the point I suppose.

> 
> As you just mentioned there are at least two issues:
> 1. The mapping can be requested via gpio_to_irq but it doesn't have to. A
> driver could
Maybe you missed it in our previous discussion with Linus but, no you can't
create mapping in gpio_to_irq. This callback is fast and creating mappings might
sleep because of the irq_domain mutex

https://patchwork.ozlabs.org/patch/684208/

>    also request a GPIO IRQ via DT.
> 2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation
> of two
>    parent interrupts.
> 
> When looking at the drivers under drivers/irqchip basically all of them
> implement not
> only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to
> be
> the main criteria whether something should be considered an interrupt
> controller
> (please correct me if this understanding is wrong).
> 
> The STM32 EXTI drivers seems to me to be unnecessarily complex due to not
> using
> GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of
> the
> GPIO IRQ complexity.
I can only speculate regarding the design choice of STM EXTI, but I suppose the
EXTI controller is independent of the pinmux/gpio subsystem HW wise. It's only
weakly linked to the gpios, in the way that you have (or can create) a "map"
between the gpio and the irq line number provided.

That is also the case for the amlogic gpio-irq. I suppose that's why Neil
suggested to look at EXTI. That's also why I commented that this should be first
implemented as an independent controller of the gpio subsys  (so in
drivers/irqchip)

You may find it more complex, which is arguable, but it is a more accurate
representation of the HW.

> 
> Coming back to my version:
> The new driver just implements an irqchip, no IRQ domain.
>  This irqchip then is
> provided to the IRQ domains bound to the respective gpio controllers (for AO
> and PERIPHS
> GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for
> free.
> 
> Adding the interrupt-controller property to the gpio-controller nodes is one
> thing
> still missing in my patch series. Then a driver can request a GPIO IRQ also
> via DT, e.g.:
> 
> interrupt-parent = <&gpio>;
> interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;
> 
> interrupt-parent = <&gpio_ao>;
> interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;
> 

Which is also possible in the series, where the controller is gpio-irq device
itself. This is what the HW provide, so it should be possible. It would be nice
to have your way working as well, which I think is possible (PSB)

> Advantage of having an IRQ domain per GPIO controller is being able to to use
> the GPIO
> defines as is. Or in other words:
> GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one
> IRQ domain
> per GPIO controller (both using the same new irqchip).
> 

I think you are onto something. As I told you previously, the problem was to
create the mappings in pinctrl w/o allocating the parent. I struggled with that
back in November and I had no time to really get back to it since.

Creating domains in each gpio controller, w/ all the mapping created at startup,
  stacked to the domain provided by the driver/irqchip would solve the problem.

We would just have to call find_mapping in gpio_to_irq, which is fine and
allocate the parent irq in the request_ressource callback.

> And all I had to do is implementing one irq_chip and changing one line in the
> existing
> pinctrl driver.
> Whilst I have to admit that e.g. calling request_irq for a parent irq from the
> irq_set_type
> callback may be something people find ugly and hacky.
> 
The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would
be nice to find a way to support it anyway. There two possibilities for it:
* Change the change the edge type depending on the next expected edge: Marc
already stated that he is firmly against that. It is indeed not very robust
* Allocate and deallocate additional parent irq to get secondary irq in
set_type: This indeed looks hacky, I'd like to get the view of irq maintainers
on this.


> Compared to this implementation the STM32 EXTI drivers needs significantly
> more data
> structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical
> IRQ domain
> handling + further irqchip and irq domain + irq domain ops in the pinctrl
> driver.

Within reasonable limits, having more or less data, code lines does not make a
driver better or worse. 

> 
> Last but not least coming back to the initial talk with Rob about where to
> best place the
> DT docu for this irqchip implementation:
> If acceptable I'd prefer to do it like .e.g in
> bindings/pinctrl/allwinner,sunxi-pinctrl.txt
> or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
> There the interrupt-controller properties are documented as part of the
> pinctrl driver and
> not separately under bindings/interrupt-controller.
> 
> Maybe this explains a little bit better why I chose this approach.
> 
> Rgds, Heiner
> 
> > > 
> > > In the irqchip implementation we need the SoC-specific mapping from GPIO
> > > number
> > > to internal GPIO IRQ number. Having to export this from
> > > drivers/pinctrl/meson for use
> > > under drivers/irqchip most likely would also cause objections.
> > 
> > You won't need, this interrupt controller will take the number either from
> > DT either
> > from a mapping creating from the pinctrl driver. The link will only be
> > through the
> > irq subsystem.
> > 
> > > 
> > > So far my impression is that the very specific way GPIO IRQ's are handled
> > > on Meson
> > > doesn't fit perfectly into the current IRQ subsystem. Therefore the
> > > discussion
> > > about Jerome's version didn't result in the IRQ maintainers stating: do it
> > > this way ..
> > > Having said that most likely every possible approach is going to raise
> > > some concerns.
> > 
> > It doesn't fit exactly, but the subsystem can certainly be used to achieve
> > it either by
> > using all it's capacity or by eventually discussing with the maintainers to
> > adapt it.
> > 
> > Jerome has some hints hot to achieve the pinctrl part with everyone "happy".
> > 
> > > 
> > > > Please move it and make independent, you should be able to request irqs
> > > > without any links
> > > > to the pinmux/gpio since physically the GPIO lines input are always
> > > > connected to this
> > > > irq controller, and the pinmux has no impact on the interrupt management
> > > > here.
> > > > > From the GPIO-IRQ Controller perspective, the GPIOs are only a number
> > > > > and the pinmux code
> > > > 
> > > > is only here to make the translation to a specific GPIO to a GPIO-IRQ
> > > > number.
> > > > 
> > > > For more chance to have it upstreamed in the right way, we should :
> > > > 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype,
> > > > forums, ...
> > > > 2) Push an independent IRQ controller that matches the capacity of the
> > > > HW
> > > > 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping
> > > > done in the right way
> > > > 
> > > > Jerome spent quite a lot of time and had a chat with the IRQ subsystem
> > > > maintainers to have s
> > > > clear image of how this should be implemented, and it would be a good
> > > > point to actually
> > > > have a chat with them to elaborate find a strong solution.
> > > > 
> > > 
> > > I know and I really appreciate Jerome's work and his discussion with the
> > > IRQ maintainers.
> > > My current attempt was inspired by his work.
> > > However the discussion last year ended w/o result and the topic of GPIO
> > > IRQs has been dead
> > > since then. And I think discussing approaches works best based on a
> > > concrete piece of code.
> > > Therefore I submitted my version as discussion basis. I didn't expect that
> > > everybody would
> > > be totally happy with it and it would go to mainline unchanged.
> > 
> > Sure, thanks for the work,
> > 
> > > 
> > > > I'm sorry to say that pushing this code without really understanding how
> > > > and why will lead to
> > > > nothing expect frustration from everybody.
> > > > 
> > > 
> > > I can only speak for myself: I'm not frustrated and I can live with
> > > critical review comments.
> > 
> > Great ! Anyway thanks for your work and I hope this will lead to mainline !
> > 
> > > Regards, Heiner
> > > 
> > > > Neil
> > 
> > Neil
> > 
> > [...]
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html