On Thu, Oct 27, 2022 at 04:40:04PM +0100, Jon Hunter wrote:
>
> On 27/10/2022 15:17, Jon Hunter wrote:
>
> ...
>
> > However, I see that I have been focused on the current issue in
> > front of me and this works. The alternative that I see would be to
> > stick with the maximum rate permitted ...
> >
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index 8a33c500f93b..2099ecca4237 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -148,12 +148,14 @@ 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);
> >
> > - err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
> > - if (err < 0)
> > - return -EINVAL;
> > -
> > - /* Store the new rate for further references */
> > - pc->clk_rate = clk_get_rate(pc->clk);
> > + if (required_clk_rate <= clk_round_rate(pc->clk,
> > required_clk_rate)) {
> > + err = dev_pm_opp_set_rate(pc->dev,
> > required_clk_rate);
> > + if (err < 0)
> > + return -EINVAL;
> > +
> > + /* Store the new rate for further references */
> > + pc->clk_rate = clk_get_rate(pc->clk);
> > + }
> > }
>
>
> Thinking about it some more, it is probably simpler and better to ...
>
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 8a33c500f93b..16855f7686db 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -148,6 +148,17 @@ 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 we will fail to configure the PWM,
This is unclear. clk_round_rate(pc->clk, required_clk_rate) isn't the
(maximal) rate that can be provided. It's just the rate you get when
requesting required_clk_rate.
> + * because the 'rate' calculation below will return 0 and which
> + * will cause this function to return -EINVAL. To avoid this, if
> + * the 'required_clk_rate' is greater than the rate returned by
> + * clk_round_rate(), set the PWM clock to the max frequency.
> + */
> + if (required_clk_rate > clk_round_rate(pc->clk, required_clk_rate))
> + required_clk_rate = ULONG_MAX;
> +
That looks wrong. Assume the clk can implement either
51 MHz, 102 MHz, 204 MHz or 408 MHz
Say you want at least 52 MHz and clk_round_rate(..., 52000000) =
51000000. Then you want to pick 102 MHz, not 408 MHz, don't you?
> err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
> if (err < 0)
> return -EINVAL;
>
> Setting the 'required_clk_rate' to ULONG_MAX will cause the PWM to run
> at the max clock. For Tegra234, this is 408MHz (assuming the PLLP is the
> parent).
It's another implicit assumption that using ULONG_MAX configures the
maximal possible rate ...
I'd write:
if (required_clk_rate > clk_round_rate(pc->clk, required_clk_rate))
/*
* required_clk_rate is a lower bound for the input
* rate; for lower rates there is no value for PWM_SCALE
* that yields a period less than or equal to the
* requested period. So increase required_clk_rate.
*
* Some more talk about the properties of clk that
* motivate that doubling (or whatever you pick) is a
* sane strategy.
*/
required_clk_rate *= 2;
I'd not explain the details about the calculation, someone interested in
that will/should look at the code anyhow.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
