| Message ID | 1430391335-7588-2-git-send-email-ahaslam@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >From: Axel Haslam <ahaslam+renesas@baylibre.com> > >prepare generic power domain init function parameters >to accept a pointer to the states structure that describes >the possible states that a power domain can enter. > >There is no functional change, as support for multiple >domains will be added in subsequent patches. > >Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> <...> > extern void pm_genpd_init(struct generic_pm_domain *genpd, >- struct dev_power_governor *gov, bool is_off); >+ struct dev_power_governor *gov, >+ const struct genpd_power_state *states, >+ unsigned int state_count, bool is_off); > Wouldnt it be better to setup another function pm_genpd_init_simple() that calls into pm_genpd_init() with no arguments? static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off) { return pm_genpd_init(genpd, gov, NULL, 0, is_off); } It would be explicit that way to indicate the new genpd feature that would let domains support multiple states. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina, On 30/04/2015 17:29, Lina Iyer wrote: > On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >> From: Axel Haslam <ahaslam+renesas@baylibre.com> >> >> prepare generic power domain init function parameters >> to accept a pointer to the states structure that describes >> the possible states that a power domain can enter. >> >> There is no functional change, as support for multiple >> domains will be added in subsequent patches. >> >> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> > > <...> > >> extern void pm_genpd_init(struct generic_pm_domain *genpd, >> - struct dev_power_governor *gov, bool is_off); >> + struct dev_power_governor *gov, >> + const struct genpd_power_state *states, >> + unsigned int state_count, bool is_off); >> > > Wouldnt it be better to setup another function pm_genpd_init_simple() > that calls into pm_genpd_init() with no arguments? > > static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off) > { > return pm_genpd_init(genpd, gov, NULL, 0, is_off); > } Im not against adding the wrapper if it simplifies things. But, in general, i think all latencies should be set, otherwise genpd may violate device constraints by turning a power domain off when it should not. So maybe, by leaving the arguments, it kind of sends the message to the developer that something important is needed. Regards, Axel > > It would be explicit that way to indicate the new genpd feature that > would let > domains support multiple states. > > Thanks, > Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 30 2015 at 10:06 -0600, Axel Haslam wrote: >Hi Lina, > >On 30/04/2015 17:29, Lina Iyer wrote: >>On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >>>From: Axel Haslam <ahaslam+renesas@baylibre.com> >>> >>>prepare generic power domain init function parameters >>>to accept a pointer to the states structure that describes >>>the possible states that a power domain can enter. >>> >>>There is no functional change, as support for multiple >>>domains will be added in subsequent patches. >>> >>>Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> >> >><...> >> >>>extern void pm_genpd_init(struct generic_pm_domain *genpd, >>>- struct dev_power_governor *gov, bool is_off); >>>+ struct dev_power_governor *gov, >>>+ const struct genpd_power_state *states, >>>+ unsigned int state_count, bool is_off); >>> >> >>Wouldnt it be better to setup another function pm_genpd_init_simple() >>that calls into pm_genpd_init() with no arguments? >> >>static inline void pm_genpd_init_simple(struct generic_pm_domain *genpd, >> struct dev_power_governor *gov, bool is_off) >>{ >> return pm_genpd_init(genpd, gov, NULL, 0, is_off); >>} > >Im not against adding the wrapper if it simplifies things. But, in >general, i think all latencies should be set, otherwise genpd may >violate device constraints by turning a power domain off when it >should not. So maybe, by leaving the arguments, it kind of sends the >message to the developer that something important is needed. hmm. Alright. > >Regards, >Axel > >> >>It would be explicit that way to indicate the new genpd feature that >>would let >>domains support multiple states. >> >>Thanks, >>Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 30, 2015 at 6:06 PM, Axel Haslam <ahaslam@baylibre.com> wrote: > Im not against adding the wrapper if it simplifies things. But, in general, > i think all latencies should be set, otherwise genpd may violate device > constraints by turning a power domain off when it should not. So maybe, by > leaving the arguments, it kind of sends the message to the developer that > something important is needed. Note that in the absence of latencies, device constraints may indeed be violated once. After that the genpd core will have measured the real latency value, and will start using that. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-04-30 19:55 GMT+09:00 <ahaslam@baylibre.com>: > From: Axel Haslam <ahaslam+renesas@baylibre.com> > > prepare generic power domain init function parameters > to accept a pointer to the states structure that describes > the possible states that a power domain can enter. > > There is no functional change, as support for multiple > domains will be added in subsequent patches. > > Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> > --- > arch/arm/mach-exynos/pm_domains.c | 2 +- For Exynos: Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> However could you fix up your GIT or email app? It is wrapping lines in commit message around ~55 character making the message unreadable. Just use default value of 75 characters (gui.commitmsgwidth?). Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On Fri, May 8, 2015 at 2:36 AM, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > 2015-04-30 19:55 GMT+09:00 <ahaslam@baylibre.com>: >> From: Axel Haslam <ahaslam+renesas@baylibre.com> >> >> prepare generic power domain init function parameters >> to accept a pointer to the states structure that describes >> the possible states that a power domain can enter. >> >> There is no functional change, as support for multiple >> domains will be added in subsequent patches. >> >> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> >> --- >> arch/arm/mach-exynos/pm_domains.c | 2 +- > > For Exynos: > Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > However could you fix up your GIT or email app? It is wrapping lines > in commit message around ~55 character making the message unreadable. > Just use default value of 75 characters (gui.commitmsgwidth?). Ok, ill change the message width in the next series, ill wait for acks or comments to the rest of the patches before sending it. Regards, Axel. > > Best regards, > Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >From: Axel Haslam <ahaslam+renesas@baylibre.com> > >prepare generic power domain init function parameters >to accept a pointer to the states structure that describes >the possible states that a power domain can enter. > >There is no functional change, as support for multiple >domains will be added in subsequent patches. > >Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> [...] >diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >index 681ccb0..6b4802e 100644 >--- a/include/linux/pm_domain.h >+++ b/include/linux/pm_domain.h >@@ -46,6 +46,12 @@ struct gpd_cpuidle_data { > struct cpuidle_state *idle_state; > }; > >+struct genpd_power_state { >+ char *name; >+ s64 power_off_latency_ns; >+ s64 power_on_latency_ns; >+}; >+ I know you carried this over from the existing code, but any reason why this should be signed? -- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina, On Tue, May 12, 2015 at 5:37 PM, Lina Iyer <lina.iyer@linaro.org> wrote: > On Thu, Apr 30 2015 at 04:57 -0600, ahaslam@baylibre.com wrote: >> >> From: Axel Haslam <ahaslam+renesas@baylibre.com> >> >> prepare generic power domain init function parameters >> to accept a pointer to the states structure that describes >> the possible states that a power domain can enter. >> >> There is no functional change, as support for multiple >> domains will be added in subsequent patches. >> >> Signed-off-by: Axel Haslam <ahaslam+renesas@baylibre.com> > > > [...] > >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 681ccb0..6b4802e 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -46,6 +46,12 @@ struct gpd_cpuidle_data { >> struct cpuidle_state *idle_state; >> }; >> >> +struct genpd_power_state { >> + char *name; >> + s64 power_off_latency_ns; >> + s64 power_on_latency_ns; >> +}; >> + > > > I know you carried this over from the existing code, but any reason why > this should be signed? > correct, i just carried this values over as they were originally. i suppose they can be changed to u64. maybe a separate cleanup patch, as it would make sense to change some other s64's in domain.c, like "s64 elapsed_ns;" > -- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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 cbe56b3..3c68239 100644 --- a/arch/arm/mach-exynos/pm_domains.c +++ b/arch/arm/mach-exynos/pm_domains.c @@ -176,7 +176,7 @@ static __init int exynos4_pm_init_power_domain(void) no_clk: on = __raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN; - pm_genpd_init(&pd->pd, NULL, !on); + pm_genpd_init(&pd->pd, NULL, NULL, 0, !on); of_genpd_add_provider_simple(np, &pd->pd); } diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c index 4d60005..092b4ae 100644 --- a/arch/arm/mach-imx/gpc.c +++ b/arch/arm/mach-imx/gpc.c @@ -421,7 +421,7 @@ static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg) imx6q_pm_pu_power_on(&imx6q_pu_domain.base); } - pm_genpd_init(&imx6q_pu_domain.base, NULL, is_off); + pm_genpd_init(&imx6q_pu_domain.base, NULL, NULL, 0, is_off); return of_genpd_add_provider_onecell(dev->of_node, &imx_gpc_onecell_data); diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c index 75b14e7..0e79375 100644 --- a/arch/arm/mach-s3c64xx/pm.c +++ b/arch/arm/mach-s3c64xx/pm.c @@ -317,10 +317,11 @@ int __init s3c64xx_pm_init(void) for (i = 0; i < ARRAY_SIZE(s3c64xx_always_on_pm_domains); i++) pm_genpd_init(&s3c64xx_always_on_pm_domains[i]->pd, - &pm_domain_always_on_gov, false); + &pm_domain_always_on_gov, NULL, 0, false); for (i = 0; i < ARRAY_SIZE(s3c64xx_pm_domains); i++) - pm_genpd_init(&s3c64xx_pm_domains[i]->pd, NULL, false); + pm_genpd_init(&s3c64xx_pm_domains[i]->pd, + NULL, NULL, 0, false); #ifdef CONFIG_S3C_DEV_FB if (dev_get_platdata(&s3c_device_fb.dev)) diff --git a/arch/arm/mach-shmobile/pm-r8a7779.c b/arch/arm/mach-shmobile/pm-r8a7779.c index 44a74c4..a89a94f 100644 --- a/arch/arm/mach-shmobile/pm-r8a7779.c +++ b/arch/arm/mach-shmobile/pm-r8a7779.c @@ -84,7 +84,7 @@ static void r8a7779_init_pm_domain(struct r8a7779_pm_domain *r8a7779_pd) struct generic_pm_domain *genpd = &r8a7779_pd->genpd; genpd->flags = GENPD_FLAG_PM_CLK; - pm_genpd_init(genpd, NULL, false); + pm_genpd_init(genpd, NULL, NULL, 0, false); genpd->dev_ops.active_wakeup = pd_active_wakeup; genpd->power_off = pd_power_down; genpd->power_on = pd_power_up; diff --git a/arch/arm/mach-shmobile/pm-rmobile.c b/arch/arm/mach-shmobile/pm-rmobile.c index 9501820..18f87c5 100644 --- a/arch/arm/mach-shmobile/pm-rmobile.c +++ b/arch/arm/mach-shmobile/pm-rmobile.c @@ -154,7 +154,7 @@ static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd) struct dev_power_governor *gov = rmobile_pd->gov; genpd->flags = GENPD_FLAG_PM_CLK; - pm_genpd_init(genpd, gov ? : &simple_qos_governor, false); + pm_genpd_init(genpd, gov ? : &simple_qos_governor, NULL, 0, false); genpd->dev_ops.active_wakeup = rmobile_pd_active_wakeup; genpd->power_off = rmobile_pd_power_down; genpd->power_on = rmobile_pd_power_up; diff --git a/arch/arm/mach-ux500/pm_domains.c b/arch/arm/mach-ux500/pm_domains.c index 4d71c90..7bd4d98 100644 --- a/arch/arm/mach-ux500/pm_domains.c +++ b/arch/arm/mach-ux500/pm_domains.c @@ -72,7 +72,7 @@ int __init ux500_pm_domains_init(void) genpd_data->num_domains = ARRAY_SIZE(ux500_pm_domains); for (i = 0; i < ARRAY_SIZE(ux500_pm_domains); ++i) - pm_genpd_init(ux500_pm_domains[i], NULL, false); + pm_genpd_init(ux500_pm_domains[i], NULL, NULL, 0, false); of_genpd_add_provider_onecell(np, genpd_data); return 0; diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 2327613..b30a42f 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1882,10 +1882,14 @@ static int pm_genpd_default_restore_state(struct device *dev) * pm_genpd_init - Initialize a generic I/O PM domain object. * @genpd: PM domain object to initialize. * @gov: PM domain governor to associate with the domain (may be NULL). + * @states: Array of possible low power states when powering off. + * @state_count: Number of low power states. * @is_off: Initial value of the domain's power_is_off field. */ void pm_genpd_init(struct generic_pm_domain *genpd, - struct dev_power_governor *gov, bool is_off) + struct dev_power_governor *gov, + const struct genpd_power_state *states, + unsigned int state_count, bool is_off) { if (IS_ERR_OR_NULL(genpd)) return; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 681ccb0..6b4802e 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -46,6 +46,12 @@ struct gpd_cpuidle_data { struct cpuidle_state *idle_state; }; +struct genpd_power_state { + char *name; + s64 power_off_latency_ns; + s64 power_on_latency_ns; +}; + struct generic_pm_domain { struct dev_pm_domain domain; /* PM domain operations */ struct list_head gpd_list_node; /* Node in the global PM domains list */ @@ -149,7 +155,9 @@ extern int pm_genpd_name_attach_cpuidle(const char *name, int state); extern int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd); extern int pm_genpd_name_detach_cpuidle(const char *name); extern void pm_genpd_init(struct generic_pm_domain *genpd, - struct dev_power_governor *gov, bool is_off); + struct dev_power_governor *gov, + const struct genpd_power_state *states, + unsigned int state_count, bool is_off); extern int pm_genpd_poweron(struct generic_pm_domain *genpd); extern int pm_genpd_name_poweron(const char *domain_name); @@ -216,7 +224,9 @@ static inline int pm_genpd_name_detach_cpuidle(const char *name) return -ENOSYS; } static inline void pm_genpd_init(struct generic_pm_domain *genpd, - struct dev_power_governor *gov, bool is_off) + struct dev_power_governor *gov, + const struct genpd_power_state *states, + unsigned int state_count, bool is_off) { } static inline int pm_genpd_poweron(struct generic_pm_domain *genpd)