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

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@linaro.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/pwm/pwm-tegra.c | 82 ++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 18 deletions(-)

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@linaro.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  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@pengutronix.de>

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

Hello Uwe,

21.02.2022 11:17, Uwe Kleine-König пишет:
>> @@ -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@pengutronix.de>
> 
> 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.

Why? The PWM driver itself will stop the h/w on suspend.

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

This series, including this patch, was already applied to 5.17 via the
tegra/soc tree. No action is needed anymore.

Hello,

On Mon, Feb 21, 2022 at 12:53:58PM +0300, Dmitry Osipenko wrote:
> 21.02.2022 11:17, Uwe Kleine-König пишет:
> >> @@ -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@pengutronix.de>
> > 
> > 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.
> 
> Why? The PWM driver itself will stop the h/w on suspend.

Stopping the PWM might be bad. Only the consumer can know if it's ok to
stop the PWM on suspend. If so the consumer should stop the PWM in their
suspend callback and the PWM should prevent suspend if it wasn't
stopped.

> > 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.
> 
> This series, including this patch, was already applied to 5.17 via the
> tegra/soc tree. No action is needed anymore.

Ah, I missed that, thanks.

Best regards
Uwe