On Tue, 26 Oct 2021 at 00:45, Dmitry Osipenko <digetx@xxxxxxxxx> 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@xxxxxxxxx>
Just a minor nitpick, see below. Nevertheless feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
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
