| Message ID | 20220727114302.302201-2-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve CLOCK_EVT_FEAT_C3STOP feature setting | expand |
On 27/07/2022 13:43, Anup Patel wrote: > We add an optional DT property riscv,timer-can-wake-cpu which if present > in CPU DT node then CPU timer is always powered-on and never loses context. > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > index d632ac76532e..b60b64b4113a 100644 > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > @@ -78,6 +78,12 @@ properties: > - rv64imac > - rv64imafdc > > + riscv,timer-can-wake-cpu: > + type: boolean > + description: > + If present, the timer interrupt can wake up the CPU from > + suspend/idle state. Isn't this a property of a timer, not CPU? IOW, your timer node should have "wakeup-source" property. Now that's actual problem: why the RISC-V timer is bound to "riscv" compatible, not to dedicated timer node? How is it related to actual CPU (not SoC)? Best regards, Krzysztof
On Wed, Jul 27, 2022 at 05:13:01PM +0530, Anup Patel wrote: > We add an optional DT property riscv,timer-can-wake-cpu which if present > in CPU DT node then CPU timer is always powered-on and never loses context. > I don't have much idea on idle states on RISC-V but associating this property in just CPU node seems like not so good idea. This will be applicable for all CPU idle states which means you can't use this even if one of the deepest idle state switches off the timer. We have local-timer-stop in each idle states node. IIRC RISC-V uses the binding which is now not arm specific[0] and IIRC you moved the binding yourself. Any reason why not can't be used and any specific reason for needing this extra property. [0] Documentation/devicetree/bindings/cpu/idle-states.yaml -- Regards, Sudeep
On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 27/07/2022 13:43, Anup Patel wrote: > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > > index d632ac76532e..b60b64b4113a 100644 > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > > @@ -78,6 +78,12 @@ properties: > > - rv64imac > > - rv64imafdc > > > > + riscv,timer-can-wake-cpu: > > + type: boolean > > + description: > > + If present, the timer interrupt can wake up the CPU from > > + suspend/idle state. > > Isn't this a property of a timer, not CPU? IOW, your timer node should > have "wakeup-source" property. Historically (since the early days), we never had a timer node in the RISC-V world. > > Now that's actual problem: why the RISC-V timer is bound to "riscv" > compatible, not to dedicated timer node? How is it related to actual CPU > (not SoC)? The RISC-V timer is always present on all RISC-V platforms because the "time" CSR is defined by RISC-V privileged specification. The method to program per-CPU timer events in either using SBI call or Sstc CSRs. Since, there is no dedicated timer node, we use CPU compatible string for probing the per-CPU timer. Regards, Anup
On Wed, Jul 27, 2022 at 5:48 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 05:13:01PM +0530, Anup Patel wrote: > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > I don't have much idea on idle states on RISC-V but associating this > property in just CPU node seems like not so good idea. > > This will be applicable for all CPU idle states which means you > can't use this even if one of the deepest idle state switches off > the timer. > > We have local-timer-stop in each idle states node. IIRC RISC-V uses the > binding which is now not arm specific[0] and IIRC you moved the binding > yourself. Any reason why not can't be used and any specific reason for > needing this extra property. Indeed, the "local-timer-stop" property should be used. I guess, Allwinner D1 should use this property in idle state and we should not unconditionally set CLOCK_EVT_FEAT_C3STOP in the timer driver. @Samuel Holland Can you confirm that the "local-timer-stop" property works for Allwinner D1 ? If yes, then please send a patch to remove CLOCK_EVT_FEAT_C3STOP from timer driver. Regards, Anup
On 27/07/2022 14:21, Anup Patel wrote: > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 27/07/2022 13:43, Anup Patel wrote: >>> We add an optional DT property riscv,timer-can-wake-cpu which if present >>> in CPU DT node then CPU timer is always powered-on and never loses context. >>> >>> Signed-off-by: Anup Patel <apatel@ventanamicro.com> >>> --- >>> Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml >>> index d632ac76532e..b60b64b4113a 100644 >>> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml >>> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml >>> @@ -78,6 +78,12 @@ properties: >>> - rv64imac >>> - rv64imafdc >>> >>> + riscv,timer-can-wake-cpu: >>> + type: boolean >>> + description: >>> + If present, the timer interrupt can wake up the CPU from >>> + suspend/idle state. >> >> Isn't this a property of a timer, not CPU? IOW, your timer node should >> have "wakeup-source" property. > > Historically (since the early days), we never had a timer node in the > RISC-V world. > >> >> Now that's actual problem: why the RISC-V timer is bound to "riscv" >> compatible, not to dedicated timer node? How is it related to actual CPU >> (not SoC)? > > The RISC-V timer is always present on all RISC-V platforms because Timer is always present also on ARMv8 (and ARMv7) yet it has its node. > the "time" CSR is defined by RISC-V privileged specification. The method > to program per-CPU timer events in either using SBI call or Sstc CSRs. Timer is still not part of CPU. Otherwise you are claiming here that CPU can wakeup CPU... > > Since, there is no dedicated timer node, we use CPU compatible string > for probing the per-CPU timer. Next time you add a properties: riscv,saata-can-wake-cpu riscv,usb-can-wake-cpu riscv,interrupt-controller-can-wake-cpu and so on and keep explaining that "historically" you did not define separate nodes, so thus must be in CPU node. You need to properly reflect hardware in the DTS instead of such hacks. Best regards, Krzysztof
On Wed, Jul 27, 2022 at 02:07:50PM +0200, Krzysztof Kozlowski wrote: > On 27/07/2022 13:43, Anup Patel wrote: > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > > index d632ac76532e..b60b64b4113a 100644 > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > > @@ -78,6 +78,12 @@ properties: > > - rv64imac > > - rv64imafdc > > > > + riscv,timer-can-wake-cpu: > > + type: boolean > > + description: > > + If present, the timer interrupt can wake up the CPU from > > + suspend/idle state. > > Isn't this a property of a timer, not CPU? IOW, your timer node should > have "wakeup-source" property. > I agree on the concept that this is property of the timer and not CPU. However we generally don't need to use wakeup-source property for timer as we ideally use this for waking up from system sleep state and we don't want to be running timer when we enter the state. > Now that's actual problem: why the RISC-V timer is bound to "riscv" > compatible, not to dedicated timer node? How is it related to actual CPU > (not SoC)? We have "always-on" property for this on arm arch timer, and I also see "regulator-always-on" or something similar defined. So in absence of timer node probably "local-timer-always-on" make sense ? Thoughts ? -- Regards, Sudeep
On Wed, Jul 27, 2022 at 6:16 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 02:07:50PM +0200, Krzysztof Kozlowski wrote: > > On 27/07/2022 13:43, Anup Patel wrote: > > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > --- > > > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > index d632ac76532e..b60b64b4113a 100644 > > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > @@ -78,6 +78,12 @@ properties: > > > - rv64imac > > > - rv64imafdc > > > > > > + riscv,timer-can-wake-cpu: > > > + type: boolean > > > + description: > > > + If present, the timer interrupt can wake up the CPU from > > > + suspend/idle state. > > > > Isn't this a property of a timer, not CPU? IOW, your timer node should > > have "wakeup-source" property. > > > > I agree on the concept that this is property of the timer and not CPU. > However we generally don't need to use wakeup-source property for timer > as we ideally use this for waking up from system sleep state and we don't > want to be running timer when we enter the state. > > > Now that's actual problem: why the RISC-V timer is bound to "riscv" > > compatible, not to dedicated timer node? How is it related to actual CPU > > (not SoC)? > > We have "always-on" property for this on arm arch timer, and I also see > "regulator-always-on" or something similar defined. So in absence of timer > node probably "local-timer-always-on" make sense ? Thoughts ? I agree. In the v1 patch, I had named it "riscv,timer-always-on" but I chose a more specific name in v2 based on comments from Samuel. I think we should use more consistent naming between ARM and RISC-V so "riscv,timer-always-on" makes more sense to me. Regards, Anup
On Wed, Jul 27, 2022 at 6:05 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 27/07/2022 14:21, Anup Patel wrote: > > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 27/07/2022 13:43, Anup Patel wrote: > >>> We add an optional DT property riscv,timer-can-wake-cpu which if present > >>> in CPU DT node then CPU timer is always powered-on and never loses context. > >>> > >>> Signed-off-by: Anup Patel <apatel@ventanamicro.com> > >>> --- > >>> Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > >>> index d632ac76532e..b60b64b4113a 100644 > >>> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > >>> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > >>> @@ -78,6 +78,12 @@ properties: > >>> - rv64imac > >>> - rv64imafdc > >>> > >>> + riscv,timer-can-wake-cpu: > >>> + type: boolean > >>> + description: > >>> + If present, the timer interrupt can wake up the CPU from > >>> + suspend/idle state. > >> > >> Isn't this a property of a timer, not CPU? IOW, your timer node should > >> have "wakeup-source" property. > > > > Historically (since the early days), we never had a timer node in the > > RISC-V world. > > > >> > >> Now that's actual problem: why the RISC-V timer is bound to "riscv" > >> compatible, not to dedicated timer node? How is it related to actual CPU > >> (not SoC)? > > > > The RISC-V timer is always present on all RISC-V platforms because > > Timer is always present also on ARMv8 (and ARMv7) yet it has its node. > > > the "time" CSR is defined by RISC-V privileged specification. The method > > to program per-CPU timer events in either using SBI call or Sstc CSRs. > > Timer is still not part of CPU. Otherwise you are claiming here that CPU > can wakeup CPU... The clocksource (i.e. "time" register) is part of the CPU but it is an alias/copy of a free running system counter whereas clockevent devices (i.e. "mtimecmp" or "stimecmp" registers) are tightly coupled-with/part-of-the CPU. Some of the CPU suspend/idle states may or may not preserve state of timer registers so we might not get a timer interrupt to wake up the cpu from idle state. > > > > > Since, there is no dedicated timer node, we use CPU compatible string > > for probing the per-CPU timer. > > Next time you add a properties: > riscv,saata-can-wake-cpu > riscv,usb-can-wake-cpu > riscv,interrupt-controller-can-wake-cpu > > and so on and keep explaining that "historically" you did not define > separate nodes, so thus must be in CPU node. This is a one-of-case with RISC-V DeviceTree where we are living with the fact that there is no timer DT node. If we add a timer DT node now then we have to deal with compatibility for existing platforms. > > You need to properly reflect hardware in the DTS instead of such hacks. > > Best regards, > Krzysztof Regards, Anup
Hi Sudeep, On Wed, Jul 27, 2022 at 6:16 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jul 27, 2022 at 02:07:50PM +0200, Krzysztof Kozlowski wrote: > > On 27/07/2022 13:43, Anup Patel wrote: > > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > --- > > > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > index d632ac76532e..b60b64b4113a 100644 > > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > @@ -78,6 +78,12 @@ properties: > > > - rv64imac > > > - rv64imafdc > > > > > > + riscv,timer-can-wake-cpu: > > > + type: boolean > > > + description: > > > + If present, the timer interrupt can wake up the CPU from > > > + suspend/idle state. > > > > Isn't this a property of a timer, not CPU? IOW, your timer node should > > have "wakeup-source" property. > > > > I agree on the concept that this is property of the timer and not CPU. > However we generally don't need to use wakeup-source property for timer > as we ideally use this for waking up from system sleep state and we don't > want to be running timer when we enter the state. It seems ARM is using two separate timer DT properties: one for system suspend (i.e. arm,no-tick-in-suspend) and another for CPU system (i.e. always-on). Is this understanding correct ? > > > Now that's actual problem: why the RISC-V timer is bound to "riscv" > > compatible, not to dedicated timer node? How is it related to actual CPU > > (not SoC)? > > We have "always-on" property for this on arm arch timer, and I also see > "regulator-always-on" or something similar defined. So in absence of timer > node probably "local-timer-always-on" make sense ? Thoughts ? > > -- > Regards, > Sudeep Regards, Anup
On Wed, Jul 27, 2022 at 07:15:28PM +0530, Anup Patel wrote: > Hi Sudeep, > > On Wed, Jul 27, 2022 at 6:16 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Jul 27, 2022 at 02:07:50PM +0200, Krzysztof Kozlowski wrote: > > > On 27/07/2022 13:43, Anup Patel wrote: > > > > We add an optional DT property riscv,timer-can-wake-cpu which if present > > > > in CPU DT node then CPU timer is always powered-on and never loses context. > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > > --- > > > > Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > > index d632ac76532e..b60b64b4113a 100644 > > > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml > > > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml > > > > @@ -78,6 +78,12 @@ properties: > > > > - rv64imac > > > > - rv64imafdc > > > > > > > > + riscv,timer-can-wake-cpu: > > > > + type: boolean > > > > + description: > > > > + If present, the timer interrupt can wake up the CPU from > > > > + suspend/idle state. > > > > > > Isn't this a property of a timer, not CPU? IOW, your timer node should > > > have "wakeup-source" property. > > > > > > > I agree on the concept that this is property of the timer and not CPU. > > However we generally don't need to use wakeup-source property for timer > > as we ideally use this for waking up from system sleep state and we don't > > want to be running timer when we enter the state. > > It seems ARM is using two separate timer DT properties: one for > system suspend (i.e. arm,no-tick-in-suspend) and another for CPU > system (i.e. always-on). Is this understanding correct ? > TBH, I hadn't looked at "arm,no-tick-in-suspend" before though it seem to be there for few years now. Based on the log, I see this as quirk for some platform and not used on many platforms. Ideally to account how much time was spent in suspend, we could use the counter as it continues to tick during the suspend as well i.e. just the timers are not available if not always. This was added to handle systems that are not conformant to the expectation of always available counter.
Hey Anup, I've been meaning to get back to you on this stuff for quite a while, but unfortunately I've gotten distracted with other stuff every time I got close. Apologies for that :( On Wed, Jul 27, 2022 at 07:04:57PM +0530, Anup Patel wrote: > On Wed, Jul 27, 2022 at 6:05 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 27/07/2022 14:21, Anup Patel wrote: > > > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > >> > > >> On 27/07/2022 13:43, Anup Patel wrote: > > > > > > Since, there is no dedicated timer node, we use CPU compatible string > > > for probing the per-CPU timer. > > > > Next time you add a properties: > > riscv,saata-can-wake-cpu > > riscv,usb-can-wake-cpu > > riscv,interrupt-controller-can-wake-cpu > > > > and so on and keep explaining that "historically" you did not define > > separate nodes, so thus must be in CPU node. > > This is a one-of-case with RISC-V DeviceTree where we are living with > the fact that there is no timer DT node. If we add a timer DT node now > then we have to deal with compatibility for existing platforms. I don't really understand the argument here. Perhaps this made sense a few months ago, but it no longer does IMO. We have existing platforms that interpreted the SBI spec (or perhaps predated the SBI spec in the relevant form?) differently. I've pasted it several times now I feel but it's relevant so pasting it here again... On the subject of suspend, the RISC-V SBI spec states: > Request the SBI implementation to put the calling hart in a platform > specific suspend (or low power) state specified by the suspend_type > parameter. The hart will automatically come out of suspended state and > resume normal execution when it receives an interrupt or platform > specific hardware event. This does not cover whether a given event actually reaches the hart or not, just what the hart will do if it receives an event. For the implementation on the Allwinner D1, timer events are not received during suspend. Through-out the various bits of conversation so far, I have been operating on the assumption that on PolarFire SoC, and potentially other SiFive based implementations, events from the RISC-V timer do reach a hart during suspend. I realised while writing this response that I have never actually tested it - the C3STOP flag caused problems for me during regular operation & not while using some DT defined sleep states. I've been learning/piecing together the bits of what is happening here as time goes on, so I made an assumption that may or may not be correct, and I am still oh-so-far from an understanding. I just took it for granted that the existing driver worked correctly for "old" SiFive stuff which MPFS is based on & figured that with ~the same core complex as the fu540 that we'd behave similarly. Perhaps that was not a good idea & please let me know if I've been barking up the wrong tree. Do we know definitively what is/isn't the case for any of the existing platforms? I can test some stuff, but it'll take some time as it's a bad week in my neck of the woods. > 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? 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? 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? On a D1, with the C3STOP "feature" flag set, and it's custom timer implementation unused, how do timers behave? Hopefully I've missed something blatant here Anup! Thanks, Conor.
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
Hey Samuel, On Tue, Nov 22, 2022 at 11:43:04PM -0600, Samuel Holland wrote: > 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. Aye, which I think is fine, in the context of platforms supported by upstream Linux. Right now, nothing in-tree seems to use idle states: - the SiFive stuff is more demo than anything - we've not really got to that point with our reference PolarFire stuff (although I can't speak for any customers) - the K210 is a toy (sorry Damien!) - the StarFive lads have moved on to the jh7110 - the D1 (although it's not an in-tree config) needs C3STOP by default, so its behaviour is positively affected. If there's someone with an out-of-tree idle config, there's not really much that we can do about it? > 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. Yeah, *assuming* no such platforms exist I agree - but the D1 is one of such platforms (albeit in a specific configuration) so I think we have to default C3STOP to on. > 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. Yeah, I was just trying to understand where Anup was coming from and teasing out the different bits of logic. I do not think that using the CPU compatible is a good idea - my understanding was that how a CPU with a given compatible is integrated into a core complex determines which timer (or interrupt etc) is capable of what. > > 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. Right & that makes sense for someone building a D1 focused kernel (and is what I do for my Nezha IIRC) but if someone builds a multiplatform kernel you're going to end up with CONFIG_SMP=y (but I'd imagine that in that scenario they'll have the sunxi,foo-timer's driver enabled). At this point, it's something I should go and dig out my board for though.. I was mainly just curious if the D1 also exhibits the borked timer behaviour that I see. Thanks again Samuel, Conor.
Hey Anup, (keeping all the context since you didn't reply to this mail yet) On Tue, Nov 22, 2022 at 02:57:05PM +0000, Conor Dooley wrote: > Hey Anup, > > I've been meaning to get back to you on this stuff for quite a while, > but unfortunately I've gotten distracted with other stuff every time I > got close. Apologies for that :( > > On Wed, Jul 27, 2022 at 07:04:57PM +0530, Anup Patel wrote: > > On Wed, Jul 27, 2022 at 6:05 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 27/07/2022 14:21, Anup Patel wrote: > > > > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > >> > > > >> On 27/07/2022 13:43, Anup Patel wrote: > > > > > > > > Since, there is no dedicated timer node, we use CPU compatible string > > > > for probing the per-CPU timer. > > > > > > Next time you add a properties: > > > riscv,saata-can-wake-cpu > > > riscv,usb-can-wake-cpu > > > riscv,interrupt-controller-can-wake-cpu > > > > > > and so on and keep explaining that "historically" you did not define > > > separate nodes, so thus must be in CPU node. > > > > This is a one-of-case with RISC-V DeviceTree where we are living with > > the fact that there is no timer DT node. If we add a timer DT node now > > then we have to deal with compatibility for existing platforms. > > I don't really understand the argument here. Perhaps this made sense a > few months ago, but it no longer does IMO. > > We have existing platforms that interpreted the SBI spec (or perhaps > predated the SBI spec in the relevant form?) differently. I've pasted it > several times now I feel but it's relevant so pasting it here again... > > On the subject of suspend, the RISC-V SBI spec states: > > Request the SBI implementation to put the calling hart in a platform > > specific suspend (or low power) state specified by the suspend_type > > parameter. The hart will automatically come out of suspended state and > > resume normal execution when it receives an interrupt or platform > > specific hardware event. > > This does not cover whether a given event actually reaches the hart or > not, just what the hart will do if it receives an event. For the > implementation on the Allwinner D1, timer events are not received during > suspend. > > Through-out the various bits of conversation so far, I have been > operating on the assumption that on PolarFire SoC, and potentially other > SiFive based implementations, events from the RISC-V timer do reach a > hart during suspend. > I realised while writing this response that I have never actually tested > it - the C3STOP flag caused problems for me during regular operation & > not while using some DT defined sleep states. > I've been learning/piecing together the bits of what is happening here as > time goes on, so I made an assumption that may or may not be correct, and > I am still oh-so-far from an understanding. > I just took it for granted that the existing driver worked correctly for > "old" SiFive stuff which MPFS is based on & figured that with ~the same > core complex as the fu540 that we'd behave similarly. > Perhaps that was not a good idea & please let me know if I've been > barking up the wrong tree. > > Do we know definitively what is/isn't the case for any of the existing > platforms? > I can test some stuff, but it'll take some time as it's a bad week in > my neck of the woods. > > > 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? > > 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? > > 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? > On a D1, with the C3STOP "feature" flag set, and it's custom timer > implementation unused, how do timers behave? > > Hopefully I've missed something blatant here Anup! So what I missed, as Anup pointed out else where, is: > me: > > I don't really follow. How is there a compatibility issue created by > > adding a new node that is not added for a new property? Both will > > require changes to the device tree. (You need not reply here, I am going > > to review the other thread, it's been on my todo list for too long. Been > > caught up with non-coherent stuff & our sw release cycle..) > > Adding a new timer DT node would mean, the RISC-V timer driver > will now be probed using the compatible to the new DT node whereas > the RISC-V timer driver is currently probed using CPU DT nodes. In that case, we would have to retain the ability to match against the "riscv". Spitballing: - add a new timer node - keep matching against "riscv" - look up a timer node during init w/ of_find_matching_node() that contains any new timer properties I think it's unlikely that this will be the last time we have to add some timer properties & we should avoid doing odd things in a DT to suit an operating system? Would something along those lines work Anup, or am I, yet again, missing something? Thanks, Conor.
On Wed, Nov 23, 2022 at 7:17 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > Hey Anup, > > (keeping all the context since you didn't reply to this mail yet) > > On Tue, Nov 22, 2022 at 02:57:05PM +0000, Conor Dooley wrote: > > Hey Anup, > > > > I've been meaning to get back to you on this stuff for quite a while, > > but unfortunately I've gotten distracted with other stuff every time I > > got close. Apologies for that :( > > > > On Wed, Jul 27, 2022 at 07:04:57PM +0530, Anup Patel wrote: > > > On Wed, Jul 27, 2022 at 6:05 PM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > > > On 27/07/2022 14:21, Anup Patel wrote: > > > > > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski > > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > >> > > > > >> On 27/07/2022 13:43, Anup Patel wrote: > > > > > > > > > > Since, there is no dedicated timer node, we use CPU compatible string > > > > > for probing the per-CPU timer. > > > > > > > > Next time you add a properties: > > > > riscv,saata-can-wake-cpu > > > > riscv,usb-can-wake-cpu > > > > riscv,interrupt-controller-can-wake-cpu > > > > > > > > and so on and keep explaining that "historically" you did not define > > > > separate nodes, so thus must be in CPU node. > > > > > > This is a one-of-case with RISC-V DeviceTree where we are living with > > > the fact that there is no timer DT node. If we add a timer DT node now > > > then we have to deal with compatibility for existing platforms. > > > > I don't really understand the argument here. Perhaps this made sense a > > few months ago, but it no longer does IMO. > > > > We have existing platforms that interpreted the SBI spec (or perhaps > > predated the SBI spec in the relevant form?) differently. I've pasted it > > several times now I feel but it's relevant so pasting it here again... > > > > On the subject of suspend, the RISC-V SBI spec states: > > > Request the SBI implementation to put the calling hart in a platform > > > specific suspend (or low power) state specified by the suspend_type > > > parameter. The hart will automatically come out of suspended state and > > > resume normal execution when it receives an interrupt or platform > > > specific hardware event. > > > > This does not cover whether a given event actually reaches the hart or > > not, just what the hart will do if it receives an event. For the > > implementation on the Allwinner D1, timer events are not received during > > suspend. > > > > Through-out the various bits of conversation so far, I have been > > operating on the assumption that on PolarFire SoC, and potentially other > > SiFive based implementations, events from the RISC-V timer do reach a > > hart during suspend. > > I realised while writing this response that I have never actually tested > > it - the C3STOP flag caused problems for me during regular operation & > > not while using some DT defined sleep states. > > I've been learning/piecing together the bits of what is happening here as > > time goes on, so I made an assumption that may or may not be correct, and > > I am still oh-so-far from an understanding. > > I just took it for granted that the existing driver worked correctly for > > "old" SiFive stuff which MPFS is based on & figured that with ~the same > > core complex as the fu540 that we'd behave similarly. > > Perhaps that was not a good idea & please let me know if I've been > > barking up the wrong tree. > > > > Do we know definitively what is/isn't the case for any of the existing > > platforms? > > I can test some stuff, but it'll take some time as it's a bad week in > > my neck of the woods. > > > > > 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? > > > > 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? > > > > 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? > > On a D1, with the C3STOP "feature" flag set, and it's custom timer > > implementation unused, how do timers behave? > > > > Hopefully I've missed something blatant here Anup! > > So what I missed, as Anup pointed out else where, is: > > > me: > > > I don't really follow. How is there a compatibility issue created by > > > adding a new node that is not added for a new property? Both will > > > require changes to the device tree. (You need not reply here, I am going > > > to review the other thread, it's been on my todo list for too long. Been > > > caught up with non-coherent stuff & our sw release cycle..) > > > > Adding a new timer DT node would mean, the RISC-V timer driver > > will now be probed using the compatible to the new DT node whereas > > the RISC-V timer driver is currently probed using CPU DT nodes. > > In that case, we would have to retain the ability to match against the > "riscv". Spitballing: > - add a new timer node > - keep matching against "riscv" > - look up a timer node during init w/ of_find_matching_node() that > contains any new timer properties > > I think it's unlikely that this will be the last time we have to add > some timer properties & we should avoid doing odd things in a DT to suit > an operating system? > > Would something along those lines work Anup, or am I, yet again, missing > something? I was already working on v3 along these lines. I will try to post a v3 this week itself. Regards, Anup > > Thanks, > Conor. >
On Wed, Nov 23, 2022 at 09:16:55PM +0530, Anup Patel wrote: > On Wed, Nov 23, 2022 at 7:17 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > Adding a new timer DT node would mean, the RISC-V timer driver > > > will now be probed using the compatible to the new DT node whereas > > > the RISC-V timer driver is currently probed using CPU DT nodes. > > > > In that case, we would have to retain the ability to match against the > > "riscv". Spitballing: > > - add a new timer node > > - keep matching against "riscv" > > - look up a timer node during init w/ of_find_matching_node() that > > contains any new timer properties > > > > I think it's unlikely that this will be the last time we have to add > > some timer properties & we should avoid doing odd things in a DT to suit > > an operating system? > > > > Would something along those lines work Anup, or am I, yet again, missing > > something? > > I was already working on v3 along these lines. I will try to post a v3 this > week itself. Cool, I'll keep my eyes peeled :) Thanks Anup!
diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml index d632ac76532e..b60b64b4113a 100644 --- a/Documentation/devicetree/bindings/riscv/cpus.yaml +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml @@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdc + riscv,timer-can-wake-cpu: + type: boolean + description: + If present, the timer interrupt can wake up the CPU from + suspend/idle state. + # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here timebase-frequency: false
We add an optional DT property riscv,timer-can-wake-cpu which if present in CPU DT node then CPU timer is always powered-on and never loses context. Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)