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@xxxxxxxxxx> 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@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?
>
> 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