| Message ID | 20190617125238.13761-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: renesas: rcar-gen2/gen3: Switch to .determine_rate() | expand |
On Mon, Jun 17, 2019 at 02:52:34PM +0200, Geert Uytterhoeven wrote: > As the .round_rate() callback returns a long clock rate, it cannot > return clock rates that do not fit in signed long, but do fit in > unsigned long. Hence switch the Z clock on R-Car Gen2 from the old > .round_rate() callback to the newer .determine_rate() callback, which > does not suffer from this limitation. > > This includes implementing range checking. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/clk/renesas/clk-rcar-gen2.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/renesas/clk-rcar-gen2.c b/drivers/clk/renesas/clk-rcar-gen2.c > index da9fe3f032eb2a0d..f85837716d2840cf 100644 > --- a/drivers/clk/renesas/clk-rcar-gen2.c > +++ b/drivers/clk/renesas/clk-rcar-gen2.c > @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, > return div_u64((u64)parent_rate * mult, 32); > } > > -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, > - unsigned long *parent_rate) > +static int cpg_z_clk_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > { > - unsigned long prate = *parent_rate; > - unsigned int mult; > + unsigned long prate = req->best_parent_rate; > + unsigned int min_mult, max_mult, mult; > > - if (!prate) > - prate = 1; > + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); > + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); nit: the type of the second parameter doesn't look correct to me, div_u64 expects a u32 divisor. > + if (max_mult < min_mult) > + return -EINVAL; > > - mult = div_u64((u64)rate * 32, prate); > - mult = clamp(mult, 1U, 32U); > + mult = div_u64(req->rate * 32ULL, prate); Likewise, do we care that prate will be 64bit on 64bit platforms? (Yes, I know gen2 SoCs are 32bit :) > + mult = clamp(mult, min_mult, max_mult); > > - return *parent_rate / 32 * mult; > + req->rate = prate / 32 * mult; > + return 0; > } > > static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate, > @@ -129,7 +132,7 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate, > > static const struct clk_ops cpg_z_clk_ops = { > .recalc_rate = cpg_z_clk_recalc_rate, > - .round_rate = cpg_z_clk_round_rate, > + .determine_rate = cpg_z_clk_determine_rate, > .set_rate = cpg_z_clk_set_rate, > }; > > -- > 2.17.1 >
Hi Simon, On Tue, Jun 18, 2019 at 1:09 PM Simon Horman <horms@verge.net.au> wrote: > On Mon, Jun 17, 2019 at 02:52:34PM +0200, Geert Uytterhoeven wrote: > > As the .round_rate() callback returns a long clock rate, it cannot > > return clock rates that do not fit in signed long, but do fit in > > unsigned long. Hence switch the Z clock on R-Car Gen2 from the old > > .round_rate() callback to the newer .determine_rate() callback, which > > does not suffer from this limitation. > > > > This includes implementing range checking. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/clk/renesas/clk-rcar-gen2.c > > +++ b/drivers/clk/renesas/clk-rcar-gen2.c > > @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, > > return div_u64((u64)parent_rate * mult, 32); > > } > > > > -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, > > - unsigned long *parent_rate) > > +static int cpg_z_clk_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > { > > - unsigned long prate = *parent_rate; > > - unsigned int mult; > > + unsigned long prate = req->best_parent_rate; > > + unsigned int min_mult, max_mult, mult; > > > > - if (!prate) > > - prate = 1; > > + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); > > + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); > > nit: the type of the second parameter doesn't look correct to me, > div_u64 expects a u32 divisor. Yes, this should use div64_ul() instead. > > + if (max_mult < min_mult) > > + return -EINVAL; > > > > - mult = div_u64((u64)rate * 32, prate); > > - mult = clamp(mult, 1U, 32U); > > + mult = div_u64(req->rate * 32ULL, prate); > > Likewise, do we care that prate will be 64bit on 64bit platforms? > (Yes, I know gen2 SoCs are 32bit :) Likewise, div64_ul(). Thanks a lot! Gr{oetje,eeting}s, Geert
On Fri, Aug 30, 2019 at 10:43:01AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Tue, Jun 18, 2019 at 1:09 PM Simon Horman <horms@verge.net.au> wrote: > > On Mon, Jun 17, 2019 at 02:52:34PM +0200, Geert Uytterhoeven wrote: > > > As the .round_rate() callback returns a long clock rate, it cannot > > > return clock rates that do not fit in signed long, but do fit in > > > unsigned long. Hence switch the Z clock on R-Car Gen2 from the old > > > .round_rate() callback to the newer .determine_rate() callback, which > > > does not suffer from this limitation. > > > > > > This includes implementing range checking. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- a/drivers/clk/renesas/clk-rcar-gen2.c > > > +++ b/drivers/clk/renesas/clk-rcar-gen2.c > > > @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, > > > return div_u64((u64)parent_rate * mult, 32); > > > } > > > > > > -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, > > > - unsigned long *parent_rate) > > > +static int cpg_z_clk_determine_rate(struct clk_hw *hw, > > > + struct clk_rate_request *req) > > > { > > > - unsigned long prate = *parent_rate; > > > - unsigned int mult; > > > + unsigned long prate = req->best_parent_rate; > > > + unsigned int min_mult, max_mult, mult; > > > > > > - if (!prate) > > > - prate = 1; > > > + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); > > > + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); > > > > nit: the type of the second parameter doesn't look correct to me, > > div_u64 expects a u32 divisor. > > Yes, this should use div64_ul() instead. Ok, but in that case should the constants be "UL" instead of "UUL" ? > > > > + if (max_mult < min_mult) > > > + return -EINVAL; > > > > > > - mult = div_u64((u64)rate * 32, prate); > > > - mult = clamp(mult, 1U, 32U); > > > + mult = div_u64(req->rate * 32ULL, prate); > > > > Likewise, do we care that prate will be 64bit on 64bit platforms? > > (Yes, I know gen2 SoCs are 32bit :) > > Likewise, div64_ul(). > > Thanks a lot! > > 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 >
Hi Simon, On Mon, Sep 2, 2019 at 10:31 AM Simon Horman <horms@verge.net.au> wrote: > On Fri, Aug 30, 2019 at 10:43:01AM +0200, Geert Uytterhoeven wrote: > > On Tue, Jun 18, 2019 at 1:09 PM Simon Horman <horms@verge.net.au> wrote: > > > On Mon, Jun 17, 2019 at 02:52:34PM +0200, Geert Uytterhoeven wrote: > > > > As the .round_rate() callback returns a long clock rate, it cannot > > > > return clock rates that do not fit in signed long, but do fit in > > > > unsigned long. Hence switch the Z clock on R-Car Gen2 from the old > > > > .round_rate() callback to the newer .determine_rate() callback, which > > > > does not suffer from this limitation. > > > > > > > > This includes implementing range checking. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > --- a/drivers/clk/renesas/clk-rcar-gen2.c > > > > +++ b/drivers/clk/renesas/clk-rcar-gen2.c > > > > @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, > > > > return div_u64((u64)parent_rate * mult, 32); > > > > } > > > > > > > > -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, > > > > - unsigned long *parent_rate) > > > > +static int cpg_z_clk_determine_rate(struct clk_hw *hw, > > > > + struct clk_rate_request *req) > > > > { > > > > - unsigned long prate = *parent_rate; > > > > - unsigned int mult; > > > > + unsigned long prate = req->best_parent_rate; > > > > + unsigned int min_mult, max_mult, mult; > > > > > > > > - if (!prate) > > > > - prate = 1; > > > > + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); > > > > + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); > > > > > > nit: the type of the second parameter doesn't look correct to me, > > > div_u64 expects a u32 divisor. > > > > Yes, this should use div64_ul() instead. > > Ok, but in that case should the constants be "UL" instead of "UUL" ? The first or the second? ;-) The multiplication should always be calculated using 64-bit arithmetic, hence the first ULL suffix. The max() macro needs two parameters of the same type, and div64_ul() returns u64, hence the second ULL suffix. Gr{oetje,eeting}s, Geert
On Mon, Sep 02, 2019 at 10:44:25AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Mon, Sep 2, 2019 at 10:31 AM Simon Horman <horms@verge.net.au> wrote: > > On Fri, Aug 30, 2019 at 10:43:01AM +0200, Geert Uytterhoeven wrote: > > > On Tue, Jun 18, 2019 at 1:09 PM Simon Horman <horms@verge.net.au> wrote: > > > > On Mon, Jun 17, 2019 at 02:52:34PM +0200, Geert Uytterhoeven wrote: > > > > > As the .round_rate() callback returns a long clock rate, it cannot > > > > > return clock rates that do not fit in signed long, but do fit in > > > > > unsigned long. Hence switch the Z clock on R-Car Gen2 from the old > > > > > .round_rate() callback to the newer .determine_rate() callback, which > > > > > does not suffer from this limitation. > > > > > > > > > > This includes implementing range checking. > > > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > --- a/drivers/clk/renesas/clk-rcar-gen2.c > > > > > +++ b/drivers/clk/renesas/clk-rcar-gen2.c > > > > > @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, > > > > > return div_u64((u64)parent_rate * mult, 32); > > > > > } > > > > > > > > > > -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, > > > > > - unsigned long *parent_rate) > > > > > +static int cpg_z_clk_determine_rate(struct clk_hw *hw, > > > > > + struct clk_rate_request *req) > > > > > { > > > > > - unsigned long prate = *parent_rate; > > > > > - unsigned int mult; > > > > > + unsigned long prate = req->best_parent_rate; > > > > > + unsigned int min_mult, max_mult, mult; > > > > > > > > > > - if (!prate) > > > > > - prate = 1; > > > > > + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); > > > > > + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); > > > > > > > > nit: the type of the second parameter doesn't look correct to me, > > > > div_u64 expects a u32 divisor. > > > > > > Yes, this should use div64_ul() instead. > > > > Ok, but in that case should the constants be "UL" instead of "UUL" ? > > The first or the second? ;-) > > The multiplication should always be calculated using 64-bit arithmetic, > hence the first ULL suffix. > The max() macro needs two parameters of the same type, and > div64_ul() returns u64, hence the second ULL suffix. Thanks, I see that now.
diff --git a/drivers/clk/renesas/clk-rcar-gen2.c b/drivers/clk/renesas/clk-rcar-gen2.c index da9fe3f032eb2a0d..f85837716d2840cf 100644 --- a/drivers/clk/renesas/clk-rcar-gen2.c +++ b/drivers/clk/renesas/clk-rcar-gen2.c @@ -66,19 +66,22 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw, return div_u64((u64)parent_rate * mult, 32); } -static long cpg_z_clk_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int cpg_z_clk_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { - unsigned long prate = *parent_rate; - unsigned int mult; + unsigned long prate = req->best_parent_rate; + unsigned int min_mult, max_mult, mult; - if (!prate) - prate = 1; + min_mult = max(div_u64(req->min_rate * 32ULL, prate), 1ULL); + max_mult = min(div_u64(req->max_rate * 32ULL, prate), 32ULL); + if (max_mult < min_mult) + return -EINVAL; - mult = div_u64((u64)rate * 32, prate); - mult = clamp(mult, 1U, 32U); + mult = div_u64(req->rate * 32ULL, prate); + mult = clamp(mult, min_mult, max_mult); - return *parent_rate / 32 * mult; + req->rate = prate / 32 * mult; + return 0; } static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate, @@ -129,7 +132,7 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate, static const struct clk_ops cpg_z_clk_ops = { .recalc_rate = cpg_z_clk_recalc_rate, - .round_rate = cpg_z_clk_round_rate, + .determine_rate = cpg_z_clk_determine_rate, .set_rate = cpg_z_clk_set_rate, };
As the .round_rate() callback returns a long clock rate, it cannot return clock rates that do not fit in signed long, but do fit in unsigned long. Hence switch the Z clock on R-Car Gen2 from the old .round_rate() callback to the newer .determine_rate() callback, which does not suffer from this limitation. This includes implementing range checking. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/clk/renesas/clk-rcar-gen2.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)