Message-ID: <Y3zjQXqEHsaoVVvf@wendy>
Date:   Tue, 22 Nov 2022 14:57:05 +0000
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Anup Patel <anup@...infault.org>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Anup Patel <apatel@...tanamicro.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Jones <ajones@...tanamicro.com>,
        Atish Patra <atishp@...shpatra.org>,
        Samuel Holland <samuel@...lland.org>,
        <devicetree@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: 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 :(

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@...aro.org> wrote:
> >
> > On 27/07/2022 14:21, Anup Patel wrote:
> > > On Wed, Jul 27, 2022 at 5:37 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@...aro.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.

Powered by blists - more mailing lists