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

[Jon Hunter <jonathanh@nvidia.com>]

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.

> 
>>   		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
>>   		if (err < 0)
>>   			return -EINVAL;
> 
> Is it obvious that dev_pm_opp_set_rate(pc->dev, ...) and
> clk_round_rate() correlate enough that the latter tells anything about
> the former? Would it make sense to use clk_set_rate instead of
> dev_pm_opp_set_rate?

We call clk_get_rate() after calling dev_pm_opp_set_rate() and so 
hopefully when reviewing the complete code it is clearer. I don't think 
we can use clk_set_rate() and this was changed from calling 
clk_set_rate() by commit 3da9b0feaa16 ("pwm: tegra: Add runtime PM and 
OPP support").

Thanks
Jon

-- 
nvpublic