| Message ID | 20211025224032.21012-2-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.17 | expand |
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
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 --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;
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(+)