2021-03-13 03:39:40

by Tianling Shen

[permalink] [raw]
Subject: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

This adds support for the NanoPi R4S from FriendlyArm.

Rockchip RK3399 SoC
1GB DDR3 or 4GB LPDDR4 RAM
Gigabit Ethernet (WAN)
Gigabit Ethernet (PCIe) (LAN)
USB 3.0 Port x 2
MicroSD slot
Reset button
WAN - LAN - SYS LED

[initial DTS file]
Co-developed-by: Jensen Huang <[email protected]>
Signed-off-by: Jensen Huang <[email protected]>
[minor adjustments]
Co-developed-by: Marty Jones <[email protected]>
Signed-off-by: Marty Jones <[email protected]>
[fixed format issues]
Signed-off-by: Tianling Shen <[email protected]>

Reported-by: kernel test robot <[email protected]>
---
arch/arm64/boot/dts/rockchip/Makefile | 1 +
.../boot/dts/rockchip/rk3399-nanopi-r4s.dts | 179 ++++++++++++++++++
2 files changed, 180 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 62d3abc17a24..c3e00c0e2db7 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
new file mode 100644
index 000000000000..41b3d5c5043c
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * FriendlyElec NanoPC-T4 board device tree source
+ *
+ * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ *
+ * Copyright (c) 2020 Jensen Huang <[email protected]>
+ * Copyright (c) 2020 Marty Jones <[email protected]>
+ * Copyright (c) 2021 Tianling Shen <[email protected]>
+ */
+
+/dts-v1/;
+#include "rk3399-nanopi4.dtsi"
+
+/ {
+ model = "FriendlyElec NanoPi R4S";
+ compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
+
+ /delete-node/ gpio-leds;
+ gpio-leds {
+ compatible = "gpio-leds";
+ pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
+ pinctrl-names = "default";
+
+ lan_led: led-0 {
+ gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
+ label = "nanopi-r4s:green:lan";
+ };
+
+ sys_led: led-1 {
+ gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
+ label = "nanopi-r4s:red:sys";
+ default-state = "on";
+ };
+
+ wan_led: led-2 {
+ gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
+ label = "nanopi-r4s:green:wan";
+ };
+ };
+
+ /delete-node/ gpio-keys;
+ gpio-keys {
+ compatible = "gpio-keys";
+ pinctrl-names = "default";
+ pinctrl-0 = <&reset_button_pin>;
+
+ reset {
+ debounce-interval = <50>;
+ gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
+ label = "reset";
+ linux,code = <KEY_RESTART>;
+ };
+ };
+
+ vdd_5v: vdd-5v {
+ compatible = "regulator-fixed";
+ regulator-name = "vdd_5v";
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ fan: pwm-fan {
+ compatible = "pwm-fan";
+ /*
+ * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
+ * work out to 0, ~1200, ~3000, and 5000RPM respectively.
+ */
+ cooling-levels = <0 12 18 255>;
+ #cooling-cells = <2>;
+ fan-supply = <&vdd_5v>;
+ pwms = <&pwm1 0 50000 0>;
+ };
+};
+
+&cpu_thermal {
+ trips {
+ cpu_warm: cpu_warm {
+ temperature = <55000>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+
+ cpu_hot: cpu_hot {
+ temperature = <65000>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ map2 {
+ trip = <&cpu_warm>;
+ cooling-device = <&fan THERMAL_NO_LIMIT 1>;
+ };
+
+ map3 {
+ trip = <&cpu_hot>;
+ cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
+ };
+ };
+};
+
+&emmc_phy {
+ status = "disabled";
+};
+
+&fusb0 {
+ status = "disabled";
+};
+
+&pcie0 {
+ max-link-speed = <1>;
+ num-lanes = <1>;
+ vpcie3v3-supply = <&vcc3v3_sys>;
+
+ pcie@0 {
+ reg = <0x00000000 0 0 0 0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ };
+};
+
+&pinctrl {
+ /delete-node/ gpio-leds;
+ gpio-leds {
+ lan_led_pin: lan-led-pin {
+ rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
+
+ sys_led_pin: sys-led-pin {
+ rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
+
+ wan_led_pin: wan-led-pin {
+ rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
+ };
+
+ /delete-node/ rockchip-key;
+ rockchip-key {
+ reset_button_pin: reset-button-pin {
+ rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
+ };
+ };
+};
+
+&sdhci {
+ status = "disabled";
+};
+
+&sdio0 {
+ status = "disabled";
+};
+
+&sdmmc {
+ sd-uhs-sdr12;
+ sd-uhs-sdr25;
+ sd-uhs-sdr50;
+};
+
+&u2phy0_host {
+ phy-supply = <&vdd_5v>;
+};
+
+&u2phy1_host {
+ status = "disabled";
+};
+
+&usbdrd_dwc3_0 {
+ dr_mode = "host";
+};
+
+&vcc3v3_sys {
+ vin-supply = <&vcc5v0_sys>;
+};
--
2.17.1


2021-03-13 11:58:54

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

On 2021-03-13 03:25, Tianling Shen wrote:
> This adds support for the NanoPi R4S from FriendlyArm.
>
> Rockchip RK3399 SoC
> 1GB DDR3 or 4GB LPDDR4 RAM
> Gigabit Ethernet (WAN)
> Gigabit Ethernet (PCIe) (LAN)
> USB 3.0 Port x 2
> MicroSD slot
> Reset button
> WAN - LAN - SYS LED
>
> [initial DTS file]
> Co-developed-by: Jensen Huang <[email protected]>
> Signed-off-by: Jensen Huang <[email protected]>
> [minor adjustments]
> Co-developed-by: Marty Jones <[email protected]>
> Signed-off-by: Marty Jones <[email protected]>
> [fixed format issues]
> Signed-off-by: Tianling Shen <[email protected]>
>
> Reported-by: kernel test robot <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/Makefile | 1 +
> .../boot/dts/rockchip/rk3399-nanopi-r4s.dts | 179 ++++++++++++++++++
> 2 files changed, 180 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
>
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 62d3abc17a24..c3e00c0e2db7 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> new file mode 100644
> index 000000000000..41b3d5c5043c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * FriendlyElec NanoPC-T4 board device tree source
> + *
> + * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
> + * (http://www.friendlyarm.com)
> + *
> + * Copyright (c) 2018 Collabora Ltd.
> + *
> + * Copyright (c) 2020 Jensen Huang <[email protected]>
> + * Copyright (c) 2020 Marty Jones <[email protected]>
> + * Copyright (c) 2021 Tianling Shen <[email protected]>
> + */
> +
> +/dts-v1/;
> +#include "rk3399-nanopi4.dtsi"
> +
> +/ {
> + model = "FriendlyElec NanoPi R4S";
> + compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
> +
> + /delete-node/ gpio-leds;

Why? You could justify deleting &status_led, but redefining the whole
node from scratch seems unnecessary.

> + gpio-leds {
> + compatible = "gpio-leds";
> + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> + pinctrl-names = "default";
> +
> + lan_led: led-0 {
> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> + label = "nanopi-r4s:green:lan";
> + };
> +
> + sys_led: led-1 {
> + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> + label = "nanopi-r4s:red:sys";
> + default-state = "on";
> + };
> +
> + wan_led: led-2 {
> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> + label = "nanopi-r4s:green:wan";
> + };
> + };
> +
> + /delete-node/ gpio-keys;

Ditto - just removing the power key node itself should suffice.

> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-names = "default";
> + pinctrl-0 = <&reset_button_pin>;
> +
> + reset {
> + debounce-interval = <50>;
> + gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
> + label = "reset";
> + linux,code = <KEY_RESTART>;
> + };
> + };
> +
> + vdd_5v: vdd-5v {
> + compatible = "regulator-fixed";
> + regulator-name = "vdd_5v";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + fan: pwm-fan {
> + compatible = "pwm-fan";
> + /*
> + * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
> + * work out to 0, ~1200, ~3000, and 5000RPM respectively.
> + */
> + cooling-levels = <0 12 18 255>;

This is clearly not true - those numbers refer to a 12V fan on my
NanoPC-T4's 12V PWM circuit, while the output circuit here is 5V. If you
really want a placeholder here maybe just use <0 255>, or figure out
some empirical values with a suitable 5V fan that are actually meaningful.

> + #cooling-cells = <2>;
> + fan-supply = <&vdd_5v>;
> + pwms = <&pwm1 0 50000 0>;
> + };
> +};
> +
> +&cpu_thermal {
> + trips {
> + cpu_warm: cpu_warm {
> + temperature = <55000>;
> + hysteresis = <2000>;
> + type = "active";
> + };
> +
> + cpu_hot: cpu_hot {
> + temperature = <65000>;
> + hysteresis = <2000>;
> + type = "active";
> + };
> + };
> +
> + cooling-maps {
> + map2 {
> + trip = <&cpu_warm>;
> + cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> + };
> +
> + map3 {
> + trip = <&cpu_hot>;
> + cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> + };
> + };
> +};
> +
> +&emmc_phy {
> + status = "disabled";
> +};
> +
> +&fusb0 {
> + status = "disabled";

This can never be enabled since it doesn't exist in the design at all,
so it's one place where deletion *would* make good sense. AFAICS this
means you also don't need i2c4 enabled either.

> +};

It might be nice to disable HDMI and all the other display pieces given
that the board is physically headless.

> +
> +&pcie0 {
> + max-link-speed = <1>;
> + num-lanes = <1>;
> + vpcie3v3-supply = <&vcc3v3_sys>;
> +
> + pcie@0 {
> + reg = <0x00000000 0 0 0 0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + };

What's this for?

> +};
> +
> +&pinctrl {
> + /delete-node/ gpio-leds;

Again, at most you'd only need to delete &status_led_pin.

> + gpio-leds {
> + lan_led_pin: lan-led-pin {
> + rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> +
> + sys_led_pin: sys-led-pin {
> + rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> +
> + wan_led_pin: wan-led-pin {
> + rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> +
> + /delete-node/ rockchip-key;

Ditto for &power_key.

> + rockchip-key {
> + reset_button_pin: reset-button-pin {
> + rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
> + };
> + };
> +};
> +
> +&sdhci {
> + status = "disabled";
> +};
> +
> +&sdio0 {
> + status = "disabled";
> +};
> +
> +&sdmmc {
> + sd-uhs-sdr12;
> + sd-uhs-sdr25;
> + sd-uhs-sdr50;

Are those modes unique to this particular board?

> +};
> +

What about the Bluetooth stuff on uart0?

Robin.

> +&u2phy0_host {
> + phy-supply = <&vdd_5v>;
> +};
> +
> +&u2phy1_host {
> + status = "disabled";
> +};
> +
> +&usbdrd_dwc3_0 {
> + dr_mode = "host";
> +};
> +
> +&vcc3v3_sys {
> + vin-supply = <&vcc5v0_sys>;
> +};
>

2021-03-13 13:24:05

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
>
> On 2021-03-13 03:25, Tianling Shen wrote:
> > This adds support for the NanoPi R4S from FriendlyArm.
> >
> > Rockchip RK3399 SoC
> > 1GB DDR3 or 4GB LPDDR4 RAM
> > Gigabit Ethernet (WAN)
> > Gigabit Ethernet (PCIe) (LAN)
> > USB 3.0 Port x 2
> > MicroSD slot
> > Reset button
> > WAN - LAN - SYS LED
> >
> > [initial DTS file]
> > Co-developed-by: Jensen Huang <[email protected]>
> > Signed-off-by: Jensen Huang <[email protected]>
> > [minor adjustments]
> > Co-developed-by: Marty Jones <[email protected]>
> > Signed-off-by: Marty Jones <[email protected]>
> > [fixed format issues]
> > Signed-off-by: Tianling Shen <[email protected]>
> >
> > Reported-by: kernel test robot <[email protected]>
> > ---
> > arch/arm64/boot/dts/rockchip/Makefile | 1 +
> > .../boot/dts/rockchip/rk3399-nanopi-r4s.dts | 179 ++++++++++++++++++
> > 2 files changed, 180 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > index 62d3abc17a24..c3e00c0e2db7 100644
> > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > @@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
> > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> > new file mode 100644
> > index 000000000000..41b3d5c5043c
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * FriendlyElec NanoPC-T4 board device tree source
> > + *
> > + * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
> > + * (http://www.friendlyarm.com)
> > + *
> > + * Copyright (c) 2018 Collabora Ltd.
> > + *
> > + * Copyright (c) 2020 Jensen Huang <[email protected]>
> > + * Copyright (c) 2020 Marty Jones <[email protected]>
> > + * Copyright (c) 2021 Tianling Shen <[email protected]>
> > + */
> > +
> > +/dts-v1/;
> > +#include "rk3399-nanopi4.dtsi"
> > +
> > +/ {
> > + model = "FriendlyElec NanoPi R4S";
> > + compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
> > +
> > + /delete-node/ gpio-leds;
>
> Why? You could justify deleting &status_led, but redefining the whole
> node from scratch seems unnecessary.

First of all, thank you for reviewing, and sorry for my poor English.

I need to redefine `pinctrl-0`, but if I use `/delete-property/
pinctrl-0;`, it will throw an error,
so maybe I made a mistake? And I will try again...
>
> > + gpio-leds {
> > + compatible = "gpio-leds";
> > + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> > + pinctrl-names = "default";
> > +
> > + lan_led: led-0 {
> > + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> > + label = "nanopi-r4s:green:lan";
> > + };
> > +
> > + sys_led: led-1 {
> > + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> > + label = "nanopi-r4s:red:sys";
> > + default-state = "on";
> > + };
> > +
> > + wan_led: led-2 {
> > + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> > + label = "nanopi-r4s:green:wan";
> > + };
> > + };
> > +
> > + /delete-node/ gpio-keys;
>
> Ditto - just removing the power key node itself should suffice.

Just like gpio-leds.
>
> > + gpio-keys {
> > + compatible = "gpio-keys";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&reset_button_pin>;
> > +
> > + reset {
> > + debounce-interval = <50>;
> > + gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
> > + label = "reset";
> > + linux,code = <KEY_RESTART>;
> > + };
> > + };
> > +
> > + vdd_5v: vdd-5v {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vdd_5v";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > +
> > + fan: pwm-fan {
> > + compatible = "pwm-fan";
> > + /*
> > + * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
> > + * work out to 0, ~1200, ~3000, and 5000RPM respectively.
> > + */
> > + cooling-levels = <0 12 18 255>;
>
> This is clearly not true - those numbers refer to a 12V fan on my
> NanoPC-T4's 12V PWM circuit, while the output circuit here is 5V. If you
> really want a placeholder here maybe just use <0 255>, or figure out
> some empirical values with a suitable 5V fan that are actually meaningful.

Okay... I'll drop these as they're not really meaningful.
>
> > + #cooling-cells = <2>;
> > + fan-supply = <&vdd_5v>;
> > + pwms = <&pwm1 0 50000 0>;
> > + };
> > +};
> > +
> > +&cpu_thermal {
> > + trips {
> > + cpu_warm: cpu_warm {
> > + temperature = <55000>;
> > + hysteresis = <2000>;
> > + type = "active";
> > + };
> > +
> > + cpu_hot: cpu_hot {
> > + temperature = <65000>;
> > + hysteresis = <2000>;
> > + type = "active";
> > + };
> > + };
> > +
> > + cooling-maps {
> > + map2 {
> > + trip = <&cpu_warm>;
> > + cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> > + };
> > +
> > + map3 {
> > + trip = <&cpu_hot>;
> > + cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> > + };
> > + };
> > +};
> > +
> > +&emmc_phy {
> > + status = "disabled";
> > +};
> > +
> > +&fusb0 {
> > + status = "disabled";
>
> This can never be enabled since it doesn't exist in the design at all,
> so it's one place where deletion *would* make good sense. AFAICS this
> means you also don't need i2c4 enabled either.

Is it fine to disable i2c4 directly?
>
> > +};
>
> It might be nice to disable HDMI and all the other display pieces given
> that the board is physically headless.

Fine, I will delete `display-subsystem` node.
>
> > +
> > +&pcie0 {
> > + max-link-speed = <1>;
> > + num-lanes = <1>;
> > + vpcie3v3-supply = <&vcc3v3_sys>;
> > +
> > + pcie@0 {
> > + reg = <0x00000000 0 0 0 0>;
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + };
>
> What's this for?

This is for the on-board PCIe ethernet adapter (RTL8111h).
>
> > +};
> > +
> > +&pinctrl {
> > + /delete-node/ gpio-leds;
>
> Again, at most you'd only need to delete &status_led_pin.

Yes, I will do it.
>
> > + gpio-leds {
> > + lan_led_pin: lan-led-pin {
> > + rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> > + };
> > +
> > + sys_led_pin: sys-led-pin {
> > + rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> > + };
> > +
> > + wan_led_pin: wan-led-pin {
> > + rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> > + };
> > + };
> > +
> > + /delete-node/ rockchip-key;
>
> Ditto for &power_key.

Yes.
>
> > + rockchip-key {
> > + reset_button_pin: reset-button-pin {
> > + rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
> > + };
> > + };
> > +};
> > +
> > +&sdhci {
> > + status = "disabled";
> > +};
> > +
> > +&sdio0 {
> > + status = "disabled";
> > +};
> > +
> > +&sdmmc {
> > + sd-uhs-sdr12;
> > + sd-uhs-sdr25;
> > + sd-uhs-sdr50;
>
> Are those modes unique to this particular board?

These seem not right and I will drop them.
>
> > +};
> > +
>
> What about the Bluetooth stuff on uart0?

R4S doesn't have it, so I guess I should disable uart0, like i2c4.
>
> Robin.
>
> > +&u2phy0_host {
> > + phy-supply = <&vdd_5v>;
> > +};
> > +
> > +&u2phy1_host {
> > + status = "disabled";
> > +};
> > +
> > +&usbdrd_dwc3_0 {
> > + dr_mode = "host";
> > +};
> > +
> > +&vcc3v3_sys {
> > + vin-supply = <&vcc5v0_sys>;
> > +};
> >

Tianling.

2021-03-15 16:34:08

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

On 2021-03-13 13:22, CN_SZTL wrote:
> Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
>>
>> On 2021-03-13 03:25, Tianling Shen wrote:
>>> This adds support for the NanoPi R4S from FriendlyArm.
>>>
>>> Rockchip RK3399 SoC
>>> 1GB DDR3 or 4GB LPDDR4 RAM
>>> Gigabit Ethernet (WAN)
>>> Gigabit Ethernet (PCIe) (LAN)
>>> USB 3.0 Port x 2
>>> MicroSD slot
>>> Reset button
>>> WAN - LAN - SYS LED
>>>
>>> [initial DTS file]
>>> Co-developed-by: Jensen Huang <[email protected]>
>>> Signed-off-by: Jensen Huang <[email protected]>
>>> [minor adjustments]
>>> Co-developed-by: Marty Jones <[email protected]>
>>> Signed-off-by: Marty Jones <[email protected]>
>>> [fixed format issues]
>>> Signed-off-by: Tianling Shen <[email protected]>
>>>
>>> Reported-by: kernel test robot <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/rockchip/Makefile | 1 +
>>> .../boot/dts/rockchip/rk3399-nanopi-r4s.dts | 179 ++++++++++++++++++
>>> 2 files changed, 180 insertions(+)
>>> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
>>> index 62d3abc17a24..c3e00c0e2db7 100644
>>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>>> @@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
>>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
>>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
>>> new file mode 100644
>>> index 000000000000..41b3d5c5043c
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
>>> @@ -0,0 +1,179 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * FriendlyElec NanoPC-T4 board device tree source
>>> + *
>>> + * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
>>> + * (http://www.friendlyarm.com)
>>> + *
>>> + * Copyright (c) 2018 Collabora Ltd.
>>> + *
>>> + * Copyright (c) 2020 Jensen Huang <[email protected]>
>>> + * Copyright (c) 2020 Marty Jones <[email protected]>
>>> + * Copyright (c) 2021 Tianling Shen <[email protected]>
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "rk3399-nanopi4.dtsi"
>>> +
>>> +/ {
>>> + model = "FriendlyElec NanoPi R4S";
>>> + compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
>>> +
>>> + /delete-node/ gpio-leds;
>>
>> Why? You could justify deleting &status_led, but redefining the whole
>> node from scratch seems unnecessary.
>
> First of all, thank you for reviewing, and sorry for my poor English.
>
> I need to redefine `pinctrl-0`, but if I use `/delete-property/
> pinctrl-0;`, it will throw an error,
> so maybe I made a mistake? And I will try again...

You don't need to delete the property itself though - simply specifying
it replaces whatever previous value was inherited from the DTSI. Think
about how all those "status = ..." lines work, for example.

Similarly, given that you're redefining the led-0 node anyway you
wouldn't really *need* to delete that either; doing so just avoids the
extra &status_led label hanging around if the DTB is built with symbols,
and saves having to explicitly override/delete the default trigger
property if necessary.

>>> + gpio-leds {
>>> + compatible = "gpio-leds";
>>> + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
>>> + pinctrl-names = "default";
>>> +
>>> + lan_led: led-0 {
>>> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
>>> + label = "nanopi-r4s:green:lan";
>>> + };
>>> +
>>> + sys_led: led-1 {
>>> + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
>>> + label = "nanopi-r4s:red:sys";
>>> + default-state = "on";
>>> + };
>>> +
>>> + wan_led: led-2 {
>>> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
>>> + label = "nanopi-r4s:green:wan";
>>> + };

Nit: (apologies for overlooking it before) there isn't an obvious
definitive order for the LEDs, but the order here is certainly not
consistent with anything. The most logical would probably be sys, wan,
lan since that's both in order of GPIO number and how they are
physically positioned relative to each other on the board/case (although
you could also argue for wan, lan, sys in that regard, depending on how
you look at it).

>>> + };
>>> +
>>> + /delete-node/ gpio-keys;
>>
>> Ditto - just removing the power key node itself should suffice.
>
> Just like gpio-leds.
>>
>>> + gpio-keys {
>>> + compatible = "gpio-keys";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&reset_button_pin>;
>>> +
>>> + reset {
>>> + debounce-interval = <50>;
>>> + gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
>>> + label = "reset";
>>> + linux,code = <KEY_RESTART>;
>>> + };
>>> + };
>>> +
>>> + vdd_5v: vdd-5v {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "vdd_5v";
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + };
>>> +
>>> + fan: pwm-fan {
>>> + compatible = "pwm-fan";
>>> + /*
>>> + * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
>>> + * work out to 0, ~1200, ~3000, and 5000RPM respectively.
>>> + */
>>> + cooling-levels = <0 12 18 255>;
>>
>> This is clearly not true - those numbers refer to a 12V fan on my
>> NanoPC-T4's 12V PWM circuit, while the output circuit here is 5V. If you
>> really want a placeholder here maybe just use <0 255>, or figure out
>> some empirical values with a suitable 5V fan that are actually meaningful.
>
> Okay... I'll drop these as they're not really meaningful.
>>
>>> + #cooling-cells = <2>;
>>> + fan-supply = <&vdd_5v>;
>>> + pwms = <&pwm1 0 50000 0>;
>>> + };
>>> +};
>>> +
>>> +&cpu_thermal {
>>> + trips {
>>> + cpu_warm: cpu_warm {
>>> + temperature = <55000>;
>>> + hysteresis = <2000>;
>>> + type = "active";
>>> + };
>>> +
>>> + cpu_hot: cpu_hot {
>>> + temperature = <65000>;
>>> + hysteresis = <2000>;
>>> + type = "active";
>>> + };
>>> + };
>>> +
>>> + cooling-maps {
>>> + map2 {
>>> + trip = <&cpu_warm>;
>>> + cooling-device = <&fan THERMAL_NO_LIMIT 1>;
>>> + };
>>> +
>>> + map3 {
>>> + trip = <&cpu_hot>;
>>> + cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
>>> + };
>>> + };
>>> +};
>>> +
>>> +&emmc_phy {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&fusb0 {
>>> + status = "disabled";
>>
>> This can never be enabled since it doesn't exist in the design at all,
>> so it's one place where deletion *would* make good sense. AFAICS this
>> means you also don't need i2c4 enabled either.
>
> Is it fine to disable i2c4 directly?

I think it would make sense, since it's not physically available short
of trying to solder on to the 0201 pull-up resistors.

>>
>>> +};
>>
>> It might be nice to disable HDMI and all the other display pieces given
>> that the board is physically headless.
>
> Fine, I will delete `display-subsystem` node.
>>
>>> +
>>> +&pcie0 {
>>> + max-link-speed = <1>;
>>> + num-lanes = <1>;
>>> + vpcie3v3-supply = <&vcc3v3_sys>;
>>> +
>>> + pcie@0 {
>>> + reg = <0x00000000 0 0 0 0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + };
>>
>> What's this for?
>
> This is for the on-board PCIe ethernet adapter (RTL8111h).

OK, but *how* exactly does the ethernet adapter need an empty DT node
describing the root port?

>>
>>> +};
>>> +
>>> +&pinctrl {
>>> + /delete-node/ gpio-leds;
>>
>> Again, at most you'd only need to delete &status_led_pin.
>
> Yes, I will do it.
>>
>>> + gpio-leds {
>>> + lan_led_pin: lan-led-pin {
>>> + rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + };
>>> +
>>> + sys_led_pin: sys-led-pin {
>>> + rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + };
>>> +
>>> + wan_led_pin: wan-led-pin {
>>> + rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + };
>>> + };
>>> +
>>> + /delete-node/ rockchip-key;
>>
>> Ditto for &power_key.
>
> Yes.
>>
>>> + rockchip-key {
>>> + reset_button_pin: reset-button-pin {
>>> + rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
>>> + };
>>> + };
>>> +};
>>> +
>>> +&sdhci {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&sdio0 {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&sdmmc {
>>> + sd-uhs-sdr12;
>>> + sd-uhs-sdr25;
>>> + sd-uhs-sdr50;
>>
>> Are those modes unique to this particular board?
>
> These seem not right and I will drop them.

I mean that if the other boards already support SDR104, they can
presumably support slower modes as well, so if these are worth having at
all then they could probably go in the common DTSI.

>>
>>> +};
>>> +
>>
>> What about the Bluetooth stuff on uart0?
>
> R4S doesn't have it, so I guess I should disable uart0, like i2c4.

Yes, the UART itself isn't available on the board, and either way you
certainly don't want the kernel wasting time and possibly throwing
errors trying to probe a non-existent device through it.

Thanks,
Robin.

2021-03-15 18:38:06

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi Robin,

On Mon, Mar 15, 2021 at 5:32 PM Robin Murphy <[email protected]> wrote:
> On 2021-03-13 13:22, CN_SZTL wrote:
> > Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
> >>
> >> On 2021-03-13 03:25, Tianling Shen wrote:
> >>> + gpio-leds {
> >>> + compatible = "gpio-leds";
> >>> + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> >>> + pinctrl-names = "default";
> >>> +
> >>> + lan_led: led-0 {
> >>> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> >>> + label = "nanopi-r4s:green:lan";
> >>> + };
> >>> +
> >>> + sys_led: led-1 {
> >>> + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> >>> + label = "nanopi-r4s:red:sys";
> >>> + default-state = "on";
> >>> + };
> >>> +
> >>> + wan_led: led-2 {
> >>> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> >>> + label = "nanopi-r4s:green:wan";
> >>> + };
>
> Nit: (apologies for overlooking it before) there isn't an obvious
> definitive order for the LEDs, but the order here is certainly not
> consistent with anything. The most logical would probably be sys, wan,

Looks like alphabetical sort order to me ;-)

> lan since that's both in order of GPIO number and how they are
> physically positioned relative to each other on the board/case (although
> you could also argue for wan, lan, sys in that regard, depending on how
> you look at it).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-03-15 18:38:39

by Heiko Stuebner

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Am Montag, 15. März 2021, 17:38:37 CET schrieb Geert Uytterhoeven:
> Hi Robin,
>
> On Mon, Mar 15, 2021 at 5:32 PM Robin Murphy <[email protected]> wrote:
> > On 2021-03-13 13:22, CN_SZTL wrote:
> > > Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
> > >>
> > >> On 2021-03-13 03:25, Tianling Shen wrote:
> > >>> + gpio-leds {
> > >>> + compatible = "gpio-leds";
> > >>> + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> > >>> + pinctrl-names = "default";
> > >>> +
> > >>> + lan_led: led-0 {
> > >>> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> > >>> + label = "nanopi-r4s:green:lan";
> > >>> + };
> > >>> +
> > >>> + sys_led: led-1 {
> > >>> + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> > >>> + label = "nanopi-r4s:red:sys";
> > >>> + default-state = "on";
> > >>> + };
> > >>> +
> > >>> + wan_led: led-2 {
> > >>> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> > >>> + label = "nanopi-r4s:green:wan";
> > >>> + };
> >
> > Nit: (apologies for overlooking it before) there isn't an obvious
> > definitive order for the LEDs, but the order here is certainly not
> > consistent with anything. The most logical would probably be sys, wan,
>
> Looks like alphabetical sort order to me ;-)

yep ... led-0, led-1, led-2 looks pretty sorted ;-)

Generally I'd prefer sorting by node-names ... especially as the phandle
is sort of optional for most things - and they sometimes come and go
in dt-files.


Heiko


2021-03-15 20:31:45

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi Heiko,

On Mon, Mar 15, 2021 at 5:52 PM Heiko Stübner <[email protected]> wrote:
> Am Montag, 15. März 2021, 17:38:37 CET schrieb Geert Uytterhoeven:
> > On Mon, Mar 15, 2021 at 5:32 PM Robin Murphy <[email protected]> wrote:
> > > On 2021-03-13 13:22, CN_SZTL wrote:
> > > > Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
> > > >>
> > > >> On 2021-03-13 03:25, Tianling Shen wrote:
> > > >>> + gpio-leds {
> > > >>> + compatible = "gpio-leds";
> > > >>> + pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> > > >>> + pinctrl-names = "default";
> > > >>> +
> > > >>> + lan_led: led-0 {
> > > >>> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> > > >>> + label = "nanopi-r4s:green:lan";
> > > >>> + };
> > > >>> +
> > > >>> + sys_led: led-1 {
> > > >>> + gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> > > >>> + label = "nanopi-r4s:red:sys";
> > > >>> + default-state = "on";
> > > >>> + };
> > > >>> +
> > > >>> + wan_led: led-2 {
> > > >>> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> > > >>> + label = "nanopi-r4s:green:wan";
> > > >>> + };
> > >
> > > Nit: (apologies for overlooking it before) there isn't an obvious
> > > definitive order for the LEDs, but the order here is certainly not
> > > consistent with anything. The most logical would probably be sys, wan,
> >
> > Looks like alphabetical sort order to me ;-)
>
> yep ... led-0, led-1, led-2 looks pretty sorted ;-)

Actually I had "lan, sys, wan" in mind ;-)

> Generally I'd prefer sorting by node-names ... especially as the phandle
> is sort of optional for most things - and they sometimes come and go
> in dt-files.

The node names are sorted, too, as you've just pointed out...

Personally, I'm not so fond of the <foo>-%u node names, and prefer
<foo>-<function>. With the former, it's way too easy to have a silent
override in your .dts(i) stack.
Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
Fix backlight regulator name")

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-03-16 16:25:28

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

On 2021-03-16 12:32, Robin Murphy <[email protected]> wrote:
>
> On 2021-03-13 13:22, CN_SZTL wrote:
> > Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
> >>
> >> On 2021-03-13 03:25, Tianling Shen wrote:
> >>> This adds support for the NanoPi R4S from FriendlyArm.
> >>>
> >>> Rockchip RK3399 SoC
> >>> 1GB DDR3 or 4GB LPDDR4 RAM
> >>> Gigabit Ethernet (WAN)
> >>> Gigabit Ethernet (PCIe) (LAN)
> >>> USB 3.0 Port x 2
> >>> MicroSD slot
> >>> Reset button
> >>> WAN - LAN - SYS LED
> >>>
> >>> [initial DTS file]
> >>> Co-developed-by: Jensen Huang <[email protected]>
> >>> Signed-off-by: Jensen Huang <[email protected]>
> >>> [minor adjustments]
> >>> Co-developed-by: Marty Jones <[email protected]>
> >>> Signed-off-by: Marty Jones <[email protected]>
> >>> [fixed format issues]
> >>> Signed-off-by: Tianling Shen <[email protected]>
> >>>
> >>> Reported-by: kernel test robot <[email protected]>
> >>> ---
> >>>    arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> >>>    .../boot/dts/rockchip/rk3399-nanopi-r4s.dts   | 179 ++++++++++++++++++
> >>>    2 files changed, 180 insertions(+)
> >>>    create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> >>> index 62d3abc17a24..c3e00c0e2db7 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/Makefile
> >>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> >>> @@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
> >>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>> new file mode 100644
> >>> index 000000000000..41b3d5c5043c
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>> @@ -0,0 +1,179 @@
> >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>> +/*
> >>> + * FriendlyElec NanoPC-T4 board device tree source
> >>> + *
> >>> + * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
> >>> + * (http://www.friendlyarm.com)
> >>> + *
> >>> + * Copyright (c) 2018 Collabora Ltd.
> >>> + *
> >>> + * Copyright (c) 2020 Jensen Huang <[email protected]>
> >>> + * Copyright (c) 2020 Marty Jones <[email protected]>
> >>> + * Copyright (c) 2021 Tianling Shen <[email protected]>
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "rk3399-nanopi4.dtsi"
> >>> +
> >>> +/ {
> >>> +     model = "FriendlyElec NanoPi R4S";
> >>> +     compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
> >>> +
> >>> +     /delete-node/ gpio-leds;
> >>
> >> Why? You could justify deleting &status_led, but redefining the whole
> >> node from scratch seems unnecessary.
> >
> > First of all, thank you for reviewing, and sorry for my poor English.
> >
> > I need to redefine `pinctrl-0`, but if I use `/delete-property/
> > pinctrl-0;`, it will throw an error,
> > so maybe I made a mistake? And I will try again...
>
> You don't need to delete the property itself though - simply specifying
> it replaces whatever previous value was inherited from the DTSI. Think
> about how all those "status = ..." lines work, for example.

I see, thank you so much!

>
> Similarly, given that you're redefining the led-0 node anyway you
> wouldn't really *need* to delete that either; doing so just avoids the
> extra &status_led label hanging around if the DTB is built with symbols,
> and saves having to explicitly override/delete the default trigger
> property if necessary.

I plan to take advice from Geert, rename them to `lan-led`, `sys-led`,`wan-led`, so deleting `led-0` might to be need here...>

> >>> +     gpio-leds {
> >>> +             compatible = "gpio-leds";
> >>> +             pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> >>> +             pinctrl-names = "default";
> >>> +
> >>> +             lan_led: led-0 {
> >>> +                     gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:green:lan";
> >>> +             };
> >>> +
> >>> +             sys_led: led-1 {
> >>> +                     gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:red:sys";
> >>> +                     default-state = "on";
> >>> +             };
> >>> +
> >>> +             wan_led: led-2 {
> >>> +                     gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:green:wan";
> >>> +             };
>
> Nit: (apologies for overlooking it before) there isn't an obvious
> definitive order for the LEDs, but the order here is certainly not
> consistent with anything. The most logical would probably be sys, wan,
> lan since that's both in order of GPIO number and how they are
> physically positioned relative to each other on the board/case (although
> you could also argue for wan, lan, sys in that regard, depending on how
> you look at it).
>
> >>> +     };
> >>> +
> >>> +     /delete-node/ gpio-keys;
> >>
> >> Ditto - just removing the power key node itself should suffice.
> >
> > Just like gpio-leds.
> >>
> >>> +     gpio-keys {
> >>> +             compatible = "gpio-keys";
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&reset_button_pin>;
> >>> +
> >>> +             reset {
> >>> +                     debounce-interval = <50>;
> >>> +                     gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
> >>> +                     label = "reset";
> >>> +                     linux,code = <KEY_RESTART>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     vdd_5v: vdd-5v {
> >>> +             compatible = "regulator-fixed";
> >>> +             regulator-name = "vdd_5v";
> >>> +             regulator-always-on;
> >>> +             regulator-boot-on;
> >>> +     };
> >>> +
> >>> +     fan: pwm-fan {
> >>> +             compatible = "pwm-fan";
> >>> +             /*
> >>> +              * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
> >>> +              * work out to 0, ~1200, ~3000, and 5000RPM respectively.
> >>> +              */
> >>> +             cooling-levels = <0 12 18 255>;
> >>
> >> This is clearly not true - those numbers refer to a 12V fan on my
> >> NanoPC-T4's 12V PWM circuit, while the output circuit here is 5V. If you
> >> really want a placeholder here maybe just use <0 255>, or figure out
> >> some empirical values with a suitable 5V fan that are actually meaningful.
> >
> > Okay... I'll drop these as they're not really meaningful.
> >>
> >>> +             #cooling-cells = <2>;
> >>> +             fan-supply = <&vdd_5v>;
> >>> +             pwms = <&pwm1 0 50000 0>;
> >>> +     };
> >>> +};
> >>> +
> >>> +&cpu_thermal {
> >>> +     trips {
> >>> +             cpu_warm: cpu_warm {
> >>> +                     temperature = <55000>;
> >>> +                     hysteresis = <2000>;
> >>> +                     type = "active";
> >>> +             };
> >>> +
> >>> +             cpu_hot: cpu_hot {
> >>> +                     temperature = <65000>;
> >>> +                     hysteresis = <2000>;
> >>> +                     type = "active";
> >>> +             };
> >>> +     };
> >>> +
> >>> +     cooling-maps {
> >>> +             map2 {
> >>> +                     trip = <&cpu_warm>;
> >>> +                     cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> >>> +             };
> >>> +
> >>> +             map3 {
> >>> +                     trip = <&cpu_hot>;
> >>> +                     cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> +
> >>> +&emmc_phy {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&fusb0 {
> >>> +     status = "disabled";
> >>
> >> This can never be enabled since it doesn't exist in the design at all,
> >> so it's one place where deletion *would* make good sense. AFAICS this
> >> means you also don't need i2c4 enabled either.
> >
> > Is it fine to disable i2c4 directly?
>
> I think it would make sense, since it's not physically available short
> of trying to solder on to the 0201 pull-up resistors.
>
> >>
> >>> +};
> >>
> >> It might be nice to disable HDMI and all the other display pieces given
> >> that the board is physically headless.
> >
> > Fine, I will delete `display-subsystem` node.
> >>
> >>> +
> >>> +&pcie0 {
> >>> +     max-link-speed = <1>;
> >>> +     num-lanes = <1>;
> >>> +     vpcie3v3-supply = <&vcc3v3_sys>;
> >>> +
> >>> +     pcie@0 {
> >>> +             reg = <0x00000000 0 0 0 0>;
> >>> +             #address-cells = <3>;
> >>> +             #size-cells = <2>;
> >>> +     };
> >>
> >> What's this for?
> >
> > This is for the on-board PCIe ethernet adapter (RTL8111h).
>
> OK, but *how* exactly does the ethernet adapter need an empty DT node
> describing the root port?

Actually I just took this from the vendor.
This seems useless, and I'll drop it.

>
> >>
> >>> +};
> >>> +
> >>> +&pinctrl {
> >>> +     /delete-node/ gpio-leds;
> >>
> >> Again, at most you'd only need to delete &status_led_pin.
> >
> > Yes, I will do it.
> >>
> >>> +     gpio-leds {
> >>> +             lan_led_pin: lan-led-pin {
> >>> +                     rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +
> >>> +             sys_led_pin: sys-led-pin {
> >>> +                     rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +
> >>> +             wan_led_pin: wan-led-pin {
> >>> +                     rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /delete-node/ rockchip-key;
> >>
> >> Ditto for &power_key.
> >
> > Yes.
> >>
> >>> +     rockchip-key {
> >>> +             reset_button_pin: reset-button-pin {
> >>> +                     rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> +
> >>> +&sdhci {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&sdio0 {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&sdmmc {
> >>> +     sd-uhs-sdr12;
> >>> +     sd-uhs-sdr25;
> >>> +     sd-uhs-sdr50;
> >>
> >> Are those modes unique to this particular board?
> >
> > These seem not right and I will drop them.
>
> I mean that if the other boards already support SDR104, they can
> presumably support slower modes as well, so if these are worth having at
> all then they could probably go in the common DTSI.

I'm not sure, just based on the dts of R2S, and I added them here.
However they should be general for all NanoPi4 boards.

>
> >>
> >>> +};
> >>> +
> >>
> >> What about the Bluetooth stuff on uart0?
> >
> > R4S doesn't have it, so I guess I should disable uart0, like i2c4.
>
> Yes, the UART itself isn't available on the board, and either way you
> certainly don't want the kernel wasting time and possibly throwing
> errors trying to probe a non-existent device through it.
>
> Thanks,
> Robin.

Thanks,
Tianling.

2021-03-16 16:29:28

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi Geert,

On 2021-03-16 02:23 Geert Uytterhoeven <[email protected]> wrote:
> Personally, I'm not so fond of the <foo>-%u node names, and prefer
> <foo>-<function>.  With the former, it's way too easy to have a silent
> override in your .dts(i) stack.
> Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
> Fix backlight regulator name")

How about using `lan-led`, `sys-led` and `wan-led` here?

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Thanks,
Tianling.

2021-03-16 20:34:48

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Robin Murphy <[email protected]> wrote:
>
> On 2021-03-13 13:22, CN_SZTL wrote:
> > Robin Murphy <[email protected]> 于2021年3月13日周六 下午7:55写道:
> >>
> >> On 2021-03-13 03:25, Tianling Shen wrote:
> >>> This adds support for the NanoPi R4S from FriendlyArm.
> >>>
> >>> Rockchip RK3399 SoC
> >>> 1GB DDR3 or 4GB LPDDR4 RAM
> >>> Gigabit Ethernet (WAN)
> >>> Gigabit Ethernet (PCIe) (LAN)
> >>> USB 3.0 Port x 2
> >>> MicroSD slot
> >>> Reset button
> >>> WAN - LAN - SYS LED
> >>>
> >>> [initial DTS file]
> >>> Co-developed-by: Jensen Huang <[email protected]>
> >>> Signed-off-by: Jensen Huang <[email protected]>
> >>> [minor adjustments]
> >>> Co-developed-by: Marty Jones <[email protected]>
> >>> Signed-off-by: Marty Jones <[email protected]>
> >>> [fixed format issues]
> >>> Signed-off-by: Tianling Shen <[email protected]>
> >>>
> >>> Reported-by: kernel test robot <[email protected]>
> >>> ---
> >>>    arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> >>>    .../boot/dts/rockchip/rk3399-nanopi-r4s.dts   | 179 ++++++++++++++++++
> >>>    2 files changed, 180 insertions(+)
> >>>    create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> >>> index 62d3abc17a24..c3e00c0e2db7 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/Makefile
> >>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> >>> @@ -36,6 +36,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
> >>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
> >>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>> new file mode 100644
> >>> index 000000000000..41b3d5c5043c
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> >>> @@ -0,0 +1,179 @@
> >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>> +/*
> >>> + * FriendlyElec NanoPC-T4 board device tree source
> >>> + *
> >>> + * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
> >>> + * (http://www.friendlyarm.com)
> >>> + *
> >>> + * Copyright (c) 2018 Collabora Ltd.
> >>> + *
> >>> + * Copyright (c) 2020 Jensen Huang <[email protected]>
> >>> + * Copyright (c) 2020 Marty Jones <[email protected]>
> >>> + * Copyright (c) 2021 Tianling Shen <[email protected]>
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "rk3399-nanopi4.dtsi"
> >>> +
> >>> +/ {
> >>> +     model = "FriendlyElec NanoPi R4S";
> >>> +     compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
> >>> +
> >>> +     /delete-node/ gpio-leds;
> >>
> >> Why? You could justify deleting &status_led, but redefining the whole
> >> node from scratch seems unnecessary.
> >
> > First of all, thank you for reviewing, and sorry for my poor English.
> >
> > I need to redefine `pinctrl-0`, but if I use `/delete-property/
> > pinctrl-0;`, it will throw an error,
> > so maybe I made a mistake? And I will try again...
>
> You don't need to delete the property itself though - simply specifying
> it replaces whatever previous value was inherited from the DTSI. Think
> about how all those "status = ..." lines work, for example.

I see, thank you so much!

>
> Similarly, given that you're redefining the led-0 node anyway you
> wouldn't really *need* to delete that either; doing so just avoids the
> extra &status_led label hanging around if the DTB is built with symbols,
> and saves having to explicitly override/delete the default trigger
> property if necessary.

I plan to take advice from Geert, rename them to `lan-led`, `sys-led`,`wan-led`, so deleting `led-0` might to be need here...>

> >>> +     gpio-leds {
> >>> +             compatible = "gpio-leds";
> >>> +             pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> >>> +             pinctrl-names = "default";
> >>> +
> >>> +             lan_led: led-0 {
> >>> +                     gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:green:lan";
> >>> +             };
> >>> +
> >>> +             sys_led: led-1 {
> >>> +                     gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:red:sys";
> >>> +                     default-state = "on";
> >>> +             };
> >>> +
> >>> +             wan_led: led-2 {
> >>> +                     gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> >>> +                     label = "nanopi-r4s:green:wan";
> >>> +             };
>
> Nit: (apologies for overlooking it before) there isn't an obvious
> definitive order for the LEDs, but the order here is certainly not
> consistent with anything. The most logical would probably be sys, wan,
> lan since that's both in order of GPIO number and how they are
> physically positioned relative to each other on the board/case (although
> you could also argue for wan, lan, sys in that regard, depending on how
> you look at it).
>
> >>> +     };
> >>> +
> >>> +     /delete-node/ gpio-keys;
> >>
> >> Ditto - just removing the power key node itself should suffice.
> >
> > Just like gpio-leds.
> >>
> >>> +     gpio-keys {
> >>> +             compatible = "gpio-keys";
> >>> +             pinctrl-names = "default";
> >>> +             pinctrl-0 = <&reset_button_pin>;
> >>> +
> >>> +             reset {
> >>> +                     debounce-interval = <50>;
> >>> +                     gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
> >>> +                     label = "reset";
> >>> +                     linux,code = <KEY_RESTART>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     vdd_5v: vdd-5v {
> >>> +             compatible = "regulator-fixed";
> >>> +             regulator-name = "vdd_5v";
> >>> +             regulator-always-on;
> >>> +             regulator-boot-on;
> >>> +     };
> >>> +
> >>> +     fan: pwm-fan {
> >>> +             compatible = "pwm-fan";
> >>> +             /*
> >>> +              * With 20KHz PWM and an EVERCOOL EC4007H12SA fan, these levels
> >>> +              * work out to 0, ~1200, ~3000, and 5000RPM respectively.
> >>> +              */
> >>> +             cooling-levels = <0 12 18 255>;
> >>
> >> This is clearly not true - those numbers refer to a 12V fan on my
> >> NanoPC-T4's 12V PWM circuit, while the output circuit here is 5V. If you
> >> really want a placeholder here maybe just use <0 255>, or figure out
> >> some empirical values with a suitable 5V fan that are actually meaningful.
> >
> > Okay... I'll drop these as they're not really meaningful.
> >>
> >>> +             #cooling-cells = <2>;
> >>> +             fan-supply = <&vdd_5v>;
> >>> +             pwms = <&pwm1 0 50000 0>;
> >>> +     };
> >>> +};
> >>> +
> >>> +&cpu_thermal {
> >>> +     trips {
> >>> +             cpu_warm: cpu_warm {
> >>> +                     temperature = <55000>;
> >>> +                     hysteresis = <2000>;
> >>> +                     type = "active";
> >>> +             };
> >>> +
> >>> +             cpu_hot: cpu_hot {
> >>> +                     temperature = <65000>;
> >>> +                     hysteresis = <2000>;
> >>> +                     type = "active";
> >>> +             };
> >>> +     };
> >>> +
> >>> +     cooling-maps {
> >>> +             map2 {
> >>> +                     trip = <&cpu_warm>;
> >>> +                     cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> >>> +             };
> >>> +
> >>> +             map3 {
> >>> +                     trip = <&cpu_hot>;
> >>> +                     cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> +
> >>> +&emmc_phy {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&fusb0 {
> >>> +     status = "disabled";
> >>
> >> This can never be enabled since it doesn't exist in the design at all,
> >> so it's one place where deletion *would* make good sense. AFAICS this
> >> means you also don't need i2c4 enabled either.
> >
> > Is it fine to disable i2c4 directly?
>
> I think it would make sense, since it's not physically available short
> of trying to solder on to the 0201 pull-up resistors.
>
> >>
> >>> +};
> >>
> >> It might be nice to disable HDMI and all the other display pieces given
> >> that the board is physically headless.
> >
> > Fine, I will delete `display-subsystem` node.
> >>
> >>> +
> >>> +&pcie0 {
> >>> +     max-link-speed = <1>;
> >>> +     num-lanes = <1>;
> >>> +     vpcie3v3-supply = <&vcc3v3_sys>;
> >>> +
> >>> +     pcie@0 {
> >>> +             reg = <0x00000000 0 0 0 0>;
> >>> +             #address-cells = <3>;
> >>> +             #size-cells = <2>;
> >>> +     };
> >>
> >> What's this for?
> >
> > This is for the on-board PCIe ethernet adapter (RTL8111h).
>
> OK, but *how* exactly does the ethernet adapter need an empty DT node
> describing the root port?

Actually I just took this from the vendor.
This seems useless, and I'll drop it.

>
> >>
> >>> +};
> >>> +
> >>> +&pinctrl {
> >>> +     /delete-node/ gpio-leds;
> >>
> >> Again, at most you'd only need to delete &status_led_pin.
> >
> > Yes, I will do it.
> >>
> >>> +     gpio-leds {
> >>> +             lan_led_pin: lan-led-pin {
> >>> +                     rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +
> >>> +             sys_led_pin: sys-led-pin {
> >>> +                     rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +
> >>> +             wan_led_pin: wan-led-pin {
> >>> +                     rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> +             };
> >>> +     };
> >>> +
> >>> +     /delete-node/ rockchip-key;
> >>
> >> Ditto for &power_key.
> >
> > Yes.
> >>
> >>> +     rockchip-key {
> >>> +             reset_button_pin: reset-button-pin {
> >>> +                     rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
> >>> +             };
> >>> +     };
> >>> +};
> >>> +
> >>> +&sdhci {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&sdio0 {
> >>> +     status = "disabled";
> >>> +};
> >>> +
> >>> +&sdmmc {
> >>> +     sd-uhs-sdr12;
> >>> +     sd-uhs-sdr25;
> >>> +     sd-uhs-sdr50;
> >>
> >> Are those modes unique to this particular board?
> >
> > These seem not right and I will drop them.
>
> I mean that if the other boards already support SDR104, they can
> presumably support slower modes as well, so if these are worth having at
> all then they could probably go in the common DTSI.

I'm not sure, just based on the dts of R2S, and I added them here.
However they should be general for all NanoPi4 boards.

>
> >>
> >>> +};
> >>> +
> >>
> >> What about the Bluetooth stuff on uart0?
> >
> > R4S doesn't have it, so I guess I should disable uart0, like i2c4.
>
> Yes, the UART itself isn't available on the board, and either way you
> certainly don't want the kernel wasting time and possibly throwing
> errors trying to probe a non-existent device through it.
>
> Thanks,
> Robin.

Thanks,
Tianling.

2021-03-16 20:47:15

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi Tianling,

CC Jacek, Pavel

On Tue, Mar 16, 2021 at 4:00 PM Tianling Shen <[email protected]> wrote:
> On 2021-03-16 02:23 Geert Uytterhoeven <[email protected]> wrote:
> > Personally, I'm not so fond of the <foo>-%u node names, and prefer
> > <foo>-<function>. With the former, it's way too easy to have a silent
> > override in your .dts(i) stack.
> > Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
> > Fix backlight regulator name")
>
> How about using `lan-led`, `sys-led` and `wan-led` here?

Documentation/devicetree/bindings/leds/leds-gpio.yaml says "led-%u"
is the preferred form, but that anything containing "led" as a substring
is accepted. So I'd go for "led-lan" etc.

BTW, you can validate your DTB against the leds-gpio DT bindings
by running:

make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml

Background info for CCed parties:
https://lore.kernel.org/linux-arm-kernel/[email protected]/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-03-16 20:54:05

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

On 2021-03-16 11:35, Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Tianling,
>
> CC Jacek, Pavel
>
> On Tue, Mar 16, 2021 at 4:00 PM Tianling Shen <[email protected]> wrote:
> > On 2021-03-16 02:23 Geert Uytterhoeven <[email protected]> wrote:
> > > Personally, I'm not so fond of the <foo>-%u node names, and prefer
> > > <foo>-<function>. With the former, it's way too easy to have a silent
> > > override in your .dts(i) stack.
> > > Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
> > > Fix backlight regulator name")
> >
> > How about using `lan-led`, `sys-led` and `wan-led` here?
>
> Documentation/devicetree/bindings/leds/leds-gpio.yaml says "led-%u"
> is the preferred form, but that anything containing "led" as a substring
> is accepted. So I'd go for "led-lan" etc.
>
> BTW, you can validate your DTB against the leds-gpio DT bindings
> by running:
>
> make dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
>

Thank you so much!
I renamed them to `led-lan` etc., and the result of dtbs_check seems fine.

> Background info for CCed parties:
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

Tianling.

2021-03-16 21:23:12

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

On Tue 2021-03-16 16:34:50, Geert Uytterhoeven wrote:
> Hi Tianling,
>
> CC Jacek, Pavel
>
> On Tue, Mar 16, 2021 at 4:00 PM Tianling Shen <[email protected]> wrote:
> > On 2021-03-16 02:23 Geert Uytterhoeven <[email protected]> wrote:
> > > Personally, I'm not so fond of the <foo>-%u node names, and prefer
> > > <foo>-<function>. With the former, it's way too easy to have a silent
> > > override in your .dts(i) stack.
> > > Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
> > > Fix backlight regulator name")
> >
> > How about using `lan-led`, `sys-led` and `wan-led` here?
>
> Documentation/devicetree/bindings/leds/leds-gpio.yaml says "led-%u"
> is the preferred form, but that anything containing "led" as a substring
> is accepted. So I'd go for "led-lan" etc.
>
> BTW, you can validate your DTB against the leds-gpio DT bindings
> by running:
>
> make dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
>
> Background info for CCed parties:
>
https://lore.kernel.org/linux-arm-kernel/[email protected]/

I don't care much either way, lan-0 is okay as is lan-led.

but...

+ label = "nanopi-r4s:green:lan";
+ label = "nanopi-r4s:red:sys";
+ label = "nanopi-r4s:green:wan";


It would be good to have common labels, that means LED_FUNCTION_LAN,
LED_FUNCTION_WAN, and figuring out something better than "sys",
possibly LED_FUNCTION_FAULT?

Thanks,
Pavel

--
http://www.livejournal.com/~pavelmachek


Attachments:
(No filename) (1.52 kB)
signature.asc (201.00 B)
Download all attachments

2021-03-17 04:43:39

by Tianling Shen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi Pavel,

On 2021-03-17 03:38, Pavel Machek <[email protected]> wrote:
>
> On Tue 2021-03-16 16:34:50, Geert Uytterhoeven wrote:
> > Hi Tianling,
> >
> > CC Jacek, Pavel
> >
> > On Tue, Mar 16, 2021 at 4:00 PM Tianling Shen <[email protected]> wrote:
> > > On 2021-03-16 02:23 Geert Uytterhoeven <[email protected]> wrote:
> > > > Personally, I'm not so fond of the <foo>-%u node names, and prefer
> > > > <foo>-<function>. With the former, it's way too easy to have a silent
> > > > override in your .dts(i) stack.
> > > > Cfr. commit 45f5d5a9e34d3fe4 ("arm64: dts: renesas: r8a77995: draak:
> > > > Fix backlight regulator name")
> > >
> > > How about using `lan-led`, `sys-led` and `wan-led` here?
> >
> > Documentation/devicetree/bindings/leds/leds-gpio.yaml says "led-%u"
> > is the preferred form, but that anything containing "led" as a substring
> > is accepted. So I'd go for "led-lan" etc.
> >
> > BTW, you can validate your DTB against the leds-gpio DT bindings
> > by running:
> >
> > make dtbs_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
> >
> > Background info for CCed parties:
> >
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> I don't care much either way, lan-0 is okay as is lan-led.
>
> but...
>
> + label = "nanopi-r4s:green:lan";
> + label = "nanopi-r4s:red:sys";
> + label = "nanopi-r4s:green:wan";
>
>
> It would be good to have common labels, that means LED_FUNCTION_LAN,
> LED_FUNCTION_WAN, and figuring out something better than "sys",
> possibly LED_FUNCTION_FAULT?

LED_FUNCTION_POWER for "sys" would be fine, I think.

However, Documentation/leds/leds-class.rst says the form of naming is
"devicename:color:function",
and according to the given examples, as well as other dts(i), would it
be okay to use `green:lan`
etc. as the lable?

>
> Thanks,
> Pavel
>
> --
> http://www.livejournal.com/~pavelmachek

Thanks,
Tianling.

2021-04-25 21:29:39

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S

Hi!

> However, Documentation/leds/leds-class.rst says the form of naming is
> "devicename:color:function",
> and according to the given examples, as well as other dts(i), would it
> be okay to use `green:lan`
> etc. as the lable?

Yes, that sounds right.

Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek


Attachments:
(No filename) (343.00 B)
signature.asc (188.00 B)
Digital signature
Download all attachments