On Wed, Jan 09, 2013 at 06:24:41PM +0530, Prasanna Kumar wrote: > After Suspend-Resume operation, it is observed that CLK_TOP_SRC3 register > gets modified if the G-Scaler/MFC devices are power gated. > > The clock for G-Scaler gets set to XXTI which results in the device > running very slow.This issue also seen for MFC. > > To solve above issue, the existing clock framework of exynos5 is used > to save and restore clocks while power gating instead of accessing > CLK_SRC_TOP3 register directly.The clock names are read from DT file. > > Please refer below URL to know the background of this issue. > http://www.mail-archive.com/linux-samsung-soc@xxxxxxxxxxxxxxx/msg14347.html. > > Signed-off-by: Prasanna Kumar <prasanna.ps@xxxxxxxxxxx> > --- > arch/arm/mach-exynos/pm_domains.c | 125 +++++++++++++++++++++++++++++++++++++ > 1 files changed, 125 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c > index 9f1351d..2f49de9 100644 > --- a/arch/arm/mach-exynos/pm_domains.c > +++ b/arch/arm/mach-exynos/pm_domains.c > @@ -21,9 +21,14 @@ > #include <linux/of_address.h> > #include <linux/of_platform.h> > #include <linux/sched.h> > +#include <linux/clk.h> > +#include <linux/list.h> > > #include <mach/regs-pmu.h> > #include <plat/devs.h> > +#include <plat/clock.h> > +#include <plat/clock-clksrc.h> > + > > /* > * Exynos specific wrapper around the generic power domain > @@ -33,7 +38,69 @@ struct exynos_pm_domain { > char const *name; > bool is_off; > struct generic_pm_domain pd; > + struct list_head list_pdclks; > + struct list_head saved_list_pdclks; > + int pd_clks; > +}; > + > +struct exynos_pd_clk { > + struct list_head node; > + struct clk *clk; > }; Missing blank line, but that's the least of the problems... > +static int exynos_pdclk_save(struct exynos_pm_domain *epd) > +{ > + struct exynos_pd_clk *pdclk; > + struct exynos_pd_clk *saved_pdclk; > + > + list_for_each_entry(pdclk, &epd->list_pdclks, node) { > + if (pdclk) { > + saved_pdclk = kzalloc(sizeof(struct exynos_pd_clk), > + GFP_KERNEL); > + if (!saved_pdclk) { > + pr_err("%s: failed to allocate memory\n", > + __func__); > + return -ENOMEM; > + } > + saved_pdclk->clk = clk_get_parent(pdclk->clk); > + if (IS_ERR(saved_pdclk->clk)) { Congratulations on using the correct macro to check for failure. > + pr_err(" Cannot get parent for %s\n", You don't need a space character before "Cannot", and it should be "Can not". > + pdclk->clk->name); > + return PTR_ERR(saved_pdclk->clk); Memory leak. > + } > + list_add_tail(&saved_pdclk->node, > + &epd->saved_list_pdclks); > + } > + } > + return 0; > +} > + > +static void exynos_pdclk_restore(struct exynos_pm_domain *epd) > +{ > + struct exynos_pd_clk *pdclk; > + struct exynos_pd_clk *saved_pdclk; > + struct list_head *p_clk; > + struct list_head *p_saved_clk; > + int ret; > + > + p_saved_clk = epd->saved_list_pdclks.next; > + p_clk = epd->list_pdclks.next; > + > + for ( ; p_saved_clk != &epd->saved_list_pdclks && > + p_clk != &epd->list_pdclks; > + p_clk = p_clk->next, p_saved_clk = p_saved_clk->next) { > + > + saved_pdclk = list_entry(p_saved_clk, > + struct exynos_pd_clk, node); > + pdclk = list_entry(p_clk, struct exynos_pd_clk, node); > + if (saved_pdclk && pdclk) { > + ret = clk_set_parent(pdclk->clk, saved_pdclk->clk); > + if (ret) > + pr_err("Failed to set %s as parent of %s\n", > + saved_pdclk->clk->name, pdclk->clk->name); > + } > + } So here, you're walking to lists in unison. What if exynos_pdclk_save() failed to build the full list of saved clocks? I suspect things explode. Moreover, wouldn't it be more efficient to record the old parent in the main list - it's only another 8 bytes, and it's not like you're cleaning up and freeing the saved list, so that will save some memory (probably number_of_clocks * L1 cache line size). > + return; > +} > > static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on) > { > @@ -45,6 +112,13 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on) > pd = container_of(domain, struct exynos_pm_domain, pd); > base = pd->base; > > + if (!power_on) { > + if (pd->pd_clks > 0) > + if (exynos_pdclk_save(pd)) This can be simplified: if (!power_on && pd->pd_clks > 0 && exynos_pdclk_save(pd)) > + pr_err("Failed to save pdclks for %s\n", > + domain->name); Hmm, exynos_pdclk_save() prints its own diagnostics on error, is there really a need for two error printks? If not, then wouldn't: if (!power_on && pd->pd_clks > 0) exynos_pdclk_save(pd); be a more readable way to code this? > + } > + > pwr = power_on ? S5P_INT_LOCAL_PWR_EN : 0; > __raw_writel(pwr, base); > > @@ -61,6 +135,11 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on) > cpu_relax(); > usleep_range(80, 100); > } > + > + if (!power_on) { > + if (pd->pd_clks > 0) > + exynos_pdclk_restore(pd); > + } Same comments here. > return 0; > } > > @@ -157,10 +236,48 @@ static struct notifier_block platform_nb = { > .notifier_call = exynos_pm_notifier_call, > }; > > +static int exynos_read_pdclk_from_dt(struct device_node *dev_node, > + struct exynos_pm_domain *pd) > +{ > + int i = 0; > + const char *clk_name; > + struct exynos_pd_clk *pd_clk; > + int err = 0; > + > + INIT_LIST_HEAD(&pd->list_pdclks); > + INIT_LIST_HEAD(&pd->saved_list_pdclks); > + > + for (i = 0; i < pd->pd_clks; i++) { > + pd_clk = kzalloc(sizeof(struct exynos_pd_clk), > + GFP_KERNEL); > + if (!pd_clk) { > + pr_err("%s: failed to allocate memory\n", > + __func__); > + return -ENOMEM; > + } > + err = of_property_read_string_index(dev_node, > + "samsung,exynos-pd-clks", > + i, > + &clk_name); > + if (err) { > + pr_err("failed to read pd_clks\n"); pd_clk is leaked. > + return err; > + } > + pd_clk->clk = clk_get(NULL, clk_name); > + if (IS_ERR(pd_clk->clk)) { > + pr_err("clk_get failed for %s\n", clk_name); od_clk is leaked. Don't we have an interface to get a clk from DT? > + return PTR_ERR(pd_clk->clk); > + } > + list_add(&pd_clk->node, &pd->list_pdclks); > + } > + return 0; > +} > + > static __init int exynos_pm_dt_parse_domains(void) > { > struct platform_device *pdev; > struct device_node *np; > + int err = 0; > > for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") { > struct exynos_pm_domain *pd; > @@ -182,6 +299,14 @@ static __init int exynos_pm_dt_parse_domains(void) > pd->pd.power_on = exynos_pd_power_on; > pd->pd.of_node = np; > > + pd->pd_clks = of_property_count_strings(np, > + "samsung,exynos-pd-clks"); > + if (pd->pd_clks > 0) { > + err = exynos_read_pdclk_from_dt(np, pd); > + if (err) > + return err; > + } > + > platform_set_drvdata(pdev, pd); > > on = __raw_readl(pd->base + 0x4) & S5P_INT_LOCAL_PWR_EN; > -- > 1.7.5.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |