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

From: Robin Murphy
Date: Sat Mar 13 2021 - 06:56:30 EST


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 <jensenhuang@xxxxxxxxxxxxxxx>
Signed-off-by: Jensen Huang <jensenhuang@xxxxxxxxxxxxxxx>
[minor adjustments]
Co-developed-by: Marty Jones <mj8263788@xxxxxxxxx>
Signed-off-by: Marty Jones <mj8263788@xxxxxxxxx>
[fixed format issues]
Signed-off-by: Tianling Shen <cnsztl@xxxxxxxxx>

Reported-by: kernel test robot <lkp@xxxxxxxxx>
---
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 <jensenhuang@xxxxxxxxxxxxxxx>
+ * Copyright (c) 2020 Marty Jones <mj8263788@xxxxxxxxx>
+ * Copyright (c) 2021 Tianling Shen <cnsztl@xxxxxxxxx>
+ */
+
+/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>;
+};