Re: [PATCH v2 1/2] arm64: dts: S32G3: Introduce device tree for S32G-VNP-RDB3

From: Ghennadi Procopciuc
Date: Wed Mar 20 2024 - 03:00:37 EST

On 3/20/24 00:16, Wadim Mueller wrote:
[...]
> +++ b/arch/arm64/boot/dts/freescale/s32g3.dtsi
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2021-2023 NXP
> + *
> + * Authors: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
> + * Ciprian Costea <ciprianmarian.costea@xxxxxxx>
> + * Andra-Teodora Ilie <andra.ilie@xxxxxxx>

This pollutes the content of the file. Please make them part of the
commit description using 'Signed-off-by' tags.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +#include <dt-bindings/interrupt-controller/arm-gic.h>

Missing empty line ?

> +/ {
> + compatible = "nxp,s32g3";
> + interrupt-parent = <&gic>;
> + #address-cells = <0x02>;
> + #size-cells = <0x02>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&cpu0>;
> + };
> +
> + core1 {
> + cpu = <&cpu1>;
> + };
> +
> + core2 {
> + cpu = <&cpu2>;
> + };
> +
> + core3 {
> + cpu = <&cpu3>;
> + };
> + };
> +
> + cluster1 {
> + core0 {
> + cpu = <&cpu4>;
> + };
> +
> + core1 {
> + cpu = <&cpu5>;
> + };
> +
> + core2 {
> + cpu = <&cpu6>;
> + };
> +
> + core3 {
> + cpu = <&cpu7>;
> + };
> + };
> + };
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x0>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu1: cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x1>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu2: cpu@2 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x2>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu3: cpu@3 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x3>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu4: cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x100>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu5: cpu@101 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x101>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu6: cpu@102 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x102>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> +
> + cpu7: cpu@103 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x103>;
> + enable-method = "psci";
> + clocks = <&dfs 0>;
> + };
> + };
> +
> + pmu {
> + compatible = "arm,cortex-a53-pmu";
> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, /* sec-phys */
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, /* virt */
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, /* hyp-virt */
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>, /* sec-phys */
> + <GIC_PPI 12 IRQ_TYPE_LEVEL_LOW>; /* phys */

The interrupt order does not seem right.
>From drivers/clocksource/arm_arch_timer.c:

static const char *arch_timer_ppi_names[ARCH_TIMER_MAX_TIMER_PPI] = {
[ARCH_TIMER_PHYS_SECURE_PPI] = "sec-phys",
[ARCH_TIMER_PHYS_NONSECURE_PPI] = "phys",
[ARCH_TIMER_VIRT_PPI] = "virt",
[ARCH_TIMER_HYP_PPI] = "hyp-phys",
[ARCH_TIMER_HYP_VIRT_PPI] = "hyp-virt",
};

[...]

static int __init arch_timer_of_init(struct device_node *np)
{
[...]

for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI;
i++) {
if (has_names)
irq = of_irq_get_byname(np, arch_timer_ppi_names[i]);
else
irq = of_irq_get(np, i);
if (irq > 0)
arch_timer_ppi[i] = irq;

}
[...]
}

As I understand it, if the names are not provided, then the interrupt
IDs must strictly follow the order specified in the arch_timer_ppi_names
list, which is: sec-phys, phys, virt, hyp-phys, hyp-virt. That is why I
personally prefer using interrupt names instead of the raw list of IDs.

> + arm,no-tick-in-suspend;
> + };
> +
> + reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + scmi_shmem: shm@d0000000 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x0 0xd0000000 0x0 0x80>;
> + no-map;
> + };
> + };
> +
> + firmware {
> + scmi: scmi {
> + compatible = "arm,scmi-smc";
> + shmem = <&scmi_shmem>;
> + arm,smc-id = <0xc20000fe>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;

This interrupt is not needed if the RX is not used.

> +
> + dfs: protocol@13 {
> + reg = <0x13>;
> + #clock-cells = <1>;
> + };
> +
> + clks: protocol@14 {
> + reg = <0x14>;
> + #clock-cells = <1>;
> + };
> + };
> +
> + psci: psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> + };
> +
> + soc@0 {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0 0 0x80000000>;
> +
> + uart0: serial@401c8000 {
> + compatible = "nxp,s32g3-linflexuart",

>From 'Submitting Devicetree (DT) binding patches' [0]:
5. The Documentation/ portion of the patch should come in the series
before the code implementing the binding.

> + "fsl,s32v234-linflexuart";
> + reg = <0x401c8000 0x3000>;
> + interrupts = <GIC_SPI 82 IRQ_TYPE_EDGE_RISING>;
> + status = "disabled";
> + };

Krzysztof, would it make sense to factor the common part of the s32g3
and s32g2 into a new dtsi file (e.g. s32g.dtsi) since both SoCs are part
of the same family and share the memory map with some exceptions?

> +
> + uart1: serial@401cc000 {
> + compatible = "nxp,s32g3-linflexuart",
> + "fsl,s32v234-linflexuart";
> + reg = <0x401cc000 0x3000>;
> + interrupts = <GIC_SPI 83 IRQ_TYPE_EDGE_RISING>;
> + status = "disabled";
> + };
> +
> + uart2: serial@402bc000 {
> + compatible = "nxp,s32g3-linflexuart",
> + "fsl,s32v234-linflexuart";
> + reg = <0x402bc000 0x3000>;
> + interrupts = <GIC_SPI 84 IRQ_TYPE_EDGE_RISING>;
> + status = "disabled";
> + };
> +
> + gic: interrupt-controller@50800000 {
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + reg = <0x50800000 0x10000>,
> + <0x50900000 0x200000>,
> + <0x50400000 0x2000>,
> + <0x50410000 0x2000>,
> + <0x50420000 0x2000>;
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + usdhc0: mmc@402f0000 {
> + compatible = "nxp,s32g2-usdhc";

Is there a reason why the UART uses an S32G3 specific compatible string,
but the SD does not?

> + reg = <0x402f0000 0x1000>;
> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clks 32>,
> + <&clks 31>,
> + <&clks 33>;
> + clock-names = "ipg", "ahb", "per";
> + status = "disabled";
> + };

Missing bus-width = <8> ?

> + };
> +};
> diff --git a/arch/arm64/boot/dts/freescale/s32g399a-rdb3.dts b/arch/arm64/boot/dts/freescale/s32g399a-rdb3.dts
> new file mode 100644
> index 000000000000..199605b28df3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/s32g399a-rdb3.dts
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2021-2023 NXP
> + *
> + * NXP S32G3 Reference Design Board 3 (S32G-VNP-RDB3)
> + */
> +
> +/dts-v1/;
> +
> +#include "s32g3.dtsi"
> +
> +/ {
> + model = "NXP S32G3 Reference Design Board 3 (S32G-VNP-RDB3)";
> + compatible = "nxp,s32g399a-rdb3", "nxp,s32g3";
> +
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart1;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + /* 4GiB RAM */
> + memory@80000000 {
> + device_type = "memory";
> + reg = <0x0 0x80000000 0 0x80000000>,
> + <0x8 0x80000000 0 0x80000000>;
> + };
> +};
> +
> +&uart0 {
> + status = "okay";
> +};
> +
> +&uart1 {
> + status = "okay";
> +};
> +
> +&usdhc0 {
> + status = "okay";
> +}
[0]
https://docs.kernel.org/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Regards,
Ghennadi