Message-ID: <MA0P287MB0332A937E4DF0044594B19CCFE8EA@MA0P287MB0332.INDP287.PROD.OUTLOOK.COM>
Date:   Tue, 12 Dec 2023 10:22:28 +0800
From:   Chen Wang <unicorn_wang@...look.com>
To:     Conor Dooley <conor@...nel.org>, Chen Wang <unicornxw@...il.com>
Cc:     aou@...s.berkeley.edu, chao.wei@...hgo.com,
        krzysztof.kozlowski+dt@...aro.org, mturquette@...libre.com,
        palmer@...belt.com, paul.walmsley@...ive.com,
        richardcochran@...il.com, robh+dt@...nel.org, sboyd@...nel.org,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
        haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com,
        guoren@...nel.org, jszhang@...nel.org, inochiama@...look.com,
        samuel.holland@...ive.com
Subject: Re: [PATCH v6 3/4] clk: sophgo: Add SG2042 clock generator driver

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

Powered by blists - more mailing lists