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

From: Conor Dooley
Date: Tue Jan 10 2023 - 19:16:14 EST

On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

> > + delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > + if ((delay_us / 1000) > MAX_UDELAY_MS)
> > + msleep(delay_us / 1000 + 1);
>
> Is this better than
>
> msleep(DIV_ROUND_UP(delay_us, 1000);
>
> ? Also I wonder about your usage of MAX_UDELAY_MS. This is about

I probably started hacking on the example you gave and didn't notice
the U. What I have here is ~what you suggested last time.

> udelay() but you're using usleep_range()?
>
> > + else
> > + usleep_range(delay_us, delay_us * 2);
>
> I wonder if there isn't a function that implements something like
>
> wait_until(mchp_core_pwm->update_timestamp);
>
> which would be a bit nicer than doing this by hand. Maybe fsleep()?

That'd be fsleep(delay_us), but does at least clean up some of the
messing.

> > +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state, u64 duty_steps,
> > + u8 period_steps)
> > +{
> > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> > + u8 posedge, negedge;
> > + u8 period_steps_val = PREG_TO_VAL(period_steps);
> > +
> > + /*
> > + * Setting posedge == negedge doesn't yield a constant output,
> > + * so that's an unsuitable setting to model duty_steps = 0.
> > + * In that case set the unwanted edge to a value that never
> > + * triggers.
> > + */
> > + if (state->polarity == PWM_POLARITY_INVERSED) {
> > + negedge = !duty_steps ? period_steps_val : 0u;
>
> IMHO
>
> negedge = duty_steps ? 0 : period_steps_val;
>
> is a bit easier to parse.
>
> > + posedge = duty_steps;
> > + } else {
> > + posedge = !duty_steps ? period_steps_val : 0u;
> > + negedge = duty_steps;
> > + }
>
> The following code is equivalent:
>
> u8 first_edge = 0, second_edge = duty_steps;
>
> /*
> * Setting posedge == negedge doesn't yield a constant output,
> * so that's an unsuitable setting to model duty_steps = 0.
> * In that case set the unwanted edge to a value that never
> * triggers.
> */
> if (duty_steps == 0)
> first_edge = period_steps_val;
>
> if (state->polarity == PWM_POLARITY_INVERSED) {
> negedge = first_edge;
> posedge = second_edge;
> } else {
> posedge = first_edge;
> negedge = second_edge;
> }
>
> I'm not sure if it's easier to understand. What do you think?

Despite having used them, I dislike ternary statements.

> > + writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > + writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > +}
> > +
> > +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 did discuss it previously AFAICT;
https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@xxxxxxxxxxxxx/

In that version of the code, prescale_val meant the mathematical value
used for calculations & "prescale" was the value written into the
register.

Now, we have ditched prescale_val and operate directly with what gets
written into the register.

I ran a test case through the calculation, and it seemed to work out?

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

Say period = 2000 ns, clk_rate = 62.5 Mhz, giving a register value for
prescale of 0.49019...

> period * clk_rate
> period_steps = ----------------------------- - 1
> NSEC_PER_SEC * (prescale + 1)

Same numbers, but we use the PREG_TO_VAL() macro so the mathematical value
is 1.49019.

2000 * 62.5E6
--------------------- - 1 = 82.88360....
1E9 * (0.49016 + 1)

>
> assuming exact arithmetic putting these into the above equation we get:
>
>
> period * clk_rate period * clk_rate
> (------------------- + 1) * (-----------------------------) / clk_rate
> NSEC_PER_SEC * 0xff NSEC_PER_SEC * (prescale + 1)
>
> and then substituting prescale this doesn't resolve to period, does it?
> Correct me if I'm wrong.)

(0.49016 + 1) * (82.88360 + 1) 124.99...
------------------------------ = ------------- = 0.00000199999
62.5E6 62.5E6

And then accounting for that fact that 2000 was really 2000E-9,
we arrive back where we started, give or take some rounding?

Doing that with integer maths works out more cleanly since 0.49016
becomes 0.
2000 * 62.5E6
------------------ - 1 = 124
1E9 * (0 + 1)

(0 + 1) * (124 + 1) 125
------------------- = --------- = 0.000002
62.5E6 62.5E6

Unfortunately, I don't think I am seeing what you're seeing.

> period * clk_rate period * clk_rate
> (------------------- + 1) * (-----------------------------) / clk_rate
> NSEC_PER_SEC * 0xff NSEC_PER_SEC * (prescale + 1)
^
It may be this + 1, which I don't seem to have accounted for in my quick
run through a calculation?

*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);

*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;

The code does not add a 1 when it calculates prescale, only when it uses
the result to calculate period_steps, since prescale & period_steps are
the register values, not the "mathematical" ones.

Hopefully I've not gone and made a fool of myself...

> > +static inline void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *mchp_core_pwm,
> > + u8 prescale, u8 period_steps)
> > +{
> > + writel_relaxed(prescale, mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
> > + writel_relaxed(period_steps, mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
> > +}
>
> There is only one caller for this two-line function. I suggest to unroll it?

Sure.

> > + ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
> > + if (ret < 0)
> > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > + /*
> > + * Enabled synchronous update for channels with shadow registers
> > + * enabled. For channels without shadow registers, this has no effect
> > + * at all so is unconditionally enabled.
> > + */
> > + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> > + mchp_core_pwm->update_timestamp = ktime_get();
>
> This needs to be done before devm_pwmchip_add().

Makes sense, woops. I think I've revised this to the point that my
blinkers have turned on & I'll wait a while before resubmitting in order
to hopefully reset that.

Perhaps I need to watch a lecture on how to write a PWM driver since I
am clearly no good at it, given the 15 revisions. Do you know of any?

Thanks,
Conor.

Attachment: signature.asc
Description: PGP signature