| Message ID | 20221026101305.30670-2-jonathanh@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] pwm: tegra: Improve required rate calculation | expand |
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
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
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
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
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
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
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;
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(+)