Re: [PATCH v6 3/4] clk: sophgo: Add SG2042 clock generator driver

[Chen Wang <unicorn_wang@xxxxxxxxxxx>]

hi,

Thank you Conor for your carefully review, I need some time to digest 
your comments. While I have some quick questions embeded.

On 2023/12/9 0:47, Conor Dooley wrote:
> On Fri, Dec 08, 2023 at 09:14:32AM +0800, Chen Wang wrote:
>
> > +#define div_mask(width) ((1 << (width)) - 1)
> Looks like this should be genmask.
Yes, I will improve this.
> > +#define ENCODE_PLL_CTRL(fbdiv, p1, p2, refdiv) \
> > +	(((fbdiv & 0xfff) << 16) | ((p2 & 0x7) << 12) | ((p1 & 0x7) << 8) | (refdiv & 0x3f))
> IMO this should be a function not a macro.
Would like to listen why it should be a function instead of a macro? Any 
experiences you can share with me?
> > +
> > +static inline int __sg2042_pll_enable(struct sg2042_pll_clock *pll, bool en)
> Why the __ prefixing of function names btw?

:) I just want to differ/highlight these functions agaist those clk 
operators. Anyway, this is indeed confusing, I will remove this ugly "__".

> > +{
> > +	unsigned int value = 0;
> > +	unsigned long enter;
> > +	struct regmap *map = pll->map;
> > +
> > +	if (en) {
> > +		/* wait pll lock */
> > +		enter = jiffies;
> > +		regmap_read(map, pll->offset_status, &value);
> > +		while (!((value >> pll->shift_status_lock) & 0x1)) {
> > +			regmap_read(map, pll->shift_status_lock, &value);
> > +			if (time_after(jiffies, enter + HZ / 10))
> > +				pr_warn("%s not locked\n", pll->name);
> > +		}
> Can't you just use read_poll_timeout()?
> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L16
It looks like this, will check more.
> > +		/* wait pll updating */
> > +		enter = jiffies;
> > +		regmap_read(map, pll->shift_status_updating, &value);
> > +		while (((value >> pll->shift_status_updating) & 0x1)) {
> > +			regmap_read(map, pll->shift_status_updating, &value);
> > +			if (time_after(jiffies, enter + HZ / 10))
> > +				pr_warn("%s still updating\n", pll->name);
> > +		}
> > +		/* enable pll */
> > +		regmap_read(map, pll->offset_enable, &value);
> > +		regmap_write(map, pll->offset_enable, value | (1 << pll->shift_enable));
> > +	} else {
> > +		/* disable pll */
> > +		regmap_read(map, pll->offset_enable, &value);
> > +		regmap_write(map, pll->offset_enable, value & (~(1 << pll->shift_enable)));
> > +	}
> > +
> > +	return 0;
> > +}
> > + +static unsigned int __sg2042_get_table_div(
> > +	const struct clk_div_table *table,
> > +	unsigned int val)
> > +{
> > +	const struct clk_div_table *clkt;
> > +
> > +	for (clkt = table; clkt->div; clkt++)
> > +		if (clkt->val == val)
> > +			return clkt->div;
> > +	return 0;
> > +}
> > +
> > +static unsigned int __sg2042_get_div(
> > +	const struct clk_div_table *table,
> > +	unsigned int val,
> > +	unsigned long flags, u8 width)
> > +{
> > +	if (flags & CLK_DIVIDER_ONE_BASED)
> > +		return val;
> > +	if (flags & CLK_DIVIDER_POWER_OF_TWO)
> > +		return 1 << val;
> > +	if (flags & CLK_DIVIDER_MAX_AT_ZERO)
> > +		return val ? val : div_mask(width) + 1;
> > +	if (table)
> > +		return __sg2042_get_table_div(table, val);
> > +	return val + 1;
> > +}
> Are these not effectively the same as clk_divider's ops?
Will double check.
> > +
> > +/*
> > + * @reg_value: current register value
> > + * @parent_rate: parent frequency
> > + *
> > + * This function is used to calculate below "rate" in equation
> > + * rate = (parent_rate/REFDIV) x FBDIV/POSTDIV1/POSTDIV2
> > + *      = (parent_rate x FBDIV) / (REFDIV x POSTDIV1 x POSTDIV2)
> > + */
> > +static unsigned long __sg2042_pll_recalc_rate(
> > +	unsigned int reg_value,
> > +	unsigned long parent_rate)
> > +{
> > +	unsigned int fbdiv, refdiv;
> > +	unsigned int postdiv1, postdiv2;
> > +	u64 rate, numerator, denominator;
> > +
> > +	fbdiv = (reg_value >> 16) & 0xfff;
> > +	refdiv = reg_value & 0x3f;
> > +	postdiv1 = (reg_value >> 8) & 0x7;
> > +	postdiv2 = (reg_value >> 12) & 0x7;
> IMO all of these are opportunities for GENMASK and defines.
Looks like this, will double check.
> > +
> > +	numerator = parent_rate * fbdiv;
> > +	denominator = refdiv * postdiv1 * postdiv2;
> > +	do_div(numerator, denominator);
> > +	rate = numerator;
> > +
> > +	return rate;
> > +}
> > +
> > +/*
> > + * Below array is the total combination lists of POSTDIV1 and POSTDIV2
> > + * for example:
> > + * postdiv1_2[0] = {2, 4, 8}
> > + *           ==> div1 = 2, div2 = 4 , div1 * div2 = 8
> > + * And POSTDIV_RESULT_INDEX point to 3rd element in the array
> > + */
> > +#define	POSTDIV_RESULT_INDEX	2
> > +static int postdiv1_2[][3] = {
> > +	{2, 4,  8}, {3, 3,  9}, {2, 5, 10}, {2, 6, 12},
> > +	{2, 7, 14}, {3, 5, 15}, {4, 4, 16}, {3, 6, 18},
> > +	{4, 5, 20}, {3, 7, 21}, {4, 6, 24}, {5, 5, 25},
> > +	{4, 7, 28}, {5, 6, 30}, {5, 7, 35}, {6, 6, 36},
> > +	{6, 7, 42}, {7, 7, 49}
> > +};
> > +
> > +/*
> > + * Based on input rate/prate/fbdiv/refdiv, look up the postdiv1_2 table
> > + * to get the closest postdiiv combination.
> > + * @rate: FOUTPOSTDIV
> > + * @prate: parent rate, i.e. FREF
> > + * @fbdiv: FBDIV
> > + * @refdiv: REFDIV
> > + * @postdiv1: POSTDIV1, output
> > + * @postdiv2: POSTDIV2, output
> > + * See TRM:
> > + * FOUTPOSTDIV = FREF * FBDIV / REFDIV / (POSTDIV1 * POSTDIV2)
> > + * So we get following formula to get POSTDIV1 and POSTDIV2:
> > + * POSTDIV = (prate/REFDIV) x FBDIV/rate
> > + * above POSTDIV = POSTDIV1*POSTDIV2
> > + */
> > +static int __sg2042_pll_get_postdiv_1_2(
> > +	unsigned long rate,
> > +	unsigned long prate,
> > +	unsigned int fbdiv,
> > +	unsigned int refdiv,
> > +	unsigned int *postdiv1,
> > +	unsigned int *postdiv2)
> This is not the coding style btw.
Agree, will fix this.
> > +{
> > +	int index = 0;
> > +	int ret = 0;
> > +	u64 tmp0;
> > +
> > +	/* prate/REFDIV and result save to tmp0 */
> > +	tmp0 = prate;
> > +	do_div(tmp0, refdiv);
> > +
> > +	/* ((prate/REFDIV) x FBDIV) and result save to tmp0 */
> > +	tmp0 *= fbdiv;
> > +
> > +	/* ((prate/REFDIV) x FBDIV)/rate and result save to tmp0 */
> > +	do_div(tmp0, rate);
> > +
> > +	/* tmp0 is POSTDIV1*POSTDIV2, now we calculate div1 and div2 value */
> > +	if (tmp0 <= 7) {
> > +		/* (div1 * div2) <= 7, no need to use array search */
> > +		*postdiv1 = tmp0;
> > +		*postdiv2 = 1;
> > +	} else {
> > +		/* (div1 * div2) > 7, use array search */
> > +		for (index = 0; index < ARRAY_SIZE(postdiv1_2); index++) {
> > +			if (tmp0 > postdiv1_2[index][POSTDIV_RESULT_INDEX]) {
> > +				continue;
> > +			} else {
> > +				/* found it */
> > +				break;
> > +			}
> > +		}
> > +		if (index < ARRAY_SIZE(postdiv1_2)) {
> > +			*postdiv1 = postdiv1_2[index][1];
> > +			*postdiv2 = postdiv1_2[index][0];
> > +		} else {
> > +			pr_debug("%s can not find in postdiv array!\n", __func__);
> > +			ret = -EINVAL;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> Reading this function it makes me wonder if (and I am far from the best
> person to comment, someone like Stephen is vastly more qualified) you
> should model this as several "stages", each implemented by the
> "standard" clocks - like clk_divider etc. The code here is quite
> complicated IMO as it seems to be trying to implement several stages of
> division in one go.

The objective of __sg2042_pll_get_postdiv_1_2() is straightforward: 
based on the formula defined by the TRM, with input 
rate/prate/fbdiv/refdiv, we can get the possiblle combination of 
POSTDIV1 and POSTDIV2 by looking up the table of postdiv1_2. We will 
later use it to setup the clock register.

Though the codes looks a bit complicated, but accually it is calculate 
with the formula : POSTDIV = (prate/REFDIV) x FBDIV/rate, I just 
separate it into several steps to make it easy to understand, I have 
listed the formula in the comment on top of the function.

> There's quite a lot in the driver and I will admit that I have not read
> it all my any means (I skimmed from here onwards), but in general my
> advice would be to try and reuse the generic code as much as possible.
Agree, I will double check and try to optimize the code in next revision.
> Thanks,
> Conor.
......