Hi Uwe,
Thanks for your reply!
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
>
[..]
> > + if (low > period) /* 0% duty cycle */
> > + val = 0;
> > + else if (!low) /* 100% duty cycle */
> > + val = period;
> > + else
> > + val = period - low;
>
> This could be simplified to:
>
> if (low > period) /* 0% duty cycle */
> val = 0;
> else
> val = period - low;
Agreed. And also, I believe there is a tidier way of addressing this.
I'll rework this for v6.
>
> Maybe it makes sense add another variable "duty_cycle" here and use
> that
> for improved clarity? And then maybe rename "val" to "ctr"?
Agreed.
>
> > + if (period)
> > + period += 1;
>
> This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
Agreed. We should always add 1.
>
> Also it suggests that the hardware cannot do a 100% relative duty
> cycle?
It does support 100% duty cycle.
PWMCYC = 0 actually means 1 clock cycle, that's why the faff with
increment and decrement operations, and that's why the confusion.
> If I didn't miss anything here, please add that to the list of
> Limitations above.
Thankfully not a limitation.
>
> > + state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4
> * prescale),
> > + rzv2m_pwm->rate);
>
> The multiplication can overflow. I see no easy way to prevent this
> without introducing a mul_u64_u64_div_roundup helper. Maybe I miss
> something?
It does overflow, good catch!
I think we could split this in three operations for now, and maybe
improve it later on:
period = NSEC_PER_SEC * (cyc + 1);
period = DIV64_U64_ROUND_UP(period, rzv2m_pwm->rate);
period <<= rzv2m_pwm_prescale_to_shift(prescale);
with rzv2m_pwm_prescale_to_shift as below:
static inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
{
return prescale == 3 ? 11 : prescale * 4;
}
As it turns out "<< (4 * prescale)" and ">> (4 * prescale)" are not
correct for prescale == 3.
As per manual: PWMPS = 3 means a frequency division by 2048, and not
4096.
I'll address this in v6.
>
> > + state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val <<
> (4 * prescale),
> > + rzv2m_pwm->rate);
> > + pm_runtime_put(chip->dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2m_pwm_config(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + unsigned long pwm_cyc, pwm_low;
> > + u8 prescale;
> > + u32 pwm_ctr;
> > + u64 pc, dc;
> > +
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflowing the following
> > + * calculation.
> > + */
> > + if (rzv2m_pwm->rate > NSEC_PER_SEC)
> > + return -EINVAL;
> > +
> > + /*
> > + * Formula for calculating PWM Cycle Setting Register
> > + * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div
> ratio)) - 1
> > + */
> > + pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate,
> NSEC_PER_SEC);
>
> rzv2m_pwm->rate is an unsigned long. So mul_u64_u32_div is the wrong
> function (at least on archs where sizeof(long) > 4).
Indeed. I think mul_u64_u64_div_u64 is more appropriate.
>
> > + dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate,
> NSEC_PER_SEC);
> > + prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> > +
> > + pwm_cyc = pc >> (4 * prescale);
>
> The division above might have round down the result, so you're losing
> precision here?
Indeed. I think DIV_ROUND_UP will do better.
>
> > + if (pwm_cyc)
> > + pwm_cyc -= 1;
>
> If the requested period is too small, please refuse the request.
Agreed.
> PWM_DEBUG should catch that and emit a warning.
>
> > + if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
> > + pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
> > +
> > + /*
> > + * Formula for calculating PWMLOW register
> > + * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> > + */
> > + pwm_low = dc >> (4 * prescale);
> > + if (!dc) /* 0% duty cycle */
> > + pwm_low = pwm_cyc + 1;
>
> if pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD) that + 1 isn't a good
> idea,
> is it?
Indeed, the final value should be tested. Perhaps we should refuse the
request if the value of PWMCYC doesn't fit, and we should probably do
the same thing with the value of PWMLOW (which is higher than PWMCYC
when duty cycle is 0%).
>
> > + else if (pc == dc) /* 100% duty cycle */
> > + pwm_low = 0;
> > + else
> > + pwm_low = pwm_cyc - pwm_low;
> > +
> > + /*
> > + * If the PWM channel is disabled, make sure to turn on the clock
> > + * before writing the register.
> > + */
> > + if (!pwm->state.enabled)
> > + pm_runtime_get_sync(rzv2m_pwm->chip.dev);
>
> Can it happen that this already makes the hardware emit a (probably
> wrong) signal?
It doesn't look like this can happen with v5.
PWME set to 1 basically enables operations, and that happens after the
PWM has been configured.
[..]
> > +
> > +static int rzv2m_pwm_pm_runtime_resume(struct device *dev)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
> > + int err;
> > +
> > + err = clk_prepare_enable(rzv2m_pwm->apb_clk);
> > + if (err)
> > + return err;
> > +
> > + err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > + if (err)
> > + return err;
>
> It would be consequent here to disable apb_clk in the error case to
> prevent a clk enable imbalance.
Indeed.
Cheers,
Fab
>
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://www.pengutronix.de/ |
