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

[Conor Dooley <conor@kernel.org>]

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

On Thu, Jan 12, 2023 at 03:10:09PM +0000, Conor Dooley wrote:
> 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.

*sigh*, me again...

Picked this up this afternoon to try and respin a v14 and ran into a bit
of an issue with this suggestion:

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

In theory this works out, but with integer division it doesn't AFAICT.

Take for example:
period_ns = 10,000
clk_rate = 62.5 MHz

The prescale calculation is:
(10000 * 62.5E6) / (1E9 * 255) - 1 = 1.451 -> 1

For period_steps then:
(10000 * 62.5E6) / (1E9 * (1 + 1)) - 1 = 311.5 -> 311

prescale is a u16, since it's valid for that division to return 256.
The register is only 8 bits wide though, so 311 overflows it and 55 ends
up in the register.
The period then is:
(55 + 1) * (1 + 1) / 62.5E6, or a period of 1792 ns.

I think it needs to be a div_u64_round_up(), which I know is not a real
function, otherwise we compute invalid values for period steps.

Quoting you previously:
> I thought a bit about that (without pencil and paper on my way to work
> today), and the optimal solution is non-trivial. In fact you have to
> pick parameters A and B such that for a given C
> 
> 	(A + 1) * (B + 1) is maximal with
> 
> 	0 <= A < 0x100
> 	0 <= B < 0xff
> 	(A + 1) * (B + 1) <= C

As we need (period_steps + 1) * (prescale + 1) maximal, *but* also want
to maximise period_steps so that duty resolution etc is maximal too.
We pick prescale, using the formula below, by setting period_steps to
the maximum possible value:
> 	            period * clk_rate
> 	prescale = -------------------
> 	           NSEC_PER_SEC * 0xff

Given we set period_steps to the max here, we only actually pick the
lower bound on prescale, not the actual value of it.
We must round it to the nearest possible value before using it to go
back and calculate period_steps.

With the previous code, missing the -= 1, the division result was purely
truncated:
(10000 * 62.5E6) / (1E9 * 255) = 2.451 -> 2
That 2 was written into the register, and then the hardware rounds that
up to 3, as does the period_steps calculation.

period_steps here is:
(10000 * 62.5E6) / (1E9 * (2 + 1)) - 1 = 207.833 -> 207

Finally, the period:
(207 + 1) * (2 + 1) / 62.5E6, or a period of 9984 ns.

This is the same operation, unless I am yet again overlooking something,
as doing a div_u64_round_up() type thing, followed by a subtraction of 1

We don't need to worry about writing something too big into the register
either, as we are protected against values of tmp that, when divided by
0xff, would produce values larger than 255:
> /*
>  * The hardware adds one to the register value, so decrement by one to
>  * account for the offset
>  */
> if (tmp >= MCHPCOREPWM_PERIOD_MAX) { (PERIOD_MAX is 0xff00)
> 	*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> 	*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> 
> 	return;
> }

What am I missing this time? haha

Thanks,
Conor.


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