| Message ID | 1383892025-14759-1-git-send-email-sachin.kamat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> +}; >> + >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + /* * 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, +}; + 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); 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; pd->pd.of_node = np; platform_set_drvdata(pdev, pd);