[PATCH V3 0/2] irqchip: add NXP INTMUX interrupt controller

[PATCH V3 0/2] irqchip: add NXP INTMUX interrupt controller

[Joakim Zhang]

Hi Marc,

   Thanks for your kindly reminder:-), I understood a little more. After
adding the .select callback, we can assign an interrupt to one of irq
domains from interrupt specifier. Thanks a lot.

ChangeLogs:
V2->V3:
	*impletement .xlate and .select callback.

V1->V2:
	*squash patches:
		drivers/irqchip: enable INTMUX interrupt controller driver
 		drivers/irqchip: add NXP INTMUX interrupt multiplexer support
	*remove properity "fsl,intmux_chans", only support channel 0 by
	default.
	*delete two unused macros.
	*align the various field in struct intmux_data.
	*turn to spin lock _irqsave version.
	*delete struct intmux_irqchip_data.
	*disable interrupt in probe stage and clear pending status in remove
	stage.

Joakim Zhang (2):
  dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
  drivers/irqchip: add NXP INTMUX interrupt multiplexer support

 .../interrupt-controller/fsl,intmux.txt       |  36 ++
 drivers/irqchip/Kconfig                       |   6 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-imx-intmux.c              | 311 ++++++++++++++++++
 4 files changed, 354 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
 create mode 100644 drivers/irqchip/irq-imx-intmux.c

-- 
2.17.1

[PATCH V3 1/2] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer

[Joakim Zhang]

This patch adds the DT bindings for the NXP INTMUX interrupt multiplexer
for i.MX8 family SoCs.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 .../interrupt-controller/fsl,intmux.txt       | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
new file mode 100644
index 000000000000..3ebe9cac5f20
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
@@ -0,0 +1,36 @@
+Freescale INTMUX interrupt multiplexer
+
+Required properties:
+
+- compatible: Should be:
+   - "fsl,imx-intmux"
+- reg: Physical base address and size of registers.
+- interrupts: Should contain the parent interrupt lines (up to 8) used to
+  multiplex the input interrupts.
+- clocks: Should contain one clock for entry in clock-names.
+- clock-names:
+   - "ipg": main logic clock
+- interrupt-controller: Identifies the node as an interrupt controller.
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value must be 2.
+   - the 1st cell: hardware interrupt number
+   - the 2nd cell: channel index, value must smaller than channels used
+
+Optional properties:
+
+- fsl,intmux_chans: The number of channels used for interrupt source. The
+  Maximum value is 8. If this property is not set in DT then driver uses
+  1 channel by default.
+
+Example:
+
+	intmux@37400000 {
+		compatible = "fsl,imx-intmux";
+		reg = <0x37400000 0x1000>;
+		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clk IMX8QM_CM40_IPG_CLK>;
+		clock-names = "ipg";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+	};
+
-- 
2.17.1

RE: [PATCH V3 1/2] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年12月20日 15:37
> To: maz@kernel.org; tglx@linutronix.de; jason@lakedaemon.net;
> robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; Andy Duan <fugang.duan@nxp.com>;
> linux-arm-kernel@lists.infradead.org; Joakim Zhang
> <qiangqing.zhang@nxp.com>
> Subject: [PATCH V3 1/2] dt-bindings/irq: add binding for NXP INTMUX interrupt
> multiplexer
> 
> This patch adds the DT bindings for the NXP INTMUX interrupt multiplexer for
> i.MX8 family SoCs.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../interrupt-controller/fsl,intmux.txt       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> new file mode 100644
> index 000000000000..3ebe9cac5f20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.
> +++ txt
> @@ -0,0 +1,36 @@
> +Freescale INTMUX interrupt multiplexer
> +
> +Required properties:
> +
> +- compatible: Should be:
> +   - "fsl,imx-intmux"
> +- reg: Physical base address and size of registers.
> +- interrupts: Should contain the parent interrupt lines (up to 8) used
> +to
> +  multiplex the input interrupts.
> +- clocks: Should contain one clock for entry in clock-names.
> +- clock-names:
> +   - "ipg": main logic clock
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> +  interrupt source. The value must be 2.
> +   - the 1st cell: hardware interrupt number
> +   - the 2nd cell: channel index, value must smaller than channels used
> +
> +Optional properties:
> +
> +- fsl,intmux_chans: The number of channels used for interrupt source.
> +The
> +  Maximum value is 8. If this property is not set in DT then driver
> +uses
> +  1 channel by default.
> +
> +Example:
> +
> +	intmux@37400000 {
> +		compatible = "fsl,imx-intmux";
> +		reg = <0x37400000 0x1000>;
> +		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clk IMX8QM_CM40_IPG_CLK>;
> +		clock-names = "ipg";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
Hi Marc,

I am so sorry, this should be #interrupt-cells = <2>, will correct it in next version.

Best Regards,
Joakim Zhang
> +	};
> +
> --
> 2.17.1

Re: [PATCH V3 1/2] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer

[Lokesh Vutla]

On 20/12/19 1:07 PM, Joakim Zhang wrote:
> This patch adds the DT bindings for the NXP INTMUX interrupt multiplexer
> for i.MX8 family SoCs.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../interrupt-controller/fsl,intmux.txt       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> new file mode 100644
> index 000000000000..3ebe9cac5f20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> @@ -0,0 +1,36 @@
> +Freescale INTMUX interrupt multiplexer
> +
> +Required properties:
> +
> +- compatible: Should be:
> +   - "fsl,imx-intmux"
> +- reg: Physical base address and size of registers.
> +- interrupts: Should contain the parent interrupt lines (up to 8) used to
> +  multiplex the input interrupts.
> +- clocks: Should contain one clock for entry in clock-names.
> +- clock-names:
> +   - "ipg": main logic clock
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> +  interrupt source. The value must be 2.
> +   - the 1st cell: hardware interrupt number> +   - the 2nd cell: channel index, value must smaller than channels used

As per the xlate function, 1st cell is channel index and 2nd cell is hw
interrupt number no?

Thanks and regards,
Lokesh

> +
> +Optional properties:
> +
> +- fsl,intmux_chans: The number of channels used for interrupt source. The
> +  Maximum value is 8. If this property is not set in DT then driver uses
> +  1 channel by default.
> +
> +Example:
> +
> +	intmux@37400000 {
> +		compatible = "fsl,imx-intmux";
> +		reg = <0x37400000 0x1000>;
> +		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clk IMX8QM_CM40_IPG_CLK>;
> +		clock-names = "ipg";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +	};
> +
> 

Re: [PATCH V3 1/2] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer

[Lokesh Vutla]

On 20/12/19 5:10 PM, Lokesh Vutla wrote:
> 
> 
> On 20/12/19 1:07 PM, Joakim Zhang wrote:
>> This patch adds the DT bindings for the NXP INTMUX interrupt multiplexer
>> for i.MX8 family SoCs.
>>
>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> ---
>>  .../interrupt-controller/fsl,intmux.txt       | 36 +++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
>> new file mode 100644
>> index 000000000000..3ebe9cac5f20
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
>> @@ -0,0 +1,36 @@
>> +Freescale INTMUX interrupt multiplexer
>> +
>> +Required properties:
>> +
>> +- compatible: Should be:
>> +   - "fsl,imx-intmux"
>> +- reg: Physical base address and size of registers.
>> +- interrupts: Should contain the parent interrupt lines (up to 8) used to
>> +  multiplex the input interrupts.
>> +- clocks: Should contain one clock for entry in clock-names.
>> +- clock-names:
>> +   - "ipg": main logic clock
>> +- interrupt-controller: Identifies the node as an interrupt controller.
>> +- #interrupt-cells: Specifies the number of cells needed to encode an
>> +  interrupt source. The value must be 2.
>> +   - the 1st cell: hardware interrupt number> +   - the 2nd cell: channel index, value must smaller than channels used
> 
> As per the xlate function, 1st cell is channel index and 2nd cell is hw
> interrupt number no?

Sorry my bad, I read it wrong. Ignore this comment.

Thanks and regards,
Lokesh

> 
> Thanks and regards,
> Lokesh
> 
>> +
>> +Optional properties:
>> +
>> +- fsl,intmux_chans: The number of channels used for interrupt source. The
>> +  Maximum value is 8. If this property is not set in DT then driver uses
>> +  1 channel by default.
>> +
>> +Example:
>> +
>> +	intmux@37400000 {
>> +		compatible = "fsl,imx-intmux";
>> +		reg = <0x37400000 0x1000>;
>> +		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&clk IMX8QM_CM40_IPG_CLK>;
>> +		clock-names = "ipg";
>> +		interrupt-controller;
>> +		#interrupt-cells = <1>;
>> +	};
>> +
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

The Interrupt Multiplexer (INTMUX) expands the number of peripherals
that can interrupt the core:
* The INTMUX has 8 channels that are assigned to 8 NVIC interrupt slots.
* Each INTMUX channel can receive up to 32 interrupt sources and has 1
  interrupt output.
* The INTMUX routes the interrupt sources to the interrupt outputs.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/irqchip/Kconfig          |   6 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-imx-intmux.c | 311 +++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-intmux.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index ba152954324b..7e2b1e9d0b45 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -457,6 +457,12 @@ config IMX_IRQSTEER
 	help
 	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
 
+config IMX_INTMUX
+	def_bool y if ARCH_MXC
+	select IRQ_DOMAIN
+	help
+	  Support for the i.MX INTMUX interrupt multiplexer.
+
 config LS1X_IRQ
 	bool "Loongson-1 Interrupt Controller"
 	depends on MACH_LOONGSON32
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e806dda690ea..af976a79d1fb 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
+obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
 obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
 obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
new file mode 100644
index 000000000000..94c67fdd7163
--- /dev/null
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2017 NXP
+
+/*                     INTMUX Block Diagram
+ *
+ *                               ________________
+ * interrupt source #  0  +---->|                |
+ *                        |     |                |
+ * interrupt source #  1  +++-->|                |
+ *            ...         | |   |   channel # 0  |--------->interrupt out # 0
+ *            ...         | |   |                |
+ *            ...         | |   |                |
+ * interrupt source # X-1 +++-->|________________|
+ *                        | | |
+ *                        | | |
+ *                        | | |  ________________
+ *                        +---->|                |
+ *                        | | | |                |
+ *                        | +-->|                |
+ *                        | | | |   channel # 1  |--------->interrupt out # 1
+ *                        | | +>|                |
+ *                        | | | |                |
+ *                        | | | |________________|
+ *                        | | |
+ *                        | | |
+ *                        | | |       ...
+ *                        | | |       ...
+ *                        | | |
+ *                        | | |  ________________
+ *                        +---->|                |
+ *                          | | |                |
+ *                          +-->|                |
+ *                            | |   channel # N  |--------->interrupt out # N
+ *                            +>|                |
+ *                              |                |
+ *                              |________________|
+ *
+ *
+ * N: Interrupt Channel Instance Number (N=7)
+ * X: Interrupt Source Number for each channel (X=32)
+ *
+ * The INTMUX interrupt multiplexer has 8 channels, each channel receives 32
+ * interrupt sources and generates 1 interrupt output.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+
+#define CHANIER(n)	(0x10 + (0x40 * n))
+#define CHANIPR(n)	(0x20 + (0x40 * n))
+
+#define CHAN_MAX_NUM		0x8
+
+struct intmux_irqchip_data {
+	int			chanidx;
+	int			irq;
+	struct irq_domain	*domain;
+};
+
+struct intmux_data {
+	raw_spinlock_t			lock;
+	void __iomem			*regs;
+	struct clk			*ipg_clk;
+	int				channum;
+	struct intmux_irqchip_data	irqchip_data[];
+};
+
+static void imx_intmux_irq_mask(struct irq_data *d)
+{
+	struct intmux_irqchip_data *irqchip_data = d->chip_data;
+	int idx = irqchip_data->chanidx;
+	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
+						irqchip_data[idx]);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 val;
+
+	raw_spin_lock_irqsave(&data->lock, flags);
+	reg = data->regs + CHANIER(idx);
+	val = readl_relaxed(reg);
+	/* disable the interrupt source of this channel */
+	val &= ~BIT(d->hwirq);
+	writel_relaxed(val, reg);
+	raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void imx_intmux_irq_unmask(struct irq_data *d)
+{
+	struct intmux_irqchip_data *irqchip_data = d->chip_data;
+	int idx = irqchip_data->chanidx;
+	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
+						irqchip_data[idx]);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 val;
+
+	raw_spin_lock_irqsave(&data->lock, flags);
+	reg = data->regs + CHANIER(idx);
+	val = readl_relaxed(reg);
+	/* enable the interrupt source of this channel */
+	val |= BIT(d->hwirq);
+	writel_relaxed(val, reg);
+	raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static struct irq_chip imx_intmux_irq_chip = {
+	.name		= "intmux",
+	.irq_mask	= imx_intmux_irq_mask,
+	.irq_unmask	= imx_intmux_irq_unmask,
+};
+
+static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	irq_set_status_flags(irq, IRQ_LEVEL);
+	irq_set_chip_data(irq, h->host_data);
+	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
+
+	return 0;
+}
+
+static int imx_intmux_irq_xlate(struct irq_domain *d, struct device_node *node,
+				const u32 *intspec, unsigned int intsize,
+				unsigned long *out_hwirq, unsigned int *out_type)
+{
+	struct intmux_irqchip_data *irqchip_data = d->host_data;
+	int idx = irqchip_data->chanidx;
+	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
+						irqchip_data[idx]);
+
+	/* two cells needed in interrupt specifier:
+	 * the 1st cell: hw interrupt number
+	 * the 2nd cell: channel index
+	 */
+	if (WARN_ON(intsize != 2))
+		return -EINVAL;
+
+	if (WARN_ON(intspec[1] >= data->channum))
+		return -EINVAL;
+
+	*out_hwirq = intspec[0];
+	*out_type = IRQ_TYPE_NONE;
+
+	return 0;
+}
+
+static int imx_intmux_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+				 enum irq_domain_bus_token bus_token)
+{
+	struct intmux_irqchip_data *irqchip_data = d->host_data;
+
+	/* Not for us */
+	if (fwspec->fwnode != d->fwnode)
+		return false;
+
+	if (irqchip_data->chanidx == fwspec->param[1])
+		return true;
+	else
+		return false;
+}
+
+static const struct irq_domain_ops imx_intmux_domain_ops = {
+	.map		= imx_intmux_irq_map,
+	.xlate		= imx_intmux_irq_xlate,
+	.select		= imx_intmux_irq_select,
+};
+
+static void imx_intmux_irq_handler(struct irq_desc *desc)
+{
+	struct intmux_irqchip_data *irqchip_data = irq_desc_get_handler_data(desc);
+	int idx = irqchip_data->chanidx;
+	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
+						irqchip_data[idx]);
+	unsigned long irqstat;
+	int pos, virq;
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+
+	/* read the interrupt source pending status of this channel */
+	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
+
+	for_each_set_bit(pos, &irqstat, 32) {
+		virq = irq_find_mapping(irqchip_data->domain, pos);
+		if (virq)
+			generic_handle_irq(virq);
+	}
+
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_intmux_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct intmux_data *data;
+	int channum;
+	int i, ret;
+
+	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
+	if (ret) {
+		channum = 1;
+	} else if (channum > CHAN_MAX_NUM) {
+		dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
+			CHAN_MAX_NUM);
+		return -EINVAL;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data) +
+			    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->regs)) {
+		dev_err(&pdev->dev, "failed to initialize reg\n");
+		return PTR_ERR(data->regs);
+	}
+
+	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(data->ipg_clk)) {
+		ret = PTR_ERR(data->ipg_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
+		return ret;
+	}
+
+	data->channum = channum;
+	raw_spin_lock_init(&data->lock);
+
+	ret = clk_prepare_enable(data->ipg_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < channum; i++) {
+		data->irqchip_data[i].chanidx = i;
+
+		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
+		if (data->irqchip_data[i].irq <= 0) {
+			ret = -EINVAL;
+			dev_err(&pdev->dev, "failed to get irq\n");
+			goto out;
+		}
+
+		data->irqchip_data[i].domain =
+			irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
+					      &data->irqchip_data[i]);
+		if (!data->irqchip_data[i].domain) {
+			ret = -ENOMEM;
+			dev_err(&pdev->dev, "failed to create IRQ domain\n");
+			goto out;
+		}
+
+		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
+						 imx_intmux_irq_handler,
+						 &data->irqchip_data[i]);
+
+		/* disable interrupt sources of this channel firstly */
+		writel_relaxed(0, data->regs + CHANIER(i));
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+out:
+	clk_disable_unprepare(data->ipg_clk);
+	return ret;
+}
+
+static int imx_intmux_remove(struct platform_device *pdev)
+{
+	struct intmux_data *data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < data->channum; i++) {
+		/* clear interrupt sources pending status of this channel */
+		writel_relaxed(0, data->regs + CHANIPR(i));
+
+		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
+						 NULL, NULL);
+
+		irq_domain_remove(data->irqchip_data[i].domain);
+	}
+
+	clk_disable_unprepare(data->ipg_clk);
+
+	return 0;
+}
+
+static const struct of_device_id imx_intmux_id[] = {
+	{ .compatible = "fsl,imx-intmux", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver imx_intmux_driver = {
+	.driver = {
+		.name = "imx-intmux",
+		.of_match_table = imx_intmux_id,
+	},
+	.probe = imx_intmux_probe,
+	.remove = imx_intmux_remove,
+};
+builtin_platform_driver(imx_intmux_driver);
-- 
2.17.1

Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Marc Zyngier]

On 2019-12-20 07:37, Joakim Zhang wrote:
> The Interrupt Multiplexer (INTMUX) expands the number of peripherals
> that can interrupt the core:
> * The INTMUX has 8 channels that are assigned to 8 NVIC interrupt 
> slots.
> * Each INTMUX channel can receive up to 32 interrupt sources and has 
> 1
>   interrupt output.
> * The INTMUX routes the interrupt sources to the interrupt outputs.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/Kconfig          |   6 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-intmux.c | 311 
> +++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-intmux.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ba152954324b..7e2b1e9d0b45 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -457,6 +457,12 @@ config IMX_IRQSTEER
>  	help
>  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
>
> +config IMX_INTMUX
> +	def_bool y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the i.MX INTMUX interrupt multiplexer.
> +
>  config LS1X_IRQ
>  	bool "Loongson-1 Interrupt Controller"
>  	depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e806dda690ea..af976a79d1fb 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-imx-intmux.c
> b/drivers/irqchip/irq-imx-intmux.c
> new file mode 100644
> index 000000000000..94c67fdd7163
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2017 NXP
> +
> +/*                     INTMUX Block Diagram
> + *
> + *                               ________________
> + * interrupt source #  0  +---->|                |
> + *                        |     |                |
> + * interrupt source #  1  +++-->|                |
> + *            ...         | |   |   channel # 0
> |--------->interrupt out # 0
> + *            ...         | |   |                |
> + *            ...         | |   |                |
> + * interrupt source # X-1 +++-->|________________|
> + *                        | | |
> + *                        | | |
> + *                        | | |  ________________
> + *                        +---->|                |
> + *                        | | | |                |
> + *                        | +-->|                |
> + *                        | | | |   channel # 1
> |--------->interrupt out # 1
> + *                        | | +>|                |
> + *                        | | | |                |
> + *                        | | | |________________|
> + *                        | | |
> + *                        | | |
> + *                        | | |       ...
> + *                        | | |       ...
> + *                        | | |
> + *                        | | |  ________________
> + *                        +---->|                |
> + *                          | | |                |
> + *                          +-->|                |
> + *                            | |   channel # N
> |--------->interrupt out # N
> + *                            +>|                |
> + *                              |                |
> + *                              |________________|
> + *
> + *
> + * N: Interrupt Channel Instance Number (N=7)
> + * X: Interrupt Source Number for each channel (X=32)
> + *
> + * The INTMUX interrupt multiplexer has 8 channels, each channel 
> receives 32
> + * interrupt sources and generates 1 interrupt output.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CHANIER(n)	(0x10 + (0x40 * n))
> +#define CHANIPR(n)	(0x20 + (0x40 * n))
> +
> +#define CHAN_MAX_NUM		0x8
> +
> +struct intmux_irqchip_data {
> +	int			chanidx;
> +	int			irq;
> +	struct irq_domain	*domain;
> +};
> +
> +struct intmux_data {
> +	raw_spinlock_t			lock;
> +	void __iomem			*regs;
> +	struct clk			*ipg_clk;
> +	int				channum;
> +	struct intmux_irqchip_data	irqchip_data[];
> +};
> +
> +static void imx_intmux_irq_mask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct 
> intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long flags;
> +	void __iomem *reg;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	reg = data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* disable the interrupt source of this channel */
> +	val &= ~BIT(d->hwirq);
> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static void imx_intmux_irq_unmask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct 
> intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long flags;
> +	void __iomem *reg;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	reg = data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* enable the interrupt source of this channel */
> +	val |= BIT(d->hwirq);
> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static struct irq_chip imx_intmux_irq_chip = {
> +	.name		= "intmux",
> +	.irq_mask	= imx_intmux_irq_mask,
> +	.irq_unmask	= imx_intmux_irq_unmask,
> +};
> +
> +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int 
> irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_status_flags(irq, IRQ_LEVEL);

You shouldn't need to do this if you had it right in the xlate 
callback,
see below.

> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, 
> handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static int imx_intmux_irq_xlate(struct irq_domain *d, struct
> device_node *node,
> +				const u32 *intspec, unsigned int intsize,
> +				unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct 
> intmux_data,
> +						irqchip_data[idx]);
> +
> +	/* two cells needed in interrupt specifier:
> +	 * the 1st cell: hw interrupt number
> +	 * the 2nd cell: channel index
> +	 */

The comment format is:

         /*
          * comments
          */

> +	if (WARN_ON(intsize != 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(intspec[1] >= data->channum))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = IRQ_TYPE_NONE;

Why NONE? Your interrupts are all level, if I trust the map function.

> +
> +	return 0;
> +}
> +
> +static int imx_intmux_irq_select(struct irq_domain *d, struct
> irq_fwspec *fwspec,
> +				 enum irq_domain_bus_token bus_token)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> +
> +	/* Not for us */
> +	if (fwspec->fwnode != d->fwnode)
> +		return false;
> +
> +	if (irqchip_data->chanidx == fwspec->param[1])
> +		return true;
> +	else
> +		return false;


This is trivially simplified as

         return irqchip_data->chanidx == fwspec->param[1];

> +}
> +
> +static const struct irq_domain_ops imx_intmux_domain_ops = {
> +	.map		= imx_intmux_irq_map,
> +	.xlate		= imx_intmux_irq_xlate,
> +	.select		= imx_intmux_irq_select,
> +};
> +
> +static void imx_intmux_irq_handler(struct irq_desc *desc)
> +{
> +	struct intmux_irqchip_data *irqchip_data = 
> irq_desc_get_handler_data(desc);
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct 
> intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long irqstat;
> +	int pos, virq;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	/* read the interrupt source pending status of this channel */
> +	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
> +
> +	for_each_set_bit(pos, &irqstat, 32) {
> +		virq = irq_find_mapping(irqchip_data->domain, pos);
> +		if (virq)
> +			generic_handle_irq(virq);
> +	}
> +
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +static int imx_intmux_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct intmux_data *data;
> +	int channum;
> +	int i, ret;
> +
> +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> +	if (ret) {
> +		channum = 1;
> +	} else if (channum > CHAN_MAX_NUM) {
> +		dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
> +			CHAN_MAX_NUM);
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data) +
> +			    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize reg\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(data->ipg_clk)) {
> +		ret = PTR_ERR(data->ipg_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	data->channum = channum;
> +	raw_spin_lock_init(&data->lock);
> +
> +	ret = clk_prepare_enable(data->ipg_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < channum; i++) {
> +		data->irqchip_data[i].chanidx = i;
> +
> +		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
> +		if (data->irqchip_data[i].irq <= 0) {
> +			ret = -EINVAL;
> +			dev_err(&pdev->dev, "failed to get irq\n");
> +			goto out;
> +		}
> +
> +		data->irqchip_data[i].domain =
> +			irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
> +					      &data->irqchip_data[i]);

Same problem as before. If the line is too long, use an intermediate
variable. Or leave the assignment as a single long line. Don't split
assignment like this.

> +		if (!data->irqchip_data[i].domain) {
> +			ret = -ENOMEM;
> +			dev_err(&pdev->dev, "failed to create IRQ domain\n");
> +			goto out;
> +		}
> +
> +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> +						 imx_intmux_irq_handler,
> +						 &data->irqchip_data[i]);
> +
> +		/* disable interrupt sources of this channel firstly */
> +		writel_relaxed(0, data->regs + CHANIER(i));

So you disable interrupts *after* enabling the mux interrupt. If you
have screaming interrupts, this will lockup. You really need to do
it *before* enabling the chained interrupt.

> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +out:
> +	clk_disable_unprepare(data->ipg_clk);
> +	return ret;
> +}
> +
> +static int imx_intmux_remove(struct platform_device *pdev)
> +{
> +	struct intmux_data *data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < data->channum; i++) {
> +		/* clear interrupt sources pending status of this channel */
> +		writel_relaxed(0, data->regs + CHANIPR(i));

If these are level interrupts, this won't do a thing. You need to
actively *disable* them.

> +
> +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> +						 NULL, NULL);
> +
> +		irq_domain_remove(data->irqchip_data[i].domain);
> +	}
> +
> +	clk_disable_unprepare(data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_intmux_id[] = {
> +	{ .compatible = "fsl,imx-intmux", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver imx_intmux_driver = {
> +	.driver = {
> +		.name = "imx-intmux",
> +		.of_match_table = imx_intmux_id,
> +	},
> +	.probe = imx_intmux_probe,
> +	.remove = imx_intmux_remove,
> +};
> +builtin_platform_driver(imx_intmux_driver);

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月20日 18:49
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; Andy Duan <fugang.duan@nxp.com>;
> linux-arm-kernel@lists.infradead.org; S.j. Wang <shengjiu.wang@nxp.com>
> Subject: Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> On 2019-12-20 07:37, Joakim Zhang wrote:
> > The Interrupt Multiplexer (INTMUX) expands the number of peripherals
> > that can interrupt the core:
> > * The INTMUX has 8 channels that are assigned to 8 NVIC interrupt
> > slots.
> > * Each INTMUX channel can receive up to 32 interrupt sources and has
> > 1
> >   interrupt output.
> > * The INTMUX routes the interrupt sources to the interrupt outputs.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig          |   6 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-intmux.c | 311
> > +++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-intmux.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > ba152954324b..7e2b1e9d0b45 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -457,6 +457,12 @@ config IMX_IRQSTEER
> >  	help
> >  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
> >
> > +config IMX_INTMUX
> > +	def_bool y if ARCH_MXC
> > +	select IRQ_DOMAIN
> > +	help
> > +	  Support for the i.MX INTMUX interrupt multiplexer.
> > +
> >  config LS1X_IRQ
> >  	bool "Loongson-1 Interrupt Controller"
> >  	depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > e806dda690ea..af976a79d1fb 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+=
> irq-csky-mpintc.o
> >  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> > +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
> >  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > new file mode 100644
> > index 000000000000..94c67fdd7163
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -0,0 +1,311 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2017 NXP
> > +
> > +/*                     INTMUX Block Diagram
> > + *
> > + *                               ________________
> > + * interrupt source #  0  +---->|                |
> > + *                        |     |                |
> > + * interrupt source #  1  +++-->|                |
> > + *            ...         | |   |   channel # 0
> > |--------->interrupt out # 0
> > + *            ...         | |   |                |
> > + *            ...         | |   |                |
> > + * interrupt source # X-1 +++-->|________________|
> > + *                        | | |
> > + *                        | | |
> > + *                        | | |  ________________
> > + *                        +---->|                |
> > + *                        | | | |                |
> > + *                        | +-->|                |
> > + *                        | | | |   channel # 1
> > |--------->interrupt out # 1
> > + *                        | | +>|                |
> > + *                        | | | |                |
> > + *                        | | | |________________|
> > + *                        | | |
> > + *                        | | |
> > + *                        | | |       ...
> > + *                        | | |       ...
> > + *                        | | |
> > + *                        | | |  ________________
> > + *                        +---->|                |
> > + *                          | | |                |
> > + *                          +-->|                |
> > + *                            | |   channel # N
> > |--------->interrupt out # N
> > + *                            +>|                |
> > + *                              |                |
> > + *                              |________________|
> > + *
> > + *
> > + * N: Interrupt Channel Instance Number (N=7)
> > + * X: Interrupt Source Number for each channel (X=32)
> > + *
> > + * The INTMUX interrupt multiplexer has 8 channels, each channel
> > receives 32
> > + * interrupt sources and generates 1 interrupt output.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/kernel.h> #include <linux/of_irq.h> #include
> > +<linux/of_platform.h> #include <linux/spinlock.h>
> > +
> > +#define CHANIER(n)	(0x10 + (0x40 * n))
> > +#define CHANIPR(n)	(0x20 + (0x40 * n))
> > +
> > +#define CHAN_MAX_NUM		0x8
> > +
> > +struct intmux_irqchip_data {
> > +	int			chanidx;
> > +	int			irq;
> > +	struct irq_domain	*domain;
> > +};
> > +
> > +struct intmux_data {
> > +	raw_spinlock_t			lock;
> > +	void __iomem			*regs;
> > +	struct clk			*ipg_clk;
> > +	int				channum;
> > +	struct intmux_irqchip_data	irqchip_data[];
> > +};
> > +
> > +static void imx_intmux_irq_mask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> > intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long flags;
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&data->lock, flags);
> > +	reg = data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* disable the interrupt source of this channel */
> > +	val &= ~BIT(d->hwirq);
> > +	writel_relaxed(val, reg);
> > +	raw_spin_unlock_irqrestore(&data->lock, flags); }
> > +
> > +static void imx_intmux_irq_unmask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> > intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long flags;
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&data->lock, flags);
> > +	reg = data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* enable the interrupt source of this channel */
> > +	val |= BIT(d->hwirq);
> > +	writel_relaxed(val, reg);
> > +	raw_spin_unlock_irqrestore(&data->lock, flags); }
> > +
> > +static struct irq_chip imx_intmux_irq_chip = {
> > +	.name		= "intmux",
> > +	.irq_mask	= imx_intmux_irq_mask,
> > +	.irq_unmask	= imx_intmux_irq_unmask,
> > +};
> > +
> > +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int
> > irq,
> > +			      irq_hw_number_t hwirq)
> > +{
> > +	irq_set_status_flags(irq, IRQ_LEVEL);
> 
> You shouldn't need to do this if you had it right in the xlate callback, see below.
Yes.

> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip,
> > handle_level_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_intmux_irq_xlate(struct irq_domain *d, struct
> > device_node *node,
> > +				const u32 *intspec, unsigned int intsize,
> > +				unsigned long *out_hwirq, unsigned int *out_type) {
> > +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> > intmux_data,
> > +						irqchip_data[idx]);
> > +
> > +	/* two cells needed in interrupt specifier:
> > +	 * the 1st cell: hw interrupt number
> > +	 * the 2nd cell: channel index
> > +	 */
> 
> The comment format is:
> 
>          /*
>           * comments
>           */
Go it.

> > +	if (WARN_ON(intsize != 2))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(intspec[1] >= data->channum))
> > +		return -EINVAL;
> > +
> > +	*out_hwirq = intspec[0];
> > +	*out_type = IRQ_TYPE_NONE;
> 
> Why NONE? Your interrupts are all level, if I trust the map function.
It is all level, I will correct it.

> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_intmux_irq_select(struct irq_domain *d, struct
> > irq_fwspec *fwspec,
> > +				 enum irq_domain_bus_token bus_token) {
> > +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> > +
> > +	/* Not for us */
> > +	if (fwspec->fwnode != d->fwnode)
> > +		return false;
> > +
> > +	if (irqchip_data->chanidx == fwspec->param[1])
> > +		return true;
> > +	else
> > +		return false;
> 
> 
> This is trivially simplified as
> 
>          return irqchip_data->chanidx == fwspec->param[1];
Got it.

> > +}
> > +
> > +static const struct irq_domain_ops imx_intmux_domain_ops = {
> > +	.map		= imx_intmux_irq_map,
> > +	.xlate		= imx_intmux_irq_xlate,
> > +	.select		= imx_intmux_irq_select,
> > +};
> > +
> > +static void imx_intmux_irq_handler(struct irq_desc *desc) {
> > +	struct intmux_irqchip_data *irqchip_data =
> > irq_desc_get_handler_data(desc);
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> > intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long irqstat;
> > +	int pos, virq;
> > +
> > +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +
> > +	/* read the interrupt source pending status of this channel */
> > +	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
> > +
> > +	for_each_set_bit(pos, &irqstat, 32) {
> > +		virq = irq_find_mapping(irqchip_data->domain, pos);
> > +		if (virq)
> > +			generic_handle_irq(virq);
> > +	}
> > +
> > +	chained_irq_exit(irq_desc_get_chip(desc), desc); }
> > +
> > +static int imx_intmux_probe(struct platform_device *pdev) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct intmux_data *data;
> > +	int channum;
> > +	int i, ret;
> > +
> > +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> > +	if (ret) {
> > +		channum = 1;
> > +	} else if (channum > CHAN_MAX_NUM) {
> > +		dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
> > +			CHAN_MAX_NUM);
> > +		return -EINVAL;
> > +	}
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data) +
> > +			    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(data->regs)) {
> > +		dev_err(&pdev->dev, "failed to initialize reg\n");
> > +		return PTR_ERR(data->regs);
> > +	}
> > +
> > +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > +	if (IS_ERR(data->ipg_clk)) {
> > +		ret = PTR_ERR(data->ipg_clk);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	data->channum = channum;
> > +	raw_spin_lock_init(&data->lock);
> > +
> > +	ret = clk_prepare_enable(data->ipg_clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < channum; i++) {
> > +		data->irqchip_data[i].chanidx = i;
> > +
> > +		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
> > +		if (data->irqchip_data[i].irq <= 0) {
> > +			ret = -EINVAL;
> > +			dev_err(&pdev->dev, "failed to get irq\n");
> > +			goto out;
> > +		}
> > +
> > +		data->irqchip_data[i].domain =
> > +			irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
> > +					      &data->irqchip_data[i]);
> 
> Same problem as before. If the line is too long, use an intermediate variable. Or
> leave the assignment as a single long line. Don't split assignment like this.
Got it.

> > +		if (!data->irqchip_data[i].domain) {
> > +			ret = -ENOMEM;
> > +			dev_err(&pdev->dev, "failed to create IRQ domain\n");
> > +			goto out;
> > +		}
> > +
> > +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> > +						 imx_intmux_irq_handler,
> > +						 &data->irqchip_data[i]);
> > +
> > +		/* disable interrupt sources of this channel firstly */
> > +		writel_relaxed(0, data->regs + CHANIER(i));
> 
> So you disable interrupts *after* enabling the mux interrupt. If you have
> screaming interrupts, this will lockup. You really need to do it *before* enabling
> the chained interrupt.
Make sense. Thanks.

> > +	}
> > +
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	return 0;
> > +out:
> > +	clk_disable_unprepare(data->ipg_clk);
> > +	return ret;
> > +}
> > +
> > +static int imx_intmux_remove(struct platform_device *pdev) {
> > +	struct intmux_data *data = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < data->channum; i++) {
> > +		/* clear interrupt sources pending status of this channel */
> > +		writel_relaxed(0, data->regs + CHANIPR(i));
> 
> If these are level interrupts, this won't do a thing. You need to actively
> *disable* them.
Yes.

Thanks for your kindly review, Marc :-)

Best Regards,
Joakim Zhang
> > +
> > +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> > +						 NULL, NULL);
> > +
> > +		irq_domain_remove(data->irqchip_data[i].domain);
> > +	}
> > +
> > +	clk_disable_unprepare(data->ipg_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx_intmux_id[] = {
> > +	{ .compatible = "fsl,imx-intmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static struct platform_driver imx_intmux_driver = {
> > +	.driver = {
> > +		.name = "imx-intmux",
> > +		.of_match_table = imx_intmux_id,
> > +	},
> > +	.probe = imx_intmux_probe,
> > +	.remove = imx_intmux_remove,
> > +};
> > +builtin_platform_driver(imx_intmux_driver);
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Lokesh Vutla]

On 20/12/19 1:07 PM, Joakim Zhang wrote:
> The Interrupt Multiplexer (INTMUX) expands the number of peripherals
> that can interrupt the core:
> * The INTMUX has 8 channels that are assigned to 8 NVIC interrupt slots.
> * Each INTMUX channel can receive up to 32 interrupt sources and has 1
>   interrupt output.
> * The INTMUX routes the interrupt sources to the interrupt outputs.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/Kconfig          |   6 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-intmux.c | 311 +++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-intmux.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ba152954324b..7e2b1e9d0b45 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -457,6 +457,12 @@ config IMX_IRQSTEER
>  	help
>  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
>  
> +config IMX_INTMUX
> +	def_bool y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the i.MX INTMUX interrupt multiplexer.
> +
>  config LS1X_IRQ
>  	bool "Loongson-1 Interrupt Controller"
>  	depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e806dda690ea..af976a79d1fb 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
> new file mode 100644
> index 000000000000..94c67fdd7163
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2017 NXP
> +
> +/*                     INTMUX Block Diagram
> + *
> + *                               ________________
> + * interrupt source #  0  +---->|                |
> + *                        |     |                |
> + * interrupt source #  1  +++-->|                |
> + *            ...         | |   |   channel # 0  |--------->interrupt out # 0
> + *            ...         | |   |                |
> + *            ...         | |   |                |
> + * interrupt source # X-1 +++-->|________________|
> + *                        | | |
> + *                        | | |
> + *                        | | |  ________________
> + *                        +---->|                |
> + *                        | | | |                |
> + *                        | +-->|                |
> + *                        | | | |   channel # 1  |--------->interrupt out # 1
> + *                        | | +>|                |
> + *                        | | | |                |
> + *                        | | | |________________|
> + *                        | | |
> + *                        | | |
> + *                        | | |       ...
> + *                        | | |       ...
> + *                        | | |
> + *                        | | |  ________________
> + *                        +---->|                |
> + *                          | | |                |
> + *                          +-->|                |
> + *                            | |   channel # N  |--------->interrupt out # N
> + *                            +>|                |
> + *                              |                |
> + *                              |________________|
> + *
> + *
> + * N: Interrupt Channel Instance Number (N=7)
> + * X: Interrupt Source Number for each channel (X=32)
> + *
> + * The INTMUX interrupt multiplexer has 8 channels, each channel receives 32
> + * interrupt sources and generates 1 interrupt output.

Does the user care to which channel does the interrupt source goes to? If not,
interrupt-cells in DT can just be a single entry and the channel selection can
be controlled by the driver no? I am trying to understand why user should
specify the channel no.

Thanks and regards,
Lokesh

> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CHANIER(n)	(0x10 + (0x40 * n))
> +#define CHANIPR(n)	(0x20 + (0x40 * n))
> +
> +#define CHAN_MAX_NUM		0x8
> +
> +struct intmux_irqchip_data {
> +	int			chanidx;
> +	int			irq;
> +	struct irq_domain	*domain;
> +};
> +
> +struct intmux_data {
> +	raw_spinlock_t			lock;
> +	void __iomem			*regs;
> +	struct clk			*ipg_clk;
> +	int				channum;
> +	struct intmux_irqchip_data	irqchip_data[];
> +};
> +
> +static void imx_intmux_irq_mask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long flags;
> +	void __iomem *reg;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	reg = data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* disable the interrupt source of this channel */
> +	val &= ~BIT(d->hwirq);
> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static void imx_intmux_irq_unmask(struct irq_data *d)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long flags;
> +	void __iomem *reg;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	reg = data->regs + CHANIER(idx);
> +	val = readl_relaxed(reg);
> +	/* enable the interrupt source of this channel */
> +	val |= BIT(d->hwirq);
> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static struct irq_chip imx_intmux_irq_chip = {
> +	.name		= "intmux",
> +	.irq_mask	= imx_intmux_irq_mask,
> +	.irq_unmask	= imx_intmux_irq_unmask,
> +};
> +
> +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static int imx_intmux_irq_xlate(struct irq_domain *d, struct device_node *node,
> +				const u32 *intspec, unsigned int intsize,
> +				unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
> +						irqchip_data[idx]);
> +
> +	/* two cells needed in interrupt specifier:
> +	 * the 1st cell: hw interrupt number
> +	 * the 2nd cell: channel index
> +	 */
> +	if (WARN_ON(intsize != 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(intspec[1] >= data->channum))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = IRQ_TYPE_NONE;
> +
> +	return 0;
> +}
> +
> +static int imx_intmux_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> +				 enum irq_domain_bus_token bus_token)
> +{
> +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> +
> +	/* Not for us */
> +	if (fwspec->fwnode != d->fwnode)
> +		return false;
> +
> +	if (irqchip_data->chanidx == fwspec->param[1])
> +		return true;
> +	else
> +		return false;
> +}
> +
> +static const struct irq_domain_ops imx_intmux_domain_ops = {
> +	.map		= imx_intmux_irq_map,
> +	.xlate		= imx_intmux_irq_xlate,
> +	.select		= imx_intmux_irq_select,
> +};
> +
> +static void imx_intmux_irq_handler(struct irq_desc *desc)
> +{
> +	struct intmux_irqchip_data *irqchip_data = irq_desc_get_handler_data(desc);
> +	int idx = irqchip_data->chanidx;
> +	struct intmux_data *data = container_of(irqchip_data, struct intmux_data,
> +						irqchip_data[idx]);
> +	unsigned long irqstat;
> +	int pos, virq;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	/* read the interrupt source pending status of this channel */
> +	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
> +
> +	for_each_set_bit(pos, &irqstat, 32) {
> +		virq = irq_find_mapping(irqchip_data->domain, pos);
> +		if (virq)
> +			generic_handle_irq(virq);
> +	}
> +
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +static int imx_intmux_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct intmux_data *data;
> +	int channum;
> +	int i, ret;
> +
> +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> +	if (ret) {
> +		channum = 1;
> +	} else if (channum > CHAN_MAX_NUM) {
> +		dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
> +			CHAN_MAX_NUM);
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data) +
> +			    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize reg\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(data->ipg_clk)) {
> +		ret = PTR_ERR(data->ipg_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	data->channum = channum;
> +	raw_spin_lock_init(&data->lock);
> +
> +	ret = clk_prepare_enable(data->ipg_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < channum; i++) {
> +		data->irqchip_data[i].chanidx = i;
> +
> +		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
> +		if (data->irqchip_data[i].irq <= 0) {
> +			ret = -EINVAL;
> +			dev_err(&pdev->dev, "failed to get irq\n");
> +			goto out;
> +		}
> +
> +		data->irqchip_data[i].domain =
> +			irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
> +					      &data->irqchip_data[i]);
> +		if (!data->irqchip_data[i].domain) {
> +			ret = -ENOMEM;
> +			dev_err(&pdev->dev, "failed to create IRQ domain\n");
> +			goto out;
> +		}
> +
> +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> +						 imx_intmux_irq_handler,
> +						 &data->irqchip_data[i]);
> +
> +		/* disable interrupt sources of this channel firstly */
> +		writel_relaxed(0, data->regs + CHANIER(i));
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +out:
> +	clk_disable_unprepare(data->ipg_clk);
> +	return ret;
> +}
> +
> +static int imx_intmux_remove(struct platform_device *pdev)
> +{
> +	struct intmux_data *data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < data->channum; i++) {
> +		/* clear interrupt sources pending status of this channel */
> +		writel_relaxed(0, data->regs + CHANIPR(i));
> +
> +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> +						 NULL, NULL);
> +
> +		irq_domain_remove(data->irqchip_data[i].domain);
> +	}
> +
> +	clk_disable_unprepare(data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_intmux_id[] = {
> +	{ .compatible = "fsl,imx-intmux", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver imx_intmux_driver = {
> +	.driver = {
> +		.name = "imx-intmux",
> +		.of_match_table = imx_intmux_id,
> +	},
> +	.probe = imx_intmux_probe,
> +	.remove = imx_intmux_remove,
> +};
> +builtin_platform_driver(imx_intmux_driver);
> 

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

> -----Original Message-----
> From: Lokesh Vutla <lokeshvutla@ti.com>
> Sent: 2019年12月20日 19:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; maz@kernel.org;
> tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: Andy Duan <fugang.duan@nxp.com>; S.j. Wang
> <shengjiu.wang@nxp.com>; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> 
> 
> On 20/12/19 1:07 PM, Joakim Zhang wrote:
> > The Interrupt Multiplexer (INTMUX) expands the number of peripherals
> > that can interrupt the core:
> > * The INTMUX has 8 channels that are assigned to 8 NVIC interrupt slots.
> > * Each INTMUX channel can receive up to 32 interrupt sources and has 1
> >   interrupt output.
> > * The INTMUX routes the interrupt sources to the interrupt outputs.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig          |   6 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-intmux.c | 311
> > +++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-intmux.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > ba152954324b..7e2b1e9d0b45 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -457,6 +457,12 @@ config IMX_IRQSTEER
> >  	help
> >  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
> >
> > +config IMX_INTMUX
> > +	def_bool y if ARCH_MXC
> > +	select IRQ_DOMAIN
> > +	help
> > +	  Support for the i.MX INTMUX interrupt multiplexer.
> > +
> >  config LS1X_IRQ
> >  	bool "Loongson-1 Interrupt Controller"
> >  	depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > e806dda690ea..af976a79d1fb 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+=
> irq-csky-mpintc.o
> >  obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> > +obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
> >  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > new file mode 100644
> > index 000000000000..94c67fdd7163
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -0,0 +1,311 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2017 NXP
> > +
> > +/*                     INTMUX Block Diagram
> > + *
> > + *                               ________________
> > + * interrupt source #  0  +---->|                |
> > + *                        |     |                |
> > + * interrupt source #  1  +++-->|                |
> > + *            ...         | |   |   channel # 0  |--------->interrupt out
> # 0
> > + *            ...         | |   |                |
> > + *            ...         | |   |                |
> > + * interrupt source # X-1 +++-->|________________|
> > + *                        | | |
> > + *                        | | |
> > + *                        | | |  ________________
> > + *                        +---->|                |
> > + *                        | | | |                |
> > + *                        | +-->|                |
> > + *                        | | | |   channel # 1  |--------->interrupt out
> # 1
> > + *                        | | +>|                |
> > + *                        | | | |                |
> > + *                        | | | |________________|
> > + *                        | | |
> > + *                        | | |
> > + *                        | | |       ...
> > + *                        | | |       ...
> > + *                        | | |
> > + *                        | | |  ________________
> > + *                        +---->|                |
> > + *                          | | |                |
> > + *                          +-->|                |
> > + *                            | |   channel # N  |--------->interrupt
> out # N
> > + *                            +>|                |
> > + *                              |                |
> > + *                              |________________|
> > + *
> > + *
> > + * N: Interrupt Channel Instance Number (N=7)
> > + * X: Interrupt Source Number for each channel (X=32)
> > + *
> > + * The INTMUX interrupt multiplexer has 8 channels, each channel
> > +receives 32
> > + * interrupt sources and generates 1 interrupt output.
> 
> Does the user care to which channel does the interrupt source goes to? If not,
> interrupt-cells in DT can just be a single entry and the channel selection can be
> controlled by the driver no? I am trying to understand why user should specify
> the channel no.
Hi Lokesh,

If a fixed channel is specified in the driver, all interrupt sources will be connected to this channel, affecting the interrupt priority to some extent.

From my point of view, a fixed channel could be enough if don't care interrupt priority.

Best Regards,
Joakim Zhang
> Thanks and regards,
> Lokesh
> 
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/kernel.h> #include <linux/of_irq.h> #include
> > +<linux/of_platform.h> #include <linux/spinlock.h>
> > +
> > +#define CHANIER(n)	(0x10 + (0x40 * n))
> > +#define CHANIPR(n)	(0x20 + (0x40 * n))
> > +
> > +#define CHAN_MAX_NUM		0x8
> > +
> > +struct intmux_irqchip_data {
> > +	int			chanidx;
> > +	int			irq;
> > +	struct irq_domain	*domain;
> > +};
> > +
> > +struct intmux_data {
> > +	raw_spinlock_t			lock;
> > +	void __iomem			*regs;
> > +	struct clk			*ipg_clk;
> > +	int				channum;
> > +	struct intmux_irqchip_data	irqchip_data[];
> > +};
> > +
> > +static void imx_intmux_irq_mask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long flags;
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&data->lock, flags);
> > +	reg = data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* disable the interrupt source of this channel */
> > +	val &= ~BIT(d->hwirq);
> > +	writel_relaxed(val, reg);
> > +	raw_spin_unlock_irqrestore(&data->lock, flags); }
> > +
> > +static void imx_intmux_irq_unmask(struct irq_data *d) {
> > +	struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long flags;
> > +	void __iomem *reg;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&data->lock, flags);
> > +	reg = data->regs + CHANIER(idx);
> > +	val = readl_relaxed(reg);
> > +	/* enable the interrupt source of this channel */
> > +	val |= BIT(d->hwirq);
> > +	writel_relaxed(val, reg);
> > +	raw_spin_unlock_irqrestore(&data->lock, flags); }
> > +
> > +static struct irq_chip imx_intmux_irq_chip = {
> > +	.name		= "intmux",
> > +	.irq_mask	= imx_intmux_irq_mask,
> > +	.irq_unmask	= imx_intmux_irq_unmask,
> > +};
> > +
> > +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> > +			      irq_hw_number_t hwirq)
> > +{
> > +	irq_set_status_flags(irq, IRQ_LEVEL);
> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip,
> > +handle_level_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_intmux_irq_xlate(struct irq_domain *d, struct device_node
> *node,
> > +				const u32 *intspec, unsigned int intsize,
> > +				unsigned long *out_hwirq, unsigned int *out_type) {
> > +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> intmux_data,
> > +						irqchip_data[idx]);
> > +
> > +	/* two cells needed in interrupt specifier:
> > +	 * the 1st cell: hw interrupt number
> > +	 * the 2nd cell: channel index
> > +	 */
> > +	if (WARN_ON(intsize != 2))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(intspec[1] >= data->channum))
> > +		return -EINVAL;
> > +
> > +	*out_hwirq = intspec[0];
> > +	*out_type = IRQ_TYPE_NONE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_intmux_irq_select(struct irq_domain *d, struct irq_fwspec
> *fwspec,
> > +				 enum irq_domain_bus_token bus_token) {
> > +	struct intmux_irqchip_data *irqchip_data = d->host_data;
> > +
> > +	/* Not for us */
> > +	if (fwspec->fwnode != d->fwnode)
> > +		return false;
> > +
> > +	if (irqchip_data->chanidx == fwspec->param[1])
> > +		return true;
> > +	else
> > +		return false;
> > +}
> > +
> > +static const struct irq_domain_ops imx_intmux_domain_ops = {
> > +	.map		= imx_intmux_irq_map,
> > +	.xlate		= imx_intmux_irq_xlate,
> > +	.select		= imx_intmux_irq_select,
> > +};
> > +
> > +static void imx_intmux_irq_handler(struct irq_desc *desc) {
> > +	struct intmux_irqchip_data *irqchip_data =
> irq_desc_get_handler_data(desc);
> > +	int idx = irqchip_data->chanidx;
> > +	struct intmux_data *data = container_of(irqchip_data, struct
> intmux_data,
> > +						irqchip_data[idx]);
> > +	unsigned long irqstat;
> > +	int pos, virq;
> > +
> > +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +
> > +	/* read the interrupt source pending status of this channel */
> > +	irqstat = readl_relaxed(data->regs + CHANIPR(idx));
> > +
> > +	for_each_set_bit(pos, &irqstat, 32) {
> > +		virq = irq_find_mapping(irqchip_data->domain, pos);
> > +		if (virq)
> > +			generic_handle_irq(virq);
> > +	}
> > +
> > +	chained_irq_exit(irq_desc_get_chip(desc), desc); }
> > +
> > +static int imx_intmux_probe(struct platform_device *pdev) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct intmux_data *data;
> > +	int channum;
> > +	int i, ret;
> > +
> > +	ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> > +	if (ret) {
> > +		channum = 1;
> > +	} else if (channum > CHAN_MAX_NUM) {
> > +		dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
> > +			CHAN_MAX_NUM);
> > +		return -EINVAL;
> > +	}
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data) +
> > +			    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(data->regs)) {
> > +		dev_err(&pdev->dev, "failed to initialize reg\n");
> > +		return PTR_ERR(data->regs);
> > +	}
> > +
> > +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > +	if (IS_ERR(data->ipg_clk)) {
> > +		ret = PTR_ERR(data->ipg_clk);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	data->channum = channum;
> > +	raw_spin_lock_init(&data->lock);
> > +
> > +	ret = clk_prepare_enable(data->ipg_clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < channum; i++) {
> > +		data->irqchip_data[i].chanidx = i;
> > +
> > +		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
> > +		if (data->irqchip_data[i].irq <= 0) {
> > +			ret = -EINVAL;
> > +			dev_err(&pdev->dev, "failed to get irq\n");
> > +			goto out;
> > +		}
> > +
> > +		data->irqchip_data[i].domain =
> > +			irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
> > +					      &data->irqchip_data[i]);
> > +		if (!data->irqchip_data[i].domain) {
> > +			ret = -ENOMEM;
> > +			dev_err(&pdev->dev, "failed to create IRQ domain\n");
> > +			goto out;
> > +		}
> > +
> > +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> > +						 imx_intmux_irq_handler,
> > +						 &data->irqchip_data[i]);
> > +
> > +		/* disable interrupt sources of this channel firstly */
> > +		writel_relaxed(0, data->regs + CHANIER(i));
> > +	}
> > +
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	return 0;
> > +out:
> > +	clk_disable_unprepare(data->ipg_clk);
> > +	return ret;
> > +}
> > +
> > +static int imx_intmux_remove(struct platform_device *pdev) {
> > +	struct intmux_data *data = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < data->channum; i++) {
> > +		/* clear interrupt sources pending status of this channel */
> > +		writel_relaxed(0, data->regs + CHANIPR(i));
> > +
> > +		irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
> > +						 NULL, NULL);
> > +
> > +		irq_domain_remove(data->irqchip_data[i].domain);
> > +	}
> > +
> > +	clk_disable_unprepare(data->ipg_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx_intmux_id[] = {
> > +	{ .compatible = "fsl,imx-intmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static struct platform_driver imx_intmux_driver = {
> > +	.driver = {
> > +		.name = "imx-intmux",
> > +		.of_match_table = imx_intmux_id,
> > +	},
> > +	.probe = imx_intmux_probe,
> > +	.remove = imx_intmux_remove,
> > +};
> > +builtin_platform_driver(imx_intmux_driver);
> >

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Marc Zyngier]

On 2019-12-20 14:10, Joakim Zhang wrote:
>> -----Original Message-----
>> From: Lokesh Vutla <lokeshvutla@ti.com>

[...]

>> Does the user care to which channel does the interrupt source goes 
>> to? If not,
>> interrupt-cells in DT can just be a single entry and the channel 
>> selection can be
>> controlled by the driver no? I am trying to understand why user 
>> should specify
>> the channel no.
> Hi Lokesh,
>
> If a fixed channel is specified in the driver, all interrupt sources
> will be connected to this channel, affecting the interrupt priority 
> to
> some extent.
>
> From my point of view, a fixed channel could be enough if don't care
> interrupt priority.

Hold on a sec:

Is the channel to which an interrupt is routed to programmable? What 
has
the priority of the interrupt to do with this? How does this affect
interrupt delivery?

It looks like this HW does more that you initially explained...

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月20日 22:20
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Lokesh Vutla <lokeshvutla@ti.com>; tglx@linutronix.de;
> jason@lakedaemon.net; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; Andy Duan
> <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> On 2019-12-20 14:10, Joakim Zhang wrote:
> >> -----Original Message-----
> >> From: Lokesh Vutla <lokeshvutla@ti.com>
> 
> [...]
> 
> >> Does the user care to which channel does the interrupt source goes
> >> to? If not, interrupt-cells in DT can just be a single entry and the
> >> channel selection can be controlled by the driver no? I am trying to
> >> understand why user should specify the channel no.
> > Hi Lokesh,
> >
> > If a fixed channel is specified in the driver, all interrupt sources
> > will be connected to this channel, affecting the interrupt priority to
> > some extent.
> >
> > From my point of view, a fixed channel could be enough if don't care
> > interrupt priority.
> 
> Hold on a sec:
> 
> Is the channel to which an interrupt is routed to programmable? What has the
> priority of the interrupt to do with this? How does this affect interrupt
> delivery?
> 
> It looks like this HW does more that you initially explained...
Hi Marc,

The channel to which an interrupt is routed to is not programmable. Each channel has the same 32 interrupt sources.
Each channel has mask, unmask and status register.
If use 1 channel, 32 interrupt sources input and 1 interrupt output.
If use 2 channels, 32 interrupt sources input and 2 interrupts output.
And so on. You can see above INTMUX block diagram. This is how HW works.

For example:
1) use 1 channel:
We can enable 0~31 interrupt in channel 0. And 1 interrupt output. If generate interrupt, we cannot figure out which half happened first.
2)use 2 channels:
We can enable 0~15 interrupt in channel 0, and enable 16~31 in channel 1. And 2 interrupts output. If generate interrupt, at least we can find channel 0 or channel 1 first. Then find 0~15 or 16~31 first.

This is my understanding of the interrupt priority from this intmux, I don't know if it is my misunderstanding.

Best Regards,
Joakim Zhang
>          M.
> --
> Jazz is not dead. It just smells funny...

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年12月20日 23:26
> To: 'Marc Zyngier' <maz@kernel.org>
> Cc: Lokesh Vutla <lokeshvutla@ti.com>; tglx@linutronix.de;
> jason@lakedaemon.net; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; Andy Duan
> <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: 2019年12月20日 22:20
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: Lokesh Vutla <lokeshvutla@ti.com>; tglx@linutronix.de;
> > jason@lakedaemon.net; robh+dt@kernel.org; mark.rutland@arm.com;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; Andy Duan
> > <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
> > Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> > multiplexer support
> >
> > On 2019-12-20 14:10, Joakim Zhang wrote:
> > >> -----Original Message-----
> > >> From: Lokesh Vutla <lokeshvutla@ti.com>
> >
> > [...]
> >
> > >> Does the user care to which channel does the interrupt source goes
> > >> to? If not, interrupt-cells in DT can just be a single entry and
> > >> the channel selection can be controlled by the driver no? I am
> > >> trying to understand why user should specify the channel no.
> > > Hi Lokesh,
> > >
> > > If a fixed channel is specified in the driver, all interrupt sources
> > > will be connected to this channel, affecting the interrupt priority
> > > to some extent.
> > >
> > > From my point of view, a fixed channel could be enough if don't care
> > > interrupt priority.
> >
> > Hold on a sec:
> >
> > Is the channel to which an interrupt is routed to programmable? What
> > has the priority of the interrupt to do with this? How does this
> > affect interrupt delivery?
> >
> > It looks like this HW does more that you initially explained...
> Hi Marc,
> 
> The channel to which an interrupt is routed to is not programmable. Each
> channel has the same 32 interrupt sources.
> Each channel has mask, unmask and status register.
> If use 1 channel, 32 interrupt sources input and 1 interrupt output.
> If use 2 channels, 32 interrupt sources input and 2 interrupts output.
> And so on. You can see above INTMUX block diagram. This is how HW works.
> 
> For example:
> 1) use 1 channel:
> We can enable 0~31 interrupt in channel 0. And 1 interrupt output. If generate
> interrupt, we cannot figure out which half happened first.
> 2)use 2 channels:
> We can enable 0~15 interrupt in channel 0, and enable 16~31 in channel 1.
> And 2 interrupts output. If generate interrupt, at least we can find channel 0 or
> channel 1 first. Then find 0~15 or 16~31 first.
> 
> This is my understanding of the interrupt priority from this intmux, I don't
> know if it is my misunderstanding.

So assign interrupt sources to multi-channels will generate multi-interrupt output. And these output interrupts are sequential. Could this be interpreted as interrupt priority?

Best Regards,
Joakim Zhang
> Best Regards,
> Joakim Zhang
> >          M.
> > --
> > Jazz is not dead. It just smells funny...

Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Lokesh Vutla]

On 20/12/19 8:56 PM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: 2019年12月20日 22:20
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>
>> Cc: Lokesh Vutla <lokeshvutla@ti.com>; tglx@linutronix.de;
>> jason@lakedaemon.net; robh+dt@kernel.org; mark.rutland@arm.com;
>> shawnguo@kernel.org; s.hauer@pengutronix.de; Andy Duan
>> <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
>> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
>> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
>> Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
>> multiplexer support
>>
>> On 2019-12-20 14:10, Joakim Zhang wrote:
>>>> -----Original Message-----
>>>> From: Lokesh Vutla <lokeshvutla@ti.com>
>>
>> [...]
>>
>>>> Does the user care to which channel does the interrupt source goes
>>>> to? If not, interrupt-cells in DT can just be a single entry and the
>>>> channel selection can be controlled by the driver no? I am trying to
>>>> understand why user should specify the channel no.
>>> Hi Lokesh,
>>>
>>> If a fixed channel is specified in the driver, all interrupt sources
>>> will be connected to this channel, affecting the interrupt priority to
>>> some extent.
>>>
>>> From my point of view, a fixed channel could be enough if don't care
>>> interrupt priority.
>>
>> Hold on a sec:
>>
>> Is the channel to which an interrupt is routed to programmable? What has the
>> priority of the interrupt to do with this? How does this affect interrupt
>> delivery?
>>
>> It looks like this HW does more that you initially explained...
> Hi Marc,
> 
> The channel to which an interrupt is routed to is not programmable. Each channel has the same 32 interrupt sources.

But the interrupt source to channel is programmable right? I guess you are
worried about the affinity for each interrupt. You can bring the logic inside
the driver to assign the channel to each interrupt source and can maintain the
affinity to some extent..

> Each channel has mask, unmask and status register.
> If use 1 channel, 32 interrupt sources input and 1 interrupt output.
> If use 2 channels, 32 interrupt sources input and 2 interrupts output.
> And so on. You can see above INTMUX block diagram. This is how HW works.
> 
> For example:
> 1) use 1 channel:
> We can enable 0~31 interrupt in channel 0. And 1 interrupt output. If generate interrupt, we cannot figure out which half happened first.

Hmm...does this mean that each channel is capable of handling only 15 interrupt
sources or did I missunderstood the hardware?

Thanks and regards,
Lokesh

> 2)use 2 channels:
> We can enable 0~15 interrupt in channel 0, and enable 16~31 in channel 1. And 2 interrupts output. If generate interrupt, at least we can find channel 0 or channel 1 first. Then find 0~15 or 16~31 first.
> 
> This is my understanding of the interrupt priority from this intmux, I don't know if it is my misunderstanding.
> 
> Best Regards,
> Joakim Zhang
>>          M.
>> --
>> Jazz is not dead. It just smells funny...

RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Joakim Zhang]

