Re: [PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

Re: [PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

[Hsin-Yi Wang]

On Thu, Jun 17, 2021 at 1:46 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
>
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
>   the parent cpu opp_table.
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
>   the CPUs are running at their max frequency, the device runs at its
>   max frequency. If the CPUs are running at their min frequency, the
>   device runs at its min frequency. It is interpolated for frequencies
>   in between.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> [Chanwoo: Fix conflict with latest code and clean code up]
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/governor.h         |  22 +++
>  drivers/devfreq/governor_passive.c | 264 ++++++++++++++++++++++++++++-
>  include/linux/devfreq.h            |  16 +-
>  3 files changed, 293 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index 9a9495f94ac6..3c36c92c89a9 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -47,6 +47,28 @@
>  #define DEVFREQ_GOV_ATTR_POLLING_INTERVAL              BIT(0)
>  #define DEVFREQ_GOV_ATTR_TIMER                         BIT(1)
>
> +/**
> + * struct devfreq_cpu_data - Hold the per-cpu data
> + * @dev:       reference to cpu device.
> + * @first_cpu: the cpumask of the first cpu of a policy.
> + * @opp_table: reference to cpu opp table.
> + * @cur_freq:  the current frequency of the cpu.
> + * @min_freq:  the min frequency of the cpu.
> + * @max_freq:  the max frequency of the cpu.
> + *
> + * This structure stores the required cpu_data of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_data {
> +       struct device *dev;
> +       unsigned int first_cpu;
> +
> +       struct opp_table *opp_table;
> +       unsigned int cur_freq;
> +       unsigned int min_freq;
> +       unsigned int max_freq;
> +};
> +
>  /**
>   * struct devfreq_governor - Devfreq policy governor
>   * @node:              list node - contains registered devfreq governors
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index fc09324a03e0..07e864509b7e 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,84 @@
>   */
>
>  #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/devfreq.h>
>  #include "governor.h"
>
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +#define HZ_PER_KHZ     1000
> +
> +static unsigned long get_taget_freq_by_required_opp(struct device *p_dev,
> +                                               struct opp_table *p_opp_table,
> +                                               struct opp_table *opp_table,
> +                                               unsigned long freq)
> +{
> +       struct dev_pm_opp *opp = NULL, *p_opp = NULL;
> +
> +       if (!p_dev || !p_opp_table || !opp_table || !freq)
> +               return 0;
> +
> +       p_opp = devfreq_recommended_opp(p_dev, &freq, 0);
> +       if (IS_ERR(p_opp))
> +               return 0;
> +
> +       opp = dev_pm_opp_xlate_required_opp(p_opp_table, opp_table, p_opp);
> +       dev_pm_opp_put(p_opp);
> +
> +       if (IS_ERR(opp))
> +               return 0;
> +
> +       freq = dev_pm_opp_get_freq(opp);
> +       dev_pm_opp_put(opp);
> +
> +       return freq;
> +}
> +
> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> +                                       unsigned long *target_freq)
> +{
> +       struct devfreq_passive_data *p_data =
> +                               (struct devfreq_passive_data *)devfreq->data;
> +       struct devfreq_cpu_data *cpu_data;
We might have to rename the cpu_data variable.

For some architectures, cpu_data is defined as macro. This results in
errors such as

include/linux/devfreq.h:331:27: note: in expansion of macro 'cpu_data'
     331 |  struct devfreq_cpu_data *cpu_data[NR_CPUS];
         |                           ^~~~~~~~
   In file included from include/linux/devfreq_cooling.h:13,
                    from drivers/devfreq/devfreq.c:14:
   include/linux/devfreq.h:332:1: warning: no semicolon at end of
struct or union

> +       unsigned long cpu, cpu_cur, cpu_min, cpu_max, cpu_percent;
> +       unsigned long dev_min, dev_max;
> +       unsigned long freq = 0;
> +
> +       for_each_online_cpu(cpu) {
> +               cpu_data = p_data->cpu_data[cpu];
> +               if (!cpu_data || cpu_data->first_cpu != cpu)
> +                       continue;
> +
> +               /* Get target freq via required opps */
> +               cpu_cur = cpu_data->cur_freq * HZ_PER_KHZ;
> +               freq = get_taget_freq_by_required_opp(cpu_data->dev,
> +                                       cpu_data->opp_table,
> +                                       devfreq->opp_table, cpu_cur);
> +               if (freq) {
> +                       *target_freq = max(freq, *target_freq);
> +                       continue;
> +               }
> +
> +               /* Use Interpolation if required opps is not available */
> +               devfreq_get_freq_range(devfreq, &dev_min, &dev_max);
> +
> +               cpu_min = cpu_data->min_freq;
> +               cpu_max = cpu_data->max_freq;
> +               cpu_cur = cpu_data->cur_freq;
> +
> +               cpu_percent = ((cpu_cur - cpu_min) * 100) / (cpu_max - cpu_min);
> +               freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> +
> +               *target_freq = max(freq, *target_freq);
> +       }
> +
> +       return 0;
> +}
> +
> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>                                         unsigned long *freq)
>  {
>         struct devfreq_passive_data *p_data
> @@ -99,6 +172,172 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>         return 0;
>  }
>
> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +                                          unsigned long *freq)
> +{
> +       struct devfreq_passive_data *p_data =
> +                               (struct devfreq_passive_data *)devfreq->data;
> +       int ret;
> +
> +       if (!p_data)
> +               return -EINVAL;
> +
> +       /*
> +        * If the devfreq device with passive governor has the specific method
> +        * to determine the next frequency, should use the get_target_freq()
> +        * of struct devfreq_passive_data.
> +        */
> +       if (p_data->get_target_freq)
> +               return p_data->get_target_freq(devfreq, freq);
> +
> +       switch (p_data->parent_type) {
> +       case DEVFREQ_PARENT_DEV:
> +               ret = get_target_freq_with_devfreq(devfreq, freq);
> +               break;
> +       case CPUFREQ_PARENT_DEV:
> +               ret = get_target_freq_with_cpufreq(devfreq, freq);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               dev_err(&devfreq->dev, "Invalid parent type\n");
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> +                                        unsigned long event, void *ptr)
> +{
> +       struct devfreq_passive_data *data =
> +                       container_of(nb, struct devfreq_passive_data, nb);
> +       struct devfreq *devfreq = (struct devfreq *)data->this;
> +       struct devfreq_cpu_data *cpu_data;
> +       struct cpufreq_freqs *freqs = ptr;
> +       unsigned int cur_freq;
> +       int ret;
> +
> +       if (event != CPUFREQ_POSTCHANGE || !freqs ||
> +               !data->cpu_data[freqs->policy->cpu])
> +               return 0;
> +
> +       cpu_data = data->cpu_data[freqs->policy->cpu];
> +       if (cpu_data->cur_freq == freqs->new)
> +               return 0;
> +
> +       cur_freq = cpu_data->cur_freq;
> +       cpu_data->cur_freq = freqs->new;
> +
> +       mutex_lock(&devfreq->lock);
> +       ret = devfreq_update_target(devfreq, freqs->new);
> +       mutex_unlock(&devfreq->lock);
> +       if (ret) {
> +               cpu_data->cur_freq = cur_freq;
> +               dev_err(&devfreq->dev, "failed to update the frequency.\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
> +{
> +       struct devfreq_passive_data *p_data
> +                       = (struct devfreq_passive_data *)devfreq->data;
> +       struct device *dev = devfreq->dev.parent;
> +       struct opp_table *opp_table = NULL;
> +       struct devfreq_cpu_data *cpu_data;
> +       struct cpufreq_policy *policy;
> +       struct device *cpu_dev;
> +       unsigned int cpu;
> +       int ret;
> +
> +       get_online_cpus();
> +
> +       p_data->nb.notifier_call = cpufreq_passive_notifier_call;
> +       ret = cpufreq_register_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
> +       if (ret) {
> +               dev_err(dev, "failed to register cpufreq notifier\n");
> +               p_data->nb.notifier_call = NULL;
> +               goto out;
> +       }
> +
> +       for_each_online_cpu(cpu) {
> +               if (p_data->cpu_data[cpu])
> +                       continue;
> +
> +               policy = cpufreq_cpu_get(cpu);
> +               if (policy) {
> +                       cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
> +                       if (!cpu_data) {
> +                               ret = -ENOMEM;
> +                               goto out;
> +                       }
> +
> +                       cpu_dev = get_cpu_device(cpu);
> +                       if (!cpu_dev) {
> +                               dev_err(dev, "failed to get cpu device\n");
> +                               ret = -ENODEV;
> +                               goto out;
> +                       }
> +
> +                       opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> +                       if (IS_ERR(opp_table)) {
> +                               ret = PTR_ERR(opp_table);
> +                               goto out;
> +                       }
> +
> +                       cpu_data->dev = cpu_dev;
> +                       cpu_data->opp_table = opp_table;
> +                       cpu_data->first_cpu = cpumask_first(policy->related_cpus);
> +                       cpu_data->cur_freq = policy->cur;
> +                       cpu_data->min_freq = policy->cpuinfo.min_freq;
> +                       cpu_data->max_freq = policy->cpuinfo.max_freq;
> +
> +                       p_data->cpu_data[cpu] = cpu_data;
> +                       cpufreq_cpu_put(policy);
> +               } else {
> +                       ret = -EPROBE_DEFER;
> +                       goto out;
> +               }
> +       }
> +out:
> +       put_online_cpus();
> +       if (ret)
> +               return ret;
> +
> +       mutex_lock(&devfreq->lock);
> +       ret = devfreq_update_target(devfreq, 0L);
> +       mutex_unlock(&devfreq->lock);
> +       if (ret)
> +               dev_err(dev, "failed to update the frequency\n");
> +
> +       return ret;
> +}
> +
> +static int cpufreq_passive_unregister_notifier(struct devfreq *devfreq)
> +{
> +       struct devfreq_passive_data *p_data
> +                       = (struct devfreq_passive_data *)devfreq->data;
> +       struct devfreq_cpu_data *cpu_data;
> +       int cpu;
> +
> +       if (p_data->nb.notifier_call)
> +               cpufreq_unregister_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
> +
> +       for_each_possible_cpu(cpu) {
> +               cpu_data = p_data->cpu_data[cpu];
> +               if (cpu_data) {
> +                       if (cpu_data->opp_table)
> +                               dev_pm_opp_put_opp_table(cpu_data->opp_table);
> +                       kfree(cpu_data);
> +                       cpu_data = NULL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int devfreq_passive_notifier_call(struct notifier_block *nb,
>                                 unsigned long event, void *ptr)
>  {
> @@ -140,7 +379,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>         struct notifier_block *nb = &p_data->nb;
>         int ret = 0;
>
> -       if (!parent)
> +       if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
>                 return -EPROBE_DEFER;
>
>         switch (event) {
> @@ -148,13 +387,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>                 if (!p_data->this)
>                         p_data->this = devfreq;
>
> -               nb->notifier_call = devfreq_passive_notifier_call;
> -               ret = devfreq_register_notifier(parent, nb,
> -                                       DEVFREQ_TRANSITION_NOTIFIER);
> +               if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> +                       nb->notifier_call = devfreq_passive_notifier_call;
> +                       ret = devfreq_register_notifier(parent, nb,
> +                                               DEVFREQ_TRANSITION_NOTIFIER);
> +               } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> +                       ret = cpufreq_passive_register_notifier(devfreq);
> +               } else {
> +                       ret = -EINVAL;
> +               }
>                 break;
>         case DEVFREQ_GOV_STOP:
> -               WARN_ON(devfreq_unregister_notifier(parent, nb,
> -                                       DEVFREQ_TRANSITION_NOTIFIER));
> +               if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> +                       WARN_ON(devfreq_unregister_notifier(parent, nb,
> +                                               DEVFREQ_TRANSITION_NOTIFIER));
> +               else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> +                       WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
> +               else
> +                       ret = -EINVAL;
>                 break;
>         default:
>                 break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 142474b4af96..cfa0ef54841e 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -38,6 +38,7 @@ enum devfreq_timer {
>
>  struct devfreq;
>  struct devfreq_governor;
> +struct devfreq_cpu_data;
>  struct thermal_cooling_device;
>
>  /**
> @@ -288,6 +289,11 @@ struct devfreq_simple_ondemand_data {
>  #endif
>
>  #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +enum devfreq_parent_dev_type {
> +       DEVFREQ_PARENT_DEV,
> +       CPUFREQ_PARENT_DEV,
> +};
> +
>  /**
>   * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
>   *     and devfreq_add_device
> @@ -299,8 +305,10 @@ struct devfreq_simple_ondemand_data {
>   *                     using governors except for passive governor.
>   *                     If the devfreq device has the specific method to decide
>   *                     the next frequency, should use this callback.
> - * @this:      the devfreq instance of own device.
> - * @nb:                the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + + * @parent_type      parent type of the device
> + + * @this:            the devfreq instance of own device.
> + + * @nb:              the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + + * @cpu_data:                the state min/max/current frequency of all online cpu's
>   *
>   * The devfreq_passive_data have to set the devfreq instance of parent
>   * device with governors except for the passive governor. But, don't need to
> @@ -314,9 +322,13 @@ struct devfreq_passive_data {
>         /* Optional callback to decide the next frequency of passvice device */
>         int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>
> +       /* Should set the type of parent device */
> +       enum devfreq_parent_dev_type parent_type;
> +
>         /* For passive governor's internal use. Don't need to set them */
>         struct devfreq *this;
>         struct notifier_block nb;
> +       struct devfreq_cpu_data *cpu_data[NR_CPUS];
>  };
>  #endif
>
> --
> 2.17.1
>

[PATCH 0/4] PM / devfreq: Add cpu based scaling support to passive governor

[Chanwoo Choi]

The devfreq passive governor has already supported the devfreq parent device
for coupling the frequency change if some hardware have the constraints
such as power sharing and so on.

Add cpu based scaling support to passive governor with required-opp property.
It uses the cpufreq notifier to catch the frequency change timing of cpufreq
and get the next frequency according to new cpu frequency by using required-opp
property. It is based on patch[1] and then just code clean-up by myself.

Make the common code for both passive_devfreq and passive_cpufreq
parent type to remove the duplicate code.

The patch[2] is required for this patchset to use required-opp property.

[1] [RFC,v2] PM / devfreq: Add cpu based scaling support to passive_governor
- https://lore.kernel.org/patchwork/patch/1101049/
[2] opp: Allow required-opps to be used for non genpd use cases
- https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/commit/?h=opp/linux-next&id=5736929761d187499bdf8e2019090e3ed43d3d7b


Dear Andrew-sh.Cheng and Hsin-Yi Wang,
Please test your patches based on this patchset and then reply the test result
with your Tested-by tag or reviewed-by tag. Thanks for your work.

Chanwoo Choi (3):
  PM / devfreq: passive: Fix get_target_freq when not using required-opp
  PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq
  PM / devfreq: passive: Reduce duplicate code when passive_devfreq case

Saravana Kannan (1):
  PM / devfreq: Add cpu based scaling support to passive governor

 drivers/devfreq/devfreq.c          |  17 +-
 drivers/devfreq/governor.h         |  24 +++
 drivers/devfreq/governor_passive.c | 325 +++++++++++++++++++++++------
 include/linux/devfreq.h            |  16 +-
 4 files changed, 310 insertions(+), 72 deletions(-)

-- 
2.17.1

[PATCH 1/4] PM / devfreq: passive: Fix get_target_freq when not using required-opp

[Chanwoo Choi]

The 86ad9a24f21e ("PM / devfreq: Add required OPPs support to passive governor")
supported the required-opp property for using devfreq passive governor.
But, 86ad9a24f21e has caused the problem on use-case when required-opp
is not used such as exynos-bus.c devfreq driver. So that fix the
get_target_freq of passive governor for supporting the case of when
required-opp is not used.

Cc: stable@vger.kernel.org
Fixes: 86ad9a24f21e ("PM / devfreq: Add required OPPs support to passive governor")
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/governor_passive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index b094132bd20b..fc09324a03e0 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -65,7 +65,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 		dev_pm_opp_put(p_opp);
 
 		if (IS_ERR(opp))
-			return PTR_ERR(opp);
+			goto no_required_opp;
 
 		*freq = dev_pm_opp_get_freq(opp);
 		dev_pm_opp_put(opp);
@@ -73,6 +73,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 		return 0;
 	}
 
+no_required_opp:
 	/*
 	 * Get the OPP table's index of decided frequency by governor
 	 * of parent device.
-- 
2.17.1

[PATCH 2/4] PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq

[Chanwoo Choi]

In order to get frequency range within devfreq governors,
export devfreq_get_freq_ragne symbol within devfreq.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c  | 17 +++++++++--------
 drivers/devfreq/governor.h |  2 ++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 28f3e0ba6cdd..a15545c42fc2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -112,16 +112,16 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 }
 
 /**
- * get_freq_range() - Get the current freq range
+ * devfreq_get_freq_range() - Get the current freq range
  * @devfreq:	the devfreq instance
  * @min_freq:	the min frequency
  * @max_freq:	the max frequency
  *
  * This takes into consideration all constraints.
  */
-static void get_freq_range(struct devfreq *devfreq,
-			   unsigned long *min_freq,
-			   unsigned long *max_freq)
+void devfreq_get_freq_range(struct devfreq *devfreq,
+			    unsigned long *min_freq,
+			    unsigned long *max_freq)
 {
 	unsigned long *freq_table = devfreq->profile->freq_table;
 	s32 qos_min_freq, qos_max_freq;
@@ -158,6 +158,7 @@ static void get_freq_range(struct devfreq *devfreq,
 	if (*min_freq > *max_freq)
 		*min_freq = *max_freq;
 }
+EXPORT_SYMBOL(devfreq_get_freq_range);
 
 /**
  * devfreq_get_freq_level() - Lookup freq_table for the frequency
@@ -418,7 +419,7 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned long freq)
 	err = devfreq->governor->get_target_freq(devfreq, &freq);
 	if (err)
 		return err;
-	get_freq_range(devfreq, &min_freq, &max_freq);
+	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
 
 	if (freq < min_freq) {
 		freq = min_freq;
@@ -1561,7 +1562,7 @@ static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 	unsigned long min_freq, max_freq;
 
 	mutex_lock(&df->lock);
-	get_freq_range(df, &min_freq, &max_freq);
+	devfreq_get_freq_range(df, &min_freq, &max_freq);
 	mutex_unlock(&df->lock);
 
 	return sprintf(buf, "%lu\n", min_freq);
@@ -1615,7 +1616,7 @@ static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 	unsigned long min_freq, max_freq;
 
 	mutex_lock(&df->lock);
-	get_freq_range(df, &min_freq, &max_freq);
+	devfreq_get_freq_range(df, &min_freq, &max_freq);
 	mutex_unlock(&df->lock);
 
 	return sprintf(buf, "%lu\n", max_freq);
@@ -1929,7 +1930,7 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
 
 		mutex_lock(&devfreq->lock);
 		cur_freq = devfreq->previous_freq;
-		get_freq_range(devfreq, &min_freq, &max_freq);
+		devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
 		timer = devfreq->profile->timer;
 
 		if (IS_SUPPORTED_ATTR(devfreq->governor->attrs, POLLING_INTERVAL))
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 2d69a0ce6291..9a9495f94ac6 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -86,6 +86,8 @@ 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);
+void devfreq_get_freq_range(struct devfreq *devfreq, unsigned long *min_freq,
+			    unsigned long *max_freq);
 
 static inline int devfreq_update_stats(struct devfreq *df)
 {
-- 
2.17.1

[PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

[Chanwoo Choi]

From: Saravana Kannan <skannan@codeaurora.org>

Many CPU architectures have caches that can scale independent of the
CPUs. Frequency scaling of the caches is necessary to make sure that the
cache is not a performance bottleneck that leads to poor performance and
power. The same idea applies for RAM/DDR.

To achieve this, this patch adds support for cpu based scaling to the
passive governor. This is accomplished by taking the current frequency
of each CPU frequency domain and then adjust the frequency of the cache
(or any devfreq device) based on the frequency of the CPUs. It listens
to CPU frequency transition notifiers to keep itself up to date on the
current CPU frequency.

To decide the frequency of the device, the governor does one of the
following:
* Derives the optimal devfreq device opp from required-opps property of
  the parent cpu opp_table.

* Scales the device frequency in proportion to the CPU frequency. So, if
  the CPUs are running at their max frequency, the device runs at its
  max frequency. If the CPUs are running at their min frequency, the
  device runs at its min frequency. It is interpolated for frequencies
  in between.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
[Sibi: Integrated cpu-freqmap governor into passive_governor]
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
[Chanwoo: Fix conflict with latest code and clean code up]
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/governor.h         |  22 +++
 drivers/devfreq/governor_passive.c | 264 ++++++++++++++++++++++++++++-
 include/linux/devfreq.h            |  16 +-
 3 files changed, 293 insertions(+), 9 deletions(-)

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 9a9495f94ac6..3c36c92c89a9 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -47,6 +47,28 @@
 #define DEVFREQ_GOV_ATTR_POLLING_INTERVAL		BIT(0)
 #define DEVFREQ_GOV_ATTR_TIMER				BIT(1)
 
+/**
+ * struct devfreq_cpu_data - Hold the per-cpu data
+ * @dev:	reference to cpu device.
+ * @first_cpu:	the cpumask of the first cpu of a policy.
+ * @opp_table:	reference to cpu opp table.
+ * @cur_freq:	the current frequency of the cpu.
+ * @min_freq:	the min frequency of the cpu.
+ * @max_freq:	the max frequency of the cpu.
+ *
+ * This structure stores the required cpu_data of a cpu.
+ * This is auto-populated by the governor.
+ */
+struct devfreq_cpu_data {
+	struct device *dev;
+	unsigned int first_cpu;
+
+	struct opp_table *opp_table;
+	unsigned int cur_freq;
+	unsigned int min_freq;
+	unsigned int max_freq;
+};
+
 /**
  * struct devfreq_governor - Devfreq policy governor
  * @node:		list node - contains registered devfreq governors
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index fc09324a03e0..07e864509b7e 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -8,11 +8,84 @@
  */
 
 #include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/cpumask.h>
+#include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/devfreq.h>
 #include "governor.h"
 
-static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+#define HZ_PER_KHZ	1000
+
+static unsigned long get_taget_freq_by_required_opp(struct device *p_dev,
+						struct opp_table *p_opp_table,
+						struct opp_table *opp_table,
+						unsigned long freq)
+{
+	struct dev_pm_opp *opp = NULL, *p_opp = NULL;
+
+	if (!p_dev || !p_opp_table || !opp_table || !freq)
+		return 0;
+
+	p_opp = devfreq_recommended_opp(p_dev, &freq, 0);
+	if (IS_ERR(p_opp))
+		return 0;
+
+	opp = dev_pm_opp_xlate_required_opp(p_opp_table, opp_table, p_opp);
+	dev_pm_opp_put(p_opp);
+
+	if (IS_ERR(opp))
+		return 0;
+
+	freq = dev_pm_opp_get_freq(opp);
+	dev_pm_opp_put(opp);
+
+	return freq;
+}
+
+static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
+					unsigned long *target_freq)
+{
+	struct devfreq_passive_data *p_data =
+				(struct devfreq_passive_data *)devfreq->data;
+	struct devfreq_cpu_data *cpu_data;
+	unsigned long cpu, cpu_cur, cpu_min, cpu_max, cpu_percent;
+	unsigned long dev_min, dev_max;
+	unsigned long freq = 0;
+
+	for_each_online_cpu(cpu) {
+		cpu_data = p_data->cpu_data[cpu];
+		if (!cpu_data || cpu_data->first_cpu != cpu)
+			continue;
+
+		/* Get target freq via required opps */
+		cpu_cur = cpu_data->cur_freq * HZ_PER_KHZ;
+		freq = get_taget_freq_by_required_opp(cpu_data->dev,
+					cpu_data->opp_table,
+					devfreq->opp_table, cpu_cur);
+		if (freq) {
+			*target_freq = max(freq, *target_freq);
+			continue;
+		}
+
+		/* Use Interpolation if required opps is not available */
+		devfreq_get_freq_range(devfreq, &dev_min, &dev_max);
+
+		cpu_min = cpu_data->min_freq;
+		cpu_max = cpu_data->max_freq;
+		cpu_cur = cpu_data->cur_freq;
+
+		cpu_percent = ((cpu_cur - cpu_min) * 100) / (cpu_max - cpu_min);
+		freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
+
+		*target_freq = max(freq, *target_freq);
+	}
+
+	return 0;
+}
+
+static int get_target_freq_with_devfreq(struct devfreq *devfreq,
 					unsigned long *freq)
 {
 	struct devfreq_passive_data *p_data
@@ -99,6 +172,172 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	return 0;
 }
 
+static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+					   unsigned long *freq)
+{
+	struct devfreq_passive_data *p_data =
+				(struct devfreq_passive_data *)devfreq->data;
+	int ret;
+
+	if (!p_data)
+		return -EINVAL;
+
+	/*
+	 * If the devfreq device with passive governor has the specific method
+	 * to determine the next frequency, should use the get_target_freq()
+	 * of struct devfreq_passive_data.
+	 */
+	if (p_data->get_target_freq)
+		return p_data->get_target_freq(devfreq, freq);
+
+	switch (p_data->parent_type) {
+	case DEVFREQ_PARENT_DEV:
+		ret = get_target_freq_with_devfreq(devfreq, freq);
+		break;
+	case CPUFREQ_PARENT_DEV:
+		ret = get_target_freq_with_cpufreq(devfreq, freq);
+		break;
+	default:
+		ret = -EINVAL;
+		dev_err(&devfreq->dev, "Invalid parent type\n");
+		break;
+	}
+
+	return ret;
+}
+
+static int cpufreq_passive_notifier_call(struct notifier_block *nb,
+					 unsigned long event, void *ptr)
+{
+	struct devfreq_passive_data *data =
+			container_of(nb, struct devfreq_passive_data, nb);
+	struct devfreq *devfreq = (struct devfreq *)data->this;
+	struct devfreq_cpu_data *cpu_data;
+	struct cpufreq_freqs *freqs = ptr;
+	unsigned int cur_freq;
+	int ret;
+
+	if (event != CPUFREQ_POSTCHANGE || !freqs ||
+		!data->cpu_data[freqs->policy->cpu])
+		return 0;
+
+	cpu_data = data->cpu_data[freqs->policy->cpu];
+	if (cpu_data->cur_freq == freqs->new)
+		return 0;
+
+	cur_freq = cpu_data->cur_freq;
+	cpu_data->cur_freq = freqs->new;
+
+	mutex_lock(&devfreq->lock);
+	ret = devfreq_update_target(devfreq, freqs->new);
+	mutex_unlock(&devfreq->lock);
+	if (ret) {
+		cpu_data->cur_freq = cur_freq;
+		dev_err(&devfreq->dev, "failed to update the frequency.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
+{
+	struct devfreq_passive_data *p_data
+			= (struct devfreq_passive_data *)devfreq->data;
+	struct device *dev = devfreq->dev.parent;
+	struct opp_table *opp_table = NULL;
+	struct devfreq_cpu_data *cpu_data;
+	struct cpufreq_policy *policy;
+	struct device *cpu_dev;
+	unsigned int cpu;
+	int ret;
+
+	get_online_cpus();
+
+	p_data->nb.notifier_call = cpufreq_passive_notifier_call;
+	ret = cpufreq_register_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
+	if (ret) {
+		dev_err(dev, "failed to register cpufreq notifier\n");
+		p_data->nb.notifier_call = NULL;
+		goto out;
+	}
+
+	for_each_online_cpu(cpu) {
+		if (p_data->cpu_data[cpu])
+			continue;
+
+		policy = cpufreq_cpu_get(cpu);
+		if (policy) {
+			cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
+			if (!cpu_data) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			cpu_dev = get_cpu_device(cpu);
+			if (!cpu_dev) {
+				dev_err(dev, "failed to get cpu device\n");
+				ret = -ENODEV;
+				goto out;
+			}
+
+			opp_table = dev_pm_opp_get_opp_table(cpu_dev);
+			if (IS_ERR(opp_table)) {
+				ret = PTR_ERR(opp_table);
+				goto out;
+			}
+
+			cpu_data->dev = cpu_dev;
+			cpu_data->opp_table = opp_table;
+			cpu_data->first_cpu = cpumask_first(policy->related_cpus);
+			cpu_data->cur_freq = policy->cur;
+			cpu_data->min_freq = policy->cpuinfo.min_freq;
+			cpu_data->max_freq = policy->cpuinfo.max_freq;
+
+			p_data->cpu_data[cpu] = cpu_data;
+			cpufreq_cpu_put(policy);
+		} else {
+			ret = -EPROBE_DEFER;
+			goto out;
+		}
+	}
+out:
+	put_online_cpus();
+	if (ret)
+		return ret;
+
+	mutex_lock(&devfreq->lock);
+	ret = devfreq_update_target(devfreq, 0L);
+	mutex_unlock(&devfreq->lock);
+	if (ret)
+		dev_err(dev, "failed to update the frequency\n");
+
+	return ret;
+}
+
+static int cpufreq_passive_unregister_notifier(struct devfreq *devfreq)
+{
+	struct devfreq_passive_data *p_data
+			= (struct devfreq_passive_data *)devfreq->data;
+	struct devfreq_cpu_data *cpu_data;
+	int cpu;
+
+	if (p_data->nb.notifier_call)
+		cpufreq_unregister_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
+
+	for_each_possible_cpu(cpu) {
+		cpu_data = p_data->cpu_data[cpu];
+		if (cpu_data) {
+			if (cpu_data->opp_table)
+				dev_pm_opp_put_opp_table(cpu_data->opp_table);
+			kfree(cpu_data);
+			cpu_data = NULL;
+		}
+	}
+
+	return 0;
+}
+
 static int devfreq_passive_notifier_call(struct notifier_block *nb,
 				unsigned long event, void *ptr)
 {
@@ -140,7 +379,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 	struct notifier_block *nb = &p_data->nb;
 	int ret = 0;
 
-	if (!parent)
+	if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
 		return -EPROBE_DEFER;
 
 	switch (event) {
@@ -148,13 +387,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 		if (!p_data->this)
 			p_data->this = devfreq;
 
-		nb->notifier_call = devfreq_passive_notifier_call;
-		ret = devfreq_register_notifier(parent, nb,
-					DEVFREQ_TRANSITION_NOTIFIER);
+		if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
+			nb->notifier_call = devfreq_passive_notifier_call;
+			ret = devfreq_register_notifier(parent, nb,
+						DEVFREQ_TRANSITION_NOTIFIER);
+		} else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
+			ret = cpufreq_passive_register_notifier(devfreq);
+		} else {
+			ret = -EINVAL;
+		}
 		break;
 	case DEVFREQ_GOV_STOP:
-		WARN_ON(devfreq_unregister_notifier(parent, nb,
-					DEVFREQ_TRANSITION_NOTIFIER));
+		if (p_data->parent_type == DEVFREQ_PARENT_DEV)
+			WARN_ON(devfreq_unregister_notifier(parent, nb,
+						DEVFREQ_TRANSITION_NOTIFIER));
+		else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
+			WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
+		else
+			ret = -EINVAL;
 		break;
 	default:
 		break;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 142474b4af96..cfa0ef54841e 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -38,6 +38,7 @@ enum devfreq_timer {
 
 struct devfreq;
 struct devfreq_governor;
+struct devfreq_cpu_data;
 struct thermal_cooling_device;
 
 /**
@@ -288,6 +289,11 @@ struct devfreq_simple_ondemand_data {
 #endif
 
 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
+enum devfreq_parent_dev_type {
+	DEVFREQ_PARENT_DEV,
+	CPUFREQ_PARENT_DEV,
+};
+
 /**
  * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
  *	and devfreq_add_device
@@ -299,8 +305,10 @@ struct devfreq_simple_ondemand_data {
  *			using governors except for passive governor.
  *			If the devfreq device has the specific method to decide
  *			the next frequency, should use this callback.
- * @this:	the devfreq instance of own device.
- * @nb:		the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ + * @parent_type	parent type of the device
+ + * @this:		the devfreq instance of own device.
+ + * @nb:		the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ + * @cpu_data:		the state min/max/current frequency of all online cpu's
  *
  * The devfreq_passive_data have to set the devfreq instance of parent
  * device with governors except for the passive governor. But, don't need to
@@ -314,9 +322,13 @@ struct devfreq_passive_data {
 	/* Optional callback to decide the next frequency of passvice device */
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 
+	/* Should set the type of parent device */
+	enum devfreq_parent_dev_type parent_type;
+
 	/* For passive governor's internal use. Don't need to set them */
 	struct devfreq *this;
 	struct notifier_block nb;
+	struct devfreq_cpu_data *cpu_data[NR_CPUS];
 };
 #endif
 
-- 
2.17.1

[PATCH 4/4] PM / devfreq: passive: Reduce duplicate code when passive_devfreq case

[Chanwoo Choi]

In order to keep the consistent coding style between passive_devfreq
and passive_cpufreq, use common code for handling required opp property.
Also remove the unneed conditional statement and unify the comment
of both passive_devfreq and passive_cpufreq when getting the target frequency.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/governor_passive.c | 80 ++++++------------------------
 1 file changed, 15 insertions(+), 65 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 07e864509b7e..7102cb7eb30d 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -91,66 +91,17 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
 	struct devfreq_passive_data *p_data
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
-	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp, *p_opp;
-	int i, count;
-
-	/*
-	 * If the devfreq device with passive governor has the specific method
-	 * to determine the next frequency, should use the get_target_freq()
-	 * of struct devfreq_passive_data.
-	 */
-	if (p_data->get_target_freq)
-		return p_data->get_target_freq(devfreq, freq);
-
-	/*
-	 * If the parent and passive devfreq device uses the OPP table,
-	 * get the next frequency by using the OPP table.
-	 */
-
-	/*
-	 * - parent devfreq device uses the governors except for passive.
-	 * - passive devfreq device uses the passive governor.
-	 *
-	 * Each devfreq has the OPP table. After deciding the new frequency
-	 * from the governor of parent devfreq device, the passive governor
-	 * need to get the index of new frequency on OPP table of parent
-	 * device. And then the index is used for getting the suitable
-	 * new frequency for passive devfreq device.
-	 */
-	if (!devfreq->profile || !devfreq->profile->freq_table
-		|| devfreq->profile->max_state <= 0)
-		return -EINVAL;
-
-	/*
-	 * The passive governor have to get the correct frequency from OPP
-	 * list of parent device. Because in this case, *freq is temporary
-	 * value which is decided by ondemand governor.
-	 */
-	if (devfreq->opp_table && parent_devfreq->opp_table) {
-		p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent,
-						freq, 0);
-		if (IS_ERR(p_opp))
-			return PTR_ERR(p_opp);
-
-		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
-						    devfreq->opp_table, p_opp);
-		dev_pm_opp_put(p_opp);
-
-		if (IS_ERR(opp))
-			goto no_required_opp;
-
-		*freq = dev_pm_opp_get_freq(opp);
-		dev_pm_opp_put(opp);
-
-		return 0;
-	}
+	unsigned long target_freq;
+	int i;
+
+	/* Get target freq via required opps */
+	target_freq = get_taget_freq_by_required_opp(parent_devfreq->dev.parent,
+						parent_devfreq->opp_table,
+						devfreq->opp_table, *freq);
+	if (target_freq)
+		goto out;
 
-no_required_opp:
-	/*
-	 * Get the OPP table's index of decided frequency by governor
-	 * of parent device.
-	 */
+	/* Use Interpolation if required opps is not available */
 	for (i = 0; i < parent_devfreq->profile->max_state; i++)
 		if (parent_devfreq->profile->freq_table[i] == *freq)
 			break;
@@ -158,16 +109,15 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
 	if (i == parent_devfreq->profile->max_state)
 		return -EINVAL;
 
-	/* Get the suitable frequency by using index of parent device. */
 	if (i < devfreq->profile->max_state) {
-		child_freq = devfreq->profile->freq_table[i];
+		target_freq = devfreq->profile->freq_table[i];
 	} else {
-		count = devfreq->profile->max_state;
-		child_freq = devfreq->profile->freq_table[count - 1];
+		i = devfreq->profile->max_state;
+		target_freq = devfreq->profile->freq_table[i - 1];
 	}
 
-	/* Return the suitable frequency for passive device. */
-	*freq = child_freq;
+out:
+	*freq = target_freq;
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

[Chanwoo Choi]

On 6/17/21 2:51 PM, Hsin-Yi Wang wrote:
> On Thu, Jun 17, 2021 at 1:46 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>
>> From: Saravana Kannan <skannan@codeaurora.org>
>>
>> Many CPU architectures have caches that can scale independent of the
>> CPUs. Frequency scaling of the caches is necessary to make sure that the
>> cache is not a performance bottleneck that leads to poor performance and
>> power. The same idea applies for RAM/DDR.
>>
>> To achieve this, this patch adds support for cpu based scaling to the
>> passive governor. This is accomplished by taking the current frequency
>> of each CPU frequency domain and then adjust the frequency of the cache
>> (or any devfreq device) based on the frequency of the CPUs. It listens
>> to CPU frequency transition notifiers to keep itself up to date on the
>> current CPU frequency.
>>
>> To decide the frequency of the device, the governor does one of the
>> following:
>> * Derives the optimal devfreq device opp from required-opps property of
>>   the parent cpu opp_table.
>>
>> * Scales the device frequency in proportion to the CPU frequency. So, if
>>   the CPUs are running at their max frequency, the device runs at its
>>   max frequency. If the CPUs are running at their min frequency, the
>>   device runs at its min frequency. It is interpolated for frequencies
>>   in between.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> [Sibi: Integrated cpu-freqmap governor into passive_governor]
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> [Chanwoo: Fix conflict with latest code and clean code up]
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/governor.h         |  22 +++
>>  drivers/devfreq/governor_passive.c | 264 ++++++++++++++++++++++++++++-
>>  include/linux/devfreq.h            |  16 +-
>>  3 files changed, 293 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>> index 9a9495f94ac6..3c36c92c89a9 100644
>> --- a/drivers/devfreq/governor.h
>> +++ b/drivers/devfreq/governor.h
>> @@ -47,6 +47,28 @@
>>  #define DEVFREQ_GOV_ATTR_POLLING_INTERVAL              BIT(0)
>>  #define DEVFREQ_GOV_ATTR_TIMER                         BIT(1)
>>
>> +/**
>> + * struct devfreq_cpu_data - Hold the per-cpu data
>> + * @dev:       reference to cpu device.
>> + * @first_cpu: the cpumask of the first cpu of a policy.
>> + * @opp_table: reference to cpu opp table.
>> + * @cur_freq:  the current frequency of the cpu.
>> + * @min_freq:  the min frequency of the cpu.
>> + * @max_freq:  the max frequency of the cpu.
>> + *
>> + * This structure stores the required cpu_data of a cpu.
>> + * This is auto-populated by the governor.
>> + */
>> +struct devfreq_cpu_data {
>> +       struct device *dev;
>> +       unsigned int first_cpu;
>> +
>> +       struct opp_table *opp_table;
>> +       unsigned int cur_freq;
>> +       unsigned int min_freq;
>> +       unsigned int max_freq;
>> +};
>> +
>>  /**
>>   * struct devfreq_governor - Devfreq policy governor
>>   * @node:              list node - contains registered devfreq governors
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index fc09324a03e0..07e864509b7e 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -8,11 +8,84 @@
>>   */
>>
>>  #include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>>  #include <linux/device.h>
>>  #include <linux/devfreq.h>
>>  #include "governor.h"
>>
>> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> +#define HZ_PER_KHZ     1000
>> +
>> +static unsigned long get_taget_freq_by_required_opp(struct device *p_dev,
>> +                                               struct opp_table *p_opp_table,
>> +                                               struct opp_table *opp_table,
>> +                                               unsigned long freq)
>> +{
>> +       struct dev_pm_opp *opp = NULL, *p_opp = NULL;
>> +
>> +       if (!p_dev || !p_opp_table || !opp_table || !freq)
>> +               return 0;
>> +
>> +       p_opp = devfreq_recommended_opp(p_dev, &freq, 0);
>> +       if (IS_ERR(p_opp))
>> +               return 0;
>> +
>> +       opp = dev_pm_opp_xlate_required_opp(p_opp_table, opp_table, p_opp);
>> +       dev_pm_opp_put(p_opp);
>> +
>> +       if (IS_ERR(opp))
>> +               return 0;
>> +
>> +       freq = dev_pm_opp_get_freq(opp);
>> +       dev_pm_opp_put(opp);
>> +
>> +       return freq;
>> +}
>> +
>> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
>> +                                       unsigned long *target_freq)
>> +{
>> +       struct devfreq_passive_data *p_data =
>> +                               (struct devfreq_passive_data *)devfreq->data;
>> +       struct devfreq_cpu_data *cpu_data;
> We might have to rename the cpu_data variable.
> 
> For some architectures, cpu_data is defined as macro. This results in
> errors such as
> 
> include/linux/devfreq.h:331:27: note: in expansion of macro 'cpu_data'
>      331 |  struct devfreq_cpu_data *cpu_data[NR_CPUS];
>          |                           ^~~~~~~~
>    In file included from include/linux/devfreq_cooling.h:13,
>                     from drivers/devfreq/devfreq.c:14:
>    include/linux/devfreq.h:332:1: warning: no semicolon at end of
> struct or union

OK. I'll rename. 

> 
>> +       unsigned long cpu, cpu_cur, cpu_min, cpu_max, cpu_percent;
>> +       unsigned long dev_min, dev_max;
>> +       unsigned long freq = 0;
>> +
>> +       for_each_online_cpu(cpu) {
>> +               cpu_data = p_data->cpu_data[cpu];
>> +               if (!cpu_data || cpu_data->first_cpu != cpu)
>> +                       continue;
>> +
>> +               /* Get target freq via required opps */
>> +               cpu_cur = cpu_data->cur_freq * HZ_PER_KHZ;
>> +               freq = get_taget_freq_by_required_opp(cpu_data->dev,
>> +                                       cpu_data->opp_table,
>> +                                       devfreq->opp_table, cpu_cur);
>> +               if (freq) {
>> +                       *target_freq = max(freq, *target_freq);
>> +                       continue;
>> +               }
>> +
>> +               /* Use Interpolation if required opps is not available */
>> +               devfreq_get_freq_range(devfreq, &dev_min, &dev_max);
>> +
>> +               cpu_min = cpu_data->min_freq;
>> +               cpu_max = cpu_data->max_freq;
>> +               cpu_cur = cpu_data->cur_freq;
>> +
>> +               cpu_percent = ((cpu_cur - cpu_min) * 100) / (cpu_max - cpu_min);
>> +               freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
>> +
>> +               *target_freq = max(freq, *target_freq);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>>                                         unsigned long *freq)
>>  {
>>         struct devfreq_passive_data *p_data
>> @@ -99,6 +172,172 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>         return 0;
>>  }
>>
>> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> +                                          unsigned long *freq)
>> +{
>> +       struct devfreq_passive_data *p_data =
>> +                               (struct devfreq_passive_data *)devfreq->data;
>> +       int ret;
>> +
>> +       if (!p_data)
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * If the devfreq device with passive governor has the specific method
>> +        * to determine the next frequency, should use the get_target_freq()
>> +        * of struct devfreq_passive_data.
>> +        */
>> +       if (p_data->get_target_freq)
>> +               return p_data->get_target_freq(devfreq, freq);
>> +
>> +       switch (p_data->parent_type) {
>> +       case DEVFREQ_PARENT_DEV:
>> +               ret = get_target_freq_with_devfreq(devfreq, freq);
>> +               break;
>> +       case CPUFREQ_PARENT_DEV:
>> +               ret = get_target_freq_with_cpufreq(devfreq, freq);
>> +               break;
>> +       default:
>> +               ret = -EINVAL;
>> +               dev_err(&devfreq->dev, "Invalid parent type\n");
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
>> +                                        unsigned long event, void *ptr)
>> +{
>> +       struct devfreq_passive_data *data =
>> +                       container_of(nb, struct devfreq_passive_data, nb);
>> +       struct devfreq *devfreq = (struct devfreq *)data->this;
>> +       struct devfreq_cpu_data *cpu_data;
>> +       struct cpufreq_freqs *freqs = ptr;
>> +       unsigned int cur_freq;
>> +       int ret;
>> +
>> +       if (event != CPUFREQ_POSTCHANGE || !freqs ||
>> +               !data->cpu_data[freqs->policy->cpu])
>> +               return 0;
>> +
>> +       cpu_data = data->cpu_data[freqs->policy->cpu];
>> +       if (cpu_data->cur_freq == freqs->new)
>> +               return 0;
>> +
>> +       cur_freq = cpu_data->cur_freq;
>> +       cpu_data->cur_freq = freqs->new;
>> +
>> +       mutex_lock(&devfreq->lock);
>> +       ret = devfreq_update_target(devfreq, freqs->new);
>> +       mutex_unlock(&devfreq->lock);
>> +       if (ret) {
>> +               cpu_data->cur_freq = cur_freq;
>> +               dev_err(&devfreq->dev, "failed to update the frequency.\n");
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>> +{
>> +       struct devfreq_passive_data *p_data
>> +                       = (struct devfreq_passive_data *)devfreq->data;
>> +       struct device *dev = devfreq->dev.parent;
>> +       struct opp_table *opp_table = NULL;
>> +       struct devfreq_cpu_data *cpu_data;
>> +       struct cpufreq_policy *policy;
>> +       struct device *cpu_dev;
>> +       unsigned int cpu;
>> +       int ret;
>> +
>> +       get_online_cpus();
>> +
>> +       p_data->nb.notifier_call = cpufreq_passive_notifier_call;
>> +       ret = cpufreq_register_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
>> +       if (ret) {
>> +               dev_err(dev, "failed to register cpufreq notifier\n");
>> +               p_data->nb.notifier_call = NULL;
>> +               goto out;
>> +       }
>> +
>> +       for_each_online_cpu(cpu) {
>> +               if (p_data->cpu_data[cpu])
>> +                       continue;
>> +
>> +               policy = cpufreq_cpu_get(cpu);
>> +               if (policy) {
>> +                       cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
>> +                       if (!cpu_data) {
>> +                               ret = -ENOMEM;
>> +                               goto out;
>> +                       }
>> +
>> +                       cpu_dev = get_cpu_device(cpu);
>> +                       if (!cpu_dev) {
>> +                               dev_err(dev, "failed to get cpu device\n");
>> +                               ret = -ENODEV;
>> +                               goto out;
>> +                       }
>> +
>> +                       opp_table = dev_pm_opp_get_opp_table(cpu_dev);
>> +                       if (IS_ERR(opp_table)) {
>> +                               ret = PTR_ERR(opp_table);
>> +                               goto out;
>> +                       }
>> +
>> +                       cpu_data->dev = cpu_dev;
>> +                       cpu_data->opp_table = opp_table;
>> +                       cpu_data->first_cpu = cpumask_first(policy->related_cpus);
>> +                       cpu_data->cur_freq = policy->cur;
>> +                       cpu_data->min_freq = policy->cpuinfo.min_freq;
>> +                       cpu_data->max_freq = policy->cpuinfo.max_freq;
>> +
>> +                       p_data->cpu_data[cpu] = cpu_data;
>> +                       cpufreq_cpu_put(policy);
>> +               } else {
>> +                       ret = -EPROBE_DEFER;
>> +                       goto out;
>> +               }
>> +       }
>> +out:
>> +       put_online_cpus();
>> +       if (ret)
>> +               return ret;
>> +
>> +       mutex_lock(&devfreq->lock);
>> +       ret = devfreq_update_target(devfreq, 0L);
>> +       mutex_unlock(&devfreq->lock);
>> +       if (ret)
>> +               dev_err(dev, "failed to update the frequency\n");
>> +
>> +       return ret;
>> +}
>> +
>> +static int cpufreq_passive_unregister_notifier(struct devfreq *devfreq)
>> +{
>> +       struct devfreq_passive_data *p_data
>> +                       = (struct devfreq_passive_data *)devfreq->data;
>> +       struct devfreq_cpu_data *cpu_data;
>> +       int cpu;
>> +
>> +       if (p_data->nb.notifier_call)
>> +               cpufreq_unregister_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_data = p_data->cpu_data[cpu];
>> +               if (cpu_data) {
>> +                       if (cpu_data->opp_table)
>> +                               dev_pm_opp_put_opp_table(cpu_data->opp_table);
>> +                       kfree(cpu_data);
>> +                       cpu_data = NULL;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int devfreq_passive_notifier_call(struct notifier_block *nb,
>>                                 unsigned long event, void *ptr)
>>  {
>> @@ -140,7 +379,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>         struct notifier_block *nb = &p_data->nb;
>>         int ret = 0;
>>
>> -       if (!parent)
>> +       if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
>>                 return -EPROBE_DEFER;
>>
>>         switch (event) {
>> @@ -148,13 +387,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>>                 if (!p_data->this)
>>                         p_data->this = devfreq;
>>
>> -               nb->notifier_call = devfreq_passive_notifier_call;
>> -               ret = devfreq_register_notifier(parent, nb,
>> -                                       DEVFREQ_TRANSITION_NOTIFIER);
>> +               if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
>> +                       nb->notifier_call = devfreq_passive_notifier_call;
>> +                       ret = devfreq_register_notifier(parent, nb,
>> +                                               DEVFREQ_TRANSITION_NOTIFIER);
>> +               } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
>> +                       ret = cpufreq_passive_register_notifier(devfreq);
>> +               } else {
>> +                       ret = -EINVAL;
>> +               }
>>                 break;
>>         case DEVFREQ_GOV_STOP:
>> -               WARN_ON(devfreq_unregister_notifier(parent, nb,
>> -                                       DEVFREQ_TRANSITION_NOTIFIER));
>> +               if (p_data->parent_type == DEVFREQ_PARENT_DEV)
>> +                       WARN_ON(devfreq_unregister_notifier(parent, nb,
>> +                                               DEVFREQ_TRANSITION_NOTIFIER));
>> +               else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
>> +                       WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
>> +               else
>> +                       ret = -EINVAL;
>>                 break;
>>         default:
>>                 break;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 142474b4af96..cfa0ef54841e 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -38,6 +38,7 @@ enum devfreq_timer {
>>
>>  struct devfreq;
>>  struct devfreq_governor;
>> +struct devfreq_cpu_data;
>>  struct thermal_cooling_device;
>>
>>  /**
>> @@ -288,6 +289,11 @@ struct devfreq_simple_ondemand_data {
>>  #endif
>>
>>  #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> +enum devfreq_parent_dev_type {
>> +       DEVFREQ_PARENT_DEV,
>> +       CPUFREQ_PARENT_DEV,
>> +};
>> +
>>  /**
>>   * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
>>   *     and devfreq_add_device
>> @@ -299,8 +305,10 @@ struct devfreq_simple_ondemand_data {
>>   *                     using governors except for passive governor.
>>   *                     If the devfreq device has the specific method to decide
>>   *                     the next frequency, should use this callback.
>> - * @this:      the devfreq instance of own device.
>> - * @nb:                the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>> + + * @parent_type      parent type of the device
>> + + * @this:            the devfreq instance of own device.
>> + + * @nb:              the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>> + + * @cpu_data:                the state min/max/current frequency of all online cpu's
>>   *
>>   * The devfreq_passive_data have to set the devfreq instance of parent
>>   * device with governors except for the passive governor. But, don't need to
>> @@ -314,9 +322,13 @@ struct devfreq_passive_data {
>>         /* Optional callback to decide the next frequency of passvice device */
>>         int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>>
>> +       /* Should set the type of parent device */
>> +       enum devfreq_parent_dev_type parent_type;
>> +
>>         /* For passive governor's internal use. Don't need to set them */
>>         struct devfreq *this;
>>         struct notifier_block nb;
>> +       struct devfreq_cpu_data *cpu_data[NR_CPUS];
>>  };
>>  #endif
>>
>> --
>> 2.17.1
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

[Matthias Kaehlcke]

On Thu, Jun 17, 2021 at 03:05:45PM +0900, Chanwoo Choi wrote:
> From: Saravana Kannan <skannan@codeaurora.org>
> 
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
> 
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
> 
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
>   the parent cpu opp_table.
> 
> * Scales the device frequency in proportion to the CPU frequency. So, if
>   the CPUs are running at their max frequency, the device runs at its
>   max frequency. If the CPUs are running at their min frequency, the
>   device runs at its min frequency. It is interpolated for frequencies
>   in between.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> [Chanwoo: Fix conflict with latest code and clean code up]
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/governor.h         |  22 +++
>  drivers/devfreq/governor_passive.c | 264 ++++++++++++++++++++++++++++-
>  include/linux/devfreq.h            |  16 +-
>  3 files changed, 293 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index 9a9495f94ac6..3c36c92c89a9 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -47,6 +47,28 @@
>  #define DEVFREQ_GOV_ATTR_POLLING_INTERVAL		BIT(0)
>  #define DEVFREQ_GOV_ATTR_TIMER				BIT(1)
>  
> +/**
> + * struct devfreq_cpu_data - Hold the per-cpu data
> + * @dev:	reference to cpu device.
> + * @first_cpu:	the cpumask of the first cpu of a policy.
> + * @opp_table:	reference to cpu opp table.
> + * @cur_freq:	the current frequency of the cpu.
> + * @min_freq:	the min frequency of the cpu.
> + * @max_freq:	the max frequency of the cpu.
> + *
> + * This structure stores the required cpu_data of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_data {
> +	struct device *dev;
> +	unsigned int first_cpu;
> +
> +	struct opp_table *opp_table;
> +	unsigned int cur_freq;
> +	unsigned int min_freq;
> +	unsigned int max_freq;
> +};
> +
>  /**
>   * struct devfreq_governor - Devfreq policy governor
>   * @node:		list node - contains registered devfreq governors
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index fc09324a03e0..07e864509b7e 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
>
> ...
>
> +static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
> +{
> +	struct devfreq_passive_data *p_data
> +			= (struct devfreq_passive_data *)devfreq->data;
> +	struct device *dev = devfreq->dev.parent;
> +	struct opp_table *opp_table = NULL;
> +	struct devfreq_cpu_data *cpu_data;
> +	struct cpufreq_policy *policy;
> +	struct device *cpu_dev;
> +	unsigned int cpu;
> +	int ret;
> +
> +	get_online_cpus();
> +
> +	p_data->nb.notifier_call = cpufreq_passive_notifier_call;
> +	ret = cpufreq_register_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
> +	if (ret) {
> +		dev_err(dev, "failed to register cpufreq notifier\n");
> +		p_data->nb.notifier_call = NULL;
> +		goto out;
> +	}
> +
> +	for_each_online_cpu(cpu) {


Is this really needed for each CPU? Wouldn't it be enough to create
a 'cpu_data' for each 'policy CPU'?

In any case should this be for_each_possible_cpu() as in _unregister_notifier()
to also support CPUs that may be offline when the notifier is registered?

> +		if (p_data->cpu_data[cpu])
> +			continue;
> +
> +		policy = cpufreq_cpu_get(cpu);
> +		if (policy) {
> +			cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
> +			if (!cpu_data) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +
> +			cpu_dev = get_cpu_device(cpu);
> +			if (!cpu_dev) {
> +				dev_err(dev, "failed to get cpu device\n");
> +				ret = -ENODEV;
> +				goto out;

Memory for 'cpu_data' is not freed in this path.

Also applies to CPUs from possible prior iterations.

> +			}
> +
> +			opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> +			if (IS_ERR(opp_table)) {
> +				ret = PTR_ERR(opp_table);
> +				goto out;

Ditto and cpufreq_cpu_put() is missing too.

> +			}
> +
> +			cpu_data->dev = cpu_dev;
> +			cpu_data->opp_table = opp_table;
> +			cpu_data->first_cpu = cpumask_first(policy->related_cpus);
> +			cpu_data->cur_freq = policy->cur;
> +			cpu_data->min_freq = policy->cpuinfo.min_freq;
> +			cpu_data->max_freq = policy->cpuinfo.max_freq;
> +
> +			p_data->cpu_data[cpu] = cpu_data;
> +			cpufreq_cpu_put(policy);
> +		} else {
> +			ret = -EPROBE_DEFER;
> +			goto out;

Resources from possible prior iterations aren't freed.

> +		}
> +	}
> +out:
> +	put_online_cpus();
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&devfreq->lock);
> +	ret = devfreq_update_target(devfreq, 0L);
> +	mutex_unlock(&devfreq->lock);
> +	if (ret)
> +		dev_err(dev, "failed to update the frequency\n");
> +
> +	return ret;
> +}
> +
> +static int cpufreq_passive_unregister_notifier(struct devfreq *devfreq)
> +{
> +	struct devfreq_passive_data *p_data
> +			= (struct devfreq_passive_data *)devfreq->data;
> +	struct devfreq_cpu_data *cpu_data;
> +	int cpu;
> +
> +	if (p_data->nb.notifier_call)
> +		cpufreq_unregister_notifier(&p_data->nb, CPUFREQ_TRANSITION_NOTIFIER);
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_data = p_data->cpu_data[cpu];
> +		if (cpu_data) {
> +			if (cpu_data->opp_table)
> +				dev_pm_opp_put_opp_table(cpu_data->opp_table);
> +			kfree(cpu_data);
> +			cpu_data = NULL;

Assignment to NULL is not needed.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int devfreq_passive_notifier_call(struct notifier_block *nb,
>  				unsigned long event, void *ptr)
>  {
> @@ -140,7 +379,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>  	struct notifier_block *nb = &p_data->nb;
>  	int ret = 0;
>  
> -	if (!parent)
> +	if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
>  		return -EPROBE_DEFER;
>  
>  	switch (event) {
> @@ -148,13 +387,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>  		if (!p_data->this)
>  			p_data->this = devfreq;
>  
> -		nb->notifier_call = devfreq_passive_notifier_call;
> -		ret = devfreq_register_notifier(parent, nb,
> -					DEVFREQ_TRANSITION_NOTIFIER);
> +		if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> +			nb->notifier_call = devfreq_passive_notifier_call;
> +			ret = devfreq_register_notifier(parent, nb,
> +						DEVFREQ_TRANSITION_NOTIFIER);
> +		} else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> +			ret = cpufreq_passive_register_notifier(devfreq);
> +		} else {
> +			ret = -EINVAL;
> +		}
>  		break;
>  	case DEVFREQ_GOV_STOP:
> -		WARN_ON(devfreq_unregister_notifier(parent, nb,
> -					DEVFREQ_TRANSITION_NOTIFIER));
> +		if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> +			WARN_ON(devfreq_unregister_notifier(parent, nb,
> +						DEVFREQ_TRANSITION_NOTIFIER));
> +		else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> +			WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
> +		else
> +			ret = -EINVAL;
>  		break;
>  	default:
>  		break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 142474b4af96..cfa0ef54841e 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -38,6 +38,7 @@ enum devfreq_timer {
>  
>  struct devfreq;
>  struct devfreq_governor;
> +struct devfreq_cpu_data;
>  struct thermal_cooling_device;
>  
>  /**
> @@ -288,6 +289,11 @@ struct devfreq_simple_ondemand_data {
>  #endif
>  
>  #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> +enum devfreq_parent_dev_type {
> +	DEVFREQ_PARENT_DEV,
> +	CPUFREQ_PARENT_DEV,
> +};
> +
>  /**
>   * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
>   *	and devfreq_add_device
> @@ -299,8 +305,10 @@ struct devfreq_simple_ondemand_data {
>   *			using governors except for passive governor.
>   *			If the devfreq device has the specific method to decide
>   *			the next frequency, should use this callback.
> - * @this:	the devfreq instance of own device.
> - * @nb:		the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + + * @parent_type	parent type of the device
> + + * @this:		the devfreq instance of own device.
> + + * @nb:		the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + + * @cpu_data:		the state min/max/current frequency of all online cpu's
>   *
>   * The devfreq_passive_data have to set the devfreq instance of parent
>   * device with governors except for the passive governor. But, don't need to
> @@ -314,9 +322,13 @@ struct devfreq_passive_data {
>  	/* Optional callback to decide the next frequency of passvice device */
>  	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>  
> +	/* Should set the type of parent device */
> +	enum devfreq_parent_dev_type parent_type;
> +
>  	/* For passive governor's internal use. Don't need to set them */
>  	struct devfreq *this;
>  	struct notifier_block nb;
> +	struct devfreq_cpu_data *cpu_data[NR_CPUS];

Could memory usage be a concern on systems with a really high number of CPUs
(e.g. 8k for x86 with MAXSMP)? One could argue that such systems likely have
significant amount of RAM too and a chunk of memory in the order of 100k
wouldn't make a big impact. I'm assuming that 'cpu_data' is only needed for
'policy CPUs'.

Re: [PATCH 2/4] PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq

[Matthias Kaehlcke]

On Thu, Jun 17, 2021 at 03:05:44PM +0900, Chanwoo Choi wrote:

> Subject: PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq

nit: s/ragne/range/

>
> In order to get frequency range within devfreq governors,
> export devfreq_get_freq_ragne symbol within devfreq.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 4/4] PM / devfreq: passive: Reduce duplicate code when passive_devfreq case

[Matthias Kaehlcke]

On Thu, Jun 17, 2021 at 03:05:46PM +0900, Chanwoo Choi wrote:
> In order to keep the consistent coding style between passive_devfreq
> and passive_cpufreq, use common code for handling required opp property.
> Also remove the unneed conditional statement and unify the comment
> of both passive_devfreq and passive_cpufreq when getting the target frequency.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/governor_passive.c | 80 ++++++------------------------
>  1 file changed, 15 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 07e864509b7e..7102cb7eb30d 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -91,66 +91,17 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>  	struct devfreq_passive_data *p_data
>  			= (struct devfreq_passive_data *)devfreq->data;
>  	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> -	unsigned long child_freq = ULONG_MAX;
> -	struct dev_pm_opp *opp, *p_opp;
> -	int i, count;
> -
> -	/*
> -	 * If the devfreq device with passive governor has the specific method
> -	 * to determine the next frequency, should use the get_target_freq()
> -	 * of struct devfreq_passive_data.
> -	 */
> -	if (p_data->get_target_freq)
> -		return p_data->get_target_freq(devfreq, freq);
> -
> -	/*
> -	 * If the parent and passive devfreq device uses the OPP table,
> -	 * get the next frequency by using the OPP table.
> -	 */
> -
> -	/*
> -	 * - parent devfreq device uses the governors except for passive.
> -	 * - passive devfreq device uses the passive governor.
> -	 *
> -	 * Each devfreq has the OPP table. After deciding the new frequency
> -	 * from the governor of parent devfreq device, the passive governor
> -	 * need to get the index of new frequency on OPP table of parent
> -	 * device. And then the index is used for getting the suitable
> -	 * new frequency for passive devfreq device.
> -	 */
> -	if (!devfreq->profile || !devfreq->profile->freq_table
> -		|| devfreq->profile->max_state <= 0)
> -		return -EINVAL;
> -
> -	/*
> -	 * The passive governor have to get the correct frequency from OPP
> -	 * list of parent device. Because in this case, *freq is temporary
> -	 * value which is decided by ondemand governor.
> -	 */
> -	if (devfreq->opp_table && parent_devfreq->opp_table) {
> -		p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent,
> -						freq, 0);
> -		if (IS_ERR(p_opp))
> -			return PTR_ERR(p_opp);
> -
> -		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
> -						    devfreq->opp_table, p_opp);
> -		dev_pm_opp_put(p_opp);
> -
> -		if (IS_ERR(opp))
> -			goto no_required_opp;
> -
> -		*freq = dev_pm_opp_get_freq(opp);
> -		dev_pm_opp_put(opp);
> -
> -		return 0;
> -	}
> +	unsigned long target_freq;
> +	int i;
> +
> +	/* Get target freq via required opps */
> +	target_freq = get_taget_freq_by_required_opp(parent_devfreq->dev.parent,
> +						parent_devfreq->opp_table,
> +						devfreq->opp_table, *freq);

s/get_taget_freq_by_required_opp/get_target_freq_by_required_opp/

Also need to be fixed in "[3/4] PM / devfreq: Add cpu based scaling
support to passive governor".

> +	if (target_freq)
> +		goto out;
>  
> -no_required_opp:
> -	/*
> -	 * Get the OPP table's index of decided frequency by governor
> -	 * of parent device.
> -	 */
> +	/* Use Interpolation if required opps is not available */

s/Interpolation/interpolation/

>  	for (i = 0; i < parent_devfreq->profile->max_state; i++)
>  		if (parent_devfreq->profile->freq_table[i] == *freq)
>  			break;
> @@ -158,16 +109,15 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>  	if (i == parent_devfreq->profile->max_state)
>  		return -EINVAL;
>  
> -	/* Get the suitable frequency by using index of parent device. */
>  	if (i < devfreq->profile->max_state) {
> -		child_freq = devfreq->profile->freq_table[i];
> +		target_freq = devfreq->profile->freq_table[i];
>  	} else {
> -		count = devfreq->profile->max_state;
> -		child_freq = devfreq->profile->freq_table[count - 1];
> +		i = devfreq->profile->max_state;
> +		target_freq = devfreq->profile->freq_table[i - 1];
>  	}
>  
> -	/* Return the suitable frequency for passive device. */
> -	*freq = child_freq;
> +out:
> +	*freq = target_freq;

You might want to split the child_freq => target_freq and commentary change into
a separate patch, since it is not directly related with the switch to
get_target_freq_by_required_opp().

Re: [PATCH 3/4] PM / devfreq: Add cpu based scaling support to passive governor

[Matthias Kaehlcke]

On Thu, Jun 17, 2021 at 03:05:45PM +0900, Chanwoo Choi wrote:
> From: Saravana Kannan <skannan@codeaurora.org>
> 
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
> 
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
> 
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
>   the parent cpu opp_table.
> 
> * Scales the device frequency in proportion to the CPU frequency. So, if
>   the CPUs are running at their max frequency, the device runs at its
>   max frequency. If the CPUs are running at their min frequency, the
>   device runs at its min frequency. It is interpolated for frequencies
>   in between.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> [Chanwoo: Fix conflict with latest code and clean code up]
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/governor.h         |  22 +++
>  drivers/devfreq/governor_passive.c | 264 ++++++++++++++++++++++++++++-
>  include/linux/devfreq.h            |  16 +-
>  3 files changed, 293 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index 9a9495f94ac6..3c36c92c89a9 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -47,6 +47,28 @@
>  #define DEVFREQ_GOV_ATTR_POLLING_INTERVAL		BIT(0)
>  #define DEVFREQ_GOV_ATTR_TIMER				BIT(1)
>  
> +/**
> + * struct devfreq_cpu_data - Hold the per-cpu data
> + * @dev:	reference to cpu device.
> + * @first_cpu:	the cpumask of the first cpu of a policy.
> + * @opp_table:	reference to cpu opp table.
> + * @cur_freq:	the current frequency of the cpu.
> + * @min_freq:	the min frequency of the cpu.
> + * @max_freq:	the max frequency of the cpu.
> + *
> + * This structure stores the required cpu_data of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_data {
> +	struct device *dev;
> +	unsigned int first_cpu;
> +
> +	struct opp_table *opp_table;
> +	unsigned int cur_freq;
> +	unsigned int min_freq;
> +	unsigned int max_freq;
> +};
> +
>  /**
>   * struct devfreq_governor - Devfreq policy governor
>   * @node:		list node - contains registered devfreq governors
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index fc09324a03e0..07e864509b7e 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,84 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/devfreq.h>
>  #include "governor.h"
>  
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +#define HZ_PER_KHZ	1000
> +
> +static unsigned long get_taget_freq_by_required_opp(struct device *p_dev,
> +						struct opp_table *p_opp_table,
> +						struct opp_table *opp_table,
> +						unsigned long freq)
> +{

s/get_taget_freq_by_required_opp/get_target_freq_by_required_opp/

Re: [PATCH 1/4] PM / devfreq: passive: Fix get_target_freq when not using required-opp

[Chanwoo Choi]

On 6/17/21 3:05 PM, Chanwoo Choi wrote:
> The 86ad9a24f21e ("PM / devfreq: Add required OPPs support to passive governor")
> supported the required-opp property for using devfreq passive governor.
> But, 86ad9a24f21e has caused the problem on use-case when required-opp
> is not used such as exynos-bus.c devfreq driver. So that fix the
> get_target_freq of passive governor for supporting the case of when
> required-opp is not used.
> 
> Cc: stable@vger.kernel.org
> Fixes: 86ad9a24f21e ("PM / devfreq: Add required OPPs support to passive governor")
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/governor_passive.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index b094132bd20b..fc09324a03e0 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -65,7 +65,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  		dev_pm_opp_put(p_opp);
>  
>  		if (IS_ERR(opp))
> -			return PTR_ERR(opp);
> +			goto no_required_opp;
>  
>  		*freq = dev_pm_opp_get_freq(opp);
>  		dev_pm_opp_put(opp);
> @@ -73,6 +73,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>  		return 0;
>  	}
>  
> +no_required_opp:
>  	/*
>  	 * Get the OPP table's index of decided frequency by governor
>  	 * of parent device.
> 

Applied it.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 4/4] PM / devfreq: passive: Reduce duplicate code when passive_devfreq case

[Chanwoo Choi]

Hi Matthias,

On 21. 6. 23. 오전 3:35, Matthias Kaehlcke wrote:
> On Thu, Jun 17, 2021 at 03:05:46PM +0900, Chanwoo Choi wrote:
>> In order to keep the consistent coding style between passive_devfreq
>> and passive_cpufreq, use common code for handling required opp property.
>> Also remove the unneed conditional statement and unify the comment
>> of both passive_devfreq and passive_cpufreq when getting the target frequency.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/devfreq/governor_passive.c | 80 ++++++------------------------
>>   1 file changed, 15 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index 07e864509b7e..7102cb7eb30d 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -91,66 +91,17 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>>   	struct devfreq_passive_data *p_data
>>   			= (struct devfreq_passive_data *)devfreq->data;
>>   	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
>> -	unsigned long child_freq = ULONG_MAX;
>> -	struct dev_pm_opp *opp, *p_opp;
>> -	int i, count;
>> -
>> -	/*
>> -	 * If the devfreq device with passive governor has the specific method
>> -	 * to determine the next frequency, should use the get_target_freq()
>> -	 * of struct devfreq_passive_data.
>> -	 */
>> -	if (p_data->get_target_freq)
>> -		return p_data->get_target_freq(devfreq, freq);
>> -
>> -	/*
>> -	 * If the parent and passive devfreq device uses the OPP table,
>> -	 * get the next frequency by using the OPP table.
>> -	 */
>> -
>> -	/*
>> -	 * - parent devfreq device uses the governors except for passive.
>> -	 * - passive devfreq device uses the passive governor.
>> -	 *
>> -	 * Each devfreq has the OPP table. After deciding the new frequency
>> -	 * from the governor of parent devfreq device, the passive governor
>> -	 * need to get the index of new frequency on OPP table of parent
>> -	 * device. And then the index is used for getting the suitable
>> -	 * new frequency for passive devfreq device.
>> -	 */
>> -	if (!devfreq->profile || !devfreq->profile->freq_table
>> -		|| devfreq->profile->max_state <= 0)
>> -		return -EINVAL;
>> -
>> -	/*
>> -	 * The passive governor have to get the correct frequency from OPP
>> -	 * list of parent device. Because in this case, *freq is temporary
>> -	 * value which is decided by ondemand governor.
>> -	 */
>> -	if (devfreq->opp_table && parent_devfreq->opp_table) {
>> -		p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent,
>> -						freq, 0);
>> -		if (IS_ERR(p_opp))
>> -			return PTR_ERR(p_opp);
>> -
>> -		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
>> -						    devfreq->opp_table, p_opp);
>> -		dev_pm_opp_put(p_opp);
>> -
>> -		if (IS_ERR(opp))
>> -			goto no_required_opp;
>> -
>> -		*freq = dev_pm_opp_get_freq(opp);
>> -		dev_pm_opp_put(opp);
>> -
>> -		return 0;
>> -	}
>> +	unsigned long target_freq;
>> +	int i;
>> +
>> +	/* Get target freq via required opps */
>> +	target_freq = get_taget_freq_by_required_opp(parent_devfreq->dev.parent,
>> +						parent_devfreq->opp_table,
>> +						devfreq->opp_table, *freq);
> 
> s/get_taget_freq_by_required_opp/get_target_freq_by_required_opp/
> 
> Also need to be fixed in "[3/4] PM / devfreq: Add cpu based scaling
> support to passive governor".

Thanks for catch. I'll fix it.

> 
>> +	if (target_freq)
>> +		goto out;
>>   
>> -no_required_opp:
>> -	/*
>> -	 * Get the OPP table's index of decided frequency by governor
>> -	 * of parent device.
>> -	 */
>> +	/* Use Interpolation if required opps is not available */
> 
> s/Interpolation/interpolation/

I'll fix it.

> 
>>   	for (i = 0; i < parent_devfreq->profile->max_state; i++)
>>   		if (parent_devfreq->profile->freq_table[i] == *freq)
>>   			break;
>> @@ -158,16 +109,15 @@ static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>>   	if (i == parent_devfreq->profile->max_state)
>>   		return -EINVAL;
>>   
>> -	/* Get the suitable frequency by using index of parent device. */
>>   	if (i < devfreq->profile->max_state) {
>> -		child_freq = devfreq->profile->freq_table[i];
>> +		target_freq = devfreq->profile->freq_table[i];
>>   	} else {
>> -		count = devfreq->profile->max_state;
>> -		child_freq = devfreq->profile->freq_table[count - 1];
>> +		i = devfreq->profile->max_state;
>> +		target_freq = devfreq->profile->freq_table[i - 1];
>>   	}
>>   
>> -	/* Return the suitable frequency for passive device. */
>> -	*freq = child_freq;
>> +out:
>> +	*freq = target_freq;
> 
> You might want to split the child_freq => target_freq and commentary change into
> a separate patch, since it is not directly related with the switch to
> get_target_freq_by_required_opp().

OK. I will not change about child_freq -> target_freq.


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH 2/4] PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq

[Chanwoo Choi]

On 21. 6. 23. 오전 3:23, Matthias Kaehlcke wrote:
> On Thu, Jun 17, 2021 at 03:05:44PM +0900, Chanwoo Choi wrote:
> 
>> Subject: PM / devfreq: Export devfreq_get_freq_ragne symbol within devfreq
> 
> nit: s/ragne/range/
> 
>>
>> In order to get frequency range within devfreq governors,
>> export devfreq_get_freq_ragne symbol within devfreq.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> 

Thanks for the review.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi