* pwm: imx27: pwm-backlight strange behavior
@ 2020-12-03 12:00 Johannes Pointner
2020-12-04 11:48 ` Uwe Kleine-König
0 siblings, 1 reply; 5+ messages in thread
From: Johannes Pointner @ 2020-12-03 12:00 UTC (permalink / raw)
To: u.kleine-koenig, thierry.reding; +Cc: linux-pwm, linux-imx
Hello,
I just tested 5.10-rc6 with a imx6dl-board and found an issue
regarding the pwm-backlight.
Using 5.10 at about level 67 I got a new maximum and with level 68
it's restarting at about level 1.
This was working properly for me with kernel 5.4.
I tracked down the commit, which I think is responsible for the new behavior:
commit aef1a3799b5c ("pwm: imx27: Fix rounding behavior")
Reverting this commit fixes it for me.
Has anyone seen a similar behavior? Is something wrong with my
setup/configuration?
My devicetree backlight configuration looks like this:
pwms = <&pwm4 0 5000000>;
brightness-levels = <
0 1 2 3 4 5 6 7
8 9 10 11 12 13 14 15
16 17 18 19 20 21 22 23
24 25 26 27 28 29 30 31
32 33 34 35 36 37 38 39
40 41 42 43 44 45 46 47
48 49 50 51 52 53 54 55
56 57 58 59 60 61 62 63
64 65 66 67 68 69 70 71
72 73 74 75 76 77 78 79
80 81 82 83 84 85 86 87
88 89 90 91 92 93 94 95
96 97 98 99 100>;
Thanks,
Hannes
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: pwm: imx27: pwm-backlight strange behavior
2020-12-03 12:00 pwm: imx27: pwm-backlight strange behavior Johannes Pointner
@ 2020-12-04 11:48 ` Uwe Kleine-König
2020-12-05 11:18 ` Johannes Pointner
0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2020-12-04 11:48 UTC (permalink / raw)
To: Johannes Pointner; +Cc: thierry.reding, linux-pwm, linux-imx
[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]
Hello Johannes,
On Thu, Dec 03, 2020 at 01:00:56PM +0100, Johannes Pointner wrote:
> I just tested 5.10-rc6 with a imx6dl-board and found an issue
> regarding the pwm-backlight.
> Using 5.10 at about level 67 I got a new maximum and with level 68
> it's restarting at about level 1.
> This was working properly for me with kernel 5.4.
Reverting only the last hunk helps already I assume? I starred at the
patch for some time now and don't see a relevant change.
Can you please enable PWM_DEBUG and TRACING in the kernel configuration
and then do:
echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
reproduce a wrong setting (the less you do other than that the easier it
will be to analyse the trace) and then send me the contents of
/sys/kernel/debug/tracing/trace
? Also please lookup the frequency of the per clk (grep for "pwm" in
/sys/kernel/debug/clk/clk_summary).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: pwm: imx27: pwm-backlight strange behavior
2020-12-04 11:48 ` Uwe Kleine-König
@ 2020-12-05 11:18 ` Johannes Pointner
2020-12-05 17:42 ` Uwe Kleine-König
0 siblings, 1 reply; 5+ messages in thread
From: Johannes Pointner @ 2020-12-05 11:18 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: thierry.reding, linux-pwm, linux-imx
[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]
Hello Uwe,
On Fri, Dec 4, 2020 at 12:48 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Johannes,
>
> On Thu, Dec 03, 2020 at 01:00:56PM +0100, Johannes Pointner wrote:
> > I just tested 5.10-rc6 with a imx6dl-board and found an issue
> > regarding the pwm-backlight.
> > Using 5.10 at about level 67 I got a new maximum and with level 68
> > it's restarting at about level 1.
> > This was working properly for me with kernel 5.4.
>
> Reverting only the last hunk helps already I assume? I starred at the
> patch for some time now and don't see a relevant change.
Yes, that's right. If I just revert the calculation in the function
pwm_imx27_apply it works again.
>
> Can you please enable PWM_DEBUG and TRACING in the kernel configuration
> and then do:
>
> echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
>
> reproduce a wrong setting (the less you do other than that the easier it
> will be to analyse the trace) and then send me the contents of
>
> /sys/kernel/debug/tracing/trace
>
I attached the trace log. I also added a trace where I
reverted(trace_revert.log) the commit.
I did for both logs the following sequence of commands:
root# echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
root# echo 0 > /sys/class/backlight/backlight/brightness
root# echo 1 > /sys/class/backlight/backlight/brightness
root# echo 50 > /sys/class/backlight/backlight/brightness
root# echo 67 > /sys/class/backlight/backlight/brightness
root# echo 68 > /sys/class/backlight/backlight/brightness
> ? Also please lookup the frequency of the per clk (grep for "pwm" in
> /sys/kernel/debug/clk/clk_summary).
root# grep pwm /sys/kernel/debug/clk/clk_summary
pwm4 1 1 0
66000000 0 0 50000
Regards,
Hannes
[-- Attachment #2: trace.log --]
[-- Type: text/x-log, Size: 3406 bytes --]
# tracer: nop
#
# entries-in-buffer/entries-written: 24/24 #P:2
#
# _-----=> irqs-off
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / delay
# TASK-PID CPU# |||| TIMESTAMP FUNCTION
# | | | |||| | |
sh-445 [001] .... 115.088274: pwm_apply: b851ce54: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-445 [001] .... 115.088299: pwm_get: b851ce54: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-445 [001] .... 115.089377: pwm_apply: b851ce54: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-445 [001] .... 115.089394: pwm_get: b851ce54: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-445 [000] .... 118.160721: pwm_apply: b851ce54: period=5000000 duty_cycle=50000 polarity=0 enabled=0
sh-445 [000] .... 118.160745: pwm_get: b851ce54: period=5000000 duty_cycle=175910 polarity=0 enabled=0
sh-445 [000] .... 118.161854: pwm_apply: b851ce54: period=5000000 duty_cycle=175910 polarity=0 enabled=0
sh-445 [000] .... 118.161873: pwm_get: b851ce54: period=5000000 duty_cycle=619000 polarity=0 enabled=0
sh-445 [000] .... 118.477205: pwm_apply: b851ce54: period=5000000 duty_cycle=50000 polarity=0 enabled=1
sh-445 [000] .... 118.477227: pwm_get: b851ce54: period=5000000 duty_cycle=175910 polarity=0 enabled=1
sh-445 [000] .... 118.477286: pwm_apply: b851ce54: period=5000000 duty_cycle=175910 polarity=0 enabled=1
sh-445 [000] .... 118.477299: pwm_get: b851ce54: period=5000000 duty_cycle=175910 polarity=0 enabled=1
sh-445 [001] .... 122.975143: pwm_apply: b851ce54: period=5000000 duty_cycle=2500000 polarity=0 enabled=1
sh-445 [001] .... 122.975167: pwm_get: b851ce54: period=5000000 duty_cycle=2839637 polarity=0 enabled=1
sh-445 [001] .... 122.975227: pwm_apply: b851ce54: period=5000000 duty_cycle=2839637 polarity=0 enabled=1
sh-445 [001] .... 122.975239: pwm_get: b851ce54: period=5000000 duty_cycle=2839637 polarity=0 enabled=1
sh-445 [001] .... 131.086210: pwm_apply: b851ce54: period=5000000 duty_cycle=3350000 polarity=0 enabled=1
sh-445 [001] .... 131.086234: pwm_get: b851ce54: period=5000000 duty_cycle=5830728 polarity=0 enabled=1
sh-445 [001] .... 131.086293: pwm_apply: b851ce54: period=5000000 duty_cycle=5830728 polarity=0 enabled=1
sh-445 [001] .... 131.086305: pwm_get: b851ce54: period=5000000 duty_cycle=5830728 polarity=0 enabled=1
sh-445 [001] .... 137.845339: pwm_apply: b851ce54: period=5000000 duty_cycle=3400000 polarity=0 enabled=1
sh-445 [001] .... 137.845365: pwm_get: b851ce54: period=5000000 duty_cycle=48910 polarity=0 enabled=1
sh-445 [001] .... 137.845372: pwm_apply: b851ce54: period=5000000 duty_cycle=48910 polarity=0 enabled=1
sh-445 [001] .... 137.845382: pwm_get: b851ce54: period=5000000 duty_cycle=48910 polarity=0 enabled=1
[-- Attachment #3: trace_revert.log --]
[-- Type: text/x-log, Size: 3406 bytes --]
# tracer: nop
#
# entries-in-buffer/entries-written: 24/24 #P:2
#
# _-----=> irqs-off
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / delay
# TASK-PID CPU# |||| TIMESTAMP FUNCTION
# | | | |||| | |
sh-444 [001] .... 59.519376: pwm_apply: c563b93c: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-444 [001] .... 59.519411: pwm_get: c563b93c: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-444 [001] .... 59.520549: pwm_apply: c563b93c: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-444 [001] .... 59.520571: pwm_get: c563b93c: period=5000000 duty_cycle=0 polarity=0 enabled=0
sh-444 [000] .... 63.559905: pwm_apply: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=0
sh-444 [000] .... 63.559929: pwm_get: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=0
sh-444 [000] .... 63.560990: pwm_apply: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=0
sh-444 [000] .... 63.561006: pwm_get: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=0
sh-444 [000] .... 63.878189: pwm_apply: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=1
sh-444 [000] .... 63.878212: pwm_get: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=1
sh-444 [000] .... 63.878219: pwm_apply: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=1
sh-444 [000] .... 63.878229: pwm_get: c563b93c: period=5000000 duty_cycle=50000 polarity=0 enabled=1
sh-444 [000] .... 68.221972: pwm_apply: c563b93c: period=5000000 duty_cycle=2500000 polarity=0 enabled=1
sh-444 [000] .... 68.221991: pwm_get: c563b93c: period=5000000 duty_cycle=2500000 polarity=0 enabled=1
sh-444 [000] .... 68.221996: pwm_apply: c563b93c: period=5000000 duty_cycle=2500000 polarity=0 enabled=1
sh-444 [000] .... 68.222001: pwm_get: c563b93c: period=5000000 duty_cycle=2500000 polarity=0 enabled=1
sh-444 [001] .... 73.701325: pwm_apply: c563b93c: period=5000000 duty_cycle=3350000 polarity=0 enabled=1
sh-444 [001] .... 73.701350: pwm_get: c563b93c: period=5000000 duty_cycle=3350000 polarity=0 enabled=1
sh-444 [001] .... 73.701357: pwm_apply: c563b93c: period=5000000 duty_cycle=3350000 polarity=0 enabled=1
sh-444 [001] .... 73.701366: pwm_get: c563b93c: period=5000000 duty_cycle=3350000 polarity=0 enabled=1
sh-444 [000] .... 78.564468: pwm_apply: c563b93c: period=5000000 duty_cycle=3400000 polarity=0 enabled=1
sh-444 [000] .... 78.564483: pwm_get: c563b93c: period=5000000 duty_cycle=3400000 polarity=0 enabled=1
sh-444 [000] .... 78.564487: pwm_apply: c563b93c: period=5000000 duty_cycle=3400000 polarity=0 enabled=1
sh-444 [000] .... 78.564493: pwm_get: c563b93c: period=5000000 duty_cycle=3400000 polarity=0 enabled=1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: pwm: imx27: pwm-backlight strange behavior
2020-12-05 11:18 ` Johannes Pointner
@ 2020-12-05 17:42 ` Uwe Kleine-König
2020-12-05 18:39 ` Uwe Kleine-König
0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2020-12-05 17:42 UTC (permalink / raw)
To: Johannes Pointner; +Cc: thierry.reding, linux-pwm, linux-imx
[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]
On Sat, Dec 05, 2020 at 12:18:25PM +0100, Johannes Pointner wrote:
> Hello Uwe,
>
> On Fri, Dec 4, 2020 at 12:48 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Johannes,
> >
> > On Thu, Dec 03, 2020 at 01:00:56PM +0100, Johannes Pointner wrote:
> > > I just tested 5.10-rc6 with a imx6dl-board and found an issue
> > > regarding the pwm-backlight.
> > > Using 5.10 at about level 67 I got a new maximum and with level 68
> > > it's restarting at about level 1.
> > > This was working properly for me with kernel 5.4.
> >
> > Reverting only the last hunk helps already I assume? I starred at the
> > patch for some time now and don't see a relevant change.
> Yes, that's right. If I just revert the calculation in the function
> pwm_imx27_apply it works again.
> >
> > Can you please enable PWM_DEBUG and TRACING in the kernel configuration
> > and then do:
> >
> > echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
> >
> > reproduce a wrong setting (the less you do other than that the easier it
> > will be to analyse the trace) and then send me the contents of
> >
> > /sys/kernel/debug/tracing/trace
> >
> I attached the trace log. I also added a trace where I
> reverted(trace_revert.log) the commit.
> I did for both logs the following sequence of commands:
> root# echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
> root# echo 0 > /sys/class/backlight/backlight/brightness
> root# echo 1 > /sys/class/backlight/backlight/brightness
> root# echo 50 > /sys/class/backlight/backlight/brightness
> root# echo 67 > /sys/class/backlight/backlight/brightness
> root# echo 68 > /sys/class/backlight/backlight/brightness
>
> > ? Also please lookup the frequency of the per clk (grep for "pwm" in
> > /sys/kernel/debug/clk/clk_summary).
> root# grep pwm /sys/kernel/debug/clk/clk_summary
> pwm4 1 1 0
> 66000000 0 0 50000
Ah, I understood. The problem is:
do_div(c, NSEC_PER_SEC * prescale);
with (in your case) prescale = 6. This make the second argument
1000000000 * 6 = 6000000000 which doesn't fit into an u32 and so is
discarded to something considerably smaller.
A quick and dirty fix would be:
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..e7449edb6dc1 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -235,7 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
period_cycles /= prescale;
c = clkrate * state->duty_cycle;
- do_div(c, NSEC_PER_SEC * prescale);
+ do_div(c, NSEC_PER_SEC);
+ do_div(c, prescale);
duty_cycles = c;
/*
but that's ugly. I'll think about how to properly fix this.
Thanks for your report
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: pwm: imx27: pwm-backlight strange behavior
2020-12-05 17:42 ` Uwe Kleine-König
@ 2020-12-05 18:39 ` Uwe Kleine-König
0 siblings, 0 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2020-12-05 18:39 UTC (permalink / raw)
To: Johannes Pointner; +Cc: thierry.reding, linux-pwm, linux-imx
[-- Attachment #1: Type: text/plain, Size: 3507 bytes --]
On Sat, Dec 05, 2020 at 06:42:55PM +0100, Uwe Kleine-König wrote:
> On Sat, Dec 05, 2020 at 12:18:25PM +0100, Johannes Pointner wrote:
> > Hello Uwe,
> >
> > On Fri, Dec 4, 2020 at 12:48 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello Johannes,
> > >
> > > On Thu, Dec 03, 2020 at 01:00:56PM +0100, Johannes Pointner wrote:
> > > > I just tested 5.10-rc6 with a imx6dl-board and found an issue
> > > > regarding the pwm-backlight.
> > > > Using 5.10 at about level 67 I got a new maximum and with level 68
> > > > it's restarting at about level 1.
> > > > This was working properly for me with kernel 5.4.
> > >
> > > Reverting only the last hunk helps already I assume? I starred at the
> > > patch for some time now and don't see a relevant change.
> > Yes, that's right. If I just revert the calculation in the function
> > pwm_imx27_apply it works again.
> > >
> > > Can you please enable PWM_DEBUG and TRACING in the kernel configuration
> > > and then do:
> > >
> > > echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
> > >
> > > reproduce a wrong setting (the less you do other than that the easier it
> > > will be to analyse the trace) and then send me the contents of
> > >
> > > /sys/kernel/debug/tracing/trace
> > >
> > I attached the trace log. I also added a trace where I
> > reverted(trace_revert.log) the commit.
> > I did for both logs the following sequence of commands:
> > root# echo 1 > /sys/kernel/debug/tracing/events/pwm/enable
> > root# echo 0 > /sys/class/backlight/backlight/brightness
> > root# echo 1 > /sys/class/backlight/backlight/brightness
> > root# echo 50 > /sys/class/backlight/backlight/brightness
> > root# echo 67 > /sys/class/backlight/backlight/brightness
> > root# echo 68 > /sys/class/backlight/backlight/brightness
> >
> > > ? Also please lookup the frequency of the per clk (grep for "pwm" in
> > > /sys/kernel/debug/clk/clk_summary).
> > root# grep pwm /sys/kernel/debug/clk/clk_summary
> > pwm4 1 1 0
> > 66000000 0 0 50000
>
> Ah, I understood. The problem is:
>
> do_div(c, NSEC_PER_SEC * prescale);
>
> with (in your case) prescale = 6. This make the second argument
> 1000000000 * 6 = 6000000000 which doesn't fit into an u32 and so is
> discarded to something considerably smaller.
>
> A quick and dirty fix would be:
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index c50d453552bd..e7449edb6dc1 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -235,7 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>
> period_cycles /= prescale;
> c = clkrate * state->duty_cycle;
> - do_div(c, NSEC_PER_SEC * prescale);
> + do_div(c, NSEC_PER_SEC);
> + do_div(c, prescale);
> duty_cycles = c;
>
> /*
>
>
> but that's ugly. I'll think about how to properly fix this.
Argh, that looses more precision than I thought at first. Another
(probably more sensible) workaround is to reduce the period length to at
most 606060 ns such that prescale doesn't get bigger than 4. (Assuming
your display works with this, too. I would expect it does, but you want
to check the datasheet first.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-05 18:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 12:00 pwm: imx27: pwm-backlight strange behavior Johannes Pointner
2020-12-04 11:48 ` Uwe Kleine-König
2020-12-05 11:18 ` Johannes Pointner
2020-12-05 17:42 ` Uwe Kleine-König
2020-12-05 18:39 ` Uwe Kleine-König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.