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

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello,

On Fri, Jun 26, 2026 at 08:24:22PM +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.
> 
> Fixes: 2b62c89448dd ("pwm: Add Loongson PWM controller support")
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
>  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

.

> +	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?

>  	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

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