[PATCH 3/3] ARM: exynos5: Add clock save and restore
Russell King - ARM Linux
linux at arm.linux.org.uk
Wed Jan 9 08:53:44 EST 2013
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@vger.kernel.org/msg14347.html.
>
> Signed-off-by: Prasanna Kumar <prasanna.ps at samsung.com>
> ---
> 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list