| 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: <c1259bad-be4d-4489-840d-4ab2f4e466f2@arm.com>
Date: Fri, 30 Jan 2026 09:58:56 +0000
From: Debbie Horsfall <debbie.horsfall@....com>
To: Andre Przywara <andre.przywara@....com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Liviu Dudau <liviu.dudau@....com>,
Sudeep Holla <sudeep.holla@....com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] arm64: dts: zena: Add support for Zena CSS
On 27/01/2026 13:22, Andre Przywara wrote:
> On Fri, 23 Jan 2026 17:37:47 +0000
> Debbie Horsfall <debbie.horsfall@....com> wrote:
>
> Hi Debbie,
>
> thanks for taking the time to send this upstream!
>
Thank you for reviewing it! I have responded to all of your points and
will send a v2 patch set.
>> Introduce the Zena CSS Fixed Virtual Platform (FVP) dts. This is
>> currently the only Zena CSS variant, however the common definitions are
>> included in a common dtsi for extensibility.
>>
>> Signed-off-by: Debbie Horsfall <debbie.horsfall@....com>
>> ---
>> MAINTAINERS | 1 +
>> arch/arm64/boot/dts/arm/Makefile | 1 +
>> arch/arm64/boot/dts/arm/zena-css-fvp.dts | 55 ++
>> arch/arm64/boot/dts/arm/zena-css.dtsi | 826 +++++++++++++++++++++++++++++++
>> 4 files changed, 883 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 90d88137adf1..d1d2dae6a71e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3727,6 +3727,7 @@ ARM/ZENA CSS PLATFORM
>> M: Debbie Horsfall <debbie.horsfall@....com>
>> S: Maintained
>> F: Documentation/devicetree/bindings/arm/arm,zena-css.yaml
>> +F: arch/arm64/boot/dts/arm/zena-css*
>>
>> ARM/ZYNQ ARCHITECTURE
>> M: Michal Simek <michal.simek@....com>
>> diff --git a/arch/arm64/boot/dts/arm/Makefile b/arch/arm64/boot/dts/arm/Makefile
>> index f30ee045dc95..770fb145b4a9 100644
>> --- a/arch/arm64/boot/dts/arm/Makefile
>> +++ b/arch/arm64/boot/dts/arm/Makefile
>> @@ -8,3 +8,4 @@ dtb-$(CONFIG_ARCH_VEXPRESS) += vexpress-v2f-1xv7-ca53x2.dtb
>> dtb-$(CONFIG_ARCH_VEXPRESS) += fvp-base-revc.dtb
>> dtb-$(CONFIG_ARCH_VEXPRESS) += corstone1000-fvp.dtb corstone1000-mps3.dtb
>> dtb-$(CONFIG_ARCH_VEXPRESS) += morello-sdp.dtb morello-fvp.dtb
>> +dtb-$(CONFIG_ARCH_VEXPRESS) += zena-css-fvp.dtb
>> diff --git a/arch/arm64/boot/dts/arm/zena-css-fvp.dts b/arch/arm64/boot/dts/arm/zena-css-fvp.dts
>> new file mode 100644
>> index 000000000000..d3c649e894d1
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/arm/zena-css-fvp.dts
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>> +/*
>> + * Copyright (c) 2025, Arm Limited. All rights reserved.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "zena-css.dtsi"
>> +
>> +/ {
>> + model = "Zena CSS Fixed Virtual Platform";
>> + compatible = "arm,zena-css-fvp", "arm,zena-css";
>> +
>> + chosen {
>> + stdout-path = &soc_serial0;
>> + };
>> +};
>> +
>> +&soc {
>> + virtio@...60000 {
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30060000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + virtio@...20000 {
>
> I think the nodes should be ordered by their address. Do you make any
> assumptions about naming of devices (/dev/vda, /dev/vdb) in your setup?
>
I will move virtio@...60000 into order. I don't think there are any
assumptions on device naming dependent on ordering but I will run all of
our tests with them in address order to check.
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30020000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + virtio@...30000 {
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30030000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + virtio@...40000 {
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30040000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + virtio@...50000 {
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30050000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>
> Do you know if there is something at 0x30070000? Maybe something that
> needs explicit enablement on the model command line? In this case we might
> want a comment here.
>
The memory map documentation says there is no device there.
>> + virtio@...80000 {
>> + compatible = "virtio,mmio";
>> + reg = <0x0 0x30080000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +};
>> diff --git a/arch/arm64/boot/dts/arm/zena-css.dtsi b/arch/arm64/boot/dts/arm/zena-css.dtsi
>> new file mode 100644
>> index 000000000000..7825e93df0a6
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/arm/zena-css.dtsi
>> @@ -0,0 +1,826 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>> +/*
>> + * Copyright (c) 2025, Arm Limited. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> + interrupt-parent = <&gic>;
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + cpus {
>> + #address-cells = <2>;
>> + #size-cells = <0>;
>> +
>> + /*
>> + * The latency and residency numbers below are for illustrative
>> + * purpose only and may vary on actual silicon. These values are
>> + * considered just to demonstrate that the cpuidle governor
>> + * logic works.
>> + */
>> + idle-states {
>> + entry-method = "psci";
>> +
>> + CPU_SLEEP: cpu-sleep {
>> + compatible = "arm,idle-state";
>> + arm,psci-suspend-param = <0x0010000>;
>> + local-timer-stop;
>> + entry-latency-us = <800>;
>> + exit-latency-us = <3200>;
>> + min-residency-us = <4200>;
>> + };
>> + CLUSTER_SLEEP: cluster-sleep {
>> + compatible = "arm,idle-state";
>> + arm,psci-suspend-param = <0x1010000>;
>> + local-timer-stop;
>> + entry-latency-us = <1000>;
>> + exit-latency-us = <3200>;
>> + min-residency-us = <4500>;
>> + };
>> + };
>> +
>> + 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>;
>> + };
>> + };
>> +
>> + cluster2 {
>> +
>> + core0 {
>> + cpu = <&CPU8>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU9>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU10>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU11>;
>> + };
>> + };
>> +
>> + cluster3 {
>> +
>> + core0 {
>> + cpu = <&CPU12>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU13>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU14>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU15>;
>> + };
>> + };
>> + };
>> +
>> + CPU0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a720ae";
>> + reg = <0x00 0x00>;
>> + enable-method = "psci";
>> + i-cache-size = <0x10000>;
>> + i-cache-line-size = <0x40>;
>> + i-cache-sets = <0x100>;
>
> So for those numbers that are meaningful in decimal (64 bytes cache line,
> 256 sets), I think it's better to express them in decimal directly.
> For the small size here you could do it as well, though for the bigger
> sizes (L2 and L3 below) it's probably better left in hex.
>
Will do.
>> + d-cache-size = <0x10000>;
>> + d-cache-line-size = <0x40>;
>> + d-cache-sets = <0x100>;
>> + clocks = <&scmi_dvfs 0x00>;
>> + cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + next-level-cache = <&CL0_L2_0>;
>> +
>> + CL0_L2_0: l2-cache {
>> + compatible = "cache";
>> + cache-unified;
>> + cache-level = <0x02>;
>
> Same here, cache-level should be just <2>.
>
Will do.
>> + /* 512KB */
>> + cache-size = <0x80000>;
>
> Please add the comment right behind the number, that disrupts the flow
> less, I feel.
>
Will do
>> + /* 64B */
>> + cache-line-size = <0x40>;
>
> ... and as above, just write <64>, also allows you to lose the comment.
>
Will do.
>> + /* 8-way set */
>> + cache-sets = <0x400>;
>> + next-level-cache = <&CL0_L3>;
>> + };
>> + };
>> +
>> + CPU1: cpu@100 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a720ae";
>> + reg = <0x00 0x100>;
>
> for the records: I verified that the other CPU nodes are the same, except
> for their obvious differences in cache numbers and MPIDRs.
>
> <snip>
>
Thank you.
>> +
>> + CL0_L3: l3-cache0 {
>> + compatible = "cache";
>> + cache-unified;
>> + cache-level = <0x03>;
>> + /* 4MB */
>> + cache-size = <0x400000>;
>> + /* 64B */
>> + cache-line-size = <0x40>;
>> + /* 16-way set */
>> + cache-sets = <0x1000>;
>> + };
>> +
>> + CL1_L3: l3-cache1 {
>> + compatible = "cache";
>> + cache-unified;
>> + cache-level = <0x03>;
>> + /* 4MB */
>> + cache-size = <0x400000>;
>> + /* 64B */
>> + cache-line-size = <0x40>;
>> + /* 16-way set */
>> + cache-sets = <0x1000>;
>> + };
>> +
>> + CL2_L3: l3-cache2 {
>> + compatible = "cache";
>> + cache-unified;
>> + cache-level = <0x03>;
>> + /* 4MB */
>> + cache-size = <0x400000>;
>> + /* 64B */
>> + cache-line-size = <0x40>;
>> + /* 16-way set */
>> + cache-sets = <0x1000>;
>> + };
>> +
>> + CL3_L3: l3-cache3 {
>> + compatible = "cache";
>> + cache-unified;
>> + cache-level = <0x03>;
>> + /* 4MB */
>> + cache-size = <0x400000>;
>> + /* 64B */
>> + cache-line-size = <0x40>;
>> + /* 16-way set */
>> + cache-sets = <0x1000>;
>> + };
>> + };
>> +
>> + dsu-pmu-0 {
>> + compatible = "arm,dsu-pmu";
>> + cpus = <&CPU0 &CPU1 &CPU2 &CPU3>;
>> + interrupts = <GIC_SPI 216 IRQ_TYPE_EDGE_RISING>;
>
> Shouldn't that IRQ number be 184? According to the (internal) IRQ
> map document this lists 216 as the GIC interrupt ID, but the SPI ID would
> then need to be 32 less, so 184 (the column next to it in the spreadsheet).
> Same for the other DSU PMUs, but the other SPIs seem to be correct (timer,
> UART, ...).
>
Thank you for noticing this - it is an issue but wasn't picked up by our
tests. I will change them.
>> + };
>> +
>> + dsu-pmu-1 {
>> + compatible = "arm,dsu-pmu";
>> + cpus = <&CPU4 &CPU5 &CPU6 &CPU7>;
>> + interrupts = <GIC_SPI 217 IRQ_TYPE_EDGE_RISING>;
>> + };
>> +
>> + dsu-pmu-2 {
>> + compatible = "arm,dsu-pmu";
>> + cpus = <&CPU8 &CPU9 &CPU10 &CPU11>;
>> + interrupts = <GIC_SPI 218 IRQ_TYPE_EDGE_RISING>;
>> + };
>> +
>> + dsu-pmu-3 {
>> + compatible = "arm,dsu-pmu";
>> + cpus = <&CPU12 &CPU13 &CPU14 &CPU15>;
>> + interrupts = <GIC_SPI 219 IRQ_TYPE_EDGE_RISING>;
>> + };
>> +
>> + memory@...00000 {
>> + device_type = "memory";
>> +
>> + /* Bank 0: start = 0x0000_0000_8000_0000, size = ~2 GiB (0x7F00_0000) */
>
> That comment is somewhat redundant, as it mirrors the line below.
> If we need a comment, I'd suggest something like: ~2GB mapped at 2GB,
> another 2GB at 2TB.
>
Thanks, I will take the new comment suggestion.
>> + reg = <
>> + 0x00000000 0x80000000 0x00000000 0x7F000000
>> + 0x00000200 0x00000000 0x00000000 0x80000000
>> + >;
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
>> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
>> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
>> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>,
>> + <GIC_PPI 12 IRQ_TYPE_LEVEL_LOW>;
>> + };
>> +
>> + soc_clk24mhz: clock-24000000 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <24000000>;
>> + clock-output-names = "refclk24mhz";
>> + };
>> +
>> + soc: soc {
>> + compatible = "simple-bus";
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + timer@...10000 {
>> + compatible = "arm,armv7-timer-mem";
>> + reg = <0x0 0x1a810000 0 0x10000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + /* Map child space [0x0..0x30000) to parent @ 0x1a810000 */
>> + ranges = <0x0 0x0 0x1a810000 0x00030000>;
>> +
>> + frame@...00 {
>> + frame-number = <0>;
>> + interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>> + reg = <0x20000 0x10000>;
>> + };
>> + };
>> +
>> + gic: interrupt-controller@...00000 {
>> + compatible = "arm,gic-v3";
>> + #redistributor-regions = <16>;
>> + reg = <0x0 0x20800000 0x0 0x10000>, /* GICD */
>> + <0x0 0x20880000 0x0 0x40000>, /* 16 * GICR */
>> + <0x0 0x208c0000 0x0 0x40000>,
>
> Those look as if they are all contiguous, aren't they?
> Then you wouldn't need the #redistributor-regions property above, and can
> just go with one big GICR region.
>
This is a workaround for the AP GIC Multiview. Is it acceptable?
>> + <0x0 0x20900000 0x0 0x40000>,
>> + <0x0 0x20940000 0x0 0x40000>,
>> + <0x0 0x20980000 0x0 0x40000>,
>> + <0x0 0x209c0000 0x0 0x40000>,
>> + <0x0 0x20a00000 0x0 0x40000>,
>> + <0x0 0x20a40000 0x0 0x40000>,
>> + <0x0 0x20a80000 0x0 0x40000>,
>> + <0x0 0x20ac0000 0x0 0x40000>,
>> + <0x0 0x20b00000 0x0 0x40000>,
>> + <0x0 0x20b40000 0x0 0x40000>,
>> + <0x0 0x20b80000 0x0 0x40000>,
>> + <0x0 0x20bc0000 0x0 0x40000>,
>> + <0x0 0x20c00000 0x0 0x40000>,
>> + <0x0 0x20c40000 0x0 0x40000>;
>> + #interrupt-cells = <3>;
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> + interrupt-controller;
>> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + its1: msi-controller@...40000 {
>
> Are there multiple ITSes, and some are just not shown here?
> If not, please just use "its" as the label name.
>
There's only one so I'll rename it.
>> + compatible = "arm,gic-v3-its";
>> + reg = <0x0 0x20840000 0x0 0x40000>;
>> + msi-controller;
>> + #msi-cells = <1>;
>> + };
>> + };
>> +
>> + /* UART is fixed as 24MHz, both UARTCLK and PCLK */
>> + soc_serial0: serial@...00000 {
>> + compatible = "arm,pl011", "arm,primecell";
>> + reg = <0x0 0x1a400000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&soc_clk24mhz>, <&soc_clk24mhz>;
>> + clock-names = "uartclk", "apb_pclk";
>> + };
>> +
>> + watchdog@...20000 {
>> + compatible = "arm,sbsa-gwdt";
>> + reg = <0x0 0x1a420000 0x0 0x10000>,
>> + <0x0 0x1a430000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + rtc@...d0000 {
>> + compatible = "arm,pl031", "arm,primecell";
>> + reg = <0x0 0x300d0000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;
>
> Can you please double check this interrupt ID? The IRQ mapping document
> just lists some "expansion range" here, but I cannot verify if this is
> using the SPI offset of 32 or not.
>
I have confirmed with our interrupt map.
>> + clocks = <&soc_clk24mhz>;
>> + clock-names = "apb_pclk";
>> + };
>> +
>> + };
>> +
>> + psci {
>> + compatible = "arm,psci-1.0", "arm,psci-0.2", "arm,psci";
>
> You don't need compatibility to the pre 0.2 PSCI standard, so drop the
> last compatible name.
>
I will remove this.
>> + method = "smc";
>> + cpu_suspend = <0xc4000001>;
>> + cpu_off = <0x84000002>;
>> + cpu_on = <0xc4000003>;
>
> And those three function IDs are only needed for this pre-0.2 name, so you
> can remove them.
>
I will remove them.
>> + };
>> +
>> + sram: sram@...000 {
>> + compatible = "mmio-sram";
>> + reg = <0x0 0x104000 0x0 0x00001000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0x0 0x104000 0x00001000>;
>> +
>> + scmi_shmem_tx: scpshmem-sram-section@0 {
>> + compatible = "arm,scmi-shmem";
>> + reg = <0x0 0x100>;
>> + };
>> + scmi_shmem_rx: scpshmem-sram-section@100 {
>> + compatible = "arm,scmi-shmem";
>> + reg = <0x100 0x100>;
>> + };
>> + };
>> +
>> + mbox_db_tx: mailbox@...20000 {
>> + compatible = "arm,mhuv3";
>> + reg = <0x0 0x40020000 0x0 0x30000>;
>> + clocks = <&soc_clk24mhz>;
>> + #mbox-cells = <3>;
>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "combined";
>> + };
>> +
>> + mbox_db_rx: mailbox@...60000 {
>> + compatible = "arm,mhuv3";
>> + reg = <0x0 0x40060000 0x0 0x30000>;
>> + clocks = <&soc_clk24mhz>;
>> + #mbox-cells = <3>;
>> + interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "combined";
>> + };
>> +
>> + firmware {
>> + scmi {
>> + compatible = "arm,scmi";
>> + mbox-names = "tx", "rx";
>> + mboxes = <&mbox_db_tx 0 0 0 &mbox_db_rx 0 0 0 &mbox_db_rx 0 0 2>;
>
> What is this third mailbox about? I think this would have to match
> mbox-names also? I guess this is not needed?
>
The team is confirming which mbox names are appropriate. To pass the
Devicetree validation the only valid combination of three is: "tx",
"tx_reply", "rx". However, the second mbox needs to be rx else the SCMI
communication fails. I'll investigate further and make sure the names match.
> Cheers,
> Andre
>
>> + shmem = <&scmi_shmem_tx &scmi_shmem_rx>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + scmi_dvfs: protocol@13 {
>> + reg = <0x13>;
>> + #clock-cells = <1>;
>> + };
>> + };
>> + };
>> +};
>>
>
--
Kind regards,
Debbie
Powered by blists - more mailing lists