On 02/08/2022 09:46, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Jul 21, 2022 at 06:21:09PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>
>> Add a driver that supports the Microchip FPGA "soft" PWM IP core.
>>
>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>> ---
>> +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
>> + bool enable, u64 period)
>> +{
>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> + u8 channel_enable, reg_offset, shift;
>> +
>> + /*
>> + * There are two adjacent 8 bit control regs, the lower reg controls
>> + * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg
>> + * and if so, offset by the bus width.
>> + */
>> + reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3);
>> + shift = pwm->hwpwm & 7;
>> +
>> + channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset);
>> + channel_enable &= ~(1 << shift);
>> + channel_enable |= (enable << shift);
>> +
>> + writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset);
>> + mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm);
>> + mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm;
>> +
>> + /*
>> + * Notify the block to update the waveform from the shadow registers.
>> + * The updated values will not appear on the bus until they have been
>> + * applied to the waveform at the beginning of the next period. We must
>> + * write these registers and wait for them to be applied before calling
>> + * enable().
>
> What does "calling enable()" mean? There is no such function or callback
> with that name?!
I relocated the comment but forgot to proof read it!
s/calling enable()/considering the channel enabled/
I'm not sure where it comes from, but I keep thinking that there is an
enable() callback...
>
>> + */
>> + if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
>> + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
>> + usleep_range(period, period * 2);
>
> So if period = 5000 *ns* you sleep between 5000 and 10000 *us* here?
/facepalm
>
>> + }
>> +}
>> +
>> +static u64 mchp_core_pwm_calc_duty(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state, u8 prescale, u8 period_steps)
>> +{
>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> + u64 duty_steps, period, tmp;
>> + u16 prescale_val = PREG_TO_VAL(prescale);
>> + u8 period_steps_val = PREG_TO_VAL(period_steps);
>
> Can it happen that period_steps is 0xff? Then period_steps_val ends up
> being 0.
I guess that register could have a value it in from the bootloader etc and
therefore handling it is a good idea - but not in this function since it
is never used...
>
>> +
>> + period = period_steps_val * prescale_val * NSEC_PER_SEC;
>> + period = DIV64_U64_ROUND_UP(period, clk_get_rate(mchp_core_pwm->clk));
>
> The value you are calculating for period isn't used?!
huh, I am surprised that this was not caught by a W=1 C=1 build. Or maybe it
was and I just didn't notice - but I am 99% sure I made sure there were none.
>
>> +
>> + /*
>> + * Calculate the duty cycle in multiples of the prescaled period:
>> + * duty_steps = duty_in_ns / step_in_ns
>> + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate
>> + * The code below is rearranged slightly to only divide once.
>> + */
>> + duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
>> + tmp = prescale_val * NSEC_PER_SEC;
>> + return div64_u64(duty_steps, tmp);
>> +}
>> +
>> +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);
>> +
>> + /*
>> + * Turn the output on unless posedge == negedge, in which case the
>> + * duty is intended to be 0, but limitations of the IP block don't
>> + * allow a zero length duty cycle - so just set the max high/low time
>> + * respectively.
>> + */
>
> I don't understand that comment. Maybe you mean?:
>
> /*
> * 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.
> */
Yeah, this is a better comment. Thanks.
>
>> + if (state->polarity == PWM_POLARITY_INVERSED) {
>> + negedge = !duty_steps ? period_steps_val : 0u;
>> + posedge = duty_steps;
>> + } else {
>> + posedge = !duty_steps ? period_steps_val : 0u;
>> + negedge = duty_steps;
>> + }
>> +
>> + writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
>> + writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
>> +}
>> +
>> +static int mchp_core_pwm_calc_period(struct pwm_chip *chip, const struct pwm_state *state,
>> + u8 *prescale, u8 *period_steps)
>> +{
>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> + u64 tmp, clk_rate;
>> +
>> + /*
>> + * 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.
>> + */
>> + clk_rate = clk_get_rate(mchp_core_pwm->clk);
>> +
>> + /*
>> + * If clk_rate is too big, the following multiplication might overflow.
>> + * However this is implausible, as the fabric of current FPGAs cannot
>> + * provide clocks at a rate high enough.
>> + */
>> + if (clk_rate >= NSEC_PER_SEC)
>> + return -EINVAL;
>> +
>> + tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
>> +
>> + if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
>> + *prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
>
> why -1 here?
Because the hardware adds 1 to the register value. I had tried to explain
in the large comment above, but I will reword the comment for v8.
>
>> + *period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
>> + return 0;
>> + }
>> +
>> + *prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
>> + /* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
>
> That should explain the cast to u32? If this were really necessary
> (hint: it isn't) it would IMHO be better to hide that cast in the macro.
>
>> + *period_steps = div_u64(tmp, PREG_TO_VAL((u32)*prescale)) - 1;
>> +
>> + return 0;
>> +}
>> +
>> +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);
>> +}
>> +
>> +static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> + struct pwm_state current_state = pwm->state;
>> + bool period_locked;
>> + u64 duty_steps;
>> + u8 prescale, period_steps, hw_prescale, hw_period_steps;
>> + int ret;
>> +
>> + ret = mutex_lock_interruptible(&mchp_core_pwm->lock);
>> + if (ret)
>> + return ret;
>
> I would have used mutex_lock() here. Why should a signal prevent
> reconfiguration of the PWM?
Cool, willdo.
>
>> +
>> + if (!state->enabled) {
>> + mchp_core_pwm_enable(chip, pwm, false, current_state.period);
>> + mutex_unlock(&mchp_core_pwm->lock);
>> + return 0;
>> + }
>> +
>> + /*
>> + * If the only thing that has changed is the duty cycle or the polarity,
>> + * we can shortcut the calculations and just compute/apply the new duty
>> + * cycle pos & neg edges
>> + * As all the channels share the same period, do not allow it to be
>> + * changed if any other channels are enabled.
>> + * If the period is locked, it may not be possible to use a period
>> + * less than that requested. In that case, we just abort.
>> + */
>> + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
>> +
>> + if (period_locked) {
>> + mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
>> + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
>> + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
>> +
>> + if ((period_steps * prescale) < (hw_period_steps * hw_prescale)) {
>
> You need
>
> if ((period_steps + 1) * (prescale + 1) < (hw_period_steps + 1) * (hw_prescale + 1))
>
> here, don't you?
Yikes, yeah...
>
>> + mutex_unlock(&mchp_core_pwm->lock);
>> + return -EINVAL;
>> + }
>> +
>> + prescale = hw_prescale;
>> + period_steps = hw_period_steps;
>
> The two hw_* variables are only used in this branch. So their
> declaration can move into here.
>
>> + } else if (!current_state.enabled || current_state.period != state->period) {
>> + ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
>> + if (ret) {
>> + mutex_unlock(&mchp_core_pwm->lock);
>> + return ret;
>> + }
>> + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
>> + } else {
>> + prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
>> + period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
>> + }
>> +
>> + duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);
>> +
>> + /*
>> + * Because the period is per channel, it is possible that the requested
>> + * duty cycle is longer than the period, in which case cap it to the
>> + * period, IOW a 100% duty cycle.
>> + */
>> + if (duty_steps > period_steps)
>> + duty_steps = period_steps + 1;
>> +
>> + mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps);
>> +
>> + mchp_core_pwm_enable(chip, pwm, true, state->period);
>> +
>> + mutex_unlock(&mchp_core_pwm->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static void mchp_core_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
>> + u16 prescale;
>> + u8 period_steps, duty_steps, posedge, negedge;
>> + int ret;
>> +
>> + ret = mutex_lock_interruptible(&mchp_core_pwm->lock);
>> + if (ret)
>> + return;
>> +
>> + if (mchp_core_pwm->channel_enabled & (1 << pwm->hwpwm))
>
> channel_enabled is initialized to 0 in .probe(), so a PWM is never
> diagnosed to be running when the core initially wants to determine the
> current state.
Good point. I'll initialise it in probe.
>
>> + state->enabled = true;
>> + else
>> + state->enabled = false;
>> +
>> + prescale = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE));
>> +
>> + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD));
>> + state->period = period_steps * prescale * NSEC_PER_SEC;
>> + state->period = DIV64_U64_ROUND_UP(state->period, clk_get_rate(mchp_core_pwm->clk));
>> +
>> + posedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
>> + negedge = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
>> +
>> + if (negedge == posedge) {
>> + state->duty_cycle = state->period / 2;
>
> I thought that's:
>
> state->duty_cycle = state->period;
> state->period *= 2;
Correct, as usual..
Thanks for your review Uwe! I'll fix it all up & submit v8 after -rc1.
Conor.
