* [RFC] gpio: about the need to manage irq mapping dynamically. @ 2017-06-15 16:20 Jerome Brunet 2017-06-20 8:39 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Jerome Brunet @ 2017-06-15 16:20 UTC (permalink / raw) To: Linus Walleij, linux-gpio, Marc Zyngier Cc: open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel Hi Linus, I would to (re)start the discussion around the management of the irq_mapping in the gpio framework: Many gpio controllers are able to provide a working irq to all the gpios at the same time. On those controller we can create all the irq mapping during the probe of the controller. Things are easy. Then, there is another type of controller, with constraints that make it impossible to provide a working irq to all pins at the same it. On Amlogic SoC for example, we have only 8 parent irqs with and 8 to many (the number of pins) router. Ressource has be to managed in a more "dynamic" fashion. To handle this we tried: - [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that this is not allowed as gpio_to_irq is callable in irq context, therefore it should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq handlers. I would also add that "gpio_to_irq" has no "free" counter part, so this approach leaks mappings. The fact that 26 gpio drivers [3] create mapping in the gpio_to_irq callback shows that the problem of managing the irq mapping is not specific to Amlogic and that an evolution might be useful. - [1]: To create an empty domain to get a virq for each pins but delay the setup of the interrupt hierarchy to the "irq_request_resources" callback. Testing this approach led me to some nasty race conditions in the interrupt handlers. I discussed this solution with Marc and he told me that doing the setup of the interrupt hierarchy in the irqchip callbacks is "too late" . If I understood correctly (Marc feel to correct me if necessary), you should only get a virq when the full interrupt hierarchy is setup, from the triggering device to the CPU. With this last comment, I don't think there is a (clean) way ATM for a gpio driver to create the mapping "on demand". By "on-demand", I mean the consumer drivers doing this kind of thing: ret = request_irq(gpio_to_irq(GPIOX_Y), ... ) I would to like propose a way to fix that. It is not going to be a oneline fix, but I'm convinced we can do it w/o breaking anything: 1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare): Drivers will be free to implement those callbacks or not. Driver which can create all their mappings at probe time would obviously skip those. 2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This functions would do refcounting to handle shared irq and avoid wrongly disposing of used mappings. Then call the new drivers callbacks, if defined. A devm version of gpio_irq_prepare might be usefull 3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use the gpio_to_irq callback. I don't think this is going to be that hard to do, but it's going be to long and boring ... As long as a platform does not implement these new callbacks, all the above is basically a noop and should not change anything. I think this course of action would allow us to slowly fix those offending drivers [3], providing them with a solution. As this is not going to be a small task, I would like to get your view on it before going further. Best Regards Jerome [0]: https://patchwork.ozlabs.org/patch/684208/ [1]: http://lkml.kernel.org/r/5b352c8d-a426-fa73-58b7-0c935979492b@gmail.com [2]: Drivers using gpio_to_irq in irq_handlers * drivers/gpio/gpio-ep93xx.c * drivers/gpio/gpio-pxa.c * drivers/gpio/gpio-tegra.c [3]: Drivers creating mapping in the gpio_to_irq callback (either through irq_create_mapping, irq_create_fwspec_mapping or regmap_irq_get_virq) * arch/sh/boards/mach-x3proto/gpio.c * drivers/gpio/gpio-bcm-kona.c * drivers/gpio/gpio-da9052.c * drivers/gpio/gpio-da9055.c * drivers/gpio/gpio-wm831x.c * drivers/gpio/gpio-wm8994.c * drivers/pinctrl/nomadik/pinctrl-abx500.c * drivers/pinctrl/pinctrl-as3722.c * drivers/pinctrl/pinctrl-rockchip.c * drivers/pinctrl/samsung/pinctrl-samsung.c * drivers/pinctrl/stm32/pinctrl-stm32.c * arch/arm/plat-orion/gpio.c * drivers/gpio/gpio-davinci.c * drivers/gpio/gpio-em.c * drivers/gpio/gpio-grgpio.c * drivers/gpio/gpio-max77620.c * drivers/gpio/gpio-mpc8xxx.c * drivers/gpio/gpio-mvebu.c * drivers/gpio/gpio-palmas.c * drivers/gpio/gpio-tb10x.c * drivers/gpio/gpio-tps6586x.c * drivers/gpio/gpio-tz1090.c * drivers/gpio/gpio-xgene-sb.c * drivers/pinctrl/pinctrl-adi2.c * drivers/pinctrl/samsung/pinctrl-exynos5440.c ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-15 16:20 [RFC] gpio: about the need to manage irq mapping dynamically Jerome Brunet @ 2017-06-20 8:39 ` Linus Walleij 2017-06-20 10:26 ` Jerome Brunet 0 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2017-06-20 8:39 UTC (permalink / raw) To: Jerome Brunet Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Thu, Jun 15, 2017 at 6:20 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > To handle this we tried: > - [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that > this is not allowed as gpio_to_irq is callable in irq context, therefore it > should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq > handlers. This is not the problem. The problem is that gpio_to_irq() is entirely optional: it is not at all requires to have called gpio_to_irq() before using an IRQ from a GPIO line, as the interrupt chips and gpio chips are orthogonal. So if gpio_to_irq() is called, then a mapping needs to be retrieved, using an irqdomain or similar, and if one does not already exist, it should be mapped at this point. But actually doing the mapping/unmapping of IRQs and related resource management should be handled by the irqchip/irqdomain, not ever by gpio_to_irq(). And someone may just be requesting an IRQ from the irqchip without calling gpio_to_irq(). Anytime. And with devicetree this happens all of the time. > I would also add that "gpio_to_irq" has no "free" counter part, so this approach > leaks mappings. Exactly. > The fact that 26 gpio drivers [3] create mapping in the > gpio_to_irq callback shows that the problem of managing the irq mapping is not > specific to Amlogic and that an evolution might be useful. Calling irq_create_mapping() in gpio_to_irq() is not a problem if the mapping is free:ed elsewhere. But yeah, there are lots of old drivers with old and erroneous solutions to this problem. > - [1]: To create an empty domain to get a virq for each pins but delay the setup > of the interrupt hierarchy to the "irq_request_resources" callback. Testing this > approach led me to some nasty race conditions in the interrupt handlers. I > discussed this solution with Marc and he told me that doing the setup of the > interrupt hierarchy in the irqchip callbacks is "too late" . If I understood > correctly (Marc feel to correct me if necessary), you should only get a virq > when the full interrupt hierarchy is setup, from the triggering device to the > CPU. OK I am as always confused by "virq" which I guess means "virtual IRQ" which is confusing since all Linux IRQ numbers are in some sense virtual, and since this has nothing to do with virtualization, which is another area where IRQchips are very delicate. But I get it. > With this last comment, I don't think there is a (clean) way ATM for a gpio > driver to create the mapping "on demand". By "on-demand", I mean the consumer > drivers doing this kind of thing: > > ret = request_irq(gpio_to_irq(GPIOX_Y), ... ) > > I would to like propose a way to fix that. OK solutions are good.... > It is not going to be a oneline fix, > but I'm convinced we can do it w/o breaking anything: > > 1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare): Drivers > will be free to implement those callbacks or not. Driver which can create all > their mappings at probe time would obviously skip those. > > 2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This > functions would do refcounting to handle shared irq and avoid wrongly disposing > of used mappings. Then call the new drivers callbacks, if defined. A devm > version of gpio_irq_prepare might be usefull > > 3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use the > gpio_to_irq callback. I don't think this is going to be that hard to do, but > it's going be to long and boring ... So how are you intending to deal with mixed semantics when, as in the devicetree case, the GPIO driver is also flagged as an interrupt-controller and consumers can request IRQs from it with just platform_get_irq() and expect it to work? It seems this usecase drives a truck through that approach. The keyword is separation of concerns, the irqchip abstraction should deal with interrupts, we do not want the gpiochip to do half of the work. Here is an example from arch/arm/boot/dts/ste-snowball.dts: ethernet@0 { compatible = "smsc,lan9115"; reg = <0 0x10000>; interrupts = <12 IRQ_TYPE_EDGE_RISING>; interrupt-parent = <&gpio4>; (...) }; Notice: no GPIOs are taken by this driver, this ethernet driver does not know that its interrupt is supplied by a GPIO, in fact on many platforms this ethernet chip has a dedicated IRQ line so that should not be required, i.e. the ethernet chip does not need to know that a GPIO controller is supplying this interrupt, why should it? It is just some interrupt. The driver looks like so drivers/net/ethernet/smsc/smsc911x.c: irq = platform_get_irq(pdev, 0); And that is all it does. The above usecase is not uncommon. If this interrupt+GPIO controller was using your scheme, where would these new callbacks be called? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-20 8:39 ` Linus Walleij @ 2017-06-20 10:26 ` Jerome Brunet 2017-06-20 16:37 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Jerome Brunet @ 2017-06-20 10:26 UTC (permalink / raw) To: Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote: > On Thu, Jun 15, 2017 at 6:20 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > To handle this we tried: > > - [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that > > this is not allowed as gpio_to_irq is callable in irq context, therefore it > > should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq > > handlers. > > This is not the problem. The problem is that gpio_to_irq() is entirely > optional: it is not at all requires to have called gpio_to_irq() before using > an IRQ from a GPIO line, as the interrupt chips and gpio chips are > orthogonal. Agreed. no problem with this If you don't use gpio_to_irq, they are *completely* orthogonal. If you do use gpio_to_irq, you are creating this tiny relationship which makes all the difference, IMO. > > So if gpio_to_irq() is called, then a mapping needs to be retrieved, using > an irqdomain or similar, and if one does not already exist, it should be > mapped > at this point. > > But actually doing the mapping/unmapping of IRQs and related resource > management should be handled by the irqchip/irqdomain, not ever by > gpio_to_irq(). > > And someone may just be requesting an IRQ from the irqchip without > calling gpio_to_irq(). Anytime. And with devicetree this happens all of the > time. Yes, for sure. But in this case there no issue as the mapping creation is handled in device-tree code. PSB > > > I would also add that "gpio_to_irq" has no "free" counter part, so this > > approach > > leaks mappings. > > Exactly. > > > The fact that 26 gpio drivers [3] create mapping in the > > gpio_to_irq callback shows that the problem of managing the irq mapping is > > not > > specific to Amlogic and that an evolution might be useful. > > Calling irq_create_mapping() in gpio_to_irq() is not a problem if > the mapping is free:ed elsewhere. But yeah, there are lots of old > drivers with old and erroneous solutions to this problem. A lot of these drivers are old indeed, and dealing with all this history must be quite a challenge ;) However, some are fairly recent. > > > - [1]: To create an empty domain to get a virq for each pins but delay the > > setup > > of the interrupt hierarchy to the "irq_request_resources" callback. Testing > > this > > approach led me to some nasty race conditions in the interrupt handlers. I > > discussed this solution with Marc and he told me that doing the setup of the > > interrupt hierarchy in the irqchip callbacks is "too late" . If I understood > > correctly (Marc feel to correct me if necessary), you should only get a virq > > when the full interrupt hierarchy is setup, from the triggering device to > > the > > CPU. > > OK I am as always confused by "virq" which I guess means "virtual IRQ" > which is confusing since all Linux IRQ numbers are in some sense > virtual, and since this has nothing to do with virtualization, which is > another > area where IRQchips are very delicate. > Just using the usual irq wording (hwirq for the numbers specific to the controller, virq for the linux interrupt numbers) > But I get it. > > > With this last comment, I don't think there is a (clean) way ATM for a gpio > > driver to create the mapping "on demand". By "on-demand", I mean the > > consumer > > drivers doing this kind of thing: > > > > ret = request_irq(gpio_to_irq(GPIOX_Y), ... ) > > > > I would to like propose a way to fix that. > > OK solutions are good.... > > > It is not going to be a oneline fix, > > but I'm convinced we can do it w/o breaking anything: > > > > 1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare): > > Drivers > > will be free to implement those callbacks or not. Driver which can create > > all > > their mappings at probe time would obviously skip those. > > > > 2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This > > functions would do refcounting to handle shared irq and avoid wrongly > > disposing > > of used mappings. Then call the new drivers callbacks, if defined. A devm > > version of gpio_irq_prepare might be usefull > > > > 3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use > > the > > gpio_to_irq callback. I don't think this is going to be that hard to do, but > > it's going be to long and boring ... > > So how are you intending to deal with mixed semantics when, as in the > devicetree case, the GPIO driver is also flagged as an interrupt-controller > and consumers can request IRQs from it with just platform_get_irq() > and expect it to work? I don't think there any issue with this. PSB > > It seems this usecase drives a truck through that approach. > > The keyword is separation of concerns, the irqchip abstraction should > deal with interrupts, we do not want the gpiochip to do half of the work. > > Here is an example from arch/arm/boot/dts/ste-snowball.dts: > > ethernet@0 { > compatible = "smsc,lan9115"; > reg = <0 0x10000>; > interrupts = <12 IRQ_TYPE_EDGE_RISING>; > interrupt-parent = <&gpio4>; > (...) > }; > > Notice: no GPIOs are taken by this driver, this ethernet driver does not know > that its interrupt is supplied by a GPIO, in fact on many platforms > this ethernet > chip has a dedicated IRQ line so that should not be required, i.e. the > ethernet > chip does not need to know that a GPIO controller is supplying this interrupt, > why should it? It is just some interrupt. Agreed. > > The driver looks like so drivers/net/ethernet/smsc/smsc911x.c: > > irq = platform_get_irq(pdev, 0); > > And that is all it does. > > The above usecase is not uncommon. > 100% agreed, this is a common use case. In fact, I think driver should prefer doing so instead of using gpio_to_irq when possible ... but this another issue and we have to deal with what we have, right ? So there is 2 use cases: * What you described above: This is indeed completely orthogonal to gpio framework. It will be using the irqchip callbacks only. In this particular case, it is the device tree platform code which will handle the mapping creation for us. See: of_device_alloc in drivers/of/platform.c:144, this will eventually call irq_parse_and_map, which calls irq_create_mapping. * When a driver use the following approach: -- ret = request_irq(gpio_to_irq(GPIOX_Y), ... ) of course the device tree code won't create the mapping for us. It needs to be created in some other way. I believe, only the gpio framework can do it at this point, and this where there is a tiny relationship between the irq and gpio part, as mentioned above. If we are lucky, all the mappings can be created for all pins at probe time. No real management to be done in this case, just find your mapping a be done with it. If, for whatever reason, the mappings can't all be created at probe time, then you have problem with the drivers using request_irq(gpio_to_irq(GPIOX_Y), ... ) style ... where should we create this mapping ? There is no place holder for it. IOW, DT provides a way to get irq and it manages the mapping creation. By providing the gpio_to_irq call, the gpio framework also provides a path to get interrupts for consumer driver. Mapping also needs to be managed. I think there a hole there. I think the solution described in this RFC provide a solution for this, but does not change anything for the other approaches (DT-based interrupt specifier or probe time mapping creation) > If this interrupt+GPIO controller was using your scheme, where would these > new callbacks be called? In the consumer drivers which use the "request_irq(gpio_to_irq(GPIOX_Y), ... )" approach, most likely in the probe/init function. Driver have a choice, either * use the gpio approach. I believe these drivers are actually interested in the gpio state but would like to avoid polling monitoring. * use the DT approach, like your example with the ste-snowball. You don't care about the gpio here, all care about is the event. Regards Jerome > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-20 10:26 ` Jerome Brunet @ 2017-06-20 16:37 ` Linus Walleij 2017-06-20 17:23 ` Jerome Brunet 0 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2017-06-20 16:37 UTC (permalink / raw) To: Jerome Brunet Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, Jun 20, 2017 at 12:26 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: So I finally understood that what you want is to handle those cases where gpio_to_irq() is currently in use, and not expand the use of that function. And that is good. > On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote: >> > The fact that 26 gpio drivers [3] create mapping in the >> > gpio_to_irq callback shows that the problem of managing the irq mapping is >> > not >> > specific to Amlogic and that an evolution might be useful. >> >> Calling irq_create_mapping() in gpio_to_irq() is not a problem if >> the mapping is free:ed elsewhere. But yeah, there are lots of old >> drivers with old and erroneous solutions to this problem. > > A lot of these drivers are old indeed, and dealing with all this history must be > quite a challenge ;) > However, some are fairly recent. Unfortunately the API has merits. Like for keys: OK there is a key for an GPIO line, and then it asks "can I have an IRQ for that"? There are cases where the IRQ is really optional, not a required feature. > So there is 2 use cases: > * What you described above: This is indeed completely orthogonal to gpio > framework. It will be using the irqchip callbacks only. In this particular case, > it is the device tree platform code which will handle the mapping creation for > us. OK So the problem is really to use drivers that cannot do all their mappings at probe time with drivers that use gpio_to_irq(), right? So, if the idea is to patch *ALL* drivers using gpio_to_irq(), I think for each case it should be investigated whether that should really be using gpio_to_irq() but I suspect they are all pretty solid. Well you're probably right, the API has to change for the case where the number of IRQs are limited and cannot just be handled out to the left and right, but we should really replace *ALL* occurences of gpio_to_irq() with a pair of gpio_irq_prepare() that also returns the valid IRQ if found, and gpio_irq_unprepare() that removes it. For the transitional period, gpio_to_irq() should *FAIL* if gpio_irq_prepare() was called first for the same GPIO line. Eventually gpio_to_irq() should be DELETED and replaced in full with the prepare/unprepare calls. And I would like strong confidence that the change will be carried all the way through, not half-done and left to me to complete. I already have too many problems of that type. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-20 16:37 ` Linus Walleij @ 2017-06-20 17:23 ` Jerome Brunet 2017-06-21 20:50 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Jerome Brunet @ 2017-06-20 17:23 UTC (permalink / raw) To: Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: > On Tue, Jun 20, 2017 at 12:26 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > So I finally understood that what you want is to handle those cases > where gpio_to_irq() is currently in use, and not expand the use of that > function. And that is good. Indeed > > > On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote: > > > > The fact that 26 gpio drivers [3] create mapping in the > > > > gpio_to_irq callback shows that the problem of managing the irq mapping > > > > is > > > > not > > > > specific to Amlogic and that an evolution might be useful. > > > > > > Calling irq_create_mapping() in gpio_to_irq() is not a problem if > > > the mapping is free:ed elsewhere. But yeah, there are lots of old > > > drivers with old and erroneous solutions to this problem. > > > > A lot of these drivers are old indeed, and dealing with all this history > > must be > > quite a challenge ;) > > However, some are fairly recent. > > Unfortunately the API has merits. Like for keys: OK there is a key > for an GPIO line, and then it asks "can I have an IRQ for that"? > There are cases where the IRQ is really optional, not a required > feature. > I think you misunderstood me. I have nothing against the API. I absolutely want to keep it. I'd like to extend it. > > So there is 2 use cases: > > * What you described above: This is indeed completely orthogonal to gpio > > framework. It will be using the irqchip callbacks only. In this particular > > case, > > it is the device tree platform code which will handle the mapping creation > > for > > us. > > OK > > So the problem is really to use drivers that cannot do all their > mappings at probe > time with drivers that use gpio_to_irq(), right? Yep > > So, if the idea is to patch *ALL* drivers using gpio_to_irq(), I think > for each case > it should be investigated whether that should really be using gpio_to_irq() > but I suspect they are all pretty solid. > > Well you're probably right, the API has to change for the case where the > number > of IRQs are limited and cannot just be handled out to the left and right, but > we > should really replace *ALL* occurences of gpio_to_irq() with a pair of > gpio_irq_prepare() that also returns the valid IRQ if found, and > gpio_irq_unprepare() > that removes it. > > For the transitional period, gpio_to_irq() should *FAIL* if gpio_irq_prepare() > was called first for the same GPIO line. > > Eventually gpio_to_irq() should be DELETED and replaced in full with > the prepare/unprepare calls. Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it would be a mess and it is a useful call. The gpio_irq_prepare is meant so that the consumer can tell the gpio driver it will want to get irq from a particular gpio at some point. IOW, it's the consumer saying to the gpio driver "please do whatever you need to do, if anything, so this gpio can generate an interrupt" This is a much simpler change. Using devm, all we need is to put a devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using gpio_to_irq. Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be fairly easy. > > And I would like strong confidence that the change will be carried all the way > through, not half-done and left to me to complete. I already have too many > problems of that type. Fair enough. > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-20 17:23 ` Jerome Brunet @ 2017-06-21 20:50 ` Linus Walleij 2017-06-22 14:25 ` Jerome Brunet 0 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2017-06-21 20:50 UTC (permalink / raw) To: Jerome Brunet Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: >> Eventually gpio_to_irq() should be DELETED and replaced in full with >> the prepare/unprepare calls. > > Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it would > be a mess and it is a useful call. > > The gpio_irq_prepare is meant so that the consumer can tell the gpio driver it > will want to get irq from a particular gpio at some point. > > IOW, it's the consumer saying to the gpio driver "please do whatever you need to > do, if anything, so this gpio can generate an interrupt" > > This is a much simpler change. Using devm, all we need is to put a > devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using gpio_to_irq. > > Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be fairly > easy. So why can't we just return the IRQ from prepare() and be done with it instead of having two calls? (Plus a third eventual unprepare()). Clocks, regulators and godknowswhat is managed by two symmetrical calls, so why shouldn't GPIO IRQs be? It would be counterintuitive to have a third call in the middle. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-21 20:50 ` Linus Walleij @ 2017-06-22 14:25 ` Jerome Brunet 2017-06-27 17:49 ` Grygorii Strashko 0 siblings, 1 reply; 18+ messages in thread From: Jerome Brunet @ 2017-06-22 14:25 UTC (permalink / raw) To: Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote: > On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: > > > Eventually gpio_to_irq() should be DELETED and replaced in full with > > > the prepare/unprepare calls. > > > > Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it > > would > > be a mess and it is a useful call. > > > > The gpio_irq_prepare is meant so that the consumer can tell the gpio driver > > it > > will want to get irq from a particular gpio at some point. > > > > IOW, it's the consumer saying to the gpio driver "please do whatever you > > need to > > do, if anything, so this gpio can generate an interrupt" > > > > This is a much simpler change. Using devm, all we need is to put a > > devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using > > gpio_to_irq. > > > > Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be > > fairly > > easy. > > So why can't we just return the IRQ from prepare() and be done with it We can return it here as well, it's actually a good idea. New drivers could just use that one if they are keeping track of the irq number. > instead of having two calls? (Plus a third eventual unprepare()). > > Clocks, regulators and godknowswhat is managed by two symmetrical > calls, so why shouldn't GPIO IRQs be? The approach is exactly the same as what we trying to follow in the irq framework: framework | irq | gpio ----------------------------------------------------- index | hwirq | gpio_num creation | irq_create_mapping | gpio_irq_prepare ? removal | irq_dispose_mapping | gpio_irq_unprepare ? (fast) lookup | irq_find_mapping | gpio_to_irq We are going to have at lookup function somehow, why not expose it ? Another reason for keeping gpio_to_irq is that many existing drivers using the callback don't keep track of their irq number, just the gpio. For every irq operation, they call gpio_to_irq. Things like this: * irq_enable(gpio_to_irq(<gpio_num>)) * irq_release(gpio_to_irq(<gpio_num>)) * etc ... It's a bit lazy maybe, but I don't think there is anything utterly wrong with it. Getting rid of gpio_to_irq would mean reworking all those drivers so they keep track of their irq number. I think this would be a mess for a very little gain. Also, except for the 26 gpio drivers I have listed, the rest should already be implementing what qualify as a "lookup" function in gpio_to_irq. I don't think we should by modifying every single gpio driver when there is a solution to 'surgically' address the matter. The series would already affect a significant amount of drivers, I'm trying to keep it as simple an contained as possible. If that is OK with you, I could send an RFC implementing the idea but fixing only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss on, it might be easier. > > It would be counterintuitive to have a third call in the middle. > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-22 14:25 ` Jerome Brunet @ 2017-06-27 17:49 ` Grygorii Strashko 2017-06-27 18:25 ` Jerome Brunet 0 siblings, 1 reply; 18+ messages in thread From: Grygorii Strashko @ 2017-06-27 17:49 UTC (permalink / raw) To: Jerome Brunet, Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On 06/22/2017 09:25 AM, Jerome Brunet wrote: > On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote: >> On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: >>> On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: >>>> Eventually gpio_to_irq() should be DELETED and replaced in full with >>>> the prepare/unprepare calls. >>> >>> Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it >>> would >>> be a mess and it is a useful call. >>> >>> The gpio_irq_prepare is meant so that the consumer can tell the gpio driver >>> it >>> will want to get irq from a particular gpio at some point. >>> >>> IOW, it's the consumer saying to the gpio driver "please do whatever you >>> need to >>> do, if anything, so this gpio can generate an interrupt" >>> >>> This is a much simpler change. Using devm, all we need is to put a >>> devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using >>> gpio_to_irq. >>> >>> Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be >>> fairly >>> easy. >> >> So why can't we just return the IRQ from prepare() and be done with it > > We can return it here as well, it's actually a good idea. New drivers could just > use that one if they are keeping track of the irq number. > >> instead of having two calls? (Plus a third eventual unprepare()). >> >> Clocks, regulators and godknowswhat is managed by two symmetrical >> calls, so why shouldn't GPIO IRQs be? > > The approach is exactly the same as what we trying to follow in the irq > framework: > > framework | irq | gpio > ----------------------------------------------------- > index | hwirq | gpio_num > creation | irq_create_mapping | gpio_irq_prepare ? > removal | irq_dispose_mapping | gpio_irq_unprepare ? > (fast) lookup | irq_find_mapping | gpio_to_irq > > We are going to have at lookup function somehow, why not expose it ? > > Another reason for keeping gpio_to_irq is that many existing drivers using the > callback don't keep track of their irq number, just the gpio. For every irq > operation, they call gpio_to_irq. Things like this: > > * irq_enable(gpio_to_irq(<gpio_num>)) > * irq_release(gpio_to_irq(<gpio_num>)) > * etc ... > > It's a bit lazy maybe, but I don't think there is anything utterly wrong with > it. > > Getting rid of gpio_to_irq would mean reworking all those drivers so they keep > track of their irq number. I think this would be a mess for a very little gain. > > Also, except for the 26 gpio drivers I have listed, the rest should already be > implementing what qualify as a "lookup" function in gpio_to_irq. I don't think > we should by modifying every single gpio driver when there is a solution to > 'surgically' address the matter. > > The series would already affect a significant amount of drivers, I'm trying to > keep it as simple an contained as possible. > > If that is OK with you, I could send an RFC implementing the idea but fixing > only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss on, > it might be easier. > I'd like to add my 5 cents here :) 1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this is not allowed as gpio_to_irq is callable in irq context, therefore it should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq handlers." It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should already exist at that time and It might require to do sleepable calls to crate IRQ mappings and configure HW. Drivers, pointed out in first e-mail, should use other APIs in their IRQ handlers: drivers/gpio/gpio-ep93xx.c ^ direct call to ep93xx_gpio_to_irq() * drivers/gpio/gpio-pxa.c ^ use irq_find_mapping() * drivers/gpio/gpio-tegra.c ^ use irq_find_mapping() Also note, IRQ mappings can be created as dynamically (each time gpio_to_irq() is called) as statically (in probe). The last approach is widely used in gpio drivers due to compatibility and legacy reasons. 2) As per above I do not really understand why gpio_irq_prepare() is required. 3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs might be required, but what is the real use-case? Modules reloading or unloading one module and loading another instead? Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping created by first call to gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just re-use already created mapping. -- regards, -grygorii ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-27 17:49 ` Grygorii Strashko @ 2017-06-27 18:25 ` Jerome Brunet 2017-06-27 20:43 ` Grygorii Strashko 2017-06-29 14:14 ` Linus Walleij 0 siblings, 2 replies; 18+ messages in thread From: Jerome Brunet @ 2017-06-27 18:25 UTC (permalink / raw) To: Grygorii Strashko, Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, 2017-06-27 at 12:49 -0500, Grygorii Strashko wrote: > > On 06/22/2017 09:25 AM, Jerome Brunet wrote: > > On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote: > > > On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> > > > wrote: > > > > On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: > > > > > Eventually gpio_to_irq() should be DELETED and replaced in full with > > > > > the prepare/unprepare calls. > > > > > > > > Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of > > > > it > > > > would > > > > be a mess and it is a useful call. > > > > > > > > The gpio_irq_prepare is meant so that the consumer can tell the gpio > > > > driver > > > > it > > > > will want to get irq from a particular gpio at some point. > > > > > > > > IOW, it's the consumer saying to the gpio driver "please do whatever you > > > > need to > > > > do, if anything, so this gpio can generate an interrupt" > > > > > > > > This is a much simpler change. Using devm, all we need is to put a > > > > devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using > > > > gpio_to_irq. > > > > > > > > Mandating call to gpio_irq_prepare before any call to gpio_to_irq will > > > > be > > > > fairly > > > > easy. > > > > > > So why can't we just return the IRQ from prepare() and be done with it > > > > We can return it here as well, it's actually a good idea. New drivers could > > just > > use that one if they are keeping track of the irq number. > > > > > instead of having two calls? (Plus a third eventual unprepare()). > > > > > > Clocks, regulators and godknowswhat is managed by two symmetrical > > > calls, so why shouldn't GPIO IRQs be? > > > > The approach is exactly the same as what we trying to follow in the irq > > framework: > > > > framework | irq | gpio > > ----------------------------------------------------- > > index | hwirq | gpio_num > > creation | irq_create_mapping | gpio_irq_prepare ? > > removal | irq_dispose_mapping | gpio_irq_unprepare ? > > (fast) lookup | irq_find_mapping | gpio_to_irq > > > > We are going to have at lookup function somehow, why not expose it ? > > > > Another reason for keeping gpio_to_irq is that many existing drivers using > > the > > callback don't keep track of their irq number, just the gpio. For every irq > > operation, they call gpio_to_irq. Things like this: > > > > * irq_enable(gpio_to_irq(<gpio_num>)) > > * irq_release(gpio_to_irq(<gpio_num>)) > > * etc ... > > > > It's a bit lazy maybe, but I don't think there is anything utterly wrong > > with > > it. > > > > Getting rid of gpio_to_irq would mean reworking all those drivers so they > > keep > > track of their irq number. I think this would be a mess for a very little > > gain. > > > > Also, except for the 26 gpio drivers I have listed, the rest should already > > be > > implementing what qualify as a "lookup" function in gpio_to_irq. I don't > > think > > we should by modifying every single gpio driver when there is a solution to > > 'surgically' address the matter. > > > > The series would already affect a significant amount of drivers, I'm trying > > to > > keep it as simple an contained as possible. > > > > If that is OK with you, I could send an RFC implementing the idea but fixing > > only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss > > on, > > it might be easier. > > > > I'd like to add my 5 cents here :) > 1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this > is not > allowed as gpio_to_irq is callable in irq context, therefore it should not > sleep. > Actually 3 drivers [2] are calling gpio_to_irq in irq handlers." > > It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should > already exist at > that time and It might require to do sleepable calls to crate IRQ mappings and > configure HW. > > Drivers, pointed out in first e-mail, should use other APIs in their IRQ > handlers: > drivers/gpio/gpio-ep93xx.c > ^ direct call to ep93xx_gpio_to_irq() > * drivers/gpio/gpio-pxa.c > ^ use irq_find_mapping() > * drivers/gpio/gpio-tegra.c > ^ use irq_find_mapping() > > Also note, IRQ mappings can be created as dynamically (each > time gpio_to_irq() is called) Not according to a previous reply from Linus. Right or wrong, it would have made my life a lot easier if it was OK :) > as > statically (in probe). The last approach is widely used in gpio drivers due to > compatibility and > legacy reasons. Agreed this is the current situation. > > 2) As per above I do not really understand why gpio_irq_prepare() is required. I'd like to point you the thread which initially triggered this rfc: https://patchwork.ozlabs.org/patch/684208/ At the time Linus strongly rejected the idea of calling irq_create_mapping (or any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016 (sorry for quoting such an old discussion but this is really the starting point) * Me: There is really a *lot* of gpio drivers which use irq_create_mapping in the to_irq callback, are these all wrong ? * Linus: Yes they are all wrong. They should all be using irq_find_mapping(). * Me: If this should not be used, what should we all do instead ? * Linus: Call irq_create_mapping() in some other place. gpio_prepare_irq is a proposition for this 'other place'. > > 3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs > might be required, > but what is the real use-case? Modules reloading or unloading one module and > loading another instead? > Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping > created by first call to > gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just > re-use already created mapping. Providing that you always create mapping for the same pins, yes What happens when gpio irq (the pin) is different each time ? and you exhaust the ressource ? It is a corner case, but possible isn't it ? With the gpio char driver interface maybe. You would have to reboot to flush the old mappings and continue, right ? You could also fail to set the trigger type (which you won't be able to set in gpio_to_irq). If so, shouldn't you release the mapping ? (that's real use-case we are having by the way). Cheers Jerome ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-27 18:25 ` Jerome Brunet @ 2017-06-27 20:43 ` Grygorii Strashko 2017-06-29 14:16 ` Linus Walleij 2017-06-29 14:14 ` Linus Walleij 1 sibling, 1 reply; 18+ messages in thread From: Grygorii Strashko @ 2017-06-27 20:43 UTC (permalink / raw) To: Jerome Brunet, Linus Walleij Cc: linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On 06/27/2017 01:25 PM, Jerome Brunet wrote: > On Tue, 2017-06-27 at 12:49 -0500, Grygorii Strashko wrote: >> >> On 06/22/2017 09:25 AM, Jerome Brunet wrote: >>> On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote: >>>> On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> >>>> wrote: >>>>> On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote: >>>>>> Eventually gpio_to_irq() should be DELETED and replaced in full with >>>>>> the prepare/unprepare calls. >>>>> >>>>> Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of >>>>> it >>>>> would >>>>> be a mess and it is a useful call. >>>>> >>>>> The gpio_irq_prepare is meant so that the consumer can tell the gpio >>>>> driver >>>>> it >>>>> will want to get irq from a particular gpio at some point. >>>>> >>>>> IOW, it's the consumer saying to the gpio driver "please do whatever you >>>>> need to >>>>> do, if anything, so this gpio can generate an interrupt" >>>>> >>>>> This is a much simpler change. Using devm, all we need is to put a >>>>> devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using >>>>> gpio_to_irq. >>>>> >>>>> Mandating call to gpio_irq_prepare before any call to gpio_to_irq will >>>>> be >>>>> fairly >>>>> easy. >>>> >>>> So why can't we just return the IRQ from prepare() and be done with it >>> >>> We can return it here as well, it's actually a good idea. New drivers could >>> just >>> use that one if they are keeping track of the irq number. >>> >>>> instead of having two calls? (Plus a third eventual unprepare()). >>>> >>>> Clocks, regulators and godknowswhat is managed by two symmetrical >>>> calls, so why shouldn't GPIO IRQs be? >>> >>> The approach is exactly the same as what we trying to follow in the irq >>> framework: >>> >>> framework | irq | gpio >>> ----------------------------------------------------- >>> index | hwirq | gpio_num >>> creation | irq_create_mapping | gpio_irq_prepare ? >>> removal | irq_dispose_mapping | gpio_irq_unprepare ? >>> (fast) lookup | irq_find_mapping | gpio_to_irq >>> >>> We are going to have at lookup function somehow, why not expose it ? >>> >>> Another reason for keeping gpio_to_irq is that many existing drivers using >>> the >>> callback don't keep track of their irq number, just the gpio. For every irq >>> operation, they call gpio_to_irq. Things like this: >>> >>> * irq_enable(gpio_to_irq(<gpio_num>)) >>> * irq_release(gpio_to_irq(<gpio_num>)) >>> * etc ... >>> >>> It's a bit lazy maybe, but I don't think there is anything utterly wrong >>> with >>> it. >>> >>> Getting rid of gpio_to_irq would mean reworking all those drivers so they >>> keep >>> track of their irq number. I think this would be a mess for a very little >>> gain. >>> >>> Also, except for the 26 gpio drivers I have listed, the rest should already >>> be >>> implementing what qualify as a "lookup" function in gpio_to_irq. I don't >>> think >>> we should by modifying every single gpio driver when there is a solution to >>> 'surgically' address the matter. >>> >>> The series would already affect a significant amount of drivers, I'm trying >>> to >>> keep it as simple an contained as possible. >>> >>> If that is OK with you, I could send an RFC implementing the idea but fixing >>> only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss >>> on, >>> it might be easier. >>> >> >> I'd like to add my 5 cents here :) >> 1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this >> is not >> allowed as gpio_to_irq is callable in irq context, therefore it should not >> sleep. >> Actually 3 drivers [2] are calling gpio_to_irq in irq handlers." >> >> It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should >> already exist at >> that time and It might require to do sleepable calls to crate IRQ mappings and >> configure HW. >> >> Drivers, pointed out in first e-mail, should use other APIs in their IRQ >> handlers: >> drivers/gpio/gpio-ep93xx.c >> ^ direct call to ep93xx_gpio_to_irq() >> * drivers/gpio/gpio-pxa.c >> ^ use irq_find_mapping() >> * drivers/gpio/gpio-tegra.c >> ^ use irq_find_mapping() >> >> Also note, IRQ mappings can be created as dynamically (each >> time gpio_to_irq() is called) > > Not according to a previous reply from Linus. Right or wrong, it would have made > my life a lot easier if it was OK :) > >> as >> statically (in probe). The last approach is widely used in gpio drivers due to >> compatibility and >> legacy reasons. > > Agreed this is the current situation. > >> >> 2) As per above I do not really understand why gpio_irq_prepare() is required. > > I'd like to point you the thread which initially triggered this rfc: > https://patchwork.ozlabs.org/patch/684208/ > > At the time Linus strongly rejected the idea of calling irq_create_mapping (or > any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016 > (sorry for quoting such an old discussion but this is really the starting point) > > * Me: There is really a *lot* of gpio drivers which use irq_create_mapping in > the to_irq callback, are these all wrong ? > * Linus: Yes they are all wrong. They should all be using irq_find_mapping(). > > * Me: If this should not be used, what should we all do instead ? > * Linus: Call irq_create_mapping() in some other place. > > gpio_prepare_irq is a proposition for this 'other place'. And my opinion is still the same here - It should be perfectly valid to create mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and irq descriptors on demand). The valid use case for gpio_to_irq() is below (and only one valid use-case): - consumer driver probe() (or other init function): gpio = [devm_]gpiod_get() irq = gpio_to_irq(gpio) |- in general case -> irq_create_mapping() request_irq(irq,...) >From another side, driver may request GPIO IRQ without even knowing about gpiochip - consumer driver probe() (or other init function): irq = platform_get_irq()/of_irq_get() |- in general case -> irq_create_mapping() request_irq(irq,...) In both cases, IRQ mapping is expected to be created by corresponding IRQ domain. More over, drivers can share GPIO IRQ and in this case first call to irq_create_mapping() will actually create mapping while second will be transformed to irq_find_mapping(). > >> >> 3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs >> might be required, >> but what is the real use-case? Modules reloading or unloading one module and >> loading another instead? >> Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping >> created by first call to >> gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just >> re-use already created mapping. > > Providing that you always create mapping for the same pins, yes > What happens when gpio irq (the pin) is different each time ? and you exhaust > the ressource ? So, do you expect use-case like: - request GPIO IRQs for pins A1..A8 - free GPIO IRQs A5..A8 - request GPIO IRQs for pins B1..A4 ... (you have 8 gpio irqs as per your description) This is not smth really common and that why I'm asking ;) and that's why nobody really care about GPIO IRQs un-mapping. It's kinda expected to plan GPIO IRQ usage very carefully hence you have only 8 of them. > > It is a corner case, but possible isn't it ? With the gpio char driver > interface maybe. You would have to reboot to flush the old mappings and > continue, right ? true, but no complains till now ;) And, as I said, It might be reasonable to have gpio_irq_unprepare() - if you have real use-case description ;) But note, it's unclear how to integrate it? Will it be required to modify all drivers which are using gpio_to_irq() (not good) or it can be integrated in gpiolib and triggered automatically (good)? > > You could also fail to set the trigger type (which you won't be able to set in > gpio_to_irq). If so, shouldn't you release the mapping ? (that's real use-case > we are having by the way). Sry, I didn't get this. IRQ type is set in request_irq() or by irqd_set_trigger_type(). Its actually expected to be reset (configured to safe state) in gpio-irqchip->irq_shutdown() callback. Also, It might be helpful to take a look on drivers/irqchip/irq-crossbar.c. -- regards, -grygorii ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-27 20:43 ` Grygorii Strashko @ 2017-06-29 14:16 ` Linus Walleij 2017-06-30 19:54 ` Grygorii Strashko 0 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2017-06-29 14:16 UTC (permalink / raw) To: Grygorii Strashko Cc: Jerome Brunet, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > And my opinion is still the same here - It should be perfectly valid to create > mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and > gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and > irq descriptors on demand). You are right. I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO lines and IRQs should not do it, they should map up them all at probe(). Drivers that actually have fewer IRQs than GPIO lines should be able to create the mappings in gpio_to_irq(). Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-29 14:16 ` Linus Walleij @ 2017-06-30 19:54 ` Grygorii Strashko 2017-07-02 15:01 ` Linus Walleij 0 siblings, 1 reply; 18+ messages in thread From: Grygorii Strashko @ 2017-06-30 19:54 UTC (permalink / raw) To: Linus Walleij Cc: Jerome Brunet, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On 06/29/2017 09:16 AM, Linus Walleij wrote: > On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> And my opinion is still the same here - It should be perfectly valid to create >> mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and >> gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and >> irq descriptors on demand). > > You are right. > > I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO > lines and IRQs should not do it, they should map up them all at probe(). > Sry, can't completely agree here :( There could be 300 (or even thousands) of gpios and only dozen of them will be used as GPIO IRQ, so statical mapping will just waste system resources. So, better not define such kind of restrictions - it seems platform/system specific. > Drivers that actually have fewer IRQs than GPIO lines should be able to > create the mappings in gpio_to_irq(). -- regards, -grygorii ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-30 19:54 ` Grygorii Strashko @ 2017-07-02 15:01 ` Linus Walleij 0 siblings, 0 replies; 18+ messages in thread From: Linus Walleij @ 2017-07-02 15:01 UTC (permalink / raw) To: Grygorii Strashko Cc: Jerome Brunet, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Fri, Jun 30, 2017 at 9:54 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 06/29/2017 09:16 AM, Linus Walleij wrote: >> On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko >> <grygorii.strashko@ti.com> wrote: >> >>> And my opinion is still the same here - It should be perfectly valid to create >>> mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and >>> gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and >>> irq descriptors on demand). >> >> You are right. >> >> I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO >> lines and IRQs should not do it, they should map up them all at probe(). >> > > Sry, can't completely agree here :( There could be 300 (or even thousands) > of gpios and only dozen of them will be used as GPIO IRQ, so statical mapping will > just waste system resources. So, better not define such kind of restrictions - > it seems platform/system specific. If there could, yeah. But the majority of the worlds systems use a hardcoded value of 512 GPIOs. See include/asm-generic/gpio.h: #ifndef ARCH_NR_GPIOS #if defined(CONFIG_ARCH_NR_GPIO) && CONFIG_ARCH_NR_GPIO > 0 #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO #else #define ARCH_NR_GPIOS 512 #endif #endif The only arch that overrides this is ARM, which has CONFIG_ARCH_NR_GPIO: # The GPIO number here must be sorted by descending number. In case of # a multiplatform kernel, we just want the highest value required by the # selected platforms. config ARCH_NR_GPIO int default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \ ARCH_ZYNQ default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \ SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210 default 416 if ARCH_SUNXI default 392 if ARCH_U8500 default 352 if ARCH_VT8500 default 288 if ARCH_ROCKCHIP default 264 if MACH_H4700 default 0 help Maximum number of GPIOs in the system. If unsure, leave the default value. So actually, noone use more than 1024 GPIOs. For each GPIO a descriptor of 16 bytes is allocated. So in worst case 16KiB. Also this is an upper cap: it just means the nax we have on any platform is 1024 statically allocated GPIO descriptors, not that they all get mapped to IRQs. That happens on a per-gpiochip basis. But every irq descriptor is pretty big (somewhere around 64+ bytes), so you have a point. If every GPIO allocates an IRQ descriptor, it may add up to as much as 64KiB on a machine with 1024 GPIOs. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-27 18:25 ` Jerome Brunet 2017-06-27 20:43 ` Grygorii Strashko @ 2017-06-29 14:14 ` Linus Walleij 2017-06-29 14:57 ` Jerome Brunet 1 sibling, 1 reply; 18+ messages in thread From: Linus Walleij @ 2017-06-29 14:14 UTC (permalink / raw) To: Jerome Brunet Cc: Grygorii Strashko, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > At the time Linus strongly rejected the idea of calling irq_create_mapping (or > any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016 > (sorry for quoting such an old discussion but this is really the starting point) > > * Me: There is really a *lot* of gpio drivers which use irq_create_mapping in > the to_irq callback, are these all wrong ? > * Linus: Yes they are all wrong. They should all be using irq_find_mapping(). > > * Me: If this should not be used, what should we all do instead ? > * Linus: Call irq_create_mapping() in some other place. > > gpio_prepare_irq is a proposition for this 'other place'. There is a misunderstanding here. I wrote (at the time): > Yes, but you want to call irq_create_mapping() in slowpath (irq setup) > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ > may result in unwelcomed surprises. This does not mean that irq_create_mapping() cannot be called in irq context because I think it can. What it means is that I think that is suboptimal from a performance point of view if called from gpio_to_irq(), because nominally, at this point, the mapping should already exist, since the GPIO and IRQ frameworks are orthogonal. But that may not apply to the case with many-to-few interrupt mappings... so I think I was in some 1-to-1-mapping context when I wrote this. Sorry :( So I changed my mind, it is fine for this type of driver to call irq_create_mapping() in gpio_to_irq(). Preferably with some comment around the call. It remains to see what happens if gpio_to_irq() would fail, as can happen in this case. Like if gpio_to_irq() would return 0 (NO_IRQ) or something negative. I think many drivers are not equipped for handling this. So I guess if you could change the semantics of all drivers calling gpio_to_irq() to also handle say 0 as an error and bail out, we can call irq_create_mapping() in gpio_to_irq(). Sorry if I'm confused... or confusing. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-29 14:14 ` Linus Walleij @ 2017-06-29 14:57 ` Jerome Brunet 2017-06-29 16:53 ` Marc Zyngier 2017-06-29 22:16 ` Linus Walleij 0 siblings, 2 replies; 18+ messages in thread From: Jerome Brunet @ 2017-06-29 14:57 UTC (permalink / raw) To: Linus Walleij Cc: Grygorii Strashko, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote: > On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > At the time Linus strongly rejected the idea of calling irq_create_mapping > > (or > > any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, > > 2016 > > (sorry for quoting such an old discussion but this is really the starting > > point) > > > > * Me: There is really a *lot* of gpio drivers which use irq_create_mapping > > in > > the to_irq callback, are these all wrong ? > > * Linus: Yes they are all wrong. They should all be using > > irq_find_mapping(). > > > > * Me: If this should not be used, what should we all do instead ? > > * Linus: Call irq_create_mapping() in some other place. > > > > gpio_prepare_irq is a proposition for this 'other place'. > > There is a misunderstanding here. > > I wrote (at the time): > > > Yes, but you want to call irq_create_mapping() in slowpath (irq setup) > > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ > > may result in unwelcomed surprises. > > This does not mean that irq_create_mapping() cannot be called > in irq context because I think it can. > > What it means is that I think that is suboptimal from a performance > point of view if called from gpio_to_irq(), because nominally, at this > point, the mapping should already exist, since the GPIO and IRQ > frameworks are orthogonal. Agreed. It should. In such case, irq_create_mapping is just a call to irq_find_mapping which is fine. If, for whatever reason this is not the case, this is going to call sleeping function in irq context. It should not happen but it is not entirely impossible ... > > But that may not apply to the case with many-to-few interrupt > mappings... so I think I was in some 1-to-1-mapping context > when I wrote this. Sorry :( > > So I changed my mind, it is fine for this type of driver to call > irq_create_mapping() in gpio_to_irq(). Preferably with some comment > around the call. What about disposing of the mapping ? there still is no counter part function to gpio_to_irq. It seems weird to leave them lying around, don't you think ? Here is a real use we will be having a meson: * MMC driver load and call gpio_to_irq on its cd pin This is going to create a mapping * MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't support at the moment). request_irq fails. * MMC defaults to polling the GPIO Remember that we have only 8 possibles mappings. Now there is one (useless) mapping lying around which can't get rid of. If there is more drivers doing this sort of tricks, we are going to exhaust the ressource pretty quickly. That's just one exemple. The fact is, as soon as a driver calls gpio_to_irq, the mapping is going to stay forever. Don't you think we should try address this ? I will happily (re)submit a patch with irq_create_mapping in gpio_to_irq to bring the 'initial' gpio irq support in meson, until something better comes up... > > It remains to see what happens if gpio_to_irq() would fail, as can > happen in this case. Like if gpio_to_irq() would return 0 (NO_IRQ) > or something negative. I think many drivers are not equipped for > handling this. I think this is already fine, gpio_to_irq is supposed to return 0 if there is no mapping, isn't it ? > > So I guess if you could change the semantics of all drivers > calling gpio_to_irq() to also handle say 0 as an error and bail > out, we can call irq_create_mapping() in gpio_to_irq(). > > Sorry if I'm confused... or confusing. Aren't we all ... we'll get there, someday :) > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-29 14:57 ` Jerome Brunet @ 2017-06-29 16:53 ` Marc Zyngier 2017-06-29 18:33 ` Jerome Brunet 2017-06-29 22:16 ` Linus Walleij 1 sibling, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2017-06-29 16:53 UTC (permalink / raw) To: Jerome Brunet, Linus Walleij Cc: Grygorii Strashko, linux-gpio, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On 29/06/17 15:57, Jerome Brunet wrote: > On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote: >> On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> >>> At the time Linus strongly rejected the idea of calling irq_create_mapping >>> (or >>> any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, >>> 2016 >>> (sorry for quoting such an old discussion but this is really the starting >>> point) >>> >>> * Me: There is really a *lot* of gpio drivers which use irq_create_mapping >>> in >>> the to_irq callback, are these all wrong ? >>> * Linus: Yes they are all wrong. They should all be using >>> irq_find_mapping(). >>> >>> * Me: If this should not be used, what should we all do instead ? >>> * Linus: Call irq_create_mapping() in some other place. >>> >>> gpio_prepare_irq is a proposition for this 'other place'. >> >> There is a misunderstanding here. >> >> I wrote (at the time): >> >>> Yes, but you want to call irq_create_mapping() in slowpath (irq setup) >>> and irq_find_mapping() in fastpath (irq handler). Else the first IRQ >>> may result in unwelcomed surprises. >> >> This does not mean that irq_create_mapping() cannot be called >> in irq context because I think it can. >> >> What it means is that I think that is suboptimal from a performance >> point of view if called from gpio_to_irq(), because nominally, at this >> point, the mapping should already exist, since the GPIO and IRQ >> frameworks are orthogonal. > > Agreed. It should. In such case, irq_create_mapping is just a call to > irq_find_mapping which is fine. If, for whatever reason this is not the case, > this is going to call sleeping function in irq context. It should not happen but > it is not entirely impossible ... > >> >> But that may not apply to the case with many-to-few interrupt >> mappings... so I think I was in some 1-to-1-mapping context >> when I wrote this. Sorry :( >> >> So I changed my mind, it is fine for this type of driver to call >> irq_create_mapping() in gpio_to_irq(). Preferably with some comment >> around the call. > > What about disposing of the mapping ? there still is no counter part function to > gpio_to_irq. It seems weird to leave them lying around, don't you think ? > > Here is a real use we will be having a meson: > * MMC driver load and call gpio_to_irq on its cd pin > This is going to create a mapping > * MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't > support at the moment). request_irq fails. > * MMC defaults to polling the GPIO > > Remember that we have only 8 possibles mappings. Now there is one (useless) > mapping lying around which can't get rid of. Which should be dropped by a dispose_mapping() call in the MMC driver (or ideally some gpio-specific wrapper around this function). > If there is more drivers doing this sort of tricks, we are going to exhaust the > ressource pretty quickly. We can fix those drivers as we go. It's not like there's a huge variety of potential HW on this particular platform of yours. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-29 16:53 ` Marc Zyngier @ 2017-06-29 18:33 ` Jerome Brunet 0 siblings, 0 replies; 18+ messages in thread From: Jerome Brunet @ 2017-06-29 18:33 UTC (permalink / raw) To: Marc Zyngier, Linus Walleij Cc: Grygorii Strashko, linux-gpio, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Thu, 2017-06-29 at 17:53 +0100, Marc Zyngier wrote: > On 29/06/17 15:57, Jerome Brunet wrote: > > On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote: > > > On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> > > > wrote: > > > > > > > At the time Linus strongly rejected the idea of > > > > calling irq_create_mapping > > > > (or > > > > any sleeping functions) in gpio_to_irq: please see the reply from Oct > > > > 26, > > > > 2016 > > > > (sorry for quoting such an old discussion but this is really the > > > > starting > > > > point) > > > > > > > > * Me: There is really a *lot* of gpio drivers which use > > > > irq_create_mapping > > > > in > > > > the to_irq callback, are these all wrong ? > > > > * Linus: Yes they are all wrong. They should all be using > > > > irq_find_mapping(). > > > > > > > > * Me: If this should not be used, what should we all do instead ? > > > > * Linus: Call irq_create_mapping() in some other place. > > > > > > > > gpio_prepare_irq is a proposition for this 'other place'. > > > > > > There is a misunderstanding here. > > > > > > I wrote (at the time): > > > > > > > Yes, but you want to call irq_create_mapping() in slowpath (irq setup) > > > > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ > > > > may result in unwelcomed surprises. > > > > > > This does not mean that irq_create_mapping() cannot be called > > > in irq context because I think it can. > > > > > > What it means is that I think that is suboptimal from a performance > > > point of view if called from gpio_to_irq(), because nominally, at this > > > point, the mapping should already exist, since the GPIO and IRQ > > > frameworks are orthogonal. > > > > Agreed. It should. In such case, irq_create_mapping is just a call to > > irq_find_mapping which is fine. If, for whatever reason this is not the > > case, > > this is going to call sleeping function in irq context. It should not happen > > but > > it is not entirely impossible ... > > > > > > > > But that may not apply to the case with many-to-few interrupt > > > mappings... so I think I was in some 1-to-1-mapping context > > > when I wrote this. Sorry :( > > > > > > So I changed my mind, it is fine for this type of driver to call > > > irq_create_mapping() in gpio_to_irq(). Preferably with some comment > > > around the call. > > > > What about disposing of the mapping ? there still is no counter part > > function to > > gpio_to_irq. It seems weird to leave them lying around, don't you think ? > > > > Here is a real use we will be having a meson: > > * MMC driver load and call gpio_to_irq on its cd pin > > This is going to create a mapping > > * MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't > > support at the moment). request_irq fails. > > * MMC defaults to polling the GPIO > > > > Remember that we have only 8 possibles mappings. Now there is one (useless) > > mapping lying around which can't get rid of. > > Which should be dropped by a dispose_mapping() call in the MMC driver > (or ideally some gpio-specific wrapper around this function). Actually you must go through a gpio-specific function. Not all gpio drivers use irq domains and mapping unfortunately (legacy...) Only the gpio driver knows how this "mapping" (between the gpio number and the virq) was done. The proposition at the start of this is my idea for this gpio-specific function. I'm open to other ideas, of course :) > > > If there is more drivers doing this sort of tricks, we are going to exhaust > > the > > ressource pretty quickly. > > We can fix those drivers as we go. It's not like there's a huge variety > of potential HW on this particular platform of yours. > Those drivers are consumer of gpios. They should not have platform specific fixes/quirks , right? Like the mmc framework, or the gpio-button driver... In addition, this problem of un-disposed mappings may also affect any of the 26 driver which create mapping in gpio_to_irq, so it is not specific to a single platform. > Thanks, > > M. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC] gpio: about the need to manage irq mapping dynamically. 2017-06-29 14:57 ` Jerome Brunet 2017-06-29 16:53 ` Marc Zyngier @ 2017-06-29 22:16 ` Linus Walleij 1 sibling, 0 replies; 18+ messages in thread From: Linus Walleij @ 2017-06-29 22:16 UTC (permalink / raw) To: Jerome Brunet Cc: Grygorii Strashko, linux-gpio, Marc Zyngier, open list:ARM/Amlogic Meson..., Kevin Hilman, Linux Kernel On Thu, Jun 29, 2017 at 4:57 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote: >> So I changed my mind, it is fine for this type of driver to call >> irq_create_mapping() in gpio_to_irq(). Preferably with some comment >> around the call. > > What about disposing of the mapping ? there still is no counter part function to > gpio_to_irq. It seems weird to leave them lying around, don't you think ? Yeah that sucks. We need a way to fix that. Is gpio_irq_prepare/unprepare the right approach? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-07-02 15:01 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-15 16:20 [RFC] gpio: about the need to manage irq mapping dynamically Jerome Brunet 2017-06-20 8:39 ` Linus Walleij 2017-06-20 10:26 ` Jerome Brunet 2017-06-20 16:37 ` Linus Walleij 2017-06-20 17:23 ` Jerome Brunet 2017-06-21 20:50 ` Linus Walleij 2017-06-22 14:25 ` Jerome Brunet 2017-06-27 17:49 ` Grygorii Strashko 2017-06-27 18:25 ` Jerome Brunet 2017-06-27 20:43 ` Grygorii Strashko 2017-06-29 14:16 ` Linus Walleij 2017-06-30 19:54 ` Grygorii Strashko 2017-07-02 15:01 ` Linus Walleij 2017-06-29 14:14 ` Linus Walleij 2017-06-29 14:57 ` Jerome Brunet 2017-06-29 16:53 ` Marc Zyngier 2017-06-29 18:33 ` Jerome Brunet 2017-06-29 22:16 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).