Re: [PATCH 2/2] pwm: tegra: Fix required rate when clock is lower than needed

[Jon Hunter <jonathanh@nvidia.com>]

On 27/10/2022 07:40, Uwe Kleine-König wrote:
> 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@nvidia.com>
>>>> ---
>>>>    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().)


So actually, I am not claiming that doubling the clock rate will do.
All I am claiming is that we know that based upon the rate returned
by clk_round_rate(), we can determine if the original
required_clk_rate we calculated will work or not. If we determine
that this does not work because it is less than we need, then the
next logical step would be to try a higher rate.

We already know that the period is greater than the minimum period
that is allowed, because we check this earlier on. So if the period
is greater than the min period, it would seem that doubling the
clock rate might be sufficient. Worse case it is not and we still
return -EINVAL and we are no better off.

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);
+               }
         }

Jon

-- 
nvpublic