On 17 November 2015 at 23:37, Lina Iyer <lina.i...@linaro.org> wrote:
> From: Marc Titinger <mtitin...@baylibre.com>
>
> 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 <mtitin...@baylibre.com>
A general comment. Static functions in genpd shall start with one of
the following prefix.
genpd_*
_genpd_*
__genpd_*
Please change accordingly.
> ---
> 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.
> + 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!?
> + 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
