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

Commit Message

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

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

Signed-off-by: Joakim Zhang <[e-mail]>
---
 .../interrupt-controller/fsl,intmux.txt       | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,intmux.txt

Comments

Marc Zyngier Dec. 18, 2019, 9:45 a.m. UTC | #1

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 <[e-mail]>
> ---
>  .../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.

Joakim Zhang Dec. 18, 2019, 10:21 a.m. UTC | #2

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: 2019年12月18日 17:45
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; S.j.
> Wang <[e-mail]>; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; Andy Duan <[e-mail]>;
> Aisheng Dong <[e-mail]>
> 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 <[e-mail]>
> > ---
> >  .../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...

Joakim Zhang Dec. 18, 2019, 11:34 a.m. UTC | #3

> -----Original Message-----
> From: Joakim Zhang <[e-mail]>
> Sent: 2019年12月18日 18:22
> To: Marc Zyngier <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; S.j.
> Wang <[e-mail]>; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; Andy Duan <[e-mail]>;
> Aisheng Dong <[e-mail]>
> Subject: RE: [PATCH 1/3] dt-bindings/irq: add binding for NXP INTMUX interrupt
> multiplexer
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <[e-mail]>
> > Sent: 2019年12月18日 17:45
> > To: Joakim Zhang <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> S.j.
> > Wang <[e-mail]>; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]; Andy Duan <[e-mail]>;
> > Aisheng Dong <[e-mail]>
> > 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 <[e-mail]>
> > > ---
> > >  .../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...

Marc Zyngier Dec. 18, 2019, 11:49 a.m. UTC | #4

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

Patch

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>;
+	};
+