| Message ID | 20191017081059.31761-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: pwm_bl: configure pwm only once per backlight toggle | expand |
On 17. 10. 19 10:10, Uwe Kleine-König wrote: > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state")) changed the > semantic of pwm_get_state() and disclosed an (as it seems) common > problem in lowlevel PWM drivers. By not relying on the period and duty > cycle being retrievable from a disabled PWM this type of problem is > worked around. > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > combo once is also more effective. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > There are now two reports about 01ccf903edd6 breaking a backlight. As > far as I understand the problem this is a combination of the backend pwm > driver yielding surprising results and the pwm-bl driver doing things > more complicated than necessary. > > So I guess this patch works around these problems. Still it would be > interesting to find out the details in the imx driver that triggers the > problem. So Adam, can you please instrument the pwm-imx27 driver to > print *state at the beginning of pwm_imx27_apply() and the end of > pwm_imx27_get_state() and provide the results? > > Note I only compile tested this change. Hi Uwe, I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" thread that I have a similar problem when you submitted this patch. So here are my few cents: My setup is as follows: - imx6dl-yapp4-draco with i.MX6Solo - backlight is controlled with inverted PWM signal - max brightness level = 32, default brightness level set to 32 in DT. 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state): - System boots to userspace and backlight is enabled all the time from power up. $ dmesg | grep state [ 1.763381] get state end: -1811360608, enabled: 0 [ 4.939658] apply state start: -1797997308, enabled: 0 [ 4.946437] apply state start: -1797997436, enabled: 0 [ 4.952936] apply state start: -1797997420, enabled: 1 - $ cat brightness 32 - $ echo 32 > brightness # nothing happens, max. brightness - $ echo 1 > brightness # backlight goes down to lowest level [ 59.876561] apply state start: -1711276636, enabled: 1 - $ echo 0 > brightness # backlight goes up to max. level, this is # problem of the inverted PWM on i.MX we attempted # to solve some time ago. [ 102.456582] apply state start: -1711276700, enabled: 0 2. Backlight behavior on v5.4-rc3: - System boots to userspace and backlight is enabled all the time from power up. - $ dmesg | grep state [ 1.773056] get state end: -1811319520, enabled: 0 [ 4.994003] apply state start: -1797948160, enabled: 0 [ 5.000617] get state end: -1811319520, enabled: 0 [ 5.006950] apply state start: -1797948292, enabled: 0 [ 5.013446] get state end: -1811319520, enabled: 0 [ 5.019602] apply state start: -1797948276, enabled: 1 [ 5.027536] get state end: -1811319520, enabled: 1 - $ cat brightness 32 - $ echo 32 > brightness # backlight goes down [ 189.956756] apply state start: -1711923804, enabled: 1 [ 189.965907] get state end: -1811319520, enabled: 1 - $ echo 1 > brightness # backlight goes up to high level [ 464.136633] apply state start: -1711923804, enabled: 1 [ 464.145625] get state end: -1811319520, enabled: 1 - $ echo 0 > brightness # backlight goes up to highest level [ 527.136640] apply state start: -1711923868, enabled: 0 [ 527.145538] get state end: -1811319520, enabled: 0 So the behavior is clearly inverted to how it worked prior to 01ccf903edd6 with the weird exception that the initial brightness level 32 is not applied. 3. Backlight behavior on v5.4-rc3 + this patch: - System boots with backlight enabled. In the middle of kernel boot backlight is disabled. - $ dmesg | grep state [ 1.773233] get state end: -1811319520, enabled: 0 [ 5.002753] apply state start: -1797948160, enabled: 0 [ 5.009397] get state end: -1811319520, enabled: 0 [ 5.015652] apply state start: -1797948276, enabled: 1 [ 5.026682] get state end: -1811319520, enabled: 1 - $ cat brightness 32 - $ echo 32 > brightness # nothing happens, backlight is down - $ echo 1 > brightness # backlight goes to high level [ 305.377274] apply state start: -1711915596, enabled: 1 [ 305.386374] get state end: -1811319520, enabled: 1 - $ echo 0 > brightness # backlight goes to max brightness [ 362.637297] apply state start: -1711915596, enabled: 0 [ 362.646325] get state end: -1811319520, enabled: 0 Same behavior as (2) but the default state from DT is apparently applied. I only did this experiments. I did not delve into the code to track what is going on in there yet. Hopefully this helps you a bit and feel free to request other experiments, Michal > drivers/video/backlight/pwm_bl.c | 34 +++++++++++--------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 746eebc411df..ddebd62b3978 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -42,10 +42,8 @@ struct pwm_bl_data { > > static void pwm_backlight_power_on(struct pwm_bl_data *pb) > { > - struct pwm_state state; > int err; > > - pwm_get_state(pb->pwm, &state); > if (pb->enabled) > return; > > @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > if (err < 0) > dev_err(pb->dev, "failed to enable power supply\n"); > > - state.enabled = true; > - pwm_apply_state(pb->pwm, &state); > - > if (pb->post_pwm_on_delay) > msleep(pb->post_pwm_on_delay); > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > { > - struct pwm_state state; > - > - pwm_get_state(pb->pwm, &state); > - if (!pb->enabled) > - return; > - > if (pb->enable_gpio) > gpiod_set_value_cansleep(pb->enable_gpio, 0); > > if (pb->pwm_off_delay) > msleep(pb->pwm_off_delay); > > - state.enabled = false; > - state.duty_cycle = 0; > - pwm_apply_state(pb->pwm, &state); > - > regulator_disable(pb->power_supply); > pb->enabled = false; > } > > -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state) > { > unsigned int lth = pb->lth_brightness; > - struct pwm_state state; > u64 duty_cycle; > > - pwm_get_state(pb->pwm, &state); > - > if (pb->levels) > duty_cycle = pb->levels[brightness]; > else > duty_cycle = brightness; > > - duty_cycle *= state.period - lth; > + duty_cycle *= state->period - lth; > do_div(duty_cycle, pb->scale); > > return duty_cycle + lth; > @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > if (brightness > 0) { > pwm_get_state(pb->pwm, &state); > - state.duty_cycle = compute_duty_cycle(pb, brightness); > + state.duty_cycle = compute_duty_cycle(pb, brightness, &state); > + state.enabled = true; > pwm_apply_state(pb->pwm, &state); > + > pwm_backlight_power_on(pb); > - } else > + } else { > pwm_backlight_power_off(pb); > > + pwm_get_state(pb->pwm, &state); > + state.enabled = false; > + state.duty_cycle = 0; > + pwm_apply_state(pb->pwm, &state); > + } > + > if (pb->notify_after) > pb->notify_after(pb->dev, brightness); > >
On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > pwm_get_state() return the last implemented state")) changed the > > semantic of pwm_get_state() and disclosed an (as it seems) common > > problem in lowlevel PWM drivers. By not relying on the period and duty > > cycle being retrievable from a disabled PWM this type of problem is > > worked around. > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > combo once is also more effective. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello, > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > far as I understand the problem this is a combination of the backend pwm > > driver yielding surprising results and the pwm-bl driver doing things > > more complicated than necessary. > > > > So I guess this patch works around these problems. Still it would be > > interesting to find out the details in the imx driver that triggers the > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > print *state at the beginning of pwm_imx27_apply() and the end of > > pwm_imx27_get_state() and provide the results? > > > > Note I only compile tested this change. > > Hi Uwe, > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > thread that I have a similar problem when you submitted this patch. > > So here are my few cents: > > My setup is as follows: > - imx6dl-yapp4-draco with i.MX6Solo > - backlight is controlled with inverted PWM signal > - max brightness level = 32, default brightness level set to 32 in DT. > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state): > > - System boots to userspace and backlight is enabled all the time from > power up. > > $ dmesg | grep state > [ 1.763381] get state end: -1811360608, enabled: 0 What is -1811360608? When I wrote "print *state" above, I thought about something like: pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", __func__, state->period, state->duty_cycle, state->polarity, state->enabled); A quick look into drivers/pwm/pwm-imx27.c shows that this is another driver that yields duty_cycle = 0 when the hardware is off. Best regards Uwe
On 17. 10. 19 11:48, Michal Vokáč wrote: > On 17. 10. 19 10:10, Uwe Kleine-König wrote: >> A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let >> pwm_get_state() return the last implemented state")) changed the >> semantic of pwm_get_state() and disclosed an (as it seems) common >> problem in lowlevel PWM drivers. By not relying on the period and duty >> cycle being retrievable from a disabled PWM this type of problem is >> worked around. >> >> Apart from this issue only calling the pwm_get_state/pwm_apply_state >> combo once is also more effective. >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> --- >> Hello, >> >> There are now two reports about 01ccf903edd6 breaking a backlight. As >> far as I understand the problem this is a combination of the backend pwm >> driver yielding surprising results and the pwm-bl driver doing things >> more complicated than necessary. >> >> So I guess this patch works around these problems. Still it would be >> interesting to find out the details in the imx driver that triggers the >> problem. So Adam, can you please instrument the pwm-imx27 driver to >> print *state at the beginning of pwm_imx27_apply() and the end of >> pwm_imx27_get_state() and provide the results? >> >> Note I only compile tested this change. > > Hi Uwe, > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > thread that I have a similar problem when you submitted this patch. > > So here are my few cents: Once again with updated and more detailed debug messages. > My setup is as follows: > - imx6dl-yapp4-draco with i.MX6Solo > - backlight is controlled with inverted PWM signal > - max brightness level = 32, default brightness level set to 32 in DT. > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state): > > - System boots to userspace and backlight is enabled all the time from > power up. > - $ dmesg | grep pwm_ [ 1.761546] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.012352] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 5.021143] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 [ 5.030182] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > - $ cat brightness > 32 > > - $ echo 32 > brightness # nothing happens, max. brightness > > - $ echo 1 > brightness # backlight goes down to lowest level [ 93.976354] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 > > - $ echo 0 > brightness # backlight goes up to max. level, this is > # problem of the inverted PWM on i.MX we attempted > # to solve some time ago. [ 115.496350] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > 2. Backlight behavior on v5.4-rc3: > > - System boots to userspace and backlight is enabled all the time from > power up. > - $ dmesg | grep pwm_ [ 1.774071] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.003961] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 5.012649] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.021694] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 [ 5.030732] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.039643] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 [ 5.049605] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > - $ cat brightness > 32 > > - $ echo 32 > brightness # backlight goes down [ 707.946970] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 [ 707.958551] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > - $ echo 1 > brightness # backlight goes up to high level [ 757.516845] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 [ 757.528438] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > - $ echo 0 > brightness # backlight goes up to highest level [ 783.386838] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 783.398025] pwm_imx27_get_state: period: 496485, duty: 0, polarity: 0, enabled: 0 > > So the behavior is clearly inverted to how it worked prior to 01ccf903edd6 > with the weird exception that the initial brightness level 32 is > not applied. > > 3. Backlight behavior on v5.4-rc3 + this patch: > > - System boots with backlight enabled. In the middle of kernel boot > backlight is disabled. > > - $ dmesg | grep state [ 1.773099] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.002532] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 5.011263] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.020307] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 [ 5.031066] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > - $ cat brightness > 32 > > - $ echo 32 > brightness # nothing happens, backlight is down > > - $ echo 1 > brightness # backlight goes to high level [ 73.786926] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 [ 73.798469] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > - $ echo 0 > brightness # backlight goes to max brightness [ 104.636908] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 104.648093] pwm_imx27_get_state: period: 496485, duty: 0, polarity: 0, enabled: 0 > > Same behavior as (2) but the default state from DT is apparently applied. > > I only did this experiments. I did not delve into the code to track what is > going on in there yet. > > Hopefully this helps you a bit and feel free to request other experiments, > Michal > >> drivers/video/backlight/pwm_bl.c | 34 +++++++++++--------------------- >> 1 file changed, 12 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c >> index 746eebc411df..ddebd62b3978 100644 >> --- a/drivers/video/backlight/pwm_bl.c >> +++ b/drivers/video/backlight/pwm_bl.c >> @@ -42,10 +42,8 @@ struct pwm_bl_data { >> static void pwm_backlight_power_on(struct pwm_bl_data *pb) >> { >> - struct pwm_state state; >> int err; >> - pwm_get_state(pb->pwm, &state); >> if (pb->enabled) >> return; >> @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) >> if (err < 0) >> dev_err(pb->dev, "failed to enable power supply\n"); >> - state.enabled = true; >> - pwm_apply_state(pb->pwm, &state); >> - >> if (pb->post_pwm_on_delay) >> msleep(pb->post_pwm_on_delay); >> @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) >> static void pwm_backlight_power_off(struct pwm_bl_data *pb) >> { >> - struct pwm_state state; >> - >> - pwm_get_state(pb->pwm, &state); >> - if (!pb->enabled) >> - return; >> - >> if (pb->enable_gpio) >> gpiod_set_value_cansleep(pb->enable_gpio, 0); >> if (pb->pwm_off_delay) >> msleep(pb->pwm_off_delay); >> - state.enabled = false; >> - state.duty_cycle = 0; >> - pwm_apply_state(pb->pwm, &state); >> - >> regulator_disable(pb->power_supply); >> pb->enabled = false; >> } >> -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) >> +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state) >> { >> unsigned int lth = pb->lth_brightness; >> - struct pwm_state state; >> u64 duty_cycle; >> - pwm_get_state(pb->pwm, &state); >> - >> if (pb->levels) >> duty_cycle = pb->levels[brightness]; >> else >> duty_cycle = brightness; >> - duty_cycle *= state.period - lth; >> + duty_cycle *= state->period - lth; >> do_div(duty_cycle, pb->scale); >> return duty_cycle + lth; >> @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl) >> if (brightness > 0) { >> pwm_get_state(pb->pwm, &state); >> - state.duty_cycle = compute_duty_cycle(pb, brightness); >> + state.duty_cycle = compute_duty_cycle(pb, brightness, &state); >> + state.enabled = true; >> pwm_apply_state(pb->pwm, &state); >> + >> pwm_backlight_power_on(pb); >> - } else >> + } else { >> pwm_backlight_power_off(pb); >> + pwm_get_state(pb->pwm, &state); >> + state.enabled = false; >> + state.duty_cycle = 0; >> + pwm_apply_state(pb->pwm, &state); >> + } >> + >> if (pb->notify_after) >> pb->notify_after(pb->dev, brightness); >> >
On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state")) changed the > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > cycle being retrievable from a disabled PWM this type of problem is > > > worked around. > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > combo once is also more effective. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > Hello, > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > far as I understand the problem this is a combination of the backend pwm > > > driver yielding surprising results and the pwm-bl driver doing things > > > more complicated than necessary. > > > > > > So I guess this patch works around these problems. Still it would be > > > interesting to find out the details in the imx driver that triggers the > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > pwm_imx27_get_state() and provide the results? > > > > > > Note I only compile tested this change. > > > > Hi Uwe, > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > thread that I have a similar problem when you submitted this patch. > > > > So here are my few cents: > > > > My setup is as follows: > > - imx6dl-yapp4-draco with i.MX6Solo > > - backlight is controlled with inverted PWM signal > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > pwm_get_state() return the last implemented state): > > > > - System boots to userspace and backlight is enabled all the time from > > power up. > > > > $ dmesg | grep state > > [ 1.763381] get state end: -1811360608, enabled: 0 > > What is -1811360608? When I wrote "print *state" above, I thought about > something like: > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > driver that yields duty_cycle = 0 when the hardware is off. It seems to me like the best recourse to fix this for now would be to patch up the drivers that return 0 when the hardware is off by caching the currently configured duty cycle. How about the patch below? Thierry --- >8 --- From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 From: Thierry Reding <thierry.reding@gmail.com> Date: Thu, 17 Oct 2019 12:56:00 +0200 Subject: [PATCH] pwm: imx27: Cache duty cycle register value The hardware register containing the duty cycle value cannot be accessed when the PWM is disabled. This causes the ->get_state() callback to read back a duty cycle value of 0, which can confuse consumer drivers. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index ae11d8577f18..4113d5cd4c62 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -85,6 +85,13 @@ struct pwm_imx27_chip { struct clk *clk_per; void __iomem *mmio_base; struct pwm_chip chip; + + /* + * The driver cannot read the current duty cycle from the hardware if + * the hardware is disabled. Cache the last programmed duty cycle + * value to return in that case. + */ + unsigned int duty_cycle; }; #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, tmp = NSEC_PER_SEC * (u64)(period + 2); state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); - /* PWMSAR can be read only if PWM is enabled */ - if (state->enabled) { + /* + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, + * use the cached value. + */ + if (state->enabled) val = readl(imx->mmio_base + MX3_PWMSAR); - tmp = NSEC_PER_SEC * (u64)(val); - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); - } else { - state->duty_cycle = 0; - } + else + val = imx->duty_cycle; + + tmp = NSEC_PER_SEC * (u64)(val); + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); if (!state->enabled) pwm_imx27_clk_disable_unprepare(chip); @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); writel(period_cycles, imx->mmio_base + MX3_PWMPR); + /* + * Store the duty cycle for future reference in cases where + * the MX3_PWMSAR register can't be read (i.e. when the PWM + * is disabled). + */ + imx->duty_cycle = duty_cycles; + cr = MX3_PWMCR_PRESCALER_SET(prescale) | MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state")) changed the > semantic of pwm_get_state() and disclosed an (as it seems) common > problem in lowlevel PWM drivers. By not relying on the period and duty > cycle being retrievable from a disabled PWM this type of problem is > worked around. > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > combo once is also more effective. I'm only interested in the second paragraph here. There seems to be a reasonable consensus that the i.MX27 and cros-ec PWM drivers should be fixed for the benefit of other PWM clients. So we make this change because it makes the pwm-bl better... not to work around bugs ;-). > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 746eebc411df..ddebd62b3978 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > { > - struct pwm_state state; > - > - pwm_get_state(pb->pwm, &state); > - if (!pb->enabled) > - return; > - Why remove the pb->enabled check? I thought that was there to ensure we don't mess up the regular reference counts. Daniel.
On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state")) changed the > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > worked around. > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > combo once is also more effective. > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > Hello, > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > far as I understand the problem this is a combination of the backend pwm > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > more complicated than necessary. > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > interesting to find out the details in the imx driver that triggers the > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > Note I only compile tested this change. > > > > > > Hi Uwe, > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > thread that I have a similar problem when you submitted this patch. > > > > > > So here are my few cents: > > > > > > My setup is as follows: > > > - imx6dl-yapp4-draco with i.MX6Solo > > > - backlight is controlled with inverted PWM signal > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state): > > > > > > - System boots to userspace and backlight is enabled all the time from > > > power up. > > > > > > $ dmesg | grep state > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > something like: > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > driver that yields duty_cycle = 0 when the hardware is off. > > It seems to me like the best recourse to fix this for now would be to > patch up the drivers that return 0 when the hardware is off by caching > the currently configured duty cycle. > > How about the patch below? > > Thierry > > --- >8 --- > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <thierry.reding@gmail.com> > Date: Thu, 17 Oct 2019 12:56:00 +0200 > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > The hardware register containing the duty cycle value cannot be accessed > when the PWM is disabled. This causes the ->get_state() callback to read > back a duty cycle value of 0, which can confuse consumer drivers. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index ae11d8577f18..4113d5cd4c62 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > struct clk *clk_per; > void __iomem *mmio_base; > struct pwm_chip chip; > + > + /* > + * The driver cannot read the current duty cycle from the hardware if > + * the hardware is disabled. Cache the last programmed duty cycle > + * value to return in that case. > + */ > + unsigned int duty_cycle; > }; > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > tmp = NSEC_PER_SEC * (u64)(period + 2); > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - /* PWMSAR can be read only if PWM is enabled */ > - if (state->enabled) { > + /* > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > + * use the cached value. > + */ > + if (state->enabled) > val = readl(imx->mmio_base + MX3_PWMSAR); > - tmp = NSEC_PER_SEC * (u64)(val); > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > - } else { > - state->duty_cycle = 0; > - } > + else > + val = imx->duty_cycle; > + > + tmp = NSEC_PER_SEC * (u64)(val); > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > if (!state->enabled) > pwm_imx27_clk_disable_unprepare(chip); > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > + /* > + * Store the duty cycle for future reference in cases where > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > + * is disabled). > + */ > + imx->duty_cycle = duty_cycles; > + I wonder if it would be more sensible to do this in the pwm core instead. Currently there are two drivers known with this problem. I wouldn't be surprised if there were more. If we want to move clients to not rely on .period and .duty_cycle for a disabled PWM (do we?) a single change in the core is also beneficial compared to fixing several lowlevel drivers. Best regards Uwe
On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > pwm_get_state() return the last implemented state")) changed the > > semantic of pwm_get_state() and disclosed an (as it seems) common > > problem in lowlevel PWM drivers. By not relying on the period and duty > > cycle being retrievable from a disabled PWM this type of problem is > > worked around. > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > combo once is also more effective. > > I'm only interested in the second paragraph here. > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > PWM drivers should be fixed for the benefit of other PWM clients. > So we make this change because it makes the pwm-bl better... not to > work around bugs ;-). That's fine, still I think it's fair to explain the motivation of creating this patch. > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > index 746eebc411df..ddebd62b3978 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > { > > - struct pwm_state state; > > - > > - pwm_get_state(pb->pwm, &state); > > - if (!pb->enabled) > > - return; > > - > > Why remove the pb->enabled check? I thought that was there to ensure we > don't mess up the regular reference counts. I havn't looked yet, but I guess I have to respin. Expect a v2 later today. Best regards Uwe
On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state")) changed the > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > cycle being retrievable from a disabled PWM this type of problem is > > > worked around. > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > combo once is also more effective. > > > > I'm only interested in the second paragraph here. > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > PWM drivers should be fixed for the benefit of other PWM clients. > > So we make this change because it makes the pwm-bl better... not to > > work around bugs ;-). > > That's fine, still I think it's fair to explain the motivation of > creating this patch. > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > index 746eebc411df..ddebd62b3978 100644 > > > --- a/drivers/video/backlight/pwm_bl.c > > > +++ b/drivers/video/backlight/pwm_bl.c > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > > > > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > > { > > > - struct pwm_state state; > > > - > > > - pwm_get_state(pb->pwm, &state); > > > - if (!pb->enabled) > > > - return; > > > - > > > > Why remove the pb->enabled check? I thought that was there to ensure we > > don't mess up the regular reference counts. > > I havn't looked yet, but I guess I have to respin. Expect a v2 later > today. I would agree that a high-level fix is better than a series of low level driver fixes. For what its worth, your V1 patch worked fine on my i.MX6Q. I can test the V2 patch when its ready. adam > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Oct 17, 2019 at 7:34 AM Adam Ford <aford173@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state")) changed the > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > worked around. > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > combo once is also more effective. > > > > > > I'm only interested in the second paragraph here. > > > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > > PWM drivers should be fixed for the benefit of other PWM clients. > > > So we make this change because it makes the pwm-bl better... not to > > > work around bugs ;-). > > > > That's fine, still I think it's fair to explain the motivation of > > creating this patch. > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > index 746eebc411df..ddebd62b3978 100644 > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > > > > > > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > > > { > > > > - struct pwm_state state; > > > > - > > > > - pwm_get_state(pb->pwm, &state); > > > > - if (!pb->enabled) > > > > - return; > > > > - > > > > > > Why remove the pb->enabled check? I thought that was there to ensure we > > > don't mess up the regular reference counts. > > > > I havn't looked yet, but I guess I have to respin. Expect a v2 later > > today. > > I would agree that a high-level fix is better than a series of low > level driver fixes. For what its worth, your V1 patch worked fine on > my i.MX6Q. I can test the V2 patch when its ready. I may have spoken too soon. The patch fixes the display in that it comes on when it previously did not, but changes to brightness do not appear to do anything anymore. I don't have an oscilloscope where I am now, so I cannot verify whether or not the PWM duty cycle changes. To my eye, the following do not appear to change the brightness of the screen: echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness adam > > adam > > > > Best regards > > Uwe > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Oct 17, 2019 at 3:11 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state")) changed the > semantic of pwm_get_state() and disclosed an (as it seems) common > problem in lowlevel PWM drivers. By not relying on the period and duty > cycle being retrievable from a disabled PWM this type of problem is > worked around. > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > combo once is also more effective. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > There are now two reports about 01ccf903edd6 breaking a backlight. As > far as I understand the problem this is a combination of the backend pwm > driver yielding surprising results and the pwm-bl driver doing things > more complicated than necessary. > > So I guess this patch works around these problems. Still it would be > interesting to find out the details in the imx driver that triggers the > problem. So Adam, can you please instrument the pwm-imx27 driver to > print *state at the beginning of pwm_imx27_apply() and the end of > pwm_imx27_get_state() and provide the results? > > Note I only compile tested this change. > > Best regards > Uwe > > drivers/video/backlight/pwm_bl.c | 34 +++++++++++--------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 746eebc411df..ddebd62b3978 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -42,10 +42,8 @@ struct pwm_bl_data { > > static void pwm_backlight_power_on(struct pwm_bl_data *pb) > { > - struct pwm_state state; > int err; > > - pwm_get_state(pb->pwm, &state); > if (pb->enabled) > return; > > @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > if (err < 0) > dev_err(pb->dev, "failed to enable power supply\n"); > > - state.enabled = true; > - pwm_apply_state(pb->pwm, &state); > - > if (pb->post_pwm_on_delay) > msleep(pb->post_pwm_on_delay); > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > { > - struct pwm_state state; > - > - pwm_get_state(pb->pwm, &state); > - if (!pb->enabled) > - return; > - > if (pb->enable_gpio) > gpiod_set_value_cansleep(pb->enable_gpio, 0); > > if (pb->pwm_off_delay) > msleep(pb->pwm_off_delay); > > - state.enabled = false; > - state.duty_cycle = 0; > - pwm_apply_state(pb->pwm, &state); > - > regulator_disable(pb->power_supply); > pb->enabled = false; > } > > -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state) > { > unsigned int lth = pb->lth_brightness; > - struct pwm_state state; > u64 duty_cycle; > > - pwm_get_state(pb->pwm, &state); > - > if (pb->levels) > duty_cycle = pb->levels[brightness]; > else > duty_cycle = brightness; > > - duty_cycle *= state.period - lth; > + duty_cycle *= state->period - lth; > do_div(duty_cycle, pb->scale); > > return duty_cycle + lth; > @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > if (brightness > 0) { > pwm_get_state(pb->pwm, &state); > - state.duty_cycle = compute_duty_cycle(pb, brightness); > + state.duty_cycle = compute_duty_cycle(pb, brightness, &state); > + state.enabled = true; > pwm_apply_state(pb->pwm, &state); > + > pwm_backlight_power_on(pb); > - } else > + } else { > pwm_backlight_power_off(pb); > > + pwm_get_state(pb->pwm, &state); > + state.enabled = false; > + state.duty_cycle = 0; > + pwm_apply_state(pb->pwm, &state); Both cases where (brightness > 0) and 'else' contain the pwm_apply_state() call with the same parameters. Can this be moved outside of the if statements? > + } > + > if (pb->notify_after) > pb->notify_after(pb->dev, brightness); > > -- > 2.23.0 >
On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > worked around. > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > combo once is also more effective. > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > --- > > > > > Hello, > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > more complicated than necessary. > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > interesting to find out the details in the imx driver that triggers the > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > Note I only compile tested this change. > > > > > > > > Hi Uwe, > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > So here are my few cents: > > > > > > > > My setup is as follows: > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > - backlight is controlled with inverted PWM signal > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state): > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > power up. > > > > > > > > $ dmesg | grep state > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > something like: > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > It seems to me like the best recourse to fix this for now would be to > > patch up the drivers that return 0 when the hardware is off by caching > > the currently configured duty cycle. > > > > How about the patch below? > > > > Thierry > > > > --- >8 --- > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > The hardware register containing the duty cycle value cannot be accessed > > when the PWM is disabled. This causes the ->get_state() callback to read > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index ae11d8577f18..4113d5cd4c62 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > struct clk *clk_per; > > void __iomem *mmio_base; > > struct pwm_chip chip; > > + > > + /* > > + * The driver cannot read the current duty cycle from the hardware if > > + * the hardware is disabled. Cache the last programmed duty cycle > > + * value to return in that case. > > + */ > > + unsigned int duty_cycle; > > }; > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > - /* PWMSAR can be read only if PWM is enabled */ > > - if (state->enabled) { > > + /* > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > + * use the cached value. > > + */ > > + if (state->enabled) > > val = readl(imx->mmio_base + MX3_PWMSAR); > > - tmp = NSEC_PER_SEC * (u64)(val); > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - } else { > > - state->duty_cycle = 0; > > - } > > + else > > + val = imx->duty_cycle; > > + > > + tmp = NSEC_PER_SEC * (u64)(val); > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > if (!state->enabled) > > pwm_imx27_clk_disable_unprepare(chip); > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > + /* > > + * Store the duty cycle for future reference in cases where > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > + * is disabled). > > + */ > > + imx->duty_cycle = duty_cycles; > > + > > I wonder if it would be more sensible to do this in the pwm core > instead. Currently there are two drivers known with this problem. I > wouldn't be surprised if there were more. I've inspected all the drivers and didn't spot any beyond cros-ec and i.MX that have this problem. There's also no good way to do this in the core, because the core doesn't know whether or not the driver is capable of returning the correct duty cycle on hardare readout. So the core would have to rely on state->duty_cycle that is passed in, but then the offending commit becomes useless because the whole point was to return the state as written to hardware (rather than the software state which was being returned before that patch). > If we want to move clients to not rely on .period and .duty_cycle for a > disabled PWM (do we?) a single change in the core is also beneficial > compared to fixing several lowlevel drivers. These are really two orthogonal problems. We don't currently consider enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not prepared to do that at this point in the release cycle either. What this here has shown is that we have at least two drivers that don't behave the way they are supposed to according to the API and they break consumers. If they break for pwm-backlight, it's possible that they will break for other consumers as well. So the right thing to do is fix the two drivers that are broken. After -rc1 we no longer experiment. Instead we clean up the messes we've made. We can revisit the other points once mainline is fixed. Thierry
On Thu, Oct 17, 2019 at 07:40:47AM -0500, Adam Ford wrote: > On Thu, Oct 17, 2019 at 7:34 AM Adam Ford <aford173@gmail.com> wrote: > > > > On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > worked around. > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > combo once is also more effective. > > > > > > > > I'm only interested in the second paragraph here. > > > > > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > > > PWM drivers should be fixed for the benefit of other PWM clients. > > > > So we make this change because it makes the pwm-bl better... not to > > > > work around bugs ;-). > > > > > > That's fine, still I think it's fair to explain the motivation of > > > creating this patch. > > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > index 746eebc411df..ddebd62b3978 100644 > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > > > > > > > > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > > > > { > > > > > - struct pwm_state state; > > > > > - > > > > > - pwm_get_state(pb->pwm, &state); > > > > > - if (!pb->enabled) > > > > > - return; > > > > > - > > > > > > > > Why remove the pb->enabled check? I thought that was there to ensure we > > > > don't mess up the regular reference counts. > > > > > > I havn't looked yet, but I guess I have to respin. Expect a v2 later > > > today. > > > > I would agree that a high-level fix is better than a series of low > > level driver fixes. For what its worth, your V1 patch worked fine on > > my i.MX6Q. I can test the V2 patch when its ready. > > I may have spoken too soon. The patch fixes the display in that it > comes on when it previously did not, but changes to brightness do not > appear to do anything anymore. I don't have an oscilloscope where I > am now, so I cannot verify whether or not the PWM duty cycle changes. > > To my eye, the following do not appear to change the brightness of the screen: > echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness > echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness > echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness Hi Adam, can you try the i.MX PWM patch that I posted earlier instead? I really think we need to fix this in the PWM drivers because they are broken. pwm-backlight is not. -rc3 is really not a time to work around breakage in consumers. If my patch doesn't help, can you try reverting the offending patch? If we can't come up with a good fix for the drivers, reverting is the next best option. Thierry
On Thu, Oct 17, 2019 at 8:05 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 07:40:47AM -0500, Adam Ford wrote: > > On Thu, Oct 17, 2019 at 7:34 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > > > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > worked around. > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > combo once is also more effective. > > > > > > > > > > I'm only interested in the second paragraph here. > > > > > > > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > > > > PWM drivers should be fixed for the benefit of other PWM clients. > > > > > So we make this change because it makes the pwm-bl better... not to > > > > > work around bugs ;-). > > > > > > > > That's fine, still I think it's fair to explain the motivation of > > > > creating this patch. > > > > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > > index 746eebc411df..ddebd62b3978 100644 > > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > > > > > > > > > > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > > > > > > { > > > > > > - struct pwm_state state; > > > > > > - > > > > > > - pwm_get_state(pb->pwm, &state); > > > > > > - if (!pb->enabled) > > > > > > - return; > > > > > > - > > > > > > > > > > Why remove the pb->enabled check? I thought that was there to ensure we > > > > > don't mess up the regular reference counts. > > > > > > > > I havn't looked yet, but I guess I have to respin. Expect a v2 later > > > > today. > > > > > > I would agree that a high-level fix is better than a series of low > > > level driver fixes. For what its worth, your V1 patch worked fine on > > > my i.MX6Q. I can test the V2 patch when its ready. > > > > I may have spoken too soon. The patch fixes the display in that it > > comes on when it previously did not, but changes to brightness do not > > appear to do anything anymore. I don't have an oscilloscope where I > > am now, so I cannot verify whether or not the PWM duty cycle changes. > > > > To my eye, the following do not appear to change the brightness of the screen: > > echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness > > echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness > > echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness > > Hi Adam, > > can you try the i.MX PWM patch that I posted earlier instead? I really > think we need to fix this in the PWM drivers because they are broken. > pwm-backlight is not. -rc3 is really not a time to work around breakage > in consumers. I did try your patch, but it did not appear to make any difference on my i.MX6Q. > > If my patch doesn't help, can you try reverting the offending patch? If > we can't come up with a good fix for the drivers, reverting is the next > best option. I am able to get an image after reverting the offending patch. I did not try playing with brightness levels after reverting. > > Thierry
On Thu, Oct 17, 2019 at 02:19:45PM +0200, Uwe Kleine-König wrote: > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state")) changed the > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > cycle being retrievable from a disabled PWM this type of problem is > > > worked around. > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > combo once is also more effective. > > > > I'm only interested in the second paragraph here. > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > PWM drivers should be fixed for the benefit of other PWM clients. > > So we make this change because it makes the pwm-bl better... not to > > work around bugs ;-). > > That's fine, still I think it's fair to explain the motivation of > creating this patch. Maybe. Whether this patch is a workaround or simply an improvement to pwm-bl does need to be clear since it affects whether Lee steers it towards v5.4-rcX or linux-next . Daniel.
On Thu, Oct 17, 2019 at 6:11 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state")) changed the > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > worked around. > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > combo once is also more effective. > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > Hello, > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > far as I understand the problem this is a combination of the backend pwm > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > more complicated than necessary. > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > interesting to find out the details in the imx driver that triggers the > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > Note I only compile tested this change. > > > > > > Hi Uwe, > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > thread that I have a similar problem when you submitted this patch. > > > > > > So here are my few cents: > > > > > > My setup is as follows: > > > - imx6dl-yapp4-draco with i.MX6Solo > > > - backlight is controlled with inverted PWM signal > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state): > > > > > > - System boots to userspace and backlight is enabled all the time from > > > power up. > > > > > > $ dmesg | grep state > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > something like: > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > driver that yields duty_cycle = 0 when the hardware is off. > > It seems to me like the best recourse to fix this for now would be to > patch up the drivers that return 0 when the hardware is off by caching > the currently configured duty cycle. > > How about the patch below? > > Thierry > > --- >8 --- > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <thierry.reding@gmail.com> > Date: Thu, 17 Oct 2019 12:56:00 +0200 > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > The hardware register containing the duty cycle value cannot be accessed > when the PWM is disabled. This causes the ->get_state() callback to read > back a duty cycle value of 0, which can confuse consumer drivers. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index ae11d8577f18..4113d5cd4c62 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > struct clk *clk_per; > void __iomem *mmio_base; > struct pwm_chip chip; > + > + /* > + * The driver cannot read the current duty cycle from the hardware if > + * the hardware is disabled. Cache the last programmed duty cycle > + * value to return in that case. > + */ > + unsigned int duty_cycle; > }; > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > tmp = NSEC_PER_SEC * (u64)(period + 2); > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - /* PWMSAR can be read only if PWM is enabled */ > - if (state->enabled) { > + /* > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > + * use the cached value. > + */ > + if (state->enabled) > val = readl(imx->mmio_base + MX3_PWMSAR); > - tmp = NSEC_PER_SEC * (u64)(val); > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > - } else { > - state->duty_cycle = 0; > - } > + else > + val = imx->duty_cycle; > + > + tmp = NSEC_PER_SEC * (u64)(val); > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); Is this right? It seems like the tmp and state->duty_cycle caltulations should be kept inside "if (state->enabled)" because if we set val to the duty_cycle in the else, I would think it is going to calculate this again. I think the 'else' should be 'state->duty_cycle = imx->duty_cycle' because we shouldn't need to recalculate this again. Am I missing something? adam > > if (!state->enabled) > pwm_imx27_clk_disable_unprepare(chip); > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > + /* > + * Store the duty cycle for future reference in cases where > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > + * is disabled). > + */ > + imx->duty_cycle = duty_cycles; > + > cr = MX3_PWMCR_PRESCALER_SET(prescale) | > MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > -- > 2.23.0 >
On 17. 10. 19 14:59, Thierry Reding wrote: > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: >> On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: >>> On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: >>>> On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: >>>>> On 17. 10. 19 10:10, Uwe Kleine-König wrote: >>>>>> A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let >>>>>> pwm_get_state() return the last implemented state")) changed the >>>>>> semantic of pwm_get_state() and disclosed an (as it seems) common >>>>>> problem in lowlevel PWM drivers. By not relying on the period and duty >>>>>> cycle being retrievable from a disabled PWM this type of problem is >>>>>> worked around. >>>>>> >>>>>> Apart from this issue only calling the pwm_get_state/pwm_apply_state >>>>>> combo once is also more effective. >>>>>> >>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>>>>> --- >>>>>> Hello, >>>>>> >>>>>> There are now two reports about 01ccf903edd6 breaking a backlight. As >>>>>> far as I understand the problem this is a combination of the backend pwm >>>>>> driver yielding surprising results and the pwm-bl driver doing things >>>>>> more complicated than necessary. >>>>>> >>>>>> So I guess this patch works around these problems. Still it would be >>>>>> interesting to find out the details in the imx driver that triggers the >>>>>> problem. So Adam, can you please instrument the pwm-imx27 driver to >>>>>> print *state at the beginning of pwm_imx27_apply() and the end of >>>>>> pwm_imx27_get_state() and provide the results? >>>>>> >>>>>> Note I only compile tested this change. >>>>> >>>>> Hi Uwe, >>>>> I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" >>>>> thread that I have a similar problem when you submitted this patch. >>>>> >>>>> So here are my few cents: >>>>> >>>>> My setup is as follows: >>>>> - imx6dl-yapp4-draco with i.MX6Solo >>>>> - backlight is controlled with inverted PWM signal >>>>> - max brightness level = 32, default brightness level set to 32 in DT. >>>>> >>>>> 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let >>>>> pwm_get_state() return the last implemented state): >>>>> >>>>> - System boots to userspace and backlight is enabled all the time from >>>>> power up. >>>>> >>>>> $ dmesg | grep state >>>>> [ 1.763381] get state end: -1811360608, enabled: 0 >>>> >>>> What is -1811360608? When I wrote "print *state" above, I thought about >>>> something like: >>>> >>>> pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", >>>> __func__, state->period, state->duty_cycle, state->polarity, state->enabled); >>>> >>>> A quick look into drivers/pwm/pwm-imx27.c shows that this is another >>>> driver that yields duty_cycle = 0 when the hardware is off. >>> >>> It seems to me like the best recourse to fix this for now would be to >>> patch up the drivers that return 0 when the hardware is off by caching >>> the currently configured duty cycle. >>> >>> How about the patch below? >>> >>> Thierry >>> >>> --- >8 --- >>> From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 >>> From: Thierry Reding <thierry.reding@gmail.com> >>> Date: Thu, 17 Oct 2019 12:56:00 +0200 >>> Subject: [PATCH] pwm: imx27: Cache duty cycle register value >>> >>> The hardware register containing the duty cycle value cannot be accessed >>> when the PWM is disabled. This causes the ->get_state() callback to read >>> back a duty cycle value of 0, which can confuse consumer drivers. >>> >>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> >>> --- >>> drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- >>> 1 file changed, 24 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c >>> index ae11d8577f18..4113d5cd4c62 100644 >>> --- a/drivers/pwm/pwm-imx27.c >>> +++ b/drivers/pwm/pwm-imx27.c >>> @@ -85,6 +85,13 @@ struct pwm_imx27_chip { >>> struct clk *clk_per; >>> void __iomem *mmio_base; >>> struct pwm_chip chip; >>> + >>> + /* >>> + * The driver cannot read the current duty cycle from the hardware if >>> + * the hardware is disabled. Cache the last programmed duty cycle >>> + * value to return in that case. >>> + */ >>> + unsigned int duty_cycle; >>> }; >>> >>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) >>> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, >>> tmp = NSEC_PER_SEC * (u64)(period + 2); >>> state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>> >>> - /* PWMSAR can be read only if PWM is enabled */ >>> - if (state->enabled) { >>> + /* >>> + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, >>> + * use the cached value. >>> + */ >>> + if (state->enabled) >>> val = readl(imx->mmio_base + MX3_PWMSAR); >>> - tmp = NSEC_PER_SEC * (u64)(val); >>> - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>> - } else { >>> - state->duty_cycle = 0; >>> - } >>> + else >>> + val = imx->duty_cycle; >>> + >>> + tmp = NSEC_PER_SEC * (u64)(val); >>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>> >>> if (!state->enabled) >>> pwm_imx27_clk_disable_unprepare(chip); >>> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>> writel(period_cycles, imx->mmio_base + MX3_PWMPR); >>> >>> + /* >>> + * Store the duty cycle for future reference in cases where >>> + * the MX3_PWMSAR register can't be read (i.e. when the PWM >>> + * is disabled). >>> + */ >>> + imx->duty_cycle = duty_cycles; >>> + >> >> I wonder if it would be more sensible to do this in the pwm core >> instead. Currently there are two drivers known with this problem. I >> wouldn't be surprised if there were more. > > I've inspected all the drivers and didn't spot any beyond cros-ec and > i.MX that have this problem. There's also no good way to do this in the > core, because the core doesn't know whether or not the driver is capable > of returning the correct duty cycle on hardare readout. So the core > would have to rely on state->duty_cycle that is passed in, but then the > offending commit becomes useless because the whole point was to return > the state as written to hardware (rather than the software state which > was being returned before that patch). > >> If we want to move clients to not rely on .period and .duty_cycle for a >> disabled PWM (do we?) a single change in the core is also beneficial >> compared to fixing several lowlevel drivers. > > These are really two orthogonal problems. We don't currently consider > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > prepared to do that at this point in the release cycle either. > > What this here has shown is that we have at least two drivers that don't > behave the way they are supposed to according to the API and they break > consumers. If they break for pwm-backlight, it's possible that they will > break for other consumers as well. So the right thing to do is fix the > two drivers that are broken. > > After -rc1 we no longer experiment. Instead we clean up the messes we've > made. We can revisit the other points once mainline is fixed. Hi Thierry, I just tried your patch with v5.4-rc3 with this result: root@hydraco:~# dmesg | grep pwm_ [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 Backlight is on with full brightness at this stage. root@hydraco:/sys/class/backlight/backlight# cat brightness 32 root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 Backlight goes down. root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 Backlight goes up to almost full brightness. root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 Backlight goes up to full brightness. So your patch does not solve my issue. The main problem I see is incorrect polarity setting. In my DT the pwm-backlight consumer requests PWM_POLARITY_INVERTED and period 500000ns. Though after reset the PWM HW registers are configured to normal polarity. This initial setting is read out and used by the consumer instead of the DT configuration. Michal
On Thu, Oct 17, 2019 at 8:30 AM Adam Ford <aford173@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 6:11 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > worked around. > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > combo once is also more effective. > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > --- > > > > > Hello, > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > more complicated than necessary. > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > interesting to find out the details in the imx driver that triggers the > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > Note I only compile tested this change. > > > > > > > > Hi Uwe, > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > So here are my few cents: > > > > > > > > My setup is as follows: > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > - backlight is controlled with inverted PWM signal > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state): > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > power up. > > > > > > > > $ dmesg | grep state > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > something like: > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > It seems to me like the best recourse to fix this for now would be to > > patch up the drivers that return 0 when the hardware is off by caching > > the currently configured duty cycle. > > > > How about the patch below? > > > > Thierry > > > > --- >8 --- > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > The hardware register containing the duty cycle value cannot be accessed > > when the PWM is disabled. This causes the ->get_state() callback to read > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> Your patch doesn't appear to being the PWM on by default, but I appear to be able to do stuff without the screen going blank, so I think we're making some progress. I unrolled the pwm_bl changes, but kept yours but I am not seeing any ability to change the brightness. Level 1-7 all appear to me to be the same. > > --- > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index ae11d8577f18..4113d5cd4c62 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > struct clk *clk_per; > > void __iomem *mmio_base; > > struct pwm_chip chip; > > + > > + /* > > + * The driver cannot read the current duty cycle from the hardware if > > + * the hardware is disabled. Cache the last programmed duty cycle > > + * value to return in that case. > > + */ > > + unsigned int duty_cycle; > > }; > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > - /* PWMSAR can be read only if PWM is enabled */ > > - if (state->enabled) { > > + /* > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > + * use the cached value. > > + */ > > + if (state->enabled) > > val = readl(imx->mmio_base + MX3_PWMSAR); > > - tmp = NSEC_PER_SEC * (u64)(val); > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - } else { > > - state->duty_cycle = 0; > > - } > > + else > > + val = imx->duty_cycle; > > + > > + tmp = NSEC_PER_SEC * (u64)(val); > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > Is this right? It seems like the tmp and state->duty_cycle > caltulations should be kept inside "if (state->enabled)" because if we > set val to the duty_cycle in the else, I would think it is going to > calculate this again. > > I think the 'else' should be 'state->duty_cycle = imx->duty_cycle' > because we shouldn't need to recalculate this again. > > Am I missing something? I figured out what I was missing. > > adam > > > > if (!state->enabled) > > pwm_imx27_clk_disable_unprepare(chip); > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > + /* > > + * Store the duty cycle for future reference in cases where > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > + * is disabled). > > + */ > > + imx->duty_cycle = duty_cycles; > > + > > cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > -- > > 2.23.0 > >
On Thu, Oct 17, 2019 at 3:11 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > pwm_get_state() return the last implemented state")) changed the > semantic of pwm_get_state() and disclosed an (as it seems) common > problem in lowlevel PWM drivers. By not relying on the period and duty > cycle being retrievable from a disabled PWM this type of problem is > worked around. > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > combo once is also more effective. Looking at the pwm core file in linunx/master, it looks like we're checking to see if all items are the same. If they are, we return 0. This make sense because we're not changing anything. If anything is different, we're using the cached value because we're not checking if/what has changed. If we want to change the polarity or the duty cycle, this appears to me like it would not change because it just uses the cached value. Somehow it seems to me like we should just blindly update the state to what we want and let the lower-level functions cache what they need to prevent the recalculation, but if we're changing duty cycle or polarity, I am not seeing how that will be updated. Both of those appear to be broken depending on which board is having an issue. adam > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > There are now two reports about 01ccf903edd6 breaking a backlight. As > far as I understand the problem this is a combination of the backend pwm > driver yielding surprising results and the pwm-bl driver doing things > more complicated than necessary. > > So I guess this patch works around these problems. Still it would be > interesting to find out the details in the imx driver that triggers the > problem. So Adam, can you please instrument the pwm-imx27 driver to > print *state at the beginning of pwm_imx27_apply() and the end of > pwm_imx27_get_state() and provide the results? > > Note I only compile tested this change. > > Best regards > Uwe > > drivers/video/backlight/pwm_bl.c | 34 +++++++++++--------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 746eebc411df..ddebd62b3978 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -42,10 +42,8 @@ struct pwm_bl_data { > > static void pwm_backlight_power_on(struct pwm_bl_data *pb) > { > - struct pwm_state state; > int err; > > - pwm_get_state(pb->pwm, &state); > if (pb->enabled) > return; > > @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > if (err < 0) > dev_err(pb->dev, "failed to enable power supply\n"); > > - state.enabled = true; > - pwm_apply_state(pb->pwm, &state); > - > if (pb->post_pwm_on_delay) > msleep(pb->post_pwm_on_delay); > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > { > - struct pwm_state state; > - > - pwm_get_state(pb->pwm, &state); > - if (!pb->enabled) > - return; > - > if (pb->enable_gpio) > gpiod_set_value_cansleep(pb->enable_gpio, 0); > > if (pb->pwm_off_delay) > msleep(pb->pwm_off_delay); > > - state.enabled = false; > - state.duty_cycle = 0; > - pwm_apply_state(pb->pwm, &state); > - > regulator_disable(pb->power_supply); > pb->enabled = false; > } > > -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state) > { > unsigned int lth = pb->lth_brightness; > - struct pwm_state state; > u64 duty_cycle; > > - pwm_get_state(pb->pwm, &state); > - > if (pb->levels) > duty_cycle = pb->levels[brightness]; > else > duty_cycle = brightness; > > - duty_cycle *= state.period - lth; > + duty_cycle *= state->period - lth; > do_div(duty_cycle, pb->scale); > > return duty_cycle + lth; > @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > if (brightness > 0) { > pwm_get_state(pb->pwm, &state); > - state.duty_cycle = compute_duty_cycle(pb, brightness); > + state.duty_cycle = compute_duty_cycle(pb, brightness, &state); > + state.enabled = true; > pwm_apply_state(pb->pwm, &state); > + > pwm_backlight_power_on(pb); > - } else > + } else { > pwm_backlight_power_off(pb); > > + pwm_get_state(pb->pwm, &state); > + state.enabled = false; > + state.duty_cycle = 0; > + pwm_apply_state(pb->pwm, &state); > + } > + > if (pb->notify_after) > pb->notify_after(pb->dev, brightness); > > -- > 2.23.0 >
On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > On 17. 10. 19 14:59, Thierry Reding wrote: > > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > > worked around. > > > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > > combo once is also more effective. > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > --- > > > > > > > Hello, > > > > > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > > > more complicated than necessary. > > > > > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > > > interesting to find out the details in the imx driver that triggers the > > > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > > > > > Note I only compile tested this change. > > > > > > > > > > > > Hi Uwe, > > > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > > > > > So here are my few cents: > > > > > > > > > > > > My setup is as follows: > > > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > > > - backlight is controlled with inverted PWM signal > > > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > > > pwm_get_state() return the last implemented state): > > > > > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > > > power up. > > > > > > > > > > > > $ dmesg | grep state > > > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > > > something like: > > > > > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > > > > > It seems to me like the best recourse to fix this for now would be to > > > > patch up the drivers that return 0 when the hardware is off by caching > > > > the currently configured duty cycle. > > > > > > > > How about the patch below? > > > > > > > > Thierry > > > > > > > > --- >8 --- > > > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > > > From: Thierry Reding <thierry.reding@gmail.com> > > > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > > > > > The hardware register containing the duty cycle value cannot be accessed > > > > when the PWM is disabled. This causes the ->get_state() callback to read > > > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > --- > > > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > index ae11d8577f18..4113d5cd4c62 100644 > > > > --- a/drivers/pwm/pwm-imx27.c > > > > +++ b/drivers/pwm/pwm-imx27.c > > > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > > > struct clk *clk_per; > > > > void __iomem *mmio_base; > > > > struct pwm_chip chip; > > > > + > > > > + /* > > > > + * The driver cannot read the current duty cycle from the hardware if > > > > + * the hardware is disabled. Cache the last programmed duty cycle > > > > + * value to return in that case. > > > > + */ > > > > + unsigned int duty_cycle; > > > > }; > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > - /* PWMSAR can be read only if PWM is enabled */ > > > > - if (state->enabled) { > > > > + /* > > > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > > > + * use the cached value. > > > > + */ > > > > + if (state->enabled) > > > > val = readl(imx->mmio_base + MX3_PWMSAR); > > > > - tmp = NSEC_PER_SEC * (u64)(val); > > > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > - } else { > > > > - state->duty_cycle = 0; > > > > - } > > > > + else > > > > + val = imx->duty_cycle; > > > > + > > > > + tmp = NSEC_PER_SEC * (u64)(val); > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > if (!state->enabled) > > > > pwm_imx27_clk_disable_unprepare(chip); > > > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > + /* > > > > + * Store the duty cycle for future reference in cases where > > > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > > + * is disabled). > > > > + */ > > > > + imx->duty_cycle = duty_cycles; > > > > + > > > > > > I wonder if it would be more sensible to do this in the pwm core > > > instead. Currently there are two drivers known with this problem. I > > > wouldn't be surprised if there were more. > > > > I've inspected all the drivers and didn't spot any beyond cros-ec and > > i.MX that have this problem. There's also no good way to do this in the > > core, because the core doesn't know whether or not the driver is capable > > of returning the correct duty cycle on hardare readout. So the core > > would have to rely on state->duty_cycle that is passed in, but then the > > offending commit becomes useless because the whole point was to return > > the state as written to hardware (rather than the software state which > > was being returned before that patch). > > > > > If we want to move clients to not rely on .period and .duty_cycle for a > > > disabled PWM (do we?) a single change in the core is also beneficial > > > compared to fixing several lowlevel drivers. > > > > These are really two orthogonal problems. We don't currently consider > > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > > prepared to do that at this point in the release cycle either. > > > > What this here has shown is that we have at least two drivers that don't > > behave the way they are supposed to according to the API and they break > > consumers. If they break for pwm-backlight, it's possible that they will > > break for other consumers as well. So the right thing to do is fix the > > two drivers that are broken. > > > > After -rc1 we no longer experiment. Instead we clean up the messes we've > > made. We can revisit the other points once mainline is fixed. > > Hi Thierry, > I just tried your patch with v5.4-rc3 with this result: > > root@hydraco:~# dmesg | grep pwm_ > [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 Okay... this is interesting. If I understand correctly, that first line here is where the initial hardware readout happens. The second one is the first time when the backlight is configured, so it sets period and polarity. But then for some reason when we read out after that to read what state was written... we see that actually nothing was written at all. And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we don't actually program any of the registers, so it's not a surprise that things fall apart. > [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > Backlight is on with full brightness at this stage. > > root@hydraco:/sys/class/backlight/backlight# cat brightness > 32 > > root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > Backlight goes down. > > root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > Backlight goes up to almost full brightness. > > root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > > Backlight goes up to full brightness. > > So your patch does not solve my issue. > > The main problem I see is incorrect polarity setting. In my DT > the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > period 500000ns. Though after reset the PWM HW registers are > configured to normal polarity. This initial setting is read out > and used by the consumer instead of the DT configuration. So the problem with the i.MX driver is that it doesn't actually write the full state to the hardware and therefore the patch that caused these things to break reads back an incomplete state. So we've basically got two options: 1) make sure the hardware state is fully written or 2) make sure that we return the cached state. I think 2) doesn't really make sense because it is conflicts with the purpose of the ->get_state() callback. The only time where we should be returning cached data is if the hardware registers don't contain the information (as in the case of the cros-ec driver) or if we can't access it for other reasons (such as in the case of i.MX's duty cycle). Does the attached patch help with your issue? The idea is to always write the full state to the hardware, even if period and duty cycle are unused when the PWM is disabled. That's really the kind of contract that we have added with the offending patch in the core. It looks like all other drivers handle this more or less correctly, so if we only need to fix up cros-ec and i.MX this seems like a realistic way to fix things up. If other drivers are problematic in this regard, we should probably revert and then fix the drivers before we can apply that patch again. Thierry --- >8 --- From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 From: Thierry Reding <thierry.reding@gmail.com> Date: Thu, 17 Oct 2019 17:11:41 +0200 Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware The i.MX driver currently uses a shortcut and doesn't write all of the state through to the hardware when the PWM is disabled. This causes an inconsistent state to be read back by consumers with the result of them malfunctioning. Fix this by always writing the full state through to the hardware registers so that the correct state can always be read back. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 61 deletions(-) diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 4113d5cd4c62..59d8b1289808 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, pwm_get_state(pwm, &cstate); - if (state->enabled) { - c = clk_get_rate(imx->clk_per); - c *= state->period; - - do_div(c, 1000000000); - period_cycles = c; - - prescale = period_cycles / 0x10000 + 1; - - period_cycles /= prescale; - c = (unsigned long long)period_cycles * state->duty_cycle; - do_div(c, state->period); - duty_cycles = c; - - /* - * according to imx pwm RM, the real period value should be - * PERIOD value in PWMPR plus 2. - */ - if (period_cycles > 2) - period_cycles -= 2; - else - period_cycles = 0; - - /* - * Wait for a free FIFO slot if the PWM is already enabled, and - * flush the FIFO if the PWM was disabled and is about to be - * enabled. - */ - if (cstate.enabled) { - pwm_imx27_wait_fifo_slot(chip, pwm); - } else { - ret = pwm_imx27_clk_prepare_enable(chip); - if (ret) - return ret; - - pwm_imx27_sw_reset(chip); - } - - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); - writel(period_cycles, imx->mmio_base + MX3_PWMPR); - - /* - * Store the duty cycle for future reference in cases where - * the MX3_PWMSAR register can't be read (i.e. when the PWM - * is disabled). - */ - imx->duty_cycle = duty_cycles; - - cr = MX3_PWMCR_PRESCALER_SET(prescale) | - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; - - if (state->polarity == PWM_POLARITY_INVERSED) - cr |= FIELD_PREP(MX3_PWMCR_POUTC, - MX3_PWMCR_POUTC_INVERTED); - - writel(cr, imx->mmio_base + MX3_PWMCR); - } else if (cstate.enabled) { - writel(0, imx->mmio_base + MX3_PWMCR); + c = clk_get_rate(imx->clk_per); + c *= state->period; - pwm_imx27_clk_disable_unprepare(chip); + do_div(c, 1000000000); + period_cycles = c; + + prescale = period_cycles / 0x10000 + 1; + + period_cycles /= prescale; + c = (unsigned long long)period_cycles * state->duty_cycle; + do_div(c, state->period); + duty_cycles = c; + + /* + * according to imx pwm RM, the real period value should be PERIOD + * value in PWMPR plus 2. + */ + if (period_cycles > 2) + period_cycles -= 2; + else + period_cycles = 0; + + /* + * Wait for a free FIFO slot if the PWM is already enabled, and flush + * the FIFO if the PWM was disabled and is about to be enabled. + */ + if (cstate.enabled) { + pwm_imx27_wait_fifo_slot(chip, pwm); + } else { + ret = pwm_imx27_clk_prepare_enable(chip); + if (ret) + return ret; + + pwm_imx27_sw_reset(chip); } + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); + writel(period_cycles, imx->mmio_base + MX3_PWMPR); + + /* + * Store the duty cycle for future reference in cases where the + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). + */ + imx->duty_cycle = duty_cycles; + + cr = MX3_PWMCR_PRESCALER_SET(prescale) | + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | + MX3_PWMCR_DBGEN; + + if (state->polarity == PWM_POLARITY_INVERSED) + cr |= FIELD_PREP(MX3_PWMCR_POUTC, + MX3_PWMCR_POUTC_INVERTED); + + if (state->enabled) + cr |= MX3_PWMCR_EN; + + writel(cr, imx->mmio_base + MX3_PWMCR); + + if (!state->enabled && cstate.enabled) + pwm_imx27_clk_disable_unprepare(chip); + return 0; }
On Thu, Oct 17, 2019 at 10:14 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > > On 17. 10. 19 14:59, Thierry Reding wrote: > > > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > > > worked around. > > > > > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > > > combo once is also more effective. > > > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > > --- > > > > > > > > Hello, > > > > > > > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > > > > more complicated than necessary. > > > > > > > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > > > > interesting to find out the details in the imx driver that triggers the > > > > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > > > > > > > Note I only compile tested this change. > > > > > > > > > > > > > > Hi Uwe, > > > > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > > > > > > > So here are my few cents: > > > > > > > > > > > > > > My setup is as follows: > > > > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > > > > - backlight is controlled with inverted PWM signal > > > > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > > > > pwm_get_state() return the last implemented state): > > > > > > > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > > > > power up. > > > > > > > > > > > > > > $ dmesg | grep state > > > > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > > > > something like: > > > > > > > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > > > > > > > It seems to me like the best recourse to fix this for now would be to > > > > > patch up the drivers that return 0 when the hardware is off by caching > > > > > the currently configured duty cycle. > > > > > > > > > > How about the patch below? > > > > > > > > > > Thierry > > > > > > > > > > --- >8 --- > > > > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > > > > From: Thierry Reding <thierry.reding@gmail.com> > > > > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > > > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > > > > > > > The hardware register containing the duty cycle value cannot be accessed > > > > > when the PWM is disabled. This causes the ->get_state() callback to read > > > > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > > --- > > > > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > > index ae11d8577f18..4113d5cd4c62 100644 > > > > > --- a/drivers/pwm/pwm-imx27.c > > > > > +++ b/drivers/pwm/pwm-imx27.c > > > > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > > > > struct clk *clk_per; > > > > > void __iomem *mmio_base; > > > > > struct pwm_chip chip; > > > > > + > > > > > + /* > > > > > + * The driver cannot read the current duty cycle from the hardware if > > > > > + * the hardware is disabled. Cache the last programmed duty cycle > > > > > + * value to return in that case. > > > > > + */ > > > > > + unsigned int duty_cycle; > > > > > }; > > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > > > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > > > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > > > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > - /* PWMSAR can be read only if PWM is enabled */ > > > > > - if (state->enabled) { > > > > > + /* > > > > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > > > > + * use the cached value. > > > > > + */ > > > > > + if (state->enabled) > > > > > val = readl(imx->mmio_base + MX3_PWMSAR); > > > > > - tmp = NSEC_PER_SEC * (u64)(val); > > > > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > - } else { > > > > > - state->duty_cycle = 0; > > > > > - } > > > > > + else > > > > > + val = imx->duty_cycle; > > > > > + > > > > > + tmp = NSEC_PER_SEC * (u64)(val); > > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > if (!state->enabled) > > > > > pwm_imx27_clk_disable_unprepare(chip); > > > > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > > + /* > > > > > + * Store the duty cycle for future reference in cases where > > > > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > > > + * is disabled). > > > > > + */ > > > > > + imx->duty_cycle = duty_cycles; > > > > > + > > > > > > > > I wonder if it would be more sensible to do this in the pwm core > > > > instead. Currently there are two drivers known with this problem. I > > > > wouldn't be surprised if there were more. > > > > > > I've inspected all the drivers and didn't spot any beyond cros-ec and > > > i.MX that have this problem. There's also no good way to do this in the > > > core, because the core doesn't know whether or not the driver is capable > > > of returning the correct duty cycle on hardare readout. So the core > > > would have to rely on state->duty_cycle that is passed in, but then the > > > offending commit becomes useless because the whole point was to return > > > the state as written to hardware (rather than the software state which > > > was being returned before that patch). > > > > > > > If we want to move clients to not rely on .period and .duty_cycle for a > > > > disabled PWM (do we?) a single change in the core is also beneficial > > > > compared to fixing several lowlevel drivers. > > > > > > These are really two orthogonal problems. We don't currently consider > > > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > > > prepared to do that at this point in the release cycle either. > > > > > > What this here has shown is that we have at least two drivers that don't > > > behave the way they are supposed to according to the API and they break > > > consumers. If they break for pwm-backlight, it's possible that they will > > > break for other consumers as well. So the right thing to do is fix the > > > two drivers that are broken. > > > > > > After -rc1 we no longer experiment. Instead we clean up the messes we've > > > made. We can revisit the other points once mainline is fixed. > > > > Hi Thierry, > > I just tried your patch with v5.4-rc3 with this result: > > > > root@hydraco:~# dmesg | grep pwm_ > > [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > Okay... this is interesting. If I understand correctly, that first line > here is where the initial hardware readout happens. The second one is > the first time when the backlight is configured, so it sets period and > polarity. But then for some reason when we read out after that to read > what state was written... we see that actually nothing was written at > all. > > And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we > don't actually program any of the registers, so it's not a surprise that > things fall apart. > > > [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > > [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > > [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > Backlight is on with full brightness at this stage. > > > > root@hydraco:/sys/class/backlight/backlight# cat brightness > > 32 > > > > root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > > [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > Backlight goes down. > > > > root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > > [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > Backlight goes up to almost full brightness. > > > > root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > > [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > > > > Backlight goes up to full brightness. > > > > So your patch does not solve my issue. > > > > The main problem I see is incorrect polarity setting. In my DT > > the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > > period 500000ns. Though after reset the PWM HW registers are > > configured to normal polarity. This initial setting is read out > > and used by the consumer instead of the DT configuration. > > So the problem with the i.MX driver is that it doesn't actually write > the full state to the hardware and therefore the patch that caused these > things to break reads back an incomplete state. So we've basically got > two options: 1) make sure the hardware state is fully written or 2) make > sure that we return the cached state. > > I think 2) doesn't really make sense because it is conflicts with the > purpose of the ->get_state() callback. The only time where we should be > returning cached data is if the hardware registers don't contain the > information (as in the case of the cros-ec driver) or if we can't access > it for other reasons (such as in the case of i.MX's duty cycle). > > Does the attached patch help with your issue? The idea is to always > write the full state to the hardware, even if period and duty cycle are > unused when the PWM is disabled. That's really the kind of contract that > we have added with the offending patch in the core. > > It looks like all other drivers handle this more or less correctly, so > if we only need to fix up cros-ec and i.MX this seems like a realistic > way to fix things up. If other drivers are problematic in this regard, > we should probably revert and then fix the drivers before we can apply > that patch again. This patch combined with your previous patch appears to have worked. If you end up sending a patch series to fix this, go ahead and add Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd Thank you adam > > Thierry > > --- >8 --- > From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <thierry.reding@gmail.com> > Date: Thu, 17 Oct 2019 17:11:41 +0200 > Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware > > The i.MX driver currently uses a shortcut and doesn't write all of the > state through to the hardware when the PWM is disabled. This causes an > inconsistent state to be read back by consumers with the result of them > malfunctioning. > > Fix this by always writing the full state through to the hardware > registers so that the correct state can always be read back. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- > 1 file changed, 59 insertions(+), 61 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 4113d5cd4c62..59d8b1289808 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > pwm_get_state(pwm, &cstate); > > - if (state->enabled) { > - c = clk_get_rate(imx->clk_per); > - c *= state->period; > - > - do_div(c, 1000000000); > - period_cycles = c; > - > - prescale = period_cycles / 0x10000 + 1; > - > - period_cycles /= prescale; > - c = (unsigned long long)period_cycles * state->duty_cycle; > - do_div(c, state->period); > - duty_cycles = c; > - > - /* > - * according to imx pwm RM, the real period value should be > - * PERIOD value in PWMPR plus 2. > - */ > - if (period_cycles > 2) > - period_cycles -= 2; > - else > - period_cycles = 0; > - > - /* > - * Wait for a free FIFO slot if the PWM is already enabled, and > - * flush the FIFO if the PWM was disabled and is about to be > - * enabled. > - */ > - if (cstate.enabled) { > - pwm_imx27_wait_fifo_slot(chip, pwm); > - } else { > - ret = pwm_imx27_clk_prepare_enable(chip); > - if (ret) > - return ret; > - > - pwm_imx27_sw_reset(chip); > - } > - > - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > - writel(period_cycles, imx->mmio_base + MX3_PWMPR); > - > - /* > - * Store the duty cycle for future reference in cases where > - * the MX3_PWMSAR register can't be read (i.e. when the PWM > - * is disabled). > - */ > - imx->duty_cycle = duty_cycles; > - > - cr = MX3_PWMCR_PRESCALER_SET(prescale) | > - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; > - > - if (state->polarity == PWM_POLARITY_INVERSED) > - cr |= FIELD_PREP(MX3_PWMCR_POUTC, > - MX3_PWMCR_POUTC_INVERTED); > - > - writel(cr, imx->mmio_base + MX3_PWMCR); > - } else if (cstate.enabled) { > - writel(0, imx->mmio_base + MX3_PWMCR); > + c = clk_get_rate(imx->clk_per); > + c *= state->period; > > - pwm_imx27_clk_disable_unprepare(chip); > + do_div(c, 1000000000); > + period_cycles = c; > + > + prescale = period_cycles / 0x10000 + 1; > + > + period_cycles /= prescale; > + c = (unsigned long long)period_cycles * state->duty_cycle; > + do_div(c, state->period); > + duty_cycles = c; > + > + /* > + * according to imx pwm RM, the real period value should be PERIOD > + * value in PWMPR plus 2. > + */ > + if (period_cycles > 2) > + period_cycles -= 2; > + else > + period_cycles = 0; > + > + /* > + * Wait for a free FIFO slot if the PWM is already enabled, and flush > + * the FIFO if the PWM was disabled and is about to be enabled. > + */ > + if (cstate.enabled) { > + pwm_imx27_wait_fifo_slot(chip, pwm); > + } else { > + ret = pwm_imx27_clk_prepare_enable(chip); > + if (ret) > + return ret; > + > + pwm_imx27_sw_reset(chip); > } > > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > + > + /* > + * Store the duty cycle for future reference in cases where the > + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). > + */ > + imx->duty_cycle = duty_cycles; > + > + cr = MX3_PWMCR_PRESCALER_SET(prescale) | > + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > + MX3_PWMCR_DBGEN; > + > + if (state->polarity == PWM_POLARITY_INVERSED) > + cr |= FIELD_PREP(MX3_PWMCR_POUTC, > + MX3_PWMCR_POUTC_INVERTED); > + > + if (state->enabled) > + cr |= MX3_PWMCR_EN; > + > + writel(cr, imx->mmio_base + MX3_PWMCR); > + > + if (!state->enabled && cstate.enabled) > + pwm_imx27_clk_disable_unprepare(chip); > + > return 0; > } > > -- > 2.23.0 >
On Thu, Oct 17, 2019 at 12:07:21PM -0500, Adam Ford wrote: > On Thu, Oct 17, 2019 at 10:14 AM Thierry Reding > <thierry.reding@gmail.com> wrote: > > > > On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > > > On 17. 10. 19 14:59, Thierry Reding wrote: > > > > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > > > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > > > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > > > > worked around. > > > > > > > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > > > > combo once is also more effective. > > > > > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > > > --- > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > > > > > more complicated than necessary. > > > > > > > > > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > > > > > interesting to find out the details in the imx driver that triggers the > > > > > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > > > > > > > > > Note I only compile tested this change. > > > > > > > > > > > > > > > > Hi Uwe, > > > > > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > > > > > > > > > So here are my few cents: > > > > > > > > > > > > > > > > My setup is as follows: > > > > > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > > > > > - backlight is controlled with inverted PWM signal > > > > > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > > > > > pwm_get_state() return the last implemented state): > > > > > > > > > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > > > > > power up. > > > > > > > > > > > > > > > > $ dmesg | grep state > > > > > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > > > > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > > > > > something like: > > > > > > > > > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > > > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > > > > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > > > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > > > > > > > > > It seems to me like the best recourse to fix this for now would be to > > > > > > patch up the drivers that return 0 when the hardware is off by caching > > > > > > the currently configured duty cycle. > > > > > > > > > > > > How about the patch below? > > > > > > > > > > > > Thierry > > > > > > > > > > > > --- >8 --- > > > > > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > > > > > From: Thierry Reding <thierry.reding@gmail.com> > > > > > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > > > > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > > > > > > > > > The hardware register containing the duty cycle value cannot be accessed > > > > > > when the PWM is disabled. This causes the ->get_state() callback to read > > > > > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > > > --- > > > > > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > > > index ae11d8577f18..4113d5cd4c62 100644 > > > > > > --- a/drivers/pwm/pwm-imx27.c > > > > > > +++ b/drivers/pwm/pwm-imx27.c > > > > > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > > > > > struct clk *clk_per; > > > > > > void __iomem *mmio_base; > > > > > > struct pwm_chip chip; > > > > > > + > > > > > > + /* > > > > > > + * The driver cannot read the current duty cycle from the hardware if > > > > > > + * the hardware is disabled. Cache the last programmed duty cycle > > > > > > + * value to return in that case. > > > > > > + */ > > > > > > + unsigned int duty_cycle; > > > > > > }; > > > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > > > > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > > > > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > > > > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > - /* PWMSAR can be read only if PWM is enabled */ > > > > > > - if (state->enabled) { > > > > > > + /* > > > > > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > > > > > + * use the cached value. > > > > > > + */ > > > > > > + if (state->enabled) > > > > > > val = readl(imx->mmio_base + MX3_PWMSAR); > > > > > > - tmp = NSEC_PER_SEC * (u64)(val); > > > > > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > - } else { > > > > > > - state->duty_cycle = 0; > > > > > > - } > > > > > > + else > > > > > > + val = imx->duty_cycle; > > > > > > + > > > > > > + tmp = NSEC_PER_SEC * (u64)(val); > > > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > if (!state->enabled) > > > > > > pwm_imx27_clk_disable_unprepare(chip); > > > > > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > > > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > > > + /* > > > > > > + * Store the duty cycle for future reference in cases where > > > > > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > > > > + * is disabled). > > > > > > + */ > > > > > > + imx->duty_cycle = duty_cycles; > > > > > > + > > > > > > > > > > I wonder if it would be more sensible to do this in the pwm core > > > > > instead. Currently there are two drivers known with this problem. I > > > > > wouldn't be surprised if there were more. > > > > > > > > I've inspected all the drivers and didn't spot any beyond cros-ec and > > > > i.MX that have this problem. There's also no good way to do this in the > > > > core, because the core doesn't know whether or not the driver is capable > > > > of returning the correct duty cycle on hardare readout. So the core > > > > would have to rely on state->duty_cycle that is passed in, but then the > > > > offending commit becomes useless because the whole point was to return > > > > the state as written to hardware (rather than the software state which > > > > was being returned before that patch). > > > > > > > > > If we want to move clients to not rely on .period and .duty_cycle for a > > > > > disabled PWM (do we?) a single change in the core is also beneficial > > > > > compared to fixing several lowlevel drivers. > > > > > > > > These are really two orthogonal problems. We don't currently consider > > > > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > > > > prepared to do that at this point in the release cycle either. > > > > > > > > What this here has shown is that we have at least two drivers that don't > > > > behave the way they are supposed to according to the API and they break > > > > consumers. If they break for pwm-backlight, it's possible that they will > > > > break for other consumers as well. So the right thing to do is fix the > > > > two drivers that are broken. > > > > > > > > After -rc1 we no longer experiment. Instead we clean up the messes we've > > > > made. We can revisit the other points once mainline is fixed. > > > > > > Hi Thierry, > > > I just tried your patch with v5.4-rc3 with this result: > > > > > > root@hydraco:~# dmesg | grep pwm_ > > > [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > > [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > > Okay... this is interesting. If I understand correctly, that first line > > here is where the initial hardware readout happens. The second one is > > the first time when the backlight is configured, so it sets period and > > polarity. But then for some reason when we read out after that to read > > what state was written... we see that actually nothing was written at > > all. > > > > And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we > > don't actually program any of the registers, so it's not a surprise that > > things fall apart. > > > > > [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > > > [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > > > Backlight is on with full brightness at this stage. > > > > > > root@hydraco:/sys/class/backlight/backlight# cat brightness > > > 32 > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > > > [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > > > Backlight goes down. > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > > > [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > > > Backlight goes up to almost full brightness. > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > > > [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > > > > > > Backlight goes up to full brightness. > > > > > > So your patch does not solve my issue. > > > > > > The main problem I see is incorrect polarity setting. In my DT > > > the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > > > period 500000ns. Though after reset the PWM HW registers are > > > configured to normal polarity. This initial setting is read out > > > and used by the consumer instead of the DT configuration. > > > > So the problem with the i.MX driver is that it doesn't actually write > > the full state to the hardware and therefore the patch that caused these > > things to break reads back an incomplete state. So we've basically got > > two options: 1) make sure the hardware state is fully written or 2) make > > sure that we return the cached state. > > > > I think 2) doesn't really make sense because it is conflicts with the > > purpose of the ->get_state() callback. The only time where we should be > > returning cached data is if the hardware registers don't contain the > > information (as in the case of the cros-ec driver) or if we can't access > > it for other reasons (such as in the case of i.MX's duty cycle). > > > > Does the attached patch help with your issue? The idea is to always > > write the full state to the hardware, even if period and duty cycle are > > unused when the PWM is disabled. That's really the kind of contract that > > we have added with the offending patch in the core. > > > > It looks like all other drivers handle this more or less correctly, so > > if we only need to fix up cros-ec and i.MX this seems like a realistic > > way to fix things up. If other drivers are problematic in this regard, > > we should probably revert and then fix the drivers before we can apply > > that patch again. > > This patch combined with your previous patch appears to have worked. > If you end up sending a patch series to fix this, go ahead and add > > Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd Excellent! Thanks for testing this. I'll wait until tomorrow to see if there's some feedback from Enric for the cros-ec change. I'll send out the total of three patches again in the hopes that those are really the only two cases that are broken. Thierry > > --- >8 --- > > From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Thu, 17 Oct 2019 17:11:41 +0200 > > Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware > > > > The i.MX driver currently uses a shortcut and doesn't write all of the > > state through to the hardware when the PWM is disabled. This causes an > > inconsistent state to be read back by consumers with the result of them > > malfunctioning. > > > > Fix this by always writing the full state through to the hardware > > registers so that the correct state can always be read back. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- > > 1 file changed, 59 insertions(+), 61 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index 4113d5cd4c62..59d8b1289808 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > pwm_get_state(pwm, &cstate); > > > > - if (state->enabled) { > > - c = clk_get_rate(imx->clk_per); > > - c *= state->period; > > - > > - do_div(c, 1000000000); > > - period_cycles = c; > > - > > - prescale = period_cycles / 0x10000 + 1; > > - > > - period_cycles /= prescale; > > - c = (unsigned long long)period_cycles * state->duty_cycle; > > - do_div(c, state->period); > > - duty_cycles = c; > > - > > - /* > > - * according to imx pwm RM, the real period value should be > > - * PERIOD value in PWMPR plus 2. > > - */ > > - if (period_cycles > 2) > > - period_cycles -= 2; > > - else > > - period_cycles = 0; > > - > > - /* > > - * Wait for a free FIFO slot if the PWM is already enabled, and > > - * flush the FIFO if the PWM was disabled and is about to be > > - * enabled. > > - */ > > - if (cstate.enabled) { > > - pwm_imx27_wait_fifo_slot(chip, pwm); > > - } else { > > - ret = pwm_imx27_clk_prepare_enable(chip); > > - if (ret) > > - return ret; > > - > > - pwm_imx27_sw_reset(chip); > > - } > > - > > - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > - writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > - > > - /* > > - * Store the duty cycle for future reference in cases where > > - * the MX3_PWMSAR register can't be read (i.e. when the PWM > > - * is disabled). > > - */ > > - imx->duty_cycle = duty_cycles; > > - > > - cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; > > - > > - if (state->polarity == PWM_POLARITY_INVERSED) > > - cr |= FIELD_PREP(MX3_PWMCR_POUTC, > > - MX3_PWMCR_POUTC_INVERTED); > > - > > - writel(cr, imx->mmio_base + MX3_PWMCR); > > - } else if (cstate.enabled) { > > - writel(0, imx->mmio_base + MX3_PWMCR); > > + c = clk_get_rate(imx->clk_per); > > + c *= state->period; > > > > - pwm_imx27_clk_disable_unprepare(chip); > > + do_div(c, 1000000000); > > + period_cycles = c; > > + > > + prescale = period_cycles / 0x10000 + 1; > > + > > + period_cycles /= prescale; > > + c = (unsigned long long)period_cycles * state->duty_cycle; > > + do_div(c, state->period); > > + duty_cycles = c; > > + > > + /* > > + * according to imx pwm RM, the real period value should be PERIOD > > + * value in PWMPR plus 2. > > + */ > > + if (period_cycles > 2) > > + period_cycles -= 2; > > + else > > + period_cycles = 0; > > + > > + /* > > + * Wait for a free FIFO slot if the PWM is already enabled, and flush > > + * the FIFO if the PWM was disabled and is about to be enabled. > > + */ > > + if (cstate.enabled) { > > + pwm_imx27_wait_fifo_slot(chip, pwm); > > + } else { > > + ret = pwm_imx27_clk_prepare_enable(chip); > > + if (ret) > > + return ret; > > + > > + pwm_imx27_sw_reset(chip); > > } > > > > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > + > > + /* > > + * Store the duty cycle for future reference in cases where the > > + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). > > + */ > > + imx->duty_cycle = duty_cycles; > > + > > + cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > + MX3_PWMCR_DBGEN; > > + > > + if (state->polarity == PWM_POLARITY_INVERSED) > > + cr |= FIELD_PREP(MX3_PWMCR_POUTC, > > + MX3_PWMCR_POUTC_INVERTED); > > + > > + if (state->enabled) > > + cr |= MX3_PWMCR_EN; > > + > > + writel(cr, imx->mmio_base + MX3_PWMCR); > > + > > + if (!state->enabled && cstate.enabled) > > + pwm_imx27_clk_disable_unprepare(chip); > > + > > return 0; > > } > > > > -- > > 2.23.0 > >
On Thu, Oct 17, 2019 at 12:13 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Oct 17, 2019 at 12:07:21PM -0500, Adam Ford wrote: > > On Thu, Oct 17, 2019 at 10:14 AM Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > > > > On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > > > > On 17. 10. 19 14:59, Thierry Reding wrote: > > > > > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > > > > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > > > > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > > > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > > > > > worked around. > > > > > > > > > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > > > > > combo once is also more effective. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > > > > --- > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > > > > > > more complicated than necessary. > > > > > > > > > > > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > > > > > > interesting to find out the details in the imx driver that triggers the > > > > > > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > > > > > > > > > > > Note I only compile tested this change. > > > > > > > > > > > > > > > > > > Hi Uwe, > > > > > > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > > > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > > > > > > > > > > > So here are my few cents: > > > > > > > > > > > > > > > > > > My setup is as follows: > > > > > > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > > > > > > - backlight is controlled with inverted PWM signal > > > > > > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > > > > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > > > > > > pwm_get_state() return the last implemented state): > > > > > > > > > > > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > > > > > > power up. > > > > > > > > > > > > > > > > > > $ dmesg | grep state > > > > > > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > > > > > > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > > > > > > something like: > > > > > > > > > > > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > > > > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > > > > > > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > > > > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > > > > > > > > > > > It seems to me like the best recourse to fix this for now would be to > > > > > > > patch up the drivers that return 0 when the hardware is off by caching > > > > > > > the currently configured duty cycle. > > > > > > > > > > > > > > How about the patch below? > > > > > > > > > > > > > > Thierry > > > > > > > > > > > > > > --- >8 --- > > > > > > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > > > > > > From: Thierry Reding <thierry.reding@gmail.com> > > > > > > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > > > > > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > > > > > > > > > > > The hardware register containing the duty cycle value cannot be accessed > > > > > > > when the PWM is disabled. This causes the ->get_state() callback to read > > > > > > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > > > > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > > > > --- > > > > > > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > > > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > > > > index ae11d8577f18..4113d5cd4c62 100644 > > > > > > > --- a/drivers/pwm/pwm-imx27.c > > > > > > > +++ b/drivers/pwm/pwm-imx27.c > > > > > > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > > > > > > struct clk *clk_per; > > > > > > > void __iomem *mmio_base; > > > > > > > struct pwm_chip chip; > > > > > > > + > > > > > > > + /* > > > > > > > + * The driver cannot read the current duty cycle from the hardware if > > > > > > > + * the hardware is disabled. Cache the last programmed duty cycle > > > > > > > + * value to return in that case. > > > > > > > + */ > > > > > > > + unsigned int duty_cycle; > > > > > > > }; > > > > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > > > > > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > > > > > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > > > > > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > > - /* PWMSAR can be read only if PWM is enabled */ > > > > > > > - if (state->enabled) { > > > > > > > + /* > > > > > > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > > > > > > + * use the cached value. > > > > > > > + */ > > > > > > > + if (state->enabled) > > > > > > > val = readl(imx->mmio_base + MX3_PWMSAR); > > > > > > > - tmp = NSEC_PER_SEC * (u64)(val); > > > > > > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > > - } else { > > > > > > > - state->duty_cycle = 0; > > > > > > > - } > > > > > > > + else > > > > > > > + val = imx->duty_cycle; > > > > > > > + > > > > > > > + tmp = NSEC_PER_SEC * (u64)(val); > > > > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > > > if (!state->enabled) > > > > > > > pwm_imx27_clk_disable_unprepare(chip); > > > > > > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > > > > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > > > > + /* > > > > > > > + * Store the duty cycle for future reference in cases where > > > > > > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > > > > > + * is disabled). > > > > > > > + */ > > > > > > > + imx->duty_cycle = duty_cycles; > > > > > > > + > > > > > > > > > > > > I wonder if it would be more sensible to do this in the pwm core > > > > > > instead. Currently there are two drivers known with this problem. I > > > > > > wouldn't be surprised if there were more. > > > > > > > > > > I've inspected all the drivers and didn't spot any beyond cros-ec and > > > > > i.MX that have this problem. There's also no good way to do this in the > > > > > core, because the core doesn't know whether or not the driver is capable > > > > > of returning the correct duty cycle on hardare readout. So the core > > > > > would have to rely on state->duty_cycle that is passed in, but then the > > > > > offending commit becomes useless because the whole point was to return > > > > > the state as written to hardware (rather than the software state which > > > > > was being returned before that patch). > > > > > > > > > > > If we want to move clients to not rely on .period and .duty_cycle for a > > > > > > disabled PWM (do we?) a single change in the core is also beneficial > > > > > > compared to fixing several lowlevel drivers. > > > > > > > > > > These are really two orthogonal problems. We don't currently consider > > > > > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > > > > > prepared to do that at this point in the release cycle either. > > > > > > > > > > What this here has shown is that we have at least two drivers that don't > > > > > behave the way they are supposed to according to the API and they break > > > > > consumers. If they break for pwm-backlight, it's possible that they will > > > > > break for other consumers as well. So the right thing to do is fix the > > > > > two drivers that are broken. > > > > > > > > > > After -rc1 we no longer experiment. Instead we clean up the messes we've > > > > > made. We can revisit the other points once mainline is fixed. > > > > > > > > Hi Thierry, > > > > I just tried your patch with v5.4-rc3 with this result: > > > > > > > > root@hydraco:~# dmesg | grep pwm_ > > > > [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > > [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > > > [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > > > > Okay... this is interesting. If I understand correctly, that first line > > > here is where the initial hardware readout happens. The second one is > > > the first time when the backlight is configured, so it sets period and > > > polarity. But then for some reason when we read out after that to read > > > what state was written... we see that actually nothing was written at > > > all. > > > > > > And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we > > > don't actually program any of the registers, so it's not a surprise that > > > things fall apart. > > > > > > > [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > > > > [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > > [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > > > > > Backlight is on with full brightness at this stage. > > > > > > > > root@hydraco:/sys/class/backlight/backlight# cat brightness > > > > 32 > > > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > > > > [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > > > > > Backlight goes down. > > > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > > > > [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > > > > > Backlight goes up to almost full brightness. > > > > > > > > root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > > > > [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > > > > [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > > > > > > > > Backlight goes up to full brightness. > > > > > > > > So your patch does not solve my issue. > > > > > > > > The main problem I see is incorrect polarity setting. In my DT > > > > the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > > > > period 500000ns. Though after reset the PWM HW registers are > > > > configured to normal polarity. This initial setting is read out > > > > and used by the consumer instead of the DT configuration. > > > > > > So the problem with the i.MX driver is that it doesn't actually write > > > the full state to the hardware and therefore the patch that caused these > > > things to break reads back an incomplete state. So we've basically got > > > two options: 1) make sure the hardware state is fully written or 2) make > > > sure that we return the cached state. > > > > > > I think 2) doesn't really make sense because it is conflicts with the > > > purpose of the ->get_state() callback. The only time where we should be > > > returning cached data is if the hardware registers don't contain the > > > information (as in the case of the cros-ec driver) or if we can't access > > > it for other reasons (such as in the case of i.MX's duty cycle). > > > > > > Does the attached patch help with your issue? The idea is to always > > > write the full state to the hardware, even if period and duty cycle are > > > unused when the PWM is disabled. That's really the kind of contract that > > > we have added with the offending patch in the core. > > > > > > It looks like all other drivers handle this more or less correctly, so > > > if we only need to fix up cros-ec and i.MX this seems like a realistic > > > way to fix things up. If other drivers are problematic in this regard, > > > we should probably revert and then fix the drivers before we can apply > > > that patch again. > > > > This patch combined with your previous patch appears to have worked. > > If you end up sending a patch series to fix this, go ahead and add > > > > Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd > > Excellent! Thanks for testing this. I'll wait until tomorrow to see if > there's some feedback from Enric for the cros-ec change. I'll send out > the total of three patches again in the hopes that those are really > the only two cases that are broken. When you do, can you mark it with the Fixes note? I am hoping the maintainers can hopefully incorporate this into 5.4 since it fixes a regression. adam > > Thierry > > > > --- >8 --- > > > From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 > > > From: Thierry Reding <thierry.reding@gmail.com> > > > Date: Thu, 17 Oct 2019 17:11:41 +0200 > > > Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware > > > > > > The i.MX driver currently uses a shortcut and doesn't write all of the > > > state through to the hardware when the PWM is disabled. This causes an > > > inconsistent state to be read back by consumers with the result of them > > > malfunctioning. > > > > > > Fix this by always writing the full state through to the hardware > > > registers so that the correct state can always be read back. > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > --- > > > drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- > > > 1 file changed, 59 insertions(+), 61 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > index 4113d5cd4c62..59d8b1289808 100644 > > > --- a/drivers/pwm/pwm-imx27.c > > > +++ b/drivers/pwm/pwm-imx27.c > > > @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > pwm_get_state(pwm, &cstate); > > > > > > - if (state->enabled) { > > > - c = clk_get_rate(imx->clk_per); > > > - c *= state->period; > > > - > > > - do_div(c, 1000000000); > > > - period_cycles = c; > > > - > > > - prescale = period_cycles / 0x10000 + 1; > > > - > > > - period_cycles /= prescale; > > > - c = (unsigned long long)period_cycles * state->duty_cycle; > > > - do_div(c, state->period); > > > - duty_cycles = c; > > > - > > > - /* > > > - * according to imx pwm RM, the real period value should be > > > - * PERIOD value in PWMPR plus 2. > > > - */ > > > - if (period_cycles > 2) > > > - period_cycles -= 2; > > > - else > > > - period_cycles = 0; > > > - > > > - /* > > > - * Wait for a free FIFO slot if the PWM is already enabled, and > > > - * flush the FIFO if the PWM was disabled and is about to be > > > - * enabled. > > > - */ > > > - if (cstate.enabled) { > > > - pwm_imx27_wait_fifo_slot(chip, pwm); > > > - } else { > > > - ret = pwm_imx27_clk_prepare_enable(chip); > > > - if (ret) > > > - return ret; > > > - > > > - pwm_imx27_sw_reset(chip); > > > - } > > > - > > > - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > - writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > - > > > - /* > > > - * Store the duty cycle for future reference in cases where > > > - * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > - * is disabled). > > > - */ > > > - imx->duty_cycle = duty_cycles; > > > - > > > - cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > > - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > > - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > > - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; > > > - > > > - if (state->polarity == PWM_POLARITY_INVERSED) > > > - cr |= FIELD_PREP(MX3_PWMCR_POUTC, > > > - MX3_PWMCR_POUTC_INVERTED); > > > - > > > - writel(cr, imx->mmio_base + MX3_PWMCR); > > > - } else if (cstate.enabled) { > > > - writel(0, imx->mmio_base + MX3_PWMCR); > > > + c = clk_get_rate(imx->clk_per); > > > + c *= state->period; > > > > > > - pwm_imx27_clk_disable_unprepare(chip); > > > + do_div(c, 1000000000); > > > + period_cycles = c; > > > + > > > + prescale = period_cycles / 0x10000 + 1; > > > + > > > + period_cycles /= prescale; > > > + c = (unsigned long long)period_cycles * state->duty_cycle; > > > + do_div(c, state->period); > > > + duty_cycles = c; > > > + > > > + /* > > > + * according to imx pwm RM, the real period value should be PERIOD > > > + * value in PWMPR plus 2. > > > + */ > > > + if (period_cycles > 2) > > > + period_cycles -= 2; > > > + else > > > + period_cycles = 0; > > > + > > > + /* > > > + * Wait for a free FIFO slot if the PWM is already enabled, and flush > > > + * the FIFO if the PWM was disabled and is about to be enabled. > > > + */ > > > + if (cstate.enabled) { > > > + pwm_imx27_wait_fifo_slot(chip, pwm); > > > + } else { > > > + ret = pwm_imx27_clk_prepare_enable(chip); > > > + if (ret) > > > + return ret; > > > + > > > + pwm_imx27_sw_reset(chip); > > > } > > > > > > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > + > > > + /* > > > + * Store the duty cycle for future reference in cases where the > > > + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). > > > + */ > > > + imx->duty_cycle = duty_cycles; > > > + > > > + cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > > + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > > + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > > + MX3_PWMCR_DBGEN; > > > + > > > + if (state->polarity == PWM_POLARITY_INVERSED) > > > + cr |= FIELD_PREP(MX3_PWMCR_POUTC, > > > + MX3_PWMCR_POUTC_INVERTED); > > > + > > > + if (state->enabled) > > > + cr |= MX3_PWMCR_EN; > > > + > > > + writel(cr, imx->mmio_base + MX3_PWMCR); > > > + > > > + if (!state->enabled && cstate.enabled) > > > + pwm_imx27_clk_disable_unprepare(chip); > > > + > > > return 0; > > > } > > > > > > -- > > > 2.23.0 > > >
On Thu, Oct 17, 2019 at 02:18:02PM +0100, Daniel Thompson wrote: > On Thu, Oct 17, 2019 at 02:19:45PM +0200, Uwe Kleine-König wrote: > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote: > > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote: > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > pwm_get_state() return the last implemented state")) changed the > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > worked around. > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > combo once is also more effective. > > > > > > I'm only interested in the second paragraph here. > > > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec > > > PWM drivers should be fixed for the benefit of other PWM clients. > > > So we make this change because it makes the pwm-bl better... not to > > > work around bugs ;-). > > > > That's fine, still I think it's fair to explain the motivation of > > creating this patch. > > Maybe. > > Whether this patch is a workaround or simply an improvement to pwm-bl > does need to be clear since it affects whether Lee steers it towards > v5.4-rcX or linux-next . Given that there will be a a fix in the pwm subsystem I'd say linux-next sounds right. Best regards Uwe
Hello Thierry, On Thu, Oct 17, 2019 at 02:59:32PM +0200, Thierry Reding wrote: > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > index ae11d8577f18..4113d5cd4c62 100644 > > > --- a/drivers/pwm/pwm-imx27.c > > > +++ b/drivers/pwm/pwm-imx27.c > > > [...] > > > > I wonder if it would be more sensible to do this in the pwm core > > instead. Currently there are two drivers known with this problem. I > > wouldn't be surprised if there were more. > > I've inspected all the drivers and didn't spot any beyond cros-ec and > i.MX that have this problem. I took a look, too, and I'd say pwm-atmel.c, pwm-imx-tpm.c, pwm-lpss.c, pwm-meson.c, pwm-sifive.c, pwm-sprd.c and pwm-stm32-lp.c are affected. > So the core would have to rely on state->duty_cycle that is passed in, > but then the offending commit becomes useless because the whole point > was to return the state as written to hardware (rather than the > software state which was being returned before that patch). I like allowing lowlevel drivers to implement the .enabled = false case lazily as there is little value in calculating the needed register values for a request like .duty_cycle = X, .period = Y, .enabled = false even if the next request might be .duty_cycle = X, .period = Y, .enabled = true because quite likely the same calculation is done for the second request again and there is no benefit to save X and Y in the hardware (or the driver if the hardware is incapable) apart from returning it to a consumer who maybe even doesn't care because these values don't tell anything at all about the implemented wave form and so it seems natural to me that the lowlevel driver shouldn't care. > > If we want to move clients to not rely on .period and .duty_cycle for a > > disabled PWM (do we?) a single change in the core is also beneficial > > compared to fixing several lowlevel drivers. > > These are really two orthogonal problems. We don't currently consider > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > prepared to do that at this point in the release cycle either. Yeah, I fully agree that we should not do that now. Given the number of affected drivers I opt for reverting and retrying again more carefully later. > What this here has shown is that we have at least two drivers that don't > behave the way they are supposed to according to the API and they break > consumers. If they break for pwm-backlight, it's possible that they will > break for other consumers as well. So the right thing to do is fix the > two drivers that are broken. In my eyes shifting the definition of "expected behaviour" and adapting the core code accordingly to make this invisible to consumers is also a viable option. Also shifting the definition and adapt all consumers not to rely on the old behaviour is fine. But as above, this is not a good idea at the current point in time. Best regards Uwe
On Thu, Oct 17, 2019 at 12:59:20PM +0200, Michal Vokáč wrote: > On 17. 10. 19 11:48, Michal Vokáč wrote: > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > pwm_get_state() return the last implemented state")) changed the > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > cycle being retrievable from a disabled PWM this type of problem is > > > worked around. > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > combo once is also more effective. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > Hello, > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > far as I understand the problem this is a combination of the backend pwm > > > driver yielding surprising results and the pwm-bl driver doing things > > > more complicated than necessary. > > > > > > So I guess this patch works around these problems. Still it would be > > > interesting to find out the details in the imx driver that triggers the > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > pwm_imx27_get_state() and provide the results? > > > > > > Note I only compile tested this change. > > > > Hi Uwe, > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > thread that I have a similar problem when you submitted this patch. > > > > So here are my few cents: > > Once again with updated and more detailed debug messages. > > > My setup is as follows: > > - imx6dl-yapp4-draco with i.MX6Solo > > - backlight is controlled with inverted PWM signal > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > pwm_get_state() return the last implemented state): > > > > - System boots to userspace and backlight is enabled all the time from > > power up. > > > - $ dmesg | grep pwm_ > [ 1.761546] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.012352] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 5.021143] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 > [ 5.030182] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > > > - $ cat brightness > > 32 > > > > - $ echo 32 > brightness # nothing happens, max. brightness > > > > - $ echo 1 > brightness # backlight goes down to lowest level > [ 93.976354] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 > > > > - $ echo 0 > brightness # backlight goes up to max. level, this is > > # problem of the inverted PWM on i.MX we attempted > > # to solve some time ago. > [ 115.496350] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > > > 2. Backlight behavior on v5.4-rc3: > > > > - System boots to userspace and backlight is enabled all the time from > > power up. > > > - $ dmesg | grep pwm_ > [ 1.774071] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.003961] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 5.012649] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.021694] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > [ 5.030732] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.039643] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > [ 5.049605] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > - $ cat brightness > > 32 > > > > - $ echo 32 > brightness # backlight goes down > [ 707.946970] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > [ 707.958551] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > - $ echo 1 > brightness # backlight goes up to high level > [ 757.516845] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > [ 757.528438] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > - $ echo 0 > brightness # backlight goes up to highest level > [ 783.386838] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 783.398025] pwm_imx27_get_state: period: 496485, duty: 0, polarity: 0, enabled: 0 > > > > So the behavior is clearly inverted to how it worked prior to 01ccf903edd6 > > with the weird exception that the initial brightness level 32 is > > not applied. > > > > 3. Backlight behavior on v5.4-rc3 + this patch: > > > > - System boots with backlight enabled. In the middle of kernel boot > > backlight is disabled. > > > > - $ dmesg | grep state > [ 1.773099] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.002532] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 5.011263] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 Here is another corner case: pwm_imx27_apply doesn't even set polarity in hardware if .enabled is false. This way .polarity is lost as obviously the wrong setting is returned by pwm_get_state() later. I didn't check in detail, but at least atmel, atmel-hlcdc, fsl-ftm, stm32 and stm32-lp seem to suffer from the same problem. Best regards Uwe
Hello Thierry, On Thu, Oct 17, 2019 at 05:14:37PM +0200, Thierry Reding wrote: > On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > > On 17. 10. 19 14:59, Thierry Reding wrote: > > > On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > > > > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > > > > > On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > > > > > > On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > > > > > > > On 17. 10. 19 10:10, Uwe Kleine-König wrote: > > > > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > > > > > > > > pwm_get_state() return the last implemented state")) changed the > > > > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common > > > > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty > > > > > > > > cycle being retrievable from a disabled PWM this type of problem is > > > > > > > > worked around. > > > > > > > > > > > > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state > > > > > > > > combo once is also more effective. > > > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > > --- > > > > > > > > Hello, > > > > > > > > > > > > > > > > There are now two reports about 01ccf903edd6 breaking a backlight. As > > > > > > > > far as I understand the problem this is a combination of the backend pwm > > > > > > > > driver yielding surprising results and the pwm-bl driver doing things > > > > > > > > more complicated than necessary. > > > > > > > > > > > > > > > > So I guess this patch works around these problems. Still it would be > > > > > > > > interesting to find out the details in the imx driver that triggers the > > > > > > > > problem. So Adam, can you please instrument the pwm-imx27 driver to > > > > > > > > print *state at the beginning of pwm_imx27_apply() and the end of > > > > > > > > pwm_imx27_get_state() and provide the results? > > > > > > > > > > > > > > > > Note I only compile tested this change. > > > > > > > > > > > > > > Hi Uwe, > > > > > > > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > > > > > > > thread that I have a similar problem when you submitted this patch. > > > > > > > > > > > > > > So here are my few cents: > > > > > > > > > > > > > > My setup is as follows: > > > > > > > - imx6dl-yapp4-draco with i.MX6Solo > > > > > > > - backlight is controlled with inverted PWM signal > > > > > > > - max brightness level = 32, default brightness level set to 32 in DT. > > > > > > > > > > > > > > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > > > > > > > pwm_get_state() return the last implemented state): > > > > > > > > > > > > > > - System boots to userspace and backlight is enabled all the time from > > > > > > > power up. > > > > > > > > > > > > > > $ dmesg | grep state > > > > > > > [ 1.763381] get state end: -1811360608, enabled: 0 > > > > > > > > > > > > What is -1811360608? When I wrote "print *state" above, I thought about > > > > > > something like: > > > > > > > > > > > > pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > > > > > > __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > > > > > > > > > > > > A quick look into drivers/pwm/pwm-imx27.c shows that this is another > > > > > > driver that yields duty_cycle = 0 when the hardware is off. > > > > > > > > > > It seems to me like the best recourse to fix this for now would be to > > > > > patch up the drivers that return 0 when the hardware is off by caching > > > > > the currently configured duty cycle. > > > > > > > > > > How about the patch below? > > > > > > > > > > Thierry > > > > > > > > > > --- >8 --- > > > > > From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > > > > > From: Thierry Reding <thierry.reding@gmail.com> > > > > > Date: Thu, 17 Oct 2019 12:56:00 +0200 > > > > > Subject: [PATCH] pwm: imx27: Cache duty cycle register value > > > > > > > > > > The hardware register containing the duty cycle value cannot be accessed > > > > > when the PWM is disabled. This causes the ->get_state() callback to read > > > > > back a duty cycle value of 0, which can confuse consumer drivers. > > > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > > --- > > > > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > > index ae11d8577f18..4113d5cd4c62 100644 > > > > > --- a/drivers/pwm/pwm-imx27.c > > > > > +++ b/drivers/pwm/pwm-imx27.c > > > > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > > > > struct clk *clk_per; > > > > > void __iomem *mmio_base; > > > > > struct pwm_chip chip; > > > > > + > > > > > + /* > > > > > + * The driver cannot read the current duty cycle from the hardware if > > > > > + * the hardware is disabled. Cache the last programmed duty cycle > > > > > + * value to return in that case. > > > > > + */ > > > > > + unsigned int duty_cycle; > > > > > }; > > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > > > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > > > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > > > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > - /* PWMSAR can be read only if PWM is enabled */ > > > > > - if (state->enabled) { > > > > > + /* > > > > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > > > > + * use the cached value. > > > > > + */ > > > > > + if (state->enabled) > > > > > val = readl(imx->mmio_base + MX3_PWMSAR); > > > > > - tmp = NSEC_PER_SEC * (u64)(val); > > > > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > - } else { > > > > > - state->duty_cycle = 0; > > > > > - } > > > > > + else > > > > > + val = imx->duty_cycle; > > > > > + > > > > > + tmp = NSEC_PER_SEC * (u64)(val); > > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > > if (!state->enabled) > > > > > pwm_imx27_clk_disable_unprepare(chip); > > > > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > > > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > > + /* > > > > > + * Store the duty cycle for future reference in cases where > > > > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > > > > + * is disabled). > > > > > + */ > > > > > + imx->duty_cycle = duty_cycles; > > > > > + > > > > > > > > I wonder if it would be more sensible to do this in the pwm core > > > > instead. Currently there are two drivers known with this problem. I > > > > wouldn't be surprised if there were more. > > > > > > I've inspected all the drivers and didn't spot any beyond cros-ec and > > > i.MX that have this problem. There's also no good way to do this in the > > > core, because the core doesn't know whether or not the driver is capable > > > of returning the correct duty cycle on hardare readout. So the core > > > would have to rely on state->duty_cycle that is passed in, but then the > > > offending commit becomes useless because the whole point was to return > > > the state as written to hardware (rather than the software state which > > > was being returned before that patch). > > > > > > > If we want to move clients to not rely on .period and .duty_cycle for a > > > > disabled PWM (do we?) a single change in the core is also beneficial > > > > compared to fixing several lowlevel drivers. > > > > > > These are really two orthogonal problems. We don't currently consider > > > enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > > > prepared to do that at this point in the release cycle either. > > > > > > What this here has shown is that we have at least two drivers that don't > > > behave the way they are supposed to according to the API and they break > > > consumers. If they break for pwm-backlight, it's possible that they will > > > break for other consumers as well. So the right thing to do is fix the > > > two drivers that are broken. > > > > > > After -rc1 we no longer experiment. Instead we clean up the messes we've > > > made. We can revisit the other points once mainline is fixed. > > > > Hi Thierry, > > I just tried your patch with v5.4-rc3 with this result: > > > > root@hydraco:~# dmesg | grep pwm_ > > [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > > [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > Okay... this is interesting. If I understand correctly, that first line > here is where the initial hardware readout happens. The second one is > the first time when the backlight is configured, so it sets period and > polarity. But then for some reason when we read out after that to read > what state was written... we see that actually nothing was written at > all. > > And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we > don't actually program any of the registers, so it's not a surprise that > things fall apart. > > > [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > > [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > > [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > > > > Backlight is on with full brightness at this stage. > > > > root@hydraco:/sys/class/backlight/backlight# cat brightness > > 32 > > > > root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > > [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > > > > Backlight goes down. > > > > root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > > [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > > > > Backlight goes up to almost full brightness. > > > > root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > > [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > > [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > > > > Backlight goes up to full brightness. > > > > So your patch does not solve my issue. > > > > The main problem I see is incorrect polarity setting. In my DT > > the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > > period 500000ns. Though after reset the PWM HW registers are > > configured to normal polarity. This initial setting is read out > > and used by the consumer instead of the DT configuration. > > So the problem with the i.MX driver is that it doesn't actually write > the full state to the hardware and therefore the patch that caused these > things to break reads back an incomplete state. So we've basically got > two options: 1) make sure the hardware state is fully written or 2) make > sure that we return the cached state. My preference would be 3) make consumers not rely on .duty_cycle and .period if the PWM is disabled. This questions of course the common PWM idiom pwm_get_state(...) only = change(what, you, think, is, necessary); pwm_apply_state(...) but I don't like this that much anyhow. YMMV. Best regards Uwe
On 17. 10. 19 19:44, Adam Ford wrote: > On Thu, Oct 17, 2019 at 12:13 PM Thierry Reding > <thierry.reding@gmail.com> wrote: >> >> On Thu, Oct 17, 2019 at 12:07:21PM -0500, Adam Ford wrote: >>> On Thu, Oct 17, 2019 at 10:14 AM Thierry Reding >>> <thierry.reding@gmail.com> wrote: >>>> >>>> On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: >>>>> On 17. 10. 19 14:59, Thierry Reding wrote: >>>>>> On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: >>>>>>> On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: >>>>>>>> On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: >>>>>>>>> On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: >>>>>>>>>> On 17. 10. 19 10:10, Uwe Kleine-König wrote: >>>>>>>>>>> A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let >>>>>>>>>>> pwm_get_state() return the last implemented state")) changed the >>>>>>>>>>> semantic of pwm_get_state() and disclosed an (as it seems) common >>>>>>>>>>> problem in lowlevel PWM drivers. By not relying on the period and duty >>>>>>>>>>> cycle being retrievable from a disabled PWM this type of problem is >>>>>>>>>>> worked around. >>>>>>>>>>> >>>>>>>>>>> Apart from this issue only calling the pwm_get_state/pwm_apply_state >>>>>>>>>>> combo once is also more effective. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>>>>>>>>>> --- >>>>>>>>>>> Hello, >>>>>>>>>>> >>>>>>>>>>> There are now two reports about 01ccf903edd6 breaking a backlight. As >>>>>>>>>>> far as I understand the problem this is a combination of the backend pwm >>>>>>>>>>> driver yielding surprising results and the pwm-bl driver doing things >>>>>>>>>>> more complicated than necessary. >>>>>>>>>>> >>>>>>>>>>> So I guess this patch works around these problems. Still it would be >>>>>>>>>>> interesting to find out the details in the imx driver that triggers the >>>>>>>>>>> problem. So Adam, can you please instrument the pwm-imx27 driver to >>>>>>>>>>> print *state at the beginning of pwm_imx27_apply() and the end of >>>>>>>>>>> pwm_imx27_get_state() and provide the results? >>>>>>>>>>> >>>>>>>>>>> Note I only compile tested this change. >>>>>>>>>> >>>>>>>>>> Hi Uwe, >>>>>>>>>> I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" >>>>>>>>>> thread that I have a similar problem when you submitted this patch. >>>>>>>>>> >>>>>>>>>> So here are my few cents: >>>>>>>>>> >>>>>>>>>> My setup is as follows: >>>>>>>>>> - imx6dl-yapp4-draco with i.MX6Solo >>>>>>>>>> - backlight is controlled with inverted PWM signal >>>>>>>>>> - max brightness level = 32, default brightness level set to 32 in DT. >>>>>>>>>> >>>>>>>>>> 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let >>>>>>>>>> pwm_get_state() return the last implemented state): >>>>>>>>>> >>>>>>>>>> - System boots to userspace and backlight is enabled all the time from >>>>>>>>>> power up. >>>>>>>>>> >>>>>>>>>> $ dmesg | grep state >>>>>>>>>> [ 1.763381] get state end: -1811360608, enabled: 0 >>>>>>>>> >>>>>>>>> What is -1811360608? When I wrote "print *state" above, I thought about >>>>>>>>> something like: >>>>>>>>> >>>>>>>>> pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", >>>>>>>>> __func__, state->period, state->duty_cycle, state->polarity, state->enabled); >>>>>>>>> >>>>>>>>> A quick look into drivers/pwm/pwm-imx27.c shows that this is another >>>>>>>>> driver that yields duty_cycle = 0 when the hardware is off. >>>>>>>> >>>>>>>> It seems to me like the best recourse to fix this for now would be to >>>>>>>> patch up the drivers that return 0 when the hardware is off by caching >>>>>>>> the currently configured duty cycle. >>>>>>>> >>>>>>>> How about the patch below? >>>>>>>> >>>>>>>> Thierry >>>>>>>> >>>>>>>> --- >8 --- >>>>>>>> From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 >>>>>>>> From: Thierry Reding <thierry.reding@gmail.com> >>>>>>>> Date: Thu, 17 Oct 2019 12:56:00 +0200 >>>>>>>> Subject: [PATCH] pwm: imx27: Cache duty cycle register value >>>>>>>> >>>>>>>> The hardware register containing the duty cycle value cannot be accessed >>>>>>>> when the PWM is disabled. This causes the ->get_state() callback to read >>>>>>>> back a duty cycle value of 0, which can confuse consumer drivers. >>>>>>>> >>>>>>>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> >>>>>>>> --- >>>>>>>> drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- >>>>>>>> 1 file changed, 24 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c >>>>>>>> index ae11d8577f18..4113d5cd4c62 100644 >>>>>>>> --- a/drivers/pwm/pwm-imx27.c >>>>>>>> +++ b/drivers/pwm/pwm-imx27.c >>>>>>>> @@ -85,6 +85,13 @@ struct pwm_imx27_chip { >>>>>>>> struct clk *clk_per; >>>>>>>> void __iomem *mmio_base; >>>>>>>> struct pwm_chip chip; >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * The driver cannot read the current duty cycle from the hardware if >>>>>>>> + * the hardware is disabled. Cache the last programmed duty cycle >>>>>>>> + * value to return in that case. >>>>>>>> + */ >>>>>>>> + unsigned int duty_cycle; >>>>>>>> }; >>>>>>>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) >>>>>>>> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, >>>>>>>> tmp = NSEC_PER_SEC * (u64)(period + 2); >>>>>>>> state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>>>>>>> - /* PWMSAR can be read only if PWM is enabled */ >>>>>>>> - if (state->enabled) { >>>>>>>> + /* >>>>>>>> + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, >>>>>>>> + * use the cached value. >>>>>>>> + */ >>>>>>>> + if (state->enabled) >>>>>>>> val = readl(imx->mmio_base + MX3_PWMSAR); >>>>>>>> - tmp = NSEC_PER_SEC * (u64)(val); >>>>>>>> - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>>>>>>> - } else { >>>>>>>> - state->duty_cycle = 0; >>>>>>>> - } >>>>>>>> + else >>>>>>>> + val = imx->duty_cycle; >>>>>>>> + >>>>>>>> + tmp = NSEC_PER_SEC * (u64)(val); >>>>>>>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); >>>>>>>> if (!state->enabled) >>>>>>>> pwm_imx27_clk_disable_unprepare(chip); >>>>>>>> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>>>>>>> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>>>>>>> writel(period_cycles, imx->mmio_base + MX3_PWMPR); >>>>>>>> + /* >>>>>>>> + * Store the duty cycle for future reference in cases where >>>>>>>> + * the MX3_PWMSAR register can't be read (i.e. when the PWM >>>>>>>> + * is disabled). >>>>>>>> + */ >>>>>>>> + imx->duty_cycle = duty_cycles; >>>>>>>> + >>>>>>> >>>>>>> I wonder if it would be more sensible to do this in the pwm core >>>>>>> instead. Currently there are two drivers known with this problem. I >>>>>>> wouldn't be surprised if there were more. >>>>>> >>>>>> I've inspected all the drivers and didn't spot any beyond cros-ec and >>>>>> i.MX that have this problem. There's also no good way to do this in the >>>>>> core, because the core doesn't know whether or not the driver is capable >>>>>> of returning the correct duty cycle on hardare readout. So the core >>>>>> would have to rely on state->duty_cycle that is passed in, but then the >>>>>> offending commit becomes useless because the whole point was to return >>>>>> the state as written to hardware (rather than the software state which >>>>>> was being returned before that patch). >>>>>> >>>>>>> If we want to move clients to not rely on .period and .duty_cycle for a >>>>>>> disabled PWM (do we?) a single change in the core is also beneficial >>>>>>> compared to fixing several lowlevel drivers. >>>>>> >>>>>> These are really two orthogonal problems. We don't currently consider >>>>>> enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not >>>>>> prepared to do that at this point in the release cycle either. >>>>>> >>>>>> What this here has shown is that we have at least two drivers that don't >>>>>> behave the way they are supposed to according to the API and they break >>>>>> consumers. If they break for pwm-backlight, it's possible that they will >>>>>> break for other consumers as well. So the right thing to do is fix the >>>>>> two drivers that are broken. >>>>>> >>>>>> After -rc1 we no longer experiment. Instead we clean up the messes we've >>>>>> made. We can revisit the other points once mainline is fixed. >>>>> >>>>> Hi Thierry, >>>>> I just tried your patch with v5.4-rc3 with this result: >>>>> >>>>> root@hydraco:~# dmesg | grep pwm_ >>>>> [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 >>>>> [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 >>>>> [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 >>>> >>>> Okay... this is interesting. If I understand correctly, that first line >>>> here is where the initial hardware readout happens. The second one is >>>> the first time when the backlight is configured, so it sets period and >>>> polarity. But then for some reason when we read out after that to read >>>> what state was written... we see that actually nothing was written at >>>> all. >>>> >>>> And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we >>>> don't actually program any of the registers, so it's not a surprise that >>>> things fall apart. >>>> >>>>> [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 >>>>> [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 >>>>> [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 >>>>> [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 >>>>> >>>>> Backlight is on with full brightness at this stage. >>>>> >>>>> root@hydraco:/sys/class/backlight/backlight# cat brightness >>>>> 32 >>>>> >>>>> root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness >>>>> [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 >>>>> [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 >>>>> >>>>> Backlight goes down. >>>>> >>>>> root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness >>>>> [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 >>>>> [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 >>>>> >>>>> Backlight goes up to almost full brightness. >>>>> >>>>> root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness >>>>> [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 >>>>> [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 >>>>> >>>>> Backlight goes up to full brightness. >>>>> >>>>> So your patch does not solve my issue. >>>>> >>>>> The main problem I see is incorrect polarity setting. In my DT >>>>> the pwm-backlight consumer requests PWM_POLARITY_INVERTED and >>>>> period 500000ns. Though after reset the PWM HW registers are >>>>> configured to normal polarity. This initial setting is read out >>>>> and used by the consumer instead of the DT configuration. >>>> >>>> So the problem with the i.MX driver is that it doesn't actually write >>>> the full state to the hardware and therefore the patch that caused these >>>> things to break reads back an incomplete state. So we've basically got >>>> two options: 1) make sure the hardware state is fully written or 2) make >>>> sure that we return the cached state. >>>> >>>> I think 2) doesn't really make sense because it is conflicts with the >>>> purpose of the ->get_state() callback. The only time where we should be >>>> returning cached data is if the hardware registers don't contain the >>>> information (as in the case of the cros-ec driver) or if we can't access >>>> it for other reasons (such as in the case of i.MX's duty cycle). >>>> >>>> Does the attached patch help with your issue? The idea is to always >>>> write the full state to the hardware, even if period and duty cycle are >>>> unused when the PWM is disabled. That's really the kind of contract that >>>> we have added with the offending patch in the core. >>>> >>>> It looks like all other drivers handle this more or less correctly, so >>>> if we only need to fix up cros-ec and i.MX this seems like a realistic >>>> way to fix things up. If other drivers are problematic in this regard, >>>> we should probably revert and then fix the drivers before we can apply >>>> that patch again. >>> >>> This patch combined with your previous patch appears to have worked. >>> If you end up sending a patch series to fix this, go ahead and add >>> >>> Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd >> >> Excellent! Thanks for testing this. I'll wait until tomorrow to see if >> there's some feedback from Enric for the cros-ec change. I'll send out >> the total of three patches again in the hopes that those are really >> the only two cases that are broken. > > When you do, can you mark it with the Fixes note? I am hoping the > maintainers can hopefully incorporate this into 5.4 since it fixes a > regression. Hi Thierry, I can confirm that the combination of your two patches: - ("pwm: imx27: Unconditionally write state to hardware") - ("pwm: imx27: Cache duty cycle register value") works OK and solve my problem as well. root@hydraco:~# dmesg | grep pwm_ [ 1.695306] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 5.387271] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 5.396433] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 5.405500] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 [ 5.418802] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 0 [ 5.428208] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 [ 5.442633] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 Backlight is on from power up to userspace. root@hydraco:~# cd /sys/class/backlight/backlight/ root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# cat brightness 32 root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness Nothing happens. root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 1 > brightness [ 513.629043] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 [ 513.639899] pwm_imx27_get_state: period: 500000, duty: 7833, polarity: 1, enabled: 1 Backlight goes to low brightness. root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 0 > brightness [ 519.677088] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 519.687733] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 Backlight goes to max brightness, unresolved i.MX6 limitation. root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness [ 923.921292] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 [ 923.933331] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 0 [ 923.944546] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 [ 923.963931] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 Backlight remains at max brightness, OK. If I apply the patch from Uwe ("backlight: pwm_bl: configure pwm only once per backlight toggle") on top of that, it still works and I do not see any change in the behavior. root@hydraco:~# dmesg | grep pwm_ [ 1.687461] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 [ 4.875087] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 4.884796] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 4.893922] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 [ 4.908473] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 root@hydraco:~# cd /sys/class/backlight/backlight/ root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# cat brightness 32 root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 1 > brightness [ 110.775650] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 [ 110.786512] pwm_imx27_get_state: period: 500000, duty: 7833, polarity: 1, enabled: 1 root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 0 > brightness [ 128.224036] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 [ 128.234675] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness [ 138.208072] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 [ 138.220271] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 The only difference is here when the state is changed from enabled=0 to enabled=1. The apply/get_state combo is called only once. So this looks good to me. Tested-by: Michal Vokáč <michal.vokac@ysoft.com> Thank you all for the very prompt reaction! Michal >>>> --- >8 --- >>>> From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 >>>> From: Thierry Reding <thierry.reding@gmail.com> >>>> Date: Thu, 17 Oct 2019 17:11:41 +0200 >>>> Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware >>>> >>>> The i.MX driver currently uses a shortcut and doesn't write all of the >>>> state through to the hardware when the PWM is disabled. This causes an >>>> inconsistent state to be read back by consumers with the result of them >>>> malfunctioning. >>>> >>>> Fix this by always writing the full state through to the hardware >>>> registers so that the correct state can always be read back. >>>> >>>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> >>>> --- >>>> drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- >>>> 1 file changed, 59 insertions(+), 61 deletions(-) >>>> >>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c >>>> index 4113d5cd4c62..59d8b1289808 100644 >>>> --- a/drivers/pwm/pwm-imx27.c >>>> +++ b/drivers/pwm/pwm-imx27.c >>>> @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>>> >>>> pwm_get_state(pwm, &cstate); >>>> >>>> - if (state->enabled) { >>>> - c = clk_get_rate(imx->clk_per); >>>> - c *= state->period; >>>> - >>>> - do_div(c, 1000000000); >>>> - period_cycles = c; >>>> - >>>> - prescale = period_cycles / 0x10000 + 1; >>>> - >>>> - period_cycles /= prescale; >>>> - c = (unsigned long long)period_cycles * state->duty_cycle; >>>> - do_div(c, state->period); >>>> - duty_cycles = c; >>>> - >>>> - /* >>>> - * according to imx pwm RM, the real period value should be >>>> - * PERIOD value in PWMPR plus 2. >>>> - */ >>>> - if (period_cycles > 2) >>>> - period_cycles -= 2; >>>> - else >>>> - period_cycles = 0; >>>> - >>>> - /* >>>> - * Wait for a free FIFO slot if the PWM is already enabled, and >>>> - * flush the FIFO if the PWM was disabled and is about to be >>>> - * enabled. >>>> - */ >>>> - if (cstate.enabled) { >>>> - pwm_imx27_wait_fifo_slot(chip, pwm); >>>> - } else { >>>> - ret = pwm_imx27_clk_prepare_enable(chip); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - pwm_imx27_sw_reset(chip); >>>> - } >>>> - >>>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>>> - writel(period_cycles, imx->mmio_base + MX3_PWMPR); >>>> - >>>> - /* >>>> - * Store the duty cycle for future reference in cases where >>>> - * the MX3_PWMSAR register can't be read (i.e. when the PWM >>>> - * is disabled). >>>> - */ >>>> - imx->duty_cycle = duty_cycles; >>>> - >>>> - cr = MX3_PWMCR_PRESCALER_SET(prescale) | >>>> - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | >>>> - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | >>>> - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; >>>> - >>>> - if (state->polarity == PWM_POLARITY_INVERSED) >>>> - cr |= FIELD_PREP(MX3_PWMCR_POUTC, >>>> - MX3_PWMCR_POUTC_INVERTED); >>>> - >>>> - writel(cr, imx->mmio_base + MX3_PWMCR); >>>> - } else if (cstate.enabled) { >>>> - writel(0, imx->mmio_base + MX3_PWMCR); >>>> + c = clk_get_rate(imx->clk_per); >>>> + c *= state->period; >>>> >>>> - pwm_imx27_clk_disable_unprepare(chip); >>>> + do_div(c, 1000000000); >>>> + period_cycles = c; >>>> + >>>> + prescale = period_cycles / 0x10000 + 1; >>>> + >>>> + period_cycles /= prescale; >>>> + c = (unsigned long long)period_cycles * state->duty_cycle; >>>> + do_div(c, state->period); >>>> + duty_cycles = c; >>>> + >>>> + /* >>>> + * according to imx pwm RM, the real period value should be PERIOD >>>> + * value in PWMPR plus 2. >>>> + */ >>>> + if (period_cycles > 2) >>>> + period_cycles -= 2; >>>> + else >>>> + period_cycles = 0; >>>> + >>>> + /* >>>> + * Wait for a free FIFO slot if the PWM is already enabled, and flush >>>> + * the FIFO if the PWM was disabled and is about to be enabled. >>>> + */ >>>> + if (cstate.enabled) { >>>> + pwm_imx27_wait_fifo_slot(chip, pwm); >>>> + } else { >>>> + ret = pwm_imx27_clk_prepare_enable(chip); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + pwm_imx27_sw_reset(chip); >>>> } >>>> >>>> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>>> + writel(period_cycles, imx->mmio_base + MX3_PWMPR); >>>> + >>>> + /* >>>> + * Store the duty cycle for future reference in cases where the >>>> + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). >>>> + */ >>>> + imx->duty_cycle = duty_cycles; >>>> + >>>> + cr = MX3_PWMCR_PRESCALER_SET(prescale) | >>>> + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | >>>> + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | >>>> + MX3_PWMCR_DBGEN; >>>> + >>>> + if (state->polarity == PWM_POLARITY_INVERSED) >>>> + cr |= FIELD_PREP(MX3_PWMCR_POUTC, >>>> + MX3_PWMCR_POUTC_INVERTED); >>>> + >>>> + if (state->enabled) >>>> + cr |= MX3_PWMCR_EN; >>>> + >>>> + writel(cr, imx->mmio_base + MX3_PWMCR); >>>> + >>>> + if (!state->enabled && cstate.enabled) >>>> + pwm_imx27_clk_disable_unprepare(chip); >>>> + >>>> return 0; >>>> } >>>> >>>> -- >>>> 2.23.0 >>>>
On Fri, Oct 18, 2019 at 4:36 AM Michal Vokáč <michal.vokac@ysoft.com> wrote: > > On 17. 10. 19 19:44, Adam Ford wrote: > > On Thu, Oct 17, 2019 at 12:13 PM Thierry Reding > > <thierry.reding@gmail.com> wrote: > >> > >> On Thu, Oct 17, 2019 at 12:07:21PM -0500, Adam Ford wrote: > >>> On Thu, Oct 17, 2019 at 10:14 AM Thierry Reding > >>> <thierry.reding@gmail.com> wrote: > >>>> > >>>> On Thu, Oct 17, 2019 at 03:58:25PM +0200, Michal Vokáč wrote: > >>>>> On 17. 10. 19 14:59, Thierry Reding wrote: > >>>>>> On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote: > >>>>>>> On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote: > >>>>>>>> On Thu, Oct 17, 2019 at 12:11:16PM +0200, Uwe Kleine-König wrote: > >>>>>>>>> On Thu, Oct 17, 2019 at 11:48:08AM +0200, Michal Vokáč wrote: > >>>>>>>>>> On 17. 10. 19 10:10, Uwe Kleine-König wrote: > >>>>>>>>>>> A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let > >>>>>>>>>>> pwm_get_state() return the last implemented state")) changed the > >>>>>>>>>>> semantic of pwm_get_state() and disclosed an (as it seems) common > >>>>>>>>>>> problem in lowlevel PWM drivers. By not relying on the period and duty > >>>>>>>>>>> cycle being retrievable from a disabled PWM this type of problem is > >>>>>>>>>>> worked around. > >>>>>>>>>>> > >>>>>>>>>>> Apart from this issue only calling the pwm_get_state/pwm_apply_state > >>>>>>>>>>> combo once is also more effective. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >>>>>>>>>>> --- > >>>>>>>>>>> Hello, > >>>>>>>>>>> > >>>>>>>>>>> There are now two reports about 01ccf903edd6 breaking a backlight. As > >>>>>>>>>>> far as I understand the problem this is a combination of the backend pwm > >>>>>>>>>>> driver yielding surprising results and the pwm-bl driver doing things > >>>>>>>>>>> more complicated than necessary. > >>>>>>>>>>> > >>>>>>>>>>> So I guess this patch works around these problems. Still it would be > >>>>>>>>>>> interesting to find out the details in the imx driver that triggers the > >>>>>>>>>>> problem. So Adam, can you please instrument the pwm-imx27 driver to > >>>>>>>>>>> print *state at the beginning of pwm_imx27_apply() and the end of > >>>>>>>>>>> pwm_imx27_get_state() and provide the results? > >>>>>>>>>>> > >>>>>>>>>>> Note I only compile tested this change. > >>>>>>>>>> > >>>>>>>>>> Hi Uwe, > >>>>>>>>>> I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+" > >>>>>>>>>> thread that I have a similar problem when you submitted this patch. > >>>>>>>>>> > >>>>>>>>>> So here are my few cents: > >>>>>>>>>> > >>>>>>>>>> My setup is as follows: > >>>>>>>>>> - imx6dl-yapp4-draco with i.MX6Solo > >>>>>>>>>> - backlight is controlled with inverted PWM signal > >>>>>>>>>> - max brightness level = 32, default brightness level set to 32 in DT. > >>>>>>>>>> > >>>>>>>>>> 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let > >>>>>>>>>> pwm_get_state() return the last implemented state): > >>>>>>>>>> > >>>>>>>>>> - System boots to userspace and backlight is enabled all the time from > >>>>>>>>>> power up. > >>>>>>>>>> > >>>>>>>>>> $ dmesg | grep state > >>>>>>>>>> [ 1.763381] get state end: -1811360608, enabled: 0 > >>>>>>>>> > >>>>>>>>> What is -1811360608? When I wrote "print *state" above, I thought about > >>>>>>>>> something like: > >>>>>>>>> > >>>>>>>>> pr_info("%s: period: %u, duty: %u, polarity: %d, enabled: %d", > >>>>>>>>> __func__, state->period, state->duty_cycle, state->polarity, state->enabled); > >>>>>>>>> > >>>>>>>>> A quick look into drivers/pwm/pwm-imx27.c shows that this is another > >>>>>>>>> driver that yields duty_cycle = 0 when the hardware is off. > >>>>>>>> > >>>>>>>> It seems to me like the best recourse to fix this for now would be to > >>>>>>>> patch up the drivers that return 0 when the hardware is off by caching > >>>>>>>> the currently configured duty cycle. > >>>>>>>> > >>>>>>>> How about the patch below? > >>>>>>>> > >>>>>>>> Thierry > >>>>>>>> > >>>>>>>> --- >8 --- > >>>>>>>> From 15a52a7f1b910804fabd74a5882befd3f9d6bb37 Mon Sep 17 00:00:00 2001 > >>>>>>>> From: Thierry Reding <thierry.reding@gmail.com> > >>>>>>>> Date: Thu, 17 Oct 2019 12:56:00 +0200 > >>>>>>>> Subject: [PATCH] pwm: imx27: Cache duty cycle register value > >>>>>>>> > >>>>>>>> The hardware register containing the duty cycle value cannot be accessed > >>>>>>>> when the PWM is disabled. This causes the ->get_state() callback to read > >>>>>>>> back a duty cycle value of 0, which can confuse consumer drivers. > >>>>>>>> > >>>>>>>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > >>>>>>>> --- > >>>>>>>> drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > >>>>>>>> 1 file changed, 24 insertions(+), 7 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > >>>>>>>> index ae11d8577f18..4113d5cd4c62 100644 > >>>>>>>> --- a/drivers/pwm/pwm-imx27.c > >>>>>>>> +++ b/drivers/pwm/pwm-imx27.c > >>>>>>>> @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > >>>>>>>> struct clk *clk_per; > >>>>>>>> void __iomem *mmio_base; > >>>>>>>> struct pwm_chip chip; > >>>>>>>> + > >>>>>>>> + /* > >>>>>>>> + * The driver cannot read the current duty cycle from the hardware if > >>>>>>>> + * the hardware is disabled. Cache the last programmed duty cycle > >>>>>>>> + * value to return in that case. > >>>>>>>> + */ > >>>>>>>> + unsigned int duty_cycle; > >>>>>>>> }; > >>>>>>>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > >>>>>>>> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > >>>>>>>> tmp = NSEC_PER_SEC * (u64)(period + 2); > >>>>>>>> state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > >>>>>>>> - /* PWMSAR can be read only if PWM is enabled */ > >>>>>>>> - if (state->enabled) { > >>>>>>>> + /* > >>>>>>>> + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > >>>>>>>> + * use the cached value. > >>>>>>>> + */ > >>>>>>>> + if (state->enabled) > >>>>>>>> val = readl(imx->mmio_base + MX3_PWMSAR); > >>>>>>>> - tmp = NSEC_PER_SEC * (u64)(val); > >>>>>>>> - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > >>>>>>>> - } else { > >>>>>>>> - state->duty_cycle = 0; > >>>>>>>> - } > >>>>>>>> + else > >>>>>>>> + val = imx->duty_cycle; > >>>>>>>> + > >>>>>>>> + tmp = NSEC_PER_SEC * (u64)(val); > >>>>>>>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > >>>>>>>> if (!state->enabled) > >>>>>>>> pwm_imx27_clk_disable_unprepare(chip); > >>>>>>>> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > >>>>>>>> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > >>>>>>>> writel(period_cycles, imx->mmio_base + MX3_PWMPR); > >>>>>>>> + /* > >>>>>>>> + * Store the duty cycle for future reference in cases where > >>>>>>>> + * the MX3_PWMSAR register can't be read (i.e. when the PWM > >>>>>>>> + * is disabled). > >>>>>>>> + */ > >>>>>>>> + imx->duty_cycle = duty_cycles; > >>>>>>>> + > >>>>>>> > >>>>>>> I wonder if it would be more sensible to do this in the pwm core > >>>>>>> instead. Currently there are two drivers known with this problem. I > >>>>>>> wouldn't be surprised if there were more. > >>>>>> > >>>>>> I've inspected all the drivers and didn't spot any beyond cros-ec and > >>>>>> i.MX that have this problem. There's also no good way to do this in the > >>>>>> core, because the core doesn't know whether or not the driver is capable > >>>>>> of returning the correct duty cycle on hardare readout. So the core > >>>>>> would have to rely on state->duty_cycle that is passed in, but then the > >>>>>> offending commit becomes useless because the whole point was to return > >>>>>> the state as written to hardware (rather than the software state which > >>>>>> was being returned before that patch). > >>>>>> > >>>>>>> If we want to move clients to not rely on .period and .duty_cycle for a > >>>>>>> disabled PWM (do we?) a single change in the core is also beneficial > >>>>>>> compared to fixing several lowlevel drivers. > >>>>>> > >>>>>> These are really two orthogonal problems. We don't currently consider > >>>>>> enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not > >>>>>> prepared to do that at this point in the release cycle either. > >>>>>> > >>>>>> What this here has shown is that we have at least two drivers that don't > >>>>>> behave the way they are supposed to according to the API and they break > >>>>>> consumers. If they break for pwm-backlight, it's possible that they will > >>>>>> break for other consumers as well. So the right thing to do is fix the > >>>>>> two drivers that are broken. > >>>>>> > >>>>>> After -rc1 we no longer experiment. Instead we clean up the messes we've > >>>>>> made. We can revisit the other points once mainline is fixed. > >>>>> > >>>>> Hi Thierry, > >>>>> I just tried your patch with v5.4-rc3 with this result: > >>>>> > >>>>> root@hydraco:~# dmesg | grep pwm_ > >>>>> [ 1.772089] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > >>>>> [ 4.938759] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > >>>>> [ 4.947431] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > >>>> > >>>> Okay... this is interesting. If I understand correctly, that first line > >>>> here is where the initial hardware readout happens. The second one is > >>>> the first time when the backlight is configured, so it sets period and > >>>> polarity. But then for some reason when we read out after that to read > >>>> what state was written... we see that actually nothing was written at > >>>> all. > >>>> > >>>> And we can see why in pwm_imx27_apply(): If the PWM is not enabled, we > >>>> don't actually program any of the registers, so it's not a surprise that > >>>> things fall apart. > >>>> > >>>>> [ 4.956484] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0 > >>>>> [ 4.965473] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > >>>>> [ 4.974410] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1 > >>>>> [ 4.988617] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1 > >>>>> > >>>>> Backlight is on with full brightness at this stage. > >>>>> > >>>>> root@hydraco:/sys/class/backlight/backlight# cat brightness > >>>>> 32 > >>>>> > >>>>> root@hydraco:/sys/class/backlight/backlight# echo 32 > brightness > >>>>> [ 153.386391] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1 > >>>>> [ 153.398311] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1 > >>>>> > >>>>> Backlight goes down. > >>>>> > >>>>> root@hydraco:/sys/class/backlight/backlight# echo 1 > brightness > >>>>> [ 168.506261] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1 > >>>>> [ 168.518064] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1 > >>>>> > >>>>> Backlight goes up to almost full brightness. > >>>>> > >>>>> root@hydraco:/sys/class/backlight/backlight# echo 0 > brightness > >>>>> [ 177.496265] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0 > >>>>> [ 177.507602] pwm_imx27_get_state: period: 496485, duty: 7788, polarity: 0, enabled: 0 > >>>>> > >>>>> Backlight goes up to full brightness. > >>>>> > >>>>> So your patch does not solve my issue. > >>>>> > >>>>> The main problem I see is incorrect polarity setting. In my DT > >>>>> the pwm-backlight consumer requests PWM_POLARITY_INVERTED and > >>>>> period 500000ns. Though after reset the PWM HW registers are > >>>>> configured to normal polarity. This initial setting is read out > >>>>> and used by the consumer instead of the DT configuration. > >>>> > >>>> So the problem with the i.MX driver is that it doesn't actually write > >>>> the full state to the hardware and therefore the patch that caused these > >>>> things to break reads back an incomplete state. So we've basically got > >>>> two options: 1) make sure the hardware state is fully written or 2) make > >>>> sure that we return the cached state. > >>>> > >>>> I think 2) doesn't really make sense because it is conflicts with the > >>>> purpose of the ->get_state() callback. The only time where we should be > >>>> returning cached data is if the hardware registers don't contain the > >>>> information (as in the case of the cros-ec driver) or if we can't access > >>>> it for other reasons (such as in the case of i.MX's duty cycle). > >>>> > >>>> Does the attached patch help with your issue? The idea is to always > >>>> write the full state to the hardware, even if period and duty cycle are > >>>> unused when the PWM is disabled. That's really the kind of contract that > >>>> we have added with the offending patch in the core. > >>>> > >>>> It looks like all other drivers handle this more or less correctly, so > >>>> if we only need to fix up cros-ec and i.MX this seems like a realistic > >>>> way to fix things up. If other drivers are problematic in this regard, > >>>> we should probably revert and then fix the drivers before we can apply > >>>> that patch again. > >>> > >>> This patch combined with your previous patch appears to have worked. > >>> If you end up sending a patch series to fix this, go ahead and add > >>> > >>> Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd > >> > >> Excellent! Thanks for testing this. I'll wait until tomorrow to see if > >> there's some feedback from Enric for the cros-ec change. I'll send out > >> the total of three patches again in the hopes that those are really > >> the only two cases that are broken. > > > > When you do, can you mark it with the Fixes note? I am hoping the > > maintainers can hopefully incorporate this into 5.4 since it fixes a > > regression. > > Hi Thierry, > > I can confirm that the combination of your two patches: > - ("pwm: imx27: Unconditionally write state to hardware") > - ("pwm: imx27: Cache duty cycle register value") > > works OK and solve my problem as well. > > root@hydraco:~# dmesg | grep pwm_ > [ 1.695306] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 5.387271] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 5.396433] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 5.405500] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 > [ 5.418802] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 0 > [ 5.428208] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > [ 5.442633] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > Backlight is on from power up to userspace. > > root@hydraco:~# cd /sys/class/backlight/backlight/ > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# cat brightness > 32 > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness > > Nothing happens. > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 1 > brightness > [ 513.629043] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 > [ 513.639899] pwm_imx27_get_state: period: 500000, duty: 7833, polarity: 1, enabled: 1 > > Backlight goes to low brightness. > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 0 > brightness > [ 519.677088] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 519.687733] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 > > Backlight goes to max brightness, unresolved i.MX6 limitation. > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness > [ 923.921292] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0 > [ 923.933331] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 0 > [ 923.944546] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > [ 923.963931] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > Backlight remains at max brightness, OK. > > If I apply the patch from Uwe ("backlight: pwm_bl: configure pwm only once > per backlight toggle") on top of that, it still works and I do not see > any change in the behavior. > > root@hydraco:~# dmesg | grep pwm_ > [ 1.687461] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0 > [ 4.875087] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 4.884796] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 4.893922] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > [ 4.908473] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > root@hydraco:~# cd /sys/class/backlight/backlight/ > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# cat brightness > 32 > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 1 > brightness > [ 110.775650] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1 > [ 110.786512] pwm_imx27_get_state: period: 500000, duty: 7833, polarity: 1, enabled: 1 > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 0 > brightness > [ 128.224036] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0 > [ 128.234675] pwm_imx27_get_state: period: 500000, duty: 0, polarity: 1, enabled: 0 > > root@hydraco:/sys/devices/soc0/backlight/backlight/backlight# echo 32 > brightness > [ 138.208072] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1 > [ 138.220271] pwm_imx27_get_state: period: 500000, duty: 500000, polarity: 1, enabled: 1 > > The only difference is here when the state is changed from enabled=0 > to enabled=1. The apply/get_state combo is called only once. > > So this looks good to me. > > Tested-by: Michal Vokáč <michal.vokac@ysoft.com> > > Thank you all for the very prompt reaction! What is the plan to address the regression for 5.4? I wasn't sure if we're going to apply the i.mx fixes or temporarily revert the offending patch, or something else. (or maybe nothing at all) thanks adam > Michal > > >>>> --- >8 --- > >>>> From 7040f0038e04a1caa6dda5b6f675a9fdee0271f4 Mon Sep 17 00:00:00 2001 > >>>> From: Thierry Reding <thierry.reding@gmail.com> > >>>> Date: Thu, 17 Oct 2019 17:11:41 +0200 > >>>> Subject: [PATCH] pwm: imx27: Unconditionally write state to hardware > >>>> > >>>> The i.MX driver currently uses a shortcut and doesn't write all of the > >>>> state through to the hardware when the PWM is disabled. This causes an > >>>> inconsistent state to be read back by consumers with the result of them > >>>> malfunctioning. > >>>> > >>>> Fix this by always writing the full state through to the hardware > >>>> registers so that the correct state can always be read back. > >>>> > >>>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > >>>> --- > >>>> drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++-------------------- > >>>> 1 file changed, 59 insertions(+), 61 deletions(-) > >>>> > >>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > >>>> index 4113d5cd4c62..59d8b1289808 100644 > >>>> --- a/drivers/pwm/pwm-imx27.c > >>>> +++ b/drivers/pwm/pwm-imx27.c > >>>> @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > >>>> > >>>> pwm_get_state(pwm, &cstate); > >>>> > >>>> - if (state->enabled) { > >>>> - c = clk_get_rate(imx->clk_per); > >>>> - c *= state->period; > >>>> - > >>>> - do_div(c, 1000000000); > >>>> - period_cycles = c; > >>>> - > >>>> - prescale = period_cycles / 0x10000 + 1; > >>>> - > >>>> - period_cycles /= prescale; > >>>> - c = (unsigned long long)period_cycles * state->duty_cycle; > >>>> - do_div(c, state->period); > >>>> - duty_cycles = c; > >>>> - > >>>> - /* > >>>> - * according to imx pwm RM, the real period value should be > >>>> - * PERIOD value in PWMPR plus 2. > >>>> - */ > >>>> - if (period_cycles > 2) > >>>> - period_cycles -= 2; > >>>> - else > >>>> - period_cycles = 0; > >>>> - > >>>> - /* > >>>> - * Wait for a free FIFO slot if the PWM is already enabled, and > >>>> - * flush the FIFO if the PWM was disabled and is about to be > >>>> - * enabled. > >>>> - */ > >>>> - if (cstate.enabled) { > >>>> - pwm_imx27_wait_fifo_slot(chip, pwm); > >>>> - } else { > >>>> - ret = pwm_imx27_clk_prepare_enable(chip); > >>>> - if (ret) > >>>> - return ret; > >>>> - > >>>> - pwm_imx27_sw_reset(chip); > >>>> - } > >>>> - > >>>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > >>>> - writel(period_cycles, imx->mmio_base + MX3_PWMPR); > >>>> - > >>>> - /* > >>>> - * Store the duty cycle for future reference in cases where > >>>> - * the MX3_PWMSAR register can't be read (i.e. when the PWM > >>>> - * is disabled). > >>>> - */ > >>>> - imx->duty_cycle = duty_cycles; > >>>> - > >>>> - cr = MX3_PWMCR_PRESCALER_SET(prescale) | > >>>> - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > >>>> - FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > >>>> - MX3_PWMCR_DBGEN | MX3_PWMCR_EN; > >>>> - > >>>> - if (state->polarity == PWM_POLARITY_INVERSED) > >>>> - cr |= FIELD_PREP(MX3_PWMCR_POUTC, > >>>> - MX3_PWMCR_POUTC_INVERTED); > >>>> - > >>>> - writel(cr, imx->mmio_base + MX3_PWMCR); > >>>> - } else if (cstate.enabled) { > >>>> - writel(0, imx->mmio_base + MX3_PWMCR); > >>>> + c = clk_get_rate(imx->clk_per); > >>>> + c *= state->period; > >>>> > >>>> - pwm_imx27_clk_disable_unprepare(chip); > >>>> + do_div(c, 1000000000); > >>>> + period_cycles = c; > >>>> + > >>>> + prescale = period_cycles / 0x10000 + 1; > >>>> + > >>>> + period_cycles /= prescale; > >>>> + c = (unsigned long long)period_cycles * state->duty_cycle; > >>>> + do_div(c, state->period); > >>>> + duty_cycles = c; > >>>> + > >>>> + /* > >>>> + * according to imx pwm RM, the real period value should be PERIOD > >>>> + * value in PWMPR plus 2. > >>>> + */ > >>>> + if (period_cycles > 2) > >>>> + period_cycles -= 2; > >>>> + else > >>>> + period_cycles = 0; > >>>> + > >>>> + /* > >>>> + * Wait for a free FIFO slot if the PWM is already enabled, and flush > >>>> + * the FIFO if the PWM was disabled and is about to be enabled. > >>>> + */ > >>>> + if (cstate.enabled) { > >>>> + pwm_imx27_wait_fifo_slot(chip, pwm); > >>>> + } else { > >>>> + ret = pwm_imx27_clk_prepare_enable(chip); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + pwm_imx27_sw_reset(chip); > >>>> } > >>>> > >>>> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > >>>> + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > >>>> + > >>>> + /* > >>>> + * Store the duty cycle for future reference in cases where the > >>>> + * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled). > >>>> + */ > >>>> + imx->duty_cycle = duty_cycles; > >>>> + > >>>> + cr = MX3_PWMCR_PRESCALER_SET(prescale) | > >>>> + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > >>>> + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > >>>> + MX3_PWMCR_DBGEN; > >>>> + > >>>> + if (state->polarity == PWM_POLARITY_INVERSED) > >>>> + cr |= FIELD_PREP(MX3_PWMCR_POUTC, > >>>> + MX3_PWMCR_POUTC_INVERTED); > >>>> + > >>>> + if (state->enabled) > >>>> + cr |= MX3_PWMCR_EN; > >>>> + > >>>> + writel(cr, imx->mmio_base + MX3_PWMCR); > >>>> + > >>>> + if (!state->enabled && cstate.enabled) > >>>> + pwm_imx27_clk_disable_unprepare(chip); > >>>> + > >>>> return 0; > >>>> } > >>>> > >>>> -- > >>>> 2.23.0 > >>>> >
On Wed, Oct 23, 2019 at 11:16 AM Adam Ford <aford173@gmail.com> wrote: > What is the plan to address the regression for 5.4? I wasn't sure if > we're going to apply the i.mx fixes or temporarily revert the > offending patch, or something else. (or maybe nothing at all) Yes, I do see the regression on a imx53 board with 5.4-rc too and also interested on a fix. Thanks
On Wed, Oct 23, 2019 at 11:23:11AM -0300, Fabio Estevam wrote: > On Wed, Oct 23, 2019 at 11:16 AM Adam Ford <aford173@gmail.com> wrote: > > > What is the plan to address the regression for 5.4? I wasn't sure if > > we're going to apply the i.mx fixes or temporarily revert the > > offending patch, or something else. (or maybe nothing at all) > > Yes, I do see the regression on a imx53 board with 5.4-rc too and also > interested on a fix. Thierry already sent a revert of my change to the list. We only discussed the wording shortly and I expect that this revert will make it into 5.4. Best regards Uwe
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 746eebc411df..ddebd62b3978 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -42,10 +42,8 @@ struct pwm_bl_data { static void pwm_backlight_power_on(struct pwm_bl_data *pb) { - struct pwm_state state; int err; - pwm_get_state(pb->pwm, &state); if (pb->enabled) return; @@ -53,9 +51,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) if (err < 0) dev_err(pb->dev, "failed to enable power supply\n"); - state.enabled = true; - pwm_apply_state(pb->pwm, &state); - if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay); @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) static void pwm_backlight_power_off(struct pwm_bl_data *pb) { - struct pwm_state state; - - pwm_get_state(pb->pwm, &state); - if (!pb->enabled) - return; - if (pb->enable_gpio) gpiod_set_value_cansleep(pb->enable_gpio, 0); if (pb->pwm_off_delay) msleep(pb->pwm_off_delay); - state.enabled = false; - state.duty_cycle = 0; - pwm_apply_state(pb->pwm, &state); - regulator_disable(pb->power_supply); pb->enabled = false; } -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state) { unsigned int lth = pb->lth_brightness; - struct pwm_state state; u64 duty_cycle; - pwm_get_state(pb->pwm, &state); - if (pb->levels) duty_cycle = pb->levels[brightness]; else duty_cycle = brightness; - duty_cycle *= state.period - lth; + duty_cycle *= state->period - lth; do_div(duty_cycle, pb->scale); return duty_cycle + lth; @@ -122,12 +104,20 @@ static int pwm_backlight_update_status(struct backlight_device *bl) if (brightness > 0) { pwm_get_state(pb->pwm, &state); - state.duty_cycle = compute_duty_cycle(pb, brightness); + state.duty_cycle = compute_duty_cycle(pb, brightness, &state); + state.enabled = true; pwm_apply_state(pb->pwm, &state); + pwm_backlight_power_on(pb); - } else + } else { pwm_backlight_power_off(pb); + pwm_get_state(pb->pwm, &state); + state.enabled = false; + state.duty_cycle = 0; + pwm_apply_state(pb->pwm, &state); + } + if (pb->notify_after) pb->notify_after(pb->dev, brightness);
A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state")) changed the semantic of pwm_get_state() and disclosed an (as it seems) common problem in lowlevel PWM drivers. By not relying on the period and duty cycle being retrievable from a disabled PWM this type of problem is worked around. Apart from this issue only calling the pwm_get_state/pwm_apply_state combo once is also more effective. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, There are now two reports about 01ccf903edd6 breaking a backlight. As far as I understand the problem this is a combination of the backend pwm driver yielding surprising results and the pwm-bl driver doing things more complicated than necessary. So I guess this patch works around these problems. Still it would be interesting to find out the details in the imx driver that triggers the problem. So Adam, can you please instrument the pwm-imx27 driver to print *state at the beginning of pwm_imx27_apply() and the end of pwm_imx27_get_state() and provide the results? Note I only compile tested this change. Best regards Uwe drivers/video/backlight/pwm_bl.c | 34 +++++++++++--------------------- 1 file changed, 12 insertions(+), 22 deletions(-)