From: Simon Horman <horms@verge.net.au>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
linux-clk <linux-clk@vger.kernel.org>,
Takeshi Kihara <takeshi.kihara.df@renesas.com>
Subject: Re: [PATCH] clk: renesas: rcar-gen3: Fix cpg_sd_clock_round_rate() return value
Date: Fri, 29 Mar 2019 11:37:27 +0100 [thread overview]
Message-ID: <20190329103725.y2kbdjsza5vytgen@verge.net.au> (raw)
In-Reply-To: <CAMuHMdXL1s3ZyaZnB2JJM=Dx23uAN_6Pz5_FmLP+WSSywH9FNg@mail.gmail.com>
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!
prev parent reply other threads:[~2019-03-29 10:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190329103725.y2kbdjsza5vytgen@verge.net.au \
--to=horms@verge.net.au \
--cc=geert+renesas@glider.be \
--cc=geert@linux-m68k.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=sboyd@kernel.org \
--cc=takeshi.kihara.df@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).