Messages in this thread | | | Date | Wed, 11 Jan 2023 00:15:29 +0000 | From | Conor Dooley <> | Subject | Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver |
| |
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@microchip.com>
> > + 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@microchip.com/
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.
[unhandled content-type:application/pgp-signature] | |