GENPD API improvements

PATCH In review Dmitry Osipenko 3 participants 6 messages Jan 20, 2021 – Jan 22, 2021

Subsystems (5)

Tegra architecture support Driver core Suspend to RAM Hibernation +1 more subsystems

lore

Share & tools

lore

Dmitry Osipenko

author

2021-01-20 21:25 UTC #0 lore raw

v5 cover GENPD API improvements

Hi,

This series is a prerequisite for the power domain driver of NVIDIA Tegra SoCs [1]. It extends the GENPD core with a better support of performance states and eases linking of child/parent PDs for the PD drivers.

[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130

Changelog:

v5: - Renamed genpd_xlate_performance_state() and its arguments like it
was requested by Ulf Hansson and Viresh Kumar in a v4 review.

- Added r-b from Ulf Hansson to "Make set_performance_state() callback optional" patch.

v4: - Updated the "Make set_performance_state() callback optional" patch.
Now the case where one of intermediate domains doesn't implement the set_performance_state() callback is handled properly, thanks to Viresh and Ulf for catching this drawback and suggesting the fix.

- Added more r-bs from Ulf Hansson and Viresh Kumar.

v3: - Added r-b from Ulf Hansson.

- Added new patch "Make of_genpd_add_subdomain() to return -EPROBE_DEFER", which was suggested by Ulf Hansson.

- Improved "Add "performance" column to debug summary" patch by correcting the formatting of debug output, which had a superfluous whitespace.

Dmitry Osipenko (3):
PM: domains: Make set_performance_state() callback optional
PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER
PM: domains: Add "performance" column to debug summary

 drivers/base/power/domain.c | 54 +++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 20 deletions(-)

Signature · 1 line

2.29.2

Dmitry Osipenko

author

2021-01-20 21:25 UTC #1 lore raw

PATCH v5 1/3 PM: domains: Make set_performance_state() callback optional

Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.

Tested-by Peter Geis <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Matt Merhar <[email protected]>

[tested on NVIDIA Tegra20/30/124 SoCs]

Reviewed-by Ulf Hansson <[email protected]>

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

---
 drivers/base/power/domain.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

drivers/base/power/domain.c +18 ~~-15~~

index 9a14eedacb92..5c843772f7f4 100644
@@ -297,6 +297,18 @@ static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
 	return state;
 }
 
+static int genpd_xlate_performance_state(struct generic_pm_domain *genpd,
+					 struct generic_pm_domain *parent,
+					 unsigned int pstate)
+{
+	if (!parent->set_performance_state)
+		return pstate;
+
+	return dev_pm_opp_xlate_performance_state(genpd->opp_table,
+						  parent->opp_table,
+						  pstate);
+}
+
 static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 					unsigned int state, int depth)
 {

@@ -311,13 +323,8 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 	list_for_each_entry(link, &genpd->child_links, child_node) {
 		parent = link->parent;
 
-		if (!parent->set_performance_state)
-			continue;
-
 		/* Find parent's performance state */
-		ret = dev_pm_opp_xlate_performance_state(genpd->opp_table,
-							 parent->opp_table,
-							 state);
+		ret = genpd_xlate_performance_state(genpd, parent, state);
 		if (unlikely(ret < 0))
 			goto err;

@@ -339,9 +346,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 			goto err;
 	}
 
-	ret = genpd->set_performance_state(genpd, state);
-	if (ret)
-		goto err;
+	if (genpd->set_performance_state) {
+		ret = genpd->set_performance_state(genpd, state);
+		if (ret)
+			goto err;
+	}
 
 	genpd->performance_state = state;
 	return 0;

@@ -352,9 +361,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 					     child_node) {
 		parent = link->parent;
 
-		if (!parent->set_performance_state)
-			continue;
-
 		genpd_lock_nested(parent, depth + 1);
 
 		parent_state = link->prev_performance_state;

@@ -399,9 +405,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	if (!genpd)
 		return -ENODEV;
 
-	if (unlikely(!genpd->set_performance_state))
-		return -EINVAL;
-
 	if (WARN_ON(!dev->power.subsys_data ||
 		     !dev->power.subsys_data->domain_data))
 		return -EINVAL;

Signature · 1 line

2.29.2

Viresh Kumar

Reviewed-by on v5 1/3

2021-01-21 04:09 UTC #2 lore raw

Re: [PATCH v5 1/3] PM: domains: Make set_performance_state() callback optional

Quoted 12 lines · Dmitry Osipenko

Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.
Tested-by Peter Geis <[email protected]>
Tested-by Nicolas Chauvet <[email protected]>
Tested-by Matt Merhar <[email protected]>
[tested on NVIDIA Tegra20/30/124 SoCs]
Suggested-by Ulf Hansson <[email protected]>
Reviewed-by Ulf Hansson <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>

Quoted 3 lines

---
 drivers/base/power/domain.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Reviewed-by Viresh Kumar <[email protected]>

Signature · 1 line

viresh

Dmitry Osipenko

author

2021-01-20 21:27 UTC #3 lore raw

PATCH v5 2/3 PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER

In reply to Dmitry Osipenko

Driver of a power domain provider may not be ready at the time of of_genpd_add_subdomain() invocation. Make this function to return -EPROBE_DEFER instead of -ENOENT in order to remove a need from power domain drivers to handle the error code specially.

Tested-by Peter Geis <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Matt Merhar <[email protected]>

[tested on NVIDIA Tegra20/30/124 SoCs]

Reviewed-by Ulf Hansson <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

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

---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/power/domain.c +1 -1

index 5c843772f7f4..25b346bb2d97 100644
@@ -2466,7 +2466,7 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
 out:
 	mutex_unlock(&gpd_list_lock);
 
-	return ret;
+	return ret == -ENOENT ? -EPROBE_DEFER : ret;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);

Signature · 1 line

2.29.2

Dmitry Osipenko

author

2021-01-20 21:25 UTC #4 lore raw

PATCH v5 3/3 PM: domains: Add "performance" column to debug summary

In reply to Dmitry Osipenko

Add "performance" column to debug summary which shows performance state of all power domains and theirs devices.

Tested-by Peter Geis <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Matt Merhar <[email protected]>

[tested on NVIDIA Tegra20/30/124 SoCs]

Reviewed-by Ulf Hansson <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

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

---
 drivers/base/power/domain.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

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

index 25b346bb2d97..40b72b1d8e8f 100644
@@ -2955,7 +2955,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
 	else
 		WARN_ON(1);
 
-	seq_puts(s, p);
+	seq_printf(s, "%-25s  ", p);
+}
+
+static void perf_status_str(struct seq_file *s, struct device *dev)
+{
+	struct generic_pm_domain_data *gpd_data;
+
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	seq_put_decimal_ull(s, "", gpd_data->performance_state);
 }
 
 static int genpd_summary_one(struct seq_file *s,

@@ -2983,7 +2991,7 @@ static int genpd_summary_one(struct seq_file *s,
 	else
 		snprintf(state, sizeof(state), "%s",
 			 status_lookup[genpd->status]);
-	seq_printf(s, "%-30s  %-15s ", genpd->name, state);
+	seq_printf(s, "%-30s  %-50s %u", genpd->name, state, genpd->performance_state);
 
 	/*
 	 * Modifications on the list require holding locks on both

@@ -2991,6 +2999,8 @@ static int genpd_summary_one(struct seq_file *s,
 	 * Also genpd->name is immutable.
 	 */
 	list_for_each_entry(link, &genpd->parent_links, parent_node) {
+		if (list_is_first(&link->parent_node, &genpd->parent_links))
+			seq_printf(s, "\n%48s", " ");
 		seq_printf(s, "%s", link->child->name);
 		if (!list_is_last(&link->parent_node, &genpd->parent_links))
 			seq_puts(s, ", ");

@@ -3005,6 +3015,7 @@ static int genpd_summary_one(struct seq_file *s,
 
 		seq_printf(s, "\n    %-50s  ", kobj_path);
 		rtpm_status_str(s, pm_data->dev);
+		perf_status_str(s, pm_data->dev);
 		kfree(kobj_path);
 	}

@@ -3020,9 +3031,9 @@ static int summary_show(struct seq_file *s, void *data)
 	struct generic_pm_domain *genpd;
 	int ret = 0;
 
-	seq_puts(s, "domain                          status          children\n");
+	seq_puts(s, "domain                          status          children                           performance\n");
 	seq_puts(s, "    /device                                             runtime status\n");
-	seq_puts(s, "----------------------------------------------------------------------\n");
+	seq_puts(s, "----------------------------------------------------------------------------------------------\n");
 
 	ret = mutex_lock_interruptible(&gpd_list_lock);
 	if (ret)

Signature · 1 line

2.29.2

Rafael J. Wysocki

maintainer

2021-01-22 16:19 UTC #5 lore raw

In reply to Dmitry Osipenko

Quoted 39 lines · Dmitry Osipenko

Hi,
This series is a prerequisite for the power domain driver of NVIDIA Tegra SoCs [1]. It extends the GENPD core with a better support of performance states and eases linking of child/parent PDs for the PD drivers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
Changelog:
v5: - Renamed genpd_xlate_performance_state() and its arguments like it
was requested by Ulf Hansson and Viresh Kumar in a v4 review.
- Added r-b from Ulf Hansson to "Make set_performance_state() callback optional" patch.
v4: - Updated the "Make set_performance_state() callback optional" patch.
Now the case where one of intermediate domains doesn't implement the set_performance_state() callback is handled properly, thanks to Viresh and Ulf for catching this drawback and suggesting the fix.
- Added more r-bs from Ulf Hansson and Viresh Kumar.
v3: - Added r-b from Ulf Hansson.
- Added new patch "Make of_genpd_add_subdomain() to return -EPROBE_DEFER", which was suggested by Ulf Hansson.
- Improved "Add "performance" column to debug summary" patch by correcting the formatting of debug output, which had a superfluous whitespace.
Dmitry Osipenko (3):
PM: domains: Make set_performance_state() callback optional
PM: domains: Make of_genpd_add_subdomain() to return -EPROBE_DEFER
PM: domains: Add "performance" column to debug summary

Quoted 4 lines

 drivers/base/power/domain.c | 54 +++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 20 deletions(-)

All patches applied as 5.12 material, thanks!