Hello Jon,
On Wed, Oct 26, 2022 at 09:17:08PM +0100, Jon Hunter wrote:
> On 26/10/2022 15:23, Uwe Kleine-König wrote:
> > On Wed, Oct 26, 2022 at 11:13:05AM +0100, Jon Hunter wrote:
> > > If the 'required_clk_rate' is greater than the clock rate that can be
> > > provided, then when mul_u64_u64_div_u64() is called to determine the
> > > 'rate' for the PWM divider, 0 will be returned. If 'rate' is 0, then we
> > > will return -EINVAL and fail to configure the PWM. Fix this by adding 1
> > > to the PWM_DUTY_WIDTH when calculating the 'required_clk_rate' to ensure
> > > that 'rate' is greater or equal to 1. This fixes an issue on Tegra234
> > > where configuring the PWM fan fails.
> > >
> > > Fixes: 8c193f4714df ("pwm: tegra: Optimize period calculation")
> > > Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> > > ---
> > > drivers/pwm/pwm-tegra.c | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > > index 8a33c500f93b..973e2c1533ab 100644
> > > --- a/drivers/pwm/pwm-tegra.c
> > > +++ b/drivers/pwm/pwm-tegra.c
> > > @@ -148,6 +148,19 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > required_clk_rate = DIV_ROUND_UP_ULL((NSEC_PER_SEC << PWM_DUTY_WIDTH),
> > > period_ns);
> > > + /*
> > > + * If the 'required_clk_rate' is greater than the clock rate
> > > + * that can be provided, then when mul_u64_u64_div_u64() is
> > > + * called to determine the 'rate' for the PWM divider, 0 will
> > > + * be returned. If 'rate' is 0, then we will return -EINVAL and
> > > + * fail to configure the PWM. If this case, add 1 to the
> > > + * PWM_DUTY_WIDTH when calculating the 'required_clk_rate' to
> > > + * ensure that 'rate' is greater or equal to 1.
> > > + */
> > > + if (required_clk_rate > clk_round_rate(pc->clk, required_clk_rate))
> > > + required_clk_rate = DIV_ROUND_UP_ULL((NSEC_PER_SEC << (PWM_DUTY_WIDTH + 1)),
> > > + period_ns);
> > > +
> >
> > It's implicit knowledge that (roughly) doubling the clk rate is the
> > right value (i.e the minimal value to get a
> > clk_rate >= (NSEC_PER_SEC << PWM_DUTY_WIDTH) / period_ns?
>
> Are you suggesting I drop the comment? Sorry not sure what you are trying to
> say here and if you think something should be changed.
No, I just wondered about that +1 being the right thing to do. Consider
period_ns was 400003. Then you get required_clk_rate = 639996.
Now we want to prevent that calling dev_pm_opp_set_rate(..., 639996)
yields a rate less than 639996.
You're implicitly claiming that 1279991 will do. But without further
knowledge also that value might yield a rate less than 639996; or 959993
might yield a rate that better fits our needs (i.e.
639996 <= clk_round_rate(..., 959993) < clk_round_rate(..., 1279991)
). So my question was about "why 1279991?" and if there is implicit
knowledge that makes 1279991 the right choice. Assuming there is such a
reasoning, I'd prefer a comment like:
/*
* To achieve a period not bigger than the requested period we
* must ensure that the input clock runs with at least
* $required_clk_rate Hz. As consecutive possible rates differ
* by a factor of two we double our request if
* $required_clk_rate yields a too slow rate.
*/
I'm not entirely sure this would be a sound assumption but I think you
get the point?! (It might be necessary to double exactly the requested
value and then you still have to make some (reasonable) assumption about
clk_round_rate().)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
