* [PATCH 0/3] drivers/irqchip: Add NXP INTMUX interrupt
@ 2019-12-18 7:20 Joakim Zhang
2019-12-18 7:20 ` [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer Joakim Zhang
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 7:20 UTC (permalink / raw)
To: tglx, jason, maz, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang
Cc: kernel, festevam, linux-imx, linux-kernel, devicetree,
linux-arm-kernel, fugang.duan, aisheng.dong, Joakim Zhang
This patch set adds driver for NXP INTMUX interrupt controller.
Joakim Zhang (2):
dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
drivers/irqchip: enable INTMUX interrupt controller driver
Shengjiu Wang (1):
drivers/irqchip: add NXP INTMUX interrupt multiplexer support
.../interrupt-controller/fsl,intmux.txt | 34 +++
drivers/irqchip/Kconfig | 6 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-imx-intmux.c | 220 ++++++++++++++++++
4 files changed, 261 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
create mode 100644 drivers/irqchip/irq-imx-intmux.c
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
2019-12-18 7:20 [PATCH 0/3] drivers/irqchip: Add NXP INTMUX interrupt Joakim Zhang
@ 2019-12-18 7:20 ` Joakim Zhang
2019-12-18 9:45 ` Marc Zyngier
2019-12-18 7:20 ` [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support Joakim Zhang
2019-12-18 7:20 ` [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver Joakim Zhang
2 siblings, 1 reply; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 7:20 UTC (permalink / raw)
To: tglx, jason, maz, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang
Cc: kernel, festevam, linux-imx, linux-kernel, devicetree,
linux-arm-kernel, fugang.duan, aisheng.dong, Joakim Zhang
This patch adds the DT bindings for the NXP INTMUX interrupt multiplexer
found in the i.MX8 family SoCs.
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
.../interrupt-controller/fsl,intmux.txt | 34 +++++++++++++++++++
1 file changed, 34 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..be3c6848f36c
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
@@ -0,0 +1,34 @@
+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 1.
+
+Optional properties:
+
+- fsl,intmux_chans: The number of channels used for interrupt source. The
+ Maximum value is 8.
+
+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>;
+ fsl,intmux_chans = <1>;
+ };
+
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support
2019-12-18 7:20 [PATCH 0/3] drivers/irqchip: Add NXP INTMUX interrupt Joakim Zhang
2019-12-18 7:20 ` [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer Joakim Zhang
@ 2019-12-18 7:20 ` Joakim Zhang
2019-12-18 9:37 ` Marc Zyngier
2019-12-18 7:20 ` [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver Joakim Zhang
2 siblings, 1 reply; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 7:20 UTC (permalink / raw)
To: tglx, jason, maz, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang
Cc: kernel, festevam, linux-imx, linux-kernel, devicetree,
linux-arm-kernel, fugang.duan, aisheng.dong, Joakim Zhang
From: Shengjiu Wang <shengjiu.wang@nxp.com>
The intmux module is used to output internal interrupt in subsystem
to system with 32-to-8 configuration. It has several multiplex
channels depends on system.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
drivers/irqchip/irq-imx-intmux.c | 220 +++++++++++++++++++++++++++++++
1 file changed, 220 insertions(+)
create mode 100644 drivers/irqchip/irq-imx-intmux.c
diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
new file mode 100644
index 000000000000..fa24b968f30b
--- /dev/null
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2017 NXP
+
+#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/module.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+
+#define CHANCSR(n) (0x0 + 0x40 * n)
+#define CHANVEC(n) (0x4 + 0x40 * n)
+#define CHANIER(n) (0x10 + (0x40 * n))
+#define CHANIPR(n) (0x20 + (0x40 * n))
+
+struct intmux_irqchip_data {
+ int chanidx;
+ int irq;
+ struct irq_domain *domain;
+ unsigned int irqstat;
+};
+
+struct intmux_data {
+ spinlock_t lock;
+ struct platform_device *pdev;
+ 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;
+ u32 idx = irqchip_data->chanidx;
+ struct intmux_data *intmux_data = container_of(irqchip_data,
+ struct intmux_data, irqchip_data[idx]);
+ void __iomem *reg;
+ u32 val;
+
+ spin_lock(&intmux_data->lock);
+ reg = intmux_data->regs + CHANIER(idx);
+ val = readl_relaxed(reg);
+ /* disable the interrupt source of this channel */
+ val &= ~(1 << d->hwirq);
+ writel_relaxed(val, reg);
+ spin_unlock(&intmux_data->lock);
+}
+
+static void imx_intmux_irq_unmask(struct irq_data *d)
+{
+ struct intmux_irqchip_data *irqchip_data = d->chip_data;
+ u32 idx = irqchip_data->chanidx;
+ struct intmux_data *intmux_data = container_of(irqchip_data,
+ struct intmux_data, irqchip_data[idx]);
+ void __iomem *reg;
+ u32 val;
+
+ spin_lock(&intmux_data->lock);
+ reg = intmux_data->regs + CHANIER(idx);
+ val = readl_relaxed(reg);
+ /* enable the interrupt source of this channel */
+ val |= 1 << d->hwirq;
+ writel_relaxed(val, reg);
+ spin_unlock(&intmux_data->lock);
+}
+
+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 const struct irq_domain_ops imx_intmux_domain_ops = {
+ .map = imx_intmux_irq_map,
+ .xlate = irq_domain_xlate_onecell,
+};
+
+static void imx_intmux_update_irqstat(struct intmux_irqchip_data *irqchip_data)
+{
+ int i = irqchip_data->chanidx;
+ struct intmux_data *intmux_data = container_of(irqchip_data,
+ struct intmux_data, irqchip_data[i]);
+
+ /* read the interrupt source pending status of this channel */
+ irqchip_data->irqstat = readl_relaxed(intmux_data->regs + CHANIPR(i));
+}
+
+static void imx_intmux_irq_handler(struct irq_desc *desc)
+{
+ struct intmux_irqchip_data *irqchip_data = irq_desc_get_handler_data(desc);
+ int pos, virq;
+
+ chained_irq_enter(irq_desc_get_chip(desc), desc);
+
+ imx_intmux_update_irqstat(irqchip_data);
+
+ for_each_set_bit(pos, (unsigned long *)&irqchip_data->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 *intmux_data;
+ int channum;
+ int i, ret;
+
+ ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
+ if (ret)
+ channum = 1;
+
+ intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) +
+ channum * sizeof(intmux_data->irqchip_data[0]),
+ GFP_KERNEL);
+ if (!intmux_data)
+ return -ENOMEM;
+
+ intmux_data->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(intmux_data->regs)) {
+ dev_err(&pdev->dev, "failed to initialize reg\n");
+ return PTR_ERR(intmux_data->regs);
+ }
+
+ intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+ if (IS_ERR(intmux_data->ipg_clk)) {
+ ret = PTR_ERR(intmux_data->ipg_clk);
+ dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
+ return ret;
+ }
+
+ intmux_data->channum = channum;
+ intmux_data->pdev = pdev;
+ spin_lock_init(&intmux_data->lock);
+
+ ret = clk_prepare_enable(intmux_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++) {
+ intmux_data->irqchip_data[i].chanidx = i;
+ intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i);
+ if (intmux_data->irqchip_data[i].irq <= 0) {
+ dev_err(&pdev->dev, "failed to get irq\n");
+ return -ENODEV;
+ }
+
+ intmux_data->irqchip_data[i].domain = irq_domain_add_linear(
+ np,
+ 32,
+ &imx_intmux_domain_ops,
+ &intmux_data->irqchip_data[i]);
+ if (!intmux_data->irqchip_data[i].domain) {
+ dev_err(&intmux_data->pdev->dev,
+ "failed to create IRQ domain\n");
+ return -ENOMEM;
+ }
+
+ irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
+ imx_intmux_irq_handler,
+ &intmux_data->irqchip_data[i]);
+ }
+
+ platform_set_drvdata(pdev, intmux_data);
+
+ return 0;
+}
+
+static int imx_intmux_remove(struct platform_device *pdev)
+{
+ struct intmux_data *intmux_data = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < intmux_data->channum; i++) {
+ irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
+ NULL, NULL);
+
+ irq_domain_remove(intmux_data->irqchip_data[i].domain);
+ }
+
+ platform_set_drvdata(pdev, NULL);
+ clk_disable_unprepare(intmux_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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver
2019-12-18 7:20 [PATCH 0/3] drivers/irqchip: Add NXP INTMUX interrupt Joakim Zhang
2019-12-18 7:20 ` [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer Joakim Zhang
2019-12-18 7:20 ` [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support Joakim Zhang
@ 2019-12-18 7:20 ` Joakim Zhang
2019-12-18 9:40 ` Marc Zyngier
2 siblings, 1 reply; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 7:20 UTC (permalink / raw)
To: tglx, jason, maz, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang
Cc: kernel, festevam, linux-imx, linux-kernel, devicetree,
linux-arm-kernel, fugang.duan, aisheng.dong, Joakim Zhang
Enable INTMUX interrupt controller driver for i.MX platforms.
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
drivers/irqchip/Kconfig | 6 ++++++
drivers/irqchip/Makefile | 1 +
2 files changed, 7 insertions(+)
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
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support
2019-12-18 7:20 ` [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support Joakim Zhang
@ 2019-12-18 9:37 ` Marc Zyngier
2019-12-18 10:11 ` Joakim Zhang
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2019-12-18 9:37 UTC (permalink / raw)
To: Joakim Zhang
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang, kernel, festevam, linux-imx, linux-kernel,
devicetree, linux-arm-kernel, fugang.duan, aisheng.dong
On 2019-12-18 07:20, Joakim Zhang wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
>
> The intmux module is used to output internal interrupt in subsystem
> to system with 32-to-8 configuration. It has several multiplex
> channels depends on system.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> drivers/irqchip/irq-imx-intmux.c | 220
> +++++++++++++++++++++++++++++++
> 1 file changed, 220 insertions(+)
> create mode 100644 drivers/irqchip/irq-imx-intmux.c
>
> diff --git a/drivers/irqchip/irq-imx-intmux.c
> b/drivers/irqchip/irq-imx-intmux.c
> new file mode 100644
> index 000000000000..fa24b968f30b
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2017 NXP
> +
> +#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/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CHANCSR(n) (0x0 + 0x40 * n)
> +#define CHANVEC(n) (0x4 + 0x40 * n)
These two macros are unused.
> +#define CHANIER(n) (0x10 + (0x40 * n))
> +#define CHANIPR(n) (0x20 + (0x40 * n))
> +
> +struct intmux_irqchip_data {
> + int chanidx;
> + int irq;
> + struct irq_domain *domain;
> + unsigned int irqstat;
It would make things a bit readable if you aligned the various fields:
struct intmux_irqchip_data {
int chanidx;
int irq;
struct irq_domain *domain;
[...]
};
> +};
> +
> +struct intmux_data {
> + spinlock_t lock;
> + struct platform_device *pdev;
> + 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;
> + u32 idx = irqchip_data->chanidx;
> + struct intmux_data *intmux_data = container_of(irqchip_data,
> + struct intmux_data, irqchip_data[idx]);
> + void __iomem *reg;
> + u32 val;
> +
> + spin_lock(&intmux_data->lock);
This is racy. you could take an interrupt while executing
disable_irq(),
which calls this. In turn, the interrupt handler will try to acquire
this
lock -> deadlock.
Please turn this into its _irqsave version.
> + reg = intmux_data->regs + CHANIER(idx);
> + val = readl_relaxed(reg);
> + /* disable the interrupt source of this channel */
> + val &= ~(1 << d->hwirq);
val &= ~BIT(d->hwirq);
> + writel_relaxed(val, reg);
> + spin_unlock(&intmux_data->lock);
> +}
> +
> +static void imx_intmux_irq_unmask(struct irq_data *d)
> +{
> + struct intmux_irqchip_data *irqchip_data = d->chip_data;
> + u32 idx = irqchip_data->chanidx;
> + struct intmux_data *intmux_data = container_of(irqchip_data,
> + struct intmux_data, irqchip_data[idx]);
> + void __iomem *reg;
> + u32 val;
> +
> + spin_lock(&intmux_data->lock);
> + reg = intmux_data->regs + CHANIER(idx);
> + val = readl_relaxed(reg);
> + /* enable the interrupt source of this channel */
> + val |= 1 << d->hwirq;
> + writel_relaxed(val, reg);
> + spin_unlock(&intmux_data->lock);
> +}
> +
> +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 const struct irq_domain_ops imx_intmux_domain_ops = {
> + .map = imx_intmux_irq_map,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +
> +static void imx_intmux_update_irqstat(struct intmux_irqchip_data
> *irqchip_data)
> +{
> + int i = irqchip_data->chanidx;
> + struct intmux_data *intmux_data = container_of(irqchip_data,
> + struct intmux_data, irqchip_data[i]);
> +
> + /* read the interrupt source pending status of this channel */
> + irqchip_data->irqstat = readl_relaxed(intmux_data->regs +
> CHANIPR(i));
Why does it need to be stored into the data structure, instead of
sinply being returned by the function?
> +}
> +
> +static void imx_intmux_irq_handler(struct irq_desc *desc)
> +{
> + struct intmux_irqchip_data *irqchip_data =
> irq_desc_get_handler_data(desc);
> + int pos, virq;
> +
> + chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> + imx_intmux_update_irqstat(irqchip_data);
> +
> + for_each_set_bit(pos, (unsigned long *)&irqchip_data->irqstat, 32)
> {
This is broken on big-endian. Never cast a smaller type into unsigned
long
if you're going to use any of the bit iterators.
> + 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 *intmux_data;
> + int channum;
> + int i, ret;
> +
> + ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> + if (ret)
> + channum = 1;
> +
> + intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) +
> + channum * sizeof(intmux_data->irqchip_data[0]),
> + GFP_KERNEL);
> + if (!intmux_data)
> + return -ENOMEM;
> +
> + intmux_data->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(intmux_data->regs)) {
> + dev_err(&pdev->dev, "failed to initialize reg\n");
> + return PTR_ERR(intmux_data->regs);
> + }
> +
> + intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(intmux_data->ipg_clk)) {
> + ret = PTR_ERR(intmux_data->ipg_clk);
> + dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> + return ret;
> + }
> +
> + intmux_data->channum = channum;
> + intmux_data->pdev = pdev;
What is the point of keeping track of this? The only instance where you
go from MUX to device is just below, and you already have the device
at hand.
> + spin_lock_init(&intmux_data->lock);
> +
> + ret = clk_prepare_enable(intmux_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++) {
> + intmux_data->irqchip_data[i].chanidx = i;
> + intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i);
> + if (intmux_data->irqchip_data[i].irq <= 0) {
> + dev_err(&pdev->dev, "failed to get irq\n");
> + return -ENODEV;
> + }
> +
> + intmux_data->irqchip_data[i].domain = irq_domain_add_linear(
> + np,
> + 32,
> + &imx_intmux_domain_ops,
> + &intmux_data->irqchip_data[i]);
Please indent this in a readable manner. If you need an intermediate
variable,
so be it. Or have a long line if you want, but don't write things like
this.
> + if (!intmux_data->irqchip_data[i].domain) {
> + dev_err(&intmux_data->pdev->dev,
> + "failed to create IRQ domain\n");
> + return -ENOMEM;
> + }
> +
> + irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> + imx_intmux_irq_handler,
> + &intmux_data->irqchip_data[i]);
Shouldn't you initialize the HW to some sane state here? Like having
having all interrupts masked?
> + }
> +
> + platform_set_drvdata(pdev, intmux_data);
> +
> + return 0;
> +}
> +
> +static int imx_intmux_remove(struct platform_device *pdev)
> +{
> + struct intmux_data *intmux_data = platform_get_drvdata(pdev);
> + int i;
> +
> + for (i = 0; i < intmux_data->channum; i++) {
> + irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> + NULL, NULL);
Same thing here. Shouldn't you make sure that no interrupt can fire
anymore?
> +
> + irq_domain_remove(intmux_data->irqchip_data[i].domain);
> + }
> +
> + platform_set_drvdata(pdev, NULL);
> + clk_disable_unprepare(intmux_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);
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver
2019-12-18 7:20 ` [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver Joakim Zhang
@ 2019-12-18 9:40 ` Marc Zyngier
2019-12-18 10:11 ` Joakim Zhang
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2019-12-18 9:40 UTC (permalink / raw)
To: Joakim Zhang
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang, kernel, festevam, linux-imx, linux-kernel,
devicetree, linux-arm-kernel, fugang.duan, aisheng.dong
On 2019-12-18 07:20, Joakim Zhang wrote:
> Enable INTMUX interrupt controller driver for i.MX platforms.
>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> drivers/irqchip/Kconfig | 6 ++++++
> drivers/irqchip/Makefile | 1 +
> 2 files changed, 7 insertions(+)
>
> 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
Please merge this with the previous patch, it doesn't really warrant
a separate patch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
2019-12-18 7:20 ` [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer Joakim Zhang
@ 2019-12-18 9:45 ` Marc Zyngier
2019-12-18 10:21 ` Joakim Zhang
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2019-12-18 9:45 UTC (permalink / raw)
To: Joakim Zhang
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer,
shengjiu.wang, kernel, festevam, linux-imx, linux-kernel,
devicetree, linux-arm-kernel, fugang.duan, aisheng.dong
On 2019-12-18 07:20, Joakim Zhang wrote:
> This patch adds the DT bindings for the NXP INTMUX interrupt
> multiplexer
> found in the i.MX8 family SoCs.
>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> .../interrupt-controller/fsl,intmux.txt | 34
> +++++++++++++++++++
> 1 file changed, 34 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..be3c6848f36c
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> @@ -0,0 +1,34 @@
> +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 1.
> +
> +Optional properties:
> +
> +- fsl,intmux_chans: The number of channels used for interrupt
> source. The
> + Maximum value is 8.
> +
> +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>;
> + fsl,intmux_chans = <1>;
> + };
> +
What I don't understand is how the interrupt descriptor can indicate
which channel it is multiplexed on. The driver doesn't makes this
clear either, and I strongly suspect that it was never tested with
more than a single channel...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support
2019-12-18 9:37 ` Marc Zyngier
@ 2019-12-18 10:11 ` Joakim Zhang
0 siblings, 0 replies; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 10:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer, S.j. Wang,
kernel, festevam, dl-linux-imx, linux-kernel, devicetree,
linux-arm-kernel, Andy Duan, Aisheng Dong
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月18日 17:38
> 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; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
>
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > The intmux module is used to output internal interrupt in subsystem to
> > system with 32-to-8 configuration. It has several multiplex channels
> > depends on system.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > drivers/irqchip/irq-imx-intmux.c | 220
> > +++++++++++++++++++++++++++++++
> > 1 file changed, 220 insertions(+)
> > create mode 100644 drivers/irqchip/irq-imx-intmux.c
> >
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > new file mode 100644
> > index 000000000000..fa24b968f30b
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2017 NXP
> > +
> > +#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/module.h> #include
> > +<linux/of_platform.h> #include <linux/spinlock.h>
> > +
> > +#define CHANCSR(n) (0x0 + 0x40 * n)
> > +#define CHANVEC(n) (0x4 + 0x40 * n)
>
> These two macros are unused.
Hi Marc,
Yes, we defined these two macros and have not used yet. I will remove it firstly in V2.
> > +#define CHANIER(n) (0x10 + (0x40 * n))
> > +#define CHANIPR(n) (0x20 + (0x40 * n))
> > +
> > +struct intmux_irqchip_data {
> > + int chanidx;
> > + int irq;
> > + struct irq_domain *domain;
> > + unsigned int irqstat;
>
> It would make things a bit readable if you aligned the various fields:
>
> struct intmux_irqchip_data {
> int chanidx;
> int irq;
> struct irq_domain *domain;
> [...]
> };
Ok, I will do it in V2.
Thanks.
Best Regards,
Joakim Zhang
>
> > +};
> > +
> > +struct intmux_data {
> > + spinlock_t lock;
> > + struct platform_device *pdev;
> > + 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;
> > + u32 idx = irqchip_data->chanidx;
> > + struct intmux_data *intmux_data = container_of(irqchip_data,
> > + struct intmux_data, irqchip_data[idx]);
> > + void __iomem *reg;
> > + u32 val;
> > +
> > + spin_lock(&intmux_data->lock);
>
> This is racy. you could take an interrupt while executing disable_irq(), which
> calls this. In turn, the interrupt handler will try to acquire this lock -> deadlock.
>
> Please turn this into its _irqsave version.
>
> > + reg = intmux_data->regs + CHANIER(idx);
> > + val = readl_relaxed(reg);
> > + /* disable the interrupt source of this channel */
> > + val &= ~(1 << d->hwirq);
>
> val &= ~BIT(d->hwirq);
>
> > + writel_relaxed(val, reg);
> > + spin_unlock(&intmux_data->lock);
> > +}
> > +
> > +static void imx_intmux_irq_unmask(struct irq_data *d) {
> > + struct intmux_irqchip_data *irqchip_data = d->chip_data;
> > + u32 idx = irqchip_data->chanidx;
> > + struct intmux_data *intmux_data = container_of(irqchip_data,
> > + struct intmux_data, irqchip_data[idx]);
> > + void __iomem *reg;
> > + u32 val;
> > +
> > + spin_lock(&intmux_data->lock);
> > + reg = intmux_data->regs + CHANIER(idx);
> > + val = readl_relaxed(reg);
> > + /* enable the interrupt source of this channel */
> > + val |= 1 << d->hwirq;
> > + writel_relaxed(val, reg);
> > + spin_unlock(&intmux_data->lock);
> > +}
> > +
> > +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 const struct irq_domain_ops imx_intmux_domain_ops = {
> > + .map = imx_intmux_irq_map,
> > + .xlate = irq_domain_xlate_onecell,
> > +};
> > +
> > +static void imx_intmux_update_irqstat(struct intmux_irqchip_data
> > *irqchip_data)
> > +{
> > + int i = irqchip_data->chanidx;
> > + struct intmux_data *intmux_data = container_of(irqchip_data,
> > + struct intmux_data, irqchip_data[i]);
> > +
> > + /* read the interrupt source pending status of this channel */
> > + irqchip_data->irqstat = readl_relaxed(intmux_data->regs +
> > CHANIPR(i));
>
> Why does it need to be stored into the data structure, instead of
> sinply being returned by the function?
>
> > +}
> > +
> > +static void imx_intmux_irq_handler(struct irq_desc *desc)
> > +{
> > + struct intmux_irqchip_data *irqchip_data =
> > irq_desc_get_handler_data(desc);
> > + int pos, virq;
> > +
> > + chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +
> > + imx_intmux_update_irqstat(irqchip_data);
> > +
> > + for_each_set_bit(pos, (unsigned long *)&irqchip_data->irqstat, 32)
> > {
>
> This is broken on big-endian. Never cast a smaller type into unsigned
> long
> if you're going to use any of the bit iterators.
>
> > + 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 *intmux_data;
> > + int channum;
> > + int i, ret;
> > +
> > + ret = of_property_read_u32(np, "fsl,intmux_chans", &channum);
> > + if (ret)
> > + channum = 1;
> > +
> > + intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) +
> > + channum * sizeof(intmux_data->irqchip_data[0]),
> > + GFP_KERNEL);
> > + if (!intmux_data)
> > + return -ENOMEM;
> > +
> > + intmux_data->regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(intmux_data->regs)) {
> > + dev_err(&pdev->dev, "failed to initialize reg\n");
> > + return PTR_ERR(intmux_data->regs);
> > + }
> > +
> > + intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > + if (IS_ERR(intmux_data->ipg_clk)) {
> > + ret = PTR_ERR(intmux_data->ipg_clk);
> > + dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + intmux_data->channum = channum;
> > + intmux_data->pdev = pdev;
>
> What is the point of keeping track of this? The only instance where you
> go from MUX to device is just below, and you already have the device
> at hand.
>
> > + spin_lock_init(&intmux_data->lock);
> > +
> > + ret = clk_prepare_enable(intmux_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++) {
> > + intmux_data->irqchip_data[i].chanidx = i;
> > + intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i);
> > + if (intmux_data->irqchip_data[i].irq <= 0) {
> > + dev_err(&pdev->dev, "failed to get irq\n");
> > + return -ENODEV;
> > + }
> > +
> > + intmux_data->irqchip_data[i].domain = irq_domain_add_linear(
> > + np,
> > + 32,
> > + &imx_intmux_domain_ops,
> > + &intmux_data->irqchip_data[i]);
>
> Please indent this in a readable manner. If you need an intermediate
> variable,
> so be it. Or have a long line if you want, but don't write things like
> this.
>
> > + if (!intmux_data->irqchip_data[i].domain) {
> > + dev_err(&intmux_data->pdev->dev,
> > + "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > +
> irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> > + imx_intmux_irq_handler,
> > + &intmux_data->irqchip_data[i]);
>
> Shouldn't you initialize the HW to some sane state here? Like having
> having all interrupts masked?
>
> > + }
> > +
> > + platform_set_drvdata(pdev, intmux_data);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx_intmux_remove(struct platform_device *pdev)
> > +{
> > + struct intmux_data *intmux_data = platform_get_drvdata(pdev);
> > + int i;
> > +
> > + for (i = 0; i < intmux_data->channum; i++) {
> > +
> irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq,
> > + NULL, NULL);
>
> Same thing here. Shouldn't you make sure that no interrupt can fire
> anymore?
>
> > +
> > + irq_domain_remove(intmux_data->irqchip_data[i].domain);
> > + }
> > +
> > + platform_set_drvdata(pdev, NULL);
> > + clk_disable_unprepare(intmux_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);
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver
2019-12-18 9:40 ` Marc Zyngier
@ 2019-12-18 10:11 ` Joakim Zhang
0 siblings, 0 replies; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 10:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer, S.j. Wang,
kernel, festevam, dl-linux-imx, linux-kernel, devicetree,
linux-arm-kernel, Andy Duan, Aisheng Dong
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月18日 17:41
> 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; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller
> driver
>
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > Enable INTMUX interrupt controller driver for i.MX platforms.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > drivers/irqchip/Kconfig | 6 ++++++
> > drivers/irqchip/Makefile | 1 +
> > 2 files changed, 7 insertions(+)
> >
> > 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
>
> Please merge this with the previous patch, it doesn't really warrant a separate
> patch.
>
> Thanks,
Got it. Thanks.
Best Regards,
Joakim Zhang
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
2019-12-18 9:45 ` Marc Zyngier
@ 2019-12-18 10:21 ` Joakim Zhang
2019-12-18 11:34 ` Joakim Zhang
0 siblings, 1 reply; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 10:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer, S.j. Wang,
kernel, festevam, dl-linux-imx, linux-kernel, devicetree,
linux-arm-kernel, Andy Duan, Aisheng Dong
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2019年12月18日 17:45
> 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; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt
> multiplexer
>
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > This patch adds the DT bindings for the NXP INTMUX interrupt
> > multiplexer found in the i.MX8 family SoCs.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > .../interrupt-controller/fsl,intmux.txt | 34
> > +++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt
> >
> > diff --git
> >
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.tx
> > t
> >
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.tx
> > t
> > new file mode 100644
> > index 000000000000..be3c6848f36c
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.tx
> > t
> > @@ -0,0 +1,34 @@
> > +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 1.
> > +
> > +Optional properties:
> > +
> > +- fsl,intmux_chans: The number of channels used for interrupt
> > source. The
> > + Maximum value is 8.
> > +
> > +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>;
> > + fsl,intmux_chans = <1>;
> > + };
> > +
>
> What I don't understand is how the interrupt descriptor can indicate which
> channel it is multiplexed on. The driver doesn't makes this clear either, and I
> strongly suspect that it was never tested with more than a single channel...
Yes, to be frank, I tested with a signle channel, I will take this into consideration. Thanks.
Best Regards,
Joakim Zhang
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
2019-12-18 10:21 ` Joakim Zhang
@ 2019-12-18 11:34 ` Joakim Zhang
2019-12-18 11:49 ` Marc Zyngier
0 siblings, 1 reply; 12+ messages in thread
From: Joakim Zhang @ 2019-12-18 11:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer, S.j. Wang,
kernel, festevam, dl-linux-imx, linux-kernel, devicetree,
linux-arm-kernel, Andy Duan, Aisheng Dong
> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2019年12月18日 18:22
> To: 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; S.j.
> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>
> Subject: RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt
> multiplexer
>
>
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: 2019年12月18日 17:45
> > 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;
> S.j.
> > Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
> > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; Andy Duan <fugang.duan@nxp.com>;
> > Aisheng Dong <aisheng.dong@nxp.com>
> > Subject: Re: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX
> > interrupt multiplexer
> >
> > On 2019-12-18 07:20, Joakim Zhang wrote:
> > > This patch adds the DT bindings for the NXP INTMUX interrupt
> > > multiplexer found in the i.MX8 family SoCs.
> > >
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > > .../interrupt-controller/fsl,intmux.txt | 34
> > > +++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.tx
> > > t
> > >
> > > diff --git
> > >
> > > a/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.
> > > tx
> > > t
> > >
> > > b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.
> > > tx
> > > t
> > > new file mode 100644
> > > index 000000000000..be3c6848f36c
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.
> > > tx
> > > t
> > > @@ -0,0 +1,34 @@
> > > +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 1.
> > > +
> > > +Optional properties:
> > > +
> > > +- fsl,intmux_chans: The number of channels used for interrupt
> > > source. The
> > > + Maximum value is 8.
> > > +
> > > +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>;
> > > + fsl,intmux_chans = <1>;
> > > + };
> > > +
> >
> > What I don't understand is how the interrupt descriptor can indicate
> > which channel it is multiplexed on. The driver doesn't makes this
> > clear either, and I strongly suspect that it was never tested with more than a
> single channel...
>
> Yes, to be frank, I tested with a signle channel, I will take this into
> consideration. Thanks.
Hi Marc,
I tested channels from 1 to 8, and no issue found.
We register irq handler with irq_set_chained_handler_and_data(), so the interrupt descriptor could find the controller's private data, and channel index is one part of private data.
I think this can explain the interrupt descriptor how to indicate which channel it is multiplexed.
> Best Regards,
> Joakim Zhang
> > Thanks,
> >
> > M.
> > --
> > Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer
2019-12-18 11:34 ` Joakim Zhang
@ 2019-12-18 11:49 ` Marc Zyngier
0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2019-12-18 11:49 UTC (permalink / raw)
To: Joakim Zhang
Cc: tglx, jason, robh+dt, mark.rutland, shawnguo, s.hauer, S.j. Wang,
kernel, festevam, dl-linux-imx, linux-kernel, devicetree,
linux-arm-kernel, Andy Duan, Aisheng Dong
On 2019-12-18 11:34, Joakim Zhang wrote:
>> -----Original Message-----
>> From: Joakim Zhang <qiangqing.zhang@nxp.com>
>> Sent: 2019年12月18日 18:22
>> To: 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;
>> S.j.
>> Wang <shengjiu.wang@nxp.com>; kernel@pengutronix.de;
>> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; Andy Duan
>> <fugang.duan@nxp.com>;
>> Aisheng Dong <aisheng.dong@nxp.com>
>> Subject: RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX
>> interrupt
>> multiplexer
[...]
>> > What I don't understand is how the interrupt descriptor can
>> indicate
>> > which channel it is multiplexed on. The driver doesn't makes this
>> > clear either, and I strongly suspect that it was never tested with
>> more than a
>> single channel...
>>
>> Yes, to be frank, I tested with a signle channel, I will take this
>> into
>> consideration. Thanks.
> Hi Marc,
>
> I tested channels from 1 to 8, and no issue found.
>
> We register irq handler with irq_set_chained_handler_and_data(), so
> the interrupt descriptor could find the controller's private data,
> and
> channel index is one part of private data.
> I think this can explain the interrupt descriptor how to indicate
> which channel it is multiplexed.
But that doesn't explain how the driver can find which channel a given
interrupts is wired to. Nothing in your binding shows how you can
extract
the channel number from the interrupt descriptor. Nothing in the driver
even *computes* a channel number.
As far as I can see, you register a bunch of domains, all with the same
OF node, so all your interrupts end-up with the same domain. Is it
really
what you expect?
This driver looks terribly wrong.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-12-18 11:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 7:20 [PATCH 0/3] drivers/irqchip: Add NXP INTMUX interrupt Joakim Zhang
2019-12-18 7:20 ` [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt multiplexer Joakim Zhang
2019-12-18 9:45 ` Marc Zyngier
2019-12-18 10:21 ` Joakim Zhang
2019-12-18 11:34 ` Joakim Zhang
2019-12-18 11:49 ` Marc Zyngier
2019-12-18 7:20 ` [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support Joakim Zhang
2019-12-18 9:37 ` Marc Zyngier
2019-12-18 10:11 ` Joakim Zhang
2019-12-18 7:20 ` [PATCH 3/3] drivers/irqchip: enable INTMUX interrupt controller driver Joakim Zhang
2019-12-18 9:40 ` Marc Zyngier
2019-12-18 10:11 ` Joakim Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).