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

[Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>]

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@nxp.com>
> + *          Ciprian Costea <ciprianmarian.costea@nxp.com>
> + *          Andra-Teodora Ilie <andra.ilie@nxp.com>

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel