Re: [PATCH] arm64: versal-net: Add retention state for cpu idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The CPU_RETENTION_0 state duplicates the built-in WFI state and fails to avoid firmware dependency, contradicting the patch's stated goal.
--

--- Patch [1]: [PATCH] arm64: versal-net: Add retention state for cpu idle ---
commit 945569ec7efab4f9ca915d207005b24c2747571f
Author: Jay Buddhabhatti <jay.buddhabhatti@amd.com>

arm64: versal-net: Add retention state for cpu idle

This commit replaces the sleep state with a retention state for CPU idle in
the versal-net device tree. The stated goal is to avoid firmware dependencies
by using a PSCI suspend parameter of 0, intending for the core to simply
execute WFI without powering off.

> diff --git a/arch/arm64/boot/dts/xilinx/versal-net.dtsi b/arch/arm64/boot/dts/xilinx/versal-net.dtsi
> --- a/arch/arm64/boot/dts/xilinx/versal-net.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/versal-net.dtsi
[ ... ]
> @@ -636,13 +636,12 @@ llc: l4-cache { /* LLC inside CMN */
>  		idle-states {
>  			entry-method = "psci";
>  
> -			CPU_SLEEP_0: cpu-sleep-0 {
> +			CPU_RETENTION_0: cpu-retention-0 {
>  				compatible = "arm,idle-state";
> -				arm,psci-suspend-param = <0x40000000>;
> -				local-timer-stop;
> -				entry-latency-us = <300>;
> -				exit-latency-us = <600>;
> -				min-residency-us = <10000>;
> +				arm,psci-suspend-param = <0>;
> +				entry-latency-us = <1>;
> +				exit-latency-us = <1>;
> +				min-residency-us = <1>;
>  			};

[Severity: Medium]
The commit message states that this state avoids dependencies on firmware
because the core simply executes WFI. However, does defining this PSCI idle
state still trigger an SMC call to EL3?

Looking at the cpuidle-psci driver, any registered PSCI idle state invokes
psci_cpu_suspend_enter(), which issues a CPU_SUSPEND SMC call. This implies
the firmware would still be involved on every idle entry, conflicting with
the stated goal.

Additionally, the cpuidle-psci driver already unconditionally creates a
default WFI state at index 0. Does adding CPU_RETENTION_0 with
arm,psci-suspend-param = <0> create a duplicate, functionally identical
idle state in the cpuidle governor?

If the genuine goal is to execute a native hardware WFI without trapping
to firmware, should the cpu-idle-states property be omitted from the CPU
nodes entirely? This would cause the cpuidle-psci driver to fail to probe,
allowing the kernel to natively fall back to arch_cpu_idle() which invokes
the wfi instruction directly from EL1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727121842.3410495-1-jay.buddhabhatti@amd.com?part=1