Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu

[Samuel Holland <samuel@sholland.org>]

Hi Conor,

On 11/22/22 08:57, Conor Dooley wrote:
>> If we add a timer DT node now
>> then we have to deal with compatibility for existing platforms.
> 
> In terms of what to encode in a DT, and given the spec never says that
> the timer interrupt must arrive during suspend, we must assume, by
> default, that no timer events arrive during suspend.
> 
> We have a bunch of existing platforms that may (do?) get timer events
> during suspend, the opposite of the proposed default behaviour.
> 
> I'm trying to follow the line of reasoning but I fail to see how taking
> either the property or node approach allows us to maintain behaviour for
> exiting platforms that that do see timer events during suspend without
> adding *something* to the DT. No matter what we add, we've got some sort
> of backwards compatibility issue, right?

In the absence of bugs/limitations in Linux timer code (like the ones
you are seeing on PolarFire), the backwards compatibility issue with
setting C3STOP by default is that non-retentive idle states will be
ignored unless:
 1) the DT property is added (i.e. firmware upgrade), or
 2) some other timer driver is available.
No other behavior should be affected.

On the other hand, if C3STOP defaults to off, then the backwards
compatibility issue concerns platforms that can currently boot Linux,
but which cannot use cpuidle because they need the flag. If they were to
upgrade their firmware, and Linux is provided a DTB that includes both
idle states and the property, these platforms would unexpectedly fail to
boot. (They would enter an idle state and never wake up.)

Assuming no such platforms exist, then it would actually be better to
default C3STOP to off.

Now, this says nothing about how the property should be named -- we can
set C3STOP based on the absence of a property, just as easily as we can
clear C3STOP based on the presence of a property.

> I noted the above:
> 
>> Since, there is no dedicated timer node, we use CPU compatible string
>> for probing the per-CPU timer.
> 
> If we could rely on the cpu compatible why would we need to add a
> dt-property anyway? Forgive my naivety here, but is the timer event in
> suspend behaviour not a "core complex" level attribute rather than a
> something that can be consistently determined by the cpu compatible?

I do not support using either the CPU compatible (not specific enough)
or the board compatible (too many to list, but still not specific
enough). Consider that not all CPUs in a system may need this property.

> Either way, we need to figure out why enabling C3STOP is causing other
> timer issues even when we are not in some sort of sleep state & do
> something about that - or figure out some different way to communicate
> the behavioural differences.
> I would expect timers to continue working "normally" with the flag set,
> even if how they work is subtly different?

Definitely agree here. My intention was not to affect anything other
than cpuidle behavior.

> On a D1, with the C3STOP "feature" flag set, and it's custom timer
> implementation unused, how do timers behave?

D1 is uniprocessor, so I build with CONFIG_SMP=n. In this case,
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=n, and thus
__tick_broadcast_oneshot_control() returns -EBUSY, forcing
cpuidle_enter_state() to choose a retentive idle state.

Regards,
Samuel