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@xxxxxxxxxx> wrote:
>
> On Tue, Jun 16, 2026 at 07:13:17PM +0800, Keguang Zhang via B4 Relay wrote:
> > From: Keguang Zhang <keguang.zhang@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >  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