* [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
* 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 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
* [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
* 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 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
* [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 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 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
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).