Date: Sun, 25 Feb 2024 19:56:48 +0800
From: Yang Xiwen <forbidden405@...look.com>
To: David Yang <mmyangfl@...il.com>, linux-clk@...r.kernel.org
Cc: 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

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.


>
> 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.


>
> `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