| Message ID | 2e77dfe8-8d4f-4b77-bf1b-831ad1572c23@VA3EHSMHS046.ehs.local |
|---|---|
| State | New |
| Headers | show |
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/ >
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. 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. Care to take a crack at that approach? 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
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.
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
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
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.
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
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
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
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
Quoting Sören Brinkmann (2013-04-01 16:24:12) > 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? Cc'ing Russell if he has any comments on adding clk_round_rate_nearest, since he is author of the original clk api. Regards, Mike > 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
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))
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(-)