Re: [PATCH] cpuidle: riscv-sbi: Stop using non-retentive suspend

[Samuel Holland]

On 11/23/22 00:10, Anup Patel wrote:
> On Wed, Nov 23, 2022 at 11:08 AM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>>
>> Hi Anup,
>>
>> On 11/22/22 23:35, Anup Patel wrote:
>>> On Wed, Nov 23, 2022 at 10:41 AM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>>>> On 11/22/22 09:28, Palmer Dabbelt wrote:
>>>>> I also think we should stop entering non-retentive suspend until we can
>>>>> sort out how reliably wake up from it, as the SBI makes that a
>>>>> platform-specific detail.  If the answer there is "non-retentive suspend
>>>>> is fine on the D1 as long as we don't use the SBI timers" then that
>>>>> seems fine, we just need some way to describe that in Linux -- that
>>>>> doesn't fix other platforms and other interrupts, but at least it's a
>>>>> start.
>>>>
>>>> We need some way to describe the situation from the SBI implementation
>>>> to Linux.
>>>>
>>>> Non-retentive suspend is fine on the D1 as long as either one of these
>>>> conditions is met:
>>>>  1) we don't use the SBI timers, or
>>>>  2) the SBI timer implementation does not use the CLINT
>>>>
>>>> And it is up to the SBI implementation which timer hardware it uses, so
>>>> the SBI implementation needs to patch this information in to the DT at
>>>> runtime.
>>>
>>> Rather than SBI implementation patching information in DT, it is much
>>> simpler to add a quirk in RISC-V timer driver for D1 platform (i.e. based
>>> on D1 compatible string in root node).
>>
>> It would be simpler, but it would be wrong, as I just explained.
>>
>> Only the SBI implementation knows if the SBI timer extension can wake
>> any given CPU from any given non-retentive suspend state.
> 
> The SBI implementation would derive this information from platform
> compatible string which is already available to the Linux kernel so
> why does SBI implementation have to patch the DTB and put the
> same information in a different way ?

It is not the same information. The SBI implementation also chooses, at
runtime, which timer hardware (CLINT, platform-specific MMIO timer,
etc.) is used to implement the SBI timer extension. The value of the
sbi-timer-can-wake-cpu property depends on this choice.

Using D1 as an example, there are two MMIO timer peripherals ("sun4i"
TIMER and "sun5i" HSTIMER) where the sbi-timer-can-wake-cpu property
should be set. But the property should not be set if the CLINT is used
by SBI.

It would be perfectly reasonable for the SBI implementation to claim one
of the wakeup-capable MMIO timers for itself, mark it as "reserved" in
the DT passed to Linux, and thus force Linux to use the SBI timer or a
native CLINT driver (C906 CLINT has S-mode extensions). Then the SBI
timer _would_ be capable of waking the CPU from non-retentive suspend.

Regards,
Samuel