On 11/12/2015 12:46, Ulf Hansson wrote:
On 17 November 2015 at 23:37, Lina Iyer <lina.iyer@xxxxxxxxxx> wrote:
Hi Ulf,
thanks for the review, comments and questions below.
Best Regards,
Marc.
From: Marc Titinger <mtitinger@xxxxxxxxxxxx>
This purpose of these debug seq-files is to help investigate
generic power domain state transitions, based on device constraints.
requires the "multiple states" patches from Axel Haslam.
This last sentence doesn't belong in the change-log, please remove it.
also rename 'summary' from 'pm_genpd_summary'
sample output for 'states'
==========================
Domain State name Eval = 2200nter + Exit = Min_off_on (ns)
-----------------------------------------------------------------------
a53_pd cluster-sleep-0 1500000+800000=2300000
a57_pd d1-retention 1000000+800000=1800000
a57_pd cluster-sleep-0 1500000+800000=2300000
sample output for 'timings'
===========================
Domain Devices, Timings in ns
Stop/Start Save/Restore, Effective
---------------------------------------------------- ---
a53_pd
/cpus/cpu@100 1060 /660 1580 /1940 ,0 (cached stop)
/cpus/cpu@101 1060 /740 1520 /1600 ,0 (cached stop)
/cpus/cpu@102 880 /620 1380 /1780 ,0 (cached stop)
/cpus/cpu@103 1080 /640 1340 /1600 ,0 (cached stop)
a57_pd
/cpus/cpu@0 1160 /740 3280 /1800 ,0 (cached stop)
/cpus/cpu@1 780 /1400 1440 /2080 ,0 (cached stop)
/D1 600 /540 7140 /6420 ,2199460 (cached stop)
Signed-off-by: Marc Titinger <mtitinger@xxxxxxxxxxxx>
A general comment. Static functions in genpd shall start with one of
the following prefix.
genpd_*
_genpd_*
__genpd_*
Please change accordingly.
Many static routines were already prefixed like the exported functions
with "pm_", shall I make a separate patch for this renaming ?
---
drivers/base/power/domain.c | 115 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 107 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index c300293..9a0df09 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2169,21 +2169,120 @@ static const struct file_operations pm_genpd_summary_fops = {
.release = single_release,
};
+static int pm_genpd_states_show(struct seq_file *s, void *data)
+{
+ struct generic_pm_domain *genpd;
+
+ seq_puts(s,
+ "\n Domain State name Enter + Exit = Min_off_on (ns)\n");
+ seq_puts(s,
+ "-----------------------------------------------------------------------\n");
+
You must hold the gpd_list_lock while traversing the gpd list.
+ list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+
+ int i;
+
+ for (i = 0; i < genpd->state_count; i++) {
To be sure you have valid data, you should hold the genpd lock here.
At some point while testing with calling suspend from the power_off
handler, the cpu would go to sleep with the lock held, hence using this
seq-file would not work.
while I agree, I think it is not super likely that a
domain/child/devices/states are added or removed at this point (the DT
is parsed already), would using list_for_each_entry_safe be safe enough ?
+ seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+ genpd->name,
+ genpd->states[i].name,
+ genpd->states[i].power_on_latency_ns,
+ genpd->states[i].power_off_latency_ns,
+ genpd->states[i].power_off_latency_ns
+ + genpd->states[i].power_on_latency_ns);
+ }
+
+ }
+
+ seq_puts(s, "\n");
+
+ return 0;
+}
+
+static int pm_genpd_states_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pm_genpd_states_show, NULL);
+}
+
+static const struct file_operations pm_genpd_states_fops = {
+ .open = pm_genpd_states_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int pm_genpd_timing_show(struct seq_file *s, void *data)
+{
+ struct generic_pm_domain *genpd;
+
+ seq_puts(s, "\n Domain Devices, Timings in ns\n");
+ seq_puts(s,
+ " Stop/Start Save/Restore, Effective\n");
+ seq_puts(s,
+ "---------------------------------------------------- ---\n");
+
You must hold the gpd_list_lock while traversing the gpd list.
+ list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+ struct pm_domain_data *pm_data;
+
+ seq_printf(s, "%-30s", genpd->name);
+
You must hold the genpd lock while traversing the device list.
+ list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
+ struct gpd_timing_data *td = &to_gpd_data(pm_data)->td;
+
+ if (!pm_data->dev->of_node)
+ continue;
+
+ seq_printf(s,
+ "\n %-20s %-6lld/%-6lld %-6lld/%-6lld,%lld %s%s",
+ pm_data->dev->of_node->full_name,
+ td->stop_latency_ns, td->start_latency_ns,
+ td->save_state_latency_ns,
+ td->restore_state_latency_ns,
This needs a re-base as these values have been merged.
+ td->effective_constraint_ns,
+ td->cached_stop_ok ? "(cached stop) " : "",
+ td->constraint_changed ? "(changed)" : "");
+ }
+ seq_puts(s, "\n");
+ }
+ return 0;
+}
+
+static int pm_genpd_timing_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pm_genpd_timing_show, NULL);
+}
+
+static const struct file_operations pm_genpd_timing_fops = {
+ .open = pm_genpd_timing_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
static int __init pm_genpd_debug_init(void)
{
- struct dentry *d;
+ struct dentry *d = NULL;
pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
- if (!pm_genpd_debugfs_dir)
- return -ENOMEM;
+ if (pm_genpd_debugfs_dir)
In case when CONFIG_DEBUG_FS is unset, this doesn't work very well, as
pm_genpd_debugfs_dir will contain an error code.
Since you are anyway make quite big change to the debugfs support for
genpd, perhaps you can try to fix that up as well!?
Ah yes ok, will do.
+ d = debugfs_create_file("summary", S_IRUGO,
+ pm_genpd_debugfs_dir, NULL,
+ &pm_genpd_summary_fops);
+ if (d)
+ d = debugfs_create_file("states", S_IRUGO,
+ pm_genpd_debugfs_dir, NULL,
+ &pm_genpd_states_fops);
+ if (d)
+ d = debugfs_create_file("timings", S_IRUGO,
+ pm_genpd_debugfs_dir, NULL,
+ &pm_genpd_timing_fops);
+ if (d)
+ return 0;
- d = debugfs_create_file("pm_genpd_summary", S_IRUGO,
- pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
- if (!d)
- return -ENOMEM;
+ debugfs_remove_recursive(pm_genpd_debugfs_dir /*can be null*/);
- return 0;
+ return -ENOMEM;
}
late_initcall(pm_genpd_debug_init);
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
