| Message ID | 20230614171457.69191-1-ben.dooks@sifive.com |
|---|---|
| Headers | show |
| Series | DesignWare PWM driver updates | expand |
On 2023-06-20 13:59, Jarkko Nikula wrote: > On 6/14/23 20:14, Ben Dooks wrote: >> This series is an update for the DesignWare PWM driver to add support >> for >> OF (and split the PCI bits out if aynone else wants them). This is >> mostly >> the same as the v7 series, but with code moved around and >> module-namespace >> added, plus review comments processed. >> >> Since we no longer have the hardware, the clock code hasn't been >> changed to >> either lock the rate whilst the PWM is running, or to deal with any >> sort >> of change callback. This is left to future work (and I would rather >> get >> something in that does currently work) (second note, the hardware we >> did >> use had a fixed clock tree anyway) >> >> This account is probably going away soon, I have cc'd my main work >> email to catch any responses. >> >> Thank you all for the reviews. >> > I tested the patchset on Intel Elkhart Lake and didn't see issues. > > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Great, thank you. Is this series likely to get into the next kernel release?
On Wed, Jun 14, 2023 at 06:14:56PM +0100, Ben Dooks wrote: > As noted, the clock-rate when not a nice multiple of ns is probably > going to end up with inacurate caculations, as well as on a non pci s/caculation/calculations/ > system the rate may change (although we've not put a clock rate > change notifier in this code yet) so we also add some quick checks > of the rate when we do any calculations with it. An externally triggered clock rate change is bad. If you drive a motor you probably want to prevent an uncontrolled change here. I already considered to add a call to clk_rate_exclusive_get() in various pwm drivers for that reason, but didn't come around yet. > Signed-off-by; Ben Dooks <ben.dooks@sifive.com> > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > v8: > - fixup post rename > - move to earlier in series > --- > drivers/pwm/pwm-dwc-core.c | 24 +++++++++++++++--------- > drivers/pwm/pwm-dwc.h | 2 +- > 2 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c > index 38cd2163fe01..0f07e26e6c30 100644 > --- a/drivers/pwm/pwm-dwc-core.c > +++ b/drivers/pwm/pwm-dwc-core.c > @@ -49,13 +49,14 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc, > * periods and check are the result within HW limits between 1 and > * 2^32 periods. > */ > - tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns); > + tmp = state->duty_cycle * dwc->clk_rate; > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); New drivers should implement round-down behaviour (i.e. pick the biggest period (and duty_cycle) that is not bigger than the requested value. With clk_ns = 10 (which it is the hardcoded value up to now) it doesn't matter much how you round the division. I suggest to use the opportunity to align to how new drivers should round. (That would be a separate patch.) > if (tmp < 1 || tmp > (1ULL << 32)) > return -ERANGE; > low = tmp - 1; > > - tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle, > - dwc->clk_ns); > + tmp = (state->period - state->duty_cycle) * dwc->clk_rate; > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > if (tmp < 1 || tmp > (1ULL << 32)) > return -ERANGE; > high = tmp - 1; > @@ -121,11 +122,14 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_state *state) > { > struct dwc_pwm *dwc = to_dwc_pwm(chip); > + unsigned long clk_rate; > u64 duty, period; > u32 ctrl, ld, ld2; > > pm_runtime_get_sync(chip->dev); > > + clk_rate = dwc->clk_rate; > + > ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm)); > ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm)); > ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm)); > @@ -136,17 +140,19 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > * based on the timer load-count only. > */ > if (ctrl & DWC_TIM_CTRL_PWM) { > - duty = (ld + 1) * dwc->clk_ns; > - period = (ld2 + 1) * dwc->clk_ns; > + duty = ld + 1; > + period = ld2 + 1; > period += duty; > } else { > - duty = (ld + 1) * dwc->clk_ns; > + duty = ld + 1; > period = duty * 2; > } > > + duty *= NSEC_PER_SEC; > + period *= NSEC_PER_SEC; > + state->period = DIV_ROUND_CLOSEST_ULL(period, clk_rate); > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(duty, clk_rate); > state->polarity = PWM_POLARITY_INVERSED; > - state->period = period; > - state->duty_cycle = duty; > > pm_runtime_put_sync(chip->dev); > > @@ -167,7 +173,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev) > if (!dwc) > return NULL; > > - dwc->clk_ns = 10; > + dwc->clk_rate = NSEC_PER_SEC / 10; > dwc->chip.dev = dev; > dwc->chip.ops = &dwc_pwm_ops; > dwc->chip.npwm = DWC_TIMERS_TOTAL; > diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h > index 64795247c54c..e0a940fd6e87 100644 > --- a/drivers/pwm/pwm-dwc.h > +++ b/drivers/pwm/pwm-dwc.h > @@ -42,7 +42,7 @@ struct dwc_pwm_ctx { > struct dwc_pwm { > struct pwm_chip chip; > void __iomem *base; > - unsigned int clk_ns; > + unsigned long clk_rate; > struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL]; > }; > #define to_dwc_pwm(p) (container_of((p), struct dwc_pwm, chip)) Best regards Uwe