Message ID | 20211130232347.950-1-digetx@gmail.com |
---|---|
Headers | show |
Series | NVIDIA Tegra power management patches for 5.17 | expand |
On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote: > This series adds runtime PM support to Tegra drivers and enables core > voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. > > All patches in this series are interdependent and should go via Tegra tree > for simplicity. So these can be applied in any order without breaking anything? Thierry
15.12.2021 18:55, Thierry Reding пишет: > On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote: >> This series adds runtime PM support to Tegra drivers and enables core >> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. >> >> All patches in this series are interdependent and should go via Tegra tree >> for simplicity. > > So these can be applied in any order without breaking anything? Please notice that the word is *inter* dependent, not *in* dependent. There is a build dependency for the patches. The first two "soc/tegra" must be applied first. The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30" *must* be the last applied patch if we want to preserve bisectability. The core voltage scaling can be enabled only once all the drivers got the power management support. The rest could be applied out-of-order.
On Wed, Dec 15, 2021 at 07:11:53PM +0300, Dmitry Osipenko wrote: > 15.12.2021 18:55, Thierry Reding пишет: > > On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote: > >> This series adds runtime PM support to Tegra drivers and enables core > >> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. > >> > >> All patches in this series are interdependent and should go via Tegra tree > >> for simplicity. > > > > So these can be applied in any order without breaking anything? > > Please notice that the word is *inter* dependent, not *in* dependent. > > There is a build dependency for the patches. The first two "soc/tegra" > must be applied first. Okay, so I've separated the first two patches out into a separate stable branch that I can share between the Tegra and drm/tegra trees to pull in the build dependency and then I've applied the driver patches to those two trees and I've verified that the two branches build correctly. I've not done any runtime testing, but I'll trust you on that. > The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30" > *must* be the last applied patch if we want to preserve bisectability. > The core voltage scaling can be enabled only once all the drivers got > the power management support. > > The rest could be applied out-of-order. One last remaining question: I don't think I can apply that one patch if it requires that all the others are enabled first because it would basically create a circular dependency. Can I pick up the final 7 patches (the DT ones) independently of that one patch without things breaking? If so, one option we could try is to wait for both Tegra and drm/tegra trees to get merged into v5.17-rc1 and then send that one patch (which is only a 4-line diff) right after v5.17-rc1 so that it makes it into v5.17-rc2. That avoids the circular dependency and should get everything enabled for v5.17. Do you see any problems with that? Thierry
16.12.2021 16:14, Thierry Reding пишет: > On Wed, Dec 15, 2021 at 07:11:53PM +0300, Dmitry Osipenko wrote: >> 15.12.2021 18:55, Thierry Reding пишет: >>> On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote: >>>> This series adds runtime PM support to Tegra drivers and enables core >>>> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. >>>> >>>> All patches in this series are interdependent and should go via Tegra tree >>>> for simplicity. >>> >>> So these can be applied in any order without breaking anything? >> >> Please notice that the word is *inter* dependent, not *in* dependent. >> >> There is a build dependency for the patches. The first two "soc/tegra" >> must be applied first. > > Okay, so I've separated the first two patches out into a separate stable > branch that I can share between the Tegra and drm/tegra trees to pull in > the build dependency and then I've applied the driver patches to those > two trees and I've verified that the two branches build correctly. I've > not done any runtime testing, but I'll trust you on that. I only compile-tested VIC and NVDEC drivers, but they should be okay, and thus, everything should be good. >> The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30" >> *must* be the last applied patch if we want to preserve bisectability. >> The core voltage scaling can be enabled only once all the drivers got >> the power management support. >> >> The rest could be applied out-of-order. > > One last remaining question: I don't think I can apply that one patch if > it requires that all the others are enabled first because it would > basically create a circular dependency. > > Can I pick up the final 7 patches (the DT ones) independently of that > one patch without things breaking? If so, one option we could try is to > wait for both Tegra and drm/tegra trees to get merged into v5.17-rc1 and > then send that one patch (which is only a 4-line diff) right after > v5.17-rc1 so that it makes it into v5.17-rc2. That avoids the circular > dependency and should get everything enabled for v5.17. > > Do you see any problems with that? Deferring that one patch till v5.17-rc2 will work, thank you.
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