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

Commit Message

Joakim Zhang Dec. 20, 2019, 7:37 a.m. UTC

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 <[e-mail]>
Signed-off-by: Joakim Zhang <[e-mail]>
---
 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

Comments

Marc Zyngier Dec. 20, 2019, 10:49 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Joakim Zhang <[e-mail]>
> ---
>  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.

Lokesh Vutla Dec. 20, 2019, 11:45 a.m. UTC | #2

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 <[e-mail]>
> Signed-off-by: Joakim Zhang <[e-mail]>
> ---
>  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);
>

Joakim Zhang Dec. 20, 2019, 11:59 a.m. UTC | #3

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: 2019年12月20日 18:49
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; Andy Duan <[e-mail]>;
> [e-mail]; S.j. Wang <[e-mail]>
> 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 <[e-mail]>
> > Signed-off-by: Joakim Zhang <[e-mail]>
> > ---
> >  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...

Joakim Zhang Dec. 20, 2019, 2:10 p.m. UTC | #4

> -----Original Message-----
> From: Lokesh Vutla <[e-mail]>
> Sent: 2019年12月20日 19:45
> To: Joakim Zhang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: Andy Duan <[e-mail]>; S.j. Wang
> <[e-mail]>; [e-mail]; dl-linux-imx
> <[e-mail]>; [e-mail];
> [e-mail]
> 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 <[e-mail]>
> > Signed-off-by: Joakim Zhang <[e-mail]>
> > ---
> >  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);
> >

Marc Zyngier Dec. 20, 2019, 2:20 p.m. UTC | #5

On 2019-12-20 14:10, Joakim Zhang wrote:
>> -----Original Message-----
>> From: Lokesh Vutla <[e-mail]>

[...]

>> 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.

Joakim Zhang Dec. 20, 2019, 3:26 p.m. UTC | #6

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: 2019年12月20日 22:20
> To: Joakim Zhang <[e-mail]>
> Cc: Lokesh Vutla <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Andy Duan
> <[e-mail]>; S.j. Wang <[e-mail]>;
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> 
> [...]
> 
> >> 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...

Joakim Zhang Dec. 20, 2019, 3:35 p.m. UTC | #7

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年12月20日 23:26
> To: 'Marc Zyngier' <[e-mail]>
> Cc: Lokesh Vutla <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Andy Duan
> <[e-mail]>; S.j. Wang <[e-mail]>;
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <[e-mail]>
> > Sent: 2019年12月20日 22:20
> > To: Joakim Zhang <[e-mail]>
> > Cc: Lokesh Vutla <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Andy Duan
> > <[e-mail]>; S.j. Wang <[e-mail]>;
> > [e-mail]; dl-linux-imx <[e-mail]>;
> > [e-mail]; [e-mail]
> > 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 <[e-mail]>
> >
> > [...]
> >
> > >> 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...

Lokesh Vutla Dec. 23, 2019, 10:10 a.m. UTC | #8

On 20/12/19 8:56 PM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Marc Zyngier <[e-mail]>
>> Sent: 2019年12月20日 22:20
>> To: Joakim Zhang <[e-mail]>
>> Cc: Lokesh Vutla <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Andy Duan
>> <[e-mail]>; S.j. Wang <[e-mail]>;
>> [e-mail]; dl-linux-imx <[e-mail]>;
>> [e-mail]; [e-mail]
>> 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 <[e-mail]>
>>
>> [...]
>>
>>>> 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...

Joakim Zhang Dec. 23, 2019, 11:11 a.m. UTC | #9

> -----Original Message-----
> From: Lokesh Vutla <[e-mail]>
> Sent: 2019年12月23日 18:11
> To: Joakim Zhang <[e-mail]>; Marc Zyngier
> <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Andy Duan <[e-mail]>; S.j. Wang <[e-mail]>;
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Sent: 2019年12月20日 22:20
> >> To: Joakim Zhang <[e-mail]>
> >> Cc: Lokesh Vutla <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Andy Duan
> >> <[e-mail]>; S.j. Wang <[e-mail]>;
> >> [e-mail]; dl-linux-imx <[e-mail]>;
> >> [e-mail]; [e-mail]
> >> 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 <[e-mail]>
> >>
> >> [...]
> >>
> >>>> 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...

Patch

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