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

[Samuel Holland]

On 11/23/22 00:41, Anup Patel wrote:
> On Wed, Nov 23, 2022 at 11:57 AM Samuel Holland <samuel at sholland.org> wrote:
>>
>> On 11/23/22 00:10, Anup Patel wrote:
>>> On Wed, Nov 23, 2022 at 11:08 AM Samuel Holland <samuel at sholland.org> wrote:
>>>>
>>>> Hi Anup,
>>>>
>>>> On 11/22/22 23:35, Anup Patel wrote:
>>>>> On Wed, Nov 23, 2022 at 10:41 AM Samuel Holland <samuel at sholland.org> 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.
> 
> Fair enough but the DT property should not be SBI specific because same
> situation can happen with Sstc as well where a particular non-retentive state
> does not preserve the state of stimecmp CSRs in the HARTs.
> 
> Better to keep the DT property name as "riscv,timer-can-wake-cpu".

Consider a platform where the Sstc-based timer cannot wake the CPU, but
the SBI timer can, because it uses different timer hardware or a
different interrupt delivery method. It seems like we need two different
properties, one for Sstc and the other for the SBI timer. If both are
supported, firmware cannot know which one S-mode software will use.

Regards,
Samuel