Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610 PLL1/PLL2

[Stephen Boyd <sboyd@codeaurora.org>]

On 12/13, Nikita Yushchenko wrote:
> >> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> index 19f9b622981a..24a9e914e0d5 100644
> >> --- a/drivers/clk/imx/clk-pllv3.c
> >> +++ b/drivers/clk/imx/clk-pllv3.c
> 
> >> +
> >> +	temp64 *= mfn;
> >> +	do_div(temp64, mfd);
> >> +
> >> +	return (parent_rate * div) + (u32)temp64;
> >> +}
> >> +
> >> +static long clk_pllv3_vf610_round_rate(struct clk_hw *hw, unsigned long rate,
> >> +				    unsigned long *prate)
> >> +{
> >> +	unsigned long parent_rate = *prate;
> >> +	unsigned int mfi = (rate >= 22 * parent_rate) ? 22 : 20;
> > 
> > What is the importance of 22 and 20? Hint, at the least it needs
> > a comment.
> 
> These come directly from datasheet:
> 
> Frequency multipler selection (MFI).
> 0: Fout = Fref * 20
> 1: Fout = Fref * 22
> 
> These numbers (20 / 22) are common among flavours of pllv3 hardware.
> In similar places in the same file (e.g. in clk_pllv3_recalc_rate(), in
> clk_pllv3_set_rate() ,etc) there are no comments explaining them.
> 
> Are you sure this place is special and comment is needed here?

Probably an oversight when merging the code before. Please add a
comment, or make some sort of function like

	rate_to_mfi(rate, parent_rate)

And then add a comment above the function to describe what's
special about 22 and 20.

> 
> >> +	u32 mfn, mfd = 0x3fffffff;
> >> +	u64 temp64;
> >> +
> >> +	if (rate <= parent_rate * mfi)
> >> +		mfn = 0;
> >> +	else if (rate >= parent_rate * (mfi + 1))
> >> +		mfn = mfd - 1;
> >> +	else {
> >> +		/* rate = parent_rate * (mfi + mfn/mfd) */
> >> +		temp64 = rate - parent_rate * mfi;
> >> +		temp64 *= mfd;
> >> +		do_div(temp64, parent_rate);
> >> +		mfn = temp64;
> >> +	}
> >> +
> >> +	temp64 = ((u64)mfd * mfi + mfn) * parent_rate;
> >> +	do_div(temp64, mfd);
> >> +	return (u32)temp64;
> > 
> > Do we need the cast here for some reason?
> 
> Just for readability, can remove if it hurts.

Please do.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project