From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from kirsty.vergenet.net ([202.4.237.240]:58158 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726248AbeKGWA3 (ORCPT ); Wed, 7 Nov 2018 17:00:29 -0500 Date: Wed, 7 Nov 2018 13:30:10 +0100 From: Simon Horman To: Geert Uytterhoeven Cc: Yoshihiro Kaneko , Linux-Renesas , Magnus Damm , Linux ARM Subject: Re: [PATCH/RFT] arm64: dts: renesas: r8a77990: Add I2C-DVFS device node Message-ID: <20181107123002.t6xg4m4vbq7cuapb@verge.net.au> References: <1540071349-27483-1-git-send-email-ykaneko0929@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On Mon, Nov 05, 2018 at 09:52:48AM +0100, Geert Uytterhoeven wrote: > Hi Kaneko-san, > > On Sat, Oct 20, 2018 at 11:35 PM Yoshihiro Kaneko wrote: > > From: Takeshi Kihara > > > > This patch adds I2C-DVFS device node for the R8A77990 SoC. > > > > Signed-off-by: Takeshi Kihara > > Signed-off-by: Yoshihiro Kaneko > > Thanks for your patch! > > > --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi > > @@ -22,7 +22,8 @@ > > i2c4 = &i2c4; > > i2c5 = &i2c5; > > i2c6 = &i2c6; > > - i2c7 = &i2c7; > > + i2c7 = &i2c_dvfs; > > + i2c8 = &i2c7; > > Please don't change existing aliases. > While this makes the use of i2c7 for PMIC access uniform across the > range of R-Car Gen3 SoCs that have it, I think this is a bad idea, and that > it is better not to rely on I2C aliases at all. > > I guess the BSP did this to configure the BD9571 PMIC for DDR backup > mode using i2cset? Upstream has another method, avoiding the need for > i2cset, cfr. Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator. Dropping the above hung makes sense to me. Out of interest, what would be the implication of removing existing aliases? > > > }; > > > > cpus { > > @@ -337,6 +338,22 @@ > > reg = <0 0xe6060000 0 0x508>; > > }; > > > > + i2c_dvfs: i2c@e60b0000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "renesas,iic-r8a77990", > > "renesas,iic-r8a77990" is not yet documented. Thanks, as per my comment below I wonder if as well as documenting "renesas,iic-r8a77990" we also to teach the driver about it. > > > + "renesas,rcar-gen3-iic", > > + "renesas,rmobile-iic"; > > Also, IIC on R-Car E3 does not have the automatic transmission registers. > Does this affect claiming compatibility with the family-specific or generic > compatible values? My cursory reading of the driver indicates that only register that is used by it but documented as not existing on E3 is ICSTART (offset 0x70). It seems to me that we should confirm with Renesas that the register does indeed not exist - as this patch comes from the BSP which implies it is being used there. And if it does not exist we should try teaching the driver not to use ICSTART via the "renesas,iic-r8a77990" compat string. What do you think? > > + reg = <0 0xe60b0000 0 0x34>; > > Why 0x34? Last (byte-sized) register documented is at 0x14 => 0x15? I can't explain 0x34 but I agree 0x15 would match the documentation. > > > + interrupts = ; > > + clocks = <&cpg CPG_MOD 926>; > > + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; > > + resets = <&cpg 926>; > > + dmas = <&dmac0 0x11>, <&dmac0 0x10>; > > + dma-names = "tx", "rx"; > > + status = "disabled"; > > + }; > > + From mboxrd@z Thu Jan 1 00:00:00 1970 From: horms@verge.net.au (Simon Horman) Date: Wed, 7 Nov 2018 13:30:10 +0100 Subject: [PATCH/RFT] arm64: dts: renesas: r8a77990: Add I2C-DVFS device node In-Reply-To: References: <1540071349-27483-1-git-send-email-ykaneko0929@gmail.com> Message-ID: <20181107123002.t6xg4m4vbq7cuapb@verge.net.au> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Nov 05, 2018 at 09:52:48AM +0100, Geert Uytterhoeven wrote: > Hi Kaneko-san, > > On Sat, Oct 20, 2018 at 11:35 PM Yoshihiro Kaneko wrote: > > From: Takeshi Kihara > > > > This patch adds I2C-DVFS device node for the R8A77990 SoC. > > > > Signed-off-by: Takeshi Kihara > > Signed-off-by: Yoshihiro Kaneko > > Thanks for your patch! > > > --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi > > +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi > > @@ -22,7 +22,8 @@ > > i2c4 = &i2c4; > > i2c5 = &i2c5; > > i2c6 = &i2c6; > > - i2c7 = &i2c7; > > + i2c7 = &i2c_dvfs; > > + i2c8 = &i2c7; > > Please don't change existing aliases. > While this makes the use of i2c7 for PMIC access uniform across the > range of R-Car Gen3 SoCs that have it, I think this is a bad idea, and that > it is better not to rely on I2C aliases at all. > > I guess the BSP did this to configure the BD9571 PMIC for DDR backup > mode using i2cset? Upstream has another method, avoiding the need for > i2cset, cfr. Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator. Dropping the above hung makes sense to me. Out of interest, what would be the implication of removing existing aliases? > > > }; > > > > cpus { > > @@ -337,6 +338,22 @@ > > reg = <0 0xe6060000 0 0x508>; > > }; > > > > + i2c_dvfs: i2c at e60b0000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "renesas,iic-r8a77990", > > "renesas,iic-r8a77990" is not yet documented. Thanks, as per my comment below I wonder if as well as documenting "renesas,iic-r8a77990" we also to teach the driver about it. > > > + "renesas,rcar-gen3-iic", > > + "renesas,rmobile-iic"; > > Also, IIC on R-Car E3 does not have the automatic transmission registers. > Does this affect claiming compatibility with the family-specific or generic > compatible values? My cursory reading of the driver indicates that only register that is used by it but documented as not existing on E3 is ICSTART (offset 0x70). It seems to me that we should confirm with Renesas that the register does indeed not exist - as this patch comes from the BSP which implies it is being used there. And if it does not exist we should try teaching the driver not to use ICSTART via the "renesas,iic-r8a77990" compat string. What do you think? > > + reg = <0 0xe60b0000 0 0x34>; > > Why 0x34? Last (byte-sized) register documented is at 0x14 => 0x15? I can't explain 0x34 but I agree 0x15 would match the documentation. > > > + interrupts = ; > > + clocks = <&cpg CPG_MOD 926>; > > + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; > > + resets = <&cpg 926>; > > + dmas = <&dmac0 0x11>, <&dmac0 0x10>; > > + dma-names = "tx", "rx"; > > + status = "disabled"; > > + }; > > +