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

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

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

Hello Keguang,

On Wed, Jul 15, 2026 at 07:05:23PM +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 | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> index f2fb35b7af2b..e703217a6d5e 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)

Can you please make this a minimal fix and do the rework in a separate
patch? This way it a backport to stable is easier to motivate.

> +
>  /* 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,21 @@ 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, duty, 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)))
> +		period = LOONGSON_PWM_MAX_PERIOD;
>  
> -	pwm_loongson_writel(ddata, duty, LOONGSON_PWM_REG_DUTY);
> +	/* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
> +	duty = mul_u64_u64_div_u64_roundup(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
> +	low = period - duty;

This might overflow. I think in that case the right thing happens (at
least I failed to find an example that results in bogous settings), but
that might be worth to be handled (either by a comment explaining it or
explicitly limiting duty to be <= period first).

> +	if ((!FIELD_FIT(LOONGSON_PWM_MAX_LOW, low)))
> +		low = LOONGSON_PWM_MAX_LOW;
> +
> +	pwm_loongson_writel(ddata, low, LOONGSON_PWM_REG_LOW);
>  	pwm_loongson_writel(ddata, period, LOONGSON_PWM_REG_PERIOD);
>  
>  	return 0;
> @@ -166,15 +171,20 @@ 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 */

2^32-1 is the maximal value, right? (This was already wrong before.)

> +	if (low > period)
> +		state->duty_cycle = 0;
> +	else
> +		state->duty_cycle =
> +		    DIV64_U64_ROUND_UP((u64)(period - low) * NSEC_PER_SEC, ddata->clk_rate);
> +
>  	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;

You could do s/>/>=/ as a micro optimisation, but I don't care much.
Otherwise the adaption to pwm_loongson_get_state() looks fine.

Best regards
Uwe

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