[PATCH] pwm: ipq: fix period calculation

[PATCH] pwm: ipq: fix period calculation

[Stephane Lepain]

From: Kenneth Kasilag <kenneth@kasilag.me>

ipq_pwm_apply() fixes pwm_div at its maximum and derives only pre_div
from the requested period. Since the period spans
(pre_div + 1) * (pwm_div + 1) input clocks, pinning pwm_div near its
maximum forces pre_div towards zero for short periods: once pre_div
rounds to 0 the shortest representable period is (pwm_div + 1) / clk_rate,
and any shorter request is rejected outright:

	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
				      (u64)NSEC_PER_SEC * (pwm_div + 1));
	if (!pre_div)
		return -ERANGE;

Four-wire fans commonly expect a ~25 kHz PWM, which is therefore
unusable. On an IPQ6018 with the PWM block clocked at 100 MHz, a
40,000 ns (25 kHz) request computes floor(0.061) == 0 and returns
-ERANGE deterministically. Where a request is not rejected outright, the
high duration truncates to 0 and the output collapses to ~0% duty.

Search for the (pre_div, pwm_div) pair whose period best approximates
the request instead of fixing pwm_div. Starting pre_div at the smallest
value that keeps pwm_div within its field and stopping once pre_div
exceeds pwm_div bounds the loop and keeps pwm_div as large as possible
for fine duty resolution. For a 25 kHz request at 100 MHz this selects
pre_div = 0, pwm_div = 3999, i.e. exactly 4000 clocks, with full 0..4000
duty resolution.

While reworking the high-duration computation, round it to nearest
rather than truncating, so mid-range duty cycles are not biased low, and
clamp it to pwm_div + 1. Rounding, or a 100% duty request, could
otherwise push hi_dur past the period length and overflow the 16-bit
HI_DURATION field.

Also compute hi_div in get_state() in 64-bit; hi_dur * (pre_div + 1) can
exceed 32 bits before the existing promotion.

This was first fixed downstream in OpenWrt for the qualcommbe target
after testing on the Askey SBE1V1K, and has since been applied to
OpenWrt's qualcommax target as well.

Tested on a GL.iNet GL-AXT1800 (IPQ6018, 100 MHz PWM clock) whose DTS
requests a 25 kHz period for its four-wire fan:

  pwms = <&pwm 1 40000 0>;

Before, pwm-fan failed to probe on every boot:

  pwm-fan pwm-fan: failed to enable PWM
  pwm-fan pwm-fan: Failed to configure PWM: -34
  pwm-fan pwm-fan: probe with driver pwm-fan failed with error -34

The same failure is reproducible without pwm-fan, straight from sysfs:

  # echo 40000   > period; echo 1 > enable   -> write error (-ERANGE)
  # echo 2700000 > period; echo 1 > enable   -> succeeds

Because probe returns before the tachometer IRQ is requested and before
fan-supply is claimed, the board also lost fan RPM reporting and its
vcc_fan regulator stayed disabled, leaving the DTS cooling-maps with no
cooling device to bind to.

After, pwm-fan probes cleanly and the fan is confirmed spinning by its
own tachometer:

  /sys/class/hwmon/hwmon7/name                          = pwmfan
  /sys/devices/platform/pwm-fan/hwmon/hwmon7/fan1_input = 3548
  /sys/class/regulator/regulator.3 (vcc_fan)            = enabled
  /sys/class/thermal/cooling_device1                    = pwm-fan

with idle SoC temperature dropping from ~76 °C to ~51 °C.

Fixes: c436e3e9c265 ("pwm: Driver for qualcomm ipq6018 pwm block")
Signed-off-by: Kenneth Kasilag <kenneth@kasilag.me>
Tested-by: Stephane Lepain <stephanelepain@gmail.com>
Signed-off-by: Stephane Lepain <stephanelepain@gmail.com>
---
 drivers/pwm/pwm-ipq.c | 101 +++++++++++++++++++++++++++++++-----------
 1 file changed, 76 insertions(+), 25 deletions(-)

diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
index c533739..2d8a013 100644
--- a/drivers/pwm/pwm-ipq.c
+++ b/drivers/pwm/pwm-ipq.c
@@ -89,10 +89,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
 	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip);
-	unsigned int pre_div, pwm_div;
-	u64 period_ns, duty_ns;
+	unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
+	u64 period_ns, duty_ns, period_rate, min_diff;
 	unsigned long val = 0;
-	unsigned long hi_dur;
+	u64 hi_dur;
 
 	if (!state->enabled) {
 		/* clear IPQ_PWM_REG1_ENABLE */
@@ -113,34 +113,85 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	duty_ns = min(state->duty_cycle, period_ns);
 
 	/*
-	 * Pick the maximal value for PWM_DIV that still allows a
-	 * 100% relative duty cycle. This allows a fine grained
-	 * selection of duty cycles.
+	 * The period spans (pre_div + 1) * (pwm_div + 1) input clocks. Rather
+	 * than fixing pwm_div at its maximum (which gives usable duty
+	 * resolution only for long periods and collapses to ~0% for short
+	 * periods) search for the (pre_div, pwm_div) split whose period best
+	 * approximates the request while leaving pwm_div large enough to
+	 * resolve the duty cycle.
 	 */
-	pwm_div = IPQ_PWM_MAX_DIV - 1;
+	if (ipq_chip->clk_rate > 16ULL * GIGA)
+		return -EINVAL;
+	period_rate = period_ns * ipq_chip->clk_rate;
+
+	best_pre_div = IPQ_PWM_MAX_DIV;
+	best_pwm_div = IPQ_PWM_MAX_DIV;
+	min_diff = period_rate;
 
 	/*
-	 * although mul_u64_u64_div_u64 returns a u64, in practice it
-	 * won't overflow due to above constraints. Take the max period
-	 * of 10^9 (NSEC_PER_SEC) and the pwm_div + 1 (IPQ_PWM_MAX_DIV)
-	 *  10^9 * 10^8
-	 * ------------- => which fits well into a 32-bit unsigned int.
-	 * 10^9 * 65,535
+	 * Smaller pre_div than this cannot represent the period (pwm_div would
+	 * have to exceed its field), so start the search there.
 	 */
-	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
-				      (u64)NSEC_PER_SEC * (pwm_div + 1));
-
-	if (!pre_div)
-		return -ERANGE;
+	pre_div = div64_u64(period_rate,
+			    (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));
+
+	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+		u64 remainder;
+
+		pwm_div = div64_u64_rem(period_rate,
+					(u64)NSEC_PER_SEC * (pre_div + 1),
+					&remainder);
+		/* pwm_div is unsigned; the swap check below catches underflow */
+		pwm_div--;
+
+		/*
+		 * Swapping pre_div and pwm_div yields the same period but a
+		 * larger pwm_div gives finer duty resolution, so once pre_div
+		 * exceeds pwm_div every further candidate is strictly worse.
+		 */
+		if (pre_div > pwm_div)
+			break;
+
+		/* need room for 100% duty, where hi_dur == pwm_div + 1 */
+		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
+			continue;
+
+		if (remainder < min_diff) {
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+			min_diff = remainder;
+
+			if (min_diff == 0)
+				break;
+		}
+	}
 
-	pre_div -= 1;
+	pre_div = best_pre_div;
+	pwm_div = best_pwm_div;
 
-	if (pre_div > IPQ_PWM_MAX_DIV)
-		pre_div = IPQ_PWM_MAX_DIV;
+	/*
+	 * If the search found no usable candidate, best_pwm_div is left at
+	 * IPQ_PWM_MAX_DIV; cap it so pwm_div + 1 still fits the 16-bit field
+	 * and 100% duty remains expressible.
+	 */
+	if (pwm_div > IPQ_PWM_MAX_DIV - 1)
+		pwm_div = IPQ_PWM_MAX_DIV - 1;
 
-	/* pwm duty = HI_DUR * (PRE_DIV + 1) / clk_rate */
-	hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate,
-				     (u64)NSEC_PER_SEC * (pre_div + 1));
+	/*
+	 * high duration = duty_ratio * (pwm_div + 1)
+	 *              = duty_ns * clk_rate / ((pre_div + 1) * NSEC_PER_SEC)
+	 *
+	 * Round to nearest to avoid biasing every duty cycle low, then clamp
+	 * to (pwm_div + 1): rounding or a 100% duty request can otherwise push
+	 * hi_dur past the period length and overflow the 16-bit HI_DURATION field
+	 * (which would alias a full-on request down to a near-zero high time)
+	 * and asking the hardware to stay high beyond one period. pwm_div is
+	 * at most IPQ_PWM_MAX_DIV - 1, so pwm_div + 1 always fits the field.
+	 */
+	hi_dur = DIV64_U64_ROUND_CLOSEST(duty_ns * ipq_chip->clk_rate,
+					 (u64)(pre_div + 1) * NSEC_PER_SEC);
+	if (hi_dur > (u64)pwm_div + 1)
+		hi_dur = (u64)pwm_div + 1;
 
 	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
 		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
@@ -186,7 +237,7 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC,
 					   ipq_chip->clk_rate);
 
-	hi_div = hi_dur * (pre_div + 1);
+	hi_div = (u64)hi_dur * (pre_div + 1);
 	state->duty_cycle = DIV64_U64_ROUND_UP(hi_div * NSEC_PER_SEC,
 					       ipq_chip->clk_rate);
 
-- 
2.55.0

Re: [PATCH] pwm: ipq: fix period calculation

[Konrad Dybcio]

On 7/31/26 9:05 AM, Stephane Lepain wrote:
> From: Kenneth Kasilag <kenneth@kasilag.me>
> 
> ipq_pwm_apply() fixes pwm_div at its maximum and derives only pre_div
> from the requested period. Since the period spans
> (pre_div + 1) * (pwm_div + 1) input clocks, pinning pwm_div near its
> maximum forces pre_div towards zero for short periods: once pre_div
> rounds to 0 the shortest representable period is (pwm_div + 1) / clk_rate,
> and any shorter request is rejected outright:

[...]

>  	/*
> -	 * Pick the maximal value for PWM_DIV that still allows a
> -	 * 100% relative duty cycle. This allows a fine grained
> -	 * selection of duty cycles.
> +	 * The period spans (pre_div + 1) * (pwm_div + 1) input clocks. Rather
> +	 * than fixing pwm_div at its maximum (which gives usable duty
> +	 * resolution only for long periods and collapses to ~0% for short
> +	 * periods) search for the (pre_div, pwm_div) split whose period best
> +	 * approximates the request while leaving pwm_div large enough to
> +	 * resolve the duty cycle.
>  	 */

I think the comment can just go

> -	pwm_div = IPQ_PWM_MAX_DIV - 1;
> +	if (ipq_chip->clk_rate > 16ULL * GIGA)
> +		return -EINVAL;

That's a very fast clock..

[...]

> -	hi_div = hi_dur * (pre_div + 1);
> +	hi_div = (u64)hi_dur * (pre_div + 1);

This looks like a separate fix

Konrad

Re: [PATCH] pwm: ipq: fix period calculation

[kernel test robot]

Hi Stephane,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v7.2-rc5 next-20260731]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stephane-Lepain/pwm-ipq-fix-period-calculation/20260731-152907
base:   linus/master
patch link:    https://lore.kernel.org/r/20260731070542.155398-1-stephanelepain%40gmail.com
patch subject: [PATCH] pwm: ipq: fix period calculation
config: arm-randconfig-004-20260731 (https://download.01.org/0day-ci/archive/20260801/202608010336.m82kvZZ8-lkp@intel.com/config)
compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project bacfe2950f8218268fcc0a8765644ea0c15f0360)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260801/202608010336.m82kvZZ8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202608010336.m82kvZZ8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-ipq.c:123:25: warning: result of comparison of constant 16000000000 with expression of type 'unsigned long' is always false [-Wtautological-constant-out-of-range-compare]
     123 |         if (ipq_chip->clk_rate > 16ULL * GIGA)
         |             ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
   1 warning generated.


vim +123 drivers/pwm/pwm-ipq.c

    87	
    88	static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
    89				 const struct pwm_state *state)
    90	{
    91		struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip);
    92		unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
    93		u64 period_ns, duty_ns, period_rate, min_diff;
    94		unsigned long val = 0;
    95		u64 hi_dur;
    96	
    97		if (!state->enabled) {
    98			/* clear IPQ_PWM_REG1_ENABLE */
    99			ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, IPQ_PWM_REG1_UPDATE);
   100			return 0;
   101		}
   102	
   103		if (state->polarity != PWM_POLARITY_NORMAL)
   104			return -EINVAL;
   105	
   106		/*
   107		 * Check the upper and lower bounds for the period as per
   108		 * hardware limits
   109		 */
   110		if (state->period < IPQ_PWM_MIN_PERIOD_NS)
   111			return -ERANGE;
   112		period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
   113		duty_ns = min(state->duty_cycle, period_ns);
   114	
   115		/*
   116		 * The period spans (pre_div + 1) * (pwm_div + 1) input clocks. Rather
   117		 * than fixing pwm_div at its maximum (which gives usable duty
   118		 * resolution only for long periods and collapses to ~0% for short
   119		 * periods) search for the (pre_div, pwm_div) split whose period best
   120		 * approximates the request while leaving pwm_div large enough to
   121		 * resolve the duty cycle.
   122		 */
 > 123		if (ipq_chip->clk_rate > 16ULL * GIGA)
   124			return -EINVAL;
   125		period_rate = period_ns * ipq_chip->clk_rate;
   126	
   127		best_pre_div = IPQ_PWM_MAX_DIV;
   128		best_pwm_div = IPQ_PWM_MAX_DIV;
   129		min_diff = period_rate;
   130	
   131		/*
   132		 * Smaller pre_div than this cannot represent the period (pwm_div would
   133		 * have to exceed its field), so start the search there.
   134		 */
   135		pre_div = div64_u64(period_rate,
   136				    (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));
   137	
   138		for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
   139			u64 remainder;
   140	
   141			pwm_div = div64_u64_rem(period_rate,
   142						(u64)NSEC_PER_SEC * (pre_div + 1),
   143						&remainder);
   144			/* pwm_div is unsigned; the swap check below catches underflow */
   145			pwm_div--;
   146	
   147			/*
   148			 * Swapping pre_div and pwm_div yields the same period but a
   149			 * larger pwm_div gives finer duty resolution, so once pre_div
   150			 * exceeds pwm_div every further candidate is strictly worse.
   151			 */
   152			if (pre_div > pwm_div)
   153				break;
   154	
   155			/* need room for 100% duty, where hi_dur == pwm_div + 1 */
   156			if (pwm_div > IPQ_PWM_MAX_DIV - 1)
   157				continue;
   158	
   159			if (remainder < min_diff) {
   160				best_pre_div = pre_div;
   161				best_pwm_div = pwm_div;
   162				min_diff = remainder;
   163	
   164				if (min_diff == 0)
   165					break;
   166			}
   167		}
   168	
   169		pre_div = best_pre_div;
   170		pwm_div = best_pwm_div;
   171	
   172		/*
   173		 * If the search found no usable candidate, best_pwm_div is left at
   174		 * IPQ_PWM_MAX_DIV; cap it so pwm_div + 1 still fits the 16-bit field
   175		 * and 100% duty remains expressible.
   176		 */
   177		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
   178			pwm_div = IPQ_PWM_MAX_DIV - 1;
   179	
   180		/*
   181		 * high duration = duty_ratio * (pwm_div + 1)
   182		 *              = duty_ns * clk_rate / ((pre_div + 1) * NSEC_PER_SEC)
   183		 *
   184		 * Round to nearest to avoid biasing every duty cycle low, then clamp
   185		 * to (pwm_div + 1): rounding or a 100% duty request can otherwise push
   186		 * hi_dur past the period length and overflow the 16-bit HI_DURATION field
   187		 * (which would alias a full-on request down to a near-zero high time)
   188		 * and asking the hardware to stay high beyond one period. pwm_div is
   189		 * at most IPQ_PWM_MAX_DIV - 1, so pwm_div + 1 always fits the field.
   190		 */
   191		hi_dur = DIV64_U64_ROUND_CLOSEST(duty_ns * ipq_chip->clk_rate,
   192						 (u64)(pre_div + 1) * NSEC_PER_SEC);
   193		if (hi_dur > (u64)pwm_div + 1)
   194			hi_dur = (u64)pwm_div + 1;
   195	
   196		val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
   197			FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
   198		ipq_pwm_reg_write(pwm, IPQ_PWM_REG0, val);
   199	
   200		val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
   201		ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
   202	
   203		/* PWM enable toggle needs a separate write to REG1 */
   204		val |= IPQ_PWM_REG1_UPDATE | IPQ_PWM_REG1_ENABLE;
   205		ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
   206	
   207		return 0;
   208	}
   209	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki