[v6,10/10] pwm: dwc: use clock rate in hz to avoid rounding issues
Commit Message
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
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.
Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-dwc-of.c | 2 +-
drivers/pwm/pwm-dwc.c | 29 ++++++++++++++++++++---------
drivers/pwm/pwm-dwc.h | 2 +-
3 files changed, 22 insertions(+), 11 deletions(-)
Comments
Hello Ben,
On Thu, Oct 20, 2022 at 04:16:10PM +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
> 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.
>
> Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/pwm/pwm-dwc-of.c | 2 +-
> drivers/pwm/pwm-dwc.c | 29 ++++++++++++++++++++---------
> drivers/pwm/pwm-dwc.h | 2 +-
> 3 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> index c5b4351cc7b0..5f7f066859d4 100644
> --- a/drivers/pwm/pwm-dwc-of.c
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -50,7 +50,7 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(dwc->clk),
> "failed to get timer clock\n");
>
> - dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
> + dwc->clk_rate = clk_get_rate(dwc->clk);
Given that clk_ns is introduced only in this series, I suggest to make
it right from the start.
Best regards
Uwe
On 10/11/2022 15:42, Uwe Kleine-König wrote:
> Hello Ben,
>
> On Thu, Oct 20, 2022 at 04:16:10PM +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
>> 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.
>>
>> Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
>> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> drivers/pwm/pwm-dwc-of.c | 2 +-
>> drivers/pwm/pwm-dwc.c | 29 ++++++++++++++++++++---------
>> drivers/pwm/pwm-dwc.h | 2 +-
>> 3 files changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
>> index c5b4351cc7b0..5f7f066859d4 100644
>> --- a/drivers/pwm/pwm-dwc-of.c
>> +++ b/drivers/pwm/pwm-dwc-of.c
>> @@ -50,7 +50,7 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
>> return dev_err_probe(dev, PTR_ERR(dwc->clk),
>> "failed to get timer clock\n");
>>
>> - dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
>> + dwc->clk_rate = clk_get_rate(dwc->clk);
>
> Given that clk_ns is introduced only in this series, I suggest to make
> it right from the start.
I was trying to keep the splitting of the driver and the clock changes
separate to make any possible bisection easier.
@@ -50,7 +50,7 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(dwc->clk),
"failed to get timer clock\n");
- dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
+ dwc->clk_rate = clk_get_rate(dwc->clk);
return devm_pwmchip_add(dev, &dwc->chip);
}
@@ -43,18 +43,22 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
u32 high;
u32 low;
+ if (dwc->clk)
+ dwc->clk_rate = clk_get_rate(dwc->clk);
+
/*
* Calculate width of low and high period in terms of input clock
* 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);
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;
@@ -120,6 +124,7 @@ static void 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;
@@ -129,22 +134,28 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
+ if (dwc->clk)
+ dwc->clk_rate = clk_get_rate(dwc->clk);
+
+ clk_rate = dwc->clk_rate;
state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
/* If we're not in PWM, technically the output is a 50-50
* 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;
}
- state->period = period;
- state->duty_cycle = duty;
+ 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;
pm_runtime_put_sync(chip->dev);
@@ -164,7 +175,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;
@@ -41,7 +41,7 @@ struct dwc_pwm {
struct pwm_chip chip;
void __iomem *base;
struct clk *clk;
- 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))