Message-ID: <7794863.EvYhyI6sBW@jernej-laptop>
Date: Sat, 26 Apr 2025 20:26:41 +0200
From: Jernej Škrabec <jernej.skrabec@...il.com>
To: Andre Przywara <andre.przywara@....com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, wens@...e.org,
 samuel@...lland.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS

Dne petek, 25. april 2025 ob 14:54:29 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> On Sun, 13 Apr 2025 15:42:57 +0200
> Jernej Skrabec <jernej.skrabec@...il.com> wrote:
> 
> Hi Jernej,
> 
> thanks for sending this, I now went through this in more detail and
> compared against the schematic, so some more nits below.
> I added the two comments from my other email before, so you can ignore that
> one now.
> 
> > OrangePi 3 LTS is quite similar to original OrangePi 3, but it has a lot
> > small changes that makes DT sharing unpractical with it.
> > 
> > OrangePi 3 LTS has following features:
> > - Allwinner H6 quad-core 64-bit ARM Cortex-A53
> > - GPU Mali-T720
> > - 2 GB LPDDR3 RAM
> > - AXP805 PMIC
> > - AW859A Wifi/BT 5.0
> > - 2x USB 2.0 host port (A)
> > - USB 3.0 Host
> > - Gigabit Ethernet (Motorcomm YT8531C phy)
> > - HDMI 2.0 port
> > - soldered 8 GB eMMC
> > - 2x LED
> > - microphone
> > - audio jack
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@...il.com>
> > ---
> >  arch/arm64/boot/dts/allwinner/Makefile        |   1 +
> >  .../allwinner/sun50i-h6-orangepi-3-lts.dts    | 351 ++++++++++++++++++
> >  2 files changed, 352 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > index 00bed412ee31..72c43bd0e2ab 100644
> > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > @@ -33,6 +33,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-zero-plus.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-zero-plus2.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-beelink-gs1.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-3.dtb
> > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-3-lts.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-lite2.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-one-plus.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-pine-h64.dtb
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts
> > new file mode 100644
> > index 000000000000..c8830d5c2f09
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts
> > @@ -0,0 +1,351 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +// Copyright (C) 2025 Jernej Skrabec <jernej.skrabec@...il.com>
> > +// Based on sun50i-h6-orangepi-3.dts, which is:
> > +// Copyright (C) 2019 Ondřej Jirman <megous@...ous.com>
> > +
> > +/dts-v1/;
> > +
> > +#include "sun50i-h6.dtsi"
> > +#include "sun50i-h6-cpu-opp.dtsi"
> > +#include "sun50i-h6-gpu-opp.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/leds/common.h>
> > +
> > +/ {
> > +	model = "OrangePi 3 LTS";
> > +	compatible = "xunlong,orangepi-3-lts", "allwinner,sun50i-h6";
> > +
> > +	aliases {
> > +		ethernet0 = &emac;
> > +		ethernet1 = &aw859a;
> > +		serial0 = &uart0;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	connector {
> > +		compatible = "hdmi-connector";
> > +		ddc-en-gpios = <&pio 7 2 GPIO_ACTIVE_HIGH>; /* PH2 */
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con_in: endpoint {
> > +				remote-endpoint = <&hdmi_out_con>;
> > +			};
> > +		};
> > +	};
> > +
> > +	ext_osc32k: ext_osc32k_clk {
> 
> For the sake of completeness, as mentioned in the other mail, I think we
> want dashes in the node name.

Right.

> 
> > +		#clock-cells = <0>;
> > +		compatible = "fixed-clock";
> > +		clock-frequency = <32768>;
> > +		clock-output-names = "ext_osc32k";
> 
> Should the output name also contain dashes instead?
> 
> > +	};
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		led-0 {
> > +			function = LED_FUNCTION_POWER;
> > +			color = <LED_COLOR_ID_RED>;
> > +			gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
> > +			default-state = "on";
> 
> Maybe something for a follow up patch, but I noticed that the schematic
> does not show current limiting resistors for the LEDs. This probably works
> because the default drive strength is 0b01, so level 1 (in a range from 0
> to 3, which we map to 10, 20, 30, 40 mA). The exact LED is not mentioned,
> but I would imagine that more than 20 mA would not be healthy, and even
> this might be a stretch over longer times.
> 
> So should we force the drive-strength to <10> or <20> somewhere? How does
> this even work for those gpios references?

Interestingly, this was already done for "allwinner,auxtek-t003" board and
others with strength 20. I doubt that's 20 mA, since that would certainly
kill LED.

> 
> > +		};
> > +
> > +		led-1 {
> > +			function = LED_FUNCTION_STATUS;
> > +			color = <LED_COLOR_ID_GREEN>;
> > +			gpios = <&r_pio 0 7 GPIO_ACTIVE_HIGH>; /* PL7 */
> > +		};
> > +	};
> > +
> > +	reg_gmac_3v3: gmac-3v3 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "gmac-3v3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		startup-delay-us = <150000>;
> > +		enable-active-high;
> > +		gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> 
> Can you please add a "vin-supply = <&reg_vcc5v>;" line here, to chain them
> up nicely?

