Re: [PATCH 1/4] arm64: dts: allwinner: h700: Add MMC2 for Anbernic RG35XX

[Jernej Škrabec <jernej.skrabec@xxxxxxxxx>]

Hi Chris,

sorry it took so long.

Dne petek, 18. oktober 2024 ob 18:06:14 Srednjeevropski standardni čas je Chris Morgan napisal(a):
> From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> 
> Add support for the second MMC slot on the Anbernic RG35XX series.
> The second MMC card is connected to MMC2 (WiFi if present is MMC1).
> The MMC logic is powered by cldo3 via the power domain to which
> it is connected, and also has an external 3.3v GPIO controlled
> regulator required for functionality.
> 
> Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> ---
>  .../sun50i-h700-anbernic-rg35xx-2024.dts      | 27 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> index 80ccab7b5ba7..e2039fd76b3d 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts
> @@ -18,6 +18,9 @@ / {
>  	compatible = "anbernic,rg35xx-2024", "allwinner,sun50i-h700";
>  
>  	aliases {
> +		mmc0 = &mmc0;
> +		mmc1 = &mmc2;
> +		mmc2 = &mmc1;

Please drop those. As a rule, we don't have it in any DT, since there are more
universal to identify root partition than a relying on device order.

>  		serial0 = &uart0;
>  	};
>  
> @@ -175,6 +178,15 @@ reg_vcc5v: regulator-vcc5v { /* USB-C power input */
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
>  	};
> +
> +	reg_vcc3v3_mmc2: regulator-vcc3v3-mmc2 {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "vcc3v3-mmc2";
> +	};
>  };
>  
>  &cpu0 {
> @@ -186,10 +198,21 @@ &ehci0 {
>  };
>  
>  &mmc0 {
> -	vmmc-supply = <&reg_cldo3>;
> -	disable-wp;
> +	bus-width = <4>;
>  	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> +	disable-wp;
> +	no-1-8-v;

Please leave properties in same order to minimize patch size. From
what I can see, only above property is added, which is not needed anyway.

> +	vmmc-supply = <&reg_cldo3>;
> +	status = "okay";
> +};
> +
> +&mmc2 {
>  	bus-width = <4>;
> +	cd-gpios = <&pio 4 22 GPIO_ACTIVE_LOW>; /* PE22 */
> +	disable-wp;
> +	no-1-8-v;
> +	vmmc-supply = <&reg_vcc3v3_mmc2>;
> +	vqmmc-supply = <&reg_cldo3>;

Is this another SD card slot or eMMC? You configured it as it would be SD card,
but mmc2 slots are usually used for eMMC, which needs different configuration.

Best regards,
Jernej

>  	status = "okay";
>  };
>  
>