diff mbox series

[3/3] arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E

Message ID 20210110035846.9155-4-wens@kernel.org (mailing list archive)
State New
Headers show
Series arm64: rockchip: rk3328: Add Radxa ROCK Pi E | expand

Commit Message

Chen-Yu Tsai Jan. 10, 2021, 3:58 a.m. UTC
From: Chen-Yu Tsai <wens@csie.org>

Radxa ROCK Pi E is a router oriented SBC based on Rockchip's RK3328 SoC.
As the official wiki page puts it, "E for Ethernets".

It features the RK3328 SoC, gigabit and fast Ethernet RJ45 ports, both
directly served by Ethernet controllers in the SoC, a USB 3.0 host port,
a power-only USB type-C port, a 3.5mm headphone jack for audio output,
two LEDs, a 40-pin Raspberry Pi style GPIO header, and optional WiFi+BT
and PoE header.

The board comes in multiple configurations, differing in the amount of
onboard RAM, the level of WiFi+BT (none, 802.11n 2.4GHz, or 802.11ac
2.4 GHz & 5 GHz), and whether PoE is supported or not. These variants
can all share the same device tree.

The USB 2.0 OTG controller is available on the 40-pin header. This is
not enabled in the device tree, since it is possible to use it in a
host-only configuration, or in OTG mode with an extra pin from the
header as the ID pin.

The device tree is based on the one of the Rock64, with various parts
modified to match the ROCK Pi E, and some parts updated to newer styles,
such as the gmac2io node's mdio sub-node.

Add a new device tree file for the new board.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../boot/dts/rockchip/rk3328-rock-pi-e.dts    | 369 ++++++++++++++++++
 2 files changed, 370 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts

Comments

Johan Jonker Jan. 10, 2021, 2:45 p.m. UTC | #1
Hi Chen-Yu,

Some comments, have a look if it is useful...

On 1/10/21 4:58 AM, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
> 
> Radxa ROCK Pi E is a router oriented SBC based on Rockchip's RK3328 SoC.
> As the official wiki page puts it, "E for Ethernets".
> 
> It features the RK3328 SoC, gigabit and fast Ethernet RJ45 ports, both
> directly served by Ethernet controllers in the SoC, a USB 3.0 host port,
> a power-only USB type-C port, a 3.5mm headphone jack for audio output,

> two LEDs, a 40-pin Raspberry Pi style GPIO header, and optional WiFi+BT
> and PoE header.
> 
> The board comes in multiple configurations, differing in the amount of
> onboard RAM, the level of WiFi+BT (none, 802.11n 2.4GHz, or 802.11ac
> 2.4 GHz & 5 GHz), and whether PoE is supported or not. These variants
> can all share the same device tree.
> 
> The USB 2.0 OTG controller is available on the 40-pin header. This is
> not enabled in the device tree, since it is possible to use it in a
> host-only configuration, or in OTG mode with an extra pin from the
> header as the ID pin.
> 
> The device tree is based on the one of the Rock64, with various parts
> modified to match the ROCK Pi E, and some parts updated to newer styles,
> such as the gmac2io node's mdio sub-node.
> 
> Add a new device tree file for the new board.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../boot/dts/rockchip/rk3328-rock-pi-e.dts    | 369 ++++++++++++++++++
>  2 files changed, 370 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 622d320ddd13..62d3abc17a24 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-geekbox.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> new file mode 100644
> index 000000000000..7818d2e8180c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> @@ -0,0 +1,369 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * (C) Copyright 2020 Chen-Yu Tsai <wens@csie.org>
> + *
> + * Based on ./rk3328-rock64.dts, which is
> + *
> + * Copyright (c) 2017 PINE64
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include "rk3328.dtsi"
> +
> +/ {
> +	model = "Radxa ROCK Pi E";
> +	compatible = "radxa,rockpi-e", "rockchip,rk3328";
> +
> +	chosen {
> +		stdout-path = "serial2:1500000n8";
> +	};
> +
> +	gmac_clkin: external-gmac-clock {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "gmac_clkin";
> +		#clock-cells = <0>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-0 = <&led_pin>;
> +		pinctrl-names = "default";
> +
> +		led-0 {

> +			/* schematic say green but the actual thing is blue */

In rockpie-v1.2-20200427-sch.pdf this led is already called "LED_BLUE",
so comment maybe not needed anymore?

> +			color = <LED_COLOR_ID_BLUE>;
> +			gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
> +			linux,default-trigger = "heartbeat";
> +		};> +	};
> +
> +	vcc_sd: sdmmc-regulator {
> +		compatible = "regulator-fixed";
> +		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sdmmc0m1_pin>;

> +		regulator-boot-on;
> +		regulator-name = "vcc_sd";

regulator-name above other regulator properties

regulator voltage missing
make things as complete as possible

from fixed-regulator.yaml:

description:
  Any property defined as part of the core regulator binding, defined in
  regulator.yaml, can also be used. However a fixed voltage regulator is
  expected to have the regulator-min-microvolt and regulator-max-microvolt
  to be the same.

> +		vin-supply = <&vcc_io>;
> +	};
> +

> +	vcc_host_5v: vcc-host-5v-regulator {
> +		compatible = "regulator-fixed";
> +		gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb30_host_drv>;
> +		enable-active-high;
> +		regulator-name = "vcc_host_5v";

idem limits

> +		regulator-always-on;
> +		regulator-boot-on;
> +		vin-supply = <&vcc_sys>;
> +	};

For Heiko: ?? remove ??
usb3 has no support in mainline.
Regulators not in use are disabled.
For mainline this node has no use....

> +
> +	vcc_sys: vcc-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_sys";

> +		regulator-always-on;
> +		regulator-boot-on;

At the other regulators this is sort below the regulator limits.

> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +
> +	vcc_wifi: vcc-wifi-regulator {
> +		compatible = "regulator-fixed";
> +		gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_en>;
> +		regulator-name = "vcc_wifi";

idem limits

> +		regulator-always-on;
> +		regulator-boot-on;
> +		vin-supply = <&vcc_io>;
> +	};
> +};
> +
> +&analog_sound {
> +	status = "okay";
> +};
> +
> +&codec {
> +	status = "okay";
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu1 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&emmc {
> +	bus-width = <8>;
> +	cap-mmc-highspeed;

> +	max-frequency = <150000000>;

remove
already defined in dtsi

> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	non-removable;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_bus8>;
> +	vmmc-supply = <&vcc_io>;
> +	vqmmc-supply = <&vcc18_emmc>;
> +	status = "okay";
> +};

////////////////////////
	emmc: mmc@ff520000 {
		compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc";
		reg = <0x0 0xff520000 0x0 0x4000>;
		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
		clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
			 <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
		clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
		fifo-depth = <0x100>;
		max-frequency = <150000000>;
		status = "disabled";
	};
////////////////////////

> +
> +&gmac2io {
> +	assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
> +	assigned-clock-parents = <&gmac_clkin>, <&gmac_clkin>;
> +	clock_in_out = "input";
> +	phy-handle = <&rtl8211e>;
> +	phy-mode = "rgmii";
> +	phy-supply = <&vcc_io>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&rgmiim1_pins>;
> +	snps,aal;
> +	snps,rxpbl = <0x4>;
> +	snps,txpbl = <0x4>;
> +	tx_delay = <0x26>;
> +	rx_delay = <0x11>;
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		rtl8211e: ethernet-phy@1 {
> +			reg = <1>;
> +			pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
> +			pinctrl-names = "default";
> +			interrupt-parent = <&gpio1>;
> +			interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
> +			reset-assert-us = <10000>;
> +			reset-deassert-us = <50000>;
> +			reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +};
> +
> +&gmac2phy {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&fephyled_linkm1>, <&fephyled_rxm1>;
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +
> +	rk805: pmic@18 {
> +		compatible = "rockchip,rk805";
> +		reg = <0x18>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <6 IRQ_TYPE_LEVEL_LOW>;

> +		#clock-cells = <1>;

all thing that start with "#" down the list

> +		clock-output-names = "xin32k", "rk805-clkout2";
> +		gpio-controller;

> +		#gpio-cells = <2>;

idem

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmic_int_l>;
> +		rockchip,system-power-controller;
> +		wakeup-source;
> +
> +		vcc1-supply = <&vcc_sys>;
> +		vcc2-supply = <&vcc_sys>;
> +		vcc3-supply = <&vcc_sys>;
> +		vcc4-supply = <&vcc_sys>;
> +		vcc5-supply = <&vcc_io>;
> +		vcc6-supply = <&vcc_sys>;
> +
> +		regulators {
> +			vdd_log: DCDC_REG1 {
> +				regulator-name = "vdd_log";
> +				regulator-min-microvolt = <712500>;
> +				regulator-max-microvolt = <1450000>;
> +				regulator-ramp-delay = <12500>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1000000>;
> +				};
> +			};
> +
> +			vdd_arm: DCDC_REG2 {
> +				regulator-name = "vdd_arm";
> +				regulator-min-microvolt = <712500>;
> +				regulator-max-microvolt = <1450000>;
> +				regulator-ramp-delay = <12500>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <950000>;
> +				};
> +			};
> +
> +			vcc_ddr: DCDC_REG3 {
> +				regulator-name = "vcc_ddr";
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +				};
> +			};
> +
> +			vcc_io: DCDC_REG4 {
> +				regulator-name = "vcc_io";
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <3300000>;
> +				};
> +			};
> +
> +			vcc_18: LDO_REG1 {
> +				regulator-name = "vcc_18";
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +			};
> +
> +			vcc18_emmc: LDO_REG2 {
> +				regulator-name = "vcc18_emmc";
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1800000>;
> +				};
> +			};
> +
> +			vdd_10: LDO_REG3 {
> +				regulator-name = "vdd_10";
> +				regulator-min-microvolt = <1000000>;
> +				regulator-max-microvolt = <1000000>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +
> +				regulator-state-mem {
> +					regulator-on-in-suspend;
> +					regulator-suspend-microvolt = <1000000>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&i2s1 {
> +	status = "okay";
> +};
> +
> +&io_domains {
> +	pmuio-supply = <&vcc_io>;
> +	vccio1-supply = <&vcc_io>;
> +	vccio2-supply = <&vcc18_emmc>;
> +	vccio3-supply = <&vcc_io>;
> +	vccio4-supply = <&vcc_io>;
> +	vccio5-supply = <&vcc_io>;
> +	vccio6-supply = <&vcc_io>;
> +	status = "okay";
> +};
> +
> +&pinctrl {

> +	ethernet-phy {

gmac2io

phy / ethernet-phy is a reserved node name
use something else

make ARCH=arm64 dtbs_check

/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dt.yaml: ethernet-phy:
'reg' is a required property
	From schema: Documentation/devicetree/bindings/net/ethernet-phy.yaml


> +		eth_phy_int_pin: eth-phy-int-pin {
> +			rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		eth_phy_reset_pin: eth-phy-reset-pin {
> +			rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +	};
> +
> +	leds {
> +		led_pin: led-pin {
> +			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	pmic {
> +		pmic_int_l: pmic-int-l {
> +			rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +

> +	usb3 {

usb

Last numbers in nodenames are more related to the sort order then to
capabillity.
ie: mmc0, mmc1
All usb pin related things here.

> +		usb30_host_drv: usb30-host-drv {
> +			rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	wifi {
> +		wifi_en: wifi-en {
> +			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +};
> +
> +&sdmmc {
> +	bus-width = <4>;

> +	cap-mmc-highspeed;

remove
micro SD only

> +	cap-sd-highspeed;
> +	disable-wp;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdmmc0_clk>, <&sdmmc0_cmd>, <&sdmmc0_dectn>, <&sdmmc0_bus4>;
> +	vmmc-supply = <&vcc_sd>;
> +	status = "okay";
> +};
> +

> +&saradc {
> +	vref-supply = <&vcc_18>;
> +	status = "okay";
> +};

What happened to the recovery key from the schematic?

> +
> +&tsadc {
> +	status = "okay";
> +};
> +
> +&u2phy {
> +	status = "okay";
> +};
> +
> +&u2phy_host {
> +	status = "okay";
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> +
> +&usb_host0_ehci {
> +	status = "okay";
> +};
>
Chen-Yu Tsai Jan. 10, 2021, 3:37 p.m. UTC | #2
Hi,

On Sun, Jan 10, 2021 at 10:45 PM Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi Chen-Yu,
>
> Some comments, have a look if it is useful...
>
> On 1/10/21 4:58 AM, Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > Radxa ROCK Pi E is a router oriented SBC based on Rockchip's RK3328 SoC.
> > As the official wiki page puts it, "E for Ethernets".
> >
> > It features the RK3328 SoC, gigabit and fast Ethernet RJ45 ports, both
> > directly served by Ethernet controllers in the SoC, a USB 3.0 host port,
> > a power-only USB type-C port, a 3.5mm headphone jack for audio output,
>
> > two LEDs, a 40-pin Raspberry Pi style GPIO header, and optional WiFi+BT
> > and PoE header.
> >
> > The board comes in multiple configurations, differing in the amount of
> > onboard RAM, the level of WiFi+BT (none, 802.11n 2.4GHz, or 802.11ac
> > 2.4 GHz & 5 GHz), and whether PoE is supported or not. These variants
> > can all share the same device tree.
> >
> > The USB 2.0 OTG controller is available on the 40-pin header. This is
> > not enabled in the device tree, since it is possible to use it in a
> > host-only configuration, or in OTG mode with an extra pin from the
> > header as the ID pin.
> >
> > The device tree is based on the one of the Rock64, with various parts
> > modified to match the ROCK Pi E, and some parts updated to newer styles,
> > such as the gmac2io node's mdio sub-node.
> >
> > Add a new device tree file for the new board.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> >  .../boot/dts/rockchip/rk3328-rock-pi-e.dts    | 369 ++++++++++++++++++
> >  2 files changed, 370 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > index 622d320ddd13..62d3abc17a24 100644
> > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
> > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
> >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-geekbox.dtb
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> > new file mode 100644
> > index 000000000000..7818d2e8180c
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> > @@ -0,0 +1,369 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * (C) Copyright 2020 Chen-Yu Tsai <wens@csie.org>
> > + *
> > + * Based on ./rk3328-rock64.dts, which is
> > + *
> > + * Copyright (c) 2017 PINE64
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/leds/common.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/pinctrl/rockchip.h>
> > +#include "rk3328.dtsi"
> > +
> > +/ {
> > +     model = "Radxa ROCK Pi E";
> > +     compatible = "radxa,rockpi-e", "rockchip,rk3328";
> > +
> > +     chosen {
> > +             stdout-path = "serial2:1500000n8";
> > +     };
> > +
> > +     gmac_clkin: external-gmac-clock {
> > +             compatible = "fixed-clock";
> > +             clock-frequency = <125000000>;
> > +             clock-output-names = "gmac_clkin";
> > +             #clock-cells = <0>;
> > +     };
> > +
> > +     leds {
> > +             compatible = "gpio-leds";
> > +             pinctrl-0 = <&led_pin>;
> > +             pinctrl-names = "default";
> > +
> > +             led-0 {
>
> > +                     /* schematic say green but the actual thing is blue */
>
> In rockpie-v1.2-20200427-sch.pdf this led is already called "LED_BLUE",
> so comment maybe not needed anymore?

Thanks. Did not notice there was a new revision.

> > +                     color = <LED_COLOR_ID_BLUE>;
> > +                     gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
> > +                     linux,default-trigger = "heartbeat";
> > +             };> +   };
> > +
> > +     vcc_sd: sdmmc-regulator {
> > +             compatible = "regulator-fixed";
> > +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&sdmmc0m1_pin>;
>
> > +             regulator-boot-on;
> > +             regulator-name = "vcc_sd";
>
> regulator-name above other regulator properties

That is actually what I was used to, but some other rockchip dts files
have all the properties sorted alphabetically. So I stuck with what I
saw.

> regulator voltage missing
> make things as complete as possible
>
> from fixed-regulator.yaml:
>
> description:
>   Any property defined as part of the core regulator binding, defined in
>   regulator.yaml, can also be used. However a fixed voltage regulator is
>   expected to have the regulator-min-microvolt and regulator-max-microvolt
>   to be the same.

However this is not a real regulator; it is merely an on/off switch.
I believe in this case it should just pass through the voltage from
its upstream.

> > +             vin-supply = <&vcc_io>;
> > +     };
> > +
>
> > +     vcc_host_5v: vcc-host-5v-regulator {
> > +             compatible = "regulator-fixed";
> > +             gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&usb30_host_drv>;
> > +             enable-active-high;
> > +             regulator-name = "vcc_host_5v";
>
> idem limits

Same here.

> > +             regulator-always-on;
> > +             regulator-boot-on;
> > +             vin-supply = <&vcc_sys>;
> > +     };
>
> For Heiko: ?? remove ??
> usb3 has no support in mainline.
> Regulators not in use are disabled.
> For mainline this node has no use....

As it already has a defined binding, we can put it in the device tree.

> > +
> > +     vcc_sys: vcc-sys {
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "vcc_sys";
>
> > +             regulator-always-on;
> > +             regulator-boot-on;
>
> At the other regulators this is sort below the regulator limits.

Again, alphabetically sorted vs preferred sorting method.

> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> > +     };
> > +
> > +     vcc_wifi: vcc-wifi-regulator {
> > +             compatible = "regulator-fixed";
> > +             gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&wifi_en>;
> > +             regulator-name = "vcc_wifi";
>
> idem limits

Again, it is just a switch.

> > +             regulator-always-on;
> > +             regulator-boot-on;
> > +             vin-supply = <&vcc_io>;
> > +     };
> > +};
> > +
> > +&analog_sound {
> > +     status = "okay";
> > +};
> > +
> > +&codec {
> > +     status = "okay";
> > +};
> > +
> > +&cpu0 {
> > +     cpu-supply = <&vdd_arm>;
> > +};
> > +
> > +&cpu1 {
> > +     cpu-supply = <&vdd_arm>;
> > +};
> > +
> > +&cpu2 {
> > +     cpu-supply = <&vdd_arm>;
> > +};
> > +
> > +&cpu3 {
> > +     cpu-supply = <&vdd_arm>;
> > +};
> > +
> > +&emmc {
> > +     bus-width = <8>;
> > +     cap-mmc-highspeed;
>
> > +     max-frequency = <150000000>;
>
> remove
> already defined in dtsi

OK.

> > +     mmc-ddr-1_8v;
> > +     mmc-hs200-1_8v;
> > +     non-removable;
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_bus8>;
> > +     vmmc-supply = <&vcc_io>;
> > +     vqmmc-supply = <&vcc18_emmc>;
> > +     status = "okay";
> > +};
>
> ////////////////////////
>         emmc: mmc@ff520000 {
>                 compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc";
>                 reg = <0x0 0xff520000 0x0 0x4000>;
>                 interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>                 clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
>                          <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
>                 clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>                 fifo-depth = <0x100>;
>                 max-frequency = <150000000>;
>                 status = "disabled";
>         };
> ////////////////////////
>
> > +
> > +&gmac2io {
> > +     assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
> > +     assigned-clock-parents = <&gmac_clkin>, <&gmac_clkin>;
> > +     clock_in_out = "input";
> > +     phy-handle = <&rtl8211e>;
> > +     phy-mode = "rgmii";
> > +     phy-supply = <&vcc_io>;
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&rgmiim1_pins>;
> > +     snps,aal;
> > +     snps,rxpbl = <0x4>;
> > +     snps,txpbl = <0x4>;
> > +     tx_delay = <0x26>;
> > +     rx_delay = <0x11>;
> > +     status = "okay";
> > +
> > +     mdio {
> > +             compatible = "snps,dwmac-mdio";
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
> > +
> > +             rtl8211e: ethernet-phy@1 {
> > +                     reg = <1>;
> > +                     pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
> > +                     pinctrl-names = "default";
> > +                     interrupt-parent = <&gpio1>;
> > +                     interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
> > +                     reset-assert-us = <10000>;
> > +                     reset-deassert-us = <50000>;
> > +                     reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
> > +             };
> > +     };
> > +};
> > +
> > +&gmac2phy {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&fephyled_linkm1>, <&fephyled_rxm1>;
> > +     status = "okay";
> > +};
> > +
> > +&i2c1 {
> > +     status = "okay";
> > +
> > +     rk805: pmic@18 {
> > +             compatible = "rockchip,rk805";
> > +             reg = <0x18>;
> > +             interrupt-parent = <&gpio2>;
> > +             interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
>
> > +             #clock-cells = <1>;
>
> all thing that start with "#" down the list

Is there a proper "preferred" sorting method defined somewhere?

> > +             clock-output-names = "xin32k", "rk805-clkout2";
> > +             gpio-controller;
>
> > +             #gpio-cells = <2>;
>
> idem
>
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pmic_int_l>;
> > +             rockchip,system-power-controller;
> > +             wakeup-source;
> > +
> > +             vcc1-supply = <&vcc_sys>;
> > +             vcc2-supply = <&vcc_sys>;
> > +             vcc3-supply = <&vcc_sys>;
> > +             vcc4-supply = <&vcc_sys>;
> > +             vcc5-supply = <&vcc_io>;
> > +             vcc6-supply = <&vcc_sys>;
> > +
> > +             regulators {
> > +                     vdd_log: DCDC_REG1 {
> > +                             regulator-name = "vdd_log";
> > +                             regulator-min-microvolt = <712500>;
> > +                             regulator-max-microvolt = <1450000>;
> > +                             regulator-ramp-delay = <12500>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <1000000>;
> > +                             };
> > +                     };
> > +
> > +                     vdd_arm: DCDC_REG2 {
> > +                             regulator-name = "vdd_arm";
> > +                             regulator-min-microvolt = <712500>;
> > +                             regulator-max-microvolt = <1450000>;
> > +                             regulator-ramp-delay = <12500>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <950000>;
> > +                             };
> > +                     };
> > +
> > +                     vcc_ddr: DCDC_REG3 {
> > +                             regulator-name = "vcc_ddr";
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                             };
> > +                     };
> > +
> > +                     vcc_io: DCDC_REG4 {
> > +                             regulator-name = "vcc_io";
> > +                             regulator-min-microvolt = <3300000>;
> > +                             regulator-max-microvolt = <3300000>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <3300000>;
> > +                             };
> > +                     };
> > +
> > +                     vcc_18: LDO_REG1 {
> > +                             regulator-name = "vcc_18";
> > +                             regulator-min-microvolt = <1800000>;
> > +                             regulator-max-microvolt = <1800000>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <1800000>;
> > +                             };
> > +                     };
> > +
> > +                     vcc18_emmc: LDO_REG2 {
> > +                             regulator-name = "vcc18_emmc";
> > +                             regulator-min-microvolt = <1800000>;
> > +                             regulator-max-microvolt = <1800000>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <1800000>;
> > +                             };
> > +                     };
> > +
> > +                     vdd_10: LDO_REG3 {
> > +                             regulator-name = "vdd_10";
> > +                             regulator-min-microvolt = <1000000>;
> > +                             regulator-max-microvolt = <1000000>;
> > +                             regulator-always-on;
> > +                             regulator-boot-on;
> > +
> > +                             regulator-state-mem {
> > +                                     regulator-on-in-suspend;
> > +                                     regulator-suspend-microvolt = <1000000>;
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> > +&i2s1 {
> > +     status = "okay";
> > +};
> > +
> > +&io_domains {
> > +     pmuio-supply = <&vcc_io>;
> > +     vccio1-supply = <&vcc_io>;
> > +     vccio2-supply = <&vcc18_emmc>;
> > +     vccio3-supply = <&vcc_io>;
> > +     vccio4-supply = <&vcc_io>;
> > +     vccio5-supply = <&vcc_io>;
> > +     vccio6-supply = <&vcc_io>;
> > +     status = "okay";
> > +};
> > +
> > +&pinctrl {
>
> > +     ethernet-phy {
>
> gmac2io

OK.

> phy / ethernet-phy is a reserved node name
> use something else
>
> make ARCH=arm64 dtbs_check
>
> /arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dt.yaml: ethernet-phy:
> 'reg' is a required property
>         From schema: Documentation/devicetree/bindings/net/ethernet-phy.yaml

That's somewhat annoying. :(

I wouldn't say the name is "reserved", just that the binding checking
mechanism can't account for these situations.

> > +             eth_phy_int_pin: eth-phy-int-pin {
> > +                     rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
> > +             };
> > +
> > +             eth_phy_reset_pin: eth-phy-reset-pin {
> > +                     rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
> > +             };
> > +     };
> > +
> > +     leds {
> > +             led_pin: led-pin {
> > +                     rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
> > +             };
> > +     };
> > +
> > +     pmic {
> > +             pmic_int_l: pmic-int-l {
> > +                     rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;
> > +             };
> > +     };
> > +
>
> > +     usb3 {
>
> usb
>
> Last numbers in nodenames are more related to the sort order then to
> capabillity.
> ie: mmc0, mmc1
> All usb pin related things here.

I'd say it is more related to functionality in this case, as in "this group
is for USB3 related pins". Makes more sense if the board supported both USB2
and USB3.

> > +             usb30_host_drv: usb30-host-drv {
> > +                     rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_none>;
> > +             };
> > +     };
> > +
> > +     wifi {
> > +             wifi_en: wifi-en {
> > +                     rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +             };
> > +     };
> > +};
> > +
> > +&sdmmc {
> > +     bus-width = <4>;
>
> > +     cap-mmc-highspeed;
>
> remove
> micro SD only
>
> > +     cap-sd-highspeed;
> > +     disable-wp;
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&sdmmc0_clk>, <&sdmmc0_cmd>, <&sdmmc0_dectn>, <&sdmmc0_bus4>;
> > +     vmmc-supply = <&vcc_sd>;
> > +     status = "okay";
> > +};
> > +
>
> > +&saradc {
> > +     vref-supply = <&vcc_18>;
> > +     status = "okay";
> > +};
>
> What happened to the recovery key from the schematic?

I believe I originally planned on adding it, but failed to find a proper
key event for it. Any suggestions?

AFAIK only U-boot handles the recovery button, but in its case it just
looks for the saradc and reads from a predefined channel.


Regards
ChenYu

> > +
> > +&tsadc {
> > +     status = "okay";
> > +};
> > +
> > +&u2phy {
> > +     status = "okay";
> > +};
> > +
> > +&u2phy_host {
> > +     status = "okay";
> > +};
> > +
> > +&uart2 {
> > +     status = "okay";
> > +};
> > +
> > +&usb_host0_ehci {
> > +     status = "okay";
> > +};
> >
>
Heiko Stübner Jan. 10, 2021, 8:06 p.m. UTC | #3
Hi,

Am Sonntag, 10. Januar 2021, 16:37:15 CET schrieb Chen-Yu Tsai:
> > > +     vcc_sd: sdmmc-regulator {
> > > +             compatible = "regulator-fixed";
> > > +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > > +             pinctrl-names = "default";
> > > +             pinctrl-0 = <&sdmmc0m1_pin>;
> >
> > > +             regulator-boot-on;
> > > +             regulator-name = "vcc_sd";
> >
> > regulator-name above other regulator properties
> 
> That is actually what I was used to, but some other rockchip dts files
> have all the properties sorted alphabetically. So I stuck with what I
> saw.

I try to keep it alphabetical except for the exceptions :-D .

regulator-name is such an exception. Similar to compatibles, the
regulator-name is an entry needed to see if you're at the right node,
so I really like it being the topmost regulator-foo property - just makes
reading easier.

(same for the compatible first, then regs, interrupts parts, as well
as "status-last")

But oftentimes, I just fix the ordering when applying - but seem to have
missed this somewhere in those "other Rockchip dts files" ;-) .


> > regulator voltage missing
> > make things as complete as possible
> >
> > from fixed-regulator.yaml:
> >
> > description:
> >   Any property defined as part of the core regulator binding, defined in
> >   regulator.yaml, can also be used. However a fixed voltage regulator is
> >   expected to have the regulator-min-microvolt and regulator-max-microvolt
> >   to be the same.
> 
> However this is not a real regulator; it is merely an on/off switch.
> I believe in this case it should just pass through the voltage from
> its upstream.

regulator-voltages are not marked required so can stay away if it's just
a dumb switch. I guess it's ok both ways and for individual board-
devicetrees the impact either way is minimal.


> > > +&i2c1 {
> > > +     status = "okay";
> > > +
> > > +     rk805: pmic@18 {
> > > +             compatible = "rockchip,rk805";
> > > +             reg = <0x18>;
> > > +             interrupt-parent = <&gpio2>;
> > > +             interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> >
> > > +             #clock-cells = <1>;
> >
> > all thing that start with "#" down the list
> 
> Is there a proper "preferred" sorting method defined somewhere?

I struggle with that often as well, but normally I'd do #clocks to clocks
with out "#", but really don't have a hard preference here.

especially as just ignoring the "#" would make #address-cells + #size-cells
look strangely sorted ... so more of a common sense thingy.


> > > +             eth_phy_int_pin: eth-phy-int-pin {
> > > +                     rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
> > > +             };
> > > +
> > > +             eth_phy_reset_pin: eth-phy-reset-pin {
> > > +                     rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
> > > +             };
> > > +     };
> > > +
> > > +     leds {
> > > +             led_pin: led-pin {
> > > +                     rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
> > > +             };
> > > +     };
> > > +
> > > +     pmic {
> > > +             pmic_int_l: pmic-int-l {
> > > +                     rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;
> > > +             };
> > > +     };
> > > +
> >
> > > +     usb3 {
> >
> > usb
> >
> > Last numbers in nodenames are more related to the sort order then to
> > capabillity.
> > ie: mmc0, mmc1
> > All usb pin related things here.
> 
> I'd say it is more related to functionality in this case, as in "this group
> is for USB3 related pins". Makes more sense if the board supported both USB2
> and USB3.

I'd agree :-) ... especially as usb controllers on Rockchip boards are not
really numbered and I think we already have precedent for
usb2 -> usb version 2 pins in some other boards ;-)


> > > +     cap-sd-highspeed;
> > > +     disable-wp;
> > > +     pinctrl-names = "default";
> > > +     pinctrl-0 = <&sdmmc0_clk>, <&sdmmc0_cmd>, <&sdmmc0_dectn>, <&sdmmc0_bus4>;
> > > +     vmmc-supply = <&vcc_sd>;
> > > +     status = "okay";
> > > +};
> > > +
> >
> > > +&saradc {
> > > +     vref-supply = <&vcc_18>;
> > > +     status = "okay";
> > > +};
> >
> > What happened to the recovery key from the schematic?
> 
> I believe I originally planned on adding it, but failed to find a proper
> key event for it. Any suggestions?

Most boards seem to use the KEY_VENDOR keycode.


Heiko
Johan Jonker Jan. 10, 2021, 8:17 p.m. UTC | #4
Hi Chen-Yu,

Most is already answered by Heiko.

On 1/10/21 4:37 PM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sun, Jan 10, 2021 at 10:45 PM Johan Jonker <jbx6244@gmail.com> wrote:
>>
>> Hi Chen-Yu,
>>
>> Some comments, have a look if it is useful...
>>
>> On 1/10/21 4:58 AM, Chen-Yu Tsai wrote:
>>> From: Chen-Yu Tsai <wens@csie.org>
>>>
>>> Radxa ROCK Pi E is a router oriented SBC based on Rockchip's RK3328 SoC.
>>> As the official wiki page puts it, "E for Ethernets".
>>>
>>> It features the RK3328 SoC, gigabit and fast Ethernet RJ45 ports, both
>>> directly served by Ethernet controllers in the SoC, a USB 3.0 host port,
>>> a power-only USB type-C port, a 3.5mm headphone jack for audio output,
>>
>>> two LEDs, a 40-pin Raspberry Pi style GPIO header, and optional WiFi+BT
>>> and PoE header.
>>>
>>> The board comes in multiple configurations, differing in the amount of
>>> onboard RAM, the level of WiFi+BT (none, 802.11n 2.4GHz, or 802.11ac
>>> 2.4 GHz & 5 GHz), and whether PoE is supported or not. These variants
>>> can all share the same device tree.
>>>
>>> The USB 2.0 OTG controller is available on the 40-pin header. This is
>>> not enabled in the device tree, since it is possible to use it in a
>>> host-only configuration, or in OTG mode with an extra pin from the
>>> header as the ID pin.
>>>
>>> The device tree is based on the one of the Rock64, with various parts
>>> modified to match the ROCK Pi E, and some parts updated to newer styles,
>>> such as the gmac2io node's mdio sub-node.
>>>
>>> Add a new device tree file for the new board.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>>>  .../boot/dts/rockchip/rk3328-rock-pi-e.dts    | 369 ++++++++++++++++++
>>>  2 files changed, 370 insertions(+)
>>>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
>>> index 622d320ddd13..62d3abc17a24 100644
>>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>>> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
>>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-geekbox.dtb
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>> new file mode 100644
>>> index 000000000000..7818d2e8180c
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>> @@ -0,0 +1,369 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * (C) Copyright 2020 Chen-Yu Tsai <wens@csie.org>
>>> + *
>>> + * Based on ./rk3328-rock64.dts, which is
>>> + *
>>> + * Copyright (c) 2017 PINE64
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include <dt-bindings/leds/common.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/pinctrl/rockchip.h>
>>> +#include "rk3328.dtsi"
>>> +
>>> +/ {
>>> +     model = "Radxa ROCK Pi E";
>>> +     compatible = "radxa,rockpi-e", "rockchip,rk3328";
>>> +
>>> +     chosen {
>>> +             stdout-path = "serial2:1500000n8";
>>> +     };
>>> +
>>> +     gmac_clkin: external-gmac-clock {
>>> +             compatible = "fixed-clock";
>>> +             clock-frequency = <125000000>;
>>> +             clock-output-names = "gmac_clkin";
>>> +             #clock-cells = <0>;
>>> +     };
>>> +
>>> +     leds {
>>> +             compatible = "gpio-leds";
>>> +             pinctrl-0 = <&led_pin>;
>>> +             pinctrl-names = "default";
>>> +
>>> +             led-0 {
>>
>>> +                     /* schematic say green but the actual thing is blue */
>>
>> In rockpie-v1.2-20200427-sch.pdf this led is already called "LED_BLUE",
>> so comment maybe not needed anymore?
> 
> Thanks. Did not notice there was a new revision.
> 
>>> +                     color = <LED_COLOR_ID_BLUE>;
>>> +                     gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
>>> +                     linux,default-trigger = "heartbeat";
>>> +             };> +   };
>>> +
>>> +     vcc_sd: sdmmc-regulator {
>>> +             compatible = "regulator-fixed";
>>> +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&sdmmc0m1_pin>;
>>
>>> +             regulator-boot-on;
>>> +             regulator-name = "vcc_sd";
>>
>> regulator-name above other regulator properties
> 
> That is actually what I was used to, but some other rockchip dts files
> have all the properties sorted alphabetically. So I stuck with what I
> saw.
> 
>> regulator voltage missing
>> make things as complete as possible
>>
>> from fixed-regulator.yaml:
>>
>> description:
>>   Any property defined as part of the core regulator binding, defined in
>>   regulator.yaml, can also be used. However a fixed voltage regulator is
>>   expected to have the regulator-min-microvolt and regulator-max-microvolt
>>   to be the same.
> 
> However this is not a real regulator; it is merely an on/off switch.
> I believe in this case it should just pass through the voltage from
> its upstream.

This board is not black box. The schematics are public, so finding out
limits was not the problem. Up to you if added or not...

> 
>>> +             vin-supply = <&vcc_io>;
>>> +     };
>>> +
>>
>>> +     vcc_host_5v: vcc-host-5v-regulator {
>>> +             compatible = "regulator-fixed";
>>> +             gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&usb30_host_drv>;
>>> +             enable-active-high;
>>> +             regulator-name = "vcc_host_5v";
>>
>> idem limits
> 
> Same here.
> 
>>> +             regulator-always-on;
>>> +             regulator-boot-on;
>>> +             vin-supply = <&vcc_sys>;
>>> +     };
>>
>> For Heiko: ?? remove ??
>> usb3 has no support in mainline.
>> Regulators not in use are disabled.
>> For mainline this node has no use....
> 
> As it already has a defined binding, we can put it in the device tree.
> 
>>> +
>>> +     vcc_sys: vcc-sys {
>>> +             compatible = "regulator-fixed";
>>> +             regulator-name = "vcc_sys";
>>
>>> +             regulator-always-on;
>>> +             regulator-boot-on;
>>
>> At the other regulators this is sort below the regulator limits.
> 
> Again, alphabetically sorted vs preferred sorting method.

There's some room for "creativity", but you must use that "style"
document wise. But not above limits in "vcc_sys" and below in "vcc_io".

From Heiko:

compatible
reg
interrupts
[alphabetical]
status [if needed]

////////////////////

My list:

For nodes:
If exists on top: model, compatible and chosen.
Sort things without reg alphabetical first,
then sort the rest by reg address.

Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.
Keep an empty line between properties and nodes.

Exceptions:
Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
and dma-names.
Sort simple-audio-card,name above other simple-audio-card properties.
Sort regulator-name above other regulator properties.
Sort regulator-min-microvolt above regulator-max-microvolt.

> 
>>> +             regulator-min-microvolt = <5000000>;
>>> +             regulator-max-microvolt = <5000000>;
>>> +     };
>>> +
>>> +     vcc_wifi: vcc-wifi-regulator {
>>> +             compatible = "regulator-fixed";
>>> +             gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&wifi_en>;
>>> +             regulator-name = "vcc_wifi";
>>
>> idem limits
> 
> Again, it is just a switch.
> 
>>> +             regulator-always-on;
>>> +             regulator-boot-on;
>>> +             vin-supply = <&vcc_io>;
>>> +     };
>>> +};
>>> +
>>> +&analog_sound {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&codec {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&cpu0 {
>>> +     cpu-supply = <&vdd_arm>;
>>> +};
>>> +
>>> +&cpu1 {
>>> +     cpu-supply = <&vdd_arm>;
>>> +};
>>> +
>>> +&cpu2 {
>>> +     cpu-supply = <&vdd_arm>;
>>> +};
>>> +
>>> +&cpu3 {
>>> +     cpu-supply = <&vdd_arm>;
>>> +};
>>> +
>>> +&emmc {
>>> +     bus-width = <8>;
>>> +     cap-mmc-highspeed;
>>
>>> +     max-frequency = <150000000>;
>>
>> remove
>> already defined in dtsi
> 
> OK.
> 
>>> +     mmc-ddr-1_8v;
>>> +     mmc-hs200-1_8v;
>>> +     non-removable;
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_bus8>;
>>> +     vmmc-supply = <&vcc_io>;
>>> +     vqmmc-supply = <&vcc18_emmc>;
>>> +     status = "okay";
>>> +};
>>
>> ////////////////////////
>>         emmc: mmc@ff520000 {
>>                 compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc";
>>                 reg = <0x0 0xff520000 0x0 0x4000>;
>>                 interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>                 clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
>>                          <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
>>                 clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>>                 fifo-depth = <0x100>;
>>                 max-frequency = <150000000>;
>>                 status = "disabled";
>>         };
>> ////////////////////////
>>
>>> +
>>> +&gmac2io {
>>> +     assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>> +     assigned-clock-parents = <&gmac_clkin>, <&gmac_clkin>;
>>> +     clock_in_out = "input";
>>> +     phy-handle = <&rtl8211e>;
>>> +     phy-mode = "rgmii";
>>> +     phy-supply = <&vcc_io>;
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&rgmiim1_pins>;
>>> +     snps,aal;
>>> +     snps,rxpbl = <0x4>;
>>> +     snps,txpbl = <0x4>;
>>> +     tx_delay = <0x26>;
>>> +     rx_delay = <0x11>;
>>> +     status = "okay";
>>> +
>>> +     mdio {
>>> +             compatible = "snps,dwmac-mdio";
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>> +
>>> +             rtl8211e: ethernet-phy@1 {
>>> +                     reg = <1>;
>>> +                     pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
>>> +                     pinctrl-names = "default";
>>> +                     interrupt-parent = <&gpio1>;
>>> +                     interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
>>> +                     reset-assert-us = <10000>;
>>> +                     reset-deassert-us = <50000>;
>>> +                     reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&gmac2phy {
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&fephyled_linkm1>, <&fephyled_rxm1>;
>>> +     status = "okay";
>>> +};
>>> +
>>> +&i2c1 {
>>> +     status = "okay";
>>> +
>>> +     rk805: pmic@18 {
>>> +             compatible = "rockchip,rk805";
>>> +             reg = <0x18>;
>>> +             interrupt-parent = <&gpio2>;
>>> +             interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
>>
>>> +             #clock-cells = <1>;
>>
>> all thing that start with "#" down the list
> 
> Is there a proper "preferred" sorting method defined somewhere?

See above.

> 
>>> +             clock-output-names = "xin32k", "rk805-clkout2";
>>> +             gpio-controller;
>>
>>> +             #gpio-cells = <2>;
>>
>> idem
>>
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&pmic_int_l>;
>>> +             rockchip,system-power-controller;
>>> +             wakeup-source;
>>> +
>>> +             vcc1-supply = <&vcc_sys>;
>>> +             vcc2-supply = <&vcc_sys>;
>>> +             vcc3-supply = <&vcc_sys>;
>>> +             vcc4-supply = <&vcc_sys>;
>>> +             vcc5-supply = <&vcc_io>;
>>> +             vcc6-supply = <&vcc_sys>;
>>> +
>>> +             regulators {
>>> +                     vdd_log: DCDC_REG1 {
>>> +                             regulator-name = "vdd_log";
>>> +                             regulator-min-microvolt = <712500>;
>>> +                             regulator-max-microvolt = <1450000>;
>>> +                             regulator-ramp-delay = <12500>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <1000000>;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vdd_arm: DCDC_REG2 {
>>> +                             regulator-name = "vdd_arm";
>>> +                             regulator-min-microvolt = <712500>;
>>> +                             regulator-max-microvolt = <1450000>;
>>> +                             regulator-ramp-delay = <12500>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <950000>;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vcc_ddr: DCDC_REG3 {
>>> +                             regulator-name = "vcc_ddr";
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vcc_io: DCDC_REG4 {
>>> +                             regulator-name = "vcc_io";
>>> +                             regulator-min-microvolt = <3300000>;
>>> +                             regulator-max-microvolt = <3300000>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <3300000>;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vcc_18: LDO_REG1 {
>>> +                             regulator-name = "vcc_18";
>>> +                             regulator-min-microvolt = <1800000>;
>>> +                             regulator-max-microvolt = <1800000>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <1800000>;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vcc18_emmc: LDO_REG2 {
>>> +                             regulator-name = "vcc18_emmc";
>>> +                             regulator-min-microvolt = <1800000>;
>>> +                             regulator-max-microvolt = <1800000>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <1800000>;
>>> +                             };
>>> +                     };
>>> +
>>> +                     vdd_10: LDO_REG3 {
>>> +                             regulator-name = "vdd_10";
>>> +                             regulator-min-microvolt = <1000000>;
>>> +                             regulator-max-microvolt = <1000000>;
>>> +                             regulator-always-on;
>>> +                             regulator-boot-on;
>>> +
>>> +                             regulator-state-mem {
>>> +                                     regulator-on-in-suspend;
>>> +                                     regulator-suspend-microvolt = <1000000>;
>>> +                             };
>>> +                     };
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&i2s1 {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&io_domains {
>>> +     pmuio-supply = <&vcc_io>;
>>> +     vccio1-supply = <&vcc_io>;
>>> +     vccio2-supply = <&vcc18_emmc>;
>>> +     vccio3-supply = <&vcc_io>;
>>> +     vccio4-supply = <&vcc_io>;
>>> +     vccio5-supply = <&vcc_io>;
>>> +     vccio6-supply = <&vcc_io>;
>>> +     status = "okay";
>>> +};
>>> +
>>> +&pinctrl {
>>
>>> +     ethernet-phy {
>>
>> gmac2io
> 
> OK.
> 
>> phy / ethernet-phy is a reserved node name
>> use something else
>>
>> make ARCH=arm64 dtbs_check
>>
>> /arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dt.yaml: ethernet-phy:
>> 'reg' is a required property
>>         From schema: Documentation/devicetree/bindings/net/ethernet-phy.yaml
> 
> That's somewhat annoying. :(
> 
> I wouldn't say the name is "reserved", just that the binding checking
> mechanism can't account for these situations.
> 
>>> +             eth_phy_int_pin: eth-phy-int-pin {
>>> +                     rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
>>> +             };
>>> +
>>> +             eth_phy_reset_pin: eth-phy-reset-pin {
>>> +                     rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
>>> +             };
>>> +     };
>>> +
>>> +     leds {
>>> +             led_pin: led-pin {
>>> +                     rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
>>> +             };
>>> +     };
>>> +
>>> +     pmic {
>>> +             pmic_int_l: pmic-int-l {
>>> +                     rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;
>>> +             };
>>> +     };
>>> +
>>
>>> +     usb3 {
>>
>> usb
>>
>> Last numbers in nodenames are more related to the sort order then to
>> capabillity.
>> ie: mmc0, mmc1
>> All usb pin related things here.
> 
> I'd say it is more related to functionality in this case, as in "this group
> is for USB3 related pins". Makes more sense if the board supported both USB2
> and USB3.
> 
>>> +             usb30_host_drv: usb30-host-drv {
>>> +                     rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_none>;
>>> +             };
>>> +     };
>>> +
>>> +     wifi {
>>> +             wifi_en: wifi-en {
>>> +                     rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&sdmmc {
>>> +     bus-width = <4>;
>>
>>> +     cap-mmc-highspeed;
>>
>> remove
>> micro SD only
>>
>>> +     cap-sd-highspeed;
>>> +     disable-wp;
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&sdmmc0_clk>, <&sdmmc0_cmd>, <&sdmmc0_dectn>, <&sdmmc0_bus4>;
>>> +     vmmc-supply = <&vcc_sd>;
>>> +     status = "okay";
>>> +};
>>> +
>>
>>> +&saradc {
>>> +     vref-supply = <&vcc_18>;
>>> +     status = "okay";
>>> +};
>>
>> What happened to the recovery key from the schematic?
> 
> I believe I originally planned on adding it, but failed to find a proper
> key event for it. Any suggestions?

The consensus seem to be "KEY_VENDOR".

Example:

	adc-keys {
		compatible = "adc-keys";
		io-channels = <&saradc 0>;
		io-channel-names = "buttons";
		keyup-threshold-microvolt = <1800000>;
		poll-interval = <100>;

		recovery {
			label = "recovery";
			linux,code = <KEY_VENDOR>;
			press-threshold-microvolt = <17000>;
		};
	};

> 
> AFAIK only U-boot handles the recovery button, but in its case it just
> looks for the saradc and reads from a predefined channel.
> 
> 
> Regards
> ChenYu
> 
>>> +
>>> +&tsadc {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&u2phy {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&u2phy_host {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&uart2 {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&usb_host0_ehci {
>>> +     status = "okay";
>>> +};
>>>
>>
Chen-Yu Tsai Jan. 11, 2021, 3:27 a.m. UTC | #5
On Mon, Jan 11, 2021 at 4:06 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi,
>
> Am Sonntag, 10. Januar 2021, 16:37:15 CET schrieb Chen-Yu Tsai:
> > > > +     vcc_sd: sdmmc-regulator {
> > > > +             compatible = "regulator-fixed";
> > > > +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > > > +             pinctrl-names = "default";
> > > > +             pinctrl-0 = <&sdmmc0m1_pin>;
> > >
> > > > +             regulator-boot-on;
> > > > +             regulator-name = "vcc_sd";
> > >
> > > regulator-name above other regulator properties
> >
> > That is actually what I was used to, but some other rockchip dts files
> > have all the properties sorted alphabetically. So I stuck with what I
> > saw.
>
> I try to keep it alphabetical except for the exceptions :-D .
>
> regulator-name is such an exception. Similar to compatibles, the
> regulator-name is an entry needed to see if you're at the right node,
> so I really like it being the topmost regulator-foo property - just makes
> reading easier.
>
> (same for the compatible first, then regs, interrupts parts, as well
> as "status-last")
>
> But oftentimes, I just fix the ordering when applying - but seem to have
> missed this somewhere in those "other Rockchip dts files" ;-) .

I was slightly confused. I looked again and yes regulator-name is always the
first regulator related property. What's off is that in some cases min/max
voltage comes before always-on/boot-on, and in others vice versa.

For example in the Rock64 and ROC-RK3328-CC device trees, in the fixed
regulators, always-on/boot-on come before min/max voltage, while in the
PMIC the other order is used.


ChenYu
Chen-Yu Tsai Jan. 11, 2021, 3:42 a.m. UTC | #6
On Mon, Jan 11, 2021 at 4:17 AM Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi Chen-Yu,
>
> Most is already answered by Heiko.
>
> On 1/10/21 4:37 PM, Chen-Yu Tsai wrote:
> > Hi,
> >
> > On Sun, Jan 10, 2021 at 10:45 PM Johan Jonker <jbx6244@gmail.com> wrote:
> >>
> >> Hi Chen-Yu,
> >>
> >> Some comments, have a look if it is useful...
> >>
> >> On 1/10/21 4:58 AM, Chen-Yu Tsai wrote:
> >>> From: Chen-Yu Tsai <wens@csie.org>
> >>>
> >>> Radxa ROCK Pi E is a router oriented SBC based on Rockchip's RK3328 SoC.
> >>> As the official wiki page puts it, "E for Ethernets".
> >>>
> >>> It features the RK3328 SoC, gigabit and fast Ethernet RJ45 ports, both
> >>> directly served by Ethernet controllers in the SoC, a USB 3.0 host port,
> >>> a power-only USB type-C port, a 3.5mm headphone jack for audio output,
> >>
> >>> two LEDs, a 40-pin Raspberry Pi style GPIO header, and optional WiFi+BT
> >>> and PoE header.
> >>>
> >>> The board comes in multiple configurations, differing in the amount of
> >>> onboard RAM, the level of WiFi+BT (none, 802.11n 2.4GHz, or 802.11ac
> >>> 2.4 GHz & 5 GHz), and whether PoE is supported or not. These variants
> >>> can all share the same device tree.
> >>>
> >>> The USB 2.0 OTG controller is available on the 40-pin header. This is
> >>> not enabled in the device tree, since it is possible to use it in a
> >>> host-only configuration, or in OTG mode with an extra pin from the
> >>> header as the ID pin.
> >>>
> >>> The device tree is based on the one of the Rock64, with various parts
> >>> modified to match the ROCK Pi E, and some parts updated to newer styles,
> >>> such as the gmac2io node's mdio sub-node.
> >>>
> >>> Add a new device tree file for the new board.
> >>>
> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> >>>  .../boot/dts/rockchip/rk3328-rock-pi-e.dts    | 369 ++++++++++++++++++
> >>>  2 files changed, 370 insertions(+)
> >>>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> >>> index 622d320ddd13..62d3abc17a24 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/Makefile
> >>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> >>> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
> >>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
> >>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-geekbox.dtb
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> >>> new file mode 100644
> >>> index 000000000000..7818d2e8180c
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
> >>> @@ -0,0 +1,369 @@
> >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>> +/*
> >>> + * (C) Copyright 2020 Chen-Yu Tsai <wens@csie.org>
> >>> + *
> >>> + * Based on ./rk3328-rock64.dts, which is
> >>> + *
> >>> + * Copyright (c) 2017 PINE64
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +
> >>> +#include <dt-bindings/leds/common.h>
> >>> +#include <dt-bindings/gpio/gpio.h>
> >>> +#include <dt-bindings/pinctrl/rockchip.h>
> >>> +#include "rk3328.dtsi"
> >>> +
> >>> +/ {
> >>> +     model = "Radxa ROCK Pi E";
> >>> +     compatible = "radxa,rockpi-e", "rockchip,rk3328";
> >>> +
> >>> +     chosen {
> >>> +             stdout-path = "serial2:1500000n8";
> >>> +     };
> >>> +
> >>> +     gmac_clkin: external-gmac-clock {
> >>> +             compatible = "fixed-clock";
> >>> +             clock-frequency = <125000000>;
> >>> +             clock-output-names = "gmac_clkin";
> >>> +             #clock-cells = <0>;
> >>> +     };
> >>> +
> >>> +     leds {
> >>> +             compatible = "gpio-leds";
> >>> +             pinctrl-0 = <&led_pin>;
> >>> +             pinctrl-names = "default";
> >>> +
> >>> +             led-0 {
> >>
> >>> +                     /* schematic say green but the actual thing is blue */
> >>
> >> In rockpie-v1.2-20200427-sch.pdf this led is already called "LED_BLUE",
> >> so comment maybe not needed anymore?
> >
> > Thanks. Did not notice there was a new revision.
> >
> >>> +                     color = <LED_COLOR_ID_BLUE>;
> >>> +                     gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
> >>> +                     linux,default-trigger = "heartbeat";
> >>> +             };> +   };
> >>> +
> >>> +     vcc_sd: sdmmc-regulator {
> >>> +             compatible = "regulator-fixed";
> >>> +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&sdmmc0m1_pin>;
> >>
> >>> +             regulator-boot-on;
> >>> +             regulator-name = "vcc_sd";
> >>
> >> regulator-name above other regulator properties
> >
> > That is actually what I was used to, but some other rockchip dts files
> > have all the properties sorted alphabetically. So I stuck with what I
> > saw.
> >
> >> regulator voltage missing
> >> make things as complete as possible
> >>
> >> from fixed-regulator.yaml:
> >>
> >> description:
> >>   Any property defined as part of the core regulator binding, defined in
> >>   regulator.yaml, can also be used. However a fixed voltage regulator is
> >>   expected to have the regulator-min-microvolt and regulator-max-microvolt
> >>   to be the same.
> >
> > However this is not a real regulator; it is merely an on/off switch.
> > I believe in this case it should just pass through the voltage from
> > its upstream.
>
> This board is not black box. The schematics are public, so finding out
> limits was not the problem. Up to you if added or not...

As far as Linux is concerned, if the regulator does not have constraints /
voltages, then its upstream one's values is passed through. I believe that
correctly represents what the hardware is doing, so I will stick to that.
I might have asked Mark about this in the past, but I don't remember
exactly when and where.

> >>> +             vin-supply = <&vcc_io>;
> >>> +     };
> >>> +
> >>
> >>> +     vcc_host_5v: vcc-host-5v-regulator {
> >>> +             compatible = "regulator-fixed";
> >>> +             gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>;
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&usb30_host_drv>;
> >>> +             enable-active-high;
> >>> +             regulator-name = "vcc_host_5v";
> >>
> >> idem limits
> >
> > Same here.
> >
> >>> +             regulator-always-on;
> >>> +             regulator-boot-on;
> >>> +             vin-supply = <&vcc_sys>;
> >>> +     };
> >>
> >> For Heiko: ?? remove ??
> >> usb3 has no support in mainline.
> >> Regulators not in use are disabled.
> >> For mainline this node has no use....
> >
> > As it already has a defined binding, we can put it in the device tree.
> >
> >>> +
> >>> +     vcc_sys: vcc-sys {
> >>> +             compatible = "regulator-fixed";
> >>> +             regulator-name = "vcc_sys";
> >>
> >>> +             regulator-always-on;
> >>> +             regulator-boot-on;
> >>
> >> At the other regulators this is sort below the regulator limits.
> >
> > Again, alphabetically sorted vs preferred sorting method.
>
> There's some room for "creativity", but you must use that "style"
> document wise. But not above limits in "vcc_sys" and below in "vcc_io".

Sorry. I thought you were referring to the other fixed regulators.
I see now you meant the regulators in the PMIC. I will fix those up
to have consistent styling.

Seems that other boards have the same problem.

> From Heiko:
>
> compatible
> reg
> interrupts
> [alphabetical]
> status [if needed]
>
> ////////////////////
>
> My list:
>
> For nodes:
> If exists on top: model, compatible and chosen.
> Sort things without reg alphabetical first,
> then sort the rest by reg address.
>
> Inside nodes:
> If exists on top: compatible, reg and interrupts.
> In alphabetical order the required properties.
> Then in alphabetical order the other properties.
> And as last things that start with '#' in alphabetical order.

For me, it makes more sense to keep #gpio-cells grouped together with
gpio-controller, and the same for other controller types. #clock-cells
grouped with clock-output-names also makes it easier to read.

#address-cells and #size-cells naturally go to the end of the list as
what they really affect are the child nodes, which get listed after.

Since Heiko is OK either way, I'll stick to my ordering for now.

> Add status below all other properties for soc internal components with
> any board-specifics.
> Keep an empty line between properties and nodes.
>
> Exceptions:
> Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
> and dma-names.
> Sort simple-audio-card,name above other simple-audio-card properties.
> Sort regulator-name above other regulator properties.
> Sort regulator-min-microvolt above regulator-max-microvolt.
>
> >
> >>> +             regulator-min-microvolt = <5000000>;
> >>> +             regulator-max-microvolt = <5000000>;
> >>> +     };
> >>> +
> >>> +     vcc_wifi: vcc-wifi-regulator {
> >>> +             compatible = "regulator-fixed";
> >>> +             gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&wifi_en>;
> >>> +             regulator-name = "vcc_wifi";
> >>
> >> idem limits
> >
> > Again, it is just a switch.
> >
> >>> +             regulator-always-on;
> >>> +             regulator-boot-on;
> >>> +             vin-supply = <&vcc_io>;
> >>> +     };
> >>> +};
> >>> +
> >>> +&analog_sound {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&codec {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&cpu0 {
> >>> +     cpu-supply = <&vdd_arm>;
> >>> +};
> >>> +
> >>> +&cpu1 {
> >>> +     cpu-supply = <&vdd_arm>;
> >>> +};
> >>> +
> >>> +&cpu2 {
> >>> +     cpu-supply = <&vdd_arm>;
> >>> +};
> >>> +
> >>> +&cpu3 {
> >>> +     cpu-supply = <&vdd_arm>;
> >>> +};
> >>> +
> >>> +&emmc {
> >>> +     bus-width = <8>;
> >>> +     cap-mmc-highspeed;
> >>
> >>> +     max-frequency = <150000000>;
> >>
> >> remove
> >> already defined in dtsi
> >
> > OK.
> >
> >>> +     mmc-ddr-1_8v;
> >>> +     mmc-hs200-1_8v;
> >>> +     non-removable;
> >>> +     pinctrl-names = "default";
> >>> +     pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_bus8>;
> >>> +     vmmc-supply = <&vcc_io>;
> >>> +     vqmmc-supply = <&vcc18_emmc>;
> >>> +     status = "okay";
> >>> +};
> >>
> >> ////////////////////////
> >>         emmc: mmc@ff520000 {
> >>                 compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc";
> >>                 reg = <0x0 0xff520000 0x0 0x4000>;
> >>                 interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> >>                 clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
> >>                          <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
> >>                 clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> >>                 fifo-depth = <0x100>;
> >>                 max-frequency = <150000000>;
> >>                 status = "disabled";
> >>         };
> >> ////////////////////////
> >>
> >>> +
> >>> +&gmac2io {
> >>> +     assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
> >>> +     assigned-clock-parents = <&gmac_clkin>, <&gmac_clkin>;
> >>> +     clock_in_out = "input";
> >>> +     phy-handle = <&rtl8211e>;
> >>> +     phy-mode = "rgmii";
> >>> +     phy-supply = <&vcc_io>;
> >>> +     pinctrl-names = "default";
> >>> +     pinctrl-0 = <&rgmiim1_pins>;
> >>> +     snps,aal;
> >>> +     snps,rxpbl = <0x4>;
> >>> +     snps,txpbl = <0x4>;
> >>> +     tx_delay = <0x26>;
> >>> +     rx_delay = <0x11>;
> >>> +     status = "okay";
> >>> +
> >>> +     mdio {
> >>> +             compatible = "snps,dwmac-mdio";
> >>> +             #address-cells = <1>;
> >>> +             #size-cells = <0>;
> >>> +
> >>> +             rtl8211e: ethernet-phy@1 {
> >>> +                     reg = <1>;
> >>> +                     pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
> >>> +                     pinctrl-names = "default";
> >>> +                     interrupt-parent = <&gpio1>;
> >>> +                     interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
> >>> +                     reset-assert-us = <10000>;
> >>> +                     reset-deassert-us = <50000>;
> >>> +                     reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> +
> >>> +&gmac2phy {
> >>> +     pinctrl-names = "default";
> >>> +     pinctrl-0 = <&fephyled_linkm1>, <&fephyled_rxm1>;
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&i2c1 {
> >>> +     status = "okay";
> >>> +
> >>> +     rk805: pmic@18 {
> >>> +             compatible = "rockchip,rk805";
> >>> +             reg = <0x18>;
> >>> +             interrupt-parent = <&gpio2>;
> >>> +             interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> >>
> >>> +             #clock-cells = <1>;
> >>
> >> all thing that start with "#" down the list
> >
> > Is there a proper "preferred" sorting method defined somewhere?
>
> See above.
>
> >
> >>> +             clock-output-names = "xin32k", "rk805-clkout2";
> >>> +             gpio-controller;
> >>
> >>> +             #gpio-cells = <2>;
> >>
> >> idem
> >>

snip

> >>> +
> >>> +&pinctrl {
> >>
> >>> +     ethernet-phy {
> >>
> >> gmac2io
> >
> > OK.

I thought about this a bit more, and I think e-phy or ephy would be a
better name, since these pin functions are not directly related to
gmac2io, but the external Ethernet PHY.

It would be nicer if we could fix the ethernet-phy binding though.

> >> phy / ethernet-phy is a reserved node name
> >> use something else
> >>
> >> make ARCH=arm64 dtbs_check
> >>
> >> /arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dt.yaml: ethernet-phy:
> >> 'reg' is a required property
> >>         From schema: Documentation/devicetree/bindings/net/ethernet-phy.yaml
> >
> > That's somewhat annoying. :(
> >
> > I wouldn't say the name is "reserved", just that the binding checking
> > mechanism can't account for these situations.
> >
> >>> +             eth_phy_int_pin: eth-phy-int-pin {
> >>> +                     rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
> >>> +             };
> >>> +
> >>> +             eth_phy_reset_pin: eth-phy-reset-pin {
> >>> +                     rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
> >>> +             };
> >>> +     };
> >>> +

snip

> >>> +&saradc {
> >>> +     vref-supply = <&vcc_18>;
> >>> +     status = "okay";
> >>> +};
> >>
> >> What happened to the recovery key from the schematic?
> >
> > I believe I originally planned on adding it, but failed to find a proper
> > key event for it. Any suggestions?
>
> The consensus seem to be "KEY_VENDOR".
>
> Example:
>
>         adc-keys {
>                 compatible = "adc-keys";
>                 io-channels = <&saradc 0>;
>                 io-channel-names = "buttons";
>                 keyup-threshold-microvolt = <1800000>;
>                 poll-interval = <100>;
>
>                 recovery {
>                         label = "recovery";
>                         linux,code = <KEY_VENDOR>;
>                         press-threshold-microvolt = <17000>;
>                 };
>         };

Thanks. Though I really wonder what Linux or other OSes could do with it.


ChenYu
Heiko Stübner Jan. 11, 2021, 7:50 a.m. UTC | #7
Am Montag, 11. Januar 2021, 04:27:47 CET schrieb Chen-Yu Tsai:
> On Mon, Jan 11, 2021 at 4:06 AM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Hi,
> >
> > Am Sonntag, 10. Januar 2021, 16:37:15 CET schrieb Chen-Yu Tsai:
> > > > > +     vcc_sd: sdmmc-regulator {
> > > > > +             compatible = "regulator-fixed";
> > > > > +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > > > > +             pinctrl-names = "default";
> > > > > +             pinctrl-0 = <&sdmmc0m1_pin>;
> > > >
> > > > > +             regulator-boot-on;
> > > > > +             regulator-name = "vcc_sd";
> > > >
> > > > regulator-name above other regulator properties
> > >
> > > That is actually what I was used to, but some other rockchip dts files
> > > have all the properties sorted alphabetically. So I stuck with what I
> > > saw.
> >
> > I try to keep it alphabetical except for the exceptions :-D .
> >
> > regulator-name is such an exception. Similar to compatibles, the
> > regulator-name is an entry needed to see if you're at the right node,
> > so I really like it being the topmost regulator-foo property - just makes
> > reading easier.
> >
> > (same for the compatible first, then regs, interrupts parts, as well
> > as "status-last")
> >
> > But oftentimes, I just fix the ordering when applying - but seem to have
> > missed this somewhere in those "other Rockchip dts files" ;-) .
> 
> I was slightly confused. I looked again and yes regulator-name is always the
> first regulator related property. What's off is that in some cases min/max
> voltage comes before always-on/boot-on, and in others vice versa.
> 
> For example in the Rock64 and ROC-RK3328-CC device trees, in the fixed
> regulators, always-on/boot-on come before min/max voltage, while in the
> PMIC the other order is used.

That's likely undecidednes on my part ;-)

There could be an argument for a "name, voltages, flags" sorting, but on
the other hand just keeping it alphabetical with the naming on top
creates less special cases.


Heiko
Chen-Yu Tsai Jan. 11, 2021, 8:04 a.m. UTC | #8
On Mon, Jan 11, 2021 at 3:50 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Montag, 11. Januar 2021, 04:27:47 CET schrieb Chen-Yu Tsai:
> > On Mon, Jan 11, 2021 at 4:06 AM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Hi,
> > >
> > > Am Sonntag, 10. Januar 2021, 16:37:15 CET schrieb Chen-Yu Tsai:
> > > > > > +     vcc_sd: sdmmc-regulator {
> > > > > > +             compatible = "regulator-fixed";
> > > > > > +             gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > > > > > +             pinctrl-names = "default";
> > > > > > +             pinctrl-0 = <&sdmmc0m1_pin>;
> > > > >
> > > > > > +             regulator-boot-on;
> > > > > > +             regulator-name = "vcc_sd";
> > > > >
> > > > > regulator-name above other regulator properties
> > > >
> > > > That is actually what I was used to, but some other rockchip dts files
> > > > have all the properties sorted alphabetically. So I stuck with what I
> > > > saw.
> > >
> > > I try to keep it alphabetical except for the exceptions :-D .
> > >
> > > regulator-name is such an exception. Similar to compatibles, the
> > > regulator-name is an entry needed to see if you're at the right node,
> > > so I really like it being the topmost regulator-foo property - just makes
> > > reading easier.
> > >
> > > (same for the compatible first, then regs, interrupts parts, as well
> > > as "status-last")
> > >
> > > But oftentimes, I just fix the ordering when applying - but seem to have
> > > missed this somewhere in those "other Rockchip dts files" ;-) .
> >
> > I was slightly confused. I looked again and yes regulator-name is always the
> > first regulator related property. What's off is that in some cases min/max
> > voltage comes before always-on/boot-on, and in others vice versa.
> >
> > For example in the Rock64 and ROC-RK3328-CC device trees, in the fixed
> > regulators, always-on/boot-on come before min/max voltage, while in the
> > PMIC the other order is used.
>
> That's likely undecidednes on my part ;-)
>
> There could be an argument for a "name, voltages, flags" sorting, but on
> the other hand just keeping it alphabetical with the naming on top
> creates less special cases.

And min before max? :D
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 622d320ddd13..62d3abc17a24 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -11,6 +11,7 @@  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-geekbox.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
new file mode 100644
index 000000000000..7818d2e8180c
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
@@ -0,0 +1,369 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * (C) Copyright 2020 Chen-Yu Tsai <wens@csie.org>
+ *
+ * Based on ./rk3328-rock64.dts, which is
+ *
+ * Copyright (c) 2017 PINE64
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pinctrl/rockchip.h>
+#include "rk3328.dtsi"
+
+/ {
+	model = "Radxa ROCK Pi E";
+	compatible = "radxa,rockpi-e", "rockchip,rk3328";
+
+	chosen {
+		stdout-path = "serial2:1500000n8";
+	};
+
+	gmac_clkin: external-gmac-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "gmac_clkin";
+		#clock-cells = <0>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-0 = <&led_pin>;
+		pinctrl-names = "default";
+
+		led-0 {
+			/* schematic say green but the actual thing is blue */
+			color = <LED_COLOR_ID_BLUE>;
+			gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
+			linux,default-trigger = "heartbeat";
+		};
+	};
+
+	vcc_sd: sdmmc-regulator {
+		compatible = "regulator-fixed";
+		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sdmmc0m1_pin>;
+		regulator-boot-on;
+		regulator-name = "vcc_sd";
+		vin-supply = <&vcc_io>;
+	};
+
+	vcc_host_5v: vcc-host-5v-regulator {
+		compatible = "regulator-fixed";
+		gpio = <&gpio3 RK_PA7 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb30_host_drv>;
+		enable-active-high;
+		regulator-name = "vcc_host_5v";
+		regulator-always-on;
+		regulator-boot-on;
+		vin-supply = <&vcc_sys>;
+	};
+
+	vcc_sys: vcc-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+	};
+
+	vcc_wifi: vcc-wifi-regulator {
+		compatible = "regulator-fixed";
+		gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&wifi_en>;
+		regulator-name = "vcc_wifi";
+		regulator-always-on;
+		regulator-boot-on;
+		vin-supply = <&vcc_io>;
+	};
+};
+
+&analog_sound {
+	status = "okay";
+};
+
+&codec {
+	status = "okay";
+};
+
+&cpu0 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu1 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu2 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu3 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&emmc {
+	bus-width = <8>;
+	cap-mmc-highspeed;
+	max-frequency = <150000000>;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	non-removable;
+	pinctrl-names = "default";
+	pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_bus8>;
+	vmmc-supply = <&vcc_io>;
+	vqmmc-supply = <&vcc18_emmc>;
+	status = "okay";
+};
+
+&gmac2io {
+	assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
+	assigned-clock-parents = <&gmac_clkin>, <&gmac_clkin>;
+	clock_in_out = "input";
+	phy-handle = <&rtl8211e>;
+	phy-mode = "rgmii";
+	phy-supply = <&vcc_io>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&rgmiim1_pins>;
+	snps,aal;
+	snps,rxpbl = <0x4>;
+	snps,txpbl = <0x4>;
+	tx_delay = <0x26>;
+	rx_delay = <0x11>;
+	status = "okay";
+
+	mdio {
+		compatible = "snps,dwmac-mdio";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		rtl8211e: ethernet-phy@1 {
+			reg = <1>;
+			pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
+			pinctrl-names = "default";
+			interrupt-parent = <&gpio1>;
+			interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
+			reset-assert-us = <10000>;
+			reset-deassert-us = <50000>;
+			reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
+		};
+	};
+};
+
+&gmac2phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&fephyled_linkm1>, <&fephyled_rxm1>;
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+
+	rk805: pmic@18 {
+		compatible = "rockchip,rk805";
+		reg = <0x18>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
+		#clock-cells = <1>;
+		clock-output-names = "xin32k", "rk805-clkout2";
+		gpio-controller;
+		#gpio-cells = <2>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		rockchip,system-power-controller;
+		wakeup-source;
+
+		vcc1-supply = <&vcc_sys>;
+		vcc2-supply = <&vcc_sys>;
+		vcc3-supply = <&vcc_sys>;
+		vcc4-supply = <&vcc_sys>;
+		vcc5-supply = <&vcc_io>;
+		vcc6-supply = <&vcc_sys>;
+
+		regulators {
+			vdd_log: DCDC_REG1 {
+				regulator-name = "vdd_log";
+				regulator-min-microvolt = <712500>;
+				regulator-max-microvolt = <1450000>;
+				regulator-ramp-delay = <12500>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+
+			vdd_arm: DCDC_REG2 {
+				regulator-name = "vdd_arm";
+				regulator-min-microvolt = <712500>;
+				regulator-max-microvolt = <1450000>;
+				regulator-ramp-delay = <12500>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <950000>;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-name = "vcc_ddr";
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_io: DCDC_REG4 {
+				regulator-name = "vcc_io";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3300000>;
+				};
+			};
+
+			vcc_18: LDO_REG1 {
+				regulator-name = "vcc_18";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc18_emmc: LDO_REG2 {
+				regulator-name = "vcc18_emmc";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vdd_10: LDO_REG3 {
+				regulator-name = "vdd_10";
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1000000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+		};
+	};
+};
+
+&i2s1 {
+	status = "okay";
+};
+
+&io_domains {
+	pmuio-supply = <&vcc_io>;
+	vccio1-supply = <&vcc_io>;
+	vccio2-supply = <&vcc18_emmc>;
+	vccio3-supply = <&vcc_io>;
+	vccio4-supply = <&vcc_io>;
+	vccio5-supply = <&vcc_io>;
+	vccio6-supply = <&vcc_io>;
+	status = "okay";
+};
+
+&pinctrl {
+	ethernet-phy {
+		eth_phy_int_pin: eth-phy-int-pin {
+			rockchip,pins = <1 RK_PD0 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		eth_phy_reset_pin: eth-phy-reset-pin {
+			rockchip,pins = <1 RK_PC2 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+
+	leds {
+		led_pin: led-pin {
+			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	pmic {
+		pmic_int_l: pmic-int-l {
+			rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+	usb3 {
+		usb30_host_drv: usb30-host-drv {
+			rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	wifi {
+		wifi_en: wifi-en {
+			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+};
+
+&sdmmc {
+	bus-width = <4>;
+	cap-mmc-highspeed;
+	cap-sd-highspeed;
+	disable-wp;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdmmc0_clk>, <&sdmmc0_cmd>, <&sdmmc0_dectn>, <&sdmmc0_bus4>;
+	vmmc-supply = <&vcc_sd>;
+	status = "okay";
+};
+
+&saradc {
+	vref-supply = <&vcc_18>;
+	status = "okay";
+};
+
+&tsadc {
+	status = "okay";
+};
+
+&u2phy {
+	status = "okay";
+};
+
+&u2phy_host {
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
+
+&usb_host0_ehci {
+	status = "okay";
+};