* 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.