LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Phil Edworthy <phil.edworthy@renesas.com>
To: Rob Herring <robh@kernel.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Mark Rutland <mark.rutland@arm.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: RE: [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding
Date: Wed, 31 Oct 2018 13:43:59 +0000	[thread overview]
Message-ID: <TY1PR01MB176943688317D3099AD69727F5CD0@TY1PR01MB1769.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20181030230420.GA25133@bogus>

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
>

  reply	other threads:[~2018-10-31 13:44 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-30 10:44 [PATCH v2 0/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer Phil Edworthy
2018-10-30 10:44 ` [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding Phil Edworthy
2018-10-30 23:04   ` Rob Herring
2018-10-31 13:43     ` Phil Edworthy [this message]
2018-10-30 10:44 ` [PATCH v2 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer Phil Edworthy
2018-10-31  8:02   ` Marc Zyngier
2018-10-31 15:09     ` Phil Edworthy
2018-10-31 15:30       ` Marc Zyngier
2018-10-31 15:38         ` Phil Edworthy
2018-11-06 13:15           ` Phil Edworthy
2018-11-08 15:37             ` Phil Edworthy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=TY1PR01MB176943688317D3099AD69727F5CD0@TY1PR01MB1769.jpnprd01.prod.outlook.com \
    --to=phil.edworthy@renesas.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert@linux-m68k.org \
    --cc=jason@lakedaemon.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=robh@kernel.org \
    --cc=tglx@linutronix.de \
    --subject='RE: [PATCH v2 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).