From: Ulf Hansson <ulf.hansson@linaro.org> To: Marek Szyprowski <m.szyprowski@samsung.com> Cc: linux-clk <linux-clk@vger.kernel.org>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, linux-samsung-soc <linux-samsung-soc@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Stephen Boyd <sboyd@codeaurora.org>, Michael Turquette <mturquette@baylibre.com>, Sylwester Nawrocki <s.nawrocki@samsung.com>, Chanwoo Choi <cw00.choi@samsung.com>, Inki Dae <inki.dae@samsung.com>, Krzysztof Kozlowski <krzk@kernel.org>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Subject: Re: [PATCH v5 3/4] clk: samsung: exynos5433: Add runtime PM support Date: Mon, 13 Mar 2017 14:31:32 +0100 [thread overview] Message-ID: <CAPDyKFotSo5qcJPt9w99Rf3i_kvQnBZE36iBbfDfuBkiKhsoTw@mail.gmail.com> (raw) In-Reply-To: <1485345194-3196-4-git-send-email-m.szyprowski@samsung.com> On 25 January 2017 at 12:53, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Add runtime pm support for all clock controller units (CMU), which belongs > to power domains and require special handling during on/off operations. > Typically special values has to be written to MUX registers to change > internal clocks parents to OSC clock before turning power off. During such > operation all clocks, which enters CMU has to be enabled to let MUX to > stabilize. Also for each CMU there is one special parent clock, which has > to be enabled all the time when any access to CMU registers is done. > > This patch solves most of the mysterious external abort and freeze issues > caused by a lack of proper parent CMU clock enabled or incorrect turn off > procedure. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- [...] > --- a/drivers/clk/samsung/clk-exynos5433.c > +++ b/drivers/clk/samsung/clk-exynos5433.c > @@ -9,9 +9,14 @@ > * Common Clock Framework support for Exynos5443 SoC. > */ > > +#include <linux/clk.h> > #include <linux/clk-provider.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > > #include <dt-bindings/clock/exynos5433.h> > [...] > +struct exynos5433_cmu_data { > + struct samsung_clk_provider ctx; > + > + struct samsung_clk_reg_dump *clk_save; > + unsigned int nr_clk_save; > + const struct samsung_clk_reg_dump *clk_suspend; > + unsigned int nr_clk_suspend; > + > + struct clk *clk; > + struct clk **pclks; > + int nr_pclks; > +}; > + > +static int exynos5433_cmu_suspend(struct device *dev) > +{ > + struct exynos5433_cmu_data *data = dev_get_drvdata(dev); > + int i; > + > + samsung_clk_save(data->ctx.reg_base, data->clk_save, > + data->nr_clk_save); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_enable(data->pclks[i]); > + > + samsung_clk_restore(data->ctx.reg_base, data->clk_suspend, > + data->nr_clk_suspend); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_disable(data->pclks[i]); > + > + clk_disable(data->clk); > + > + return 0; > +} > + > +static int exynos5433_cmu_resume(struct device *dev) > +{ > + struct exynos5433_cmu_data *data = dev_get_drvdata(dev); > + int i; > + > + clk_enable(data->clk); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_enable(data->pclks[i]); > + > + samsung_clk_restore(data->ctx.reg_base, data->clk_save, > + data->nr_clk_save); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_disable(data->pclks[i]); > + > + return 0; > +} > + > +static int __init exynos5433_cmu_probe(struct platform_device *pdev) > +{ > + const struct samsung_cmu_info *info; > + struct exynos5433_cmu_data *data; > + struct samsung_clk_provider *ctx; > + struct device *dev = &pdev->dev; > + struct resource *res; > + void __iomem *reg_base; > + struct clk **clk_table; > + int i; > + > + info = of_device_get_match_data(dev); > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + ctx = &data->ctx; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + reg_base = devm_ioremap_resource(dev, res); > + if (!reg_base) { > + dev_err(dev, "failed to map registers\n"); > + return -ENOMEM; > + } > + > + clk_table = devm_kcalloc(dev, info->nr_clk_ids, sizeof(struct clk *), > + GFP_KERNEL); > + if (!clk_table) > + return -ENOMEM; > + > + for (i = 0; i < info->nr_clk_ids; ++i) > + clk_table[i] = ERR_PTR(-ENOENT); > + > + ctx->clk_data.clks = clk_table; > + ctx->clk_data.clk_num = info->nr_clk_ids; > + ctx->reg_base = reg_base; > + ctx->dev = dev; > + spin_lock_init(&ctx->lock); > + > + data->clk_save = samsung_clk_alloc_reg_dump(info->clk_regs, > + info->nr_clk_regs); > + data->nr_clk_save = info->nr_clk_regs; > + data->clk_suspend = info->suspend_regs; > + data->nr_clk_suspend = info->nr_suspend_regs; > + data->nr_pclks = of_count_phandle_with_args(dev->of_node, "clocks", > + "#clock-cells"); > + if (data->nr_pclks > 0) { > + data->pclks = devm_kcalloc(dev, sizeof(struct clk *), > + data->nr_pclks, GFP_KERNEL); > + > + for (i = 0; i < data->nr_pclks; i++) { > + struct clk *clk = of_clk_get(dev->of_node, i); > + > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + data->pclks[i] = clk; > + } > + } > + > + /* > + * Prepare all parent clocks here to avoid potential deadlock caused > + * by global clock "prepare lock" grabbed by runtime pm callbacks > + * from pm workers. > + */ I don't understand why this potentially could cause a deadlock. Can you please elaborate. To me, I think you should be fine doing clk_prepare_enable() and clk_disable_unprepare() from exynos5433_cmu_suspend|resume(), but I may be missing something. > + for (i = 0; i < data->nr_pclks; i++) > + clk_prepare(data->pclks[i]); > + > + if (info->clk_name) > + data->clk = clk_get(dev, info->clk_name); > + clk_prepare_enable(data->clk); > + > + platform_set_drvdata(pdev, data); > + > + /* > + * Enable runtime pm here, so clock core with use runtime pm for all > + * registered clocks. Additionally call pm_runtime_get_sync to ensure > + * that clock core will not runtime suspend our clock controller in > + * the middle of clock registration (clock core might call runtime pm > + * on newly registered clocks, for example to recalculate clock rate). > + */ Instead of the rather confusing comment about the call to pm_runtime_get_sync(), let's instead state that we increase the runtime PM usage count during the clock registration, to avoid the clock core from runtime suspending the device. In other words, before calling pm_runtime_set_active(), add a call pm_runtime_get_noresume() and then remove the latter call to pm_runtime_get_sync(). > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > + > + if (info->pll_clks) > + samsung_clk_register_pll(ctx, info->pll_clks, info->nr_pll_clks, > + reg_base); > + if (info->mux_clks) > + samsung_clk_register_mux(ctx, info->mux_clks, > + info->nr_mux_clks); > + if (info->div_clks) > + samsung_clk_register_div(ctx, info->div_clks, > + info->nr_div_clks); > + if (info->gate_clks) > + samsung_clk_register_gate(ctx, info->gate_clks, > + info->nr_gate_clks); > + if (info->fixed_clks) > + samsung_clk_register_fixed_rate(ctx, info->fixed_clks, > + info->nr_fixed_clks); > + if (info->fixed_factor_clks) > + samsung_clk_register_fixed_factor(ctx, info->fixed_factor_clks, > + info->nr_fixed_factor_clks); > + > + samsung_clk_of_add_provider(dev->of_node, ctx); > + pm_runtime_put(dev); > + > + return 0; > +} > [...] Kind regards Uffe
WARNING: multiple messages have this Message-ID (diff)
From: ulf.hansson@linaro.org (Ulf Hansson) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 3/4] clk: samsung: exynos5433: Add runtime PM support Date: Mon, 13 Mar 2017 14:31:32 +0100 [thread overview] Message-ID: <CAPDyKFotSo5qcJPt9w99Rf3i_kvQnBZE36iBbfDfuBkiKhsoTw@mail.gmail.com> (raw) In-Reply-To: <1485345194-3196-4-git-send-email-m.szyprowski@samsung.com> On 25 January 2017 at 12:53, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Add runtime pm support for all clock controller units (CMU), which belongs > to power domains and require special handling during on/off operations. > Typically special values has to be written to MUX registers to change > internal clocks parents to OSC clock before turning power off. During such > operation all clocks, which enters CMU has to be enabled to let MUX to > stabilize. Also for each CMU there is one special parent clock, which has > to be enabled all the time when any access to CMU registers is done. > > This patch solves most of the mysterious external abort and freeze issues > caused by a lack of proper parent CMU clock enabled or incorrect turn off > procedure. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- [...] > --- a/drivers/clk/samsung/clk-exynos5433.c > +++ b/drivers/clk/samsung/clk-exynos5433.c > @@ -9,9 +9,14 @@ > * Common Clock Framework support for Exynos5443 SoC. > */ > > +#include <linux/clk.h> > #include <linux/clk-provider.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > > #include <dt-bindings/clock/exynos5433.h> > [...] > +struct exynos5433_cmu_data { > + struct samsung_clk_provider ctx; > + > + struct samsung_clk_reg_dump *clk_save; > + unsigned int nr_clk_save; > + const struct samsung_clk_reg_dump *clk_suspend; > + unsigned int nr_clk_suspend; > + > + struct clk *clk; > + struct clk **pclks; > + int nr_pclks; > +}; > + > +static int exynos5433_cmu_suspend(struct device *dev) > +{ > + struct exynos5433_cmu_data *data = dev_get_drvdata(dev); > + int i; > + > + samsung_clk_save(data->ctx.reg_base, data->clk_save, > + data->nr_clk_save); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_enable(data->pclks[i]); > + > + samsung_clk_restore(data->ctx.reg_base, data->clk_suspend, > + data->nr_clk_suspend); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_disable(data->pclks[i]); > + > + clk_disable(data->clk); > + > + return 0; > +} > + > +static int exynos5433_cmu_resume(struct device *dev) > +{ > + struct exynos5433_cmu_data *data = dev_get_drvdata(dev); > + int i; > + > + clk_enable(data->clk); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_enable(data->pclks[i]); > + > + samsung_clk_restore(data->ctx.reg_base, data->clk_save, > + data->nr_clk_save); > + > + for (i = 0; i < data->nr_pclks; i++) > + clk_disable(data->pclks[i]); > + > + return 0; > +} > + > +static int __init exynos5433_cmu_probe(struct platform_device *pdev) > +{ > + const struct samsung_cmu_info *info; > + struct exynos5433_cmu_data *data; > + struct samsung_clk_provider *ctx; > + struct device *dev = &pdev->dev; > + struct resource *res; > + void __iomem *reg_base; > + struct clk **clk_table; > + int i; > + > + info = of_device_get_match_data(dev); > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + ctx = &data->ctx; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + reg_base = devm_ioremap_resource(dev, res); > + if (!reg_base) { > + dev_err(dev, "failed to map registers\n"); > + return -ENOMEM; > + } > + > + clk_table = devm_kcalloc(dev, info->nr_clk_ids, sizeof(struct clk *), > + GFP_KERNEL); > + if (!clk_table) > + return -ENOMEM; > + > + for (i = 0; i < info->nr_clk_ids; ++i) > + clk_table[i] = ERR_PTR(-ENOENT); > + > + ctx->clk_data.clks = clk_table; > + ctx->clk_data.clk_num = info->nr_clk_ids; > + ctx->reg_base = reg_base; > + ctx->dev = dev; > + spin_lock_init(&ctx->lock); > + > + data->clk_save = samsung_clk_alloc_reg_dump(info->clk_regs, > + info->nr_clk_regs); > + data->nr_clk_save = info->nr_clk_regs; > + data->clk_suspend = info->suspend_regs; > + data->nr_clk_suspend = info->nr_suspend_regs; > + data->nr_pclks = of_count_phandle_with_args(dev->of_node, "clocks", > + "#clock-cells"); > + if (data->nr_pclks > 0) { > + data->pclks = devm_kcalloc(dev, sizeof(struct clk *), > + data->nr_pclks, GFP_KERNEL); > + > + for (i = 0; i < data->nr_pclks; i++) { > + struct clk *clk = of_clk_get(dev->of_node, i); > + > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + data->pclks[i] = clk; > + } > + } > + > + /* > + * Prepare all parent clocks here to avoid potential deadlock caused > + * by global clock "prepare lock" grabbed by runtime pm callbacks > + * from pm workers. > + */ I don't understand why this potentially could cause a deadlock. Can you please elaborate. To me, I think you should be fine doing clk_prepare_enable() and clk_disable_unprepare() from exynos5433_cmu_suspend|resume(), but I may be missing something. > + for (i = 0; i < data->nr_pclks; i++) > + clk_prepare(data->pclks[i]); > + > + if (info->clk_name) > + data->clk = clk_get(dev, info->clk_name); > + clk_prepare_enable(data->clk); > + > + platform_set_drvdata(pdev, data); > + > + /* > + * Enable runtime pm here, so clock core with use runtime pm for all > + * registered clocks. Additionally call pm_runtime_get_sync to ensure > + * that clock core will not runtime suspend our clock controller in > + * the middle of clock registration (clock core might call runtime pm > + * on newly registered clocks, for example to recalculate clock rate). > + */ Instead of the rather confusing comment about the call to pm_runtime_get_sync(), let's instead state that we increase the runtime PM usage count during the clock registration, to avoid the clock core from runtime suspending the device. In other words, before calling pm_runtime_set_active(), add a call pm_runtime_get_noresume() and then remove the latter call to pm_runtime_get_sync(). > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > + > + if (info->pll_clks) > + samsung_clk_register_pll(ctx, info->pll_clks, info->nr_pll_clks, > + reg_base); > + if (info->mux_clks) > + samsung_clk_register_mux(ctx, info->mux_clks, > + info->nr_mux_clks); > + if (info->div_clks) > + samsung_clk_register_div(ctx, info->div_clks, > + info->nr_div_clks); > + if (info->gate_clks) > + samsung_clk_register_gate(ctx, info->gate_clks, > + info->nr_gate_clks); > + if (info->fixed_clks) > + samsung_clk_register_fixed_rate(ctx, info->fixed_clks, > + info->nr_fixed_clks); > + if (info->fixed_factor_clks) > + samsung_clk_register_fixed_factor(ctx, info->fixed_factor_clks, > + info->nr_fixed_factor_clks); > + > + samsung_clk_of_add_provider(dev->of_node, ctx); > + pm_runtime_put(dev); > + > + return 0; > +} > [...] Kind regards Uffe
next prev parent reply other threads:[~2017-03-13 13:31 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <CGME20170125115327eucas1p172866fa87417f59f1dea60185d6d8b60@eucas1p1.samsung.com> 2017-01-25 11:53 ` [PATCH v5 0/4] Add runtime PM support for clocks (on Exynos SoC example) Marek Szyprowski 2017-01-25 11:53 ` Marek Szyprowski [not found] ` <CGME20170125115328eucas1p1d97fac29286805b58039f8aec2b9bfe6@eucas1p1.samsung.com> 2017-01-25 11:53 ` [PATCH v5 1/4] clk: Add support for runtime PM Marek Szyprowski 2017-01-25 11:53 ` Marek Szyprowski 2017-03-13 12:20 ` Ulf Hansson 2017-03-13 12:20 ` Ulf Hansson [not found] ` <CGME20170125115329eucas1p1fc59d9be3421bc763d16aa331375b9f6@eucas1p1.samsung.com> 2017-01-25 11:53 ` [PATCH v5 2/4] clk: samsung: " Marek Szyprowski 2017-01-25 11:53 ` Marek Szyprowski 2017-03-13 13:34 ` Ulf Hansson 2017-03-13 13:34 ` Ulf Hansson [not found] ` <CGME20170125115329eucas1p1f42bbb54d5973b6192e964dc8b3a8081@eucas1p1.samsung.com> 2017-01-25 11:53 ` [PATCH v5 3/4] clk: samsung: exynos5433: Add runtime PM support Marek Szyprowski 2017-01-25 11:53 ` Marek Szyprowski 2017-03-13 13:31 ` Ulf Hansson [this message] 2017-03-13 13:31 ` Ulf Hansson 2017-03-22 8:26 ` Marek Szyprowski 2017-03-22 8:26 ` Marek Szyprowski 2017-03-22 8:26 ` Marek Szyprowski [not found] ` <CGME20170125115330eucas1p16f9de097e0966bc9d9901bc27caa085c@eucas1p1.samsung.com> 2017-01-25 11:53 ` [PATCH v5 4/4] clk: samsung: exynos-audss: Use runtime PM Marek Szyprowski 2017-01-25 11:53 ` Marek Szyprowski 2017-01-26 1:28 ` kbuild test robot 2017-01-26 1:28 ` kbuild test robot 2017-01-26 1:28 ` kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAPDyKFotSo5qcJPt9w99Rf3i_kvQnBZE36iBbfDfuBkiKhsoTw@mail.gmail.com \ --to=ulf.hansson@linaro.org \ --cc=b.zolnierkie@samsung.com \ --cc=cw00.choi@samsung.com \ --cc=inki.dae@samsung.com \ --cc=krzk@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=mturquette@baylibre.com \ --cc=s.nawrocki@samsung.com \ --cc=sboyd@codeaurora.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.