[PATCH 1/3] pwm: lpss: Bug-fix + 2 improvements

[PATCH 1/3] pwm: lpss: Bug-fix + 2 improvements

[Hans de Goede]

Hi Thierry,

Here are 3 pwm-lpss patches, note the first one is a bug /
regression fix and should be applied to 4.11 as it fixes a bug
in your current for-next branch.

Regards,

Hans

[PATCH 1/3] pwm: lpss: Do not set / wait_for update_bit when not enabled

[Hans de Goede]

At least on cherrytrail, the update bit will never go low when the
enabled bit is not set.

This causes the backlight on my cube iwork8 air tablet to never go on
again after being turned off, because the enable path does:

	pwm_lpss_prepare();
	ret = pwm_lpss_update(pwm);
	if (ret)
		return ret;
	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);

And the pwm_lpss_update() call fails, as the setting of the
UPDATE bit never gets acked, because the ENABLE bit is not set.

Subsequent calls then all fail because of the pwm_lpss_is_updating()
check done by pwm_lpss_apply().

This commit fixes this by setting the enable bit before calling
pwm_lpss_update().

Fixes: 10d56a4cb1c6 ("pwm: lpss: Avoid reconfiguring while UPDATE bit...")
Cc: Ilkka Koskinen <ilkka.koskinen@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 689d2c1..6c99abc 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 				return ret;
 			}
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
+			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
 			ret = pwm_lpss_update(pwm);
 			if (ret) {
 				pm_runtime_put(chip->dev);
 				return ret;
 			}
-			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
 		} else {
 			ret = pwm_lpss_is_updating(pwm);
 			if (ret)
-- 
2.9.3

Re: [PATCH 1/3] pwm: lpss: Do not set / wait_for update_bit when not enabled

[Ilkka Koskinen]

+Andy


Hi,

Thanks for the patch!

On Mon, Feb 20, 2017 at 10:16:55PM +0200, Hans de Goede wrote:
> At least on cherrytrail, the update bit will never go low when the
> enabled bit is not set.
> 
> This causes the backlight on my cube iwork8 air tablet to never go on
> again after being turned off, because the enable path does:
> 
> 	pwm_lpss_prepare();
> 	ret = pwm_lpss_update(pwm);
> 	if (ret)
> 		return ret;
> 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
> 
> And the pwm_lpss_update() call fails, as the setting of the
> UPDATE bit never gets acked, because the ENABLE bit is not set.
> 
> Subsequent calls then all fail because of the pwm_lpss_is_updating()
> check done by pwm_lpss_apply().
> 
> This commit fixes this by setting the enable bit before calling
> pwm_lpss_update().
> 
> Fixes: 10d56a4cb1c6 ("pwm: lpss: Avoid reconfiguring while UPDATE bit...")
> Cc: Ilkka Koskinen <ilkka.koskinen@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 689d2c1..6c99abc 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  				return ret;
>  			}
>  			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
> +			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>  			ret = pwm_lpss_update(pwm);

The BXT documentation that I have recommends to set update bit before enable
one. However, based on your experiment on Cherryview, we still have to set it
before read_poll_timeout(). 

Andy, should we indeed remove the return value from apply() and just print a warning
like Mika initially suggested?

--Ilkka

>  			if (ret) {
>  				pm_runtime_put(chip->dev);
>  				return ret;
>  			}
> -			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>  		} else {
>  			ret = pwm_lpss_is_updating(pwm);
>  			if (ret)
> -- 
> 2.9.3
> 

Re: [PATCH 1/3] pwm: lpss: Do not set / wait_for update_bit when not enabled

[Andy Shevchenko]

On Thu, 2017-02-23 at 00:06 -0800, Ilkka Koskinen wrote:

> > At least on cherrytrail, the update bit will never go low when the
> > enabled bit is not set.
> > 
> > This causes the backlight on my cube iwork8 air tablet to never go
> > on
> > again after being turned off, because the enable path does:
> > 
> > 	pwm_lpss_prepare();
> > 	ret = pwm_lpss_update(pwm);
> > 	if (ret)
> > 		return ret;
> > 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
> > 
> > And the pwm_lpss_update() call fails, as the setting of the
> > UPDATE bit never gets acked, because the ENABLE bit is not set.
> > 
> > Subsequent calls then all fail because of the pwm_lpss_is_updating()
> > check done by pwm_lpss_apply().
> > 

> > This commit fixes this by setting the enable bit before calling
> > pwm_lpss_update().

This might break other systems.

> > @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip
> > *chip, struct pwm_device *pwm,
> >  				return ret;
> >  			}
> >  			pwm_lpss_prepare(lpwm, pwm, state-
> > >duty_cycle, state->period);
> > +			pwm_lpss_write(pwm, pwm_lpss_read(pwm) |
> > PWM_ENABLE);
> >  			ret = pwm_lpss_update(pwm);
> 
> The BXT documentation that I have recommends to set update bit before
> enable
> one.

We have the same work flow for all SoCs that have this PWM IP. I suspect
it might be a silicon bug somewhere, but I have never heard of it.

>  However, based on your experiment on Cherryview, we still have to set
> it
> before read_poll_timeout(). 

I would test and confirm the issue on our machines to see that the fix
doesn't break the rest. Seems like we have several subtle different
implementation of it: BYT, CHT, MRFLD, BXT.

> Andy, should we indeed remove the return value from apply() and just
> print a warning
> like Mika initially suggested?

I definitely consider it better than Hans' initial proposal.

Hans, can you just replace

 return ret;

by

 dev_warn(..., "UPDATE bit is not cleared!\n");

and test it?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/3] pwm: lpss: Do not set / wait_for update_bit when not enabled

[Hans de Goede]

Hi,

On 28-02-17 10:46, Andy Shevchenko wrote:
> On Thu, 2017-02-23 at 00:06 -0800, Ilkka Koskinen wrote:
>
>>> At least on cherrytrail, the update bit will never go low when the
>>> enabled bit is not set.
>>>
>>> This causes the backlight on my cube iwork8 air tablet to never go
>>> on
>>> again after being turned off, because the enable path does:
>>>
>>> 	pwm_lpss_prepare();
>>> 	ret = pwm_lpss_update(pwm);
>>> 	if (ret)
>>> 		return ret;
>>> 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
>>>
>>> And the pwm_lpss_update() call fails, as the setting of the
>>> UPDATE bit never gets acked, because the ENABLE bit is not set.
>>>
>>> Subsequent calls then all fail because of the pwm_lpss_is_updating()
>>> check done by pwm_lpss_apply().
>>>
>
>>> This commit fixes this by setting the enable bit before calling
>>> pwm_lpss_update().
>
> This might break other systems.
>
>>> @@ -137,12 +137,12 @@ static int pwm_lpss_apply(struct pwm_chip
>>> *chip, struct pwm_device *pwm,
>>>  				return ret;
>>>  			}
>>>  			pwm_lpss_prepare(lpwm, pwm, state-
>>>> duty_cycle, state->period);
>>> +			pwm_lpss_write(pwm, pwm_lpss_read(pwm) |
>>> PWM_ENABLE);
>>>  			ret = pwm_lpss_update(pwm);
>>
>> The BXT documentation that I have recommends to set update bit before
>> enable
>> one.
>
> We have the same work flow for all SoCs that have this PWM IP. I suspect
> it might be a silicon bug somewhere, but I have never heard of it.
>
>>  However, based on your experiment on Cherryview, we still have to set
>> it
>> before read_poll_timeout().
>
> I would test and confirm the issue on our machines to see that the fix
> doesn't break the rest. Seems like we have several subtle different
> implementation of it: BYT, CHT, MRFLD, BXT.
>
>> Andy, should we indeed remove the return value from apply() and just
>> print a warning
>> like Mika initially suggested?
>
> I definitely consider it better than Hans' initial proposal.
>
> Hans, can you just replace
>
>  return ret;
>
> by
>
>  dev_warn(..., "UPDATE bit is not cleared!\n");
>
> and test it?

Sure, done, as expected that fixes it, but not really in a pretty way,
now at boot and every time the screen goes into DPMS / off I get:

[    9.461180] pwm-lpss 80862288:00: PWM_SW_UPDATE was not cleared
[    9.461190] pwm-lpss 80862288:00: UPDATE bit is not cleared!
[  124.642002] pwm-lpss 80862288:00: PWM_SW_UPDATE was not cleared
[  124.642007] pwm-lpss 80862288:00: UPDATE bit is not cleared!

I believe it would be better to split pwm_lpss_update into setting the
update-bit and pwm_lpss_wait_for_update, and move the
pwm_lpss_wait_for_update call in the enable path to after setting the
enable-bit.

I've just finished testing a patch which does that, probably easier
to discuss it when you can see the code, so I will send that out as
a v2 of this one.

Regards,

Hans

[PATCH 2/3] pwm: lpss: Simplify update check in pwm_lpss_apply

[Hans de Goede]

Simple keep a pm ref over the entire function and do the update check
once at the beginning. This simplifies the code and makes sure we also
check the update bit when disabling the pwm.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 6c99abc..0b549dc 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -128,34 +128,30 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
 	int ret;
 
+	pm_runtime_get_sync(chip->dev);
+	ret = pwm_lpss_is_updating(pwm);
+	if (ret)
+		goto out;
+
 	if (state->enabled) {
 		if (!pwm_is_enabled(pwm)) {
-			pm_runtime_get_sync(chip->dev);
-			ret = pwm_lpss_is_updating(pwm);
-			if (ret) {
-				pm_runtime_put(chip->dev);
-				return ret;
-			}
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
 			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
 			ret = pwm_lpss_update(pwm);
-			if (ret) {
-				pm_runtime_put(chip->dev);
-				return ret;
-			}
+			if (ret == 0)
+				pm_runtime_get(chip->dev);
 		} else {
-			ret = pwm_lpss_is_updating(pwm);
-			if (ret)
-				return ret;
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
-			return pwm_lpss_update(pwm);
+			ret = pwm_lpss_update(pwm);
 		}
 	} else if (pwm_is_enabled(pwm)) {
 		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
 		pm_runtime_put(chip->dev);
 	}
 
-	return 0;
+out:
+	pm_runtime_put(chip->dev);
+	return ret;
 }
 
 static const struct pwm_ops pwm_lpss_ops = {
-- 
2.9.3

[PATCH 3/3] pwm: lpss: Add get_state callback

[Hans de Goede]

Add a get_state callback so that the initial state correctly reflects
the actual hardware state.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
Changes in v2:
-Rebase on top of linux-pwm/for-next
---
 drivers/pwm/pwm-lpss.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 0b549dc..5e7e9ac 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -154,8 +154,36 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
+static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+	unsigned long base_unit_range, freq;
+	unsigned long long base_unit, on_time_div;
+	u32 ctrl;
+
+	base_unit_range = BIT(lpwm->info->base_unit_bits);
+
+	ctrl = pwm_lpss_read(pwm);
+	on_time_div = ctrl & PWM_ON_TIME_DIV_MASK;
+	base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) & (base_unit_range - 1);
+
+	freq = base_unit * lpwm->info->clk_rate / base_unit_range;
+	if (freq == 0)
+		freq = 1;
+
+	state->period = NSEC_PER_SEC / freq;
+	state->duty_cycle = state->period * on_time_div / 255ULL;
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = (ctrl & PWM_ENABLE) ? true : false;
+
+	if (state->enabled)
+		pm_runtime_get_sync(chip->dev);
+}
+
 static const struct pwm_ops pwm_lpss_ops = {
 	.apply = pwm_lpss_apply,
+	.get_state = pwm_lpss_get_state,
 	.owner = THIS_MODULE,
 };
 
-- 
2.9.3

Re: [PATCH 3/3] pwm: lpss: Add get_state callback

[Andy Shevchenko]

On Mon, 2017-02-20 at 21:16 +0100, Hans de Goede wrote:
> Add a get_state callback so that the initial state correctly reflects
> the actual hardware state.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>

 
> +static void pwm_lpss_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> +			       struct pwm_state *state)
> +{
> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +	unsigned long base_unit_range, freq;
> +	unsigned long long base_unit, on_time_div;

Something with types.

on_time_div is 8-bit only.
OTOH freq (due to multiplication below) might need bigger type.

Have you tried 32-bit compilation, btw? No warnings for division?

> +	u32 ctrl;
> +
> +	base_unit_range = BIT(lpwm->info->base_unit_bits);
> +
> +	ctrl = pwm_lpss_read(pwm);
> +	on_time_div = ctrl & PWM_ON_TIME_DIV_MASK;
> +	base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) & (base_unit_range
> - 1);
> +
> +	freq = base_unit * lpwm->info->clk_rate / base_unit_range;

> +	if (freq == 0)
> +		freq = 1;

Why?
I'm not sure it will give correct value.

> +

> +	state->period = NSEC_PER_SEC / freq;


> +	state->duty_cycle = state->period * on_time_div / 255ULL;
> +	state->polarity = PWM_POLARITY_NORMAL;

> +	state->enabled = (ctrl & PWM_ENABLE) ? true : false;

!!(ctrl & PWM_ENABLE)

> +

> +	if (state->enabled)
> +		pm_runtime_get_sync(chip->dev);

How is this supposed to work in balance with ->apply() ?

> +}
> +
>  static const struct pwm_ops pwm_lpss_ops = {
>  	.apply = pwm_lpss_apply,
> +	.get_state = pwm_lpss_get_state,
>  	.owner = THIS_MODULE,
>  };
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] pwm: lpss: Add get_state callback

[Hans de Goede]

Hi,

On 21-02-17 11:33, Andy Shevchenko wrote:
> On Mon, 2017-02-20 at 21:16 +0100, Hans de Goede wrote:
>> Add a get_state callback so that the initial state correctly reflects
>> the actual hardware state.
>>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>
>
>> +static void pwm_lpss_get_state(struct pwm_chip *chip, struct
>> pwm_device *pwm,
>> +			       struct pwm_state *state)
>> +{
>> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>> +	unsigned long base_unit_range, freq;
>> +	unsigned long long base_unit, on_time_div;
>
> Something with types.
>
> on_time_div is 8-bit only.

Right, but state->period can be up to 1000000000, so we need
64 bits after multiplying that by max 255:

	state->duty_cycle = state->period * on_time_div / 255ULL;

Basically I've forced base_unit and on_time_div to be 64 bits
to avoid overflows when multiplying them later. Just like
pwm_lpss_prepare is doing.

> OTOH freq (due to multiplication below) might need bigger type.

base_unit is never bigger then base_unit_range, so:

	freq = base_unit * lpwm->info->clk_rate / base_unit_range;

Can never result in freq > lpwm->info->clk_rate which is an
unsigned long, so using an unsigned long should be enough,
but to be able to use do_div I need to make it an unsigned long long
regardless.

> Have you tried 32-bit compilation, btw? No warnings for division?

No, I have not tried, but you're right I need to use do_div here to
avoid issues with 64 bit division on 32 bit archs.


>
>> +	u32 ctrl;
>> +
>> +	base_unit_range = BIT(lpwm->info->base_unit_bits);
>> +
>> +	ctrl = pwm_lpss_read(pwm);
>> +	on_time_div = ctrl & PWM_ON_TIME_DIV_MASK;
>> +	base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) & (base_unit_range
>> - 1);
>> +
>> +	freq = base_unit * lpwm->info->clk_rate / base_unit_range;
>
>> +	if (freq == 0)
>> +		freq = 1;
>
> Why?
> I'm not sure it will give correct value.

To avoid divide by 0 in case the ctrl register reads
all 0 for the base_unit, but I've solved this slightly
different for v2.

>
>> +
>
>> +	state->period = NSEC_PER_SEC / freq;
>
>
>> +	state->duty_cycle = state->period * on_time_div / 255ULL;
>> +	state->polarity = PWM_POLARITY_NORMAL;
>
>> +	state->enabled = (ctrl & PWM_ENABLE) ? true : false;
>
> !!(ctrl & PWM_ENABLE)

Will fix for v2.

>> +
>
>> +	if (state->enabled)
>> +		pm_runtime_get_sync(chip->dev);
>
> How is this supposed to work in balance with ->apply() ?

apply() only does the pm_runtime_get when going from not
enabled to enabled (and releases it in the other direction):

         if (state->enabled) {
                 if (!pwm_is_enabled(pwm)) {
                         pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->pe
                         pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
                         ret = pwm_lpss_update(pwm);
                         if (ret == 0)
                                 pm_runtime_get(chip->dev);
                 } else {
                         pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->pe
                         ret = pwm_lpss_update(pwm);
                 }
         } else if (pwm_is_enabled(pwm)) {

Where state->enabled is looking at the new state and
pwm_is_enabled(pwm) at the current state, which on the
first call to apply is the state returned by
pwm_lpss_get_state during probe.

If the pwm is enabled when we get probed, we need to do a
pm_runtime_get to avoid runtime suspend kicking in while
enabled and to balance the put which apply() will do an
a transition to disabled.

Regards,

Hans


>
>> +}
>> +
>>  static const struct pwm_ops pwm_lpss_ops = {
>>  	.apply = pwm_lpss_apply,
>> +	.get_state = pwm_lpss_get_state,
>>  	.owner = THIS_MODULE,
>>  };
>>
>