Re: [PATCH/RFC] drivers/irqchip: add irq-inverter

[Marc Zyngier]

On Tue, 28 Dec 2021 19:03:23 +0000,
Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
    Link: https://lkml.kernel.org/r/1416902662-19281-3-git-send-email-yingjoe.chen@xxxxxxxxxxxx
    Signed-off-by: Jason Cooper <jason@xxxxxxxxxxxxxx>

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");

-- 
Without deviation from the norm, progress is not possible.