lkml.org 
[lkml]   [2021]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v14 01/39] soc/tegra: Enable runtime PM during OPP state-syncing
    From
    Date
    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.

    \
     
     \ /
      Last update: 2021-10-27 21:33    [W:2.516 / U:0.532 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site