On 20/02/2025 13:30, Peter Chen wrote:

quoted

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.

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.

quoted

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

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

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

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 {

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