Message-ID: <CAGb2v64uoeY-=4Dw5c5Zc5LxYqtUys76Bbe_S5P6_9xiC9eC3g@mail.gmail.com>
Date: Fri, 25 Apr 2025 22:37:57 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Andre Przywara <andre.przywara@....com>
Cc: Jernej Skrabec <jernej.skrabec@...il.com>, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.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

On Fri, Apr 25, 2025 at 8:54 PM Andre Przywara <andre.przywara@....com> wrote:
>
> 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.
>
> > +             #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?

I think it's possible to have a pinctrl setting node without the
"function" property? That way the pin config settings are applied,
but no function is muxed, thereby not conflicting with GPIO usage.

I've not actually tried it though.

This is also why I didn't want new pinctrl bindings that "force"
one to mux the pin.

ChenYu

> > +             };
> > +
> > +             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?
>
> > +     };
> > +
> > +     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.
>
> > +             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?
>
> > +     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?
>
> > +};
> > +
> > +&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?
>
> > +                     };
> > +
> > +                     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?
>
> 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,
> 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