[4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

Message ID 1540830201-2947-4-git-send-email-daniel.lezcano@linaro.org
State New
Headers show
Series
  • [1/4] base/drivers/arch_topology: Remove useless check
Related show

Commit Message

Daniel Lezcano Oct. 29, 2018, 4:23 p.m.
In the case of assymetric SoC with the same micro-architecture, we
have a group of CPUs with smaller OPPs than the other group. One
example is the 96boards dragonboard 820c. There is no dmips/MHz
difference between both groups, so no need to specify the values in
the DT. Unfortunately, without these defined, there is no scaling
capacity comutation triggered, so we need to write
'capacity-dmips-mhz' for each CPU with the same value in order to
force the scaled capacity computation.

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
'capacity-dmips-mhz' is defined in the DT.

This was tested on db820c:
 - specified values in the DT (correct results)
 - partial values defined in the DT (error + fallback to defaults)
 - no specified values in the DT (correct results)

correct results are:
  cat /sys/devices/system/cpu/cpu*/cpu_capacity
   758
   758
  1024
  1024

  ... respectively for CPU0, CPU1, CPU2 and CPU3.

That reflects the capacity for the max frequencies 1593600 and 2150400.

Cc: Chris Redpath <chris.redpath@linaro.org>
Cc: Quentin Perret <quentin.perret@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
Cc: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Viresh Kumar Oct. 30, 2018, 7:13 a.m. | #1
On 29-10-18, 17:23, Daniel Lezcano wrote:
> In the case of assymetric SoC with the same micro-architecture, we

> have a group of CPUs with smaller OPPs than the other group. One

> example is the 96boards dragonboard 820c. There is no dmips/MHz

> difference between both groups, so no need to specify the values in

> the DT. Unfortunately, without these defined, there is no scaling

> capacity comutation triggered, so we need to write

> 'capacity-dmips-mhz' for each CPU with the same value in order to

> force the scaled capacity computation.

> 

> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

> 'capacity-dmips-mhz' is defined in the DT.

> 

> This was tested on db820c:

>  - specified values in the DT (correct results)

>  - partial values defined in the DT (error + fallback to defaults)

>  - no specified values in the DT (correct results)

> 

> correct results are:

>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>    758

>    758

>   1024

>   1024

> 

>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

> 

> That reflects the capacity for the max frequencies 1593600 and 2150400.

> 

> Cc: Chris Redpath <chris.redpath@linaro.org>

> Cc: Quentin Perret <quentin.perret@linaro.org>

> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: Amit Kucheria <amit.kucheria@linaro.org>

> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

> Cc: Niklas Cassel <niklas.cassel@linaro.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

>  1 file changed, 26 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

> index 7311641..7d594a6 100644

> --- a/drivers/base/arch_topology.c

> +++ b/drivers/base/arch_topology.c

> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

>  	.notifier_call = init_cpu_capacity_callback,

>  };

>  

> +static int topology_set_default_capacity(void)

> +{

> +	int cpu;

> +

> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

> +			       GFP_KERNEL);


kmalloc ?

> +	if (!raw_capacity)

> +		return -ENOMEM;

> +

> +	for_each_possible_cpu(cpu)

> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;


It makes it look as if it is important to set default values to
SCHED_CAPACITY_SCALE while that is not the case. Maybe add a comment
that all CPUs are assumed to have same capacity now. Maybe initialize
to 1 instead ? Not sure if that would be better or not.

> +

> +	return 0;

> +}

> +

>  static int __init register_cpufreq_notifier(void)

>  {

>  	int ret;

> @@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)

>  	 * until we have the necessary code to parse the cpu capacity, so

>  	 * skip registering cpufreq notifier.

>  	 */

> -	if (!acpi_disabled || !raw_capacity)

> +	if (!acpi_disabled)

>  		return -EINVAL;

>  

> +	if (!raw_capacity) {

> +

> +		pr_info("cpu_capacity: No capacity defined in DT, set default "

> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);


So we will end up doing this also for the case where the DT is
partially filled with the capacity info and we already printed error
messages for it. Should we really do that ?

-- 
viresh
Daniel Lezcano Oct. 30, 2018, 8:39 a.m. | #2
On 30/10/2018 08:13, Viresh Kumar wrote:
> On 29-10-18, 17:23, Daniel Lezcano wrote:

>> In the case of assymetric SoC with the same micro-architecture, we

>> have a group of CPUs with smaller OPPs than the other group. One

>> example is the 96boards dragonboard 820c. There is no dmips/MHz

>> difference between both groups, so no need to specify the values in

>> the DT. Unfortunately, without these defined, there is no scaling

>> capacity comutation triggered, so we need to write

>> 'capacity-dmips-mhz' for each CPU with the same value in order to

>> force the scaled capacity computation.

>>

>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

>> 'capacity-dmips-mhz' is defined in the DT.

>>

>> This was tested on db820c:

>>  - specified values in the DT (correct results)

>>  - partial values defined in the DT (error + fallback to defaults)

>>  - no specified values in the DT (correct results)

>>

>> correct results are:

>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>>    758

>>    758

>>   1024

>>   1024

>>

>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

>>

>> That reflects the capacity for the max frequencies 1593600 and 2150400.

>>

>> Cc: Chris Redpath <chris.redpath@linaro.org>

>> Cc: Quentin Perret <quentin.perret@linaro.org>

>> Cc: Viresh Kumar <viresh.kumar@linaro.org>

>> Cc: Amit Kucheria <amit.kucheria@linaro.org>

>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

>> Cc: Niklas Cassel <niklas.cassel@linaro.org>

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>> ---

>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

>>  1 file changed, 26 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

>> index 7311641..7d594a6 100644

>> --- a/drivers/base/arch_topology.c

>> +++ b/drivers/base/arch_topology.c

>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

>>  	.notifier_call = init_cpu_capacity_callback,

>>  };

>>  

>> +static int topology_set_default_capacity(void)

>> +{

>> +	int cpu;

>> +

>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

>> +			       GFP_KERNEL);

> 

> kmalloc ?

> 

>> +	if (!raw_capacity)

>> +		return -ENOMEM;

>> +

>> +	for_each_possible_cpu(cpu)

>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

> 

> It makes it look as if it is important to set default values to

> SCHED_CAPACITY_SCALE while that is not the case. Maybe add a comment

> that all CPUs are assumed to have same capacity now. Maybe initialize

> to 1 instead ? Not sure if that would be better or not.


SCHED_CAPACITY_SCALE is the default value in this file.

eg.

DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE
...
pr_err("cpu_capacity: partial information: fallback to 1024 for all
CPUs\n");
...

So I prefer to use the SCHED_CAPACITY_SCALE for consistency.

>> +

>> +	return 0;

>> +}

>> +

>>  static int __init register_cpufreq_notifier(void)

>>  {

>>  	int ret;

>> @@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)

>>  	 * until we have the necessary code to parse the cpu capacity, so

>>  	 * skip registering cpufreq notifier.

>>  	 */

>> -	if (!acpi_disabled || !raw_capacity)

>> +	if (!acpi_disabled)

>>  		return -EINVAL;

>>  

>> +	if (!raw_capacity) {

>> +

>> +		pr_info("cpu_capacity: No capacity defined in DT, set default "

>> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);

> 

> So we will end up doing this also for the case where the DT is

> partially filled with the capacity info and we already printed error

> messages for it. Should we really do that ?


Yes, it is the default behavior as stated in the documentation:

"capacity-dmips-mhz property is all-or-nothing: if it is specified for a
cpu node, it has to be specified for every other cpu nodes, or the
system will fall back to the default capacity value for every CPU."

and also in the error message:

pr_err("cpu_capacity: partial information: fallback to 1024 for all
CPUs\n");

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Viresh Kumar Oct. 30, 2018, 8:45 a.m. | #3
On 30-10-18, 09:39, Daniel Lezcano wrote:
> SCHED_CAPACITY_SCALE is the default value in this file.

> 

> eg.

> 

> DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE

> ...

> pr_err("cpu_capacity: partial information: fallback to 1024 for all

> CPUs\n");

> ...

> 

> So I prefer to use the SCHED_CAPACITY_SCALE for consistency.


> Yes, it is the default behavior as stated in the documentation:

> 

> "capacity-dmips-mhz property is all-or-nothing: if it is specified for a

> cpu node, it has to be specified for every other cpu nodes, or the

> system will fall back to the default capacity value for every CPU."

> 

> and also in the error message:

> 

> pr_err("cpu_capacity: partial information: fallback to 1024 for all

> CPUs\n");


Okay.

-- 
viresh
Viresh Kumar Oct. 30, 2018, 8:58 a.m. | #4
s/dmpis/dmips/ in $subject

On 29-10-18, 17:23, Daniel Lezcano wrote:
> In the case of assymetric SoC with the same micro-architecture, we


                 asymmetric ?

> have a group of CPUs with smaller OPPs than the other group. One

> example is the 96boards dragonboard 820c. There is no dmips/MHz

> difference between both groups, so no need to specify the values in

> the DT. Unfortunately, without these defined, there is no scaling

> capacity comutation triggered, so we need to write


           computation

> 'capacity-dmips-mhz' for each CPU with the same value in order to

> force the scaled capacity computation.

> 

> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

> 'capacity-dmips-mhz' is defined in the DT.

> 

> This was tested on db820c:

>  - specified values in the DT (correct results)

>  - partial values defined in the DT (error + fallback to defaults)

>  - no specified values in the DT (correct results)

> 

> correct results are:

>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>    758

>    758

>   1024

>   1024

> 

>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

> 

> That reflects the capacity for the max frequencies 1593600 and 2150400.

> 

> Cc: Chris Redpath <chris.redpath@linaro.org>

> Cc: Quentin Perret <quentin.perret@linaro.org>

> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: Amit Kucheria <amit.kucheria@linaro.org>

> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

> Cc: Niklas Cassel <niklas.cassel@linaro.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

>  1 file changed, 26 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

> index 7311641..7d594a6 100644

> --- a/drivers/base/arch_topology.c

> +++ b/drivers/base/arch_topology.c

> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

>  	.notifier_call = init_cpu_capacity_callback,

>  };

>  

> +static int topology_set_default_capacity(void)

> +{

> +	int cpu;

> +

> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

> +			       GFP_KERNEL);

> +	if (!raw_capacity)

> +		return -ENOMEM;

> +

> +	for_each_possible_cpu(cpu)

> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;


This isn't actually required as the value of raw_capacity isn't used
anymore after this point in code. Rather it is forcefully updated in
init_cpu_capacity_callback():

raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
                    policy->cpuinfo.max_freq / 1000UL;

Maybe it is better to allocate raw_capacity once at boot and use
another global variable as flag (raw_capacity is used as a flag right
now at many places).

-- 
viresh
Daniel Lezcano Nov. 21, 2018, 10:12 p.m. | #5
On 30/10/2018 09:58, Viresh Kumar wrote:
> s/dmpis/dmips/ in $subject

> 

> On 29-10-18, 17:23, Daniel Lezcano wrote:

>> In the case of assymetric SoC with the same micro-architecture, we

> 

>                  asymmetric ?

> 

>> have a group of CPUs with smaller OPPs than the other group. One

>> example is the 96boards dragonboard 820c. There is no dmips/MHz

>> difference between both groups, so no need to specify the values in

>> the DT. Unfortunately, without these defined, there is no scaling

>> capacity comutation triggered, so we need to write

> 

>            computation

> 

>> 'capacity-dmips-mhz' for each CPU with the same value in order to

>> force the scaled capacity computation.

>>

>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

>> 'capacity-dmips-mhz' is defined in the DT.

>>

>> This was tested on db820c:

>>  - specified values in the DT (correct results)

>>  - partial values defined in the DT (error + fallback to defaults)

>>  - no specified values in the DT (correct results)

>>

>> correct results are:

>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>>    758

>>    758

>>   1024

>>   1024

>>

>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

>>

>> That reflects the capacity for the max frequencies 1593600 and 2150400.

>>

>> Cc: Chris Redpath <chris.redpath@linaro.org>

>> Cc: Quentin Perret <quentin.perret@linaro.org>

>> Cc: Viresh Kumar <viresh.kumar@linaro.org>

>> Cc: Amit Kucheria <amit.kucheria@linaro.org>

>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

>> Cc: Niklas Cassel <niklas.cassel@linaro.org>

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>> ---

>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

>>  1 file changed, 26 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

>> index 7311641..7d594a6 100644

>> --- a/drivers/base/arch_topology.c

>> +++ b/drivers/base/arch_topology.c

>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

>>  	.notifier_call = init_cpu_capacity_callback,

>>  };

>>  

>> +static int topology_set_default_capacity(void)

>> +{

>> +	int cpu;

>> +

>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

>> +			       GFP_KERNEL);

>> +	if (!raw_capacity)

>> +		return -ENOMEM;

>> +

>> +	for_each_possible_cpu(cpu)

>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

> 

> This isn't actually required as the value of raw_capacity isn't used

> anymore after this point in code. Rather it is forcefully updated in

> init_cpu_capacity_callback():

> 

> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *

>                     policy->cpuinfo.max_freq / 1000UL;

> 

> Maybe it is better to allocate raw_capacity once at boot and use

> another global variable as flag (raw_capacity is used as a flag right

> now at many places).


Can we keep the proposed change as is to simply fix the default value?

I want to do a separate change with a raw_capacity rewrite and remove
the workqueue freeing it.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Viresh Kumar Nov. 22, 2018, 4:29 a.m. | #6
On 21-11-18, 23:12, Daniel Lezcano wrote:
> On 30/10/2018 09:58, Viresh Kumar wrote:

> > s/dmpis/dmips/ in $subject

> > 

> > On 29-10-18, 17:23, Daniel Lezcano wrote:

> >> In the case of assymetric SoC with the same micro-architecture, we

> > 

> >                  asymmetric ?

> > 

> >> have a group of CPUs with smaller OPPs than the other group. One

> >> example is the 96boards dragonboard 820c. There is no dmips/MHz

> >> difference between both groups, so no need to specify the values in

> >> the DT. Unfortunately, without these defined, there is no scaling

> >> capacity comutation triggered, so we need to write

> > 

> >            computation

> > 

> >> 'capacity-dmips-mhz' for each CPU with the same value in order to

> >> force the scaled capacity computation.

> >>

> >> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

> >> 'capacity-dmips-mhz' is defined in the DT.

> >>

> >> This was tested on db820c:

> >>  - specified values in the DT (correct results)

> >>  - partial values defined in the DT (error + fallback to defaults)

> >>  - no specified values in the DT (correct results)

> >>

> >> correct results are:

> >>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

> >>    758

> >>    758

> >>   1024

> >>   1024

> >>

> >>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

> >>

> >> That reflects the capacity for the max frequencies 1593600 and 2150400.

> >>

> >> Cc: Chris Redpath <chris.redpath@linaro.org>

> >> Cc: Quentin Perret <quentin.perret@linaro.org>

> >> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> >> Cc: Amit Kucheria <amit.kucheria@linaro.org>

> >> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

> >> Cc: Niklas Cassel <niklas.cassel@linaro.org>

> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> >> ---

> >>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

> >>  1 file changed, 26 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

> >> index 7311641..7d594a6 100644

> >> --- a/drivers/base/arch_topology.c

> >> +++ b/drivers/base/arch_topology.c

> >> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

> >>  	.notifier_call = init_cpu_capacity_callback,

> >>  };

> >>  

> >> +static int topology_set_default_capacity(void)

> >> +{

> >> +	int cpu;

> >> +

> >> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

> >> +			       GFP_KERNEL);

> >> +	if (!raw_capacity)

> >> +		return -ENOMEM;

> >> +

> >> +	for_each_possible_cpu(cpu)

> >> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

> > 

> > This isn't actually required as the value of raw_capacity isn't used

> > anymore after this point in code. Rather it is forcefully updated in

> > init_cpu_capacity_callback():

> > 

> > raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *

> >                     policy->cpuinfo.max_freq / 1000UL;

> > 

> > Maybe it is better to allocate raw_capacity once at boot and use

> > another global variable as flag (raw_capacity is used as a flag right

> > now at many places).

> 

> Can we keep the proposed change as is to simply fix the default value?

> 

> I want to do a separate change with a raw_capacity rewrite and remove

> the workqueue freeing it.


Sure. But you still don't need to update raw_capacity[cpu] above as I pointed
out earlier. You can drop those lines at least.

-- 
viresh
Daniel Lezcano Nov. 22, 2018, 10:29 a.m. | #7
On 22/11/2018 05:29, Viresh Kumar wrote:
> On 21-11-18, 23:12, Daniel Lezcano wrote:

>> On 30/10/2018 09:58, Viresh Kumar wrote:

>>> s/dmpis/dmips/ in $subject

>>>

>>> On 29-10-18, 17:23, Daniel Lezcano wrote:

>>>> In the case of assymetric SoC with the same micro-architecture, we

>>>

>>>                  asymmetric ?

>>>

>>>> have a group of CPUs with smaller OPPs than the other group. One

>>>> example is the 96boards dragonboard 820c. There is no dmips/MHz

>>>> difference between both groups, so no need to specify the values in

>>>> the DT. Unfortunately, without these defined, there is no scaling

>>>> capacity comutation triggered, so we need to write

>>>

>>>            computation

>>>

>>>> 'capacity-dmips-mhz' for each CPU with the same value in order to

>>>> force the scaled capacity computation.

>>>>

>>>> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no

>>>> 'capacity-dmips-mhz' is defined in the DT.

>>>>

>>>> This was tested on db820c:

>>>>  - specified values in the DT (correct results)

>>>>  - partial values defined in the DT (error + fallback to defaults)

>>>>  - no specified values in the DT (correct results)

>>>>

>>>> correct results are:

>>>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>>>>    758

>>>>    758

>>>>   1024

>>>>   1024

>>>>

>>>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

>>>>

>>>> That reflects the capacity for the max frequencies 1593600 and 2150400.

>>>>

>>>> Cc: Chris Redpath <chris.redpath@linaro.org>

>>>> Cc: Quentin Perret <quentin.perret@linaro.org>

>>>> Cc: Viresh Kumar <viresh.kumar@linaro.org>

>>>> Cc: Amit Kucheria <amit.kucheria@linaro.org>

>>>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

>>>> Cc: Niklas Cassel <niklas.cassel@linaro.org>

>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>>>> ---

>>>>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-

>>>>  1 file changed, 26 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

>>>> index 7311641..7d594a6 100644

>>>> --- a/drivers/base/arch_topology.c

>>>> +++ b/drivers/base/arch_topology.c

>>>> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {

>>>>  	.notifier_call = init_cpu_capacity_callback,

>>>>  };

>>>>  

>>>> +static int topology_set_default_capacity(void)

>>>> +{

>>>> +	int cpu;

>>>> +

>>>> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),

>>>> +			       GFP_KERNEL);

>>>> +	if (!raw_capacity)

>>>> +		return -ENOMEM;

>>>> +

>>>> +	for_each_possible_cpu(cpu)

>>>> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

>>>

>>> This isn't actually required as the value of raw_capacity isn't used

>>> anymore after this point in code. Rather it is forcefully updated in

>>> init_cpu_capacity_callback():

>>>

>>> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *

>>>                     policy->cpuinfo.max_freq / 1000UL;

>>>

>>> Maybe it is better to allocate raw_capacity once at boot and use

>>> another global variable as flag (raw_capacity is used as a flag right

>>> now at many places).

>>

>> Can we keep the proposed change as is to simply fix the default value?

>>

>> I want to do a separate change with a raw_capacity rewrite and remove

>> the workqueue freeing it.

> 

> Sure. But you still don't need to update raw_capacity[cpu] above as I pointed

> out earlier. You can drop those lines at least.


Oh ... actually raw_capacity is not needed at all!



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Viresh Kumar Nov. 22, 2018, 10:31 a.m. | #8
On 22-11-18, 11:29, Daniel Lezcano wrote:
> Oh ... actually raw_capacity is not needed at all!


It is required as another routine writes some values to it I believe :)

-- 
viresh
Daniel Lezcano Nov. 22, 2018, 10:32 a.m. | #9
On 22/11/2018 11:31, Viresh Kumar wrote:
> On 22-11-18, 11:29, Daniel Lezcano wrote:

>> Oh ... actually raw_capacity is not needed at all!

> 

> It is required as another routine writes some values to it I believe :)


Let's see if I can remove it.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Daniel Lezcano Nov. 22, 2018, 11:11 a.m. | #10
On 22/11/2018 11:31, Viresh Kumar wrote:
> On 22-11-18, 11:29, Daniel Lezcano wrote:

>> Oh ... actually raw_capacity is not needed at all!

> 

> It is required as another routine writes some values to it I believe :)


Well actually it is accessed 'later' by topology_normalize_cpu_scale()
by a call from arch/arm/kernel/topology.c and arch/arm64/kernel/topology.c

This code deserves a bit of rework, I don't have the time to take care
of it, so I will resend with your suggestion and do some cleanup later.

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Patch

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 7311641..7d594a6 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -205,6 +205,21 @@  static struct notifier_block init_cpu_capacity_notifier = {
 	.notifier_call = init_cpu_capacity_callback,
 };
 
+static int topology_set_default_capacity(void)
+{
+	int cpu;
+
+	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
+			       GFP_KERNEL);
+	if (!raw_capacity)
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu)
+		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;
+
+	return 0;
+}
+
 static int __init register_cpufreq_notifier(void)
 {
 	int ret;
@@ -214,9 +229,19 @@  static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || !raw_capacity)
+	if (!acpi_disabled)
 		return -EINVAL;
 
+	if (!raw_capacity) {
+
+		pr_info("cpu_capacity: No capacity defined in DT, set default "
+		       "values to %ld\n", SCHED_CAPACITY_SCALE);
+
+		ret = topology_set_default_capacity();
+		if (ret)
+			return ret;
+	}
+
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
 		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
 		return -ENOMEM;