[PATCH v2 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[PATCH v2 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Jonathan Richardson]

    The config procedure doesn't follow the spec which periodically results
    in failing to enable the output signal. This happens one in ten or
    twenty attempts. Following the spec and adding a 400ns delay in the
    appropriate locations resolves this problem. It also ensures that the
    signal transition is smooth.

    If config is called when the pwm is disabled and there is nothing to do,
    the while loop to calculate duty cycle and period doesn't need to be
    done. The function now just returns if the pwm state is disabled.

    The disable procedure now also follows the spec to ensure a smooth
    transition. Not following the spec can cause non-smooth transitions.

    The enable procedure now clears the enabled bit if enabling failed.
    Enabling can fail if an invalid duty cycle and period is set. This
    prevents the sysfs interface from reporting the pwm is enabled after a
    failed call to enable.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..6d92026 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	/* Clear trigger bit but set smooth bit to maintain old output */
-	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-	writel(value, kp->base + PWM_CONTROL_OFFSET);
+	/*
+	 * There must be a min 400ns delay between clearing enable and setting
+	 * it. Failing to do this may result in no PWM signal.
+	 */
+	ndelay(400);
 
 	/* Set trigger bit and clear smooth bit to apply new settings */
 	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* PWMOUT_ENABLE must be held high for at least 400 ns. */
+	ndelay(400);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -99,6 +103,9 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long prescale = PRESCALE_MIN, pc, dc;
 	unsigned int value, chan = pwm->hwpwm;
 
+	if (!test_bit(PWMF_ENABLED, &pwm->flags))
+		return 0;
+
 	/*
 	 * Find period count, duty count and prescale to suit duty_ns and
 	 * period_ns. This is done according to formulas described below:
@@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
 			return -EINVAL;
+		}
+
+		/* If duty_ns is not achievable then return */
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
+			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-		value = readl(kp->base + PRESCALE_OFFSET);
-		value &= ~PRESCALE_MASK(chan);
-		value |= prescale << PRESCALE_SHIFT(chan);
-		writel(value, kp->base + PRESCALE_OFFSET);
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
 
-		writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+	/*
+	 * Clear trigger bit but set smooth bit to maintain old
+	 * output.
+	 */
+	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-		kona_pwmc_apply_settings(kp, chan);
-	}
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	value |= prescale << PRESCALE_SHIFT(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+
+	writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	return 0;
 }
@@ -173,11 +209,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
-
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
-
 	clk_disable_unprepare(kp->clk);
 
 	return 0;
@@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	ret = clk_prepare_enable(kp->clk);
 	if (ret < 0) {
+		clear_bit(PWMF_ENABLED, &pwm->flags);
 		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
 		return ret;
 	}
 
 	ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
 	if (ret < 0) {
+		clear_bit(PWMF_ENABLED, &pwm->flags);
 		clk_disable_unprepare(kp->clk);
 		return ret;
 	}
@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+	/* Set smooth type to 0 and disable */
+	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	kona_pwmc_apply_settings(kp, chan);
+	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
+	/* Set prescale to 0 for this channel */
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	clk_disable_unprepare(kp->clk);
 }
-- 
1.7.9.5

[PATCH v2 2/2] pwm: kona: Remove setting default smooth type and polarity for all channels

[Jonathan Richardson]

Setting the default polarity in probe to normal for all channels caused
the speaker pwm channel to click. The polarity does need to be set to
normal because the hw default is inversed whereas the pwm framework
defaults to normal. If a channel is enabled without setting the polarity
then the signal would be inversed while linux reports normal. A check
is now done prior to enabling the channel to ensure that the hw polarity
matches the desired polarity and is changed if there is a discrepency. This
prevents unnecessary settings being applied to unused channels but still
ensures the correct polarity to be set.

Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 6d92026..46a3da5 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -95,6 +95,32 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	ndelay(400);
 }
 
+static void kona_pwmc_check_set_polarity(struct pwm_chip *chip,
+	struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = to_kona_pwmc(chip);
+	unsigned int chan = pwm->hwpwm;
+	enum pwm_polarity polarity = pwm->polarity;
+	unsigned int hw_pol;
+	unsigned int value = 0;
+
+	value = hw_pol = readl(kp->base + PWM_CONTROL_OFFSET);
+	hw_pol = (hw_pol >> PWM_CONTROL_POLARITY_SHIFT(chan)) & 0x1;
+
+	/*
+	 * If current polarity not the same as h/w then set polarity so that
+	 * they match.
+	 */
+	if (!hw_pol != polarity) {
+		if (polarity == PWM_POLARITY_NORMAL)
+			value |= 1 << PWM_CONTROL_POLARITY_SHIFT(chan);
+		else
+			value &= ~(1 << PWM_CONTROL_POLARITY_SHIFT(chan));
+
+		writel(value, kp->base + PWM_CONTROL_OFFSET);
+	}
+}
+
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
@@ -162,6 +188,13 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
 		chan, pc, dc, prescale);
 
+	/*
+	 * Ensure polarity is set properly. The default value for h/w and the
+	 * PWM framework are different. If a channel is enabled without setting
+	 * the polarity, the default value would be inconsistent to the signal.
+	 */
+	kona_pwmc_check_set_polarity(chip, pwm);
+
 	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
 	/*
@@ -310,11 +343,8 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 	}
 
 	/* Set smooth mode, push/pull, and normal polarity for all channels */
-	for (chan = 0; chan < kp->chip.npwm; chan++) {
-		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	for (chan = 0; chan < kp->chip.npwm; chan++)
 		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
-		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
-	}
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-- 
1.7.9.5

Re: [PATCH v2 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Tim Kryger]

On Wed, Dec 10, 2014 at 5:07 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:

>     If config is called when the pwm is disabled and there is nothing to do,
>     the while loop to calculate duty cycle and period doesn't need to be
>     done. The function now just returns if the pwm state is disabled.

It doesn't take long to figure out whether the duty and period are achievable.

If the caller specifies bad settings, why not return an error immediately?

> @@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>         unsigned int chan = pwm->hwpwm;
> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +       /* Set smooth type to 0 and disable */
> +       value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>
>         /* Simulate a disable by configuring for zero duty */
>         writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> -       kona_pwmc_apply_settings(kp, chan);
> +       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>
> -       /* Wait for waveform to settle before gating off the clock */
> -       ndelay(400);
> +       /* Set prescale to 0 for this channel */
> +       value = readl(kp->base + PRESCALE_OFFSET);
> +       value &= ~PRESCALE_MASK(chan);
> +       writel(value, kp->base + PRESCALE_OFFSET);
> +
> +       kona_pwmc_apply_settings(kp, chan);
>
>         clk_disable_unprepare(kp->clk);
>  }

I've mentioned this before but I will say it again, when the smooth
and trigger bit are both low, the output is constant high.

If you look at the PWM output on a scope you will see it go high for
400 ns during your disable even if the duty prior to the disable was
zero.

How are you testing your proposed changes?

Thanks,
Tim Kryger

Re: [PATCH v2 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Jonathan Richardson]

On 14-12-14 11:18 PM, Tim Kryger wrote:
> On Wed, Dec 10, 2014 at 5:07 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
> 
>>     If config is called when the pwm is disabled and there is nothing to do,
>>     the while loop to calculate duty cycle and period doesn't need to be
>>     done. The function now just returns if the pwm state is disabled.
> 
> It doesn't take long to figure out whether the duty and period are achievable.
> 
> If the caller specifies bad settings, why not return an error immediately?

Sorry, not sure what you're suggesting here. It's returning as soon as
it can isn't it? Nothing changed in the way the period and duty cycle
were calculated and checked in the while loop.

> 
>> @@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>  {
>>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>>         unsigned int chan = pwm->hwpwm;
>> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>> +
>> +       /* Set smooth type to 0 and disable */
>> +       value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>>         /* Simulate a disable by configuring for zero duty */
>>         writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> -       kona_pwmc_apply_settings(kp, chan);
>> +       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>>
>> -       /* Wait for waveform to settle before gating off the clock */
>> -       ndelay(400);
>> +       /* Set prescale to 0 for this channel */
>> +       value = readl(kp->base + PRESCALE_OFFSET);
>> +       value &= ~PRESCALE_MASK(chan);
>> +       writel(value, kp->base + PRESCALE_OFFSET);
>> +
>> +       kona_pwmc_apply_settings(kp, chan);
>>
>>         clk_disable_unprepare(kp->clk);
>>  }
> 
> I've mentioned this before but I will say it again, when the smooth
> and trigger bit are both low, the output is constant high.
> 
> If you look at the PWM output on a scope you will see it go high for
> 400 ns during your disable even if the duty prior to the disable was
> zero.
> 
> How are you testing your proposed changes?
> 

I see what you mean now and verified it. For a smooth transition even on
disable the smooth bit should be set high, not low. It's the same
procedure as in config. Setting the bit high instead of low gets rid of
the 400ns transition high. I can make this change.

> Thanks,
> Tim Kryger
> 

[PATCH v3 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Jonathan Richardson]

    The config procedure doesn't follow the spec which periodically results
    in failing to enable the output signal. This happens one in ten or
    twenty attempts. Following the spec and adding a 400ns delay in the
    appropriate locations resolves this problem. It also ensures that the
    signal transition is smooth.

    If config is called when the pwm is disabled and there is nothing to do,
    the while loop to calculate duty cycle and period doesn't need to be
    done. The function now just returns if the pwm state is disabled.

    The disable procedure now also follows the spec to ensure a smooth
    transition. Not following the spec can cause non-smooth transitions.

    The enable procedure now clears the enabled bit if enabling failed.
    Enabling can fail if an invalid duty cycle and period is set. This
    prevents the sysfs interface from reporting the pwm is enabled after a
    failed call to enable.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..a831bb2 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	/* Clear trigger bit but set smooth bit to maintain old output */
-	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-	writel(value, kp->base + PWM_CONTROL_OFFSET);
+	/*
+	 * There must be a min 400ns delay between clearing enable and setting
+	 * it. Failing to do this may result in no PWM signal.
+	 */
+	ndelay(400);
 
 	/* Set trigger bit and clear smooth bit to apply new settings */
 	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* PWMOUT_ENABLE must be held high for at least 400 ns. */
+	ndelay(400);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -99,6 +103,9 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long prescale = PRESCALE_MIN, pc, dc;
 	unsigned int value, chan = pwm->hwpwm;
 
+	if (!test_bit(PWMF_ENABLED, &pwm->flags))
+		return 0;
+
 	/*
 	 * Find period count, duty count and prescale to suit duty_ns and
 	 * period_ns. This is done according to formulas described below:
@@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
 			return -EINVAL;
+		}
+
+		/* If duty_ns is not achievable then return */
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
+			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-		value = readl(kp->base + PRESCALE_OFFSET);
-		value &= ~PRESCALE_MASK(chan);
-		value |= prescale << PRESCALE_SHIFT(chan);
-		writel(value, kp->base + PRESCALE_OFFSET);
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
 
-		writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+	/*
+	 * Clear trigger bit but set smooth bit to maintain old
+	 * output.
+	 */
+	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-		kona_pwmc_apply_settings(kp, chan);
-	}
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	value |= prescale << PRESCALE_SHIFT(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+
+	writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	return 0;
 }
@@ -173,11 +209,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
-
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
-
 	clk_disable_unprepare(kp->clk);
 
 	return 0;
@@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	ret = clk_prepare_enable(kp->clk);
 	if (ret < 0) {
+		clear_bit(PWMF_ENABLED, &pwm->flags);
 		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
 		return ret;
 	}
 
 	ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
 	if (ret < 0) {
+		clear_bit(PWMF_ENABLED, &pwm->flags);
 		clk_disable_unprepare(kp->clk);
 		return ret;
 	}
@@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+	/* Set smooth type to 1 and disable */
+	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	kona_pwmc_apply_settings(kp, chan);
+	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
+	/* Set prescale to 0 for this channel */
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	clk_disable_unprepare(kp->clk);
 }
-- 
1.7.9.5

[PATCH v3 2/2] pwm: kona: Remove setting default smooth type and polarity for all channels

[Jonathan Richardson]

Setting the default polarity in probe to normal for all channels caused
the speaker pwm channel to click. The polarity does need to be set to
normal because the hw default is inversed whereas the pwm framework
defaults to normal. If a channel is enabled without setting the polarity
then the signal would be inversed while linux reports normal. A check
is now done prior to enabling the channel to ensure that the hw polarity
matches the desired polarity and is changed if there is a discrepency. This
prevents unnecessary settings being applied to unused channels but still
ensures the correct polarity to be set.

Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index a831bb2..101137d 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -95,6 +95,32 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	ndelay(400);
 }
 
+static void kona_pwmc_check_set_polarity(struct pwm_chip *chip,
+	struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = to_kona_pwmc(chip);
+	unsigned int chan = pwm->hwpwm;
+	enum pwm_polarity polarity = pwm->polarity;
+	unsigned int hw_pol;
+	unsigned int value = 0;
+
+	value = hw_pol = readl(kp->base + PWM_CONTROL_OFFSET);
+	hw_pol = (hw_pol >> PWM_CONTROL_POLARITY_SHIFT(chan)) & 0x1;
+
+	/*
+	 * If current polarity not the same as h/w then set polarity so that
+	 * they match.
+	 */
+	if (!hw_pol != polarity) {
+		if (polarity == PWM_POLARITY_NORMAL)
+			value |= 1 << PWM_CONTROL_POLARITY_SHIFT(chan);
+		else
+			value &= ~(1 << PWM_CONTROL_POLARITY_SHIFT(chan));
+
+		writel(value, kp->base + PWM_CONTROL_OFFSET);
+	}
+}
+
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
@@ -162,6 +188,13 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
 		chan, pc, dc, prescale);
 
+	/*
+	 * Ensure polarity is set properly. The default value for h/w and the
+	 * PWM framework are different. If a channel is enabled without setting
+	 * the polarity, the default value would be inconsistent to the signal.
+	 */
+	kona_pwmc_check_set_polarity(chip, pwm);
+
 	value = readl(kp->base + PWM_CONTROL_OFFSET);
 
 	/*
@@ -310,11 +343,8 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 	}
 
 	/* Set smooth mode, push/pull, and normal polarity for all channels */
-	for (chan = 0; chan < kp->chip.npwm; chan++) {
-		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	for (chan = 0; chan < kp->chip.npwm; chan++)
 		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
-		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
-	}
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-- 
1.7.9.5

Re: [PATCH v3 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Tim Kryger]

On Wed, Dec 17, 2014 at 10:46 AM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
>     The config procedure doesn't follow the spec which periodically results
>     in failing to enable the output signal. This happens one in ten or
>     twenty attempts. Following the spec and adding a 400ns delay in the
>     appropriate locations resolves this problem. It also ensures that the
>     signal transition is smooth.

This patch does not result in smooth transitions.

Please ensure that your commit message reflects the code.

>
>     If config is called when the pwm is disabled and there is nothing to do,
>     the while loop to calculate duty cycle and period doesn't need to be
>     done. The function now just returns if the pwm state is disabled.
>
>     The disable procedure now also follows the spec to ensure a smooth
>     transition. Not following the spec can cause non-smooth transitions.
>
>     The enable procedure now clears the enabled bit if enabling failed.
>     Enabling can fail if an invalid duty cycle and period is set. This
>     prevents the sysfs interface from reporting the pwm is enabled after a
>     failed call to enable.
>
> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Tested-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>

Normally when people send a multi-part patch series they include a
cover letter that describes the overall purpose of the changes with a
description of changes introduced in each revision of the series.

Please follow this convention.

> ---
>  drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 67 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 02bc048..a831bb2 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  {
>         unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> -       /* Clear trigger bit but set smooth bit to maintain old output */
> -       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> -       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> -       writel(value, kp->base + PWM_CONTROL_OFFSET);
> +       /*
> +        * There must be a min 400ns delay between clearing enable and setting
> +        * it. Failing to do this may result in no PWM signal.
> +        */
> +       ndelay(400);
>
>         /* Set trigger bit and clear smooth bit to apply new settings */
>         value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>         value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>         writel(value, kp->base + PWM_CONTROL_OFFSET);
> +
> +       /* PWMOUT_ENABLE must be held high for at least 400 ns. */
> +       ndelay(400);
>  }
>

Can you try to get a better understanding of what these timing
requirements actually are?

When I wrote this driver, the documentation simply stated that if the
trigger bit didn't stay high for 400 ns then the new settings would
not get applied.

It wasn't essential that we wait 400 ns here because the only time
when the trigger bit would be lowered is when we were about to load
even newer settings which meant we didn't care if the previous
settings were lost.

>  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -99,6 +103,9 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         unsigned long prescale = PRESCALE_MIN, pc, dc;
>         unsigned int value, chan = pwm->hwpwm;
>
> +       if (!test_bit(PWMF_ENABLED, &pwm->flags))
> +               return 0;
> +
>         /*
>          * Find period count, duty count and prescale to suit duty_ns and
>          * period_ns. This is done according to formulas described below:

Like I said last time, this is wrong.  You need to sanity check the
requested period and duty cycle here.

You can't just return success immediately when the channel isn't enabled.

> @@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                 dc = div64_u64(val, div);
>
>                 /* If duty_ns or period_ns are not achievable then return */
> -               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> +               if (pc < PERIOD_COUNT_MIN) {
> +                       dev_warn(chip->dev,
> +                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
> +                               __func__, chan, period_ns, pc, prescale);
>                         return -EINVAL;
> +               }
> +
> +               /* If duty_ns is not achievable then return */
> +               if (dc < DUTY_CYCLE_HIGH_MIN) {
> +                       if (0 != duty_ns) {
> +                               dev_warn(chip->dev,
> +                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
> +                                       __func__, chan, duty_ns, dc, prescale);
> +                       }
> +                       return -EINVAL;
> +               }
>
>                 /* If pc and dc are in bounds, the calculation is done */
>                 if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>                         break;
>
>                 /* Otherwise, increase prescale and recalculate pc and dc */
> -               if (++prescale > PRESCALE_MAX)
> +               if (++prescale > PRESCALE_MAX) {
> +                       dev_warn(chip->dev,
> +                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
> +                               __func__, chan, prescale, PRESCALE_MAX,
> +                               period_ns, duty_ns);
>                         return -EINVAL;
> +               }
>         }
>
> -       /* If the PWM channel is enabled, write the settings to the HW */
> -       if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -               value = readl(kp->base + PRESCALE_OFFSET);
> -               value &= ~PRESCALE_MASK(chan);
> -               value |= prescale << PRESCALE_SHIFT(chan);
> -               writel(value, kp->base + PRESCALE_OFFSET);
> +       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
> +               chan, pc, dc, prescale);
>
> -               writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +       value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> -               writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +       /*
> +        * Clear trigger bit but set smooth bit to maintain old
> +        * output.
> +        */
> +       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> -               kona_pwmc_apply_settings(kp, chan);
> -       }
> +       value = readl(kp->base + PRESCALE_OFFSET);
> +       value &= ~PRESCALE_MASK(chan);
> +       value |= prescale << PRESCALE_SHIFT(chan);
> +       writel(value, kp->base + PRESCALE_OFFSET);
> +
> +       writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +
> +       writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +
> +       kona_pwmc_apply_settings(kp, chan);
>
>         return 0;
>  }

There is no requirement to lower the trigger bit prior to writing the
period and duty registers.

This change is unnecessary.  The old sequence was fine.

> @@ -173,11 +209,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>
>         writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> -       kona_pwmc_apply_settings(kp, chan);
> -
> -       /* Wait for waveform to settle before gating off the clock */
> -       ndelay(400);
> -
>         clk_disable_unprepare(kp->clk);
>
>         return 0;

I'm not completely convinced that changing the polarity behavior is beneficial.

Please mention this change in your commit description and provide
appropriate justification.

> @@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>
>         ret = clk_prepare_enable(kp->clk);
>         if (ret < 0) {
> +               clear_bit(PWMF_ENABLED, &pwm->flags);
>                 dev_err(chip->dev, "failed to enable clock: %d\n", ret);
>                 return ret;
>         }
>
>         ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>         if (ret < 0) {
> +               clear_bit(PWMF_ENABLED, &pwm->flags);
>                 clk_disable_unprepare(kp->clk);
>                 return ret;
>         }

Don't try to fix this in the Kona PWM driver, fix it in the PWM core.

> @@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>         unsigned int chan = pwm->hwpwm;
> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +       /* Set smooth type to 1 and disable */
> +       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>
>         /* Simulate a disable by configuring for zero duty */
>         writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> -       kona_pwmc_apply_settings(kp, chan);
> +       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>
> -       /* Wait for waveform to settle before gating off the clock */
> -       ndelay(400);
> +       /* Set prescale to 0 for this channel */
> +       value = readl(kp->base + PRESCALE_OFFSET);
> +       value &= ~PRESCALE_MASK(chan);
> +       writel(value, kp->base + PRESCALE_OFFSET);

Can you explain why it is necessary to change the prescale here?

> +
> +       kona_pwmc_apply_settings(kp, chan);
>
>         clk_disable_unprepare(kp->clk);
>  }
> --
> 1.7.9.5
>

Re: [PATCH v3 1/2] pwm: kona: Fix incorrect enable, config, and disable procedures

[Jonathan Richardson]

On 14-12-20 02:38 PM, Tim Kryger wrote:
> On Wed, Dec 17, 2014 at 10:46 AM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>>     The config procedure doesn't follow the spec which periodically results
>>     in failing to enable the output signal. This happens one in ten or
>>     twenty attempts. Following the spec and adding a 400ns delay in the
>>     appropriate locations resolves this problem. It also ensures that the
>>     signal transition is smooth.
> 
> This patch does not result in smooth transitions.
> 
> Please ensure that your commit message reflects the code.

I'll remove the last sentence. I would have to re-verify that it's accurate.

> 
>>
>>     If config is called when the pwm is disabled and there is nothing to do,
>>     the while loop to calculate duty cycle and period doesn't need to be
>>     done. The function now just returns if the pwm state is disabled.
>>
>>     The disable procedure now also follows the spec to ensure a smooth
>>     transition. Not following the spec can cause non-smooth transitions.
>>
>>     The enable procedure now clears the enabled bit if enabling failed.
>>     Enabling can fail if an invalid duty cycle and period is set. This
>>     prevents the sysfs interface from reporting the pwm is enabled after a
>>     failed call to enable.
>>
>> Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Tested-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> 
> Normally when people send a multi-part patch series they include a
> cover letter that describes the overall purpose of the changes with a
> description of changes introduced in each revision of the series.
> 
> Please follow this convention.

Will do.

> 
>> ---
>>  drivers/pwm/pwm-bcm-kona.c |   91 ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 67 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index 02bc048..a831bb2 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>>  {
>>         unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>>
>> -       /* Clear trigger bit but set smooth bit to maintain old output */
>> -       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> -       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> -       writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +       /*
>> +        * There must be a min 400ns delay between clearing enable and setting
>> +        * it. Failing to do this may result in no PWM signal.
>> +        */
>> +       ndelay(400);
>>
>>         /* Set trigger bit and clear smooth bit to apply new settings */
>>         value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>>         value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>>         writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +
>> +       /* PWMOUT_ENABLE must be held high for at least 400 ns. */
>> +       ndelay(400);
>>  }
>>
> 
> Can you try to get a better understanding of what these timing
> requirements actually are?
> 
> When I wrote this driver, the documentation simply stated that if the
> trigger bit didn't stay high for 400 ns then the new settings would
> not get applied.
> 
> It wasn't essential that we wait 400 ns here because the only time
> when the trigger bit would be lowered is when we were about to load
> even newer settings which meant we didn't care if the previous
> settings were lost.

I'm not spending any more time trying to understand exactly why these
delays are required. All I know is that they are required. Failing to
follow the updated documentation from the ASIC engineers sometimes
resulted in no output signal as stated in the commit message. The docs
you had at the time may have been inaccurate or incomplete.

> 
>>  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> @@ -99,6 +103,9 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>         unsigned long prescale = PRESCALE_MIN, pc, dc;
>>         unsigned int value, chan = pwm->hwpwm;
>>
>> +       if (!test_bit(PWMF_ENABLED, &pwm->flags))
>> +               return 0;
>> +
>>         /*
>>          * Find period count, duty count and prescale to suit duty_ns and
>>          * period_ns. This is done according to formulas described below:
> 
> Like I said last time, this is wrong.  You need to sanity check the
> requested period and duty cycle here.

I don't agree. I actually like the code in its original form in this
case. The period and duty cycle are calculated as a function of the
prescale so the loop is responsible for figuring out what values are
correct or not as it iterates. Unless there is a more compelling reason
to change it, I'm going to leave it as is.

> 
> You can't just return success immediately when the channel isn't enabled.

I'll make it return an error.

> 
>> @@ -121,31 +128,60 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>                 dc = div64_u64(val, div);
>>
>>                 /* If duty_ns or period_ns are not achievable then return */
>> -               if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
>> +               if (pc < PERIOD_COUNT_MIN) {
>> +                       dev_warn(chip->dev,
>> +                               "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
>> +                               __func__, chan, period_ns, pc, prescale);
>>                         return -EINVAL;
>> +               }
>> +
>> +               /* If duty_ns is not achievable then return */
>> +               if (dc < DUTY_CYCLE_HIGH_MIN) {
>> +                       if (0 != duty_ns) {
>> +                               dev_warn(chip->dev,
>> +                                       "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
>> +                                       __func__, chan, duty_ns, dc, prescale);
>> +                       }
>> +                       return -EINVAL;
>> +               }
>>
>>                 /* If pc and dc are in bounds, the calculation is done */
>>                 if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>>                         break;
>>
>>                 /* Otherwise, increase prescale and recalculate pc and dc */
>> -               if (++prescale > PRESCALE_MAX)
>> +               if (++prescale > PRESCALE_MAX) {
>> +                       dev_warn(chip->dev,
>> +                               "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
>> +                               __func__, chan, prescale, PRESCALE_MAX,
>> +                               period_ns, duty_ns);
>>                         return -EINVAL;
>> +               }
>>         }
>>
>> -       /* If the PWM channel is enabled, write the settings to the HW */
>> -       if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>> -               value = readl(kp->base + PRESCALE_OFFSET);
>> -               value &= ~PRESCALE_MASK(chan);
>> -               value |= prescale << PRESCALE_SHIFT(chan);
>> -               writel(value, kp->base + PRESCALE_OFFSET);
>> +       dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
>> +               chan, pc, dc, prescale);
>>
>> -               writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
>> +       value = readl(kp->base + PWM_CONTROL_OFFSET);
>>
>> -               writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> +       /*
>> +        * Clear trigger bit but set smooth bit to maintain old
>> +        * output.
>> +        */
>> +       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>> -               kona_pwmc_apply_settings(kp, chan);
>> -       }
>> +       value = readl(kp->base + PRESCALE_OFFSET);
>> +       value &= ~PRESCALE_MASK(chan);
>> +       value |= prescale << PRESCALE_SHIFT(chan);
>> +       writel(value, kp->base + PRESCALE_OFFSET);
>> +
>> +       writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
>> +
>> +       writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> +
>> +       kona_pwmc_apply_settings(kp, chan);
>>
>>         return 0;
>>  }
> 
> There is no requirement to lower the trigger bit prior to writing the
> period and duty registers.
> 
> This change is unnecessary.  The old sequence was fine.

Incorrect. The old sequence was flawed as already discussed. I'll refer
you the updated spec in previous discussion that was already sent to
you. Not following the spec really does occasionally result in no output
signal as the commit message states.

> 
>> @@ -173,11 +209,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>>
>>         writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>> -       kona_pwmc_apply_settings(kp, chan);
>> -
>> -       /* Wait for waveform to settle before gating off the clock */
>> -       ndelay(400);
>> -
>>         clk_disable_unprepare(kp->clk);
>>
>>         return 0;
> 
> I'm not completely convinced that changing the polarity behavior is beneficial.
> 
> Please mention this change in your commit description and provide
> appropriate justification.

I'll update the commit message. You won't get any polarity change
without the added code. It doesn't work.

> 
>> @@ -190,12 +221,14 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>
>>         ret = clk_prepare_enable(kp->clk);
>>         if (ret < 0) {
>> +               clear_bit(PWMF_ENABLED, &pwm->flags);
>>                 dev_err(chip->dev, "failed to enable clock: %d\n", ret);
>>                 return ret;
>>         }
>>
>>         ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>>         if (ret < 0) {
>> +               clear_bit(PWMF_ENABLED, &pwm->flags);
>>                 clk_disable_unprepare(kp->clk);
>>                 return ret;
>>         }
> 
> Don't try to fix this in the Kona PWM driver, fix it in the PWM core.

I agree that's where the fix should go. I wasn't sure I could make the
change or not. I'll make the change and put it in a different commit.

> 
>> @@ -207,13 +240,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>  {
>>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>>         unsigned int chan = pwm->hwpwm;
>> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>> +
>> +       /* Set smooth type to 1 and disable */
>> +       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> +       value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>>         /* Simulate a disable by configuring for zero duty */
>>         writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> -       kona_pwmc_apply_settings(kp, chan);
>> +       writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>>
>> -       /* Wait for waveform to settle before gating off the clock */
>> -       ndelay(400);
>> +       /* Set prescale to 0 for this channel */
>> +       value = readl(kp->base + PRESCALE_OFFSET);
>> +       value &= ~PRESCALE_MASK(chan);
>> +       writel(value, kp->base + PRESCALE_OFFSET);
> 
> Can you explain why it is necessary to change the prescale here?

Spec recommendation and it's cleaner. We'd like the code to match the
spec now that we have more detailed documentation. There is nothing
dysfunctional about setting the prescale to the default value on disable
that I can see.

> 
>> +
>> +       kona_pwmc_apply_settings(kp, chan);
>>
>>         clk_disable_unprepare(kp->clk);
>>  }
>> --
>> 1.7.9.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH v4 0/3] Fix bugs in kona pwm driver and pwm core

[Jonathan Richardson]

This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also 
fixes a bug in the pwm core where the state was incorrect on failed calls to
enable.

Changes from v3:
- Removed setting the pwm set to disabled if enable fails. This is now
  done in the pwm core.
- Removed previous change in kona_pwmc_config() that returned right away if
  the state was disabled. The loop needs to execute to ensure that the period
  and duty cycle are valid. Delaying this check to when the pwm is enabled is 
  incorrect.
- Added comments to clarify code.
- Changed commit messages to more accurately reflect the code changes.

Jonathan Richardson (3):
  pwm: kona: Fix incorrect config, disable, and polarity procedures
  pwm: kona: Remove setting default smooth type and polarity for all
    channels
  pwm: core: Set enable state properly on failed call to enable

 drivers/pwm/core.c         |   10 +++-
 drivers/pwm/pwm-bcm-kona.c |  116 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 104 insertions(+), 22 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/3] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Jonathan Richardson]

The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.

The disable procedure now also follows the spec. The old disable
procedure would result in no change in signal when called.

The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   75 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..5ae4bf7 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	/* Clear trigger bit but set smooth bit to maintain old output */
-	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-	writel(value, kp->base + PWM_CONTROL_OFFSET);
+	/*
+	 * There must be a min 400ns delay between clearing enable and setting
+	 * it. Failing to do this may result in no PWM signal.
+	 */
+	ndelay(400);
 
 	/* Set trigger bit and clear smooth bit to apply new settings */
 	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* PWMOUT_ENABLE must be held high for at least 400 ns. */
+	ndelay(400);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -121,20 +125,56 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
+			return -EINVAL;
+		}
+
+		/* If duty_ns is not achievable then return */
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
 			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
+
+	/*
+	 * Don't apply settings if disabled. The period and duty cycle are
+	 * always calculated above to ensure the new values are
+	 * validated immediately instead of on enable.
+	 */
 	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+		/*
+		 * Clear trigger bit but set smooth bit to maintain old
+		 * output.
+		 */
+		value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+		value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+		writel(value, kp->base + PWM_CONTROL_OFFSET);
+
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
 		value |= prescale << PRESCALE_SHIFT(chan);
@@ -173,11 +213,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
-
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
-
 	clk_disable_unprepare(kp->clk);
 
 	return 0;
@@ -207,13 +242,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+	/* Set smooth type to 1 and disable */
+	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	kona_pwmc_apply_settings(kp, chan);
+	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
+	/* Set prescale to 0 for this channel */
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	clk_disable_unprepare(kp->clk);
 }
-- 
1.7.9.5

[PATCH v4 2/3] pwm: kona: Remove setting default smooth type and polarity for all channels

[Jonathan Richardson]

Setting the default polarity in probe to normal for all channels caused
the speaker pwm channel to click. The polarity does need to be set to
normal because the hw default is inversed whereas the pwm framework
defaults to normal. If a channel is enabled without setting the polarity
then the signal would be inversed while linux reports normal. A check
is now done prior to enabling the channel to ensure that the hw polarity
matches the desired polarity and is changed if there is a discrepency. This
prevents unnecessary settings being applied to unused channels but still
ensures the correct polarity to be set.

Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 5ae4bf7..34d434a 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -95,6 +95,32 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	ndelay(400);
 }
 
+static void kona_pwmc_check_set_polarity(struct pwm_chip *chip,
+	struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = to_kona_pwmc(chip);
+	unsigned int chan = pwm->hwpwm;
+	enum pwm_polarity polarity = pwm->polarity;
+	unsigned int hw_pol;
+	unsigned int value = 0;
+
+	value = hw_pol = readl(kp->base + PWM_CONTROL_OFFSET);
+	hw_pol = (hw_pol >> PWM_CONTROL_POLARITY_SHIFT(chan)) & 0x1;
+
+	/*
+	 * If current polarity not the same as h/w then set polarity so that
+	 * they match.
+	 */
+	if (!hw_pol != polarity) {
+		if (polarity == PWM_POLARITY_NORMAL)
+			value |= 1 << PWM_CONTROL_POLARITY_SHIFT(chan);
+		else
+			value &= ~(1 << PWM_CONTROL_POLARITY_SHIFT(chan));
+
+		writel(value, kp->base + PWM_CONTROL_OFFSET);
+	}
+}
+
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
@@ -165,6 +191,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * validated immediately instead of on enable.
 	 */
 	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		/*
+		 * Ensure polarity is set properly. The default value
+		 * for h/w and the PWM framework are different. If a
+		 * channel is enabled without setting the polarity, the
+		 * default value would be inconsistent to the signal.
+		 */
+		kona_pwmc_check_set_polarity(chip, pwm);
+
 		value = readl(kp->base + PWM_CONTROL_OFFSET);
 
 		/*
@@ -311,12 +345,9 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Set smooth mode, push/pull, and normal polarity for all channels */
-	for (chan = 0; chan < kp->chip.npwm; chan++) {
-		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	/* Set push/pull for all channels */
+	for (chan = 0; chan < kp->chip.npwm; chan++)
 		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
-		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
-	}
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-- 
1.7.9.5

[PATCH v4 3/3] pwm: core: Set enable state properly on failed call to enable

[Jonathan Richardson]

The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/core.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 966497d..641f6ec 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -457,8 +457,14 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip, pwm);
+	int err;
+
+	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
+		if (err)
+			clear_bit(PWMF_ENABLED, &pwm->flags);
+		return err;
+	}
 
 	return pwm ? 0 : -EINVAL;
 }
-- 
1.7.9.5

Re: [PATCH v4 2/3] pwm: kona: Remove setting default smooth type and polarity for all channels

[Tim Kryger]

On Tue, Dec 30, 2014 at 2:43 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> Setting the default polarity in probe to normal for all channels caused
> the speaker pwm channel to click. The polarity does need to be set to
> normal because the hw default is inversed whereas the pwm framework
> defaults to normal. If a channel is enabled without setting the polarity
> then the signal would be inversed while linux reports normal. A check
> is now done prior to enabling the channel to ensure that the hw polarity
> matches the desired polarity and is changed if there is a discrepency. This
> prevents unnecessary settings being applied to unused channels but still
> ensures the correct polarity to be set.

A more direct solution that avoids the potentially undesirable
consequences of delaying when polarity changes are written to hardware
would be to update the PWM framework to allow for the registration of
chips with inversed default polarity.

I will post a patch for your review.

Re: [PATCH v4 2/3] pwm: kona: Remove setting default smooth type and polarity for all channels

[Tim Kryger]

On Sun, Jan 4, 2015 at 5:12 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
> On Tue, Dec 30, 2014 at 2:43 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> Setting the default polarity in probe to normal for all channels caused
>> the speaker pwm channel to click. The polarity does need to be set to
>> normal because the hw default is inversed whereas the pwm framework
>> defaults to normal. If a channel is enabled without setting the polarity
>> then the signal would be inversed while linux reports normal. A check
>> is now done prior to enabling the channel to ensure that the hw polarity
>> matches the desired polarity and is changed if there is a discrepency. This
>> prevents unnecessary settings being applied to unused channels but still
>> ensures the correct polarity to be set.
>
> A more direct solution that avoids the potentially undesirable
> consequences of delaying when polarity changes are written to hardware
> would be to update the PWM framework to allow for the registration of
> chips with inversed default polarity.
>
> I will post a patch for your review.

Please see:  https://lkml.org/lkml/2015/1/4/199

[PATCH v5 0/2] Fix bugs in kona pwm driver and pwm core

[Jonathan Richardson]

This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also 
fixes a bug in the pwm core where the state was incorrect on failed calls to
enable.

Changes from v4:
  - Rebased to Tim Kryger's patch that adds support in pwm core to add driver 
    with inversed polarity.   
  - Removed patch 2 that resolved difference between hardware default polarity
    and pwm framework default polarity. The default polarity is set properly now
    when the pwm driver is registered with the pwm framework.

Jonathan Richardson (2):
  pwm: kona: Fix incorrect config, disable, and polarity procedures
  pwm: core: Set enable state properly on failed call to enable

 drivers/pwm/core.c         |   10 ++++--
 drivers/pwm/pwm-bcm-kona.c |   75 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH v5 1/2] pwm: kona: Fix incorrect config, disable, and polarity procedures

[Jonathan Richardson]

The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.

The disable procedure now also follows the spec. The old disable
procedure would result in no change in signal when called.

The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.

Reviewed-by: Arun Ramamurthy <arunrama@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/pwm-bcm-kona.c |   75 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..8a1dfba 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 {
 	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
 
-	/* Clear trigger bit but set smooth bit to maintain old output */
-	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
-	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
-	writel(value, kp->base + PWM_CONTROL_OFFSET);
+	/*
+	 * There must be a min 400ns delay between clearing enable and setting
+	 * it. Failing to do this may result in no PWM signal.
+	 */
+	ndelay(400);
 
 	/* Set trigger bit and clear smooth bit to apply new settings */
 	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
 	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* PWMOUT_ENABLE must be held high for at least 400 ns. */
+	ndelay(400);
 }
 
 static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -121,20 +125,56 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		dc = div64_u64(val, div);
 
 		/* If duty_ns or period_ns are not achievable then return */
-		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+		if (pc < PERIOD_COUNT_MIN) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n",
+				__func__, chan, period_ns, pc, prescale);
+			return -EINVAL;
+		}
+
+		/* If duty_ns is not achievable then return */
+		if (dc < DUTY_CYCLE_HIGH_MIN) {
+			if (0 != duty_ns) {
+				dev_warn(chip->dev,
+					"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n",
+					__func__, chan, duty_ns, dc, prescale);
+			}
 			return -EINVAL;
+		}
 
 		/* If pc and dc are in bounds, the calculation is done */
 		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
 			break;
 
 		/* Otherwise, increase prescale and recalculate pc and dc */
-		if (++prescale > PRESCALE_MAX)
+		if (++prescale > PRESCALE_MAX) {
+			dev_warn(chip->dev,
+				"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n",
+				__func__, chan, prescale, PRESCALE_MAX,
+				period_ns, duty_ns);
 			return -EINVAL;
+		}
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
+	dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+		chan, pc, dc, prescale);
+
+	/*
+	 * Don't apply settings if disabled. The period and duty cycle are
+	 * always calculated above to ensure the new values are
+	 * validated immediately instead of on enable.
+	 */
 	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+		/*
+		 * Clear trigger bit but set smooth bit to maintain old
+		 * output.
+		 */
+		value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+		value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+		writel(value, kp->base + PWM_CONTROL_OFFSET);
+
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
 		value |= prescale << PRESCALE_SHIFT(chan);
@@ -173,11 +213,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
-	kona_pwmc_apply_settings(kp, chan);
-
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
-
 	clk_disable_unprepare(kp->clk);
 
 	return 0;
@@ -207,13 +242,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	unsigned int chan = pwm->hwpwm;
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+	/* Set smooth type to 1 and disable */
+	value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
+	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
 
 	/* Simulate a disable by configuring for zero duty */
 	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
-	kona_pwmc_apply_settings(kp, chan);
+	writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
 
-	/* Wait for waveform to settle before gating off the clock */
-	ndelay(400);
+	/* Set prescale to 0 for this channel */
+	value = readl(kp->base + PRESCALE_OFFSET);
+	value &= ~PRESCALE_MASK(chan);
+	writel(value, kp->base + PRESCALE_OFFSET);
+
+	kona_pwmc_apply_settings(kp, chan);
 
 	clk_disable_unprepare(kp->clk);
 }
-- 
1.7.9.5

[PATCH v5 2/2] pwm: core: Set enable state properly on failed call to enable

[Jonathan Richardson]

The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.

Tested-by: Jonathan Richardson <jonathar@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/pwm/core.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f28c4ce..c33e24f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,8 +477,14 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip, pwm);
+	int err;
+
+	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
+		if (err)
+			clear_bit(PWMF_ENABLED, &pwm->flags);
+		return err;
+	}
 
 	return pwm ? 0 : -EINVAL;
 }
-- 
1.7.9.5

Re: [PATCH v5 2/2] pwm: core: Set enable state properly on failed call to enable

[Dmitry Torokhov]

On Wed, Jan 07, 2015 at 11:42:51AM -0800, Jonathan Richardson wrote:
> The pwm_enable function didn't clear the enabled bit if a call to a
> clients enable function returned an error. The result was that the state
> of the pwm core was wrong. Clearing the bit when enable returns an error
> ensures the state is properly set.
> 
> Tested-by: Jonathan Richardson <jonathar@broadcom.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/pwm/core.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index f28c4ce..c33e24f 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -477,8 +477,14 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>   */
>  int pwm_enable(struct pwm_device *pwm)
>  {
> -	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> -		return pwm->chip->ops->enable(pwm->chip, pwm);
> +	int err;
> +
> +	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> +		err = pwm->chip->ops->enable(pwm->chip, pwm);
> +		if (err)
> +			clear_bit(PWMF_ENABLED, &pwm->flags);
> +		return err;
> +	}
>  
>  	return pwm ? 0 : -EINVAL;

Seems fine in principle, but somewhat messy. Can we do the following:

int pwm_enable(struct pwm_device *pwm)
{
	int err;

	if (!pwm)
		return -EINVAL;

	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
		err = pwm->chip->ops->enable(pwm->chip, pwm);
		if (err) {
			clear_bit(PWMF_ENABLED, &pwm->flags);
			return err;
		}
	}

	return 0;
}

Otherwise:

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry