On 25 January 2017 at 12:53, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
[...]
> --- 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
--
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
