Re: [PATCH v2 4/6] arm64: dts: axiado: Add initial support for AX3000 SoC and eval board

[Harshit Shah <hshah@axiado.com>]

Hi Krzysztof,

Thank you very much for the reviews.

On 6/15/2025 11:09 PM, Krzysztof Kozlowski wrote:
> On 16/06/2025 06:31, Harshit Shah wrote:
> +             serial0 = &uart0;
> +             serial1 = &uart1;
> +             serial2 = &uart2;
> +             serial3 = &uart3;
> None of these are properties of SoC, but board. Move respective aliases
> to the board files.

Make sense. I will move it to the board file. Also I will only keep that 
we use in the board file.

>> +
>> +     timer:timer {
> Missing space before node name, but anyway label is unused.

Noted.

>> +             compatible = "arm,armv8-timer";
>> +             interrupt-parent = <&gic500>;
>> +             interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <GIC_PPI 14 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> +             arm,cpu-registers-not-fw-configured;
>> +     };
>> +
>> +     clocks {
> Keep proper sorting of nodes, see DTS coding style.

Ah, we missed that. thank you. I will keep the nodes as per the 
alphabetical order.

=====
cpus

clocks

soc {

     gic

     gpios

     i3cs

     uarts

}

timer

=====

>> +             refclk: refclk {
>> +                     compatible = "fixed-clock";
>> +                     #clock-cells = <0>;
>> +                     clock-frequency = <125000000>;
>> +             };
>> +
>> +             ref_clk: ref_clk {
> This makes no sense. You have refclk and ref_clk. These ARE THE SAME.
> 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
Thank you for the document, we will follow this document and will remove 
redundant nodes.
>> +                     compatible = "fixed-clock";
>> +                     #clock-cells = <0>;
>> +                     clock-frequency = <1>;
>> +             };
>> +
>> +             spi_clk: spi_clk {
>> +                     compatible = "fixed-clock";
>> +                     #clock-cells = <0>;
>> +                     clock-frequency = <25000000>;
>> +             };
>> +
>> +             apb_pclk: apb_pclk {
> No underscores in node names, but all these look incorrect - don't you
> have clock controller?
Noted, we will remove the "_" from the nodes. We do have clock 
controller however that is being accessed by other CPU before Linux will 
come-up.

So, the purpose of this clock nodes is to calculate the frequencies for 
other peripherals. (We will update the nodes with clock-<freq>)


>> +                     compatible = "fixed-clock";
>> +                     #clock-cells = <0>;
>> +                     clock-frequency = <25000000>;
>> +             };
>> +     };
>> +
>> +     soc {
>> +             compatible = "simple-bus";
>> +             #address-cells = <2>;
>> +             #size-cells = <2>;
>> +             interrupt-parent = <&gic500>;
>> +             ranges;
>> +
>> +             gic500: interrupt-controller@80300000 {
>> +                     compatible = "arm,gic-v3";
>> +                     #address-cells = <2>;
>> +                     #size-cells = <2>;
>> +                     ranges;
>> +                     #interrupt-cells = <3>;
>> +                     interrupt-controller;
>> +                     #redistributor-regions = <1>;
>> +                     reg = <0x00 0x80300000 0x00 0x10000>,
>> +                               <0x00 0x80380000 0x00 0x80000>;
> DTS coding style, incorrect order.
Thank you, we are following this reference: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml?h=v6.11-rc1#n242

gic500: interrupt-controller@80300000 {
                         compatible = "arm,gic-v3";
                         #interrupt-cells = <3>;
                         #address-cells = <2>;
                         #size-cells = <2>;
                         ranges;
                         interrupt-controller;
                         #redistributor-regions = <1>;
                         reg = <0x00 0x80300000 0x00 0x10000>,
                                   <0x00 0x80380000 0x00 0x80000>;
                         interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;

                 };


>
>> +                     interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +             };
>> +
>> +             uart0: serial@80520000 {
>> +                     compatible = "xlnx,zynqmp-uart", "cdns,uart-r1p12";
>> +                     interrupt-parent = <&gic500>;
>> +                     interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
>> +                     reg = <0x00 0x80520000 0x00 0x100>;
> DTS coding style.

Got it. Will move "reg = " in the second line.


>
>> +
>> +
>> +             /* GPIO Controller banks 0 - 7 */
>> +             gpio0: gpio-controller@80500000 {
>> +                     compatible = "cdns,gpio-r1p02";
>> +                     clocks = <&refclk>;
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +                     interrupt-parent = <&gic500>;
>> +                     interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>;
>> +                     reg = <0x00 0x80500000 0x00  0x400>;
> DTS coding style.

Noted, will update on this and other nodes.


>
>
>
>
>> +
>> +                     status = "disabled";
>> +             };
>> +
>> +             i3c16: i3c@80620400 {
>> +                     compatible = "cdns,i3c-master";
>> +                     clocks = <&refclk &clk_xin>;
>> +                     clock-names = "pclk", "sysclk";
>> +                     interrupt-parent = <&gic500>;
>> +                     interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
>> +                     i2c-scl-hz = <100000>;
>> +                     i3c-scl-hz = <400000>;
>> +                     reg = <0x00 0x80620400 0x00 0x400>;
>> +                     #address-cells = <3>;
>> +                     #size-cells = <0>;
>> +                     status = "disabled";
>> +             };
>> +
> Drop stray blank lines.
Okay, make sense.
>
>> +     };
>> +};
>> diff --git a/arch/arm64/boot/dts/axiado/ax3000_evk.dts b/arch/arm64/boot/dts/axiado/ax3000_evk.dts
>>
>> +
>> +     chosen {
>> +             bootargs = "console=ttyPS3,115200 earlyprintk nr_cpus=4 earlycon";
> Drop bootargs. Not needed and not suitable for mainline. earlycon (not
> earlyprintk!) is debugging tool, not wide mainline usage.
Make sense. I will remove the bootargs.
> Best regards,
> Krzysztof

Regards,

Harshit.