Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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
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-ker...@lists.infradead.org > > > > > Cc: linux-kernel@vger.kernel.org > > > > > Signed-off-by: Soren Brinkmann > > > > > --- > > > > > 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:133320, rate:0, 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 > > > > > 22 33 66 > > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > > > 66 > > > > > > > > > > So far, so good. But now, let's scale the frequency: > > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > > > > > > scaling_setspeed > > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > > > 26 > > > > > > > > > > And the corresponding debug line: > > > > > prate:133320, rate:33000, div:5 > > > > > > > > > > So, with DIV_ROUND_UP an actual divider of 4.0396 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- | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to
Re: [PATCH] clk: divider: Use DIV_ROUND_CLOSEST
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-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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 22 33 66 zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 66 So far, so good. But now, let's scale the frequency: zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 26 And the corresponding debug line: prate:133320, rate:33000, div:5 So, with DIV_ROUND_UP an actual divider of 4.0396 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- | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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
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-ker...@lists.infradead.org > > > > Cc: linux-kernel@vger.kernel.org > > > > Signed-off-by: Soren Brinkmann > > > > --- > > > > 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:133320, rate:0, 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 > > > > 22 33 66 > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > > 66 > > > > > > > > So far, so good. But now, let's scale the frequency: > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > > > > > scaling_setspeed > > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > > 26 > > > > > > > > And the corresponding debug line: > > > > prate:133320, rate:33000, div:5 > > > > > > > > So, with DIV_ROUND_UP an actual divider of 4.0396 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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/ |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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
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-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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 22 33 66 zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 66 So far, so good. But now, let's scale the frequency: zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 26 And the corresponding debug line: prate:133320, rate:33000, div:5 So, with DIV_ROUND_UP an actual divider of 4.0396 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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
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-ker...@lists.infradead.org > > > Cc: linux-kernel@vger.kernel.org > > > Signed-off-by: Soren Brinkmann > > > --- > > > 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:133320, rate:0, 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 > > > 22 33 66 > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > 66 > > > > > > So far, so good. But now, let's scale the frequency: > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > scaling_setspeed > > > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq > > > 26 > > > > > > And the corresponding debug line: > > > prate:133320, rate:33000, div:5 > > > > > > So, with DIV_ROUND_UP an actual divider of 4.0396 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- | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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-ker...@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Soren Brinkmann
> > ---
> > 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:133320, rate:0, 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
> > 22 33 66
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > 66
> >
> > So far, so good. But now, let's scale the frequency:
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > scaling_setspeed
> > zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> > 26
> >
> > And the corresponding debug line:
> > prate:133320, rate:33000, div:5
> >
> > So, with DIV_ROUND_UP an actual divider of 4.0396 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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
22 33 66
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
66
So far, so good. But now, let's scale the frequency:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
26
And the corresponding debug line:
prate:133320, rate:33000, div:5
So, with DIV_ROUND_UP an actual divider of 4.0396 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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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
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-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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 22 33 66 zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 66 So far, so good. But now, let's scale the frequency: zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq 26 And the corresponding debug line: prate:133320, rate:33000, div:5 So, with DIV_ROUND_UP an actual divider of 4.0396 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- | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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
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-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Soren Brinkmann
> ---
> 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:133320, rate:0, 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
> 22 33 66
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> 66
>
> So far, so good. But now, let's scale the frequency:
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > scaling_setspeed
> zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
> 26
>
> And the corresponding debug line:
> prate:133320, rate:33000, div:5
>
> So, with DIV_ROUND_UP an actual divider of 4.0396 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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
22 33 66
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
66
So far, so good. But now, let's scale the frequency:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
26
And the corresponding debug line:
prate:133320, rate:33000, div:5
So, with DIV_ROUND_UP an actual divider of 4.0396 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] clk: divider: Use DIV_ROUND_CLOSEST
Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
rounding errors.
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Soren Brinkmann
---
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:133320, rate:0, 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
22 33 66
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
66
So far, so good. But now, let's scale the frequency:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 > scaling_setspeed
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
26
And the corresponding debug line:
prate:133320, rate:33000, div:5
So, with DIV_ROUND_UP an actual divider of 4.0396 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] clk: divider: Use DIV_ROUND_CLOSEST
Use the DIV_ROUND_CLOSEST macro to calculate divider values and minimize
rounding errors.
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Soren Brinkmann soren.brinkm...@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:133320, rate:0, 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
22 33 66
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
66
So far, so good. But now, let's scale the frequency:
zynq:/sys/devices/system/cpu/cpu0/cpufreq # echo 33 scaling_setspeed
zynq:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_cur_freq
26
And the corresponding debug line:
prate:133320, rate:33000, div:5
So, with DIV_ROUND_UP an actual divider of 4.0396 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
