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! Thanks, Conor.
