Date: Sun, 25 Feb 2024 20:50:29 +0800
From: Yangfl <mmyangfl@...il.com>
To: Yang Xiwen <forbidden405@...look.com>
Cc: linux-clk@...r.kernel.org, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 01/13] clk: hisilicon: Add helper functions for
 platform driver

Yang Xiwen <forbidden405@...look.com> 于2024年2月25日周日 19:56写道：
>
> On 2/25/2024 2:52 PM, David Yang wrote:
> > Helper functions extract common operations on platform drivers.
> >
> > During migration to devm APIs, (virtual) fixed clocks were found hard on
> > devm APIs, since they often depended by crucial peripherals, thus require
> > early initialization before device probing, and cannot use devm APIs.
>
>
> We have core_initcall() in drivers so CRGs are probed very early. This
> shouldn't be a problem.
>

This is also handled within hisi_clk_early_init(), and tested against
real devices.

>
> >
> > One solution to this problem is to add a "fixed-clock" node to device tree,
> > independent to clock device, and make those peripherals depend on that.
> > However, there is also some devices that do use fixed clocks provided by
> > drivers, for example clk-hi3660.c .
> >
> > To simplify codes, we migrate clocks of other types to devm APIs, while
> > keep fixed clocks self-managed, alongside with struct hisi_clock_data, and
> > remove devm-managed hisi_clock_data.
>
>
> Do we really want? How about leave old SoCs alone and just introduce a
> new set of APIs for new SoCs?
>
>
> Just like CCF, devm_ functions are simply wrappers of old APIs with the
> help of devres, the old APIs are still available.
>
>
> So for HiSilicon, I think we can take a similar approach, i.e., add a
> new set of wrapper functions with the help of devres rather than
> modifying old code.
>
>
> The implementation of officially provided devm_ APIs can be a good example.
>

This is somewhat requested by the maintainer, see replies in v5 of the
previous series
https://lore.kernel.org/all/d3b057408117a71bcd153f4a91bcdfe1.sboyd@kernel.org/

While this series migrates into devm_, it mainly serves as a cleanup.
Existing drivers share most of registration flows, while lack of
proper maintenance over times. This series improves overall code
quality, and reduce redundancy code by introducing newly-designed
common functions.

We might as well keep old API, but this series has no intention on how
new device will be introduced.

