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:

quoted

+#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";

quoted

+
+       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.

quoted

+               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";'

quoted

+       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.

quoted

+               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.

quoted

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

quoted

+               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";

};

quoted

+       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