RE: [PATCH 2/3] arm64: dts: renesas: rzg2l-smarc-som: Enable eMMC on SMARC platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/3] arm64: dts: renesas: rzg2l-smarc-som: Enable eMMC
> on SMARC platform
> 
> Hi Biju,
> 
> On Thu, Oct 7, 2021 at 5:55 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > RZ/G2L SoM has both 64Gb eMMC and micro SD connected to SDHI0.
> >
> > Both these interfaces are mutually exclusive and the SD0 device
> > selection is based on the XOR between GPIO_SD0_DEV_SEL and SW1[2]
> > switch position.
> >
> > This patch sets GPIO_SD0_DEV_SEL to high in DT. Use the below switch
> > setting logic for device selection between eMMC and microSD slot
> > connected to SDHI0.
> >
> > Set SW1[2] to position 2/OFF for selecting eMMC Set SW1[2] to position
> > 3/ON for selecting micro SD
> >
> > This patch enables eMMC on RZ/G2L SMARC platform by default.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> 
> > --- a/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
> > @@ -5,14 +5,55 @@
> >   * Copyright (C) 2021 Renesas Electronics Corp.
> >   */
> >
> > +#include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
> >
> > +/* SW1[2] should be at position 2/OFF to enable 64Gb eMMC */
> 

> 64 GB (the MTFC64GASAQHD datasheet isn't clear about the exact size and
> the meaning of GB)

It is 64 GB.

> 
> > +#define EMMC   1
> > +
> > +/*
> > + * To enable uSD card on CN3,
> > + * SW1[2] should be at position 3/ON.
> > + * Disable eMMC by setting "#define EMMC       0" above.
> > + */
> > +#define SDHI   (!EMMC)
> > +
> >  / {
> >         memory@48000000 {
> >                 device_type = "memory";
> >                 /* first 128MB is reserved for secure area. */
> >                 reg = <0x0 0x48000000 0x0 0x78000000>;
> >         };
> > +
> > +       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";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               regulator-boot-on;
> > +               regulator-always-on;
> > +       };
> > +
> > +       vccq_sdhi0: regulator-vccq-sdhi0 {
> > +               compatible = "regulator-gpio";
> > +
> > +               regulator-name = "SDHI0 VccQ";
> > +               regulator-min-microvolt = <1800000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               states = <3300000 1 1800000 0>;
> 
> "make dtbs_check" says:
> 
>     regulator-vccq-sdhi0: states:0: [3300000, 1, 1800000, 0] is too long
> 
> Please group the two states using angular brackets.

OK.

> 
> > +               regulator-boot-on;
> > +               gpios = <&pinctrl RZG2L_GPIO(39, 0) GPIO_ACTIVE_HIGH>;
> > +               regulator-always-on;
> > +       };
> >  };
> >
> >  &adc {
> > @@ -32,4 +73,108 @@
> >         adc_pins: adc {
> >                 pinmux = <RZG2L_PORT_PINMUX(9, 0, 2)>; /* ADC_TRG */
> >         };
> > +
> > +       /*
> > +        * SDO device selection is XOR between GPIO_SD0_DEV_SEL and
> SW1[2]
> > +        * The below switch logic can be used to select the device
> between
> > +        * eMMC and microSD, after setting GPIO_SD0_DEV_SEL to high in
> DT.
> > +        * SW1[2] should be at position 2/OFF to enable 64Gb eMMC
> > +        * SW1[2] should be at position 3/ON to enable uSD card CN3
> > +        */
> > +       gpio_sd0_dev_sel {
> 
>     gpio_sd0_dev_sel: $nodename:0: 'gpio_sd0_dev_sel' does not match
> '^(hog-[0-9]+|.+-hog(-[0-9]+)?)$'
>     (for all hogs)
> 
> Please use e.g. "sd0-dev-sel-hog".

OK.

> 
> > +               gpio-hog;
> > +               gpios = <RZG2L_GPIO(41, 1) GPIO_ACTIVE_HIGH>;
> > +               output-high;
> > +               line-name = "gpio_sd0_dev_sel";

OK. Here as well.

> > +       };
> > +
> > +       sd0_pwr_en {
> 
> gpio-sd0-pwr-en-hog (with "gpio-" prefix, to match schematics)
OK.

> 
> > +               gpio-hog;
> > +               gpios = <RZG2L_GPIO(4, 1) GPIO_ACTIVE_HIGH>;
> > +               output-high;
> > +               line-name = "sd0_pwr_en";
> 
> gpio_sd0_pwr_en
OK.

Will add this changes to next version.

Regards,
Biju

> 
> > +       };
> 
> The rest looks good to me.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux