Re: [PATCH v8 2/3] clock: eswin: Add eic7700 clock driver

[Troy Mitchell <troy.mitchell@linux.dev>]

Hi Xuyang,
This is a quick review.

On Thu, Nov 13, 2025 at 09:38:46AM +0800, dongxuyang@eswincomputing.com wrote:
> From: Xuyang Dong <dongxuyang@eswincomputing.com>
> 
> Add clock drivers for the EIC7700 SoC. The clock controller on the ESWIN
> EIC7700 provides various clocks to different IP blocks within the SoC.
> 
> Signed-off-by: Yifeng Huang <huangyifeng@eswincomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
> ---
>  drivers/clk/Kconfig             |    1 +
>  drivers/clk/Makefile            |    1 +
>  drivers/clk/eswin/Kconfig       |   15 +
>  drivers/clk/eswin/Makefile      |    8 +
>  drivers/clk/eswin/clk-eic7700.c | 1033 +++++++++++++++++++++++++++++++
>  drivers/clk/eswin/clk-eic7700.h |  122 ++++
>  drivers/clk/eswin/clk.c         |  481 ++++++++++++++
>  drivers/clk/eswin/clk.h         |  256 ++++++++
>  8 files changed, 1917 insertions(+)
>  create mode 100644 drivers/clk/eswin/Kconfig
>  create mode 100644 drivers/clk/eswin/Makefile
>  create mode 100644 drivers/clk/eswin/clk-eic7700.c
>  create mode 100644 drivers/clk/eswin/clk-eic7700.h
>  create mode 100644 drivers/clk/eswin/clk.c
>  create mode 100644 drivers/clk/eswin/clk.h
> 
[...]
> diff --git a/drivers/clk/eswin/clk-eic7700.c b/drivers/clk/eswin/clk-eic7700.c
> new file mode 100644
> index 000000000000..03540db9cb7d
> --- /dev/null
> +++ b/drivers/clk/eswin/clk-eic7700.c
[...]
> +static int eic7700_clk_probe(struct platform_device *pdev)
> +{
> +	struct eswin_clock_data *clk_data;
> +	struct device *dev = &pdev->dev;
> +
> +	clk_data = eswin_clk_init(dev, EIC7700_NR_CLKS);
> +	if (!clk_data)
> +		return dev_err_probe(dev, -EAGAIN, "failed to get clk data!\n");
> +
> +	eswin_clk_register_fixed_rate(eic7700_fixed_rate_clks,
> +				      ARRAY_SIZE(eic7700_fixed_rate_clks),
> +				      clk_data, dev);
Please check returned value here and check other places.

> +
> +	eswin_clk_register_pll(eic7700_pll_clks, ARRAY_SIZE(eic7700_pll_clks),
> +			       clk_data, dev);
> +
> +	eswin_clk_register_fixed_factor(eic7700_fixed_factor_clks,
> +					ARRAY_SIZE(eic7700_fixed_factor_clks),
> +					clk_data, dev);
> +
> +	eswin_clk_register_mux(eic7700_mux_clks, ARRAY_SIZE(eic7700_mux_clks),
> +			       clk_data, dev);
> +
> +	eswin_clk_register_divider(eic7700_div_clks,
> +				   ARRAY_SIZE(eic7700_div_clks), clk_data, dev);
> +
> +	eswin_clk_register_gate(eic7700_gate_clks,
> +				ARRAY_SIZE(eic7700_gate_clks), clk_data, dev);
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   &clk_data->clk_data);
> +}
> +
[...]
> diff --git a/drivers/clk/eswin/clk.c b/drivers/clk/eswin/clk.c
> new file mode 100644
> index 000000000000..92998da98009
> --- /dev/null
> +++ b/drivers/clk/eswin/clk.c
[...]
> +static int eswin_calc_pll(u32 *frac_val, u32 *fbdiv_val, u64 rate,
> +			  const struct eswin_clk_pll *clk)
> +{
> +	u64 rem = 0;
> +	u32 tmp1 = 0, tmp2 = 0;
> +
> +	if (clk->id == EIC7700_CLK_APLL_FOUT1 ||
> +	    clk->id == EIC7700_CLK_PLL_CPU) {
clk.c appears to be a generic library for ESWIN SoCs, but it references
specific IDs (EIC7700_...) from a specific chip.

If you add the chip like EIC77XX later, will you add more if/else here?

Could you ove the algorithmic parameters (like rate * 4) into the
eswin_clk_pll structure flags or data fields. The calculation logic
should be generic based on the struct fields, not the ID.

Please check other places.
> +		rate = rate * 4;
> +		rem = do_div(rate, 1000);
> +		if (rem)
> +			tmp1 = rem;
> +
> +		rem = do_div(rate, 1000);
> +		if (rem)
> +			tmp2 = rem;
> +
> +		rem = do_div(rate, 24);
> +		/* fbdiv = rate * 4 / 24000000 */
> +		*fbdiv_val = rate;
> +		/* frac = rate * 4 % 24000000 * (2 ^ 24) */
> +		*frac_val = (u64)((1000 * (1000 * rem + tmp2) + tmp1) << 24)
> +				  / 24 / 1000000;
> +	} else {
> +		pr_err("Invalid pll set req, rate %lld, clk id %d\n", rate,
> +		       clk->id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline struct eswin_clk_pll *to_pll_clk(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct eswin_clk_pll, hw);
> +}
> +
> +static int clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{
> +	struct eswin_clk_pll *clk = to_pll_clk(hw);
> +	struct clk *clk_cpu_lp_pll = NULL;
> +	struct clk *clk_cpu_mux = NULL;
> +	struct clk *clk_cpu_pll = NULL;
> +	u32 postdiv1_val = 0, refdiv_val = 1;
> +	u32 frac_val, fbdiv_val, val;
> +	bool lock_flag = false;
> +	int try_count = 0;
> +	int ret;
> +
> +	ret = eswin_calc_pll(&frac_val,  &fbdiv_val, (u64)rate, clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Must switch the CPU to other CLK before we change the CPU PLL. */
> +	if (clk->id == EIC7700_CLK_PLL_CPU) {
> +		clk_cpu_mux = __clk_lookup("mux_cpu_root_3mux1_gfree");
It seems you want to switch to a safe clock source before setting up the
PLL, right?

I am not sure whether your approach is correct, but the use of
__clk_lookup() should be avoided whenever possible. 
Would it be feasible to obtain a proper clock handle somewhere and
perform the necessary configuration from within a clk_notifier instead?
> +		if (!clk_cpu_mux) {
> +			pr_err("failed to get clk: %s\n",
> +			       "mux_cpu_root_3mux1_gfree");
> +			return -EINVAL;
> +		}