[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Tomasz Figa]

Hi Sachin, Prasanna,

[CCing Rafael and respective mailing lists]

Please see my comments inline. Also please always remember to add all
appropriate recipients on CC list. More reviewers means higher chance of
spotting (and so eliminating) potential issues.

On Friday 08 of November 2013 11:57:05 Sachin Kamat wrote:
> From: Prasanna Kumar <prasanna.ps@samsung.com>
> 
> Power domain and device timing data are intialized with default
> values to avoid dump of warnings from various power domains
> during power gating.
> 
> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  arch/arm/mach-exynos/pm_domains.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
> index 84e0483a0500..9bbb4ac23980 100644
> --- a/arch/arm/mach-exynos/pm_domains.c
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -25,6 +25,10 @@
>  #include <mach/regs-pmu.h>
>  #include <plat/devs.h>
>  
> +#define DEFAULT_DEV_LATENCY_NS			1000000UL
> +#define DEFAULT_PD_PWRON_LATENCY_NS		10000000UL
> +#define DEFAULT_PD_PWROFF_LATENCY_NS		10000000UL

Is there any rationale behind choosing these particular values?

> +
>  /*
>   * Exynos specific wrapper around the generic power domain
>   */
> @@ -36,6 +40,13 @@ struct exynos_pm_domain {
>  	u32 enable;
>  };
>  
> +static struct gpd_timing_data dev_latencies = {
> +	.stop_latency_ns = DEFAULT_DEV_LATENCY_NS,
> +	.start_latency_ns = DEFAULT_DEV_LATENCY_NS,
> +	.save_state_latency_ns = DEFAULT_DEV_LATENCY_NS,
> +	.restore_state_latency_ns = DEFAULT_DEV_LATENCY_NS,

I don't think that stop, start, save and restore latencies should be all
the same.

> +};
> +
>  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>  {
>  	struct exynos_pm_domain *pd;
> @@ -83,7 +94,7 @@ static void exynos_add_device_to_domain(struct exynos_pm_domain *pd,
>  	dev_dbg(dev, "adding to power domain %s\n", pd->pd.name);
>  
>  	while (1) {
> -		ret = pm_genpd_add_device(&pd->pd, dev);
> +		ret = __pm_genpd_add_device(&pd->pd, dev, &dev_latencies);

The double underscore prefix scares me a bit. Is this function really
supposed to be used like this?

>  		if (ret != -EAGAIN)
>  			break;
>  		cond_resched();
> @@ -173,6 +184,8 @@ static __init int exynos4_pm_init_power_domain(void)
>  		pd->base = of_iomap(np, 0);
>  		pd->pd.power_off = exynos_pd_power_off;
>  		pd->pd.power_on = exynos_pd_power_on;
> +		pd->pd.power_off_latency_ns = DEFAULT_PD_PWROFF_LATENCY_NS;
> +		pd->pd.power_on_latency_ns = DEFAULT_PD_PWRON_LATENCY_NS;

It might be worth to set up the latencies indeed, but wrong values can do
more harm than good.

Best regards,
Tomasz

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Sachin Kamat]

Hi Tomasz,

Thanks for reviewing.

On 10 November 2013 22:25, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sachin, Prasanna,
>
> [CCing Rafael and respective mailing lists]
>
> Please see my comments inline. Also please always remember to add all
> appropriate recipients on CC list. More reviewers means higher chance of
> spotting (and so eliminating) potential issues.

Indeed. Thanks for adding. get_maintainers somehow did not list PM
related folks.
So missed this.

>
> On Friday 08 of November 2013 11:57:05 Sachin Kamat wrote:
>> From: Prasanna Kumar <prasanna.ps@samsung.com>
>>
>> Power domain and device timing data are intialized with default
>> values to avoid dump of warnings from various power domains
>> during power gating.
>>
>> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
>> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  arch/arm/mach-exynos/pm_domains.c |   15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>> index 84e0483a0500..9bbb4ac23980 100644
>> --- a/arch/arm/mach-exynos/pm_domains.c
>> +++ b/arch/arm/mach-exynos/pm_domains.c
>> @@ -25,6 +25,10 @@
>>  #include <mach/regs-pmu.h>
>>  #include <plat/devs.h>
>>
>> +#define DEFAULT_DEV_LATENCY_NS                       1000000UL
>> +#define DEFAULT_PD_PWRON_LATENCY_NS          10000000UL
>> +#define DEFAULT_PD_PWROFF_LATENCY_NS         10000000UL
>
> Is there any rationale behind choosing these particular values?

IMO, these values were obtained more from experimentation. Prasanna,
please comment.

>
>> +
>>  /*
>>   * Exynos specific wrapper around the generic power domain
>>   */
>> @@ -36,6 +40,13 @@ struct exynos_pm_domain {
>>       u32 enable;
>>  };
>>
>> +static struct gpd_timing_data dev_latencies = {
>> +     .stop_latency_ns = DEFAULT_DEV_LATENCY_NS,
>> +     .start_latency_ns = DEFAULT_DEV_LATENCY_NS,
>> +     .save_state_latency_ns = DEFAULT_DEV_LATENCY_NS,
>> +     .restore_state_latency_ns = DEFAULT_DEV_LATENCY_NS,
>
> I don't think that stop, start, save and restore latencies should be all
> the same.

They could be different. But again as I said it was based on
experimentation rather than reference.

>
>> +};
>> +
>>  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>>  {
>>       struct exynos_pm_domain *pd;
>> @@ -83,7 +94,7 @@ static void exynos_add_device_to_domain(struct exynos_pm_domain *pd,
>>       dev_dbg(dev, "adding to power domain %s\n", pd->pd.name);
>>
>>       while (1) {
>> -             ret = pm_genpd_add_device(&pd->pd, dev);
>> +             ret = __pm_genpd_add_device(&pd->pd, dev, &dev_latencies);
>
> The double underscore prefix scares me a bit. Is this function really
> supposed to be used like this?

We did not find a wrapper/helper function to set this. Hence had to
use this. In fact I did not
find any other users of this function (pm_genpd_add_device) too.

>
>>               if (ret != -EAGAIN)
>>                       break;
>>               cond_resched();
>> @@ -173,6 +184,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>               pd->base = of_iomap(np, 0);
>>               pd->pd.power_off = exynos_pd_power_off;
>>               pd->pd.power_on = exynos_pd_power_on;
>> +             pd->pd.power_off_latency_ns = DEFAULT_PD_PWROFF_LATENCY_NS;
>> +             pd->pd.power_on_latency_ns = DEFAULT_PD_PWRON_LATENCY_NS;
>
> It might be worth to set up the latencies indeed, but wrong values can do
> more harm than good.

Right. As of this now, this is a well tested set of values.

-- 
With warm regards,
Sachin

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Prasanna Kumar]

On Mon, Nov 11, 2013 at 8:52 AM, Sachin Kamat <sachin.kamat@linaro.org>
wrote:
>
> Hi Tomasz,
>
> Thanks for reviewing.
>
> On 10 November 2013 22:25, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > Hi Sachin, Prasanna,
> >
> > [CCing Rafael and respective mailing lists]
> >
> > Please see my comments inline. Also please always remember to add all
> > appropriate recipients on CC list. More reviewers means higher chance of
> > spotting (and so eliminating) potential issues.
>
> Indeed. Thanks for adding. get_maintainers somehow did not list PM
> related folks.
> So missed this.
>
> >
> > On Friday 08 of November 2013 11:57:05 Sachin Kamat wrote:
> >> From: Prasanna Kumar <prasanna.ps@samsung.com>
> >>
> >> Power domain and device timing data are intialized with default
> >> values to avoid dump of warnings from various power domains
> >> during power gating.
> >>
> >> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
> >> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >>  arch/arm/mach-exynos/pm_domains.c |   15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/mach-exynos/pm_domains.c
> >> b/arch/arm/mach-exynos/pm_domains.c
> >> index 84e0483a0500..9bbb4ac23980 100644
> >> --- a/arch/arm/mach-exynos/pm_domains.c
> >> +++ b/arch/arm/mach-exynos/pm_domains.c
> >> @@ -25,6 +25,10 @@
> >>  #include <mach/regs-pmu.h>
> >>  #include <plat/devs.h>
> >>
> >> +#define DEFAULT_DEV_LATENCY_NS                       1000000UL
> >> +#define DEFAULT_PD_PWRON_LATENCY_NS          10000000UL
> >> +#define DEFAULT_PD_PWROFF_LATENCY_NS         10000000UL
> >
> > Is there any rationale behind choosing these particular values?
>
> IMO, these values were obtained more from experimentation. Prasanna,
> please comment.
As Sachin mentioned, the values are obtained from experimentation to avoid
latency warning messages. These values can be changed when the
information becomes available from the specs.
As of now , this seems to be good set of values.
>
> >
> >> +
> >>  /*
> >>   * Exynos specific wrapper around the generic power domain
> >>   */
> >> @@ -36,6 +40,13 @@ struct exynos_pm_domain {
> >>       u32 enable;
> >>  };
> >>
> >> +static struct gpd_timing_data dev_latencies = {
> >> +     .stop_latency_ns = DEFAULT_DEV_LATENCY_NS,
> >> +     .start_latency_ns = DEFAULT_DEV_LATENCY_NS,
> >> +     .save_state_latency_ns = DEFAULT_DEV_LATENCY_NS,
> >> +     .restore_state_latency_ns = DEFAULT_DEV_LATENCY_NS,
> >
> > I don't think that stop, start, save and restore latencies should be all
> > the same.
>
> They could be different. But again as I said it was based on
> experimentation rather than reference.
>
> >
> >> +};
> >> +
> >>  static int exynos_pd_power(struct generic_pm_domain *domain, bool
> >> power_on)
> >>  {
> >>       struct exynos_pm_domain *pd;
> >> @@ -83,7 +94,7 @@ static void exynos_add_device_to_domain(struct
> >> exynos_pm_domain *pd,
> >>       dev_dbg(dev, "adding to power domain %s\n", pd->pd.name);
> >>
> >>       while (1) {
> >> -             ret = pm_genpd_add_device(&pd->pd, dev);
> >> +             ret = __pm_genpd_add_device(&pd->pd, dev,
> >> &dev_latencies);
> >
> > The double underscore prefix scares me a bit. Is this function really
> > supposed to be used like this?
>
> We did not find a wrapper/helper function to set this. Hence had to
> use this. In fact I did not
> find any other users of this function (pm_genpd_add_device) too.
>
> >
> >>               if (ret != -EAGAIN)
> >>                       break;
> >>               cond_resched();
> >> @@ -173,6 +184,8 @@ static __init int
> >> exynos4_pm_init_power_domain(void)
> >>               pd->base = of_iomap(np, 0);
> >>               pd->pd.power_off = exynos_pd_power_off;
> >>               pd->pd.power_on = exynos_pd_power_on;
> >> +             pd->pd.power_off_latency_ns =
> >> DEFAULT_PD_PWROFF_LATENCY_NS;
> >> +             pd->pd.power_on_latency_ns = DEFAULT_PD_PWRON_LATENCY_NS;
> >
> > It might be worth to set up the latencies indeed, but wrong values can
> > do
> > more harm than good.
>
> Right. As of this now, this is a well tested set of values.
>
> --
regards,
Prasanna

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Tomasz Figa]

Hi Prasanna,

On Monday 11 of November 2013 10:19:40 Prasanna Kumar wrote:
> On Mon, Nov 11, 2013 at 8:52 AM, Sachin Kamat <sachin.kamat@linaro.org>
> wrote:
> >
> > Hi Tomasz,
> >
> > Thanks for reviewing.
> >
> > On 10 November 2013 22:25, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > > Hi Sachin, Prasanna,
> > >
> > > [CCing Rafael and respective mailing lists]
> > >
> > > Please see my comments inline. Also please always remember to add all
> > > appropriate recipients on CC list. More reviewers means higher chance of
> > > spotting (and so eliminating) potential issues.
> >
> > Indeed. Thanks for adding. get_maintainers somehow did not list PM
> > related folks.
> > So missed this.
> >
> > >
> > > On Friday 08 of November 2013 11:57:05 Sachin Kamat wrote:
> > >> From: Prasanna Kumar <prasanna.ps@samsung.com>
> > >>
> > >> Power domain and device timing data are intialized with default
> > >> values to avoid dump of warnings from various power domains
> > >> during power gating.
> > >>
> > >> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
> > >> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> > >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > >> ---
> > >>  arch/arm/mach-exynos/pm_domains.c |   15 ++++++++++++++-
> > >>  1 file changed, 14 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/arm/mach-exynos/pm_domains.c
> > >> b/arch/arm/mach-exynos/pm_domains.c
> > >> index 84e0483a0500..9bbb4ac23980 100644
> > >> --- a/arch/arm/mach-exynos/pm_domains.c
> > >> +++ b/arch/arm/mach-exynos/pm_domains.c
> > >> @@ -25,6 +25,10 @@
> > >>  #include <mach/regs-pmu.h>
> > >>  #include <plat/devs.h>
> > >>
> > >> +#define DEFAULT_DEV_LATENCY_NS                       1000000UL
> > >> +#define DEFAULT_PD_PWRON_LATENCY_NS          10000000UL
> > >> +#define DEFAULT_PD_PWROFF_LATENCY_NS         10000000UL
> > >
> > > Is there any rationale behind choosing these particular values?
> >
> > IMO, these values were obtained more from experimentation. Prasanna,
> > please comment.
> As Sachin mentioned, the values are obtained from experimentation to avoid
> latency warning messages. These values can be changed when the
> information becomes available from the specs.
> As of now , this seems to be good set of values.

Do you have any measurements that confirm that there is no regression
in power consumption (e.g. caused by unnecessarily delayed runtime PM
operations) after applying this patch?

Best regards,
Tomasz

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Yadwinder Singh Brar]

>> +};
>> +
>>  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>>  {
>>       struct exynos_pm_domain *pd;
>> @@ -83,7 +94,7 @@ static void exynos_add_device_to_domain(struct exynos_pm_domain *pd,
>>       dev_dbg(dev, "adding to power domain %s\n", pd->pd.name);
>>
>>       while (1) {
>> -             ret = pm_genpd_add_device(&pd->pd, dev);
>> +             ret = __pm_genpd_add_device(&pd->pd, dev, &dev_latencies);
>
> The double underscore prefix scares me a bit. Is this function really
> supposed to be used like this?
>

Moreover, it also seems little bit odd in the first place, to pass
dev_latencies(QoS timing parameters) when we are not using/providing
any governor for the genpd. QoS timing parameters have to be provided
to be used by governor. In our case since we are not using/providing
any governor yet, so it seems odd to provide QoS timing parameters. It
seems, here core pd code is giving unnecessary warning in our case,
since we are not providing governor(not interested in QoS). So IMO
warning should be fixed instead of just suppressing it by giving some
big values of timing parameters.


Regards,
Yadwinder

>>               if (ret != -EAGAIN)
>>                       break;
>>               cond_resched();
>> @@ -173,6 +184,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>               pd->base = of_iomap(np, 0);
>>               pd->pd.power_off = exynos_pd_power_off;
>>               pd->pd.power_on = exynos_pd_power_on;
>> +             pd->pd.power_off_latency_ns = DEFAULT_PD_PWROFF_LATENCY_NS;
>> +             pd->pd.power_on_latency_ns = DEFAULT_PD_PWRON_LATENCY_NS;
>
> It might be worth to set up the latencies indeed, but wrong values can do
> more harm than good.
>
> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] ARM: EXYNOS: Add default latency values for Device and Power Domain

[Tomasz Figa]

On Monday 11 of November 2013 23:11:41 Yadwinder Singh Brar wrote:
> >> +};
> >> +
> >>  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
> >>  {
> >>       struct exynos_pm_domain *pd;
> >> @@ -83,7 +94,7 @@ static void exynos_add_device_to_domain(struct exynos_pm_domain *pd,
> >>       dev_dbg(dev, "adding to power domain %s\n", pd->pd.name);
> >>
> >>       while (1) {
> >> -             ret = pm_genpd_add_device(&pd->pd, dev);
> >> +             ret = __pm_genpd_add_device(&pd->pd, dev, &dev_latencies);
> >
> > The double underscore prefix scares me a bit. Is this function really
> > supposed to be used like this?
> >
> 
> Moreover, it also seems little bit odd in the first place, to pass
> dev_latencies(QoS timing parameters) when we are not using/providing
> any governor for the genpd. QoS timing parameters have to be provided
> to be used by governor. In our case since we are not using/providing
> any governor yet, so it seems odd to provide QoS timing parameters. It
> seems, here core pd code is giving unnecessary warning in our case,
> since we are not providing governor(not interested in QoS). So IMO
> warning should be fixed instead of just suppressing it by giving some
> big values of timing parameters.
> 

Yes, that would be probably much better option than providing some random
values that do not have any rationale behind them.

Rafael, could you comment on this?

Best regards,
Tomasz