| Message ID | 20221007113512.91501-1-conor.dooley@microchip.com |
|---|---|
| Headers | show |
| Series | Microchip soft ip corePWM driver | expand |
Hello, On Fri, Oct 07, 2022 at 12:35:12PM +0100, Conor Dooley wrote: > [...] > +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, tmp; > + u16 prescale_val = PREG_TO_VAL(prescale); > + > + /* > + * 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); The assignment to duty_steps can overflow. So you have to use mul_u64_u64_div_u64 here, too. > +} > + > [...] > + > +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; > + bool period_locked; > + u64 duty_steps; > + u16 prescale; > + u8 period_steps; > + > + if (!state->enabled) { > + mchp_core_pwm_enable(chip, pwm, false, current_state.period); > + 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, &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 + 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 { > + int ret; > + > + ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps); > + if (ret) > + return ret; > + > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > + } > + > + duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps); Both mchp_core_pwm_calc_period and mchp_core_pwm_calc_duty call clk_get_rate(), I suggest call this only once and pass the rate to these two functions. Both branches of the if above start with calling mchp_core_pwm_calc_period, this could be simplified, too. (Hmm, in exactly one of them you check the return code, wouldn't that be sensible for both callers?) > + > + /* > + * 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); > + > + return 0; > +} Best regards Uwe
On Tue, Nov 08, 2022 at 04:50:41PM +0100, Uwe Kleine-König wrote: > Hello, Hello! Thanks for the review Uwe :) > On Fri, Oct 07, 2022 at 12:35:12PM +0100, Conor Dooley wrote: > > +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; > > + bool period_locked; > > + u64 duty_steps; > > + u16 prescale; > > + u8 period_steps; > > + > > + if (!state->enabled) { > > + mchp_core_pwm_enable(chip, pwm, false, current_state.period); > > + 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, &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 + 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 { > > + int ret; > > + > > + ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps); > > + if (ret) > > + return ret; > > + > > + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps); > > + } > > + > > + duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps); > > Both mchp_core_pwm_calc_period and mchp_core_pwm_calc_duty call > clk_get_rate(), I suggest call this only once and pass the rate to these > two functions. Sure. I think the signatures of both of those functions could be reduced in the process which would be nice. > Both branches of the if above start with calling > mchp_core_pwm_calc_period, this could be simplified, too. ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps); if (ret) return ret; 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(chip, pwm, state, prescale, period_steps); I'll aim for something like the (absolutely untested) above then when I respin. > (Hmm, in > exactly one of them you check the return code, wouldn't that be sensible > for both callers?) Been messing with rust a bit of late, I love the #[must_use] attribute. Looks to be an oversight since it's only going to return an error if the clock rate exceeds what the FPGA is actually capable of. Thanks again, Conor.
On Fri, Oct 07, 2022 at 12:35:13PM +0100, Conor Dooley wrote: > Add the newly introduced pwm driver to the existing PolarFire SoC entry. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I assume you will rework the series and resend this one with the driver patche. Applying patch #4 alone doesn't make sense, so I'm marking this one as "changes requested", too, in the PWM patchwork instance. IMHO patches #1 and #2 make sense to be applied already without the driver given the binding is already there. I assume they will go in via the riscv tree, so I will mark these two as "handled elsewhere". Best regards Uwe
On Wed, Nov 09, 2022 at 10:35:25AM +0100, Uwe Kleine-König wrote: > On Fri, Oct 07, 2022 at 12:35:13PM +0100, Conor Dooley wrote: > > Add the newly introduced pwm driver to the existing PolarFire SoC entry. > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > I assume you will rework the series and resend this one with the driver > patche. Applying patch #4 alone doesn't make sense, so I'm marking this > one as "changes requested", too, in the PWM patchwork instance. > > IMHO patches #1 and #2 make sense to be applied already without the > driver given the binding is already there. I assume they will go in via > the riscv tree, so I will mark these two as "handled elsewhere". Right. Makes sense to me - I'll take the dt-binding & the dt via the riscv (or soc, we're changing things up there [a]) tree. Thanks, Conor. [a] - https://lore.kernel.org/linux-riscv/mhng-e4210f56-fcc3-4db8-abdb-d43b3ebe695d@palmer-ri-x1c9a/
From: Conor Dooley <conor.dooley@microchip.com> On Fri, 7 Oct 2022 12:35:09 +0100, Conor Dooley wrote: > Hey Uwe, all, > > ~6.0-rc1 has rolled around so here is the promised v8v9~. > v11 is based on 6.0 stuff still & there will be a change to the dts > patch in v6.1, but I did a test merge and there was nothing to resolve. > I'll take the dts change myself just to be on the safe side. > > [...] Applied to dt-for-next, thanks! [1/4] dt-bindings: pwm: fix microchip corePWM's pwm-cells https://git.kernel.org/conor/c/a62d196e8988 [2/4] riscv: dts: fix the icicle's #pwm-cells https://git.kernel.org/conor/c/ac2bcd194cc5 Thanks, Conor.