Re: [PATCH 2/2] arm64: dts: allwinner: h616: Add Sipeed Longan SoM 3H and Pi 3H board support

[Jernej Škrabec]

Dne sobota, 03. februar 2024 ob 13:36:42 CET je Jisheng Zhang napisal(a):
> On Tue, Jan 09, 2024 at 09:37:03PM +0100, Jernej Škrabec wrote:
> > Hi Jisheng,
> 
> Hi Jernej,
> 
> > 
> > Andre gave you thorough review already, so just one additional comment
> > below.
> > 
> > Dne četrtek, 28. december 2023 ob 15:56:47 CET je Jisheng Zhang napisal(a):
> > > The Sipeed Longan SoM 3H is a system on module based on the Allwinner
> > > H618 SoC. The SoM features:
> > > 
> > > - Four ARM Cortex-A53 cores, Mali-G31 MP2 GPU
> > > - 2/4 GiB LPDDR4 DRAM SoMs
> > > - AXP313a PMIC
> > > - eMMC
> > > 
> > > The Sipeed Longan PI 3H is a development board based on the above SoM.
> > > The board features:
> > > - Longan SoM 3H
> > > - Raspberry-Pi-1 compatible GPIO header
> > > - 2 USB 2.0 host port
> > > - 1 USB 2.0 type C port (power supply + OTG)
> > > - MicroSD slot
> > > - 1Gbps Ethernet port (via RTL8211 PHY)
> > > - HDMI port
> > > - WiFi/BT chip
> > > 
> > > Add the devicetree file describing the currently supported features,
> > > namely PMIC, LEDs, UART, SD card, eMMC, USB and Ethernet.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> > > ---
> > >  arch/arm64/boot/dts/allwinner/Makefile        |   1 +
> > >  .../sun50i-h618-longan-module-3h.dtsi         |  82 +++++++++++
> > >  .../dts/allwinner/sun50i-h618-longanpi-3h.dts | 133 ++++++++++++++++++
> > >  3 files changed, 216 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h618-longan-module-3h.dtsi
> > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts
> > > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > index 3aca6787a167..00db504a9b8c 100644
> > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > @@ -42,4 +42,5 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-cb1-manta.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-pi.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-orangepi-zero2.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-x96-mate.dtb
> > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-longanpi-3h.dtb
> > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h618-longan-module-3h.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h618-longan-module-3h.dtsi
> > > new file mode 100644
> > > index 000000000000..88a7d287b73c
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h618-longan-module-3h.dtsi
> > > @@ -0,0 +1,82 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright (C) Jisheng Zhang <jszhang@xxxxxxxxxx>
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "sun50i-h616.dtsi"
> > > +
> > > +/ {
> > > +	model = "Sipeed Longan Module 3H";
> > > +	compatible = "sipeed,longan-module-3h", "allwinner,sun50i-h618";
> > > +};
> > > +
> > > +&mmc2 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&mmc2_pins>;
> > > +	vmmc-supply = <&reg_dldo1>;
> > > +	vqmmc-supply = <&reg_aldo1>;
> > > +	bus-width = <8>;
> > > +	non-removable;
> > > +	cap-mmc-hw-reset;
> > > +	mmc-ddr-1_8v;
> > 
> > I think above is superfluous if hs200 is also defined.
> 
> I read the mmc code again, IMHO, mmc-ddr-1_8v and mmc-hs200-1_8v
> are for different caps, the former is for MMC_CAP_1_8V_DDR and
> the later is for MMC_CAP2_HS200_1_8V_SDR, I must admit that
> mmc-hs200-1_8v does imply mmc-ddr-1_8v in normal host controllers
> but they are different, and I found other dts files also contain
> both, so I kept this as is in v2.

Yeah, this was oversight. Most DTs have only second one defined.

Best regards,
Jernej

> 
> Thanks
> 
> > 
> > > +	mmc-hs200-1_8v;
> > > +	status = "okay";
> > > +};
> > > +
>  
>