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

[Phil Edworthy]

Hi Rob,

On 01 May 2018 14:29 Rob Herring wrote:
> On Mon, Apr 23, 2018 at 02:33:06PM +0100, 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@xxxxxxxxxxx>
> > ---
> >  .../interrupt-controller/renesas,rzn1-mux.txt      |  85 ++++++++++
> 
> Please split bindings to a separate patch.
Will do.

> >  drivers/irqchip/Kconfig                            |  10 ++
> >  drivers/irqchip/Makefile                           |   1 +
> >  drivers/irqchip/irq-rzn1-irq-mux.c                 | 178
> +++++++++++++++++++++
> >  4 files changed, 274 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mu
> > x.txt  create mode 100644 drivers/irqchip/irq-rzn1-irq-mux.c
> >
> > 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 0000000..f28a365
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,r
> > +++ zn1-mux.txt
> > @@ -0,0 +1,85 @@
> > +* Renesas RZ/N1 GPIO Interrupt Multiplexer
> > +
> > +On Renesas 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.
> > +
> > +A single node in the device tree is used to describe the GPIO IRQ Muxer.
> > +
> > +Required properties:
> > +- compatible: the SoC specific name, i.e. "renesas,r9a06g032-gpioirq"
> > +   or "renesas,r9a06g033-gpioirq" followed by the SoC family name, i.e.
> > +   "renesas,rzn1-gpioirq".
> > +- 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: The list of interrupts generated by the muxer which are then
> > +   connected to a parent interrupt controller. The format of the interrupt
> > +   specifier depends in the interrupt parent controller.
> > +- gpioirq-#N: One property for each interrupt output from the GPIO IRQ
> Muxer
> > +   that specifies the input interrupt to use, #N is from 0 to 7.
> 
> Why do you need this in DT? Can't the driver handle this dynamically?
> When you request an interrupt on a GPIO line, then connect that GPIO line
> to a free IRQ line.
On the SoC that has this block, 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. The firmware runs before Linux is
up, and luckily setting up the registers again won't affect the interrupts.

> If you really need this in DT, then interrupt-map can be used here.
Ok

> > +
> > +Optional properties:
> > +- interrupt-parent: pHandle of the parent interrupt controller, if not
> > +   inherited from the parent node.
> > +
> > +
> > +Example:
> > +
> > +	The following is an example for the RZ/N1D SoC.
> > +
> > +	gpioirq: gpioirq@51000480 {
> > +		compatible = "renesas,r9a06g032-gpioirq",
> > +				"renesas,rzn1-gpioirq";
> > +		reg = <0x51000480 0x20>;
> > +		interrupts =
> > +			<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		status = "disabled";
> 
> Don't show status in examples.
Ok

> > +	};
> > +
> > +	gpio0: gpio@5000b000 {
> > +		compatible = "snps,dw-apb-gpio";
> > +		reg = <0x5000b000 0x80>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		clock-names = "bus";
> > +		clocks = <&hclk_gpio0>;
> > +		status = "disabled";
> > +
> > +		gpio0a: gpio-controller@0 {
> > +			compatible = "snps,dw-apb-gpio-port";
> > +			bank-name = "gpio0a";
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			snps,nr-gpios = <32>;
> > +			reg = <0>;
> > +
> > +			interrupt-controller;
> > +			interrupt-parent = <&gpioirq>;
> > +			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>;
> > +		};
> > +	};
> > +
> > +
> > +	The following is an example for a board using this.
> 
> Don't show the soc/board split. This is convention, but not part of the
> binding.
Ok

> > +
> > +	&gpioirq {
> > +		status = "okay";
> > +		gpioirq-0 = <24>;	/* gpio0a 24 */
> > +		gpioirq-4 = <3>;	/* gpio0a 3 */
> > +	};

Thanks for your comments,
Phil