| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220221081727.jeq2jff5ewjzubxv@pengutronix.de>
Date: Mon, 21 Feb 2022 09:17:27 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Dmitry Osipenko <digetx@...il.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Ulf Hansson <ulf.hansson@...aro.org>,
Viresh Kumar <vireshk@...nel.org>,
Stephen Boyd <sboyd@...nel.org>,
Peter De Schrijver <pdeschrijver@...dia.com>,
Mikko Perttunen <mperttunen@...dia.com>,
Lee Jones <lee.jones@...aro.org>, Nishanth Menon <nm@...com>,
Adrian Hunter <adrian.hunter@...el.com>,
Michael Turquette <mturquette@...libre.com>,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
linux-pm@...r.kernel.org, linux-pwm@...r.kernel.org,
linux-mmc@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-clk@...r.kernel.org, David Heidelberg <david@...t.cz>
Subject: Re: [PATCH v16 21/40] pwm: tegra: Add runtime PM and OPP support
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@...aro.org>
> Signed-off-by: Dmitry Osipenko <digetx@...il.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@...gutronix.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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists