Hi Liang,

Am Mittwoch, 21. April 2021, 08:59:20 CEST schrieb cl@rock-chips.com:

From: Liang Chen <redacted>

RK3568 is a high-performance and low power quad-core application processor
designed for personal mobile internet device and AIoT equipments.

This patch add basic core dtsi file for it.

Signed-off-by: Liang Chen <redacted>

this is a first round of basic stuff :-) .

First of all, I really like the move of moving the pretty standardized
pinconfig entries to the rockchip-pinconf.dtsi .

(1) But please move this into a separate patch to make that more visible
and maybe even convert _some_ or all arm64 Rockchip socs to use that
as well

"arm64: dts: rockchip: add generic pinconfig settings used by most Rockchip socs

The pinconfig settings for Rockchip SoCs are pretty similar on all socs,
so move them to a shared dtsi to be included, instead of redefining them
for each soc"

(2) I also like the external rk3568-pinctrl approach with the dtsi getting
auto-generated. This will probably help us in keeping pinctrl settings
synchronous between mainline and the vendor kernel.

(3) From my basic understanding the rk3568 is basically a rk3566 + more
peripherals, so ideally they would share the basic ones in a rk3566.dtsi
which the rk3568.dtsi then could include and extend with its additional
peripherals.

With at least the pine64 boards being based on the rk3566, there probably
will be quite a mainline use of it as well.

Or is there something that would prevent this?


More below:

can you adapt your pin2dts tool to insert a blank line here please?

+	audiopwm {
+		/omit-if-no-ref/
+		audiopwm_lout: audiopwm-lout {
+			rockchip,pins =
+				/* audiopwm_lout */
+				<1 RK_PA0 4 &pcfg_pull_none>;
+		};

same here and of course below.

I.e. in a list of subnodes it would be really nice if you could add a blank
line above every subnode - except the first of course ;-)

So,
+	audiopwm {
+		/omit-if-no-ref/
+		audiopwm_lout: audiopwm-lout {

does not get a blank line before audiopwm-lout, but between audiopwn-lout
and the next sub node.

+		/omit-if-no-ref/
+		audiopwm_loutn: audiopwm-loutn {
+			rockchip,pins =
+				/* audiopwm_loutn */
+				<1 RK_PA1 6 &pcfg_pull_none>;
+		};

[...]

what is <&scmi_clk 0>.

I see the rk3568 clk-driver defining our standard ARMCLK, so I'd ask for
an explanation (in the commit message maybe) what makes the scmi clk
different and especially what firmware-dependencies arise.

Also it would be very cool if Rockchip could upstream rk3568-support to
mainline TF-A, so that people don't need binary blobs for this.

+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
+		};

blank lines between nodes in the same hierarchy level

+		cpu1: cpu@100 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
+		};
+		cpu2: cpu@200 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x0 0x200>;
+			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
+		};
+		cpu3: cpu@300 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x0 0x300>;
+			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
+		};
+	};
+
+	cpu0_opp_table: cpu0-opp-table {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <825000 825000 1150000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <825000 825000 1150000>;
+		};
+		opp-816000000 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <825000 825000 1150000>;
+			opp-suspend;
+		};
+		opp-1104000000 {
+			opp-hz = /bits/ 64 <1104000000>;
+			opp-microvolt = <825000 825000 1150000>;
+		};
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <900000 900000 1150000>;
+		};
+		opp-1608000000 {
+			opp-hz = /bits/ 64 <1608000000>;
+			opp-microvolt = <975000 975000 1150000>;
+		};
+		opp-1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <1050000 1050000 1150000>;
+		};
+		opp-1992000000 {
+			opp-hz = /bits/ 64 <1992000000>;
+			opp-microvolt = <1150000 1150000 1150000>;
+		};
+	};
+
+	arm-pmu {
+		compatible = "arm,cortex-a55-pmu", "arm,armv8-pmuv3";
+		interrupts = <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 231 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
+	};
+
+	firmware {
+		scmi: scmi {
+			compatible = "arm,scmi-smc";
+			shmem = <&scmi_shmem>;
+			arm,smc-id = <0x82000010>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			scmi_clk: protocol@14 {
+				reg = <0x14>;
+				#clock-cells = <1>;
+			};
+		};
+
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+		arm,no-tick-in-suspend;
+	};
+
+	xin24m: xin24m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+		clock-output-names = "xin24m";
+	};
+
+	xin32k: xin32k {
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "xin32k";
+		#clock-cells = <0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&clk32k_out0>;
+	};
+
+	scmi_shmem: scmi-shmem@10f000 {
+		compatible = "arm,scmi-shmem";
+		reg = <0x0 0x0010f000 0x0 0x100>;

are you sure this works in mainline?

I.e. looking at Documentation/devicetree/bindings/arm/arm,scmi.txt
it talks about arm,scmi-shmem being part of a defined sram area
while the node above seems to be in main memory?

+	};
+
+	gic: interrupt-controller@fd400000 {
+		compatible = "arm,gic-v3";

reg + interrupts up here below compatible please (+rest alphabetically sorted)

+		#interrupt-cells = <3>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		interrupt-controller;
+
+		reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
+		      <0x0 0xfd460000 0 0xc0000>; /* GICR */
+		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+	};

no blank line here please ;-)

The other blank lines below are correct though

And as said at the top, please move into a separate patch and
maybe convert some other Rockchip arm64 socs to it as well ;-) .

+
+	/omit-if-no-ref/
+	pcfg_pull_up: pcfg-pull-up {
+		bias-pull-up;
+	};
+
+	/omit-if-no-ref/
+	pcfg_pull_down: pcfg-pull-down {
+		bias-pull-down;
+	};
+
+	/omit-if-no-ref/
+	pcfg_pull_none: pcfg-pull-none {
+		bias-disable;
+	};
+

Thanks
Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel