Message-Id: <20250616-rockchip-pwm-rounding-fix-v2-1-a9c65acad7b6@collabora.com>
Date: Mon, 16 Jun 2025 17:14:17 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Uwe Kleine-König <ukleinek@...nel.org>, 
 Heiko Stuebner <heiko@...ech.de>, Brian Norris <briannorris@...omium.org>, 
 Boris Brezillon <bbrezillon@...nel.org>, 
 Thierry Reding <thierry.reding@...il.com>
Cc: kernel@...labora.com, linux-pwm@...r.kernel.org, 
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, 
 linux-kernel@...r.kernel.org, 
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Subject: [PATCH v2] pwm: rockchip: round period/duty down on apply, up on
 get

With CONFIG_PWM_DEBUG=y, the rockchip PWM driver produces warnings like
this:

  rockchip-pwm fd8b0010.pwm: .apply is supposed to round down
  duty_cycle (requested: 23529/50000, applied: 23542/50000)

This is because the driver chooses ROUND_CLOSEST for purported
idempotency reasons. However, it's possible to keep idempotency while
always rounding down in .apply.

Do this by making get_state always round up, and making apply always
round down. This is done with u64 maths, and setting both period and
duty to U32_MAX (the biggest the hardware can support) if they would
exceed their 32 bits confines.

Fixes: 12f9ce4a5198 ("pwm: rockchip: Fix period and duty cycle approximation")
Fixes: 1ebb74cf3537 ("pwm: rockchip: Add support for hardware readout")
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
---
This fix may need some careful testing from others before definitely
being applied and backported. While I did test it myself of course,
making sure to try a combination of periods and duty cycles, I really
don't want to accidentally undo someone else's fix.

Some of the u64 math is a bit overkill, but I don't want to assume
prescalers will never get larger than 4, which is where we start needing
the 64-bit prescaled NSECS_PER_SEC value. clk_rate could also
comfortably fit within u32 for any expected clock rate, but unsigned
long can fit more depending on architecture, even if nobody is running
the PWM hardware at 4.294967296 GHz.
---
Changes in v2:
- rename "period" and "duty" to "period_ticks" and "duty_ticks" as per
  ukleinek's review
- slightly modify commit message to add "purported", in order to make it
  clearer that ROUND_CLOSEST and idempotency are orthogonal things and
  that it worked out in this case was happenstance
- Link to v1: https://lore.kernel.org/r/20250522-rockchip-pwm-rounding-fix-v1-1-b516ad76a25a@collabora.com
---
 drivers/pwm/pwm-rockchip.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index c5f50e5eaf41ac7539f59fa03f427eee6263ca90..67b85bdb491b13cedb67c52de614f4ad9be427c5 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -8,6 +8,8 @@
 
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -61,6 +63,7 @@ static int rockchip_pwm_get_state(struct pwm_chip *chip,
 				  struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u64 prescaled_ns = (u64)pc->data->prescaler * NSEC_PER_SEC;
 	u32 enable_conf = pc->data->enable_conf;
 	unsigned long clk_rate;
 	u64 tmp;
@@ -78,12 +81,12 @@ static int rockchip_pwm_get_state(struct pwm_chip *chip,
 	clk_rate = clk_get_rate(pc->clk);
 
 	tmp = readl_relaxed(pc->base + pc->data->regs.period);
-	tmp *= pc->data->prescaler * NSEC_PER_SEC;
-	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+	tmp *= prescaled_ns;
+	state->period = DIV_U64_ROUND_UP(tmp, clk_rate);
 
 	tmp = readl_relaxed(pc->base + pc->data->regs.duty);
-	tmp *= pc->data->prescaler * NSEC_PER_SEC;
-	state->duty_cycle =  DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+	tmp *= prescaled_ns;
+	state->duty_cycle =  DIV_U64_ROUND_UP(tmp, clk_rate);
 
 	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	state->enabled = (val & enable_conf) == enable_conf;
@@ -103,8 +106,9 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       const struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	unsigned long period, duty;
-	u64 clk_rate, div;
+	u64 prescaled_ns = (u64)pc->data->prescaler * NSEC_PER_SEC;
+	u64 clk_rate, tmp;
+	u32 period_ticks, duty_ticks;
 	u32 ctrl;
 
 	clk_rate = clk_get_rate(pc->clk);
@@ -114,12 +118,15 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * bits, every possible input period can be obtained using the
 	 * default prescaler value for all practical clock rate values.
 	 */
-	div = clk_rate * state->period;
-	period = DIV_ROUND_CLOSEST_ULL(div,
-				       pc->data->prescaler * NSEC_PER_SEC);
+	tmp = mul_u64_u64_div_u64(clk_rate, state->period, prescaled_ns);
+	if (tmp > U32_MAX)
+		tmp = U32_MAX;
+	period_ticks = tmp;
 
-	div = clk_rate * state->duty_cycle;
-	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
+	tmp = mul_u64_u64_div_u64(clk_rate, state->duty_cycle, prescaled_ns);
+	if (tmp > U32_MAX)
+		tmp = U32_MAX;
+	duty_ticks = tmp;
 
 	/*
 	 * Lock the period and duty of previous configuration, then
@@ -131,8 +138,8 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		writel_relaxed(ctrl, pc->base + pc->data->regs.ctrl);
 	}
 
-	writel(period, pc->base + pc->data->regs.period);
-	writel(duty, pc->base + pc->data->regs.duty);
+	writel(period_ticks, pc->base + pc->data->regs.period);
+	writel(duty_ticks, pc->base + pc->data->regs.duty);
 
 	if (pc->data->supports_polarity) {
 		ctrl &= ~PWM_POLARITY_MASK;

---
base-commit: 67a08d8299798b4748f0194002566abc14c0cb23
change-id: 20250522-rockchip-pwm-rounding-fix-98a360c90e81

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@...labora.com>

Powered by blists - more mailing lists