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

[Conor Dooley <conor.dooley@microchip.com>]

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

On Thu, Jan 12, 2023 at 01:01:41PM +0100, Uwe Kleine-König wrote:
> 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.

I stood in front of the whiteboard for a bit to puzzle it all out and
have come to the realisation that you are right. I was implicitly
converting in my head from "mathematical" values to register values &
therefore not subtracting. I must've hyperfocused on the underflow when
I adjusted your suggestion back in v5 or w/e it was.

I must also have got rather unlucky that the configurations I picked to
check whether the calculations worked out, happened to. I suppose as
well, the way in which it was mis-calculating also avoids the PWM_DEBUG
complaints too.

Perhaps I'll insert your nice formulae in the next version in comments,
so they're there for next time.

Thanks again & sorry for consuming so much of your time on this,
Conor.


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