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

[Conor Dooley]

Hey Uwe!

Was nice to meet you at FOSDEM, if I'd seen that presentation before
working on this driver I think the version count would have been
substantially lower!

On Fri, Jan 20, 2023 at 07:40:32PM +0000, Conor Dooley wrote:
> 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:
> > > On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:

> *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

So my "plan" I guess is to rebase this on v6.3-rc1 & re-submit with the
various minor bits fixed.
Trying the proposed "fixes" to the period calculation that we had
discussed pointed out the above issue, and it appears, to me at least,
that we made a mess of translating from regular calculations to integer
ones.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature