[PATCH v2 0/2] Improve CLOCK_EVT_FEAT_C3STOP feature setting

[PATCH v2 0/2] Improve CLOCK_EVT_FEAT_C3STOP feature setting

[Anup Patel]

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(-)

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu

[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 <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.
+
   # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
   timebase-frequency: false
 
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/2] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT property

[Anup Patel]

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/clocksource/timer-riscv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 593d5a957b69..92a0b9832d8c 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -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,
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Krzysztof Kozlowski]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Sudeep Holla]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Krzysztof Kozlowski]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Sudeep Holla]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[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 :(

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.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Samuel Holland]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Conor Dooley]

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.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Conor Dooley]

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.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
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

[Conor Dooley]

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!


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv