Re: [PATCH RFC 01/27] PM / Domains: core changes for multiple states

[Axel Haslam]

On Wed, Dec 9, 2015 at 2:58 PM, Ulf Hansson  wrote:
> On 17 November 2015 at 23:37, Lina Iyer  wrote:
>> From: Axel Haslam 
>>
>> From: Axel Haslam 
>>
>> Add the core changes to be able to declare multiple states.
>> When trying to set a power domain to off, genpd will be able to choose
>> from an array of states declared by the platform. The power on and off
>> latencies are now tied to a state.
>
> I would like to get an answer to *why* we need this.
>
> For example, we should mention that some HWs supports these multiple
> states - at least.
>
>>
>> States should be declared in ascending order from shallowest to deepest,
>
> I guess *should* isn't good enough. Perhaps *shall* is better?
>
>> deepest meaning the state which takes longer to enter and exit.
>>
>> the power_off and power_on function can use the 'state_idx' field of the
>> generic_pm_domain structure, to distinguish between the different states
>> and act accordingly.
>
> This needs some more explanation.
>
> First, please use the wording of "callbacks", like ->power_on() to
> better describe what function you are talking about.
> Second, what is "state_idx"? What's does it really tell the SOC PM
> domain driver here?
>
>>
>> Example:
>>
>> static int pd1_power_on(struct generic_pm_domain *domain)
>> {
>> /* domain->state_idx = state the domain is coming from */
>> }
>>
>> static int pd1_power_off(struct generic_pm_domain *domain)
>> {
>> /* domain->state_idx = desired powered off state */
>> }
>>
>> const struct genpd_power_state pd_states[] = {
>> {
>> .name = "RET",
>> .power_on_latency_ns = ON_LATENCY_FAST,
>> .power_off_latency_ns = OFF_LATENCY_FAST,
>> },
>> {
>> .name = "DEEP_RET",
>> .power_on_latency_ns = ON_LATENCY_MED,
>> .power_off_latency_ns = OFF_LATENCY_MED,
>> },
>> {
>> .name = "OFF",
>> .power_on_latency_ns = ON_LATENCY_SLOW,
>> .power_off_latency_ns = OFF_LATENCY_SLOW,
>> }
>> };
>>
>> struct generic_pm_domain pd1 = {
>> .name = "PD1",
>> .power_on = pd1_power_on,
>> .power_off = pd1_power_off,
>> [...]
>> };
>>
>> int xxx_init_pm_domain(){
>>
>> pd1->state = copy_of(pd_states);
>> pd1->state_count = ARRAY_SIZE(pd_states);
>> pm_genpd_init(pd1, true);
>>
>> }
>
> Well, even if the above describes this quite good, I think it better
> belongs in the Documentation rather than in the change log.
>
> Can we try to the improve the text in the change-log instead?
>
>>
>> Signed-off-by: Axel Haslam 
>> Signed-off-by: Lina Iyer 
>> [Lina: Modified genpd state initialization and remove use of
>> save_state_latency_ns in genpd timing data]
>>
>> Signed-off-by: Lina Iyer 
>> ---
>>  drivers/base/power/domain.c  | 136 
>> +--
>>  drivers/base/power/domain_governor.c |  13 +++-
>>  include/linux/pm_domain.h|  10 +++
>>  3 files changed, 151 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index e03b1ad..3242854 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -34,6 +34,12 @@
>> __ret;  \
>>  })
>>
>> +#define GENPD_MAX_NAME_SIZE 20
>> +
>> +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
>
> Please avoid forward declarations.
>
> Also, we have just got rid of all *name* based genpd API/functions. I
> don't want us to start adding another.
>
> Or perhaps it's just the name of the function that I don't like!? :-)
>
>> +  const struct genpd_power_state *st,
>> +  unsigned int st_count);
>> +
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>>
>> @@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct 
>> generic_pm_domain *genpd)
>>
>>  static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>  {
>> +   unsigned int state_idx = genpd->state_idx;
>> ktime_t time_start;
>> s64 elapsed_ns;
>> int ret;
>> @@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain 
>> *genpd, bool timed)
>> return ret;
>>
>> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
>> -   if (elapsed_ns <= genpd->power_on_latency_ns)
>> +   if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
>> return ret;
>>
>> -   genpd->power_on_latency_ns = elapsed_ns;
>> +   genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
>> genpd->max_off_time_changed = true;
>> 

Re: [PATCH RFC 01/27] PM / Domains: core changes for multiple states

[Ulf Hansson]

On 17 November 2015 at 23:37, Lina Iyer  wrote:
> From: Axel Haslam 
>
> From: Axel Haslam 
>
> Add the core changes to be able to declare multiple states.
> When trying to set a power domain to off, genpd will be able to choose
> from an array of states declared by the platform. The power on and off
> latencies are now tied to a state.

I would like to get an answer to *why* we need this.

For example, we should mention that some HWs supports these multiple
states - at least.

>
> States should be declared in ascending order from shallowest to deepest,

I guess *should* isn't good enough. Perhaps *shall* is better?

> deepest meaning the state which takes longer to enter and exit.
>
> the power_off and power_on function can use the 'state_idx' field of the
> generic_pm_domain structure, to distinguish between the different states
> and act accordingly.

This needs some more explanation.

First, please use the wording of "callbacks", like ->power_on() to
better describe what function you are talking about.
Second, what is "state_idx"? What's does it really tell the SOC PM
domain driver here?

>
> Example:
>
> static int pd1_power_on(struct generic_pm_domain *domain)
> {
> /* domain->state_idx = state the domain is coming from */
> }
>
> static int pd1_power_off(struct generic_pm_domain *domain)
> {
> /* domain->state_idx = desired powered off state */
> }
>
> const struct genpd_power_state pd_states[] = {
> {
> .name = "RET",
> .power_on_latency_ns = ON_LATENCY_FAST,
> .power_off_latency_ns = OFF_LATENCY_FAST,
> },
> {
> .name = "DEEP_RET",
> .power_on_latency_ns = ON_LATENCY_MED,
> .power_off_latency_ns = OFF_LATENCY_MED,
> },
> {
> .name = "OFF",
> .power_on_latency_ns = ON_LATENCY_SLOW,
> .power_off_latency_ns = OFF_LATENCY_SLOW,
> }
> };
>
> struct generic_pm_domain pd1 = {
> .name = "PD1",
> .power_on = pd1_power_on,
> .power_off = pd1_power_off,
> [...]
> };
>
> int xxx_init_pm_domain(){
>
> pd1->state = copy_of(pd_states);
> pd1->state_count = ARRAY_SIZE(pd_states);
> pm_genpd_init(pd1, true);
>
> }

Well, even if the above describes this quite good, I think it better
belongs in the Documentation rather than in the change log.

Can we try to the improve the text in the change-log instead?

>
> Signed-off-by: Axel Haslam 
> Signed-off-by: Lina Iyer 
> [Lina: Modified genpd state initialization and remove use of
> save_state_latency_ns in genpd timing data]
>
> Signed-off-by: Lina Iyer 
> ---
>  drivers/base/power/domain.c  | 136 
> +--
>  drivers/base/power/domain_governor.c |  13 +++-
>  include/linux/pm_domain.h|  10 +++
>  3 files changed, 151 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..3242854 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -34,6 +34,12 @@
> __ret;  \
>  })
>
> +#define GENPD_MAX_NAME_SIZE 20
> +
> +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,

Please avoid forward declarations.

Also, we have just got rid of all *name* based genpd API/functions. I
don't want us to start adding another.

Or perhaps it's just the name of the function that I don't like!? :-)

> +  const struct genpd_power_state *st,
> +  unsigned int st_count);
> +
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> @@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain 
> *genpd)
>
>  static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
> +   unsigned int state_idx = genpd->state_idx;
> ktime_t time_start;
> s64 elapsed_ns;
> int ret;
> @@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain 
> *genpd, bool timed)
> return ret;
>
> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> -   if (elapsed_ns <= genpd->power_on_latency_ns)
> +   if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> return ret;
>
> -   genpd->power_on_latency_ns = elapsed_ns;
> +   genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
> genpd->max_off_time_changed = true;
> pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>  genpd->name, "on", elapsed_ns);
> @@ -131,6 +138,7 @@ static int genpd_power_on(struct generic_pm_domain 
> *genpd, bool timed)
>
>  static int