[PATCH 0/2] PM / devfreq: Add debugfs support

[PATCH 0/2] PM / devfreq: Add debugfs support

[Chanwoo Choi]

Add debugfs interface to provide debugging information of devfreq device.
It contains 'devfreq_summary' and 'debug_transitions' debugfs file
in order to provide the simple profiling to user via debugfs
without any specific profiling tool.

[Added debugfs file]
- "/sys/kernel/debug/devfreq/devfreq_summary"
  : Show the summary of the registered devfreq devices.
- "/sys/kernel/debug/devfreq/devfreq_transitions"
  : Show the frequency transition of the registered devfreq devices.

Recommend the each patch to check the detailed description
of each fields of both devfreq_summary and devfreq_transitions.

This series contains the patch[1] and add the patch2 for 'devfreq_transitions'
[1] https://patchwork.kernel.org/patch/11320265/
- [v3] PM / devfreq: Add debugfs support with devfreq_summary file


For example on Exynos5422-based Odroid-XU3 board,
- In order to show the multiple governors on devfreq_summay result,
change the governor of devfreq0 from simple_ondemand to userspace.

$ cat /sys/kernel/debug/devfreq/devfreq_summary
dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000

$ cat /sys/kernel/debug/devfreq/devfreq_transitions
time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
[snip]


Depends on:
It depends on patch[2] for preventing the merge conflic.
[2] https://patchwork.kernel.org/patch/11320257/
- PM / devfreq: Add missing function description and rename static functions

Chanwoo Choi (2):
  PM / devfreq: Add debugfs support with devfreq_summary file
  PM / devfreq: Add devfreq_transitions debugfs file

 drivers/devfreq/Kconfig            |  13 ++
 drivers/devfreq/devfreq.c          | 206 +++++++++++++++++++++++++++++
 drivers/devfreq/governor.h         |   3 +
 drivers/devfreq/governor_passive.c |   2 +
 include/linux/devfreq.h            |   1 +
 5 files changed, 225 insertions(+)

-- 
2.17.1

[PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Chanwoo Choi]

Add debugfs interface to provide debugging information of devfreq device.
It contains 'devfreq_summary' entry to show the summary of registered
devfreq devices as following and the additional debugfs file will be added.
- /sys/kernel/debug/devfreq/devfreq_summary

[Detailed description of each field of 'devfreq_summary' debugfs file]
- dev_name	: Device name of h/w.
- dev		: Device name made by devfreq core.
- parent_dev	: If devfreq device uses the passive governor,
		  show parent devfreq device name.
- governor	: Devfreq governor.
- polling_ms	: If devfreq device uses the simple_ondemand governor,
		  polling_ms is necessary for the period. (unit: millisecond)
- cur_freq_hz	: Current Frequency (unit: hz)
- old_freq_hz	: Frequency before changing. (unit: hz)
- new_freq_hz	: Frequency after changed. (unit: hz)

[For example on Exynos5422-based Odroid-XU3 board]
- In order to show the multiple governors on devfreq_summay result,
change the governor of devfreq0 from simple_ondemand to userspace.

$ cat /sys/kernel/debug/devfreq/devfreq_summary
dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 254f11b31824..c7f5e4e06420 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/kmod.h>
 #include <linux/sched.h>
+#include <linux/debugfs.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -33,6 +34,7 @@
 #define HZ_PER_KHZ	1000
 
 static struct class *devfreq_class;
+static struct dentry *devfreq_debugfs;
 
 /*
  * devfreq core provides delayed work based load monitoring helper
@@ -1717,6 +1719,74 @@ static struct attribute *devfreq_attrs[] = {
 };
 ATTRIBUTE_GROUPS(devfreq);
 
+/**
+ * devfreq_summary_show() - Show the summary of the registered devfreq devices
+ *				via 'devfreq_summary' debugfs file.
+ */
+static int devfreq_summary_show(struct seq_file *s, void *data)
+{
+	struct devfreq *devfreq;
+	struct devfreq *p_devfreq = NULL;
+	unsigned long cur_freq, min_freq, max_freq;
+	unsigned int polling_ms;
+
+	seq_printf(s, "%-30s %-10s %-10s %-15s %-10s %-12s %-12s %-12s\n",
+			"dev_name",
+			"dev",
+			"parent_dev",
+			"governor",
+			"polling_ms",
+			"cur_freq_hz",
+			"min_freq_hz",
+			"max_freq_hz");
+	seq_printf(s, "%-30s %-10s %-10s %-15s %-10s %-12s %-12s %-12s\n",
+			"------------------------------",
+			"----------",
+			"----------",
+			"---------------",
+			"----------",
+			"------------",
+			"------------",
+			"------------");
+
+	mutex_lock(&devfreq_list_lock);
+
+	list_for_each_entry_reverse(devfreq, &devfreq_list, node) {
+#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
+		if (!strncmp(devfreq->governor_name, DEVFREQ_GOV_PASSIVE,
+							DEVFREQ_NAME_LEN)) {
+			struct devfreq_passive_data *data = devfreq->data;
+
+			if (data)
+				p_devfreq = data->parent;
+		} else {
+			p_devfreq = NULL;
+		}
+#endif
+		mutex_lock(&devfreq->lock);
+		cur_freq = devfreq->previous_freq,
+		get_freq_range(devfreq, &min_freq, &max_freq);
+		polling_ms = devfreq->profile->polling_ms,
+		mutex_unlock(&devfreq->lock);
+
+		seq_printf(s,
+			"%-30s %-10s %-10s %-15s %-10d %-12ld %-12ld %-12ld\n",
+			dev_name(devfreq->dev.parent),
+			dev_name(&devfreq->dev),
+			p_devfreq ? dev_name(&p_devfreq->dev) : "",
+			devfreq->governor_name,
+			polling_ms,
+			cur_freq,
+			min_freq,
+			max_freq);
+	}
+
+	mutex_unlock(&devfreq_list_lock);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
+
 static int __init devfreq_init(void)
 {
 	devfreq_class = class_create(THIS_MODULE, "devfreq");
@@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
 	}
 	devfreq_class->dev_groups = devfreq_groups;
 
+	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
+	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {
+		devfreq_debugfs = NULL;
+		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
+	} else {
+		debugfs_create_file("devfreq_summary", 0444,
+				devfreq_debugfs, NULL,
+				&devfreq_summary_fops);
+	}
+
 	return 0;
 }
 subsys_initcall(devfreq_init);
-- 
2.17.1

[PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

Add new devfreq_transitions debugfs file to track the frequency transitions
of all devfreq devices for the simple profiling as following:
- /sys/kernel/debug/devfreq/devfreq_transitions

And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
in Kconfig in order to save the transition history.

[Detailed description of each field of 'devfreq_transitions' debugfs file]
- time_ms	: Change time of frequency transition. (unit: millisecond)
- dev_name	: Device name of h/w.
- dev		: Device name made by devfreq core.
- parent_dev	: If devfreq device uses the passive governor,
		  show parent devfreq device name.
- load_%	: If devfreq device uses the simple_ondemand governor,
		  load is used by governor whene deciding the new frequency.
		  (unit: percentage)
- old_freq_hz	: Frequency before changing. (unit: hz)
- new_freq_hz	: Frequency after changed. (unit: hz)

[For example on Exynos5422-based Odroid-XU3 board]
$ cat /sys/kernel/debug/devfreq/devfreq_transitions
time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
[snip]

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/Kconfig            |  13 +++
 drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
 drivers/devfreq/governor.h         |   3 +
 drivers/devfreq/governor_passive.c |   2 +
 include/linux/devfreq.h            |   1 +
 5 files changed, 145 insertions(+)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 0b1df12e0f21..84936eec0ef9 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
 	  through sysfs entries. The passive governor recommends that
 	  devfreq device uses the OPP table to get the frequency/voltage.
 
+comment "DEVFREQ Debugging"
+
+config NR_DEVFREQ_TRANSITIONS
+	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
+	depends on DEBUG_FS
+	range 10 1000
+	default "100"
+	help
+	  Show the frequency transitions of all devfreq devices via
+	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
+	  profiling. It needs to decide the storage size to save transition
+	  history of all devfreq devices.
+
 comment "DEVFREQ Drivers"
 
 config ARM_EXYNOS_BUS_DEVFREQ
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index c7f5e4e06420..7abaae06fa65 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
 }
 EXPORT_SYMBOL(devfreq_update_status);
 
+/**
+ * devfreq_update_transitions() - Update frequency transitions for debugfs file
+ * @devfreq:	the devfreq instance
+ * @old_freq:	the previous frequency before changing the frequency
+ * @new_freq:	the new frequency after frequency is changed
+ */
+struct devfreq_transitions {
+	struct devfreq *devfreq;
+	struct devfreq_freqs freqs;
+	unsigned long load;
+} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
+
+static spinlock_t devfreq_debugfs_lock;
+static int debugfs_transitions_index;
+
+void devfreq_update_transitions(struct devfreq *devfreq,
+			unsigned long old_freq, unsigned long new_freq,
+			unsigned long busy_time, unsigned long total_time)
+{
+	unsigned long load;
+	int i;
+
+	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
+		return;
+
+	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
+
+	i = debugfs_transitions_index;
+
+	/*
+	 * Calculate the load and if load is larger than 100,
+	 * initialize to 100 because the unit of load is percentage.
+	 */
+	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
+	if (load > 100)
+		load = 100;
+
+	debugfs_transitions[i].devfreq = devfreq;
+	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
+	debugfs_transitions[i].freqs.old = old_freq;
+	debugfs_transitions[i].freqs.new = new_freq;
+	debugfs_transitions[i].load = load;
+
+	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
+		i = 0;
+	debugfs_transitions_index = i;
+
+	spin_unlock(&devfreq_debugfs_lock);
+}
+EXPORT_SYMBOL(devfreq_update_transitions);
+
 /**
  * find_devfreq_governor() - Find devfreq governor from name
  * @name:	name of the governor
@@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
 		return err;
 	}
 
+	devfreq_update_transitions(devfreq, cur_freq, new_freq,
+					devfreq->last_status.busy_time,
+					devfreq->last_status.total_time);
+
 	freqs.new = new_freq;
 	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
 
@@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
 
+/**
+ * devfreq_transitions_show() - Show the frequency transitions of the registered
+ *			devfreq devices via 'devfreq_transitions' debugfs file.
+ */
+static int devfreq_transitions_show(struct seq_file *s, void *data)
+{
+	struct devfreq *devfreq = NULL;
+	struct devfreq *p_devfreq = NULL;
+	struct devfreq_freqs *freqs = NULL;
+	unsigned long load;
+	int i = debugfs_transitions_index;
+	int count;
+
+	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
+			"time_ms",
+			"dev_name",
+			"dev",
+			"parent_dev",
+			"load_%",
+			"old_freq_hz",
+			"new_freq_hz");
+	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
+			"----------",
+			"------------------------------",
+			"----------",
+			"----------",
+			"----------",
+			"------------",
+			"------------");
+
+	spin_lock(&devfreq_debugfs_lock);
+	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
+		devfreq = debugfs_transitions[i].devfreq;
+		freqs = &debugfs_transitions[i].freqs;
+		load = debugfs_transitions[i].load;
+
+		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
+		if (!devfreq)
+			continue;
+
+#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
+		if (!strncmp(devfreq->governor_name,
+				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
+			struct devfreq_passive_data *data = devfreq->data;
+
+			if (data)
+				p_devfreq = data->parent;
+		} else {
+			p_devfreq = NULL;
+		}
+#endif
+		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
+			freqs->time,
+			dev_name(devfreq->dev.parent),
+			dev_name(&devfreq->dev),
+			p_devfreq ? dev_name(&p_devfreq->dev) : "",
+			load,
+			freqs->old,
+			freqs->new);
+	}
+	spin_unlock(&devfreq_debugfs_lock);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
+
 static int __init devfreq_init(void)
 {
 	devfreq_class = class_create(THIS_MODULE, "devfreq");
@@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
 		devfreq_debugfs = NULL;
 		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
 	} else {
+		spin_lock_init(&devfreq_debugfs_lock);
+
 		debugfs_create_file("devfreq_summary", 0444,
 				devfreq_debugfs, NULL,
 				&devfreq_summary_fops);
+		debugfs_create_file("devfreq_transitions", 0444,
+				devfreq_debugfs, NULL,
+				&devfreq_transitions_fops);
 	}
 
 	return 0;
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index dc7533ccc3db..01eecfdaf2d6 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
 extern int devfreq_remove_governor(struct devfreq_governor *governor);
 
 extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
+extern void devfreq_update_transitions(struct devfreq *devfreq,
+			unsigned long old_freq, unsigned long new_freq,
+			unsigned long busy_time, unsigned long total_time);
 
 static inline int devfreq_update_stats(struct devfreq *df)
 {
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index be6eeab9c814..05fa654239f5 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
 	if (ret < 0)
 		goto out;
 
+	devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
+
 	if (devfreq->profile->freq_table
 		&& (devfreq_update_status(devfreq, freq)))
 		dev_err(&devfreq->dev,
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 49cdb2378030..933692e5d867 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -196,6 +196,7 @@ struct devfreq {
 };
 
 struct devfreq_freqs {
+	s64 time;
 	unsigned long old;
 	unsigned long new;
 };
-- 
2.17.1

Re: [PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Bjorn Andersson]

On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:

> Add debugfs interface to provide debugging information of devfreq device.
> It contains 'devfreq_summary' entry to show the summary of registered
> devfreq devices as following and the additional debugfs file will be added.
> - /sys/kernel/debug/devfreq/devfreq_summary
> 
> [Detailed description of each field of 'devfreq_summary' debugfs file]
> - dev_name	: Device name of h/w.
> - dev		: Device name made by devfreq core.
> - parent_dev	: If devfreq device uses the passive governor,
> 		  show parent devfreq device name.
> - governor	: Devfreq governor.
> - polling_ms	: If devfreq device uses the simple_ondemand governor,
> 		  polling_ms is necessary for the period. (unit: millisecond)
> - cur_freq_hz	: Current Frequency (unit: hz)
> - old_freq_hz	: Frequency before changing. (unit: hz)
> - new_freq_hz	: Frequency after changed. (unit: hz)
> 
> [For example on Exynos5422-based Odroid-XU3 board]
> - In order to show the multiple governors on devfreq_summay result,
> change the governor of devfreq0 from simple_ondemand to userspace.
> 
> $ cat /sys/kernel/debug/devfreq/devfreq_summary
> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000

This looks quite useful.

> 
> Reported-by: kbuild test robot <lkp@intel.com>

May I ask how the build test robot came up with this idea?

> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
[..]
> +/**
> + * devfreq_summary_show() - Show the summary of the registered devfreq devices
> + *				via 'devfreq_summary' debugfs file.

Please make this proper kerneldoc, i.e:

 * func() - short description
 * @s:
 * @data:
 * 
 * Long description
 * 
 * Return: foo on bar

[..]
> @@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
>  	}
>  	devfreq_class->dev_groups = devfreq_groups;
>  
> +	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
> +	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {

Don't PTR_ERR() before IS_ERR().

> +		devfreq_debugfs = NULL;

I don't think you need this, given that debugfs_create_file() will fail
gracefully when passed and IS_ERR()

> +		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);

Afaict debugfs_create_() won't fail without printing a message already.

> +	} else {
> +		debugfs_create_file("devfreq_summary", 0444,
> +				devfreq_debugfs, NULL,
> +				&devfreq_summary_fops);
> +	}
> +
>  	return 0;
>  }
>  subsys_initcall(devfreq_init);

Regards,
Bjorn

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

Hello Chanwoo,

07.01.2020 12:05, Chanwoo Choi пишет:
> Add new devfreq_transitions debugfs file to track the frequency transitions
> of all devfreq devices for the simple profiling as following:
> - /sys/kernel/debug/devfreq/devfreq_transitions
> 
> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
> in Kconfig in order to save the transition history.
> 
> [Detailed description of each field of 'devfreq_transitions' debugfs file]
> - time_ms	: Change time of frequency transition. (unit: millisecond)
> - dev_name	: Device name of h/w.
> - dev		: Device name made by devfreq core.
> - parent_dev	: If devfreq device uses the passive governor,
> 		  show parent devfreq device name.
> - load_%	: If devfreq device uses the simple_ondemand governor,
> 		  load is used by governor whene deciding the new frequency.
> 		  (unit: percentage)
> - old_freq_hz	: Frequency before changing. (unit: hz)
> - new_freq_hz	: Frequency after changed. (unit: hz)
> 
> [For example on Exynos5422-based Odroid-XU3 board]
> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> [snip]
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/Kconfig            |  13 +++
>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>  drivers/devfreq/governor.h         |   3 +
>  drivers/devfreq/governor_passive.c |   2 +
>  include/linux/devfreq.h            |   1 +
>  5 files changed, 145 insertions(+)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..84936eec0ef9 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>  	  through sysfs entries. The passive governor recommends that
>  	  devfreq device uses the OPP table to get the frequency/voltage.
>  
> +comment "DEVFREQ Debugging"
> +
> +config NR_DEVFREQ_TRANSITIONS
> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
> +	depends on DEBUG_FS
> +	range 10 1000
> +	default "100"
> +	help
> +	  Show the frequency transitions of all devfreq devices via
> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
> +	  profiling. It needs to decide the storage size to save transition
> +	  history of all devfreq devices.
> +
>  comment "DEVFREQ Drivers"
>  
>  config ARM_EXYNOS_BUS_DEVFREQ
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index c7f5e4e06420..7abaae06fa65 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  }
>  EXPORT_SYMBOL(devfreq_update_status);
> +/**
> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
> + * @devfreq:	the devfreq instance
> + * @old_freq:	the previous frequency before changing the frequency
> + * @new_freq:	the new frequency after frequency is changed
> + */
> +struct devfreq_transitions {
> +	struct devfreq *devfreq;
> +	struct devfreq_freqs freqs;
> +	unsigned long load;
> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
> +
> +static spinlock_t devfreq_debugfs_lock;

This could be:

static DEFINE_SPINLOCK(devfreq_debugfs_lock);

and then spin_lock_init() isn't needed.


Also, The "<linux/spinlock.h>" should be included directly by devfreq.c

> +static int debugfs_transitions_index;
> +
> +void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time)
> +{
> +	unsigned long load;
> +	int i;
> +
> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
> +		return;
> +
> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
> +
> +	i = debugfs_transitions_index;
> +
> +	/*
> +	 * Calculate the load and if load is larger than 100,
> +	 * initialize to 100 because the unit of load is percentage.
> +	 */
> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
> +	if (load > 100)
> +		load = 100;
> +
> +	debugfs_transitions[i].devfreq = devfreq;
> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
> +	debugfs_transitions[i].freqs.old = old_freq;
> +	debugfs_transitions[i].freqs.new = new_freq;
> +	debugfs_transitions[i].load = load;
> +
> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
> +		i = 0;
> +	debugfs_transitions_index = i;
> +
> +	spin_unlock(&devfreq_debugfs_lock);
> +}
> +EXPORT_SYMBOL(devfreq_update_transitions);

What about EXPORT_SYMBOL_GPL()?

>  /**
>   * find_devfreq_governor() - Find devfreq governor from name
>   * @name:	name of the governor
> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>  		return err;
>  	}
>  
> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
> +					devfreq->last_status.busy_time,
> +					devfreq->last_status.total_time);
> +
>  	freqs.new = new_freq;
>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>  
> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>  
> +/**
> + * devfreq_transitions_show() - Show the frequency transitions of the registered
> + *			devfreq devices via 'devfreq_transitions' debugfs file.
> + */
> +static int devfreq_transitions_show(struct seq_file *s, void *data)
> +{
> +	struct devfreq *devfreq = NULL;
> +	struct devfreq *p_devfreq = NULL;
> +	struct devfreq_freqs *freqs = NULL;
> +	unsigned long load;
> +	int i = debugfs_transitions_index;
> +	int count;
> +
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"time_ms",
> +			"dev_name",
> +			"dev",
> +			"parent_dev",
> +			"load_%",
> +			"old_freq_hz",
> +			"new_freq_hz");
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"----------",
> +			"------------------------------",
> +			"----------",
> +			"----------",
> +			"----------",
> +			"------------",
> +			"------------");
> +
> +	spin_lock(&devfreq_debugfs_lock);> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
> +		devfreq = debugfs_transitions[i].devfreq;
> +		freqs = &debugfs_transitions[i].freqs;
> +		load = debugfs_transitions[i].load;
> +
> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
> +		if (!devfreq)
> +			continue;
> +
> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +		if (!strncmp(devfreq->governor_name,
> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
> +			struct devfreq_passive_data *data = devfreq->data;
> +
> +			if (data)
> +				p_devfreq = data->parent;
> +		} else {
> +			p_devfreq = NULL;
> +		}
> +#endif
> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
> +			freqs->time,
> +			dev_name(devfreq->dev.parent),
> +			dev_name(&devfreq->dev),
> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
> +			load,
> +			freqs->old,
> +			freqs->new);
> +	}
> +	spin_unlock(&devfreq_debugfs_lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
> +
>  static int __init devfreq_init(void)
>  {
>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>  		devfreq_debugfs = NULL;
>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>  	} else {
> +		spin_lock_init(&devfreq_debugfs_lock);
> +
>  		debugfs_create_file("devfreq_summary", 0444,
>  				devfreq_debugfs, NULL,
>  				&devfreq_summary_fops);
> +		debugfs_create_file("devfreq_transitions", 0444,
> +				devfreq_debugfs, NULL,
> +				&devfreq_transitions_fops);
>  	}
>  
>  	return 0;
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index dc7533ccc3db..01eecfdaf2d6 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>  
>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
> +extern void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time);

The 'extern' attribute isn't needed for function prototypes defined in
header files.

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Bjorn Andersson]

On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:

> Add new devfreq_transitions debugfs file to track the frequency transitions
> of all devfreq devices for the simple profiling as following:
> - /sys/kernel/debug/devfreq/devfreq_transitions
> 
> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
> in Kconfig in order to save the transition history.
> 
> [Detailed description of each field of 'devfreq_transitions' debugfs file]
> - time_ms	: Change time of frequency transition. (unit: millisecond)
> - dev_name	: Device name of h/w.
> - dev		: Device name made by devfreq core.
> - parent_dev	: If devfreq device uses the passive governor,
> 		  show parent devfreq device name.
> - load_%	: If devfreq device uses the simple_ondemand governor,
> 		  load is used by governor whene deciding the new frequency.
> 		  (unit: percentage)
> - old_freq_hz	: Frequency before changing. (unit: hz)
> - new_freq_hz	: Frequency after changed. (unit: hz)
> 
> [For example on Exynos5422-based Odroid-XU3 board]
> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> [snip]
> 

Wouldn't it make more sense to expose this through the tracing
framework - like many other subsystems does?

Regards,
Bjorn

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

07.01.2020 12:05, Chanwoo Choi пишет:
> Add new devfreq_transitions debugfs file to track the frequency transitions
> of all devfreq devices for the simple profiling as following:
> - /sys/kernel/debug/devfreq/devfreq_transitions
> 
> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
> in Kconfig in order to save the transition history.
> 
> [Detailed description of each field of 'devfreq_transitions' debugfs file]
> - time_ms	: Change time of frequency transition. (unit: millisecond)
> - dev_name	: Device name of h/w.
> - dev		: Device name made by devfreq core.
> - parent_dev	: If devfreq device uses the passive governor,
> 		  show parent devfreq device name.
> - load_%	: If devfreq device uses the simple_ondemand governor,
> 		  load is used by governor whene deciding the new frequency.
> 		  (unit: percentage)
> - old_freq_hz	: Frequency before changing. (unit: hz)
> - new_freq_hz	: Frequency after changed. (unit: hz)
> 
> [For example on Exynos5422-based Odroid-XU3 board]
> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> [snip]
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/Kconfig            |  13 +++
>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>  drivers/devfreq/governor.h         |   3 +
>  drivers/devfreq/governor_passive.c |   2 +
>  include/linux/devfreq.h            |   1 +
>  5 files changed, 145 insertions(+)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..84936eec0ef9 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>  	  through sysfs entries. The passive governor recommends that
>  	  devfreq device uses the OPP table to get the frequency/voltage.
>  
> +comment "DEVFREQ Debugging"
> +
> +config NR_DEVFREQ_TRANSITIONS
> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
> +	depends on DEBUG_FS
> +	range 10 1000
> +	default "100"
> +	help
> +	  Show the frequency transitions of all devfreq devices via
> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
> +	  profiling. It needs to decide the storage size to save transition
> +	  history of all devfreq devices.
> +
>  comment "DEVFREQ Drivers"
>  
>  config ARM_EXYNOS_BUS_DEVFREQ
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index c7f5e4e06420..7abaae06fa65 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  }
>  EXPORT_SYMBOL(devfreq_update_status);
>  
> +/**
> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
> + * @devfreq:	the devfreq instance
> + * @old_freq:	the previous frequency before changing the frequency
> + * @new_freq:	the new frequency after frequency is changed
> + */
> +struct devfreq_transitions {
> +	struct devfreq *devfreq;
> +	struct devfreq_freqs freqs;
> +	unsigned long load;
> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
> +
> +static spinlock_t devfreq_debugfs_lock;
> +static int debugfs_transitions_index;
> +
> +void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time)
> +{
> +	unsigned long load;
> +	int i;
> +
> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
> +		return;
> +
> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
> +
> +	i = debugfs_transitions_index;
> +
> +	/*
> +	 * Calculate the load and if load is larger than 100,
> +	 * initialize to 100 because the unit of load is percentage.
> +	 */
> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
> +	if (load > 100)
> +		load = 100;
> +
> +	debugfs_transitions[i].devfreq = devfreq;
> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
> +	debugfs_transitions[i].freqs.old = old_freq;
> +	debugfs_transitions[i].freqs.new = new_freq;
> +	debugfs_transitions[i].load = load;
> +
> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
> +		i = 0;
> +	debugfs_transitions_index = i;
> +
> +	spin_unlock(&devfreq_debugfs_lock);
> +}
> +EXPORT_SYMBOL(devfreq_update_transitions);
> +
>  /**
>   * find_devfreq_governor() - Find devfreq governor from name
>   * @name:	name of the governor
> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>  		return err;
>  	}
>  
> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
> +					devfreq->last_status.busy_time,
> +					devfreq->last_status.total_time);
> +
>  	freqs.new = new_freq;
>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>  
> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>  
> +/**
> + * devfreq_transitions_show() - Show the frequency transitions of the registered
> + *			devfreq devices via 'devfreq_transitions' debugfs file.
> + */
> +static int devfreq_transitions_show(struct seq_file *s, void *data)
> +{
> +	struct devfreq *devfreq = NULL;
> +	struct devfreq *p_devfreq = NULL;
> +	struct devfreq_freqs *freqs = NULL;
> +	unsigned long load;
> +	int i = debugfs_transitions_index;
> +	int count;
> +
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"time_ms",
> +			"dev_name",
> +			"dev",
> +			"parent_dev",
> +			"load_%",
> +			"old_freq_hz",
> +			"new_freq_hz");
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"----------",
> +			"------------------------------",
> +			"----------",
> +			"----------",
> +			"----------",
> +			"------------",
> +			"------------");

Isn't this needed here?

mutex_lock(&devfreq_list_lock);

> +	spin_lock(&devfreq_debugfs_lock);
> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
> +		devfreq = debugfs_transitions[i].devfreq;
> +		freqs = &debugfs_transitions[i].freqs;
> +		load = debugfs_transitions[i].load;
> +
> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
> +		if (!devfreq)
> +			continue;

I suppose debugfs_transitions[i].devfreq should be set to NULL when
devfreq device is removed, but I don't see it happening anywhere in this
patch.

> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +		if (!strncmp(devfreq->governor_name,
> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {

This could be:

if (IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) &&
    !strncmp(devfreq->governor_name,
		  DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {

> +			struct devfreq_passive_data *data = devfreq->data;
> +
> +			if (data)
> +				p_devfreq = data->parent;

const char *devname = "";

...

	if (data)
		devname = dev_name(data->parent);

> +		} else {
> +			p_devfreq = NULL;
> +		}
> +#endif
> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
> +			freqs->time,
> +			dev_name(devfreq->dev.parent),
> +			dev_name(&devfreq->dev),
> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
> +			load,
> +			freqs->old,
> +			freqs->new);
> +	}
> +	spin_unlock(&devfreq_debugfs_lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
> +
>  static int __init devfreq_init(void)
>  {
>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>  		devfreq_debugfs = NULL;
>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>  	} else {
> +		spin_lock_init(&devfreq_debugfs_lock);
> +
>  		debugfs_create_file("devfreq_summary", 0444,
>  				devfreq_debugfs, NULL,
>  				&devfreq_summary_fops);
> +		debugfs_create_file("devfreq_transitions", 0444,
> +				devfreq_debugfs, NULL,
> +				&devfreq_transitions_fops);
>  	}
>  
>  	return 0;
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index dc7533ccc3db..01eecfdaf2d6 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>  
>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
> +extern void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time);
>  
>  static inline int devfreq_update_stats(struct devfreq *df)
>  {
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index be6eeab9c814..05fa654239f5 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>  	if (ret < 0)
>  		goto out;
>  
> +	devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
> +
>  	if (devfreq->profile->freq_table
>  		&& (devfreq_update_status(devfreq, freq)))
>  		dev_err(&devfreq->dev,
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 49cdb2378030..933692e5d867 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -196,6 +196,7 @@ struct devfreq {
>  };
>  
>  struct devfreq_freqs {
> +	s64 time;
>  	unsigned long old;
>  	unsigned long new;
>  };
> 

Re: [PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Chanwoo Choi]

On 1/8/20 6:15 AM, Bjorn Andersson wrote:
> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
> 
>> Add debugfs interface to provide debugging information of devfreq device.
>> It contains 'devfreq_summary' entry to show the summary of registered
>> devfreq devices as following and the additional debugfs file will be added.
>> - /sys/kernel/debug/devfreq/devfreq_summary
>>
>> [Detailed description of each field of 'devfreq_summary' debugfs file]
>> - dev_name	: Device name of h/w.
>> - dev		: Device name made by devfreq core.
>> - parent_dev	: If devfreq device uses the passive governor,
>> 		  show parent devfreq device name.
>> - governor	: Devfreq governor.
>> - polling_ms	: If devfreq device uses the simple_ondemand governor,
>> 		  polling_ms is necessary for the period. (unit: millisecond)
>> - cur_freq_hz	: Current Frequency (unit: hz)
>> - old_freq_hz	: Frequency before changing. (unit: hz)
>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> - In order to show the multiple governors on devfreq_summay result,
>> change the governor of devfreq0 from simple_ondemand to userspace.
>>
>> $ cat /sys/kernel/debug/devfreq/devfreq_summary
>> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
>> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
>> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
>> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
>> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
>> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
>> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
>> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
>> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
>> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
>> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
>> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
>> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
>> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
>> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
>> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
>> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
> 
> This looks quite useful.
> 
>>
>> Reported-by: kbuild test robot <lkp@intel.com>
> 
> May I ask how the build test robot came up with this idea?

I'm missing the detailed what are the reported by kbuild test robot.
It reported the possible build error. I'll add the following description
on next version:
	[lkp: Reported the build error]

> 
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 80 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> [..]
>> +/**
>> + * devfreq_summary_show() - Show the summary of the registered devfreq devices
>> + *				via 'devfreq_summary' debugfs file.
> 
> Please make this proper kerneldoc, i.e:
> 
>  * func() - short description
>  * @s:
>  * @data:
>  * 
>  * Long description
>  * 
>  * Return: foo on bar

OK.

> 
> [..]
>> @@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
>>  	}
>>  	devfreq_class->dev_groups = devfreq_groups;
>>  
>> +	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
>> +	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {
> 
> Don't PTR_ERR() before IS_ERR().

Before checking IS_ERR(), have to check the PTR_ERR(devfreq_debugfs)
is -ENODEV or not because debugfs_create_dir return the '-ENODEV'
if DEBUG_FS is disabled. If return the -ENODEV, it is not error.

> 
>> +		devfreq_debugfs = NULL;
> 
> I don't think you need this, given that debugfs_create_file() will fail
> gracefully when passed and IS_ERR()

Right. Jut on patch2 checks the 'devfreq_debugfs' is NULL or not
in order to catch the DEBUG_FS is well working for devfreq.
Anyway, it would be better to add it to patch2 because devfreq_debugfs
is not used when failed to create debugfs dir.

> 
>> +		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
> 
> Afaict debugfs_create_() won't fail without printing a message already.

It just print the error message when DEBUG_FS is enabled
and debugfs_create_dir() returns the error.

> 
>> +	} else {
>> +		debugfs_create_file("devfreq_summary", 0444,
>> +				devfreq_debugfs, NULL,
>> +				&devfreq_summary_fops);
>> +	}
>> +
>>  	return 0;
>>  }
>>  subsys_initcall(devfreq_init);
> 
> Regards,
> Bjorn
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

Hi,

On 1/8/20 6:31 AM, Dmitry Osipenko wrote:
> Hello Chanwoo,
> 
> 07.01.2020 12:05, Chanwoo Choi пишет:
>> Add new devfreq_transitions debugfs file to track the frequency transitions
>> of all devfreq devices for the simple profiling as following:
>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>
>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>> in Kconfig in order to save the transition history.
>>
>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>> - dev_name	: Device name of h/w.
>> - dev		: Device name made by devfreq core.
>> - parent_dev	: If devfreq device uses the passive governor,
>> 		  show parent devfreq device name.
>> - load_%	: If devfreq device uses the simple_ondemand governor,
>> 		  load is used by governor whene deciding the new frequency.
>> 		  (unit: percentage)
>> - old_freq_hz	: Frequency before changing. (unit: hz)
>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>> [snip]
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/Kconfig            |  13 +++
>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>  drivers/devfreq/governor.h         |   3 +
>>  drivers/devfreq/governor_passive.c |   2 +
>>  include/linux/devfreq.h            |   1 +
>>  5 files changed, 145 insertions(+)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 0b1df12e0f21..84936eec0ef9 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>  	  through sysfs entries. The passive governor recommends that
>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>  
>> +comment "DEVFREQ Debugging"
>> +
>> +config NR_DEVFREQ_TRANSITIONS
>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>> +	depends on DEBUG_FS
>> +	range 10 1000
>> +	default "100"
>> +	help
>> +	  Show the frequency transitions of all devfreq devices via
>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>> +	  profiling. It needs to decide the storage size to save transition
>> +	  history of all devfreq devices.
>> +
>>  comment "DEVFREQ Drivers"
>>  
>>  config ARM_EXYNOS_BUS_DEVFREQ
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index c7f5e4e06420..7abaae06fa65 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>  }
>>  EXPORT_SYMBOL(devfreq_update_status);
>> +/**
>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>> + * @devfreq:	the devfreq instance
>> + * @old_freq:	the previous frequency before changing the frequency
>> + * @new_freq:	the new frequency after frequency is changed
>> + */
>> +struct devfreq_transitions {
>> +	struct devfreq *devfreq;
>> +	struct devfreq_freqs freqs;
>> +	unsigned long load;
>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>> +
>> +static spinlock_t devfreq_debugfs_lock;
> 
> This could be:
> 
> static DEFINE_SPINLOCK(devfreq_debugfs_lock);
> 
> and then spin_lock_init() isn't needed.

OK

> 
> 
> Also, The "<linux/spinlock.h>" should be included directly by devfreq.c

OK.

> 
>> +static int debugfs_transitions_index;
>> +
>> +void devfreq_update_transitions(struct devfreq *devfreq,
>> +			unsigned long old_freq, unsigned long new_freq,
>> +			unsigned long busy_time, unsigned long total_time)
>> +{
>> +	unsigned long load;
>> +	int i;
>> +
>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>> +		return;
>> +
>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>> +
>> +	i = debugfs_transitions_index;
>> +
>> +	/*
>> +	 * Calculate the load and if load is larger than 100,
>> +	 * initialize to 100 because the unit of load is percentage.
>> +	 */
>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>> +	if (load > 100)
>> +		load = 100;
>> +
>> +	debugfs_transitions[i].devfreq = devfreq;
>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>> +	debugfs_transitions[i].freqs.old = old_freq;
>> +	debugfs_transitions[i].freqs.new = new_freq;
>> +	debugfs_transitions[i].load = load;
>> +
>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>> +		i = 0;
>> +	debugfs_transitions_index = i;
>> +
>> +	spin_unlock(&devfreq_debugfs_lock);
>> +}
>> +EXPORT_SYMBOL(devfreq_update_transitions);
> 
> What about EXPORT_SYMBOL_GPL()?

I'll remove it.

> 
>>  /**
>>   * find_devfreq_governor() - Find devfreq governor from name
>>   * @name:	name of the governor
>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>  		return err;
>>  	}
>>  
>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>> +					devfreq->last_status.busy_time,
>> +					devfreq->last_status.total_time);
>> +
>>  	freqs.new = new_freq;
>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>  
>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>  }
>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>  
>> +/**
>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>> + */
>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>> +{
>> +	struct devfreq *devfreq = NULL;
>> +	struct devfreq *p_devfreq = NULL;
>> +	struct devfreq_freqs *freqs = NULL;
>> +	unsigned long load;
>> +	int i = debugfs_transitions_index;
>> +	int count;
>> +
>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> +			"time_ms",
>> +			"dev_name",
>> +			"dev",
>> +			"parent_dev",
>> +			"load_%",
>> +			"old_freq_hz",
>> +			"new_freq_hz");
>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> +			"----------",
>> +			"------------------------------",
>> +			"----------",
>> +			"----------",
>> +			"----------",
>> +			"------------",
>> +			"------------");
>> +
>> +	spin_lock(&devfreq_debugfs_lock);> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>> +		devfreq = debugfs_transitions[i].devfreq;
>> +		freqs = &debugfs_transitions[i].freqs;
>> +		load = debugfs_transitions[i].load;
>> +
>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>> +		if (!devfreq)
>> +			continue;
>> +
>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> +		if (!strncmp(devfreq->governor_name,
>> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
>> +			struct devfreq_passive_data *data = devfreq->data;
>> +
>> +			if (data)
>> +				p_devfreq = data->parent;
>> +		} else {
>> +			p_devfreq = NULL;
>> +		}
>> +#endif
>> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
>> +			freqs->time,
>> +			dev_name(devfreq->dev.parent),
>> +			dev_name(&devfreq->dev),
>> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
>> +			load,
>> +			freqs->old,
>> +			freqs->new);
>> +	}
>> +	spin_unlock(&devfreq_debugfs_lock);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
>> +
>>  static int __init devfreq_init(void)
>>  {
>>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
>> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>>  		devfreq_debugfs = NULL;
>>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>>  	} else {
>> +		spin_lock_init(&devfreq_debugfs_lock);
>> +
>>  		debugfs_create_file("devfreq_summary", 0444,
>>  				devfreq_debugfs, NULL,
>>  				&devfreq_summary_fops);
>> +		debugfs_create_file("devfreq_transitions", 0444,
>> +				devfreq_debugfs, NULL,
>> +				&devfreq_transitions_fops);
>>  	}
>>  
>>  	return 0;
>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>> index dc7533ccc3db..01eecfdaf2d6 100644
>> --- a/drivers/devfreq/governor.h
>> +++ b/drivers/devfreq/governor.h
>> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>>  
>>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>> +extern void devfreq_update_transitions(struct devfreq *devfreq,
>> +			unsigned long old_freq, unsigned long new_freq,
>> +			unsigned long busy_time, unsigned long total_time);
> 
> The 'extern' attribute isn't needed for function prototypes defined in
> header files.
> 
> 

Right. I'll remove it.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/8/20 6:56 AM, Dmitry Osipenko wrote:
> 07.01.2020 12:05, Chanwoo Choi пишет:
>> Add new devfreq_transitions debugfs file to track the frequency transitions
>> of all devfreq devices for the simple profiling as following:
>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>
>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>> in Kconfig in order to save the transition history.
>>
>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>> - dev_name	: Device name of h/w.
>> - dev		: Device name made by devfreq core.
>> - parent_dev	: If devfreq device uses the passive governor,
>> 		  show parent devfreq device name.
>> - load_%	: If devfreq device uses the simple_ondemand governor,
>> 		  load is used by governor whene deciding the new frequency.
>> 		  (unit: percentage)
>> - old_freq_hz	: Frequency before changing. (unit: hz)
>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>> [snip]
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/Kconfig            |  13 +++
>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>  drivers/devfreq/governor.h         |   3 +
>>  drivers/devfreq/governor_passive.c |   2 +
>>  include/linux/devfreq.h            |   1 +
>>  5 files changed, 145 insertions(+)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 0b1df12e0f21..84936eec0ef9 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>  	  through sysfs entries. The passive governor recommends that
>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>  
>> +comment "DEVFREQ Debugging"
>> +
>> +config NR_DEVFREQ_TRANSITIONS
>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>> +	depends on DEBUG_FS
>> +	range 10 1000
>> +	default "100"
>> +	help
>> +	  Show the frequency transitions of all devfreq devices via
>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>> +	  profiling. It needs to decide the storage size to save transition
>> +	  history of all devfreq devices.
>> +
>>  comment "DEVFREQ Drivers"
>>  
>>  config ARM_EXYNOS_BUS_DEVFREQ
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index c7f5e4e06420..7abaae06fa65 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>  }
>>  EXPORT_SYMBOL(devfreq_update_status);
>>  
>> +/**
>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>> + * @devfreq:	the devfreq instance
>> + * @old_freq:	the previous frequency before changing the frequency
>> + * @new_freq:	the new frequency after frequency is changed
>> + */
>> +struct devfreq_transitions {
>> +	struct devfreq *devfreq;
>> +	struct devfreq_freqs freqs;
>> +	unsigned long load;
>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>> +
>> +static spinlock_t devfreq_debugfs_lock;
>> +static int debugfs_transitions_index;
>> +
>> +void devfreq_update_transitions(struct devfreq *devfreq,
>> +			unsigned long old_freq, unsigned long new_freq,
>> +			unsigned long busy_time, unsigned long total_time)
>> +{
>> +	unsigned long load;
>> +	int i;
>> +
>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>> +		return;
>> +
>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>> +
>> +	i = debugfs_transitions_index;
>> +
>> +	/*
>> +	 * Calculate the load and if load is larger than 100,
>> +	 * initialize to 100 because the unit of load is percentage.
>> +	 */
>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>> +	if (load > 100)
>> +		load = 100;
>> +
>> +	debugfs_transitions[i].devfreq = devfreq;
>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>> +	debugfs_transitions[i].freqs.old = old_freq;
>> +	debugfs_transitions[i].freqs.new = new_freq;
>> +	debugfs_transitions[i].load = load;
>> +
>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>> +		i = 0;
>> +	debugfs_transitions_index = i;
>> +
>> +	spin_unlock(&devfreq_debugfs_lock);
>> +}
>> +EXPORT_SYMBOL(devfreq_update_transitions);
>> +
>>  /**
>>   * find_devfreq_governor() - Find devfreq governor from name
>>   * @name:	name of the governor
>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>  		return err;
>>  	}
>>  
>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>> +					devfreq->last_status.busy_time,
>> +					devfreq->last_status.total_time);
>> +
>>  	freqs.new = new_freq;
>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>  
>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>  }
>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>  
>> +/**
>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>> + */
>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>> +{
>> +	struct devfreq *devfreq = NULL;
>> +	struct devfreq *p_devfreq = NULL;
>> +	struct devfreq_freqs *freqs = NULL;
>> +	unsigned long load;
>> +	int i = debugfs_transitions_index;
>> +	int count;
>> +
>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> +			"time_ms",
>> +			"dev_name",
>> +			"dev",
>> +			"parent_dev",
>> +			"load_%",
>> +			"old_freq_hz",
>> +			"new_freq_hz");
>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> +			"----------",
>> +			"------------------------------",
>> +			"----------",
>> +			"----------",
>> +			"----------",
>> +			"------------",
>> +			"------------");
> 
> Isn't this needed here?
> 
> mutex_lock(&devfreq_list_lock);

It doesn't touch the devfreq instance of devfreq_list.
So, it is not necessary locked of devfreq_list_lock.

> 
>> +	spin_lock(&devfreq_debugfs_lock);
>> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>> +		devfreq = debugfs_transitions[i].devfreq;
>> +		freqs = &debugfs_transitions[i].freqs;
>> +		load = debugfs_transitions[i].load;
>> +
>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>> +		if (!devfreq)
>> +			continue;
> 
> I suppose debugfs_transitions[i].devfreq should be set to NULL when
> devfreq device is removed, but I don't see it happening anywhere in this
> patch.

When debugfs_transitions[] array is not fully filled out
by devfreq_update_transitions(), debugfs_transitions[i].devfreq is NULL.
In this case, if user execute 'cat /sys/kernel/debug/devfreq/devfreq_transitions',
devfreq_transitions_show() need to check the debugfs_transitions[i].devfreq
is NULL or not.

After filled out the debugfs_transitions[] array,
actually, 'if(!devfreq)' is not necessary. Maybe, this style is inefficient
It need to rework. I'll think again.

> 
>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> +		if (!strncmp(devfreq->governor_name,
>> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
> 
> This could be:
> 
> if (IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) &&
>     !strncmp(devfreq->governor_name,
> 		  DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {

Good. It is more clear. I'll edit it according to your comment.

> 
>> +			struct devfreq_passive_data *data = devfreq->data;
>> +
>> +			if (data)
>> +				p_devfreq = data->parent;
> 
> const char *devname = "";
> 
> ...
> 
> 	if (data)
> 		devname = dev_name(data->parent);
> 

'devname' word is too general. It is difficult to know
this name is for parent devfreq device. So, I prefer
to keep the origin style.

>> +		} else {
>> +			p_devfreq = NULL;
>> +		}
>> +#endif
>> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
>> +			freqs->time,
>> +			dev_name(devfreq->dev.parent),
>> +			dev_name(&devfreq->dev),
>> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
>> +			load,
>> +			freqs->old,
>> +			freqs->new);
>> +	}
>> +	spin_unlock(&devfreq_debugfs_lock);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
>> +
>>  static int __init devfreq_init(void)
>>  {
>>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
>> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>>  		devfreq_debugfs = NULL;
>>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>>  	} else {
>> +		spin_lock_init(&devfreq_debugfs_lock);
>> +
>>  		debugfs_create_file("devfreq_summary", 0444,
>>  				devfreq_debugfs, NULL,
>>  				&devfreq_summary_fops);
>> +		debugfs_create_file("devfreq_transitions", 0444,
>> +				devfreq_debugfs, NULL,
>> +				&devfreq_transitions_fops);
>>  	}
>>  
>>  	return 0;
>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>> index dc7533ccc3db..01eecfdaf2d6 100644
>> --- a/drivers/devfreq/governor.h
>> +++ b/drivers/devfreq/governor.h
>> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>>  
>>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>> +extern void devfreq_update_transitions(struct devfreq *devfreq,
>> +			unsigned long old_freq, unsigned long new_freq,
>> +			unsigned long busy_time, unsigned long total_time);
>>  
>>  static inline int devfreq_update_stats(struct devfreq *df)
>>  {
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index be6eeab9c814..05fa654239f5 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>>  	if (ret < 0)
>>  		goto out;
>>  
>> +	devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
>> +
>>  	if (devfreq->profile->freq_table
>>  		&& (devfreq_update_status(devfreq, freq)))
>>  		dev_err(&devfreq->dev,
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 49cdb2378030..933692e5d867 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -196,6 +196,7 @@ struct devfreq {
>>  };
>>  
>>  struct devfreq_freqs {
>> +	s64 time;
>>  	unsigned long old;
>>  	unsigned long new;
>>  };
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/8/20 8:22 PM, Chanwoo Choi wrote:
> On 1/8/20 6:56 AM, Dmitry Osipenko wrote:
>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>> of all devfreq devices for the simple profiling as following:
>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>
>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> in Kconfig in order to save the transition history.
>>>
>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>> 		  load is used by governor whene deciding the new frequency.
>>> 		  (unit: percentage)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>> [snip]
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/Kconfig            |  13 +++
>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>  drivers/devfreq/governor.h         |   3 +
>>>  drivers/devfreq/governor_passive.c |   2 +
>>>  include/linux/devfreq.h            |   1 +
>>>  5 files changed, 145 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 0b1df12e0f21..84936eec0ef9 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>  	  through sysfs entries. The passive governor recommends that
>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>  
>>> +comment "DEVFREQ Debugging"
>>> +
>>> +config NR_DEVFREQ_TRANSITIONS
>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>> +	depends on DEBUG_FS
>>> +	range 10 1000
>>> +	default "100"
>>> +	help
>>> +	  Show the frequency transitions of all devfreq devices via
>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>> +	  profiling. It needs to decide the storage size to save transition
>>> +	  history of all devfreq devices.
>>> +
>>>  comment "DEVFREQ Drivers"
>>>  
>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index c7f5e4e06420..7abaae06fa65 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>  }
>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>  
>>> +/**
>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>> + * @devfreq:	the devfreq instance
>>> + * @old_freq:	the previous frequency before changing the frequency
>>> + * @new_freq:	the new frequency after frequency is changed
>>> + */
>>> +struct devfreq_transitions {
>>> +	struct devfreq *devfreq;
>>> +	struct devfreq_freqs freqs;
>>> +	unsigned long load;
>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>> +
>>> +static spinlock_t devfreq_debugfs_lock;
>>> +static int debugfs_transitions_index;
>>> +
>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>> +			unsigned long old_freq, unsigned long new_freq,
>>> +			unsigned long busy_time, unsigned long total_time)
>>> +{
>>> +	unsigned long load;
>>> +	int i;
>>> +
>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>> +		return;
>>> +
>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>> +
>>> +	i = debugfs_transitions_index;
>>> +
>>> +	/*
>>> +	 * Calculate the load and if load is larger than 100,
>>> +	 * initialize to 100 because the unit of load is percentage.
>>> +	 */
>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>> +	if (load > 100)
>>> +		load = 100;
>>> +
>>> +	debugfs_transitions[i].devfreq = devfreq;
>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>> +	debugfs_transitions[i].load = load;
>>> +
>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> +		i = 0;
>>> +	debugfs_transitions_index = i;
>>> +
>>> +	spin_unlock(&devfreq_debugfs_lock);
>>> +}
>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>> +
>>>  /**
>>>   * find_devfreq_governor() - Find devfreq governor from name
>>>   * @name:	name of the governor
>>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>>  		return err;
>>>  	}
>>>  
>>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>>> +					devfreq->last_status.busy_time,
>>> +					devfreq->last_status.total_time);
>>> +
>>>  	freqs.new = new_freq;
>>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>  
>>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>>  }
>>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>>  
>>> +/**
>>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>>> + */
>>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>>> +{
>>> +	struct devfreq *devfreq = NULL;
>>> +	struct devfreq *p_devfreq = NULL;
>>> +	struct devfreq_freqs *freqs = NULL;
>>> +	unsigned long load;
>>> +	int i = debugfs_transitions_index;
>>> +	int count;
>>> +
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"time_ms",
>>> +			"dev_name",
>>> +			"dev",
>>> +			"parent_dev",
>>> +			"load_%",
>>> +			"old_freq_hz",
>>> +			"new_freq_hz");
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"----------",
>>> +			"------------------------------",
>>> +			"----------",
>>> +			"----------",
>>> +			"----------",
>>> +			"------------",
>>> +			"------------");
>>
>> Isn't this needed here?
>>
>> mutex_lock(&devfreq_list_lock);
> 
> It doesn't touch the devfreq instance of devfreq_list.
> So, it is not necessary locked of devfreq_list_lock.
> 
>>
>>> +	spin_lock(&devfreq_debugfs_lock);
>>> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>>> +		devfreq = debugfs_transitions[i].devfreq;
>>> +		freqs = &debugfs_transitions[i].freqs;
>>> +		load = debugfs_transitions[i].load;
>>> +
>>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>>> +		if (!devfreq)
>>> +			continue;
>>
>> I suppose debugfs_transitions[i].devfreq should be set to NULL when
>> devfreq device is removed, but I don't see it happening anywhere in this
>> patch.
> 
> When debugfs_transitions[] array is not fully filled out
> by devfreq_update_transitions(), debugfs_transitions[i].devfreq is NULL.
> In this case, if user execute 'cat /sys/kernel/debug/devfreq/devfreq_transitions',
> devfreq_transitions_show() need to check the debugfs_transitions[i].devfreq
> is NULL or not.
> 
> After filled out the debugfs_transitions[] array,
> actually, 'if(!devfreq)' is not necessary. Maybe, this style is inefficient
> It need to rework. I'll think again.
> 
>>
>>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>>> +		if (!strncmp(devfreq->governor_name,
>>> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
>>
>> This could be:
>>
>> if (IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) &&
>>     !strncmp(devfreq->governor_name,
>> 		  DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
> 
> Good. It is more clear. I'll edit it according to your comment.

It is my mistake. 'struct devfreq_passive_data' is defined
when CONFIG_DEVFREQ_GOV_PASSIVE is enabled.

Firstly, I should make 'struct devfreq_passive_data' possible used
always regardless of CONFIG_DEVFREQ_GOV_PASSIVE is enabled or not
as following:

diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 36235327bf50..4e9241d0b569 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -277,7 +277,6 @@ struct devfreq_simple_ondemand_data {
 };
 #endif
 
-#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
 /**
  * struct devfreq_passive_data - void *data fed to struct devfreq
  *     and devfreq_add_device
@@ -308,7 +307,6 @@ struct devfreq_passive_data {
        struct devfreq *this;
        struct notifier_block nb;
 };
-#endif
 



> 
>>
>>> +			struct devfreq_passive_data *data = devfreq->data;
>>> +
>>> +			if (data)
>>> +				p_devfreq = data->parent;
>>
>> const char *devname = "";
>>
>> ...
>>
>> 	if (data)
>> 		devname = dev_name(data->parent);
>>
> 
> 'devname' word is too general. It is difficult to know
> this name is for parent devfreq device. So, I prefer
> to keep the origin style.
> 
>>> +		} else {
>>> +			p_devfreq = NULL;
>>> +		}
>>> +#endif
>>> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
>>> +			freqs->time,
>>> +			dev_name(devfreq->dev.parent),
>>> +			dev_name(&devfreq->dev),
>>> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
>>> +			load,
>>> +			freqs->old,
>>> +			freqs->new);
>>> +	}
>>> +	spin_unlock(&devfreq_debugfs_lock);
>>> +
>>> +	return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
>>> +
>>>  static int __init devfreq_init(void)
>>>  {
>>>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
>>> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>>>  		devfreq_debugfs = NULL;
>>>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>>>  	} else {
>>> +		spin_lock_init(&devfreq_debugfs_lock);
>>> +
>>>  		debugfs_create_file("devfreq_summary", 0444,
>>>  				devfreq_debugfs, NULL,
>>>  				&devfreq_summary_fops);
>>> +		debugfs_create_file("devfreq_transitions", 0444,
>>> +				devfreq_debugfs, NULL,
>>> +				&devfreq_transitions_fops);
>>>  	}
>>>  
>>>  	return 0;
>>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>>> index dc7533ccc3db..01eecfdaf2d6 100644
>>> --- a/drivers/devfreq/governor.h
>>> +++ b/drivers/devfreq/governor.h
>>> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>>>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>>>  
>>>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>>> +extern void devfreq_update_transitions(struct devfreq *devfreq,
>>> +			unsigned long old_freq, unsigned long new_freq,
>>> +			unsigned long busy_time, unsigned long total_time);
>>>  
>>>  static inline int devfreq_update_stats(struct devfreq *df)
>>>  {
>>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>>> index be6eeab9c814..05fa654239f5 100644
>>> --- a/drivers/devfreq/governor_passive.c
>>> +++ b/drivers/devfreq/governor_passive.c
>>> @@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>>>  	if (ret < 0)
>>>  		goto out;
>>>  
>>> +	devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
>>> +
>>>  	if (devfreq->profile->freq_table
>>>  		&& (devfreq_update_status(devfreq, freq)))
>>>  		dev_err(&devfreq->dev,
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index 49cdb2378030..933692e5d867 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -196,6 +196,7 @@ struct devfreq {
>>>  };
>>>  
>>>  struct devfreq_freqs {
>>> +	s64 time;
>>>  	unsigned long old;
>>>  	unsigned long new;
>>>  };
>>>
>>
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/8/20 7:56 PM, Chanwoo Choi wrote:
> Hi,
> 
> On 1/8/20 6:31 AM, Dmitry Osipenko wrote:
>> Hello Chanwoo,
>>
>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>> of all devfreq devices for the simple profiling as following:
>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>
>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> in Kconfig in order to save the transition history.
>>>
>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>> 		  load is used by governor whene deciding the new frequency.
>>> 		  (unit: percentage)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>> [snip]
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/Kconfig            |  13 +++
>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>  drivers/devfreq/governor.h         |   3 +
>>>  drivers/devfreq/governor_passive.c |   2 +
>>>  include/linux/devfreq.h            |   1 +
>>>  5 files changed, 145 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 0b1df12e0f21..84936eec0ef9 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>  	  through sysfs entries. The passive governor recommends that
>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>  
>>> +comment "DEVFREQ Debugging"
>>> +
>>> +config NR_DEVFREQ_TRANSITIONS
>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>> +	depends on DEBUG_FS
>>> +	range 10 1000
>>> +	default "100"
>>> +	help
>>> +	  Show the frequency transitions of all devfreq devices via
>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>> +	  profiling. It needs to decide the storage size to save transition
>>> +	  history of all devfreq devices.
>>> +
>>>  comment "DEVFREQ Drivers"
>>>  
>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index c7f5e4e06420..7abaae06fa65 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>  }
>>>  EXPORT_SYMBOL(devfreq_update_status);
>>> +/**
>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>> + * @devfreq:	the devfreq instance
>>> + * @old_freq:	the previous frequency before changing the frequency
>>> + * @new_freq:	the new frequency after frequency is changed
>>> + */
>>> +struct devfreq_transitions {
>>> +	struct devfreq *devfreq;
>>> +	struct devfreq_freqs freqs;
>>> +	unsigned long load;
>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>> +
>>> +static spinlock_t devfreq_debugfs_lock;
>>
>> This could be:
>>
>> static DEFINE_SPINLOCK(devfreq_debugfs_lock);
>>
>> and then spin_lock_init() isn't needed.
> 
> OK
> 
>>
>>
>> Also, The "<linux/spinlock.h>" should be included directly by devfreq.c
> 
> OK.
> 
>>
>>> +static int debugfs_transitions_index;
>>> +
>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>> +			unsigned long old_freq, unsigned long new_freq,
>>> +			unsigned long busy_time, unsigned long total_time)
>>> +{
>>> +	unsigned long load;
>>> +	int i;
>>> +
>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>> +		return;
>>> +
>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>> +
>>> +	i = debugfs_transitions_index;
>>> +
>>> +	/*
>>> +	 * Calculate the load and if load is larger than 100,
>>> +	 * initialize to 100 because the unit of load is percentage.
>>> +	 */
>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>> +	if (load > 100)
>>> +		load = 100;
>>> +
>>> +	debugfs_transitions[i].devfreq = devfreq;
>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>> +	debugfs_transitions[i].load = load;
>>> +
>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> +		i = 0;
>>> +	debugfs_transitions_index = i;
>>> +
>>> +	spin_unlock(&devfreq_debugfs_lock);
>>> +}
>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>
>> What about EXPORT_SYMBOL_GPL()?
> 
> I'll remove it.

Ah. It is needed to support module build.
it is used by passive governor.

> 
>>
>>>  /**
>>>   * find_devfreq_governor() - Find devfreq governor from name
>>>   * @name:	name of the governor
>>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>>  		return err;
>>>  	}
>>>  
>>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>>> +					devfreq->last_status.busy_time,
>>> +					devfreq->last_status.total_time);
>>> +
>>>  	freqs.new = new_freq;
>>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>  
>>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>>  }
>>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>>  
>>> +/**
>>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>>> + */
>>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>>> +{
>>> +	struct devfreq *devfreq = NULL;
>>> +	struct devfreq *p_devfreq = NULL;
>>> +	struct devfreq_freqs *freqs = NULL;
>>> +	unsigned long load;
>>> +	int i = debugfs_transitions_index;
>>> +	int count;
>>> +
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"time_ms",
>>> +			"dev_name",
>>> +			"dev",
>>> +			"parent_dev",
>>> +			"load_%",
>>> +			"old_freq_hz",
>>> +			"new_freq_hz");
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"----------",
>>> +			"------------------------------",
>>> +			"----------",
>>> +			"----------",
>>> +			"----------",
>>> +			"------------",
>>> +			"------------");
>>> +
>>> +	spin_lock(&devfreq_debugfs_lock);> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>>> +		devfreq = debugfs_transitions[i].devfreq;
>>> +		freqs = &debugfs_transitions[i].freqs;
>>> +		load = debugfs_transitions[i].load;
>>> +
>>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>>> +		if (!devfreq)
>>> +			continue;
>>> +
>>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>>> +		if (!strncmp(devfreq->governor_name,
>>> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
>>> +			struct devfreq_passive_data *data = devfreq->data;
>>> +
>>> +			if (data)
>>> +				p_devfreq = data->parent;
>>> +		} else {
>>> +			p_devfreq = NULL;
>>> +		}
>>> +#endif
>>> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
>>> +			freqs->time,
>>> +			dev_name(devfreq->dev.parent),
>>> +			dev_name(&devfreq->dev),
>>> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
>>> +			load,
>>> +			freqs->old,
>>> +			freqs->new);
>>> +	}
>>> +	spin_unlock(&devfreq_debugfs_lock);
>>> +
>>> +	return 0;
>>> +}
>>> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
>>> +
>>>  static int __init devfreq_init(void)
>>>  {
>>>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
>>> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>>>  		devfreq_debugfs = NULL;
>>>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>>>  	} else {
>>> +		spin_lock_init(&devfreq_debugfs_lock);
>>> +
>>>  		debugfs_create_file("devfreq_summary", 0444,
>>>  				devfreq_debugfs, NULL,
>>>  				&devfreq_summary_fops);
>>> +		debugfs_create_file("devfreq_transitions", 0444,
>>> +				devfreq_debugfs, NULL,
>>> +				&devfreq_transitions_fops);
>>>  	}
>>>  
>>>  	return 0;
>>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>>> index dc7533ccc3db..01eecfdaf2d6 100644
>>> --- a/drivers/devfreq/governor.h
>>> +++ b/drivers/devfreq/governor.h
>>> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>>>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>>>  
>>>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>>> +extern void devfreq_update_transitions(struct devfreq *devfreq,
>>> +			unsigned long old_freq, unsigned long new_freq,
>>> +			unsigned long busy_time, unsigned long total_time);
>>
>> The 'extern' attribute isn't needed for function prototypes defined in
>> header files.
>>
>>
> 
> Right. I'll remove it.
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

08.01.2020 14:22, Chanwoo Choi пишет:
> On 1/8/20 6:56 AM, Dmitry Osipenko wrote:
>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>> of all devfreq devices for the simple profiling as following:
>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>
>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> in Kconfig in order to save the transition history.
>>>
>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>> 		  load is used by governor whene deciding the new frequency.
>>> 		  (unit: percentage)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>> [snip]
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/Kconfig            |  13 +++
>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>  drivers/devfreq/governor.h         |   3 +
>>>  drivers/devfreq/governor_passive.c |   2 +
>>>  include/linux/devfreq.h            |   1 +
>>>  5 files changed, 145 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 0b1df12e0f21..84936eec0ef9 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>  	  through sysfs entries. The passive governor recommends that
>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>  
>>> +comment "DEVFREQ Debugging"
>>> +
>>> +config NR_DEVFREQ_TRANSITIONS
>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>> +	depends on DEBUG_FS
>>> +	range 10 1000
>>> +	default "100"
>>> +	help
>>> +	  Show the frequency transitions of all devfreq devices via
>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>> +	  profiling. It needs to decide the storage size to save transition
>>> +	  history of all devfreq devices.
>>> +
>>>  comment "DEVFREQ Drivers"
>>>  
>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index c7f5e4e06420..7abaae06fa65 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>  }
>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>  
>>> +/**
>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>> + * @devfreq:	the devfreq instance
>>> + * @old_freq:	the previous frequency before changing the frequency
>>> + * @new_freq:	the new frequency after frequency is changed
>>> + */
>>> +struct devfreq_transitions {
>>> +	struct devfreq *devfreq;
>>> +	struct devfreq_freqs freqs;
>>> +	unsigned long load;
>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>> +
>>> +static spinlock_t devfreq_debugfs_lock;
>>> +static int debugfs_transitions_index;
>>> +
>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>> +			unsigned long old_freq, unsigned long new_freq,
>>> +			unsigned long busy_time, unsigned long total_time)
>>> +{
>>> +	unsigned long load;
>>> +	int i;
>>> +
>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>> +		return;
>>> +
>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>> +
>>> +	i = debugfs_transitions_index;
>>> +
>>> +	/*
>>> +	 * Calculate the load and if load is larger than 100,
>>> +	 * initialize to 100 because the unit of load is percentage.
>>> +	 */
>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>> +	if (load > 100)
>>> +		load = 100;
>>> +
>>> +	debugfs_transitions[i].devfreq = devfreq;
>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>> +	debugfs_transitions[i].load = load;
>>> +
>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> +		i = 0;
>>> +	debugfs_transitions_index = i;
>>> +
>>> +	spin_unlock(&devfreq_debugfs_lock);
>>> +}
>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>> +
>>>  /**
>>>   * find_devfreq_governor() - Find devfreq governor from name
>>>   * @name:	name of the governor
>>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>>  		return err;
>>>  	}
>>>  
>>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>>> +					devfreq->last_status.busy_time,
>>> +					devfreq->last_status.total_time);
>>> +
>>>  	freqs.new = new_freq;
>>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>  
>>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>>  }
>>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>>  
>>> +/**
>>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>>> + */
>>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>>> +{
>>> +	struct devfreq *devfreq = NULL;
>>> +	struct devfreq *p_devfreq = NULL;
>>> +	struct devfreq_freqs *freqs = NULL;
>>> +	unsigned long load;
>>> +	int i = debugfs_transitions_index;
>>> +	int count;
>>> +
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"time_ms",
>>> +			"dev_name",
>>> +			"dev",
>>> +			"parent_dev",
>>> +			"load_%",
>>> +			"old_freq_hz",
>>> +			"new_freq_hz");
>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>> +			"----------",
>>> +			"------------------------------",
>>> +			"----------",
>>> +			"----------",
>>> +			"----------",
>>> +			"------------",
>>> +			"------------");
>>
>> Isn't this needed here?
>>
>> mutex_lock(&devfreq_list_lock);
> 
> It doesn't touch the devfreq instance of devfreq_list.
> So, it is not necessary locked of devfreq_list_lock.

What stops devfreq device to be removed by another CPU thread while this
function is in a process of execution?

This condition is unlikely to happen in practice ever, but technically
it should be possible to happen.

>>> +	spin_lock(&devfreq_debugfs_lock);
>>> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>>> +		devfreq = debugfs_transitions[i].devfreq;
>>> +		freqs = &debugfs_transitions[i].freqs;
>>> +		load = debugfs_transitions[i].load;
>>> +
>>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>>> +		if (!devfreq)
>>> +			continue;
>>
>> I suppose debugfs_transitions[i].devfreq should be set to NULL when
>> devfreq device is removed, but I don't see it happening anywhere in this
>> patch.
> 
> When debugfs_transitions[] array is not fully filled out
> by devfreq_update_transitions(), debugfs_transitions[i].devfreq is NULL.
> In this case, if user execute 'cat /sys/kernel/debug/devfreq/devfreq_transitions',
> devfreq_transitions_show() need to check the debugfs_transitions[i].devfreq
> is NULL or not.
> 
> After filled out the debugfs_transitions[] array,
> actually, 'if(!devfreq)' is not necessary. Maybe, this style is inefficient
> It need to rework. I'll think again.

Imagine this situation:

1. there is a devfreq device, let's name it defreq123

2. the debugfs_transitions array is getting filled and now it has this
entry:

	debugfs_transitions[0].devfreq = defreq123

3. user removes defreq123 driver module

	# rmmod defreq123

4. the defreq123 is released now

5. at what memory location debugfs_transitions[0].devfreq is pointing now?

Re: [PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Dmitry Osipenko]

08.01.2020 10:56, Chanwoo Choi пишет:
> On 1/8/20 6:15 AM, Bjorn Andersson wrote:
>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>
>>> Add debugfs interface to provide debugging information of devfreq device.
>>> It contains 'devfreq_summary' entry to show the summary of registered
>>> devfreq devices as following and the additional debugfs file will be added.
>>> - /sys/kernel/debug/devfreq/devfreq_summary
>>>
>>> [Detailed description of each field of 'devfreq_summary' debugfs file]
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - governor	: Devfreq governor.
>>> - polling_ms	: If devfreq device uses the simple_ondemand governor,
>>> 		  polling_ms is necessary for the period. (unit: millisecond)
>>> - cur_freq_hz	: Current Frequency (unit: hz)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> - In order to show the multiple governors on devfreq_summay result,
>>> change the governor of devfreq0 from simple_ondemand to userspace.
>>>
>>> $ cat /sys/kernel/debug/devfreq/devfreq_summary
>>> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
>>> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
>>> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
>>> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
>>> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
>>> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
>>> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
>>> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
>>> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
>>> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
>>> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
>>> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
>>> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
>>> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
>>> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
>>> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
>>> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
>>> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
>>> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
>>
>> This looks quite useful.
>>
>>>
>>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> May I ask how the build test robot came up with this idea?
> 
> I'm missing the detailed what are the reported by kbuild test robot.
> It reported the possible build error. I'll add the following description
> on next version:
> 	[lkp: Reported the build error]
> 
>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> [..]
>>> +/**
>>> + * devfreq_summary_show() - Show the summary of the registered devfreq devices
>>> + *				via 'devfreq_summary' debugfs file.
>>
>> Please make this proper kerneldoc, i.e:
>>
>>  * func() - short description
>>  * @s:
>>  * @data:
>>  * 
>>  * Long description
>>  * 
>>  * Return: foo on bar
> 
> OK.
> 
>>
>> [..]
>>> @@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
>>>  	}
>>>  	devfreq_class->dev_groups = devfreq_groups;
>>>  
>>> +	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
>>> +	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {
>>
>> Don't PTR_ERR() before IS_ERR().
> 
> Before checking IS_ERR(), have to check the PTR_ERR(devfreq_debugfs)
> is -ENODEV or not because debugfs_create_dir return the '-ENODEV'
> if DEBUG_FS is disabled. If return the -ENODEV, it is not error.

You could write it this way:

	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
	err = PTR_ERR_OR_ZERO(devfreq_debugfs);
	if (err && err != -ENODEV) {
	...

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

08.01.2020 00:48, Bjorn Andersson пишет:
> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
> 
>> Add new devfreq_transitions debugfs file to track the frequency transitions
>> of all devfreq devices for the simple profiling as following:
>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>
>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>> in Kconfig in order to save the transition history.
>>
>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>> - dev_name	: Device name of h/w.
>> - dev		: Device name made by devfreq core.
>> - parent_dev	: If devfreq device uses the passive governor,
>> 		  show parent devfreq device name.
>> - load_%	: If devfreq device uses the simple_ondemand governor,
>> 		  load is used by governor whene deciding the new frequency.
>> 		  (unit: percentage)
>> - old_freq_hz	: Frequency before changing. (unit: hz)
>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>> [snip]
>>
> 
> Wouldn't it make more sense to expose this through the tracing
> framework - like many other subsystems does?

I think devfreq core already has some tracing support and indeed it
should be better to extend it rather than duplicate.

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Dmitry Osipenko]

08.01.2020 15:01, Chanwoo Choi пишет:
> On 1/8/20 7:56 PM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 1/8/20 6:31 AM, Dmitry Osipenko wrote:
>>> Hello Chanwoo,
>>>
>>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>> - dev_name	: Device name of h/w.
>>>> - dev		: Device name made by devfreq core.
>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>> 		  show parent devfreq device name.
>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>> 		  load is used by governor whene deciding the new frequency.
>>>> 		  (unit: percentage)
>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>> [snip]
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  drivers/devfreq/Kconfig            |  13 +++
>>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>>  drivers/devfreq/governor.h         |   3 +
>>>>  drivers/devfreq/governor_passive.c |   2 +
>>>>  include/linux/devfreq.h            |   1 +
>>>>  5 files changed, 145 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index 0b1df12e0f21..84936eec0ef9 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>>  	  through sysfs entries. The passive governor recommends that
>>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>>  
>>>> +comment "DEVFREQ Debugging"
>>>> +
>>>> +config NR_DEVFREQ_TRANSITIONS
>>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>>> +	depends on DEBUG_FS
>>>> +	range 10 1000
>>>> +	default "100"
>>>> +	help
>>>> +	  Show the frequency transitions of all devfreq devices via
>>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>>> +	  profiling. It needs to decide the storage size to save transition
>>>> +	  history of all devfreq devices.
>>>> +
>>>>  comment "DEVFREQ Drivers"
>>>>  
>>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index c7f5e4e06420..7abaae06fa65 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>  }
>>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>> +/**
>>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>>> + * @devfreq:	the devfreq instance
>>>> + * @old_freq:	the previous frequency before changing the frequency
>>>> + * @new_freq:	the new frequency after frequency is changed
>>>> + */
>>>> +struct devfreq_transitions {
>>>> +	struct devfreq *devfreq;
>>>> +	struct devfreq_freqs freqs;
>>>> +	unsigned long load;
>>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>>> +
>>>> +static spinlock_t devfreq_debugfs_lock;
>>>
>>> This could be:
>>>
>>> static DEFINE_SPINLOCK(devfreq_debugfs_lock);
>>>
>>> and then spin_lock_init() isn't needed.
>>
>> OK
>>
>>>
>>>
>>> Also, The "<linux/spinlock.h>" should be included directly by devfreq.c
>>
>> OK.
>>
>>>
>>>> +static int debugfs_transitions_index;
>>>> +
>>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>>> +			unsigned long old_freq, unsigned long new_freq,
>>>> +			unsigned long busy_time, unsigned long total_time)
>>>> +{
>>>> +	unsigned long load;
>>>> +	int i;
>>>> +
>>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>>> +		return;
>>>> +
>>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>>> +
>>>> +	i = debugfs_transitions_index;
>>>> +
>>>> +	/*
>>>> +	 * Calculate the load and if load is larger than 100,
>>>> +	 * initialize to 100 because the unit of load is percentage.
>>>> +	 */
>>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>>> +	if (load > 100)
>>>> +		load = 100;
>>>> +
>>>> +	debugfs_transitions[i].devfreq = devfreq;
>>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>>> +	debugfs_transitions[i].load = load;
>>>> +
>>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> +		i = 0;
>>>> +	debugfs_transitions_index = i;
>>>> +
>>>> +	spin_unlock(&devfreq_debugfs_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>>
>>> What about EXPORT_SYMBOL_GPL()?
>>
>> I'll remove it.
> 
> Ah. It is needed to support module build.
> it is used by passive governor.

My point was about the "GPL" part. The EXPORT_SYMBOL_GPL() prohibits
closed source drivers to use the exported API.

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Lukasz Luba]

Hi,

On 1/8/20 2:20 PM, Dmitry Osipenko wrote:
> 08.01.2020 00:48, Bjorn Andersson пишет:
>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>
>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>> of all devfreq devices for the simple profiling as following:
>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>
>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> in Kconfig in order to save the transition history.
>>>
>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>> 		  load is used by governor whene deciding the new frequency.
>>> 		  (unit: percentage)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>> [snip]
>>>
>>
>> Wouldn't it make more sense to expose this through the tracing
>> framework - like many other subsystems does?
> 
> I think devfreq core already has some tracing support and indeed it
> should be better to extend it rather than duplicate.
> 

In my opinion this debugfs interface should be considered as a helpful
validation entry point. We had some issues with wrong bootloader
configurations in clock tree, where some frequencies could not be set
in the kernel. Similar useful description can be find in clock subsystem
where there is clock tree summary file.

It is much cheaper to poke a few files in debug dir by some automated
test than starting tracing, provoking desired code flow in the
devfreq for every device, paring the results... A simple boot test
which reads only these new files can be enough to rise the flag.
Secondly the tracing is not always compiled.

It could capture old/wrong bootloaders which pinned devices
improperly to PLLs or wrong DT values in OPP table.
(a workaround for Odroid xu4 patchset:
https://lkml.org/lkml/2019/7/15/276
)

Chanwoo what do think about some sanity check summary?
It could be presented in a 3rd file: 'devfreq_sanity', which
could report if the devices could set their registered OPPs
and got the same values, i.e. set 166MHz --> set to 150MHz
in reality. If a config option i.e. DEVFREQ_SANITY is set
then during the registration of a new device it checks OPPs
if they are possible to set. It could be done before assigning
the governor for the device and results present in of of your files.

Regards,
Lukasz

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/8/20 11:23 PM, Dmitry Osipenko wrote:
> 08.01.2020 15:01, Chanwoo Choi пишет:
>> On 1/8/20 7:56 PM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 1/8/20 6:31 AM, Dmitry Osipenko wrote:
>>>> Hello Chanwoo,
>>>>
>>>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>>> of all devfreq devices for the simple profiling as following:
>>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>
>>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>> in Kconfig in order to save the transition history.
>>>>>
>>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>>> - dev_name	: Device name of h/w.
>>>>> - dev		: Device name made by devfreq core.
>>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>>> 		  show parent devfreq device name.
>>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>>> 		  load is used by governor whene deciding the new frequency.
>>>>> 		  (unit: percentage)
>>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>>
>>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>>> [snip]
>>>>>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> ---
>>>>>  drivers/devfreq/Kconfig            |  13 +++
>>>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>>>  drivers/devfreq/governor.h         |   3 +
>>>>>  drivers/devfreq/governor_passive.c |   2 +
>>>>>  include/linux/devfreq.h            |   1 +
>>>>>  5 files changed, 145 insertions(+)
>>>>>
>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>> index 0b1df12e0f21..84936eec0ef9 100644
>>>>> --- a/drivers/devfreq/Kconfig
>>>>> +++ b/drivers/devfreq/Kconfig
>>>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>>>  	  through sysfs entries. The passive governor recommends that
>>>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>>>  
>>>>> +comment "DEVFREQ Debugging"
>>>>> +
>>>>> +config NR_DEVFREQ_TRANSITIONS
>>>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>>>> +	depends on DEBUG_FS
>>>>> +	range 10 1000
>>>>> +	default "100"
>>>>> +	help
>>>>> +	  Show the frequency transitions of all devfreq devices via
>>>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>>>> +	  profiling. It needs to decide the storage size to save transition
>>>>> +	  history of all devfreq devices.
>>>>> +
>>>>>  comment "DEVFREQ Drivers"
>>>>>  
>>>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index c7f5e4e06420..7abaae06fa65 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>>  }
>>>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>>> +/**
>>>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>>>> + * @devfreq:	the devfreq instance
>>>>> + * @old_freq:	the previous frequency before changing the frequency
>>>>> + * @new_freq:	the new frequency after frequency is changed
>>>>> + */
>>>>> +struct devfreq_transitions {
>>>>> +	struct devfreq *devfreq;
>>>>> +	struct devfreq_freqs freqs;
>>>>> +	unsigned long load;
>>>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>>>> +
>>>>> +static spinlock_t devfreq_debugfs_lock;
>>>>
>>>> This could be:
>>>>
>>>> static DEFINE_SPINLOCK(devfreq_debugfs_lock);
>>>>
>>>> and then spin_lock_init() isn't needed.
>>>
>>> OK
>>>
>>>>
>>>>
>>>> Also, The "<linux/spinlock.h>" should be included directly by devfreq.c
>>>
>>> OK.
>>>
>>>>
>>>>> +static int debugfs_transitions_index;
>>>>> +
>>>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>>>> +			unsigned long old_freq, unsigned long new_freq,
>>>>> +			unsigned long busy_time, unsigned long total_time)
>>>>> +{
>>>>> +	unsigned long load;
>>>>> +	int i;
>>>>> +
>>>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>>>> +		return;
>>>>> +
>>>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>>>> +
>>>>> +	i = debugfs_transitions_index;
>>>>> +
>>>>> +	/*
>>>>> +	 * Calculate the load and if load is larger than 100,
>>>>> +	 * initialize to 100 because the unit of load is percentage.
>>>>> +	 */
>>>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>>>> +	if (load > 100)
>>>>> +		load = 100;
>>>>> +
>>>>> +	debugfs_transitions[i].devfreq = devfreq;
>>>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>>>> +	debugfs_transitions[i].load = load;
>>>>> +
>>>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>> +		i = 0;
>>>>> +	debugfs_transitions_index = i;
>>>>> +
>>>>> +	spin_unlock(&devfreq_debugfs_lock);
>>>>> +}
>>>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>>>
>>>> What about EXPORT_SYMBOL_GPL()?
>>>
>>> I'll remove it.
>>
>> Ah. It is needed to support module build.
>> it is used by passive governor.
> 
> My point was about the "GPL" part. The EXPORT_SYMBOL_GPL() prohibits
> closed source drivers to use the exported API.

I'm sorry of my misunderstanding.
OK. I'll use EXPORT_SYMBOL_GPL()

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

Hi Bjorn and Dmitry,

I replied from Bjorn and Dmitry opinion.

On 1/8/20 11:20 PM, Dmitry Osipenko wrote:
> 08.01.2020 00:48, Bjorn Andersson пишет:
>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>
>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>> of all devfreq devices for the simple profiling as following:
>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>
>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>> in Kconfig in order to save the transition history.
>>>
>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>> - dev_name	: Device name of h/w.
>>> - dev		: Device name made by devfreq core.
>>> - parent_dev	: If devfreq device uses the passive governor,
>>> 		  show parent devfreq device name.
>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>> 		  load is used by governor whene deciding the new frequency.
>>> 		  (unit: percentage)
>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>
>>> [For example on Exynos5422-based Odroid-XU3 board]
>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>> [snip]
>>>
>>
>> Wouldn't it make more sense to expose this through the tracing
>> framework - like many other subsystems does?
> 
> I think devfreq core already has some tracing support and indeed it
> should be better to extend it rather than duplicate.
> 

First of all, thanks for comments.

Before developing it, I have considered what is better to
support debugging features for devfreq device. As you commented,
trace event is more general way to catch the detailed behavior.

But, I hope to provide the very easy simple profiling way
for user if it is not harmful to the principle of linux kernel.

In order to prevent the performance regression when DEBUG_FS is enabled,
I will add the CONFIG_DEVFREQ_TRANSITIONS_DEBUG for 'devfreq_transitions'
to make it selectable.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/8/20 11:10 PM, Dmitry Osipenko wrote:
> 08.01.2020 14:22, Chanwoo Choi пишет:
>> On 1/8/20 6:56 AM, Dmitry Osipenko wrote:
>>> 07.01.2020 12:05, Chanwoo Choi пишет:
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>> - dev_name	: Device name of h/w.
>>>> - dev		: Device name made by devfreq core.
>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>> 		  show parent devfreq device name.
>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>> 		  load is used by governor whene deciding the new frequency.
>>>> 		  (unit: percentage)
>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>> [snip]
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  drivers/devfreq/Kconfig            |  13 +++
>>>>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>>>>  drivers/devfreq/governor.h         |   3 +
>>>>  drivers/devfreq/governor_passive.c |   2 +
>>>>  include/linux/devfreq.h            |   1 +
>>>>  5 files changed, 145 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index 0b1df12e0f21..84936eec0ef9 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>>>>  	  through sysfs entries. The passive governor recommends that
>>>>  	  devfreq device uses the OPP table to get the frequency/voltage.
>>>>  
>>>> +comment "DEVFREQ Debugging"
>>>> +
>>>> +config NR_DEVFREQ_TRANSITIONS
>>>> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>>>> +	depends on DEBUG_FS
>>>> +	range 10 1000
>>>> +	default "100"
>>>> +	help
>>>> +	  Show the frequency transitions of all devfreq devices via
>>>> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>>>> +	  profiling. It needs to decide the storage size to save transition
>>>> +	  history of all devfreq devices.
>>>> +
>>>>  comment "DEVFREQ Drivers"
>>>>  
>>>>  config ARM_EXYNOS_BUS_DEVFREQ
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index c7f5e4e06420..7abaae06fa65 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>  }
>>>>  EXPORT_SYMBOL(devfreq_update_status);
>>>>  
>>>> +/**
>>>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>>>> + * @devfreq:	the devfreq instance
>>>> + * @old_freq:	the previous frequency before changing the frequency
>>>> + * @new_freq:	the new frequency after frequency is changed
>>>> + */
>>>> +struct devfreq_transitions {
>>>> +	struct devfreq *devfreq;
>>>> +	struct devfreq_freqs freqs;
>>>> +	unsigned long load;
>>>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>>>> +
>>>> +static spinlock_t devfreq_debugfs_lock;
>>>> +static int debugfs_transitions_index;
>>>> +
>>>> +void devfreq_update_transitions(struct devfreq *devfreq,
>>>> +			unsigned long old_freq, unsigned long new_freq,
>>>> +			unsigned long busy_time, unsigned long total_time)
>>>> +{
>>>> +	unsigned long load;
>>>> +	int i;
>>>> +
>>>> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>>>> +		return;
>>>> +
>>>> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>>>> +
>>>> +	i = debugfs_transitions_index;
>>>> +
>>>> +	/*
>>>> +	 * Calculate the load and if load is larger than 100,
>>>> +	 * initialize to 100 because the unit of load is percentage.
>>>> +	 */
>>>> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>>>> +	if (load > 100)
>>>> +		load = 100;
>>>> +
>>>> +	debugfs_transitions[i].devfreq = devfreq;
>>>> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>>>> +	debugfs_transitions[i].freqs.old = old_freq;
>>>> +	debugfs_transitions[i].freqs.new = new_freq;
>>>> +	debugfs_transitions[i].load = load;
>>>> +
>>>> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> +		i = 0;
>>>> +	debugfs_transitions_index = i;
>>>> +
>>>> +	spin_unlock(&devfreq_debugfs_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(devfreq_update_transitions);
>>>> +
>>>>  /**
>>>>   * find_devfreq_governor() - Find devfreq governor from name
>>>>   * @name:	name of the governor
>>>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>>>>  		return err;
>>>>  	}
>>>>  
>>>> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
>>>> +					devfreq->last_status.busy_time,
>>>> +					devfreq->last_status.total_time);
>>>> +
>>>>  	freqs.new = new_freq;
>>>>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>>  
>>>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>>>>  }
>>>>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>>>  
>>>> +/**
>>>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>>>> + *			devfreq devices via 'devfreq_transitions' debugfs file.
>>>> + */
>>>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>>>> +{
>>>> +	struct devfreq *devfreq = NULL;
>>>> +	struct devfreq *p_devfreq = NULL;
>>>> +	struct devfreq_freqs *freqs = NULL;
>>>> +	unsigned long load;
>>>> +	int i = debugfs_transitions_index;
>>>> +	int count;
>>>> +
>>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>>> +			"time_ms",
>>>> +			"dev_name",
>>>> +			"dev",
>>>> +			"parent_dev",
>>>> +			"load_%",
>>>> +			"old_freq_hz",
>>>> +			"new_freq_hz");
>>>> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>>>> +			"----------",
>>>> +			"------------------------------",
>>>> +			"----------",
>>>> +			"----------",
>>>> +			"----------",
>>>> +			"------------",
>>>> +			"------------");
>>>
>>> Isn't this needed here?
>>>
>>> mutex_lock(&devfreq_list_lock);
>>
>> It doesn't touch the devfreq instance of devfreq_list.
>> So, it is not necessary locked of devfreq_list_lock.
> 
> What stops devfreq device to be removed by another CPU thread while this
> function is in a process of execution?
> 
> This condition is unlikely to happen in practice ever, but technically
> it should be possible to happen.
> 
>>>> +	spin_lock(&devfreq_debugfs_lock);
>>>> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>>>> +		devfreq = debugfs_transitions[i].devfreq;
>>>> +		freqs = &debugfs_transitions[i].freqs;
>>>> +		load = debugfs_transitions[i].load;
>>>> +
>>>> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>>>> +		if (!devfreq)
>>>> +			continue;
>>>
>>> I suppose debugfs_transitions[i].devfreq should be set to NULL when
>>> devfreq device is removed, but I don't see it happening anywhere in this
>>> patch.
>>
>> When debugfs_transitions[] array is not fully filled out
>> by devfreq_update_transitions(), debugfs_transitions[i].devfreq is NULL.
>> In this case, if user execute 'cat /sys/kernel/debug/devfreq/devfreq_transitions',
>> devfreq_transitions_show() need to check the debugfs_transitions[i].devfreq
>> is NULL or not.
>>
>> After filled out the debugfs_transitions[] array,
>> actually, 'if(!devfreq)' is not necessary. Maybe, this style is inefficient
>> It need to rework. I'll think again.
> 
> Imagine this situation:
> 
> 1. there is a devfreq device, let's name it defreq123
> 
> 2. the debugfs_transitions array is getting filled and now it has this
> entry:
> 
> 	debugfs_transitions[0].devfreq = defreq123
> 
> 3. user removes defreq123 driver module
> 
> 	# rmmod defreq123
> 
> 4. the defreq123 is released now
> 
> 5. at what memory location debugfs_transitions[0].devfreq is pointing now?
> 
> 

You're right. It is my missing point.
Instead of storing the devfreq pointer into debugfs_transitions[].devfreq,
It is better to copy the necessary information to debugfs_transitions[]
for preventing the mentioned situation.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/9/20 12:44 AM, Lukasz Luba wrote:
> Hi,
> 
> On 1/8/20 2:20 PM, Dmitry Osipenko wrote:
>> 08.01.2020 00:48, Bjorn Andersson пишет:
>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_ms    : Change time of frequency transition. (unit: millisecond)
>>>> - dev_name    : Device name of h/w.
>>>> - dev        : Device name made by devfreq core.
>>>> - parent_dev    : If devfreq device uses the passive governor,
>>>>           show parent devfreq device name.
>>>> - load_%    : If devfreq device uses the simple_ondemand governor,
>>>>           load is used by governor whene deciding the new frequency.
>>>>           (unit: percentage)
>>>> - old_freq_hz    : Frequency before changing. (unit: hz)
>>>> - new_freq_hz    : Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>> [snip]
>>>>
>>>
>>> Wouldn't it make more sense to expose this through the tracing
>>> framework - like many other subsystems does?
>>
>> I think devfreq core already has some tracing support and indeed it
>> should be better to extend it rather than duplicate.
>>
> 
> In my opinion this debugfs interface should be considered as a helpful
> validation entry point. We had some issues with wrong bootloader
> configurations in clock tree, where some frequencies could not be set
> in the kernel. Similar useful description can be find in clock subsystem
> where there is clock tree summary file.
> 
> It is much cheaper to poke a few files in debug dir by some automated
> test than starting tracing, provoking desired code flow in the
> devfreq for every device, paring the results... A simple boot test
> which reads only these new files can be enough to rise the flag.
> Secondly the tracing is not always compiled.
> 
> It could capture old/wrong bootloaders which pinned devices
> improperly to PLLs or wrong DT values in OPP table.
> (a workaround for Odroid xu4 patchset:
> https://protect2.fireeye.com/url?k=364d2fbb-6b9993d3-364ca4f4-0cc47a3356b2-98db7e7cf023414c&u=https://lkml.org/lkml/2019/7/15/276
> )
> 
> Chanwoo what do think about some sanity check summary?
> It could be presented in a 3rd file: 'devfreq_sanity', which
> could report if the devices could set their registered OPPs
> and got the same values, i.e. set 166MHz --> set to 150MHz
> in reality. If a config option i.e. DEVFREQ_SANITY is set
> then during the registration of a new device it checks OPPs
> if they are possible to set. It could be done before assigning
> the governor for the device and results present in of of your files.

Firstly, I'm welcoming to add new debugging feature.

As you suggested, it is required for the OPP control.
But, I'm not sure that it have to be verified in either OPP or devfreq.
Or it have be verified when adding the OPP table into devicetree file.

If we add this debugging feature, I think 'resource_sanity' as following path.
/sys/kernel/debug/devfreq/devfreqX/resource_sanity (not fixed name)
- show the sanity result of regulator voltage and frequency

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 0/2] PM / devfreq: Add debugfs support

[Kamil Konieczny]

Hi,

On 07.01.2020 10:05, Chanwoo Choi wrote:
> Add debugfs interface to provide debugging information of devfreq device.
> It contains 'devfreq_summary' and 'debug_transitions' debugfs file
> in order to provide the simple profiling to user via debugfs
> without any specific profiling tool.
> 
> [Added debugfs file]
> - "/sys/kernel/debug/devfreq/devfreq_summary"
>   : Show the summary of the registered devfreq devices.
> - "/sys/kernel/debug/devfreq/devfreq_transitions"
>   : Show the frequency transition of the registered devfreq devices.
> 
> Recommend the each patch to check the detailed description
> of each fields of both devfreq_summary and devfreq_transitions.
> 
> This series contains the patch[1] and add the patch2 for 'devfreq_transitions'
> [1] https://patchwork.kernel.org/patch/11320265/
> - [v3] PM / devfreq: Add debugfs support with devfreq_summary file
> 
> 
> For example on Exynos5422-based Odroid-XU3 board,
> - In order to show the multiple governors on devfreq_summay result,
> change the governor of devfreq0 from simple_ondemand to userspace.
> 
> $ cat /sys/kernel/debug/devfreq/devfreq_summary
> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz

Imho either drop "_hz" or change it to "_Hz".

> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000

Imho it better looks with freq aligned to right, eg.:

soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000     88700000     266000000
soc:bus_peri                   devfreq8   devfreq1   passive         0           66600000     66600000      66600000
soc:bus_g2d                    devfreq9   devfreq1   passive         0                  0     83250000     333000000

> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
> 
> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> [snip]
> 
> 
> Depends on:
> It depends on patch[2] for preventing the merge conflic.
> [2] https://patchwork.kernel.org/patch/11320257/
> - PM / devfreq: Add missing function description and rename static functions
> 
> Chanwoo Choi (2):
>   PM / devfreq: Add debugfs support with devfreq_summary file
>   PM / devfreq: Add devfreq_transitions debugfs file
> 
>  drivers/devfreq/Kconfig            |  13 ++
>  drivers/devfreq/devfreq.c          | 206 +++++++++++++++++++++++++++++
>  drivers/devfreq/governor.h         |   3 +
>  drivers/devfreq/governor_passive.c |   2 +
>  include/linux/devfreq.h            |   1 +
>  5 files changed, 225 insertions(+)
> 

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Bjorn Andersson]

On Thu 09 Jan 00:07 PST 2020, Chanwoo Choi wrote:

> Hi Bjorn and Dmitry,
> 
> I replied from Bjorn and Dmitry opinion.
> 
> On 1/8/20 11:20 PM, Dmitry Osipenko wrote:
> > 08.01.2020 00:48, Bjorn Andersson ??????????:
> >> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
> >>
> >>> Add new devfreq_transitions debugfs file to track the frequency transitions
> >>> of all devfreq devices for the simple profiling as following:
> >>> - /sys/kernel/debug/devfreq/devfreq_transitions
> >>>
> >>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
> >>> in Kconfig in order to save the transition history.
> >>>
> >>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
> >>> - time_ms	: Change time of frequency transition. (unit: millisecond)
> >>> - dev_name	: Device name of h/w.
> >>> - dev		: Device name made by devfreq core.
> >>> - parent_dev	: If devfreq device uses the passive governor,
> >>> 		  show parent devfreq device name.
> >>> - load_%	: If devfreq device uses the simple_ondemand governor,
> >>> 		  load is used by governor whene deciding the new frequency.
> >>> 		  (unit: percentage)
> >>> - old_freq_hz	: Frequency before changing. (unit: hz)
> >>> - new_freq_hz	: Frequency after changed. (unit: hz)
> >>>
> >>> [For example on Exynos5422-based Odroid-XU3 board]
> >>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> >>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
> >>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> >>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> >>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
> >>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> >>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> >>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> >>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> >>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> >>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> >>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> >>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> >>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> >>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> >>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> >>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> >>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> >>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> >>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> >>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> >>> [snip]
> >>>
> >>
> >> Wouldn't it make more sense to expose this through the tracing
> >> framework - like many other subsystems does?
> > 
> > I think devfreq core already has some tracing support and indeed it
> > should be better to extend it rather than duplicate.
> > 
> 
> First of all, thanks for comments.
> 
> Before developing it, I have considered what is better to
> support debugging features for devfreq device. As you commented,
> trace event is more general way to catch the detailed behavior.
> 

It's more general, it has already dealt with the locking and life cycle
questions that was brought up by others and it allows getting traces
devfreq traces in the same timeline as other events (to give insight in
cross-framework behavior).

> But, I hope to provide the very easy simple profiling way
> for user if it is not harmful to the principle of linux kernel.
> 

You would achieve the same simplicity by integrating with the trace
framework instead of rolling your own subset of the functionality.

I know that it's the principle of the Linux kernel that everyone should
have their own ring buffer implementation, but you should try to use the
existing ones when it makes sense. And in my view this is a prime
example - with many additional benefits of doing so.

> In order to prevent the performance regression when DEBUG_FS is enabled,
> I will add the CONFIG_DEVFREQ_TRANSITIONS_DEBUG for 'devfreq_transitions'
> to make it selectable.
> 

The tracing framework has both static and dynamic mechanisms for
avoiding performance penalties when tracing is disabled and does provide
better performance than your proposal when active.

Relying on a Kconfig option means that with e.g. arm64 devices being
built from a single defconfig we will either all be missing this feature
or we will all always keep logging devfreq transitions to your buffer.

Regards,
Bjorn

Re: [PATCH 0/2] PM / devfreq: Add debugfs support

[Chanwoo Choi]

On 1/10/20 12:06 AM, Kamil Konieczny wrote:
> Hi,
> 
> On 07.01.2020 10:05, Chanwoo Choi wrote:
>> Add debugfs interface to provide debugging information of devfreq device.
>> It contains 'devfreq_summary' and 'debug_transitions' debugfs file
>> in order to provide the simple profiling to user via debugfs
>> without any specific profiling tool.
>>
>> [Added debugfs file]
>> - "/sys/kernel/debug/devfreq/devfreq_summary"
>>   : Show the summary of the registered devfreq devices.
>> - "/sys/kernel/debug/devfreq/devfreq_transitions"
>>   : Show the frequency transition of the registered devfreq devices.
>>
>> Recommend the each patch to check the detailed description
>> of each fields of both devfreq_summary and devfreq_transitions.
>>
>> This series contains the patch[1] and add the patch2 for 'devfreq_transitions'
>> [1] https://patchwork.kernel.org/patch/11320265/
>> - [v3] PM / devfreq: Add debugfs support with devfreq_summary file
>>
>>
>> For example on Exynos5422-based Odroid-XU3 board,
>> - In order to show the multiple governors on devfreq_summay result,
>> change the governor of devfreq0 from simple_ondemand to userspace.
>>
>> $ cat /sys/kernel/debug/devfreq/devfreq_summary
>> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
> 
> Imho either drop "_hz" or change it to "_Hz".

hz -> Hz

> 
>> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
>> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
>> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
>> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
>> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
>> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
>> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
>> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
>> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
>> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
> 
> Imho it better looks with freq aligned to right, eg.:

OK.

> 
> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000     88700000     266000000
> soc:bus_peri                   devfreq8   devfreq1   passive         0           66600000     66600000      66600000
> soc:bus_g2d                    devfreq9   devfreq1   passive         0                  0     83250000     333000000
> 
>> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
>> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
>> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
>> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
>> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
>> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
>> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
>>
>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>> [snip]
>>
>>
>> Depends on:
>> It depends on patch[2] for preventing the merge conflic.
>> [2] https://patchwork.kernel.org/patch/11320257/
>> - PM / devfreq: Add missing function description and rename static functions
>>
>> Chanwoo Choi (2):
>>   PM / devfreq: Add debugfs support with devfreq_summary file
>>   PM / devfreq: Add devfreq_transitions debugfs file
>>
>>  drivers/devfreq/Kconfig            |  13 ++
>>  drivers/devfreq/devfreq.c          | 206 +++++++++++++++++++++++++++++
>>  drivers/devfreq/governor.h         |   3 +
>>  drivers/devfreq/governor_passive.c |   2 +
>>  include/linux/devfreq.h            |   1 +
>>  5 files changed, 225 insertions(+)
>>
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/10/20 2:21 AM, Bjorn Andersson wrote:
> On Thu 09 Jan 00:07 PST 2020, Chanwoo Choi wrote:
> 
>> Hi Bjorn and Dmitry,
>>
>> I replied from Bjorn and Dmitry opinion.
>>
>> On 1/8/20 11:20 PM, Dmitry Osipenko wrote:
>>> 08.01.2020 00:48, Bjorn Andersson ??????????:
>>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>>
>>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>>> of all devfreq devices for the simple profiling as following:
>>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>
>>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>> in Kconfig in order to save the transition history.
>>>>>
>>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>>> - dev_name	: Device name of h/w.
>>>>> - dev		: Device name made by devfreq core.
>>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>>> 		  show parent devfreq device name.
>>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>>> 		  load is used by governor whene deciding the new frequency.
>>>>> 		  (unit: percentage)
>>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>>
>>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>>> [snip]
>>>>>
>>>>
>>>> Wouldn't it make more sense to expose this through the tracing
>>>> framework - like many other subsystems does?
>>>
>>> I think devfreq core already has some tracing support and indeed it
>>> should be better to extend it rather than duplicate.
>>>
>>
>> First of all, thanks for comments.
>>
>> Before developing it, I have considered what is better to
>> support debugging features for devfreq device. As you commented,
>> trace event is more general way to catch the detailed behavior.
>>
> 
> It's more general, it has already dealt with the locking and life cycle
> questions that was brought up by others and it allows getting traces
> devfreq traces in the same timeline as other events (to give insight in
> cross-framework behavior).
> 
>> But, I hope to provide the very easy simple profiling way
>> for user if it is not harmful to the principle of linux kernel.
>>
> 
> You would achieve the same simplicity by integrating with the trace
> framework instead of rolling your own subset of the functionality.
> 
> I know that it's the principle of the Linux kernel that everyone should
> have their own ring buffer implementation, but you should try to use the
> existing ones when it makes sense. And in my view this is a prime
> example - with many additional benefits of doing so.

When we are usually using the profiling tool, existing trace framework
is the best. Actually, might need to read the frequency transitions
on the user-space process which is related to monitoring, without
the enabled trace configuration.

> 
>> In order to prevent the performance regression when DEBUG_FS is enabled,
>> I will add the CONFIG_DEVFREQ_TRANSITIONS_DEBUG for 'devfreq_transitions'
>> to make it selectable.
>>
> 
> The tracing framework has both static and dynamic mechanisms for
> avoiding performance penalties when tracing is disabled and does provide
> better performance than your proposal when active.

It provides the separate configuration to select them by user.
It is optional. It means that if CONFIG_DEVFREQ_TRANSITIONS_DEBUG
is enabled by user, the user has made a choice this situation with
even if the some regression happen and instead get the frequency
transition for monitoring on user-space process.

> 
> Relying on a Kconfig option means that with e.g. arm64 devices being
> built from a single defconfig we will either all be missing this feature
> or we will all always keep logging devfreq transitions to your buffer.

The single defconfig doesn't contain the all configuration provided
from linux kernel. Furthermore, the debug option is optional by user.
I think that it doesn't matter.
-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

Hi Bjorn,

On 1/10/20 2:04 PM, Chanwoo Choi wrote:
> On 1/10/20 2:21 AM, Bjorn Andersson wrote:
>> On Thu 09 Jan 00:07 PST 2020, Chanwoo Choi wrote:
>>
>>> Hi Bjorn and Dmitry,
>>>
>>> I replied from Bjorn and Dmitry opinion.
>>>
>>> On 1/8/20 11:20 PM, Dmitry Osipenko wrote:
>>>> 08.01.2020 00:48, Bjorn Andersson ??????????:
>>>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>>>
>>>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>>>> of all devfreq devices for the simple profiling as following:
>>>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>>
>>>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>>> in Kconfig in order to save the transition history.
>>>>>>
>>>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>>>> - dev_name	: Device name of h/w.
>>>>>> - dev		: Device name made by devfreq core.
>>>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>>>> 		  show parent devfreq device name.
>>>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>>>> 		  load is used by governor whene deciding the new frequency.
>>>>>> 		  (unit: percentage)
>>>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>>>
>>>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>>>> [snip]
>>>>>>
>>>>>
>>>>> Wouldn't it make more sense to expose this through the tracing
>>>>> framework - like many other subsystems does?
>>>>
>>>> I think devfreq core already has some tracing support and indeed it
>>>> should be better to extend it rather than duplicate.
>>>>
>>>
>>> First of all, thanks for comments.
>>>
>>> Before developing it, I have considered what is better to
>>> support debugging features for devfreq device. As you commented,
>>> trace event is more general way to catch the detailed behavior.
>>>
>>
>> It's more general, it has already dealt with the locking and life cycle
>> questions that was brought up by others and it allows getting traces
>> devfreq traces in the same timeline as other events (to give insight in
>> cross-framework behavior).
>>
>>> But, I hope to provide the very easy simple profiling way
>>> for user if it is not harmful to the principle of linux kernel.
>>>
>>
>> You would achieve the same simplicity by integrating with the trace
>> framework instead of rolling your own subset of the functionality.
>>
>> I know that it's the principle of the Linux kernel that everyone should
>> have their own ring buffer implementation, but you should try to use the
>> existing ones when it makes sense. And in my view this is a prime
>> example - with many additional benefits of doing so.
> 
> When we are usually using the profiling tool, existing trace framework
> is the best. Actually, might need to read the frequency transitions
> on the user-space process which is related to monitoring, without
> the enabled trace configuration.
> 
>>
>>> In order to prevent the performance regression when DEBUG_FS is enabled,
>>> I will add the CONFIG_DEVFREQ_TRANSITIONS_DEBUG for 'devfreq_transitions'
>>> to make it selectable.
>>>
>>
>> The tracing framework has both static and dynamic mechanisms for
>> avoiding performance penalties when tracing is disabled and does provide
>> better performance than your proposal when active.
> 
> It provides the separate configuration to select them by user.
> It is optional. It means that if CONFIG_DEVFREQ_TRANSITIONS_DEBUG
> is enabled by user, the user has made a choice this situation with
> even if the some regression happen and instead get the frequency
> transition for monitoring on user-space process.
> 
>>
>> Relying on a Kconfig option means that with e.g. arm64 devices being
>> built from a single defconfig we will either all be missing this feature
>> or we will all always keep logging devfreq transitions to your buffer.
> 
> The single defconfig doesn't contain the all configuration provided
> from linux kernel. Furthermore, the debug option is optional by user.
> I think that it doesn't matter.
> 

Basically, I agree that trace point is better. Just as I said,
I hope to provide the very easy simple profiling way.
But, as you suggested and I knew that it seems that
duplicate feature. So, I'll drop it and extend the existing
trace point of devfreq. Thanks for your comment and discussion.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Kamil Konieczny]

Hi Chanwoo,

On 07.01.2020 10:05, Chanwoo Choi wrote:
> Add new devfreq_transitions debugfs file to track the frequency transitions
> of all devfreq devices for the simple profiling as following:
> - /sys/kernel/debug/devfreq/devfreq_transitions
> 
> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
> in Kconfig in order to save the transition history.
> 
> [Detailed description of each field of 'devfreq_transitions' debugfs file]
> - time_ms	: Change time of frequency transition. (unit: millisecond)
> - dev_name	: Device name of h/w.
> - dev		: Device name made by devfreq core.
> - parent_dev	: If devfreq device uses the passive governor,
> 		  show parent devfreq device name.
> - load_%	: If devfreq device uses the simple_ondemand governor,
> 		  load is used by governor whene deciding the new frequency.
> 		  (unit: percentage)
> - old_freq_hz	: Frequency before changing. (unit: hz)

s/hz/Hz/

> - new_freq_hz	: Frequency after changed. (unit: hz)

s/hz/Hz/

> 
> [For example on Exynos5422-based Odroid-XU3 board]
> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz

s/hz/Hz/

> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000

Imho it is better to align freq numbers to right, for example:

14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000     67000000
14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000

> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
> [snip]
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/Kconfig            |  13 +++
>  drivers/devfreq/devfreq.c          | 126 +++++++++++++++++++++++++++++
>  drivers/devfreq/governor.h         |   3 +
>  drivers/devfreq/governor_passive.c |   2 +
>  include/linux/devfreq.h            |   1 +
>  5 files changed, 145 insertions(+)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..84936eec0ef9 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>  	  through sysfs entries. The passive governor recommends that
>  	  devfreq device uses the OPP table to get the frequency/voltage.
>  
> +comment "DEVFREQ Debugging"
> +
> +config NR_DEVFREQ_TRANSITIONS
> +	int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
> +	depends on DEBUG_FS
> +	range 10 1000
> +	default "100"
> +	help
> +	  Show the frequency transitions of all devfreq devices via
> +	  '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
> +	  profiling. It needs to decide the storage size to save transition
> +	  history of all devfreq devices.
> +
>  comment "DEVFREQ Drivers"
>  
>  config ARM_EXYNOS_BUS_DEVFREQ
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index c7f5e4e06420..7abaae06fa65 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  }
>  EXPORT_SYMBOL(devfreq_update_status);
>  
> +/**
> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
> + * @devfreq:	the devfreq instance
> + * @old_freq:	the previous frequency before changing the frequency
> + * @new_freq:	the new frequency after frequency is changed
> + */
> +struct devfreq_transitions {
> +	struct devfreq *devfreq;
> +	struct devfreq_freqs freqs;
> +	unsigned long load;
> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
> +
> +static spinlock_t devfreq_debugfs_lock;
> +static int debugfs_transitions_index;
> +
> +void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time)
> +{
> +	unsigned long load;
> +	int i;
> +
> +	if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
> +		return;
> +
> +	spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
> +
> +	i = debugfs_transitions_index;
> +
> +	/*
> +	 * Calculate the load and if load is larger than 100,
> +	 * initialize to 100 because the unit of load is percentage.
> +	 */

Remove this comment, it is better to print numbers as they are so one can
find a problem.

> +	load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
> +	if (load > 100)
> +		load = 100;

Imho it should always be busy_time <= total_time, so

	if (load > 100) {
		WARN_ONCE(busy_time > total_time, "devfreq: busy_time > total_time");
		load = 100;
	}

Or drop this "if (load > 100) load=100;" and print load as is in logs.

> +
> +	debugfs_transitions[i].devfreq = devfreq;
> +	debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
> +	debugfs_transitions[i].freqs.old = old_freq;
> +	debugfs_transitions[i].freqs.new = new_freq;
> +	debugfs_transitions[i].load = load;
> +
> +	if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
> +		i = 0;
> +	debugfs_transitions_index = i;
> +
> +	spin_unlock(&devfreq_debugfs_lock);
> +}
> +EXPORT_SYMBOL(devfreq_update_transitions);
> +
>  /**
>   * find_devfreq_governor() - Find devfreq governor from name
>   * @name:	name of the governor
> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>  		return err;
>  	}
>  
> +	devfreq_update_transitions(devfreq, cur_freq, new_freq,
> +					devfreq->last_status.busy_time,
> +					devfreq->last_status.total_time);
> +
>  	freqs.new = new_freq;
>  	notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>  
> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>  
> +/**
> + * devfreq_transitions_show() - Show the frequency transitions of the registered
> + *			devfreq devices via 'devfreq_transitions' debugfs file.
> + */
> +static int devfreq_transitions_show(struct seq_file *s, void *data)
> +{
> +	struct devfreq *devfreq = NULL;
> +	struct devfreq *p_devfreq = NULL;
> +	struct devfreq_freqs *freqs = NULL;
> +	unsigned long load;
> +	int i = debugfs_transitions_index;
> +	int count;
> +
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"time_ms",
> +			"dev_name",
> +			"dev",
> +			"parent_dev",
> +			"load_%",
> +			"old_freq_hz",
> +			"new_freq_hz");
> +	seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
> +			"----------",
> +			"------------------------------",
> +			"----------",
> +			"----------",
> +			"----------",
> +			"------------",
> +			"------------");
> +
> +	spin_lock(&devfreq_debugfs_lock);
> +	for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
> +		devfreq = debugfs_transitions[i].devfreq;
> +		freqs = &debugfs_transitions[i].freqs;
> +		load = debugfs_transitions[i].load;
> +
> +		i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
> +		if (!devfreq)
> +			continue;
> +
> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +		if (!strncmp(devfreq->governor_name,
> +				DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
> +			struct devfreq_passive_data *data = devfreq->data;
> +
> +			if (data)
> +				p_devfreq = data->parent;
> +		} else {
> +			p_devfreq = NULL;
> +		}
> +#endif
> +		seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
> +			freqs->time,
> +			dev_name(devfreq->dev.parent),
> +			dev_name(&devfreq->dev),
> +			p_devfreq ? dev_name(&p_devfreq->dev) : "",
> +			load,
> +			freqs->old,
> +			freqs->new);
> +	}
> +	spin_unlock(&devfreq_debugfs_lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
> +
>  static int __init devfreq_init(void)
>  {
>  	devfreq_class = class_create(THIS_MODULE, "devfreq");
> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>  		devfreq_debugfs = NULL;
>  		pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>  	} else {
> +		spin_lock_init(&devfreq_debugfs_lock);
> +
>  		debugfs_create_file("devfreq_summary", 0444,
>  				devfreq_debugfs, NULL,
>  				&devfreq_summary_fops);
> +		debugfs_create_file("devfreq_transitions", 0444,
> +				devfreq_debugfs, NULL,
> +				&devfreq_transitions_fops);
>  	}
>  
>  	return 0;
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index dc7533ccc3db..01eecfdaf2d6 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>  extern int devfreq_remove_governor(struct devfreq_governor *governor);
>  
>  extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
> +extern void devfreq_update_transitions(struct devfreq *devfreq,
> +			unsigned long old_freq, unsigned long new_freq,
> +			unsigned long busy_time, unsigned long total_time);
>  
>  static inline int devfreq_update_stats(struct devfreq *df)
>  {
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index be6eeab9c814..05fa654239f5 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>  	if (ret < 0)
>  		goto out;
>  
> +	devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
> +
>  	if (devfreq->profile->freq_table
>  		&& (devfreq_update_status(devfreq, freq)))
>  		dev_err(&devfreq->dev,
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 49cdb2378030..933692e5d867 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -196,6 +196,7 @@ struct devfreq {
>  };
>  
>  struct devfreq_freqs {
> +	s64 time;

Imho is should be moved to struct devfreq_transitions.
Or do you plan to change load calculations based on time ?

>  	unsigned long old;
>  	unsigned long new;
>  };
> 

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland

Re: [PATCH 1/2] PM / devfreq: Add debugfs support with devfreq_summary file

[Chanwoo Choi]

On 1/8/20 11:14 PM, Dmitry Osipenko wrote:
> 08.01.2020 10:56, Chanwoo Choi пишет:
>> On 1/8/20 6:15 AM, Bjorn Andersson wrote:
>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>
>>>> Add debugfs interface to provide debugging information of devfreq device.
>>>> It contains 'devfreq_summary' entry to show the summary of registered
>>>> devfreq devices as following and the additional debugfs file will be added.
>>>> - /sys/kernel/debug/devfreq/devfreq_summary
>>>>
>>>> [Detailed description of each field of 'devfreq_summary' debugfs file]
>>>> - dev_name	: Device name of h/w.
>>>> - dev		: Device name made by devfreq core.
>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>> 		  show parent devfreq device name.
>>>> - governor	: Devfreq governor.
>>>> - polling_ms	: If devfreq device uses the simple_ondemand governor,
>>>> 		  polling_ms is necessary for the period. (unit: millisecond)
>>>> - cur_freq_hz	: Current Frequency (unit: hz)
>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> - In order to show the multiple governors on devfreq_summay result,
>>>> change the governor of devfreq0 from simple_ondemand to userspace.
>>>>
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_summary
>>>> dev_name                       dev        parent_dev governor        polling_ms cur_freq_hz  min_freq_hz  max_freq_hz
>>>> ------------------------------ ---------- ---------- --------------- ---------- ------------ ------------ ------------
>>>> 10c20000.memory-controller     devfreq0              userspace       0          206000000    165000000    825000000
>>>> soc:bus_wcore                  devfreq1              simple_ondemand 50         532000000    88700000     532000000
>>>> soc:bus_noc                    devfreq2   devfreq1   passive         0          111000000    66600000     111000000
>>>> soc:bus_fsys_apb               devfreq3   devfreq1   passive         0          222000000    111000000    222000000
>>>> soc:bus_fsys                   devfreq4   devfreq1   passive         0          200000000    75000000     200000000
>>>> soc:bus_fsys2                  devfreq5   devfreq1   passive         0          200000000    75000000     200000000
>>>> soc:bus_mfc                    devfreq6   devfreq1   passive         0          333000000    83250000     333000000
>>>> soc:bus_gen                    devfreq7   devfreq1   passive         0          266000000    88700000     266000000
>>>> soc:bus_peri                   devfreq8   devfreq1   passive         0          66600000     66600000     66600000
>>>> soc:bus_g2d                    devfreq9   devfreq1   passive         0          0            83250000     333000000
>>>> soc:bus_g2d_acp                devfreq10  devfreq1   passive         0          0            66500000     266000000
>>>> soc:bus_jpeg                   devfreq11  devfreq1   passive         0          0            75000000     300000000
>>>> soc:bus_jpeg_apb               devfreq12  devfreq1   passive         0          0            83250000     166500000
>>>> soc:bus_disp1_fimd             devfreq13  devfreq1   passive         0          0            120000000    200000000
>>>> soc:bus_disp1                  devfreq14  devfreq1   passive         0          0            120000000    300000000
>>>> soc:bus_gscl_scaler            devfreq15  devfreq1   passive         0          0            150000000    300000000
>>>> soc:bus_mscl                   devfreq16  devfreq1   passive         0          0            84000000     666000000
>>>
>>> This looks quite useful.
>>>
>>>>
>>>> Reported-by: kbuild test robot <lkp@intel.com>
>>>
>>> May I ask how the build test robot came up with this idea?
>>
>> I'm missing the detailed what are the reported by kbuild test robot.
>> It reported the possible build error. I'll add the following description
>> on next version:
>> 	[lkp: Reported the build error]
>>
>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  drivers/devfreq/devfreq.c | 80 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 80 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> [..]
>>>> +/**
>>>> + * devfreq_summary_show() - Show the summary of the registered devfreq devices
>>>> + *				via 'devfreq_summary' debugfs file.
>>>
>>> Please make this proper kerneldoc, i.e:
>>>
>>>  * func() - short description
>>>  * @s:
>>>  * @data:
>>>  * 
>>>  * Long description
>>>  * 
>>>  * Return: foo on bar
>>
>> OK.
>>
>>>
>>> [..]
>>>> @@ -1733,6 +1803,16 @@ static int __init devfreq_init(void)
>>>>  	}
>>>>  	devfreq_class->dev_groups = devfreq_groups;
>>>>  
>>>> +	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
>>>> +	if (PTR_ERR(devfreq_debugfs) != -ENODEV && IS_ERR(devfreq_debugfs)) {
>>>
>>> Don't PTR_ERR() before IS_ERR().
>>
>> Before checking IS_ERR(), have to check the PTR_ERR(devfreq_debugfs)
>> is -ENODEV or not because debugfs_create_dir return the '-ENODEV'
>> if DEBUG_FS is disabled. If return the -ENODEV, it is not error.
> 
> You could write it this way:
> 
> 	devfreq_debugfs = debugfs_create_dir("devfreq", NULL);
> 	err = PTR_ERR_OR_ZERO(devfreq_debugfs);
> 	if (err && err != -ENODEV) {

It is same result between your suggestion and following statement'
without any separate local variable. 

	if (IS_ERR(devfreq_debugfs) && PTR_ERR(devfreq_debugfs) != -ENODEV)


> 	...
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Leonard Crestez]

On 08.01.2020 17:44, Lukasz Luba wrote:
> On 1/8/20 2:20 PM, Dmitry Osipenko wrote:
>> 08.01.2020 00:48, Bjorn Andersson пишет:
>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>> - dev_name	: Device name of h/w.
>>>> - dev		: Device name made by devfreq core.
>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>> 		  show parent devfreq device name.
>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>> 		  load is used by governor whene deciding the new frequency.
>>>> 		  (unit: percentage)
>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>> [snip]
>>>>
>>>
>>> Wouldn't it make more sense to expose this through the tracing
>>> framework - like many other subsystems does?
>>
>> I think devfreq core already has some tracing support and indeed it
>> should be better to extend it rather than duplicate.

+1 for tracing
> In my opinion this debugfs interface should be considered as a helpful
> validation entry point. We had some issues with wrong bootloader
> configurations in clock tree, where some frequencies could not be set
> in the kernel. Similar useful description can be find in clock subsystem
> where there is clock tree summary file.
> 
> It is much cheaper to poke a few files in debug dir by some automated
> test than starting tracing, provoking desired code flow in the
> devfreq for every device, paring the results... A simple boot test
> which reads only these new files can be enough to rise the flag.

Tracepoints are also very powerful for debugging boot issues! You can 
add "tp_printk trace_event=devfreq:*" to boot arguments and you will see 
console messages for all relevant events. This works even if boot fails 
before userspace is available to mount debugfs.

> Secondly the tracing is not always compiled.

Tracing is deliberately light-weight and should be enabled even on 
production systems.

> It could capture old/wrong bootloaders which pinned devices
> improperly to PLLs or wrong DT values in OPP table.
> (a workaround for Odroid xu4 patchset:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F7%2F15%2F276&data=02%7C01%7Cleonard.crestez%40nxp.com%7C8397d37b41474137f8cf08d79451a007%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637140950611913278&sdata=rcbWCyFmf0ZO7LU27D05mftTf8YdSvGPYNsst1GnNjQ%3D&reserved=0
> )
> 
> Chanwoo what do think about some sanity check summary?
> It could be presented in a 3rd file: 'devfreq_sanity', which
> could report if the devices could set their registered OPPs
> and got the same values, i.e. set 166MHz --> set to 150MHz
> in reality. If a config option i.e. DEVFREQ_SANITY is set
> then during the registration of a new device it checks OPPs
> if they are possible to set. It could be done before assigning
> the governor for the device and results present in of of your files.

The new devfreq_transition tracepoint could include a field for 
"new_effective freq" next to "old_freq" and "new_requested_freq".

For imx8m-ddrc I handled this inside the target() function: clk_get_rate 
is called after the transition and an error is reported if rate doesn't 
match.

It might make sense for devfreq core to handle this internally by 
calling get_cur_freq instead.

--
Regards,
Leonard

Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Lukasz Luba]

Hi Leonard,

On 1/13/20 5:19 PM, Leonard Crestez wrote:
> On 08.01.2020 17:44, Lukasz Luba wrote:
>> On 1/8/20 2:20 PM, Dmitry Osipenko wrote:
>>> 08.01.2020 00:48, Bjorn Andersson пишет:
>>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>>
>>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>>> of all devfreq devices for the simple profiling as following:
>>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>>
>>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>>> in Kconfig in order to save the transition history.
>>>>>
>>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>>> - time_ms	: Change time of frequency transition. (unit: millisecond)
>>>>> - dev_name	: Device name of h/w.
>>>>> - dev		: Device name made by devfreq core.
>>>>> - parent_dev	: If devfreq device uses the passive governor,
>>>>> 		  show parent devfreq device name.
>>>>> - load_%	: If devfreq device uses the simple_ondemand governor,
>>>>> 		  load is used by governor whene deciding the new frequency.
>>>>> 		  (unit: percentage)
>>>>> - old_freq_hz	: Frequency before changing. (unit: hz)
>>>>> - new_freq_hz	: Frequency after changed. (unit: hz)
>>>>>
>>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>>> time_ms    dev_name                       dev        parent_dev load_% old_freq_hz  new_freq_hz
>>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>>> 14600      soc:bus_noc                    devfreq2   devfreq1   0      100000000    67000000
>>>>> 14600      soc:bus_fsys_apb               devfreq3   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys                   devfreq4   devfreq1   0      200000000    100000000
>>>>> 14600      soc:bus_fsys2                  devfreq5   devfreq1   0      150000000    75000000
>>>>> 14602      soc:bus_mfc                    devfreq6   devfreq1   0      222000000    96000000
>>>>> 14602      soc:bus_gen                    devfreq7   devfreq1   0      267000000    89000000
>>>>> 14602      soc:bus_g2d                    devfreq9   devfreq1   0      300000000    84000000
>>>>> 14602      soc:bus_g2d_acp                devfreq10  devfreq1   0      267000000    67000000
>>>>> 14602      soc:bus_jpeg                   devfreq11  devfreq1   0      300000000    75000000
>>>>> 14602      soc:bus_jpeg_apb               devfreq12  devfreq1   0      167000000    84000000
>>>>> 14603      soc:bus_disp1_fimd             devfreq13  devfreq1   0      200000000    120000000
>>>>> 14603      soc:bus_disp1                  devfreq14  devfreq1   0      300000000    120000000
>>>>> 14606      soc:bus_gscl_scaler            devfreq15  devfreq1   0      300000000    150000000
>>>>> 14606      soc:bus_mscl                   devfreq16  devfreq1   0      333000000    84000000
>>>>> 14608      soc:bus_wcore                  devfreq1              9      333000000    84000000
>>>>> 14783      10c20000.memory-controller     devfreq0              35     825000000    633000000
>>>>> 15873      soc:bus_wcore                  devfreq1              41     84000000     400000000
>>>>> 15873      soc:bus_noc                    devfreq2   devfreq1   0      67000000     100000000
>>>>> [snip]
>>>>>
>>>>
>>>> Wouldn't it make more sense to expose this through the tracing
>>>> framework - like many other subsystems does?
>>>
>>> I think devfreq core already has some tracing support and indeed it
>>> should be better to extend it rather than duplicate.
> 
> +1 for tracing
>> In my opinion this debugfs interface should be considered as a helpful
>> validation entry point. We had some issues with wrong bootloader
>> configurations in clock tree, where some frequencies could not be set
>> in the kernel. Similar useful description can be find in clock subsystem
>> where there is clock tree summary file.
>>
>> It is much cheaper to poke a few files in debug dir by some automated
>> test than starting tracing, provoking desired code flow in the
>> devfreq for every device, paring the results... A simple boot test
>> which reads only these new files can be enough to rise the flag.
> 
> Tracepoints are also very powerful for debugging boot issues! You can
> add "tp_printk trace_event=devfreq:*" to boot arguments and you will see
> console messages for all relevant events. This works even if boot fails
> before userspace is available to mount debugfs.
> 
>> Secondly the tracing is not always compiled.
> 
> Tracing is deliberately light-weight and should be enabled even on
> production systems.
> 
>> It could capture old/wrong bootloaders which pinned devices
>> improperly to PLLs or wrong DT values in OPP table.
>> (a workaround for Odroid xu4 patchset:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F7%2F15%2F276&data=02%7C01%7Cleonard.crestez%40nxp.com%7C8397d37b41474137f8cf08d79451a007%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637140950611913278&sdata=rcbWCyFmf0ZO7LU27D05mftTf8YdSvGPYNsst1GnNjQ%3D&reserved=0
>> )
>>
>> Chanwoo what do think about some sanity check summary?
>> It could be presented in a 3rd file: 'devfreq_sanity', which
>> could report if the devices could set their registered OPPs
>> and got the same values, i.e. set 166MHz --> set to 150MHz
>> in reality. If a config option i.e. DEVFREQ_SANITY is set
>> then during the registration of a new device it checks OPPs
>> if they are possible to set. It could be done before assigning
>> the governor for the device and results present in of of your files.
> 
> The new devfreq_transition tracepoint could include a field for
> "new_effective freq" next to "old_freq" and "new_requested_freq".

I would suggest to keep it aligned with cpufreq trace. The timestamps
in trace would tell you the history, 'old_freq' is not needed.
The trace_devfreq_monitor that I have added should give you this
information when you parse all the events.

> 
> For imx8m-ddrc I handled this inside the target() function: clk_get_rate
> is called after the transition and an error is reported if rate doesn't
> match.

Interesting driver, it uses ARM SMCCC like rk3399.
It handles this validation of DT OPPs vs firmware OPPs and disables
not matched frequencies. Small nit. The error is printed when the 'ret'
is 0 and freq does not match, but then 'ret' is returned from target().
You don't also revert the update parent stuff in such case.
Maybe you can also check the res.a0 != SMCCC_RET_SUCCESS in
static void imx8m_ddrc_smc_set_freq(int target_freq)
to bail out earlier and not switch to new parents when the freq
switch failed or does not match. Or I am missing something.
You can also reorder the includes alphabetically.

Regards,
Lukasz

> 
> It might make sense for devfreq core to handle this internally by
> calling get_cur_freq instead.
> 
> --
> Regards,
> Leonard
>