Re: [PATCH v19] pwm: airoha: Add support for EN7581 SoC

[Christian Marangi <ansuelsmth@gmail.com>]

On Wed, Jul 02, 2025 at 08:01:05AM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 01, 2025 at 09:20:24PM +0200, Christian Marangi wrote:
> > On Tue, Jul 01, 2025 at 09:40:03AM +0200, Uwe Kleine-König wrote:
> > > > +	shift = AIROHA_PWM_REG_CYCLE_CFG_SHIFT(shift);
> > > > +
> > > > +	/* Configure frequency divisor */
> > > > +	mask = AIROHA_PWM_WAVE_GEN_CYCLE << shift;
> > > > +	val = FIELD_PREP(AIROHA_PWM_WAVE_GEN_CYCLE, period_ticks) << shift;
> > > > +	ret = regmap_update_bits(pc->regmap, AIROHA_PWM_REG_CYCLE_CFG_VALUE(offset),
> > > > +				 mask, val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	offset = bucket;
> > > > +	shift = do_div(offset, AIROHA_PWM_BUCKET_PER_FLASH_PROD);
> > > > +	shift = AIROHA_PWM_REG_GPIO_FLASH_PRD_SHIFT(shift);
> > > > +
> > > > +	/* Configure duty cycle */
> > > > +	mask = AIROHA_PWM_GPIO_FLASH_PRD_HIGH << shift;
> > > > +	val = FIELD_PREP(AIROHA_PWM_GPIO_FLASH_PRD_HIGH, duty_ticks) << shift;
> > > > +	ret = regmap_update_bits(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> > > > +				 mask, val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	mask = AIROHA_PWM_GPIO_FLASH_PRD_LOW << shift;
> > > > +	val = FIELD_PREP(AIROHA_PWM_GPIO_FLASH_PRD_LOW,
> > > > +			 AIROHA_PWM_DUTY_FULL - duty_ticks) << shift;
> > > > +	return regmap_update_bits(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> > > > +				  mask, val);
> > > 
> > > Strange hardware, why do you have to configure both the high and the low
> > > relative duty? What happens if AIROHA_PWM_GPIO_FLASH_PRD_LOW +
> > > AIROHA_PWM_GPIO_FLASH_PRD_HIGH != AIROHA_PWM_DUTY_FULL?
> > 
> > From documentation it gets rejected and configured bucket doesn't work.
> 
> ok.
> 
> > > > [...]
> > > > +	/*
> > > > +	 * Duty is divided in 255 segment, normalize it to check if we
> > > > +	 * can share a generator.
> > > > +	 */
> > > > +	duty_ns = DIV_U64_ROUND_UP(duty_ns * AIROHA_PWM_DUTY_FULL,
> > > > +				   AIROHA_PWM_DUTY_FULL);
> > > 
> > > This looks bogus. This is just duty_ns = duty_ns, or what do I miss?
> > > Also duty_ns is an u32 and AIROHA_PWM_DUTY_FULL an int, so there is no
> > > need for a 64 bit division.
> > 
> > duty_ns * 255 goes beyond max u32.
> 
> In that case duty_ns * AIROHA_PWM_DUTY_FULL overflows to a smaller
> value. Just because the value then is used by DIV_U64_ROUND_UP doesn't
> fix the overflow. You need (u64)duty_ns * AIROHA_PWM_DUTY_FULL then.
> 
> > 225000000000.
> > 
> > Some revision ago it was asked to round also the duty_ns. And this is
> > really to round_up duty in 255 segment.
> 
> Yes, and I identified this as the code that intends to do that. Please
> double check this really works. I would claim you need:
> 
> 	duty_ns = DIV_ROUND_UP(duty_ns, AIROHA_PWM_DUTY_FULL) * AIROHA_PWM_DUTY_FULL;
> 
> here because no matter if you round up or down, dividing
> n * AIROHA_PWM_DUTY_FULL by AIROHA_PWM_DUTY_FULL yields n.
> 

Ok I made some test with a testing program to simulate various way to
normalize the value and yes you are right there is a problem here.

Also duty_ns = DIV_ROUND_UP(duty_ns, AIROHA_PWM_DUTY_FULL) *
AIROHA_PWM_DUTY_FULL; doesn't really fit here.

The solution I found is the following

DIV_U64_ROUND_UP(airoha_pwm_get_duty_ticks_from_ns(period, duty) * period, 255)

and airoha_pwm_get_duty_ticks_from_ns is (duty * 255 / period)

I also tested other viable way to reduce the redundant formula but the
main problem is that on big numbers (example when duty = period, too
many division error for integer division adds up (due to necessary
rounding) so we end up with not precise number that the tick actually
reflect or even goin beyond the period number (as duty must be <=
period)

But the thing is that since duty tick depends on period and now the
bucket base everything on the tick, I really feel normalizing duty is
not needed at all.

With the working normalize we would have 

duty_ns = DIV_U64_ROUND_UP(airoha_pwm_get_duty_ticks_from_ns(period, duty) * period, 255);

duty_tick = airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns) 

With the normalization already done by
airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns).

What do you think?

-- 
	Ansuel