* [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value
@ 2019-03-27 12:31 Geert Uytterhoeven
2019-03-29 8:45 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Geert Uytterhoeven @ 2019-03-27 12:31 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-renesas-soc, linux-clk, Takeshi Kihara, Geert Uytterhoeven
From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
cpg_sd_clock_round_rate() may return an unsupported clock rate for the
requested clock rate. Therefore, when cpg_sd_clock_set_rate() sets the
clock rate acquired by cpg_sd_clock_round_rate(), an error may occur.
This is not conform the clk API design.
This patch fixes that by making sure cpg_sd_clock_calc_div() considers
only the division values defined in cpg_sd_div_table[].
With this fix, the cpg_sd_clock_round_rate() always return a support
clock rate.
Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
Fixes: 90c073e53909da85 ("clk: shmobile: r8a7795: Add SD divider support")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
To be queued in clk-renesas-for-v5.2.
drivers/clk/renesas/rcar-gen3-cpg.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index d5fb768b089ff1c1..d2745c57207efc01 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -3,6 +3,7 @@
* R-Car Gen3 Clock Pulse Generator
*
* Copyright (C) 2015-2018 Glider bvba
+ * Copyright (C) 2019 Renesas Electronics Corp.
*
* Based on clk-rcar-gen3.c
*
@@ -236,8 +237,6 @@ struct sd_clock {
const struct sd_div_table *div_table;
struct cpg_simple_notifier csn;
unsigned int div_num;
- unsigned int div_min;
- unsigned int div_max;
unsigned int cur_div_idx;
};
@@ -314,14 +313,20 @@ static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
unsigned long rate,
unsigned long parent_rate)
{
- unsigned int div;
-
- if (!rate)
- rate = 1;
+ unsigned long calc_rate, best_rate = 0, diff, diff_min = ULONG_MAX;
+ unsigned int i;
- div = DIV_ROUND_CLOSEST(parent_rate, rate);
+ for (i = 0; i < clock->div_num; i++) {
+ calc_rate = DIV_ROUND_CLOSEST(parent_rate,
+ clock->div_table[i].div);
+ diff = calc_rate > rate ? calc_rate - rate : rate - calc_rate;
+ if (diff <= diff_min) {
+ best_rate = calc_rate;
+ diff_min = diff;
+ }
+ }
- return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
+ return DIV_ROUND_CLOSEST(parent_rate, best_rate);
}
static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
@@ -405,13 +410,6 @@ static struct clk * __init cpg_sd_clk_register(const char *name,
val |= CPG_SD_STP_MASK | (clock->div_table[0].val & CPG_SD_FC_MASK);
writel(val, clock->csn.reg);
- clock->div_max = clock->div_table[0].div;
- clock->div_min = clock->div_max;
- for (i = 1; i < clock->div_num; i++) {
- clock->div_max = max(clock->div_max, clock->div_table[i].div);
- clock->div_min = min(clock->div_min, clock->div_table[i].div);
- }
-
clk = clk_register(NULL, &clock->hw);
if (IS_ERR(clk))
goto free_clock;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value
2019-03-27 12:31 [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value Geert Uytterhoeven
@ 2019-03-29 8:45 ` Simon Horman
2019-03-29 8:58 ` Geert Uytterhoeven
0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2019-03-29 8:45 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, linux-renesas-soc, linux-clk,
Takeshi Kihara
On Wed, Mar 27, 2019 at 01:31:33PM +0100, Geert Uytterhoeven wrote:
> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>
> cpg_sd_clock_round_rate() may return an unsupported clock rate for the
> requested clock rate. Therefore, when cpg_sd_clock_set_rate() sets the
> clock rate acquired by cpg_sd_clock_round_rate(), an error may occur.
>
> This is not conform the clk API design.
>
> This patch fixes that by making sure cpg_sd_clock_calc_div() considers
> only the division values defined in cpg_sd_div_table[].
> With this fix, the cpg_sd_clock_round_rate() always return a support
> clock rate.
>
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Fixes: 90c073e53909da85 ("clk: shmobile: r8a7795: Add SD divider support")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> To be queued in clk-renesas-for-v5.2.
>
> drivers/clk/renesas/rcar-gen3-cpg.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> index d5fb768b089ff1c1..d2745c57207efc01 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -3,6 +3,7 @@
> * R-Car Gen3 Clock Pulse Generator
> *
> * Copyright (C) 2015-2018 Glider bvba
> + * Copyright (C) 2019 Renesas Electronics Corp.
> *
> * Based on clk-rcar-gen3.c
> *
> @@ -236,8 +237,6 @@ struct sd_clock {
> const struct sd_div_table *div_table;
> struct cpg_simple_notifier csn;
> unsigned int div_num;
> - unsigned int div_min;
> - unsigned int div_max;
> unsigned int cur_div_idx;
> };
>
> @@ -314,14 +313,20 @@ static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
> unsigned long rate,
> unsigned long parent_rate)
> {
> - unsigned int div;
> -
> - if (!rate)
> - rate = 1;
> + unsigned long calc_rate, best_rate = 0, diff, diff_min = ULONG_MAX;
> + unsigned int i;
>
> - div = DIV_ROUND_CLOSEST(parent_rate, rate);
> + for (i = 0; i < clock->div_num; i++) {
> + calc_rate = DIV_ROUND_CLOSEST(parent_rate,
> + clock->div_table[i].div);
> + diff = calc_rate > rate ? calc_rate - rate : rate - calc_rate;
> + if (diff <= diff_min) {
> + best_rate = calc_rate;
> + diff_min = diff;
> + }
> + }
My reading is that.
a) this algorithm picks a clock based on the div field of the
members of cpg_sd_div_table. and
b) in the case of duplicate values of that field the first one is chosen; and
c) such duplicates (of sd_div values) do exist in cpg_sd_div_table
Is this the intended behaviour?
>
> - return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
> + return DIV_ROUND_CLOSEST(parent_rate, best_rate);
Would it be better to return the value of clock->div_table[i].div
that yielded best_rate?
> }
>
> static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
> @@ -405,13 +410,6 @@ static struct clk * __init cpg_sd_clk_register(const char *name,
> val |= CPG_SD_STP_MASK | (clock->div_table[0].val & CPG_SD_FC_MASK);
> writel(val, clock->csn.reg);
>
> - clock->div_max = clock->div_table[0].div;
> - clock->div_min = clock->div_max;
> - for (i = 1; i < clock->div_num; i++) {
> - clock->div_max = max(clock->div_max, clock->div_table[i].div);
> - clock->div_min = min(clock->div_min, clock->div_table[i].div);
> - }
> -
> clk = clk_register(NULL, &clock->hw);
> if (IS_ERR(clk))
> goto free_clock;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value
2019-03-29 8:45 ` Simon Horman
@ 2019-03-29 8:58 ` Geert Uytterhoeven
2019-03-29 10:37 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Geert Uytterhoeven @ 2019-03-29 8:58 UTC (permalink / raw)
To: Simon Horman
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
Linux-Renesas, linux-clk, Takeshi Kihara
Hi Simon,
On Fri, Mar 29, 2019 at 9:45 AM Simon Horman <horms@verge.net.au> wrote:
> On Wed, Mar 27, 2019 at 01:31:33PM +0100, Geert Uytterhoeven wrote:
> > From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> >
> > cpg_sd_clock_round_rate() may return an unsupported clock rate for the
> > requested clock rate. Therefore, when cpg_sd_clock_set_rate() sets the
> > clock rate acquired by cpg_sd_clock_round_rate(), an error may occur.
> >
> > This is not conform the clk API design.
> >
> > This patch fixes that by making sure cpg_sd_clock_calc_div() considers
> > only the division values defined in cpg_sd_div_table[].
> > With this fix, the cpg_sd_clock_round_rate() always return a support
> > clock rate.
> >
> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> > Fixes: 90c073e53909da85 ("clk: shmobile: r8a7795: Add SD divider support")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > To be queued in clk-renesas-for-v5.2.
> >
> > drivers/clk/renesas/rcar-gen3-cpg.c | 28 +++++++++++++---------------
> > 1 file changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > index d5fb768b089ff1c1..d2745c57207efc01 100644
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > @@ -3,6 +3,7 @@
> > * R-Car Gen3 Clock Pulse Generator
> > *
> > * Copyright (C) 2015-2018 Glider bvba
> > + * Copyright (C) 2019 Renesas Electronics Corp.
> > *
> > * Based on clk-rcar-gen3.c
> > *
> > @@ -236,8 +237,6 @@ struct sd_clock {
> > const struct sd_div_table *div_table;
> > struct cpg_simple_notifier csn;
> > unsigned int div_num;
> > - unsigned int div_min;
> > - unsigned int div_max;
> > unsigned int cur_div_idx;
> > };
> >
> > @@ -314,14 +313,20 @@ static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
> > unsigned long rate,
> > unsigned long parent_rate)
> > {
> > - unsigned int div;
> > -
> > - if (!rate)
> > - rate = 1;
> > + unsigned long calc_rate, best_rate = 0, diff, diff_min = ULONG_MAX;
> > + unsigned int i;
> >
> > - div = DIV_ROUND_CLOSEST(parent_rate, rate);
> > + for (i = 0; i < clock->div_num; i++) {
> > + calc_rate = DIV_ROUND_CLOSEST(parent_rate,
> > + clock->div_table[i].div);
> > + diff = calc_rate > rate ? calc_rate - rate : rate - calc_rate;
> > + if (diff <= diff_min) {
> > + best_rate = calc_rate;
> > + diff_min = diff;
> > + }
> > + }
>
> My reading is that.
> a) this algorithm picks a clock based on the div field of the
> members of cpg_sd_div_table. and
> b) in the case of duplicate values of that field the first one is chosen; and
> c) such duplicates (of sd_div values) do exist in cpg_sd_div_table
>
> Is this the intended behaviour?
Actually cpg_sd_clock_calc_div() picks the last one, due to the "<=" :-)
But that doesn't matter here, as both dividers are equal.
cpg_sd_clock_set_rate() will pick the first entry from the table with matching
div value later.
> > - return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
> > + return DIV_ROUND_CLOSEST(parent_rate, best_rate);
>
> Would it be better to return the value of clock->div_table[i].div
> that yielded best_rate?
True. The DIV_ROUND_CLOSEST() is not needed, as we could have
saved best_div instead of best_rate. Will fix.
Thanks!
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value
2019-03-29 8:58 ` Geert Uytterhoeven
@ 2019-03-29 10:37 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2019-03-29 10:37 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
Linux-Renesas, linux-clk, Takeshi Kihara
On Fri, Mar 29, 2019 at 09:58:24AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
>
> On Fri, Mar 29, 2019 at 9:45 AM Simon Horman <horms@verge.net.au> wrote:
> > On Wed, Mar 27, 2019 at 01:31:33PM +0100, Geert Uytterhoeven wrote:
> > > From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> > >
> > > cpg_sd_clock_round_rate() may return an unsupported clock rate for the
> > > requested clock rate. Therefore, when cpg_sd_clock_set_rate() sets the
> > > clock rate acquired by cpg_sd_clock_round_rate(), an error may occur.
> > >
> > > This is not conform the clk API design.
> > >
> > > This patch fixes that by making sure cpg_sd_clock_calc_div() considers
> > > only the division values defined in cpg_sd_div_table[].
> > > With this fix, the cpg_sd_clock_round_rate() always return a support
> > > clock rate.
> > >
> > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> > > Fixes: 90c073e53909da85 ("clk: shmobile: r8a7795: Add SD divider support")
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > To be queued in clk-renesas-for-v5.2.
> > >
> > > drivers/clk/renesas/rcar-gen3-cpg.c | 28 +++++++++++++---------------
> > > 1 file changed, 13 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > > index d5fb768b089ff1c1..d2745c57207efc01 100644
> > > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > > @@ -3,6 +3,7 @@
> > > * R-Car Gen3 Clock Pulse Generator
> > > *
> > > * Copyright (C) 2015-2018 Glider bvba
> > > + * Copyright (C) 2019 Renesas Electronics Corp.
> > > *
> > > * Based on clk-rcar-gen3.c
> > > *
> > > @@ -236,8 +237,6 @@ struct sd_clock {
> > > const struct sd_div_table *div_table;
> > > struct cpg_simple_notifier csn;
> > > unsigned int div_num;
> > > - unsigned int div_min;
> > > - unsigned int div_max;
> > > unsigned int cur_div_idx;
> > > };
> > >
> > > @@ -314,14 +313,20 @@ static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
> > > unsigned long rate,
> > > unsigned long parent_rate)
> > > {
> > > - unsigned int div;
> > > -
> > > - if (!rate)
> > > - rate = 1;
> > > + unsigned long calc_rate, best_rate = 0, diff, diff_min = ULONG_MAX;
> > > + unsigned int i;
> > >
> > > - div = DIV_ROUND_CLOSEST(parent_rate, rate);
> > > + for (i = 0; i < clock->div_num; i++) {
> > > + calc_rate = DIV_ROUND_CLOSEST(parent_rate,
> > > + clock->div_table[i].div);
> > > + diff = calc_rate > rate ? calc_rate - rate : rate - calc_rate;
> > > + if (diff <= diff_min) {
> > > + best_rate = calc_rate;
> > > + diff_min = diff;
> > > + }
> > > + }
> >
> > My reading is that.
> > a) this algorithm picks a clock based on the div field of the
> > members of cpg_sd_div_table. and
> > b) in the case of duplicate values of that field the first one is chosen; and
> > c) such duplicates (of sd_div values) do exist in cpg_sd_div_table
> >
> > Is this the intended behaviour?
>
> Actually cpg_sd_clock_calc_div() picks the last one, due to the "<=" :-)
> But that doesn't matter here, as both dividers are equal.
True :)
> cpg_sd_clock_set_rate() will pick the first entry from the table with matching
> div value later.
>
> > > - return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
> > > + return DIV_ROUND_CLOSEST(parent_rate, best_rate);
> >
> > Would it be better to return the value of clock->div_table[i].div
> > that yielded best_rate?
>
> True. The DIV_ROUND_CLOSEST() is not needed, as we could have
> saved best_div instead of best_rate. Will fix.
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-29 10:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-27 12:31 [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value Geert Uytterhoeven
2019-03-29 8:45 ` Simon Horman
2019-03-29 8:58 ` Geert Uytterhoeven
2019-03-29 10:37 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).