NVIDIA Tegra power management patches for 5.16

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

drivers/opp/core.c +1 -1

index 04b4691a8aac..dde8a5cc948c 100644
@@ -939,7 +939,7 @@ 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 opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;

@@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		opp = _find_freq_ceil(opp_table, &freq);
 	}

drivers/opp/core.c +10 -2

@@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return _disable_opp_table(dev, 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);
+	_find_and_set_current_opp(opp_table);
 
 	old_opp = opp_table->current_opp;
 

@@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_get_current() - Get current OPP
+ * @dev:	device for which we do this operation
+ *
+ * Get current OPP of a device based on current clock rate or by other means.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */

Quoted 26 lines

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

drivers/opp/core.c +1 -1

index 04b4691a8aac..dde8a5cc948c 100644
@@ -939,7 +939,7 @@ 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 opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;

@@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		opp = _find_freq_ceil(opp_table, &freq);
 	}
 

drivers/opp/core.c static void _find_current_opp(struct device *dev, struct opp_table *opp_table) · line 960

Quoted 9 lines

drivers/opp/core.c +9 -0

+	return opp;
+}
+
+static void _find_and_set_current_opp(struct opp_table *opp_table)
+{
+	struct dev_pm_opp *opp;
+
+	if (opp_table->current_opp)
+		return;

Why move this from caller as well ?

drivers/opp/core.c static void _find_current_opp(struct device *dev, struct opp_table *opp_table) · line 966

Quoted 6 lines

drivers/opp/core.c +3 -0

+
+	opp = _find_current_opp(opp_table);
+
 	/*
 	 * Unable to find the current OPP ? Pick the first from the list since
 	 * it is in ascending order, otherwise rest of the code will need to

If we aren't able to find current OPP based on current freq, then this picks the first value from the list. Why shouldn't this be done in your case as well ?

Quoted 30 lines

drivers/opp/core.c +17 -2

@@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return _disable_opp_table(dev, 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);
+	_find_and_set_current_opp(opp_table);
 
 	old_opp = opp_table->current_opp;
 

@@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_get_current() - Get current OPP
+ * @dev:	device for which we do this operation
+ *
+ * Get current OPP of a device based on current clock rate or by other means.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *opp;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))

drivers/opp/core.c int dev_pm_opp_sync_regulators(struct device *dev) · line 2963

Quoted 3 lines

drivers/opp/core.c +3 -0

+		return ERR_CAST(opp_table);
+
+	opp = _find_current_opp(opp_table);

This should not just go find the OPP based on current freq. This first needs to check if curret_opp is set or not. If set, then just return it, else run the _find_current_opp() function and update the current_opp pointer as well.

You will get OPP which corresponds to the lowest freq, while h/w runs on unsupported high freq. This may end with a tragedy.

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 the clock's replacement.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/opp/core.c +1 -1

index dde8a5cc948c..602e502d092e 100644
@@ -2146,7 +2146,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;
 	}

Signature · 1 line

2.32.0

Quoted 2 lines

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,

I am not sure why are you required to set the clk again here ? I mean, users aren't expected to put clkname in the middle of using it. The set-name API also checks that the OPP list should be empty in such a case.

01.09.2021 07:42, Viresh Kumar пишет:

Quoted 4 lines · Dmitry Osipenko

Quoted 2 lines

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,

Quoted 4 lines

I am not sure why are you required to set the clk again here ? I mean, users aren't expected to put clkname in the middle of using it. The set-name API also checks that the OPP list should be empty in such a case.

I added explanatory comment to tegra_pmc_pd_dev_get_performance_state(), isn't it enough?

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 6 +++---
 include/linux/pm_opp.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

drivers/opp/core.c +0 -0

index 602e502d092e..d4e706a8b70d 100644

Quoted 9 lines

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 6 +++---
 include/linux/pm_opp.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

drivers/opp/core.c static void _opp_detach_genpd(struct opp_table *opp_table) · line 2362

Quoted 10 lines

drivers/opp/core.c +1 -1

index 602e502d092e..d4e706a8b70d 100644
@@ -2359,12 +2359,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)

I am sure there are issues around space around * here. Please run checkpatch with --strict option for your series.

01.09.2021 07:41, Viresh Kumar пишет:

Quoted 10 lines · Dmitry Osipenko

Quoted 9 lines

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

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 6 +++---
 include/linux/pm_opp.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

Quoted 14 lines

drivers/opp/core.c static void _opp_detach_genpd(struct opp_table *opp_table) · line 2362

Quoted 10 lines

drivers/opp/core.c +1 -1

index 602e502d092e..d4e706a8b70d 100644
@@ -2359,12 +2359,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)

I am sure there are issues around space around * here. Please run checkpatch with --strict option for your series.

It is the other way around. This fixes the checkpatch warning and that's what checkpatch wants. You may also grep the kernel to find that this is the only variant used in practice.

Add optional dev_get_performance_state() callback that retrieves performance state of a device attached to a power domain. The new callback returns performance states of the given device, which should be read out from hardware.

GENPD core assumes that initially performance state of all devices is zero and consumer device driver is responsible for management of the performance state. GENPD core now manages performance state across RPM suspend/resume of consumer devices by dropping and restoring the state, this allowed to move a part of performance management code into GENPD core out of consumer drivers. The part that hasn't been moved to GENPD core yet is the initialization of the performance state.

Hardware may be preprogrammed to a specific performance state which isn't zero initially, hence the PD's performance state is inconsistent with hardware at the init time. This requires each consumer driver to explicitly sync the state. For some platforms this is a boilerplate code replicated by each driver.

The new callback allows GENPD core to initialize the performance state, allowing to remove the remaining boilerplate code from consumer drivers. Now, when consumer device is resumed for the first time, the performance state is either already set by GENPD core or it will be set in accordance to the hardware state that was retrieved using the new callback when device was attached to a power domain. Still, consumer drivers are free to override the initial performance state configured by GENPD, if necessary.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/base/power/domain.c | 90 +++++++++++++++++++++++++++++++------
 include/linux/pm_domain.h   |  2 +
 2 files changed, 79 insertions(+), 13 deletions(-)

drivers/base/power/domain.c +63 -0

index 5db704f02e71..598528abce01 100644
@@ -2644,12 +2644,85 @@ static void genpd_dev_pm_sync(struct device *dev)
        genpd_queue_power_off_work(pd);
 }
 
+static int genpd_dev_get_current_performance_state(struct device *dev,
+                                                  struct device *base_dev,
+                                                  unsigned int index,
+                                                  bool *state_default,
+                                                  bool *suspended)
+{
+       struct generic_pm_domain *pd = dev_to_genpd(dev);
+       int ret;
+
+       ret = of_get_required_opp_performance_state(dev->of_node, index);
+       if (ret < 0 && ret != -ENODEV && ret != -EOPNOTSUPP) {
+               dev_err(dev, "failed to get required performance state for power-domain %s: %d\n",
+                       pd->name, ret);
+
+               return ret;
+       } else if (ret >= 0) {
+               *state_default = true;
+
+               return ret;
+       }
+
+       if (pd->dev_get_performance_state) {
+               ret = pd->dev_get_performance_state(pd, base_dev);
+               if (ret >= 0)
+                       *suspended = pm_runtime_status_suspended(base_dev);
+               else
+                       dev_err(dev, "failed to get performance state of %s for power-domain %s: %d\n",
+                               dev_name(base_dev), pd->name, ret);
+
+               return ret;
+       }
+
+       return 0;
+}
+
+static int genpd_dev_initialize_performance_state(struct device *dev,
+                                                 struct device *base_dev,
+                                                 unsigned int index)
+{
+       struct generic_pm_domain *pd = dev_to_genpd(dev);
+       bool state_default = false, suspended = false;
+       int pstate, err;
+
+       pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
+                                                        &state_default,
+                                                        &suspended);
+       if (pstate <= 0)
+               return pstate;
+
+       /*
+        * If device is suspended, then performance state will be applied
+        * on RPM-resume. This prevents potential extra power consumption
+        * if device won't be resumed soon.
+        *
+        * If device is known to be active at the moment, then the performance
+        * state should be updated immediately to sync state with hardware.
+        */
+       if (suspended) {
+               dev_gpd_data(dev)->rpm_pstate = pstate;
+       } else {
+               err = dev_pm_genpd_set_performance_state(dev, pstate);
+               if (err) {
+                       dev_err(dev, "failed to set performance state for power-domain %s: %d\n",

drivers/base/power/domain.c +4 -13

+
 static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
                                 unsigned int index, bool power_on)
 {
        struct of_phandle_args pd_args;
        struct generic_pm_domain *pd;
-       int pstate;
        int ret;
 
        ret = of_parse_phandle_with_args(dev->of_node, "power-domains",

@@ -2693,22 +2766,13 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
                return -EPROBE_DEFER;
        }
 
-       /* Set the default performance state */
-       pstate = of_get_required_opp_performance_state(dev->of_node, index);
-       if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
-               ret = pstate;
+       ret = genpd_dev_initialize_performance_state(dev, base_dev, index);
+       if (ret)
                goto err;
-       } else if (pstate > 0) {
-               ret = dev_pm_genpd_set_performance_state(dev, pstate);
-               if (ret)
-                       goto err;
-               dev_gpd_data(dev)->default_pstate = pstate;
-       }
+
        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;
 }

include/linux/pm_domain.h +2 -1

index 67017c9390c8..0a4dd4986f34 100644
@@ -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 (*dev_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 */
--

2.32.0

Quoted 28 lines

drivers/base/power/domain.c +28 -0

+static int genpd_dev_initialize_performance_state(struct device *dev,
+                                                 struct device *base_dev,
+                                                 unsigned int index)
+{
+       struct generic_pm_domain *pd = dev_to_genpd(dev);
+       bool state_default = false, suspended = false;
+       int pstate, err;
+
+       pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
+                                                        &state_default,
+                                                        &suspended);
+       if (pstate <= 0)
+               return pstate;
+
+       /*
+        * If device is suspended, then performance state will be applied
+        * on RPM-resume. This prevents potential extra power consumption
+        * if device won't be resumed soon.
+        *
+        * If device is known to be active at the moment, then the performance
+        * state should be updated immediately to sync state with hardware.
+        */
+       if (suspended) {
+               dev_gpd_data(dev)->rpm_pstate = pstate;
+       } else {
+               err = dev_pm_genpd_set_performance_state(dev, pstate);
+               if (err) {
+                       dev_err(dev, "failed to set performance state for power-domain %s: %d\n",

drivers/base/power/domain.c static void genpd_dev_pm_sync(struct device *dev) · line 2719

Quoted 10 lines

drivers/base/power/domain.c +10 -0

+                               pd->name, err);
+                       return err;
+               }
+
+               if (state_default)
+                       dev_gpd_data(dev)->default_pstate = pstate;
+       }
+
+       return 0;
+}

As I kind of indicated in my previous reply on the earlier version, I think the above code can be made a lot less complicated. Although, perhaps I may be missing some points.

Anyway, I will post a couple patches, later this evening or tomorrow, to show more exactly what I had in mind. Let's see if that can work for you.

drivers/soc/tegra/pmc.c +71 -0

+static int
+tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
+				       struct device *dev)
+{
+	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);
+	}
+
+	/*
+	 * Older device-trees don't have OPPs, meanwhile drivers use
+	 * dev_pm_opp_set_rate() and it requires OPP table to be set
+	 * up using dev_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
+	 * for compatibility with older device-tress.  GR3D driver uses that
+	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
+	 * power domains, which now holds the reference to OPP table that
+	 * already has clk set up implicitly by OPP core for a newly created
+	 * table using dev_pm_opp_of_add_table() invoked below.
+	 *
+	 * Hence we need to explicitly set/put the clk, otherwise
+	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
+	 */
+	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 = dev_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_get_current(dev);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "failed to get current OPP: %pe\n", opp);
+		ret = PTR_ERR(opp);

