Am 17.05.2017 um 01:16 schrieb Jerome Brunet: > 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/ > My comment was misunderstandable. What I meant was that a driver doesn't have to use gpio_to_irq to request an interrupt, this can also be done via DT. >> 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. > In request_resource we don't know the irq type yet (and therefore whether we need one or wo parent interrupts). IMHO we can't get completely rid of allocating parents in set_type. >> 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. > Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed e.g. by the SD card detection (drivers/mmc/slot-gpio.c). > >> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
