| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a5268bcc-144b-4e86-a13a-33d976368f06@oss.nxp.com>
Date: Wed, 20 Mar 2024 09:00:16 +0200
From: Ghennadi Procopciuc <ghennadi.procopciuc@....nxp.com>
To: Wadim Mueller <wafgo01@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, NXP Linux Team <linux-imx@....com>,
Chester Lin <chester62515@...il.com>, Andreas Färber
<afaerber@...e.de>, Matthias Brugger <mbrugger@...e.com>,
NXP S32 Linux Team <s32@....com>, Tim Harvey <tharvey@...eworks.com>,
Alexander Stein <alexander.stein@...tq-group.com>,
Marek Vasut <marex@...x.de>,
Gregor Herburger <gregor.herburger@...tq-group.com>,
Marco Felsch <m.felsch@...gutronix.de>,
Joao Paulo Goncalves <joao.goncalves@...adex.com>,
Markus Niebel <Markus.Niebel@...tq-group.com>,
Matthias Schiffer <matthias.schiffer@...group.com>,
Stefan Wahren <stefan.wahren@...rgebyte.com>,
Bjorn Helgaas <bhelgaas@...gle.com>, Josua Mayer <josua@...id-run.com>,
Yannic Moog <y.moog@...tec.de>, Li Yang <leoyang.li@....com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/2] arm64: dts: S32G3: Introduce device tree for
S32G-VNP-RDB3
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@....com>
> + * Ciprian Costea <ciprianmarian.costea@....com>
> + * Andra-Teodora Ilie <andra.ilie@....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@...00000 {
> + 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@...c8000 {
> + 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@...cc000 {
> + compatible = "nxp,s32g3-linflexuart",
> + "fsl,s32v234-linflexuart";
> + reg = <0x401cc000 0x3000>;
> + interrupts = <GIC_SPI 83 IRQ_TYPE_EDGE_RISING>;
> + status = "disabled";
> + };
> +
> + uart2: serial@...bc000 {
> + compatible = "nxp,s32g3-linflexuart",
> + "fsl,s32v234-linflexuart";
> + reg = <0x402bc000 0x3000>;
> + interrupts = <GIC_SPI 84 IRQ_TYPE_EDGE_RISING>;
> + status = "disabled";
> + };
> +
> + gic: interrupt-controller@...00000 {
> + 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@...f0000 {
> + 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@...00000 {
> + 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
Powered by blists - more mailing lists