2020-10-08 07:45:53

by Chanwoo Choi

[permalink] [raw]
Subject: [PATCH 0/3] PM / devfreq: Add devfreq_frequency tracepoint to track frequency change

Add devfreq_tracepoint to track the correct timing of frequency change
with following information:
- device name
- current frequency
- previous frequency
- load when change frequency
- tracepoint path : /sys/kernel/debug/tracing/events/devfreq_frequency

And add devfreq_update_target() function to unify the frequency change code
on both devfreq core and devfreq passive governor because there are redundant
duplicate code. Lastly, Use fixed indentation size to improve readability
for 'devfreq_monitor' tracepoint.

Matthias already sent the patch[1]. Make patch3 by editing patch[1].
[1]https://www.mail-archive.com/[email protected]/msg2108015.html

Chanwoo Choi (2):
trace: events: devfreq: Use fixed indentation size to improve readability
PM / devfreq: Unify frequency change to devfreq_update_target func

Matthias Kaehlcke (1):
PM / devfreq: Add tracepoint for frequency changes

drivers/devfreq/devfreq.c | 37 +++++++++++++++++++++-----
drivers/devfreq/governor.h | 1 +
drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
include/trace/events/devfreq.h | 30 ++++++++++++++++++++-
4 files changed, 70 insertions(+), 40 deletions(-)

--
2.17.1


2020-10-08 08:58:22

by Chanwoo Choi

[permalink] [raw]
Subject: [PATCH 2/3] PM / devfreq: Unify frequency change to devfreq_update_target func

The update_devfreq() and update_passive_devfreq() have the duplicate
code when changing the target frequency on final stage. So, unify
frequency change code to devfreq_update_target() to remove the
duplicate code and to centralize the frequency change code.

Signed-off-by: Chanwoo Choi <[email protected]>
---
drivers/devfreq/devfreq.c | 29 ++++++++++++++++-----
drivers/devfreq/governor.h | 1 +
drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
3 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 379aaaabf25d..5b069a8a1026 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -384,18 +384,19 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
return err;
}

-/* Load monitoring helper functions for governors use */
-
/**
- * update_devfreq() - Reevaluate the device and configure frequency.
+ * devfreq_update_target() - Reevaluate the device and configure frequency
+ * on the final stage.
* @devfreq: the devfreq instance.
+ * @freq: the new frequency of parent device. This argument
+ * is only used for devfreq device using passive governor.
*
- * Note: Lock devfreq->lock before calling update_devfreq
- * This function is exported for governors.
+ * Note: Lock devfreq->lock before calling devfreq_update_target. This function
+ * should be only used by both update_devfreq() and devfreq governors.
*/
-int update_devfreq(struct devfreq *devfreq)
+int devfreq_update_target(struct devfreq *devfreq, unsigned long freq)
{
- unsigned long freq, min_freq, max_freq;
+ unsigned long min_freq, max_freq;
int err = 0;
u32 flags = 0;

@@ -420,7 +421,21 @@ int update_devfreq(struct devfreq *devfreq)
}

return devfreq_set_target(devfreq, freq, flags);
+}
+EXPORT_SYMBOL(devfreq_update_target);
+
+/* Load monitoring helper functions for governors use */

+/**
+ * update_devfreq() - Reevaluate the device and configure frequency.
+ * @devfreq: the devfreq instance.
+ *
+ * Note: Lock devfreq->lock before calling update_devfreq
+ * This function is exported for governors.
+ */
+int update_devfreq(struct devfreq *devfreq)
+{
+ return devfreq_update_target(devfreq, 0L);
}
EXPORT_SYMBOL(update_devfreq);

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index eb6392d397b3..871150be4391 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -85,6 +85,7 @@ int devfreq_add_governor(struct devfreq_governor *governor);
int devfreq_remove_governor(struct devfreq_governor *governor);

int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
+int devfreq_update_target(struct devfreq *devfreq, unsigned long freq);

static inline int devfreq_update_stats(struct devfreq *df)
{
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 432a4cc683f7..8deb071d5d26 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -92,36 +92,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
return ret;
}

-static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
-{
- int ret;
-
- if (!devfreq->governor)
- return -EINVAL;
-
- mutex_lock_nested(&devfreq->lock, SINGLE_DEPTH_NESTING);
-
- ret = devfreq->governor->get_target_freq(devfreq, &freq);
- if (ret < 0)
- goto out;
-
- ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
- if (ret < 0)
- goto out;
-
- if (devfreq->profile->freq_table
- && (devfreq_update_status(devfreq, freq)))
- dev_err(&devfreq->dev,
- "Couldn't update frequency transition information.\n");
-
- devfreq->previous_freq = freq;
-
-out:
- mutex_unlock(&devfreq->lock);
-
- return 0;
-}
-
static int devfreq_passive_notifier_call(struct notifier_block *nb,
unsigned long event, void *ptr)
{
@@ -131,17 +101,25 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
struct devfreq *parent = (struct devfreq *)data->parent;
struct devfreq_freqs *freqs = (struct devfreq_freqs *)ptr;
unsigned long freq = freqs->new;
+ int ret = 0;

+ mutex_lock_nested(&devfreq->lock, SINGLE_DEPTH_NESTING);
switch (event) {
case DEVFREQ_PRECHANGE:
if (parent->previous_freq > freq)
- update_devfreq_passive(devfreq, freq);
+ ret = devfreq_update_target(devfreq, freq);
+
break;
case DEVFREQ_POSTCHANGE:
if (parent->previous_freq < freq)
- update_devfreq_passive(devfreq, freq);
+ ret = devfreq_update_target(devfreq, freq);
break;
}
+ mutex_unlock(&devfreq->lock);
+
+ if (ret < 0)
+ dev_warn(&devfreq->dev,
+ "failed to update devfreq using passive governor\n");

return NOTIFY_DONE;
}
--
2.17.1

2020-10-19 22:51:44

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH 0/3] PM / devfreq: Add devfreq_frequency tracepoint to track frequency change

On 10/8/20 4:54 PM, Chanwoo Choi wrote:
> Add devfreq_tracepoint to track the correct timing of frequency change
> with following information:
> - device name
> - current frequency
> - previous frequency
> - load when change frequency
> - tracepoint path : /sys/kernel/debug/tracing/events/devfreq_frequency
>
> And add devfreq_update_target() function to unify the frequency change code
> on both devfreq core and devfreq passive governor because there are redundant
> duplicate code. Lastly, Use fixed indentation size to improve readability
> for 'devfreq_monitor' tracepoint.
>
> Matthias already sent the patch[1]. Make patch3 by editing patch[1].
> [1]https://www.mail-archive.com/[email protected]/msg2108015.html
>
> Chanwoo Choi (2):
> trace: events: devfreq: Use fixed indentation size to improve readability
> PM / devfreq: Unify frequency change to devfreq_update_target func
>
> Matthias Kaehlcke (1):
> PM / devfreq: Add tracepoint for frequency changes
>
> drivers/devfreq/devfreq.c | 37 +++++++++++++++++++++-----
> drivers/devfreq/governor.h | 1 +
> drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
> include/trace/events/devfreq.h | 30 ++++++++++++++++++++-
> 4 files changed, 70 insertions(+), 40 deletions(-)
>

Applied them. Thanks.

--
Best Regards,
Chanwoo Choi
Samsung Electronics