[PATCH v9 0/8] NVIDIA Tegra power management patches for 5.16

[PATCH v9 0/8] NVIDIA Tegra power management patches for 5.16

[Dmitry Osipenko]

This is a reduced version of the patchset which adds power management
support to NVIDIA Tegra drivers. Viresh Kumar asked to send these PD/OPP
patches separately for now to reduce the noise and finalize the review.

I implemented new get_performance_state() GENPD callback as was discussed
in v8. GR3D driver patch shows how it's used by consumer drivers, which
is a good example because 3d driver supports both cases of a single and
multi-domain hardware, it also uses OPP API more extensively than other
drivers.

Dmitry Osipenko (8):
  opp: Add dev_pm_opp_from_clk_rate()
  opp: Allow dev_pm_opp_set_clkname() to replace released clock
  opp: Change type of dev_pm_opp_attach_genpd(names) argument
  PM: domains: Add get_performance_state() callback
  soc/tegra: pmc: Implement get_performance_state() callback
  soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple()
  gpu: host1x: Add host1x_channel_stop()
  drm/tegra: gr3d: Support generic power domain and runtime PM

 drivers/base/power/domain.c  |  32 ++-
 drivers/gpu/drm/tegra/gr3d.c | 384 ++++++++++++++++++++++++++++++-----
 drivers/gpu/host1x/channel.c |   8 +
 drivers/opp/core.c           |  50 ++++-
 drivers/soc/tegra/pmc.c      |  86 ++++++++
 include/linux/host1x.h       |   1 +
 include/linux/pm_domain.h    |   2 +
 include/linux/pm_opp.h       |  14 +-
 include/soc/tegra/common.h   |  13 ++
 9 files changed, 522 insertions(+), 68 deletions(-)

-- 
2.32.0

[PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[Dmitry Osipenko]

Add dev_pm_opp_from_clk_rate() helper that returns OPP corresponding
to the current clock rate of a device.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 42 +++++++++++++++++++++++++++++++++++++++---
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 04b4691a8aac..fae5267f5218 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -939,7 +939,8 @@ static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+static struct dev_pm_opp *
+_find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;
@@ -961,7 +962,7 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		mutex_unlock(&opp_table->lock);
 	}
 
-	opp_table->current_opp = opp;
+	return opp;
 }
 
 static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
@@ -1003,7 +1004,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 
 	/* Find the currently set OPP if we don't know already */
 	if (unlikely(!opp_table->current_opp))
-		_find_current_opp(dev, opp_table);
+		opp_table->current_opp = _find_current_opp(dev, opp_table);
 
 	old_opp = opp_table->current_opp;
 
@@ -2931,3 +2932,38 @@ int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * 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(-ENOENT);
+	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)) {
+		opp = ERR_CAST(opp_table->clk);
+		goto put_table;
+	}
+
+	if (opp_table->clk) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+put_table:
+	/* 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/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 84150a22fd7c..57e75144dd88 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -168,6 +168,7 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 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);
+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)
 {
@@ -434,6 +435,11 @@ static inline int dev_pm_opp_sync_regulators(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)
-- 
2.32.0

[PATCH v9 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock

[Dmitry Osipenko]

The opp_table->clk is set to error once clock is released by
dev_pm_opp_put_clkname(). This doesn't allow to set clock again,
until OPP table is re-created from scratch. Check opp_table->clk for
both NULL and ERR_PTR to allow clock replacement. This is needed now
by NVIDIA Tegra 3d driver for initializing performance state of multiple
power domains, where PD driver sets and unsets OPP table clock while OPP
table reference is held outside of PD.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fae5267f5218..e26da1d4d6be 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2136,7 +2136,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	}
 
 	/* clk shouldn't be initialized at this point */
-	if (WARN_ON(opp_table->clk)) {
+	if (WARN_ON(!IS_ERR_OR_NULL(opp_table->clk))) {
 		ret = -EBUSY;
 		goto err;
 	}
-- 
2.32.0

[PATCH v9 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument

[Dmitry Osipenko]

Elements of the 'names' array are not changed by the code, constify them
for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 6 +++---
 include/linux/pm_opp.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e26da1d4d6be..8d947e4f8c68 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2349,12 +2349,12 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
  * "required-opps" are added in DT.
  */
 struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
-		const char **names, struct device ***virt_devs)
+		const char * const *names, struct device ***virt_devs)
 {
 	struct opp_table *opp_table;
 	struct device *virt_dev;
 	int index = 0, ret = -EINVAL;
-	const char **name = names;
+	const char * const *name = names;
 
 	opp_table = _add_opp_table(dev, false);
 	if (IS_ERR(opp_table))
@@ -2458,7 +2458,7 @@ static void devm_pm_opp_detach_genpd(void *data)
  *
  * Return: 0 on success and errorno otherwise.
  */
-int devm_pm_opp_attach_genpd(struct device *dev, const char **names,
+int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
 			     struct device ***virt_devs)
 {
 	struct opp_table *opp_table;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 57e75144dd88..b77f2b41c30b 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -156,9 +156,9 @@ int devm_pm_opp_set_clkname(struct device *dev, const char *name);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 int devm_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
-struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs);
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
-int devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs);
 struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
@@ -377,7 +377,7 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name)
 	return -EOPNOTSUPP;
 }
 
-static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs)
+static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
@@ -385,7 +385,7 @@ static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, cons
 static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {}
 
 static inline int devm_pm_opp_attach_genpd(struct device *dev,
-					   const char **names,
+					   const char * const *names,
 					   struct device ***virt_devs)
 {
 	return -EOPNOTSUPP;
-- 
2.32.0

[PATCH v9 4/8] PM: domains: Add get_performance_state() callback

[Dmitry Osipenko]

Add get_performance_state() callback that retrieves and initializes
performance state of a device attached to a power domain. This removes
inconsistency of the performance state with hardware state.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/base/power/domain.c | 32 +++++++++++++++++++++++++++++---
 include/linux/pm_domain.h   |  2 ++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3a13a942d012..8b828dcdf7f8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2700,15 +2700,41 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 		goto err;
 	} else if (pstate > 0) {
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
-		if (ret)
+		if (ret) {
+			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
+				pd->name, ret);
 			goto err;
+		}
 		dev_gpd_data(dev)->default_pstate = pstate;
 	}
+
+	if (pd->get_performance_state && !dev_gpd_data(dev)->default_pstate) {
+		bool dev_suspended = false;
+
+		ret = pd->get_performance_state(pd, base_dev, &dev_suspended);
+		if (ret < 0) {
+			dev_err(dev, "failed to get performance state for power-domain %s: %d\n",
+				pd->name, ret);
+			goto err;
+		}
+
+		pstate = ret;
+
+		if (dev_suspended) {
+			dev_gpd_data(dev)->rpm_pstate = pstate;
+		} else if (pstate > 0) {
+			ret = dev_pm_genpd_set_performance_state(dev, pstate);
+			if (ret) {
+				dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
+					pd->name, ret);
+				goto err;
+			}
+		}
+	}
+
 	return 1;
 
 err:
-	dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
-		pd->name, ret);
 	genpd_remove_device(pd, dev);
 	return ret;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 67017c9390c8..4f78b31791ae 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, bool *dev_suspended);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t next_wakeup;	/* Maintained by the domain governor */
-- 
2.32.0

[PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

Implement get_performance_state() callback of power domains to
initialize theirs performance state in accordance to the clock
rate of attached device.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/pmc.c | 86 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 50091c4ec948..ea552f7ed922 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -505,6 +505,90 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
 		writel(value, pmc->scratch + offset);
 }
 
+static const char * const tegra_pd_no_perf_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,
+					      bool *dev_suspended)
+{
+	struct opp_table *hw_opp_table, *clk_opp_table;
+	struct dev_pm_opp *opp;
+	u32 hw_version;
+	int ret;
+
+	/*
+	 * 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_pd_no_perf_compats))
+		return 0;
+
+	if (of_machine_is_compatible("nvidia,tegra20"))
+		hw_version = BIT(tegra_sku_info.soc_process_id);
+	else
+		hw_version = BIT(tegra_sku_info.soc_speedo_id);
+
+	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
+	if (IS_ERR(hw_opp_table)) {
+		dev_err(dev, "failed to set OPP supported HW: %pe\n",
+			hw_opp_table);
+		return PTR_ERR(hw_opp_table);
+	}
+
+	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
+	if (IS_ERR(clk_opp_table)) {
+		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
+		ret = PTR_ERR(clk_opp_table);
+		goto put_hw;
+	}
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret) {
+		dev_err(dev, "failed to add OPP table: %d\n", ret);
+		goto put_clk;
+	}
+
+	opp = dev_pm_opp_from_clk_rate(dev);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "failed to get current OPP: %pe\n", opp);
+		ret = PTR_ERR(opp);
+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	*dev_suspended = true;
+
+	dev_pm_opp_of_remove_table(dev);
+put_clk:
+	dev_pm_opp_put_clkname(clk_opp_table);
+put_hw:
+	dev_pm_opp_put_supported_hw(hw_opp_table);
+
+	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
@@ -1237,6 +1321,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;
@@ -1353,6 +1438,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
 		return -ENOMEM;
 
 	genpd->name = np->name;
+	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;
 
-- 
2.32.0

[PATCH v9 6/8] soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple()

[Dmitry Osipenko]

Only couple drivers need to get the -ENODEV error code and explicitly
initialize the performance state. Add new helper that allows to avoid
the extra boilerplate code in majority of drivers.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/soc/tegra/common.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index af41ad80ec21..265ad90e45a2 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -39,4 +39,17 @@ devm_tegra_core_dev_init_opp_table(struct device *dev,
 }
 #endif
 
+static inline int
+devm_tegra_core_dev_init_opp_table_simple(struct device *dev)
+{
+	struct tegra_core_opp_params params = {};
+	int err;
+
+	err = devm_tegra_core_dev_init_opp_table(dev, &params);
+	if (err != -ENODEV)
+		return err;
+
+	return 0;
+}
+
 #endif /* __SOC_TEGRA_COMMON_H__ */
-- 
2.32.0

[PATCH v9 7/8] gpu: host1x: Add host1x_channel_stop()

[Dmitry Osipenko]

Add host1x_channel_stop() which waits till channel becomes idle and then
stops the channel hardware. This is needed for supporting suspend/resume
by host1x drivers since the hardware state is lost after power-gating,
thus the channel needs to be stopped before client enters into suspend.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/host1x/channel.c | 8 ++++++++
 include/linux/host1x.h       | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
index 4cd212bb570d..2a9a3a8d5931 100644
--- a/drivers/gpu/host1x/channel.c
+++ b/drivers/gpu/host1x/channel.c
@@ -75,6 +75,14 @@ struct host1x_channel *host1x_channel_get_index(struct host1x *host,
 	return ch;
 }
 
+void host1x_channel_stop(struct host1x_channel *channel)
+{
+	struct host1x *host = dev_get_drvdata(channel->dev->parent);
+
+	host1x_hw_cdma_stop(host, &channel->cdma);
+}
+EXPORT_SYMBOL(host1x_channel_stop);
+
 static void release_channel(struct kref *kref)
 {
 	struct host1x_channel *channel =
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 7bccf589aba7..66473b5be0af 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -181,6 +181,7 @@ struct host1x_job;
 
 struct host1x_channel *host1x_channel_request(struct host1x_client *client);
 struct host1x_channel *host1x_channel_get(struct host1x_channel *channel);
+void host1x_channel_stop(struct host1x_channel *channel);
 void host1x_channel_put(struct host1x_channel *channel);
 int host1x_job_submit(struct host1x_job *job);
 
-- 
2.32.0

[PATCH v9 8/8] drm/tegra: gr3d: Support generic power domain and runtime PM

[Dmitry Osipenko]

Add power management and support generic power domains.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/gr3d.c | 384 ++++++++++++++++++++++++++++++-----
 1 file changed, 330 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 24442ade0da3..545eb4005a96 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -5,32 +5,47 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/host1x.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
+#include <soc/tegra/common.h>
 #include <soc/tegra/pmc.h>
 
 #include "drm.h"
 #include "gem.h"
 #include "gr3d.h"
 
+enum {
+	RST_MC,
+	RST_GR3D,
+	RST_MC2,
+	RST_GR3D2,
+	RST_GR3D_MAX,
+};
+
 struct gr3d_soc {
 	unsigned int version;
+	unsigned int num_clocks;
+	unsigned int num_resets;
 };
 
 struct gr3d {
 	struct tegra_drm_client client;
 	struct host1x_channel *channel;
-	struct clk *clk_secondary;
-	struct clk *clk;
-	struct reset_control *rst_secondary;
-	struct reset_control *rst;
 
 	const struct gr3d_soc *soc;
+	struct clk_bulk_data *clocks;
+	unsigned int nclocks;
+	struct reset_control_bulk_data resets[RST_GR3D_MAX];
+	unsigned int nresets;
 
 	DECLARE_BITMAP(addr_regs, GR3D_NUM_REGS);
 };
@@ -109,16 +124,24 @@ static int gr3d_open_channel(struct tegra_drm_client *client,
 			     struct tegra_drm_context *context)
 {
 	struct gr3d *gr3d = to_gr3d(client);
+	int err;
 
 	context->channel = host1x_channel_get(gr3d->channel);
 	if (!context->channel)
 		return -ENOMEM;
 
+	err = pm_runtime_resume_and_get(client->base.dev);
+	if (err) {
+		host1x_channel_put(context->channel);
+		return err;
+	}
+
 	return 0;
 }
 
 static void gr3d_close_channel(struct tegra_drm_context *context)
 {
+	pm_runtime_put_sync(context->client->base.dev);
 	host1x_channel_put(context->channel);
 }
 
@@ -155,14 +178,20 @@ static const struct tegra_drm_client_ops gr3d_ops = {
 
 static const struct gr3d_soc tegra20_gr3d_soc = {
 	.version = 0x20,
+	.num_clocks = 1,
+	.num_resets = 2,
 };
 
 static const struct gr3d_soc tegra30_gr3d_soc = {
 	.version = 0x30,
+	.num_clocks = 2,
+	.num_resets = 4,
 };
 
 static const struct gr3d_soc tegra114_gr3d_soc = {
 	.version = 0x35,
+	.num_clocks = 1,
+	.num_resets = 2,
 };
 
 static const struct of_device_id tegra_gr3d_match[] = {
@@ -278,9 +307,211 @@ static const u32 gr3d_addr_regs[] = {
 	GR3D_GLOBAL_SAMP23SURFADDR(15),
 };
 
+static int gr3d_power_up_legacy_domain(struct device *dev, const char *name,
+				       unsigned int id)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	struct reset_control *reset;
+	struct clk *clk;
+	unsigned int i;
+	int err;
+
+	/*
+	 * Tegra20 device-tree doesn't specify 3d clock name and there is only
+	 * one clock for Tegra20. Tegra30+ device-trees always specified names
+	 * for the clocks.
+	 */
+	if (gr3d->nclocks == 1) {
+		if (id == TEGRA_POWERGATE_3D1)
+			return 0;
+
+		clk = gr3d->clocks[0].clk;
+	} else {
+		for (i = 0; i < gr3d->nclocks; i++) {
+			if (WARN_ON(!gr3d->clocks[i].id))
+				continue;
+
+			if (!strcmp(gr3d->clocks[i].id, name)) {
+				clk = gr3d->clocks[i].clk;
+				break;
+			}
+		}
+
+		if (WARN_ON(i == gr3d->nclocks))
+			return -EINVAL;
+	}
+
+	/*
+	 * We use array of resets, which includes MC resets, and MC
+	 * reset shouldn't be asserted while hardware is gated because
+	 * MC flushing will fail for gated hardware. Hence for legacy
+	 * PD we request the individual reset separately.
+	 */
+	reset = reset_control_get_exclusive_released(dev, name);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	err = reset_control_acquire(reset);
+	if (err) {
+		dev_err(dev, "failed to acquire %s reset: %d\n", name, err);
+	} else {
+		err = tegra_powergate_sequence_power_up(id, clk, reset);
+		reset_control_release(reset);
+	}
+
+	reset_control_put(reset);
+	if (err)
+		return err;
+
+	/*
+	 * tegra_powergate_sequence_power_up() leaves clocks enabled
+	 * while GENPD not, hence keep clock-enable balanced.
+	 */
+	clk_disable_unprepare(clk);
+
+	return 0;
+}
+
+static void gr3d_del_link(void *link)
+{
+	device_link_del(link);
+}
+
+static int gr3d_init_power(struct device *dev, struct gr3d *gr3d)
+{
+	static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
+	const u32 link_flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME;
+	struct device **opp_virt_devs, *pd_dev;
+	struct device_link *link;
+	unsigned int i;
+	int err;
+
+	err = of_count_phandle_with_args(dev->of_node, "power-domains",
+					 "#power-domain-cells");
+	if (err < 0) {
+		if (err != -ENOENT)
+			return err;
+
+		/*
+		 * Older device-trees don't use GENPD. In this case we should
+		 * toggle power domain manually.
+		 */
+		err = gr3d_power_up_legacy_domain(dev, "3d",
+						  TEGRA_POWERGATE_3D);
+		if (err)
+			return err;
+
+		err = gr3d_power_up_legacy_domain(dev, "3d2",
+						  TEGRA_POWERGATE_3D1);
+		if (err)
+			return err;
+
+		return 0;
+	}
+
+	/*
+	 * The PM domain core automatically attaches a single power domain,
+	 * otherwise it skips attaching completely. We have a single domain
+	 * on Tegra20 and two domains on Tegra30+.
+	 */
+	if (dev->pm_domain)
+		return 0;
+
+	err = devm_pm_opp_attach_genpd(dev, opp_genpd_names, &opp_virt_devs);
+	if (err)
+		return err;
+
+	for (i = 0; opp_genpd_names[i]; i++) {
+		pd_dev = opp_virt_devs[i];
+		if (!pd_dev) {
+			dev_err(dev, "failed to get %s power domain\n",
+				opp_genpd_names[i]);
+			return -EINVAL;
+		}
+
+		link = device_link_add(dev, pd_dev, link_flags);
+		if (!link) {
+			dev_err(dev, "failed to link to %s\n", dev_name(pd_dev));
+			return -EINVAL;
+		}
+
+		err = devm_add_action_or_reset(dev, gr3d_del_link, link);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int gr3d_set_opp(struct dev_pm_set_opp_data *data)
+{
+	struct gr3d *gr3d = dev_get_drvdata(data->dev);
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < gr3d->nclocks; i++) {
+		err = clk_set_rate(gr3d->clocks[i].clk, data->new_opp.rate);
+		if (err) {
+			dev_err(data->dev, "failed to set %s rate to %lu: %d\n",
+				gr3d->clocks[i].id, data->new_opp.rate, err);
+			goto restore;
+		}
+	}
+
+	return 0;
+
+restore:
+	while (i--)
+		clk_set_rate(gr3d->clocks[i].clk, data->old_opp.rate);
+
+	return err;
+}
+
+static int gr3d_get_clocks(struct device *dev, struct gr3d *gr3d)
+{
+	int err;
+
+	err = devm_clk_bulk_get_all(dev, &gr3d->clocks);
+	if (err < 0) {
+		dev_err(dev, "failed to get clock: %d\n", err);
+		return err;
+	}
+	gr3d->nclocks = err;
+
+	if (gr3d->nclocks != gr3d->soc->num_clocks) {
+		dev_err(dev, "invalid number of clocks: %u\n", gr3d->nclocks);
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int gr3d_get_resets(struct device *dev, struct gr3d *gr3d)
+{
+	int err;
+
+	gr3d->resets[RST_MC].id = "mc";
+	gr3d->resets[RST_MC2].id = "mc2";
+	gr3d->resets[RST_GR3D].id = "3d";
+	gr3d->resets[RST_GR3D2].id = "3d2";
+	gr3d->nresets = gr3d->soc->num_resets;
+
+	err = devm_reset_control_bulk_get_optional_exclusive_released(
+				dev, gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to get reset: %d\n", err);
+		return err;
+	}
+
+	if (WARN_ON(!gr3d->resets[RST_GR3D].rstc) ||
+	    WARN_ON(!gr3d->resets[RST_GR3D2].rstc && gr3d->nresets == 4))
+		return -ENOENT;
+
+	return 0;
+}
+
 static int gr3d_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
 	struct host1x_syncpt **syncpts;
 	struct gr3d *gr3d;
 	unsigned int i;
@@ -290,56 +521,33 @@ static int gr3d_probe(struct platform_device *pdev)
 	if (!gr3d)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, gr3d);
+
 	gr3d->soc = of_device_get_match_data(&pdev->dev);
 
 	syncpts = devm_kzalloc(&pdev->dev, sizeof(*syncpts), GFP_KERNEL);
 	if (!syncpts)
 		return -ENOMEM;
 
-	gr3d->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(gr3d->clk)) {
-		dev_err(&pdev->dev, "cannot get clock\n");
-		return PTR_ERR(gr3d->clk);
-	}
-
-	gr3d->rst = devm_reset_control_get(&pdev->dev, "3d");
-	if (IS_ERR(gr3d->rst)) {
-		dev_err(&pdev->dev, "cannot get reset\n");
-		return PTR_ERR(gr3d->rst);
-	}
+	err = gr3d_get_clocks(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-	if (of_device_is_compatible(np, "nvidia,tegra30-gr3d")) {
-		gr3d->clk_secondary = devm_clk_get(&pdev->dev, "3d2");
-		if (IS_ERR(gr3d->clk_secondary)) {
-			dev_err(&pdev->dev, "cannot get secondary clock\n");
-			return PTR_ERR(gr3d->clk_secondary);
-		}
+	err = gr3d_get_resets(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-		gr3d->rst_secondary = devm_reset_control_get(&pdev->dev,
-								"3d2");
-		if (IS_ERR(gr3d->rst_secondary)) {
-			dev_err(&pdev->dev, "cannot get secondary reset\n");
-			return PTR_ERR(gr3d->rst_secondary);
-		}
-	}
+	err = gr3d_init_power(&pdev->dev, gr3d);
+	if (err)
+		return err;
 
-	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_3D, gr3d->clk,
-						gr3d->rst);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to power up 3D unit\n");
+	err = devm_pm_opp_register_set_opp_helper(&pdev->dev, gr3d_set_opp);
+	if (err)
 		return err;
-	}
 
-	if (gr3d->clk_secondary) {
-		err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_3D1,
-							gr3d->clk_secondary,
-							gr3d->rst_secondary);
-		if (err < 0) {
-			dev_err(&pdev->dev,
-				"failed to power up secondary 3D unit\n");
-			return err;
-		}
-	}
+	err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
+	if (err)
+		return err;
 
 	INIT_LIST_HEAD(&gr3d->client.base.list);
 	gr3d->client.base.ops = &gr3d_client_ops;
@@ -352,20 +560,28 @@ static int gr3d_probe(struct platform_device *pdev)
 	gr3d->client.version = gr3d->soc->version;
 	gr3d->client.ops = &gr3d_ops;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 200);
+
 	err = host1x_client_register(&gr3d->client.base);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register host1x client: %d\n",
 			err);
-		return err;
+		goto disable_rpm;
 	}
 
 	/* initialize address register map */
 	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
 		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
 
-	platform_set_drvdata(pdev, gr3d);
-
 	return 0;
+
+disable_rpm:
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return err;
 }
 
 static int gr3d_remove(struct platform_device *pdev)
@@ -380,23 +596,83 @@ static int gr3d_remove(struct platform_device *pdev)
 		return err;
 	}
 
-	if (gr3d->clk_secondary) {
-		reset_control_assert(gr3d->rst_secondary);
-		tegra_powergate_power_off(TEGRA_POWERGATE_3D1);
-		clk_disable_unprepare(gr3d->clk_secondary);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static int __maybe_unused gr3d_runtime_suspend(struct device *dev)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	int err;
+
+	host1x_channel_stop(gr3d->channel);
+
+	err = reset_control_bulk_assert(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to assert reset: %d\n", err);
+		return err;
+	}
+
+	usleep_range(10, 20);
+
+	/*
+	 * Older device-trees don't specify MC resets and power-gating can't
+	 * be done safely in that case. Hence we will keep the power ungated
+	 * for older DTBs. For newer DTBs, GENPD will perform the power-gating.
+	 */
+
+	clk_bulk_disable_unprepare(gr3d->nclocks, gr3d->clocks);
+	reset_control_bulk_release(gr3d->nresets, gr3d->resets);
+
+	return 0;
+}
+
+static int __maybe_unused gr3d_runtime_resume(struct device *dev)
+{
+	struct gr3d *gr3d = dev_get_drvdata(dev);
+	int err;
+
+	err = reset_control_bulk_acquire(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to acquire reset: %d\n", err);
+		return err;
+	}
+
+	err = clk_bulk_prepare_enable(gr3d->nclocks, gr3d->clocks);
+	if (err) {
+		dev_err(dev, "failed to enable clock: %d\n", err);
+		goto release_reset;
 	}
 
-	reset_control_assert(gr3d->rst);
-	tegra_powergate_power_off(TEGRA_POWERGATE_3D);
-	clk_disable_unprepare(gr3d->clk);
+	err = reset_control_bulk_deassert(gr3d->nresets, gr3d->resets);
+	if (err) {
+		dev_err(dev, "failed to deassert reset: %d\n", err);
+		goto disable_clk;
+	}
 
 	return 0;
+
+disable_clk:
+	clk_bulk_disable_unprepare(gr3d->nclocks, gr3d->clocks);
+release_reset:
+	reset_control_bulk_release(gr3d->nresets, gr3d->resets);
+
+	return err;
 }
 
+static const struct dev_pm_ops tegra_gr3d_pm = {
+	SET_RUNTIME_PM_OPS(gr3d_runtime_suspend, gr3d_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
 struct platform_driver tegra_gr3d_driver = {
 	.driver = {
 		.name = "tegra-gr3d",
 		.of_match_table = tegra_gr3d_match,
+		.pm = &tegra_gr3d_pm,
 	},
 	.probe = gr3d_probe,
 	.remove = gr3d_remove,
-- 
2.32.0

Re: [PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[Dmitry Osipenko]

27.08.2021 04:34, Dmitry Osipenko пишет:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 04b4691a8aac..fae5267f5218 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,8 @@ static int _set_required_opps(struct device *dev,
>  	return ret;
>  }
>  
> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +static struct dev_pm_opp *
> +_find_current_opp(struct device *dev, struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
> @@ -961,7 +962,7 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  		mutex_unlock(&opp_table->lock);
>  	}
>  
> -	opp_table->current_opp = opp;
> +	return opp;
>  }
>  
>  static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
> @@ -1003,7 +1004,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  
>  	/* Find the currently set OPP if we don't know already */
>  	if (unlikely(!opp_table->current_opp))
> -		_find_current_opp(dev, opp_table);
> +		opp_table->current_opp = _find_current_opp(dev, opp_table);
>  
>  	old_opp = opp_table->current_opp;
>  
> @@ -2931,3 +2932,38 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }

Please skip these lines. I missed to remove them during rebase and
haven't noticed until now.

Re: [PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on tegra-drm/drm/tegra/for-next linus/master v5.14-rc7 next-20210826]
[cannot apply to tegra/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Osipenko/NVIDIA-Tegra-power-management-patches-for-5-16/20210827-093936
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/11cffc90b58c476a1beca3ab35ff397d7363498d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dmitry-Osipenko/NVIDIA-Tegra-power-management-patches-for-5-16/20210827-093936
        git checkout 11cffc90b58c476a1beca3ab35ff397d7363498d
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=um SUBARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from <command-line>:
>> include/linux/compiler_types.h:149:16: error: expected '{' before 'inline'
     149 | #define inline inline __gnu_inline __inline_maybe_unused notrace
         |                ^~~~~~
   include/linux/pm_opp.h:438:15: note: in expansion of macro 'inline'
     438 | static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
         |               ^~~~~~
>> include/linux/compiler_types.h:149:16: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     149 | #define inline inline __gnu_inline __inline_maybe_unused notrace
         |                ^~~~~~
   include/linux/pm_opp.h:438:15: note: in expansion of macro 'inline'
     438 | static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
         |               ^~~~~~


vim +149 include/linux/compiler_types.h

71391bdd2e9aab Xiaozhou Liu    2018-12-14  141  
71391bdd2e9aab Xiaozhou Liu    2018-12-14  142  /*
71391bdd2e9aab Xiaozhou Liu    2018-12-14  143   * Prefer gnu_inline, so that extern inline functions do not emit an
71391bdd2e9aab Xiaozhou Liu    2018-12-14  144   * externally visible function. This makes extern inline behave as per gnu89
71391bdd2e9aab Xiaozhou Liu    2018-12-14  145   * semantics rather than c99. This prevents multiple symbol definition errors
71391bdd2e9aab Xiaozhou Liu    2018-12-14  146   * of extern inline functions at link time.
71391bdd2e9aab Xiaozhou Liu    2018-12-14  147   * A lot of inline functions can cause havoc with function tracing.
71391bdd2e9aab Xiaozhou Liu    2018-12-14  148   */
889b3c1245de48 Masahiro Yamada 2020-04-06 @149  #define inline inline __gnu_inline __inline_maybe_unused notrace
71391bdd2e9aab Xiaozhou Liu    2018-12-14  150  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9530 bytes --]

Re: [PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[Viresh Kumar]

On 27-08-21, 04:34, Dmitry Osipenko wrote:
> +/**
> + * 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)

I will rather call it dev_pm_opp_get_current(), and do the magic to find the
current OPP here as well. No need to reinvent the wheel.

-- 
viresh

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Viresh Kumar]

On 27-08-21, 04:34, Dmitry Osipenko wrote:
> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
> +	if (IS_ERR(clk_opp_table)) {
> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
> +		ret = PTR_ERR(clk_opp_table);
> +		goto put_hw;
> +	}

Why do you need to do it ? OPP core already does this automatically.

-- 
viresh

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 06:05, Viresh Kumar пишет:
> On 27-08-21, 04:34, Dmitry Osipenko wrote:
>> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
>> +	if (IS_ERR(clk_opp_table)) {
>> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
>> +		ret = PTR_ERR(clk_opp_table);
>> +		goto put_hw;
>> +	}
> 
> Why do you need to do it ? OPP core already does this automatically.

Indeed, thanks.

Re: [PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[Dmitry Osipenko]

27.08.2021 06:00, Viresh Kumar пишет:
> On 27-08-21, 04:34, Dmitry Osipenko wrote:
>> +/**
>> + * 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)
> 
> I will rather call it dev_pm_opp_get_current(), and do the magic to find the
> current OPP here as well. No need to reinvent the wheel.
> 

Okay, I'll change it.

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 06:28, Dmitry Osipenko пишет:
> 27.08.2021 06:05, Viresh Kumar пишет:
>> On 27-08-21, 04:34, Dmitry Osipenko wrote:
>>> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
>>> +	if (IS_ERR(clk_opp_table)) {
>>> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
>>> +		ret = PTR_ERR(clk_opp_table);
>>> +		goto put_hw;
>>> +	}
>>
>> Why do you need to do it ? OPP core already does this automatically.
> 
> Indeed, thanks.
> 

Actually, it doesn't work.

The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.

WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
Modules linked in:
CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
[<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
[<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
[<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
[<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
[<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
[<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
[<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)
[<c050735b>] (devm_tegra_core_dev_init_opp_table) from [<c05aad09>] (gr3d_probe+0x111/0x348)
[<c05aad09>] (gr3d_probe) from [<c05ba69b>] (platform_probe+0x43/0x84)
[<c05ba69b>] (platform_probe) from [<c05b8c01>] (really_probe.part.0+0x69/0x200)
[<c05b8c01>] (really_probe.part.0) from [<c05b8e0b>] (__driver_probe_device+0x73/0xd4)
[<c05b8e0b>] (__driver_probe_device) from [<c05b8ea1>] (driver_probe_device+0x35/0xd0)
[<c05b8ea1>] (driver_probe_device) from [<c05b92a9>] (__device_attach_driver+0x75/0x98)
[<c05b92a9>] (__device_attach_driver) from [<c05b769d>] (bus_for_each_drv+0x51/0x7c)
[<c05b769d>] (bus_for_each_drv) from [<c05b908f>] (__device_attach+0x8b/0x104)
[<c05b908f>] (__device_attach) from [<c05b81b3>] (bus_probe_device+0x5b/0x60)
[<c05b81b3>] (bus_probe_device) from [<c05b5d9f>] (device_add+0x293/0x65c)
[<c05b5d9f>] (device_add) from [<c0777a4f>] (of_platform_device_create_pdata+0x63/0x88)
[<c0777a4f>] (of_platform_device_create_pdata) from [<c0777b7d>] (of_platform_bus_create+0xfd/0x26c)
[<c0777b7d>] (of_platform_bus_create) from [<c0777dc5>] (of_platform_populate+0x45/0x84)
[<c0777dc5>] (of_platform_populate) from [<c0777e5d>] (devm_of_platform_populate+0x41/0x6c)
[<c0777e5d>] (devm_of_platform_populate) from [<c05490f9>] (host1x_probe+0x1e9/0x2c8)
[<c05490f9>] (host1x_probe) from [<c05ba69b>] (platform_probe+0x43/0x84)
[<c05ba69b>] (platform_probe) from [<c05b8c01>] (really_probe.part.0+0x69/0x200)
[<c05b8c01>] (really_probe.part.0) from [<c05b8e0b>] (__driver_probe_device+0x73/0xd4)
[<c05b8e0b>] (__driver_probe_device) from [<c05b8ea1>] (driver_probe_device+0x35/0xd0)
[<c05b8ea1>] (driver_probe_device) from [<c05b92a9>] (__device_attach_driver+0x75/0x98)
[<c05b92a9>] (__device_attach_driver) from [<c05b769d>] (bus_for_each_drv+0x51/0x7c)
[<c05b769d>] (bus_for_each_drv) from [<c05b908f>] (__device_attach+0x8b/0x104)
[<c05b908f>] (__device_attach) from [<c05b81b3>] (bus_probe_device+0x5b/0x60)
[<c05b81b3>] (bus_probe_device) from [<c05b8493>] (deferred_probe_work_func+0x57/0x78)
[<c05b8493>] (deferred_probe_work_func) from [<c0136f73>] (process_one_work+0x147/0x3f8)
[<c0136f73>] (process_one_work) from [<c0137759>] (worker_thread+0x21d/0x3f4)
[<c0137759>] (worker_thread) from [<c013c10f>] (kthread+0x123/0x140)
[<c013c10f>] (kthread) from [<c0100135>] (ret_from_fork+0x11/0x1c)
---[ end trace f68728a0d3053b54 ]---
tegra-gr3d 54180000.gr3d: tegra-soc: failed to set OPP clk: -16

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 06:47, Dmitry Osipenko пишет:
> 27.08.2021 06:28, Dmitry Osipenko пишет:
>> 27.08.2021 06:05, Viresh Kumar пишет:
>>> On 27-08-21, 04:34, Dmitry Osipenko wrote:
>>>> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
>>>> +	if (IS_ERR(clk_opp_table)) {
>>>> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
>>>> +		ret = PTR_ERR(clk_opp_table);
>>>> +		goto put_hw;
>>>> +	}
>>>
>>> Why do you need to do it ? OPP core already does this automatically.
>>
>> Indeed, thanks.
>>
> 
> Actually, it doesn't work.
> 
> The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.
> 
> WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
> Modules linked in:
> CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> [<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
> [<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
> [<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
> [<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
> [<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
> [<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
> [<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)
> [<c050735b>] (devm_tegra_core_dev_init_opp_table) from [<c05aad09>] (gr3d_probe+0x111/0x348)
> [<c05aad09>] (gr3d_probe) from [<c05ba69b>] (platform_probe+0x43/0x84)
> [<c05ba69b>] (platform_probe) from [<c05b8c01>] (really_probe.part.0+0x69/0x200)
> [<c05b8c01>] (really_probe.part.0) from [<c05b8e0b>] (__driver_probe_device+0x73/0xd4)
> [<c05b8e0b>] (__driver_probe_device) from [<c05b8ea1>] (driver_probe_device+0x35/0xd0)
> [<c05b8ea1>] (driver_probe_device) from [<c05b92a9>] (__device_attach_driver+0x75/0x98)
> [<c05b92a9>] (__device_attach_driver) from [<c05b769d>] (bus_for_each_drv+0x51/0x7c)
> [<c05b769d>] (bus_for_each_drv) from [<c05b908f>] (__device_attach+0x8b/0x104)
> [<c05b908f>] (__device_attach) from [<c05b81b3>] (bus_probe_device+0x5b/0x60)
> [<c05b81b3>] (bus_probe_device) from [<c05b5d9f>] (device_add+0x293/0x65c)
> [<c05b5d9f>] (device_add) from [<c0777a4f>] (of_platform_device_create_pdata+0x63/0x88)
> [<c0777a4f>] (of_platform_device_create_pdata) from [<c0777b7d>] (of_platform_bus_create+0xfd/0x26c)
> [<c0777b7d>] (of_platform_bus_create) from [<c0777dc5>] (of_platform_populate+0x45/0x84)
> [<c0777dc5>] (of_platform_populate) from [<c0777e5d>] (devm_of_platform_populate+0x41/0x6c)
> [<c0777e5d>] (devm_of_platform_populate) from [<c05490f9>] (host1x_probe+0x1e9/0x2c8)
> [<c05490f9>] (host1x_probe) from [<c05ba69b>] (platform_probe+0x43/0x84)
> [<c05ba69b>] (platform_probe) from [<c05b8c01>] (really_probe.part.0+0x69/0x200)
> [<c05b8c01>] (really_probe.part.0) from [<c05b8e0b>] (__driver_probe_device+0x73/0xd4)
> [<c05b8e0b>] (__driver_probe_device) from [<c05b8ea1>] (driver_probe_device+0x35/0xd0)
> [<c05b8ea1>] (driver_probe_device) from [<c05b92a9>] (__device_attach_driver+0x75/0x98)
> [<c05b92a9>] (__device_attach_driver) from [<c05b769d>] (bus_for_each_drv+0x51/0x7c)
> [<c05b769d>] (bus_for_each_drv) from [<c05b908f>] (__device_attach+0x8b/0x104)
> [<c05b908f>] (__device_attach) from [<c05b81b3>] (bus_probe_device+0x5b/0x60)
> [<c05b81b3>] (bus_probe_device) from [<c05b8493>] (deferred_probe_work_func+0x57/0x78)
> [<c05b8493>] (deferred_probe_work_func) from [<c0136f73>] (process_one_work+0x147/0x3f8)
> [<c0136f73>] (process_one_work) from [<c0137759>] (worker_thread+0x21d/0x3f4)
> [<c0137759>] (worker_thread) from [<c013c10f>] (kthread+0x123/0x140)
> [<c013c10f>] (kthread) from [<c0100135>] (ret_from_fork+0x11/0x1c)
> ---[ end trace f68728a0d3053b54 ]---
> tegra-gr3d 54180000.gr3d: tegra-soc: failed to set OPP clk: -16
> 

That's because devm_pm_opp_attach_genpd() holds the reference to OPP
table on Tegra30 which uses multiple power domains. See
gr3d_init_power() of the GR3D patch.

It works in case of a single-domain hardware.

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Viresh Kumar]

On 27-08-21, 06:47, Dmitry Osipenko wrote:
> Actually, it doesn't work.
> 
> The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.
> 
> WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
> Modules linked in:
> CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> [<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
> [<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
> [<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
> [<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
> [<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
> [<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
> [<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)

Why are you calling this anymore ?

-- 
viresh

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 07:02, Viresh Kumar пишет:
> On 27-08-21, 06:47, Dmitry Osipenko wrote:
>> Actually, it doesn't work.
>>
>> The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.
>>
>> WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
>> Modules linked in:
>> CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
>> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>> Workqueue: events_unbound deferred_probe_work_func
>> [<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
>> [<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
>> [<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
>> [<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
>> [<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
>> [<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
>> [<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)
> 
> Why are you calling this anymore ?

Older device-trees don't have OPPs, meanwhile drivers will use
dev_pm_opp_set_rate() and it requires OPP table to be set up using
devm_pm_opp_set_clkname().

The devm_tegra_core_dev_init_opp_table() is a common helper that sets up
OPP table for Tegra drivers and it sets the clk.

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Viresh Kumar]

On 27-08-21, 07:08, Dmitry Osipenko wrote:
> 27.08.2021 07:02, Viresh Kumar пишет:
> > On 27-08-21, 06:47, Dmitry Osipenko wrote:
> >> Actually, it doesn't work.
> >>
> >> The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.
> >>
> >> WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
> >> Modules linked in:
> >> CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
> >> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> >> Workqueue: events_unbound deferred_probe_work_func
> >> [<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
> >> [<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
> >> [<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
> >> [<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
> >> [<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
> >> [<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
> >> [<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)
> > 
> > Why are you calling this anymore ?
> 
> Older device-trees don't have OPPs, meanwhile drivers will use
> dev_pm_opp_set_rate() and it requires OPP table to be set up using
> devm_pm_opp_set_clkname().
> 
> The devm_tegra_core_dev_init_opp_table() is a common helper that sets up
> OPP table for Tegra drivers and it sets the clk.

Ahh, I see. that's okay then. Just add a comment over it to specify the same.
Doing devm_pm_opp_set_clkname(dev, NULL) is special and looks suspicious
otherwise.

-- 
viresh

Re: [PATCH v9 5/8] soc/tegra: pmc: Implement get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 07:13, Viresh Kumar пишет:
> On 27-08-21, 07:08, Dmitry Osipenko wrote:
>> 27.08.2021 07:02, Viresh Kumar пишет:
>>> On 27-08-21, 06:47, Dmitry Osipenko wrote:
>>>> Actually, it doesn't work.
>>>>
>>>> The devm_tegra_core_dev_init_opp_table() needs to set clk to support older device-tree and now OPP table already has clk being set.
>>>>
>>>> WARNING: CPU: 2 PID: 92 at drivers/opp/core.c:2146 dev_pm_opp_set_clkname+0x97/0xb8
>>>> Modules linked in:
>>>> CPU: 2 PID: 92 Comm: kworker/u8:1 Tainted: G        W         5.14.0-rc7-next-20210826-00181-g6389463cbb0a #9318
>>>> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>>> Workqueue: events_unbound deferred_probe_work_func
>>>> [<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
>>>> [<c0108d35>] (show_stack) from [<c0a6c1bd>] (dump_stack_lvl+0x2b/0x34)
>>>> [<c0a6c1bd>] (dump_stack_lvl) from [<c011fc47>] (__warn+0xbb/0x100)
>>>> [<c011fc47>] (__warn) from [<c0a696e3>] (warn_slowpath_fmt+0x4b/0x80)
>>>> [<c0a696e3>] (warn_slowpath_fmt) from [<c07407b3>] (dev_pm_opp_set_clkname+0x97/0xb8)
>>>> [<c07407b3>] (dev_pm_opp_set_clkname) from [<c07407e3>] (devm_pm_opp_set_clkname+0xf/0x64)
>>>> [<c07407e3>] (devm_pm_opp_set_clkname) from [<c050735b>] (devm_tegra_core_dev_init_opp_table+0x23/0x144)
>>>
>>> Why are you calling this anymore ?
>>
>> Older device-trees don't have OPPs, meanwhile drivers will use
>> dev_pm_opp_set_rate() and it requires OPP table to be set up using
>> devm_pm_opp_set_clkname().
>>
>> The devm_tegra_core_dev_init_opp_table() is a common helper that sets up
>> OPP table for Tegra drivers and it sets the clk.
> 
> Ahh, I see. that's okay then. Just add a comment over it to specify the same.
> Doing devm_pm_opp_set_clkname(dev, NULL) is special and looks suspicious
> otherwise.

I'll add comment, thanks.

Re: [PATCH v9 1/8] opp: Add dev_pm_opp_from_clk_rate()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5094 bytes --]

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on tegra-drm/drm/tegra/for-next linus/master v5.14-rc7 next-20210826]
[cannot apply to tegra/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Osipenko/NVIDIA-Tegra-power-management-patches-for-5-16/20210827-093936
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: hexagon-randconfig-r045-20210827 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 1076082a0d97bd5c16a25ee7cf3dbb6ee4b5a9fe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/11cffc90b58c476a1beca3ab35ff397d7363498d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dmitry-Osipenko/NVIDIA-Tegra-power-management-patches-for-5-16/20210827-093936
        git checkout 11cffc90b58c476a1beca3ab35ff397d7363498d
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/regulator/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from drivers/regulator/fixed.c:22:
>> include/linux/pm_opp.h:438:8: error: declaration of anonymous struct must be a definition
   static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
          ^
>> include/linux/pm_opp.h:438:1: warning: declaration does not declare anything [-Wmissing-declarations]
   static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
   ^~~~~~
   In file included from drivers/regulator/fixed.c:23:
   In file included from include/linux/regulator/driver.h:16:
>> include/linux/linear_range.h:29:56: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   unsigned int linear_range_values_in_range(const struct linear_range *r);
                                                          ^
   include/linux/linear_range.h:30:62: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   unsigned int linear_range_values_in_range_array(const struct linear_range *r,
                                                                ^
   include/linux/linear_range.h:32:54: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   unsigned int linear_range_get_max_value(const struct linear_range *r);
                                                        ^
   include/linux/linear_range.h:34:41: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   int linear_range_get_value(const struct linear_range *r, unsigned int selector,
                                           ^
   include/linux/linear_range.h:36:47: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   int linear_range_get_value_array(const struct linear_range *r, int ranges,
                                                 ^
   include/linux/linear_range.h:38:48: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   int linear_range_get_selector_low(const struct linear_range *r,
                                                  ^
   include/linux/linear_range.h:41:49: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   int linear_range_get_selector_high(const struct linear_range *r,
                                                   ^
   include/linux/linear_range.h:44:54: warning: declaration of 'struct linear_range' will not be visible outside of this function [-Wvisibility]
   int linear_range_get_selector_low_array(const struct linear_range *r,
                                                        ^
>> drivers/regulator/fixed.c:223:32: error: implicit declaration of function 'of_get_required_opp_performance_state' [-Werror,-Wimplicit-function-declaration]
                   drvdata->performance_state = of_get_required_opp_performance_state(dev->of_node, 0);
                                                ^
   9 warnings and 2 errors generated.


vim +438 include/linux/pm_opp.h

   437	
 > 438	static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
   439	{
   440		return ERR_PTR(-EOPNOTSUPP);
   441	}
   442	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25846 bytes --]

Re: [PATCH v9 4/8] PM: domains: Add get_performance_state() callback

[Ulf Hansson]

On Fri, 27 Aug 2021 at 03:37, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Add get_performance_state() callback that retrieves and initializes
> performance state of a device attached to a power domain. This removes
> inconsistency of the performance state with hardware state.

Can you please try to elaborate a bit more on the use case. Users need
to know when it makes sense to implement the callback - and so far we
tend to document this through detailed commit messages.

Moreover, please state that implementing the callback is optional.

>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/base/power/domain.c | 32 +++++++++++++++++++++++++++++---
>  include/linux/pm_domain.h   |  2 ++
>  2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 3a13a942d012..8b828dcdf7f8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2700,15 +2700,41 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                 goto err;
>         } else if (pstate > 0) {
>                 ret = dev_pm_genpd_set_performance_state(dev, pstate);
> -               if (ret)
> +               if (ret) {
> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> +                               pd->name, ret);

Moving the dev_err() here, leads to that we won't print an error if
of_get_required_opp_performance_state() fails, a few lines above, is
that intentional?

>                         goto err;
> +               }
>                 dev_gpd_data(dev)->default_pstate = pstate;
>         }
> +
> +       if (pd->get_performance_state && !dev_gpd_data(dev)->default_pstate) {
> +               bool dev_suspended = false;
> +
> +               ret = pd->get_performance_state(pd, base_dev, &dev_suspended);
> +               if (ret < 0) {
> +                       dev_err(dev, "failed to get performance state for power-domain %s: %d\n",
> +                               pd->name, ret);
> +                       goto err;
> +               }
> +
> +               pstate = ret;
> +
> +               if (dev_suspended) {

The dev_suspended thing looks weird.

Perhaps it was needed before dev_pm_genpd_set_performance_state()
didn't check pm_runtime_disabled()?

> +                       dev_gpd_data(dev)->rpm_pstate = pstate;
> +               } else if (pstate > 0) {
> +                       ret = dev_pm_genpd_set_performance_state(dev, pstate);
> +                       if (ret) {
> +                               dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> +                                       pd->name, ret);
> +                               goto err;
> +                       }
> +               }
> +       }

Overall, what we seem to be doing here, is to retrieve a value for an
initial/default performance state for a device and then we want to set
it to make sure the vote becomes aggregated and finally set for the
genpd.

With your suggested change, there are now two ways to get the
initial/default state. One is through the existing
of_get_required_opp_performance_state() and the other is by using a
new genpd callback.

That said, perhaps we would get a bit cleaner code by moving the "get
initial/default performance state" thingy, into a separate function
and then call it from here. If this function returns a valid
performance state, then we should continue to set the state, by
calling dev_pm_genpd_set_performance_state() and update
dev_gpd_data(dev)->default_pstate accordingly.

Would that work, do you think?

> +
>         return 1;
>
>  err:
> -       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> -               pd->name, ret);
>         genpd_remove_device(pd, dev);
>         return ret;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 67017c9390c8..4f78b31791ae 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, bool *dev_suspended);

Comparing the ->set_performance_state() callback, which sets a
performance state for the PM domain (genpd) - this new callback is
about retrieving the *initial/default* performance state for a
*device* that gets attached to a genpd.

That said, may I suggest renaming the callback to
"dev_get_performance_state", or something along those lines.

>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         ktime_t next_wakeup;    /* Maintained by the domain governor */
> --
> 2.32.0
>

Kind regards
Uffe

Re: [PATCH v9 4/8] PM: domains: Add get_performance_state() callback

[Dmitry Osipenko]

27.08.2021 17:23, Ulf Hansson пишет:
> On Fri, 27 Aug 2021 at 03:37, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Add get_performance_state() callback that retrieves and initializes
>> performance state of a device attached to a power domain. This removes
>> inconsistency of the performance state with hardware state.
> 
> Can you please try to elaborate a bit more on the use case. Users need
> to know when it makes sense to implement the callback - and so far we
> tend to document this through detailed commit messages.
> 
> Moreover, please state that implementing the callback is optional.

Noted

>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/base/power/domain.c | 32 +++++++++++++++++++++++++++++---
>>  include/linux/pm_domain.h   |  2 ++
>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 3a13a942d012..8b828dcdf7f8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2700,15 +2700,41 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                 goto err;
>>         } else if (pstate > 0) {
>>                 ret = dev_pm_genpd_set_performance_state(dev, pstate);
>> -               if (ret)
>> +               if (ret) {
>> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>> +                               pd->name, ret);
> 
> Moving the dev_err() here, leads to that we won't print an error if
> of_get_required_opp_performance_state() fails, a few lines above, is
> that intentional?

Not intentional, I'll add another message.

>>                         goto err;
>> +               }
>>                 dev_gpd_data(dev)->default_pstate = pstate;
>>         }
>> +
>> +       if (pd->get_performance_state && !dev_gpd_data(dev)->default_pstate) {
>> +               bool dev_suspended = false;
>> +
>> +               ret = pd->get_performance_state(pd, base_dev, &dev_suspended);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "failed to get performance state for power-domain %s: %d\n",
>> +                               pd->name, ret);
>> +                       goto err;
>> +               }
>> +
>> +               pstate = ret;
>> +
>> +               if (dev_suspended) {
> 
> The dev_suspended thing looks weird.
> 
> Perhaps it was needed before dev_pm_genpd_set_performance_state()
> didn't check pm_runtime_disabled()?

There are two possible variants here:

1. Device is suspended
2. Device is active

If device is suspended, then it will be activated on RPM-resume and h/w
state will require a specific performance state when resumed. Hence only
the the rpm_pstate should be set, otherwise SoC may start to consume
extra power if device won't be resumed by a consumer driver and
performance state is bumped without a real need.

If device is known to be active, then the performance state should be
updated immediately, otherwise we have inconsistent state with hardware.

For Tegra dev_suspended=true because in general it should be safe to
assume that hardware is suspended since it's either stopped by the PD
driver on initial power_on or it's assumed to be disabled by a consumer
driver during probe. Technically it's possible to check clock and reset
state of an attached device from the get_performance_state() to find the
real state of device, but it's not necessary to do so far.

I'll add comment to the code.

>> +                       dev_gpd_data(dev)->rpm_pstate = pstate;
>> +               } else if (pstate > 0) {
>> +                       ret = dev_pm_genpd_set_performance_state(dev, pstate);
>> +                       if (ret) {
>> +                               dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>> +                                       pd->name, ret);
>> +                               goto err;
>> +                       }
>> +               }
>> +       }
> 
> Overall, what we seem to be doing here, is to retrieve a value for an
> initial/default performance state for a device and then we want to set
> it to make sure the vote becomes aggregated and finally set for the
> genpd.
> 
> With your suggested change, there are now two ways to get the
> initial/default state. One is through the existing
> of_get_required_opp_performance_state() and the other is by using a
> new genpd callback.
> 
> That said, perhaps we would get a bit cleaner code by moving the "get
> initial/default performance state" thingy, into a separate function
> and then call it from here. If this function returns a valid
> performance state, then we should continue to set the state, by
> calling dev_pm_genpd_set_performance_state() and update
> dev_gpd_data(dev)->default_pstate accordingly.
> 
> Would that work, do you think?

