| Message ID | 20181105214117.11734-2-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [1/2] arm64: dts: r8a77990: Add SDHI device nodes | expand |
Hello Marek-san, > From: Marek Vasut, Sent: Tuesday, November 6, 2018 6:41 AM > > From: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > This patch enables SD card slot connected to SDHI0, micro SD card slot > connected to SDHI1 and eMMC connected to SDHI3 on the Ebisu board using > the R8A77990 SoC. > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Cc: linux-renesas-soc@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > --- Thank you for the patch! Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> I have a few nit comments below. > .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 136 ++++++++++++++++++ > 1 file changed, 136 insertions(+) > > diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > index 611f0265fcc5..bda1765dcdbd 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts <snip> > + sdhi3_pins: sd3 { > + groups = "sdhi3_data8", "sdhi3_ctrl"; > + function = "sdhi3"; > + power-source = <1800>; > + }; > + > + sdhi3_pins_uhs: sd3_uhs { > + groups = "sdhi3_data8", "sdhi3_ctrl"; > + function = "sdhi3"; > + power-source = <1800>; > + }; I assumed that we will add "sdhi3_ds" into these groups when we add support for HS400 mode. Best regards, Yoshihiro Shimoda
> + sdhi3_pins: sd3 { > + groups = "sdhi3_data8", "sdhi3_ctrl"; > + function = "sdhi3"; > + power-source = <1800>; > + }; > + > + sdhi3_pins_uhs: sd3_uhs { > + groups = "sdhi3_data8", "sdhi3_ctrl"; > + function = "sdhi3"; > + power-source = <1800>; > + }; Shouldn't we have only one pinctrl config here, like you did recently for the other Gen3 SoCs?
On 11/06/2018 06:29 AM, Yoshihiro Shimoda wrote: > Hello Marek-san, Hello Shimoda-san, >> From: Marek Vasut, Sent: Tuesday, November 6, 2018 6:41 AM >> >> From: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> >> This patch enables SD card slot connected to SDHI0, micro SD card slot >> connected to SDHI1 and eMMC connected to SDHI3 on the Ebisu board using >> the R8A77990 SoC. >> >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> Cc: linux-renesas-soc@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> --- > > Thank you for the patch! > > Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > I have a few nit comments below. > >> .../arm64/boot/dts/renesas/r8a77990-ebisu.dts | 136 ++++++++++++++++++ >> 1 file changed, 136 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >> index 611f0265fcc5..bda1765dcdbd 100644 >> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts >> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts > <snip> >> + sdhi3_pins: sd3 { >> + groups = "sdhi3_data8", "sdhi3_ctrl"; >> + function = "sdhi3"; >> + power-source = <1800>; >> + }; >> + >> + sdhi3_pins_uhs: sd3_uhs { >> + groups = "sdhi3_data8", "sdhi3_ctrl"; >> + function = "sdhi3"; >> + power-source = <1800>; >> + }; > > I assumed that we will add "sdhi3_ds" into these groups when we add support for HS400 mode. That's a good point, I think we can add it right away, so added in V2.
On 11/06/2018 09:34 AM, Wolfram Sang wrote: > >> + sdhi3_pins: sd3 { >> + groups = "sdhi3_data8", "sdhi3_ctrl"; >> + function = "sdhi3"; >> + power-source = <1800>; >> + }; >> + >> + sdhi3_pins_uhs: sd3_uhs { >> + groups = "sdhi3_data8", "sdhi3_ctrl"; >> + function = "sdhi3"; >> + power-source = <1800>; >> + }; > > Shouldn't we have only one pinctrl config here, like you did recently > for the other Gen3 SoCs? We should, thanks for pointing it out, fixed in V2.
diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts index 611f0265fcc5..bda1765dcdbd 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts @@ -119,6 +119,15 @@ }; }; + reg_1p8v: regulator0 { + compatible = "regulator-fixed"; + regulator-name = "fixed-1.8V"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + reg_3p3v: regulator1 { compatible = "regulator-fixed"; regulator-name = "fixed-3.3V"; @@ -133,6 +142,54 @@ #clock-cells = <0>; clock-frequency = <74250000>; }; + + vcc_sdhi0: regulator-vcc-sdhi0 { + compatible = "regulator-fixed"; + + regulator-name = "SDHI0 Vcc"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&gpio5 17 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + vccq_sdhi0: regulator-vccq-sdhi0 { + compatible = "regulator-gpio"; + + regulator-name = "SDHI0 VccQ"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + + gpios = <&gpio5 18 GPIO_ACTIVE_HIGH>; + gpios-states = <1>; + states = <3300000 1 + 1800000 0>; + }; + + vcc_sdhi1: regulator-vcc-sdhi1 { + compatible = "regulator-fixed"; + + regulator-name = "SDHI1 Vcc"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&gpio0 4 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + vccq_sdhi1: regulator-vccq-sdhi1 { + compatible = "regulator-gpio"; + + regulator-name = "SDHI1 VccQ"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + + gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>; + gpios-states = <1>; + states = <3300000 1 + 1800000 0>; + }; }; &avb { @@ -326,6 +383,42 @@ function = "scif2"; }; + sdhi0_pins: sd0 { + groups = "sdhi0_data4", "sdhi0_ctrl"; + function = "sdhi0"; + power-source = <3300>; + }; + + sdhi0_pins_uhs: sd0_uhs { + groups = "sdhi0_data4", "sdhi0_ctrl"; + function = "sdhi0"; + power-source = <1800>; + }; + + sdhi1_pins: sd1 { + groups = "sdhi1_data4", "sdhi1_ctrl"; + function = "sdhi1"; + power-source = <3300>; + }; + + sdhi1_pins_uhs: sd1_uhs { + groups = "sdhi1_data4", "sdhi1_ctrl"; + function = "sdhi1"; + power-source = <1800>; + }; + + sdhi3_pins: sd3 { + groups = "sdhi3_data8", "sdhi3_ctrl"; + function = "sdhi3"; + power-source = <1800>; + }; + + sdhi3_pins_uhs: sd3_uhs { + groups = "sdhi3_data8", "sdhi3_ctrl"; + function = "sdhi3"; + power-source = <1800>; + }; + usb0_pins: usb { groups = "usb0_b"; function = "usb0"; @@ -380,3 +473,46 @@ status = "okay"; }; + +&sdhi0 { + pinctrl-0 = <&sdhi0_pins>; + pinctrl-1 = <&sdhi0_pins_uhs>; + pinctrl-names = "default", "state_uhs"; + + vmmc-supply = <&vcc_sdhi0>; + vqmmc-supply = <&vccq_sdhi0>; + cd-gpios = <&gpio3 12 GPIO_ACTIVE_LOW>; + wp-gpios = <&gpio3 13 GPIO_ACTIVE_HIGH>; + bus-width = <4>; + sd-uhs-sdr50; + sd-uhs-sdr104; + status = "okay"; +}; + +&sdhi1 { + pinctrl-0 = <&sdhi1_pins>; + pinctrl-1 = <&sdhi1_pins_uhs>; + pinctrl-names = "default", "state_uhs"; + + vmmc-supply = <&vcc_sdhi1>; + vqmmc-supply = <&vccq_sdhi1>; + cd-gpios = <&gpio3 14 GPIO_ACTIVE_LOW>; + bus-width = <4>; + sd-uhs-sdr50; + sd-uhs-sdr104; + status = "okay"; +}; + +&sdhi3 { + /* used for on-board 8bit eMMC */ + pinctrl-0 = <&sdhi3_pins>; + pinctrl-1 = <&sdhi3_pins_uhs>; + pinctrl-names = "default", "state_uhs"; + + vmmc-supply = <®_3p3v>; + vqmmc-supply = <®_1p8v>; + mmc-hs200-1_8v; + bus-width = <8>; + non-removable; + status = "okay"; +};