lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/2] dt-bindings: riscv: Add optional DT property riscv,timer-can-wake-cpu
    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.

    \
     
     \ /
      Last update: 2022-11-23 12:50    [W:3.858 / U:0.460 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site