Re: [PATCH v10 3/4] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

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