Re: [PATCH 2/2] arm64: dts: renesas: condor: add eMMC support

[Simon Horman]

On Fri, Apr 20, 2018 at 09:51:06PM +0300, Sergei Shtylyov wrote:
> On 04/20/2018 01:03 PM, Simon Horman wrote:
> 
> >> Define the Condor board dependent part of the MMC0 (connected to eMMC chip)
> >> device node along with the necessary voltage regulators...
> >>
> >> Signed-off-by: Sergei Shtylyov 
> >>
> >> ---
> >>  arch/arm64/boot/dts/renesas/r8a77980-condor.dts |   43 
> >> 
> >>  1 file changed, 43 insertions(+)
> >>
> >> Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> >> ===
> >> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> >> +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> >> @@ -27,6 +27,24 @@
> >>/* first 128MB is reserved for secure area. */
> >>reg = <0 0x4800 0 0x7800>;
> >>};
> >> +
> >> +  d3_3v: regulator-0 {
> > 
> > Please use reg_3p3v: regulator1 for consistency with salvator-common.dtsi
> 
>Hm, not sure why I have to copy what I consider a bad example... the SoCs 
> are
> not pin compatible anyway. 
> 
> >> +  compatible = "regulator-fixed";
> >> +  regulator-name = "D3.3V";
> > 
> > And "fixed-3.3V"
> 
>Ugh. That's pretty poor name I think. My names do correspond to the 
> schematics
> and these only muddle things up, I think...
> 
> [...]
> >> @@ -70,6 +101,18 @@
> >>function = "avb";
> >>};
> >>  
> >> +  mmc_1_8v_pins: mmc_1_8v {
> >> +  groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
> >> +  function = "mmc";
> >> +  power-source = <1800>;
> >> +  };
> >> +
> >> +  mmc_3_3v_pins: mmc_3_3v {
> >> +  groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
> >> +  function = "mmc";
> >> +  power-source = <3300>;
> >> +  };
> > 
> > Again please make this more consistent with salvator-common.dtsi.
> 
>Ah, you mean the _uhs label name postfix? OK...

Thanks, could you send a v2?

Re: [PATCH 2/2] arm64: dts: renesas: condor: add eMMC support

[Sergei Shtylyov]

On 04/20/2018 01:03 PM, Simon Horman wrote:

>> Define the Condor board dependent part of the MMC0 (connected to eMMC chip)
>> device node along with the necessary voltage regulators...
>>
>> Signed-off-by: Sergei Shtylyov 
>>
>> ---
>>  arch/arm64/boot/dts/renesas/r8a77980-condor.dts |   43 
>> 
>>  1 file changed, 43 insertions(+)
>>
>> Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> ===
>> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> @@ -27,6 +27,24 @@
>>  /* first 128MB is reserved for secure area. */
>>  reg = <0 0x4800 0 0x7800>;
>>  };
>> +
>> +d3_3v: regulator-0 {
> 
> Please use reg_3p3v: regulator1 for consistency with salvator-common.dtsi

   Hm, not sure why I have to copy what I consider a bad example... the SoCs are
not pin compatible anyway. 

>> +compatible = "regulator-fixed";
>> +regulator-name = "D3.3V";
> 
> And "fixed-3.3V"

   Ugh. That's pretty poor name I think. My names do correspond to the 
schematics
and these only muddle things up, I think...

[...]
>> @@ -70,6 +101,18 @@
>>  function = "avb";
>>  };
>>  
>> +mmc_1_8v_pins: mmc_1_8v {
>> +groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
>> +function = "mmc";
>> +power-source = <1800>;
>> +};
>> +
>> +mmc_3_3v_pins: mmc_3_3v {
>> +groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
>> +function = "mmc";
>> +power-source = <3300>;
>> +};
> 
> Again please make this more consistent with salvator-common.dtsi.

   Ah, you mean the _uhs label name postfix? OK...

[...]

MBR, Sergei

Re: [PATCH 2/2] arm64: dts: renesas: condor: add eMMC support

[Simon Horman]

On Sat, Apr 14, 2018 at 10:28:29PM +0300, Sergei Shtylyov wrote:
> Define the Condor board dependent part of the MMC0 (connected to eMMC chip)
> device node along with the necessary voltage regulators...
> 
> Signed-off-by: Sergei Shtylyov 
> 
> ---
>  arch/arm64/boot/dts/renesas/r8a77980-condor.dts |   43 
> 
>  1 file changed, 43 insertions(+)
> 
> Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> ===
> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> @@ -27,6 +27,24 @@
>   /* first 128MB is reserved for secure area. */
>   reg = <0 0x4800 0 0x7800>;
>   };
> +
> + d3_3v: regulator-0 {

Please use reg_3p3v: regulator1 for consistency with salvator-common.dtsi

> + compatible = "regulator-fixed";
> + regulator-name = "D3.3V";

And "fixed-3.3V"

> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + vddq_vin01: regulator-1 {

And reg_1p8v: regulator0 {
> + compatible = "regulator-fixed";
> + regulator-name = "VDDQ_VIN01";

And "fixed-1.8V"

> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
>  };
>  
>   {
> @@ -52,6 +70,19 @@
>   clock-frequency = <32768>;
>  };
>  
> + {
> + pinctrl-0 = <_3_3v_pins>;
> + pinctrl-1 = <_1_8v_pins>;
> + pinctrl-names = "default", "state_uhs";
> +
> + vmmc-supply = <_3v>;
> + vqmmc-supply = <_vin01>;
> + mmc-hs200-1_8v;
> + bus-width = <8>;
> + non-removable;
> + status = "okay";
> +};
> +
>   {
>   status = "okay";
>  };
> @@ -70,6 +101,18 @@
>   function = "avb";
>   };
>  
> + mmc_1_8v_pins: mmc_1_8v {
> + groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
> + function = "mmc";
> + power-source = <1800>;
> + };
> +
> + mmc_3_3v_pins: mmc_3_3v {
> + groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
> + function = "mmc";
> + power-source = <3300>;
> + };

Again please make this more consistent with salvator-common.dtsi.

> +
>   scif0_pins: scif0 {
>   groups = "scif0_data";
>   function = "scif0";
> 

[PATCH 2/2] arm64: dts: renesas: condor: add eMMC support

[Sergei Shtylyov]

Define the Condor board dependent part of the MMC0 (connected to eMMC chip)
device node along with the necessary voltage regulators...

Signed-off-by: Sergei Shtylyov 

---
 arch/arm64/boot/dts/renesas/r8a77980-condor.dts |   43 
 1 file changed, 43 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
@@ -27,6 +27,24 @@
/* first 128MB is reserved for secure area. */
reg = <0 0x4800 0 0x7800>;
};
+
+   d3_3v: regulator-0 {
+   compatible = "regulator-fixed";
+   regulator-name = "D3.3V";
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   regulator-boot-on;
+   regulator-always-on;
+   };
+
+   vddq_vin01: regulator-1 {
+   compatible = "regulator-fixed";
+   regulator-name = "VDDQ_VIN01";
+   regulator-min-microvolt = <180>;
+   regulator-max-microvolt = <180>;
+   regulator-boot-on;
+   regulator-always-on;
+   };
 };
 
  {
@@ -52,6 +70,19 @@
clock-frequency = <32768>;
 };
 
+ {
+   pinctrl-0 = <_3_3v_pins>;
+   pinctrl-1 = <_1_8v_pins>;
+   pinctrl-names = "default", "state_uhs";
+
+   vmmc-supply = <_3v>;
+   vqmmc-supply = <_vin01>;
+   mmc-hs200-1_8v;
+   bus-width = <8>;
+   non-removable;
+   status = "okay";
+};
+
  {
status = "okay";
 };
@@ -70,6 +101,18 @@
function = "avb";
};
 
+   mmc_1_8v_pins: mmc_1_8v {
+   groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
+   function = "mmc";
+   power-source = <1800>;
+   };
+
+   mmc_3_3v_pins: mmc_3_3v {
+   groups = "mmc_data8", "mmc_ctrl", "mmc_ds";
+   function = "mmc";
+   power-source = <3300>;
+   };
+
scif0_pins: scif0 {
groups = "scif0_data";
function = "scif0";