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

Uwe Kleine-König
2022-11-17T16:50:06Z

[-- Attachment #1: Type: text/plain, Size: 5884 bytes --]

Hello Conor,

On Thu, Nov 10, 2022 at 09:35:12AM +0000, Conor Dooley wrote:
> [...]
> +
> +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
> + * considering the channel enabled.
> + * If the delay is under 1 us, sleep for at least 1 us anyway.
> + */
> + if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) {
> + u64 delay;
> +
> + delay = div_u64(period, 1000u) ? : 1u;
> + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
> + usleep_range(delay, delay * 2);
> + }

In some cases the delay could be prevented. e.g. when going from one
disabled state to another. If you don't want to complicate the driver
here, maybe point it out in a comment at least?

It's not well defined if pwm_apply should only return when the new
setting is actually active. (e.g. mxs doesn't wait)
So I wonder: Are there any hardware restrictions between setting the
SYNC_UPD flag and modifying the registers for duty and period? (I assume
writing a new duty and period might then result in a glitch if the
period just ends between the two writes.) Can you check if the hardware
waits on such a completion, e.g. by reading that register?

> +}
> +
> [...]
> +
> +static int mchp_core_pwm_apply_locked(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;

You're doing a copy of pwm->state just to use one of the members to pass
it to mchp_core_pwm_enable.

> + bool period_locked;
> + u64 duty_steps, clk_rate;

I think using unsigned long for clk_rate would be beneficial. The
comparison against NSEC_PER_SEC might get cheaper (depending on how
clever the compiler is), and calling mchp_core_pwm_calc_period
should get cheaper, too. (At least on 32 bit archs.)

> + u16 prescale;
> + u8 period_steps;
> +
> + if (!state->enabled) {
> + mchp_core_pwm_enable(chip, pwm, false, current_state.period);
> + return 0;
> + }
> +
> + /*
> + * 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.
> + */
> + clk_rate = clk_get_rate(mchp_core_pwm->clk);
> + if (clk_rate >= NSEC_PER_SEC)
> + return -EINVAL;
> +
> + mchp_core_pwm_calc_period(state, clk_rate, &prescale, &period_steps);
> +
> + /*
> + * 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) {
> + u16 hw_prescale;
> + u8 hw_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 + 1) * (prescale + 1) <
> + (hw_period_steps + 1) * (hw_prescale + 1))
> + return -EINVAL;
> +
> + /*
> + * It is possible that something could have set the period_steps
> + * register to 0xff, which would prevent us from setting a 100%
> + * or 0% relative duty cycle, as explained above in
> + * mchp_core_pwm_calc_period().
> + * The period is locked and we cannot change this, so we abort.
> + */
> + if (hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX)
> + return -EINVAL;
> +
> + prescale = hw_prescale;
> + period_steps = hw_period_steps;
> + } else {
> + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
> + }
> +
> + duty_steps = mchp_core_pwm_calc_duty(state, clk_rate, 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);

Don't you need to pass the previously configured period here?

> +
> + return 0;
> +}
> [...]

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]