diff mbox series

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

Message ID 20200413070014.12960-2-zhang.lyra@gmail.com (mailing list archive)
State RFC, archived
Headers show
Series allow idle state to be found as deepest state for s2idle only | expand

Commit Message

Chunyan Zhang April 13, 2020, 7 a.m. UTC 
From: Chunyan Zhang <chunyan.zhang@unisoc.com>

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.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/cpuidle/cpuidle.c        | 3 ++-
 drivers/cpuidle/dt_idle_states.c | 3 +++
 include/linux/cpuidle.h          | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

Comments

Chunyan Zhang April 20, 2020, 9:36 a.m. UTC | #1 
Hello,

Any comments or suggests on this? That would be very appreciated.

Thanks,
Chunyan


On Mon, Apr 13, 2020 at 5:09 PM <zhang.lyra@gmail.com> wrote:
>
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
>
> 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.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>  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
>
Mark Rutland April 20, 2020, 11:42 a.m. UTC | #2 
On Mon, Apr 13, 2020 at 03:00:13PM +0800, zhang.lyra@gmail.com wrote:
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> 
> 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?

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@unisoc.com>
> ---
>  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
>
Chunyan Zhang April 21, 2020, 7:31 a.m. UTC | #3 
Hi Mark,

Many thanks for your comments.

On Mon, 20 Apr 2020 at 19:42, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, Apr 13, 2020 at 03:00:13PM +0800, zhang.lyra@gmail.com wrote:
> > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >
> > 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@unisoc.com>
> > ---
> >  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
> >
diff mbox series

Patch

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;