2021-05-17 00:56:46

by Andreas Färber

[permalink] [raw]
Subject: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

Add an initial Device Tree for Rockchip RK1808 SoC.
Based on shipping TB-RK1808M0 DTB.

Signed-off-by: Andreas Färber <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
1 file changed, 203 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
new file mode 100644
index 000000000000..af2b51afda7d
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
+/*
+ * Copyright (c) 2021 Andreas Färber
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ compatible = "rockchip,rk1808";
+ interrupt-parent = <&gic>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart1;
+ serial2 = &uart2;
+ serial3 = &uart3;
+ serial4 = &uart4;
+ serial5 = &uart5;
+ serial6 = &uart6;
+ serial7 = &uart7;
+ };
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ cpu0: [email protected] {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <0x0 0x0>;
+ enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP>;
+ };
+
+ cpu1: [email protected] {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <0x0 0x1>;
+ enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP>;
+ };
+
+ idle-states {
+ entry-method = "psci";
+
+ CPU_SLEEP: cpu-sleep {
+ compatible = "arm,idle-state";
+ local-timer-stop;
+ arm,psci-suspend-param = <0x10000>;
+ entry-latency-us = <120>;
+ exit-latency-us = <250>;
+ min-residency-us = <900>;
+ };
+ };
+ };
+
+ arm-pmu {
+ compatible = "arm,cortex-a35-pmu";
+ interrupts = <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-affinity = <&cpu0>, <&cpu1>;
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+ arm,no-tick-in-suspend;
+ };
+
+ xin24m: xin24m {
+ compatible = "fixed-clock";
+ clock-frequency = <24000000>;
+ #clock-cells = <0>;
+ clock-output-names = "xin24m";
+ };
+
+ firmware {
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+ };
+
+ tee {
+ compatible = "linaro,optee-tz";
+ method = "smc";
+ };
+ };
+
+ soc {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ system_sram: [email protected] {
+ compatible = "mmio-sram";
+ reg = <0xfec00000 0x200000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0xfec00000 0x200000>;
+ };
+
+ gic: [email protected] {
+ compatible = "arm,gic-v3";
+ reg = <0xff100000 0x10000>, /* GICD */
+ <0xff140000 0xc0000>, /* GICR */
+ <0xff300000 0x10000>, /* GICC */
+ <0xff310000 0x10000>, /* GICH */
+ <0xff320000 0x10000>; /* GICV */
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ gic_its: [email protected] {
+ compatible = "arm,gic-v3-its";
+ reg = <0xff120000 0x20000>;
+ msi-controller;
+ #msi-cells = <1>;
+ };
+ };
+
+ uart0: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff430000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart1: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff540000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart2: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff550000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart3: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff560000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart4: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff570000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart5: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff5a0000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart6: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff5b0000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ uart7: [email protected] {
+ compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
+ reg = <0xff5c0000 0x100>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+ };
+};
--
2.31.1



2021-05-17 02:14:41

by Johan Jonker

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

Hi Andreas,

Send the complete serie to all maintainers and mail lists.

===
Heiko's sort rules:

compatible
reg
interrupts
[alphabetical]
status [if needed]

===
My incomplete list:

For nodes:
If exists on top: model, compatible and chosen.
Sort things without reg alphabetical first,
then sort the rest by reg address.

Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.
Keep an empty line between properties and nodes.

Exceptions:
Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
and dma-names.
Sort simple-audio-card,name above other simple-audio-card properties.
Sort regulator-name above other regulator properties.
Sort regulator-min-microvolt above regulator-max-microvolt.

===

Fix complete dtsi for property sort order!
Add more drivers. (cru, pinctrl)

Johan

On 5/17/21 1:05 AM, Andreas Färber wrote:
> Add an initial Device Tree for Rockchip RK1808 SoC.
> Based on shipping TB-RK1808M0 DTB.
>
> Signed-off-by: Andreas Färber <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
> 1 file changed, 203 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> new file mode 100644
> index 000000000000..af2b51afda7d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> @@ -0,0 +1,203 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
> +/*
> + * Copyright (c) 2021 Andreas Färber
> + */
> +

#include <dt-bindings/clock/rk1808-cru.h>

> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + compatible = "rockchip,rk1808";
> + interrupt-parent = <&gic>;

> + #address-cells = <1>;
> + #size-cells = <1>;


#address-cells = <2>;
#size-cells = <2>;

64 bit ??

> +
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart1;
> + serial2 = &uart2;
> + serial3 = &uart3;
> + serial4 = &uart4;
> + serial5 = &uart5;
> + serial6 = &uart6;
> + serial7 = &uart7;
> + };
> +
> + cpus {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + cpu0: [email protected] {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x0>;
> + enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP>;
> + };
> +
> + cpu1: [email protected] {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x1>;
> + enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP>;
> + };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + CPU_SLEEP: cpu-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + arm,psci-suspend-param = <0x10000>;
> + entry-latency-us = <120>;
> + exit-latency-us = <250>;
> + min-residency-us = <900>;
> + };
> + };
> + };
> +

> + arm-pmu {

sort node names

> + compatible = "arm,cortex-a35-pmu";
> + interrupts = <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-affinity = <&cpu0>, <&cpu1>;
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> + arm,no-tick-in-suspend;
> + };
> +
> + xin24m: xin24m {
> + compatible = "fixed-clock";
> + clock-frequency = <24000000>;
> + #clock-cells = <0>;
> + clock-output-names = "xin24m";
> + };
> +

> + firmware {

sort node names

> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + tee {
> + compatible = "linaro,optee-tz";
> + method = "smc";
> + };
> + };
> +

> + soc {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;

Remove, use 64bit reg.
See:

https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk1808.dtsi

> +
> + system_sram: [email protected] {
> + compatible = "mmio-sram";
> + reg = <0xfec00000 0x200000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0xfec00000 0x200000>;
> + };
> +
> + gic: [email protected] {
> + compatible = "arm,gic-v3";
> + reg = <0xff100000 0x10000>, /* GICD */
> + <0xff140000 0xc0000>, /* GICR */
> + <0xff300000 0x10000>, /* GICC */
> + <0xff310000 0x10000>, /* GICH */
> + <0xff320000 0x10000>; /* GICV */
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + gic_its: [email protected] {
> + compatible = "arm,gic-v3-its";
> + reg = <0xff120000 0x20000>;
> + msi-controller;
> + #msi-cells = <1>;
> + };
> + };
> +

> + uart0: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff430000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };

From manufacturer tree:

uart0: [email protected] {
compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
reg = <0x0 0xff430000 0x0 0x100>;
interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cru SCLK_UART0_PMU>, <&cru PCLK_UART0_PMU>;
clock-names = "baudclk", "apb_pclk";
dmas = <&dmac 0>, <&dmac 1>;
pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>;
pinctrl-names = "default";
reg-io-width = <4>;
reg-shift = <2>;
status = "disabled";
};

Sort all uart nodes.

Does this work without SCLK_UART0_PMU, PCLK_UART0_PMU and pinctrl??
Add clk-rk1808.c rk1808-cru.h

In mainline pinctrl and gpio are WIP and split elsewhere now.

pinctrl: rockchip: add support for rk1808 SoCs

https://github.com/rockchip-linux/kernel/commit/b2828bc4417c9669fdaca3b8ef392f41850c86e7

> +
> + uart1: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff540000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart2: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff550000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart3: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff560000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart4: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff570000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart5: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff5a0000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart6: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff5b0000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> +
> + uart7: [email protected] {
> + compatible = "rockchip,rk1808-uart", "snps,dw-apb-uart";
> + reg = <0xff5c0000 0x100>;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> + };
> +};
>

2021-05-17 09:23:17

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

On Mon, 17 May 2021 00:05:45 +0100,
Andreas Färber <[email protected]> wrote:
>
> Add an initial Device Tree for Rockchip RK1808 SoC.
> Based on shipping TB-RK1808M0 DTB.
>
> Signed-off-by: Andreas Färber <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
> 1 file changed, 203 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> new file mode 100644
> index 000000000000..af2b51afda7d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> @@ -0,0 +1,203 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
> +/*
> + * Copyright (c) 2021 Andreas Färber
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + compatible = "rockchip,rk1808";
> + interrupt-parent = <&gic>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart1;
> + serial2 = &uart2;
> + serial3 = &uart3;
> + serial4 = &uart4;
> + serial5 = &uart5;
> + serial6 = &uart6;
> + serial7 = &uart7;
> + };
> +
> + cpus {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + cpu0: [email protected] {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x0>;
> + enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP>;
> + };
> +
> + cpu1: [email protected] {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x1>;
> + enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP>;
> + };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + CPU_SLEEP: cpu-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + arm,psci-suspend-param = <0x10000>;
> + entry-latency-us = <120>;
> + exit-latency-us = <250>;
> + min-residency-us = <900>;
> + };
> + };
> + };
> +
> + arm-pmu {
> + compatible = "arm,cortex-a35-pmu";
> + interrupts = <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-affinity = <&cpu0>, <&cpu1>;
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> + arm,no-tick-in-suspend;

Another facepalm moment...

> + };
> +
> + xin24m: xin24m {
> + compatible = "fixed-clock";
> + clock-frequency = <24000000>;
> + #clock-cells = <0>;
> + clock-output-names = "xin24m";
> + };
> +
> + firmware {
> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + tee {
> + compatible = "linaro,optee-tz";
> + method = "smc";
> + };
> + };
> +
> + soc {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + system_sram: [email protected] {
> + compatible = "mmio-sram";
> + reg = <0xfec00000 0x200000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0xfec00000 0x200000>;
> + };
> +
> + gic: [email protected] {
> + compatible = "arm,gic-v3";
> + reg = <0xff100000 0x10000>, /* GICD */
> + <0xff140000 0xc0000>, /* GICR */

This is obviously wrong. You have two CPUs, and yet describe a range
that spans 6. I guess this is a copy paste from rk3399 again?

> + <0xff300000 0x10000>, /* GICC */
> + <0xff310000 0x10000>, /* GICH */
> + <0xff320000 0x10000>; /* GICV */
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + gic_its: [email protected] {
> + compatible = "arm,gic-v3-its";
> + reg = <0xff120000 0x20000>;
> + msi-controller;
> + #msi-cells = <1>;
> + };

What uses the ITS?

M.

--
Without deviation from the norm, progress is not possible.

2021-05-17 14:09:56

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

Hi Johan,

On 17.05.21 03:29, Johan Jonker wrote:
> Send the complete serie to all maintainers and mail lists.

That's unhelpful - all patches went to linux-rockchip, LAKML and LKML,
and get_maintainers.pl was used. The cover letter was explicitly copied
to DTML as get_maintainers.pl doesn't catch it. You'll find them here:

https://lore.kernel.org/linux-rockchip/[email protected]/

Which mailing list or maintainer do you think all should've gone to in
addition? You're not listed anywhere in linux-next MAINTAINERS. If you
want to be CC'ed, you can nicely ask me to for a v2 (and explaining why
would help for the next series), or you can send patches against
MAINTAINERS yourself.

Copying all maintainers and lists would likely not be appreciated by the
colleagues and may get mails flagged as spam.

> ===
> Heiko's sort rules:
>
> compatible
> reg
> interrupts
> [alphabetical]
> status [if needed]
>
> ===
> My incomplete list:
>
> For nodes:
> If exists on top: model, compatible and chosen.
> Sort things without reg alphabetical first,
> then sort the rest by reg address.
>
> Inside nodes:
> If exists on top: compatible, reg and interrupts.
> In alphabetical order the required properties.
> Then in alphabetical order the other properties.
> And as last things that start with '#' in alphabetical order.
> Add status below all other properties for soc internal components with
> any board-specifics.
> Keep an empty line between properties and nodes.
>
> Exceptions:
> Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
> and dma-names.
> Sort simple-audio-card,name above other simple-audio-card properties.
> Sort regulator-name above other regulator properties.
> Sort regulator-min-microvolt above regulator-max-microvolt.
>
> ===


Here's a rule for you: No top-posting on kernel lists.


>
> Fix complete dtsi for property sort order!
> Add more drivers. (cru, pinctrl)

-ETONE! I don't work for Rockchip, so don't command me to add drivers
for them that I have no source code for (cover letter) nor complete TRM.
You're welcome that I did this service to the kernel community in my
spare time... If you look up the date of Hackweek 20, you'll find that
it took me two months to get this patchset binding-documented and
cleaned up to this point, so by extrapolation it's unrealistic to expect
much more of me here anytime soon. Rudeness certainly does not motivate.

Heiko knows me - if he has any comments, he should be well capable of
voicing them himself inline. I will not follow nonsensical rules that
diverge from other mainline arm-soc vendors - unless I've missed some
new directive from Rob or arm-soc maintainers that would apply to all.

Thanks for nothing,

Andreas

--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

2021-05-24 13:34:11

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

On 17.05.21 11:21, Marc Zyngier wrote:
> On Mon, 17 May 2021 00:05:45 +0100,
> Andreas Färber <[email protected]> wrote:
>>
>> Add an initial Device Tree for Rockchip RK1808 SoC.
>> Based on shipping TB-RK1808M0 DTB.
>>
>> Signed-off-by: Andreas Färber <[email protected]>
>> ---
>> arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
>> 1 file changed, 203 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
>> new file mode 100644
>> index 000000000000..af2b51afda7d
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
[...]
>> + gic: [email protected] {
>> + compatible = "arm,gic-v3";
>> + reg = <0xff100000 0x10000>, /* GICD */
>> + <0xff140000 0xc0000>, /* GICR */
>
> This is obviously wrong. You have two CPUs, and yet describe a range
> that spans 6. I guess this is a copy paste from rk3399 again?

Not on my part at least. As indicated, these numbers are what ships in
the DTB on the RK1808 card, as per dtc -I dtb -O dts. Could be a mistake
by Rockchip, of course.

Are you suggesting 0xc0000/6*2 = 0x40000 for two CPUs here?
Works as bad as before - investigation still ongoing with latest next.

As for "obviously": The GICv3 YAML binding has no description for me to
validate those numbers: "GIC Redistributors (GICR), one range per
redistributor region" - says nothing about correlation to number of CPUs
or size per CPU, and the examples are not explaining either: 0x200000
has no number of CPUs associated, and by my calculation 0x800000 for 32
CPUs results in 0x40000 per CPU; but then again the examples also have
GICC etc. at diverging 0x2000 size.

>> + <0xff300000 0x10000>, /* GICC */
>> + <0xff310000 0x10000>, /* GICH */
>> + <0xff320000 0x10000>; /* GICV */
>> + interrupt-controller;
>> + #interrupt-cells = <3>;
>> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + gic_its: [email protected] {
>> + compatible = "arm,gic-v3-its";
>> + reg = <0xff120000 0x20000>;
>> + msi-controller;
>> + #msi-cells = <1>;
>> + };
>
> What uses the ITS?

DT-wise seemingly only the __symbols__ table (named just "its" there, I
notice), so we could drop (or rename) the label if you prefer.

Regards,
Andreas

--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

2021-05-24 15:26:31

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

On Mon, 24 May 2021 14:32:41 +0100,
Andreas Färber <[email protected]> wrote:
>
> On 17.05.21 11:21, Marc Zyngier wrote:
> > On Mon, 17 May 2021 00:05:45 +0100,
> > Andreas Färber <[email protected]> wrote:
> >>
> >> Add an initial Device Tree for Rockchip RK1808 SoC.
> >> Based on shipping TB-RK1808M0 DTB.
> >>
> >> Signed-off-by: Andreas Färber <[email protected]>
> >> ---
> >> arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
> >> 1 file changed, 203 insertions(+)
> >> create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
> >>
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> >> new file mode 100644
> >> index 000000000000..af2b51afda7d
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> [...]
> >> + gic: [email protected] {
> >> + compatible = "arm,gic-v3";
> >> + reg = <0xff100000 0x10000>, /* GICD */
> >> + <0xff140000 0xc0000>, /* GICR */
> >
> > This is obviously wrong. You have two CPUs, and yet describe a range
> > that spans 6. I guess this is a copy paste from rk3399 again?
>
> Not on my part at least. As indicated, these numbers are what ships in
> the DTB on the RK1808 card, as per dtc -I dtb -O dts. Could be a mistake
> by Rockchip, of course.
>
> Are you suggesting 0xc0000/6*2 = 0x40000 for two CPUs here? Works
> as bad as before - investigation still ongoing with latest next.
>
> As for "obviously": The GICv3 YAML binding has no description for me to
> validate those numbers: "GIC Redistributors (GICR), one range per
> redistributor region" - says nothing about correlation to number of CPUs
> or size per CPU, and the examples are not explaining either: 0x200000
> has no number of CPUs associated, and by my calculation 0x800000 for 32
> CPUs results in 0x40000 per CPU; but then again the examples also have
> GICC etc. at diverging 0x2000 size.

The GICv3/v4 architecture spec does apply, and you should really have
a look at what these sizes mean. What is the value of copy-pasting
things without understanding it the first place?

>
> >> + <0xff300000 0x10000>, /* GICC */
> >> + <0xff310000 0x10000>, /* GICH */
> >> + <0xff320000 0x10000>; /* GICV */
> >> + interrupt-controller;
> >> + #interrupt-cells = <3>;
> >> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + ranges;
> >> +
> >> + gic_its: [email protected] {
> >> + compatible = "arm,gic-v3-its";
> >> + reg = <0xff120000 0x20000>;
> >> + msi-controller;
> >> + #msi-cells = <1>;
> >> + };
> >
> > What uses the ITS?
>
> DT-wise seemingly only the __symbols__ table (named just "its" there, I
> notice), so we could drop (or rename) the label if you prefer.

No, I am asking *what* uses the ITS. Is it just dangling without any
user? No PCI bus making use of it?

M.

--
Without deviation from the norm, progress is not possible.

2021-05-24 23:58:26

by Heiko Stübner

[permalink] [raw]
Subject: Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

Am Montag, 24. Mai 2021, 17:21:41 CEST schrieb Marc Zyngier:
> On Mon, 24 May 2021 14:32:41 +0100,
> Andreas F?rber <[email protected]> wrote:
> >
> > On 17.05.21 11:21, Marc Zyngier wrote:
> > > On Mon, 17 May 2021 00:05:45 +0100,
> > > Andreas F?rber <[email protected]> wrote:
> > >>
> > >> Add an initial Device Tree for Rockchip RK1808 SoC.
> > >> Based on shipping TB-RK1808M0 DTB.
> > >>
> > >> Signed-off-by: Andreas F?rber <[email protected]>
> > >> ---
> > >> arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
> > >> 1 file changed, 203 insertions(+)
> > >> create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
> > >>
> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> > >> new file mode 100644
> > >> index 000000000000..af2b51afda7d
> > >> --- /dev/null
> > >> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> > [...]
> > >> + gic: [email protected] {
> > >> + compatible = "arm,gic-v3";
> > >> + reg = <0xff100000 0x10000>, /* GICD */
> > >> + <0xff140000 0xc0000>, /* GICR */
> > >
> > > This is obviously wrong. You have two CPUs, and yet describe a range
> > > that spans 6. I guess this is a copy paste from rk3399 again?
> >
> > Not on my part at least. As indicated, these numbers are what ships in
> > the DTB on the RK1808 card, as per dtc -I dtb -O dts. Could be a mistake
> > by Rockchip, of course.
> >
> > Are you suggesting 0xc0000/6*2 = 0x40000 for two CPUs here? Works
> > as bad as before - investigation still ongoing with latest next.
> >
> > As for "obviously": The GICv3 YAML binding has no description for me to
> > validate those numbers: "GIC Redistributors (GICR), one range per
> > redistributor region" - says nothing about correlation to number of CPUs
> > or size per CPU, and the examples are not explaining either: 0x200000
> > has no number of CPUs associated, and by my calculation 0x800000 for 32
> > CPUs results in 0x40000 per CPU; but then again the examples also have
> > GICC etc. at diverging 0x2000 size.
>
> The GICv3/v4 architecture spec does apply, and you should really have
> a look at what these sizes mean. What is the value of copy-pasting
> things without understanding it the first place?
>
> >
> > >> + <0xff300000 0x10000>, /* GICC */
> > >> + <0xff310000 0x10000>, /* GICH */
> > >> + <0xff320000 0x10000>; /* GICV */
> > >> + interrupt-controller;
> > >> + #interrupt-cells = <3>;
> > >> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> > >> + #address-cells = <1>;
> > >> + #size-cells = <1>;
> > >> + ranges;
> > >> +
> > >> + gic_its: [email protected] {
> > >> + compatible = "arm,gic-v3-its";
> > >> + reg = <0xff120000 0x20000>;
> > >> + msi-controller;
> > >> + #msi-cells = <1>;
> > >> + };
> > >
> > > What uses the ITS?
> >
> > DT-wise seemingly only the __symbols__ table (named just "its" there, I
> > notice), so we could drop (or rename) the label if you prefer.
>
> No, I am asking *what* uses the ITS. Is it just dangling without any
> user? No PCI bus making use of it?

just 2ct, as far as I remember the rk1808 does have a PCIe controller.
And the datasheet [0] does agree with my memory it seems


Heiko

[0] http://opensource.rock-chips.com/images/4/43/Rockchip_RK1808_Datasheet_V1.2_20190527.pdf