[PATCH 0/2] PM / Domains: Expand generic power domain debugfs.

[PATCH 0/2] PM / Domains: Expand generic power domain debugfs.

[Thara Gopinath]

This patch set attempts to improve the existing generic power domain
debugfs capabilities. The first  patch adds various accounting and
other bits needed to expose out the generic power domain statistics.
The second patch removes the old debugfs entry and introduces new
entries and attributes.

Thara Gopinath (2):
  PM / Domains: Add time accounting to various genpd states.
  PM / Domains: Extend generic power domain debugfs.

 drivers/base/power/domain.c | 257 ++++++++++++++++++++++++++++++--------------
 include/linux/pm_domain.h   |   4 +
 2 files changed, 178 insertions(+), 83 deletions(-)

-- 
2.1.4

[PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Thara Gopinath]

This patch adds support to calculate the time spent by the generic
power domains in on and various idle states.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
 include/linux/pm_domain.h   |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index da49a83..e733796 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
 	smp_mb__after_atomic();
 }
 
+static void genpd_update_accounting(struct generic_pm_domain *genpd)
+{
+	ktime_t delta, now;
+
+	now = ktime_get();
+	delta = ktime_sub(now, genpd->accounting_time);
+
+	if (genpd->status == GPD_STATE_ACTIVE) {
+		genpd->on_time = ktime_add(genpd->on_time, delta);
+	} else {
+		int state_idx = genpd->state_idx;
+
+		genpd->states[state_idx].active_time =
+			ktime_add(genpd->states[state_idx].active_time, delta);
+		genpd->off_time = ktime_add(genpd->off_time, delta);
+	}
+	genpd->accounting_time = now;
+}
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
 	unsigned int state_idx = genpd->state_idx;
@@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 			return ret;
 	}
 
+	genpd_update_accounting(genpd);
 	genpd->status = GPD_STATE_POWER_OFF;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
@@ -410,6 +430,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
 	if (ret)
 		goto err;
 
+	genpd_update_accounting(genpd);
 	genpd->status = GPD_STATE_ACTIVE;
 	return 0;
 
@@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 	if (_genpd_power_off(genpd, false))
 		return;
 
+	genpd_update_accounting(genpd);
 	genpd->status = GPD_STATE_POWER_OFF;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
@@ -821,6 +843,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 
 	_genpd_power_on(genpd, false);
 
+	genpd_update_accounting(genpd);
 	genpd->status = GPD_STATE_ACTIVE;
 }
 
@@ -1486,6 +1509,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->max_off_time_changed = true;
 	genpd->provider = NULL;
 	genpd->has_provider = false;
+	genpd->accounting_time = ktime_get();
 	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = genpd_runtime_resume;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b7803a2..8bee0e3 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -43,6 +43,7 @@ struct genpd_power_state {
 	s64 power_on_latency_ns;
 	s64 residency_ns;
 	struct fwnode_handle *fwnode;
+	ktime_t active_time;
 };
 
 struct genpd_lock_ops;
@@ -78,6 +79,9 @@ struct generic_pm_domain {
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
 	void *free; /* Free the state that was allocated for default */
+	ktime_t on_time;
+	ktime_t off_time;
+	ktime_t accounting_time;
 	const struct genpd_lock_ops *lock_ops;
 	union {
 		struct mutex mlock;
-- 
2.1.4

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Ulf Hansson]

On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> This patch adds support to calculate the time spent by the generic
> power domains in on and various idle states.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  4 ++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index da49a83..e733796 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>         smp_mb__after_atomic();
>  }
>
> +static void genpd_update_accounting(struct generic_pm_domain *genpd)

I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
stub for !CONFIG_DEBUG_FS.

> +{
> +       ktime_t delta, now;
> +
> +       now = ktime_get();
> +       delta = ktime_sub(now, genpd->accounting_time);
> +
> +       if (genpd->status == GPD_STATE_ACTIVE) {
> +               genpd->on_time = ktime_add(genpd->on_time, delta);
> +       } else {
> +               int state_idx = genpd->state_idx;
> +
> +               genpd->states[state_idx].active_time =

"active_time" seems to be a misleading, can we rename it to "idle_time" instead?

> +                       ktime_add(genpd->states[state_idx].active_time, delta);
> +               genpd->off_time = ktime_add(genpd->off_time, delta);

Isn't the off_time the summary of the states' genpd->states[i].active_time?

Then we could do that computation quite easily at the point when
producing the debugfs output instead.

> +       }
> +       genpd->accounting_time = now;
> +}
> +
>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>         unsigned int state_idx = genpd->state_idx;
> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>                         return ret;
>         }
>
> +       genpd_update_accounting(genpd);

It find it a bit odd to update accounting just before changing the
status. Can we re-work this and update accounting right afterwards
instead? At least to me, that makes this easier to understand.

>         genpd->status = GPD_STATE_POWER_OFF;
>
>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
> @@ -410,6 +430,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
>         if (ret)
>                 goto err;
>
> +       genpd_update_accounting(genpd);

Ditto.

>         genpd->status = GPD_STATE_ACTIVE;
>         return 0;
>
> @@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>         if (_genpd_power_off(genpd, false))
>                 return;
>
> +       genpd_update_accounting(genpd);

Timekeeping gets disabled at a certain point during suspend. Therefore
you can't update accounting here, as this function gets called in this
stages.

>         genpd->status = GPD_STATE_POWER_OFF;
>
>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
> @@ -821,6 +843,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
>
>         _genpd_power_on(genpd, false);
>
> +       genpd_update_accounting(genpd);

Ditto.

>         genpd->status = GPD_STATE_ACTIVE;
>  }
>
> @@ -1486,6 +1509,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->max_off_time_changed = true;
>         genpd->provider = NULL;
>         genpd->has_provider = false;
> +       genpd->accounting_time = ktime_get();
>         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
>         genpd->domain.ops.prepare = pm_genpd_prepare;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b7803a2..8bee0e3 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -43,6 +43,7 @@ struct genpd_power_state {
>         s64 power_on_latency_ns;
>         s64 residency_ns;
>         struct fwnode_handle *fwnode;
> +       ktime_t active_time;
>  };
>
>  struct genpd_lock_ops;
> @@ -78,6 +79,9 @@ struct generic_pm_domain {
>         unsigned int state_count; /* number of states */
>         unsigned int state_idx; /* state that genpd will go to when off */
>         void *free; /* Free the state that was allocated for default */
> +       ktime_t on_time;
> +       ktime_t off_time;
> +       ktime_t accounting_time;
>         const struct genpd_lock_ops *lock_ops;
>         union {
>                 struct mutex mlock;
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Thara Gopinath]

On 06/07/2017 09:28 AM, Ulf Hansson wrote:
> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>> This patch adds support to calculate the time spent by the generic
>> power domains in on and various idle states.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  4 ++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index da49a83..e733796 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>         smp_mb__after_atomic();
>>  }
>>
>> +static void genpd_update_accounting(struct generic_pm_domain *genpd)
> 
> I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
> stub for !CONFIG_DEBUG_FS.

Ok.
> 
>> +{
>> +       ktime_t delta, now;
>> +
>> +       now = ktime_get();
>> +       delta = ktime_sub(now, genpd->accounting_time);
>> +
>> +       if (genpd->status == GPD_STATE_ACTIVE) {
>> +               genpd->on_time = ktime_add(genpd->on_time, delta);
>> +       } else {
>> +               int state_idx = genpd->state_idx;
>> +
>> +               genpd->states[state_idx].active_time =
> 
> "active_time" seems to be a misleading, can we rename it to "idle_time" instead?

I meant it as the the active time in each state. But yes it is
confusing. Ideally states should be renamed to idle_states. But yes I
can change active_time to idle_time.
> 
>> +                       ktime_add(genpd->states[state_idx].active_time, delta);
>> +               genpd->off_time = ktime_add(genpd->off_time, delta);
> 
> Isn't the off_time the summary of the states' genpd->states[i].active_time?

Yes. it is.
> 
> Then we could do that computation quite easily at the point when
> producing the debugfs output instead.

Are yu suggesting doing away with off_time parameter all together. It is
possible.
> 
>> +       }
>> +       genpd->accounting_time = now;
>> +}
>> +
>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>  {
>>         unsigned int state_idx = genpd->state_idx;
>> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>>                         return ret;
>>         }
>>
>> +       genpd_update_accounting(genpd);
> 
> It find it a bit odd to update accounting just before changing the
> status. Can we re-work this and update accounting right afterwards
> instead? At least to me, that makes this easier to understand.

Agreed.
> 
>>         genpd->status = GPD_STATE_POWER_OFF;
>>
>>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> @@ -410,6 +430,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
>>         if (ret)
>>                 goto err;
>>
>> +       genpd_update_accounting(genpd);
> 
> Ditto.
> 
>>         genpd->status = GPD_STATE_ACTIVE;
>>         return 0;
>>
>> @@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>>         if (_genpd_power_off(genpd, false))
>>                 return;
>>
>> +       genpd_update_accounting(genpd);
> 
> Timekeeping gets disabled at a certain point during suspend. Therefore
> you can't update accounting here, as this function gets called in this
> stages.

I will remove the update of accounting at this point. But then the
statistics  can be skewed for some power domains atleast. Other option
is to restart the accounting every time after device suspend and resume.

Regards
Thara

> 
>>         genpd->status = GPD_STATE_POWER_OFF;
>>
>>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> @@ -821,6 +843,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
>>
>>         _genpd_power_on(genpd, false);
>>
>> +       genpd_update_accounting(genpd);
> 
> Ditto.
> 
>>         genpd->status = GPD_STATE_ACTIVE;
>>  }
>>
>> @@ -1486,6 +1509,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>         genpd->max_off_time_changed = true;
>>         genpd->provider = NULL;
>>         genpd->has_provider = false;
>> +       genpd->accounting_time = ktime_get();
>>         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>>         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
>>         genpd->domain.ops.prepare = pm_genpd_prepare;
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index b7803a2..8bee0e3 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -43,6 +43,7 @@ struct genpd_power_state {
>>         s64 power_on_latency_ns;
>>         s64 residency_ns;
>>         struct fwnode_handle *fwnode;
>> +       ktime_t active_time;
>>  };
>>
>>  struct genpd_lock_ops;
>> @@ -78,6 +79,9 @@ struct generic_pm_domain {
>>         unsigned int state_count; /* number of states */
>>         unsigned int state_idx; /* state that genpd will go to when off */
>>         void *free; /* Free the state that was allocated for default */
>> +       ktime_t on_time;
>> +       ktime_t off_time;
>> +       ktime_t accounting_time;
>>         const struct genpd_lock_ops *lock_ops;
>>         union {
>>                 struct mutex mlock;
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Ulf Hansson]

[...]

>> Isn't the off_time the summary of the states' genpd->states[i].active_time?
>
> Yes. it is.
>>
>> Then we could do that computation quite easily at the point when
>> producing the debugfs output instead.
>
> Are yu suggesting doing away with off_time parameter all together. It is
> possible.

Yes, I think that makes sense.

>>
>>> +       }
>>> +       genpd->accounting_time = now;
>>> +}
>>> +

[...]

>>>
>>> @@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>>>         if (_genpd_power_off(genpd, false))
>>>                 return;
>>>
>>> +       genpd_update_accounting(genpd);
>>
>> Timekeeping gets disabled at a certain point during suspend. Therefore
>> you can't update accounting here, as this function gets called in this
>> stages.
>
> I will remove the update of accounting at this point. But then the
> statistics  can be skewed for some power domains atleast. Other option
> is to restart the accounting every time after device suspend and resume.

I would suggest to start simple and just avoid updating the accounting
in these cases. Then we can look into improvements, if we think there
is a need for it.

What do you think?

[...]

Kind regards
Uffe

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Thara Gopinath]

On 06/13/2017 04:11 AM, Ulf Hansson wrote:
> [...]
> 
>>> Isn't the off_time the summary of the states' genpd->states[i].active_time?
>>
>> Yes. it is.
>>>
>>> Then we could do that computation quite easily at the point when
>>> producing the debugfs output instead.
>>
>> Are yu suggesting doing away with off_time parameter all together. It is
>> possible.
> 
> Yes, I think that makes sense.
> 
>>>
>>>> +       }
>>>> +       genpd->accounting_time = now;
>>>> +}
>>>> +
> 
> [...]
> 
>>>>
>>>> @@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>>>>         if (_genpd_power_off(genpd, false))
>>>>                 return;
>>>>
>>>> +       genpd_update_accounting(genpd);
>>>
>>> Timekeeping gets disabled at a certain point during suspend. Therefore
>>> you can't update accounting here, as this function gets called in this
>>> stages.
>>
>> I will remove the update of accounting at this point. But then the
>> statistics  can be skewed for some power domains atleast. Other option
>> is to restart the accounting every time after device suspend and resume.
> 
> I would suggest to start simple and just avoid updating the accounting
> in these cases. Then we can look into improvements, if we think there
> is a need for it.
> 
> What do you think?
I do agree. Will post the updated series.

Regards
Thara

> 
> [...]
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Thara Gopinath]

On 06/07/2017 09:28 AM, Ulf Hansson wrote:
Hello Ulf,

> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>> This patch adds support to calculate the time spent by the generic
>> power domains in on and various idle states.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  4 ++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index da49a83..e733796 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>         smp_mb__after_atomic();
>>  }
>>
>> +static void genpd_update_accounting(struct generic_pm_domain *genpd)
> 
> I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
> stub for !CONFIG_DEBUG_FS.
> 
>> +{
>> +       ktime_t delta, now;
>> +
>> +       now = ktime_get();
>> +       delta = ktime_sub(now, genpd->accounting_time);
>> +
>> +       if (genpd->status == GPD_STATE_ACTIVE) {
>> +               genpd->on_time = ktime_add(genpd->on_time, delta);
>> +       } else {
>> +               int state_idx = genpd->state_idx;
>> +
>> +               genpd->states[state_idx].active_time =
> 
> "active_time" seems to be a misleading, can we rename it to "idle_time" instead?
> 
>> +                       ktime_add(genpd->states[state_idx].active_time, delta);
>> +               genpd->off_time = ktime_add(genpd->off_time, delta);
> 
> Isn't the off_time the summary of the states' genpd->states[i].active_time?
> 
> Then we could do that computation quite easily at the point when
> producing the debugfs output instead.
> 
>> +       }
>> +       genpd->accounting_time = now;
>> +}
>> +
>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>  {
>>         unsigned int state_idx = genpd->state_idx;
>> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>>                         return ret;
>>         }
>>
>> +       genpd_update_accounting(genpd);
> 
> It find it a bit odd to update accounting just before changing the
> status. Can we re-work this and update accounting right afterwards
> instead? At least to me, that makes this easier to understand.

The accounting is being updated for the previous state and not the
current state. For eg if the domain has been previously off and just
turned on, the time spent in off state is updated. So it
upadte_accounting has to be before changing the status.
Sorry I did not mention this earlier as I just realized this as
I was reworking the patches.

Regards
Thara
> 
>>         genpd->status = GPD_STATE_POWER_OFF;
>>
>>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> @@ -410,6 +430,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
>>         if (ret)
>>                 goto err;
>>
>> +       genpd_update_accounting(genpd);
> 
> Ditto.
> 
>>         genpd->status = GPD_STATE_ACTIVE;
>>         return 0;
>>
>> @@ -774,6 +795,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>>         if (_genpd_power_off(genpd, false))
>>                 return;
>>
>> +       genpd_update_accounting(genpd);
> 
> Timekeeping gets disabled at a certain point during suspend. Therefore
> you can't update accounting here, as this function gets called in this
> stages.
> 
>>         genpd->status = GPD_STATE_POWER_OFF;
>>
>>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> @@ -821,6 +843,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
>>
>>         _genpd_power_on(genpd, false);
>>
>> +       genpd_update_accounting(genpd);
> 
> Ditto.
> 
>>         genpd->status = GPD_STATE_ACTIVE;
>>  }
>>
>> @@ -1486,6 +1509,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>         genpd->max_off_time_changed = true;
>>         genpd->provider = NULL;
>>         genpd->has_provider = false;
>> +       genpd->accounting_time = ktime_get();
>>         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>>         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
>>         genpd->domain.ops.prepare = pm_genpd_prepare;
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index b7803a2..8bee0e3 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -43,6 +43,7 @@ struct genpd_power_state {
>>         s64 power_on_latency_ns;
>>         s64 residency_ns;
>>         struct fwnode_handle *fwnode;
>> +       ktime_t active_time;
>>  };
>>
>>  struct genpd_lock_ops;
>> @@ -78,6 +79,9 @@ struct generic_pm_domain {
>>         unsigned int state_count; /* number of states */
>>         unsigned int state_idx; /* state that genpd will go to when off */
>>         void *free; /* Free the state that was allocated for default */
>> +       ktime_t on_time;
>> +       ktime_t off_time;
>> +       ktime_t accounting_time;
>>         const struct genpd_lock_ops *lock_ops;
>>         union {
>>                 struct mutex mlock;
>> --
>> 2.1.4
>>
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Ulf Hansson]

On 13 June 2017 at 22:17, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> On 06/07/2017 09:28 AM, Ulf Hansson wrote:
> Hello Ulf,
>
>> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>> This patch adds support to calculate the time spent by the generic
>>> power domains in on and various idle states.
>>>
>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>> ---
>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>  include/linux/pm_domain.h   |  4 ++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index da49a83..e733796 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>>         smp_mb__after_atomic();
>>>  }
>>>
>>> +static void genpd_update_accounting(struct generic_pm_domain *genpd)
>>
>> I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
>> stub for !CONFIG_DEBUG_FS.
>>
>>> +{
>>> +       ktime_t delta, now;
>>> +
>>> +       now = ktime_get();
>>> +       delta = ktime_sub(now, genpd->accounting_time);
>>> +
>>> +       if (genpd->status == GPD_STATE_ACTIVE) {
>>> +               genpd->on_time = ktime_add(genpd->on_time, delta);
>>> +       } else {
>>> +               int state_idx = genpd->state_idx;
>>> +
>>> +               genpd->states[state_idx].active_time =
>>
>> "active_time" seems to be a misleading, can we rename it to "idle_time" instead?
>>
>>> +                       ktime_add(genpd->states[state_idx].active_time, delta);
>>> +               genpd->off_time = ktime_add(genpd->off_time, delta);
>>
>> Isn't the off_time the summary of the states' genpd->states[i].active_time?
>>
>> Then we could do that computation quite easily at the point when
>> producing the debugfs output instead.
>>
>>> +       }
>>> +       genpd->accounting_time = now;
>>> +}
>>> +
>>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>>  {
>>>         unsigned int state_idx = genpd->state_idx;
>>> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>>>                         return ret;
>>>         }
>>>
>>> +       genpd_update_accounting(genpd);
>>
>> It find it a bit odd to update accounting just before changing the
>> status. Can we re-work this and update accounting right afterwards
>> instead? At least to me, that makes this easier to understand.
>
> The accounting is being updated for the previous state and not the
> current state. For eg if the domain has been previously off and just
> turned on, the time spent in off state is updated. So it
> upadte_accounting has to be before changing the status.
> Sorry I did not mention this earlier as I just realized this as
> I was reworking the patches.
>

Yes, I understand.

However, of course changing this also means that you need to change
how genpd_update_accounting works. That's was my main point, sorry if
that wasn't clear.

Kind regards
Uffe

Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Thara Gopinath]

On 06/14/2017 05:18 AM, Ulf Hansson wrote:
> On 13 June 2017 at 22:17, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>> On 06/07/2017 09:28 AM, Ulf Hansson wrote:
>> Hello Ulf,
>>
>>> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>> This patch adds support to calculate the time spent by the generic
>>>> power domains in on and various idle states.
>>>>
>>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>>> ---
>>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>>  include/linux/pm_domain.h   |  4 ++++
>>>>  2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index da49a83..e733796 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>>>         smp_mb__after_atomic();
>>>>  }
>>>>
>>>> +static void genpd_update_accounting(struct generic_pm_domain *genpd)
>>>
>>> I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
>>> stub for !CONFIG_DEBUG_FS.
>>>
>>>> +{
>>>> +       ktime_t delta, now;
>>>> +
>>>> +       now = ktime_get();
>>>> +       delta = ktime_sub(now, genpd->accounting_time);
>>>> +
>>>> +       if (genpd->status == GPD_STATE_ACTIVE) {
>>>> +               genpd->on_time = ktime_add(genpd->on_time, delta);
>>>> +       } else {
>>>> +               int state_idx = genpd->state_idx;
>>>> +
>>>> +               genpd->states[state_idx].active_time =
>>>
>>> "active_time" seems to be a misleading, can we rename it to "idle_time" instead?
>>>
>>>> +                       ktime_add(genpd->states[state_idx].active_time, delta);
>>>> +               genpd->off_time = ktime_add(genpd->off_time, delta);
>>>
>>> Isn't the off_time the summary of the states' genpd->states[i].active_time?
>>>
>>> Then we could do that computation quite easily at the point when
>>> producing the debugfs output instead.
>>>
>>>> +       }
>>>> +       genpd->accounting_time = now;
>>>> +}
>>>> +
>>>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>>>  {
>>>>         unsigned int state_idx = genpd->state_idx;
>>>> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>>>>                         return ret;
>>>>         }
>>>>
>>>> +       genpd_update_accounting(genpd);
>>>
>>> It find it a bit odd to update accounting just before changing the
>>> status. Can we re-work this and update accounting right afterwards
>>> instead? At least to me, that makes this easier to understand.
>>
>> The accounting is being updated for the previous state and not the
>> current state. For eg if the domain has been previously off and just
>> turned on, the time spent in off state is updated. So it
>> upadte_accounting has to be before changing the status.
>> Sorry I did not mention this earlier as I just realized this as
>> I was reworking the patches.
>>
> 
> Yes, I understand.
> 
> However, of course changing this also means that you need to change
> how genpd_update_accounting works. That's was my main point, sorry if
> that wasn't clear.
Ok. Sorry for all the confusion. It is possible to rework
genpd_update_accounting. I sent out the V2 yesterday. I will wait for a
couple of more days for any other review comments on V2 before sending
the re-worked patch set.

Regards
Thara

> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara

[PATCH 2/2] PM / Domains: Extend generic power domain debugfs.

[Thara Gopinath]

This patch extends the existing generic power domain debugfs.
Changes involve the following
- Remove the current flat format of displaying generic power domain
  summary.
- Introduce a unique debugfs entry for each generic power domain with the
  following attributes
	- current_state - Displays current state of the domain.
	- devices - Displays the devices associated with this domain.
	- slaves - Displays the sub power domains.
	- on_time - Displays the time the domain was in on state in ms.
	- idle_time - Displays the time the domain was in any of the idle
			states in ms.
	- idle_states - Displays the various idle states and the time
			spent in each idle state in ms.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/base/power/domain.c | 233 ++++++++++++++++++++++++++++----------------
 1 file changed, 150 insertions(+), 83 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e733796..3c06e33 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2214,137 +2214,204 @@ EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
 #include <linux/kobject.h>
 static struct dentry *pm_genpd_debugfs_dir;
 
-/*
- * TODO: This function is a slightly modified version of rtpm_status_show
- * from sysfs.c, so generalize it.
- */
-static void rtpm_status_str(struct seq_file *s, struct device *dev)
+static int pm_genpd_idle_states_show(struct seq_file *s, void *data)
 {
-	static const char * const status_lookup[] = {
-		[RPM_ACTIVE] = "active",
-		[RPM_RESUMING] = "resuming",
-		[RPM_SUSPENDED] = "suspended",
-		[RPM_SUSPENDING] = "suspending"
-	};
-	const char *p = "";
-
-	if (dev->power.runtime_error)
-		p = "error";
-	else if (dev->power.disable_depth)
-		p = "unsupported";
-	else if (dev->power.runtime_status < ARRAY_SIZE(status_lookup))
-		p = status_lookup[dev->power.runtime_status];
-	else
-		WARN_ON(1);
+	struct generic_pm_domain *genpd = s->private;
+	int i, ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	seq_puts(s, "State          Time Spent(ms)\n");
+
+	for (i = 0; i < genpd->state_count; i++) {
+		ktime_t delta = 0;
+		s64 msecs;
+
+		if ((genpd->status == GPD_STATE_POWER_OFF) &&
+				(genpd->state_idx == i))
+			delta = ktime_sub(ktime_get(), genpd->accounting_time);
+
+		msecs = ktime_to_ms(
+			ktime_add(genpd->states[i].active_time, delta));
+		seq_printf(s, "S%-15i %lld\n", i, msecs);
+	}
 
-	seq_puts(s, p);
+	genpd_unlock(genpd);
+	return ret;
 }
 
-static int pm_genpd_summary_one(struct seq_file *s,
-				struct generic_pm_domain *genpd)
+static int pm_genpd_slaves_show(struct seq_file *s, void *data)
 {
-	static const char * const status_lookup[] = {
-		[GPD_STATE_ACTIVE] = "on",
-		[GPD_STATE_POWER_OFF] = "off"
-	};
-	struct pm_domain_data *pm_data;
-	const char *kobj_path;
+	struct generic_pm_domain *genpd = s->private;
 	struct gpd_link *link;
-	char state[16];
-	int ret;
+	int ret = 0;
 
 	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
-	if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
-		goto exit;
-	if (!genpd_status_on(genpd))
-		snprintf(state, sizeof(state), "%s-%u",
-			 status_lookup[genpd->status], genpd->state_idx);
-	else
-		snprintf(state, sizeof(state), "%s",
-			 status_lookup[genpd->status]);
-	seq_printf(s, "%-30s  %-15s ", genpd->name, state);
-
-	/*
-	 * Modifications on the list require holding locks on both
-	 * master and slave, so we are safe.
-	 * Also genpd->name is immutable.
-	 */
 	list_for_each_entry(link, &genpd->master_links, master_node) {
 		seq_printf(s, "%s", link->slave->name);
 		if (!list_is_last(&link->master_node, &genpd->master_links))
 			seq_puts(s, ", ");
 	}
+	seq_puts(s, "\n");
 
-	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj,
-				genpd_is_irq_safe(genpd) ?
-				GFP_ATOMIC : GFP_KERNEL);
-		if (kobj_path == NULL)
-			continue;
+	genpd_unlock(genpd);
+	return ret;
+}
 
-		seq_printf(s, "\n    %-50s  ", kobj_path);
-		rtpm_status_str(s, pm_data->dev);
-		kfree(kobj_path);
-	}
+static int pm_genpd_status_show(struct seq_file *s, void *data)
+{
+	static const char * const status_lookup[] = {
+		[GPD_STATE_ACTIVE] = "on",
+		[GPD_STATE_POWER_OFF] = "off"
+	};
 
-	seq_puts(s, "\n");
+	struct generic_pm_domain *genpd = s->private;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
+		goto exit;
+
+	if (genpd->status == GPD_STATE_POWER_OFF)
+		seq_printf(s, "%s-%u\n", status_lookup[genpd->status],
+			genpd->state_idx);
+	else
+		seq_printf(s, "%s\n", status_lookup[genpd->status]);
 exit:
 	genpd_unlock(genpd);
+	return ret;
+}
 
-	return 0;
+static int pm_genpd_on_time_show(struct seq_file *s, void *data)
+{
+	struct generic_pm_domain *genpd = s->private;
+	ktime_t delta = 0;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (genpd->status == GPD_STATE_ACTIVE)
+		delta = ktime_sub(ktime_get(), genpd->accounting_time);
+
+	seq_printf(s, "%lld ms\n", ktime_to_ms(
+				ktime_add(genpd->on_time, delta)));
+
+	genpd_unlock(genpd);
+	return ret;
 }
 
-static int pm_genpd_summary_show(struct seq_file *s, void *data)
+static int pm_genpd_idle_time_show(struct seq_file *s, void *data)
 {
-	struct generic_pm_domain *genpd;
+	struct generic_pm_domain *genpd = s->private;
+	ktime_t delta = 0;
 	int ret = 0;
 
-	seq_puts(s, "domain                          status          slaves\n");
-	seq_puts(s, "    /device                                             runtime status\n");
-	seq_puts(s, "----------------------------------------------------------------------\n");
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (genpd->status == GPD_STATE_POWER_OFF)
+		delta = ktime_sub(ktime_get(), genpd->accounting_time);
+
+	seq_printf(s, "%lld ms\n",
+			ktime_to_ms(ktime_add(genpd->off_time, delta)));
 
-	ret = mutex_lock_interruptible(&gpd_list_lock);
+	genpd_unlock(genpd);
+	return ret;
+}
+
+static int pm_genpd_devices_show(struct seq_file *s, void *data)
+{
+	struct generic_pm_domain *genpd = s->private;
+	struct pm_domain_data *pm_data;
+	const char *kobj_path;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
-	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
-		ret = pm_genpd_summary_one(s, genpd);
-		if (ret)
-			break;
+	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
+		kobj_path = kobject_get_path(&pm_data->dev->kobj,
+				genpd_is_irq_safe(genpd) ?
+				GFP_ATOMIC : GFP_KERNEL);
+		if (kobj_path == NULL)
+			continue;
+
+		seq_printf(s, "%s\n", kobj_path);
+		kfree(kobj_path);
 	}
-	mutex_unlock(&gpd_list_lock);
 
+	genpd_unlock(genpd);
 	return ret;
 }
 
-static int pm_genpd_summary_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, pm_genpd_summary_show, NULL);
+#define define_pm_genpd_open_function(name) \
+static int pm_genpd_##name##_open(struct inode *inode, struct file *file) \
+{ \
+	return single_open(file, pm_genpd_##name##_show, inode->i_private); \
 }
 
-static const struct file_operations pm_genpd_summary_fops = {
-	.open = pm_genpd_summary_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
+define_pm_genpd_open_function(status);
+define_pm_genpd_open_function(slaves);
+define_pm_genpd_open_function(idle_states);
+define_pm_genpd_open_function(on_time);
+define_pm_genpd_open_function(idle_time);
+define_pm_genpd_open_function(devices);
+
+#define define_pm_genpd_debugfs_fops(name) \
+static const struct file_operations pm_genpd_##name##_fops = { \
+	.open = pm_genpd_##name##_open, \
+	.read = seq_read, \
+	.llseek = seq_lseek, \
+	.release = single_release, \
+}
+
+define_pm_genpd_debugfs_fops(status);
+define_pm_genpd_debugfs_fops(slaves);
+define_pm_genpd_debugfs_fops(idle_states);
+define_pm_genpd_debugfs_fops(on_time);
+define_pm_genpd_debugfs_fops(idle_time);
+define_pm_genpd_debugfs_fops(devices);
 
 static int __init pm_genpd_debug_init(void)
 {
 	struct dentry *d;
+	struct generic_pm_domain *genpd;
 
 	pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
 
 	if (!pm_genpd_debugfs_dir)
 		return -ENOMEM;
 
-	d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
-			pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
-	if (!d)
-		return -ENOMEM;
+	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+		d = debugfs_create_dir(genpd->name, pm_genpd_debugfs_dir);
+		if (!d)
+			return -ENOMEM;
+
+		debugfs_create_file("current_state", 0444,
+				d, genpd, &pm_genpd_status_fops);
+		debugfs_create_file("slaves", 0444,
+				d, genpd, &pm_genpd_slaves_fops);
+		debugfs_create_file("idle_states", 0444,
+				d, genpd, &pm_genpd_idle_states_fops);
+		debugfs_create_file("on_time", 0444,
+				d, genpd, &pm_genpd_on_time_fops);
+		debugfs_create_file("idle_time", 0444,
+				d, genpd, &pm_genpd_idle_time_fops);
+		debugfs_create_file("devices", 0444,
+				d, genpd, &pm_genpd_devices_fops);
+	}
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH 2/2] PM / Domains: Extend generic power domain debugfs.

