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

[Uwe Kleine-König]

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.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature