diff mbox

[v7,3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures

Message ID 1431473296-13683-4-git-send-email-jonathar@broadcom.com
State Superseded
Headers show

Commit Message

Jonathan Richardson May 12, 2015, 11:28 p.m. UTC 
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 |   47 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

Comments

Tim Kryger May 18, 2015, 4:53 a.m. UTC | #1 
On Tue, May 12, 2015 at 4:28 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:

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

Thanks for incorporating some of my suggestions in your latest version.

I'm still concerned about delaying when polarity changes take effect.

Since backlight is a common use of PWM, consider the following situation.

backlight {
        compatible = "pwm-backlight";
        pwms = <&pwm 0 5000000 PWM_POLARITY_NORMAL>;
        brightness-levels = <0 4 8 16 32 64 128 255>;
        default-brightness-level = <0>;
};

The Kona PWM hardware starts in inversed mode so it will drive output high
once its clock is enabled during the probe.

Polarity is not adjusted during probe so it stays high and it registers with
the PWM core using the new pwmchip_add_inversed() function.

Next, the pwm-backlight driver probe executes and it calls devm_pwm_get()
which then calls pwm_set_period() and most importantly pwm_set_polarity().

The output would change to constant low at this point in the original driver
but with your proposed change it will remain high.

The driver sets bl->props.brightness and calls backlight_update_status() but,
since in this case the default brightness is zero, it assumes it doesn't need
to enable the PWM.

The backlight driver probe then returns and the PWM output is incorrect.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Richardson May 21, 2015, 10:50 p.m. UTC | #2 
On 15-05-17 09:53 PM, Tim Kryger wrote:
> On Tue, May 12, 2015 at 4:28 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
> 
>> 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.
> 
> Thanks for incorporating some of my suggestions in your latest version.
> 
> I'm still concerned about delaying when polarity changes take effect.
> 
> Since backlight is a common use of PWM, consider the following situation.
> 
> backlight {
>         compatible = "pwm-backlight";
>         pwms = <&pwm 0 5000000 PWM_POLARITY_NORMAL>;
>         brightness-levels = <0 4 8 16 32 64 128 255>;
>         default-brightness-level = <0>;
> };
> 
> The Kona PWM hardware starts in inversed mode so it will drive output high
> once its clock is enabled during the probe.
> 
> Polarity is not adjusted during probe so it stays high and it registers with
> the PWM core using the new pwmchip_add_inversed() function.
> 
> Next, the pwm-backlight driver probe executes and it calls devm_pwm_get()
> which then calls pwm_set_period() and most importantly pwm_set_polarity().
> 
> The output would change to constant low at this point in the original driver
> but with your proposed change it will remain high.
> 
> The driver sets bl->props.brightness and calls backlight_update_status() but,
> since in this case the default brightness is zero, it assumes it doesn't need
> to enable the PWM.
> 
> The backlight driver probe then returns and the PWM output is incorrect.

Thanks for the detailed use case. You are right - the problem does
occur. I assumed if the pwm signal was being changed at all that it
should first be enabled. Since the bl driver can't know what 0
brightness means with different polarities, shouldn't it always enable
the pwm first, config 0 period, then disable (when brightness is 0)?
Then the polarity would have been updated properly also. 0 can mean full
brightness or off depending on polarity.

It seems odd that changing the polarity should affect the output signal
when the pwm is disabled. If using sysfs and you change the polarity,
you can't defer the signal change to when it's enabled.

If this is correct - polarity changes affect the output signal
immediately, then I can change our driver. Could you confirm first this
is what we want? This would cause polarity changes to affect all devices
immediately which is what I thought enable was for. The pwm core wanting
the pwm disabled to change the polarity implied to me that it shouldn't
cause a change in the signal until it was enabled.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tim Kryger May 25, 2015, 5:33 p.m. UTC | #3 
On Thu, May 21, 2015 at 3:50 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:

> If this is correct - polarity changes affect the output signal
> immediately, then I can change our driver. Could you confirm first this
> is what we want?

Yes.  This seems best.  Please do.

-Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..0654981 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@  static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
 	return container_of(_chip, struct kona_pwmc, chip);
 }
 
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_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);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
 	/* 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,
@@ -133,8 +150,14 @@  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			return -EINVAL;
 	}
 
-	/* If the PWM channel is enabled, write the settings to the HW */
+	/*
+	 * 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)) {
+		kona_pwmc_prepare_for_settings(kp, chan);
+
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
 		value |= prescale << PRESCALE_SHIFT(chan);
@@ -173,11 +196,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 +225,20 @@  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;
+
+	kona_pwmc_prepare_for_settings(kp, chan);
 
 	/* 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);
 }