| Message ID | 1544171373-29618-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: rcar: Add support "atomic" API and workaround | expand |
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
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,
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.
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 > >
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
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/ |