[PATCH 0/4] pwm: tiehrpwm: various fixes

[PATCH 0/4] pwm: tiehrpwm: various fixes

[Uwe Kleine-König]

Hello,

I setup the PWM device on a beaglebone black with the intention to
convert this driver to the waveform API. Before getting to that though,
I found various issues that are addressed here. The problems fixed are
all quite old, they all exist since 2012, so I think there is no urge to
get them in and I tend to let them stay in next for a while.

Best regards
Uwe

Uwe Kleine-König (4):
  pwm: tiehrpwm: Don't drop runtime PM reference in .free()
  pwm: tiehrpwm: Make code comment in .free() more useful
  pwm: tiehrpwm: Fix various off-by-one errors in duty-cycle calculation
  pwm: tiehrpwm: Fix corner case in clock divisor calculation

 drivers/pwm/pwm-tiehrpwm.c | 154 +++++++++++++++----------------------
 1 file changed, 61 insertions(+), 93 deletions(-)

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.50.1

[PATCH 1/4] pwm: tiehrpwm: Don't drop runtime PM reference in .free()

[Uwe Kleine-König]

The pwm driver calls pm_runtime_get_sync() when the hardware becomes
enabled and pm_runtime_put_sync() when it becomes disabled. The PWM's
state is kept when a consumer goes away, so the call to
pm_runtime_put_sync() in the .free() callback is unbalanced resulting in
a non-functional device and a reference underlow for the second consumer.

The easiest fix for that issue is to just not drop the runtime PM
reference in .free(), so do that.

Fixes: 19891b20e7c2 ("pwm: pwm-tiehrpwm: PWM driver support for EHRPWM")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tiehrpwm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..5e674a7bbf3b 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -391,11 +391,6 @@ static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
 
-	if (pwm_is_enabled(pwm)) {
-		dev_warn(pwmchip_parent(chip), "Removing PWM device without disabling\n");
-		pm_runtime_put_sync(pwmchip_parent(chip));
-	}
-
 	/* set period value to zero on free */
 	pc->period_cycles[pwm->hwpwm] = 0;
 }
-- 
2.50.1

[PATCH 2/4] pwm: tiehrpwm: Make code comment in .free() more useful

[Uwe Kleine-König]

Instead of explaining trivia to everyone who can read C describe the
higher-level effect of setting pc->period_cycles[pwm->hwpwm] to zero.

Fixes: 01b2d4536f02 ("pwm: pwm-tiehrpwm: Fix conflicting channel period setting")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tiehrpwm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 5e674a7bbf3b..a94b1e387b92 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -391,7 +391,7 @@ static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
 
-	/* set period value to zero on free */
+	/* Don't let a pwm without consumer block requests to the other channel */
 	pc->period_cycles[pwm->hwpwm] = 0;
 }
 
-- 
2.50.1

[PATCH 3/4] pwm: tiehrpwm: Fix various off-by-one errors in duty-cycle calculation

[Uwe Kleine-König]

In Up-Count Mode the timer is reset to zero one tick after it reaches
TBPRD, so the period length is (TBPRD + 1) * T_TBCLK. This matches both
the documentation and measurements. So the value written to the TBPRD has
to be one less than the calculated period_cycles value.

A complication here is that for a 100% relative duty-cycle the value
written to the CMPx register has to be TBPRD + 1 which might overflow if
TBPRD is 0xffff. To handle that the calculation of the AQCTLx register
has to be moved to ehrpwm_pwm_config() and the edge at CTR = CMPx has to
be skipped.

Additionally the AQCTL_PRD register field has to be 0 because that defines
the hardware's action when the maximal counter value is reached, which is
(as above) one clock tick before the period's end. The period start edge
has to happen when the counter is reset and so is defined in the AQCTL_ZRO
field.

Fixes: 19891b20e7c2 ("pwm: pwm-tiehrpwm: PWM driver support for EHRPWM")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tiehrpwm.c | 143 +++++++++++++++----------------------
 1 file changed, 58 insertions(+), 85 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index a94b1e387b92..a23e48b8523d 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -36,7 +36,7 @@
 
 #define CLKDIV_MAX		7
 #define HSPCLKDIV_MAX		7
-#define PERIOD_MAX		0xFFFF
+#define PERIOD_MAX		0x10000
 
 /* compare module registers */
 #define CMPA			0x12
@@ -65,14 +65,10 @@
 #define AQCTL_ZRO_FRCHIGH	BIT(1)
 #define AQCTL_ZRO_FRCTOGGLE	(BIT(1) | BIT(0))
 
-#define AQCTL_CHANA_POLNORMAL	(AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
-				AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANA_POLINVERSED	(AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
-				AQCTL_ZRO_FRCLOW)
-#define AQCTL_CHANB_POLNORMAL	(AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
-				AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANB_POLINVERSED	(AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
-				AQCTL_ZRO_FRCLOW)
+#define AQCTL_CHANA_POLNORMAL	(AQCTL_CAU_FRCLOW | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANA_POLINVERSED	(AQCTL_CAU_FRCHIGH | AQCTL_ZRO_FRCLOW)
+#define AQCTL_CHANB_POLNORMAL	(AQCTL_CBU_FRCLOW | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANB_POLINVERSED	(AQCTL_CBU_FRCHIGH | AQCTL_ZRO_FRCLOW)
 
 #define AQSFRC_RLDCSF_MASK	(BIT(7) | BIT(6))
 #define AQSFRC_RLDCSF_ZRO	0
@@ -108,7 +104,6 @@ struct ehrpwm_pwm_chip {
 	unsigned long clk_rate;
 	void __iomem *mmio_base;
 	unsigned long period_cycles[NUM_PWM_CHANNEL];
-	enum pwm_polarity polarity[NUM_PWM_CHANNEL];
 	struct clk *tbclk;
 	struct ehrpwm_context ctx;
 };
@@ -177,51 +172,20 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
 	return 1;
 }
 
-static void configure_polarity(struct ehrpwm_pwm_chip *pc, int chan)
-{
-	u16 aqctl_val, aqctl_mask;
-	unsigned int aqctl_reg;
-
-	/*
-	 * Configure PWM output to HIGH/LOW level on counter
-	 * reaches compare register value and LOW/HIGH level
-	 * on counter value reaches period register value and
-	 * zero value on counter
-	 */
-	if (chan == 1) {
-		aqctl_reg = AQCTLB;
-		aqctl_mask = AQCTL_CBU_MASK;
-
-		if (pc->polarity[chan] == PWM_POLARITY_INVERSED)
-			aqctl_val = AQCTL_CHANB_POLINVERSED;
-		else
-			aqctl_val = AQCTL_CHANB_POLNORMAL;
-	} else {
-		aqctl_reg = AQCTLA;
-		aqctl_mask = AQCTL_CAU_MASK;
-
-		if (pc->polarity[chan] == PWM_POLARITY_INVERSED)
-			aqctl_val = AQCTL_CHANA_POLINVERSED;
-		else
-			aqctl_val = AQCTL_CHANA_POLNORMAL;
-	}
-
-	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
-	ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
-}
-
 /*
  * period_ns = 10^9 * (ps_divval * period_cycles) / PWM_CLK_RATE
  * duty_ns   = 10^9 * (ps_divval * duty_cycles) / PWM_CLK_RATE
  */
 static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			     u64 duty_ns, u64 period_ns)
+			     u64 duty_ns, u64 period_ns, enum pwm_polarity polarity)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
 	u32 period_cycles, duty_cycles;
 	u16 ps_divval, tb_divval;
 	unsigned int i, cmp_reg;
 	unsigned long long c;
+	u16 aqctl_val, aqctl_mask;
+	unsigned int aqctl_reg;
 
 	if (period_ns > NSEC_PER_SEC)
 		return -ERANGE;
@@ -231,15 +195,10 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(c, NSEC_PER_SEC);
 	period_cycles = (unsigned long)c;
 
-	if (period_cycles < 1) {
-		period_cycles = 1;
-		duty_cycles = 1;
-	} else {
-		c = pc->clk_rate;
-		c = c * duty_ns;
-		do_div(c, NSEC_PER_SEC);
-		duty_cycles = (unsigned long)c;
-	}
+	c = pc->clk_rate;
+	c = c * duty_ns;
+	do_div(c, NSEC_PER_SEC);
+	duty_cycles = (unsigned long)c;
 
 	/*
 	 * Period values should be same for multiple PWM channels as IP uses
@@ -271,50 +230,71 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 
+	/* Update period & duty cycle with presacler division */
+	period_cycles = period_cycles / ps_divval;
+	duty_cycles = duty_cycles / ps_divval;
+
+	if (period_cycles < 1)
+		period_cycles = 1;
+
 	pm_runtime_get_sync(pwmchip_parent(chip));
 
 	/* Update clock prescaler values */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CLKDIV_MASK, tb_divval);
 
-	/* Update period & duty cycle with presacler division */
-	period_cycles = period_cycles / ps_divval;
-	duty_cycles = duty_cycles / ps_divval;
+	if (pwm->hwpwm == 1) {
+		/* Channel 1 configured with compare B register */
+		cmp_reg = CMPB;
+
+		aqctl_reg = AQCTLB;
+		aqctl_mask = AQCTL_CBU_MASK;
+
+		if (polarity == PWM_POLARITY_INVERSED)
+			aqctl_val = AQCTL_CHANB_POLINVERSED;
+		else
+			aqctl_val = AQCTL_CHANB_POLNORMAL;
+
+		/* if duty_cycle is big, don't toggle on CBU */
+		if (duty_cycles > period_cycles)
+			aqctl_val &= ~AQCTL_CBU_MASK;
+
+	} else {
+		/* Channel 0 configured with compare A register */
+		cmp_reg = CMPA;
+
+		aqctl_reg = AQCTLA;
+		aqctl_mask = AQCTL_CAU_MASK;
+
+		if (polarity == PWM_POLARITY_INVERSED)
+			aqctl_val = AQCTL_CHANA_POLINVERSED;
+		else
+			aqctl_val = AQCTL_CHANA_POLNORMAL;
+
+		/* if duty_cycle is big, don't toggle on CAU */
+		if (duty_cycles > period_cycles)
+			aqctl_val &= ~AQCTL_CAU_MASK;
+	}
+
+	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
+	ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
 
 	/* Configure shadow loading on Period register */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_PRDLD_MASK, TBCTL_PRDLD_SHDW);
 
-	ehrpwm_write(pc->mmio_base, TBPRD, period_cycles);
+	ehrpwm_write(pc->mmio_base, TBPRD, period_cycles - 1);
 
 	/* Configure ehrpwm counter for up-count mode */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
 		      TBCTL_CTRMODE_UP);
 
-	if (pwm->hwpwm == 1)
-		/* Channel 1 configured with compare B register */
-		cmp_reg = CMPB;
-	else
-		/* Channel 0 configured with compare A register */
-		cmp_reg = CMPA;
-
-	ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles);
+	if (!(duty_cycles > period_cycles))
+		ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles);
 
 	pm_runtime_put_sync(pwmchip_parent(chip));
 
 	return 0;
 }
 
-static int ehrpwm_pwm_set_polarity(struct pwm_chip *chip,
-				   struct pwm_device *pwm,
-				   enum pwm_polarity polarity)
-{
-	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
-
-	/* Configuration of polarity in hardware delayed, do at enable */
-	pc->polarity[pwm->hwpwm] = polarity;
-
-	return 0;
-}
-
 static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -339,9 +319,6 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
 
-	/* Channels polarity can be configured from action qualifier module */
-	configure_polarity(pc, pwm->hwpwm);
-
 	/* Enable TBCLK */
 	ret = clk_enable(pc->tbclk);
 	if (ret) {
@@ -406,10 +383,6 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			ehrpwm_pwm_disable(chip, pwm);
 			enabled = false;
 		}
-
-		err = ehrpwm_pwm_set_polarity(chip, pwm, state->polarity);
-		if (err)
-			return err;
 	}
 
 	if (!state->enabled) {
@@ -418,7 +391,7 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	err = ehrpwm_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	err = ehrpwm_pwm_config(chip, pwm, state->duty_cycle, state->period, state->polarity);
 	if (err)
 		return err;
 
-- 
2.50.1

[PATCH 4/4] pwm: tiehrpwm: Fix corner case in clock divisor calculation

[Uwe Kleine-König]

The function set_prescale_div() is responsible for calculating the clock
divisor settings such that the input clock rate is divided down such that
the required period length is at most 0x10000 clock ticks. If period_cycles
is an integer multiple of 0x10000, the divisor period_cycles / 0x10000 is
good enough. So round up in the calculation of the required divisor and
compare it using >= instead of >.

Fixes: 19891b20e7c2 ("pwm: pwm-tiehrpwm: PWM driver support for EHRPWM")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tiehrpwm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index a23e48b8523d..7a86cb090f76 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -161,7 +161,7 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
 
 			*prescale_div = (1 << clkdiv) *
 					(hspclkdiv ? (hspclkdiv * 2) : 1);
-			if (*prescale_div > rqst_prescaler) {
+			if (*prescale_div >= rqst_prescaler) {
 				*tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
 					(hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
 				return 0;
@@ -224,7 +224,7 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	pc->period_cycles[pwm->hwpwm] = period_cycles;
 
 	/* Configure clock prescaler to support Low frequency PWM wave */
-	if (set_prescale_div(period_cycles/PERIOD_MAX, &ps_divval,
+	if (set_prescale_div(DIV_ROUND_UP(period_cycles, PERIOD_MAX), &ps_divval,
 			     &tb_divval)) {
 		dev_err(pwmchip_parent(chip), "Unsupported values\n");
 		return -EINVAL;
-- 
2.50.1