Message-ID: <b887de8c-a40b-a62e-8abf-698e67cdb70c@gmail.com>
Date:   Sun, 22 Aug 2021 21:35:36 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>,
        Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        Peter Chen <peter.chen@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Lucas Stach <dev@...xeye.de>, Stefan Agner <stefan@...er.ch>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        linux-staging@...ts.linux.dev, linux-spi@...r.kernel.org,
        linux-pwm@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        DTML <devicetree@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>
Subject: Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper

20.08.2021 08:18, Viresh Kumar пишет:
> On 19-08-21, 16:55, Ulf Hansson wrote:
>> Right, that sounds reasonable.
>>
>> We already have pm_genpd_opp_to_performance_state() which translates
>> an OPP to a performance state. This function invokes the
>> ->opp_to_performance_state() for a genpd. Maybe we need to allow a
>> genpd to not have ->opp_to_performance_state() callback assigned
>> though, but continue up in the hierarchy to see if the parent has the
>> callback assigned, to make this work for Tegra?
>>
>> Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
>> allowing us to pass the device instead of the genpd. But that's a
>> minor thing.
> 
> I am not concerned a lot about how it gets implemented, and am not
> sure as well, as I haven't looked into these details since sometime.
> Any reasonable thing will be accepted, as simple as that.
> 
>> Finally, the precondition to use the above, is to first get a handle
>> to an OPP table. This is where I am struggling to find a generic
>> solution, because I guess that would be platform or even consumer
>> driver specific for how to do this. And at what point should we do
>> this?

GENPD core can't get OPP table handle, setting up OPP table is a platform/driver specific operation.

> Hmm, I am not very clear with the whole picture at this point of time.
> 
> Dmitry, can you try to frame a sequence of events/calls/etc that will
> define what kind of devices we are looking at here, and how this can
> be made to work ?

Could you please clarify what do you mean by a "kind of devices"?

I made hack based on the recent discussions and it partially works. Getting clock rate involves resuming device which backs the clock and it also may use GENPD, so lockings are becoming complicated. It doesn't work at all if device uses multiple domains because virtual domain device doesn't have OPP table.

Setting up the performance state from a consumer driver is a cleaner variant so far. 

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e1c8994ae225..faa0bbe99c98 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -410,11 +410,16 @@ static int genpd_drop_performance_state(struct device *dev)
 	return 0;
 }
 
-static void genpd_restore_performance_state(struct device *dev,
-					    unsigned int state)
+static int genpd_restore_performance_state(struct generic_pm_domain *genpd,
+					   struct device *dev,
+					   unsigned int state)
 {
+	int ret = 0;
+
 	if (state)
-		genpd_set_performance_state(dev, state);
+		ret = genpd_set_performance_state(dev, state);
+
+	return ret;
 }
 
 /**
@@ -435,7 +440,7 @@ static void genpd_restore_performance_state(struct device *dev,
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 {
 	struct generic_pm_domain *genpd;
-	int ret;
+	int ret = 0;
 
 	genpd = dev_to_genpd_safe(dev);
 	if (!genpd)
@@ -446,7 +451,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 		return -EINVAL;
 
 	genpd_lock(genpd);
-	ret = genpd_set_performance_state(dev, state);
+	if (pm_runtime_suspended(dev))
+		dev_gpd_data(dev)->rpm_pstate = state;
+	else
+		ret = genpd_set_performance_state(dev, state);
 	genpd_unlock(genpd);
 
 	return ret;
@@ -959,10 +967,25 @@ static int genpd_runtime_resume(struct device *dev)
 		goto out;
 	}
 
+	if (genpd->get_performance_state) {
+		ret = genpd->get_performance_state(genpd, dev);
+		if (ret < 0)
+			return ret;
+
+		if (ret > 0)
+			gpd_data->rpm_pstate = ret;
+	}
+
 	genpd_lock(genpd);
 	ret = genpd_power_on(genpd, 0);
-	if (!ret)
-		genpd_restore_performance_state(dev, gpd_data->rpm_pstate);
+	if (!ret) {
+		ret = genpd_restore_performance_state(genpd, dev,
+						      gpd_data->rpm_pstate);
+		if (ret)
+			genpd_power_off(genpd, true, 0);
+		else
+			gpd_data->rpm_pstate = 0;
+	}
 	genpd_unlock(genpd);
 
 	if (ret)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 18016e49605f..982be2dba21e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2967,3 +2967,33 @@ int dev_pm_opp_sync(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync);
+
+/**
+ * dev_pm_opp_from_clk_rate() - Get OPP from current clock rate
+ * @dev:	device for which we do this operation
+ *
+ * Get OPP which corresponds to the current clock rate of a device.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
+	struct opp_table *opp_table;
+	unsigned long freq;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	if (!IS_ERR(opp_table->clk)) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_from_clk_rate);
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 7c9bc93147f1..03bad16e5318 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -506,6 +506,63 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
 		writel(value, pmc->scratch + offset);
 }
 
+static const char * const tegra_skip_compats[] = {
+	"nvidia,tegra20-sclk",
+	"nvidia,tegra30-sclk",
+	"nvidia,tegra30-pllc",
+	"nvidia,tegra30-plle",
+	"nvidia,tegra30-pllm",
+	"nvidia,tegra20-dc",
+	"nvidia,tegra30-dc",
+	"nvidia,tegra20-emc",
+	"nvidia,tegra30-emc",
+	NULL,
+};
+
+static int tegra_pmc_pd_get_performance_state(struct generic_pm_domain *genpd,
+					      struct device *dev)
+{
+	struct dev_pm_opp *opp;
+	int ret;
+
+	/*
+	 * Tegra114+ SocS don't support OPP yet.  But if they will get OPP
+	 * support, then we want to skip OPP for older kernels to preserve
+	 * compatibility of newer DTBs with older kernels.
+	 */
+	if (!pmc->soc->supports_core_domain)
+		return 0;
+
+	/*
+	 * The EMC devices are a special case because we have a protection
+	 * from non-EMC drivers getting clock handle before EMC driver is
+	 * fully initialized.  The goal of the protection is to prevent
+	 * devfreq driver from getting failures if it will try to change
+	 * EMC clock rate until clock is fully initialized.  The EMC drivers
+	 * will initialize the performance state by themselves.
+	 *
+	 * Display controller also is a special case because only controller
+	 * driver could get the clock rate based on configuration of internal
+	 * divider.
+	 *
+	 * Clock driver uses its own state syncing.
+	 */
+	if (of_device_compatible_match(dev->of_node, tegra_skip_compats))
+		return 0;
+
+	opp = dev_pm_opp_from_clk_rate(dev);
+	if (IS_ERR(opp)) {
+		dev_err(&genpd->dev, "failed to get current OPP for %s: %pe\n",
+			dev_name(dev), opp);
+		ret = PTR_ERR(opp);
+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	return ret;
+}
+
 /*
  * TODO Figure out a way to call this with the struct tegra_pmc * passed in.
  * This currently doesn't work because readx_poll_timeout() can only operate
@@ -1238,6 +1295,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 
 	pg->id = id;
 	pg->genpd.name = np->name;
+	pg->genpd.get_performance_state = tegra_pmc_pd_get_performance_state;
 	pg->genpd.power_off = tegra_genpd_power_off;
 	pg->genpd.power_on = tegra_genpd_power_on;
 	pg->pmc = pmc;
@@ -1354,6 +1412,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
 		return -ENOMEM;
 
 	genpd->name = "core";
+	genpd->get_performance_state = tegra_pmc_pd_get_performance_state;
 	genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
 	genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 67017c9390c8..abe33be9828f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -133,6 +133,8 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*get_performance_state)(struct generic_pm_domain *genpd,
+				     struct device *dev);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t next_wakeup;	/* Maintained by the domain governor */
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 686122b59935..e7fd0dd493ca 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -169,6 +169,7 @@ void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
 int dev_pm_opp_sync(struct device *dev);
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -440,6 +441,11 @@ static inline int dev_pm_opp_sync(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 

Powered by blists - more mailing lists