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

[Anup Patel]

On Wed, Nov 23, 2022 at 11:57 AM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>
> 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.

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".

Regards,
Anup