[Geert Uytterhoeven]

Hi Thara,

On Fri, May 26, 2017 at 1:51 AM, Thara Gopinath
<thara.gopinath@linaro.org> wrote:
> This patch extends the existing generic power domain debugfs.
> Changes involve the following
> - Remove the current flat format of displaying generic power domain
>   summary.
> - Introduce a unique debugfs entry for each generic power domain with the
>   following attributes
>         - current_state - Displays current state of the domain.
>         - devices - Displays the devices associated with this domain.
>         - slaves - Displays the sub power domains.
>         - on_time - Displays the time the domain was in on state in ms.
>         - idle_time - Displays the time the domain was in any of the idle
>                         states in ms.
>         - idle_states - Displays the various idle states and the time
>                         spent in each idle state in ms.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

While I don't doubt the merits of having all this additional information in
separate debugfs files, I think it would be a good idea to keep the existing
summary.

Cfr. the clock subsystem, which provides both a summary, and clock-specific
information in individual files.

BTW, if people disagree, I'll have to write a script to extract all information
and combine it into a single file ;-) I tend to compare these summaries across
boots...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] PM / Domains: Extend generic power domain debugfs.

[Thara Gopinath]

Hi Geert,

On 05/29/2017 11:12 AM, Geert Uytterhoeven wrote:
> Hi Thara,
> 
> On Fri, May 26, 2017 at 1:51 AM, Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
>> This patch extends the existing generic power domain debugfs.
>> Changes involve the following
>> - Remove the current flat format of displaying generic power domain
>>   summary.
>> - Introduce a unique debugfs entry for each generic power domain with the
>>   following attributes
>>         - current_state - Displays current state of the domain.
>>         - devices - Displays the devices associated with this domain.
>>         - slaves - Displays the sub power domains.
>>         - on_time - Displays the time the domain was in on state in ms.
>>         - idle_time - Displays the time the domain was in any of the idle
>>                         states in ms.
>>         - idle_states - Displays the various idle states and the time
>>                         spent in each idle state in ms.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> 
> While I don't doubt the merits of having all this additional information in
> separate debugfs files, I think it would be a good idea to keep the existing
> summary.

Since you have a need for the summary, I will keep it intact and post a
new version of the patches.

> 
> Cfr. the clock subsystem, which provides both a summary, and clock-specific
> information in individual files.
> 
> BTW, if people disagree, I'll have to write a script to extract all information
> and combine it into a single file ;-) I tend to compare these summaries across
> boots...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Regards
Thara

Re: [PATCH 2/2] PM / Domains: Extend generic power domain debugfs.

[Ulf Hansson]

On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> This patch extends the existing generic power domain debugfs.
> Changes involve the following
> - Remove the current flat format of displaying generic power domain
>   summary.
> - Introduce a unique debugfs entry for each generic power domain with the
>   following attributes
>         - current_state - Displays current state of the domain.
>         - devices - Displays the devices associated with this domain.
>         - slaves - Displays the sub power domains.

Maybe "sub_domains" is a better name for the node then?

>         - on_time - Displays the time the domain was in on state in ms.

/s/on_time/active_time

>         - idle_time - Displays the time the domain was in any of the idle
>                         states in ms.

/s/idle_time/total_idle_time

>         - idle_states - Displays the various idle states and the time
>                         spent in each idle state in ms.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

Besides the nitpicks above, this looks okay to me, however please
address Geerts comments as well.

[...]

Kind regards
Uffe

Re: [PATCH 2/2] PM / Domains: Extend generic power domain debugfs.

[Thara Gopinath]

On 06/07/2017 09:51 AM, Ulf Hansson wrote:
> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>> This patch extends the existing generic power domain debugfs.
>> Changes involve the following
>> - Remove the current flat format of displaying generic power domain
>>   summary.
>> - Introduce a unique debugfs entry for each generic power domain with the
>>   following attributes
>>         - current_state - Displays current state of the domain.
>>         - devices - Displays the devices associated with this domain.
>>         - slaves - Displays the sub power domains.
> 
> Maybe "sub_domains" is a better name for the node then?
> 
>>         - on_time - Displays the time the domain was in on state in ms.
> 
> /s/on_time/active_time
> 
>>         - idle_time - Displays the time the domain was in any of the idle
>>                         states in ms.
> 
> /s/idle_time/total_idle_time
> 
>>         - idle_states - Displays the various idle states and the time
>>                         spent in each idle state in ms.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> 
> Besides the nitpicks above, this looks okay to me, however please
> address Geerts comments as well.
Will fix all the above comments and re-send

> 
> [...]
> 
> Kind regards
> Uffe
> 


-- 
Regards
Thara