[0/5] pwm: rcar: Add support "atomic" API and workaround
mbox series

Message ID 1544171373-29618-1-git-send-email-yoshihiro.shimoda.uh@renesas.com
Headers show
Series
  • pwm: rcar: Add support "atomic" API and workaround
Related show

Message

Yoshihiro Shimoda Dec. 7, 2018, 8:29 a.m. UTC
This patch adds support for PWM "atomic" API.

This patch also adds a workaround to output "pseudo" low level.
Otherwise, the PWM backlight driver doesn't work correctly, especially
it cannot output maximum brightness actually.

Yoshihiro Shimoda (5):
  pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only
  pwm: rcar: Add support "atomic" API
  pwm: rcar: Use "atomic" API on rcar_pwm_resume()
  pwm: rcar: remove legacy APIs
  pwm: rcar: add workaround to output "pseudo" low level

 drivers/pwm/pwm-rcar.c | 108 ++++++++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 46 deletions(-)

Comments

Uwe Kleine-König Dec. 7, 2018, 9:49 p.m. UTC | #1
Hello,

while looking at the driver I noticed another patch opportunity: In
rcar_pwm_get_clock_division() there is a loop:

	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
			(1 << div);
		do_div(max, clk_rate);
		if (period_ns <= max)
			break;
	}

The value of div should be calculatable without a loop. Something like:

   divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
   tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
   do_div(tmp, divider);
   div = ilog2(tmp - 1) + 1;

You might want to check if my maths are right, I didn't test.

Best regards
Uwe
Laurent Pinchart Dec. 9, 2018, 8:55 p.m. UTC | #2
Hi Uwe,

On Friday, 7 December 2018 23:49:32 EET Uwe Kleine-König wrote:
> Hello,
> 
> while looking at the driver I noticed another patch opportunity: In
> rcar_pwm_get_clock_division() there is a loop:
> 
> 	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> 		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> 			(1 << div);
> 		do_div(max, clk_rate);
> 		if (period_ns <= max)
> 			break;
> 	}
> 
> The value of div should be calculatable without a loop. Something like:
> 
>    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
>    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
>    do_div(tmp, divider);
>    div = ilog2(tmp - 1) + 1;
> 
> You might want to check if my maths are right, I didn't test.

I've noticed the same, and wrote the following patch last week, also untested.
I was planning to give it a try before sending it out, but as you've noticed
the same issue, here's the code if anyone wants to give it a try before I can.
Our calculations are similar, the main difference is the last line, and I
think yours read better.

From 22f7149916f590d3dbcc673dacc738441c741900 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date: Sun, 25 Nov 2018 16:02:39 +0200
Subject: [PATCH] pwm: rcar: Optimize rcar_pwm_get_clock_division()

Get rid of the loop over all possible divisor values by computing the
divisor directly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/pwm/pwm-rcar.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a41812fc6f95..e6d73b94d5cf 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -68,21 +68,27 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
 static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
 {
 	unsigned long clk_rate = clk_get_rate(rp->clk);
-	unsigned long long max; /* max cycle / nanoseconds */
-	unsigned int div;
+	u64 max_period_ns;
+	u32 div;
 
 	if (clk_rate == 0)
 		return -EINVAL;
 
-	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
-		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
-			(1 << div);
-		do_div(max, clk_rate);
-		if (period_ns <= max)
-			break;
-	}
+	/*
+	 * The maximum achievable period is 2^24 * 1023 cycles of the internal
+	 * bus clock.
+	 */
+	max_period_ns = (1ULL << RCAR_PWM_MAX_DIVISION) * RCAR_PWM_MAX_CYCLE
+		      * NSEC_PER_SEC;
+	do_div(max_period_ns, clk_rate);
+
+	if (period_ns > max_period_ns)
+		return -ERANGE;
 
-	return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
+	/* Calculate the divisor and round it up to the next power of two. */
+	div = DIV64_U64_ROUND_UP((u64)period_ns * clk_rate,
+				 (u64)RCAR_PWM_MAX_CYCLE * NSEC_PER_SEC);
+	return fls(2 * div - 1) - 1;
 }
 
 static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
Laurent Pinchart Dec. 9, 2018, 10:48 p.m. UTC | #3
Hi Shimoda-san,

Thank you for the patches.

On Friday, 7 December 2018 10:29:28 EET Yoshihiro Shimoda wrote:
> This patch adds support for PWM "atomic" API.
> 
> This patch also adds a workaround to output "pseudo" low level.
> Otherwise, the PWM backlight driver doesn't work correctly, especially
> it cannot output maximum brightness actually.
> 
> Yoshihiro Shimoda (5):
>   pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only
>   pwm: rcar: Add support "atomic" API
>   pwm: rcar: Use "atomic" API on rcar_pwm_resume()
>   pwm: rcar: remove legacy APIs
>   pwm: rcar: add workaround to output "pseudo" low level
> 
>  drivers/pwm/pwm-rcar.c | 108 ++++++++++++++++++++++++++--------------------
>  1 file changed, 62 insertions(+), 46 deletions(-)

For the whole series,

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

with backlight on the Draak board.

I do however agree with Uwe's comments.
Yoshihiro Shimoda Dec. 10, 2018, 5:09 a.m. UTC | #4
Hi Uwe, Laurent,

Thank you very much for your patches!
I tested patches and both codes work correctly.

> From: Laurent Pinchart, Sent: Monday, December 10, 2018 5:55 AM
> 
> Hi Uwe,
> 
> On Friday, 7 December 2018 23:49:32 EET Uwe Kleine-König wrote:
> > Hello,
> >
> > while looking at the driver I noticed another patch opportunity: In
> > rcar_pwm_get_clock_division() there is a loop:
> >
> > 	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > 		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> > 			(1 << div);
> > 		do_div(max, clk_rate);
> > 		if (period_ns <= max)
> > 			break;
> > 	}
> >
> > The value of div should be calculatable without a loop. Something like:
> >
> >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> >    do_div(tmp, divider);

This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).

> >    div = ilog2(tmp - 1) + 1;
> >
> > You might want to check if my maths are right, I didn't test.
> 
> I've noticed the same, and wrote the following patch last week, also untested.
> I was planning to give it a try before sending it out, but as you've noticed
> the same issue, here's the code if anyone wants to give it a try before I can.
> Our calculations are similar, the main difference is the last line, and I
> think yours read better.

So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
with your author and Singed-off-by?

Best regards,
Yoshihiro Shimoda

> From 22f7149916f590d3dbcc673dacc738441c741900 Mon Sep 17 00:00:00 2001
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date: Sun, 25 Nov 2018 16:02:39 +0200
> Subject: [PATCH] pwm: rcar: Optimize rcar_pwm_get_clock_division()
> 
> Get rid of the loop over all possible divisor values by computing the
> divisor directly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/pwm/pwm-rcar.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index a41812fc6f95..e6d73b94d5cf 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -68,21 +68,27 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
>  static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
>  {
>  	unsigned long clk_rate = clk_get_rate(rp->clk);
> -	unsigned long long max; /* max cycle / nanoseconds */
> -	unsigned int div;
> +	u64 max_period_ns;
> +	u32 div;
> 
>  	if (clk_rate == 0)
>  		return -EINVAL;
> 
> -	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> -		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> -			(1 << div);
> -		do_div(max, clk_rate);
> -		if (period_ns <= max)
> -			break;
> -	}
> +	/*
> +	 * The maximum achievable period is 2^24 * 1023 cycles of the internal
> +	 * bus clock.
> +	 */
> +	max_period_ns = (1ULL << RCAR_PWM_MAX_DIVISION) * RCAR_PWM_MAX_CYCLE
> +		      * NSEC_PER_SEC;
> +	do_div(max_period_ns, clk_rate);
> +
> +	if (period_ns > max_period_ns)
> +		return -ERANGE;
> 
> -	return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
> +	/* Calculate the divisor and round it up to the next power of two. */
> +	div = DIV64_U64_ROUND_UP((u64)period_ns * clk_rate,
> +				 (u64)RCAR_PWM_MAX_CYCLE * NSEC_PER_SEC);
> +	return fls(2 * div - 1) - 1;
>  }
> 
>  static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
>
Uwe Kleine-König Dec. 10, 2018, 8:04 a.m. UTC | #5
Hello,

On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> Thank you very much for your patches!
> I tested patches and both codes work correctly.

\o/, that's actually better than I expected :-)
 
> > > The value of div should be calculatable without a loop. Something like:
> > >
> > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > >    do_div(tmp, divider);
> 
> This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).

Yes, I think Laurent did this part right.
 
> > >    div = ilog2(tmp - 1) + 1;
> > >
> > > You might want to check if my maths are right, I didn't test.
> > 
> > I've noticed the same, and wrote the following patch last week, also untested.
> > I was planning to give it a try before sending it out, but as you've noticed
> > the same issue, here's the code if anyone wants to give it a try before I can.
> > Our calculations are similar, the main difference is the last line, and I
> > think yours read better.
> 
> So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> with your author and Singed-off-by?

Please no, I cannot sing good enough for this :-)

Honestly: If you take the authorship and write something like "Algorithm
suggested by Uwe Kleine-König and Laurent Pinchart" that's IMHO fine.

Best regards
Uwe
Yoshihiro Shimoda Dec. 12, 2018, 3:13 a.m. UTC | #6
Hi Uwe,

> From: Uwe Kleine-König, Sent: Monday, December 10, 2018 5:04 PM
> 
> Hello,
> 
> On Mon, Dec 10, 2018 at 05:09:31AM +0000, Yoshihiro Shimoda wrote:
> > Thank you very much for your patches!
> > I tested patches and both codes work correctly.
> 
> \o/, that's actually better than I expected :-)
> 
> > > > The value of div should be calculatable without a loop. Something like:
> > > >
> > > >    divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > > >    tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
> > > >    do_div(tmp, divider);
> >
> > This should be do_div64_u64() because the divider is 1,023,000,000,000 (over 32-bits).
> 
> Yes, I think Laurent did this part right.
> 
> > > >    div = ilog2(tmp - 1) + 1;
> > > >
> > > > You might want to check if my maths are right, I didn't test.
> > >
> > > I've noticed the same, and wrote the following patch last week, also untested.
> > > I was planning to give it a try before sending it out, but as you've noticed
> > > the same issue, here's the code if anyone wants to give it a try before I can.
> > > Our calculations are similar, the main difference is the last line, and I
> > > think yours read better.
> >
> > So, I'd like to apply Uwe's code to mainline. Uwe, may I send such a patch
> > with your author and Singed-off-by?
> 
> Please no, I cannot sing good enough for this :-)
> 
> Honestly: If you take the authorship and write something like "Algorithm
> suggested by Uwe Kleine-König and Laurent Pinchart" that's IMHO fine.

Thank you for your reply. I got it. I'll make a patch with such comment :)

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |