Re: [PATCH v16 21/40] pwm: tegra: Add runtime PM and OPP support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

On Wed, Dec 01, 2021 at 02:23:28AM +0300, Dmitry Osipenko wrote:
> The PWM on Tegra belongs to the core power domain and we're going to
> enable GENPD support for the core domain. Now PWM must be resumed using
> runtime PM API in order to initialize the PWM power state. The PWM clock
> rate must be changed using OPP API that will reconfigure the power domain
> performance state in accordance to the rate. Add runtime PM and OPP
> support to the PWM driver.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-tegra.c | 82 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 11a10b575ace..18cf974ac776 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -42,12 +42,16 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pwm.h>
>  #include <linux/platform_device.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/reset.h>
>  
> +#include <soc/tegra/common.h>
> +
>  #define PWM_ENABLE	(1 << 31)
>  #define PWM_DUTY_WIDTH	8
>  #define PWM_DUTY_SHIFT	16
> @@ -145,7 +149,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		required_clk_rate =
>  			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
>  
> -		err = clk_set_rate(pc->clk, required_clk_rate);
> +		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
>  		if (err < 0)
>  			return -EINVAL;
>  
> @@ -181,8 +185,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * before writing the register. Otherwise, keep it enabled.
>  	 */
>  	if (!pwm_is_enabled(pwm)) {
> -		err = clk_prepare_enable(pc->clk);
> -		if (err < 0)
> +		err = pm_runtime_resume_and_get(pc->dev);
> +		if (err)
>  			return err;
>  	} else
>  		val |= PWM_ENABLE;
> @@ -193,7 +197,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * If the PWM is not enabled, turn the clock off again to save power.
>  	 */
>  	if (!pwm_is_enabled(pwm))
> -		clk_disable_unprepare(pc->clk);
> +		pm_runtime_put(pc->dev);
>  
>  	return 0;
>  }
> @@ -204,8 +208,8 @@ static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	int rc = 0;
>  	u32 val;
>  
> -	rc = clk_prepare_enable(pc->clk);
> -	if (rc < 0)
> +	rc = pm_runtime_resume_and_get(pc->dev);
> +	if (rc)
>  		return rc;
>  
>  	val = pwm_readl(pc, pwm->hwpwm);
> @@ -224,7 +228,7 @@ static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	val &= ~PWM_ENABLE;
>  	pwm_writel(pc, pwm->hwpwm, val);
>  
> -	clk_disable_unprepare(pc->clk);
> +	pm_runtime_put_sync(pc->dev);
>  }
>  
>  static const struct pwm_ops tegra_pwm_ops = {
> @@ -256,11 +260,20 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->clk))
>  		return PTR_ERR(pwm->clk);
>  
> +	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	/* Set maximum frequency of the IP */
> -	ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
> +	ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
> -		return ret;
> +		goto put_pm;
>  	}
>  
>  	/*
> @@ -278,7 +291,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->rst)) {
>  		ret = PTR_ERR(pwm->rst);
>  		dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);
> -		return ret;
> +		goto put_pm;
>  	}
>  
>  	reset_control_deassert(pwm->rst);
> @@ -291,10 +304,16 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>  		reset_control_assert(pwm->rst);
> -		return ret;
> +		goto put_pm;
>  	}
>  
> +	pm_runtime_put(&pdev->dev);
> +
>  	return 0;
> +put_pm:
> +	pm_runtime_put_sync_suspend(&pdev->dev);
> +	pm_runtime_force_suspend(&pdev->dev);
> +	return ret;
>  }
>  
>  static int tegra_pwm_remove(struct platform_device *pdev)
> @@ -305,20 +324,44 @@ static int tegra_pwm_remove(struct platform_device *pdev)
>  
>  	reset_control_assert(pc->rst);
>  
> +	pm_runtime_force_suspend(&pdev->dev);
> +
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int tegra_pwm_suspend(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
>  {
> -	return pinctrl_pm_select_sleep_state(dev);
> +	struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> +	int err;
> +
> +	clk_disable_unprepare(pc->clk);
> +
> +	err = pinctrl_pm_select_sleep_state(dev);
> +	if (err) {
> +		clk_prepare_enable(pc->clk);
> +		return err;
> +	}
> +
> +	return 0;
>  }
>  
> -static int tegra_pwm_resume(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_resume(struct device *dev)
>  {
> -	return pinctrl_pm_select_default_state(dev);
> +	struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = pinctrl_pm_select_default_state(dev);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(pc->clk);
> +	if (err) {
> +		pinctrl_pm_select_sleep_state(dev);
> +		return err;
> +	}
> +
> +	return 0;
>  }
> -#endif
>  
>  static const struct tegra_pwm_soc tegra20_pwm_soc = {
>  	.num_channels = 4,
> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>  
>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> +	SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
> +			   NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
>  };
>  
>  static struct platform_driver tegra_pwm_driver = {

I admit to not completely understand the effects of this patch, but I
don't see a problem either. So for me this patch is OK:

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

I spot a problem, it's not introduced by this patch however: If the
consumer of the PWM didn't stop the hardware, the suspend should IMHO be
prevented.

I wonder if the patches in this series go in in one go via an ARM or
Tegra tree, or each patch via its respective maintainer tree.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux