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.
