Messages in this thread | | | From | Anup Patel <> | Date | Wed, 23 Nov 2022 12:43:23 +0530 | Subject | Re: [PATCH] cpuidle: riscv-sbi: Stop using non-retentive suspend |
| |
On Wed, Nov 23, 2022 at 12:20 PM Samuel Holland <samuel@sholland.org> wrote: > > On 11/23/22 00:41, Anup Patel wrote: > > On Wed, Nov 23, 2022 at 11:57 AM Samuel Holland <samuel@sholland.org> wrote: > >> > >> On 11/23/22 00:10, Anup Patel wrote: > >>> On Wed, Nov 23, 2022 at 11:08 AM Samuel Holland <samuel@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@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.
On a platform with Sstc, the SBI set_timer() call will internally update stimecmp CSR. In fact, this is what OpenSBI already does.
Here's the text from Sstc specification: "In systems in which a supervisor execution environment (SEE) provides timer facilities via an SBI function call, this SBI call will continue to support requests to schedule a timer interrupt. The SEE will simply make use of stimecmp, changing its value as appropriate. This ensures compatibility with existing S-mode software that uses this SEE facility, while new S-mode software takes advantage of stimecmp directly.)"
Based on the above, we don't need separate DT property for SBI timer and Sstc.
Regards, Anup
| |