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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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().
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 <[email protected]>
>> ---
>> 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