To be honest, I'm now confused by
of_get_required_opp_performance_state(). It assumes that device is
active all the time while attached and that device is stopped on detach.

If hardware is always-on, then it should be wrong to drop the
performance state on detach.

If hardware isn't always-on, then it might be suspended during
attachment, and thus, only the rpm_pstate should be set. It's also not
guaranteed that consumer driver will suspend device on unbind, leaving
it active on detach, thus it should be wrong to drop performance state
on detach.

Hence I think the default_pstate is a bit out of touch. If this
attach/detach behaviour is specific to QCOM driver/hardware, then maybe
of_get_required_opp_performance_state() should be moved out to a
get_performance_state() of the QCOM PD driver?

I added Rajendra Nayak to explain.

For now we're bailing out if default_pstate is set because it conflicts
with get_performance_state().

But we can factor out the code into a separate function anyways to make
it cleaner a tad.

>> +
>>         return 1;
>>
>>  err:
>> -       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>> -               pd->name, ret);
>>         genpd_remove_device(pd, dev);
>>         return ret;
>>  }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 67017c9390c8..4f78b31791ae 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, bool *dev_suspended);
> 
> Comparing the ->set_performance_state() callback, which sets a
> performance state for the PM domain (genpd) - this new callback is
> about retrieving the *initial/default* performance state for a
> *device* that gets attached to a genpd.
> 
> That said, may I suggest renaming the callback to
> "dev_get_performance_state", or something along those lines.

Noted

>>         struct gpd_dev_ops dev_ops;
>>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>>         ktime_t next_wakeup;    /* Maintained by the domain governor */
>> --
>> 2.32.0
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH v9 4/8] PM: domains: Add get_performance_state() callback

[Ulf Hansson]

+ Dmitry Baryshkov, Bjorn Andersson

On Fri, 27 Aug 2021 at 17:50, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 27.08.2021 17:23, Ulf Hansson пишет:
> > On Fri, 27 Aug 2021 at 03:37, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> Add get_performance_state() callback that retrieves and initializes
> >> performance state of a device attached to a power domain. This removes
> >> inconsistency of the performance state with hardware state.
> >
> > Can you please try to elaborate a bit more on the use case. Users need
> > to know when it makes sense to implement the callback - and so far we
> > tend to document this through detailed commit messages.
> >
> > Moreover, please state that implementing the callback is optional.
>
> Noted
>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/base/power/domain.c | 32 +++++++++++++++++++++++++++++---
> >>  include/linux/pm_domain.h   |  2 ++
> >>  2 files changed, 31 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index 3a13a942d012..8b828dcdf7f8 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -2700,15 +2700,41 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >>                 goto err;
> >>         } else if (pstate > 0) {
> >>                 ret = dev_pm_genpd_set_performance_state(dev, pstate);
> >> -               if (ret)
> >> +               if (ret) {
> >> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> >> +                               pd->name, ret);
> >
> > Moving the dev_err() here, leads to that we won't print an error if
> > of_get_required_opp_performance_state() fails, a few lines above, is
> > that intentional?
>
> Not intentional, I'll add another message.
>
> >>                         goto err;
> >> +               }
> >>                 dev_gpd_data(dev)->default_pstate = pstate;
> >>         }
> >> +
> >> +       if (pd->get_performance_state && !dev_gpd_data(dev)->default_pstate) {
> >> +               bool dev_suspended = false;
> >> +
> >> +               ret = pd->get_performance_state(pd, base_dev, &dev_suspended);
> >> +               if (ret < 0) {
> >> +                       dev_err(dev, "failed to get performance state for power-domain %s: %d\n",
> >> +                               pd->name, ret);
> >> +                       goto err;
> >> +               }
> >> +
> >> +               pstate = ret;
> >> +
> >> +               if (dev_suspended) {
> >
> > The dev_suspended thing looks weird.
> >
> > Perhaps it was needed before dev_pm_genpd_set_performance_state()
> > didn't check pm_runtime_disabled()?
>
> There are two possible variants here:
>
> 1. Device is suspended
> 2. Device is active
>
> If device is suspended, then it will be activated on RPM-resume and h/w
> state will require a specific performance state when resumed. Hence only
> the the rpm_pstate should be set, otherwise SoC may start to consume
> extra power if device won't be resumed by a consumer driver and
> performance state is bumped without a real need.
>
> If device is known to be active, then the performance state should be
> updated immediately, otherwise we have inconsistent state with hardware.
>
> For Tegra dev_suspended=true because in general it should be safe to
> assume that hardware is suspended since it's either stopped by the PD
> driver on initial power_on or it's assumed to be disabled by a consumer
> driver during probe. Technically it's possible to check clock and reset
> state of an attached device from the get_performance_state() to find the
> real state of device, but it's not necessary to do so far.

I follow your reasoning above, but I fail to understand your point, sorry.

Your recent patch ("PM: domains: Improve runtime PM performance state
handling"), made dev_pm_genpd_set_performance_state() to call
pm_runtime_suspended(), to check whether it should assign
dev_gpd_data(dev)->rpm_pstate, which postpones the vote until the
device gets runtime resumed - or call genpd_set_performance_state() to
immediately vote for a new performance state.

That updated behaviour of dev_pm_genpd_set_performance_state should be
sufficient, I think.

In other words, please drop the "dev_suspended" parameter from the
->get_performance_state() callback, as it doesn't make sense to me.

>
> I'll add comment to the code.
>
> >> +                       dev_gpd_data(dev)->rpm_pstate = pstate;
> >> +               } else if (pstate > 0) {
> >> +                       ret = dev_pm_genpd_set_performance_state(dev, pstate);
> >> +                       if (ret) {
> >> +                               dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> >> +                                       pd->name, ret);
> >> +                               goto err;
> >> +                       }
> >> +               }
> >> +       }
> >
> > Overall, what we seem to be doing here, is to retrieve a value for an
> > initial/default performance state for a device and then we want to set
> > it to make sure the vote becomes aggregated and finally set for the
> > genpd.
> >
> > With your suggested change, there are now two ways to get the
> > initial/default state. One is through the existing
> > of_get_required_opp_performance_state() and the other is by using a
> > new genpd callback.
> >
> > That said, perhaps we would get a bit cleaner code by moving the "get
> > initial/default performance state" thingy, into a separate function
> > and then call it from here. If this function returns a valid
> > performance state, then we should continue to set the state, by
> > calling dev_pm_genpd_set_performance_state() and update
> > dev_gpd_data(dev)->default_pstate accordingly.
> >
> > Would that work, do you think?
>
> To be honest, I'm now confused by
> of_get_required_opp_performance_state(). It assumes that device is
> active all the time while attached and that device is stopped on detach.
>
> If hardware is always-on, then it should be wrong to drop the
> performance state on detach.
>
> If hardware isn't always-on, then it might be suspended during
> attachment, and thus, only the rpm_pstate should be set. It's also not
> guaranteed that consumer driver will suspend device on unbind, leaving
> it active on detach, thus it should be wrong to drop performance state
> on detach.

I assume the new behaviour in dev_pm_genpd_set_performance_state()
should address most of your concerns above, no?

When it comes to the detaching, the best we can do is to drop the
performance state vote for the device, no matter if it's an always on
HW or not. Simply because after a detach, genpd loses track of the
device, which means it can't account for performance states votes for
it anyway.

>
> Hence I think the default_pstate is a bit out of touch. If this
> attach/detach behaviour is specific to QCOM driver/hardware, then maybe
> of_get_required_opp_performance_state() should be moved out to a
> get_performance_state() of the QCOM PD driver?

That may work, but I hope it's unnecessary.

Overall, the important part is the updated path in
dev_pm_genpd_set_performance_state() where we now call
pm_runtime_suspended(). I am pretty sure this should work fine for
Qcom platforms/drivers too, but let's see if Rajendra, Dmitry or Bjorn
have some concerns about this.

>
> I added Rajendra Nayak to explain.
>
> For now we're bailing out if default_pstate is set because it conflicts
> with get_performance_state().
>
> But we can factor out the code into a separate function anyways to make
> it cleaner a tad.

Yes, please.

[...]

Kind regards
Uffe