> -----Original Message-----
> From: Lokesh Vutla <lokeshvutla@ti.com>
> Sent: 2019年12月23日 18:11
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Marc Zyngier
> <maz@kernel.org>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> Andy Duan <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> 
> 
> On 20/12/19 8:56 PM, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: 2019年12月20日 22:20
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> >> Cc: Lokesh Vutla <lokeshvutla@ti.com>; tglx@linutronix.de;
> >> jason@lakedaemon.net; robh+dt@kernel.org; mark.rutland@arm.com;
> >> shawnguo@kernel.org; s.hauer@pengutronix.de; Andy Duan
> >> <fugang.duan@nxp.com>; S.j. Wang <shengjiu.wang@nxp.com>;
> >> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> >> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org
> >> Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> >> multiplexer support
> >>
> >> On 2019-12-20 14:10, Joakim Zhang wrote:
> >>>> -----Original Message-----
> >>>> From: Lokesh Vutla <lokeshvutla@ti.com>
> >>
> >> [...]
> >>
> >>>> Does the user care to which channel does the interrupt source goes
> >>>> to? If not, interrupt-cells in DT can just be a single entry and
> >>>> the channel selection can be controlled by the driver no? I am
> >>>> trying to understand why user should specify the channel no.
> >>> Hi Lokesh,
> >>>
> >>> If a fixed channel is specified in the driver, all interrupt sources
> >>> will be connected to this channel, affecting the interrupt priority
> >>> to some extent.
> >>>
> >>> From my point of view, a fixed channel could be enough if don't care
> >>> interrupt priority.
> >>
> >> Hold on a sec:
> >>
> >> Is the channel to which an interrupt is routed to programmable? What
> >> has the priority of the interrupt to do with this? How does this
> >> affect interrupt delivery?
> >>
> >> It looks like this HW does more that you initially explained...
> > Hi Marc,
> >
> > The channel to which an interrupt is routed to is not programmable. Each
> channel has the same 32 interrupt sources.
> 
> But the interrupt source to channel is programmable right? I guess you are
> worried about the affinity for each interrupt. You can bring the logic inside the
> driver to assign the channel to each interrupt source and can maintain the
> affinity to some extent..
Each channel supports 32 interrupt sources, you can unmask interrupt sources which you want generate via this channel, and other interrupt sources stay mask.

> > Each channel has mask, unmask and status register.
> > If use 1 channel, 32 interrupt sources input and 1 interrupt output.
> > If use 2 channels, 32 interrupt sources input and 2 interrupts output.
> > And so on. You can see above INTMUX block diagram. This is how HW works.
> >
> > For example:
> > 1) use 1 channel:
> > We can enable 0~31 interrupt in channel 0. And 1 interrupt output. If
> generate interrupt, we cannot figure out which half happened first.
> 
> Hmm...does this mean that each channel is capable of handling only 15
> interrupt sources or did I missunderstood the hardware?
Yes, you just need unmask interrupt sources you want for each channel.

For intmux IP:
1) 8 output interrupts can connect to different cores (A core, M4, DSP, and so on), so this is 32-to-1.
2) 8 output interrupts can connect to one core, so this is 32-to-8

In our i.MX8 SoCs, intmux is integrated in M4 and 8 output interrupts all connected to GIC in A core.
Supposed that there are 4 devices actually request to intmux, so intmux has 4 interrupt sources.
We can assign these 4 interrupt sources via interrupt specifier to channel 0. This is 4-to-1.
We also can assign 4 interrupt sources via interrupt specifier to channel 0/1/2/3. This is 4-to4.

I think interrupts handing sequence could be more closed to interrupts generate sequence if one channel unmask less interrupts sources(i.e. enable less interrupt sources for that channel).
Since we always check interrupt pending status for onechannel from sources 0 to 31.

This should not be interrupt priority, sorry for my previous answer. 

Best Regards,
Joakim Zhang
> Thanks and regards,
> Lokesh
> 
> > 2)use 2 channels:
> > We can enable 0~15 interrupt in channel 0, and enable 16~31 in channel 1.
> And 2 interrupts output. If generate interrupt, at least we can find channel 0 or
> channel 1 first. Then find 0~15 or 16~31 first.
> >
> > This is my understanding of the interrupt priority from this intmux, I don't
> know if it is my misunderstanding.
> >
> > Best Regards,
> > Joakim Zhang
> >>          M.
> >> --
> >> Jazz is not dead. It just smells funny...