Hi Jacopo,
On Friday, 26 January 2018 18:24:54 EET jacopo mondi wrote:
> On Thu, Jan 25, 2018 at 03:39:32PM +0100, Geert Uytterhoeven wrote:
> > Hi Jacopo,
>
> [snip]
>
> >>>> ---
> >>>>
> >>>> drivers/sh/clk/core.c | 9 ++++++---
> >>>> 1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
> >>>> index 92863e3..d2cb94c 100644
> >>>> --- a/drivers/sh/clk/core.c
> >>>> +++ b/drivers/sh/clk/core.c
> >>>> @@ -198,9 +198,12 @@ int clk_rate_table_find(struct clk *clk,
> >>>> {
> >>>> struct cpufreq_frequency_table *pos;
> >>>>
> >>>> - cpufreq_for_each_valid_entry(pos, freq_table)
> >>>> - if (pos->frequency == rate)
> >>>> - return pos - freq_table;
> >>>> + cpufreq_for_each_valid_entry(pos, freq_table) {
> >>>> + if (pos->frequency > rate)
> >>>> + continue;
> >>>
> >>> This assumes all frequency tables are sorted.
> >>>
> >>> Shouldn't you pick the closest frequency?
> >>>
> >>> However, that's what clk_rate_table_round() does, which is called from
> >>>
> >>> sh_clk_div_round_rate(), and thus already used as .round_rate:
> >>>
> >>> static struct sh_clk_ops sh_clk_div_enable_clk_ops = {
> >>> .recalc = sh_clk_div_recalc,
> >>> .set_rate = sh_clk_div_set_rate,
> >>> .round_rate = sh_clk_div_round_rate,
> >>> .enable = sh_clk_div_enable,
> >>> .disable = sh_clk_div_disable,
> >>>
> >>> };
> >>
> >> Does this implies clock rates should be set using clk_round_rate() and
> >> not clk_set_rate() if I understand this right?
> >
> > Not necessarily...
> >
> > Note that both cpg_div6_clock_round_rate() and cpg_div6_clock_set_rate()
> > in the CCF implementation for DIV6 clocks use rounding.
>
> Yeah but it doesn't seem to me that CCF implementation for DIV6 clocks does
> have to walk static tables like the old sh clock driver does. They
> perform rounding, but on the clock dividers given a requested rate
> and the respective parent clock, if I'm not wrong.
While clk_set_rate() doesn't explicitly document that the rate will be
rounded, the clk_round_rate() documentation does:
/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
* @rate: desired clock rate in Hz
*
* This answers the question "if I were to pass @rate to clk_set_rate(),
* what clock rate would I end up with?" without changing the hardware
* in any way. In other words:
*
* rate = clk_round_rate(clk, r);
*
* and:
*
* clk_set_rate(clk, r);
* rate = clk_get_rate(clk);
*
* are equivalent except the former does not modify the clock hardware
* in any way.
*
* Returns rounded clock rate in Hz, or negative errno.
*/
So I think the SH implementation of clk_set_rate() should round rates.
(And feel free to send a patch for the clk_set_rate() documentation in
include/linux/clk.h to state explicitly that the rate will be rounded).
> Anyway, in this case a much simpler:
> clk_set_rate(video_clk, clk_round_rate(video_clk, 10000000));
> does the job for Migo-R.
>
> I will include this in next CEU iterations, since I already have a
> small comment from you to fix there ;)
That should not be needed, but if the above code is in a board file, I can
live with that until drivers/sh/clk/ gets fixed.
> >>> (clk_rate_table_find() is called from sh_clk_div_set_rate())
> >>>
> >>> Or are you supposed to ask for the exact clock rate? Where does the 10
> >>> MHz come from?
> >>
> >> From board initialization code, in order to provide a valid input
> >> clock to OV7720 sensor.
--
Regards,
Laurent Pinchart
