| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAXyoMM0VHHtQc9GevPqaLNTjyWbvMr88LjvxKAwEK=U_shL6Q@mail.gmail.com>
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