| Message ID | 20211025224032.21012-21-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.17 | expand |
26.10.2021 01:40, Dmitry Osipenko пишет: > + ret = devm_pm_runtime_enable(&pdev->dev); > + if (ret) > + return ret; > + > + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > + if (ret) > + return ret; > + > + 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 +294,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 +307,15 @@ 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); > + return ret; > } > > static int tegra_pwm_remove(struct platform_device *pdev) > @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) > > reset_control_assert(pc->rst); > > + pm_runtime_force_suspend(&pdev->dev); I just noticed that RPM core doesn't reset RPM-enable count of a device on driver's unbind (pm_runtime_reinit). It was a bad idea to use devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is disabled twice on driver's removal, and thus, RPM will never be enabled again. I'll fix it for PWM and other drivers in this series, in v15.
On Fri, 29 Oct 2021 at 17:20, Dmitry Osipenko <digetx@gmail.com> wrote: > > 26.10.2021 01:40, Dmitry Osipenko пишет: > > + ret = devm_pm_runtime_enable(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + 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 +294,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 +307,15 @@ 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); > > + return ret; > > } > > > > static int tegra_pwm_remove(struct platform_device *pdev) > > @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) > > > > reset_control_assert(pc->rst); > > > > + pm_runtime_force_suspend(&pdev->dev); > > I just noticed that RPM core doesn't reset RPM-enable count of a device > on driver's unbind (pm_runtime_reinit). It was a bad idea to use > devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is > disabled twice on driver's removal, and thus, RPM will never be enabled > again. > > I'll fix it for PWM and other drivers in this series, in v15. Good catch - and sorry for not spotting it while reviewing! Maybe devm_pm_runtime_enable() isn't that useful after all? Should we suggest to remove it so others don't fall into the same trap? Kind regards Uffe
On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko <digetx@gmail.com> wrote: > > 26.10.2021 01:40, Dmitry Osipenko пишет: > > + ret = devm_pm_runtime_enable(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + 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 +294,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 +307,15 @@ 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); > > + return ret; > > } > > > > static int tegra_pwm_remove(struct platform_device *pdev) > > @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) > > > > reset_control_assert(pc->rst); > > > > + pm_runtime_force_suspend(&pdev->dev); > > I just noticed that RPM core doesn't reset RPM-enable count of a device > on driver's unbind (pm_runtime_reinit). It was a bad idea to use > devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is > disabled twice on driver's removal, and thus, RPM will never be enabled > again. > > I'll fix it for PWM and other drivers in this series, in v15. Well, for the record, IMV using pm_runtime_force_suspend() is generally a questionable idea.
29.10.2021 18:28, Ulf Hansson пишет: > On Fri, 29 Oct 2021 at 17:20, Dmitry Osipenko <digetx@gmail.com> wrote: >> >> 26.10.2021 01:40, Dmitry Osipenko пишет: >>> + ret = devm_pm_runtime_enable(&pdev->dev); >>> + if (ret) >>> + return ret; >>> + >>> + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); >>> + if (ret) >>> + return ret; >>> + >>> + 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 +294,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 +307,15 @@ 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); >>> + return ret; >>> } >>> >>> static int tegra_pwm_remove(struct platform_device *pdev) >>> @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) >>> >>> reset_control_assert(pc->rst); >>> >>> + pm_runtime_force_suspend(&pdev->dev); >> >> I just noticed that RPM core doesn't reset RPM-enable count of a device >> on driver's unbind (pm_runtime_reinit). It was a bad idea to use >> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is >> disabled twice on driver's removal, and thus, RPM will never be enabled >> again. >> >> I'll fix it for PWM and other drivers in this series, in v15. > > Good catch - and sorry for not spotting it while reviewing! > > Maybe devm_pm_runtime_enable() isn't that useful after all? Should we > suggest to remove it so others don't fall into the same trap? devm_pm_runtime_enable() was added to the recent v5.15 kernel. It's a useful helper, if it's used consciously. I'm not going to remove its usage entirely from this series, for example it still should be good to use for Tegra FUSE and HDMI drivers.
29.10.2021 18:56, Rafael J. Wysocki пишет: > On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> >> 26.10.2021 01:40, Dmitry Osipenko пишет: >>> + ret = devm_pm_runtime_enable(&pdev->dev); >>> + if (ret) >>> + return ret; >>> + >>> + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); >>> + if (ret) >>> + return ret; >>> + >>> + 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 +294,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 +307,15 @@ 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); >>> + return ret; >>> } >>> >>> static int tegra_pwm_remove(struct platform_device *pdev) >>> @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) >>> >>> reset_control_assert(pc->rst); >>> >>> + pm_runtime_force_suspend(&pdev->dev); >> >> I just noticed that RPM core doesn't reset RPM-enable count of a device >> on driver's unbind (pm_runtime_reinit). It was a bad idea to use >> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is >> disabled twice on driver's removal, and thus, RPM will never be enabled >> again. >> >> I'll fix it for PWM and other drivers in this series, in v15. > > Well, for the record, IMV using pm_runtime_force_suspend() is > generally a questionable idea. > Please clarify why it's a questionable idea.
On Fri, Oct 29, 2021 at 6:29 PM Dmitry Osipenko <digetx@gmail.com> wrote: > > 29.10.2021 18:56, Rafael J. Wysocki пишет: > > On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko <digetx@gmail.com> wrote: > >> > >> 26.10.2021 01:40, Dmitry Osipenko пишет: > >>> + ret = devm_pm_runtime_enable(&pdev->dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + 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 +294,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 +307,15 @@ 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); > >>> + return ret; > >>> } > >>> > >>> static int tegra_pwm_remove(struct platform_device *pdev) > >>> @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev) > >>> > >>> reset_control_assert(pc->rst); > >>> > >>> + pm_runtime_force_suspend(&pdev->dev); > >> > >> I just noticed that RPM core doesn't reset RPM-enable count of a device > >> on driver's unbind (pm_runtime_reinit). It was a bad idea to use > >> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is > >> disabled twice on driver's removal, and thus, RPM will never be enabled > >> again. > >> > >> I'll fix it for PWM and other drivers in this series, in v15. > > > > Well, for the record, IMV using pm_runtime_force_suspend() is > > generally a questionable idea. > > > > Please clarify why it's a questionable idea. There are a few reasons. Generally speaking, it makes assumptions that may not be satisfied. For instance, it assumes that the driver will never have to work with the ACPI PM domain, because the ACPI PM domain has a separate set of callbacks for system-wide suspend and resume and they are not the same as its PM-runtime callbacks, so if the driver is combined with the ACPI PM domain, running pm_runtime_force_suspend() may not work as expected. Next, it assumes that PM-runtime is actually enabled for the device and the RPM_STATUS of it is valid when it is running. Further, it assumes that the PM-runtime suspend callback of the driver will always be suitable for system-wide suspend which may not be the case if the device can generate wakeup signals and it is not allowed to wake up the system from sleep by user space. Next, if the driver has to work with a PM domain (other than the ACPI one) or bus type that doesn't take the pm_runtime_force_suspend() explicitly into account, it may end up running the runtime-suspend callback provided by that entity from within its system-wide suspend callback which may not work as expected. I guess I could add a few if I had to.
29.10.2021 21:06, Rafael J. Wysocki пишет: ... >>>> I just noticed that RPM core doesn't reset RPM-enable count of a device >>>> on driver's unbind (pm_runtime_reinit). It was a bad idea to use >>>> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is >>>> disabled twice on driver's removal, and thus, RPM will never be enabled >>>> again. >>>> >>>> I'll fix it for PWM and other drivers in this series, in v15. >>> >>> Well, for the record, IMV using pm_runtime_force_suspend() is >>> generally a questionable idea. >>> >> >> Please clarify why it's a questionable idea. > > There are a few reasons. > > Generally speaking, it makes assumptions that may not be satisfied. > > For instance, it assumes that the driver will never have to work with > the ACPI PM domain, because the ACPI PM domain has a separate set of > callbacks for system-wide suspend and resume and they are not the same > as its PM-runtime callbacks, so if the driver is combined with the > ACPI PM domain, running pm_runtime_force_suspend() may not work as > expected. ACPI is irrelevant to the drivers touched by this series. This series is about older ARM32 Tegra SoCs which either don't have ACPI at all or it's unusable by Linux, like a non-standard ACPI of M$ Surface tablets. > Next, it assumes that PM-runtime is actually enabled for the device > and the RPM_STATUS of it is valid when it is running. Runtime PM presence is mandatory for Tegra and drivers take care of enabling it, should be good here. > Further, it assumes that the PM-runtime suspend callback of the driver > will always be suitable for system-wide suspend which may not be the > case if the device can generate wakeup signals and it is not allowed > to wake up the system from sleep by user space. There are no such 'wakeup' drivers in the context of this patchset. > Next, if the driver has to work with a PM domain (other than the ACPI > one) or bus type that doesn't take the pm_runtime_force_suspend() > explicitly into account, it may end up running the runtime-suspend > callback provided by that entity from within its system-wide suspend > callback which may not work as expected. Only platform bus and generic power domain are relevant for this patchset. > I guess I could add a few if I had to. > So far I can't see any problems. If you have a better alternative on yours mind, please share.
30.10.2021 03:47, Dmitry Osipenko пишет: > 29.10.2021 21:06, Rafael J. Wysocki пишет: > ... >>>>> I just noticed that RPM core doesn't reset RPM-enable count of a device >>>>> on driver's unbind (pm_runtime_reinit). It was a bad idea to use >>>>> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is >>>>> disabled twice on driver's removal, and thus, RPM will never be enabled >>>>> again. >>>>> >>>>> I'll fix it for PWM and other drivers in this series, in v15. >>>> >>>> Well, for the record, IMV using pm_runtime_force_suspend() is >>>> generally a questionable idea. >>>> >>> >>> Please clarify why it's a questionable idea. >> >> There are a few reasons. >> >> Generally speaking, it makes assumptions that may not be satisfied. >> >> For instance, it assumes that the driver will never have to work with >> the ACPI PM domain, because the ACPI PM domain has a separate set of >> callbacks for system-wide suspend and resume and they are not the same >> as its PM-runtime callbacks, so if the driver is combined with the >> ACPI PM domain, running pm_runtime_force_suspend() may not work as >> expected. > > ACPI is irrelevant to the drivers touched by this series. > > This series is about older ARM32 Tegra SoCs which either don't have ACPI > at all or it's unusable by Linux, like a non-standard ACPI of M$ Surface > tablets. Although, there are VIC and NVDEC drivers of newer Tegra SoCs touched by this series. Maybe they could get ACPI support in the future, but this needs to be clarified. Perhaps Thierry or Mikko could comment on it. >> Next, it assumes that PM-runtime is actually enabled for the device >> and the RPM_STATUS of it is valid when it is running. > > Runtime PM presence is mandatory for Tegra and drivers take care of > enabling it, should be good here. > >> Further, it assumes that the PM-runtime suspend callback of the driver >> will always be suitable for system-wide suspend which may not be the >> case if the device can generate wakeup signals and it is not allowed >> to wake up the system from sleep by user space. > > There are no such 'wakeup' drivers in the context of this patchset. > >> Next, if the driver has to work with a PM domain (other than the ACPI >> one) or bus type that doesn't take the pm_runtime_force_suspend() >> explicitly into account, it may end up running the runtime-suspend >> callback provided by that entity from within its system-wide suspend >> callback which may not work as expected. > > Only platform bus and generic power domain are relevant for this patchset. > >> I guess I could add a few if I had to. >> > > So far I can't see any problems. > > If you have a better alternative on yours mind, please share. >
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index 11a10b575ace..0ce55644d89f 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,23 @@ static int tegra_pwm_probe(struct platform_device *pdev) if (IS_ERR(pwm->clk)) return PTR_ERR(pwm->clk); + ret = devm_pm_runtime_enable(&pdev->dev); + if (ret) + return ret; + + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); + if (ret) + return ret; + + 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 +294,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 +307,15 @@ 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); + return ret; } static int tegra_pwm_remove(struct platform_device *pdev) @@ -305,20 +326,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 +389,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 = {
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. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/pwm/pwm-tegra.c | 84 ++++++++++++++++++++++++++++++++--------- 1 file changed, 66 insertions(+), 18 deletions(-)