[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 at 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 at 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 at samsung.com>
>> Signed-off-by: Prathyush K <prathyush.k at samsung.com>
>> Signed-off-by: Sachin Kamat <sachin.kamat at 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