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

[Peter Chen <peter.chen@cixtech.com>]

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