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

[Brian Masney]

Hi Xuyang,

On Fri, Feb 13, 2026 at 05:41:57PM +0800, dongxuyang@xxxxxxxxxxxxxxxxxx wrote:
> From: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxx>
> Tested-by: Marcel Ziswiler <marcel@xxxxxxxxxxxx> # ebc77
> Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>

The commit subject has 'clock: eswin: ...', and it should be
'clk: eswin: ...'.

> +++ b/drivers/clk/eswin/clk.c
> @@ -0,0 +1,579 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2026, Beijing ESWIN Computing Technology Co., Ltd..
> + * All rights reserved.
> + *
> + * Authors:
> + *	Yifeng Huang <huangyifeng@xxxxxxxxxxxxxxxxxx>
> + *	Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include <linux/math.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +
> +struct eswin_clock_data *eswin_clk_init(struct platform_device *pdev,
> +					size_t nr_clks)
> +{
> +	struct eswin_clock_data *eclk_data;
> +
> +	eclk_data = devm_kzalloc(&pdev->dev,
> +				 struct_size(eclk_data, clk_data.hws, nr_clks),
> +				 GFP_KERNEL);
> +	if (!eclk_data)
> +		return NULL;
> +
> +	eclk_data->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(eclk_data->base)) {
> +		dev_err(&pdev->dev, "failed to map clock registers\n");
> +		return NULL;
> +	}
> +
> +	eclk_data->clk_data.num = nr_clks;
> +	/* Avoid returning NULL for unused id */
> +	memset_p((void **)eclk_data->clk_data.hws, ERR_PTR(-ENOENT), nr_clks);
> +	spin_lock_init(&eclk_data->lock);
> +
> +	return eclk_data;
> +}
> +EXPORT_SYMBOL_GPL(eswin_clk_init);
> +
> +/**
> + * eswin_calc_pll - calculate PLL values
> + * @frac_val: fractional divider
> + * @fbdiv_val: feedback divider
> + * @rate: reference rate
> + *
> + *   Calculate PLL values for frac and fbdiv
> + */
> +static void eswin_calc_pll(u32 *frac_val, u32 *fbdiv_val, u64 rate)
> +{
> +	u32 rem, tmp1 = 0, tmp2 = 0;
> +
> +	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) */

I assume the 24 MHz is the parent rate? This function is only used by
clk_pll_set_rate(). Should the parent_rate be passed in as well, and
use that variable instead of hard coding the parent rate?

> +	*frac_val = ((1000 * (1000 * rem + tmp2) + tmp1) << 24) / 24
> +		     / 1000000;
> +}
> +
> +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);
> +	u32 frac_val, fbdiv_val, val;
> +	int ret;
> +
> +	eswin_calc_pll(&frac_val, &fbdiv_val, (u64)rate);
> +
> +	/* First, disable pll */
> +	val = readl_relaxed(clk->ctrl_reg0);
> +	val &= ~(((1 << clk->pllen_width) - 1) << clk->pllen_shift);
> +	val |= 0 << clk->pllen_shift;
> +	writel_relaxed(val, clk->ctrl_reg0);
> +
> +	val = readl_relaxed(clk->ctrl_reg0);
> +	val &= ~(((1 << clk->fbdiv_width) - 1) << clk->fbdiv_shift);
> +	val &= ~(((1 << clk->refdiv_width) - 1) << clk->refdiv_shift);
> +	val |= 1 << clk->refdiv_shift;
> +	val |= fbdiv_val << clk->fbdiv_shift;
> +	writel_relaxed(val, clk->ctrl_reg0);
> +
> +	val = readl_relaxed(clk->ctrl_reg1);
> +	val &= ~(((1 << clk->frac_width) - 1) << clk->frac_shift);
> +	val |= frac_val << clk->frac_shift;
> +	writel_relaxed(val, clk->ctrl_reg1);
> +
> +	/* Last, enable pll */
> +	val = readl_relaxed(clk->ctrl_reg0);
> +	val &= ~(((1 << clk->pllen_width) - 1) << clk->pllen_shift);
> +	val |= 1 << clk->pllen_shift;
> +	writel_relaxed(val, clk->ctrl_reg0);

You can use GENMASK() and FIELD_PREP() to simplify these bitwise
operations.
> +
> +	/* Usually the pll will lock in 50us */
> +	ret = readl_poll_timeout(clk->status_reg, val,
> +				 val & (1 << clk->lock_shift), 1, 50 * 2);
> +	if (ret)
> +		pr_err("failed to lock the pll!\n");
> +
> +	return ret;
> +}
> +
> +static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
> +					 unsigned long parent_rate)
> +{
> +	struct eswin_clk_pll *clk = to_pll_clk(hw);
> +	u32 fbdiv_val, frac_val, rem, val;
> +	unsigned long rate;
> +	u64 tmp;
> +
> +	val = readl_relaxed(clk->ctrl_reg0);
> +	val = val >> clk->fbdiv_shift;
> +	val &= ((1 << clk->fbdiv_width) - 1);
> +	fbdiv_val = val;
> +
> +	val = readl_relaxed(clk->ctrl_reg1);
> +	val = val >> clk->frac_shift;
> +	val &= ((1 << clk->frac_width) - 1);
> +	frac_val = val;
> +
> +	/* rate = 24000000 * (fbdiv + frac / (2 ^ 24)) / 4 */
> +	tmp = 1000 * frac_val;
> +	rem = do_div(tmp, BIT(24));
> +	if (rem)
> +		rate = (unsigned long)(6000 * (1000 * fbdiv_val + tmp) +
> +				      ((6000 * rem) >> 24) + 1);
> +	else
> +		rate = (unsigned long)(6000 * 1000 * fbdiv_val);
> +
> +	return rate;
> +}
> +
> +static int clk_pll_determine_rate(struct clk_hw *hw,
> +				  struct clk_rate_request *req)
> +{
> +	struct eswin_clk_pll *clk = to_pll_clk(hw);
> +
> +	req->rate = clamp(req->rate, clk->min_rate, clk->max_rate);
> +	req->min_rate = clk->min_rate;
> +	req->max_rate = clk->max_rate;
> +
> +	return 0;
> +}
> +
> +int eswin_clk_register_fixed_rate(struct device *dev,
> +				  struct eswin_fixed_rate_clock *clks,
> +				  int nums, struct eswin_clock_data *data)
> +{
> +	struct clk_hw *clk_hw;
> +	int i;
> +
> +	for (i = 0; i < nums; i++) {
> +		clk_hw = devm_clk_hw_register_fixed_rate(dev, clks[i].name,
> +							 NULL, clks[i].flags,
> +							 clks[i].rate);
> +		if (IS_ERR(clk_hw))
> +			return PTR_ERR(clk_hw);
> +
> +		clks[i].hw = *clk_hw;
> +		data->clk_data.hws[clks[i].id] = clk_hw;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(eswin_clk_register_fixed_rate);
> +
> +static const struct clk_ops eswin_clk_pll_ops = {
> +	.set_rate = clk_pll_set_rate,
> +	.recalc_rate = clk_pll_recalc_rate,
> +	.determine_rate = clk_pll_determine_rate,
> +};
> +
> +int eswin_clk_register_pll(struct device *dev, struct eswin_pll_clock *clks,
> +			   int nums, struct eswin_clock_data *data)
> +{
> +	struct eswin_clk_pll *p_clk = NULL;
> +	struct clk_init_data init;
> +	struct clk_hw *clk_hw;
> +	int i, ret;
> +
> +	p_clk = devm_kzalloc(dev, sizeof(*p_clk) * nums, GFP_KERNEL);
> +	if (!p_clk)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nums; i++) {
> +		p_clk->id = clks[i].id;
> +		p_clk->ctrl_reg0 = data->base + clks[i].ctrl_reg0;
> +		p_clk->pllen_shift = clks[i].pllen_shift;
> +		p_clk->pllen_width = clks[i].pllen_width;
> +		p_clk->refdiv_shift = clks[i].refdiv_shift;
> +		p_clk->refdiv_width = clks[i].refdiv_width;
> +		p_clk->fbdiv_shift = clks[i].fbdiv_shift;
> +		p_clk->fbdiv_width = clks[i].fbdiv_width;
> +
> +		p_clk->ctrl_reg1 = data->base + clks[i].ctrl_reg1;
> +		p_clk->frac_shift = clks[i].frac_shift;
> +		p_clk->frac_width = clks[i].frac_width;
> +
> +		p_clk->ctrl_reg2 = data->base + clks[i].ctrl_reg2;
> +		p_clk->postdiv1_shift = clks[i].postdiv1_shift;
> +		p_clk->postdiv1_width = clks[i].postdiv1_width;
> +		p_clk->postdiv2_shift = clks[i].postdiv2_shift;
> +		p_clk->postdiv2_width = clks[i].postdiv2_width;

Are these postdiv[12]_* members actually used anywhere? 

Brian