Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540871 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610  PLL1/PLL2 Stephen Boyd <sboyd@codeaurora.org> - 2016-12-13 08:40 +0100
    Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610  PLL1/PLL2 Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-12-13 08:50 +0100
      Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610  PLL1/PLL2 Stephen Boyd <sboyd@codeaurora.org> - 2016-12-14 00:20 +0100
    Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610  PLL1/PLL2 Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-12-13 09:00 +0100

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

On 12/09, 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
> @@ -288,6 +291,87 @@ static const struct clk_ops clk_pllv3_av_ops = {
>  	.set_rate	= clk_pllv3_av_set_rate,
>  };
>  
> +static unsigned long clk_pllv3_vf610_recalc_rate(struct clk_hw *hw,
> +					      unsigned long parent_rate)
> +{
> +	struct clk_pllv3 *pll = to_clk_pllv3(hw);
> +	u32 mfn = readl_relaxed(pll->base + PLL_VF610_NUM_OFFSET);
> +	u32 mfd = readl_relaxed(pll->base + PLL_VF610_DENOM_OFFSET);
> +	u32 div = (readl_relaxed(pll->base) & pll->div_mask) ? 22 : 20;
> +	u64 temp64 = (u64)parent_rate;

Useless cast, please remove.

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

> +	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?

> +}
> +
> +static int clk_pllv3_vf610_set_rate(struct clk_hw *hw, unsigned long rate,
> +		unsigned long parent_rate)
> +{
> +	struct clk_pllv3 *pll = to_clk_pllv3(hw);
> +	unsigned int mfi = (rate >= 22 * parent_rate) ? 22 : 20;
> +	u32 val, 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;
> +	}
> +
> +	val = readl_relaxed(pll->base);
> +	if (mfi == 20)

Presumably this is another place 20 and 22 are special.

> +		val &= ~pll->div_mask;
> +	else
> +		val |= pll->div_mask;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1540876

>> 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
>> @@ -288,6 +291,87 @@ static const struct clk_ops clk_pllv3_av_ops = {
>>  	.set_rate	= clk_pllv3_av_set_rate,
>>  };
>>  
>> +static unsigned long clk_pllv3_vf610_recalc_rate(struct clk_hw *hw,
>> +					      unsigned long parent_rate)
>> +{
>> +	struct clk_pllv3 *pll = to_clk_pllv3(hw);
>> +	u32 mfn = readl_relaxed(pll->base + PLL_VF610_NUM_OFFSET);
>> +	u32 mfd = readl_relaxed(pll->base + PLL_VF610_DENOM_OFFSET);
>> +	u32 div = (readl_relaxed(pll->base) & pll->div_mask) ? 22 : 20;
>> +	u64 temp64 = (u64)parent_rate;
> 
> Useless cast, please remove.

Ok

>> +
>> +	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?

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

>> +}
>> +
>> +static int clk_pllv3_vf610_set_rate(struct clk_hw *hw, unsigned long rate,
>> +		unsigned long parent_rate)
>> +{
>> +	struct clk_pllv3 *pll = to_clk_pllv3(hw);
>> +	unsigned int mfi = (rate >= 22 * parent_rate) ? 22 : 20;
>> +	u32 val, 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;
>> +	}
>> +
>> +	val = readl_relaxed(pll->base);
>> +	if (mfi == 20)
> 
> Presumably this is another place 20 and 22 are special.

See my reply above. Same applies here.

Nikita

[toc] | [prev] | [next] | [standalone]

#1541555

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

[toc] | [prev] | [next] | [standalone]

#1540880

>> 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
>> @@ -288,6 +291,87 @@ static const struct clk_ops clk_pllv3_av_ops = {
>>  	.set_rate	= clk_pllv3_av_set_rate,
>>  };
>>  
>> +static unsigned long clk_pllv3_vf610_recalc_rate(struct clk_hw *hw,
>> +					      unsigned long parent_rate)
>> +{
>> +	struct clk_pllv3 *pll = to_clk_pllv3(hw);
>> +	u32 mfn = readl_relaxed(pll->base + PLL_VF610_NUM_OFFSET);
>> +	u32 mfd = readl_relaxed(pll->base + PLL_VF610_DENOM_OFFSET);
>> +	u32 div = (readl_relaxed(pll->base) & pll->div_mask) ? 22 : 20;

Should be not 'div' but 'mfi' to be consistent with datasheet and with
other routines in the same patch. Will fix.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web