Re: [PATCH] arm64: dts: st: add PSCI idle state to STM32MP23 CPU domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: st: stm32mp231 cpu-sleep local-timer-stop conflicts with always-on
--

commit 54de3bc82d13d38707edd20793e9450770876715
Author: Patrick Delaunay <patrick.delaunay@foss.st.com>

arm64: dts: st: add PSCI idle state to STM32MP23 CPU domains

This commit adds a PSCI CPU suspend idle-state node to stm32mp231.dtsi and
references it from the STM32MP23 CPU power domains so the CPU power-domain
governor can manage them in OSI mode.

> diff --git a/arch/arm64/boot/dts/st/stm32mp231.dtsi b/arch/arm64/boot/dts/st/stm32mp231.dtsi
> index fefbf6f4f1537..27231e140fbb5 100644
> --- a/arch/arm64/boot/dts/st/stm32mp231.dtsi
> +++ b/arch/arm64/boot/dts/st/stm32mp231.dtsi
> @@ -24,6 +24,19 @@ cpu0: cpu@0 {
>  			power-domains = <&cpu0_pd>;
>  			power-domain-names = "psci";
>  		};
> +
> +		idle-states {
> +			entry-method = "psci";
> +
> +			cpu_sleep: cpu-sleep {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x00000002>;
> +				local-timer-stop;
> +				entry-latency-us = <150>;
> +				exit-latency-us = <200>;
> +				min-residency-us = <1000>;
> +			};
> +		};

[Severity: High]
Does setting local-timer-stop on cpu_sleep while the timer node in
arch/arm64/boot/dts/st/stm32mp231.dtsi still specifies always-on cause CPUs
entering cpu-sleep to miss timer wakeups and hang in idle?

In arch/arm64/boot/dts/st/stm32mp231.dtsi, the architected timer node has the
always-on property:

	timer {
		compatible = "arm,armv8-timer";
		...
		always-on;
	};

Because always-on is present, arch_timer_of_init() in
drivers/clocksource/arm_arch_timer.c clears arch_timer_c3stop:

	arch_timer_c3stop = !of_property_read_bool(np, "always-on");

and __arch_timer_setup() omits CLOCK_EVT_FEAT_C3STOP from the per-CPU clock
event device.

When a CPU enters cpu_sleep, local-timer-stop sets CPUIDLE_FLAG_TIMER_STOP,
so cpuidle_enter_state() calls tick_broadcast_enter() ->
tick_broadcast_oneshot_control(TICK_BROADCAST_ENTER). However,
tick_broadcast_oneshot_control() in kernel/time/tick-common.c checks
CLOCK_EVT_FEAT_C3STOP first:

	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
		return 0;

	return __tick_broadcast_oneshot_control(state);

Because CLOCK_EVT_FEAT_C3STOP is not set, tick_broadcast_enter() returns 0
without arming a broadcast timer and without returning -EBUSY to fall back to
WFI in cpuidle_enter_state():

	if (broadcast && tick_broadcast_enter()) {
		index = find_deepest_state(drv, dev, target_state->exit_latency_ns,
					   CPUIDLE_FLAG_TIMER_STOP, false);

		target_state = &drv->states[index];
		broadcast = false;
	}

As a result, the CPU enters the PSCI cpu-sleep state where the local timer
stops, without a broadcast timer armed or a fallback to WFI when no broadcast
timer is available (stm32mp231.dtsi defines no lptimer broadcast timer nodes,
and bc_hrtimer cannot wake a single-core STM32MP231 or dual-core
STM32MP233/STM32MP235 when all CPUs enter idle).

Should the timer node in stm32mp231.dtsi replace always-on with
arm,no-tick-in-suspend, matching commit 9ec406ac4b7de3 ("arm64: dts: st:
Replace always-on by arm,no-tick-in-suspend for timer in stm32mp251.dtsi") on
stm32mp251.dtsi and stm32mp211.dtsi?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104133023.1.If2b6b99285c1e60ca11b6e7fccf7d9b130ca20dd@changeid?part=1