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

[Krzysztof Kozlowski <krzk@kernel.org>]

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