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

[Uwe Kleine-König]

Hello Conor,

On Wed, Mar 22, 2023 at 10:49:40PM +0000, Conor Dooley wrote:
> On Wed, Mar 22, 2023 at 11:55:36AM +0100, Uwe Kleine-König wrote:
> > On Mon, Mar 06, 2023 at 09:48:58AM +0000, Conor Dooley wrote:
> > > Add a driver that supports the Microchip FPGA "soft" PWM IP core.
> 
> > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > +				      u16 *prescale, u16 *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:
> > > +	 *           (prescale + 1) * (period_steps + 1)
> > > +	 * period = -------------------------------------
> > > +	 *                      clk_rate
> > > +	 * 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;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The optimal value for prescale can be calculated using the maximum
> > > +	 * permitted value of period_steps, 0xff.
> > 
> > I had to think about that one for a while. The maximal value for
> > (period_steps + 1) is 0xff with the reasoning above?! That's also what
> > the code uses.
> 
> I've become really disenfranchised with these register/variable names.
> I feel like just changing them to disconnect the variables used for
> calculation from the register names a little, so that the "is there a +1
> needed here or not" stuff becomes a little clearer.

Full ack, I considered asking for that, but after some time I was in the
"I reviewed the patch today"-mode (which is quite similar to the mode
you described :-) and forgot. (Even in that mode the PREG_TO_VAL macro
annoyed me a bit.)

> It always makes sense to be when I am in an "I respun the patch today"
> mode, but by the time we're in the review stage I get muddled.
> God forbid I have to look at this in 10 years time.
> 
> That said, there is a bit of a mistake here. The comment two above says
> "Therefore period_steps is restricted to 0xFE" when I'm capping things
> off. Some inaccuracies have probably snuck in during the various
> respins, and I think the comment above is "correct" but misleading, as
> it muddies the waters about variable versus register names.

I think it's sensible to only talk about either the register values or
the factors. I tend to think that talking about the register values is
easier at the end and recommend not to hide the +1 (or -1) in a macro.

Having said that here are my results of thinking a bit about how to
choose register values:

Let r(p) denote the period that is actually configured if p is
requested.

The nice properties we want (i.e. those a consumer might expect?) are:

 a) ∀p: r(p) ≤ p
    i.e. never configure a bigger period than requested
    (This is a bit arbitrary, but nice to get a consistent behaviour for
    all drivers and easier to handle than requesting the nearest
    possible period.)

 b) ∀p, q: p ≤ q ⟹ r(p) ≤ r(q)
    i.e. monotonicity

 c) ∀p: r(roundup(r(p))) = r(p)
    i.e. idempotency

 d) ∀p, q: r(p) ≤ q ⟹ r(p) ≤ r(q)
    i.e. pick the biggest possible period

(Sidenote: d) and a) imply b) and c))

If you always set period_steps to 0xfe
(in Python syntax:

	tmp = p * clkrate // NSPS
	period_steps = 0xfe
	prescale = tmp // (period_steps + 1) - 1

) you get this consistent behaviour.

This has the upside of being easy to implement and cheap to run.
Downside is that it's coarse and fails to implement periods that would
require e.g prescale = 0 and period_steps < 0xfe. As period_steps is
big, the domain to chose the duty cycle from is good.

If you pick period_steps and prescale such that
	(period_steps + 1) * (prescale + 1)
is the maximal value that makes r(p) ≤ p you have an increased runtime
because you have to test multiple values for period_steps. I don't think
there is an algorithm without a loop to determine an optimal pair?!
Usually this gives a better match that the easy algorithm above for the
period, but the domain for duty_cycle is (usually) smaller.
I think we have a) and d) here, so that's good.

I think for most consumers a big domain for duty_cycle is more important
that a good match for the requested period. So I tend to recommend the
easy algorithm, but I'm not religious about that and open for other
opinion and reasoning.

> > > +	writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> > 
> > This one is just for the case where there is an unapplied configuration
> > in the registers, right?
> 
> No, this is me realising that I had a misconception about how that
> register works. You write the bit once, and it enables the mode for
> channels that have been configured that way at synthesis-time, rather
> than how I somehow thought it worked which was as a "flush" from the
> shadow registers into the "real" ones.

Then I think I got that wrong in the same way.  Should this be better
described in a comment then? (I didn't recheck, maybe there is a good
description already?)

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature