[PATCH v3 0/8] pwm: New abstraction and userspace API

[PATCH v3 0/8] pwm: New abstraction and userspace API

[Uwe Kleine-König]

Hello,

here comes v3 of the series to add support for duty offset in PWM
waveforms. For a single PWM output there is no gain, but if you have a
chip with two (or more) outputs and both operate with the same period,
you can generate an output like:


               ______         ______         ______         ______
   PWM #0  ___/      \_______/      \_______/      \_______/      \_______
                 __             __             __             __    
   PWM #1  _____/  \___________/  \___________/  \___________/  \_________
              ^              ^              ^              ^

Changes since v2, which is available from
https://lore.kernel.org/linux-pwm/cover.1721040875.git.u.kleine-koenig@baylibre.com
include:

 - Degrade a dev_alert() to dev_warn() on feedback by David Lechner

 - Improvement of various comments (partly in reply to David Lechner)

 - Add _ns suffixes for members of pwm_waveform, thanks David for that suggestion.

 - Rebased to v6.11-rc1 + pwm/for-next.

Because of these changes I didn't add Trevor's Reviewed-by tag for patch
#3.

I kept the BUG_ONs. There are a few check_patch warnings about it, but I still
prefer these given that it is safe they don't trigger without further (bogus)
code changes and when they trigger crashing early is better than stack
corruption. Also checkpatch tells
        WARNING: Comparisons should place the constant on the right side of the test
        #158: FILE: drivers/pwm/core.c:262:
        +       BUG_ON(WFHWSIZE < ops->sizeof_wfhw);

But as the BUG_ON is about WFHWSIZE being wrong, this order is OK.

There are a few more checkpatch warnings about line lengths. Breaking
the criticised lines further hurts readability IMHO, so I kept them. It
gets a bit better once stm32_pwm_mul_u64_u64_div_u64_roundup() is
implemented (without the stm32_pwm prefix) alongside
mul_u64_u64_div_u64() in lib/math/div64.c, but I don't want to wait for
that. I will address that once Nicolas's patch improving precision of
mul_u64_u64_div_u64() landed. (Hmm, it's not in next any more since
next-20240724, before it was 3cc8bf1a81efde105d8e57398cf8554b57768777 +
dbbe95af0fad2a9d22a4b910cfc4b87949d61a3c).

Best regards
Uwe

Uwe Kleine-König (8):
  pwm: Simplify pwm_capture()
  pwm: Add more locking
  pwm: New abstraction for PWM waveforms
  pwm: Provide new consumer API functions for waveforms
  pwm: Add support for pwmchip devices for faster and easier userspace
    access
  pwm: Add tracing for waveform callbacks
  pwm: axi-pwmgen: Implementation of the waveform callbacks
  pwm: stm32: Implementation of the waveform callbacks

 drivers/pwm/core.c           | 809 +++++++++++++++++++++++++++++++++--
 drivers/pwm/pwm-axi-pwmgen.c | 154 +++++--
 drivers/pwm/pwm-stm32.c      | 607 ++++++++++++++++----------
 include/linux/pwm.h          |  58 ++-
 include/trace/events/pwm.h   | 134 +++++-
 include/uapi/linux/pwm.h     |  25 ++
 6 files changed, 1479 insertions(+), 308 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

base-commit: b9b6bd3dcceed371829a022caeb6b51cb9f67be9
-- 
2.43.0

[PATCH v3 8/8] pwm: stm32: Implementation of the waveform callbacks

[Uwe Kleine-König]

Convert the stm32 pwm driver to use the new callbacks for hardware
programming.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-stm32.c | 607 +++++++++++++++++++++++++---------------
 1 file changed, 386 insertions(+), 221 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index fd754a99cf2e..846da265bbfe 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -51,6 +51,386 @@ static u32 active_channels(struct stm32_pwm *dev)
 	return ccer & TIM_CCER_CCXE;
 }
 
+struct stm32_pwm_waveform {
+	u32 ccer;
+	u32 psc;
+	u32 arr;
+	u32 ccr;
+};
+
+static int stm32_pwm_round_waveform_tohw(struct pwm_chip *chip,
+					 struct pwm_device *pwm,
+					 const struct pwm_waveform *wf,
+					 void *_wfhw)
+{
+	struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	unsigned long rate;
+	u64 ccr, duty;
+	int ret;
+
+	if (wf->period_length_ns == 0) {
+		*wfhw = (struct stm32_pwm_waveform){
+			.ccer = 0,
+		};
+
+		return 0;
+	}
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	wfhw->ccer = TIM_CCER_CCxE(ch + 1);
+	if (priv->have_complementary_output)
+		wfhw->ccer = TIM_CCER_CCxNE(ch);
+
+	rate = clk_get_rate(priv->clk);
+
+	if (active_channels(priv) & ~(1 << ch * 4)) {
+		u64 arr;
+
+		/*
+		 * Other channels are already enabled, so the configured PSC and
+		 * ARR must be used for this channel, too.
+		 */
+		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
+		if (ret)
+			goto out;
+
+		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
+		if (ret)
+			goto out;
+
+		/*
+		 * calculate the best value for ARR for the given PSC, refuse if
+		 * the resulting period gets bigger than the requested one.
+		 */
+		arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
+					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+		if (arr <= wfhw->arr) {
+			/*
+			 * requested period is small than the currently
+			 * configured and unchangable period, report back the smallest
+			 * possible period, i.e. the current state; Initialize
+			 * ccr to anything valid.
+			 */
+			wfhw->ccr = 0;
+			ret = 1;
+			goto out;
+		}
+
+	} else {
+		/*
+		 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
+		 * the calculations here won't overflow.
+		 * First we need to find the minimal value for prescaler such that
+		 *
+		 *        period_ns * clkrate
+		 *   ------------------------------ < max_arr + 1
+		 *   NSEC_PER_SEC * (prescaler + 1)
+		 *
+		 * This equation is equivalent to
+		 *
+		 *        period_ns * clkrate
+		 *   ---------------------------- < prescaler + 1
+		 *   NSEC_PER_SEC * (max_arr + 1)
+		 *
+		 * Using integer division and knowing that the right hand side is
+		 * integer, this is further equivalent to
+		 *
+		 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
+		 */
+		u64 psc = mul_u64_u64_div_u64(wf->period_length_ns, rate,
+					      (u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
+		u64 arr;
+
+		wfhw->psc = min_t(u64, psc, MAX_TIM_PSC);
+
+		arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
+					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+		if (!arr) {
+			/*
+			 * requested period is too small, report back the smallest
+			 * possible period, i.e. ARR = 0. The only valid CCR
+			 * value is then zero, too.
+			 */
+			wfhw->arr = 0;
+			wfhw->ccr = 0;
+			ret = 1;
+			goto out;
+		}
+
+		/*
+		 * ARR is limited intentionally to values less than
+		 * priv->max_arr to allow 100% duty cycle.
+		 */
+		wfhw->arr = min_t(u64, arr, priv->max_arr) - 1;
+	}
+
+	duty = mul_u64_u64_div_u64(wf->duty_length_ns, rate,
+				   (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+	duty = min_t(u64, duty, wfhw->arr + 1);
+
+	if (wf->duty_length_ns && wf->duty_offset_ns &&
+	    wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns) {
+		wfhw->ccer |= TIM_CCER_CCxP(ch + 1);
+		if (priv->have_complementary_output)
+			wfhw->ccer |= TIM_CCER_CCxNP(ch + 1);
+
+		ccr = wfhw->arr + 1 - duty;
+	} else {
+		ccr = duty;
+	}
+
+	wfhw->ccr = min_t(u64, ccr, wfhw->arr + 1);
+
+	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x\n",
+		pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
+		rate, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr);
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
+/*
+ * This should be moved to lib/math/div64.c. Currently there are some changes
+ * pending to mul_u64_u64_div_u64. Uwe will care for that when the dust settles.
+ */
+static u64 stm32_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
+{
+	u64 res = mul_u64_u64_div_u64(a, b, c);
+	/* Those multiplications might overflow but it doesn't matter */
+	u64 rem = a * b - c * res;
+
+	if (rem)
+		res += 1;
+
+	return res;
+}
+
+static int stm32_pwm_round_waveform_fromhw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const void *_wfhw,
+					   struct pwm_waveform *wf)
+{
+	const struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		unsigned long rate = clk_get_rate(priv->clk);
+		u64 ccr_ns;
+
+		/* The result doesn't overflow for rate >= 15259 */
+		wf->period_length_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1),
+									     NSEC_PER_SEC, rate);
+
+		ccr_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * wfhw->ccr,
+							       NSEC_PER_SEC, rate);
+
+		if (wfhw->ccer & TIM_CCER_CCxP(ch + 1)) {
+			wf->duty_length_ns =
+				stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
+								      NSEC_PER_SEC, rate);
+
+			wf->duty_offset_ns = ccr_ns;
+		} else {
+			wf->duty_length_ns = ccr_ns;
+			wf->duty_offset_ns = 0;
+		}
+	} else {
+		*wf = (struct pwm_waveform){
+			.period_length_ns = 0,
+		};
+	}
+
+	return 0;
+}
+
+static int stm32_pwm_read_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     void *_wfhw)
+{
+	struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(priv->regmap, TIM_CCER, &wfhw->ccer);
+	if (ret)
+		goto out;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
+		if (ret)
+			goto out;
+
+		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
+		if (ret)
+			goto out;
+
+		if (wfhw->arr == U32_MAX)
+			wfhw->arr -= 1;
+
+		ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &wfhw->ccr);
+		if (ret)
+			goto out;
+
+		if (wfhw->ccr > wfhw->arr + 1)
+			wfhw->ccr = wfhw->arr + 1;
+	}
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
+static int stm32_pwm_write_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      const void *_wfhw)
+{
+	const struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		u32 ccer, mask;
+		unsigned int shift;
+		u32 ccmr;
+
+		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+		if (ret)
+			goto out;
+
+		/* If there are other channels enabled, don't update PSC and ARR */
+		if (ccer & ~TIM_CCER_CCxE(ch + 1) & TIM_CCER_CCXE) {
+			u32 psc, arr;
+
+			ret = regmap_read(priv->regmap, TIM_PSC, &psc);
+			if (ret)
+				goto out;
+
+			if (psc != wfhw->psc) {
+				ret = -EBUSY;
+				goto out;
+			}
+
+			regmap_read(priv->regmap, TIM_ARR, &arr);
+			if (ret)
+				goto out;
+
+			if (arr != wfhw->arr) {
+				ret = -EBUSY;
+				goto out;
+			}
+		} else {
+			ret = regmap_write(priv->regmap, TIM_PSC, wfhw->psc);
+			if (ret)
+				goto out;
+
+			ret = regmap_write(priv->regmap, TIM_ARR, wfhw->arr);
+			if (ret)
+				goto out;
+
+			ret = regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
+			if (ret)
+				goto out;
+
+		}
+
+		/* set polarity */
+		mask = TIM_CCER_CCxP(ch + 1) | TIM_CCER_CCxNP(ch + 1);
+		ret = regmap_update_bits(priv->regmap, TIM_CCER, mask, wfhw->ccer);
+		if (ret)
+			goto out;
+
+		ret = regmap_write(priv->regmap, TIM_CCRx(ch + 1), wfhw->ccr);
+		if (ret)
+			goto out;
+
+		/* Configure output mode */
+		shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
+		ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
+		mask = CCMR_CHANNEL_MASK << shift;
+
+		if (ch < 2)
+			ret = regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
+		else
+			ret = regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
+		if (ret)
+			goto out;
+
+		ret = regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
+		if (ret)
+			goto out;
+
+		if (!(ccer & TIM_CCER_CCxE(ch + 1))) {
+			mask = TIM_CCER_CCxE(ch + 1) | TIM_CCER_CCxNE(ch + 1);
+
+			ret = clk_enable(priv->clk);
+			if (ret)
+				goto out;
+
+			ccer = (ccer & ~mask) | (wfhw->ccer & mask);
+			regmap_write(priv->regmap, TIM_CCER, ccer);
+
+			/* Make sure that registers are updated */
+			regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
+
+			/* Enable controller */
+			regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
+		}
+
+	} else {
+		/* disable channel */
+		u32 mask, ccer;
+
+		mask = TIM_CCER_CCxE(ch + 1);
+		if (priv->have_complementary_output)
+			mask |= TIM_CCER_CCxNE(ch + 1);
+
+		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+		if (ret)
+			goto out;
+
+		if (ccer & mask) {
+			ccer = ccer & ~mask;
+
+			ret = regmap_write(priv->regmap, TIM_CCER, ccer);
+			if (ret)
+				goto out;
+
+			if (!(ccer & TIM_CCER_CCXE)) {
+				/* When all channels are disabled, we can disable the controller */
+				ret = regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
+				if (ret)
+					goto out;
+			}
+
+			clk_disable(priv->clk);
+		}
+	}
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
 #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
 #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
 #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
@@ -308,228 +688,13 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
-static int stm32_pwm_config(struct stm32_pwm *priv, unsigned int ch,
-			    u64 duty_ns, u64 period_ns)
-{
-	unsigned long long prd, dty;
-	unsigned long long prescaler;
-	u32 ccmr, mask, shift;
-
-	/*
-	 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
-	 * the calculations here won't overflow.
-	 * First we need to find the minimal value for prescaler such that
-	 *
-	 *        period_ns * clkrate
-	 *   ------------------------------ < max_arr + 1
-	 *   NSEC_PER_SEC * (prescaler + 1)
-	 *
-	 * This equation is equivalent to
-	 *
-	 *        period_ns * clkrate
-	 *   ---------------------------- < prescaler + 1
-	 *   NSEC_PER_SEC * (max_arr + 1)
-	 *
-	 * Using integer division and knowing that the right hand side is
-	 * integer, this is further equivalent to
-	 *
-	 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
-	 */
-
-	prescaler = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
-					(u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
-	if (prescaler > MAX_TIM_PSC)
-		return -EINVAL;
-
-	prd = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
-				  (u64)NSEC_PER_SEC * (prescaler + 1));
-	if (!prd)
-		return -EINVAL;
-
-	/*
-	 * All channels share the same prescaler and counter so when two
-	 * channels are active at the same time we can't change them
-	 */
-	if (active_channels(priv) & ~(1 << ch * 4)) {
-		u32 psc, arr;
-
-		regmap_read(priv->regmap, TIM_PSC, &psc);
-		regmap_read(priv->regmap, TIM_ARR, &arr);
-
-		if ((psc != prescaler) || (arr != prd - 1))
-			return -EBUSY;
-	}
-
-	regmap_write(priv->regmap, TIM_PSC, prescaler);
-	regmap_write(priv->regmap, TIM_ARR, prd - 1);
-	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
-
-	/* Calculate the duty cycles */
-	dty = mul_u64_u64_div_u64(duty_ns, clk_get_rate(priv->clk),
-				  (u64)NSEC_PER_SEC * (prescaler + 1));
-
-	regmap_write(priv->regmap, TIM_CCRx(ch + 1), dty);
-
-	/* Configure output mode */
-	shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
-	ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
-	mask = CCMR_CHANNEL_MASK << shift;
-
-	if (ch < 2)
-		regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
-	else
-		regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
-
-	regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
-
-	return 0;
-}
-
-static int stm32_pwm_set_polarity(struct stm32_pwm *priv, unsigned int ch,
-				  enum pwm_polarity polarity)
-{
-	u32 mask;
-
-	mask = TIM_CCER_CCxP(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNP(ch + 1);
-
-	regmap_update_bits(priv->regmap, TIM_CCER, mask,
-			   polarity == PWM_POLARITY_NORMAL ? 0 : mask);
-
-	return 0;
-}
-
-static int stm32_pwm_enable(struct stm32_pwm *priv, unsigned int ch)
-{
-	u32 mask;
-	int ret;
-
-	ret = clk_enable(priv->clk);
-	if (ret)
-		return ret;
-
-	/* Enable channel */
-	mask = TIM_CCER_CCxE(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNE(ch);
-
-	regmap_set_bits(priv->regmap, TIM_CCER, mask);
-
-	/* Make sure that registers are updated */
-	regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
-
-	/* Enable controller */
-	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
-
-	return 0;
-}
-
-static void stm32_pwm_disable(struct stm32_pwm *priv, unsigned int ch)
-{
-	u32 mask;
-
-	/* Disable channel */
-	mask = TIM_CCER_CCxE(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNE(ch + 1);
-
-	regmap_clear_bits(priv->regmap, TIM_CCER, mask);
-
-	/* When all channels are disabled, we can disable the controller */
-	if (!active_channels(priv))
-		regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
-
-	clk_disable(priv->clk);
-}
-
-static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   const struct pwm_state *state)
-{
-	bool enabled;
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ret;
-
-	enabled = pwm->state.enabled;
-
-	if (!state->enabled) {
-		if (enabled)
-			stm32_pwm_disable(priv, pwm->hwpwm);
-		return 0;
-	}
-
-	if (state->polarity != pwm->state.polarity)
-		stm32_pwm_set_polarity(priv, pwm->hwpwm, state->polarity);
-
-	ret = stm32_pwm_config(priv, pwm->hwpwm,
-			       state->duty_cycle, state->period);
-	if (ret)
-		return ret;
-
-	if (!enabled && state->enabled)
-		ret = stm32_pwm_enable(priv, pwm->hwpwm);
-
-	return ret;
-}
-
-static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
-				  const struct pwm_state *state)
-{
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ret;
-
-	/* protect common prescaler for all active channels */
-	mutex_lock(&priv->lock);
-	ret = stm32_pwm_apply(chip, pwm, state);
-	mutex_unlock(&priv->lock);
-
-	return ret;
-}
-
-static int stm32_pwm_get_state(struct pwm_chip *chip,
-			       struct pwm_device *pwm, struct pwm_state *state)
-{
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ch = pwm->hwpwm;
-	unsigned long rate;
-	u32 ccer, psc, arr, ccr;
-	u64 dty, prd;
-	int ret;
-
-	mutex_lock(&priv->lock);
-
-	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
-	if (ret)
-		goto out;
-
-	state->enabled = ccer & TIM_CCER_CCxE(ch + 1);
-	state->polarity = (ccer & TIM_CCER_CCxP(ch + 1)) ?
-			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
-	ret = regmap_read(priv->regmap, TIM_PSC, &psc);
-	if (ret)
-		goto out;
-	ret = regmap_read(priv->regmap, TIM_ARR, &arr);
-	if (ret)
-		goto out;
-	ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &ccr);
-	if (ret)
-		goto out;
-
-	rate = clk_get_rate(priv->clk);
-
-	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
-	state->period = DIV_ROUND_UP_ULL(prd, rate);
-	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
-	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
-
-out:
-	mutex_unlock(&priv->lock);
-	return ret;
-}
-
 static const struct pwm_ops stm32pwm_ops = {
-	.apply = stm32_pwm_apply_locked,
-	.get_state = stm32_pwm_get_state,
+	.sizeof_wfhw = sizeof(struct stm32_pwm_waveform),
+	.round_waveform_tohw = stm32_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = stm32_pwm_round_waveform_fromhw,
+	.read_waveform = stm32_pwm_read_waveform,
+	.write_waveform = stm32_pwm_write_waveform,
+
 	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
 };
 
-- 
2.43.0

Re: [PATCH v3 0/8] pwm: New abstraction and userspace API

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3210 bytes --]

On Mon, Jul 29, 2024 at 04:34:16PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> here comes v3 of the series to add support for duty offset in PWM
> waveforms. For a single PWM output there is no gain, but if you have a
> chip with two (or more) outputs and both operate with the same period,
> you can generate an output like:
> 
> 
>                ______         ______         ______         ______
>    PWM #0  ___/      \_______/      \_______/      \_______/      \_______
>                  __             __             __             __    
>    PWM #1  _____/  \___________/  \___________/  \___________/  \_________
>               ^              ^              ^              ^
> 
> Changes since v2, which is available from
> https://lore.kernel.org/linux-pwm/cover.1721040875.git.u.kleine-koenig@baylibre.com
> include:
> 
>  - Degrade a dev_alert() to dev_warn() on feedback by David Lechner
> 
>  - Improvement of various comments (partly in reply to David Lechner)
> 
>  - Add _ns suffixes for members of pwm_waveform, thanks David for that suggestion.
> 
>  - Rebased to v6.11-rc1 + pwm/for-next.
> 
> Because of these changes I didn't add Trevor's Reviewed-by tag for patch
> #3.
> 
> I kept the BUG_ONs. There are a few check_patch warnings about it, but I still
> prefer these given that it is safe they don't trigger without further (bogus)
> code changes and when they trigger crashing early is better than stack
> corruption. Also checkpatch tells
>         WARNING: Comparisons should place the constant on the right side of the test
>         #158: FILE: drivers/pwm/core.c:262:
>         +       BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> 
> But as the BUG_ON is about WFHWSIZE being wrong, this order is OK.
> 
> There are a few more checkpatch warnings about line lengths. Breaking
> the criticised lines further hurts readability IMHO, so I kept them. It
> gets a bit better once stm32_pwm_mul_u64_u64_div_u64_roundup() is
> implemented (without the stm32_pwm prefix) alongside
> mul_u64_u64_div_u64() in lib/math/div64.c, but I don't want to wait for
> that. I will address that once Nicolas's patch improving precision of
> mul_u64_u64_div_u64() landed. (Hmm, it's not in next any more since
> next-20240724, before it was 3cc8bf1a81efde105d8e57398cf8554b57768777 +
> dbbe95af0fad2a9d22a4b910cfc4b87949d61a3c).
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (8):
>   pwm: Simplify pwm_capture()
>   pwm: Add more locking
>   pwm: New abstraction for PWM waveforms
>   pwm: Provide new consumer API functions for waveforms
>   pwm: Add support for pwmchip devices for faster and easier userspace
>     access
>   pwm: Add tracing for waveform callbacks
>   pwm: axi-pwmgen: Implementation of the waveform callbacks
>   pwm: stm32: Implementation of the waveform callbacks

I applied patch #1 which is a harmless cleanup for now. I will wait a
bit for the rest of the series, as during August I won't be able to
react to fall-outs reliably and quickly. I plan to apply this series
with PWM_IOCTL_GET_NUM_PWMS dropped directly after the next merge window
for cooking in next as long as possible.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 8/8] pwm: stm32: Implementation of the waveform callbacks

[Fabrice Gasnier]

On 7/29/24 16:34, Uwe Kleine-König wrote:
> Convert the stm32 pwm driver to use the new callbacks for hardware
> programming.

Hi Uwe,

Sorry it took me some time to start to have a look. I only had an
overview on the series, and this patch. I'd have some overall question
on the waveform support (on the delay offset).

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/pwm-stm32.c | 607 +++++++++++++++++++++++++---------------
>  1 file changed, 386 insertions(+), 221 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index fd754a99cf2e..846da265bbfe 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -51,6 +51,386 @@ static u32 active_channels(struct stm32_pwm *dev)
>  	return ccer & TIM_CCER_CCXE;
>  }
>  
> +struct stm32_pwm_waveform {
> +	u32 ccer;
> +	u32 psc;
> +	u32 arr;
> +	u32 ccr;
> +};
> +
> +static int stm32_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
> +{
> +	struct stm32_pwm_waveform *wfhw = _wfhw;
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	unsigned int ch = pwm->hwpwm;
> +	unsigned long rate;
> +	u64 ccr, duty;
> +	int ret;
> +
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = (struct stm32_pwm_waveform){
> +			.ccer = 0,
> +		};
> +
> +		return 0;
> +	}
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	wfhw->ccer = TIM_CCER_CCxE(ch + 1);
> +	if (priv->have_complementary_output)
> +		wfhw->ccer = TIM_CCER_CCxNE(ch);

Need to use (ch + 1 here), and avoid overriding ccer when
have_complementary_output is true, e.g.

	if (priv->have_complementary_output)
		wfhw->ccer |= TIM_CCER_CCxNE(ch + 1);

> +
> +	rate = clk_get_rate(priv->clk);
> +
> +	if (active_channels(priv) & ~(1 << ch * 4)) {
> +		u64 arr;
> +
> +		/*
> +		 * Other channels are already enabled, so the configured PSC and
> +		 * ARR must be used for this channel, too.
> +		 */
> +		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
> +		if (ret)
> +			goto out;
> +
> +		/*
> +		 * calculate the best value for ARR for the given PSC, refuse if
> +		 * the resulting period gets bigger than the requested one.
> +		 */
> +		arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
> +					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
> +		if (arr <= wfhw->arr) {
> +			/*
> +			 * requested period is small than the currently
> +			 * configured and unchangable period, report back the smallest
> +			 * possible period, i.e. the current state; Initialize
> +			 * ccr to anything valid.
> +			 */
> +			wfhw->ccr = 0;
> +			ret = 1;

I'm suprised, I'm more used to return negative error codes. This may
cascade up to the sysfs interface. Is there some possible side effect ?

I could see in your commit message in "pwm: New abstraction for PWM
waveforms" that "... this fact is signaled by a return value of 1".

Perhaps some define could be used, to better point that ?

> +			goto out;
> +		}
> +
> +	} else {
> +		/*
> +		 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
> +		 * the calculations here won't overflow.
> +		 * First we need to find the minimal value for prescaler such that
> +		 *
> +		 *        period_ns * clkrate
> +		 *   ------------------------------ < max_arr + 1
> +		 *   NSEC_PER_SEC * (prescaler + 1)
> +		 *
> +		 * This equation is equivalent to
> +		 *
> +		 *        period_ns * clkrate
> +		 *   ---------------------------- < prescaler + 1
> +		 *   NSEC_PER_SEC * (max_arr + 1)
> +		 *
> +		 * Using integer division and knowing that the right hand side is
> +		 * integer, this is further equivalent to
> +		 *
> +		 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
> +		 */
> +		u64 psc = mul_u64_u64_div_u64(wf->period_length_ns, rate,
> +					      (u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
> +		u64 arr;
> +
> +		wfhw->psc = min_t(u64, psc, MAX_TIM_PSC);
> +
> +		arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
> +					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
> +		if (!arr) {
> +			/*
> +			 * requested period is too small, report back the smallest
> +			 * possible period, i.e. ARR = 0. The only valid CCR
> +			 * value is then zero, too.
> +			 */
> +			wfhw->arr = 0;
> +			wfhw->ccr = 0;
> +			ret = 1;

same questions here

> +			goto out;
> +		}
> +
> +		/*
> +		 * ARR is limited intentionally to values less than
> +		 * priv->max_arr to allow 100% duty cycle.
> +		 */
> +		wfhw->arr = min_t(u64, arr, priv->max_arr) - 1;
> +	}
> +
> +	duty = mul_u64_u64_div_u64(wf->duty_length_ns, rate,
> +				   (u64)NSEC_PER_SEC * (wfhw->psc + 1));
> +	duty = min_t(u64, duty, wfhw->arr + 1);
> +
> +	if (wf->duty_length_ns && wf->duty_offset_ns &&
> +	    wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns) {

The condition here (mixing && + >=) is rather hard to read, could it be
more readable ?

It's more clear when reading pwm_wf2state() and pwm_state2wf() the
condition for normal/inversed polarity rather looks like:

if (wf->period_length_ns) {
	if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns)
	/* normal */
	else
	/* inversed */

BTW I notice small difference here: (wf->duty_length_ns &&
wf->duty_offset_ns)

Suggestion: could use some (new) helper function or macro from the pwm
core ? e.g. rather than implementing in the driver ?


> +		wfhw->ccer |= TIM_CCER_CCxP(ch + 1);
> +		if (priv->have_complementary_output)
> +			wfhw->ccer |= TIM_CCER_CCxNP(ch + 1);
> +
> +		ccr = wfhw->arr + 1 - duty;
> +	} else {
> +		ccr = duty;
> +	}
> +
> +	wfhw->ccr = min_t(u64, ccr, wfhw->arr + 1);
> +
> +	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x\n",
> +		pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +		rate, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr);
> +
> +out:
> +	clk_disable(priv->clk);
> +
> +	return ret;
> +}
> +
> +/*
> + * This should be moved to lib/math/div64.c. Currently there are some changes
> + * pending to mul_u64_u64_div_u64. Uwe will care for that when the dust settles.
> + */
> +static u64 stm32_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
> +{
> +	u64 res = mul_u64_u64_div_u64(a, b, c);
> +	/* Those multiplications might overflow but it doesn't matter */
> +	u64 rem = a * b - c * res;
> +
> +	if (rem)
> +		res += 1;
> +
> +	return res;
> +}
> +
> +static int stm32_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const void *_wfhw,
> +					   struct pwm_waveform *wf)
> +{
> +	const struct stm32_pwm_waveform *wfhw = _wfhw;
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	unsigned int ch = pwm->hwpwm;
> +
> +	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
> +		unsigned long rate = clk_get_rate(priv->clk);
> +		u64 ccr_ns;
> +
> +		/* The result doesn't overflow for rate >= 15259 */
> +		wf->period_length_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1),
> +									     NSEC_PER_SEC, rate);
> +
> +		ccr_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * wfhw->ccr,
> +							       NSEC_PER_SEC, rate);
> +
> +		if (wfhw->ccer & TIM_CCER_CCxP(ch + 1)) {
> +			wf->duty_length_ns =
> +				stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
> +								      NSEC_PER_SEC, rate);
> +
> +			wf->duty_offset_ns = ccr_ns;
> +		} else {
> +			wf->duty_length_ns = ccr_ns;
> +			wf->duty_offset_ns = 0;
> +		}

Well, my main confusion is around duty_offset_ns. Indeed, it's right
here, that with PWM mode 1 (CCMR bit 5 and 6 set later on), only
possibilty is to have either 0, or the period - duty cycle as delay when
polarity is inversed.

I gave it a try (basic testing), but user doesn't get information when
setting a waveform (with a valid duty_offset_ns), but the hardware
doesn't necessarily apply it when writing the waveform ? What's your
advise / opinion ?



> +	} else {
> +		*wf = (struct pwm_waveform){
> +			.period_length_ns = 0,
> +		};
> +	}

Would be nice to add similar dev_dbg() as in
stm32_pwm_round_waveform_tohw().


Thanks for your patch,
Best Regards,
Fabrice

> +
> +	return 0;
> +}
> +
> +static int stm32_pwm_read_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     void *_wfhw)
> +{
> +	struct stm32_pwm_waveform *wfhw = _wfhw;
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	unsigned int ch = pwm->hwpwm;
> +	int ret;
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(priv->regmap, TIM_CCER, &wfhw->ccer);
> +	if (ret)
> +		goto out;
> +
> +	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
> +		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
> +		if (ret)
> +			goto out;
> +
> +		if (wfhw->arr == U32_MAX)
> +			wfhw->arr -= 1;
> +
> +		ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &wfhw->ccr);
> +		if (ret)
> +			goto out;
> +
> +		if (wfhw->ccr > wfhw->arr + 1)
> +			wfhw->ccr = wfhw->arr + 1;
> +	}
> +
> +out:
> +	clk_disable(priv->clk);
> +
> +	return ret;
> +}
> +
> +static int stm32_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	const struct stm32_pwm_waveform *wfhw = _wfhw;
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	unsigned int ch = pwm->hwpwm;
> +	int ret;
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
> +		u32 ccer, mask;
> +		unsigned int shift;
> +		u32 ccmr;
> +
> +		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +		if (ret)
> +			goto out;
> +
> +		/* If there are other channels enabled, don't update PSC and ARR */
> +		if (ccer & ~TIM_CCER_CCxE(ch + 1) & TIM_CCER_CCXE) {
> +			u32 psc, arr;
> +
> +			ret = regmap_read(priv->regmap, TIM_PSC, &psc);
> +			if (ret)
> +				goto out;
> +
> +			if (psc != wfhw->psc) {
> +				ret = -EBUSY;
> +				goto out;
> +			}
> +
> +			regmap_read(priv->regmap, TIM_ARR, &arr);
> +			if (ret)
> +				goto out;
> +
> +			if (arr != wfhw->arr) {
> +				ret = -EBUSY;
> +				goto out;
> +			}
> +		} else {
> +			ret = regmap_write(priv->regmap, TIM_PSC, wfhw->psc);
> +			if (ret)
> +				goto out;
> +
> +			ret = regmap_write(priv->regmap, TIM_ARR, wfhw->arr);
> +			if (ret)
> +				goto out;
> +
> +			ret = regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
> +			if (ret)
> +				goto out;
> +
> +		}
> +
> +		/* set polarity */
> +		mask = TIM_CCER_CCxP(ch + 1) | TIM_CCER_CCxNP(ch + 1);
> +		ret = regmap_update_bits(priv->regmap, TIM_CCER, mask, wfhw->ccer);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_write(priv->regmap, TIM_CCRx(ch + 1), wfhw->ccr);
> +		if (ret)
> +			goto out;
> +
> +		/* Configure output mode */
> +		shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
> +		ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
> +		mask = CCMR_CHANNEL_MASK << shift;
> +
> +		if (ch < 2)
> +			ret = regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
> +		else
> +			ret = regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
> +		if (ret)
> +			goto out;
> +
> +		if (!(ccer & TIM_CCER_CCxE(ch + 1))) {
> +			mask = TIM_CCER_CCxE(ch + 1) | TIM_CCER_CCxNE(ch + 1);
> +
> +			ret = clk_enable(priv->clk);
> +			if (ret)
> +				goto out;
> +
> +			ccer = (ccer & ~mask) | (wfhw->ccer & mask);
> +			regmap_write(priv->regmap, TIM_CCER, ccer);
> +
> +			/* Make sure that registers are updated */
> +			regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
> +
> +			/* Enable controller */
> +			regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> +		}
> +
> +	} else {
> +		/* disable channel */
> +		u32 mask, ccer;
> +
> +		mask = TIM_CCER_CCxE(ch + 1);
> +		if (priv->have_complementary_output)
> +			mask |= TIM_CCER_CCxNE(ch + 1);
> +
> +		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +		if (ret)
> +			goto out;
> +
> +		if (ccer & mask) {
> +			ccer = ccer & ~mask;
> +
> +			ret = regmap_write(priv->regmap, TIM_CCER, ccer);
> +			if (ret)
> +				goto out;
> +
> +			if (!(ccer & TIM_CCER_CCXE)) {
> +				/* When all channels are disabled, we can disable the controller */
> +				ret = regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> +				if (ret)
> +					goto out;
> +			}
> +
> +			clk_disable(priv->clk);
> +		}
> +	}
> +
> +out:
> +	clk_disable(priv->clk);
> +
> +	return ret;
> +}
> +
>  #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
>  #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
>  #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
> @@ -308,228 +688,13 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> -static int stm32_pwm_config(struct stm32_pwm *priv, unsigned int ch,
> -			    u64 duty_ns, u64 period_ns)
> -{
> -	unsigned long long prd, dty;
> -	unsigned long long prescaler;
> -	u32 ccmr, mask, shift;
> -
> -	/*
> -	 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
> -	 * the calculations here won't overflow.
> -	 * First we need to find the minimal value for prescaler such that
> -	 *
> -	 *        period_ns * clkrate
> -	 *   ------------------------------ < max_arr + 1
> -	 *   NSEC_PER_SEC * (prescaler + 1)
> -	 *
> -	 * This equation is equivalent to
> -	 *
> -	 *        period_ns * clkrate
> -	 *   ---------------------------- < prescaler + 1
> -	 *   NSEC_PER_SEC * (max_arr + 1)
> -	 *
> -	 * Using integer division and knowing that the right hand side is
> -	 * integer, this is further equivalent to
> -	 *
> -	 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
> -	 */
> -
> -	prescaler = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
> -					(u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
> -	if (prescaler > MAX_TIM_PSC)
> -		return -EINVAL;
> -
> -	prd = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
> -				  (u64)NSEC_PER_SEC * (prescaler + 1));
> -	if (!prd)
> -		return -EINVAL;
> -
> -	/*
> -	 * All channels share the same prescaler and counter so when two
> -	 * channels are active at the same time we can't change them
> -	 */
> -	if (active_channels(priv) & ~(1 << ch * 4)) {
> -		u32 psc, arr;
> -
> -		regmap_read(priv->regmap, TIM_PSC, &psc);
> -		regmap_read(priv->regmap, TIM_ARR, &arr);
> -
> -		if ((psc != prescaler) || (arr != prd - 1))
> -			return -EBUSY;
> -	}
> -
> -	regmap_write(priv->regmap, TIM_PSC, prescaler);
> -	regmap_write(priv->regmap, TIM_ARR, prd - 1);
> -	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
> -
> -	/* Calculate the duty cycles */
> -	dty = mul_u64_u64_div_u64(duty_ns, clk_get_rate(priv->clk),
> -				  (u64)NSEC_PER_SEC * (prescaler + 1));
> -
> -	regmap_write(priv->regmap, TIM_CCRx(ch + 1), dty);
> -
> -	/* Configure output mode */
> -	shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
> -	ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
> -	mask = CCMR_CHANNEL_MASK << shift;
> -
> -	if (ch < 2)
> -		regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
> -	else
> -		regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
> -
> -	regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
> -
> -	return 0;
> -}
> -
> -static int stm32_pwm_set_polarity(struct stm32_pwm *priv, unsigned int ch,
> -				  enum pwm_polarity polarity)
> -{
> -	u32 mask;
> -
> -	mask = TIM_CCER_CCxP(ch + 1);
> -	if (priv->have_complementary_output)
> -		mask |= TIM_CCER_CCxNP(ch + 1);
> -
> -	regmap_update_bits(priv->regmap, TIM_CCER, mask,
> -			   polarity == PWM_POLARITY_NORMAL ? 0 : mask);
> -
> -	return 0;
> -}
> -
> -static int stm32_pwm_enable(struct stm32_pwm *priv, unsigned int ch)
> -{
> -	u32 mask;
> -	int ret;
> -
> -	ret = clk_enable(priv->clk);
> -	if (ret)
> -		return ret;
> -
> -	/* Enable channel */
> -	mask = TIM_CCER_CCxE(ch + 1);
> -	if (priv->have_complementary_output)
> -		mask |= TIM_CCER_CCxNE(ch);
> -
> -	regmap_set_bits(priv->regmap, TIM_CCER, mask);
> -
> -	/* Make sure that registers are updated */
> -	regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
> -
> -	/* Enable controller */
> -	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> -
> -	return 0;
> -}
> -
> -static void stm32_pwm_disable(struct stm32_pwm *priv, unsigned int ch)
> -{
> -	u32 mask;
> -
> -	/* Disable channel */
> -	mask = TIM_CCER_CCxE(ch + 1);
> -	if (priv->have_complementary_output)
> -		mask |= TIM_CCER_CCxNE(ch + 1);
> -
> -	regmap_clear_bits(priv->regmap, TIM_CCER, mask);
> -
> -	/* When all channels are disabled, we can disable the controller */
> -	if (!active_channels(priv))
> -		regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> -
> -	clk_disable(priv->clk);
> -}
> -
> -static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> -			   const struct pwm_state *state)
> -{
> -	bool enabled;
> -	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> -	int ret;
> -
> -	enabled = pwm->state.enabled;
> -
> -	if (!state->enabled) {
> -		if (enabled)
> -			stm32_pwm_disable(priv, pwm->hwpwm);
> -		return 0;
> -	}
> -
> -	if (state->polarity != pwm->state.polarity)
> -		stm32_pwm_set_polarity(priv, pwm->hwpwm, state->polarity);
> -
> -	ret = stm32_pwm_config(priv, pwm->hwpwm,
> -			       state->duty_cycle, state->period);
> -	if (ret)
> -		return ret;
> -
> -	if (!enabled && state->enabled)
> -		ret = stm32_pwm_enable(priv, pwm->hwpwm);
> -
> -	return ret;
> -}
> -
> -static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
> -				  const struct pwm_state *state)
> -{
> -	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> -	int ret;
> -
> -	/* protect common prescaler for all active channels */
> -	mutex_lock(&priv->lock);
> -	ret = stm32_pwm_apply(chip, pwm, state);
> -	mutex_unlock(&priv->lock);
> -
> -	return ret;
> -}
> -
> -static int stm32_pwm_get_state(struct pwm_chip *chip,
> -			       struct pwm_device *pwm, struct pwm_state *state)
> -{
> -	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> -	int ch = pwm->hwpwm;
> -	unsigned long rate;
> -	u32 ccer, psc, arr, ccr;
> -	u64 dty, prd;
> -	int ret;
> -
> -	mutex_lock(&priv->lock);
> -
> -	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> -	if (ret)
> -		goto out;
> -
> -	state->enabled = ccer & TIM_CCER_CCxE(ch + 1);
> -	state->polarity = (ccer & TIM_CCER_CCxP(ch + 1)) ?
> -			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> -	ret = regmap_read(priv->regmap, TIM_PSC, &psc);
> -	if (ret)
> -		goto out;
> -	ret = regmap_read(priv->regmap, TIM_ARR, &arr);
> -	if (ret)
> -		goto out;
> -	ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &ccr);
> -	if (ret)
> -		goto out;
> -
> -	rate = clk_get_rate(priv->clk);
> -
> -	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> -	state->period = DIV_ROUND_UP_ULL(prd, rate);
> -	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> -	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> -
> -out:
> -	mutex_unlock(&priv->lock);
> -	return ret;
> -}
> -
>  static const struct pwm_ops stm32pwm_ops = {
> -	.apply = stm32_pwm_apply_locked,
> -	.get_state = stm32_pwm_get_state,
> +	.sizeof_wfhw = sizeof(struct stm32_pwm_waveform),
> +	.round_waveform_tohw = stm32_pwm_round_waveform_tohw,
> +	.round_waveform_fromhw = stm32_pwm_round_waveform_fromhw,
> +	.read_waveform = stm32_pwm_read_waveform,
> +	.write_waveform = stm32_pwm_write_waveform,
> +
>  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  

Re: [PATCH v3 8/8] pwm: stm32: Implementation of the waveform callbacks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7460 bytes --]

Hello Fabrice,

On Tue, Aug 20, 2024 at 06:09:59PM +0200, Fabrice Gasnier wrote:
> On 7/29/24 16:34, Uwe Kleine-König wrote:
> > Convert the stm32 pwm driver to use the new callbacks for hardware
> > programming.
> 
> Sorry it took me some time to start to have a look. I only had an
> overview on the series, and this patch. I'd have some overall question
> on the waveform support (on the delay offset).

No need to be sorry, I very appreciate you looking into my patch set.

> > +	wfhw->ccer = TIM_CCER_CCxE(ch + 1);
> > +	if (priv->have_complementary_output)
> > +		wfhw->ccer = TIM_CCER_CCxNE(ch);
> 
> Need to use (ch + 1 here), and avoid overriding ccer when
> have_complementary_output is true, e.g.
> 
> 	if (priv->have_complementary_output)
> 		wfhw->ccer |= TIM_CCER_CCxNE(ch + 1);

Huh, indeed. Thanks.

> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	if (active_channels(priv) & ~(1 << ch * 4)) {
> > +		u64 arr;
> > +
> > +		/*
> > +		 * Other channels are already enabled, so the configured PSC and
> > +		 * ARR must be used for this channel, too.
> > +		 */
> > +		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
> > +		if (ret)
> > +			goto out;
> > +
> > +		/*
> > +		 * calculate the best value for ARR for the given PSC, refuse if
> > +		 * the resulting period gets bigger than the requested one.
> > +		 */
> > +		arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
> > +					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
> > +		if (arr <= wfhw->arr) {
> > +			/*
> > +			 * requested period is small than the currently
> > +			 * configured and unchangable period, report back the smallest
> > +			 * possible period, i.e. the current state; Initialize
> > +			 * ccr to anything valid.
> > +			 */
> > +			wfhw->ccr = 0;
> > +			ret = 1;
> 
> I'm suprised, I'm more used to return negative error codes. This may
> cascade up to the sysfs interface. Is there some possible side effect ?

I'm not entirely happy with that 1, too, but I didn't want to use an
existing error code, because I wanted to catch exactly the condition
that no valid rounding exists and so having a dedicated value for it
looks right to me. Then I didn't want to use a negative value to be sure
to not interpret it as an errno value.

This shouldn't propagate to the sysfs interface (or even __pwm_apply()).
I need to fix that.

> I could see in your commit message in "pwm: New abstraction for PWM
> waveforms" that "... this fact is signaled by a return value of 1".
> 
> Perhaps some define could be used, to better point that ?

I shortly considered that while implementing, but decided against it
because I didn't wanted to clobber the fact, that it's a positive value.
Reading your suggestion I'll think about it again.

> > +	if (wf->duty_length_ns && wf->duty_offset_ns &&
> > +	    wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns) {
> 
> The condition here (mixing && + >=) is rather hard to read, could it be
> more readable ?
> 
> It's more clear when reading pwm_wf2state() and pwm_state2wf() the
> condition for normal/inversed polarity rather looks like:
> 
> if (wf->period_length_ns) {
> 	if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns)
> 	/* normal */
> 	else
> 	/* inversed */
> 
> BTW I notice small difference here: (wf->duty_length_ns &&
> wf->duty_offset_ns)
> 
> Suggestion: could use some (new) helper function or macro from the pwm
> core ? e.g. rather than implementing in the driver ?

Hmm, this will indeed be useful for all drivers that have no way to
configure the offset in a more flexible way than inverting the polarity
(which I'd guess are most of them). I'll try an implementation to judge
if I like it.
 
> > +		wfhw->ccer |= TIM_CCER_CCxP(ch + 1);
> > +		if (priv->have_complementary_output)
> > +			wfhw->ccer |= TIM_CCER_CCxNP(ch + 1);
> > +
> > +		ccr = wfhw->arr + 1 - duty;
> > +	} else {
> > +		ccr = duty;
> > +	}
> > +
> > +	wfhw->ccr = min_t(u64, ccr, wfhw->arr + 1);
> > +
> > +	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x\n",
> > +		pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> > +		rate, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr);
> > +
> > +out:
> > +	clk_disable(priv->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * This should be moved to lib/math/div64.c. Currently there are some changes
> > + * pending to mul_u64_u64_div_u64. Uwe will care for that when the dust settles.
> > + */
> > +static u64 stm32_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
> > +{
> > +	u64 res = mul_u64_u64_div_u64(a, b, c);
> > +	/* Those multiplications might overflow but it doesn't matter */
> > +	u64 rem = a * b - c * res;
> > +
> > +	if (rem)
> > +		res += 1;
> > +
> > +	return res;
> > +}
> > +
> > +static int stm32_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> > +					   struct pwm_device *pwm,
> > +					   const void *_wfhw,
> > +					   struct pwm_waveform *wf)
> > +{
> > +	const struct stm32_pwm_waveform *wfhw = _wfhw;
> > +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > +	unsigned int ch = pwm->hwpwm;
> > +
> > +	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
> > +		unsigned long rate = clk_get_rate(priv->clk);
> > +		u64 ccr_ns;
> > +
> > +		/* The result doesn't overflow for rate >= 15259 */
> > +		wf->period_length_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1),
> > +									     NSEC_PER_SEC, rate);
> > +
> > +		ccr_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * wfhw->ccr,
> > +							       NSEC_PER_SEC, rate);
> > +
> > +		if (wfhw->ccer & TIM_CCER_CCxP(ch + 1)) {
> > +			wf->duty_length_ns =
> > +				stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
> > +								      NSEC_PER_SEC, rate);
> > +
> > +			wf->duty_offset_ns = ccr_ns;
> > +		} else {
> > +			wf->duty_length_ns = ccr_ns;
> > +			wf->duty_offset_ns = 0;
> > +		}
> 
> Well, my main confusion is around duty_offset_ns. Indeed, it's right
> here, that with PWM mode 1 (CCMR bit 5 and 6 set later on), only
> possibilty is to have either 0, or the period - duty cycle as delay when
> polarity is inversed.
> 
> I gave it a try (basic testing), but user doesn't get information when
> setting a waveform (with a valid duty_offset_ns), but the hardware
> doesn't necessarily apply it when writing the waveform ? What's your
> advise / opinion ?

The intended behaviour is that if you pass a duty_offset_ns >= period -
duty_cycle_ns (together with duty_offset > 0), you get inversed polarity.
This isn't signaled indeed. But the same holds true for other hardware
specific adaptions (e.g. when you pass period = 12345 and that's rounded
down to 12300 because of input clock constraints). If a consumer cares
about that, there is the possiblity to use .round_waveform_tohw() +
.round_waveform_fromhw() to know beforehand.

> > +	} else {
> > +		*wf = (struct pwm_waveform){
> > +			.period_length_ns = 0,
> > +		};
> > +	}
> 
> Would be nice to add similar dev_dbg() as in
> stm32_pwm_round_waveform_tohw().

Ack.

Thanks for your two good catches and your opion on my design,
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]