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

From: CN_SZTL <cnsztl@gmail.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: "Rob Herring" <robh+dt@kernel.org>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Jagan Teki" <jagan@amarulasolutions.com>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"David Bauer" <mail@david-bauer.net>,
	"Uwe Kleine-König" <uwe@kleine-koenig.org>,
	"Johan Jonker" <jbx6244@gmail.com>,
	"Michael Trimarchi" <michael@amarulasolutions.com>,
	"Marty Jones" <mj8263788@gmail.com>,
	"Jensen Huang" <jensenhuang@friendlyarm.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	"kernel test robot" <lkp@intel.com>
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM NanoPi R4S
Date: Sat, 13 Mar 2021 21:22:17 +0800	[thread overview]
Message-ID: <CAOP2_TgUQH9H9vuESek7KjStxu=idaBxX=S5Q7_=Mn=D_s-=JA@mail.gmail.com> (raw)
In-Reply-To: <1f7f1a11-0b9b-b2e4-efbc-193f61d62832@arm.com>

Robin Murphy <robin.murphy@arm.com> 于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@friendlyarm.com>
> > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > [minor adjustments]
> > Co-developed-by: Marty Jones <mj8263788@gmail.com>
> > Signed-off-by: Marty Jones <mj8263788@gmail.com>
> > [fixed format issues]
> > Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >   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@friendlyarm.com>
> > + * Copyright (c) 2020 Marty Jones <mj8263788@gmail.com>
> > + * Copyright (c) 2021 Tianling Shen <cnsztl@gmail.com>
> > + */
> > +
> > +/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.