Re: [PATCH v2 6/6] clk: samsung: exynos5433: Extract PM support to common ARM64 layer

[Sam Protsenko]

On Wed, 8 Feb 2023 at 17:43, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
>
> Exynos5433 clock driver implements PM support internally, which might be
> also useful for other Exynos clock drivers. Extract all PM related code
> from clk-exynos5433 to common ARM64 functions.
>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> ---
> Changes in v2:
>   - Rebased on top of latest soc/for-next tree
>   - Included linux/slab.h for kfree (found by kernel test robot)
>
>  drivers/clk/samsung/clk-exynos-arm64.c | 171 ++++++++++++++++++++++++-
>  drivers/clk/samsung/clk-exynos-arm64.h |   3 +
>  drivers/clk/samsung/clk-exynos5433.c   | 157 +----------------------
>  3 files changed, 175 insertions(+), 156 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c
> index 2aa3f0a5644e..7ad7fd353dda 100644
> --- a/drivers/clk/samsung/clk-exynos-arm64.c
> +++ b/drivers/clk/samsung/clk-exynos-arm64.c
> @@ -10,6 +10,9 @@
>   */

Hi Marek,

It just occurred to me that as I'm pulling your code from
clk-exynos5433.c here, I should've probably added you to this file's
author list in the header comment. Does that sound right to you? If
so, I can re-send v3 with fixes.

Also, could you please review this series, if possible? I'm working
right now on PM_DOMAINS support for Exynos850, so along with this
series that would bring the initial PM support for ARM64 Exynos chips.

Thanks!

