On 2021-03-16 12:32, Robin Murphy <robin.murphy@xxxxxxx> wrote: > > On 2021-03-13 13:22, CN_SZTL wrote: > > Robin Murphy <robin.murphy@xxxxxxx> 于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 <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. > > > > 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.
