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