>  #include <linux/clk.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
>
>  #include "clk-exynos-arm64.h"
>
> @@ -21,6 +24,19 @@
>  #define GATE_OFF_START         0x2000
>  #define GATE_OFF_END           0x2fff
>
> +struct exynos_arm64_cmu_data {
> +       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;
> +
> +       struct samsung_clk_provider *ctx;
> +};
> +
>  /**
>   * exynos_arm64_init_clocks - Set clocks initial configuration
>   * @np:                        CMU device tree node with "reg" property (CMU addr)
> @@ -76,10 +92,16 @@ static int __init exynos_arm64_enable_bus_clk(struct device *dev,
>         if (!cmu->clk_name)
>                 return 0;
>
> -       if (dev)
> +       if (dev) {
> +               struct exynos_arm64_cmu_data *data;
> +
>                 parent_clk = clk_get(dev, cmu->clk_name);
> -       else
> +               data = dev_get_drvdata(dev);
> +               if (data)
> +                       data->clk = parent_clk;
> +       } else {
>                 parent_clk = of_clk_get_by_name(np, cmu->clk_name);
> +       }
>
>         if (IS_ERR(parent_clk))
>                 return PTR_ERR(parent_clk);
> @@ -87,6 +109,46 @@ static int __init exynos_arm64_enable_bus_clk(struct device *dev,
>         return clk_prepare_enable(parent_clk);
>  }
>
> +static int __init exynos_arm64_cmu_prepare_pm(struct device *dev,
> +               const struct samsung_cmu_info *cmu)
> +{
> +       struct exynos_arm64_cmu_data *data = dev_get_drvdata(dev);
> +       int i;
> +
> +       data->clk_save = samsung_clk_alloc_reg_dump(cmu->clk_regs,
> +                                                   cmu->nr_clk_regs);
> +       if (!data->clk_save)
> +               return -ENOMEM;
> +
> +       data->nr_clk_save = cmu->nr_clk_regs;
> +       data->clk_suspend = cmu->suspend_regs;
> +       data->nr_clk_suspend = cmu->nr_suspend_regs;
> +       data->nr_pclks = of_clk_get_parent_count(dev->of_node);
> +       if (!data->nr_pclks)
> +               return 0;
> +
> +       data->pclks = devm_kcalloc(dev, sizeof(struct clk *), data->nr_pclks,
> +                                  GFP_KERNEL);
> +       if (!data->pclks) {
> +               kfree(data->clk_save);
> +               return -ENOMEM;
> +       }
> +
> +       for (i = 0; i < data->nr_pclks; i++) {
> +               struct clk *clk = of_clk_get(dev->of_node, i);
> +
> +               if (IS_ERR(clk)) {
> +                       kfree(data->clk_save);
> +                       while (--i >= 0)
> +                               clk_put(data->pclks[i]);
> +                       return PTR_ERR(clk);
> +               }
> +               data->pclks[i] = clk;
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * exynos_arm64_register_cmu - Register specified Exynos CMU domain
>   * @dev:       Device object; may be NULL if this function is not being
> @@ -111,3 +173,108 @@ void __init exynos_arm64_register_cmu(struct device *dev,
>         exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
>         samsung_cmu_register_one(np, cmu);
>  }
> +
> +/**
> + * exynos_arm64_register_cmu_pm - Register Exynos CMU domain with PM support
> + *
> + * @pdev:      Platform device object
> + * @set_manual:        If true, set gate clocks to manual mode
> + *
> + * It's a version of exynos_arm64_register_cmu() with PM support. Should be
> + * called from probe function of platform driver.
> + *
> + * Return: 0 on success, or negative error code on error.
> + */
> +int __init exynos_arm64_register_cmu_pm(struct platform_device *pdev,
> +                                       bool set_manual)
> +{
> +       const struct samsung_cmu_info *cmu;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct exynos_arm64_cmu_data *data;
> +       void __iomem *reg_base;
> +       int ret;
> +
> +       cmu = of_device_get_match_data(dev);
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, data);
> +
> +       ret = exynos_arm64_cmu_prepare_pm(dev, cmu);
> +       if (ret)
> +               return ret;
> +
> +       ret = exynos_arm64_enable_bus_clk(dev, NULL, cmu);
> +       if (ret)
> +               return ret;
> +
> +       if (set_manual)
> +               exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
> +
> +       reg_base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(reg_base))
> +               return PTR_ERR(reg_base);
> +
> +       data->ctx = samsung_clk_init(dev, reg_base, cmu->nr_clk_ids);
> +
> +       /*
> +        * Enable runtime PM here to allow the clock core using runtime PM
> +        * for the registered clocks. Additionally, we increase the runtime
> +        * PM usage count before registering the clocks, to prevent the
> +        * clock core from runtime suspending the device.
> +        */
> +       pm_runtime_get_noresume(dev);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
> +
> +       samsung_cmu_register_clocks(data->ctx, cmu);
> +       samsung_clk_of_add_provider(dev->of_node, data->ctx);
> +       pm_runtime_put_sync(dev);
> +
> +       return 0;
> +}
> +
> +int exynos_arm64_cmu_suspend(struct device *dev)
> +{
> +       struct exynos_arm64_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_prepare_enable(data->pclks[i]);
> +
> +       /* For suspend some registers have to be set to certain values */
> +       samsung_clk_restore(data->ctx->reg_base, data->clk_suspend,
> +                           data->nr_clk_suspend);
> +
> +       for (i = 0; i < data->nr_pclks; i++)
> +               clk_disable_unprepare(data->pclks[i]);
> +
> +       clk_disable_unprepare(data->clk);
> +
> +       return 0;
> +}
> +
> +int exynos_arm64_cmu_resume(struct device *dev)
> +{
> +       struct exynos_arm64_cmu_data *data = dev_get_drvdata(dev);
> +       int i;
> +
> +       clk_prepare_enable(data->clk);
> +
> +       for (i = 0; i < data->nr_pclks; i++)
> +               clk_prepare_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_unprepare(data->pclks[i]);
> +
> +       return 0;
> +}
> diff --git a/drivers/clk/samsung/clk-exynos-arm64.h b/drivers/clk/samsung/clk-exynos-arm64.h
> index 0dd174693935..969979e714bc 100644
> --- a/drivers/clk/samsung/clk-exynos-arm64.h
> +++ b/drivers/clk/samsung/clk-exynos-arm64.h
> @@ -16,5 +16,8 @@
>
>  void exynos_arm64_register_cmu(struct device *dev,
>                 struct device_node *np, const struct samsung_cmu_info *cmu);
> +int exynos_arm64_register_cmu_pm(struct platform_device *pdev, bool set_manual);
> +int exynos_arm64_cmu_suspend(struct device *dev);
> +int exynos_arm64_cmu_resume(struct device *dev);
>
>  #endif /* __CLK_EXYNOS_ARM64_H */
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index eb72bf2aaee8..ed43233649ae 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -10,7 +10,6 @@
>  #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>
> @@ -19,6 +18,7 @@
>
>  #include "clk.h"
>  #include "clk-cpu.h"
> +#include "clk-exynos-arm64.h"
>  #include "clk-pll.h"
>
>  /*
> @@ -5478,160 +5478,9 @@ static const struct samsung_cmu_info imem_cmu_info __initconst = {
>         .clk_name               = "aclk_imem_200",
>  };
>
> -struct exynos5433_cmu_data {
> -       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;
> -
> -       /* must be the last entry */
> -       struct samsung_clk_provider ctx;
> -};
> -
> -static int __maybe_unused 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_prepare_enable(data->pclks[i]);
> -
> -       /* for suspend some registers have to be set to certain values */
> -       samsung_clk_restore(data->ctx.reg_base, data->clk_suspend,
> -                           data->nr_clk_suspend);
> -
> -       for (i = 0; i < data->nr_pclks; i++)
> -               clk_disable_unprepare(data->pclks[i]);
> -
> -       clk_disable_unprepare(data->clk);
> -
> -       return 0;
> -}
> -
> -static int __maybe_unused exynos5433_cmu_resume(struct device *dev)
> -{
> -       struct exynos5433_cmu_data *data = dev_get_drvdata(dev);
> -       int i;
> -
> -       clk_prepare_enable(data->clk);
> -
> -       for (i = 0; i < data->nr_pclks; i++)
> -               clk_prepare_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_unprepare(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;
> -       void __iomem *reg_base;
> -       int i;
> -
> -       info = of_device_get_match_data(dev);
> -
> -       data = devm_kzalloc(dev,
> -                           struct_size(data, ctx.clk_data.hws, info->nr_clk_ids),
> -                           GFP_KERNEL);
> -       if (!data)
> -               return -ENOMEM;
> -       ctx = &data->ctx;
> -
> -       reg_base = devm_platform_ioremap_resource(pdev, 0);
> -       if (IS_ERR(reg_base))
> -               return PTR_ERR(reg_base);
> -
> -       for (i = 0; i < info->nr_clk_ids; ++i)
> -               ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
> -
> -       ctx->clk_data.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);
> -       if (!data->clk_save)
> -               return -ENOMEM;
> -       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_clk_get_parent_count(dev->of_node);
> -
> -       if (data->nr_pclks > 0) {
> -               data->pclks = devm_kcalloc(dev, sizeof(struct clk *),
> -                                          data->nr_pclks, GFP_KERNEL);
> -               if (!data->pclks) {
> -                       kfree(data->clk_save);
> -                       return -ENOMEM;
> -               }
> -               for (i = 0; i < data->nr_pclks; i++) {
> -                       struct clk *clk = of_clk_get(dev->of_node, i);
> -
> -                       if (IS_ERR(clk)) {
> -                               kfree(data->clk_save);
> -                               while (--i >= 0)
> -                                       clk_put(data->pclks[i]);
> -                               return PTR_ERR(clk);
> -                       }
> -                       data->pclks[i] = clk;
> -               }
> -       }
> -
> -       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 to allow the clock core using runtime PM
> -        * for the registered clocks. Additionally, we increase the runtime
> -        * PM usage count before registering the clocks, to prevent the
> -        * clock core from runtime suspending the device.
> -        */
> -       pm_runtime_get_noresume(dev);
> -       pm_runtime_set_active(dev);
> -       pm_runtime_enable(dev);
> -
> -       if (info->pll_clks)
> -               samsung_clk_register_pll(ctx, info->pll_clks,
> -                                        info->nr_pll_clks);
> -       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_sync(dev);
> -
> -       return 0;
> +       return exynos_arm64_register_cmu_pm(pdev, false);
>  }
>
>  static const struct of_device_id exynos5433_cmu_of_match[] = {
> @@ -5679,7 +5528,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {
>  };
>
>  static const struct dev_pm_ops exynos5433_cmu_pm_ops = {
> -       SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,
> +       SET_RUNTIME_PM_OPS(exynos_arm64_cmu_suspend, exynos_arm64_cmu_resume,
>                            NULL)
>         SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>                                      pm_runtime_force_resume)
> --
> 2.39.1
>