Improve CLOCK_EVT_FEAT_C3STOP feature setting
Subsystems (3)
v2 cover Improve CLOCK_EVT_FEAT_C3STOP feature setting
This series improves the RISC-V timer driver to set CLOCK_EVT_FEAT_C3STOP feature based on RISC-V platform capabilities.
These patches can also be found in riscv_timer_dt_imp_v2 branch at:
https://github.com/avpatel/linux.git
Changes since v1:
- Rebased on Linux-5.19-rc8
- Renamed "riscv,always-on" DT property to "riscv,timer-can-wake-cpu"
Anup Patel (2):
dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
property
Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ drivers/clocksource/timer-riscv.c | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-)
Signature · 1 line
2.34.1
PATCH v2 1/2 dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
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.
--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/riscv/cpus.yaml +6 -0
@@ -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
Signature · 1 line
2.34.1
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 13 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 8 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0@@ -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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 18 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdc
Quoted 8 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 4 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 33 lines · Anup Patel
Quoted 18 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdcQuoted 8 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
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.
Quoted 2 lines
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...
Quoted 3 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 41 lines · Krzysztof Kozlowski
Quoted 33 lines · Anup Patel
Quoted 18 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdcQuoted 8 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
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
Quoted 7 lines
Timer is always present also on ARMv8 (and ARMv7) yet it has its node.
Quoted 2 lines
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.
Quoted 5 lines
Quoted 4 lines
Since, there is no dedicated timer node, we use CPU compatible string for probing the per-CPU timer.
Quoted 7 lines
Next time you add a properties:
riscv,saata-can-wake-cpu
riscv,usb-can-wake-cpu
riscv,interrupt-controller-can-wake-cpuand 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.
Quoted 5 lines
You need to properly reflect hardware in the DTS instead of such hacks.
Best regards,
Krzysztof
Regards,
Anup
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
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 :(
Quoted 17 lines · Anup Patel
Quoted 15 lines · Krzysztof Kozlowski
Quoted 7 lines · Anup Patel
Quoted 2 lines · Krzysztof Kozlowski
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
Quoted 6 lines
Quoted 2 lines
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:
Quoted 5 lines
Request the SBI implementation to put the calling hart in a platform specific suspend (or low power) state specified by the
suspend_typeparameter. 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.
Quoted 2 lines
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:
Quoted 2 lines
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.
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Hi Conor,
Quoted 7 lines · Conor Dooley
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 9 lines
I noted the above:
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Hey Samuel,
Quoted 17 lines · Samuel Holland
Quoted 7 lines · Conor Dooley
Quoted 2 lines
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.
Quoted 8 lines
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?
Quoted 7 lines
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?
Quoted 9 lines
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.
Quoted 9 lines
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.
Quoted 5 lines
I noted the above:
Quoted 2 lines
Since, there is no dedicated timer node, we use CPU compatible string for probing the per-CPU timer.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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, forcingcpuidle_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.
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
In reply to Conor DooleyHey Anup,
(keeping all the context since you didn't reply to this mail yet)
Quoted 106 lines · Conor Dooley
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 :(
Quoted 17 lines · Anup Patel
Quoted 15 lines · Krzysztof Kozlowski
Quoted 7 lines · Anup Patel
Quoted 2 lines · Krzysztof Kozlowski
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-cpuQuoted 6 lines
Quoted 2 lines
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:
Quoted 5 lines
Request the SBI implementation to put the calling hart in a platform specific suspend (or low power) state specified by the
suspend_typeparameter. 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.Quoted 2 lines
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:
Quoted 2 lines
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?
Quoted 1 line
Hopefully I've missed something blatant here Anup!
So what I missed, as Anup pointed out else where, is:
Quoted 10 lines
me:
Quoted 5 lines
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.
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 134 lines · Conor Dooley
Hey Anup,
(keeping all the context since you didn't reply to this mail yet)
Quoted 106 lines · Conor Dooley
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 :(
Quoted 17 lines · Anup Patel
Quoted 15 lines · Krzysztof Kozlowski
Quoted 7 lines · Anup Patel
Quoted 2 lines · Krzysztof Kozlowski
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-cpuQuoted 6 lines
Quoted 2 lines
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:
Quoted 5 lines
Request the SBI implementation to put the calling hart in a platform specific suspend (or low power) state specified by the
suspend_typeparameter. 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.Quoted 2 lines
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:
Quoted 2 lines
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?Quoted 1 line
Hopefully I've missed something blatant here Anup!
So what I missed, as Anup pointed out else where, is:
Quoted 10 lines
me:
Quoted 5 lines
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
Quoted 6 lines
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
Quoted 4 lines
Thanks,
Conor.
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 17 lines · Anup Patel
Quoted 15 lines · Conor Dooley
Quoted 3 lines
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 propertiesI 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?
Quoted 5 lines
Quoted 2 lines
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!
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
In reply to Krzysztof KozlowskiQuoted 17 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdc
Quoted 9 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 3 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 34 lines · Sudeep Holla
Quoted 17 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdcQuoted 9 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 7 lines
Quoted 3 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
In reply to Sudeep HollaHi Sudeep,
Quoted 29 lines · Sudeep Holla
Quoted 17 lines · Krzysztof Kozlowski
Quoted 16 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0@@ -78,6 +78,12 @@ properties: - rv64imac - rv64imafdcQuoted 9 lines
Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 5 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0+ 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.
Quoted 4 lines
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 ?
Quoted 5 lines
Quoted 4 lines
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)?
Quoted 7 lines
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 32 lines · Anup Patel
Hi Sudeep,
Quoted 29 lines · Sudeep Holla
Quoted 26 lines · Krzysztof Kozlowski
Quoted 13 lines · Anup Patel
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 <[email protected]>--- Documentation/devicetree/bindings/riscv/cpus.yaml | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/devicetree/bindings/riscv/cpus.yaml +0
-0Documentation/devicetree/bindings/riscv/cpus.yaml properties: · line 85
Quoted 8 lines
Documentation/devicetree/bindings/riscv/cpus.yaml +5
-0@@ -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.
Quoted 9 lines
Quoted 4 lines
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.
Signature · 2 lines
Regards, Sudeep
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
In reply to Anup PatelQuoted 3 lines · Anup Patel
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
Re: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
Quoted 9 lines · Sudeep Holla
Quoted 3 lines · Anup Patel
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.
Quoted 8 lines
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
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
PATCH v2 2/2 clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT property
In reply to Anup PatelWe should set CLOCK_EVT_FEAT_C3STOP for a clock_event_device only when riscv,timer-can-wake-cpu DT property is not present for the corresponding CPU.
This way CLOCK_EVT_FEAT_C3STOP feature is set for clock_event_device based on RISC-V platform capabilities rather than having it set for all RISC-V platforms.
--- drivers/clocksource/timer-riscv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/clocksource/timer-riscv.c +5 -1
@@ -34,7 +34,7 @@ static int riscv_clock_next_event(unsigned long delta,static unsigned int riscv_clock_event_irq; static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = { .name ="riscv_timer_clockevent" , - .features =CLOCK_EVT_FEAT_ONESHOT |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };
@@ -65,9 +65,13 @@ static struct clocksource riscv_clocksource = {static int riscv_timer_starting_cpu (unsigned int cpu) {struct clock_event_device *ce =per_cpu_ptr (&riscv_clock_event, cpu); +struct device_node *np =of_get_cpu_node (cpu,NULL ); ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (!of_property_read_bool (np,"riscv,timer-can-wake-cpu" )) + ce->features |=CLOCK_EVT_FEAT_C3STOP ; +of_node_put (np);clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,
Signature · 7 lines
2.34.1 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv