[PATCH 0/2] pwm: loongson: Fix PWM configuration handling

[PATCH 0/2] pwm: loongson: Fix PWM configuration handling

[Keguang Zhang via B4 Relay]

This series fixes two issues in the Loongson PWM driver:

- Fix the handling of the Low Pulse Buffer Register, which was
  incorrectly treated as a duty-cycle register.

- Ensure that PWM configuration updates take effect immediately by
  resetting the PWM counter to reload the updated settings.

The fixes were verified on LS1B, LS1C, and LS2K0300 boards.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
Keguang Zhang (2):
      pwm: loongson: Fix low pulse buffer register handling
      pwm: loongson: Reload PWM configuration through counter reset

 drivers/pwm/pwm-loongson.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)
---
base-commit: 8d6dbbbe3ba62de0a63e962ee004afb848c8e3ac
change-id: 20260612-pwm-loongson-fix-183763451e93

Best regards,
-- 
Keguang Zhang <keguang.zhang@gmail.com>

[PATCH 1/2] pwm: loongson: Fix low pulse buffer register handling

[Keguang Zhang via B4 Relay]

From: Keguang Zhang <keguang.zhang@gmail.com>

The Loongson PWM register at offset 0x4 is documented as the Low
Pulse Buffer Register, which stores the low pulse width rather than
the duty cycle.

However, this register was incorrectly defined and treated as a
duty-cycle register. As a result, the duty cycle and low pulse cycle
are swapped in the generated PWM waveform.

Program the low pulse (period - duty) into the register and
adjust pwm_loongson_get_state() accordingly when reconstructing the
duty cycle.

Also return -ERANGE when the requested period exceeds the hardware
32-bit limit instead of silently truncating the value.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
 drivers/pwm/pwm-loongson.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
index 31a57edecfd0..dc77f82fd888 100644
--- a/drivers/pwm/pwm-loongson.c
+++ b/drivers/pwm/pwm-loongson.c
@@ -22,6 +22,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/init.h>
@@ -33,10 +34,12 @@
 #include <linux/units.h>
 
 /* Loongson PWM registers */
-#define LOONGSON_PWM_REG_DUTY		0x4 /* Low Pulse Buffer Register */
+#define LOONGSON_PWM_REG_LOW		0x4 /* Low Pulse Buffer Register */
 #define LOONGSON_PWM_REG_PERIOD		0x8 /* Pulse Period Buffer Register */
 #define LOONGSON_PWM_REG_CTRL		0xc /* Control Register */
 
+#define LOONGSON_PWM_MAX_PERIOD		GENMASK(31, 0)
+
 /* Control register bits */
 #define LOONGSON_PWM_CTRL_REG_EN	BIT(0)  /* Counter Enable Bit */
 #define LOONGSON_PWM_CTRL_REG_OE	BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
@@ -118,20 +121,16 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       u64 duty_ns, u64 period_ns)
 {
-	u64 duty, period;
+	u64 low, period;
 	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
 
-	/* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
-	duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
-	if (duty > U32_MAX)
-		duty = U32_MAX;
-
-	/* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
 	period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
-	if (period > U32_MAX)
-		period = U32_MAX;
+	if ((!FIELD_FIT(LOONGSON_PWM_MAX_PERIOD, period)))
+		return -ERANGE;
+
+	low = mul_u64_u64_div_u64(period_ns - duty_ns, ddata->clk_rate, NSEC_PER_SEC);
 
-	pwm_loongson_writel(ddata, duty, LOONGSON_PWM_REG_DUTY);
+	pwm_loongson_writel(ddata, low, LOONGSON_PWM_REG_LOW);
 	pwm_loongson_writel(ddata, period, LOONGSON_PWM_REG_PERIOD);
 
 	return 0;
@@ -166,15 +165,15 @@ static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				  struct pwm_state *state)
 {
-	u32 duty, period, ctrl;
+	u32 low, period, ctrl;
 	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
 
-	duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY);
+	low = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_LOW);
 	period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD);
 	ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL);
 
-	/* duty & period have a max of 2^32, so we can't overflow */
-	state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty * NSEC_PER_SEC, ddata->clk_rate);
+	/* low & period have a max of 2^32, so we can't overflow */
+	state->duty_cycle = DIV64_U64_ROUND_UP((u64)(period - low) * NSEC_PER_SEC, ddata->clk_rate);
 	state->period = DIV64_U64_ROUND_UP((u64)period * NSEC_PER_SEC, ddata->clk_rate);
 	state->polarity = (ctrl & LOONGSON_PWM_CTRL_REG_INVERT) ? PWM_POLARITY_INVERSED :
 			  PWM_POLARITY_NORMAL;

-- 
2.43.0

[PATCH 2/2] pwm: loongson: Reload PWM configuration through counter reset

[Keguang Zhang via B4 Relay]

From: Keguang Zhang <keguang.zhang@gmail.com>

By default, the Loongson PWM controller latches the LOW and PERIOD
registers only at the start of each PWM period, causing configuration
updates to be delayed until the next period.

Reset the PWM counter when disabling the PWM and release it when enabling
the PWM to force the controller to re-latch the updated LOW and PERIOD
values, allowing configuration changes to take effect immediately.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
 drivers/pwm/pwm-loongson.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
index dc77f82fd888..eb89ced530e1 100644
--- a/drivers/pwm/pwm-loongson.c
+++ b/drivers/pwm/pwm-loongson.c
@@ -102,6 +102,7 @@ static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
 
 	val = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL);
+	val |= LOONGSON_PWM_CTRL_REG_RST;
 	val &= ~LOONGSON_PWM_CTRL_REG_EN;
 	pwm_loongson_writel(ddata, val, LOONGSON_PWM_REG_CTRL);
 }
@@ -112,6 +113,7 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
 
 	val = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL);
+	val &= ~LOONGSON_PWM_CTRL_REG_RST;
 	val |= LOONGSON_PWM_CTRL_REG_EN;
 	pwm_loongson_writel(ddata, val, LOONGSON_PWM_REG_CTRL);
 

-- 
2.43.0

Re: [PATCH 1/2] pwm: loongson: Fix low pulse buffer register handling

[Uwe Kleine-König]

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

On Tue, Jun 16, 2026 at 07:13:17PM +0800, Keguang Zhang via B4 Relay wrote:
> From: Keguang Zhang <keguang.zhang@gmail.com>
> 
> The Loongson PWM register at offset 0x4 is documented as the Low
> Pulse Buffer Register, which stores the low pulse width rather than
> the duty cycle.
> 
> However, this register was incorrectly defined and treated as a
> duty-cycle register. As a result, the duty cycle and low pulse cycle
> are swapped in the generated PWM waveform.
> 
> Program the low pulse (period - duty) into the register and
> adjust pwm_loongson_get_state() accordingly when reconstructing the
> duty cycle.
> 
> Also return -ERANGE when the requested period exceeds the hardware
> 32-bit limit instead of silently truncating the value.

This is the intended behaviour.

> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
>  drivers/pwm/pwm-loongson.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> index 31a57edecfd0..dc77f82fd888 100644
> --- a/drivers/pwm/pwm-loongson.c
> +++ b/drivers/pwm/pwm-loongson.c
> @@ -22,6 +22,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> @@ -33,10 +34,12 @@
>  #include <linux/units.h>
>  
>  /* Loongson PWM registers */
> -#define LOONGSON_PWM_REG_DUTY		0x4 /* Low Pulse Buffer Register */
> +#define LOONGSON_PWM_REG_LOW		0x4 /* Low Pulse Buffer Register */
>  #define LOONGSON_PWM_REG_PERIOD		0x8 /* Pulse Period Buffer Register */
>  #define LOONGSON_PWM_REG_CTRL		0xc /* Control Register */
>  
> +#define LOONGSON_PWM_MAX_PERIOD		GENMASK(31, 0)
> +
>  /* Control register bits */
>  #define LOONGSON_PWM_CTRL_REG_EN	BIT(0)  /* Counter Enable Bit */
>  #define LOONGSON_PWM_CTRL_REG_OE	BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> @@ -118,20 +121,16 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			       u64 duty_ns, u64 period_ns)
>  {
> -	u64 duty, period;
> +	u64 low, period;
>  	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
>  
> -	/* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
> -	duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
> -	if (duty > U32_MAX)
> -		duty = U32_MAX;
> -
> -	/* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
>  	period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
> -	if (period > U32_MAX)
> -		period = U32_MAX;
> +	if ((!FIELD_FIT(LOONGSON_PWM_MAX_PERIOD, period)))
> +		return -ERANGE;

As noted above, this is wrong. If period is too big, you're supposed to
pick the biggest possible period and not return an error.

> +
> +	low = mul_u64_u64_div_u64(period_ns - duty_ns, ddata->clk_rate, NSEC_PER_SEC);

this is also wrong. You're supposed to pick a configuration where the
duty is the biggest not bigger than the requested value. However as
mul_u64_u64_div_u64 rounds down, you're rounding in the wrong direction.

The register layout suggests that the period starts with the inactive
part, did you reverify that?

Best regards
Uwe

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

Re: [PATCH 2/2] pwm: loongson: Reload PWM configuration through counter reset

[Uwe Kleine-König]

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

Hello,

On Tue, Jun 16, 2026 at 07:13:18PM +0800, Keguang Zhang via B4 Relay wrote:
> From: Keguang Zhang <keguang.zhang@gmail.com>
> 
> By default, the Loongson PWM controller latches the LOW and PERIOD
> registers only at the start of each PWM period, causing configuration
> updates to be delayed until the next period.
>
> Reset the PWM counter when disabling the PWM and release it when enabling
> the PWM to force the controller to re-latch the updated LOW and PERIOD
> values, allowing configuration changes to take effect immediately.

To get this right, so if the hardware runs with PERIOD = 17 and then is
disabled with say the counter at 4, and later restarted with PERIOD =
12, the counter first goes up to 17 before the 12 becomes active, right? 

I think the commit log is a bit irritating, because (IIUC) you only
change the behaviour when the PWM is disabled and then reenabled. If the
configuration is changed while the PWM is running, the effect is still
delayed until the current period ends (which is complete fine and even
preferred).

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: loongson: Fix low pulse buffer register handling

[Keguang Zhang]

On Thu, Jun 18, 2026 at 12:04 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
>
> On Tue, Jun 16, 2026 at 07:13:17PM +0800, Keguang Zhang via B4 Relay wrote:
> > From: Keguang Zhang <keguang.zhang@gmail.com>
> >
> > The Loongson PWM register at offset 0x4 is documented as the Low
> > Pulse Buffer Register, which stores the low pulse width rather than
> > the duty cycle.
> >
> > However, this register was incorrectly defined and treated as a
> > duty-cycle register. As a result, the duty cycle and low pulse cycle
> > are swapped in the generated PWM waveform.
> >
> > Program the low pulse (period - duty) into the register and
> > adjust pwm_loongson_get_state() accordingly when reconstructing the
> > duty cycle.
> >
> > Also return -ERANGE when the requested period exceeds the hardware
> > 32-bit limit instead of silently truncating the value.
>
> This is the intended behaviour.
>
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> >  drivers/pwm/pwm-loongson.c | 29 ++++++++++++++---------------
> >  1 file changed, 14 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > index 31a57edecfd0..dc77f82fd888 100644
> > --- a/drivers/pwm/pwm-loongson.c
> > +++ b/drivers/pwm/pwm-loongson.c
> > @@ -22,6 +22,7 @@
> >   */
> >
> >  #include <linux/acpi.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/device.h>
> >  #include <linux/init.h>
> > @@ -33,10 +34,12 @@
> >  #include <linux/units.h>
> >
> >  /* Loongson PWM registers */
> > -#define LOONGSON_PWM_REG_DUTY                0x4 /* Low Pulse Buffer Register */
> > +#define LOONGSON_PWM_REG_LOW         0x4 /* Low Pulse Buffer Register */
> >  #define LOONGSON_PWM_REG_PERIOD              0x8 /* Pulse Period Buffer Register */
> >  #define LOONGSON_PWM_REG_CTRL                0xc /* Control Register */
> >
> > +#define LOONGSON_PWM_MAX_PERIOD              GENMASK(31, 0)
> > +
> >  /* Control register bits */
> >  #define LOONGSON_PWM_CTRL_REG_EN     BIT(0)  /* Counter Enable Bit */
> >  #define LOONGSON_PWM_CTRL_REG_OE     BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> > @@ -118,20 +121,16 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >                              u64 duty_ns, u64 period_ns)
> >  {
> > -     u64 duty, period;
> > +     u64 low, period;
> >       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> >
> > -     /* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
> > -     duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
> > -     if (duty > U32_MAX)
> > -             duty = U32_MAX;
> > -
> > -     /* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
> >       period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
> > -     if (period > U32_MAX)
> > -             period = U32_MAX;
> > +     if ((!FIELD_FIT(LOONGSON_PWM_MAX_PERIOD, period)))
> > +             return -ERANGE;
>
> As noted above, this is wrong. If period is too big, you're supposed to
> pick the biggest possible period and not return an error.
>

Understood. However, in this case the difference between the requested
and actual
period can become quite significant. Returning success means userspace
may assume the requested configuration was applied while the generated
waveform is substantially different.

Wouldn't returning an error be preferable in such cases, as it makes the
hardware limitation visible to userspace instead of silently applying a
different configuration?

> > +
> > +     low = mul_u64_u64_div_u64(period_ns - duty_ns, ddata->clk_rate, NSEC_PER_SEC);
>
> this is also wrong. You're supposed to pick a configuration where the
> duty is the biggest not bigger than the requested value. However as
> mul_u64_u64_div_u64 rounds down, you're rounding in the wrong direction.
>
Thanks for pointing this out.
I'll change it to mul_u64_u64_div_u64_roundup().

> The register layout suggests that the period starts with the inactive
> part, did you reverify that?
>
Yes, I reverified it on hardware.

The PWM period indeed starts with the inactive (low) phase. However,
with the current driver implementation, configuring a duty_cycle
results in a low phase width of duty_cycle and an active phase width of
(period - duty_cycle), meaning the requested duty_cycle is interpreted
as the inactive phase width rather than the active phase width.

> Best regards
> Uwe



--
Best regards,

Keguang Zhang

Re: [PATCH 2/2] pwm: loongson: Reload PWM configuration through counter reset

[Keguang Zhang]

On Thu, Jun 18, 2026 at 12:11 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
>
> Hello,
>
> On Tue, Jun 16, 2026 at 07:13:18PM +0800, Keguang Zhang via B4 Relay wrote:
> > From: Keguang Zhang <keguang.zhang@gmail.com>
> >
> > By default, the Loongson PWM controller latches the LOW and PERIOD
> > registers only at the start of each PWM period, causing configuration
> > updates to be delayed until the next period.
> >
> > Reset the PWM counter when disabling the PWM and release it when enabling
> > the PWM to force the controller to re-latch the updated LOW and PERIOD
> > values, allowing configuration changes to take effect immediately.
>
> To get this right, so if the hardware runs with PERIOD = 17 and then is
> disabled with say the counter at 4, and later restarted with PERIOD =
> 12, the counter first goes up to 17 before the 12 becomes active, right?
>
Yes. This is exactly what happens when updating the configuration
without setting LOONGSON_PWM_CTRL_REG_RST.

Even with disabling and re-enabling the PWM, the counter still resumes
from the previous value and completes the current period. The only
difference is that the counter is paused while the PWM is disabled.

Therefore, setting LOONGSON_PWM_CTRL_REG_RST when disabling the PWM
forces the PWM controller to re-latch the updated LOW and PERIOD values,
allowing the updated configuration to take effect immediately when the
PWM is re-enabled.

> I think the commit log is a bit irritating, because (IIUC) you only
> change the behaviour when the PWM is disabled and then reenabled. If the
> configuration is changed while the PWM is running, the effect is still
> delayed until the current period ends (which is complete fine and even
> preferred).
>
That's exactly what I mean.
Sorry for the misleading commit log. I'll clarify it in the next version.

> Best regards
> Uwe



-- 
Best regards,

Keguang Zhang

Re: [PATCH 1/2] pwm: loongson: Fix low pulse buffer register handling

[Uwe Kleine-König]

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

On Sat, Jun 20, 2026 at 04:22:55PM +0800, Keguang Zhang wrote:
> On Thu, Jun 18, 2026 at 12:04 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> >
> > On Tue, Jun 16, 2026 at 07:13:17PM +0800, Keguang Zhang via B4 Relay wrote:
> > > From: Keguang Zhang <keguang.zhang@gmail.com>
> > >
> > > The Loongson PWM register at offset 0x4 is documented as the Low
> > > Pulse Buffer Register, which stores the low pulse width rather than
> > > the duty cycle.
> > >
> > > However, this register was incorrectly defined and treated as a
> > > duty-cycle register. As a result, the duty cycle and low pulse cycle
> > > are swapped in the generated PWM waveform.
> > >
> > > Program the low pulse (period - duty) into the register and
> > > adjust pwm_loongson_get_state() accordingly when reconstructing the
> > > duty cycle.
> > >
> > > Also return -ERANGE when the requested period exceeds the hardware
> > > 32-bit limit instead of silently truncating the value.
> >
> > This is the intended behaviour.
> >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > ---
> > >  drivers/pwm/pwm-loongson.c | 29 ++++++++++++++---------------
> > >  1 file changed, 14 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > > index 31a57edecfd0..dc77f82fd888 100644
> > > --- a/drivers/pwm/pwm-loongson.c
> > > +++ b/drivers/pwm/pwm-loongson.c
> > > @@ -22,6 +22,7 @@
> > >   */
> > >
> > >  #include <linux/acpi.h>
> > > +#include <linux/bitfield.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/device.h>
> > >  #include <linux/init.h>
> > > @@ -33,10 +34,12 @@
> > >  #include <linux/units.h>
> > >
> > >  /* Loongson PWM registers */
> > > -#define LOONGSON_PWM_REG_DUTY                0x4 /* Low Pulse Buffer Register */
> > > +#define LOONGSON_PWM_REG_LOW         0x4 /* Low Pulse Buffer Register */
> > >  #define LOONGSON_PWM_REG_PERIOD              0x8 /* Pulse Period Buffer Register */
> > >  #define LOONGSON_PWM_REG_CTRL                0xc /* Control Register */
> > >
> > > +#define LOONGSON_PWM_MAX_PERIOD              GENMASK(31, 0)
> > > +
> > >  /* Control register bits */
> > >  #define LOONGSON_PWM_CTRL_REG_EN     BIT(0)  /* Counter Enable Bit */
> > >  #define LOONGSON_PWM_CTRL_REG_OE     BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> > > @@ -118,20 +121,16 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >                              u64 duty_ns, u64 period_ns)
> > >  {
> > > -     u64 duty, period;
> > > +     u64 low, period;
> > >       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > >
> > > -     /* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
> > > -     duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
> > > -     if (duty > U32_MAX)
> > > -             duty = U32_MAX;
> > > -
> > > -     /* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
> > >       period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
> > > -     if (period > U32_MAX)
> > > -             period = U32_MAX;
> > > +     if ((!FIELD_FIT(LOONGSON_PWM_MAX_PERIOD, period)))
> > > +             return -ERANGE;
> >
> > As noted above, this is wrong. If period is too big, you're supposed to
> > pick the biggest possible period and not return an error.
> >
> 
> Understood. However, in this case the difference between the requested
> and actual
> period can become quite significant. Returning success means userspace
> may assume the requested configuration was applied while the generated
> waveform is substantially different.
> 
> Wouldn't returning an error be preferable in such cases, as it makes the
> hardware limitation visible to userspace instead of silently applying a
> different configuration?

That sounds good, but doesn't work consistently and without surprises.

Assuming a clk_rate of 24 MHz the maximal period is

	0xffffffff / (24000000 Hz) = 178956970625 ns

.

Then with your suggestion implemented (as I understand it) the mapping
from requested period to actual period looks as follows::

	178956970624 ns -> 178956970583.33334 ns
	178956970625 ns -> 178956970625 ns
	...
	178956970666 ns -> 178956970625 ns
	178956970667 ns -> -ERANGE

From the consumer's point of view, how do you motivate that 178956970666
still works while 178956970667 doesn't? IMHO the only consequent
implementation then is to let 178956970626 already return an error.
But then it's surprising again, that when requesting 178956970624 ns
you get 178956970583.33334 ns on loongson, while for a hardware that has
178956970583.33334 ns as its maximal period, it's an error.

That consideration convinced me back then to not return an error for too
high values, which is the only sane option left then (unless I missed
something?)

With the waveform API you can query the capabilities and thus prevent
consumer surprises.

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: loongson: Fix low pulse buffer register handling

[Keguang Zhang]

On Mon, Jun 22, 2026 at 9:58 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
>
> On Sat, Jun 20, 2026 at 04:22:55PM +0800, Keguang Zhang wrote:
> > On Thu, Jun 18, 2026 at 12:04 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> > >
> > > On Tue, Jun 16, 2026 at 07:13:17PM +0800, Keguang Zhang via B4 Relay wrote:
> > > > From: Keguang Zhang <keguang.zhang@gmail.com>
> > > >
> > > > The Loongson PWM register at offset 0x4 is documented as the Low
> > > > Pulse Buffer Register, which stores the low pulse width rather than
> > > > the duty cycle.
> > > >
> > > > However, this register was incorrectly defined and treated as a
> > > > duty-cycle register. As a result, the duty cycle and low pulse cycle
> > > > are swapped in the generated PWM waveform.
> > > >
> > > > Program the low pulse (period - duty) into the register and
> > > > adjust pwm_loongson_get_state() accordingly when reconstructing the
> > > > duty cycle.
> > > >
> > > > Also return -ERANGE when the requested period exceeds the hardware
> > > > 32-bit limit instead of silently truncating the value.
> > >
> > > This is the intended behaviour.
> > >
> > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > > ---
> > > >  drivers/pwm/pwm-loongson.c | 29 ++++++++++++++---------------
> > > >  1 file changed, 14 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > > > index 31a57edecfd0..dc77f82fd888 100644
> > > > --- a/drivers/pwm/pwm-loongson.c
> > > > +++ b/drivers/pwm/pwm-loongson.c
> > > > @@ -22,6 +22,7 @@
> > > >   */
> > > >
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/bitfield.h>
> > > >  #include <linux/clk.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/init.h>
> > > > @@ -33,10 +34,12 @@
> > > >  #include <linux/units.h>
> > > >
> > > >  /* Loongson PWM registers */
> > > > -#define LOONGSON_PWM_REG_DUTY                0x4 /* Low Pulse Buffer Register */
> > > > +#define LOONGSON_PWM_REG_LOW         0x4 /* Low Pulse Buffer Register */
> > > >  #define LOONGSON_PWM_REG_PERIOD              0x8 /* Pulse Period Buffer Register */
> > > >  #define LOONGSON_PWM_REG_CTRL                0xc /* Control Register */
> > > >
> > > > +#define LOONGSON_PWM_MAX_PERIOD              GENMASK(31, 0)
> > > > +
> > > >  /* Control register bits */
> > > >  #define LOONGSON_PWM_CTRL_REG_EN     BIT(0)  /* Counter Enable Bit */
> > > >  #define LOONGSON_PWM_CTRL_REG_OE     BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> > > > @@ -118,20 +121,16 @@ static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > >  static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >                              u64 duty_ns, u64 period_ns)
> > > >  {
> > > > -     u64 duty, period;
> > > > +     u64 low, period;
> > > >       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > > >
> > > > -     /* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
> > > > -     duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
> > > > -     if (duty > U32_MAX)
> > > > -             duty = U32_MAX;
> > > > -
> > > > -     /* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
> > > >       period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
> > > > -     if (period > U32_MAX)
> > > > -             period = U32_MAX;
> > > > +     if ((!FIELD_FIT(LOONGSON_PWM_MAX_PERIOD, period)))
> > > > +             return -ERANGE;
> > >
> > > As noted above, this is wrong. If period is too big, you're supposed to
> > > pick the biggest possible period and not return an error.
> > >
> >
> > Understood. However, in this case the difference between the requested
> > and actual
> > period can become quite significant. Returning success means userspace
> > may assume the requested configuration was applied while the generated
> > waveform is substantially different.
> >
> > Wouldn't returning an error be preferable in such cases, as it makes the
> > hardware limitation visible to userspace instead of silently applying a
> > different configuration?
>
> That sounds good, but doesn't work consistently and without surprises.
>
> Assuming a clk_rate of 24 MHz the maximal period is
>
>         0xffffffff / (24000000 Hz) = 178956970625 ns
>
> .
>
> Then with your suggestion implemented (as I understand it) the mapping
> from requested period to actual period looks as follows::
>
>         178956970624 ns -> 178956970583.33334 ns
>         178956970625 ns -> 178956970625 ns
>         ...
>         178956970666 ns -> 178956970625 ns
>         178956970667 ns -> -ERANGE
>
> From the consumer's point of view, how do you motivate that 178956970666
> still works while 178956970667 doesn't? IMHO the only consequent
> implementation then is to let 178956970626 already return an error.
> But then it's surprising again, that when requesting 178956970624 ns
> you get 178956970583.33334 ns on loongson, while for a hardware that has
> 178956970583.33334 ns as its maximal period, it's an error.
>
> That consideration convinced me back then to not return an error for too
> high values, which is the only sane option left then (unless I missed
> something?)
>
> With the waveform API you can query the capabilities and thus prevent
> consumer surprises.
>
Thanks for the detailed explanation.

I will drop the `-ERANGE` change and restore the original behavior
in the next version.

> Best regards
> Uwe



-- 
Best regards,

Keguang Zhang