[PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Soren Brinkmann]

Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
rounding errors.

Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
Hi,

I just came across this behavior:
I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
During boot I create an opp table which in this case holds the frequencies [MHz]
666, 333 and 222. To make sure those are actually valid frequencies I call
clk_round_rate().
I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
in line 163 giving me:
	prate:1333333320, rate:333333330, div:4
for adding the 333 MHz operating point.

In the running system this gives me:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies 
222222 333333 666666 
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 
 666666

 So far, so good. But now, let's scale the frequency:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed 
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
 266666

And the corresponding debug line:
	prate:1333333320, rate:333333000, div:5

So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
huge error.

	Regards,
	Sören

 drivers/clk/clk-divider.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index a9204c6..32e1b6a 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -157,7 +157,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 
 	if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
 		parent_rate = *best_parent_rate;
-		bestdiv = DIV_ROUND_UP(parent_rate, rate);
+		bestdiv = DIV_ROUND_CLOSEST(parent_rate, rate);
 		bestdiv = bestdiv == 0 ? 1 : bestdiv;
 		bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv;
 		return bestdiv;
@@ -207,7 +207,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
-	div = parent_rate / rate;
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
 	value = _get_val(divider, div);
 
 	if (value > div_mask(divider))
-- 
1.8.1.2

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sören Brinkmann]

ping?

Any opinions?

	Thanks,
	Sören

On Tue, Jan 29, 2013 at 05:25:44PM -0800, Soren Brinkmann wrote:
> Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
> rounding errors.
> 
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
> Hi,
> 
> I just came across this behavior:
> I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
> During boot I create an opp table which in this case holds the frequencies [MHz]
> 666, 333 and 222. To make sure those are actually valid frequencies I call
> clk_round_rate().
> I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
> in line 163 giving me:
> 	prate:1333333320, rate:333333330, div:4
> for adding the 333 MHz operating point.
> 
> In the running system this gives me:
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies 
> 222222 333333 666666 
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 
>  666666
> 
>  So far, so good. But now, let's scale the frequency:
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed 
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
>  266666
> 
> And the corresponding debug line:
> 	prate:1333333320, rate:333333000, div:5
> 
> So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
> huge error.
> 
> 	Regards,
> 	Sören
> 
>  drivers/clk/clk-divider.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index a9204c6..32e1b6a 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -157,7 +157,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
>  
>  	if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
>  		parent_rate = *best_parent_rate;
> -		bestdiv = DIV_ROUND_UP(parent_rate, rate);
> +		bestdiv = DIV_ROUND_CLOSEST(parent_rate, rate);
>  		bestdiv = bestdiv == 0 ? 1 : bestdiv;
>  		bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv;
>  		return bestdiv;
> @@ -207,7 +207,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> -	div = parent_rate / rate;
> +	div = DIV_ROUND_CLOSEST(parent_rate, rate);
>  	value = _get_val(divider, div);
>  
>  	if (value > div_mask(divider))
> -- 
> 1.8.1.2
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sören Brinkmann]

Hi Mike,

On Tue, Mar 19, 2013 at 05:16:09PM -0700, Mike Turquette wrote:
> Quoting Soren Brinkmann (2013-01-29 17:25:44)
> > Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
> > rounding errors.
> >
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> > Hi,
> >
> > I just came across this behavior:
> > I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
> > During boot I create an opp table which in this case holds the frequencies [MHz]
> > 666, 333 and 222. To make sure those are actually valid frequencies I call
> > clk_round_rate().
> > I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
> > in line 163 giving me:
> >         prate:1333333320, rate:333333330, div:4
> > for adding the 333 MHz operating point.
> >
> > In the running system this gives me:
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies
> > 222222 333333 666666
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> >  666666
> >
> >  So far, so good. But now, let's scale the frequency:
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> >  266666
> >
> > And the corresponding debug line:
> >         prate:1333333320, rate:333333000, div:5
> >
> > So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
> > huge error.
> >
>
> Soren,
>
> Thanks for the patch, and apologies for it getting lost for so long.  I
> think that your issue is more about policy.  E.g. should we round the
> divider up or round the divider down?  The correct answer will vary from
> platform to platform and the clk.h api doesn't specify how
> clk_round_rate should round, other than it must specify a rate that the
> clock can actually run at.
Sure, my problem seems to be caused by different subsystems using a different resolution for clock frequencies.

>
> Unless everyone can agree that DIV_ROUND_CLOSEST gives them what they
> want I'm more inclined to make this behavior a flag specific to struct
> clk-divider.  E.g. CLK_DIVIDER_ROUND_CLOSEST.
My understanding would have been, that clk_round_rate() gives me a frequency as close to the requested one as possible. If the caller doesn't like the returned frequency he can request a different one. And he's eventually happy with the return value he calls clk_set_rate() requesting the frequency clk_round_rate() returned.
Always rounding down seems a bit odd to me.

Another issue with the current implmentation:
clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
clk_divider_set_rate() does plain integer divisions. IMHO, that should cause the actually set frequency to differ from what round_rate() returns, and in that case it is even higher than the expected.

> Care to take a crack at that approach?
If a new flag is the preferred solution, I'll sure do that.

        Sören

>
> Regards,
> Mike
>
> >         Regards,
> >         Sören
> >
> >  drivers/clk/clk-divider.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index a9204c6..32e1b6a 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -157,7 +157,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
> >
> >         if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
> >                 parent_rate = *best_parent_rate;
> > -               bestdiv = DIV_ROUND_UP(parent_rate, rate);
> > +               bestdiv = DIV_ROUND_CLOSEST(parent_rate, rate);
> >                 bestdiv = bestdiv == 0 ? 1 : bestdiv;
> >                 bestdiv = bestdiv > maxdiv ? maxdiv : bestdiv;
> >                 return bestdiv;
> > @@ -207,7 +207,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> >         unsigned long flags = 0;
> >         u32 val;
> >
> > -       div = parent_rate / rate;
> > +       div = DIV_ROUND_CLOSEST(parent_rate, rate);
> >         value = _get_val(divider, div);
> >
> >         if (value > div_mask(divider))
> > --
> > 1.8.1.2
>


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sascha Hauer]

On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> Hi Mike,
> 
> On Tue, Mar 19, 2013 at 05:16:09PM -0700, Mike Turquette wrote:
> > Quoting Soren Brinkmann (2013-01-29 17:25:44)
> > > Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
> > > rounding errors.
> > >
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > ---
> > > Hi,
> > >
> > > I just came across this behavior:
> > > I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
> > > During boot I create an opp table which in this case holds the frequencies [MHz]
> > > 666, 333 and 222. To make sure those are actually valid frequencies I call
> > > clk_round_rate().
> > > I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
> > > in line 163 giving me:
> > >         prate:1333333320, rate:333333330, div:4
> > > for adding the 333 MHz operating point.
> > >
> > > In the running system this gives me:
> > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies
> > > 222222 333333 666666
> > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > >  666666
> > >
> > >  So far, so good. But now, let's scale the frequency:
> > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed
> > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > >  266666
> > >
> > > And the corresponding debug line:
> > >         prate:1333333320, rate:333333000, div:5
> > >
> > > So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
> > > huge error.
> > >
> >
> > Soren,
> >
> > Thanks for the patch, and apologies for it getting lost for so long.  I
> > think that your issue is more about policy.  E.g. should we round the
> > divider up or round the divider down?  The correct answer will vary from
> > platform to platform and the clk.h api doesn't specify how
> > clk_round_rate should round, other than it must specify a rate that the
> > clock can actually run at.
> Sure, my problem seems to be caused by different subsystems using a different resolution for clock frequencies.
> 
> >
> > Unless everyone can agree that DIV_ROUND_CLOSEST gives them what they
> > want I'm more inclined to make this behavior a flag specific to struct
> > clk-divider.  E.g. CLK_DIVIDER_ROUND_CLOSEST.
> My understanding would have been, that clk_round_rate() gives me a
> frequency as close to the requested one as possible.

clk_round_rate clk_set_rate are implemented to give the closest possible
rate that *is not higher* than the desired clock. That was the
understanding by the time the clk framework was implemented.

> If the caller
> doesn't like the returned frequency he can request a different one.
> And he's eventually happy with the return value he calls
> clk_set_rate() requesting the frequency clk_round_rate() returned.
> Always rounding down seems a bit odd to me.
> 
> Another issue with the current implmentation:
> clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.

And that is correct. clk_divider_bestdiv is used to calculate the
maximum parent frequency for which a given divider value does not
exceed the desired rate.

That is, when you want to have a frequency of 100Hz and your divider is
2, then your parent input clock can be between 101Hz and 200Hz,
corresponding to a call to DIV_ROUND_UP.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Uwe Kleine-König]

Hello,

On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > If the caller
> > doesn't like the returned frequency he can request a different one.
> > And he's eventually happy with the return value he calls
> > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > Always rounding down seems a bit odd to me.
> > 
> > Another issue with the current implmentation:
> > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
> 
> And that is correct. clk_divider_bestdiv is used to calculate the
> maximum parent frequency for which a given divider value does not
> exceed the desired rate.
The reason for that is that the (more?) usual constraint is like: This
mmc card can handle up to 100 MHz. Or this i2c device can handle up to
this and that frequency. Of course there are different constraints, e.g.
for a UART if the target baud speed is 38400 you better run at 38402
than at 19201.

I wonder if it depends on the clock if you want "best approximation <=
requested value" or "best approximation" or on the caller. In the former
case a flag for the clock would be the right thing (as suggested in this
thread). If however it's the caller of round_rate who knows better which
rounding is preferred than better extend the clk API.

Extending the API could just be a convenience function that doesn't
affect the implementations of the clk API. E.g.:

	long clk_round_rate_nearest(struct clk *clk, unsigned long rate)
	{
		long lower_limit = clk_round_rate(clk, rate);
		long upper_limit = clk_round_rate(clk, rate + (rate - lower_limit));

		if (rate - lower_limit < upper_limit - rate)
			return lower_limit;
		else
			return upper_limit;
	}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sören Brinkmann]

On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > Hi Mike,
> >
> > On Tue, Mar 19, 2013 at 05:16:09PM -0700, Mike Turquette wrote:
> > > Quoting Soren Brinkmann (2013-01-29 17:25:44)
> > > > Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
> > > > rounding errors.
> > > >
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > > ---
> > > > Hi,
> > > >
> > > > I just came across this behavior:
> > > > I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
> > > > During boot I create an opp table which in this case holds the frequencies [MHz]
> > > > 666, 333 and 222. To make sure those are actually valid frequencies I call
> > > > clk_round_rate().
> > > > I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
> > > > in line 163 giving me:
> > > >         prate:1333333320, rate:333333330, div:4
> > > > for adding the 333 MHz operating point.
> > > >
> > > > In the running system this gives me:
> > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies
> > > > 222222 333333 666666
> > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > > >  666666
> > > >
> > > >  So far, so good. But now, let's scale the frequency:
> > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed
> > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > > >  266666
> > > >
> > > > And the corresponding debug line:
> > > >         prate:1333333320, rate:333333000, div:5
> > > >
> > > > So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
> > > > huge error.
> > > >
> > >
> > > Soren,
> > >
> > > Thanks for the patch, and apologies for it getting lost for so long.  I
> > > think that your issue is more about policy.  E.g. should we round the
> > > divider up or round the divider down?  The correct answer will vary from
> > > platform to platform and the clk.h api doesn't specify how
> > > clk_round_rate should round, other than it must specify a rate that the
> > > clock can actually run at.
> > Sure, my problem seems to be caused by different subsystems using a different resolution for clock frequencies.
> >
> > >
> > > Unless everyone can agree that DIV_ROUND_CLOSEST gives them what they
> > > want I'm more inclined to make this behavior a flag specific to struct
> > > clk-divider.  E.g. CLK_DIVIDER_ROUND_CLOSEST.
> > My understanding would have been, that clk_round_rate() gives me a
> > frequency as close to the requested one as possible.
>
> clk_round_rate clk_set_rate are implemented to give the closest possible
> rate that *is not higher* than the desired clock. That was the
> understanding by the time the clk framework was implemented.
>
> > If the caller
> > doesn't like the returned frequency he can request a different one.
> > And he's eventually happy with the return value he calls
> > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > Always rounding down seems a bit odd to me.
> >
> > Another issue with the current implmentation:
> > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
>
> And that is correct. clk_divider_bestdiv is used to calculate the
> maximum parent frequency for which a given divider value does not
> exceed the desired rate.
That is not what I mean. I was pointing at, that passing the same
frequency to round_rate and set_rate can return different results, since
round_rate rounds up whereas set_rate rounds down. Though, it's probably
fine since you shouldn't call set_rate with a frequency which wasn't returned
from round_rate.

>
> That is, when you want to have a frequency of 100Hz and your divider is
> 2, then your parent input clock can be between 101Hz and 200Hz,
> corresponding to a call to DIV_ROUND_UP.
I was not (yet) looking at cases where CLK_SET_RATE_PARENT is set. Just
an isolated clk-divider.

        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sascha Hauer]

On Thu, Mar 21, 2013 at 09:36:14AM -0700, Sören Brinkmann wrote:
> On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> > On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > > Hi Mike,
> > >
> > > On Tue, Mar 19, 2013 at 05:16:09PM -0700, Mike Turquette wrote:
> > > > Quoting Soren Brinkmann (2013-01-29 17:25:44)
> > > > > Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
> > > > > rounding errors.
> > > > >
> > > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > > > > ---
> > > > > Hi,
> > > > >
> > > > > I just came across this behavior:
> > > > > I'm using the clk-divider as cpuclock which can be scaled through cpufreq.
> > > > > During boot I create an opp table which in this case holds the frequencies [MHz]
> > > > > 666, 333 and 222. To make sure those are actually valid frequencies I call
> > > > > clk_round_rate().
> > > > > I added a debug line in clk-divider.c:clk_divider_bestdiv() before the return
> > > > > in line 163 giving me:
> > > > >         prate:1333333320, rate:333333330, div:4
> > > > > for adding the 333 MHz operating point.
> > > > >
> > > > > In the running system this gives me:
> > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_frequencies
> > > > > 222222 333333 666666
> > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > > > >  666666
> > > > >
> > > > >  So far, so good. But now, let's scale the frequency:
> > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 333333 > scaling_setspeed
> > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > > > >  266666
> > > > >
> > > > > And the corresponding debug line:
> > > > >         prate:1333333320, rate:333333000, div:5
> > > > >
> > > > > So, with DIV_ROUND_UP an actual divider of 4.00000396 becomes 5, resulting in a
> > > > > huge error.
> > > > >
> > > >
> > > > Soren,
> > > >
> > > > Thanks for the patch, and apologies for it getting lost for so long.  I
> > > > think that your issue is more about policy.  E.g. should we round the
> > > > divider up or round the divider down?  The correct answer will vary from
> > > > platform to platform and the clk.h api doesn't specify how
> > > > clk_round_rate should round, other than it must specify a rate that the
> > > > clock can actually run at.
> > > Sure, my problem seems to be caused by different subsystems using a different resolution for clock frequencies.
> > >
> > > >
> > > > Unless everyone can agree that DIV_ROUND_CLOSEST gives them what they
> > > > want I'm more inclined to make this behavior a flag specific to struct
> > > > clk-divider.  E.g. CLK_DIVIDER_ROUND_CLOSEST.
> > > My understanding would have been, that clk_round_rate() gives me a
> > > frequency as close to the requested one as possible.
> >
> > clk_round_rate clk_set_rate are implemented to give the closest possible
> > rate that *is not higher* than the desired clock. That was the
> > understanding by the time the clk framework was implemented.
> >
> > > If the caller
> > > doesn't like the returned frequency he can request a different one.
> > > And he's eventually happy with the return value he calls
> > > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > > Always rounding down seems a bit odd to me.
> > >
> > > Another issue with the current implmentation:
> > > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
> >
> > And that is correct. clk_divider_bestdiv is used to calculate the
> > maximum parent frequency for which a given divider value does not
> > exceed the desired rate.
> That is not what I mean. I was pointing at, that passing the same
> frequency to round_rate and set_rate can return different results, since
> round_rate rounds up whereas set_rate rounds down. Though, it's probably
> fine since you shouldn't call set_rate with a frequency which wasn't returned
> from round_rate.

There's no rule to call clk_set_rate only with the result of
clk_round_rate. clk_round_rate is no mandatory call at all. So, if
clk_set_rate adjusts to another rate than what clk_round_rate returned,
this is a bug.

Currently it's not clear to me why in clk_set_rate and clk_round_rate in
the divider different algorithms are used, I only notice that this is
the case since the introduction of the divider.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sören Brinkmann]

On Thu, Mar 21, 2013 at 10:15:31AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> > On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > > If the caller
> > > doesn't like the returned frequency he can request a different one.
> > > And he's eventually happy with the return value he calls
> > > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > > Always rounding down seems a bit odd to me.
> > > 
> > > Another issue with the current implmentation:
> > > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
> > 
> > And that is correct. clk_divider_bestdiv is used to calculate the
> > maximum parent frequency for which a given divider value does not
> > exceed the desired rate.
> The reason for that is that the (more?) usual constraint is like: This
> mmc card can handle up to 100 MHz. Or this i2c device can handle up to
> this and that frequency. Of course there are different constraints, e.g.
> for a UART if the target baud speed is 38400 you better run at 38402
> than at 19201.
> 
> I wonder if it depends on the clock if you want "best approximation <=
> requested value" or "best approximation" or on the caller. In the former
> case a flag for the clock would be the right thing (as suggested in this
> thread). If however it's the caller of round_rate who knows better which
> rounding is preferred than better extend the clk API.
> 
> Extending the API could just be a convenience function that doesn't
> affect the implementations of the clk API. E.g.:
> 
> 	long clk_round_rate_nearest(struct clk *clk, unsigned long rate)
> 	{
> 		long lower_limit = clk_round_rate(clk, rate);
> 		long upper_limit = clk_round_rate(clk, rate + (rate - lower_limit));
> 
> 		if (rate - lower_limit < upper_limit - rate)
> 			return lower_limit;
> 		else
> 			return upper_limit;
> 	}
> 
I guess both approaches may work. Anybody has a preference?

	Sören

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Mike Turquette]

Quoting Sören Brinkmann (2013-03-26 15:45:22)
> On Thu, Mar 21, 2013 at 10:15:31AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> > > On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > > > If the caller
> > > > doesn't like the returned frequency he can request a different one.
> > > > And he's eventually happy with the return value he calls
> > > > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > > > Always rounding down seems a bit odd to me.
> > > > 
> > > > Another issue with the current implmentation:
> > > > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
> > > 
> > > And that is correct. clk_divider_bestdiv is used to calculate the
> > > maximum parent frequency for which a given divider value does not
> > > exceed the desired rate.
> > The reason for that is that the (more?) usual constraint is like: This
> > mmc card can handle up to 100 MHz. Or this i2c device can handle up to
> > this and that frequency. Of course there are different constraints, e.g.
> > for a UART if the target baud speed is 38400 you better run at 38402
> > than at 19201.
> > 
> > I wonder if it depends on the clock if you want "best approximation <=
> > requested value" or "best approximation" or on the caller. In the former
> > case a flag for the clock would be the right thing (as suggested in this
> > thread). If however it's the caller of round_rate who knows better which
> > rounding is preferred than better extend the clk API.
> > 
> > Extending the API could just be a convenience function that doesn't
> > affect the implementations of the clk API. E.g.:
> > 
> >       long clk_round_rate_nearest(struct clk *clk, unsigned long rate)
> >       {
> >               long lower_limit = clk_round_rate(clk, rate);
> >               long upper_limit = clk_round_rate(clk, rate + (rate - lower_limit));
> > 
> >               if (rate - lower_limit < upper_limit - rate)
> >                       return lower_limit;
> >               else
> >                       return upper_limit;
> >       }
> > 
> I guess both approaches may work. Anybody has a preference?
> 

A dedicated function like the one Uwe defined is better than adding
subtlety to the existing clk_round_rate via a flag in a clock driver.

Regards,
Mike

>         Sören

Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST

[Sören Brinkmann]

On Tue, Mar 26, 2013 at 06:37:03PM -0700, Mike Turquette wrote:
> Quoting Sören Brinkmann (2013-03-26 15:45:22)
> > On Thu, Mar 21, 2013 at 10:15:31AM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Wed, Mar 20, 2013 at 07:50:51PM +0100, Sascha Hauer wrote:
> > > > On Wed, Mar 20, 2013 at 09:32:51AM -0700, Sören Brinkmann wrote:
> > > > > If the caller
> > > > > doesn't like the returned frequency he can request a different one.
> > > > > And he's eventually happy with the return value he calls
> > > > > clk_set_rate() requesting the frequency clk_round_rate() returned.
> > > > > Always rounding down seems a bit odd to me.
> > > > > 
> > > > > Another issue with the current implmentation:
> > > > > clk_divider_round_rate() calls clk_divider_bestdiv(), which uses the ROUND_UP macro, returning a rather low frequency.
> > > > 
> > > > And that is correct. clk_divider_bestdiv is used to calculate the
> > > > maximum parent frequency for which a given divider value does not
> > > > exceed the desired rate.
> > > The reason for that is that the (more?) usual constraint is like: This
> > > mmc card can handle up to 100 MHz. Or this i2c device can handle up to
> > > this and that frequency. Of course there are different constraints, e.g.
> > > for a UART if the target baud speed is 38400 you better run at 38402
> > > than at 19201.
> > > 
> > > I wonder if it depends on the clock if you want "best approximation <=
> > > requested value" or "best approximation" or on the caller. In the former
> > > case a flag for the clock would be the right thing (as suggested in this
> > > thread). If however it's the caller of round_rate who knows better which
> > > rounding is preferred than better extend the clk API.
> > > 
> > > Extending the API could just be a convenience function that doesn't
> > > affect the implementations of the clk API. E.g.:
> > > 
> > >       long clk_round_rate_nearest(struct clk *clk, unsigned long rate)
> > >       {
> > >               long lower_limit = clk_round_rate(clk, rate);
> > >               long upper_limit = clk_round_rate(clk, rate + (rate - lower_limit));
> > > 
> > >               if (rate - lower_limit < upper_limit - rate)
> > >                       return lower_limit;
> > >               else
> > >                       return upper_limit;
> > >       }
> > > 
> > I guess both approaches may work. Anybody has a preference?
> > 
> 
> A dedicated function like the one Uwe defined is better than adding
> subtlety to the existing clk_round_rate via a flag in a clock driver.
I looked at my problem again.

A new API function is probably fine for UART, ethernet drivers and
similar. Although, compared to a flag it would add some redundant
rounding, since clk_set_rate() implicitly also rounds the rate.
	clk_set_rate()
		clk_calc_new_rates()
			clk_round_rate()
But that is true for every driver which doesn't blindly call
clk_set_rate() and checks upfront through clk_round_rate() what
the actual frequency would look like.

So, do we agree to add this additional clk_round_rate_nearest()
function?
And if, should I just make Uwe's proposal another patch, additionally to
the other clk-divider change I'm working on?
Or Uwe, do you prefer to submit it yourself?


For my original problem, though, this is only part of a solution. It
appeared to be a rounding issue, but the actual root cause is the loss
of resolution when OPPs are converted to a frequency table for cpufreq.
I'm not sure how this can be resolved, yet.


	Sören