[PATCH 0/5] clk: renesas: rcar-gen2/gen3: Switch to .determine_rate()

[PATCH 0/5] clk: renesas: rcar-gen2/gen3: Switch to .determine_rate()

[Geert Uytterhoeven]

	Hi Mike, Stephen,

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.  The newer .determine_rate() callback does not suffer
from this limitation.  In addition, .determine_rate() provides the
ability to specify a rate range.

Hence this patch series switches the Z (CPU) and SD clocks in the R-Car
Gen2 and Gen3 clock drivers from the .round_rate() to the
.determine_rate() callback.

Note that the "div6" clock driver hasn't been converted yet, so div6
clocks still use .round_rate().

This has been tested on R-Car M2-W and R-Car M3-N, and should have no
behavioral impact.

To be queued in clk-renesas-for-v5.3, if approved.

Thanks for your comments!

Geert Uytterhoeven (5):
  clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()
  clk: renesas: rcar-gen2: Switch Z clock to .determine_rate()
  clk: renesas: rcar-gen3: Switch Z clocks to .determine_rate()
  clk: renesas: rcar-gen3: Avoid double table iteration in SD
    .set_rate()
  clk: renesas: rcar-gen3: Switch SD clocks to .determine_rate()

 drivers/clk/renesas/clk-rcar-gen2.c | 23 ++++++-----
 drivers/clk/renesas/rcar-gen2-cpg.c | 23 ++++++-----
 drivers/clk/renesas/rcar-gen3-cpg.c | 64 ++++++++++++++++-------------
 3 files changed, 61 insertions(+), 49 deletions(-)

-- 
2.17.1

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

[PATCH 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Geert Uytterhoeven]

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);
+	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,
 };
 
-- 
2.17.1

[PATCH 2/5] clk: renesas: rcar-gen2: Switch Z clock to .determine_rate()

[Geert Uytterhoeven]

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/rcar-gen2-cpg.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen2-cpg.c b/drivers/clk/renesas/rcar-gen2-cpg.c
index f596a2dafcf4d8d1..282266b32c51a36b 100644
--- a/drivers/clk/renesas/rcar-gen2-cpg.c
+++ b/drivers/clk/renesas/rcar-gen2-cpg.c
@@ -63,19 +63,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,
@@ -126,7 +129,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

[PATCH 3/5] clk: renesas: rcar-gen3: Switch Z clocks to .determine_rate()

[Geert Uytterhoeven]

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 clocks on R-Car Gen3 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/rcar-gen3-cpg.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index d25c8ba00a656841..2268f44ccb8fe9bc 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -114,18 +114,24 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
 				     32 * zclk->fixed_div);
 }
 
-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)
 {
 	struct cpg_z_clk *zclk = to_z_clk(hw);
+	unsigned int min_mult, max_mult, mult;
 	unsigned long prate;
-	unsigned int mult;
 
-	prate = *parent_rate / zclk->fixed_div;
-	mult = div_u64(rate * 32ULL, prate);
-	mult = clamp(mult, 1U, 32U);
+	prate = req->best_parent_rate / zclk->fixed_div;
+	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;
 
-	return (u64)prate * mult / 32;
+	mult = div_u64(req->rate * 32ULL, prate);
+	mult = clamp(mult, min_mult, max_mult);
+
+	req->rate = (u64)prate * mult / 32;
+	return 0;
 }
 
 static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -172,7 +178,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

[PATCH 4/5] clk: renesas: rcar-gen3: Avoid double table iteration in SD .set_rate()

[Geert Uytterhoeven]

The .set_rate() callback for the SD clocks is always called with a valid
clock rate, returned by .round_rate().  Hence there is no need to
iterate through the divider table twice: once to repeat the work done by
.round_rate(), and a second time to find the corresponding divider
entry.

Just iterate once, looking for the divider that matches the passed clock
rate.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 2268f44ccb8fe9bc..387b22f7d1755d02 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -345,14 +345,14 @@ static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
 }
 
 static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
-				   unsigned long parent_rate)
+				 unsigned long parent_rate)
 {
 	struct sd_clock *clock = to_sd_clock(hw);
-	unsigned int div = cpg_sd_clock_calc_div(clock, rate, parent_rate);
 	unsigned int i;
 
 	for (i = 0; i < clock->div_num; i++)
-		if (div == clock->div_table[i].div)
+		if (rate == DIV_ROUND_CLOSEST(parent_rate,
+					      clock->div_table[i].div))
 			break;
 
 	if (i >= clock->div_num)
-- 
2.17.1

[PATCH 5/5] clk: renesas: rcar-gen3: Switch SD clocks to .determine_rate()

[Geert Uytterhoeven]

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 SD clocks on R-Car Gen3 from the old
.round_rate() callback to the newer .determine_rate() callback, which
does not suffer from this limitation.

This includes implementing range checking.

Absorb cpg_sd_clock_calc_div() into cpg_sd_clock_determine_rate(), and
replace the best divider by the best rate to avoid repeating one
division.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 36 ++++++++++++++---------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 387b22f7d1755d02..9592e6e47ef330b5 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -315,33 +315,33 @@ static unsigned long cpg_sd_clock_recalc_rate(struct clk_hw *hw,
 				 clock->div_table[clock->cur_div_idx].div);
 }
 
-static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
-					  unsigned long rate,
-					  unsigned long parent_rate)
+static int cpg_sd_clock_determine_rate(struct clk_hw *hw,
+				       struct clk_rate_request *req)
 {
-	unsigned long calc_rate, diff, diff_min = ULONG_MAX;
-	unsigned int i, best_div = 0;
+	unsigned long best_rate = ULONG_MAX, diff_min = ULONG_MAX;
+	unsigned long calc_rate, diff;
+	struct sd_clock *clock = to_sd_clock(hw);
+	unsigned int i;
 
 	for (i = 0; i < clock->div_num; i++) {
-		calc_rate = DIV_ROUND_CLOSEST(parent_rate,
+		calc_rate = DIV_ROUND_CLOSEST(req->best_parent_rate,
 					      clock->div_table[i].div);
-		diff = calc_rate > rate ? calc_rate - rate : rate - calc_rate;
+		if (calc_rate < req->min_rate || calc_rate > req->max_rate)
+			continue;
+
+		diff = calc_rate > req->rate ? calc_rate - req->rate
+					     : req->rate - calc_rate;
 		if (diff < diff_min) {
-			best_div = clock->div_table[i].div;
+			best_rate = calc_rate;
 			diff_min = diff;
 		}
 	}
 
-	return best_div;
-}
-
-static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
-				      unsigned long *parent_rate)
-{
-	struct sd_clock *clock = to_sd_clock(hw);
-	unsigned int div = cpg_sd_clock_calc_div(clock, rate, *parent_rate);
+	if (best_rate == ULONG_MAX)
+		return -EINVAL;
 
-	return DIV_ROUND_CLOSEST(*parent_rate, div);
+	req->rate = best_rate;
+	return 0;
 }
 
 static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -372,7 +372,7 @@ static const struct clk_ops cpg_sd_clock_ops = {
 	.disable = cpg_sd_clock_disable,
 	.is_enabled = cpg_sd_clock_is_enabled,
 	.recalc_rate = cpg_sd_clock_recalc_rate,
-	.round_rate = cpg_sd_clock_round_rate,
+	.determine_rate = cpg_sd_clock_determine_rate,
 	.set_rate = cpg_sd_clock_set_rate,
 };
 
-- 
2.17.1

Re: [PATCH 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Simon Horman]

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
> 

Re: [PATCH 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Geert Uytterhoeven]

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

-- 
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 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Simon Horman]

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
> 

Re: [PATCH 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Geert Uytterhoeven]

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

-- 
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 1/5] clk: renesas: rcar-gen2-legacy: Switch Z clock to .determine_rate()

[Simon Horman]

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.