Ok.

> 
> > +	};
> > +
> > +	reg_vcc5v: vcc5v {
> > +		/* board wide 5V supply directly from the DC jack */
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc-5v";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		regulator-always-on;
> > +	};
> > +
> > +	reg_wifi_3v3: wifi-3v3 {
> > +		/* 3.3V regulator for WiFi and BT */
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "wifi-3v3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		enable-active-high;
> > +		gpio = <&pio 7 7 GPIO_ACTIVE_HIGH>; /* PH7 */
> 
> Same vin-supply here, please, this discrete regulator is fed by DCIN 5V.
> 
> > +	};
> > +
> > +	wifi_pwrseq: wifi-pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		clocks = <&rtc 1>;
> 
> As mentioned yesterday, please use CLK_OSC32K_FANOUT.

good catch.

> 
> > +		clock-names = "ext_clock";
> > +		reset-gpios = <&r_pio 1 3 GPIO_ACTIVE_LOW>; /* PM3 */
> > +		post-power-on-delay-ms = <200>;
> > +	};
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&reg_dcdca>;
> > +};
> > +
> > +&de {
> > +	status = "okay";
> > +};
> > +
> > +&dwc3 {
> > +	status = "okay";
> > +};
> > +
> > +&ehci0 {
> > +	status = "okay";
> > +};
> > +
> > +&ehci3 {
> > +	status = "okay";
> > +};
> > +
> > +&emac {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&ext_rgmii_pins>;
> > +	phy-mode = "rgmii-rxid";
> 
> So relating to what Andrew said earlier today, should this read rgmii-id
> instead? Since the strap resistors just set some boot-up value, but we
> want the PHY driver to enable both RX and TX delay programmatically?

As explained in other email, no. This setting reflects strap resistors
and that extra delay is needed. PHY driver sets this delay based on
this mode.

> 
> > +	phy-handle = <&ext_rgmii_phy>;
> > +	phy-supply = <&reg_gmac_3v3>;
> > +	allwinner,rx-delay-ps = <0>;
> > +	allwinner,tx-delay-ps = <700>;
> > +	status = "okay";
> > +};
> > +
> > +&gpu {
> > +	mali-supply = <&reg_dcdcc>;
> > +	status = "okay";
> > +};
> > +
> > +&hdmi {
> > +	hvcc-supply = <&reg_bldo2>;
> > +	status = "okay";
> > +};
> > +
> > +&hdmi_out {
> > +	hdmi_out_con: endpoint {
> > +		remote-endpoint = <&hdmi_con_in>;
> > +	};
> > +};
> > +
> > +&mdio {
> > +	ext_rgmii_phy: ethernet-phy@1 {
> > +		compatible = "ethernet-phy-ieee802.3-c22";
> > +		reg = <1>;
> > +
> > +		motorcomm,clk-out-frequency-hz = <125000000>;
> > +
> > +		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
> > +		reset-assert-us = <15000>;
> > +		reset-deassert-us = <100000>;
> > +	};
> > +};
> > +
> > +&mmc0 {
> > +	vmmc-supply = <&reg_cldo1>;
> > +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > +	disable-wp;
> > +	bus-width = <4>;
> > +	status = "okay";
> > +};
> > +
> > +&mmc1 {
> > +	vmmc-supply = <&reg_wifi_3v3>;
> > +	vqmmc-supply = <&reg_bldo3>;
> > +	mmc-pwrseq = <&wifi_pwrseq>;
> > +	bus-width = <4>;
> > +	non-removable;
> > +	status = "okay";
> > +
> > +	aw859a: wifi@1 {
> > +		reg = <1>;
> > +	};
> > +};
> > +
> > +&mmc2 {
> > +	vmmc-supply = <&reg_cldo1>;
> > +	vqmmc-supply = <&reg_bldo2>;
> > +	cap-mmc-hw-reset;
> > +	non-removable;
> > +	bus-width = <8>;
> > +	status = "okay";
> 
> Given that it's 1.8V on the I/O lines, I think we would need the
> mmc-ddr-1_8v and mmc-hs200-1_8v properties, for higher speed modes? Or
> does that not work?

It's untested so I wouldn't risk it now.

> 
> > +};
> > +
> > +&ohci0 {
> > +	status = "okay";
> > +};
> > +
> > +&ohci3 {
> > +	status = "okay";
> > +};
> > +
> > +&pio {
> > +	vcc-pc-supply = <&reg_bldo2>;
> > +	vcc-pd-supply = <&reg_cldo1>;
> > +	vcc-pg-supply = <&reg_bldo3>;
> > +};
> > +
> > +&r_ir {
> > +	status = "okay";
> > +};
> > +
> > +&r_i2c {
> > +	status = "okay";
> > +
> > +	axp805: pmic@36 {
> > +		compatible = "x-powers,axp805", "x-powers,axp806";
> > +		reg = <0x36>;
> > +		interrupt-parent = <&r_intc>;
> > +		interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_LOW>;
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +		x-powers,self-working-mode;
> > +		vina-supply = <&reg_vcc5v>;
> > +		vinb-supply = <&reg_vcc5v>;
> > +		vinc-supply = <&reg_vcc5v>;
> > +		vind-supply = <&reg_vcc5v>;
> > +		vine-supply = <&reg_vcc5v>;
> > +		aldoin-supply = <&reg_vcc5v>;
> > +		bldoin-supply = <&reg_vcc5v>;
> > +		cldoin-supply = <&reg_vcc5v>;
> > +
> > +		regulators {
> > +			reg_aldo1: aldo1 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <3300000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-name = "vcc-pl-led-ir";
> > +			};
> > +
> > +			reg_aldo2: aldo2 {
> > +				regulator-min-microvolt = <3300000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-name = "vcc33-audio-tv-ephy-mac";
> > +			};
> > +
> > +			/* ALDO3 is shorted to CLDO1 */
> > +			reg_aldo3: aldo3 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <3300000>;
> > +				regulator-max-microvolt = <3300000>;cl
> > +				regulator-name = "vcc33-io-pd-emmc-sd-usb-uart-1";
> > +			};
> > +
> > +			reg_bldo1: bldo1 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <1800000>;
> > +				regulator-name = "vcc18-dram-bias-pll";
> > +			};
> > +
> > +			reg_bldo2: bldo2 {
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <1800000>;
> > +				regulator-name = "vcc-efuse-pcie-hdmi-pc";
> > +			};
> > +
> > +			reg_bldo3: bldo3 {
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <1800000>;
> > +				regulator-name = "vcc-pm-pg-dcxoio-wifi";
> 
> As you mention in the name, this rail is connected to VCC_DCXO, which IIUC
> is an essential supply, for the crystal oscillator circuit. So I think this
> needs to be always on?

It most likely is enabled from boot and since it powers pin banks, it's
always enabled. I'll add it anyway, just to make it more explicit.

> 
> > +			};
> > +
> > +			bldo4 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_cldo1: cldo1 {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <3300000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-name = "vcc33-io-pd-emmc-sd-usb-uart-2";
> > +			};
> > +
> > +			cldo2 {
> > +				/* unused */
> > +			};
> > +
> > +			cldo3 {
> > +				/* unused */
> > +			};
> > +
> > +			reg_dcdca: dcdca {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <800000>;
> > +				regulator-max-microvolt = <1160000>;
> > +				regulator-ramp-delay = <2500>;
> > +				regulator-name = "vdd-cpu";
> > +			};
> 
> Can you maybe add a comment here to say that DCDCB is polyphased to DCDCA?

sure.

> 
> I went through the whole rest and compared against the schematic
> (looking at GPIOs and power rails), and that looks OK from what I can see.

Thanks for checking!

Best regards,
Jernej

> 
> Thanks,
> Andre
> 
> 
> > +
> > +			reg_dcdcc: dcdcc {
> > +				regulator-enable-ramp-delay = <32000>;
> > +				regulator-min-microvolt = <810000>;
> > +				regulator-max-microvolt = <1080000>;
> > +				regulator-ramp-delay = <2500>;
> > +				regulator-name = "vdd-gpu";
> > +			};
> > +
> > +			reg_dcdcd: dcdcd {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <960000>;
> > +				regulator-max-microvolt = <960000>;
> > +				regulator-name = "vdd-sys";
> > +			};
> > +
> > +			reg_dcdce: dcdce {
> > +				regulator-always-on;
> > +				regulator-min-microvolt = <1200000>;
> > +				regulator-max-microvolt = <1200000>;
> > +				regulator-name = "vcc-dram";
> > +			};
> > +
> > +			sw {
> > +				/* unused */
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&rtc {
> > +	clocks = <&ext_osc32k>;
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_ph_pins>;
> > +	status = "okay";
> > +};
> > +
> > +&usb2otg {
> > +	dr_mode = "host";
> > +	status = "okay";
> > +};
> > +
> > +&usb2phy {
> > +	usb0_id_det-gpios = <&pio 2 15 GPIO_ACTIVE_HIGH>; /* PC15 */
> > +	usb0_vbus-supply = <&reg_vcc5v>;
> > +	usb3_vbus-supply = <&reg_vcc5v>;
> > +	status = "okay";
> > +};
> > +
> > +&usb3phy {
> > +	status = "okay";
> > +};
> 
> 

Powered by blists - more mailing lists