CIX SKY1 SoC is high performance Armv9 SoC designed by Cixtech,
and Orion O6 is open source motherboard launched by Radxa.
See below for detail:
https://docs.radxa.com/en/orion/o6/getting-started/introduction

In this commit, it only adds limited components for running initramfs
at Orion O6.

Acked-by: Fugang.duan <fugang.duan@cixtech.com>
Signed-off-by: Peter Chen <peter.chen@cixtech.com>
---
 arch/arm64/boot/dts/Makefile              |   1 +
 arch/arm64/boot/dts/cix/Makefile          |   2 +
 arch/arm64/boot/dts/cix/sky1-orion-o6.dts |  21 ++
 arch/arm64/boot/dts/cix/sky1.dtsi         | 264 ++++++++++++++++++++++
 4 files changed, 288 insertions(+)
 create mode 100644 arch/arm64/boot/dts/cix/Makefile
 create mode 100644 arch/arm64/boot/dts/cix/sky1-orion-o6.dts
 create mode 100644 arch/arm64/boot/dts/cix/sky1.dtsi

diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index 79b73a21ddc2..8e7ccd0027bd 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -13,6 +13,7 @@ subdir-y += bitmain
 subdir-y += blaize
 subdir-y += broadcom
 subdir-y += cavium
+subdir-y += cix
 subdir-y += exynos
 subdir-y += freescale
 subdir-y += hisilicon
diff --git a/arch/arm64/boot/dts/cix/Makefile b/arch/arm64/boot/dts/cix/Makefile
new file mode 100644
index 000000000000..ed3713982012
--- /dev/null
+++ b/arch/arm64/boot/dts/cix/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_CIX) += sky1-orion-o6.dtb
diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
new file mode 100644
index 000000000000..dbee1616076d
--- /dev/null
+++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright 2025 Cix Technology Group Co., Ltd.
+ *
+ */
+
+/dts-v1/;
+
+#include "sky1.dtsi"
+/ {
+	model = "Radxa Orion O6";
+	compatible = "radxa,orion-o6";
+
+	chosen {
+		stdout-path = &uart2;
+	};
+};
+
+&uart2 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
new file mode 100644
index 000000000000..d98735f782e0
--- /dev/null
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright 2025 Cix Technology Group Co., Ltd.
+ *
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	aliases {
+		serial2 = &uart2;
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&CPU0>;
+				};
+				core1 {
+					cpu = <&CPU1>;
+				};
+				core2 {
+					cpu = <&CPU2>;
+				};
+				core3 {
+					cpu = <&CPU3>;
+				};
+				core4 {
+					cpu = <&CPU4>;
+				};
+				core5 {
+					cpu = <&CPU5>;
+				};
+				core6 {
+					cpu = <&CPU6>;
+				};
+				core7 {
+					cpu = <&CPU7>;
+				};
+				core8 {
+					cpu = <&CPU8>;
+				};
+				core9 {
+					cpu = <&CPU9>;
+				};
+				core10 {
+					cpu = <&CPU10>;
+				};
+				core11 {
+					cpu = <&CPU11>;
+				};
+			};
+		};
+
+		CPU0: cpu0@0 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x0>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <403>;
+		};
+
+		CPU1: cpu1@100 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x100>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <403>;
+		};
+
+		CPU2: cpu2@200 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x200>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <403>;
+		};
+
+		CPU3: cpu3@300 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x300>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <403>;
+		};
+
+		CPU4: cpu4@400 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x400>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU5: cpu5@500 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x500>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU6: cpu6@600 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x600>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU7: cpu7@700 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x700>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU8: cpu8@800 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x800>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU9: cpu9@900 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0x900>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU10: cpu10@a00 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0xa00>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+
+		CPU11: cpu11@b00 {
+			compatible = "arm,armv8";
+			enable-method = "psci";
+			reg = <0x0 0xb00>;
+			device_type = "cpu";
+			capacity-dmips-mhz = <1024>;
+		};
+	};
+
+	arch_timer: 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>;
+		clock-frequency = <1000000000>;
+		interrupt-parent = <&gic>;
+		arm,no-tick-in-suspend;
+	};
+
+	memory@80000000 {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		device_type = "memory";
+		reg = <0x00000000 0x80000000 0x1 0x00000000>;
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
+	pmu: pmu {
+		compatible = "arm,armv8-pmuv3";
+		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-parent = <&gic>;
+		status = "okay";
+	};
+
+	pmu_spe: pmu_spe {
+		compatible = "arm,statistical-profiling-extension-v1";
+		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-parent = <&gic>;
+		status = "okay";
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		linux,cma {
+			compatible = "shared-dma-pool";
+			reusable;
+			size = <0x0 0x28000000>;
+			linux,cma-default;
+		};
+
+	};
+
+	sky1_fixed_clocks: fixed-clocks {
+		uartclk: uartclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <100000000>;
+			clock-output-names = "uartclk";
+		};
+
+		uart_apb_pclk: uart_apb_pclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <200000000>;
+			clock-output-names = "apb_pclk";
+		};
+	};
+
+	soc@0 {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		dma-ranges;
+
+		uart2: uart@040d0000 {
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x0 0x040d0000 0x0 0x1000>;
+			interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "uartclk", "apb_pclk";
+			clocks = <&uartclk>, <&uart_apb_pclk>;
+			status = "disabled";
+		};
+
+		gic: interrupt-controller@0e001000 {
+			compatible = "arm,gic-v3";
+			#address-cells = <2>;
+			#interrupt-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+			interrupt-controller;
+			#redistributor-regions = <1>;
+			reg = <0x0 0x0e010000 0 0x10000>,	/* GICD */
+			      <0x0 0x0e090000 0 0x300000>;       /* GICR * 12 */
+			redistributor-stride = <0x40000>;
+			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
+			interrupt-parent = <&gic>;
+
+			its_pcie: its@e050000 {
+				compatible = "arm,gic-v3-its";
+				msi-controller;
+				reg = <0x0 0x0e050000 0x0 0x30000>;
+			};
+		};
+	};
+};
-- 
2.25.1

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> new file mode 100644
> index 000000000000..d98735f782e0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + *
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>

[..]

> +	arch_timer: 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>;
> +		clock-frequency = <1000000000>;
> +		interrupt-parent = <&gic>;
> +		arm,no-tick-in-suspend;
> +	};

This is not Arm v8.0 SoC so where is non-secure EL2 virtual timer?

On 25-02-23 04:05:10, Marcin Juszkiewicz wrote:
> 
> > diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> > new file mode 100644
> > index 000000000000..d98735f782e0
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> > @@ -0,0 +1,264 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright 2025 Cix Technology Group Co., Ltd.
> > + *
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> 
> [..]
> 
> > +     arch_timer: 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>;
> > +             clock-frequency = <1000000000>;
> > +             interrupt-parent = <&gic>;
> > +             arm,no-tick-in-suspend;
> > +     };
> 
> This is not Arm v8.0 SoC so where is non-secure EL2 virtual timer?

It is the Arm v9 SoC and back compatible with Arm v8.

-- 

Best regards,
Peter

W dniu 24.02.2025 o 12:36, Peter Chen pisze:
> On 25-02-23 04:05:10, Marcin Juszkiewicz wrote:
>>
>>> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
>>> new file mode 100644
>>> index 000000000000..d98735f782e0
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
>>> @@ -0,0 +1,264 @@
>>> +// SPDX-License-Identifier: BSD-3-Clause
>>> +/*
>>> + * Copyright 2025 Cix Technology Group Co., Ltd.
>>> + *
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>
>> [..]
>>
>>> +     arch_timer: 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>;
>>> +             clock-frequency = <1000000000>;
>>> +             interrupt-parent = <&gic>;
>>> +             arm,no-tick-in-suspend;
>>> +     };
>>
>> This is not Arm v8.0 SoC so where is non-secure EL2 virtual timer?
> 
> It is the Arm v9 SoC and back compatible with Arm v8.

Arm SoC has several timer interrupts:

PPI 10: Non-secure EL2 physical timer interrupt
PPI 11: Virtual timer interrupt
PPI 12: Non-secure EL2 virtual timer
PPI 13: Secure physical timer interrupt
PPI 14: Non-secure physical timer interrupt

You mention 10, 11, 13, 14 only like your SoC would be plain old Arm 
v8.0 one (Cortex-A53/A72).

Sky1 (CP/CA/CS8180) is Arm v9 so should also list PPI 12 which came with 
VHE (Virtualization host extensions) which is mandatory for each Arm cpu 
v8.1 or above (and is implemented in A520/A720 cores).

On 25-02-24 15:06:19, Marcin Juszkiewicz wrote:
> > > > diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> > > > new file mode 100644
> > > > index 000000000000..d98735f782e0
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> > > > @@ -0,0 +1,264 @@
> > > > +// SPDX-License-Identifier: BSD-3-Clause
> > > > +/*
> > > > + * Copyright 2025 Cix Technology Group Co., Ltd.
> > > > + *
> > > > + */
> > > > +
> > > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > 
> > > [..]
> > > 
> > > > +     arch_timer: 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>;
> > > > +             clock-frequency = <1000000000>;
> > > > +             interrupt-parent = <&gic>;
> > > > +             arm,no-tick-in-suspend;
> > > > +     };
> > > 
> > > This is not Arm v8.0 SoC so where is non-secure EL2 virtual timer?
> > 
> > It is the Arm v9 SoC and back compatible with Arm v8.
> 
> Arm SoC has several timer interrupts:
> 
> PPI 10: Non-secure EL2 physical timer interrupt
> PPI 11: Virtual timer interrupt
> PPI 12: Non-secure EL2 virtual timer
> PPI 13: Secure physical timer interrupt
> PPI 14: Non-secure physical timer interrupt
> 
> You mention 10, 11, 13, 14 only like your SoC would be plain old Arm
> v8.0 one (Cortex-A53/A72).
> 
> Sky1 (CP/CA/CS8180) is Arm v9 so should also list PPI 12 which came with
> VHE (Virtualization host extensions) which is mandatory for each Arm cpu
> v8.1 or above (and is implemented in A520/A720 cores).

Thanks for mentioning it. I will add PPI 12 for v2 patch set.

-- 

Best regards,
Peter

On Thu, Feb 20, 2025 at 04:40:20PM +0800, Peter Chen wrote:
> CIX SKY1 SoC is high performance Armv9 SoC designed by Cixtech,
> and Orion O6 is open source motherboard launched by Radxa.
> See below for detail:
> https://docs.radxa.com/en/orion/o6/getting-started/introduction
> 
> In this commit, it only adds limited components for running initramfs
> at Orion O6.
> 
> Acked-by: Fugang.duan <fugang.duan@cixtech.com>
> Signed-off-by: Peter Chen <peter.chen@cixtech.com>
> ---
>  arch/arm64/boot/dts/Makefile              |   1 +
>  arch/arm64/boot/dts/cix/Makefile          |   2 +
>  arch/arm64/boot/dts/cix/sky1-orion-o6.dts |  21 ++
>  arch/arm64/boot/dts/cix/sky1.dtsi         | 264 ++++++++++++++++++++++
>  4 files changed, 288 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/cix/Makefile
>  create mode 100644 arch/arm64/boot/dts/cix/sky1-orion-o6.dts
>  create mode 100644 arch/arm64/boot/dts/cix/sky1.dtsi
> 
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 79b73a21ddc2..8e7ccd0027bd 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -13,6 +13,7 @@ subdir-y += bitmain
>  subdir-y += blaize
>  subdir-y += broadcom
>  subdir-y += cavium
> +subdir-y += cix
>  subdir-y += exynos
>  subdir-y += freescale
>  subdir-y += hisilicon
> diff --git a/arch/arm64/boot/dts/cix/Makefile b/arch/arm64/boot/dts/cix/Makefile
> new file mode 100644
> index 000000000000..ed3713982012
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_CIX) += sky1-orion-o6.dtb
> diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> new file mode 100644
> index 000000000000..dbee1616076d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include "sky1.dtsi"
> +/ {
> +	model = "Radxa Orion O6";
> +	compatible = "radxa,orion-o6";
> +
> +	chosen {
> +		stdout-path = &uart2;
> +	};
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> new file mode 100644
> index 000000000000..d98735f782e0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + *
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial2 = &uart2;
> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&CPU0>;
> +				};
> +				core1 {
> +					cpu = <&CPU1>;
> +				};
> +				core2 {
> +					cpu = <&CPU2>;
> +				};
> +				core3 {
> +					cpu = <&CPU3>;
> +				};
> +				core4 {
> +					cpu = <&CPU4>;
> +				};
> +				core5 {
> +					cpu = <&CPU5>;
> +				};
> +				core6 {
> +					cpu = <&CPU6>;
> +				};
> +				core7 {
> +					cpu = <&CPU7>;
> +				};
> +				core8 {
> +					cpu = <&CPU8>;
> +				};
> +				core9 {
> +					cpu = <&CPU9>;
> +				};
> +				core10 {
> +					cpu = <&CPU10>;
> +				};
> +				core11 {
> +					cpu = <&CPU11>;
> +				};
> +			};
> +		};
> +
> +		CPU0: cpu0@0 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x0>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU1: cpu1@100 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x100>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU2: cpu2@200 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x200>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU3: cpu3@300 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x300>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU4: cpu4@400 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x400>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU5: cpu5@500 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x500>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU6: cpu6@600 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x600>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU7: cpu7@700 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x700>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU8: cpu8@800 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x800>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU9: cpu9@900 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x900>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU10: cpu10@a00 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0xa00>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU11: cpu11@b00 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0xb00>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +	};
> +
> +	arch_timer: 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>;
> +		clock-frequency = <1000000000>;
> +		interrupt-parent = <&gic>;
> +		arm,no-tick-in-suspend;
> +	};
> +
> +	memory@80000000 {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		device_type = "memory";
> +		reg = <0x00000000 0x80000000 0x1 0x00000000>;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	pmu: pmu {
> +		compatible = "arm,armv8-pmuv3";

Also needs the CPU model specific compatible string.

> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-parent = <&gic>;
> +		status = "okay";

okay is the default, don't need status.

> +	};
> +
> +	pmu_spe: pmu_spe {
> +		compatible = "arm,statistical-profiling-extension-v1";
> +		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-parent = <&gic>;
> +		status = "okay";
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		linux,cma {
> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x0 0x28000000>;
> +			linux,cma-default;
> +		};
> +
> +	};
> +
> +	sky1_fixed_clocks: fixed-clocks {

Drop this container node.

> +		uartclk: uartclk {

clock-100000000 for the node name.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <100000000>;
> +			clock-output-names = "uartclk";
> +		};
> +
> +		uart_apb_pclk: uart_apb_pclk {

Similar here.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <200000000>;
> +			clock-output-names = "apb_pclk";
> +		};
> +	};
> +
> +	soc@0 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		dma-ranges;
> +
> +		uart2: uart@040d0000 {

serial@...

> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x0 0x040d0000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
> +			clock-names = "uartclk", "apb_pclk";
> +			clocks = <&uartclk>, <&uart_apb_pclk>;
> +			status = "disabled";
> +		};
> +
> +		gic: interrupt-controller@0e001000 {
> +			compatible = "arm,gic-v3";
> +			#address-cells = <2>;
> +			#interrupt-cells = <3>;
> +			#size-cells = <2>;
> +			ranges;
> +			interrupt-controller;
> +			#redistributor-regions = <1>;
> +			reg = <0x0 0x0e010000 0 0x10000>,	/* GICD */
> +			      <0x0 0x0e090000 0 0x300000>;       /* GICR * 12 */
> +			redistributor-stride = <0x40000>;
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> +			interrupt-parent = <&gic>;
> +
> +			its_pcie: its@e050000 {

msi-controller@...

> +				compatible = "arm,gic-v3-its";
> +				msi-controller;
> +				reg = <0x0 0x0e050000 0x0 0x30000>;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.25.1
>

On 25-02-22 06:46:51, Rob Herring wrote:

Hi Rob,

Thanks for your reviewing.

> 
> > +
> > +     pmu: pmu {
> > +             compatible = "arm,armv8-pmuv3";
> 
> Also needs the CPU model specific compatible string.

For CIX Sky1 SoC, it is Armv9 big-LITTLE architecture, if we
add two compatibles for both A720 and A520, there will be two
Arm PMU devices, could it work well for user tool like perf?

> 
> > +             interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
> > +             interrupt-parent = <&gic>;
> > +             status = "okay";
> 
> okay is the default, don't need status.

Will change.

> 
> > +     };
> > +
> > +     pmu_spe: pmu_spe {
> > +             compatible = "arm,statistical-profiling-extension-v1";
> > +             interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
> > +             interrupt-parent = <&gic>;
> > +             status = "okay";
> > +     };
> > +
> > +     reserved-memory {
> > +             #address-cells = <2>;
> > +             #size-cells = <2>;
> > +             ranges;
> > +
> > +             linux,cma {
> > +                     compatible = "shared-dma-pool";
> > +                     reusable;
> > +                     size = <0x0 0x28000000>;
> > +                     linux,cma-default;
> > +             };
> > +
> > +     };
> > +
> > +     sky1_fixed_clocks: fixed-clocks {
> 
> Drop this container node.

Okay, I will delete above line.

> 
> > +             uartclk: uartclk {
> 
> clock-100000000 for the node name.

Will change.
> 
> > +                     compatible = "fixed-clock";
> > +                     #clock-cells = <0>;
> > +                     clock-frequency = <100000000>;
> > +                     clock-output-names = "uartclk";
> > +             };
> > +
> > +             uart_apb_pclk: uart_apb_pclk {
> 
> Similar here.
> 
> > +                     compatible = "fixed-clock";
> > +                     #clock-cells = <0>;
> > +                     clock-frequency = <200000000>;
> > +                     clock-output-names = "apb_pclk";
> > +             };
> > +     };
> > +
> > +     soc@0 {
> > +             compatible = "simple-bus";
> > +             #address-cells = <2>;
> > +             #size-cells = <2>;
> > +             ranges;
> > +             dma-ranges;
> > +
> > +             uart2: uart@040d0000 {
> 
> serial@...

Will change

> 
> > +                     compatible = "arm,pl011", "arm,primecell";
> > +                     reg = <0x0 0x040d0000 0x0 0x1000>;
> > +                     interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clock-names = "uartclk", "apb_pclk";
> > +                     clocks = <&uartclk>, <&uart_apb_pclk>;
> > +                     status = "disabled";
> > +             };
> > +
> > +             gic: interrupt-controller@0e001000 {
> > +                     compatible = "arm,gic-v3";
> > +                     #address-cells = <2>;
> > +                     #interrupt-cells = <3>;
> > +                     #size-cells = <2>;
> > +                     ranges;
> > +                     interrupt-controller;
> > +                     #redistributor-regions = <1>;
> > +                     reg = <0x0 0x0e010000 0 0x10000>,       /* GICD */
> > +                           <0x0 0x0e090000 0 0x300000>;       /* GICR * 12 */
> > +                     redistributor-stride = <0x40000>;
> > +                     interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> > +                     interrupt-parent = <&gic>;
> > +
> > +                     its_pcie: its@e050000 {
> 
> msi-controller@...

Will change

-- 

Best regards,
Peter

On 20/02/2025 09:40, Peter Chen wrote:
> CIX SKY1 SoC is high performance Armv9 SoC designed by Cixtech,
> and Orion O6 is open source motherboard launched by Radxa.
> See below for detail:
> https://docs.radxa.com/en/orion/o6/getting-started/introduction
> 
> In this commit, it only adds limited components for running initramfs
> at Orion O6.
> 

Test the patches before posting.

> Acked-by: Fugang.duan <fugang.duan@cixtech.com>

Again, difference in names...

> Signed-off-by: Peter Chen <peter.chen@cixtech.com>
> ---
>  arch/arm64/boot/dts/Makefile              |   1 +
>  arch/arm64/boot/dts/cix/Makefile          |   2 +
>  arch/arm64/boot/dts/cix/sky1-orion-o6.dts |  21 ++
>  arch/arm64/boot/dts/cix/sky1.dtsi         | 264 ++++++++++++++++++++++
>  4 files changed, 288 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/cix/Makefile
>  create mode 100644 arch/arm64/boot/dts/cix/sky1-orion-o6.dts
>  create mode 100644 arch/arm64/boot/dts/cix/sky1.dtsi
> 
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 79b73a21ddc2..8e7ccd0027bd 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -13,6 +13,7 @@ subdir-y += bitmain
>  subdir-y += blaize
>  subdir-y += broadcom
>  subdir-y += cavium
> +subdir-y += cix
>  subdir-y += exynos
>  subdir-y += freescale
>  subdir-y += hisilicon
> diff --git a/arch/arm64/boot/dts/cix/Makefile b/arch/arm64/boot/dts/cix/Makefile
> new file mode 100644
> index 000000000000..ed3713982012
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_CIX) += sky1-orion-o6.dtb
> diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> new file mode 100644
> index 000000000000..dbee1616076d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include "sky1.dtsi"
> +/ {
> +	model = "Radxa Orion O6";
> +	compatible = "radxa,orion-o6";

Never tested.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

> +
> +	chosen {
> +		stdout-path = &uart2;
> +	};
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> new file mode 100644
> index 000000000000..d98735f782e0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + *
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial2 = &uart2;

This is not part of the SoC, but board.

> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&CPU0>;

Labels are lowercase.

> +				};
> +				core1 {
> +					cpu = <&CPU1>;
> +				};
> +				core2 {
> +					cpu = <&CPU2>;
> +				};
> +				core3 {
> +					cpu = <&CPU3>;
> +				};
> +				core4 {
> +					cpu = <&CPU4>;
> +				};
> +				core5 {
> +					cpu = <&CPU5>;
> +				};
> +				core6 {
> +					cpu = <&CPU6>;
> +				};
> +				core7 {
> +					cpu = <&CPU7>;
> +				};
> +				core8 {
> +					cpu = <&CPU8>;
> +				};
> +				core9 {
> +					cpu = <&CPU9>;
> +				};
> +				core10 {
> +					cpu = <&CPU10>;
> +				};
> +				core11 {
> +					cpu = <&CPU11>;
> +				};
> +			};
> +		};
> +
> +		CPU0: cpu0@0 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

cpu@

> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x0>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU1: cpu1@100 {

cpu@

> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x100>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU2: cpu2@200 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x200>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU3: cpu3@300 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x300>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <403>;
> +		};
> +
> +		CPU4: cpu4@400 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x400>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU5: cpu5@500 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x500>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU6: cpu6@600 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x600>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU7: cpu7@700 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x700>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU8: cpu8@800 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x800>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU9: cpu9@900 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0x900>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU10: cpu10@a00 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0xa00>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +
> +		CPU11: cpu11@b00 {
> +			compatible = "arm,armv8";
> +			enable-method = "psci";
> +			reg = <0x0 0xb00>;
> +			device_type = "cpu";
> +			capacity-dmips-mhz = <1024>;
> +		};
> +	};
> +
> +	arch_timer: 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>;
> +		clock-frequency = <1000000000>;
> +		interrupt-parent = <&gic>;
> +		arm,no-tick-in-suspend;
> +	};
> +
> +	memory@80000000 {

This does not look like part of SoC.


> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		device_type = "memory";
> +		reg = <0x00000000 0x80000000 0x1 0x00000000>;

Order properties according to DTS coding style.

> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	pmu: pmu {

Keep some sort of order. See DTS coding style.

> +		compatible = "arm,armv8-pmuv3";
> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-parent = <&gic>;
> +		status = "okay";

Where did you disable it? Why only this has status=okay but other do not?

> +	};
> +
> +	pmu_spe: pmu_spe {

DTS coding style. No underscores.

> +		compatible = "arm,statistical-profiling-extension-v1";
> +		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-parent = <&gic>;
> +		status = "okay";
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		linux,cma {

This does not look like part of SoC.

> +			compatible = "shared-dma-pool";
> +			reusable;
> +			size = <0x0 0x28000000>;
> +			linux,cma-default;
> +		};
> +
> +	};
> +
> +	sky1_fixed_clocks: fixed-clocks {
> +		uartclk: uartclk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <100000000>;
> +			clock-output-names = "uartclk";
> +		};
> +
> +		uart_apb_pclk: uart_apb_pclk {

This code is not getting better...

Where are these clocks physically?

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <200000000>;
> +			clock-output-names = "apb_pclk";
> +		};
> +	};
> +
> +	soc@0 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		dma-ranges;
> +
> +		uart2: uart@040d0000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x0 0x040d0000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
> +			clock-names = "uartclk", "apb_pclk";
> +			clocks = <&uartclk>, <&uart_apb_pclk>;
> +			status = "disabled";
> +		};
> +
> +		gic: interrupt-controller@0e001000 {
> +			compatible = "arm,gic-v3";
> +			#address-cells = <2>;
> +			#interrupt-cells = <3>;
> +			#size-cells = <2>;
> +			ranges;
> +			interrupt-controller;
> +			#redistributor-regions = <1>;
> +			reg = <0x0 0x0e010000 0 0x10000>,	/* GICD */
> +			      <0x0 0x0e090000 0 0x300000>;       /* GICR * 12 */
> +			redistributor-stride = <0x40000>;

DTS coding style.

> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> +			interrupt-parent = <&gic>;
> +
> +			its_pcie: its@e050000 {
> +				compatible = "arm,gic-v3-its";
> +				msi-controller;
> +				reg = <0x0 0x0e050000 0x0 0x30000>;
> +			};
> +		};
> +	};
> +};


Best regards,
Krzysztof

On Thu, Feb 20, 2025, at 09:40, Peter Chen wrote:

> +#include "sky1.dtsi"
> +/ {
> +       model = "Radxa Orion O6";
> +       compatible = "radxa,orion-o6";

This should list both the compatible string for the board and
the one for the SoC.

> +
> +       aliases {
> +               serial2 = &uart2;
> +       };

Please put the aliases in the .dts file, not the chip specific
.dtsi file, as each board typically wires these up differently.

Note that the 'serial2' alias names are meant to correspond
to whatever label you find on the board, not the internal
numbering inside of the chip they are wired up to. Usually
these start with 'serial0' for the first one that is enabled.

> +               CPU0: cpu0@0 {
> +                       compatible = "arm,armv8";
> +                       enable-method = "psci";

This should list the actual identifier of the CPU core, not
just "arm,armv8" which is the generic string used in the
models for emulators that don't try to model a particular
core.

> +       memory@80000000 {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               device_type = "memory";
> +               reg = <0x00000000 0x80000000 0x1 0x00000000>;
> +       };

The memory size is not part of the SoC either, unless the only
way to use this SoC is with on-chip eDRAM or similar.

Normally this gets filled by the bootloader based on how
much RAM gets detected.

> +               linux,cma {
> +                       compatible = "shared-dma-pool";
> +                       reusable;
> +                       size = <0x0 0x28000000>;
> +                       linux,cma-default;
> +               };

Same here, this is a setting from the firmware, not the
SoC.

> +       sky1_fixed_clocks: fixed-clocks {
> +               uartclk: uartclk {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <0>;
> +                       clock-frequency = <100000000>;
> +                       clock-output-names = "uartclk";

> +               uart_apb_pclk: uart_apb_pclk {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <0>;
> +                       clock-frequency = <200000000>;
> +                       clock-output-names = "apb_pclk";


Clock names don't need "clk" in them, and there should
be no underscore -- use '-' instead of '_' when separating
strings in DT.

> +       soc@0 {
> +               compatible = "simple-bus";
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +               dma-ranges;
> +
> +               uart2: uart@040d0000 {
> +                       compatible = "arm,pl011", "arm,primecell";
> +                       reg = <0x0 0x040d0000 0x0 0x1000>;
> +                       interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
> +                       clock-names = "uartclk", "apb_pclk";
> +                       clocks = <&uartclk>, <&uart_apb_pclk>;
> +                       status = "disabled";
> +               };

It seems strange to list only "uart2" -- usually the dtsi file contains
all of the instances that are present on the chip and leave it
up to the .dts file to enable the ones that are used.

      Arnd

On 25-02-20 11:58:21, Arnd Bergmann wrote:
> 

Arnd, thanks for your review.

> On Thu, Feb 20, 2025, at 09:40, Peter Chen wrote:
> 
> > +#include "sky1.dtsi"
> > +/ {
> > +       model = "Radxa Orion O6";
> > +       compatible = "radxa,orion-o6";
> 
> This should list both the compatible string for the board and
> the one for the SoC.

Will change to compatible = "radxa,orion-o6", "cix,sky1";

> 
> > +
> > +       aliases {
> > +               serial2 = &uart2;
> > +       };
> 
> Please put the aliases in the .dts file, not the chip specific
> .dtsi file, as each board typically wires these up differently.
> 
> Note that the 'serial2' alias names are meant to correspond
> to whatever label you find on the board, not the internal
> numbering inside of the chip they are wired up to. Usually
> these start with 'serial0' for the first one that is enabled.

In fact, we would like to alias the SoC UART controller index here,
and amba-pl011.c will try to get it, see function pl011_probe_dt_alias.
It is initial dtsi file, so I only add console one which needs
to align the bootargs passed by UEFI.

> 
> > +               CPU0: cpu0@0 {
> > +                       compatible = "arm,armv8";
> > +                       enable-method = "psci";
> 
> This should list the actual identifier of the CPU core, not
> just "arm,armv8" which is the generic string used in the
> models for emulators that don't try to model a particular
> core.

Will change big core to 'compatible = "arm,cortex-a720";'
and LITTLE core to 'compatible = "arm,cortex-a520";'

> 
> > +       memory@80000000 {
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               device_type = "memory";
> > +               reg = <0x00000000 0x80000000 0x1 0x00000000>;
> > +       };
> 
> The memory size is not part of the SoC either, unless the only
> way to use this SoC is with on-chip eDRAM or similar.
> 
> Normally this gets filled by the bootloader based on how
> much RAM gets detected.

Will move it to dts file.

> 
> > +               linux,cma {
> > +                       compatible = "shared-dma-pool";
> > +                       reusable;
> > +                       size = <0x0 0x28000000>;
> > +                       linux,cma-default;
> > +               };
> 
> Same here, this is a setting from the firmware, not the
> SoC.

Will move it to dts file since our firmware has already released,
and it needs to support different kernels.

> 
> > +       sky1_fixed_clocks: fixed-clocks {
> > +               uartclk: uartclk {
> > +                       compatible = "fixed-clock";
> > +                       #clock-cells = <0>;
> > +                       clock-frequency = <100000000>;
> > +                       clock-output-names = "uartclk";
> 
> > +               uart_apb_pclk: uart_apb_pclk {
> > +                       compatible = "fixed-clock";
> > +                       #clock-cells = <0>;
> > +                       clock-frequency = <200000000>;
> > +                       clock-output-names = "apb_pclk";
> 
> 
> Clock names don't need "clk" in them, and there should
> be no underscore -- use '-' instead of '_' when separating
> strings in DT.

Will change to:
uart_apb: clock-uart-apb {
	...
	clock-output-names = "uart_apb";

};

> 
> > +       soc@0 {
> > +               compatible = "simple-bus";
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> > +               dma-ranges;
> > +
> > +               uart2: uart@040d0000 {
> > +                       compatible = "arm,pl011", "arm,primecell";
> > +                       reg = <0x0 0x040d0000 0x0 0x1000>;
> > +                       interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clock-names = "uartclk", "apb_pclk";
> > +                       clocks = <&uartclk>, <&uart_apb_pclk>;
> > +                       status = "disabled";
> > +               };
> 
> It seems strange to list only "uart2" -- usually the dtsi file contains
> all of the instances that are present on the chip and leave it
> up to the .dts file to enable the ones that are used.

Since it is the first CIX SoC support patch series, I only added basic
Kconfig build for booting minimum system for easy review. For device
node, it relates to clock/reset/power domain binding which will add later.

Regards,
Peter

On 20/02/2025 13:30, Peter Chen wrote:
>>
>>> +
>>> +       aliases {
>>> +               serial2 = &uart2;
>>> +       };
>>
>> Please put the aliases in the .dts file, not the chip specific
>> .dtsi file, as each board typically wires these up differently.
>>
>> Note that the 'serial2' alias names are meant to correspond
>> to whatever label you find on the board, not the internal
>> numbering inside of the chip they are wired up to. Usually
>> these start with 'serial0' for the first one that is enabled.
> 
> In fact, we would like to alias the SoC UART controller index here,
> and amba-pl011.c will try to get it, see function pl011_probe_dt_alias.
> It is initial dtsi file, so I only add console one which needs
> to align the bootargs passed by UEFI.


Your "in fact" is not really related to the problem described. If you
put it in the correct place, drivers will work just as fine.

> 
>>
>>> +               CPU0: cpu0@0 {
>>> +                       compatible = "arm,armv8";
>>> +                       enable-method = "psci";
>>
>> This should list the actual identifier of the CPU core, not
>> just "arm,armv8" which is the generic string used in the
>> models for emulators that don't try to model a particular
>> core.
> 
> Will change big core to 'compatible = "arm,cortex-a720";'
> and LITTLE core to 'compatible = "arm,cortex-a520";'
> 
>>
>>> +       memory@80000000 {
>>> +               #address-cells = <2>;
>>> +               #size-cells = <2>;
>>> +               device_type = "memory";
>>> +               reg = <0x00000000 0x80000000 0x1 0x00000000>;
>>> +       };
>>
>> The memory size is not part of the SoC either, unless the only
>> way to use this SoC is with on-chip eDRAM or similar.
>>
>> Normally this gets filled by the bootloader based on how
>> much RAM gets detected.
> 
> Will move it to dts file.
> 
>>
>>> +               linux,cma {
>>> +                       compatible = "shared-dma-pool";
>>> +                       reusable;
>>> +                       size = <0x0 0x28000000>;
>>> +                       linux,cma-default;
>>> +               };
>>
>> Same here, this is a setting from the firmware, not the
>> SoC.
> 
> Will move it to dts file since our firmware has already released,
> and it needs to support different kernels.
> 
>>
>>> +       sky1_fixed_clocks: fixed-clocks {
>>> +               uartclk: uartclk {
>>> +                       compatible = "fixed-clock";
>>> +                       #clock-cells = <0>;
>>> +                       clock-frequency = <100000000>;
>>> +                       clock-output-names = "uartclk";
>>
>>> +               uart_apb_pclk: uart_apb_pclk {
>>> +                       compatible = "fixed-clock";
>>> +                       #clock-cells = <0>;
>>> +                       clock-frequency = <200000000>;
>>> +                       clock-output-names = "apb_pclk";
>>
>>
>> Clock names don't need "clk" in them, and there should
>> be no underscore -- use '-' instead of '_' when separating
>> strings in DT.
> 
> Will change to:
> uart_apb: clock-uart-apb {

No, instead explain why this is part of SoC - or what are you missing
here - and use preferred naming.

Please use name for all fixed clocks which matches current format
recommendation: 'clock-<freq>' (see also the pattern in the binding for
any other options).

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



Best regards,
Krzysztof

On 25-02-21 12:42:23, Krzysztof Kozlowski wrote:

Hi Krysztof,

Thanks for your detail reviewing. I am afraid my email client did not
receive your email for comment for this patch, I reply at this one.

>> +#include "sky1.dtsi"
>> +/ {
>> +	model = "Radxa Orion O6";
>> +	compatible = "radxa,orion-o6";

> Never tested.

> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.

I am sorry for that. I just reviewed device driver patch in recent 3-4 years,
forget doing additional check for dts patch.

> 
> On 20/02/2025 13:30, Peter Chen wrote:
> >>
> >>> +
> >>> +       aliases {
> >>> +               serial2 = &uart2;
> >>> +       };
> >>
> >> Please put the aliases in the .dts file, not the chip specific
> >> .dtsi file, as each board typically wires these up differently.
> >>
> >> Note that the 'serial2' alias names are meant to correspond
> >> to whatever label you find on the board, not the internal
> >> numbering inside of the chip they are wired up to. Usually
> >> these start with 'serial0' for the first one that is enabled.
> >
> > In fact, we would like to alias the SoC UART controller index here,
> > and amba-pl011.c will try to get it, see function pl011_probe_dt_alias.
> > It is initial dtsi file, so I only add console one which needs
> > to align the bootargs passed by UEFI.
> 
> 
> Your "in fact" is not really related to the problem described. If you
> put it in the correct place, drivers will work just as fine.

You also mentioned that in your comments. Yes, indeed the board dts file
could remap physical controller index as different board serial number,
but it is not what we would like to do (at least for CIX platforms).
In our both HW and SW documents, we have fixed our uart usage cases,
for example, UART2 as AP serial ports. UART0-UART1 as uart application,eg
bluetooth. Customer will do their design to follow above rules, and
it avoids each customer writing this alias at their board file.

Meanwhile, fixed uart alias as its physical index is better to
understand SW/HW relationship. Imaging you are debugging one UART
bluetooth use case, your application passes "/dev/ttyAMA0" as its
communication port due to board dts alias, but in board schematic,
it is connects to SoC uart 1, the person to debug may confuse the
different mapping at first.

Also, in kernel device driver, it also uses alias id to its uart
port index, it could better understand device driver hardware
behaviours, esp you may dump hardware register to debug, you could
easy to find related registers if the mapping is the same.

> 
> >
> >>
> >>> +               CPU0: cpu0@0 {
> >>> +                       compatible = "arm,armv8";
> >>> +                       enable-method = "psci";

Will change Label "CPU0" as "cpu0", and name "cpu0" as "cpu"

> >>
> >> This should list the actual identifier of the CPU core, not
> >> just "arm,armv8" which is the generic string used in the
> >> models for emulators that don't try to model a particular
> >> core.
> >
> > Will change big core to 'compatible = "arm,cortex-a720";'
> > and LITTLE core to 'compatible = "arm,cortex-a520";'
> >
> >>
> >>> +       memory@80000000 {
> >>> +               #address-cells = <2>;
> >>> +               #size-cells = <2>;
> >>> +               device_type = "memory";
> >>> +               reg = <0x00000000 0x80000000 0x1 0x00000000>;
> >>> +       };
> >>
> >> The memory size is not part of the SoC either, unless the only
> >> way to use this SoC is with on-chip eDRAM or similar.
> >>
> >> Normally this gets filled by the bootloader based on how
> >> much RAM gets detected.
> >
> > Will move it to dts file.
> >
> >>
> >>> +               linux,cma {
> >>> +                       compatible = "shared-dma-pool";
> >>> +                       reusable;
> >>> +                       size = <0x0 0x28000000>;
> >>> +                       linux,cma-default;
> >>> +               };
> >>
> >> Same here, this is a setting from the firmware, not the
> >> SoC.
> >
> > Will move it to dts file since our firmware has already released,
> > and it needs to support different kernels.
> >
> >>
> >>> +       sky1_fixed_clocks: fixed-clocks {
> >>> +               uartclk: uartclk {
> >>> +                       compatible = "fixed-clock";
> >>> +                       #clock-cells = <0>;
> >>> +                       clock-frequency = <100000000>;
> >>> +                       clock-output-names = "uartclk";
> >>
> >>> +               uart_apb_pclk: uart_apb_pclk {
> >>> +                       compatible = "fixed-clock";
> >>> +                       #clock-cells = <0>;
> >>> +                       clock-frequency = <200000000>;
> >>> +                       clock-output-names = "apb_pclk";
> >>
> >>
> >> Clock names don't need "clk" in them, and there should
> >> be no underscore -- use '-' instead of '_' when separating
> >> strings in DT.
> >
> > Will change to:
> > uart_apb: clock-uart-apb {
> 
> No, instead explain why this is part of SoC - or what are you missing
> here - and use preferred naming.

It is in SoC part, APB clock uses to visit register, and the function
amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
device probes. It uses common Arm uart pl011 IP, the binding doc
described at: Documentation/devicetree/bindings/serial/pl011.yaml

Since it is the initial dts support patch, I do not want to add
more to avoid reviewing efforts, eg, clock gate, reset, etc.
That's the reason I use fixed clock here to let basic system
work, and boots to console.

> 
> Please use name for all fixed clocks which matches current format
> recommendation: 'clock-<freq>' (see also the pattern in the binding for
> any other options).
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml?h=v6.11-rc1
> 

Will change to "uart_apb: clock-1000000"

>> +	pmu: pmu {

> Keep some sort of order. See DTS coding style.

I will follow: Documentation/devicetree/bindings/dts-coding-style.rst
to fix coding sytle issue at v2 patch set.

>> +		compatible = "arm,armv8-pmuv3";
>> +		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
>> +		interrupt-parent = <&gic>;
>> +		status = "okay";

> Where did you disable it? Why only this has status=okay but other do not?

I will remove this one at v2 patch set.

-- 

Best regards,
Peter

On 24/02/2025 03:26, Peter Chen wrote:
> On 25-02-21 12:42:23, Krzysztof Kozlowski wrote:
> 
> Hi Krysztof,
> 
> Thanks for your detail reviewing. I am afraid my email client did not
> receive your email for comment for this patch, I reply at this one.
> 
>>> +#include "sky1.dtsi"
>>> +/ {
>>> +	model = "Radxa Orion O6";
>>> +	compatible = "radxa,orion-o6";
> 
>> Never tested.
> 
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
>> Maybe you need to update your dtschema and yamllint. Don't rely on
>> distro packages for dtschema and be sure you are using the latest
>> released dtschema.
> 
> I am sorry for that. I just reviewed device driver patch in recent 3-4 years,
> forget doing additional check for dts patch.
> 
>>
>> On 20/02/2025 13:30, Peter Chen wrote:
>>>>
>>>>> +
>>>>> +       aliases {
>>>>> +               serial2 = &uart2;
>>>>> +       };
>>>>
>>>> Please put the aliases in the .dts file, not the chip specific
>>>> .dtsi file, as each board typically wires these up differently.
>>>>
>>>> Note that the 'serial2' alias names are meant to correspond
>>>> to whatever label you find on the board, not the internal
>>>> numbering inside of the chip they are wired up to. Usually
>>>> these start with 'serial0' for the first one that is enabled.
>>>
>>> In fact, we would like to alias the SoC UART controller index here,
>>> and amba-pl011.c will try to get it, see function pl011_probe_dt_alias.
>>> It is initial dtsi file, so I only add console one which needs
>>> to align the bootargs passed by UEFI.
>>
>>
>> Your "in fact" is not really related to the problem described. If you
>> put it in the correct place, drivers will work just as fine.
> 
> You also mentioned that in your comments. Yes, indeed the board dts file
> could remap physical controller index as different board serial number,
> but it is not what we would like to do (at least for CIX platforms).
> In our both HW and SW documents, we have fixed our uart usage cases,
> for example, UART2 as AP serial ports. UART0-UART1 as uart application,eg
> bluetooth. Customer will do their design to follow above rules, and
> it avoids each customer writing this alias at their board file.

Follow standard rules, you don't get an exception. That's not a property
of the SoC.

> 
> Meanwhile, fixed uart alias as its physical index is better to

This is not being discussed.

> understand SW/HW relationship. Imaging you are debugging one UART
> bluetooth use case, your application passes "/dev/ttyAMA0" as its
> communication port due to board dts alias, but in board schematic,
> it is connects to SoC uart 1, the person to debug may confuse the
> different mapping at first.
> 
> Also, in kernel device driver, it also uses alias id to its uart
> port index, it could better understand device driver hardware
> behaviours, esp you may dump hardware register to debug, you could
> easy to find related registers if the mapping is the same.

Not related to topic at all.

> 
>>
>>>
>>>>
>>>>> +               CPU0: cpu0@0 {
>>>>> +                       compatible = "arm,armv8";
>>>>> +                       enable-method = "psci";
> 
> Will change Label "CPU0" as "cpu0", and name "cpu0" as "cpu"
> 
>>>>
>>>> This should list the actual identifier of the CPU core, not
>>>> just "arm,armv8" which is the generic string used in the
>>>> models for emulators that don't try to model a particular
>>>> core.
>>>
>>> Will change big core to 'compatible = "arm,cortex-a720";'
>>> and LITTLE core to 'compatible = "arm,cortex-a520";'
>>>
>>>>
>>>>> +       memory@80000000 {
>>>>> +               #address-cells = <2>;
>>>>> +               #size-cells = <2>;
>>>>> +               device_type = "memory";
>>>>> +               reg = <0x00000000 0x80000000 0x1 0x00000000>;
>>>>> +       };
>>>>
>>>> The memory size is not part of the SoC either, unless the only
>>>> way to use this SoC is with on-chip eDRAM or similar.
>>>>
>>>> Normally this gets filled by the bootloader based on how
>>>> much RAM gets detected.
>>>
>>> Will move it to dts file.
>>>
>>>>
>>>>> +               linux,cma {
>>>>> +                       compatible = "shared-dma-pool";
>>>>> +                       reusable;
>>>>> +                       size = <0x0 0x28000000>;
>>>>> +                       linux,cma-default;
>>>>> +               };
>>>>
>>>> Same here, this is a setting from the firmware, not the
>>>> SoC.
>>>
>>> Will move it to dts file since our firmware has already released,
>>> and it needs to support different kernels.
>>>
>>>>
>>>>> +       sky1_fixed_clocks: fixed-clocks {
>>>>> +               uartclk: uartclk {
>>>>> +                       compatible = "fixed-clock";
>>>>> +                       #clock-cells = <0>;
>>>>> +                       clock-frequency = <100000000>;
>>>>> +                       clock-output-names = "uartclk";
>>>>
>>>>> +               uart_apb_pclk: uart_apb_pclk {
>>>>> +                       compatible = "fixed-clock";
>>>>> +                       #clock-cells = <0>;
>>>>> +                       clock-frequency = <200000000>;
>>>>> +                       clock-output-names = "apb_pclk";
>>>>
>>>>
>>>> Clock names don't need "clk" in them, and there should
>>>> be no underscore -- use '-' instead of '_' when separating
>>>> strings in DT.
>>>
>>> Will change to:
>>> uart_apb: clock-uart-apb {
>>
>> No, instead explain why this is part of SoC - or what are you missing
>> here - and use preferred naming.
> 
> It is in SoC part, APB clock uses to visit register, and the function
> amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
> device probes. It uses common Arm uart pl011 IP, the binding doc
> described at: Documentation/devicetree/bindings/serial/pl011.yaml

So you added fake clock? Everything you wrote is not the reason to add
such clock.

DTS describes hardware, not what your drivers expect.

Drop the clock or add proper hardware description.

> 
> Since it is the initial dts support patch, I do not want to add
> more to avoid reviewing efforts, eg, clock gate, reset, etc.
> That's the reason I use fixed clock here to let basic system
> work, and boots to console.
> 
>>
Best regards,
Krzysztof

On 25-02-24 09:06:23, Krzysztof Kozlowski wrote:
> >> Your "in fact" is not really related to the problem described. If you
> >> put it in the correct place, drivers will work just as fine.
> >
> > You also mentioned that in your comments. Yes, indeed the board dts file
> > could remap physical controller index as different board serial number,
> > but it is not what we would like to do (at least for CIX platforms).
> > In our both HW and SW documents, we have fixed our uart usage cases,
> > for example, UART2 as AP serial ports. UART0-UART1 as uart application,eg
> > bluetooth. Customer will do their design to follow above rules, and
> > it avoids each customer writing this alias at their board file.
> 
> Follow standard rules, you don't get an exception. That's not a property
> of the SoC.

Okay. I see below documents describes it:

Documentation/devicetree/bindings/serial/qcom,msm-uartdm.yaml:23:  serialN aliases should be
in a .dts file instead of in a .dtsi file.
Documentation/devicetree/usage-model.rst:327:at all.  The /chosen, /aliases, and /memory nodes are
informational

> >>>
> >>>>
> >>>>> +       sky1_fixed_clocks: fixed-clocks {
> >>>>> +               uartclk: uartclk {
> >>>>> +                       compatible = "fixed-clock";
> >>>>> +                       #clock-cells = <0>;
> >>>>> +                       clock-frequency = <100000000>;
> >>>>> +                       clock-output-names = "uartclk";
> >>>>
> >>>>> +               uart_apb_pclk: uart_apb_pclk {
> >>>>> +                       compatible = "fixed-clock";
> >>>>> +                       #clock-cells = <0>;
> >>>>> +                       clock-frequency = <200000000>;
> >>>>> +                       clock-output-names = "apb_pclk";
> >>>>
> >>>>
> >>>> Clock names don't need "clk" in them, and there should
> >>>> be no underscore -- use '-' instead of '_' when separating
> >>>> strings in DT.
> >>>
> >>> Will change to:
> >>> uart_apb: clock-uart-apb {
> >>
> >> No, instead explain why this is part of SoC - or what are you missing
> >> here - and use preferred naming.
> >
> > It is in SoC part, APB clock uses to visit register, and the function
> > amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
> > device probes. It uses common Arm uart pl011 IP, the binding doc
> > described at: Documentation/devicetree/bindings/serial/pl011.yaml
> 
> So you added fake clock? Everything you wrote is not the reason to add
> such clock.

Not a fake clock, it is the real clocks, but depends on firmware open
their parents and configure their rate. It could let others do their
upstream work based on workable console.

Which option you would like to accept?
- Option-1: use fixed clock in this initial version, and will be
replaced later. 
uart_apb: clock-200000000 {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <200000000>;
	clock-output-names = "apb_pclk";
};

uart_clk: clock-200000000 {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <100000000>;
	clock-output-names = "uart_clk";
};

uart2: uart@040d0000 {
	compatible = "arm,pl011", "arm,primecell";
	reg = <0x0 0x040d0000 0x0 0x1000>;
	interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
	clock-names = "uartclk", "apb_pclk";
	clocks = <&uart_clk>, <&uart_apb>;
	status = "disabled";
};

- Option-2: delete the console uart node and its fixed clock.
In that way, the user could boot the kernel at orion-o6 board,
but could not use its console.

-- 

Best regards,
Peter

On 24/02/2025 11:39, Peter Chen wrote:
>>>>>>
>>>>>>> +       sky1_fixed_clocks: fixed-clocks {
>>>>>>> +               uartclk: uartclk {
>>>>>>> +                       compatible = "fixed-clock";
>>>>>>> +                       #clock-cells = <0>;
>>>>>>> +                       clock-frequency = <100000000>;
>>>>>>> +                       clock-output-names = "uartclk";
>>>>>>
>>>>>>> +               uart_apb_pclk: uart_apb_pclk {
>>>>>>> +                       compatible = "fixed-clock";
>>>>>>> +                       #clock-cells = <0>;
>>>>>>> +                       clock-frequency = <200000000>;
>>>>>>> +                       clock-output-names = "apb_pclk";
>>>>>>
>>>>>>
>>>>>> Clock names don't need "clk" in them, and there should
>>>>>> be no underscore -- use '-' instead of '_' when separating
>>>>>> strings in DT.
>>>>>
>>>>> Will change to:
>>>>> uart_apb: clock-uart-apb {
>>>>
>>>> No, instead explain why this is part of SoC - or what are you missing
>>>> here - and use preferred naming.
>>>
>>> It is in SoC part, APB clock uses to visit register, and the function
>>> amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
>>> device probes. It uses common Arm uart pl011 IP, the binding doc
>>> described at: Documentation/devicetree/bindings/serial/pl011.yaml
>>
>> So you added fake clock? Everything you wrote is not the reason to add
>> such clock.
> 
> Not a fake clock, it is the real clocks, but depends on firmware open
> their parents and configure their rate. It could let others do their

In one place you speak about UART, which is the consumer and not
relevant. Here you mention it is real clock. That's all confusing, so to
clarify:

We talk about clock which is generated/output by something. Something
which controls way it is generated is clock controller. Either you have
here crystal or have here clock controller. If first, fixed clock is for
that. If second, you need proper clock controller binding. You can add
stubs for missing pieces, but this requires explanation and TODO/FIXME
comment.




> upstream work based on workable console.
> 
> Which option you would like to accept?

You did not describe the hardware, so I have no clue what is there.

Best regards,
Krzysztof

On 25-02-24 13:07:45, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 24/02/2025 11:39, Peter Chen wrote:
> >>>>>>
> >>>>>>> +       sky1_fixed_clocks: fixed-clocks {
> >>>>>>> +               uartclk: uartclk {
> >>>>>>> +                       compatible = "fixed-clock";
> >>>>>>> +                       #clock-cells = <0>;
> >>>>>>> +                       clock-frequency = <100000000>;
> >>>>>>> +                       clock-output-names = "uartclk";
> >>>>>>
> >>>>>>> +               uart_apb_pclk: uart_apb_pclk {
> >>>>>>> +                       compatible = "fixed-clock";
> >>>>>>> +                       #clock-cells = <0>;
> >>>>>>> +                       clock-frequency = <200000000>;
> >>>>>>> +                       clock-output-names = "apb_pclk";
> >>>>>>
> >>>>>>
> >>>>>> Clock names don't need "clk" in them, and there should
> >>>>>> be no underscore -- use '-' instead of '_' when separating
> >>>>>> strings in DT.
> >>>>>
> >>>>> Will change to:
> >>>>> uart_apb: clock-uart-apb {
> >>>>
> >>>> No, instead explain why this is part of SoC - or what are you missing
> >>>> here - and use preferred naming.
> >>>
> >>> It is in SoC part, APB clock uses to visit register, and the function
> >>> amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
> >>> device probes. It uses common Arm uart pl011 IP, the binding doc
> >>> described at: Documentation/devicetree/bindings/serial/pl011.yaml
> >>
> >> So you added fake clock? Everything you wrote is not the reason to add
> >> such clock.
> >
> > Not a fake clock, it is the real clocks, but depends on firmware open
> > their parents and configure their rate. It could let others do their
> 
> In one place you speak about UART, which is the consumer and not
> relevant. Here you mention it is real clock. That's all confusing, so to
> clarify:
> 
> We talk about clock which is generated/output by something. Something
> which controls way it is generated is clock controller. Either you have
> here crystal or have here clock controller. If first, fixed clock is for
> that. If second, you need proper clock controller binding. You can add
> stubs for missing pieces, but this requires explanation and TODO/FIXME
> comment.
> 

Thanks for clarify, it is the second case, we have clock controller for
that, but now it is not ready to upstream due to some dependency, like
mailbox device driver, I will drop UART node at v2 patch set.

-- 

Best regards,
Peter