* [RFC PATCH v1 0/2] allow idle state to be found as deepest state for s2idle only @ 2020-04-13 7:00 zhang.lyra 2020-04-13 7:00 ` [RFC PATCH v1 1/2] cpuidle: " zhang.lyra 2020-04-13 7:00 ` [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only zhang.lyra 0 siblings, 2 replies; 7+ messages in thread From: zhang.lyra @ 2020-04-13 7:00 UTC (permalink / raw) To: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, Mark Rutland Cc: linux-pm, devicetree, linux-kernel, Chunyan Zhang, Chunyan Zhang From: Chunyan Zhang <chunyan.zhang@unisoc.com> We can add a new c-state to let system suspend enter into the same deep state as s2ram when using s2idle, but we don't want other cases to use that c-state, such as play_idle() which would find the deepest state by invoking find_deepest_state() as well. This patch-set provides users an option which can restrict c-state to be found as deepest state for s2idle only. Chunyan Zhang (2): cpuidle: allow idle state to be found as deepest state for s2idle only dt-bindings: arm: Add description to the new property for-s2idle-only Documentation/devicetree/bindings/arm/idle-states.yaml | 10 +++++++++- drivers/cpuidle/cpuidle.c | 3 ++- drivers/cpuidle/dt_idle_states.c | 3 +++ include/linux/cpuidle.h | 1 + 4 files changed, 15 insertions(+), 2 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH v1 1/2] cpuidle: allow idle state to be found as deepest state for s2idle only 2020-04-13 7:00 [RFC PATCH v1 0/2] allow idle state to be found as deepest state for s2idle only zhang.lyra @ 2020-04-13 7:00 ` zhang.lyra 2020-04-20 9:36 ` Chunyan Zhang 2020-04-20 11:42 ` Mark Rutland 2020-04-13 7:00 ` [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only zhang.lyra 1 sibling, 2 replies; 7+ messages in thread From: zhang.lyra @ 2020-04-13 7:00 UTC (permalink / raw) To: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, Mark Rutland Cc: linux-pm, devicetree, linux-kernel, Chunyan Zhang, Chunyan Zhang 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v1 1/2] cpuidle: allow idle state to be found as deepest state for s2idle only 2020-04-13 7:00 ` [RFC PATCH v1 1/2] cpuidle: " zhang.lyra @ 2020-04-20 9:36 ` Chunyan Zhang 2020-04-20 11:42 ` Mark Rutland 1 sibling, 0 replies; 7+ messages in thread From: Chunyan Zhang @ 2020-04-20 9:36 UTC (permalink / raw) To: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, Mark Rutland Cc: linux-pm, devicetree, LKML, Chunyan Zhang 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v1 1/2] cpuidle: allow idle state to be found as deepest state for s2idle only 2020-04-13 7:00 ` [RFC PATCH v1 1/2] cpuidle: " zhang.lyra 2020-04-20 9:36 ` Chunyan Zhang @ 2020-04-20 11:42 ` Mark Rutland 2020-04-21 7:31 ` Chunyan Zhang 1 sibling, 1 reply; 7+ messages in thread From: Mark Rutland @ 2020-04-20 11:42 UTC (permalink / raw) To: zhang.lyra Cc: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, linux-pm, devicetree, linux-kernel, Chunyan Zhang 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v1 1/2] cpuidle: allow idle state to be found as deepest state for s2idle only 2020-04-20 11:42 ` Mark Rutland @ 2020-04-21 7:31 ` Chunyan Zhang 0 siblings, 0 replies; 7+ messages in thread From: Chunyan Zhang @ 2020-04-21 7:31 UTC (permalink / raw) To: Mark Rutland Cc: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, linux-pm, DTML, Linux Kernel Mailing List, Chunyan Zhang 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only 2020-04-13 7:00 [RFC PATCH v1 0/2] allow idle state to be found as deepest state for s2idle only zhang.lyra 2020-04-13 7:00 ` [RFC PATCH v1 1/2] cpuidle: " zhang.lyra @ 2020-04-13 7:00 ` zhang.lyra 2020-04-20 11:44 ` Mark Rutland 1 sibling, 1 reply; 7+ messages in thread From: zhang.lyra @ 2020-04-13 7:00 UTC (permalink / raw) To: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, Mark Rutland Cc: linux-pm, devicetree, linux-kernel, Chunyan Zhang, Chunyan Zhang From: Chunyan Zhang <chunyan.zhang@unisoc.com> Add a new property for-s2idle-only. The idle-state marked with this property will be set with CPUIDLE_FLAG_S2IDLE during initialization and it would be expected to be found as deepest state for s2idle rather than other cases like play_idle(). Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> --- Documentation/devicetree/bindings/arm/idle-states.yaml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/arm/idle-states.yaml b/Documentation/devicetree/bindings/arm/idle-states.yaml index ea805c1e6b20..cec47b3a447f 100644 --- a/Documentation/devicetree/bindings/arm/idle-states.yaml +++ b/Documentation/devicetree/bindings/arm/idle-states.yaml @@ -263,7 +263,6 @@ patternProperties: description: | Each state node represents an idle state description and must be defined as follows. - The idle state entered by executing the wfi instruction (idle_standby SBSA,[3][4]) is considered standard on all ARM platforms and therefore must not be listed. @@ -283,6 +282,15 @@ patternProperties: lost on state entry, otherwise it is retained. type: boolean + for-s2idle-only: + description: + This indicates that the state only can be found as deepest state + for s2idle rather than other cases like play_idle(). In general, + the state having this property should have longer min-residency + than the cpuidle target min-residency which CPU QoS constraints + defines, to avoid being used by runtime cpuidle. + type: boolean + entry-latency-us: description: Worst case latency in microseconds required to enter the idle state. -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only 2020-04-13 7:00 ` [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only zhang.lyra @ 2020-04-20 11:44 ` Mark Rutland 0 siblings, 0 replies; 7+ messages in thread From: Mark Rutland @ 2020-04-20 11:44 UTC (permalink / raw) To: zhang.lyra Cc: Rafael J . Wysocki, Daniel Lezcano, Rob Herring, linux-pm, devicetree, linux-kernel, Chunyan Zhang On Mon, Apr 13, 2020 at 03:00:14PM +0800, zhang.lyra@gmail.com wrote: > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > Add a new property for-s2idle-only. The idle-state marked with this > property will be set with CPUIDLE_FLAG_S2IDLE during initialization > and it would be expected to be found as deepest state for s2idle > rather than other cases like play_idle(). > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > --- > Documentation/devicetree/bindings/arm/idle-states.yaml | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/arm/idle-states.yaml b/Documentation/devicetree/bindings/arm/idle-states.yaml > index ea805c1e6b20..cec47b3a447f 100644 > --- a/Documentation/devicetree/bindings/arm/idle-states.yaml > +++ b/Documentation/devicetree/bindings/arm/idle-states.yaml > @@ -263,7 +263,6 @@ patternProperties: > description: | > Each state node represents an idle state description and must be defined > as follows. > - > The idle state entered by executing the wfi instruction (idle_standby > SBSA,[3][4]) is considered standard on all ARM platforms and therefore > must not be listed. > @@ -283,6 +282,15 @@ patternProperties: > lost on state entry, otherwise it is retained. > type: boolean > > + for-s2idle-only: > + description: > + This indicates that the state only can be found as deepest state > + for s2idle rather than other cases like play_idle(). In general, > + the state having this property should have longer min-residency > + than the cpuidle target min-residency which CPU QoS constraints > + defines, to avoid being used by runtime cpuidle. > + type: boolean This is very Linux-specific, and is encoding a policy ratehr than a property of the state. As on patch 1, can you please describe the expected properties of this idle state, with some rationale as to why it's not suited for idle in general? A real example in the commit message would be very helpful. Thanks, Mark. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-21 7:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-13 7:00 [RFC PATCH v1 0/2] allow idle state to be found as deepest state for s2idle only zhang.lyra 2020-04-13 7:00 ` [RFC PATCH v1 1/2] cpuidle: " zhang.lyra 2020-04-20 9:36 ` Chunyan Zhang 2020-04-20 11:42 ` Mark Rutland 2020-04-21 7:31 ` Chunyan Zhang 2020-04-13 7:00 ` [RFC PATCH v1 2/2] dt-bindings: arm: Add description to the new property for-s2idle-only zhang.lyra 2020-04-20 11:44 ` Mark Rutland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).