Skip to contentSkip to conversation

Fix bugs in kona pwm driver and pwm core

PATCH Superseded Jonathan Richardson 2 participants 9 messages May 12, 2015 – May 25, 2015

Subsystems (2)

v7 of 10 Superseded Latest is v10 →

lore
Share & tools
lore
#0 lore raw

v7 cover Fix bugs in kona pwm driver and pwm core

This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported.

Debug info has been added to help debug configuring duty cycle and period.

These changes are required for the Kona pwm driver to work on Cygnus and the same PWM IP is being used.

Changes from v6:
- Move new debugging info for duty cycle and period in config function into its own commit.
- Add kona_pwmc_prepare_for_settings() function to remove duplicated code.

Changes from v5:
- Address Dmitry's comment on code cleanup of pwm_enable() in pwm core.
- Including all patches from different contributors in this patchset. Some were left out in v4.

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.

Arun Ramamurthy (1):
drivers: pwm: bcm-kona: Dont set polarity in probe

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

Tim Kryger (1):
drivers: pwm: core: Add pwmchip_add_inversed

 drivers/pwm/core.c         |   52 ++++++++++++++++++++++------
 drivers/pwm/pwm-bcm-kona.c |   81 +++++++++++++++++++++++++++++++++-----------
 include/linux/pwm.h        |    6 ++++
 3 files changed, 109 insertions(+), 30 deletions(-)
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH v7 1/5 drivers: pwm: core: Add pwmchip_add_inversed

From: Tim Kryger <[email protected]>

Add a new function to register a PWM chip with channels that have their initial polarity as inversed. This benefits drivers of controllers that by default operate with inversed polarity by removing the need to modify the polarity during initialization.

---
 drivers/pwm/core.c  |   36 ++++++++++++++++++++++++++++--------
 include/linux/pwm.h |    6 ++++++
 2 files changed, 34 insertions(+), 8 deletions(-)
drivers/pwm/core.c +28 -8
index ba34c7d..224645f 100644
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_get_chip_data);
 
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+				     enum pwm_polarity polarity)
 {
 	struct pwm_device *pwm;
 	unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
+		pwm->polarity = polarity;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -279,9 +274,34 @@ out:
 	mutex_unlock(&pwm_lock);
 	return ret;
 }
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used.  The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
  * pwmchip_remove() - remove a PWM chip
  * @chip: the PWM chip to remove
  *
include/linux/pwm.h +6 -0
index e90628c..358547f 100644
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
 void *pwm_get_chip_data(struct pwm_device *pwm);
 
 int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
 	return -EINVAL;
 }
 
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+	return -EINVAL;
+}
+
 static inline int pwmchip_remove(struct pwm_chip *chip)
 {
 	return -EINVAL;
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH v7 2/5 drivers: pwm: bcm-kona: Dont set polarity in probe

In reply to Jonathan Richardson

From: Arun Ramamurthy <[email protected]>

Omit setting the polarity to normal during probe and instead use the new pwmchip_add_inversed function to register a PWM chip with default polarity of inversed for all channels as this is the actual hardware default.

---
 drivers/pwm/pwm-bcm-kona.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/pwm/pwm-bcm-kona.c +3 -6
index 02bc048..32b3ec6 100644
@@ -266,18 +266,15 @@ 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);
 
 	clk_disable_unprepare(kp->clk);
 
-	ret = pwmchip_add(&kp->chip);
+	ret = pwmchip_add_inversed(&kp->chip);
 	if (ret < 0)
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
 
Signature · 1 line
1.7.9.5
#3 lore raw

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

In reply to 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.

---
 drivers/pwm/pwm-bcm-kona.c |   47 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)
drivers/pwm/pwm-bcm-kona.c +36 -11
index 32b3ec6..0654981 100644
@@ -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);
 }
Signature · 1 line
1.7.9.5
#4 lore raw

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

Quoted 7 lines · Jonathan Richardson

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.

#5 lore raw

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

Quoted 33 lines · Tim Kryger
Quoted 7 lines · Jonathan Richardson

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

Quoted 8 lines

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.

#6 lore raw

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

Quoted 3 lines · Jonathan Richardson

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

#7 lore raw

PATCH v7 4/5 pwm: kona: Add debug info to config function

In reply to Jonathan Richardson

Adds debugging info to config function where duty cycle and period are calculated and verified.

---
 drivers/pwm/pwm-bcm-kona.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/pwm/pwm-bcm-kona.c +23 -2
index 0654981..c6069c7 100644
@@ -138,18 +138,39 @@ 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 (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;
+		}
 	}
 
+	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
Signature · 1 line
1.7.9.5
#8 lore raw

PATCH v7 5/5 pwm: core: Set enable state properly on failed call to enable

In reply to 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.

---
 drivers/pwm/core.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/pwm/core.c +13 -3
index 224645f..18f5ac4 100644
@@ -477,10 +477,20 @@ 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)
+		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 pwm ? 0 : -EINVAL;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 
Signature · 1 line
1.7.9.5