[PATCH v2] drivers: irqchip: add irq-type-changer

[PATCH v2] drivers: irqchip: add irq-type-changer

[Nikita Yushchenko]

Irq type changer is a virtual irqchip useful to support boards that
change (e.g. invert) interrupt signal between producer and consumer.

Usage example, for Kingfisher extension board for Renesas Gen-3 Soc,
that has WiFi interrupt delivered over inverting level-shifter:

/ {
	gpio1_25_inverted: inverter {
		compatible = "linux,irq-type-changer";
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupt-parent = <&gpio1>;
		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
	};
};

&wlcore {
        interrupt-parent = <&gpio1_25_inverted>;
	interrupts = <0 IRQ_TYPE_EDGE_RISING>;
};

Then, wlcore driver observes IRQ_TYPE_EDGE_RISING trigger type and
configures interrupt output as such. At the same time, gpio-rcar driver
gets interrupt configured for IRQ_TYPE_EDGE_FALLING.

This version uses hierarchical irq_domain API, and works only with
parent interrupt domains compatible with that API.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
v1: https://lore.kernel.org/lkml/20220119201741.717770-1-nikita.yoush@cogentembedded.com/
Changes from v1:
- fixed order of kzalloc() args, caught by kbuild robot

 drivers/irqchip/Kconfig            |  18 ++++
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-type-changer.c | 162 +++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/irqchip/irq-type-changer.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 7038957f4a77..7f348016e82c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -617,4 +617,22 @@ config MCHP_EIC
 	help
 	  Support for Microchip External Interrupt Controller.
 
+config IRQ_TYPE_CHANGER
+	bool "Interrupt trigger type changer"
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Interrupt trigger type changer is designed to support boards that
+	  modify (e.g. invert) signal between interrupt source and interrupt
+	  controller input. So trigger type configured by a driver for some
+	  interrupt output pin does not match trigger type that shall be used
+	  to configure interrupt controller's input where that pin is connected.
+
+	  In this case, board device tree shall add an interrupt trigger
+	  type changer node and use it as the interrupt parent for the node
+	  representing interrupt source. Then, interrupt trigger type defined
+	  in the interrupt source node will be visible for the interrupt source
+	  driver, and (different) trigger type defined inside the changer node
+	  will be used to configure the interrupt controller.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index c1f611cbfbf8..57a664837857 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_TYPE_CHANGER)		+= irq-type-changer.o
diff --git a/drivers/irqchip/irq-type-changer.c b/drivers/irqchip/irq-type-changer.c
new file mode 100644
index 000000000000..0f461fda6610
--- /dev/null
+++ b/drivers/irqchip/irq-type-changer.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+
+struct changer {
+	unsigned long count;
+	struct {
+		struct irq_fwspec fwspec;
+		unsigned int type;
+	} out[0];
+};
+
+static int changer_set_type(struct irq_data *data, unsigned int type)
+{
+	struct changer *ch = data->domain->host_data;
+	struct irq_data *parent_data = data->parent_data;
+
+	return parent_data->chip->irq_set_type(parent_data,
+					       ch->out[data->hwirq].type);
+}
+
+static struct irq_chip changer_chip = {
+	.name			= "type-changer",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= changer_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 changer_domain_translate(struct irq_domain *domain,
+				    struct irq_fwspec *fwspec,
+				    unsigned long *hwirq,
+				    unsigned int *type)
+{
+	struct changer *ch = domain->host_data;
+
+	if (fwspec->param_count != 2)
+		return -EINVAL;
+	if (fwspec->param[0] >= ch->count)
+		return -ENXIO;
+
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+static int changer_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				unsigned int nr_irqs, void *arg)
+{
+	struct changer *ch = domain->host_data;
+	struct irq_fwspec *fwspec = arg;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret;
+
+	if (WARN_ON(nr_irqs != 1))
+		return -EINVAL;
+
+	ret = changer_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &changer_chip, ch);
+
+	return irq_domain_alloc_irqs_parent(domain, virq, 1,
+					    &ch->out[hwirq].fwspec);
+}
+
+static const struct irq_domain_ops changer_domain_ops = {
+	.translate	= changer_domain_translate,
+	.alloc		= changer_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+static int __init changer_of_init(struct device_node *node,
+				  struct device_node *parent)
+{
+	struct irq_domain *domain, *parent_domain;
+	int count, i, ret;
+	struct changer *ch;
+	struct of_phandle_args pargs;
+	irq_hw_number_t unused;
+
+	if (!parent) {
+		pr_err("%pOF: no parent node\n", node);
+		return -EINVAL;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("%pOF: no parent domain\n", node);
+		return -EINVAL;
+	}
+
+	if (WARN_ON(!parent_domain->ops->translate))
+		return -EINVAL;
+
+	count = of_irq_count(node);
+	if (count < 1) {
+		pr_err("%pOF: no interrupts defined\n", node);
+		return -EINVAL;
+	}
+
+	ch = kzalloc(sizeof(*ch) + count * sizeof(ch->out[0]), GFP_KERNEL);
+	if (!ch)
+		return -ENOMEM;
+	ch->count = count;
+
+	for (i = 0; i < count; i++) {
+		ret = of_irq_parse_one(node, i, &pargs);
+		if (ret) {
+			pr_err("%pOF: interrupt %d: error %d parsing\n",
+			       node, i, ret);
+			goto out_free;
+		}
+		of_phandle_args_to_fwspec(pargs.np, pargs.args,
+					  pargs.args_count,
+					  &ch->out[i].fwspec);
+		ret = parent_domain->ops->translate(parent_domain,
+						    &ch->out[i].fwspec,
+						    &unused,
+						    &ch->out[i].type);
+		if (ret) {
+			pr_err("%pOF: interrupt %d: error %d extracting type\n",
+			       node, i, ret);
+			goto out_free;
+		}
+		if (ch->out[i].type == IRQ_TYPE_NONE) {
+			pr_err("%pOF: interrupt %d: no type\n", node, i);
+			ret = -ENXIO;
+			goto out_free;
+		}
+	}
+
+	domain = irq_domain_create_hierarchy(parent_domain, 0, count,
+					     of_node_to_fwnode(node),
+					     &changer_domain_ops, ch);
+	if (!domain) {
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	return 0;
+
+out_free:
+	kfree(ch);
+	return ret;
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(changer)
+IRQCHIP_MATCH("linux,irq-type-changer", changer_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(changer)
+MODULE_AUTHOR("Nikita Yushchenko <nikita.yoush.cogentembedded.com>");
+MODULE_DESCRIPTION("Virtual irqchip to support trigger type change in route");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Marc Zyngier]

Nikita,

This patch is obviously part of a series, and yet you post them as
unrelated patches. Please don't do that. Keep the patches together and
treat them as a proper series.

On Mon, 24 Jan 2022 09:56:52 +0000,
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> 
> Irq type changer is a virtual irqchip useful to support boards that
> change (e.g. invert) interrupt signal between producer and consumer.
> 
> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc,
> that has WiFi interrupt delivered over inverting level-shifter:
> 
> / {
> 	gpio1_25_inverted: inverter {
> 		compatible = "linux,irq-type-changer";
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
> 		interrupt-parent = <&gpio1>;
> 		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> 	};
> };

You need a proper DT binding.

I also don't see why you model this as the actual device that triggers
the interrupt. This goes against the way we have modelled similar
devices in the tree. I'm also pretty sure that with the current code,
you end-up with *two* interrupts (one for the inverter and one for the
end-point). More on that later.

> 
> &wlcore {
>         interrupt-parent = <&gpio1_25_inverted>;
> 	interrupts = <0 IRQ_TYPE_EDGE_RISING>;
> };
> 
> Then, wlcore driver observes IRQ_TYPE_EDGE_RISING trigger type and
> configures interrupt output as such. At the same time, gpio-rcar driver
> gets interrupt configured for IRQ_TYPE_EDGE_FALLING.
> 
> This version uses hierarchical irq_domain API, and works only with
> parent interrupt domains compatible with that API.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
> v1: https://lore.kernel.org/lkml/20220119201741.717770-1-nikita.yoush@cogentembedded.com/
> Changes from v1:
> - fixed order of kzalloc() args, caught by kbuild robot
> 
>  drivers/irqchip/Kconfig            |  18 ++++
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-type-changer.c | 162 +++++++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/irqchip/irq-type-changer.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..7f348016e82c 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -617,4 +617,22 @@ config MCHP_EIC
>  	help
>  	  Support for Microchip External Interrupt Controller.
>  
> +config IRQ_TYPE_CHANGER
> +	bool "Interrupt trigger type changer"
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Interrupt trigger type changer is designed to support boards that
> +	  modify (e.g. invert) signal between interrupt source and interrupt
> +	  controller input. So trigger type configured by a driver for some
> +	  interrupt output pin does not match trigger type that shall be used
> +	  to configure interrupt controller's input where that pin is connected.
> +
> +	  In this case, board device tree shall add an interrupt trigger
> +	  type changer node and use it as the interrupt parent for the node
> +	  representing interrupt source. Then, interrupt trigger type defined
> +	  in the interrupt source node will be visible for the interrupt source
> +	  driver, and (different) trigger type defined inside the changer node
> +	  will be used to configure the interrupt controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c1f611cbfbf8..57a664837857 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_TYPE_CHANGER)		+= irq-type-changer.o
> diff --git a/drivers/irqchip/irq-type-changer.c b/drivers/irqchip/irq-type-changer.c
> new file mode 100644
> index 000000000000..0f461fda6610
> --- /dev/null
> +++ b/drivers/irqchip/irq-type-changer.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +
> +struct changer {

Changer doesn't quite describe it. This is an inverter, please name it
as such.

> +	unsigned long count;
> +	struct {
> +		struct irq_fwspec fwspec;
> +		unsigned int type;
> +	} out[0];

Geert commented on why this is wrong.

> +};
> +
> +static int changer_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct changer *ch = data->domain->host_data;
> +	struct irq_data *parent_data = data->parent_data;
> +
> +	return parent_data->chip->irq_set_type(parent_data,
> +					       ch->out[data->hwirq].type);

Can you actually take this at face-value? Some transformations don't
make any sense (you can't turn an edge into a level, for example).
Which is also why calling this a 'type changer' is wrong.

> +}
> +
> +static struct irq_chip changer_chip = {
> +	.name			= "type-changer",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_type		= changer_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 changer_domain_translate(struct irq_domain *domain,
> +				    struct irq_fwspec *fwspec,
> +				    unsigned long *hwirq,
> +				    unsigned int *type)
> +{
> +	struct changer *ch = domain->host_data;
> +
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +	if (fwspec->param[0] >= ch->count)
> +		return -ENXIO;
> +
> +	*hwirq = fwspec->param[0];
> +	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +
> +static int changer_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	struct changer *ch = domain->host_data;
> +	struct irq_fwspec *fwspec = arg;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int ret;
> +
> +	if (WARN_ON(nr_irqs != 1))
> +		return -EINVAL;
> +
> +	ret = changer_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &changer_chip, ch);
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, 1,
> +					    &ch->out[hwirq].fwspec);

So here, you are forcing the reallocation of an interrupt that was
already created, and overriding the existing mapping. That's not
right.

> +}
> +
> +static const struct irq_domain_ops changer_domain_ops = {
> +	.translate	= changer_domain_translate,
> +	.alloc		= changer_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +static int __init changer_of_init(struct device_node *node,
> +				  struct device_node *parent)
> +{
> +	struct irq_domain *domain, *parent_domain;
> +	int count, i, ret;
> +	struct changer *ch;
> +	struct of_phandle_args pargs;
> +	irq_hw_number_t unused;
> +
> +	if (!parent) {
> +		pr_err("%pOF: no parent node\n", node);
> +		return -EINVAL;
> +	}
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		pr_err("%pOF: no parent domain\n", node);
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(!parent_domain->ops->translate))
> +		return -EINVAL;
> +
> +	count = of_irq_count(node);
> +	if (count < 1) {
> +		pr_err("%pOF: no interrupts defined\n", node);
> +		return -EINVAL;
> +	}
> +
> +	ch = kzalloc(sizeof(*ch) + count * sizeof(ch->out[0]), GFP_KERNEL);

Use struct_size().

> +	if (!ch)
> +		return -ENOMEM;
> +	ch->count = count;
> +
> +	for (i = 0; i < count; i++) {
> +		ret = of_irq_parse_one(node, i, &pargs);
> +		if (ret) {
> +			pr_err("%pOF: interrupt %d: error %d parsing\n",
> +			       node, i, ret);
> +			goto out_free;
> +		}
> +		of_phandle_args_to_fwspec(pargs.np, pargs.args,
> +					  pargs.args_count,
> +					  &ch->out[i].fwspec);
> +		ret = parent_domain->ops->translate(parent_domain,
> +						    &ch->out[i].fwspec,
> +						    &unused,
> +						    &ch->out[i].type);
> +		if (ret) {
> +			pr_err("%pOF: interrupt %d: error %d extracting type\n",
> +			       node, i, ret);
> +			goto out_free;
> +		}
> +		if (ch->out[i].type == IRQ_TYPE_NONE) {
> +			pr_err("%pOF: interrupt %d: no type\n", node, i);
> +			ret = -ENXIO;
> +			goto out_free;
> +		}
> +	}

This would be all pretty unnecessary if you adopted the scheme I gave
you last time [1]. The interrupt duality definitely is a no-go.

> +
> +	domain = irq_domain_create_hierarchy(parent_domain, 0, count,
> +					     of_node_to_fwnode(node),
> +					     &changer_domain_ops, ch);
> +	if (!domain) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	return 0;
> +
> +out_free:
> +	kfree(ch);
> +	return ret;
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(changer)
> +IRQCHIP_MATCH("linux,irq-type-changer", changer_of_init)
> +IRQCHIP_PLATFORM_DRIVER_END(changer)
> +MODULE_AUTHOR("Nikita Yushchenko <nikita.yoush.cogentembedded.com>");
> +MODULE_DESCRIPTION("Virtual irqchip to support trigger type change in route");
> +MODULE_LICENSE("GPL v2");

Thanks,

	M.

[1] https://lore.kernel.org/lkml/87fsqbznc2.wl-maz@kernel.org

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

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Nikita Yushchenko]

Hi

 > I also don't see why you model this as the actual device that triggers
 > the interrupt.

Well, that somehow matches the physical reality. In the case of wl18xx on KF, physically the interrupt 
signal indeed originates from the intermediate chip - the inverting level-shifter.

I remember your suggestion to configure interrupt source's node with interrupt-parent set to inverter 
and interrupt details for inverter's parent. But this looks hacky and inconsistent for me.

In contrary, an abstraction of intermediate entity that does a static conversion of the trigger type and 
does not need any software control, looks sane. And, hardware designers do strange things sometimes, I 
won't be surprised observing a change from level to edge one day. Thus it looked a good idea not to 
limit the conversion to inversion, but support arbitrary one. Then, irqspec inside the node for the 
intermediate entity obtains a meaning, making the entire model consistent.

I don't strongly insist on this approach, it just looks cleaner for me.


 > I'm also pretty sure that with the current code,
 > you end-up with *two* interrupts (one for the inverter and one for the
 > end-point).

In driver's init, I only call of_irq_parse_one() for interrupt defined in the changer's node. This does 
not create a mapping for it. The mapping is only created when changer's "interrupt-child" creates a 
mapping for their interrupt - then changer's alloc() routine calls irq_domain_alloc_irqs_parent() in the 
same way as all other hierarchical irqchips do.

I don't see where double mapping can appear here. Please explain.


 > You need a proper DT binding...
 > Geert commented on why this is wrong...
 > Use struct_size()...

Will fix all that after we negotiate the basic approach.

Thanks,

Nikita

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Marc Zyngier]

On Tue, 25 Jan 2022 05:47:37 +0000,
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> 
> Hi
> 
> > I also don't see why you model this as the actual device that triggers
> > the interrupt.
> 
> Well, that somehow matches the physical reality. In the case of wl18xx
> on KF, physically the interrupt signal indeed originates from the
> intermediate chip - the inverting level-shifter.

Reality? By allowing something like an edge-to-level conversion? How
can that work?

> 
> I remember your suggestion to configure interrupt source's node with
> interrupt-parent set to inverter and interrupt details for inverter's
> parent. But this looks hacky and inconsistent for me.

We'll have to agree to disagree.

> 
> In contrary, an abstraction of intermediate entity that does a static
> conversion of the trigger type and does not need any software control,
> looks sane. And, hardware designers do strange things sometimes, I
> won't be surprised observing a change from level to edge one day.

If you think that it can happen without a HW register that latches the
edge and requires an ack, you need to question your understanding of
an interrupt life cycle, and of the properties of the various trigger
types.

> Thus it looked a good idea not to limit the conversion to inversion,
> but support arbitrary one. Then, irqspec inside the node for the
> intermediate entity obtains a meaning, making the entire model
> consistent.

Again, this cannot work, because the very *semantics* of an edge
interrupt (event) cannot directly be converted in a level (state).

> 
> I don't strongly insist on this approach, it just looks cleaner for me.
> 
> 
> > I'm also pretty sure that with the current code,
> > you end-up with *two* interrupts (one for the inverter and one for the
> > end-point).
> 
> In driver's init, I only call of_irq_parse_one() for interrupt defined
> in the changer's node. This does not create a mapping for it. The
> mapping is only created when changer's "interrupt-child" creates a
> mapping for their interrupt - then changer's alloc() routine calls
> irq_domain_alloc_irqs_parent() in the same way as all other
> hierarchical irqchips do.
> 
> I don't see where double mapping can appear here. Please explain.

Just look at your code. You start probing a device that has an
'interrupts' property. This triggers the allocation of an interrupt.
Then the endpoint also has an 'interrupts' property, also allocating
an interrupt. You then happily override the mapping of the first IRQ
with the second one in the parent irqchip.

That's just broken.

	M.

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

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Nikita Yushchenko]

>>> I also don't see why you model this as the actual device that triggers
>>> the interrupt.
>>
>> Well, that somehow matches the physical reality. In the case of wl18xx
>> on KF, physically the interrupt signal indeed originates from the
>> intermediate chip - the inverting level-shifter.
> 
> Reality? By allowing something like an edge-to-level conversion? How
> can that work?

Edge to level can be problematic, but level to edge does not cause any difficulties, nor in generating 
nor in handling.

>> In contrary, an abstraction of intermediate entity that does a static
>> conversion of the trigger type and does not need any software control,
>> looks sane. And, hardware designers do strange things sometimes, I
>> won't be surprised observing a change from level to edge one day.
> 
> If you think that it can happen without a HW register that latches the
> edge and requires an ack, you need to question your understanding of
> an interrupt life cycle, and of the properties of the various trigger
> types.

There are plenty of devices capable to signal both level and edge interrupts, configurable by a 
register. Basic handling is always the same, and involves masking the interrupt at interrupt controller 
while IRQs disabled, then enabling IRQs, then programming the device to clear the interrupt condition, 
then unmasking the interrupt at interrupt controller. If the trigger type is level or edge, is only 
interesting to interrupt controller driver, but not to a wider scope.

Nothing stops hw designers from doing all sort of strange things with interrupt signals. Right now I 
have a board on my desk where interrupt signals from several chips are wired to inputs of a logical AND 
element and the output of that is wired to SoC's gpio. I don't see what stops them tomorrow from setting 
up their CPLDs to issue a short impulse at output in return to a level change on input. And that will be 
a level to edge converter.

>> In driver's init, I only call of_irq_parse_one() for interrupt defined
>> in the changer's node. This does not create a mapping for it. The
>> mapping is only created when changer's "interrupt-child" creates a
>> mapping for their interrupt - then changer's alloc() routine calls
>> irq_domain_alloc_irqs_parent() in the same way as all other
>> hierarchical irqchips do.
>>
>> I don't see where double mapping can appear here. Please explain.
> 
> Just look at your code. You start probing a device that has an
> 'interrupts' property. This triggers the allocation of an interrupt.

Where does it trigger it?

And what is this "allocation", after all? Is it allocation of virq number? That allocation happens when 
irq_create_fwspec_mapping() calls irq_domain_alloc_irqs(). But this path does not necessary gets 
executed in probing. In the current irq_tyoe_changer driver, it is not.


Nikita

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Marc Zyngier]

On Tue, 25 Jan 2022 09:35:22 +0000,
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> 
> >>> I also don't see why you model this as the actual device that triggers
> >>> the interrupt.
> >> 
> >> Well, that somehow matches the physical reality. In the case of wl18xx
> >> on KF, physically the interrupt signal indeed originates from the
> >> intermediate chip - the inverting level-shifter.
> > 
> > Reality? By allowing something like an edge-to-level conversion? How
> > can that work?
> 
> Edge to level can be problematic, but level to edge does not cause any
> difficulties, nor in generating nor in handling.

Hmmm. A whole industry seems to disagree with you.

> 
> >> In contrary, an abstraction of intermediate entity that does a static
> >> conversion of the trigger type and does not need any software control,
> >> looks sane. And, hardware designers do strange things sometimes, I
> >> won't be surprised observing a change from level to edge one day.
> > 
> > If you think that it can happen without a HW register that latches the
> > edge and requires an ack, you need to question your understanding of
> > an interrupt life cycle, and of the properties of the various trigger
> > types.
> 
> There are plenty of devices capable to signal both level and edge
> interrupts, configurable by a register. Basic handling is always the
> same, and involves masking the interrupt at interrupt controller while
> IRQs disabled, then enabling IRQs, then programming the device to
> clear the interrupt condition, then unmasking the interrupt at
> interrupt controller. If the trigger type is level or edge, is only
> interesting to interrupt controller driver, but not to a wider scope.
> 
> Nothing stops hw designers from doing all sort of strange things with
> interrupt signals. Right now I have a board on my desk where interrupt
> signals from several chips are wired to inputs of a logical AND
> element and the output of that is wired to SoC's gpio. I don't see
> what stops them tomorrow from setting up their CPLDs to issue a short
> impulse at output in return to a level change on input. And that will
> be a level to edge converter.

And a broken one, as you'd have nothing to regenerate an edge if the
input stays high. Such HW can stay in the bin.

> 
> >> In driver's init, I only call of_irq_parse_one() for interrupt defined
> >> in the changer's node. This does not create a mapping for it. The
> >> mapping is only created when changer's "interrupt-child" creates a
> >> mapping for their interrupt - then changer's alloc() routine calls
> >> irq_domain_alloc_irqs_parent() in the same way as all other
> >> hierarchical irqchips do.
> >> 
> >> I don't see where double mapping can appear here. Please explain.
> > 
> > Just look at your code. You start probing a device that has an
> > 'interrupts' property. This triggers the allocation of an interrupt.
> 
> Where does it trigger it?

I'll let you follow the path from of_device_alloc() all the way to the
actual DT parsing of the interrupt specifier, allocation of the
interrupt descriptor and mapping in the parent domain. You then
override the mapping (I really should put a WARN_ON() in the irqdomain
code to catch this sort of things).

> And what is this "allocation", after all? Is it allocation of virq
> number?

And descriptor, and mapping in the domain.

> That allocation happens when irq_create_fwspec_mapping() calls
> irq_domain_alloc_irqs(). But this path does not necessary gets
> executed in probing. In the current irq_tyoe_changer driver, it is
> not.

I don't think you see the full extent of the problem.

	M.

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

Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Nikita Yushchenko]

 > Such HW can stay in the bin.

Huh? Industry is full of ugly hardware (that is still surprisingly useful to cover practical needs at 
low cost - which ensures it will persist). Sure I'd prefer to spend time for something better than 
negotiating workarounds to keep ugly hardware working. But after 20+ years in the industry, I can only 
agree with the statement Linus wrote back in 2006 in [1]:

| For a driver writer, there is one rule above _all_ other rules:
|
|	"Reality sucks, deal with it"
|
| That rule is inviolate, and no amount of "I wish", and "it _should_
| work this way" or "..but the documentation says" matters at all.

[1] https://lore.kernel.org/lkml/Pine.LNX.4.64.0604241156340.3701@g5.osdl.org/


Can we finally stop blaming and propose a practical way to solve the original issue?

If the only solution you are ready to accept is the code that supports inversion only, and uses node's 
"interrupts" property to provide info for interrupt-grand-parent, please state that explicitly.


<skipping all the statements I don't agree with>


 > (I really should put a WARN_ON() in the irqdomain
 > code to catch this sort of things).

I do support this idea.

Is a WARN_ON() in irq_domain_set_mapping() checking that nothing is already assigned to this hwirq in 
this domain good enough to serve this purpose?

I.e. for a linear domain (which shall be enough to catch the wl18xx-on-kf case where both parent domain 
and irq-type-changer's domain are linear):

--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -521,9 +521,10 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
                 return;

         mutex_lock(&domain->revmap_mutex);
-       if (hwirq < domain->revmap_size)
+       if (hwirq < domain->revmap_size) {
+               WARN_ON(domain->revmap[hwirq] != NULL);
                 rcu_assign_pointer(domain->revmap[hwirq], irq_data);
-       else
+       } else
                 radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
         mutex_unlock(&domain->revmap_mutex);
  }

This does not hit on my code.

Could you please suggest a better location for WARN_ON() that will hit?


Nikita