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.