>
> >
> > `hisi_clk_alloc` will be removed in the following patch.
> >
> > Signed-off-by: David Yang <mmyangfl@...il.com>
> > ---
> >   drivers/clk/hisilicon/clk.c   | 157 ++++++++++++++++++++++++++++++++++
> >   drivers/clk/hisilicon/clk.h   |  46 +++++++++-
> >   drivers/clk/hisilicon/crg.h   |   5 ++
> >   drivers/clk/hisilicon/reset.c |  42 +++++++++
> >   4 files changed, 248 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
> > index 09368fd32bef..e50115f8e236 100644
> > --- a/drivers/clk/hisilicon/clk.c
> > +++ b/drivers/clk/hisilicon/clk.c
> > @@ -88,6 +88,25 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
> >   }
> >   EXPORT_SYMBOL_GPL(hisi_clk_init);
> >
> > +void hisi_clk_free(struct device_node *np, struct hisi_clock_data *data)
> > +{
> > +     if (data->clks) {
> > +             if (data->clks->fixed_rate_clks_num)
> > +                     hisi_clk_unregister_fixed_rate(data->clks->fixed_rate_clks,
> > +                                                    data->clks->fixed_rate_clks_num,
> > +                                                    data);
> > +             if (data->clks->fixed_factor_clks_num)
> > +                     hisi_clk_unregister_fixed_factor(data->clks->fixed_factor_clks,
> > +                                                      data->clks->fixed_factor_clks_num,
> > +                                                      data);
> > +     }
> > +
> > +     of_clk_del_provider(np);
> > +     kfree(data->clk_data.clks);
> > +     kfree(data);
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_clk_free);
> > +
> >   int hisi_clk_register_fixed_rate(const struct hisi_fixed_rate_clock *clks,
> >                                        int nums, struct hisi_clock_data *data)
> >   {
> > @@ -341,3 +360,141 @@ void __init hi6220_clk_register_divider(const struct hi6220_divider_clock *clks,
> >               data->clk_data.clks[clks[i].id] = clk;
> >       }
> >   }
> > +
> > +static size_t hisi_clocks_get_nr(const struct hisi_clocks *clks)
> > +{
> > +     if (clks->nr)
> > +             return clks->nr;
> > +
> > +     return clks->fixed_rate_clks_num + clks->fixed_factor_clks_num +
> > +             clks->mux_clks_num + clks->phase_clks_num +
> > +             clks->divider_clks_num + clks->gate_clks_num +
> > +             clks->gate_sep_clks_num + clks->customized_clks_num;
> > +}
> > +
> > +int hisi_clk_early_init(struct device_node *np, const struct hisi_clocks *clks)
> > +{
> > +     struct hisi_clock_data *data;
> > +     int ret;
> > +
> > +     data = hisi_clk_init(np, hisi_clocks_get_nr(clks));
> > +     if (!data)
> > +             return -ENOMEM;
> > +     data->clks = clks;
> > +
> > +     ret = hisi_clk_register_fixed_rate(clks->fixed_rate_clks,
> > +                                        clks->fixed_rate_clks_num, data);
> > +     if (ret)
> > +             goto err;
> > +
> > +     ret = hisi_clk_register_fixed_factor(clks->fixed_factor_clks,
> > +                                          clks->fixed_factor_clks_num, data);
> > +     if (ret)
> > +             goto err;
> > +
> > +     np->data = data;
> > +     return 0;
> > +
> > +err:
> > +     hisi_clk_free(np, data);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_clk_early_init);
> > +
> > +static int hisi_clk_register(struct device *dev, const struct hisi_clocks *clks,
> > +                          struct hisi_clock_data *data)
> > +{
> > +     int ret;
> > +
> > +     if (clks->mux_clks_num) {
> > +             ret = hisi_clk_register_mux(clks->mux_clks,
> > +                                         clks->mux_clks_num, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (clks->phase_clks_num) {
> > +             ret = hisi_clk_register_phase(dev, clks->phase_clks,
> > +                                           clks->phase_clks_num, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (clks->divider_clks_num) {
> > +             ret = hisi_clk_register_divider(clks->divider_clks,
> > +                                             clks->divider_clks_num, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (clks->gate_clks_num) {
> > +             ret = hisi_clk_register_gate(clks->gate_clks,
> > +                                          clks->gate_clks_num, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (clks->gate_sep_clks_num) {
> > +             hisi_clk_register_gate_sep(clks->gate_sep_clks,
> > +                                        clks->gate_sep_clks_num, data);
> > +     }
> > +
> > +     if (clks->clk_register_customized && clks->customized_clks_num) {
> > +             ret = clks->clk_register_customized(dev, clks->customized_clks,
> > +                                                 clks->customized_clks_num, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int hisi_clk_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     const struct hisi_clocks *clks;
> > +     struct hisi_clock_data *data;
> > +     int ret;
> > +
> > +     clks = of_device_get_match_data(dev);
> > +     if (!clks)
> > +             return -ENOENT;
> > +
> > +     if (!np->data) {
> > +             ret = hisi_clk_early_init(np, clks);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     data = np->data;
> > +     np->data = NULL;
> > +
> > +     if (clks->prologue) {
> > +             ret = clks->prologue(dev, data);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     ret = hisi_clk_register(dev, clks, data);
> > +     if (ret)
> > +             goto err;
> > +
> > +     platform_set_drvdata(pdev, data);
> > +     return 0;
> > +
> > +err:
> > +     hisi_clk_free(np, data);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_clk_probe);
> > +
> > +void hisi_clk_remove(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     struct hisi_clock_data *data = platform_get_drvdata(pdev);
> > +
> > +     hisi_clk_free(np, data);
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_clk_remove);
> > diff --git a/drivers/clk/hisilicon/clk.h b/drivers/clk/hisilicon/clk.h
> > index 7a9b42e1b027..87b17e9b79a3 100644
> > --- a/drivers/clk/hisilicon/clk.h
> > +++ b/drivers/clk/hisilicon/clk.h
> > @@ -17,10 +17,12 @@
> >   #include <linux/spinlock.h>
> >
> >   struct platform_device;
> > +struct hisi_clocks;
> >
> >   struct hisi_clock_data {
> > -     struct clk_onecell_data clk_data;
> > -     void __iomem            *base;
> > +     struct clk_onecell_data         clk_data;
> > +     void __iomem                    *base;
> > +     const struct hisi_clocks        *clks;
> >   };
> >
> >   struct hisi_fixed_rate_clock {
> > @@ -103,6 +105,39 @@ struct hisi_gate_clock {
> >       const char              *alias;
> >   };
> >
> > +struct hisi_clocks {
> > +     /* if 0, sum all *_num */
> > +     size_t nr;
> > +
> > +     int (*prologue)(struct device *dev, struct hisi_clock_data *data);
> > +
> > +     const struct hisi_fixed_rate_clock *fixed_rate_clks;
> > +     size_t fixed_rate_clks_num;
> > +
> > +     const struct hisi_fixed_factor_clock *fixed_factor_clks;
> > +     size_t fixed_factor_clks_num;
> > +
> > +     const struct hisi_mux_clock *mux_clks;
> > +     size_t mux_clks_num;
> > +
> > +     const struct hisi_phase_clock *phase_clks;
> > +     size_t phase_clks_num;
> > +
> > +     const struct hisi_divider_clock *divider_clks;
> > +     size_t divider_clks_num;
> > +
> > +     const struct hisi_gate_clock *gate_clks;
> > +     size_t gate_clks_num;
> > +
> > +     const struct hisi_gate_clock *gate_sep_clks;
> > +     size_t gate_sep_clks_num;
> > +
> > +     const void *customized_clks;
> > +     size_t customized_clks_num;
> > +     int (*clk_register_customized)(struct device *dev, const void *clks,
> > +                                    size_t num, struct hisi_clock_data *data);
> > +};
> > +
> >   struct clk *hisi_register_clkgate_sep(struct device *, const char *,
> >                               const char *, unsigned long,
> >                               void __iomem *, u8,
> > @@ -113,6 +148,7 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
> >
> >   struct hisi_clock_data *hisi_clk_alloc(struct platform_device *, int);
> >   struct hisi_clock_data *hisi_clk_init(struct device_node *, int);
> > +void hisi_clk_free(struct device_node *np, struct hisi_clock_data *data);
> >   int hisi_clk_register_fixed_rate(const struct hisi_fixed_rate_clock *,
> >                               int, struct hisi_clock_data *);
> >   int hisi_clk_register_fixed_factor(const struct hisi_fixed_factor_clock *,
> > @@ -154,4 +190,10 @@ hisi_clk_unregister(mux)
> >   hisi_clk_unregister(divider)
> >   hisi_clk_unregister(gate)
> >
> > +/* helper functions for platform driver */
> > +
> > +int hisi_clk_early_init(struct device_node *np, const struct hisi_clocks *clks);
> > +int hisi_clk_probe(struct platform_device *pdev);
> > +void hisi_clk_remove(struct platform_device *pdev);
> > +
> >   #endif      /* __HISI_CLK_H */
> > diff --git a/drivers/clk/hisilicon/crg.h b/drivers/clk/hisilicon/crg.h
> > index 803f6ba6d7a2..bd8e76b1f6d7 100644
> > --- a/drivers/clk/hisilicon/crg.h
> > +++ b/drivers/clk/hisilicon/crg.h
> > @@ -22,4 +22,9 @@ struct hisi_crg_dev {
> >       const struct hisi_crg_funcs *funcs;
> >   };
> >
> > +/* helper functions for platform driver */
> > +
> > +int hisi_crg_probe(struct platform_device *pdev);
> > +void hisi_crg_remove(struct platform_device *pdev);
> > +
> >   #endif      /* __HISI_CRG_H */
> > diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> > index 93cee17db8b1..c7d4c9ea7183 100644
> > --- a/drivers/clk/hisilicon/reset.c
> > +++ b/drivers/clk/hisilicon/reset.c
> > @@ -6,11 +6,15 @@
> >    */
> >
> >   #include <linux/io.h>
> > +#include <linux/kernel.h>
> >   #include <linux/of_address.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/reset-controller.h>
> >   #include <linux/slab.h>
> >   #include <linux/spinlock.h>
> > +
> > +#include "clk.h"
> > +#include "crg.h"
> >   #include "reset.h"
> >
> >   #define     HISI_RESET_BIT_MASK     0x1f
> > @@ -116,3 +120,41 @@ void hisi_reset_exit(struct hisi_reset_controller *rstc)
> >       reset_controller_unregister(&rstc->rcdev);
> >   }
> >   EXPORT_SYMBOL_GPL(hisi_reset_exit);
> > +
> > +int hisi_crg_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct hisi_crg_dev *crg;
> > +     int ret;
> > +
> > +     crg = devm_kmalloc(dev, sizeof(*crg), GFP_KERNEL);
> > +     if (!crg)
> > +             return -ENOMEM;
> > +
> > +     ret = hisi_clk_probe(pdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     crg->rstc = hisi_reset_init(pdev);
> > +     if (!crg->rstc) {
> > +             ret = -ENOMEM;
> > +             goto err;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, crg);
> > +     return 0;
> > +
> > +err:
> > +     hisi_clk_remove(pdev);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_crg_probe);
> > +
> > +void hisi_crg_remove(struct platform_device *pdev)
> > +{
> > +     struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
> > +
> > +     hisi_reset_exit(crg->rstc);
> > +     hisi_clk_remove(pdev);
> > +}
> > +EXPORT_SYMBOL_GPL(hisi_crg_remove);
>
>
> --
> Regards,
> Yang Xiwen
>

Powered by blists - more mailing lists