On Mon, Sep 19, 2022 at 01:53:56PM +0100, Conor Dooley wrote:
> Hey Uwe,
> Thanks (as always). I've switched up my email setup a bit so I hope
> that I've not mangled anything here.
>
> On Thu, Sep 15, 2022 at 09:21:52AM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Wed, Aug 24, 2022 at 10:12:14AM +0100, Conor Dooley wrote:
> > > Add a driver that supports the Microchip FPGA "soft" PWM IP core.
> > >
> > > Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > > ---
> > > drivers/pwm/Kconfig | 10 +
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-microchip-core.c | 402 +++++++++++++++++++++++++++++++
> > > 3 files changed, 413 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-microchip-core.c
> > >
>
> > > +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;
> > > + u16 prescale;
> > > + u8 period_steps;
> > > + int ret;
> > > +
> > > + mutex_lock(&mchp_core_pwm->lock);
> > > +
> > > + 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) {
> > > + u16 hw_prescale;
> > > + u8 hw_period_steps;
> > > +
> > > + mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps);
> >
> > Huh, if (u8 *)&prescale works depends on endianness.
>
> Big endian? What's that? ;)
> I think the cast can just be dropped and the u16 used directly instead.
>
> >
> > > + 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)) {
> > > + mutex_unlock(&mchp_core_pwm->lock);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /*
> > > + * It is possible that something could have set the period_steps
> >
> > My German feel for the English language says s/could have/has/
>
> What I wrote is _fine_ but the could is redudant given the possible.
> I'll change it over.
>
> > > + * register to 0xff, which would prevent us from setting a 100%
> >
> > For my understanding: It would also prevent a 0% relative duty, right?
>
> Yeah, I guess the comment could reflect that.
>
> >
> > > + * duty cycle, as explained in the mchp_core_pwm_calc_period()
> >
> > s/duty/relative duty/; s/the //
> >
> > > + * above.
> > > + * The period is locked and we cannot change this, so we abort.
> > > + */
> > > + if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) {
> >
> > Don't you need to check hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX
> > here?
>
> D'oh.
>
> >
> > > + mutex_unlock(&mchp_core_pwm->lock);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + prescale = hw_prescale;
> > > + period_steps = hw_period_steps;
> > > + } else if (!current_state.enabled || current_state.period != state->period) {
> > > + ret = mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps);
> >
> > ret is only used in this block, so the declaration can go into here,
> > too.
> >
> > > + 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);
> > > + /*
> > > + * As above, it is possible that something could have set the
> > > + * period_steps register to 0xff, which would prevent us from
> > > + * setting a 100% duty cycle, as explained above.
> > > + * As the period is not locked, we are free to fix this.
> > > + */
> >
> > Are you sure this is safe? I think it isn't. Consider:
> >
> > pwm_apply_state(mypwm, { .duty = 0, .period = A, .enabled = true, });
> > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = false, });
> > pwm_apply_state(mypwm, { .duty = 0, .period = B, .enabled = true, });
> >
> > Then you have in the third call prescale and period_steps still
> > corresponding to A because you didn't update these registers in the 2nd
> > call as you exited early.
>
> Riiight. I think I am a little confused here - this comment does not
> refer to my comment but rather to the whole logic I have?
>
> As in, what you're concerned about is the early exit if the state is
> disabled & that I take the values in the hardware as accurate?
No, the thing I'm concerned about is assuming MCHPCOREPWM_PRESCALE and
MCHPCOREPWM_PERIOD correspond to state->period. So I'd drop the last
block use the 2nd last instead without further condition.
> What makes sense to me to do here (assuming I understood correctly)
> is to compare state->period against what is in the hardare rather than
> against what the pwm core thinks?
> Or else I could stop exiting early if the pwm is to be disabled &
> instead allow the period and duty to be set so that the state of the
> hardware is as close to the pwm core's representation of it as possible.
exiting early is fine.
> > > [...]
> > > + period_steps = PREG_TO_VAL(readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD));
> > > + state->period = period_steps * prescale * NSEC_PER_SEC;
> >
> > This is broken on 32 bit archs (here: arm):
> >
> > $ cat test.c
> > #include <inttypes.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> >
> > int main(int argc, char *argv[])
> > {
> > uint8_t period_steps = atoi(argv[1]);
> > uint16_t prescale = atoi(argv[2]);
> > uint64_t period;
> >
> > period = period_steps * prescale * 1000000000L;
> >
> > printf("period_steps = %" PRIu8 "\n", period_steps);
> > printf("prescale = %" PRIu16 "\n", prescale);
> > printf("period = %" PRIu64 "\n", period);
> >
> > return 0;
> > }
> >
> > $ make test
> > cc test.c -o test
> >
> > $ ./test 255 65535
> > period_steps = 255
> > prescale = 65535
> > period = 18446744073018591744
> >
> > The problem is that the result of 16711425 * 1000000000L isn't affected
> > by the type of period and so it's promoted to L which isn't big enough
> > to hold 16711425000000000 where longs are only 32 bit wide.
>
> I don't think this is ever going to be hit in the wild, since prescale
> comes from the hardware where it is limited to 255 - but preventing the
> issue seems trivially done by splitting the multiplication so no reason
> not to. Thanks for providing the test program btw :)
Even 255 * 255 * 1000000000 overflows. With a maintainer's hat on, it is
very valuable to prevent such issues because your driver might be used
as a template for the next driver.
> > > + 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->enabled) {
> >
> > Why do you need that state->enabled?
>
> Because I was running into conflicts between the reporting here and some
> of the checks that I have added to prevent the PWM being put into an
> invalid state. On boot both negedge and posedge will be zero & this was
> preventing me from setting the period at all.
I don't understood that.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
