Messages in this thread | | | Date | Thu, 17 Nov 2022 17:49:50 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v12 1/2] pwm: add microchip soft ip corePWM driver |
| |
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/ | [unhandled content-type:application/pgp-signature] | |