Re: [PATCH 2/2] arm64: dts: rockchip: Add NanoPC T6

[Heiko Stübner]

Am Dienstag, 8. August 2023, 06:32:45 CEST schrieb Thomas McKahan:
> On Mon, 7 Aug 2023 08:44:40 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 
> > On 02/08/2023 07:14, Thomas McKahan wrote:
> > > Add the NanoPC T6, a single board computer from FriendlyElec based on
> > > the RK3588.
> > > 
> > > Initial device tree supports debug UART, SD, eMMC, PCIe 3, PMIC,
> > > and 40 pin GPIO assignments.
> > > 
> > > Signed-off-by: Thomas McKahan <tmckahan@xxxxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> > >  .../boot/dts/rockchip/rk3588-nanopc-t6.dts    | 845 ++++++++++++++++++
> > >  2 files changed, 846 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dts
> > > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > > index 1ebbb3e9c2f9..e7728007fd1b 100644
> > > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > > @@ -100,6 +100,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3a.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-io.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6b-io.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-evb1-v10.dtb
> > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-nanopc-t6.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova.dtb
> > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-khadas-edge2.dtb
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dts b/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dts
> > > new file mode 100644
> > > index 000000000000..2362da2c53d9
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dts
> > > @@ -0,0 +1,845 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright (c) 2021 Rockchip Electronics Co., Ltd.
> > > + * Copyright (c) 2023 Thomas McKahan
> > > + *
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include <dt-bindings/gpio/gpio.h>
> > > +#include <dt-bindings/pinctrl/rockchip.h>
> > > +#include <dt-bindings/usb/pd.h>
> > > +#include "rk3588.dtsi"
> > > +
> > > +/ {
> > > +	model = "FriendlyElec NanoPC-T6";
> > > +	compatible = "friendlyarm,nanopc-t6", "rockchip,rk3588";
> > > +
> 
> ...
> 
> > > +&spi2 {
> > > +	status = "okay";
> > > +	assigned-clocks = <&cru CLK_SPI2>;
> > > +	assigned-clock-rates = <200000000>;
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&spi2m2_cs0 &spi2m2_pins>;
> > > +	num-cs = <1>;
> > > +
> > > +	pmic@0 {
> > > +		compatible = "rockchip,rk806";
> > > +		spi-max-frequency = <1000000>;
> > > +		reg = <0x0>;
> > > +
> > > +		interrupt-parent = <&gpio0>;
> > > +		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> > > +
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pmic_pins>, <&rk806_dvs1_null>,
> > > +			    <&rk806_dvs2_null>, <&rk806_dvs3_null>;
> > > +
> > > +		vcc1-supply = <&vcc4v0_sys>;
> > > +		vcc2-supply = <&vcc4v0_sys>;
> > > +		vcc3-supply = <&vcc4v0_sys>;
> > > +		vcc4-supply = <&vcc4v0_sys>;
> > > +		vcc5-supply = <&vcc4v0_sys>;
> > > +		vcc6-supply = <&vcc4v0_sys>;
> > > +		vcc7-supply = <&vcc4v0_sys>;
> > > +		vcc8-supply = <&vcc4v0_sys>;
> > > +		vcc9-supply = <&vcc4v0_sys>;
> > > +		vcc10-supply = <&vcc4v0_sys>;
> > > +		vcc11-supply = <&vcc_2v0_pldo_s3>;
> > > +		vcc12-supply = <&vcc4v0_sys>;
> > > +		vcc13-supply = <&vcc_1v1_nldo_s3>;
> > > +		vcc14-supply = <&vcc_1v1_nldo_s3>;
> > > +		vcca-supply = <&vcc4v0_sys>;
> > > +
> > > +		gpio-controller;
> > > +		#gpio-cells = <2>;
> > > +
> > > +		rk806_dvs1_null: dvs1-null-pins {
> > > +			pins = "gpio_pwrctrl2";
> > > +			function = "pin_fun0";
> > > +		};
> > > +
> > > +		rk806_dvs2_null: dvs2-null-pins {
> > > +			pins = "gpio_pwrctrl2";
> > > +			function = "pin_fun0";
> > > +		};
> > > +
> > > +		rk806_dvs3_null: dvs3-null-pins {
> > > +			pins = "gpio_pwrctrl3";
> > > +			function = "pin_fun0";
> > > +		};
> > > +
> > > +		regulators {
> > > +			vdd_gpu_s0: vdd_gpu_mem_s0: dcdc-reg1 {
> > > +				regulator-boot-on;  
> > 
> > Boolean properties are not first, but last. regulator-name is the first
> > one. This odd style...
> > 
> 
> I agree, however it seems the norm in Rockchip devices. This will
> become an outlier in Rockchip but fall in line with the general case.
> I'll put it in V2 with the other mentioned fixes unless a conflicting 
> opinion is expressed. 

I definitly do prefer regulator-name to be the first regulator-* (same as
compatible for regular nodes) and I think generally I do catch outliers
in this .

But for the rest of the propeties I guess it's different. While true I
followed a scheme of booleans at the bottom in the past, going with
"name at the top, rest alphabetically" is way easier to explain to people.

[normal nodes should do: {compatible, regs, interrupts, ..alphabetical..,
  status} of course ]

And of course regulator nodes are generally in board-files not in central
soc dtsis and if needed I'll also just move things around when applying ;-).


Heiko


> 
> > > +				regulator-min-microvolt = <550000>;
> > > +				regulator-max-microvolt = <950000>;
> > > +				regulator-ramp-delay = <12500>;
> > > +				regulator-name = "vdd_gpu_s0";
> > > +				regulator-enable-ramp-delay = <400>;
> > > +
> > > +				regulator-state-mem {
> > > +					regulator-off-in-suspend;
> > > +				};
> > > +			};  
> > 
> > 
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> Best Regards,
> Thomas
>