| Message ID | 20211228165642.2514766-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCH/RFC] drivers/irqchip: add irq-inverter | expand |
On Tue, 28 Dec 2021 16:56:43 +0000, Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > Interrupt trigger type is typically used to configure interrupt > controller to properly interpret interrupt signal sent from a device. > > However, some devices have configureable interrupt outputs, and drivers > tend to use interrupt trigger type also to configure device interrupt > output. > > This works well when device interrupt output is connected directly to > interrupt controller input. However, this is not always the case. > Sometimes the interrupt signal gets inverted between the device > producing it and the controller consuming it. Combined with both sides > using the same interrupt trigger type to configure the signal, this > results into non-working setup regardless of what interrupt trigger type > is configured. Regardless? Surely there is a canonical, working configuration. > > Irq-inverer is a solution for this case. It is a virtual irqchip that > provides additional virq number that behaves exactly as existing one, > but with inverted trigger type reported via irq_get_trigger_type() API. > > Usage example, for Kingfisher extension board for Renesas Gen-3 Soc, > that has WiFi interrupt delivered over inverting level-shifter: > > / { > wlcore_interrupt: inverter { > compatible = "linux,irq-inverter"; > interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>; > interrupt-controller; > #interrupt-cells = <0>; > }; > }; > > &wlcore { > interrupts-extended = <&wlcore_interrupt>; > }; So you don't describe the trigger at the endpoint level, but at the pseudo-interrupt controller level? /me feels mildly sick. And by the way: "An interrupt specifier is one or more cells of data (as specified by #interrupt-cells) ...". Ergo, #interrupt-cells cannot be 0 when the interrupt controller can be an interrupt-parent. > > Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from > irq_get_trigger_type() call, and configures interrupt output for that. > Then the signal is delivered inverted to the GPIO module, and handled > correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING. So this is only to avoid writing the correct device tree? > > Implementation notes: > > - why platform_driver and not IRQCHIP_DECLARE()? > - because IRQCHIP_DECLARE() does not process EPROBE_DEFER properly More importantly, IRQCHIP_DECLARE() is for root interrupt controllers that need to be probed long before we have a device model up and running. > - why not using hierarchial irq_domain? > - because with hierarchial irq_domain, same interrupt gets the same virq > number at all levels, and trigger type is tied to virq number, so need > different virq numbers for reporting different trigger types Why would you have different interrupt numbers? A given line has one configuration at any given point, and only one. > > - why using request_irq() for parent irq, instead of setting up chained > interrupt in irqchips? > - because this way code is much simpler, and shall work for all cases > (such as normal/threaded parent irq, normal/threaded child irq, > different parent interrupt chips, etc) And that's a NAK for deliberately violating the irqchip API. > > - why just not introducing separate API for consumer-side and > produced-side trigger type? > - because with the chosen approach, no changes are needed to any cases > that don't suffer from inverted interrupt routing The right way to do it is to use the existing API by exposing the inverter (there are existing examples in the tree, using the hierarchical model). It isn't rocket science, and not much more code than the pile of hacks^W^W^Wcreative approach you have. M.
Hi >> Interrupt trigger type is typically used to configure interrupt >> controller to properly interpret interrupt signal sent from a device. >> >> However, some devices have configureable interrupt outputs, and drivers >> tend to use interrupt trigger type also to configure device interrupt >> output. >> >> This works well when device interrupt output is connected directly to >> interrupt controller input. However, this is not always the case. >> Sometimes the interrupt signal gets inverted between the device >> producing it and the controller consuming it. Combined with both sides >> using the same interrupt trigger type to configure the signal, this >> results into non-working setup regardless of what interrupt trigger type >> is configured. > > Regardless? Surely there is a canonical, working configuration. It is working as long as either hardware delivers interrupt signal without inversion, or only one side (producer or consumer) is configured while the other side stays constant. It does not work when hardware inverts singnal and both producer and consumer are configured. >> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc, >> that has WiFi interrupt delivered over inverting level-shifter: >> >> / { >> wlcore_interrupt: inverter { >> compatible = "linux,irq-inverter"; >> interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>; >> interrupt-controller; >> #interrupt-cells = <0>; >> }; >> }; >> >> &wlcore { >> interrupts-extended = <&wlcore_interrupt>; >> }; > > So you don't describe the trigger at the endpoint level, but at the > pseudo-interrupt controller level? /me feels mildly sick. Could you please explain how this could be done? Regardless of what is configured at endpoint side, interrupt controller driver will use that to set up interrupt controller, and wl18xxx driver (in the case) will use that to configure wl18xx. That results into SAME settings at producer and consumer sides, and hardware requires OPPOSITE sittings at producer and consumer sides. It is not a problem in interrupt controller driver - that driver does it's job correctly, setting up the interrupt type that is requested. It is likely not a problem in interrupt source (i.e. wl18xx) driver - that driver tries to set up it's irq in the way that will work with any interrupt controller. Perhaps it can be possible to update wl18xx driver to allow fixed setup of interrupt polarity, but that looks like addressing problem at wrong location. It is not an issue with wl18xx. There are quite a few drivers in the tree that setup interrupt polarity for their devices based on what irq_get_trigger_type() returns. The root cause if the issue is the board itself, that inverts the signal. Thus it looks correct to tie the fix to the board. And a device node describing the interconnect looks like an elegant solution for this. > And by the way: "An interrupt specifier is one or more cells of data > (as specified by #interrupt-cells) ...". Ergo, #interrupt-cells cannot > be 0 when the interrupt controller can be an interrupt-parent. Code works with #interrupt-cells=0 correctly, as long as interrupts-extended property is used at producer side. >> Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from >> irq_get_trigger_type() call, and configures interrupt output for that. >> Then the signal is delivered inverted to the GPIO module, and handled >> correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING. > > So this is only to avoid writing the correct device tree? No. It is an attempt to describe a case that seems to be not currently describable. Vendor BSPs solve this by commenting out irq polarity setup in drivers. Thus obviously breaking use cases other than these BSPs are for. I'm trying to suggest a portable alternative instead. >> - why not using hierarchial irq_domain? >> - because with hierarchial irq_domain, same interrupt gets the same virq >> number at all levels, and trigger type is tied to virq number, so need >> different virq numbers for reporting different trigger types > > Why would you have different interrupt numbers? A given line has one > configuration at any given point, and only one. The goal is - make irq_get_trigger_type() returning different values for producer and consumer of the interrupt. Because, that matches the hardware behavior. While irq_get_trigger_type() is used for that, returning different values for producer and consumer is obviously impossible. Originally, I was considering to add a different API to use by interrupt producer instead of irq_get_trigger_type(), to deliver information configured by additional flag in DT node for interrupt producer. I.e. interrupts = <25 IRQ_TYPE_EDGE_RISING | IRQ_INVERTED_ROUTE> But, inverted route is not a feature of interrupt, it is a feature of connection. I.e. nothing stops from having several sources routed to the same interrupt, and having only one of these sources inverted. This means, the IRQ_INVERTED_ROUTE flag is not interrupt's flag but connection's flag. And, a virtual irqchip looked like a good abstraction to describe connection. The fact that this make the entire solution much smaller, was just a pleasant side-effect ;). >> - why using request_irq() for parent irq, instead of setting up chained >> interrupt in irqchips? >> - because this way code is much simpler, and shall work for all cases >> (such as normal/threaded parent irq, normal/threaded child irq, >> different parent interrupt chips, etc) > > And that's a NAK for deliberately violating the irqchip API. I've learned the idea of calling generic_handle_irq() from interrupt handler from Documentation/driver-api/gpio/driver.rst It looked as an elegant solution to avoid complexity (such as: a chained interrupt is activated at registration time, assuming there is a piece of hardware [chained controller] that will avoid interrupt firing until a chained source fires... but for pure-software chained interrupt, must keep parent disabled up to when a handler for child is installed) But, I definitely don't insist on this approach. >> - why just not introducing separate API for consumer-side and >> produced-side trigger type? >> - because with the chosen approach, no changes are needed to any cases >> that don't suffer from inverted interrupt routing > > The right way to do it is to use the existing API by exposing the > inverter (there are existing examples in the tree, using the > hierarchical model). It isn't rocket science, and not much more code > than the pile of hacks^W^W^Wcreative approach you have. Could you please point me to such examples? I could not find any. Thanks, Nikita
Hi Nikita, On Tue, Dec 28, 2021 at 6:23 PM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > Interrupt trigger type is typically used to configure interrupt > controller to properly interpret interrupt signal sent from a device. > > However, some devices have configureable interrupt outputs, and drivers > tend to use interrupt trigger type also to configure device interrupt > output. > > This works well when device interrupt output is connected directly to > interrupt controller input. However, this is not always the case. > Sometimes the interrupt signal gets inverted between the device > producing it and the controller consuming it. Combined with both sides > using the same interrupt trigger type to configure the signal, this > results into non-working setup regardless of what interrupt trigger type > is configured. > > Irq-inverer is a solution for this case. It is a virtual irqchip that > provides additional virq number that behaves exactly as existing one, > but with inverted trigger type reported via irq_get_trigger_type() API. > > Usage example, for Kingfisher extension board for Renesas Gen-3 Soc, > that has WiFi interrupt delivered over inverting level-shifter: > > / { > wlcore_interrupt: inverter { > compatible = "linux,irq-inverter"; > interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>; > interrupt-controller; > #interrupt-cells = <0>; > }; > }; > > &wlcore { > interrupts-extended = <&wlcore_interrupt>; > }; > > Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from > irq_get_trigger_type() call, and configures interrupt output for that. > Then the signal is delivered inverted to the GPIO module, and handled > correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING. > > Implementation notes: > > - why platform_driver and not IRQCHIP_DECLARE()? > - because IRQCHIP_DECLARE() does not process EPROBE_DEFER properly > > - why not using hierarchial irq_domain? > - because with hierarchial irq_domain, same interrupt gets the same virq > number at all levels, and trigger type is tied to virq number, so need > different virq numbers for reporting different trigger types > > - why using request_irq() for parent irq, instead of setting up chained > interrupt in irqchips? > - because this way code is much simpler, and shall work for all cases > (such as normal/threaded parent irq, normal/threaded child irq, > different parent interrupt chips, etc) > > - why just not introducing separate API for consumer-side and > produced-side trigger type? > - because with the chosen approach, no changes are needed to any cases > that don't suffer from inverted interrupt routing > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> Thanks for your patch! FTR, here's a link to the previous discussion about this topic: https://lore.kernel.org/all/20190607172958.20745-1-erosca@de.adit-jv.com/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi > FTR, here's a link to the previous discussion about this topic: > https://lore.kernel.org/all/20190607172958.20745-1-erosca@de.adit-jv.com/ Thanks for the link. AFAICS, there was no solution found that time? I was able to make it at least working... and I'm ready to apply any reasonable changes to make this, or any other, solution for the issue acceptable for mainline. Nikita
On Tue, 28 Dec 2021 19:03:23 +0000, Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > Hi > > >> Interrupt trigger type is typically used to configure interrupt > >> controller to properly interpret interrupt signal sent from a device. > >> > >> However, some devices have configureable interrupt outputs, and drivers > >> tend to use interrupt trigger type also to configure device interrupt > >> output. > >> > >> This works well when device interrupt output is connected directly to > >> interrupt controller input. However, this is not always the case. > >> Sometimes the interrupt signal gets inverted between the device > >> producing it and the controller consuming it. Combined with both sides > >> using the same interrupt trigger type to configure the signal, this > >> results into non-working setup regardless of what interrupt trigger type > >> is configured. > > > > Regardless? Surely there is a canonical, working configuration. > > It is working as long as either hardware delivers interrupt signal > without inversion, or only one side (producer or consumer) is > configured while the other side stays constant. > > It does not work when hardware inverts singnal and both producer and > consumer are configured. As in *badly* configured. > > >> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc, > >> that has WiFi interrupt delivered over inverting level-shifter: > >> > >> / { > >> wlcore_interrupt: inverter { > >> compatible = "linux,irq-inverter"; > >> interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>; > >> interrupt-controller; > >> #interrupt-cells = <0>; > >> }; > >> }; > >> > >> &wlcore { > >> interrupts-extended = <&wlcore_interrupt>; > >> }; > > > > So you don't describe the trigger at the endpoint level, but at the > > pseudo-interrupt controller level? /me feels mildly sick. > > Could you please explain how this could be done? > > Regardless of what is configured at endpoint side, interrupt > controller driver will use that to set up interrupt controller, and > wl18xxx driver (in the case) will use that to configure wl18xx. That > results into SAME settings at producer and consumer sides, and > hardware requires OPPOSITE sittings at producer and consumer sides. > > It is not a problem in interrupt controller driver - that driver does > it's job correctly, setting up the interrupt type that is requested. > > It is likely not a problem in interrupt source (i.e. wl18xx) driver - > that driver tries to set up it's irq in the way that will work with > any interrupt controller. Perhaps it can be possible to update wl18xx > driver to allow fixed setup of interrupt polarity, but that looks like > addressing problem at wrong location. It is not an issue with > wl18xx. There are quite a few drivers in the tree that setup interrupt > polarity for their devices based on what irq_get_trigger_type() > returns. > > The root cause if the issue is the board itself, that inverts the > signal. Thus it looks correct to tie the fix to the board. And a > device node describing the interconnect looks like an elegant solution > for this. This is one option. But the way you do that is pretty broken, because you place the information at the wrong level in DT, and then violate every abstraction in the book in your kernel code. > > > And by the way: "An interrupt specifier is one or more cells of data > > (as specified by #interrupt-cells) ...". Ergo, #interrupt-cells cannot > > be 0 when the interrupt controller can be an interrupt-parent. > > Code works with #interrupt-cells=0 correctly, as long as > interrupts-extended property is used at producer side. News flash: you are relying on a *BUG* in the Linux code. > > >> Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from > >> irq_get_trigger_type() call, and configures interrupt output for that. > >> Then the signal is delivered inverted to the GPIO module, and handled > >> correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING. > > > > So this is only to avoid writing the correct device tree? > > No. It is an attempt to describe a case that seems to be not > currently describable. commit 5fe3bba3088c4efab32a18649643b5075755b4b3 Author: Yingjoe Chen <yingjoe.chen@mediatek.com> Date: Tue Nov 25 16:04:20 2014 +0800 irqchip: mtk-sysirq: Add sysirq interrupt polarity support Mediatek SoCs have interrupt polarity support in sysirq which allows to invert polarity for given interrupt. Add this support using hierarchy irq domain. Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com> Link: https://lkml.kernel.org/r/1416902662-19281-3-git-send-email-yingjoe.chen@mediatek.com Signed-off-by: Jason Cooper <jason@lakedaemon.net> Not describable? You haven't quite looked hard enough. > > Vendor BSPs solve this by commenting out irq polarity setup in > drivers. Thus obviously breaking use cases other than these BSPs are > for. I'm trying to suggest a portable alternative instead. > > >> - why not using hierarchial irq_domain? > >> - because with hierarchial irq_domain, same interrupt gets the same virq > >> number at all levels, and trigger type is tied to virq number, so need > >> different virq numbers for reporting different trigger types > > > > Why would you have different interrupt numbers? A given line has one > > configuration at any given point, and only one. > > The goal is - make irq_get_trigger_type() returning different values > for producer and consumer of the interrupt. Because, that matches the > hardware behavior. And that utterly broken. From the point of view of the *device* there is only *one* trigger coming from the DT. [...] > > The right way to do it is to use the existing API by exposing the > > inverter (there are existing examples in the tree, using the > > hierarchical model). It isn't rocket science, and not much more code > > than the pile of hacks^W^W^Wcreative approach you have. > > Could you please point me to such examples? I could not find any. I pointed to such an example in the tree above. A much simpler version can be written in a few minutes, see below. It is untested, and needs to be made to support various intspec formats, but you'll hopefully understand how it works and fix it. M. diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index c1f611cbfbf8..a5ca92db1d73 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -obj-$(CONFIG_IRQCHIP) += irqchip.o +obj-$(CONFIG_IRQCHIP) += irqchip.o irq-dummy-inverter.o obj-$(CONFIG_AL_FIC) += irq-al-fic.o obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o diff --git a/drivers/irqchip/irq-dummy-inverter.c b/drivers/irqchip/irq-dummy-inverter.c new file mode 100644 index 000000000000..a7e7c13d29ad --- /dev/null +++ b/drivers/irqchip/irq-dummy-inverter.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * A stupidly dumb "driver" for inverters. + * + * inverter0: inverter { + * compatible = "dummy,inverter"; + * interrupt-controller; + * #interrupt-cells = <3>; // Matches parent + * }; + * + * endpoint-device { + * interrupt-parent = <&inverter0>; + * interrupts = <GIC_SPI 666 IRQ_TYPE_EDGE_RISING>; + * }; + */ +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/of_irq.h> + + +static int inverter_set_type(struct irq_data *data, unsigned int type) +{ + switch (type) { + case IRQ_TYPE_LEVEL_LOW: + type = IRQ_TYPE_LEVEL_HIGH; + break; + case IRQ_TYPE_LEVEL_HIGH: + type = IRQ_TYPE_LEVEL_LOW; + break; + case IRQ_TYPE_EDGE_FALLING: + type = IRQ_TYPE_EDGE_RISING; + break; + case IRQ_TYPE_EDGE_RISING: + type = IRQ_TYPE_EDGE_FALLING; + break; + } + + data = data->parent_data; + return data->chip->irq_set_type(data, type); +} + +static struct irq_chip inverter_chip = { + .name = "Upside Down", + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_eoi = irq_chip_eoi_parent, + .irq_set_type = inverter_set_type, + .irq_retrigger = irq_chip_retrigger_hierarchy, + .irq_set_affinity = irq_chip_set_affinity_parent, + .irq_set_wake = irq_chip_set_wake_parent, +}; + +static int inverter_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + /* Hardcoded for a 3-cell intspec, to be generalised */ + *hwirq = fwspec->param[1]; + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; + return 0; +} + +static int inverter_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + struct irq_fwspec *fwspec = arg; + struct irq_fwspec parent_fwspec; + irq_hw_number_t hwirq; + + /* Only deal with a single interrupt at a time */ + WARN_ON(nr_irqs != 1); + + parent_fwspec = *fwspec; + hwirq = fwspec->param[1]; + + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, + &inverter_chip, + domain->host_data); + + parent_fwspec.fwnode = domain->parent->fwnode; + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); +} + +static const struct irq_domain_ops inverter_domain_ops = { + .translate = inverter_domain_translate, + .alloc = inverter_domain_alloc, + .free = irq_domain_free_irqs_common, +}; + +static int __init inverter_of_init(struct device_node *node, + struct device_node *parent) +{ + struct irq_domain *domain; + + domain = irq_domain_create_hierarchy(irq_find_host(parent), 0, 1, + of_node_to_fwnode(node), + &inverter_domain_ops, NULL); + if (!domain) + return -ENOMEM; + + return 0; +} + +IRQCHIP_PLATFORM_DRIVER_BEGIN(dummy_inverter) +IRQCHIP_MATCH("dummy,inverter", inverter_of_init) +IRQCHIP_PLATFORM_DRIVER_END(dummy_inverter) +MODULE_DESCRIPTION("The Rise and Fall of Ziggy Stardust and the Spiders from Mars"); +MODULE_LICENSE("GPL v2");
Hi >>> The right way to do it is to use the existing API by exposing the >>> inverter (there are existing examples in the tree, using the >>> hierarchical model)... > > A much simpler version can be written in a few minutes, see below... Can something like that be used if the parent domain is not hierarchical (i.e. does not provide alloc(), but provides map() instead)? In particular, gpio-rcar currently provides irq domain that is not hierarchical. As well as quite a few other gpiochips. Nikita
On Wed, 29 Dec 2021 19:52:18 +0000, Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > Hi > > >>> The right way to do it is to use the existing API by exposing the > >>> inverter (there are existing examples in the tree, using the > >>> hierarchical model)... > > > > A much simpler version can be written in a few minutes, see below... > > Can something like that be used if the parent domain is not > hierarchical (i.e. does not provide alloc(), but provides map() > instead)? No. This definitely relies on the parent being hierarchical, as that's exactly what it was designed for the first place. > In particular, gpio-rcar currently provides irq domain that is not > hierarchical. As well as quite a few other gpiochips. Well, you just found yourself some useful work! ;-) M.
>>>>> The right way to do it is to use the existing API by exposing the >>>>> inverter (there are existing examples in the tree, using the >>>>> hierarchical model)... >>> >>> A much simpler version can be written in a few minutes, see below... >> >> Can something like that be used if the parent domain is not >> hierarchical (i.e. does not provide alloc(), but provides map() >> instead)? > > No. This definitely relies on the parent being hierarchical, as that's > exactly what it was designed for the first place. Is supporting hierarchical API now mandatory for kernel irqchips? If yes, then perhaps you can at least document it somewhere? E.g. declare irq_domain.map() as deprecated? If no, then I'd like to discuss a solution for irq_inverter that can work for non-hierarchical case. Nikita
On Thu, 30 Dec 2021 10:53:30 +0000, Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > > >>>>> The right way to do it is to use the existing API by exposing the > >>>>> inverter (there are existing examples in the tree, using the > >>>>> hierarchical model)... > >>> > >>> A much simpler version can be written in a few minutes, see below... > >> > >> Can something like that be used if the parent domain is not > >> hierarchical (i.e. does not provide alloc(), but provides map() > >> instead)? > > > > No. This definitely relies on the parent being hierarchical, as that's > > exactly what it was designed for the first place. > > Is supporting hierarchical API now mandatory for kernel irqchips? It isn't. But you are definitely giving me some ideas now. > > If yes, then perhaps you can at least document it somewhere? > E.g. declare irq_domain.map() as deprecated? > > If no, then I'd like to discuss a solution for irq_inverter that can > work for non-hierarchical case. You are still missing the point. Any active element on the interrupt path that changes the signal without multiplexing must be described in the hierarchical model. Anything else is wrong, and I am not interested in reinventing that particular wheel (it was painful enough to kill all these hacks years ago, and I'm not doing it again). Moving these GPIO chips into the hierarchical model isn't rocket science, as there is plenty of support for that already, and is the right thing to do. M.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 7038957f4a77..02040024b4da 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -617,4 +617,14 @@ config MCHP_EIC help Support for Microchip External Interrupt Controller. +config IRQ_INVERTER + bool "IRQ inverter support" + select IRQ_DOMAIN + help + Support for virtual interrupt line that represent inverted signal + of other interrupt line. + + This is intended for cases when both interrupt source and interrupt + consumer use interrupt's trigger type to configure interrupt signal + polarity, but the interrupt signal is negated by hardware in between. endmenu diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index c1f611cbfbf8..8e7d27b0c13d 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -117,3 +117,4 @@ obj-$(CONFIG_WPCM450_AIC) += irq-wpcm450-aic.o obj-$(CONFIG_IRQ_IDT3243X) += irq-idt3243x.o obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o obj-$(CONFIG_MCHP_EIC) += irq-mchp-eic.o +obj-$(CONFIG_IRQ_INVERTER) += irq-inverter.o diff --git a/drivers/irqchip/irq-inverter.c b/drivers/irqchip/irq-inverter.c new file mode 100644 index 000000000000..ef48a26aee96 --- /dev/null +++ b/drivers/irqchip/irq-inverter.c @@ -0,0 +1,159 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/interrupt.h> +#include <linux/irqchip.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +struct irq_inverter { + int parent_irq; + int child_irq; + unsigned int inverted_type; +}; + +static irqreturn_t irq_inverter_parent_irq(int irq, void *data) +{ + struct irq_inverter *inv = data; + unsigned long flags; + + raw_local_irq_save(flags); + generic_handle_irq(inv->child_irq); + raw_local_irq_restore(flags); + + return IRQ_HANDLED; +} + +static void irq_inverter_enable(struct irq_data *data) +{ + struct irq_inverter *inv = data->chip_data; + + enable_irq(inv->parent_irq); +} + +static void irq_inverter_disable(struct irq_data *data) +{ + struct irq_inverter *inv = data->chip_data; + + disable_irq_nosync(inv->parent_irq); +} + +static int irq_inverter_set_type(struct irq_data *data, unsigned int type) +{ + struct irq_inverter *inv = data->chip_data; + + return type == inv->inverted_type ? 0 : -EINVAL; +} + +static struct irq_chip irq_inverter_chip = { + .name = KBUILD_MODNAME, + .irq_enable = irq_inverter_enable, + .irq_disable = irq_inverter_disable, + .irq_set_type = irq_inverter_set_type, +}; + +static int irq_inverter_xlate(struct irq_domain *d, struct device_node *node, + const u32 *intspec, unsigned int intsize, + unsigned long *out_hwirq, unsigned int *out_type) +{ + struct irq_inverter *inv = d->host_data; + + if (intsize != 0) + return -EINVAL; + + *out_hwirq = 0; + *out_type = inv->inverted_type; + return 0; +} + +static int irq_inverter_map(struct irq_domain *d, unsigned int virq, + irq_hw_number_t hw) +{ + struct irq_inverter *inv = d->host_data; + + inv->child_irq = virq; + irq_set_chip_and_handler(virq, &irq_inverter_chip, handle_simple_irq); + irq_set_chip_data(virq, inv); + return 0; +} + +static const struct irq_domain_ops irq_inverter_domain_ops = { + .xlate = irq_inverter_xlate, + .map = irq_inverter_map, +}; + +static int irq_inverter_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + struct irq_inverter *inv; + struct irq_domain *domain; + unsigned int parent_type; + int ret; + + inv = kzalloc(sizeof(*inv), GFP_KERNEL); + if (!inv) + return -ENOMEM; + + ret = of_irq_get(node, 0); + if (ret < 0) { + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "could not get parent irq\n"); + goto err_free_inv; + } + inv->parent_irq = ret; + + parent_type = irq_get_trigger_type(inv->parent_irq); + if (!parent_type) { + dev_err(&pdev->dev, "parent irq trigger type is not defined\n"); + ret = -EINVAL; + goto err_free_inv; + } + if (parent_type & IRQ_TYPE_EDGE_RISING) + inv->inverted_type |= IRQ_TYPE_EDGE_FALLING; + if (parent_type & IRQ_TYPE_EDGE_FALLING) + inv->inverted_type |= IRQ_TYPE_EDGE_RISING; + if (parent_type & IRQ_TYPE_LEVEL_HIGH) + inv->inverted_type |= IRQ_TYPE_LEVEL_LOW; + if (parent_type & IRQ_TYPE_LEVEL_LOW) + inv->inverted_type |= IRQ_TYPE_LEVEL_HIGH; + + ret = request_irq(inv->parent_irq, irq_inverter_parent_irq, + parent_type | IRQF_NO_AUTOEN, KBUILD_MODNAME, inv); + if (ret < 0) { + dev_err(&pdev->dev, "could not request parent irq\n"); + goto err_free_inv; + } + + domain = irq_domain_add_linear(node, 1, &irq_inverter_domain_ops, inv); + if (!domain) { + ret = -ENOMEM; + goto err_free_irq; + } + + return 0; + +err_free_irq: + free_irq(inv->parent_irq, inv); +err_free_inv: + kfree(inv); + return ret; +} + +static const struct of_device_id irq_inverter_match[] = { + { .compatible = "linux,irq-inverter" }, + {} +}; + +static struct platform_driver irq_inverter_driver = { + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = irq_inverter_match, + }, + .probe = irq_inverter_probe, +}; + +static int __init irq_inverter_init(void) +{ + return platform_driver_register(&irq_inverter_driver); +} + +subsys_initcall(irq_inverter_init);
Interrupt trigger type is typically used to configure interrupt controller to properly interpret interrupt signal sent from a device. However, some devices have configureable interrupt outputs, and drivers tend to use interrupt trigger type also to configure device interrupt output. This works well when device interrupt output is connected directly to interrupt controller input. However, this is not always the case. Sometimes the interrupt signal gets inverted between the device producing it and the controller consuming it. Combined with both sides using the same interrupt trigger type to configure the signal, this results into non-working setup regardless of what interrupt trigger type is configured. Irq-inverer is a solution for this case. It is a virtual irqchip that provides additional virq number that behaves exactly as existing one, but with inverted trigger type reported via irq_get_trigger_type() API. Usage example, for Kingfisher extension board for Renesas Gen-3 Soc, that has WiFi interrupt delivered over inverting level-shifter: / { wlcore_interrupt: inverter { compatible = "linux,irq-inverter"; interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>; interrupt-controller; #interrupt-cells = <0>; }; }; &wlcore { interrupts-extended = <&wlcore_interrupt>; }; Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from irq_get_trigger_type() call, and configures interrupt output for that. Then the signal is delivered inverted to the GPIO module, and handled correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING. Implementation notes: - why platform_driver and not IRQCHIP_DECLARE()? - because IRQCHIP_DECLARE() does not process EPROBE_DEFER properly - why not using hierarchial irq_domain? - because with hierarchial irq_domain, same interrupt gets the same virq number at all levels, and trigger type is tied to virq number, so need different virq numbers for reporting different trigger types - why using request_irq() for parent irq, instead of setting up chained interrupt in irqchips? - because this way code is much simpler, and shall work for all cases (such as normal/threaded parent irq, normal/threaded child irq, different parent interrupt chips, etc) - why just not introducing separate API for consumer-side and produced-side trigger type? - because with the chosen approach, no changes are needed to any cases that don't suffer from inverted interrupt routing Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- drivers/irqchip/Kconfig | 10 +++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-inverter.c | 159 +++++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 drivers/irqchip/irq-inverter.c