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

[PATCH V2 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>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Changes since V1:
- Dropped extra parenthesis

 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..b05ea2e8accc 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 V2 2/2] pwm: tegra: Ensure the clock rate is not less than needed

[Jon Hunter]

When dynamically scaling the PWM clock, the function
dev_pm_opp_set_rate() may set the PWM clock to a rate that is lower than
what is required. The clock rate requested when calling
dev_pm_opp_set_rate() is the minimum clock rate that is needed to drive
the PWM to achieve the required period. Hence, if the actual clock
rate is less than the requested clock rate, then the required period
cannot be achieved and configuring the PWM fails. Fix this by
calling clk_round_rate() to check if the clock rate that will be provided
is sufficient and if not, double the required clock rate to ensure the
required period can be attained.

Fixes: 8c193f4714df ("pwm: tegra: Optimize period calculation")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
Changes since V1:
- Multiplied the required_clk_rate by 2 instead of adding 1 to the
  PWM_DUTY_WIDTH and recalculating the rate. Overall rate should be
  similar.
- Updated comment based upon Uwe's feedback.

 drivers/pwm/pwm-tegra.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index b05ea2e8accc..6fc4b69a3ba7 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 (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. Hence, for lower rates, double the
+			 * required_clk_rate to get a clock rate that can meet
+			 * the requested period.
+			 */
+			required_clk_rate *= 2;
+
 		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
 		if (err < 0)
 			return -EINVAL;
-- 
2.25.1

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

[Thierry Reding]

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

On Fri, Oct 28, 2022 at 01:33:55PM +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>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since V1:
> - Dropped extra parenthesis
> 
>  drivers/pwm/pwm-tegra.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH V2 2/2] pwm: tegra: Ensure the clock rate is not less than needed

[Thierry Reding]

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

On Fri, Oct 28, 2022 at 01:33:56PM +0100, Jon Hunter wrote:
> When dynamically scaling the PWM clock, the function
> dev_pm_opp_set_rate() may set the PWM clock to a rate that is lower than
> what is required. The clock rate requested when calling
> dev_pm_opp_set_rate() is the minimum clock rate that is needed to drive
> the PWM to achieve the required period. Hence, if the actual clock
> rate is less than the requested clock rate, then the required period
> cannot be achieved and configuring the PWM fails. Fix this by
> calling clk_round_rate() to check if the clock rate that will be provided
> is sufficient and if not, double the required clock rate to ensure the
> required period can be attained.
> 
> Fixes: 8c193f4714df ("pwm: tegra: Optimize period calculation")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Multiplied the required_clk_rate by 2 instead of adding 1 to the
>   PWM_DUTY_WIDTH and recalculating the rate. Overall rate should be
>   similar.
> - Updated comment based upon Uwe's feedback.
> 
>  drivers/pwm/pwm-tegra.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Uwe, any objections to this? I'm assuming not, since you proposed this
variant yourself, but would be good to get your Acked-by nevertheless.

Thanks,
Thierry

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

Re: [PATCH V2 2/2] pwm: tegra: Ensure the clock rate is not less than needed

[Uwe Kleine-König]

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

On Fri, Oct 28, 2022 at 01:33:56PM +0100, Jon Hunter wrote:
> When dynamically scaling the PWM clock, the function
> dev_pm_opp_set_rate() may set the PWM clock to a rate that is lower than
> what is required. The clock rate requested when calling
> dev_pm_opp_set_rate() is the minimum clock rate that is needed to drive
> the PWM to achieve the required period. Hence, if the actual clock
> rate is less than the requested clock rate, then the required period
> cannot be achieved and configuring the PWM fails. Fix this by
> calling clk_round_rate() to check if the clock rate that will be provided
> is sufficient and if not, double the required clock rate to ensure the
> required period can be attained.
> 
> Fixes: 8c193f4714df ("pwm: tegra: Optimize period calculation")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Multiplied the required_clk_rate by 2 instead of adding 1 to the
>   PWM_DUTY_WIDTH and recalculating the rate. Overall rate should be
>   similar.
> - Updated comment based upon Uwe's feedback.
> 
>  drivers/pwm/pwm-tegra.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index b05ea2e8accc..6fc4b69a3ba7 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 (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. Hence, for lower rates, double the
> +			 * required_clk_rate to get a clock rate that can meet
> +			 * the requested period.
> +			 */
> +			required_clk_rate *= 2;

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

Thanks
Uwe

> +
>  		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
>  		if (err < 0)
>  			return -EINVAL;
> -- 
> 2.25.1
> 
> 

-- 
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 V2 2/2] pwm: tegra: Ensure the clock rate is not less than needed

[Thierry Reding]

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

On Fri, Oct 28, 2022 at 01:33:56PM +0100, Jon Hunter wrote:
> When dynamically scaling the PWM clock, the function
> dev_pm_opp_set_rate() may set the PWM clock to a rate that is lower than
> what is required. The clock rate requested when calling
> dev_pm_opp_set_rate() is the minimum clock rate that is needed to drive
> the PWM to achieve the required period. Hence, if the actual clock
> rate is less than the requested clock rate, then the required period
> cannot be achieved and configuring the PWM fails. Fix this by
> calling clk_round_rate() to check if the clock rate that will be provided
> is sufficient and if not, double the required clock rate to ensure the
> required period can be attained.
> 
> Fixes: 8c193f4714df ("pwm: tegra: Optimize period calculation")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Multiplied the required_clk_rate by 2 instead of adding 1 to the
>   PWM_DUTY_WIDTH and recalculating the rate. Overall rate should be
>   similar.
> - Updated comment based upon Uwe's feedback.
> 
>  drivers/pwm/pwm-tegra.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Applied, thanks.

Thierry

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