Re: [RFC PATCH v1 1/2] cpuidle: allow idle state to be found as deepest state for s2idle only

[Chunyan Zhang]

Hi Mark,

Many thanks for your comments.

On Mon, 20 Apr 2020 at 19:42, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Mon, Apr 13, 2020 at 03:00:13PM +0800, zhang.lyra@xxxxxxxxx wrote:
> > From: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> >
> > Add a new flag CPUIDLE_FLAG_S2IDLE to allow c-state to be found as
> > deepest state for s2idle only, so that users can add a new c-state
> > for using s2idle and don't worry disturbing other use cases such as
> > play_idle() which probably don't want to enter into so much deep
> > idle state since devices are not suspended for that kind of cases.
>
> Can you please elaborate on this?

Ok.

The thing was, I added a new c-state (named DOMAIN_PD for example) in
DT for using s2idle, and the target power level indicated in DOMAIN_PD
would be deeper than the level for regular cpuidle (for example
level-0 is for regular cpuidle; level-1 is for system suspend and
power domain would be shutdown as well as all cores , DOMAIN_PD uses
level-1). I worried that would cause the deeper power domain to be
shutdown if DOMAIN_PD was selected by play_idle().

But after have another look at PSCI in ATF, I consider that it
probably is not a problem which would really happen. Since play_idle()
wouldn't occur on all cpus at the same time, although play_idle()
could use DOMAIN_PD, the system wouldn't enter into that so deep power
level.

Hope I've explained the things clearly :)

In a word, this patch seems not needed for now.


Thanks again,
Chunyan


>
> Why exactly are these states not suited for regular cpu idle? What
> problems do they cause? e.g. long wakeup latency?
>
> The flag and the for-s2-idle-only DT property are encoding a policy
> rarher than a property, and as such I don't think this is the right way
> to describe this in the DT. However, if there might be porperties of the
> idle state that we could describe so that the OS can come to the same
> conclusion.
>
> Thanks,
> Mark.
>
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > ---
> >  drivers/cpuidle/cpuidle.c        | 3 ++-
> >  drivers/cpuidle/dt_idle_states.c | 3 +++
> >  include/linux/cpuidle.h          | 1 +
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index de81298051b3..bb61f0c271d2 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -89,7 +89,8 @@ static int find_deepest_state(struct cpuidle_driver *drv,
> >                   s->exit_latency_ns <= latency_req ||
> >                   s->exit_latency_ns > max_latency_ns ||
> >                   (s->flags & forbidden_flags) ||
> > -                 (s2idle && !s->enter_s2idle))
> > +                 (s2idle && !s->enter_s2idle) ||
> > +                 (!s2idle && (s->flags & CPUIDLE_FLAG_S2ILDE)))
> >                       continue;
> >
> >               latency_req = s->exit_latency_ns;
> > diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> > index 252f2a9686a6..530db2726c05 100644
> > --- a/drivers/cpuidle/dt_idle_states.c
> > +++ b/drivers/cpuidle/dt_idle_states.c
> > @@ -80,6 +80,9 @@ static int init_state_node(struct cpuidle_state *idle_state,
> >       idle_state->flags = 0;
> >       if (of_property_read_bool(state_node, "local-timer-stop"))
> >               idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> > +
> > +     if (of_property_read_bool(state_node, "for-s2idle-only"))
> > +             idle_state->flags |= CPUIDLE_FLAG_S2ILDE;
> >       /*
> >        * TODO:
> >        *      replace with kstrdup and pointer assignment when name
> > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> > index ec2ef63771f0..08da701f74cd 100644
> > --- a/include/linux/cpuidle.h
> > +++ b/include/linux/cpuidle.h
> > @@ -78,6 +78,7 @@ struct cpuidle_state {
> >  #define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
> >  #define CPUIDLE_FLAG_UNUSABLE        BIT(3) /* avoid using this state */
> >  #define CPUIDLE_FLAG_OFF     BIT(4) /* disable this state by default */
> > +#define CPUIDLE_FLAG_S2ILDE  BIT(5) /* state is used for s2idle only */
> >
> >  struct cpuidle_device_kobj;
> >  struct cpuidle_state_kobj;
> > --
> > 2.20.1
> >