Hello Simon,
Thank you for your feedback!
> From: Simon Horman <horms@xxxxxxxxxxxx>
> Sent: 17 December 2018 11:50
> To: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 02/17] arm64: dts: renesas: r8a774c0: Add I2C and IIC-DVFS support
>
> On Mon, Dec 17, 2018 at 11:24:39AM +0000, Fabrizio Castro wrote:
> > Hello Simon,
> >
> > > From: Simon Horman <horms@xxxxxxxxxxxx>
> > > Sent: 16 December 2018 20:18
> > > Subject: Re: [PATCH 02/17] arm64: dts: renesas: r8a774c0: Add I2C and IIC-DVFS support
> > >
> > > On Fri, Dec 14, 2018 at 09:37:25AM +0000, Fabrizio Castro wrote:
> > > > Add the I2C[0-7] and IIC Bus Interface for DVFS (IIC for DVFS)
> > > > devices nodes to the r8a774c0 device tree.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > >
> > > Thanks Fabrizo for this patch, it looks good to me with the exception of
> > > one minor question I have below.
> > >
> > > > ---
> > > > arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 143 ++++++++++++++++++++++++++++++
> > > > 1 file changed, 143 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> > > > index 96a71e3..bf08aba 100644
> > > > --- a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi
> > > > @@ -271,6 +271,149 @@
> > > > resets = <&cpg 407>;
> > > > };
> > > >
> > > > +i2c0: i2c@e6500000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe6500000 0 0x40>;
> > > > +interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 931>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 931>;
> > > > +dmas = <&dmac1 0x91>, <&dmac1 0x90>,
> > > > + <&dmac2 0x91>, <&dmac2 0x90>;
> > > > +dma-names = "tx", "rx", "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <110>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c1: i2c@e6508000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe6508000 0 0x40>;
> > > > +interrupts = <GIC_SPI 288 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 930>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 930>;
> > > > +dmas = <&dmac1 0x93>, <&dmac1 0x92>,
> > > > + <&dmac2 0x93>, <&dmac2 0x92>;
> > > > +dma-names = "tx", "rx", "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c2: i2c@e6510000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe6510000 0 0x40>;
> > > > +interrupts = <GIC_SPI 286 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 929>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 929>;
> > > > +dmas = <&dmac1 0x95>, <&dmac1 0x94>,
> > > > + <&dmac2 0x95>, <&dmac2 0x94>;
> > > > +dma-names = "tx", "rx", "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c3: i2c@e66d0000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe66d0000 0 0x40>;
> > > > +interrupts = <GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 928>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 928>;
> > > > +dmas = <&dmac0 0x97>, <&dmac0 0x96>;
> > > > +dma-names = "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <110>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c4: i2c@e66d8000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe66d8000 0 0x40>;
> > > > +interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 927>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 927>;
> > > > +dmas = <&dmac0 0x99>, <&dmac0 0x98>;
> > > > +dma-names = "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c5: i2c@e66e0000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe66e0000 0 0x40>;
> > > > +interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 919>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 919>;
> > > > +dmas = <&dmac0 0x9b>, <&dmac0 0x9a>;
> > > > +dma-names = "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c6: i2c@e66e8000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe66e8000 0 0x40>;
> > > > +interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 918>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 918>;
> > > > +dmas = <&dmac0 0x9d>, <&dmac0 0x9c>;
> > > > +dma-names = "tx", "rx";
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c7: i2c@e6690000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,i2c-r8a774c0",
> > > > + "renesas,rcar-gen3-i2c";
> > > > +reg = <0 0xe6690000 0 0x40>;
> > > > +interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 1003>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 1003>;
> > > > +i2c-scl-internal-delay-ns = <6>;
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > +i2c_dvfs: i2c@e60b0000 {
> > > > +#address-cells = <1>;
> > > > +#size-cells = <0>;
> > > > +compatible = "renesas,iic-r8a774c0";
> > > > +reg = <0 0xe60b0000 0 0x15>;
> > >
> > > My reading of the documentation is that 0x31 would be a more appropriate
> > > size for the register window.
> >
> > Thank you for looking into this. RZ/G2 documentation about this seems a bit incomplete
> > at the moment, and we weren't too sure about what to do here. Our expectation is
> > that the IP should be the same as the one found in R-Car E3, and we thought they
> > finally wanted to document some previously undocumented registers with the RZ/G2
> > User's manual. We are waiting for some answers from Japan, and since the driver
> > doesn't support the "new" registers we thought there was no harm in using the same
> > memory region used for R-Car E3. I can see the following options:
> > * use 0x31 as you recommended
> > * keep 0x15 and change it later on to the right figure once the driver actually supports all
> > of the documented registers (and maybe updated r8a77990.dtsi as well in case the IP is the
> > same?)
> > * drop i2c_dvfs from this patch, and send it with a new patch once we get some answers
> > from Japan
> >
> > What's best option?
>
> If you think 0x15 is correct then lets just use that and follow-up
> with a fix if necessary.
Thank you for this.
Cheers,
Fab
>
> I'm now happy with this patch but would like to give others a chance to
> review it.
>
> Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
>
> > Thanks,
> > Fab
> >
> > >
> > > > +interrupts = <GIC_SPI 173 IRQ_TYPE_LEVEL_HIGH>;
> > > > +clocks = <&cpg CPG_MOD 926>;
> > > > +power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > > > +resets = <&cpg 926>;
> > > > +dmas = <&dmac0 0x11>, <&dmac0 0x10>;
> > > > +dma-names = "tx", "rx";
> > > > +status = "disabled";
> > > > +};
> > > > +
> > > > hscif0: serial@e6540000 {
> > > > compatible = "renesas,hscif-r8a774c0",
> > > > "renesas,rcar-gen3-hscif",
> > > > --
> > > > 2.7.4
> > > >
> >
> >
> > [https://www2.renesas.eu/media/email/unicef.jpg]
> >
> > This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further
> details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
> > We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
> >
> >
> >
> > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England
> & Wales under Registered No. 04586709.
> >
[https://www2.renesas.eu/media/email/unicef.jpg]
This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
