[PATCH v2 0/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[PATCH v2 0/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

On RZ/N1 devices, there are lots of GPIO interrupts that are multiplexed before
getting to the GIC interrupt controller. Other than the multiplexing, there is
no other logic applied to the signals.

The multiplexing cannot be handled dynamically because there is another CPU that
runs firmware. It's likely that the firmware will use some of these GPIO
interrupts and so we don't want them to move around.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v2:
 - Split DT bindings into separate patch.
 - Use interrupt-map to allow the GPIO controller info to be specified
   as part of the irq.
 - Don't show status in binding examples.
 - Don't show the soc/board split in binding doc.
 - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.

Phil Edworthy (2):
  dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding
  irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

 .../interrupt-controller/renesas,rzn1-mux.txt |  92 +++++++
 drivers/irqchip/Kconfig                       |  10 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/rzn1-irq-mux.c                | 235 ++++++++++++++++++
 4 files changed, 338 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
 create mode 100644 drivers/irqchip/rzn1-irq-mux.c

-- 
2.17.1

[PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding

[Phil Edworthy]

Add device binding documentation for the Renesas RZ/N1 GPIO interrupt
multiplexer.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v2:
 - Use interrupt-map to allow the GPIO controller info to be specified
   as part of the irq.
 - Don't show status in binding examples.
 - Don't show the soc/board split in binding doc.
---
 .../interrupt-controller/renesas,rzn1-mux.txt | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
new file mode 100644
index 000000000000..0b4ba27c00ef
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
@@ -0,0 +1,92 @@
+* Renesas RZ/N1 GPIO Interrupt Multiplexer
+
+On Renesas RZ/N1 devices, there are several GPIO Controllers each with a number
+of interrupt outputs. All of the interrupts from the GPIO Controllers are passed
+to the GPIO Interrupt Multiplexer, which selects a sub-set of the interrupts to
+pass onto the system interrupt controller.
+
+A single node in the device tree is used to describe the GPIO IRQ Muxer.
+
+Required properties:
+- compatible: SoC-specific compatible string "renesas,<soc-specific>-gpioirqmux"
+  followed by "renesas,rzn1-gpioirqmux" as fallback. The SoC-specific compatible
+  strings must be one of:
+	"renesas,r9a06g032-gpioirqmux" for RZ/N1D
+	"renesas,r9a06g033-gpioirqmux" for RZ/N1S
+- interrupt-controller: Identifies the node as an interrupt controller.
+- #interrupt-cells: should be <1>. The meaning of the cells is the input
+   interrupt index, 0 to 95.
+- reg: Base address and size of GPIO IRQ Muxer registers.
+- interrupts: This is a list of interrupt specifiers. Each interrupt consists of
+   three numbers that represent:
+     - (a) the index of the GPIO Interrupt Multiplexer output interrupt (0..7)
+     - (b) the index of the GPIO Controller input interrupt (0..2)
+     - (c) the interrupt index of the GPIO Controller input interrupt (0..31).
+- interrupt-parent: A phandle for a local node that specifies an interrupt-map.
+   The interrupt-map node must specify #interrupt-cells = <3>, and an
+   interrupt-map property. The interrupt-map is used to translate the interrupt
+   specifier to the output interrupts. It is used in conjunction with an
+   interrupt-map-mask property to mask (b) and (c) from the interrupt specifier
+   so that essentially there is a direct map from (a) to the output interrupt.
+   Therefore (b) and (c) can be used to determine the interrupt source and
+   configure the hardware accordingly. For information on interrupts,
+   see Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+Example:
+
+	The following is an example for the RZ/N1D SoC.
+
+	gpioirqmux: gpioirqmux@51000480 {
+		compatible = "renesas,r9a06g032-gpioirqmux",
+				"renesas,rzn1-gpioirqmux";
+		reg = <0x51000480 0x20>;
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupt-parent = <&gpioirqmux_map>;
+		interrupts =
+			<0 1 17>,	/* gpio1a 17 */
+			<1 1 24>,	/* gpio1a 24 */
+			<2 1 26>;	/* gpio1a 26 */
+
+		gpioirqmux_map: gpioirqmux-map {
+			#interrupt-cells = <3>;
+			#address-cells = <0>;
+			interrupt-map-mask = <7 0 0>;
+			interrupt-map =
+				<0 0 0 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
+				<1 0 0 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
+				<2 0 0 &gic GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
+				<3 0 0 &gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
+				<4 0 0 &gic GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+				<5 0 0 &gic GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
+				<6 0 0 &gic GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
+				<7 0 0 &gic GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+		};
+	};
+
+	gpio1: gpio@5000c000 {
+		compatible = "snps,dw-apb-gpio";
+		reg = <0x5000c000 0x80>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clock-names = "bus";
+		clocks = <&sysctrl R9A06G032_HCLK_GPIO1>;
+
+		gpio1a: gpio-controller@0 {
+			compatible = "snps,dw-apb-gpio-port";
+			bank-name = "gpio1a";
+			gpio-controller;
+			#gpio-cells = <2>;
+			snps,nr-gpios = <32>;
+			reg = <0>;
+
+			interrupt-controller;
+			interrupt-parent = <&gpioirqmux>;
+			interrupts =   < 0  1  2  3  4  5  6  7
+					 8  9 10 11 12 13 14 15
+					16 17 18 19 20 21 22 23
+					24 25 26 27 28 29 30 31 >;
+			#interrupt-cells = <2>;
+		};
+	};
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding

[Rob Herring]

On Tue, Oct 30, 2018 at 10:44:37AM +0000, Phil Edworthy wrote:
> Add device binding documentation for the Renesas RZ/N1 GPIO interrupt
> multiplexer.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v2:
>  - Use interrupt-map to allow the GPIO controller info to be specified
>    as part of the irq.
>  - Don't show status in binding examples.
>  - Don't show the soc/board split in binding doc.
> ---
>  .../interrupt-controller/renesas,rzn1-mux.txt | 92 +++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
> new file mode 100644
> index 000000000000..0b4ba27c00ef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
> @@ -0,0 +1,92 @@
> +* Renesas RZ/N1 GPIO Interrupt Multiplexer
> +
> +On Renesas RZ/N1 devices, there are several GPIO Controllers each with a number
> +of interrupt outputs. All of the interrupts from the GPIO Controllers are passed
> +to the GPIO Interrupt Multiplexer, which selects a sub-set of the interrupts to

s/sub-set/subset/

> +pass onto the system interrupt controller.
> +
> +A single node in the device tree is used to describe the GPIO IRQ Muxer.
> +
> +Required properties:
> +- compatible: SoC-specific compatible string "renesas,<soc-specific>-gpioirqmux"
> +  followed by "renesas,rzn1-gpioirqmux" as fallback. The SoC-specific compatible
> +  strings must be one of:
> +	"renesas,r9a06g032-gpioirqmux" for RZ/N1D
> +	"renesas,r9a06g033-gpioirqmux" for RZ/N1S
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- #interrupt-cells: should be <1>. The meaning of the cells is the input
> +   interrupt index, 0 to 95.
> +- reg: Base address and size of GPIO IRQ Muxer registers.
> +- interrupts: This is a list of interrupt specifiers. Each interrupt consists of
> +   three numbers that represent:
> +     - (a) the index of the GPIO Interrupt Multiplexer output interrupt (0..7)
> +     - (b) the index of the GPIO Controller input interrupt (0..2)
> +     - (c) the interrupt index of the GPIO Controller input interrupt (0..31).
> +- interrupt-parent: A phandle for a local node that specifies an interrupt-map.
> +   The interrupt-map node must specify #interrupt-cells = <3>, and an
> +   interrupt-map property. The interrupt-map is used to translate the interrupt
> +   specifier to the output interrupts. It is used in conjunction with an
> +   interrupt-map-mask property to mask (b) and (c) from the interrupt specifier
> +   so that essentially there is a direct map from (a) to the output interrupt.
> +   Therefore (b) and (c) can be used to determine the interrupt source and
> +   configure the hardware accordingly. For information on interrupts,
> +   see Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> +Example:
> +
> +	The following is an example for the RZ/N1D SoC.

This looks a bit strange...

> +
> +	gpioirqmux: gpioirqmux@51000480 {
> +		compatible = "renesas,r9a06g032-gpioirqmux",
> +				"renesas,rzn1-gpioirqmux";
> +		reg = <0x51000480 0x20>;
> +

> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupt-parent = <&gpioirqmux_map>;
> +		interrupts =
> +			<0 1 17>,	/* gpio1a 17 */
> +			<1 1 24>,	/* gpio1a 24 */
> +			<2 1 26>;	/* gpio1a 26 */

Remove all these and...

> +
> +		gpioirqmux_map: gpioirqmux-map {

Move everything in this node up a level.

> +			#interrupt-cells = <3>;

This should be 1.

Or perhaps 2 cells if you need the GPIO controller index (cell 2 above). 
But is that numbering really meaningful to the GPIO irq mux or does it 
just have Nx32 inputs? #interrupt-cells should be defined based on the 
GPIO irq mux, not what it is connected to. You could always have a 
single linear number space and mask out bits to get banks if you need 
to.

> +			#address-cells = <0>;
> +			interrupt-map-mask = <7 0 0>;

Then 1 cell here, but the mask should be 31.

> +			interrupt-map =
> +				<0 0 0 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,

Then this should be:

<17 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>

> +				<1 0 0 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,

<24 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>

If I understand what you are trying to do. :)

If the 0-7 numbering and mapping to the downstream numbering was 
important (i.e. the 1 combined with 1 and 24 in the interrupts 
property), then you can use the index of the interrupt-map entries for 
that.

> +				<2 0 0 &gic GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
> +				<3 0 0 &gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
> +				<4 0 0 &gic GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> +				<5 0 0 &gic GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> +				<6 0 0 &gic GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> +				<7 0 0 &gic GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;

These all seem to be unused?

> +		};
> +	};
> +
> +	gpio1: gpio@5000c000 {
> +		compatible = "snps,dw-apb-gpio";
> +		reg = <0x5000c000 0x80>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clock-names = "bus";
> +		clocks = <&sysctrl R9A06G032_HCLK_GPIO1>;
> +
> +		gpio1a: gpio-controller@0 {
> +			compatible = "snps,dw-apb-gpio-port";
> +			bank-name = "gpio1a";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			snps,nr-gpios = <32>;
> +			reg = <0>;
> +
> +			interrupt-controller;
> +			interrupt-parent = <&gpioirqmux>;
> +			interrupts =   < 0  1  2  3  4  5  6  7
> +					 8  9 10 11 12 13 14 15
> +					16 17 18 19 20 21 22 23
> +					24 25 26 27 28 29 30 31 >;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> -- 
> 2.17.1
> 

RE: [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding

[Phil Edworthy]

Hi Rob,

Many thanks for a quick review!

On 30 October 2018 23:04, Rob Herring wrote:
> On Tue, Oct 30, 2018 at 10:44:37AM +0000, Phil Edworthy wrote:
> > Add device binding documentation for the Renesas RZ/N1 GPIO interrupt
> > multiplexer.
> >
<snip>

> This looks a bit strange...
> 
> > +
> > +	gpioirqmux: gpioirqmux@51000480 {
> > +		compatible = "renesas,r9a06g032-gpioirqmux",
> > +				"renesas,rzn1-gpioirqmux";
> > +		reg = <0x51000480 0x20>;
> > +
> 
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		interrupt-parent = <&gpioirqmux_map>;
> > +		interrupts =
> > +			<0 1 17>,	/* gpio1a 17 */
> > +			<1 1 24>,	/* gpio1a 24 */
> > +			<2 1 26>;	/* gpio1a 26 */
> 
> Remove all these and...
>
> > +
> > +		gpioirqmux_map: gpioirqmux-map {
> 
> Move everything in this node up a level.
> 
> > +			#interrupt-cells = <3>;
> 
> This should be 1.
> 
> Or perhaps 2 cells if you need the GPIO controller index (cell 2 above).
> But is that numbering really meaningful to the GPIO irq mux or does it just
> have Nx32 inputs? #interrupt-cells should be defined based on the GPIO irq
> mux, not what it is connected to. You could always have a single linear
> number space and mask out bits to get banks if you need to.
You are absolutely correct, the irq mux should not concern itself with where
interrupts came from, all it sees is 96 inputs.


> > +			#address-cells = <0>;
> > +			interrupt-map-mask = <7 0 0>;
> 
> Then 1 cell here, but the mask should be 31.
> 
> > +			interrupt-map =
> > +				<0 0 0 &gic GIC_SPI 103
> IRQ_TYPE_LEVEL_HIGH>,
> 
> Then this should be:
> 
> <17 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>
> 
> > +				<1 0 0 &gic GIC_SPI 104
> IRQ_TYPE_LEVEL_HIGH>,
> 
> <24 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>
> 
> If I understand what you are trying to do. :)
I think you do!

> 
> If the 0-7 numbering and mapping to the downstream numbering was
> important (i.e. the 1 combined with 1 and 24 in the interrupts property), then
> you can use the index of the interrupt-map entries for that.
Yes, the 0-7 numbering and mapping are important as we need to pin the input
interrupt to a specific output interrupt due to firmware running on another core.
Using the index makes complete sense.

> > +				<2 0 0 &gic GIC_SPI 105
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<3 0 0 &gic GIC_SPI 106
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<4 0 0 &gic GIC_SPI 107
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<5 0 0 &gic GIC_SPI 108
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<6 0 0 &gic GIC_SPI 109
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<7 0 0 &gic GIC_SPI 110
> IRQ_TYPE_LEVEL_HIGH>;
> 
> These all seem to be unused?
I think that's just an artefact of the way I abused interrupt-map!

Thanks
Phil

> > +		};
> > +	};
> > +
> > +	gpio1: gpio@5000c000 {
> > +		compatible = "snps,dw-apb-gpio";
> > +		reg = <0x5000c000 0x80>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		clock-names = "bus";
> > +		clocks = <&sysctrl R9A06G032_HCLK_GPIO1>;
> > +
> > +		gpio1a: gpio-controller@0 {
> > +			compatible = "snps,dw-apb-gpio-port";
> > +			bank-name = "gpio1a";
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			snps,nr-gpios = <32>;
> > +			reg = <0>;
> > +
> > +			interrupt-controller;
> > +			interrupt-parent = <&gpioirqmux>;
> > +			interrupts =   < 0  1  2  3  4  5  6  7
> > +					 8  9 10 11 12 13 14 15
> > +					16 17 18 19 20 21 22 23
> > +					24 25 26 27 28 29 30 31 >;
> > +			#interrupt-cells = <2>;
> > +		};
> > +	};
> > --
> > 2.17.1
>

RE: [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding

[Phil Edworthy]

Hi Rob,

Many thanks for a quick review!

On 30 October 2018 23:04, Rob Herring wrote:
> On Tue, Oct 30, 2018 at 10:44:37AM +0000, Phil Edworthy wrote:
> > Add device binding documentation for the Renesas RZ/N1 GPIO interrupt
> > multiplexer.
> >
<snip>

> This looks a bit strange...
> 
> > +
> > +	gpioirqmux: gpioirqmux@51000480 {
> > +		compatible = "renesas,r9a06g032-gpioirqmux",
> > +				"renesas,rzn1-gpioirqmux";
> > +		reg = <0x51000480 0x20>;
> > +
> 
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		interrupt-parent = <&gpioirqmux_map>;
> > +		interrupts =
> > +			<0 1 17>,	/* gpio1a 17 */
> > +			<1 1 24>,	/* gpio1a 24 */
> > +			<2 1 26>;	/* gpio1a 26 */
> 
> Remove all these and...
>
> > +
> > +		gpioirqmux_map: gpioirqmux-map {
> 
> Move everything in this node up a level.
> 
> > +			#interrupt-cells = <3>;
> 
> This should be 1.
> 
> Or perhaps 2 cells if you need the GPIO controller index (cell 2 above).
> But is that numbering really meaningful to the GPIO irq mux or does it just
> have Nx32 inputs? #interrupt-cells should be defined based on the GPIO irq
> mux, not what it is connected to. You could always have a single linear
> number space and mask out bits to get banks if you need to.
You are absolutely correct, the irq mux should not concern itself with where
interrupts came from, all it sees is 96 inputs.


> > +			#address-cells = <0>;
> > +			interrupt-map-mask = <7 0 0>;
> 
> Then 1 cell here, but the mask should be 31.
> 
> > +			interrupt-map =
> > +				<0 0 0 &gic GIC_SPI 103
> IRQ_TYPE_LEVEL_HIGH>,
> 
> Then this should be:
> 
> <17 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>
> 
> > +				<1 0 0 &gic GIC_SPI 104
> IRQ_TYPE_LEVEL_HIGH>,
> 
> <24 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>
> 
> If I understand what you are trying to do. :)
I think you do!

> 
> If the 0-7 numbering and mapping to the downstream numbering was
> important (i.e. the 1 combined with 1 and 24 in the interrupts property), then
> you can use the index of the interrupt-map entries for that.
Yes, the 0-7 numbering and mapping are important as we need to pin the input
interrupt to a specific output interrupt due to firmware running on another core.
Using the index makes complete sense.

> > +				<2 0 0 &gic GIC_SPI 105
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<3 0 0 &gic GIC_SPI 106
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<4 0 0 &gic GIC_SPI 107
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<5 0 0 &gic GIC_SPI 108
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<6 0 0 &gic GIC_SPI 109
> IRQ_TYPE_LEVEL_HIGH>,
> > +				<7 0 0 &gic GIC_SPI 110
> IRQ_TYPE_LEVEL_HIGH>;
> 
> These all seem to be unused?
I think that's just an artefact of the way I abused interrupt-map!

Thanks
Phil

> > +		};
> > +	};
> > +
> > +	gpio1: gpio@5000c000 {
> > +		compatible = "snps,dw-apb-gpio";
> > +		reg = <0x5000c000 0x80>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		clock-names = "bus";
> > +		clocks = <&sysctrl R9A06G032_HCLK_GPIO1>;
> > +
> > +		gpio1a: gpio-controller@0 {
> > +			compatible = "snps,dw-apb-gpio-port";
> > +			bank-name = "gpio1a";
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			snps,nr-gpios = <32>;
> > +			reg = <0>;
> > +
> > +			interrupt-controller;
> > +			interrupt-parent = <&gpioirqmux>;
> > +			interrupts =   < 0  1  2  3  4  5  6  7
> > +					 8  9 10 11 12 13 14 15
> > +					16 17 18 19 20 21 22 23
> > +					24 25 26 27 28 29 30 31 >;
> > +			#interrupt-cells = <2>;
> > +		};
> > +	};
> > --
> > 2.17.1
>

[PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
configured to have 32 interrupt outputs, so we have a total of 96 GPIO
interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
aren't latched, so there is nothing to do in this driver when an interrupt
is received, other than tell the corresponding GPIO block.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v2:
 - Use interrupt-map to allow the GPIO controller info to be specified
   as part of the irq.
 - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
---
 drivers/irqchip/Kconfig        |  10 ++
 drivers/irqchip/Makefile       |   1 +
 drivers/irqchip/rzn1-irq-mux.c | 235 +++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/irqchip/rzn1-irq-mux.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 96451b581452..3a60a8af60dd 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -204,6 +204,16 @@ config RENESAS_IRQC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config RENESAS_RZN1_IRQ_MUX
+	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
+	depends on ARCH_RZN1
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to add support for the GPIO IRQ multiplexer embedded
+	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
+	  the interrupts coming from the GPIO controllers are used.
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b822199445ff..b090f84dd42e 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
 obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
+obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
 obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
diff --git a/drivers/irqchip/rzn1-irq-mux.c b/drivers/irqchip/rzn1-irq-mux.c
new file mode 100644
index 000000000000..767ce67e34d2
--- /dev/null
+++ b/drivers/irqchip/rzn1-irq-mux.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ/N1 GPIO Interrupt Multiplexer
+ *
+ * Copyright (C) 2018 Renesas Electronics Europe Limited
+ *
+ * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each configured
+ * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
+ * All of these are passed to the GPIO IRQ Muxer, which selects 8 of the GPIO
+ * interrupts to pass onto the GIC.
+ */
+
+#include <linux/bitops.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_irq.h>
+#include <linux/of_platform.h>
+
+#define GPIO_IRQ_SPEC_SIZE	3
+#define MAX_NR_GPIO_CONTROLLERS	3
+#define MAX_NR_GPIO_IRQ		32
+#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS * MAX_NR_GPIO_IRQ)
+#define MAX_NR_OUTPUT_IRQS	8
+
+struct irqmux_priv;
+struct irqmux_one {
+	unsigned int mapped_irq;
+	unsigned int input_irq_nr;
+	struct irqmux_priv *priv;
+};
+
+struct irqmux_priv {
+	struct device *dev;
+	struct irq_chip irq_chip;
+	struct irq_domain *irq_domain;
+	unsigned int nr_irqs;
+	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS];
+};
+
+static void irqmux_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
+	struct irqmux_priv *priv = girq->priv;
+	unsigned int irq;
+
+	chained_irq_enter(chip, desc);
+
+	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
+	generic_handle_irq(irq);
+
+	chained_irq_exit(chip, desc);
+}
+
+static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	struct irqmux_priv *priv = h->host_data;
+
+	irq_set_chip_data(irq, h->host_data);
+	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops irqmux_domain_ops = {
+	.map	= irqmux_domain_map,
+};
+
+static int irqmux_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *res;
+	u32 __iomem *regs;
+	struct irqmux_priv *priv;
+	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
+	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
+	unsigned int irqs_out_used = 0;
+	unsigned int i;
+	int nr_irqs;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	nr_irqs = of_irq_count(np);
+	if (nr_irqs < 0)
+		return nr_irqs;
+
+	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
+		dev_err(dev, "too many output interrupts\n");
+		return -ENOENT;
+	}
+
+	priv->nr_irqs = nr_irqs;
+
+	/* Get the interrupt specifers */
+	if (of_property_read_u32_array(dev->of_node, "interrupts",
+				       (u32 *)int_specs,
+				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
+		dev_err(dev, "cannot get interrupt specifiers\n");
+		return -ENOENT;
+	}
+
+	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
+
+	/* Check the interrupt specifiers */
+	for (i = 0; i < priv->nr_irqs; i++) {
+		u32 *int_spec = int_specs[i];
+		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ + int_spec[2];
+
+		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0], int_spec[1],
+			 int_spec[2]);
+
+		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
+		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
+		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
+			dev_err(dev, "invalid interrupt args\n");
+			return -ENOENT;
+		}
+
+		if (irqs_out_used & BIT(int_spec[0]) ||
+		    test_bit(input_irq, irqs_in_used)) {
+			dev_err(dev, "irq %d already used\n", i);
+			return -ENOENT;
+		}
+
+		irqs_out_used |= BIT(int_spec[0]);
+		set_bit(input_irq, irqs_in_used);
+	}
+
+	/* Create IRQ domain for the interrupts coming from the GPIO blocks */
+	priv->irq_chip.name = dev_name(dev);
+	priv->irq_domain = irq_domain_add_linear(np, MAX_NR_INPUT_IRQS,
+						 &irqmux_domain_ops, priv);
+	if (!priv->irq_domain)
+		return -ENOMEM;
+
+	/* Setup the interrupts */
+	for (i = 0; i < priv->nr_irqs; i++) {
+		struct of_phandle_args ofirq;
+		u32 *int_spec = int_specs[i];
+		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ + int_spec[2];
+		struct irqmux_one *irq = &priv->irq[i];
+
+		if (of_irq_parse_one(dev->of_node, i, &ofirq)) {
+			ret = -ENOENT;
+			goto err;
+		}
+
+		priv->irq[i].mapped_irq = irq_create_of_mapping(&ofirq);
+		if (!priv->irq[i].mapped_irq) {
+			dev_err(dev, "cannot get interrupt\n");
+			ret = -ENOENT;
+			goto err;
+		}
+
+		irq->priv = priv;
+		irq->input_irq_nr = input_irq;
+
+		irq_set_chained_handler_and_data(irq->mapped_irq,
+						 irqmux_handler, irq);
+
+		/* Set up the hardware to pass the interrupt through */
+		writel(irq->input_irq_nr, &regs[int_spec[0]]);
+	}
+
+	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
+
+	return 0;
+
+err:
+	while (i--) {
+		struct irqmux_one *irq = &priv->irq[i];
+
+		irq_set_chained_handler_and_data(irq->mapped_irq, NULL, NULL);
+		irq_dispose_mapping(irq->mapped_irq);
+	}
+	irq_domain_remove(priv->irq_domain);
+
+	return 0;
+}
+
+static int irqmux_remove(struct platform_device *pdev)
+{
+	struct irqmux_priv *priv = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	for (i = 0; i < priv->nr_irqs; i++) {
+		struct irqmux_one *irq = &priv->irq[i];
+
+		irq_set_chained_handler_and_data(irq->mapped_irq, NULL, NULL);
+		irq_dispose_mapping(irq->mapped_irq);
+	}
+	irq_domain_remove(priv->irq_domain);
+
+	return 0;
+}
+
+static const struct of_device_id irqmux_match[] = {
+	{ .compatible = "renesas,rzn1-gpioirqmux", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, irqmux_match);
+
+static struct platform_driver irqmux_driver = {
+	.driver = {
+		.name = "gpio_irq_mux",
+		.owner = THIS_MODULE,
+		.of_match_table = irqmux_match,
+	},
+	.probe = irqmux_probe,
+	.remove = irqmux_remove,
+};
+
+module_platform_driver(irqmux_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
+MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Marc Zyngier]

Hi Phil,

On Tue, 30 Oct 2018 10:44:38 +0000,
Phil Edworthy <phil.edworthy@renesas.com> wrote:
> 
> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> aren't latched, so there is nothing to do in this driver when an interrupt
> is received, other than tell the corresponding GPIO block.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v2:
>  - Use interrupt-map to allow the GPIO controller info to be specified
>    as part of the irq.
>  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
> ---
>  drivers/irqchip/Kconfig        |  10 ++
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/rzn1-irq-mux.c | 235 +++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 96451b581452..3a60a8af60dd 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -204,6 +204,16 @@ config RENESAS_IRQC
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN
>  
> +config RENESAS_RZN1_IRQ_MUX
> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> +	depends on ARCH_RZN1
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Say yes here to add support for the GPIO IRQ multiplexer embedded
> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> +	  the interrupts coming from the GPIO controllers are used.
> +
>  config ST_IRQCHIP
>  	bool
>  	select REGMAP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b822199445ff..b090f84dd42e 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
>  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> diff --git a/drivers/irqchip/rzn1-irq-mux.c b/drivers/irqchip/rzn1-irq-mux.c
> new file mode 100644
> index 000000000000..767ce67e34d2
> --- /dev/null
> +++ b/drivers/irqchip/rzn1-irq-mux.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/N1 GPIO Interrupt Multiplexer
> + *
> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> + *
> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each configured
> + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
> + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of the GPIO
> + * interrupts to pass onto the GIC.
> + */
> +
> +#include <linux/bitops.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_irq.h>
> +#include <linux/of_platform.h>
> +
> +#define GPIO_IRQ_SPEC_SIZE	3
> +#define MAX_NR_GPIO_CONTROLLERS	3
> +#define MAX_NR_GPIO_IRQ		32
> +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS * MAX_NR_GPIO_IRQ)
> +#define MAX_NR_OUTPUT_IRQS	8
> +
> +struct irqmux_priv;
> +struct irqmux_one {
> +	unsigned int mapped_irq;
> +	unsigned int input_irq_nr;
> +	struct irqmux_priv *priv;
> +};
> +
> +struct irqmux_priv {
> +	struct device *dev;
> +	struct irq_chip irq_chip;

Do we really need this to be per-device? See below.

> +	struct irq_domain *irq_domain;
> +	unsigned int nr_irqs;
> +	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS];
> +};
> +
> +static void irqmux_handler(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
> +	struct irqmux_priv *priv = girq->priv;
> +	unsigned int irq;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
> +	generic_handle_irq(irq);

No error handling? See below again, as I think this outline a
fundamental flaw in the driver.

> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
> +			     irq_hw_number_t hwirq)
> +{
> +	struct irqmux_priv *priv = h->host_data;
> +
> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops irqmux_domain_ops = {
> +	.map	= irqmux_domain_map,
> +};
> +
> +static int irqmux_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct resource *res;
> +	u32 __iomem *regs;
> +	struct irqmux_priv *priv;
> +	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
> +	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
> +	unsigned int irqs_out_used = 0;
> +	unsigned int i;
> +	int nr_irqs;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	nr_irqs = of_irq_count(np);
> +	if (nr_irqs < 0)
> +		return nr_irqs;
> +
> +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
> +		dev_err(dev, "too many output interrupts\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->nr_irqs = nr_irqs;
> +
> +	/* Get the interrupt specifers */
> +	if (of_property_read_u32_array(dev->of_node, "interrupts",
> +				       (u32 *)int_specs,
> +				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
> +		dev_err(dev, "cannot get interrupt specifiers\n");
> +		return -ENOENT;
> +	}
> +
> +	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
> +
> +	/* Check the interrupt specifiers */
> +	for (i = 0; i < priv->nr_irqs; i++) {
> +		u32 *int_spec = int_specs[i];
> +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ + int_spec[2];
> +
> +		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0], int_spec[1],
> +			 int_spec[2]);
> +
> +		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
> +		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
> +		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
> +			dev_err(dev, "invalid interrupt args\n");
> +			return -ENOENT;
> +		}
> +
> +		if (irqs_out_used & BIT(int_spec[0]) ||
> +		    test_bit(input_irq, irqs_in_used)) {
> +			dev_err(dev, "irq %d already used\n", i);
> +			return -ENOENT;
> +		}

I don't think the driver should be in the business of DT validation,
and that you should simply drop this code.

> +
> +		irqs_out_used |= BIT(int_spec[0]);
> +		set_bit(input_irq, irqs_in_used);
> +	}
> +
> +	/* Create IRQ domain for the interrupts coming from the GPIO blocks */
> +	priv->irq_chip.name = dev_name(dev);

OK, that's where I think we have a problem. Your irqchip structure
seem to only be used to display a name?!?

To start with, that's not really the primary use for this object, and
I'd like it to be a single static structure for the whole
driver. Userspace doesn't need to know about the name, so please get
rid of this.

The real issue is that you build the whole thing as a chained
interrupt controller, meaning that nothing controls the masking of the
interrupt. If, as I understand it, this IP is an interrupt router that
selects 8 out of 32 interrupts and passes them onto the GIC, then a
noisy device can just take the whole CPU down by keeping the line
asserted, and SW cannot mask it.

By the look of it, this should be turned into an interrupt hierarchy,
and not a chained interrupt. You do select IRQ_DOMAIN_HIERARCHY, and
completely fail to use the API...

> +	priv->irq_domain = irq_domain_add_linear(np, MAX_NR_INPUT_IRQS,
> +						 &irqmux_domain_ops, priv);
> +	if (!priv->irq_domain)
> +		return -ENOMEM;
> +
> +	/* Setup the interrupts */
> +	for (i = 0; i < priv->nr_irqs; i++) {
> +		struct of_phandle_args ofirq;
> +		u32 *int_spec = int_specs[i];
> +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ + int_spec[2];
> +		struct irqmux_one *irq = &priv->irq[i];
> +
> +		if (of_irq_parse_one(dev->of_node, i, &ofirq)) {
> +			ret = -ENOENT;
> +			goto err;
> +		}

Why isn't this irq_of_parse_and_map, so that we get rid of the below
create_of_mapping? And if you turn this into a full interrupt
hierarchy, this will completely go away.

> +
> +		priv->irq[i].mapped_irq = irq_create_of_mapping(&ofirq);
> +		if (!priv->irq[i].mapped_irq) {
> +			dev_err(dev, "cannot get interrupt\n");
> +			ret = -ENOENT;
> +			goto err;
> +		}
> +
> +		irq->priv = priv;
> +		irq->input_irq_nr = input_irq;
> +
> +		irq_set_chained_handler_and_data(irq->mapped_irq,
> +						 irqmux_handler, irq);
> +
> +		/* Set up the hardware to pass the interrupt through */
> +		writel(irq->input_irq_nr, &regs[int_spec[0]]);
> +	}
> +
> +	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
> +
> +	return 0;
> +
> +err:
> +	while (i--) {
> +		struct irqmux_one *irq = &priv->irq[i];
> +
> +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL, NULL);
> +		irq_dispose_mapping(irq->mapped_irq);
> +	}
> +	irq_domain_remove(priv->irq_domain);
> +
> +	return 0;
> +}
> +
> +static int irqmux_remove(struct platform_device *pdev)
> +{
> +	struct irqmux_priv *priv = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->nr_irqs; i++) {
> +		struct irqmux_one *irq = &priv->irq[i];
> +
> +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL, NULL);
> +		irq_dispose_mapping(irq->mapped_irq);
> +	}
> +	irq_domain_remove(priv->irq_domain);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id irqmux_match[] = {
> +	{ .compatible = "renesas,rzn1-gpioirqmux", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, irqmux_match);
> +
> +static struct platform_driver irqmux_driver = {
> +	.driver = {
> +		.name = "gpio_irq_mux",
> +		.owner = THIS_MODULE,
> +		.of_match_table = irqmux_match,
> +	},
> +	.probe = irqmux_probe,
> +	.remove = irqmux_remove,
> +};
> +
> +module_platform_driver(irqmux_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

To sum it up, I think the structure of this driver is flawed. Please
turn it into a full hierarchy, or alternatively tell me that I have
the wrong end of the stick!

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

Many thanks for a quick response!

On 31 October 2018 08:02, Marc Zyngier wote:
> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> >
> > On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> > interrupts. All of these are passed to the GPIO IRQ Muxer, which
> > selects
> > 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> > aren't latched, so there is nothing to do in this driver when an
> > interrupt is received, other than tell the corresponding GPIO block.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v2:
> >  - Use interrupt-map to allow the GPIO controller info to be specified
> >    as part of the irq.
> >  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
> > ---
> >  drivers/irqchip/Kconfig        |  10 ++
> >  drivers/irqchip/Makefile       |   1 +
> >  drivers/irqchip/rzn1-irq-mux.c | 235
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 246 insertions(+)
> >  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > 96451b581452..3a60a8af60dd 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -204,6 +204,16 @@ config RENESAS_IRQC
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> >
> > +config RENESAS_RZN1_IRQ_MUX
> > +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> > +	depends on ARCH_RZN1
> > +	select IRQ_DOMAIN
> > +	select IRQ_DOMAIN_HIERARCHY
> > +	help
> > +	  Say yes here to add support for the GPIO IRQ multiplexer
> embedded
> > +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> > +	  the interrupts coming from the GPIO controllers are used.
> > +
> >  config ST_IRQCHIP
> >  	bool
> >  	select REGMAP
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > b822199445ff..b090f84dd42e 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
> irq-sirfsoc.o
> >  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
> >  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
> >  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> > +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
> >  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> >  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
> >  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> > diff --git a/drivers/irqchip/rzn1-irq-mux.c
> > b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
> > 000000000000..767ce67e34d2
> > --- /dev/null
> > +++ b/drivers/irqchip/rzn1-irq-mux.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RZ/N1 GPIO Interrupt Multiplexer
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
> > + *
> > + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > +configured
> > + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
> > + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of
> > +the GPIO
> > + * interrupts to pass onto the GIC.
> > + */
> > +
> > +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
> > +
> > +#define GPIO_IRQ_SPEC_SIZE	3
> > +#define MAX_NR_GPIO_CONTROLLERS	3
> > +#define MAX_NR_GPIO_IRQ		32
> > +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
> MAX_NR_GPIO_IRQ)
> > +#define MAX_NR_OUTPUT_IRQS	8
> > +
> > +struct irqmux_priv;
> > +struct irqmux_one {
> > +	unsigned int mapped_irq;
> > +	unsigned int input_irq_nr;
> > +	struct irqmux_priv *priv;
> > +};
> > +
> > +struct irqmux_priv {
> > +	struct device *dev;
> > +	struct irq_chip irq_chip;
> 
> Do we really need this to be per-device? See below.
I thought we generally wanted everything to be per-device so that we can
cope when someone sticks two of these in a device. Am I wrong?

> 
> > +	struct irq_domain *irq_domain;
> > +	unsigned int nr_irqs;
> > +	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS]; };
> > +
> > +static void irqmux_handler(struct irq_desc *desc) {
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
> > +	struct irqmux_priv *priv = girq->priv;
> > +	unsigned int irq;
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
> > +	generic_handle_irq(irq);
> 
> No error handling? See below again, as I think this outline a fundamental flaw
> in the driver.
> 
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	struct irqmux_priv *priv = h->host_data;
> > +
> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops irqmux_domain_ops = {
> > +	.map	= irqmux_domain_map,
> > +};
> > +
> > +static int irqmux_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct resource *res;
> > +	u32 __iomem *regs;
> > +	struct irqmux_priv *priv;
> > +	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
> > +	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
> > +	unsigned int irqs_out_used = 0;
> > +	unsigned int i;
> > +	int nr_irqs;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	regs = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	nr_irqs = of_irq_count(np);
> > +	if (nr_irqs < 0)
> > +		return nr_irqs;
> > +
> > +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
> > +		dev_err(dev, "too many output interrupts\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	priv->nr_irqs = nr_irqs;
> > +
> > +	/* Get the interrupt specifers */
> > +	if (of_property_read_u32_array(dev->of_node, "interrupts",
> > +				       (u32 *)int_specs,
> > +				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
> > +		dev_err(dev, "cannot get interrupt specifiers\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
> > +
> > +	/* Check the interrupt specifiers */
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		u32 *int_spec = int_specs[i];
> > +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
> int_spec[2];
> > +
> > +		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0],
> int_spec[1],
> > +			 int_spec[2]);
> > +
> > +		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
> > +		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
> > +		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
> > +			dev_err(dev, "invalid interrupt args\n");
> > +			return -ENOENT;
> > +		}
> > +
> > +		if (irqs_out_used & BIT(int_spec[0]) ||
> > +		    test_bit(input_irq, irqs_in_used)) {
> > +			dev_err(dev, "irq %d already used\n", i);
> > +			return -ENOENT;
> > +		}
> 
> I don't think the driver should be in the business of DT validation, and that
> you should simply drop this code.
When I implement Rob H's feedback on the binding, this should no longer be
needed.

> 
> > +
> > +		irqs_out_used |= BIT(int_spec[0]);
> > +		set_bit(input_irq, irqs_in_used);
> > +	}
> > +
> > +	/* Create IRQ domain for the interrupts coming from the GPIO blocks
> */
> > +	priv->irq_chip.name = dev_name(dev);
> 
> OK, that's where I think we have a problem. Your irqchip structure seem to
> only be used to display a name?!?
Right, that wasn't the intention! So, how do I hook in my own interrupt handler
without calling irq_set_chip_and_handler()?
That's what led me to think I need an irq_chip instance.

> 
> To start with, that's not really the primary use for this object, and I'd like it to
> be a single static structure for the whole driver. Userspace doesn't need to
> know about the name, so please get rid of this.
> 
> The real issue is that you build the whole thing as a chained interrupt
> controller, meaning that nothing controls the masking of the interrupt. If, as I
> understand it, this IP is an interrupt router that selects 8 out of 32 interrupts
> and passes them onto the GIC, then a noisy device can just take the whole
> CPU down by keeping the line asserted, and SW cannot mask it.
The interrupts into this mux come from GPIO blocks that do the masking. The
GPIO blocks in this case are standard Synopsys IP blocks.
There is nothing in the irq mux hardware that can mask them, or do anything
other than select which one to use, hence why this is a chained interrupt
controller. Should I be using something else in this case?

> By the look of it, this should be turned into an interrupt hierarchy, and not a
> chained interrupt. You do select IRQ_DOMAIN_HIERARCHY, and completely
> fail to use the API...
Ok, I should not have selected that.

> 
> > +	priv->irq_domain = irq_domain_add_linear(np,
> MAX_NR_INPUT_IRQS,
> > +						 &irqmux_domain_ops, priv);
> > +	if (!priv->irq_domain)
> > +		return -ENOMEM;
> > +
> > +	/* Setup the interrupts */
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		struct of_phandle_args ofirq;
> > +		u32 *int_spec = int_specs[i];
> > +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
> int_spec[2];
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		if (of_irq_parse_one(dev->of_node, i, &ofirq)) {
> > +			ret = -ENOENT;
> > +			goto err;
> > +		}
> 
> Why isn't this irq_of_parse_and_map, so that we get rid of the below
> create_of_mapping? And if you turn this into a full interrupt hierarchy, this
> will completely go away.
That was due to the way I abused the interrupt-map, and should go.

> 
> > +
> > +		priv->irq[i].mapped_irq = irq_create_of_mapping(&ofirq);
> > +		if (!priv->irq[i].mapped_irq) {
> > +			dev_err(dev, "cannot get interrupt\n");
> > +			ret = -ENOENT;
> > +			goto err;
> > +		}
> > +
> > +		irq->priv = priv;
> > +		irq->input_irq_nr = input_irq;
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq,
> > +						 irqmux_handler, irq);
> > +
> > +		/* Set up the hardware to pass the interrupt through */
> > +		writel(irq->input_irq_nr, &regs[int_spec[0]]);
> > +	}
> > +
> > +	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
> > +
> > +	return 0;
> > +
> > +err:
> > +	while (i--) {
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL,
> NULL);
> > +		irq_dispose_mapping(irq->mapped_irq);
> > +	}
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static int irqmux_remove(struct platform_device *pdev) {
> > +	struct irqmux_priv *priv = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL,
> NULL);
> > +		irq_dispose_mapping(irq->mapped_irq);
> > +	}
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id irqmux_match[] = {
> > +	{ .compatible = "renesas,rzn1-gpioirqmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, irqmux_match);
> > +
> > +static struct platform_driver irqmux_driver = {
> > +	.driver = {
> > +		.name = "gpio_irq_mux",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = irqmux_match,
> > +	},
> > +	.probe = irqmux_probe,
> > +	.remove = irqmux_remove,
> > +};
> > +
> > +module_platform_driver(irqmux_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
> > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
> 
> To sum it up, I think the structure of this driver is flawed. Please
> turn it into a full hierarchy, or alternatively tell me that I have
> the wrong end of the stick!
I'm hoping you got the wrong end of the stick! If not, I have some
excavation ahead to work out how I should have done this...

Thanks!
Phil

> 
> Thanks,
> 
> 	M.
> 
> --
> Jazz is not dead, it just smell funny.

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

Many thanks for a quick response!

On 31 October 2018 08:02, Marc Zyngier wote:
> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> >
> > On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> > interrupts. All of these are passed to the GPIO IRQ Muxer, which
> > selects
> > 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> > aren't latched, so there is nothing to do in this driver when an
> > interrupt is received, other than tell the corresponding GPIO block.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v2:
> >  - Use interrupt-map to allow the GPIO controller info to be specified
> >    as part of the irq.
> >  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
> > ---
> >  drivers/irqchip/Kconfig        |  10 ++
> >  drivers/irqchip/Makefile       |   1 +
> >  drivers/irqchip/rzn1-irq-mux.c | 235
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 246 insertions(+)
> >  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > 96451b581452..3a60a8af60dd 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -204,6 +204,16 @@ config RENESAS_IRQC
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> >
> > +config RENESAS_RZN1_IRQ_MUX
> > +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> > +	depends on ARCH_RZN1
> > +	select IRQ_DOMAIN
> > +	select IRQ_DOMAIN_HIERARCHY
> > +	help
> > +	  Say yes here to add support for the GPIO IRQ multiplexer
> embedded
> > +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> > +	  the interrupts coming from the GPIO controllers are used.
> > +
> >  config ST_IRQCHIP
> >  	bool
> >  	select REGMAP
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > b822199445ff..b090f84dd42e 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
> irq-sirfsoc.o
> >  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
> >  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
> >  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> > +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
> >  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> >  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
> >  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> > diff --git a/drivers/irqchip/rzn1-irq-mux.c
> > b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
> > 000000000000..767ce67e34d2
> > --- /dev/null
> > +++ b/drivers/irqchip/rzn1-irq-mux.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RZ/N1 GPIO Interrupt Multiplexer
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
> > + *
> > + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > +configured
> > + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
> > + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of
> > +the GPIO
> > + * interrupts to pass onto the GIC.
> > + */
> > +
> > +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
> > +
> > +#define GPIO_IRQ_SPEC_SIZE	3
> > +#define MAX_NR_GPIO_CONTROLLERS	3
> > +#define MAX_NR_GPIO_IRQ		32
> > +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
> MAX_NR_GPIO_IRQ)
> > +#define MAX_NR_OUTPUT_IRQS	8
> > +
> > +struct irqmux_priv;
> > +struct irqmux_one {
> > +	unsigned int mapped_irq;
> > +	unsigned int input_irq_nr;
> > +	struct irqmux_priv *priv;
> > +};
> > +
> > +struct irqmux_priv {
> > +	struct device *dev;
> > +	struct irq_chip irq_chip;
> 
> Do we really need this to be per-device? See below.
I thought we generally wanted everything to be per-device so that we can
cope when someone sticks two of these in a device. Am I wrong?

> 
> > +	struct irq_domain *irq_domain;
> > +	unsigned int nr_irqs;
> > +	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS]; };
> > +
> > +static void irqmux_handler(struct irq_desc *desc) {
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
> > +	struct irqmux_priv *priv = girq->priv;
> > +	unsigned int irq;
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
> > +	generic_handle_irq(irq);
> 
> No error handling? See below again, as I think this outline a fundamental flaw
> in the driver.
> 
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	struct irqmux_priv *priv = h->host_data;
> > +
> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops irqmux_domain_ops = {
> > +	.map	= irqmux_domain_map,
> > +};
> > +
> > +static int irqmux_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct resource *res;
> > +	u32 __iomem *regs;
> > +	struct irqmux_priv *priv;
> > +	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
> > +	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
> > +	unsigned int irqs_out_used = 0;
> > +	unsigned int i;
> > +	int nr_irqs;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	regs = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	nr_irqs = of_irq_count(np);
> > +	if (nr_irqs < 0)
> > +		return nr_irqs;
> > +
> > +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
> > +		dev_err(dev, "too many output interrupts\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	priv->nr_irqs = nr_irqs;
> > +
> > +	/* Get the interrupt specifers */
> > +	if (of_property_read_u32_array(dev->of_node, "interrupts",
> > +				       (u32 *)int_specs,
> > +				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
> > +		dev_err(dev, "cannot get interrupt specifiers\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
> > +
> > +	/* Check the interrupt specifiers */
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		u32 *int_spec = int_specs[i];
> > +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
> int_spec[2];
> > +
> > +		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0],
> int_spec[1],
> > +			 int_spec[2]);
> > +
> > +		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
> > +		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
> > +		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
> > +			dev_err(dev, "invalid interrupt args\n");
> > +			return -ENOENT;
> > +		}
> > +
> > +		if (irqs_out_used & BIT(int_spec[0]) ||
> > +		    test_bit(input_irq, irqs_in_used)) {
> > +			dev_err(dev, "irq %d already used\n", i);
> > +			return -ENOENT;
> > +		}
> 
> I don't think the driver should be in the business of DT validation, and that
> you should simply drop this code.
When I implement Rob H's feedback on the binding, this should no longer be
needed.

> 
> > +
> > +		irqs_out_used |= BIT(int_spec[0]);
> > +		set_bit(input_irq, irqs_in_used);
> > +	}
> > +
> > +	/* Create IRQ domain for the interrupts coming from the GPIO blocks
> */
> > +	priv->irq_chip.name = dev_name(dev);
> 
> OK, that's where I think we have a problem. Your irqchip structure seem to
> only be used to display a name?!?
Right, that wasn't the intention! So, how do I hook in my own interrupt handler
without calling irq_set_chip_and_handler()?
That's what led me to think I need an irq_chip instance.

> 
> To start with, that's not really the primary use for this object, and I'd like it to
> be a single static structure for the whole driver. Userspace doesn't need to
> know about the name, so please get rid of this.
> 
> The real issue is that you build the whole thing as a chained interrupt
> controller, meaning that nothing controls the masking of the interrupt. If, as I
> understand it, this IP is an interrupt router that selects 8 out of 32 interrupts
> and passes them onto the GIC, then a noisy device can just take the whole
> CPU down by keeping the line asserted, and SW cannot mask it.
The interrupts into this mux come from GPIO blocks that do the masking. The
GPIO blocks in this case are standard Synopsys IP blocks.
There is nothing in the irq mux hardware that can mask them, or do anything
other than select which one to use, hence why this is a chained interrupt
controller. Should I be using something else in this case?

> By the look of it, this should be turned into an interrupt hierarchy, and not a
> chained interrupt. You do select IRQ_DOMAIN_HIERARCHY, and completely
> fail to use the API...
Ok, I should not have selected that.

> 
> > +	priv->irq_domain = irq_domain_add_linear(np,
> MAX_NR_INPUT_IRQS,
> > +						 &irqmux_domain_ops, priv);
> > +	if (!priv->irq_domain)
> > +		return -ENOMEM;
> > +
> > +	/* Setup the interrupts */
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		struct of_phandle_args ofirq;
> > +		u32 *int_spec = int_specs[i];
> > +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
> int_spec[2];
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		if (of_irq_parse_one(dev->of_node, i, &ofirq)) {
> > +			ret = -ENOENT;
> > +			goto err;
> > +		}
> 
> Why isn't this irq_of_parse_and_map, so that we get rid of the below
> create_of_mapping? And if you turn this into a full interrupt hierarchy, this
> will completely go away.
That was due to the way I abused the interrupt-map, and should go.

> 
> > +
> > +		priv->irq[i].mapped_irq = irq_create_of_mapping(&ofirq);
> > +		if (!priv->irq[i].mapped_irq) {
> > +			dev_err(dev, "cannot get interrupt\n");
> > +			ret = -ENOENT;
> > +			goto err;
> > +		}
> > +
> > +		irq->priv = priv;
> > +		irq->input_irq_nr = input_irq;
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq,
> > +						 irqmux_handler, irq);
> > +
> > +		/* Set up the hardware to pass the interrupt through */
> > +		writel(irq->input_irq_nr, &regs[int_spec[0]]);
> > +	}
> > +
> > +	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
> > +
> > +	return 0;
> > +
> > +err:
> > +	while (i--) {
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL,
> NULL);
> > +		irq_dispose_mapping(irq->mapped_irq);
> > +	}
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static int irqmux_remove(struct platform_device *pdev) {
> > +	struct irqmux_priv *priv = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		struct irqmux_one *irq = &priv->irq[i];
> > +
> > +		irq_set_chained_handler_and_data(irq->mapped_irq, NULL,
> NULL);
> > +		irq_dispose_mapping(irq->mapped_irq);
> > +	}
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id irqmux_match[] = {
> > +	{ .compatible = "renesas,rzn1-gpioirqmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, irqmux_match);
> > +
> > +static struct platform_driver irqmux_driver = {
> > +	.driver = {
> > +		.name = "gpio_irq_mux",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = irqmux_match,
> > +	},
> > +	.probe = irqmux_probe,
> > +	.remove = irqmux_remove,
> > +};
> > +
> > +module_platform_driver(irqmux_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
> > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
> 
> To sum it up, I think the structure of this driver is flawed. Please
> turn it into a full hierarchy, or alternatively tell me that I have
> the wrong end of the stick!
I'm hoping you got the wrong end of the stick! If not, I have some
excavation ahead to work out how I should have done this...

Thanks!
Phil

> 
> Thanks,
> 
> 	M.
> 
> --
> Jazz is not dead, it just smell funny.

Re: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Marc Zyngier]

Hi Phil,

On 31/10/18 15:09, Phil Edworthy wrote:
> Hi Marc,
> 
> Many thanks for a quick response!
> 
> On 31 October 2018 08:02, Marc Zyngier wote:
>> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
>>>
>>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
>>> configured to have 32 interrupt outputs, so we have a total of 96 GPIO
>>> interrupts. All of these are passed to the GPIO IRQ Muxer, which
>>> selects
>>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
>>> aren't latched, so there is nothing to do in this driver when an
>>> interrupt is received, other than tell the corresponding GPIO block.
>>>
>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>>> ---
>>> v2:
>>>  - Use interrupt-map to allow the GPIO controller info to be specified
>>>    as part of the irq.
>>>  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
>>> ---
>>>  drivers/irqchip/Kconfig        |  10 ++
>>>  drivers/irqchip/Makefile       |   1 +
>>>  drivers/irqchip/rzn1-irq-mux.c | 235
>>> +++++++++++++++++++++++++++++++++
>>>  3 files changed, 246 insertions(+)
>>>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
>>> 96451b581452..3a60a8af60dd 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -204,6 +204,16 @@ config RENESAS_IRQC
>>>  	select GENERIC_IRQ_CHIP
>>>  	select IRQ_DOMAIN
>>>
>>> +config RENESAS_RZN1_IRQ_MUX
>>> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
>>> +	depends on ARCH_RZN1
>>> +	select IRQ_DOMAIN
>>> +	select IRQ_DOMAIN_HIERARCHY
>>> +	help
>>> +	  Say yes here to add support for the GPIO IRQ multiplexer
>> embedded
>>> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
>>> +	  the interrupts coming from the GPIO controllers are used.
>>> +
>>>  config ST_IRQCHIP
>>>  	bool
>>>  	select REGMAP
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
>>> b822199445ff..b090f84dd42e 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
>> irq-sirfsoc.o
>>>  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
>>>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>>>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
>>> +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
>>>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>>>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
>>>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
>>> diff --git a/drivers/irqchip/rzn1-irq-mux.c
>>> b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
>>> 000000000000..767ce67e34d2
>>> --- /dev/null
>>> +++ b/drivers/irqchip/rzn1-irq-mux.c
>>> @@ -0,0 +1,235 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * RZ/N1 GPIO Interrupt Multiplexer
>>> + *
>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>> + *
>>> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
>>> +configured
>>> + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
>>> + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of
>>> +the GPIO
>>> + * interrupts to pass onto the GIC.
>>> + */
>>> +
>>> +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
>>> +
>>> +#define GPIO_IRQ_SPEC_SIZE	3
>>> +#define MAX_NR_GPIO_CONTROLLERS	3
>>> +#define MAX_NR_GPIO_IRQ		32
>>> +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
>> MAX_NR_GPIO_IRQ)
>>> +#define MAX_NR_OUTPUT_IRQS	8
>>> +
>>> +struct irqmux_priv;
>>> +struct irqmux_one {
>>> +	unsigned int mapped_irq;
>>> +	unsigned int input_irq_nr;
>>> +	struct irqmux_priv *priv;
>>> +};
>>> +
>>> +struct irqmux_priv {
>>> +	struct device *dev;
>>> +	struct irq_chip irq_chip;
>>
>> Do we really need this to be per-device? See below.
> I thought we generally wanted everything to be per-device so that we can
> cope when someone sticks two of these in a device. Am I wrong?

This only contains function pointers that are specific to a particular
type of interrupt controller. Nothing in struct irq_chip is
instance-specific.

> 
>>
>>> +	struct irq_domain *irq_domain;
>>> +	unsigned int nr_irqs;
>>> +	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS]; };
>>> +
>>> +static void irqmux_handler(struct irq_desc *desc) {
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
>>> +	struct irqmux_priv *priv = girq->priv;
>>> +	unsigned int irq;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +
>>> +	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
>>> +	generic_handle_irq(irq);
>>
>> No error handling? See below again, as I think this outline a fundamental flaw
>> in the driver.
>>
>>> +
>>> +	chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
>>> +			     irq_hw_number_t hwirq)
>>> +{
>>> +	struct irqmux_priv *priv = h->host_data;
>>> +
>>> +	irq_set_chip_data(irq, h->host_data);
>>> +	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct irq_domain_ops irqmux_domain_ops = {
>>> +	.map	= irqmux_domain_map,
>>> +};
>>> +
>>> +static int irqmux_probe(struct platform_device *pdev) {
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	struct resource *res;
>>> +	u32 __iomem *regs;
>>> +	struct irqmux_priv *priv;
>>> +	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
>>> +	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
>>> +	unsigned int irqs_out_used = 0;
>>> +	unsigned int i;
>>> +	int nr_irqs;
>>> +	int ret;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->dev = dev;
>>> +	platform_set_drvdata(pdev, priv);
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	regs = devm_ioremap_resource(dev, res);
>>> +	if (IS_ERR(regs))
>>> +		return PTR_ERR(regs);
>>> +
>>> +	nr_irqs = of_irq_count(np);
>>> +	if (nr_irqs < 0)
>>> +		return nr_irqs;
>>> +
>>> +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
>>> +		dev_err(dev, "too many output interrupts\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	priv->nr_irqs = nr_irqs;
>>> +
>>> +	/* Get the interrupt specifers */
>>> +	if (of_property_read_u32_array(dev->of_node, "interrupts",
>>> +				       (u32 *)int_specs,
>>> +				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
>>> +		dev_err(dev, "cannot get interrupt specifiers\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
>>> +
>>> +	/* Check the interrupt specifiers */
>>> +	for (i = 0; i < priv->nr_irqs; i++) {
>>> +		u32 *int_spec = int_specs[i];
>>> +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
>> int_spec[2];
>>> +
>>> +		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0],
>> int_spec[1],
>>> +			 int_spec[2]);
>>> +
>>> +		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
>>> +		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
>>> +		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
>>> +			dev_err(dev, "invalid interrupt args\n");
>>> +			return -ENOENT;
>>> +		}
>>> +
>>> +		if (irqs_out_used & BIT(int_spec[0]) ||
>>> +		    test_bit(input_irq, irqs_in_used)) {
>>> +			dev_err(dev, "irq %d already used\n", i);
>>> +			return -ENOENT;
>>> +		}
>>
>> I don't think the driver should be in the business of DT validation, and that
>> you should simply drop this code.
> When I implement Rob H's feedback on the binding, this should no longer be
> needed.
> 
>>
>>> +
>>> +		irqs_out_used |= BIT(int_spec[0]);
>>> +		set_bit(input_irq, irqs_in_used);
>>> +	}
>>> +
>>> +	/* Create IRQ domain for the interrupts coming from the GPIO blocks
>> */
>>> +	priv->irq_chip.name = dev_name(dev);
>>
>> OK, that's where I think we have a problem. Your irqchip structure seem to
>> only be used to display a name?!?
> Right, that wasn't the intention! So, how do I hook in my own interrupt handler
> without calling irq_set_chip_and_handler()?
> That's what led me to think I need an irq_chip instance.

That's the thing, you don't need it. each irq_chip is just a bunch of
methods, and these methods apply to all the instances of the same
controller.

>> To start with, that's not really the primary use for this object, and I'd like it to
>> be a single static structure for the whole driver. Userspace doesn't need to
>> know about the name, so please get rid of this.
>>
>> The real issue is that you build the whole thing as a chained interrupt
>> controller, meaning that nothing controls the masking of the interrupt. If, as I
>> understand it, this IP is an interrupt router that selects 8 out of 32 interrupts
>> and passes them onto the GIC, then a noisy device can just take the whole
>> CPU down by keeping the line asserted, and SW cannot mask it.
> The interrupts into this mux come from GPIO blocks that do the masking. The
> GPIO blocks in this case are standard Synopsys IP blocks.
> There is nothing in the irq mux hardware that can mask them, or do anything
> other than select which one to use, hence why this is a chained interrupt
> controller. Should I be using something else in this case?

There are two cases:
1) there is 1:1 mapping between a used input and an output, leaving some
input unused
2) there is an n:1 mapping between input and output, and all the input
can be used at any given time

If what you have is (1), you need to implement an hierarchy.
If what you have is (2), you need to implement a chained controller.

(1) requires you to revisit this driver, making it a lot more like ti's
irq-crossbar
(2) requires you to actually do some decoding in the chained handler

I believe you're in configuration (1). Am I right?

Thanks,

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

Re: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Marc Zyngier]

Hi Phil,

On 31/10/18 15:09, Phil Edworthy wrote:
> Hi Marc,
> 
> Many thanks for a quick response!
> 
> On 31 October 2018 08:02, Marc Zyngier wote:
>> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
>>>
>>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
>>> configured to have 32 interrupt outputs, so we have a total of 96 GPIO
>>> interrupts. All of these are passed to the GPIO IRQ Muxer, which
>>> selects
>>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
>>> aren't latched, so there is nothing to do in this driver when an
>>> interrupt is received, other than tell the corresponding GPIO block.
>>>
>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>>> ---
>>> v2:
>>>  - Use interrupt-map to allow the GPIO controller info to be specified
>>>    as part of the irq.
>>>  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
>>> ---
>>>  drivers/irqchip/Kconfig        |  10 ++
>>>  drivers/irqchip/Makefile       |   1 +
>>>  drivers/irqchip/rzn1-irq-mux.c | 235
>>> +++++++++++++++++++++++++++++++++
>>>  3 files changed, 246 insertions(+)
>>>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
>>> 96451b581452..3a60a8af60dd 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -204,6 +204,16 @@ config RENESAS_IRQC
>>>  	select GENERIC_IRQ_CHIP
>>>  	select IRQ_DOMAIN
>>>
>>> +config RENESAS_RZN1_IRQ_MUX
>>> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
>>> +	depends on ARCH_RZN1
>>> +	select IRQ_DOMAIN
>>> +	select IRQ_DOMAIN_HIERARCHY
>>> +	help
>>> +	  Say yes here to add support for the GPIO IRQ multiplexer
>> embedded
>>> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
>>> +	  the interrupts coming from the GPIO controllers are used.
>>> +
>>>  config ST_IRQCHIP
>>>  	bool
>>>  	select REGMAP
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
>>> b822199445ff..b090f84dd42e 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
>> irq-sirfsoc.o
>>>  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
>>>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>>>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
>>> +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
>>>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>>>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
>>>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
>>> diff --git a/drivers/irqchip/rzn1-irq-mux.c
>>> b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
>>> 000000000000..767ce67e34d2
>>> --- /dev/null
>>> +++ b/drivers/irqchip/rzn1-irq-mux.c
>>> @@ -0,0 +1,235 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * RZ/N1 GPIO Interrupt Multiplexer
>>> + *
>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>> + *
>>> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
>>> +configured
>>> + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
>>> + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of
>>> +the GPIO
>>> + * interrupts to pass onto the GIC.
>>> + */
>>> +
>>> +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
>>> +
>>> +#define GPIO_IRQ_SPEC_SIZE	3
>>> +#define MAX_NR_GPIO_CONTROLLERS	3
>>> +#define MAX_NR_GPIO_IRQ		32
>>> +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
>> MAX_NR_GPIO_IRQ)
>>> +#define MAX_NR_OUTPUT_IRQS	8
>>> +
>>> +struct irqmux_priv;
>>> +struct irqmux_one {
>>> +	unsigned int mapped_irq;
>>> +	unsigned int input_irq_nr;
>>> +	struct irqmux_priv *priv;
>>> +};
>>> +
>>> +struct irqmux_priv {
>>> +	struct device *dev;
>>> +	struct irq_chip irq_chip;
>>
>> Do we really need this to be per-device? See below.
> I thought we generally wanted everything to be per-device so that we can
> cope when someone sticks two of these in a device. Am I wrong?

This only contains function pointers that are specific to a particular
type of interrupt controller. Nothing in struct irq_chip is
instance-specific.

> 
>>
>>> +	struct irq_domain *irq_domain;
>>> +	unsigned int nr_irqs;
>>> +	struct irqmux_one irq[MAX_NR_OUTPUT_IRQS]; };
>>> +
>>> +static void irqmux_handler(struct irq_desc *desc) {
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	struct irqmux_one *girq = irq_desc_get_handler_data(desc);
>>> +	struct irqmux_priv *priv = girq->priv;
>>> +	unsigned int irq;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +
>>> +	irq = irq_find_mapping(priv->irq_domain, girq->input_irq_nr);
>>> +	generic_handle_irq(irq);
>>
>> No error handling? See below again, as I think this outline a fundamental flaw
>> in the driver.
>>
>>> +
>>> +	chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
>>> +			     irq_hw_number_t hwirq)
>>> +{
>>> +	struct irqmux_priv *priv = h->host_data;
>>> +
>>> +	irq_set_chip_data(irq, h->host_data);
>>> +	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct irq_domain_ops irqmux_domain_ops = {
>>> +	.map	= irqmux_domain_map,
>>> +};
>>> +
>>> +static int irqmux_probe(struct platform_device *pdev) {
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	struct resource *res;
>>> +	u32 __iomem *regs;
>>> +	struct irqmux_priv *priv;
>>> +	u32 int_specs[MAX_NR_OUTPUT_IRQS][GPIO_IRQ_SPEC_SIZE];
>>> +	DECLARE_BITMAP(irqs_in_used, MAX_NR_INPUT_IRQS);
>>> +	unsigned int irqs_out_used = 0;
>>> +	unsigned int i;
>>> +	int nr_irqs;
>>> +	int ret;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->dev = dev;
>>> +	platform_set_drvdata(pdev, priv);
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	regs = devm_ioremap_resource(dev, res);
>>> +	if (IS_ERR(regs))
>>> +		return PTR_ERR(regs);
>>> +
>>> +	nr_irqs = of_irq_count(np);
>>> +	if (nr_irqs < 0)
>>> +		return nr_irqs;
>>> +
>>> +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
>>> +		dev_err(dev, "too many output interrupts\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	priv->nr_irqs = nr_irqs;
>>> +
>>> +	/* Get the interrupt specifers */
>>> +	if (of_property_read_u32_array(dev->of_node, "interrupts",
>>> +				       (u32 *)int_specs,
>>> +				       priv->nr_irqs * GPIO_IRQ_SPEC_SIZE)) {
>>> +		dev_err(dev, "cannot get interrupt specifiers\n");
>>> +		return -ENOENT;
>>> +	}
>>> +
>>> +	bitmap_zero(irqs_in_used, MAX_NR_INPUT_IRQS);
>>> +
>>> +	/* Check the interrupt specifiers */
>>> +	for (i = 0; i < priv->nr_irqs; i++) {
>>> +		u32 *int_spec = int_specs[i];
>>> +		u32 input_irq = int_spec[1] * MAX_NR_GPIO_IRQ +
>> int_spec[2];
>>> +
>>> +		dev_info(dev, "irq %u=gpio%ua:%u\n", int_spec[0],
>> int_spec[1],
>>> +			 int_spec[2]);
>>> +
>>> +		if (int_spec[0] >= MAX_NR_OUTPUT_IRQS ||
>>> +		    int_spec[1] >= MAX_NR_GPIO_CONTROLLERS ||
>>> +		    int_spec[2] >= MAX_NR_GPIO_IRQ) {
>>> +			dev_err(dev, "invalid interrupt args\n");
>>> +			return -ENOENT;
>>> +		}
>>> +
>>> +		if (irqs_out_used & BIT(int_spec[0]) ||
>>> +		    test_bit(input_irq, irqs_in_used)) {
>>> +			dev_err(dev, "irq %d already used\n", i);
>>> +			return -ENOENT;
>>> +		}
>>
>> I don't think the driver should be in the business of DT validation, and that
>> you should simply drop this code.
> When I implement Rob H's feedback on the binding, this should no longer be
> needed.
> 
>>
>>> +
>>> +		irqs_out_used |= BIT(int_spec[0]);
>>> +		set_bit(input_irq, irqs_in_used);
>>> +	}
>>> +
>>> +	/* Create IRQ domain for the interrupts coming from the GPIO blocks
>> */
>>> +	priv->irq_chip.name = dev_name(dev);
>>
>> OK, that's where I think we have a problem. Your irqchip structure seem to
>> only be used to display a name?!?
> Right, that wasn't the intention! So, how do I hook in my own interrupt handler
> without calling irq_set_chip_and_handler()?
> That's what led me to think I need an irq_chip instance.

That's the thing, you don't need it. each irq_chip is just a bunch of
methods, and these methods apply to all the instances of the same
controller.

>> To start with, that's not really the primary use for this object, and I'd like it to
>> be a single static structure for the whole driver. Userspace doesn't need to
>> know about the name, so please get rid of this.
>>
>> The real issue is that you build the whole thing as a chained interrupt
>> controller, meaning that nothing controls the masking of the interrupt. If, as I
>> understand it, this IP is an interrupt router that selects 8 out of 32 interrupts
>> and passes them onto the GIC, then a noisy device can just take the whole
>> CPU down by keeping the line asserted, and SW cannot mask it.
> The interrupts into this mux come from GPIO blocks that do the masking. The
> GPIO blocks in this case are standard Synopsys IP blocks.
> There is nothing in the irq mux hardware that can mask them, or do anything
> other than select which one to use, hence why this is a chained interrupt
> controller. Should I be using something else in this case?

There are two cases:
1) there is 1:1 mapping between a used input and an output, leaving some
input unused
2) there is an n:1 mapping between input and output, and all the input
can be used at any given time

If what you have is (1), you need to implement an hierarchy.
If what you have is (2), you need to implement a chained controller.

(1) requires you to revisit this driver, making it a lot more like ti's
irq-crossbar
(2) requires you to actually do some decoding in the chained handler

I believe you're in configuration (1). Am I right?

Thanks,

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

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 31 October 2018 15:31, Marc Zyngier wrote:
> On 31/10/18 15:09, Phil Edworthy wrote:
> > On 31 October 2018 08:02, Marc Zyngier wote:
> >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> >>>
> >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> >>> configured to have 32 interrupt outputs, so we have a total of 96
> >>> GPIO interrupts. All of these are passed to the GPIO IRQ Muxer,
> >>> which selects
> >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> >>> aren't latched, so there is nothing to do in this driver when an
> >>> interrupt is received, other than tell the corresponding GPIO block.
> >>>
> >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >>> ---
> >>> v2:
> >>>  - Use interrupt-map to allow the GPIO controller info to be specified
> >>>    as part of the irq.
> >>>  - Renamed struct and funcs from 'girq' to a more comprehenisble
> 'irqmux'.
> >>> ---
> >>>  drivers/irqchip/Kconfig        |  10 ++
> >>>  drivers/irqchip/Makefile       |   1 +
> >>>  drivers/irqchip/rzn1-irq-mux.c | 235
> >>> +++++++++++++++++++++++++++++++++
> >>>  3 files changed, 246 insertions(+)
> >>>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> >>>
> >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> >>> 96451b581452..3a60a8af60dd 100644
> >>> --- a/drivers/irqchip/Kconfig
> >>> +++ b/drivers/irqchip/Kconfig
> >>> @@ -204,6 +204,16 @@ config RENESAS_IRQC
> >>>  	select GENERIC_IRQ_CHIP
> >>>  	select IRQ_DOMAIN
> >>>
> >>> +config RENESAS_RZN1_IRQ_MUX
> >>> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> >>> +	depends on ARCH_RZN1
> >>> +	select IRQ_DOMAIN
> >>> +	select IRQ_DOMAIN_HIERARCHY
> >>> +	help
> >>> +	  Say yes here to add support for the GPIO IRQ multiplexer
> >> embedded
> >>> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> >>> +	  the interrupts coming from the GPIO controllers are used.
> >>> +
> >>>  config ST_IRQCHIP
> >>>  	bool
> >>>  	select REGMAP
> >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> >>> index b822199445ff..b090f84dd42e 100644
> >>> --- a/drivers/irqchip/Makefile
> >>> +++ b/drivers/irqchip/Makefile
> >>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
> >> irq-sirfsoc.o
> >>>  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
> >>>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
> >>>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> >>> +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
> >>>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> >>>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
> >>>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> >>> diff --git a/drivers/irqchip/rzn1-irq-mux.c
> >>> b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
> >>> 000000000000..767ce67e34d2
> >>> --- /dev/null
> >>> +++ b/drivers/irqchip/rzn1-irq-mux.c
> >>> @@ -0,0 +1,235 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * RZ/N1 GPIO Interrupt Multiplexer
> >>> + *
> >>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> >>> + *
> >>> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks
> >>> +each configured
> >>> + * to have 32 interrupt outputs, so we have a total of 96 GPIO
> interrupts.
> >>> + * All of these are passed to the GPIO IRQ Muxer, which selects 8
> >>> +of the GPIO
> >>> + * interrupts to pass onto the GIC.
> >>> + */
> >>> +
> >>> +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
> >>> +
> >>> +#define GPIO_IRQ_SPEC_SIZE	3
> >>> +#define MAX_NR_GPIO_CONTROLLERS	3
> >>> +#define MAX_NR_GPIO_IRQ		32
> >>> +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
> >> MAX_NR_GPIO_IRQ)
> >>> +#define MAX_NR_OUTPUT_IRQS	8
> >>> +
> >>> +struct irqmux_priv;
> >>> +struct irqmux_one {
> >>> +	unsigned int mapped_irq;
> >>> +	unsigned int input_irq_nr;
> >>> +	struct irqmux_priv *priv;
> >>> +};
> >>> +
> >>> +struct irqmux_priv {
> >>> +	struct device *dev;
> >>> +	struct irq_chip irq_chip;
> >>
> >> Do we really need this to be per-device? See below.
> > I thought we generally wanted everything to be per-device so that we
> > can cope when someone sticks two of these in a device. Am I wrong?
> 
> This only contains function pointers that are specific to a particular type of
> interrupt controller. Nothing in struct irq_chip is instance-specific.
Ah, I see!

<snip>
> >> OK, that's where I think we have a problem. Your irqchip structure
> >> seem to only be used to display a name?!?
> > Right, that wasn't the intention! So, how do I hook in my own
> > interrupt handler without calling irq_set_chip_and_handler()?
> > That's what led me to think I need an irq_chip instance.
> 
> That's the thing, you don't need it. each irq_chip is just a bunch of methods,
> and these methods apply to all the instances of the same controller.
> 
> >> To start with, that's not really the primary use for this object, and
> >> I'd like it to be a single static structure for the whole driver.
> >> Userspace doesn't need to know about the name, so please get rid of
> this.
> >>
> >> The real issue is that you build the whole thing as a chained
> >> interrupt controller, meaning that nothing controls the masking of
> >> the interrupt. If, as I understand it, this IP is an interrupt router
> >> that selects 8 out of 32 interrupts and passes them onto the GIC,
> >> then a noisy device can just take the whole CPU down by keeping the line
> asserted, and SW cannot mask it.
> > The interrupts into this mux come from GPIO blocks that do the
> > masking. The GPIO blocks in this case are standard Synopsys IP blocks.
> > There is nothing in the irq mux hardware that can mask them, or do
> > anything other than select which one to use, hence why this is a
> > chained interrupt controller. Should I be using something else in this case?
> 
> There are two cases:
> 1) there is 1:1 mapping between a used input and an output, leaving some
> input unused
> 2) there is an n:1 mapping between input and output, and all the input can be
> used at any given time
> 
> If what you have is (1), you need to implement an hierarchy.
> If what you have is (2), you need to implement a chained controller.
> 
> (1) requires you to revisit this driver, making it a lot more like ti's irq-crossbar
> (2) requires you to actually do some decoding in the chained handler
> 
> I believe you're in configuration (1). Am I right?
Right, it's a 1:1 mapping. The information about which input to be used needs
to be specified in dt.
I didn’t think I could implement a hierarchy that didn’t mask the interrupts, so I
need to go back over that and look again...

Many thanks!
Phil

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 31 October 2018 15:31, Marc Zyngier wrote:
> On 31/10/18 15:09, Phil Edworthy wrote:
> > On 31 October 2018 08:02, Marc Zyngier wote:
> >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> >>>
> >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> >>> configured to have 32 interrupt outputs, so we have a total of 96
> >>> GPIO interrupts. All of these are passed to the GPIO IRQ Muxer,
> >>> which selects
> >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> >>> aren't latched, so there is nothing to do in this driver when an
> >>> interrupt is received, other than tell the corresponding GPIO block.
> >>>
> >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >>> ---
> >>> v2:
> >>>  - Use interrupt-map to allow the GPIO controller info to be specified
> >>>    as part of the irq.
> >>>  - Renamed struct and funcs from 'girq' to a more comprehenisble
> 'irqmux'.
> >>> ---
> >>>  drivers/irqchip/Kconfig        |  10 ++
> >>>  drivers/irqchip/Makefile       |   1 +
> >>>  drivers/irqchip/rzn1-irq-mux.c | 235
> >>> +++++++++++++++++++++++++++++++++
> >>>  3 files changed, 246 insertions(+)
> >>>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> >>>
> >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> >>> 96451b581452..3a60a8af60dd 100644
> >>> --- a/drivers/irqchip/Kconfig
> >>> +++ b/drivers/irqchip/Kconfig
> >>> @@ -204,6 +204,16 @@ config RENESAS_IRQC
> >>>  	select GENERIC_IRQ_CHIP
> >>>  	select IRQ_DOMAIN
> >>>
> >>> +config RENESAS_RZN1_IRQ_MUX
> >>> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> >>> +	depends on ARCH_RZN1
> >>> +	select IRQ_DOMAIN
> >>> +	select IRQ_DOMAIN_HIERARCHY
> >>> +	help
> >>> +	  Say yes here to add support for the GPIO IRQ multiplexer
> >> embedded
> >>> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> >>> +	  the interrupts coming from the GPIO controllers are used.
> >>> +
> >>>  config ST_IRQCHIP
> >>>  	bool
> >>>  	select REGMAP
> >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> >>> index b822199445ff..b090f84dd42e 100644
> >>> --- a/drivers/irqchip/Makefile
> >>> +++ b/drivers/irqchip/Makefile
> >>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SIRF_IRQ)			+=
> >> irq-sirfsoc.o
> >>>  obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
> >>>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
> >>>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> >>> +obj-$(CONFIG_RENESAS_RZN1_IRQ_MUX)	+= rzn1-irq-mux.o
> >>>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> >>>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
> >>>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> >>> diff --git a/drivers/irqchip/rzn1-irq-mux.c
> >>> b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
> >>> 000000000000..767ce67e34d2
> >>> --- /dev/null
> >>> +++ b/drivers/irqchip/rzn1-irq-mux.c
> >>> @@ -0,0 +1,235 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * RZ/N1 GPIO Interrupt Multiplexer
> >>> + *
> >>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> >>> + *
> >>> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks
> >>> +each configured
> >>> + * to have 32 interrupt outputs, so we have a total of 96 GPIO
> interrupts.
> >>> + * All of these are passed to the GPIO IRQ Muxer, which selects 8
> >>> +of the GPIO
> >>> + * interrupts to pass onto the GIC.
> >>> + */
> >>> +
> >>> +#include <linux/bitops.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_irq.h> #include <linux/of_platform.h>
> >>> +
> >>> +#define GPIO_IRQ_SPEC_SIZE	3
> >>> +#define MAX_NR_GPIO_CONTROLLERS	3
> >>> +#define MAX_NR_GPIO_IRQ		32
> >>> +#define MAX_NR_INPUT_IRQS	(MAX_NR_GPIO_CONTROLLERS *
> >> MAX_NR_GPIO_IRQ)
> >>> +#define MAX_NR_OUTPUT_IRQS	8
> >>> +
> >>> +struct irqmux_priv;
> >>> +struct irqmux_one {
> >>> +	unsigned int mapped_irq;
> >>> +	unsigned int input_irq_nr;
> >>> +	struct irqmux_priv *priv;
> >>> +};
> >>> +
> >>> +struct irqmux_priv {
> >>> +	struct device *dev;
> >>> +	struct irq_chip irq_chip;
> >>
> >> Do we really need this to be per-device? See below.
> > I thought we generally wanted everything to be per-device so that we
> > can cope when someone sticks two of these in a device. Am I wrong?
> 
> This only contains function pointers that are specific to a particular type of
> interrupt controller. Nothing in struct irq_chip is instance-specific.
Ah, I see!

<snip>
> >> OK, that's where I think we have a problem. Your irqchip structure
> >> seem to only be used to display a name?!?
> > Right, that wasn't the intention! So, how do I hook in my own
> > interrupt handler without calling irq_set_chip_and_handler()?
> > That's what led me to think I need an irq_chip instance.
> 
> That's the thing, you don't need it. each irq_chip is just a bunch of methods,
> and these methods apply to all the instances of the same controller.
> 
> >> To start with, that's not really the primary use for this object, and
> >> I'd like it to be a single static structure for the whole driver.
> >> Userspace doesn't need to know about the name, so please get rid of
> this.
> >>
> >> The real issue is that you build the whole thing as a chained
> >> interrupt controller, meaning that nothing controls the masking of
> >> the interrupt. If, as I understand it, this IP is an interrupt router
> >> that selects 8 out of 32 interrupts and passes them onto the GIC,
> >> then a noisy device can just take the whole CPU down by keeping the line
> asserted, and SW cannot mask it.
> > The interrupts into this mux come from GPIO blocks that do the
> > masking. The GPIO blocks in this case are standard Synopsys IP blocks.
> > There is nothing in the irq mux hardware that can mask them, or do
> > anything other than select which one to use, hence why this is a
> > chained interrupt controller. Should I be using something else in this case?
> 
> There are two cases:
> 1) there is 1:1 mapping between a used input and an output, leaving some
> input unused
> 2) there is an n:1 mapping between input and output, and all the input can be
> used at any given time
> 
> If what you have is (1), you need to implement an hierarchy.
> If what you have is (2), you need to implement a chained controller.
> 
> (1) requires you to revisit this driver, making it a lot more like ti's irq-crossbar
> (2) requires you to actually do some decoding in the chained handler
> 
> I believe you're in configuration (1). Am I right?
Right, it's a 1:1 mapping. The information about which input to be used needs
to be specified in dt.
I didn’t think I could implement a hierarchy that didn’t mask the interrupts, so I
need to go back over that and look again...

Many thanks!
Phil

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 31 October 2018 15:39, Phil Edworthy wrote
> On 31 October 2018 15:31, Marc Zyngier wrote:
> > On 31/10/18 15:09, Phil Edworthy wrote:
> > > On 31 October 2018 08:02, Marc Zyngier wote:
> > >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> > >>>
> > >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > >>> configured to have 32 interrupt outputs, so we have a total of 96
> > >>> GPIO interrupts. All of these are passed to the GPIO IRQ Muxer,
> > >>> which selects
> > >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt
> > >>> signals aren't latched, so there is nothing to do in this driver
> > >>> when an interrupt is received, other than tell the corresponding GPIO
> block.
<snip>

> > There are two cases:
> > 1) there is 1:1 mapping between a used input and an output, leaving
> > some input unused
> > 2) there is an n:1 mapping between input and output, and all the input
> > can be used at any given time
> >
> > If what you have is (1), you need to implement an hierarchy.
> > If what you have is (2), you need to implement a chained controller.
> >
> > (1) requires you to revisit this driver, making it a lot more like
> > ti's irq-crossbar
> > (2) requires you to actually do some decoding in the chained handler
> >
> > I believe you're in configuration (1). Am I right?
> Right, it's a 1:1 mapping. The information about which input to be used needs
> to be specified in dt.
> I didn’t think I could implement a hierarchy that didn’t mask the interrupts,
> so I need to go back over that and look again...

Ok, I have changed the driver to implement a hierarchy, i.e.
call irq_domain_create_hierarchy() in probe,
call irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent()
in the irq_domain_ops.alloc function.

I've got a couple of issues that I hope you can provide some input on.

First is what to do with irqs that are input to this mux but not selected.
If I don't call irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent(),
it causes the driver for the hardware that generates these interrupts (in
this case, Synopsys dwapb_gpio) to throw an exception in
irq_set_chained_handler_and_data().
As a hack, I have simply used an unused output irq.

Second is specifying the output irqs. Currently, I specify them like this:
	interrupts =
		<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
		...
		<GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
However, I am only reading the property so I can pass the fwspec to
irq_domain_alloc_irqs_parent(). I am not using anything that ends up
in of_irq_parse_raw().

Rob H has previously said that I should use interrupt-map to describe the
relationship between the input and output irqs. Unfortunately, irq
hierarchy seems to be somewhat at odds with using interrupt-map.
Or at least, I can’t see how the two can be combined.

Your feedback on this is greatly appreciated!
Thanks
Phil

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 31 October 2018 15:39, Phil Edworthy wrote
> On 31 October 2018 15:31, Marc Zyngier wrote:
> > On 31/10/18 15:09, Phil Edworthy wrote:
> > > On 31 October 2018 08:02, Marc Zyngier wote:
> > >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> > >>>
> > >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > >>> configured to have 32 interrupt outputs, so we have a total of 96
> > >>> GPIO interrupts. All of these are passed to the GPIO IRQ Muxer,
> > >>> which selects
> > >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt
> > >>> signals aren't latched, so there is nothing to do in this driver
> > >>> when an interrupt is received, other than tell the corresponding GPIO
> block.
<snip>

> > There are two cases:
> > 1) there is 1:1 mapping between a used input and an output, leaving
> > some input unused
> > 2) there is an n:1 mapping between input and output, and all the input
> > can be used at any given time
> >
> > If what you have is (1), you need to implement an hierarchy.
> > If what you have is (2), you need to implement a chained controller.
> >
> > (1) requires you to revisit this driver, making it a lot more like
> > ti's irq-crossbar
> > (2) requires you to actually do some decoding in the chained handler
> >
> > I believe you're in configuration (1). Am I right?
> Right, it's a 1:1 mapping. The information about which input to be used needs
> to be specified in dt.
> I didn’t think I could implement a hierarchy that didn’t mask the interrupts,
> so I need to go back over that and look again...

Ok, I have changed the driver to implement a hierarchy, i.e.
call irq_domain_create_hierarchy() in probe,
call irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent()
in the irq_domain_ops.alloc function.

I've got a couple of issues that I hope you can provide some input on.

First is what to do with irqs that are input to this mux but not selected.
If I don't call irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent(),
it causes the driver for the hardware that generates these interrupts (in
this case, Synopsys dwapb_gpio) to throw an exception in
irq_set_chained_handler_and_data().
As a hack, I have simply used an unused output irq.

Second is specifying the output irqs. Currently, I specify them like this:
	interrupts =
		<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
		...
		<GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
However, I am only reading the property so I can pass the fwspec to
irq_domain_alloc_irqs_parent(). I am not using anything that ends up
in of_irq_parse_raw().

Rob H has previously said that I should use interrupt-map to describe the
relationship between the input and output irqs. Unfortunately, irq
hierarchy seems to be somewhat at odds with using interrupt-map.
Or at least, I can’t see how the two can be combined.

Your feedback on this is greatly appreciated!
Thanks
Phil

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hello Marc,

On 06 November 2018 13:16 Phil Edworthy wrote:
> On 31 October 2018 15:39, Phil Edworthy wrote
> > On 31 October 2018 15:31, Marc Zyngier wrote:
> > > On 31/10/18 15:09, Phil Edworthy wrote:
> > > > On 31 October 2018 08:02, Marc Zyngier wote:
> > > >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> > > >>>
> > > >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks
> > > >>> each configured to have 32 interrupt outputs, so we have a total
> > > >>> of 96 GPIO interrupts. All of these are passed to the GPIO IRQ
> > > >>> Muxer, which selects
> > > >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt
> > > >>> signals aren't latched, so there is nothing to do in this driver
> > > >>> when an interrupt is received, other than tell the corresponding
> > > >>> GPIO
> > block.
> <snip>
> 
> > > There are two cases:
> > > 1) there is 1:1 mapping between a used input and an output, leaving
> > > some input unused
> > > 2) there is an n:1 mapping between input and output, and all the
> > > input can be used at any given time
> > >
> > > If what you have is (1), you need to implement an hierarchy.
> > > If what you have is (2), you need to implement a chained controller.
> > >
> > > (1) requires you to revisit this driver, making it a lot more like
> > > ti's irq-crossbar
> > > (2) requires you to actually do some decoding in the chained handler
> > >
> > > I believe you're in configuration (1). Am I right?
> > Right, it's a 1:1 mapping. The information about which input to be
> > used needs to be specified in dt.
> > I didn’t think I could implement a hierarchy that didn’t mask the
> > interrupts, so I need to go back over that and look again...
> 
> Ok, I have changed the driver to implement a hierarchy, i.e.
> call irq_domain_create_hierarchy() in probe, call
> irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent() in
> the irq_domain_ops.alloc function.

I suspect that I went in the wrong direction yet again...
After looking at Rob H's email again, I am now of the opinion that this
hardware, and the way to handle it, is very similar to PCIe MSI.

A cutdown DT looks like this:
	interrupts =
		<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
	#interrupt-cells = <1>;
	#address-cells = <0>;
	interrupt-map-mask = <127>;
	interrupt-map =
		/* gpio2a 24, pin 146: ETH Port 1 IRQ */
		<88 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		/* gpio2a 26, pin 148: Touchscreen_IRQ */
		<90 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;

The only issue is that I can't see how to get the first element of each
interrupt-map entry in the driver. The driver needs to know that input
interrupt hwirq 88 corresponds to GIC_SPI 103, and 90 to GIC_SPI 104.

Thanks for your time & patience,
Phil

RE: [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hello Marc,

On 06 November 2018 13:16 Phil Edworthy wrote:
> On 31 October 2018 15:39, Phil Edworthy wrote
> > On 31 October 2018 15:31, Marc Zyngier wrote:
> > > On 31/10/18 15:09, Phil Edworthy wrote:
> > > > On 31 October 2018 08:02, Marc Zyngier wote:
> > > >> On Tue, 30 Oct 2018 10:44:38 +0000, Phil Edworthy wrote:
> > > >>>
> > > >>> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks
> > > >>> each configured to have 32 interrupt outputs, so we have a total
> > > >>> of 96 GPIO interrupts. All of these are passed to the GPIO IRQ
> > > >>> Muxer, which selects
> > > >>> 8 of the GPIO interrupts to pass onto the GIC. The interrupt
> > > >>> signals aren't latched, so there is nothing to do in this driver
> > > >>> when an interrupt is received, other than tell the corresponding
> > > >>> GPIO
> > block.
> <snip>
> 
> > > There are two cases:
> > > 1) there is 1:1 mapping between a used input and an output, leaving
> > > some input unused
> > > 2) there is an n:1 mapping between input and output, and all the
> > > input can be used at any given time
> > >
> > > If what you have is (1), you need to implement an hierarchy.
> > > If what you have is (2), you need to implement a chained controller.
> > >
> > > (1) requires you to revisit this driver, making it a lot more like
> > > ti's irq-crossbar
> > > (2) requires you to actually do some decoding in the chained handler
> > >
> > > I believe you're in configuration (1). Am I right?
> > Right, it's a 1:1 mapping. The information about which input to be
> > used needs to be specified in dt.
> > I didn’t think I could implement a hierarchy that didn’t mask the
> > interrupts, so I need to go back over that and look again...
> 
> Ok, I have changed the driver to implement a hierarchy, i.e.
> call irq_domain_create_hierarchy() in probe, call
> irq_domain_set_hwirq_and_chip() and irq_domain_alloc_irqs_parent() in
> the irq_domain_ops.alloc function.

I suspect that I went in the wrong direction yet again...
After looking at Rob H's email again, I am now of the opinion that this
hardware, and the way to handle it, is very similar to PCIe MSI.

A cutdown DT looks like this:
	interrupts =
		<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
	#interrupt-cells = <1>;
	#address-cells = <0>;
	interrupt-map-mask = <127>;
	interrupt-map =
		/* gpio2a 24, pin 146: ETH Port 1 IRQ */
		<88 &gic GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
		/* gpio2a 26, pin 148: Touchscreen_IRQ */
		<90 &gic GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;

The only issue is that I can't see how to get the first element of each
interrupt-map entry in the driver. The driver needs to know that input
interrupt hwirq 88 corresponds to GIC_SPI 103, and 90 to GIC_SPI 104.

Thanks for your time & patience,
Phil