From: mtitinger@baylibre•com (Marc Titinger)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files
Date: Wed, 16 Dec 2015 12:07:13 +0100 [thread overview]
Message-ID: <567145E1.5040804@baylibre.com> (raw)
In-Reply-To: <CAPDyKFrOPKc-WDsHNpndkoZa5g6tnnbE2C5JsqZ6DMCX+6hmFw@mail.gmail.com>
On 11/12/2015 12:46, Ulf Hansson wrote:
> On 17 November 2015 at 23:37, Lina Iyer <lina.iyer@linaro•org> wrote:
Hi Ulf,
thanks for the review, comments and questions below.
Best Regards,
Marc.
>> From: Marc Titinger <mtitinger@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 at 100 1060 /660 1580 /1940 ,0 (cached stop)
>> /cpus/cpu at 101 1060 /740 1520 /1600 ,0 (cached stop)
>> /cpus/cpu at 102 880 /620 1380 /1780 ,0 (cached stop)
>> /cpus/cpu at 103 1080 /640 1340 /1600 ,0 (cached stop)
>> a57_pd
>> /cpus/cpu at 0 1160 /740 3280 /1800 ,0 (cached stop)
>> /cpus/cpu at 1 780 /1400 1440 /2080 ,0 (cached stop)
>> /D1 600 /540 7140 /6420 ,2199460 (cached stop)
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre•com>
>
> 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
>
next prev parent reply other threads:[~2015-12-16 11:07 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-17 22:37 [PATCH RFC 00/27] PM/Domains: Cluster idle support for ARM SoCs Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 01/27] PM / Domains: core changes for multiple states Lina Iyer
2015-12-09 13:58 ` Ulf Hansson
2015-12-17 17:58 ` Axel Haslam
2015-12-17 21:19 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 02/27] PM / Domains: Allow domain power states to be read from DT Lina Iyer
2015-12-10 16:53 ` Ulf Hansson
2015-12-15 10:07 ` Marc Titinger
2015-12-15 22:14 ` Lina Iyer
2015-12-16 21:36 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 03/27] PM / Domain: Add additional state specific param Lina Iyer
2015-11-19 21:33 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 04/27] PM / Domains: make governor select deepest state Lina Iyer
2015-12-11 9:13 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 05/27] PM / Domains: remove old power on/off latencies Lina Iyer
2015-11-18 14:57 ` [PATCH] ARM: imx6: pm: declare pm domain latency on power_state struct Lina Iyer
2015-11-23 13:31 ` Lucas Stach
2015-11-23 13:42 ` Lucas Stach
2015-12-04 23:19 ` Lina Iyer
2015-12-11 9:16 ` [PATCH RFC 05/27] PM / Domains: remove old power on/off latencies Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 06/27] PM / Domains: add debugfs 'states' and 'timings' seq files Lina Iyer
2015-12-11 11:46 ` Ulf Hansson
2015-12-16 11:07 ` Marc Titinger [this message]
2015-12-16 12:48 ` Ulf Hansson
2015-12-16 14:12 ` Marc Titinger
2015-11-17 22:37 ` [PATCH RFC 07/27] PM / Domains: Read domain residency from DT Lina Iyer
2015-11-24 20:41 ` Stephen Boyd
2015-12-11 11:54 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 08/27] PM / Domains: Support IRQ safe PM domains Lina Iyer
2016-01-14 14:42 ` Ulf Hansson
2016-01-14 18:33 ` Lina Iyer
2016-01-15 8:55 ` Ulf Hansson
2016-01-15 16:57 ` Lina Iyer
2016-01-15 22:08 ` Ulf Hansson
2016-01-18 16:58 ` Lina Iyer
2016-01-18 17:00 ` Lina Iyer
2016-01-19 10:01 ` Ulf Hansson
2015-11-17 22:37 ` [PATCH RFC 09/27] PM / Domains: Attempt runtime suspend of IRQ safe parent domain Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 10/27] drivers: power: Introduce PM domains for CPUs/clusters Lina Iyer
2015-11-24 20:52 ` Stephen Boyd
2015-11-17 22:37 ` [PATCH RFC 11/27] drivers: cpu: Define CPU devices as IRQ safe Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 12/27] ARM: cpuidle: remove cpu parameter from the cpuidle_ops suspend hook Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 13/27] ARM: cpuidle: Add runtime PM support for CPU idle Lina Iyer
2015-11-18 8:50 ` Zhaoyang Huang
2015-11-18 14:17 ` Lina Iyer
2015-11-19 22:10 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 14/27] tick: get next wakeup event for the CPU Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 15/27] PM / Domains: Add next_wakeup to device's timing data Lina Iyer
2015-11-19 22:19 ` Kevin Hilman
2015-11-20 15:58 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 16/27] ARM: cpuidle: Record the next wakeup event of the CPU Lina Iyer
2015-11-19 23:35 ` Kevin Hilman
2015-11-20 16:28 ` Lina Iyer
2015-11-24 18:29 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 17/27] drivers: cpu-pd: Record CPUs that are part of the domain Lina Iyer
2015-11-24 21:00 ` Stephen Boyd
2015-11-25 14:13 ` Lina Iyer
2015-11-25 19:12 ` Stephen Boyd
2015-11-25 20:20 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 18/27] drivers: cpu-pd: Add PM Domain governor for CPUs Lina Iyer
2015-11-18 18:42 ` Lorenzo Pieralisi
2015-11-19 8:50 ` Marc Titinger
2015-11-20 17:39 ` Lina Iyer
2015-11-19 23:52 ` Kevin Hilman
2015-11-20 16:21 ` Lorenzo Pieralisi
2015-11-20 16:42 ` Lina Iyer
2015-11-20 0:03 ` Kevin Hilman
2015-11-17 22:37 ` [PATCH RFC 19/27] drivers: cpu-pd: Invoke CPU PM runtime on hotplug Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 20/27] Documentation: ARM: topology: 'cluster' property for cluster nodes Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 21/27] drivers: cpu-pd: Parse topology to setup CPU PM domains Lina Iyer
2015-12-07 14:54 ` Lorenzo Pieralisi
2015-12-08 18:05 ` Lina Iyer
2015-12-10 18:11 ` Lorenzo Pieralisi
2015-12-11 9:04 ` Geert Uytterhoeven
2015-12-11 20:51 ` Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 22/27] drivers: firmware: PSCI: Export psci_has_ext_power_state() Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 23/27] ARM64: psci: Support cluster idle states for OS-Initated Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 24/27] arm64: dts: Add Qualcomm MSM8916, MTP8916, APQ8016, SBC8016 ids Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 25/27] devicetree: bindings: Document qcom, msm-id and qcom, board-id Lina Iyer
2015-11-19 14:36 ` Rob Herring
2015-11-19 15:36 ` [PATCH RFC 25/27] devicetree: bindings: Document qcom, msm-id and qcom,board-id Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 26/27] ARM64: dts: Add PSCI cpuidle support for MSM8916 Lina Iyer
2015-11-17 22:37 ` [PATCH RFC 27/27] ARM64: dts: Define CPU power domain " Lina Iyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=567145E1.5040804@baylibre.com \
--to=mtitinger@baylibre$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox