* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-25 12:54 ` Andre Przywara
@ 2025-04-25 14:37 ` Chen-Yu Tsai
2025-04-25 15:34 ` Andrew Lunn
2025-04-26 18:26 ` Jernej Škrabec
2 siblings, 0 replies; 18+ messages in thread
From: Chen-Yu Tsai @ 2025-04-25 14:37 UTC (permalink / raw)
To: Andre Przywara
Cc: Jernej Skrabec, robh, krzk+dt, conor+dt, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
On Fri, Apr 25, 2025 at 8:54 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 13 Apr 2025 15:42:57 +0200
> Jernej Skrabec <jernej.skrabec@gmail.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@gmail.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@gmail.com>
> > +// Based on sun50i-h6-orangepi-3.dts, which is:
> > +// Copyright (C) 2019 Ondřej Jirman <megous@megous.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 = <®_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 = <®_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 = <®_gmac_3v3>;
> > + allwinner,rx-delay-ps = <0>;
> > + allwinner,tx-delay-ps = <700>;
> > + status = "okay";
> > +};
> > +
> > +&gpu {
> > + mali-supply = <®_dcdcc>;
> > + status = "okay";
> > +};
> > +
> > +&hdmi {
> > + hvcc-supply = <®_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 = <®_cldo1>;
> > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > + disable-wp;
> > + bus-width = <4>;
> > + status = "okay";
> > +};
> > +
> > +&mmc1 {
> > + vmmc-supply = <®_wifi_3v3>;
> > + vqmmc-supply = <®_bldo3>;
> > + mmc-pwrseq = <&wifi_pwrseq>;
> > + bus-width = <4>;
> > + non-removable;
> > + status = "okay";
> > +
> > + aw859a: wifi@1 {
> > + reg = <1>;
> > + };
> > +};
> > +
> > +&mmc2 {
> > + vmmc-supply = <®_cldo1>;
> > + vqmmc-supply = <®_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 = <®_bldo2>;
> > + vcc-pd-supply = <®_cldo1>;
> > + vcc-pg-supply = <®_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 = <®_vcc5v>;
> > + vinb-supply = <®_vcc5v>;
> > + vinc-supply = <®_vcc5v>;
> > + vind-supply = <®_vcc5v>;
> > + vine-supply = <®_vcc5v>;
> > + aldoin-supply = <®_vcc5v>;
> > + bldoin-supply = <®_vcc5v>;
> > + cldoin-supply = <®_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 = <®_vcc5v>;
> > + usb3_vbus-supply = <®_vcc5v>;
> > + status = "okay";
> > +};
> > +
> > +&usb3phy {
> > + status = "okay";
> > +};
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-25 12:54 ` Andre Przywara
2025-04-25 14:37 ` Chen-Yu Tsai
@ 2025-04-25 15:34 ` Andrew Lunn
2025-04-26 18:00 ` Jernej Škrabec
2025-04-26 18:26 ` Jernej Škrabec
2 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2025-04-25 15:34 UTC (permalink / raw)
To: Andre Przywara
Cc: Jernej Skrabec, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
> > +&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?
Yes.
There is a checkpatch.pl patch working its way through the system
which will add warning about any rgmii value other than rgmii-id. Such
values need a comment that the PCB has extra long clock
lines. Hopefully that will make people actually stop and think about
this, rather than just copy broken vendor code.
Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-25 15:34 ` Andrew Lunn
@ 2025-04-26 18:00 ` Jernej Škrabec
2025-04-28 12:37 ` Andrew Lunn
0 siblings, 1 reply; 18+ messages in thread
From: Jernej Škrabec @ 2025-04-26 18:00 UTC (permalink / raw)
To: Andre Przywara, Andrew Lunn
Cc: robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > +&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?
>
> Yes.
>
> There is a checkpatch.pl patch working its way through the system
> which will add warning about any rgmii value other than rgmii-id. Such
> values need a comment that the PCB has extra long clock
> lines. Hopefully that will make people actually stop and think about
> this, rather than just copy broken vendor code.
I spent quite some time working on ethernet support for this board. Once
I've found PHY datasheet, I confirmed that there is added delay. So this
particular board needs "rgmii-rxid" mode.
Best regards,
Jernej
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-26 18:00 ` Jernej Škrabec
@ 2025-04-28 12:37 ` Andrew Lunn
2025-04-29 14:51 ` Jernej Škrabec
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2025-04-28 12:37 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
On Sat, Apr 26, 2025 at 08:00:49PM +0200, Jernej Škrabec wrote:
> Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > +&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?
> >
> > Yes.
> >
> > There is a checkpatch.pl patch working its way through the system
> > which will add warning about any rgmii value other than rgmii-id. Such
> > values need a comment that the PCB has extra long clock
> > lines. Hopefully that will make people actually stop and think about
> > this, rather than just copy broken vendor code.
>
> I spent quite some time working on ethernet support for this board. Once
> I've found PHY datasheet, I confirmed that there is added delay. So this
> particular board needs "rgmii-rxid" mode.
There have been numerous discussions about what these rgmii modes
mean, because DT developers frequently get them wrong.
Does the PCB have an extra long clock line in the TX direction? That
is what rgmii-rxid means, the PCB is providing the TX delay, the
MAC/PHY pair needs to add the RX delay.
Ignore strapping. That is just a power on default which gets over
ridden once the PHY driver is running.
What PHY is this?
Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-28 12:37 ` Andrew Lunn
@ 2025-04-29 14:51 ` Jernej Škrabec
2025-04-29 15:09 ` Andrew Lunn
0 siblings, 1 reply; 18+ messages in thread
From: Jernej Škrabec @ 2025-04-29 14:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
Dne ponedeljek, 28. april 2025 ob 14:37:48 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> On Sat, Apr 26, 2025 at 08:00:49PM +0200, Jernej Škrabec wrote:
> > Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > > +&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?
> > >
> > > Yes.
> > >
> > > There is a checkpatch.pl patch working its way through the system
> > > which will add warning about any rgmii value other than rgmii-id. Such
> > > values need a comment that the PCB has extra long clock
> > > lines. Hopefully that will make people actually stop and think about
> > > this, rather than just copy broken vendor code.
> >
> > I spent quite some time working on ethernet support for this board. Once
> > I've found PHY datasheet, I confirmed that there is added delay. So this
> > particular board needs "rgmii-rxid" mode.
>
> There have been numerous discussions about what these rgmii modes
> mean, because DT developers frequently get them wrong.
>
> Does the PCB have an extra long clock line in the TX direction? That
> is what rgmii-rxid means, the PCB is providing the TX delay, the
> MAC/PHY pair needs to add the RX delay.
While schematic is accessible, AFAIK PCB/gerbers are not, so I can't really
tell how long it is. But without this extra delay, ethernet doesn't work.
>
> Ignore strapping. That is just a power on default which gets over
> ridden once the PHY driver is running.
>
> What PHY is this?
Motorcomm YT8531C.
Best regards,
Jernej
>
> Andrew
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 14:51 ` Jernej Škrabec
@ 2025-04-29 15:09 ` Andrew Lunn
2025-04-29 15:24 ` Jernej Škrabec
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2025-04-29 15:09 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
On Tue, Apr 29, 2025 at 04:51:59PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 28. april 2025 ob 14:37:48 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > On Sat, Apr 26, 2025 at 08:00:49PM +0200, Jernej Škrabec wrote:
> > > Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > > > +&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?
> > > >
> > > > Yes.
> > > >
> > > > There is a checkpatch.pl patch working its way through the system
> > > > which will add warning about any rgmii value other than rgmii-id. Such
> > > > values need a comment that the PCB has extra long clock
> > > > lines. Hopefully that will make people actually stop and think about
> > > > this, rather than just copy broken vendor code.
> > >
> > > I spent quite some time working on ethernet support for this board. Once
> > > I've found PHY datasheet, I confirmed that there is added delay. So this
> > > particular board needs "rgmii-rxid" mode.
> >
> > There have been numerous discussions about what these rgmii modes
> > mean, because DT developers frequently get them wrong.
> >
> > Does the PCB have an extra long clock line in the TX direction? That
> > is what rgmii-rxid means, the PCB is providing the TX delay, the
> > MAC/PHY pair needs to add the RX delay.
>
> While schematic is accessible, AFAIK PCB/gerbers are not, so I can't really
> tell how long it is. But without this extra delay, ethernet doesn't work.
You are not adding an extra delay, you are subtracting a
delay. 'rgmii-rxid' says the TX delay is implemented by the PCB, hence
the PHY does not need to add the delay.
What is normal is that the PCB adds no delays, and the PHY adds the
delay for both the RX and the TX. And you represent this with
'rgmii-id'.
So what you need to find out is, where does the TX delay come from?
Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 15:09 ` Andrew Lunn
@ 2025-04-29 15:24 ` Jernej Škrabec
2025-04-29 15:27 ` Jernej Škrabec
0 siblings, 1 reply; 18+ messages in thread
From: Jernej Škrabec @ 2025-04-29 15:24 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
Dne torek, 29. april 2025 ob 17:09:22 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> On Tue, Apr 29, 2025 at 04:51:59PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 28. april 2025 ob 14:37:48 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > On Sat, Apr 26, 2025 at 08:00:49PM +0200, Jernej Škrabec wrote:
> > > > Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > > > > +&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?
> > > > >
> > > > > Yes.
> > > > >
> > > > > There is a checkpatch.pl patch working its way through the system
> > > > > which will add warning about any rgmii value other than rgmii-id. Such
> > > > > values need a comment that the PCB has extra long clock
> > > > > lines. Hopefully that will make people actually stop and think about
> > > > > this, rather than just copy broken vendor code.
> > > >
> > > > I spent quite some time working on ethernet support for this board. Once
> > > > I've found PHY datasheet, I confirmed that there is added delay. So this
> > > > particular board needs "rgmii-rxid" mode.
> > >
> > > There have been numerous discussions about what these rgmii modes
> > > mean, because DT developers frequently get them wrong.
> > >
> > > Does the PCB have an extra long clock line in the TX direction? That
> > > is what rgmii-rxid means, the PCB is providing the TX delay, the
> > > MAC/PHY pair needs to add the RX delay.
> >
> > While schematic is accessible, AFAIK PCB/gerbers are not, so I can't really
> > tell how long it is. But without this extra delay, ethernet doesn't work.
>
> You are not adding an extra delay, you are subtracting a
> delay. 'rgmii-rxid' says the TX delay is implemented by the PCB, hence
> the PHY does not need to add the delay.
>
> What is normal is that the PCB adds no delays, and the PHY adds the
> delay for both the RX and the TX. And you represent this with
> 'rgmii-id'.
ok, thanks for explanation.
>
> So what you need to find out is, where does the TX delay come from?
How to do that? Strapping show me this way and testing confirmed it. Not
sure what more I can do? As a hobbyist, I don't have access to anything more
than schematic.
Best regards,
Jernej
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 15:24 ` Jernej Škrabec
@ 2025-04-29 15:27 ` Jernej Škrabec
2025-04-29 15:45 ` Andrew Lunn
0 siblings, 1 reply; 18+ messages in thread
From: Jernej Škrabec @ 2025-04-29 15:27 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
Dne torek, 29. april 2025 ob 17:24:18 Srednjeevropski poletni čas je Jernej Škrabec napisal(a):
> Dne torek, 29. april 2025 ob 17:09:22 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > On Tue, Apr 29, 2025 at 04:51:59PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 28. april 2025 ob 14:37:48 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > On Sat, Apr 26, 2025 at 08:00:49PM +0200, Jernej Škrabec wrote:
> > > > > Dne petek, 25. april 2025 ob 17:34:14 Srednjeevropski poletni čas je Andrew Lunn napisal(a):
> > > > > > > > +&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?
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > There is a checkpatch.pl patch working its way through the system
> > > > > > which will add warning about any rgmii value other than rgmii-id. Such
> > > > > > values need a comment that the PCB has extra long clock
> > > > > > lines. Hopefully that will make people actually stop and think about
> > > > > > this, rather than just copy broken vendor code.
> > > > >
> > > > > I spent quite some time working on ethernet support for this board. Once
> > > > > I've found PHY datasheet, I confirmed that there is added delay. So this
> > > > > particular board needs "rgmii-rxid" mode.
> > > >
> > > > There have been numerous discussions about what these rgmii modes
> > > > mean, because DT developers frequently get them wrong.
> > > >
> > > > Does the PCB have an extra long clock line in the TX direction? That
> > > > is what rgmii-rxid means, the PCB is providing the TX delay, the
> > > > MAC/PHY pair needs to add the RX delay.
> > >
> > > While schematic is accessible, AFAIK PCB/gerbers are not, so I can't really
> > > tell how long it is. But without this extra delay, ethernet doesn't work.
> >
> > You are not adding an extra delay, you are subtracting a
> > delay. 'rgmii-rxid' says the TX delay is implemented by the PCB, hence
> > the PHY does not need to add the delay.
> >
> > What is normal is that the PCB adds no delays, and the PHY adds the
> > delay for both the RX and the TX. And you represent this with
> > 'rgmii-id'.
>
> ok, thanks for explanation.
>
> >
> > So what you need to find out is, where does the TX delay come from?
>
> How to do that? Strapping show me this way and testing confirmed it. Not
> sure what more I can do? As a hobbyist, I don't have access to anything more
> than schematic.
I just to be clear, I tested various combinations, including rgmii-id, and it
didn't work, except rgmii-rxid, which matches strapping. Of course Motorcomm
PHY driver took that into account and set registers accordingly.
Best regards,
Jernej
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 15:27 ` Jernej Škrabec
@ 2025-04-29 15:45 ` Andrew Lunn
2025-04-29 15:52 ` Chen-Yu Tsai
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2025-04-29 15:45 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Andre Przywara, robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
> I just to be clear, I tested various combinations, including rgmii-id, and it
> didn't work, except rgmii-rxid, which matches strapping. Of course Motorcomm
> PHY driver took that into account and set registers accordingly.
So we have:
&emac {
pinctrl-names = "default";
pinctrl-0 = <&ext_rgmii_pins>;
phy-mode = "rgmii-rxid";
phy-handle = <&ext_rgmii_phy>;
phy-supply = <®_gmac_3v3>;
allwinner,rx-delay-ps = <0>;
allwinner,tx-delay-ps = <700>;
status = "okay";
};
and
&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>;
};
};
The RX path looks O.K. RGMII-RXID means the PHY should be adding the
2ns delay. The allwinner,rx-delay-ps = <0> should be redundant, that
should be the driver default. And there are no properties in the PHY
node about RX. All good.
TX is the problem. The allwinner,tx-delay-ps = <700> causes the MAC to
add 700ps delay, and 'rgmii-rxid' means the PHY should not add any
delay. But 700ps is too low. It should be around 2000ps. So something
else is adding a delay, or the 700ps is not really 700ps.
You say the PHY is a YT8531C. These PHYs also accept
rx-internal-delay-ps and tx-internal-delay-ps properties in their DT
node.
Try setting 'rgmii-id', allwinner,tx-delay-ps = <0>, and both
rx-internal-delay-ps and tx-internal-delay-ps in the PHY node to 1950.
If that does not work, try other values in the PHY node.
Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 15:45 ` Andrew Lunn
@ 2025-04-29 15:52 ` Chen-Yu Tsai
2025-04-29 16:56 ` Andrew Lunn
0 siblings, 1 reply; 18+ messages in thread
From: Chen-Yu Tsai @ 2025-04-29 15:52 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jernej Škrabec, Andre Przywara, robh, krzk+dt, conor+dt,
samuel, devicetree, linux-arm-kernel, linux-sunxi, linux-kernel
On Tue, Apr 29, 2025 at 11:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I just to be clear, I tested various combinations, including rgmii-id, and it
> > didn't work, except rgmii-rxid, which matches strapping. Of course Motorcomm
> > PHY driver took that into account and set registers accordingly.
>
> So we have:
>
> &emac {
> pinctrl-names = "default";
> pinctrl-0 = <&ext_rgmii_pins>;
> phy-mode = "rgmii-rxid";
> phy-handle = <&ext_rgmii_phy>;
> phy-supply = <®_gmac_3v3>;
> allwinner,rx-delay-ps = <0>;
> allwinner,tx-delay-ps = <700>;
> status = "okay";
> };
>
> and
>
> &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>;
> };
> };
>
> The RX path looks O.K. RGMII-RXID means the PHY should be adding the
> 2ns delay. The allwinner,rx-delay-ps = <0> should be redundant, that
> should be the driver default. And there are no properties in the PHY
> node about RX. All good.
The default action when the property is missing is to leave the hardware
settings alone. I admit this doesn't match the bindings.
> TX is the problem. The allwinner,tx-delay-ps = <700> causes the MAC to
> add 700ps delay, and 'rgmii-rxid' means the PHY should not add any
> delay. But 700ps is too low. It should be around 2000ps. So something
> else is adding a delay, or the 700ps is not really 700ps.
Anything is possible. As was raised in a previous reply, it's possible
instead of extending the delay range, the decreased the step size and
added more steps. The problem is we don't really know.
> You say the PHY is a YT8531C. These PHYs also accept
> rx-internal-delay-ps and tx-internal-delay-ps properties in their DT
> node.
>
> Try setting 'rgmii-id', allwinner,tx-delay-ps = <0>, and both
> rx-internal-delay-ps and tx-internal-delay-ps in the PHY node to 1950.
> If that does not work, try other values in the PHY node.
I don't get why we should ignore the strappings instead of using them
as reference or even truth. If the strappings worked correctly w/ the
generic PHY driver (that doesn't know how to configure the delay mode
on the PHY side), isn't it working as intended?
ChenYu
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-29 15:52 ` Chen-Yu Tsai
@ 2025-04-29 16:56 ` Andrew Lunn
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2025-04-29 16:56 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Jernej Škrabec, Andre Przywara, robh, krzk+dt, conor+dt,
samuel, devicetree, linux-arm-kernel, linux-sunxi, linux-kernel
> > The RX path looks O.K. RGMII-RXID means the PHY should be adding the
> > 2ns delay. The allwinner,rx-delay-ps = <0> should be redundant, that
> > should be the driver default. And there are no properties in the PHY
> > node about RX. All good.
>
> The default action when the property is missing is to leave the hardware
> settings alone. I admit this doesn't match the bindings.
Please submit a patch fix the binding.
> > TX is the problem. The allwinner,tx-delay-ps = <700> causes the MAC to
> > add 700ps delay, and 'rgmii-rxid' means the PHY should not add any
> > delay. But 700ps is too low. It should be around 2000ps. So something
> > else is adding a delay, or the 700ps is not really 700ps.
>
> Anything is possible. As was raised in a previous reply, it's possible
> instead of extending the delay range, the decreased the step size and
> added more steps. The problem is we don't really know.
By poking around with other configuration knobs, i hope we can
determine if this 700ps actually is 700ps.
> > You say the PHY is a YT8531C. These PHYs also accept
> > rx-internal-delay-ps and tx-internal-delay-ps properties in their DT
> > node.
> >
> > Try setting 'rgmii-id', allwinner,tx-delay-ps = <0>, and both
> > rx-internal-delay-ps and tx-internal-delay-ps in the PHY node to 1950.
> > If that does not work, try other values in the PHY node.
>
> I don't get why we should ignore the strappings instead of using them
> as reference or even truth.
If you don't want the PHY to be reprogrammed pass:
* @PHY_INTERFACE_MODE_NA: Not Applicable - don't touch
rather than one of
* @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface
* @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
* @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
* @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal TX delay
However, if we do pass one of these RGMII modes, i expect the PHY to
follow them.
> If the strappings worked correctly w/ the
> generic PHY driver (that doesn't know how to configure the delay mode
> on the PHY side), isn't it working as intended?
The generic PHY driver is there as a fallback, for when the real PHY
driver is missing. It is nice if it works, but it often does not in
current systems. What we really care about is that the real PHY
driver works, and the system as a whole follows the DT bindings.
Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] arm64: dts: allwinner: h6: Add OrangePi 3 LTS DTS
2025-04-25 12:54 ` Andre Przywara
2025-04-25 14:37 ` Chen-Yu Tsai
2025-04-25 15:34 ` Andrew Lunn
@ 2025-04-26 18:26 ` Jernej Škrabec
2 siblings, 0 replies; 18+ messages in thread
From: Jernej Škrabec @ 2025-04-26 18:26 UTC (permalink / raw)
To: Andre Przywara
Cc: robh, krzk+dt, conor+dt, wens, samuel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel
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@gmail.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@gmail.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@gmail.com>
> > +// Based on sun50i-h6-orangepi-3.dts, which is:
> > +// Copyright (C) 2019 Ondřej Jirman <megous@megous.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 = <®_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 = <®_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 = <®_gmac_3v3>;
> > + allwinner,rx-delay-ps = <0>;
> > + allwinner,tx-delay-ps = <700>;
> > + status = "okay";
> > +};
> > +
> > +&gpu {
> > + mali-supply = <®_dcdcc>;
> > + status = "okay";
> > +};
> > +
> > +&hdmi {
> > + hvcc-supply = <®_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 = <®_cldo1>;
> > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > + disable-wp;
> > + bus-width = <4>;
> > + status = "okay";
> > +};
> > +
> > +&mmc1 {
> > + vmmc-supply = <®_wifi_3v3>;
> > + vqmmc-supply = <®_bldo3>;
> > + mmc-pwrseq = <&wifi_pwrseq>;
> > + bus-width = <4>;
> > + non-removable;
> > + status = "okay";
> > +
> > + aw859a: wifi@1 {
> > + reg = <1>;
> > + };
> > +};
> > +
> > +&mmc2 {
> > + vmmc-supply = <®_cldo1>;
> > + vqmmc-supply = <®_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 = <®_bldo2>;
> > + vcc-pd-supply = <®_cldo1>;
> > + vcc-pg-supply = <®_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 = <®_vcc5v>;
> > + vinb-supply = <®_vcc5v>;
> > + vinc-supply = <®_vcc5v>;
> > + vind-supply = <®_vcc5v>;
> > + vine-supply = <®_vcc5v>;
> > + aldoin-supply = <®_vcc5v>;
> > + bldoin-supply = <®_vcc5v>;
> > + cldoin-supply = <®_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 = <®_vcc5v>;
> > + usb3_vbus-supply = <®_vcc5v>;
> > + status = "okay";
> > +};
> > +
> > +&usb3phy {
> > + status = "okay";
> > +};
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread