Re: [PATCH] sh: clk: Relax clk rate match test

Re: [PATCH] sh: clk: Relax clk rate match test

[Geert Uytterhoeven]

Hi Jacopo,

CC linux-clk (yes I know this is about the legacy SH clock framework, but
the public API is/should be the same)

On Thu, Jan 25, 2018 at 12:24 PM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> When asking for a clk rate to be set, the sh core clock matches only
> exact rate values against the calculated frequency table entries. If the
> rate does not match exactly the test fails, and the whole frequency
> table is walked, resulting in selection of the last entry, corresponding to
> the lowest available clock rate.

IIUIC, the code does not select the last entry, but returns an error code,
which is propagated all the way up?

> Ie. when asking for a 10MHz clock rate on div6 clocks (ie. "video_clk" line),
> the calculated clock frequency 10088572 Hz gets ignored, and the clock is
> actually set to 5201920 Hz, which is the last available entry of the frequencies
> table.

Perhaps 5201920 is just the default (or old value)?

> Relax the clock frequency match test, allowing selection of clock rates
> immediately slower than the required one.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> ---
> Hello renesas lists,
>
> I'm now working on handling frame rate for the ov7720 image sensor to have that
> driver accepted as part of v4l2. The sensor is installed on on Migo-R board.
> In order to properly calculate pixel clock and the framerate I noticed the
> clock signal fed to the sensor from the SH7722 chip was always the lowest
> available one.
>
> This patch fixes the issues and allows me to properly select which clock
> frequency supply to the sensor, which according to datasheet does not support
> input clock frequencies slower than 10MHz (but works anyhow).
>
> As all patches for SH architecture I wonder where they should be picked up from,
> as SH seems not maintained at the moment.
>
> Thanks
>    j
>
> ---
>  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,
    };

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

> +
> +               return pos - freq_table;
> +       }
>
>         return -ENOENT;
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: clk: Relax clk rate match test

[jacopo mondi]

Hi Geert,

On Thu, Jan 25, 2018 at 02:53:41PM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> CC linux-clk (yes I know this is about the legacy SH clock framework, but
> the public API is/should be the same)
>
> On Thu, Jan 25, 2018 at 12:24 PM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
> > When asking for a clk rate to be set, the sh core clock matches only
> > exact rate values against the calculated frequency table entries. If the
> > rate does not match exactly the test fails, and the whole frequency
> > table is walked, resulting in selection of the last entry, corresponding to
> > the lowest available clock rate.
>
> IIUIC, the code does not select the last entry, but returns an error code,
> which is propagated all the way up?
>
> > Ie. when asking for a 10MHz clock rate on div6 clocks (ie. "video_clk" line),
> > the calculated clock frequency 10088572 Hz gets ignored, and the clock is
> > actually set to 5201920 Hz, which is the last available entry of the frequencies
> > table.
>
> Perhaps 5201920 is just the default (or old value)?
>
> > Relax the clock frequency match test, allowing selection of clock rates
> > immediately slower than the required one.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > Hello renesas lists,
> >
> > I'm now working on handling frame rate for the ov7720 image sensor to have that
> > driver accepted as part of v4l2. The sensor is installed on on Migo-R board.
> > In order to properly calculate pixel clock and the framerate I noticed the
> > clock signal fed to the sensor from the SH7722 chip was always the lowest
> > available one.
> >
> > This patch fixes the issues and allows me to properly select which clock
> > frequency supply to the sensor, which according to datasheet does not support
> > input clock frequencies slower than 10MHz (but works anyhow).
> >
> > As all patches for SH architecture I wonder where they should be picked up from,
> > as SH seems not maintained at the moment.
> >
> > Thanks
> >    j
> >
> > ---
> >  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?

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

The 10MHz seems like a workaround put in place to remove some
image quality issues, according to the comment:
https://elixir.free-electrons.com/linux/v4.15-rc9/source/arch/sh/boards/mach-migor/setup.c#L311

Thanks
   j

> > +
> > +               return pos - freq_table;
> > +       }
> >
> >         return -ENOENT;
> >  }
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] sh: clk: Relax clk rate match test

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Jan 25, 2018 at 3:14 PM, jacopo mondi <jacopo@jmondi.org> wrote:
> On Thu, Jan 25, 2018 at 02:53:41PM +0100, Geert Uytterhoeven wrote:
>> CC linux-clk (yes I know this is about the legacy SH clock framework, but
>> the public API is/should be the same)
>>
>> On Thu, Jan 25, 2018 at 12:24 PM, Jacopo Mondi
>> <jacopo+renesas@jmondi.org> wrote:
>> > When asking for a clk rate to be set, the sh core clock matches only
>> > exact rate values against the calculated frequency table entries. If the
>> > rate does not match exactly the test fails, and the whole frequency
>> > table is walked, resulting in selection of the last entry, corresponding to
>> > the lowest available clock rate.
>>
>> IIUIC, the code does not select the last entry, but returns an error code,
>> which is propagated all the way up?
>>
>> > Ie. when asking for a 10MHz clock rate on div6 clocks (ie. "video_clk" line),
>> > the calculated clock frequency 10088572 Hz gets ignored, and the clock is
>> > actually set to 5201920 Hz, which is the last available entry of the frequencies
>> > table.
>>
>> Perhaps 5201920 is just the default (or old value)?
>>
>> > Relax the clock frequency match test, allowing selection of clock rates
>> > immediately slower than the required one.
>> >
>> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> >
>> > ---
>> > Hello renesas lists,
>> >
>> > I'm now working on handling frame rate for the ov7720 image sensor to have that
>> > driver accepted as part of v4l2. The sensor is installed on on Migo-R board.
>> > In order to properly calculate pixel clock and the framerate I noticed the
>> > clock signal fed to the sensor from the SH7722 chip was always the lowest
>> > available one.
>> >
>> > This patch fixes the issues and allows me to properly select which clock
>> > frequency supply to the sensor, which according to datasheet does not support
>> > input clock frequencies slower than 10MHz (but works anyhow).
>> >
>> > As all patches for SH architecture I wonder where they should be picked up from,
>> > as SH seems not maintained at the moment.
>> >
>> > Thanks
>> >    j
>> >
>> > ---
>> >  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.

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: clk: Relax clk rate match test

[jacopo mondi]

Hi Geert,

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.

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 ;)

Thanks
   j

> >> (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.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] sh: clk: Relax clk rate match test

[Laurent Pinchart]

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