diff mbox series

[v14,01/39] soc/tegra: Enable runtime PM during OPP state-syncing

Message ID 20211025224032.21012-2-digetx@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series NVIDIA Tegra power management patches for 5.17 | expand

Commit Message

Dmitry Osipenko Oct. 25, 2021, 10:39 p.m. UTC
GENPD core now can set up domain's performance state properly while device
is RPM-suspended. Runtime PM of a device must be enabled during setup
because GENPD checks whether device is suspended and check doesn't work
while RPM is disabled. Instead of replicating the boilerplate RPM-enable
code around OPP helper for each driver, let's make OPP helper to take care
of enabling it.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/common.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Ulf Hansson Oct. 27, 2021, 3:06 p.m. UTC | #1
On Tue, 26 Oct 2021 at 00:45, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> GENPD core now can set up domain's performance state properly while device
> is RPM-suspended. Runtime PM of a device must be enabled during setup
> because GENPD checks whether device is suspended and check doesn't work
> while RPM is disabled. Instead of replicating the boilerplate RPM-enable
> code around OPP helper for each driver, let's make OPP helper to take care
> of enabling it.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Just a minor nitpick, see below. Nevertheless feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/soc/tegra/common.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index cd33e99249c3..d930a2b4facc 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -10,6 +10,7 @@
>  #include <linux/export.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_runtime.h>
>
>  #include <soc/tegra/common.h>
>  #include <soc/tegra/fuse.h>
> @@ -43,6 +44,7 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>  {
>         unsigned long rate;
>         struct clk *clk;
> +       bool rpm_enabled;
>         int err;
>
>         clk = devm_clk_get(dev, NULL);
> @@ -57,8 +59,22 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>                 return -EINVAL;
>         }
>
> +       /*
> +        * Runtime PM of the device must be enabled in order to set up
> +        * GENPD's performance properly because GENPD core checks whether
> +        * device is suspended and this check doesn't work while RPM is
> +        * disabled.
> +        */
> +       rpm_enabled = pm_runtime_enabled(dev);
> +       if (!rpm_enabled)
> +               pm_runtime_enable(dev);

This makes sure the OPP vote below gets cached in genpd for the
device. Instead, the vote is done the next time the device gets
runtime resumed.

I don't have an issue doing it like this, but at the same time it does
remove some flexibility for the drivers/subsystem that calls
tegra_core_dev_init_opp_state().

Isn't it better to leave this to be flexible - or you prefer to have
it done like this for everybody?

> +
>         /* first dummy rate-setting initializes voltage vote */
>         err = dev_pm_opp_set_rate(dev, rate);
> +
> +       if (!rpm_enabled)
> +               pm_runtime_disable(dev);
> +
>         if (err) {
>                 dev_err(dev, "failed to initialize OPP clock: %d\n", err);
>                 return err;

Kind regards
Uffe
Dmitry Osipenko Oct. 27, 2021, 7:32 p.m. UTC | #2
27.10.2021 18:06, Ulf Hansson пишет:
> On Tue, 26 Oct 2021 at 00:45, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> GENPD core now can set up domain's performance state properly while device
>> is RPM-suspended. Runtime PM of a device must be enabled during setup
>> because GENPD checks whether device is suspended and check doesn't work
>> while RPM is disabled. Instead of replicating the boilerplate RPM-enable
>> code around OPP helper for each driver, let's make OPP helper to take care
>> of enabling it.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> 
> Just a minor nitpick, see below. Nevertheless feel free to add:
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Kind regards
> Uffe
> 
>> ---
>>  drivers/soc/tegra/common.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
>> index cd33e99249c3..d930a2b4facc 100644
>> --- a/drivers/soc/tegra/common.c
>> +++ b/drivers/soc/tegra/common.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/export.h>
>>  #include <linux/of.h>
>>  #include <linux/pm_opp.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include <soc/tegra/common.h>
>>  #include <soc/tegra/fuse.h>
>> @@ -43,6 +44,7 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>>  {
>>         unsigned long rate;
>>         struct clk *clk;
>> +       bool rpm_enabled;
>>         int err;
>>
>>         clk = devm_clk_get(dev, NULL);
>> @@ -57,8 +59,22 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>>                 return -EINVAL;
>>         }
>>
>> +       /*
>> +        * Runtime PM of the device must be enabled in order to set up
>> +        * GENPD's performance properly because GENPD core checks whether
>> +        * device is suspended and this check doesn't work while RPM is
>> +        * disabled.
>> +        */
>> +       rpm_enabled = pm_runtime_enabled(dev);
>> +       if (!rpm_enabled)
>> +               pm_runtime_enable(dev);
> 
> This makes sure the OPP vote below gets cached in genpd for the
> device. Instead, the vote is done the next time the device gets
> runtime resumed.

Thanks, I'll extend the code's comment with this text in v15.

I also noticed that won't hurt to add extra sanity check of whether RPM
indeed got enabled since it could be disabled multiple times in a
nesting fashion.

> I don't have an issue doing it like this, but at the same time it does
> remove some flexibility for the drivers/subsystem that calls
> tegra_core_dev_init_opp_state().
> 
> Isn't it better to leave this to be flexible - or you prefer to have
> it done like this for everybody?

All the current users of the helper function want this behaviour by
default. It's unlikely that we will ever have a user that will want
different bahaviour, but even then it won't be a problem to add extra
flag to struct tegra_core_opp_params to specify that special case.
diff mbox series

Patch

diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index cd33e99249c3..d930a2b4facc 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -10,6 +10,7 @@ 
 #include <linux/export.h>
 #include <linux/of.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 
 #include <soc/tegra/common.h>
 #include <soc/tegra/fuse.h>
@@ -43,6 +44,7 @@  static int tegra_core_dev_init_opp_state(struct device *dev)
 {
 	unsigned long rate;
 	struct clk *clk;
+	bool rpm_enabled;
 	int err;
 
 	clk = devm_clk_get(dev, NULL);
@@ -57,8 +59,22 @@  static int tegra_core_dev_init_opp_state(struct device *dev)
 		return -EINVAL;
 	}
 
+	/*
+	 * Runtime PM of the device must be enabled in order to set up
+	 * GENPD's performance properly because GENPD core checks whether
+	 * device is suspended and this check doesn't work while RPM is
+	 * disabled.
+	 */
+	rpm_enabled = pm_runtime_enabled(dev);
+	if (!rpm_enabled)
+		pm_runtime_enable(dev);
+
 	/* first dummy rate-setting initializes voltage vote */
 	err = dev_pm_opp_set_rate(dev, rate);
+
+	if (!rpm_enabled)
+		pm_runtime_disable(dev);
+
 	if (err) {
 		dev_err(dev, "failed to initialize OPP clock: %d\n", err);
 		return err;