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

On Mon, Jul 6, 2026 at 4:00 PM Uwe Kleine-König <[email protected]> wrote:
>
> Hello,
>
> On Fri, Jun 26, 2026 at 08:24:22PM +0800, Keguang Zhang via B4 Relay wrote:
> > From: Keguang Zhang <[email protected]>
> >
> > 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.
> >
> > Fixes: 2b62c89448dd ("pwm: Add Loongson PWM controller support")
> > Signed-off-by: Keguang Zhang <[email protected]>
> > ---
> >  drivers/pwm/pwm-loongson.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > index 31a57edecfd0..69514b5a1324 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,13 @@
> >  #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_LOW         GENMASK(31, 0)
> > +#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 +122,20 @@ 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;
> > +     /* low = (period_ns - duty_ns) * ddata->clk_rate / NSEC_PER_SEC */
> > +     low = mul_u64_u64_div_u64_roundup(period_ns - duty_ns, ddata->clk_rate, NSEC_PER_SEC);
>
> This is wrong. Consider clk_rate = 1234567 and the request duty_ns =
> 1234 [ns] and period_ns = 123456 [ns]
>
> Then you get
>
>         low = (123456 - 1234) * 1234567 // 1e9 = 150
>         period = 123456 * 1234567 // 1e9 = 152
>
> so the actual duty cycle is (152 - 150) * 1e9 / 1234567 =
> 1620.0011826008633 which is bigger than the requested value.
>
> The correct approach is
>
>         duty = mul_u64_u64_div_u64_roundup(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
>         period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC);
>
>         low = period - duty;
>
> which yields
>
>         duty = 1
>         period = 152 (as before)
>         low = 151
>
> .
>

Thanks for catching this. I'll update the calculation as you suggested
in the next version.

> > +     if ((!FIELD_FIT(LOONGSON_PWM_MAX_LOW, low)))
> > +             low = LOONGSON_PWM_MAX_LOW;
> >
> >       /* 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)))
> > +             period = LOONGSON_PWM_MAX_PERIOD;
> >
> > -     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 +170,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);
>
> What happens if low > period?

pwm_state_valid() in drivers/pwm/core.c ensures that duty_cycle <=
period before the framework calls the driver's .apply(). Since this
driver always programs low = period - duty, low cannot exceed period
when configured through the PWM framework.
>
> >       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;
>
> Best regards
> Uwe



-- 
Best regards,

Keguang Zhang