| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOP2_TgUQH9H9vuESek7KjStxu=idaBxX=S5Q7_=Mn=D_s-=JA@mail.gmail.com>
Date: Sat, 13 Mar 2021 21:22:17 +0800
From: CN_SZTL <cnsztl@...il.com>
To: Robin Murphy <robin.murphy@....com>
Cc: Rob Herring <robh+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
Jagan Teki <jagan@...rulasolutions.com>,
Chen-Yu Tsai <wens@...e.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
David Bauer <mail@...id-bauer.net>,
Uwe Kleine-König <uwe@...ine-koenig.org>,
Johan Jonker <jbx6244@...il.com>,
Michael Trimarchi <michael@...rulasolutions.com>,
Marty Jones <mj8263788@...il.com>,
Jensen Huang <jensenhuang@...endlyarm.com>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3 2/2] rockchip: rk3399: Add support for FriendlyARM
NanoPi R4S
Robin Murphy <robin.murphy@....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@...endlyarm.com>
> > Signed-off-by: Jensen Huang <jensenhuang@...endlyarm.com>
> > [minor adjustments]
> > Co-developed-by: Marty Jones <mj8263788@...il.com>
> > Signed-off-by: Marty Jones <mj8263788@...il.com>
> > [fixed format issues]
> > Signed-off-by: Tianling Shen <cnsztl@...il.com>
> >
> > Reported-by: kernel test robot <lkp@...el.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@...endlyarm.com>
> > + * Copyright (c) 2020 Marty Jones <mj8263788@...il.com>
> > + * Copyright (c) 2021 Tianling Shen <cnsztl@...il.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.
Powered by blists - more mailing lists