Re: [PATCH 6/6] arm64: dts: cix: add initial CIX P1(SKY1) dts support

["Arnd Bergmann" <arnd@arndb.de>]

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