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

[Dmitry Osipenko <digetx@gmail.com>]

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.