Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

Hello,

On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> Hello Conor,
> 
> On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > +				      u16 *prescale, u8 *period_steps)
> > +{
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * Calculate the period cycles and prescale values.
> > +	 * The registers are each 8 bits wide & multiplied to compute the period
> > +	 * using the formula:
> > +	 * (clock_period) * (prescale + 1) * (period_steps + 1)
> > +	 * so the maximum period that can be generated is 0x10000 times the
> > +	 * period of the input clock.
> > +	 * However, due to the design of the "hardware", it is not possible to
> > +	 * attain a 100% duty cycle if the full range of period_steps is used.
> > +	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
> > +	 * of the clock period attainable is 0xFF00.
> > +	 */
> > +	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
> > +
> > +	/*
> > +	 * The hardware adds one to the register value, so decrement by one to
> > +	 * account for the offset
> > +	 */
> > +	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
> > +		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> > +		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> > +
> > +		return;
> > +	}
> > +
> > +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> > +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> > +	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> 
> This looks wrong, but I didn't think long about that. Did we discuss
> this already and/or are you sure this is correct?
> 
> (We have:
> 	          (prescale + 1) * (period_steps + 1)
> 	period = ------------------------------------
> 	                       clk_rate
> 

We want prescale small such that period_steps can be big to give
fine-grained control over the available duty_cycles. period_steps is a
8-bit value < 0xff, so we get:

                    period * clk_rate   
	prescale = ------------------- - 1
                   NSEC_PER_SEC * 0xff

which in code then reads:

	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX)
	if (*prescale)
		*prescale -= 1;


> You calculate
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff
> 
> 	                     period * clk_rate
> 	period_steps = ----------------------------- - 1
> 	               NSEC_PER_SEC * (prescale + 1)

The formula for period_steps is right.

Please double-check!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature