[PATCH 1/2] pwm: tegra: Improve required rate calculation

[PATCH 1/2] pwm: tegra: Improve required rate calculation

[Jon Hunter]

For the case where dev_pm_opp_set_rate() is called to set the PWM clock
rate, the requested rate is calculated as ...

 required_clk_rate = (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;

The above calculation may lead to rounding errors because the
NSEC_PER_SEC is divided by 'period_ns' before applying the
PWM_DUTY_WIDTH multiplication factor. For example, if the period is
45334ns, the above calculation yields a rate of 5646848Hz instead of
5646976Hz. Fix this by applying the multiplication factor before
dividing and using the DIV_ROUND_UP macro which yields the expected
result of 5646976Hz.

Fixes: 1d7796bdb63a ("pwm: tegra: Support dynamic clock frequency configuration")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/pwm/pwm-tegra.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index dad9978c9186..8a33c500f93b 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -145,8 +145,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * source clock rate as required_clk_rate, PWM controller will
 		 * be able to configure the requested period.
 		 */
-		required_clk_rate =
-			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
+		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)
-- 
2.25.1

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

[Jon Hunter]

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);
+
 		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
 		if (err < 0)
 			return -EINVAL;
-- 
2.25.1

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

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?

>  		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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jon Hunter]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3819 bytes --]

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().)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jon Hunter]

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

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

[Jon Hunter]

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,
+                * 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;
+
                 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).

Jon

-- 
nvpublic

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4356 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] pwm: tegra: Improve required rate calculation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

Hello,

On Wed, Oct 26, 2022 at 11:13:04AM +0100, Jon Hunter wrote:
> For the case where dev_pm_opp_set_rate() is called to set the PWM clock
> rate, the requested rate is calculated as ...
> 
>  required_clk_rate = (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> 
> The above calculation may lead to rounding errors because the
> NSEC_PER_SEC is divided by 'period_ns' before applying the
> PWM_DUTY_WIDTH multiplication factor. For example, if the period is
> 45334ns, the above calculation yields a rate of 5646848Hz instead of
> 5646976Hz. Fix this by applying the multiplication factor before
> dividing and using the DIV_ROUND_UP macro which yields the expected
> result of 5646976Hz.
> 
> Fixes: 1d7796bdb63a ("pwm: tegra: Support dynamic clock frequency configuration")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/pwm/pwm-tegra.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index dad9978c9186..8a33c500f93b 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -145,8 +145,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		 * source clock rate as required_clk_rate, PWM controller will
>  		 * be able to configure the requested period.
>  		 */
> -		required_clk_rate =
> -			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> +		required_clk_rate = DIV_ROUND_UP_ULL((NSEC_PER_SEC << PWM_DUTY_WIDTH),
> +						     period_ns);

This also has the nice side effect that required_clk_rate doesn't become
zero any more for big period_ns values.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] pwm: tegra: Improve required rate calculation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

Hello,

On Wed, Oct 26, 2022 at 04:17:55PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 26, 2022 at 11:13:04AM +0100, Jon Hunter wrote:
> > For the case where dev_pm_opp_set_rate() is called to set the PWM clock
> > rate, the requested rate is calculated as ...
> > 
> >  required_clk_rate = (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> > 
> > The above calculation may lead to rounding errors because the
> > NSEC_PER_SEC is divided by 'period_ns' before applying the
> > PWM_DUTY_WIDTH multiplication factor. For example, if the period is
> > 45334ns, the above calculation yields a rate of 5646848Hz instead of
> > 5646976Hz. Fix this by applying the multiplication factor before
> > dividing and using the DIV_ROUND_UP macro which yields the expected
> > result of 5646976Hz.
> > 
> > Fixes: 1d7796bdb63a ("pwm: tegra: Support dynamic clock frequency configuration")
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> >  drivers/pwm/pwm-tegra.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index dad9978c9186..8a33c500f93b 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -145,8 +145,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  		 * source clock rate as required_clk_rate, PWM controller will
> >  		 * be able to configure the requested period.
> >  		 */
> > -		required_clk_rate =
> > -			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> > +		required_clk_rate = DIV_ROUND_UP_ULL((NSEC_PER_SEC << PWM_DUTY_WIDTH),
> > +						     period_ns);
> 
> This also has the nice side effect that required_clk_rate doesn't become
> zero any more for big period_ns values.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I just noticed you could drop a pair of parenthesis in the first parameter
to DIV_ROUND_UP_ULL.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]