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 > Signed-off-by: Dmitry Osipenko > --- > 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 > #include > #include > +#include > #include > #include > #include > +#include > #include > #include > > +#include > + > #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 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/ |