diff mbox series

[3/4] arm64: dts: rockchip: Add base DT for rk3528 SoC

Message ID 20240803125510.4699-5-ziyao@disroot.org (mailing list archive)
State New
Headers show
Series Add initial support for Rockchip RK3528 SoC | expand

Commit Message

Yao Zi Aug. 3, 2024, 12:55 p.m. UTC
This initial device tree describes CPU, interrupts and UART on the chip
and is able to boot into basic kernel with only UART. Cache information
is omitted for now as there is no precise documentation. Support for
other features will be added later.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
 1 file changed, 182 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi

Comments

Krzysztof Kozlowski Aug. 4, 2024, 10:05 a.m. UTC | #1
On 03/08/2024 14:55, Yao Zi wrote:
> This initial device tree describes CPU, interrupts and UART on the chip
> and is able to boot into basic kernel with only UART. Cache information
> is omitted for now as there is no precise documentation. Support for
> other features will be added later.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
>  1 file changed, 182 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> new file mode 100644
> index 000000000000..77687d9e7e80
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> +	compatible = "rockchip,rk3528";
> +
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +		serial3 = &uart3;
> +		serial4 = &uart4;
> +		serial5 = &uart5;
> +		serial6 = &uart6;
> +		serial7 = &uart7;
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&cpu0>;
> +				};
> +				core1 {
> +					cpu = <&cpu1>;
> +				};
> +				core2 {
> +					cpu = <&cpu2>;
> +				};
> +				core3 {
> +					cpu = <&cpu3>;
> +				};
> +			};
> +		};
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x1>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu2: cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x2>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu3: cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x3>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +	};
> +
> +	xin24m: xin24m {

Please use name for all fixed clocks which matches current format
recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +		clock-output-names = "xin24m";
> +	};
> +
> +	gic: interrupt-controller@fed01000 {

Why this all is outside of SoC?

Best regards,
Krzysztof
Diederik de Haas Aug. 4, 2024, 11:27 a.m. UTC | #2
On Saturday, 3 August 2024 14:55:10 CEST Yao Zi wrote:
> +       gic: interrupt-controller@fed01000 {
> +               compatible = "arm,gic-400";
> +               #interrupt-cells = <3>;
> +               #address-cells = <0>;
> +               interrupt-controller;
> +               reg = <0x0 0xfed01000 0 0x1000>,
> +                     <0x0 0xfed02000 0 0x2000>,
> +                     <0x0 0xfed04000 0 0x2000>,
> +                     <0x0 0xfed06000 0 0x2000>;
> +               interrupts = <GIC_PPI 9
> +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +       };
> +
> +       uart0: serial@ff9f0000 {
> +               compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
> +               reg = <0x0 0xff9f0000 0x0 0x100>;
> +               interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> +               reg-shift = <2>;
> +               reg-io-width = <4>;
> +               clock-frequency = <24000000>;
> +               status = "disabled";
> +       };

The properties should be sorted as follows:
- compatible
- reg
- <other properties sorted alphabetically>
- status

This also applies to the other blocks which I didn't quote.

Cheers,
  Diederik
Heiko Stübner Aug. 4, 2024, 12:49 p.m. UTC | #3
Hi Krzysztof,

Am Sonntag, 4. August 2024, 12:05:11 CEST schrieb Krzysztof Kozlowski:
> On 03/08/2024 14:55, Yao Zi wrote:
> > This initial device tree describes CPU, interrupts and UART on the chip
> > and is able to boot into basic kernel with only UART. Cache information
> > is omitted for now as there is no precise documentation. Support for
> > other features will be added later.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---

> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> > +		clock-output-names = "xin24m";
> > +	};
> > +
> > +	gic: interrupt-controller@fed01000 {
> 
> Why this all is outside of SoC?

I guess you mean outside of a "soc {}" node?

Here the rk3528 simply follows all other Rockchip SoCs :-) .

Digging into the history, the first rk3066a and initial rk3288 submission
did use a soc {} node, which later got removed as suggested by arm-soc
maintainers at the time [0].

I guess that changed since then?


Heiko


[0] https://lore.kernel.org/linux-arm-kernel/20140716005528.GA26207@quad.lixom.net/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c3030d30d9c99c057b5ddfa289cffa637a2775f5
Yao Zi Aug. 4, 2024, 1:20 p.m. UTC | #4
On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
> On 03/08/2024 14:55, Yao Zi wrote:
> > This initial device tree describes CPU, interrupts and UART on the chip
> > and is able to boot into basic kernel with only UART. Cache information
> > is omitted for now as there is no precise documentation. Support for
> > other features will be added later.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
> >  1 file changed, 182 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > new file mode 100644
> > index 000000000000..77687d9e7e80
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +/ {
> > +	compatible = "rockchip,rk3528";
> > +
> > +	interrupt-parent = <&gic>;
> > +	#address-cells = <2>;
> > +	#size-cells = <2>;
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +		serial3 = &uart3;
> > +		serial4 = &uart4;
> > +		serial5 = &uart5;
> > +		serial6 = &uart6;
> > +		serial7 = &uart7;
> > +	};
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu-map {
> > +			cluster0 {
> > +				core0 {
> > +					cpu = <&cpu0>;
> > +				};
> > +				core1 {
> > +					cpu = <&cpu1>;
> > +				};
> > +				core2 {
> > +					cpu = <&cpu2>;
> > +				};
> > +				core3 {
> > +					cpu = <&cpu3>;
> > +				};
> > +			};
> > +		};
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x0>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu1: cpu@1 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x1>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu2: cpu@2 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x2>;
> > +			enable-method = "psci";
> > +		};
> > +
> > +		cpu3: cpu@3 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a53";
> > +			reg = <0x3>;
> > +			enable-method = "psci";
> > +		};
> > +	};
> > +
> > +	psci {
> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> > +		method = "smc";
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv8-timer";
> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> > +	};
> > +
> > +	xin24m: xin24m {
> 
> Please use name for all fixed clocks which matches current format
> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'

Will be fixed in next revision.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
> 
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> > +		clock-output-names = "xin24m";
> > +	};
> > +
> > +	gic: interrupt-controller@fed01000 {
> 
> Why this all is outside of SoC?

Just as Heiko says, device tree for all other Rockchip SoCs don't have
a "soc" node. I didn't know why before but just follow the style.

If you prefer add a soc node, I am willing to.

Best regards,
Yao Zi
Yao Zi Aug. 4, 2024, 1:22 p.m. UTC | #5
On Sun, Aug 04, 2024 at 01:27:35PM +0200, Diederik de Haas wrote:
> On Saturday, 3 August 2024 14:55:10 CEST Yao Zi wrote:
> > +       gic: interrupt-controller@fed01000 {
> > +               compatible = "arm,gic-400";
> > +               #interrupt-cells = <3>;
> > +               #address-cells = <0>;
> > +               interrupt-controller;
> > +               reg = <0x0 0xfed01000 0 0x1000>,
> > +                     <0x0 0xfed02000 0 0x2000>,
> > +                     <0x0 0xfed04000 0 0x2000>,
> > +                     <0x0 0xfed06000 0 0x2000>;
> > +               interrupts = <GIC_PPI 9
> > +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> > +       };
> > +
> > +       uart0: serial@ff9f0000 {
> > +               compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
> > +               reg = <0x0 0xff9f0000 0x0 0x100>;
> > +               interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > +               reg-shift = <2>;
> > +               reg-io-width = <4>;
> > +               clock-frequency = <24000000>;
> > +               status = "disabled";
> > +       };
> 
> The properties should be sorted as follows:
> - compatible
> - reg
> - <other properties sorted alphabetically>
> - status
> 
> This also applies to the other blocks which I didn't quote.

Thanks, will be fixed in next revision.

Best regards,
Yao Zi
Dragan Simic Aug. 4, 2024, 1:25 p.m. UTC | #6
On 2024-08-04 15:20, Yao Zi wrote:
> On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
>> On 03/08/2024 14:55, Yao Zi wrote:
>> > This initial device tree describes CPU, interrupts and UART on the chip
>> > and is able to boot into basic kernel with only UART. Cache information
>> > is omitted for now as there is no precise documentation. Support for
>> > other features will be added later.
>> >
>> > Signed-off-by: Yao Zi <ziyao@disroot.org>
>> > ---
>> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
>> >  1 file changed, 182 insertions(+)
>> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> > new file mode 100644
>> > index 000000000000..77687d9e7e80
>> > --- /dev/null
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> > @@ -0,0 +1,182 @@
>> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > +/*
>> > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
>> > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
>> > + */
>> > +
>> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> > +#include <dt-bindings/interrupt-controller/irq.h>
>> > +
>> > +/ {
>> > +	compatible = "rockchip,rk3528";
>> > +
>> > +	interrupt-parent = <&gic>;
>> > +	#address-cells = <2>;
>> > +	#size-cells = <2>;
>> > +
>> > +	aliases {
>> > +		serial0 = &uart0;
>> > +		serial1 = &uart1;
>> > +		serial2 = &uart2;
>> > +		serial3 = &uart3;
>> > +		serial4 = &uart4;
>> > +		serial5 = &uart5;
>> > +		serial6 = &uart6;
>> > +		serial7 = &uart7;
>> > +	};
>> > +
>> > +	cpus {
>> > +		#address-cells = <1>;
>> > +		#size-cells = <0>;
>> > +
>> > +		cpu-map {
>> > +			cluster0 {
>> > +				core0 {
>> > +					cpu = <&cpu0>;
>> > +				};
>> > +				core1 {
>> > +					cpu = <&cpu1>;
>> > +				};
>> > +				core2 {
>> > +					cpu = <&cpu2>;
>> > +				};
>> > +				core3 {
>> > +					cpu = <&cpu3>;
>> > +				};
>> > +			};
>> > +		};
>> > +
>> > +		cpu0: cpu@0 {
>> > +			device_type = "cpu";
>> > +			compatible = "arm,cortex-a53";
>> > +			reg = <0x0>;
>> > +			enable-method = "psci";
>> > +		};
>> > +
>> > +		cpu1: cpu@1 {
>> > +			device_type = "cpu";
>> > +			compatible = "arm,cortex-a53";
>> > +			reg = <0x1>;
>> > +			enable-method = "psci";
>> > +		};
>> > +
>> > +		cpu2: cpu@2 {
>> > +			device_type = "cpu";
>> > +			compatible = "arm,cortex-a53";
>> > +			reg = <0x2>;
>> > +			enable-method = "psci";
>> > +		};
>> > +
>> > +		cpu3: cpu@3 {
>> > +			device_type = "cpu";
>> > +			compatible = "arm,cortex-a53";
>> > +			reg = <0x3>;
>> > +			enable-method = "psci";
>> > +		};
>> > +	};
>> > +
>> > +	psci {
>> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
>> > +		method = "smc";
>> > +	};
>> > +
>> > +	timer {
>> > +		compatible = "arm,armv8-timer";
>> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> > +	};
>> > +
>> > +	xin24m: xin24m {
>> 
>> Please use name for all fixed clocks which matches current format
>> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
> 
> Will be fixed in next revision.

Hmm, why should we apply that rule to the xin24m clock, which is
named exactly like that everywhere else in Rockchip SoC dtsi files?
It's much better to remain consistent.

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
>> 
>> > +		compatible = "fixed-clock";
>> > +		#clock-cells = <0>;
>> > +		clock-frequency = <24000000>;
>> > +		clock-output-names = "xin24m";
>> > +	};
>> > +
>> > +	gic: interrupt-controller@fed01000 {
>> 
>> Why this all is outside of SoC?
> 
> Just as Heiko says, device tree for all other Rockchip SoCs don't have
> a "soc" node. I didn't know why before but just follow the style.
> 
> If you prefer add a soc node, I am willing to.
Heiko Stübner Aug. 4, 2024, 1:44 p.m. UTC | #7
Am Sonntag, 4. August 2024, 15:25:47 CEST schrieb Dragan Simic:
> On 2024-08-04 15:20, Yao Zi wrote:
> > On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
> >> On 03/08/2024 14:55, Yao Zi wrote:
> >> > This initial device tree describes CPU, interrupts and UART on the chip
> >> > and is able to boot into basic kernel with only UART. Cache information
> >> > is omitted for now as there is no precise documentation. Support for
> >> > other features will be added later.
> >> >
> >> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> >> > ---
> >> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
> >> >  1 file changed, 182 insertions(+)
> >> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> >
> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> > new file mode 100644
> >> > index 000000000000..77687d9e7e80
> >> > --- /dev/null
> >> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> > @@ -0,0 +1,182 @@
> >> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> > +/*
> >> > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> >> > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
> >> > + */
> >> > +
> >> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >> > +#include <dt-bindings/interrupt-controller/irq.h>
> >> > +
> >> > +/ {
> >> > +	compatible = "rockchip,rk3528";
> >> > +
> >> > +	interrupt-parent = <&gic>;
> >> > +	#address-cells = <2>;
> >> > +	#size-cells = <2>;
> >> > +
> >> > +	aliases {
> >> > +		serial0 = &uart0;
> >> > +		serial1 = &uart1;
> >> > +		serial2 = &uart2;
> >> > +		serial3 = &uart3;
> >> > +		serial4 = &uart4;
> >> > +		serial5 = &uart5;
> >> > +		serial6 = &uart6;
> >> > +		serial7 = &uart7;
> >> > +	};
> >> > +
> >> > +	cpus {
> >> > +		#address-cells = <1>;
> >> > +		#size-cells = <0>;
> >> > +
> >> > +		cpu-map {
> >> > +			cluster0 {
> >> > +				core0 {
> >> > +					cpu = <&cpu0>;
> >> > +				};
> >> > +				core1 {
> >> > +					cpu = <&cpu1>;
> >> > +				};
> >> > +				core2 {
> >> > +					cpu = <&cpu2>;
> >> > +				};
> >> > +				core3 {
> >> > +					cpu = <&cpu3>;
> >> > +				};
> >> > +			};
> >> > +		};
> >> > +
> >> > +		cpu0: cpu@0 {
> >> > +			device_type = "cpu";
> >> > +			compatible = "arm,cortex-a53";
> >> > +			reg = <0x0>;
> >> > +			enable-method = "psci";
> >> > +		};
> >> > +
> >> > +		cpu1: cpu@1 {
> >> > +			device_type = "cpu";
> >> > +			compatible = "arm,cortex-a53";
> >> > +			reg = <0x1>;
> >> > +			enable-method = "psci";
> >> > +		};
> >> > +
> >> > +		cpu2: cpu@2 {
> >> > +			device_type = "cpu";
> >> > +			compatible = "arm,cortex-a53";
> >> > +			reg = <0x2>;
> >> > +			enable-method = "psci";
> >> > +		};
> >> > +
> >> > +		cpu3: cpu@3 {
> >> > +			device_type = "cpu";
> >> > +			compatible = "arm,cortex-a53";
> >> > +			reg = <0x3>;
> >> > +			enable-method = "psci";
> >> > +		};
> >> > +	};
> >> > +
> >> > +	psci {
> >> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> >> > +		method = "smc";
> >> > +	};
> >> > +
> >> > +	timer {
> >> > +		compatible = "arm,armv8-timer";
> >> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> >> > +	};
> >> > +
> >> > +	xin24m: xin24m {
> >> 
> >> Please use name for all fixed clocks which matches current format
> >> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
> > 
> > Will be fixed in next revision.
> 
> Hmm, why should we apply that rule to the xin24m clock, which is
> named exactly like that everywhere else in Rockchip SoC dtsi files?
> It's much better to remain consistent.

bindings or how we write devicetrees evolve over time ... similarly the
xin24m name comes from more than 10 years ago.

We also name all those regulator nodes regulator-foo now, which in turn
automatically does enforce a nice sorting rule to keep all the regulators
around the same area ;-)

So I don't see a problem of going with xin24m: clock-xin24m {}


Heiko

> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
> >> 
> >> > +		compatible = "fixed-clock";
> >> > +		#clock-cells = <0>;
> >> > +		clock-frequency = <24000000>;
> >> > +		clock-output-names = "xin24m";
> >> > +	};
> >> > +
> >> > +	gic: interrupt-controller@fed01000 {
> >> 
> >> Why this all is outside of SoC?
> > 
> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> > a "soc" node. I didn't know why before but just follow the style.
> > 
> > If you prefer add a soc node, I am willing to.
>
Yao Zi Aug. 4, 2024, 1:58 p.m. UTC | #8
On Sun, Aug 04, 2024 at 03:25:47PM +0200, Dragan Simic wrote:
> On 2024-08-04 15:20, Yao Zi wrote:
> > On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
> > > On 03/08/2024 14:55, Yao Zi wrote:
> > > > This initial device tree describes CPU, interrupts and UART on the chip
> > > > and is able to boot into basic kernel with only UART. Cache information
> > > > is omitted for now as there is no precise documentation. Support for
> > > > other features will be added later.
> > > >
> > > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > > ---
> > > >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
> > > >  1 file changed, 182 insertions(+)
> > > >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > > > new file mode 100644
> > > > index 000000000000..77687d9e7e80
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > > > @@ -0,0 +1,182 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +/*
> > > > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> > > > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
> > > > + */
> > > > +
> > > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +#include <dt-bindings/interrupt-controller/irq.h>
> > > > +
> > > > +/ {
> > > > +	compatible = "rockchip,rk3528";
> > > > +
> > > > +	interrupt-parent = <&gic>;
> > > > +	#address-cells = <2>;
> > > > +	#size-cells = <2>;
> > > > +
> > > > +	aliases {
> > > > +		serial0 = &uart0;
> > > > +		serial1 = &uart1;
> > > > +		serial2 = &uart2;
> > > > +		serial3 = &uart3;
> > > > +		serial4 = &uart4;
> > > > +		serial5 = &uart5;
> > > > +		serial6 = &uart6;
> > > > +		serial7 = &uart7;
> > > > +	};
> > > > +
> > > > +	cpus {
> > > > +		#address-cells = <1>;
> > > > +		#size-cells = <0>;
> > > > +
> > > > +		cpu-map {
> > > > +			cluster0 {
> > > > +				core0 {
> > > > +					cpu = <&cpu0>;
> > > > +				};
> > > > +				core1 {
> > > > +					cpu = <&cpu1>;
> > > > +				};
> > > > +				core2 {
> > > > +					cpu = <&cpu2>;
> > > > +				};
> > > > +				core3 {
> > > > +					cpu = <&cpu3>;
> > > > +				};
> > > > +			};
> > > > +		};
> > > > +
> > > > +		cpu0: cpu@0 {
> > > > +			device_type = "cpu";
> > > > +			compatible = "arm,cortex-a53";
> > > > +			reg = <0x0>;
> > > > +			enable-method = "psci";
> > > > +		};
> > > > +
> > > > +		cpu1: cpu@1 {
> > > > +			device_type = "cpu";
> > > > +			compatible = "arm,cortex-a53";
> > > > +			reg = <0x1>;
> > > > +			enable-method = "psci";
> > > > +		};
> > > > +
> > > > +		cpu2: cpu@2 {
> > > > +			device_type = "cpu";
> > > > +			compatible = "arm,cortex-a53";
> > > > +			reg = <0x2>;
> > > > +			enable-method = "psci";
> > > > +		};
> > > > +
> > > > +		cpu3: cpu@3 {
> > > > +			device_type = "cpu";
> > > > +			compatible = "arm,cortex-a53";
> > > > +			reg = <0x3>;
> > > > +			enable-method = "psci";
> > > > +		};
> > > > +	};
> > > > +
> > > > +	psci {
> > > > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> > > > +		method = "smc";
> > > > +	};
> > > > +
> > > > +	timer {
> > > > +		compatible = "arm,armv8-timer";
> > > > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > > > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > > > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > > > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> > > > +	};
> > > > +
> > > > +	xin24m: xin24m {
> > > 
> > > Please use name for all fixed clocks which matches current format
> > > recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
> > 
> > Will be fixed in next revision.
> 
> Hmm, why should we apply that rule to the xin24m clock, which is
> named exactly like that everywhere else in Rockchip SoC dtsi files?
> It's much better to remain consistent.

Historical reasons should not affect new code. And if we don't follow
the new rules now, they may never get followed.

Best regards,
Yao Zi

> 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
> > > 
> > > > +		compatible = "fixed-clock";
> > > > +		#clock-cells = <0>;
> > > > +		clock-frequency = <24000000>;
> > > > +		clock-output-names = "xin24m";
> > > > +	};
> > > > +
> > > > +	gic: interrupt-controller@fed01000 {
> > > 
> > > Why this all is outside of SoC?
> > 
> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> > a "soc" node. I didn't know why before but just follow the style.
> > 
> > If you prefer add a soc node, I am willing to.
Dragan Simic Aug. 4, 2024, 1:59 p.m. UTC | #9
On 2024-08-04 15:44, Heiko Stübner wrote:
> Am Sonntag, 4. August 2024, 15:25:47 CEST schrieb Dragan Simic:
>> On 2024-08-04 15:20, Yao Zi wrote:
>> > On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
>> >> On 03/08/2024 14:55, Yao Zi wrote:
>> >> > This initial device tree describes CPU, interrupts and UART on the chip
>> >> > and is able to boot into basic kernel with only UART. Cache information
>> >> > is omitted for now as there is no precise documentation. Support for
>> >> > other features will be added later.
>> >> >
>> >> > Signed-off-by: Yao Zi <ziyao@disroot.org>
>> >> > ---
>> >> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
>> >> >  1 file changed, 182 insertions(+)
>> >> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> >> >
>> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> >> > new file mode 100644
>> >> > index 000000000000..77687d9e7e80
>> >> > --- /dev/null
>> >> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>> >> > @@ -0,0 +1,182 @@
>> >> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> >> > +/*
>> >> > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
>> >> > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
>> >> > + */
>> >> > +
>> >> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> >> > +#include <dt-bindings/interrupt-controller/irq.h>
>> >> > +
>> >> > +/ {
>> >> > +	compatible = "rockchip,rk3528";
>> >> > +
>> >> > +	interrupt-parent = <&gic>;
>> >> > +	#address-cells = <2>;
>> >> > +	#size-cells = <2>;
>> >> > +
>> >> > +	aliases {
>> >> > +		serial0 = &uart0;
>> >> > +		serial1 = &uart1;
>> >> > +		serial2 = &uart2;
>> >> > +		serial3 = &uart3;
>> >> > +		serial4 = &uart4;
>> >> > +		serial5 = &uart5;
>> >> > +		serial6 = &uart6;
>> >> > +		serial7 = &uart7;
>> >> > +	};
>> >> > +
>> >> > +	cpus {
>> >> > +		#address-cells = <1>;
>> >> > +		#size-cells = <0>;
>> >> > +
>> >> > +		cpu-map {
>> >> > +			cluster0 {
>> >> > +				core0 {
>> >> > +					cpu = <&cpu0>;
>> >> > +				};
>> >> > +				core1 {
>> >> > +					cpu = <&cpu1>;
>> >> > +				};
>> >> > +				core2 {
>> >> > +					cpu = <&cpu2>;
>> >> > +				};
>> >> > +				core3 {
>> >> > +					cpu = <&cpu3>;
>> >> > +				};
>> >> > +			};
>> >> > +		};
>> >> > +
>> >> > +		cpu0: cpu@0 {
>> >> > +			device_type = "cpu";
>> >> > +			compatible = "arm,cortex-a53";
>> >> > +			reg = <0x0>;
>> >> > +			enable-method = "psci";
>> >> > +		};
>> >> > +
>> >> > +		cpu1: cpu@1 {
>> >> > +			device_type = "cpu";
>> >> > +			compatible = "arm,cortex-a53";
>> >> > +			reg = <0x1>;
>> >> > +			enable-method = "psci";
>> >> > +		};
>> >> > +
>> >> > +		cpu2: cpu@2 {
>> >> > +			device_type = "cpu";
>> >> > +			compatible = "arm,cortex-a53";
>> >> > +			reg = <0x2>;
>> >> > +			enable-method = "psci";
>> >> > +		};
>> >> > +
>> >> > +		cpu3: cpu@3 {
>> >> > +			device_type = "cpu";
>> >> > +			compatible = "arm,cortex-a53";
>> >> > +			reg = <0x3>;
>> >> > +			enable-method = "psci";
>> >> > +		};
>> >> > +	};
>> >> > +
>> >> > +	psci {
>> >> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
>> >> > +		method = "smc";
>> >> > +	};
>> >> > +
>> >> > +	timer {
>> >> > +		compatible = "arm,armv8-timer";
>> >> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> >> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> >> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> >> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> >> > +	};
>> >> > +
>> >> > +	xin24m: xin24m {
>> >>
>> >> Please use name for all fixed clocks which matches current format
>> >> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
>> >
>> > Will be fixed in next revision.
>> 
>> Hmm, why should we apply that rule to the xin24m clock, which is
>> named exactly like that everywhere else in Rockchip SoC dtsi files?
>> It's much better to remain consistent.
> 
> bindings or how we write devicetrees evolve over time ... similarly the
> xin24m name comes from more than 10 years ago.
> 
> We also name all those regulator nodes regulator-foo now, which in turn
> automatically does enforce a nice sorting rule to keep all the 
> regulators
> around the same area ;-)
> 
> So I don't see a problem of going with xin24m: clock-xin24m {}

I agree that using "clock-xin24m" makes more sense in general, but the
trouble is that we can't rename the already existing instances of 
"xin24m",
because that has become part of the ABI.  Thus, I'm not sure that 
breaking
away from the legacy brings benefits in this particular case.

>> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
>> >>
>> >> > +		compatible = "fixed-clock";
>> >> > +		#clock-cells = <0>;
>> >> > +		clock-frequency = <24000000>;
>> >> > +		clock-output-names = "xin24m";
>> >> > +	};
>> >> > +
>> >> > +	gic: interrupt-controller@fed01000 {
>> >>
>> >> Why this all is outside of SoC?
>> >
>> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
>> > a "soc" node. I didn't know why before but just follow the style.
>> >
>> > If you prefer add a soc node, I am willing to.
>> 
> 
> 
> 
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Krzysztof Kozlowski Aug. 4, 2024, 2:05 p.m. UTC | #10
On 04/08/2024 15:20, Yao Zi wrote:
>>
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>> +		clock-output-names = "xin24m";
>>> +	};
>>> +
>>> +	gic: interrupt-controller@fed01000 {
>>
>> Why this all is outside of SoC?
> 
> Just as Heiko says, device tree for all other Rockchip SoCs don't have
> a "soc" node. I didn't know why before but just follow the style.
> 
> If you prefer add a soc node, I am willing to.

Surprising as usually we expect MMIO nodes being part of SoC to be under
soc@, but if that's Rockchip preference then fine.

Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 4, 2024, 2:09 p.m. UTC | #11
On 04/08/2024 14:49, Heiko Stübner wrote:
> 
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>> +		clock-output-names = "xin24m";
>>> +	};
>>> +
>>> +	gic: interrupt-controller@fed01000 {
>>
>> Why this all is outside of SoC?
> 
> I guess you mean outside of a "soc {}" node?
> 
> Here the rk3528 simply follows all other Rockchip SoCs :-) .
> 
> Digging into the history, the first rk3066a and initial rk3288 submission
> did use a soc {} node, which later got removed as suggested by arm-soc
> maintainers at the time [0].
> 
> I guess that changed since then?

Well, referenced patch was mixing MMIO with non-MMIO, so Olof's comment
could be understood that this is not correct approach. Even though DT
spec shows examples of "soc", it is not required. But then how do you
implement any ordering? By name or by unit address?

IOW, I think this is the only platform not using "soc" nodes.

Best regards,
Krzysztof
Heiko Stübner Aug. 4, 2024, 3:51 p.m. UTC | #12
Am Sonntag, 4. August 2024, 15:59:19 CEST schrieb Dragan Simic:
> On 2024-08-04 15:44, Heiko Stübner wrote:
> > Am Sonntag, 4. August 2024, 15:25:47 CEST schrieb Dragan Simic:
> >> On 2024-08-04 15:20, Yao Zi wrote:
> >> > On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
> >> >> On 03/08/2024 14:55, Yao Zi wrote:
> >> >> > This initial device tree describes CPU, interrupts and UART on the chip
> >> >> > and is able to boot into basic kernel with only UART. Cache information
> >> >> > is omitted for now as there is no precise documentation. Support for
> >> >> > other features will be added later.
> >> >> >
> >> >> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> >> >> > ---
> >> >> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
> >> >> >  1 file changed, 182 insertions(+)
> >> >> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> >> >
> >> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> >> > new file mode 100644
> >> >> > index 000000000000..77687d9e7e80
> >> >> > --- /dev/null
> >> >> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> >> >> > @@ -0,0 +1,182 @@
> >> >> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> >> > +/*
> >> >> > + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> >> >> > + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
> >> >> > + */
> >> >> > +
> >> >> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >> >> > +#include <dt-bindings/interrupt-controller/irq.h>
> >> >> > +
> >> >> > +/ {
> >> >> > +	compatible = "rockchip,rk3528";
> >> >> > +
> >> >> > +	interrupt-parent = <&gic>;
> >> >> > +	#address-cells = <2>;
> >> >> > +	#size-cells = <2>;
> >> >> > +
> >> >> > +	aliases {
> >> >> > +		serial0 = &uart0;
> >> >> > +		serial1 = &uart1;
> >> >> > +		serial2 = &uart2;
> >> >> > +		serial3 = &uart3;
> >> >> > +		serial4 = &uart4;
> >> >> > +		serial5 = &uart5;
> >> >> > +		serial6 = &uart6;
> >> >> > +		serial7 = &uart7;
> >> >> > +	};
> >> >> > +
> >> >> > +	cpus {
> >> >> > +		#address-cells = <1>;
> >> >> > +		#size-cells = <0>;
> >> >> > +
> >> >> > +		cpu-map {
> >> >> > +			cluster0 {
> >> >> > +				core0 {
> >> >> > +					cpu = <&cpu0>;
> >> >> > +				};
> >> >> > +				core1 {
> >> >> > +					cpu = <&cpu1>;
> >> >> > +				};
> >> >> > +				core2 {
> >> >> > +					cpu = <&cpu2>;
> >> >> > +				};
> >> >> > +				core3 {
> >> >> > +					cpu = <&cpu3>;
> >> >> > +				};
> >> >> > +			};
> >> >> > +		};
> >> >> > +
> >> >> > +		cpu0: cpu@0 {
> >> >> > +			device_type = "cpu";
> >> >> > +			compatible = "arm,cortex-a53";
> >> >> > +			reg = <0x0>;
> >> >> > +			enable-method = "psci";
> >> >> > +		};
> >> >> > +
> >> >> > +		cpu1: cpu@1 {
> >> >> > +			device_type = "cpu";
> >> >> > +			compatible = "arm,cortex-a53";
> >> >> > +			reg = <0x1>;
> >> >> > +			enable-method = "psci";
> >> >> > +		};
> >> >> > +
> >> >> > +		cpu2: cpu@2 {
> >> >> > +			device_type = "cpu";
> >> >> > +			compatible = "arm,cortex-a53";
> >> >> > +			reg = <0x2>;
> >> >> > +			enable-method = "psci";
> >> >> > +		};
> >> >> > +
> >> >> > +		cpu3: cpu@3 {
> >> >> > +			device_type = "cpu";
> >> >> > +			compatible = "arm,cortex-a53";
> >> >> > +			reg = <0x3>;
> >> >> > +			enable-method = "psci";
> >> >> > +		};
> >> >> > +	};
> >> >> > +
> >> >> > +	psci {
> >> >> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> >> >> > +		method = "smc";
> >> >> > +	};
> >> >> > +
> >> >> > +	timer {
> >> >> > +		compatible = "arm,armv8-timer";
> >> >> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> >> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> >> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> >> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> >> >> > +	};
> >> >> > +
> >> >> > +	xin24m: xin24m {
> >> >>
> >> >> Please use name for all fixed clocks which matches current format
> >> >> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
> >> >
> >> > Will be fixed in next revision.
> >> 
> >> Hmm, why should we apply that rule to the xin24m clock, which is
> >> named exactly like that everywhere else in Rockchip SoC dtsi files?
> >> It's much better to remain consistent.
> > 
> > bindings or how we write devicetrees evolve over time ... similarly the
> > xin24m name comes from more than 10 years ago.
> > 
> > We also name all those regulator nodes regulator-foo now, which in turn
> > automatically does enforce a nice sorting rule to keep all the 
> > regulators
> > around the same area ;-)
> > 
> > So I don't see a problem of going with xin24m: clock-xin24m {}
> 
> I agree that using "clock-xin24m" makes more sense in general, but the
> trouble is that we can't rename the already existing instances of 
> "xin24m",
> because that has become part of the ABI.  Thus, I'm not sure that 
> breaking
> away from the legacy brings benefits in this particular case.

In the regulator case, we have _new_ boards using the new _node_-names
but I don't see any renaming of old boards and also don't think we should.

But that still does not keep us from using the nicer naming convention in
new boards ;-) .


Same with xin24m. We're talking only about the node-name here. The
phandle stays the same and also the actual clock name stays the same and
really only the actual node name you need to look for in /proc/device-tree
changes ;-) .

So I don't see the need to go about changing all the old socs, but new
additions should use improved naming conventions.

xin24m: clock-xin24m {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <24000000>;
	clock-output-names = "xin24m";
};


Heiko



> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
> >> >>
> >> >> > +		compatible = "fixed-clock";
> >> >> > +		#clock-cells = <0>;
> >> >> > +		clock-frequency = <24000000>;
> >> >> > +		clock-output-names = "xin24m";
> >> >> > +	};
> >> >> > +
> >> >> > +	gic: interrupt-controller@fed01000 {
> >> >>
> >> >> Why this all is outside of SoC?
> >> >
> >> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> >> > a "soc" node. I didn't know why before but just follow the style.
> >> >
> >> > If you prefer add a soc node, I am willing to.
> >> 
> > 
> > 
> > 
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
Krzysztof Kozlowski Aug. 5, 2024, 5:16 a.m. UTC | #13
On 04/08/2024 17:51, Heiko Stübner wrote:
> Am Sonntag, 4. August 2024, 15:59:19 CEST schrieb Dragan Simic:
>> On 2024-08-04 15:44, Heiko Stübner wrote:
>>> Am Sonntag, 4. August 2024, 15:25:47 CEST schrieb Dragan Simic:
>>>> On 2024-08-04 15:20, Yao Zi wrote:
>>>>> On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 03/08/2024 14:55, Yao Zi wrote:
>>>>>>> This initial device tree describes CPU, interrupts and UART on the chip
>>>>>>> and is able to boot into basic kernel with only UART. Cache information
>>>>>>> is omitted for now as there is no precise documentation. Support for
>>>>>>> other features will be added later.
>>>>>>>
>>>>>>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 182 +++++++++++++++++++++++
>>>>>>>  1 file changed, 182 insertions(+)
>>>>>>>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..77687d9e7e80
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>>>>>> @@ -0,0 +1,182 @@
>>>>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
>>>>>>> + * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>>>>>> +
>>>>>>> +/ {
>>>>>>> +	compatible = "rockchip,rk3528";
>>>>>>> +
>>>>>>> +	interrupt-parent = <&gic>;
>>>>>>> +	#address-cells = <2>;
>>>>>>> +	#size-cells = <2>;
>>>>>>> +
>>>>>>> +	aliases {
>>>>>>> +		serial0 = &uart0;
>>>>>>> +		serial1 = &uart1;
>>>>>>> +		serial2 = &uart2;
>>>>>>> +		serial3 = &uart3;
>>>>>>> +		serial4 = &uart4;
>>>>>>> +		serial5 = &uart5;
>>>>>>> +		serial6 = &uart6;
>>>>>>> +		serial7 = &uart7;
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	cpus {
>>>>>>> +		#address-cells = <1>;
>>>>>>> +		#size-cells = <0>;
>>>>>>> +
>>>>>>> +		cpu-map {
>>>>>>> +			cluster0 {
>>>>>>> +				core0 {
>>>>>>> +					cpu = <&cpu0>;
>>>>>>> +				};
>>>>>>> +				core1 {
>>>>>>> +					cpu = <&cpu1>;
>>>>>>> +				};
>>>>>>> +				core2 {
>>>>>>> +					cpu = <&cpu2>;
>>>>>>> +				};
>>>>>>> +				core3 {
>>>>>>> +					cpu = <&cpu3>;
>>>>>>> +				};
>>>>>>> +			};
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu0: cpu@0 {
>>>>>>> +			device_type = "cpu";
>>>>>>> +			compatible = "arm,cortex-a53";
>>>>>>> +			reg = <0x0>;
>>>>>>> +			enable-method = "psci";
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu1: cpu@1 {
>>>>>>> +			device_type = "cpu";
>>>>>>> +			compatible = "arm,cortex-a53";
>>>>>>> +			reg = <0x1>;
>>>>>>> +			enable-method = "psci";
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu2: cpu@2 {
>>>>>>> +			device_type = "cpu";
>>>>>>> +			compatible = "arm,cortex-a53";
>>>>>>> +			reg = <0x2>;
>>>>>>> +			enable-method = "psci";
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		cpu3: cpu@3 {
>>>>>>> +			device_type = "cpu";
>>>>>>> +			compatible = "arm,cortex-a53";
>>>>>>> +			reg = <0x3>;
>>>>>>> +			enable-method = "psci";
>>>>>>> +		};
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	psci {
>>>>>>> +		compatible = "arm,psci-1.0", "arm,psci-0.2";
>>>>>>> +		method = "smc";
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	timer {
>>>>>>> +		compatible = "arm,armv8-timer";
>>>>>>> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>>>>>> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>>>>>> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>>>>>> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	xin24m: xin24m {
>>>>>>
>>>>>> Please use name for all fixed clocks which matches current format
>>>>>> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
>>>>>
>>>>> Will be fixed in next revision.
>>>>
>>>> Hmm, why should we apply that rule to the xin24m clock, which is
>>>> named exactly like that everywhere else in Rockchip SoC dtsi files?
>>>> It's much better to remain consistent.
>>>
>>> bindings or how we write devicetrees evolve over time ... similarly the
>>> xin24m name comes from more than 10 years ago.
>>>
>>> We also name all those regulator nodes regulator-foo now, which in turn
>>> automatically does enforce a nice sorting rule to keep all the 
>>> regulators
>>> around the same area ;-)
>>>
>>> So I don't see a problem of going with xin24m: clock-xin24m {}
>>
>> I agree that using "clock-xin24m" makes more sense in general, but the
>> trouble is that we can't rename the already existing instances of 
>> "xin24m",
>> because that has become part of the ABI.  Thus, I'm not sure that 
>> breaking
>> away from the legacy brings benefits in this particular case.
> 
> In the regulator case, we have _new_ boards using the new _node_-names
> but I don't see any renaming of old boards and also don't think we should.
> 
> But that still does not keep us from using the nicer naming convention in
> new boards ;-) .
> 
> 
> Same with xin24m. We're talking only about the node-name here. The
> phandle stays the same and also the actual clock name stays the same and
> really only the actual node name you need to look for in /proc/device-tree
> changes ;-) .
> 
> So I don't see the need to go about changing all the old socs, but new
> additions should use improved naming conventions.
> 
> xin24m: clock-xin24m {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <24000000>;
> 	clock-output-names = "xin24m";

Just to make it clear - doc clearly says the preferred name is:
"clock-frequency".

Best regards,
Krzysztof
Dragan Simic Aug. 5, 2024, 5:22 a.m. UTC | #14
On 2024-08-04 17:51, Heiko Stübner wrote:
> Am Sonntag, 4. August 2024, 15:59:19 CEST schrieb Dragan Simic:
>> On 2024-08-04 15:44, Heiko Stübner wrote:
>> > Am Sonntag, 4. August 2024, 15:25:47 CEST schrieb Dragan Simic:
>> >> On 2024-08-04 15:20, Yao Zi wrote:
>> >> > On Sun, Aug 04, 2024 at 12:05:11PM +0200, Krzysztof Kozlowski wrote:
>> >> >> On 03/08/2024 14:55, Yao Zi wrote:
>> >> >> > +	xin24m: xin24m {
>> >> >>
>> >> >> Please use name for all fixed clocks which matches current format
>> >> >> recommendation: 'clock-([0-9]+|[a-z0-9-]+)+'
>> >> >
>> >> > Will be fixed in next revision.
>> >>
>> >> Hmm, why should we apply that rule to the xin24m clock, which is
>> >> named exactly like that everywhere else in Rockchip SoC dtsi files?
>> >> It's much better to remain consistent.
>> >
>> > bindings or how we write devicetrees evolve over time ... similarly the
>> > xin24m name comes from more than 10 years ago.
>> >
>> > We also name all those regulator nodes regulator-foo now, which in turn
>> > automatically does enforce a nice sorting rule to keep all the
>> > regulators
>> > around the same area ;-)
>> >
>> > So I don't see a problem of going with xin24m: clock-xin24m {}
>> 
>> I agree that using "clock-xin24m" makes more sense in general, but the
>> trouble is that we can't rename the already existing instances of
>> "xin24m",
>> because that has become part of the ABI.  Thus, I'm not sure that
>> breaking
>> away from the legacy brings benefits in this particular case.
> 
> In the regulator case, we have _new_ boards using the new _node_-names
> but I don't see any renaming of old boards and also don't think we 
> should.
> 
> But that still does not keep us from using the nicer naming convention 
> in
> new boards ;-) .
> 
> Same with xin24m. We're talking only about the node-name here. The
> phandle stays the same and also the actual clock name stays the same 
> and
> really only the actual node name you need to look for in 
> /proc/device-tree
> changes ;-) .
> 
> So I don't see the need to go about changing all the old socs, but new
> additions should use improved naming conventions.
> 
> xin24m: clock-xin24m {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <24000000>;
> 	clock-output-names = "xin24m";
> };

Makes sense.  Though, updating the dtsi files for older SoCs to follow
the new rules, if possible, still remains an itch to me. :)
Yao Zi Aug. 5, 2024, 10:59 a.m. UTC | #15
On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
> On 04/08/2024 15:20, Yao Zi wrote:
> >>
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <24000000>;
> >>> +		clock-output-names = "xin24m";
> >>> +	};
> >>> +
> >>> +	gic: interrupt-controller@fed01000 {
> >>
> >> Why this all is outside of SoC?
> > 
> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> > a "soc" node. I didn't know why before but just follow the style.
> > 
> > If you prefer add a soc node, I am willing to.
> 
> Surprising as usually we expect MMIO nodes being part of SoC to be under
> soc@, but if that's Rockchip preference then fine.
> 

Okay, then I would leave it as is.

For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
follows the new rule?

Best regards,
Yao Zi
Dragan Simic Aug. 5, 2024, 11:37 a.m. UTC | #16
On 2024-08-05 12:59, Yao Zi wrote:
> On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
>> On 04/08/2024 15:20, Yao Zi wrote:
>> >>
>> >>> +		compatible = "fixed-clock";
>> >>> +		#clock-cells = <0>;
>> >>> +		clock-frequency = <24000000>;
>> >>> +		clock-output-names = "xin24m";
>> >>> +	};
>> >>> +
>> >>> +	gic: interrupt-controller@fed01000 {
>> >>
>> >> Why this all is outside of SoC?
>> >
>> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
>> > a "soc" node. I didn't know why before but just follow the style.
>> >
>> > If you prefer add a soc node, I am willing to.
>> 
>> Surprising as usually we expect MMIO nodes being part of SoC to be 
>> under
>> soc@, but if that's Rockchip preference then fine.
>> 
> 
> Okay, then I would leave it as is.
> 
> For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
> follows the new rule?

I find "xin24m: clock-xin24m { }" better, because keeping the "xin24m"
part in /sys listing(s), for example, can only be helpful.
Heiko Stübner Aug. 5, 2024, 11:47 a.m. UTC | #17
Am Montag, 5. August 2024, 13:37:11 CEST schrieb Dragan Simic:
> On 2024-08-05 12:59, Yao Zi wrote:
> > On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
> >> On 04/08/2024 15:20, Yao Zi wrote:
> >> >>
> >> >>> +		compatible = "fixed-clock";
> >> >>> +		#clock-cells = <0>;
> >> >>> +		clock-frequency = <24000000>;
> >> >>> +		clock-output-names = "xin24m";
> >> >>> +	};
> >> >>> +
> >> >>> +	gic: interrupt-controller@fed01000 {
> >> >>
> >> >> Why this all is outside of SoC?
> >> >
> >> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> >> > a "soc" node. I didn't know why before but just follow the style.
> >> >
> >> > If you prefer add a soc node, I am willing to.
> >> 
> >> Surprising as usually we expect MMIO nodes being part of SoC to be 
> >> under
> >> soc@, but if that's Rockchip preference then fine.
> >> 
> > 
> > Okay, then I would leave it as is.
> > 
> > For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
> > follows the new rule?
> 
> I find "xin24m: clock-xin24m { }" better, because keeping the "xin24m"
> part in /sys listing(s), for example, can only be helpful.

I would second that :-) . Like on a number of boards we have for example
125MHz gmac clock generators ... with 2 gmacs, there are 2 of them.

I'm not sure the preferred name accounts for that?

Similarly we also keep the naming in the regulator node,
it's regulator-vcc3v3-somename {} instead of just regulator-3v3 {}.
Yao Zi Aug. 5, 2024, 4:22 p.m. UTC | #18
On Mon, Aug 05, 2024 at 01:47:45PM +0200, Heiko Stübner wrote:
> Am Montag, 5. August 2024, 13:37:11 CEST schrieb Dragan Simic:
> > On 2024-08-05 12:59, Yao Zi wrote:
> > > On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
> > >> On 04/08/2024 15:20, Yao Zi wrote:
> > >> >>
> > >> >>> +		compatible = "fixed-clock";
> > >> >>> +		#clock-cells = <0>;
> > >> >>> +		clock-frequency = <24000000>;
> > >> >>> +		clock-output-names = "xin24m";
> > >> >>> +	};
> > >> >>> +
> > >> >>> +	gic: interrupt-controller@fed01000 {
> > >> >>
> > >> >> Why this all is outside of SoC?
> > >> >
> > >> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
> > >> > a "soc" node. I didn't know why before but just follow the style.
> > >> >
> > >> > If you prefer add a soc node, I am willing to.
> > >> 
> > >> Surprising as usually we expect MMIO nodes being part of SoC to be 
> > >> under
> > >> soc@, but if that's Rockchip preference then fine.
> > >> 
> > > 
> > > Okay, then I would leave it as is.
> > > 
> > > For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
> > > follows the new rule?
> > 
> > I find "xin24m: clock-xin24m { }" better, because keeping the "xin24m"
> > part in /sys listing(s), for example, can only be helpful.
> 
> I would second that :-) . Like on a number of boards we have for example
> 125MHz gmac clock generators ... with 2 gmacs, there are 2 of them.
> 
> I'm not sure the preferred name accounts for that?
> 
> Similarly we also keep the naming in the regulator node,
> it's regulator-vcc3v3-somename {} instead of just regulator-3v3 {}.
> 

"clock-xin24m" wouldn't be more descriptive than "clock-24m" and there
are usually only a few fixed clocks in dt, thus finding corresponding
definition isn't a problem I think.

For the gmac case, Krzysztof, do you think something like
"clock-125m-gmac1" is acceptable, just like what has been done for
regulators?

Best regards,
Yao Zi
Krzysztof Kozlowski Aug. 5, 2024, 5:02 p.m. UTC | #19
On 05/08/2024 18:22, Yao Zi wrote:
> On Mon, Aug 05, 2024 at 01:47:45PM +0200, Heiko Stübner wrote:
>> Am Montag, 5. August 2024, 13:37:11 CEST schrieb Dragan Simic:
>>> On 2024-08-05 12:59, Yao Zi wrote:
>>>> On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
>>>>> On 04/08/2024 15:20, Yao Zi wrote:
>>>>>>>
>>>>>>>> +		compatible = "fixed-clock";
>>>>>>>> +		#clock-cells = <0>;
>>>>>>>> +		clock-frequency = <24000000>;
>>>>>>>> +		clock-output-names = "xin24m";
>>>>>>>> +	};
>>>>>>>> +
>>>>>>>> +	gic: interrupt-controller@fed01000 {
>>>>>>>
>>>>>>> Why this all is outside of SoC?
>>>>>>
>>>>>> Just as Heiko says, device tree for all other Rockchip SoCs don't have
>>>>>> a "soc" node. I didn't know why before but just follow the style.
>>>>>>
>>>>>> If you prefer add a soc node, I am willing to.
>>>>>
>>>>> Surprising as usually we expect MMIO nodes being part of SoC to be 
>>>>> under
>>>>> soc@, but if that's Rockchip preference then fine.
>>>>>
>>>>
>>>> Okay, then I would leave it as is.
>>>>
>>>> For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
>>>> follows the new rule?
>>>
>>> I find "xin24m: clock-xin24m { }" better, because keeping the "xin24m"
>>> part in /sys listing(s), for example, can only be helpful.
>>
>> I would second that :-) . Like on a number of boards we have for example
>> 125MHz gmac clock generators ... with 2 gmacs, there are 2 of them.
>>
>> I'm not sure the preferred name accounts for that?
>>
>> Similarly we also keep the naming in the regulator node,
>> it's regulator-vcc3v3-somename {} instead of just regulator-3v3 {}.
>>
> 
> "clock-xin24m" wouldn't be more descriptive than "clock-24m" and there
> are usually only a few fixed clocks in dt, thus finding corresponding
> definition isn't a problem I think.
> 
> For the gmac case, Krzysztof, do you think something like
> "clock-125m-gmac1" is acceptable, just like what has been done for
> regulators?
>

How both above fit the generic node naming rule? You add now specific
part - purpose of the clock. The purpose is obvious from
clock-output-names or label. Anyway, not important topic to nak these
patches.

Best regards,
Krzysztof
Dragan Simic Aug. 5, 2024, 7:13 p.m. UTC | #20
Hello Yao,

On 2024-08-05 18:22, Yao Zi wrote:
> On Mon, Aug 05, 2024 at 01:47:45PM +0200, Heiko Stübner wrote:
>> Am Montag, 5. August 2024, 13:37:11 CEST schrieb Dragan Simic:
>> > On 2024-08-05 12:59, Yao Zi wrote:
>> > > On Sun, Aug 04, 2024 at 04:05:24PM +0200, Krzysztof Kozlowski wrote:
>> > >> On 04/08/2024 15:20, Yao Zi wrote:
>> > >> >>
>> > >> >>> +		compatible = "fixed-clock";
>> > >> >>> +		#clock-cells = <0>;
>> > >> >>> +		clock-frequency = <24000000>;
>> > >> >>> +		clock-output-names = "xin24m";
>> > >> >>> +	};
>> > >> >>> +
>> > >> >>> +	gic: interrupt-controller@fed01000 {
>> > >> >>
>> > >> >> Why this all is outside of SoC?
>> > >> >
>> > >> > Just as Heiko says, device tree for all other Rockchip SoCs don't have
>> > >> > a "soc" node. I didn't know why before but just follow the style.
>> > >> >
>> > >> > If you prefer add a soc node, I am willing to.
>> > >>
>> > >> Surprising as usually we expect MMIO nodes being part of SoC to be
>> > >> under
>> > >> soc@, but if that's Rockchip preference then fine.
>> > >>
>> > >
>> > > Okay, then I would leave it as is.
>> > >
>> > > For the fixed-clock node, I think "xin24m: clock-24m { }" is okay and
>> > > follows the new rule?
>> >
>> > I find "xin24m: clock-xin24m { }" better, because keeping the "xin24m"
>> > part in /sys listing(s), for example, can only be helpful.
>> 
>> I would second that :-) . Like on a number of boards we have for 
>> example
>> 125MHz gmac clock generators ... with 2 gmacs, there are 2 of them.
>> 
>> I'm not sure the preferred name accounts for that?
>> 
>> Similarly we also keep the naming in the regulator node,
>> it's regulator-vcc3v3-somename {} instead of just regulator-3v3 {}.
> 
> "clock-xin24m" wouldn't be more descriptive than "clock-24m" and there
> are usually only a few fixed clocks in dt, thus finding corresponding
> definition isn't a problem I think.

Well, using "clock-xin24m" comes with another benefit, which is using
the same "xin24m" as in the actual clock name.  That way, the same clock
name gets used in various /sys listings and in the debug clock summary
in /sys.  Having that kind of consistency can only be beneficial.

> For the gmac case, Krzysztof, do you think something like
> "clock-125m-gmac1" is acceptable, just like what has been done for
> regulators?
Krzysztof Kozlowski Aug. 13, 2024, 4:38 p.m. UTC | #21
On 04/08/2024 16:05, Krzysztof Kozlowski wrote:
> On 04/08/2024 15:20, Yao Zi wrote:
>>>
>>>> +		compatible = "fixed-clock";
>>>> +		#clock-cells = <0>;
>>>> +		clock-frequency = <24000000>;
>>>> +		clock-output-names = "xin24m";
>>>> +	};
>>>> +
>>>> +	gic: interrupt-controller@fed01000 {
>>>
>>> Why this all is outside of SoC?
>>
>> Just as Heiko says, device tree for all other Rockchip SoCs don't have
>> a "soc" node. I didn't know why before but just follow the style.
>>
>> If you prefer add a soc node, I am willing to.
> 
> Surprising as usually we expect MMIO nodes being part of SoC to be under
> soc@, but if that's Rockchip preference then fine.

One more comment, I forgot we actually have it documented long time ago:

https://elixir.bootlin.com/linux/v6.11-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L90

Best regards,
Krzysztof
Heiko Stübner Aug. 14, 2024, 3:21 p.m. UTC | #22
Am Dienstag, 13. August 2024, 18:38:58 CEST schrieb Krzysztof Kozlowski:
> On 04/08/2024 16:05, Krzysztof Kozlowski wrote:
> > On 04/08/2024 15:20, Yao Zi wrote:
> >>>
> >>>> +		compatible = "fixed-clock";
> >>>> +		#clock-cells = <0>;
> >>>> +		clock-frequency = <24000000>;
> >>>> +		clock-output-names = "xin24m";
> >>>> +	};
> >>>> +
> >>>> +	gic: interrupt-controller@fed01000 {
> >>>
> >>> Why this all is outside of SoC?
> >>
> >> Just as Heiko says, device tree for all other Rockchip SoCs don't have
> >> a "soc" node. I didn't know why before but just follow the style.
> >>
> >> If you prefer add a soc node, I am willing to.
> > 
> > Surprising as usually we expect MMIO nodes being part of SoC to be under
> > soc@, but if that's Rockchip preference then fine.
> 
> One more comment, I forgot we actually have it documented long time ago:
> 
> https://elixir.bootlin.com/linux/v6.11-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L90

Thanks for finding that block.

I guess we'll follow that advice then and go with a soc node :-) .


Heiko
Heiko Stübner Aug. 15, 2024, 4:44 p.m. UTC | #23
Am Dienstag, 13. August 2024, 18:38:58 CEST schrieb Krzysztof Kozlowski:
> On 04/08/2024 16:05, Krzysztof Kozlowski wrote:
> > On 04/08/2024 15:20, Yao Zi wrote:
> >>>
> >>>> +		compatible = "fixed-clock";
> >>>> +		#clock-cells = <0>;
> >>>> +		clock-frequency = <24000000>;
> >>>> +		clock-output-names = "xin24m";
> >>>> +	};
> >>>> +
> >>>> +	gic: interrupt-controller@fed01000 {
> >>>
> >>> Why this all is outside of SoC?
> >>
> >> Just as Heiko says, device tree for all other Rockchip SoCs don't have
> >> a "soc" node. I didn't know why before but just follow the style.
> >>
> >> If you prefer add a soc node, I am willing to.
> > 
> > Surprising as usually we expect MMIO nodes being part of SoC to be under
> > soc@, but if that's Rockchip preference then fine.
> 
> One more comment, I forgot we actually have it documented long time ago:
> 
> https://elixir.bootlin.com/linux/v6.11-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L90

I guess that piece of documentation should move to the dts style
guide though? Because it's not about writing bindings but how
to structure a dts/dtsi.
Krzysztof Kozlowski Aug. 16, 2024, 5:57 a.m. UTC | #24
On 15/08/2024 18:44, Heiko Stübner wrote:
>>
>> One more comment, I forgot we actually have it documented long time ago:
>>
>> https://elixir.bootlin.com/linux/v6.11-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L90
> 
> I guess that piece of documentation should move to the dts style
> guide though? Because it's not about writing bindings but how
> to structure a dts/dtsi.

Yes, it should.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
new file mode 100644
index 000000000000..77687d9e7e80
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
@@ -0,0 +1,182 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
+ * Copyright (c) 2024 Yao Zi <ziyao@disroot.org>
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+	compatible = "rockchip,rk3528";
+
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+		serial3 = &uart3;
+		serial4 = &uart4;
+		serial5 = &uart5;
+		serial6 = &uart6;
+		serial7 = &uart7;
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&cpu0>;
+				};
+				core1 {
+					cpu = <&cpu1>;
+				};
+				core2 {
+					cpu = <&cpu2>;
+				};
+				core3 {
+					cpu = <&cpu3>;
+				};
+			};
+		};
+
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53";
+			reg = <0x0>;
+			enable-method = "psci";
+		};
+
+		cpu1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53";
+			reg = <0x1>;
+			enable-method = "psci";
+		};
+
+		cpu2: cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53";
+			reg = <0x2>;
+			enable-method = "psci";
+		};
+
+		cpu3: cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53";
+			reg = <0x3>;
+			enable-method = "psci";
+		};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0", "arm,psci-0.2";
+		method = "smc";
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+
+	xin24m: xin24m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+		clock-output-names = "xin24m";
+	};
+
+	gic: interrupt-controller@fed01000 {
+		compatible = "arm,gic-400";
+		#interrupt-cells = <3>;
+		#address-cells = <0>;
+		interrupt-controller;
+		reg = <0x0 0xfed01000 0 0x1000>,
+		      <0x0 0xfed02000 0 0x2000>,
+		      <0x0 0xfed04000 0 0x2000>,
+		      <0x0 0xfed06000 0 0x2000>;
+		interrupts = <GIC_PPI 9
+			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+
+	uart0: serial@ff9f0000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xff9f0000 0x0 0x100>;
+		interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		clock-frequency = <24000000>;
+		status = "disabled";
+	};
+
+	uart1: serial@ff9f8000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xff9f8000 0x0 0x100>;
+		interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart2: serial@ffa00000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa00000 0x0 0x100>;
+		interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart3: serial@ffa08000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa08000 0x0 0x100>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart4: serial@ffa10000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa10000 0x0 0x100>;
+		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart5: serial@ffa18000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa18000 0x0 0x100>;
+		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart6: serial@ffa20000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa20000 0x0 0x100>;
+		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	uart7: serial@ffa28000 {
+		compatible = "rockchip,rk3528-uart", "snps,dw-apb-uart";
+		reg = <0x0 0xffa28000 0x0 0x100>;
+		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+};