Quoted 71 lines

drivers/soc/tegra/pmc.c +71 -0

+static int
+tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
+				       struct device *dev)
+{
+	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);
+	}
+
+	/*
+	 * Older device-trees don't have OPPs, meanwhile drivers use
+	 * dev_pm_opp_set_rate() and it requires OPP table to be set
+	 * up using dev_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
+	 * for compatibility with older device-tress.  GR3D driver uses that
+	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
+	 * power domains, which now holds the reference to OPP table that
+	 * already has clk set up implicitly by OPP core for a newly created
+	 * table using dev_pm_opp_of_add_table() invoked below.
+	 *
+	 * Hence we need to explicitly set/put the clk, otherwise
+	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
+	 */
+	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 = dev_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_get_current(dev);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "failed to get current OPP: %pe\n", opp);
+		ret = PTR_ERR(opp);

drivers/soc/tegra/pmc.c static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value, · line 602

Quoted 10 lines

drivers/soc/tegra/pmc.c +10 -0

+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	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);

So you create the OPP table and just after that you remove it ? Just to get the current OPP and pstate ? This doesn't look okay.

Moreover, this routine must be implemented with the expectation that the genpd core can call it anytime it wants, not just at the beginning. And so if the table is already setup by someone else then, then this all will just fail.

I did have a look at the comment you added above regarding devm_tegra_core_dev_init_opp_table(), but I am not sure of the series of events right now. For me, the OPP table should just be added once and for ever. You shouldn't remove and add it again. That is bound to break somewhere later.

Can you give the sequence in which the whole thing works out with respect to the OPP core? who calls devm_tegra_core_dev_init_opp_table() and when, and when exactly will this routine get called, etc ?

01.09.2021 09:10, Viresh Kumar пишет:

Quoted 7 lines

So you create the OPP table and just after that you remove it ? Just to get the current OPP and pstate ? This doesn't look okay.

Moreover, this routine must be implemented with the expectation that the genpd core can call it anytime it wants, not just at the beginning. And so if the table is already setup by someone else then, then this all will just fail.

This is not doable using the current OPP API, it doesn't allow to re-use initialized OPP table.

The current limitation is okay because genpd core doesn't call routine anytime.

Quoted 5 lines

Can you give the sequence in which the whole thing works out with respect to the OPP core? who calls devm_tegra_core_dev_init_opp_table() and when, and when exactly will this routine get called, etc ?

gr3d_probe(struct platform_device *pdev)

{
	gr3d_init_power(dev)
	{
		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };

		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
		{
			dev_pm_opp_attach_genpd(dev, names, virt_devs)
			{
				// takes and holds table reference
				opp_table = _add_opp_table(dev, false);

				while (*name) {

					// first attachment succeeds, 
					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
					dev_pm_domain_attach_by_name(dev, *name)
					{
						tegra_pmc_pd_dev_get_performance_state(dev)
						{
							dev_pm_opp_set_clkname(dev, NULL);
							dev_pm_opp_of_add_table(dev);

							opp = dev_pm_opp_get_current(dev);
							dev_pm_opp_of_remove_table(dev);
							dev_pm_opp_put_clkname(opp_table);

							...
						}
						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
					}
				}
			}
		}
	}

	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

Quoted 11 lines

01.09.2021 09:10, Viresh Kumar пишет:

Quoted 7 lines

So you create the OPP table and just after that you remove it ? Just to get the current OPP and pstate ? This doesn't look okay.

Moreover, this routine must be implemented with the expectation that the genpd core can call it anytime it wants, not just at the beginning. And so if the table is already setup by someone else then, then this all will just fail.

This is not doable using the current OPP API, it doesn't allow to re-use initialized OPP table.

That isn't correct, you can call dev_pm_opp_of_add_table() as many times as you want. It will just increase the refcount and return the next time.

Yes, you can call the APIs like set-clk-name or supported-hw, since they are supposed to be set only once.

Quoted 2 lines

The current limitation is okay because genpd core doesn't call routine anytime.

Yeah, but is broken by design. People can make changes to genpd core later on to call it as many times and they aren't required to have a look at all the users to see who abused the API and who didn't.

Quoted 7 lines

Quoted 5 lines

Can you give the sequence in which the whole thing works out with respect to the OPP core? who calls devm_tegra_core_dev_init_opp_table() and when, and when exactly will this routine get called, etc ?

gr3d_probe(struct platform_device *pdev)

Thanks for this.

Quoted 13 lines

{
	gr3d_init_power(dev)
	{
		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };

		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
		{
			dev_pm_opp_attach_genpd(dev, names, virt_devs)
			{
				// takes and holds table reference
				opp_table = _add_opp_table(dev, false);

				while (*name) {

Quoted 8 lines

					// first attachment succeeds, 
					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
					dev_pm_domain_attach_by_name(dev, *name)
					{
						tegra_pmc_pd_dev_get_performance_state(dev)
						{
							dev_pm_opp_set_clkname(dev, NULL);
							dev_pm_opp_of_add_table(dev);

What you end up doing here is pretty much like devm_tegra_core_dev_init_opp_table_simple(), right ?

Quoted 3 lines

							opp = dev_pm_opp_get_current(dev);
							dev_pm_opp_of_remove_table(dev);
							dev_pm_opp_put_clkname(opp_table);

You shouldn't be required to do this at all.

Quoted 9 lines

							...
						}
						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
					}
				}
			}
		}
	}

Quoted 1 line

	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

Can we make the call at the beginning ? before calling gr3d_init_power() ? I mean you should only be required to initialize the OPP table just once.

If not, then what about calling devm_tegra_core_dev_init_opp_table_simple() from here and from tegra_pmc_pd_dev_get_performance_state() as well ?

And update devm_tegra_core_dev_init_opp_table_simple() to call dev_pm_opp_get_opp_table() first and return early if the OPP table is already initialized ?