Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 27-11-18, 09:09, Quentin Perret wrote:
> On Tuesday 27 Nov 2018 at 09:27:35 (+0530), Viresh Kumar wrote:
> > On 26-11-18, 13:20, Daniel Lezcano wrote:
> > > diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> > > b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > index 84262cd..f53a3c9 100644
> > > --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. 
> > > If cpufreq is not
> > >  available, final capacities are calculated by directly using 
> > > capacity-dmips-
> > >  mhz values (normalized w.r.t. the highest value found while parsing the 
> > > DT).
> > >  
> > > +If capacity-dmips-mhz is not specified or if the parsing fails, the
> > > +default capacity value will be computed against the highest frequency.
> > > +When all CPUs have the same OPP, they will have the same capacity
> > > +value otherwise the capacity will be scaled down for CPUs having lower
> > > +frequencies.
> > 
> > I know you added this based on Quentin's feedback, but I wonder if this is
> > really required and if it is improving anything at all. This is what the
> > documentation says currently without this patch:
> > 
> > "
> > capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> > representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, 
> > the
> > maximum frequency available to the cpu is then used to calculate the 
> > capacity
> > value internally used by the kernel.
> > 
> > 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. If cpufreq is not
> > available, final capacities are calculated by directly using capacity-dmips-
> > mhz values (normalized w.r.t. the highest value found while parsing the DT).
> > "
> > 
> > So it already clearly says two things:
> > - If all CPUs don't have this property, we fallback to default capacity for
> >   every CPU.
> 
> Which is not what we do with this patch any more. We fallback to
> scaling with frequency.

I read it as that the documentation always said that OS shall do freq based
normalization at boot time and we weren't doing the right thing earlier. On
partially filled DT we used to fallback to the default capacity but never did
the freq based normalization, which was the second step to be done after reading
the capacity-dmips-mhz value.

Anyway, its fine if all other are in sync to get this included :)

-- 
viresh

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 27-11-18, 09:09, Quentin Perret wrote:
> On Tuesday 27 Nov 2018 at 09:27:35 (+0530), Viresh Kumar wrote:
> > On 26-11-18, 13:20, Daniel Lezcano wrote:
> > > diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> > > b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > index 84262cd..f53a3c9 100644
> > > --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > > @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. 
> > > If cpufreq is not
> > >  available, final capacities are calculated by directly using 
> > > capacity-dmips-
> > >  mhz values (normalized w.r.t. the highest value found while parsing the 
> > > DT).
> > >  
> > > +If capacity-dmips-mhz is not specified or if the parsing fails, the
> > > +default capacity value will be computed against the highest frequency.
> > > +When all CPUs have the same OPP, they will have the same capacity
> > > +value otherwise the capacity will be scaled down for CPUs having lower
> > > +frequencies.
> > 
> > I know you added this based on Quentin's feedback, but I wonder if this is
> > really required and if it is improving anything at all. This is what the
> > documentation says currently without this patch:
> > 
> > "
> > capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> > representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, 
> > the
> > maximum frequency available to the cpu is then used to calculate the 
> > capacity
> > value internally used by the kernel.
> > 
> > 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. If cpufreq is not
> > available, final capacities are calculated by directly using capacity-dmips-
> > mhz values (normalized w.r.t. the highest value found while parsing the DT).
> > "
> > 
> > So it already clearly says two things:
> > - If all CPUs don't have this property, we fallback to default capacity for
> >   every CPU.
> 
> Which is not what we do with this patch any more. We fallback to
> scaling with frequency.

I read it as that the documentation always said that OS shall do freq based
normalization at boot time and we weren't doing the right thing earlier. On
partially filled DT we used to fallback to the default capacity but never did
the freq based normalization, which was the second step to be done after reading
the capacity-dmips-mhz value.

Anyway, its fine if all other are in sync to get this included :)

-- 
viresh

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Tuesday 27 Nov 2018 at 09:27:35 (+0530), Viresh Kumar wrote:
> On 26-11-18, 13:20, Daniel Lezcano wrote:
> > diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> > b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > index 84262cd..f53a3c9 100644
> > --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. 
> > If cpufreq is not
> >  available, final capacities are calculated by directly using 
> > capacity-dmips-
> >  mhz values (normalized w.r.t. the highest value found while parsing the 
> > DT).
> >  
> > +If capacity-dmips-mhz is not specified or if the parsing fails, the
> > +default capacity value will be computed against the highest frequency.
> > +When all CPUs have the same OPP, they will have the same capacity
> > +value otherwise the capacity will be scaled down for CPUs having lower
> > +frequencies.
> 
> I know you added this based on Quentin's feedback, but I wonder if this is
> really required and if it is improving anything at all. This is what the
> documentation says currently without this patch:
> 
> "
> capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
> maximum frequency available to the cpu is then used to calculate the capacity
> value internally used by the kernel.
> 
> 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. If cpufreq is not
> available, final capacities are calculated by directly using capacity-dmips-
> mhz values (normalized w.r.t. the highest value found while parsing the DT).
> "
> 
> So it already clearly says two things:
> - If all CPUs don't have this property, we fallback to default capacity for
>   every CPU.

Which is not what we do with this patch any more. We fallback to
scaling with frequency. So I do think the doc needs updating one way or
another. You could define more clearly what "default capacity" means if
you want and say that is scaled with frequency, that'd be fine by me.

Thanks,
Quentin

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Tuesday 27 Nov 2018 at 09:27:35 (+0530), Viresh Kumar wrote:
> On 26-11-18, 13:20, Daniel Lezcano wrote:
> > diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> > b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > index 84262cd..f53a3c9 100644
> > --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. 
> > If cpufreq is not
> >  available, final capacities are calculated by directly using 
> > capacity-dmips-
> >  mhz values (normalized w.r.t. the highest value found while parsing the 
> > DT).
> >  
> > +If capacity-dmips-mhz is not specified or if the parsing fails, the
> > +default capacity value will be computed against the highest frequency.
> > +When all CPUs have the same OPP, they will have the same capacity
> > +value otherwise the capacity will be scaled down for CPUs having lower
> > +frequencies.
> 
> I know you added this based on Quentin's feedback, but I wonder if this is
> really required and if it is improving anything at all. This is what the
> documentation says currently without this patch:
> 
> "
> capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
> maximum frequency available to the cpu is then used to calculate the capacity
> value internally used by the kernel.
> 
> 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. If cpufreq is not
> available, final capacities are calculated by directly using capacity-dmips-
> mhz values (normalized w.r.t. the highest value found while parsing the DT).
> "
> 
> So it already clearly says two things:
> - If all CPUs don't have this property, we fallback to default capacity for
>   every CPU.

Which is not what we do with this patch any more. We fallback to
scaling with frequency. So I do think the doc needs updating one way or
another. You could define more clearly what "default capacity" means if
you want and say that is scaled with frequency, that'd be fine by me.

Thanks,
Quentin

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 27/11/2018 04:57, Viresh Kumar wrote:
> On 26-11-18, 13:20, Daniel Lezcano wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
>> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> index 84262cd..f53a3c9 100644
>> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
>> cpufreq is not
>>  available, final capacities are calculated by directly using capacity-dmips-
>>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>>  
>> +If capacity-dmips-mhz is not specified or if the parsing fails, the
>> +default capacity value will be computed against the highest frequency.
>> +When all CPUs have the same OPP, they will have the same capacity
>> +value otherwise the capacity will be scaled down for CPUs having lower
>> +frequencies.
> 
> I know you added this based on Quentin's feedback, but I wonder if this is
> really required and if it is improving anything at all. This is what the
> documentation says currently without this patch:
> 
> "
> capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
> maximum frequency available to the cpu is then used to calculate the capacity
> value internally used by the kernel.
> 
> 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. If cpufreq is not
> available, final capacities are calculated by directly using capacity-dmips-
> mhz values (normalized w.r.t. the highest value found while parsing the DT).
> "
> 
> So it already clearly says two things:
> - If all CPUs don't have this property, we fallback to default capacity for
>   every CPU.
> - And the OS may also normalize the capacity based on the maximum frequency.
> 
> What more do we want to add here ?

I think what is new is the silver-gold platform. I agree the description
above gives us the information but in a condensed way. With this extra
paragraph we elaborate a bit and make it more clear for SMP/AMP systems.

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 27/11/2018 04:57, Viresh Kumar wrote:
> On 26-11-18, 13:20, Daniel Lezcano wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
>> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> index 84262cd..f53a3c9 100644
>> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
>> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
>> cpufreq is not
>>  available, final capacities are calculated by directly using capacity-dmips-
>>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>>  
>> +If capacity-dmips-mhz is not specified or if the parsing fails, the
>> +default capacity value will be computed against the highest frequency.
>> +When all CPUs have the same OPP, they will have the same capacity
>> +value otherwise the capacity will be scaled down for CPUs having lower
>> +frequencies.
> 
> I know you added this based on Quentin's feedback, but I wonder if this is
> really required and if it is improving anything at all. This is what the
> documentation says currently without this patch:
> 
> "
> capacity-dmips-mhz is an optional cpu node [1] property: u32 value
> representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
> maximum frequency available to the cpu is then used to calculate the capacity
> value internally used by the kernel.
> 
> 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. If cpufreq is not
> available, final capacities are calculated by directly using capacity-dmips-
> mhz values (normalized w.r.t. the highest value found while parsing the DT).
> "
> 
> So it already clearly says two things:
> - If all CPUs don't have this property, we fallback to default capacity for
>   every CPU.
> - And the OS may also normalize the capacity based on the maximum frequency.
> 
> What more do we want to add here ?

I think what is new is the silver-gold platform. I agree the description
above gives us the information but in a condensed way. With this extra
paragraph we elaborate a bit and make it more clear for SMP/AMP systems.

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 26-11-18, 13:20, Daniel Lezcano wrote:
> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> index 84262cd..f53a3c9 100644
> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
> cpufreq is not
>  available, final capacities are calculated by directly using capacity-dmips-
>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>  
> +If capacity-dmips-mhz is not specified or if the parsing fails, the
> +default capacity value will be computed against the highest frequency.
> +When all CPUs have the same OPP, they will have the same capacity
> +value otherwise the capacity will be scaled down for CPUs having lower
> +frequencies.

I know you added this based on Quentin's feedback, but I wonder if this is
really required and if it is improving anything at all. This is what the
documentation says currently without this patch:

"
capacity-dmips-mhz is an optional cpu node [1] property: u32 value
representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
maximum frequency available to the cpu is then used to calculate the capacity
value internally used by the kernel.

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. If cpufreq is not
available, final capacities are calculated by directly using capacity-dmips-
mhz values (normalized w.r.t. the highest value found while parsing the DT).
"

So it already clearly says two things:
- If all CPUs don't have this property, we fallback to default capacity for
  every CPU.
- And the OS may also normalize the capacity based on the maximum frequency.

What more do we want to add here ?

-- 
viresh

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 26-11-18, 13:20, Daniel Lezcano wrote:
> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> index 84262cd..f53a3c9 100644
> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
> cpufreq is not
>  available, final capacities are calculated by directly using capacity-dmips-
>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>  
> +If capacity-dmips-mhz is not specified or if the parsing fails, the
> +default capacity value will be computed against the highest frequency.
> +When all CPUs have the same OPP, they will have the same capacity
> +value otherwise the capacity will be scaled down for CPUs having lower
> +frequencies.

I know you added this based on Quentin's feedback, but I wonder if this is
really required and if it is improving anything at all. This is what the
documentation says currently without this patch:

"
capacity-dmips-mhz is an optional cpu node [1] property: u32 value
representing CPU capacity expressed in normalized DMIPS/MHz. At boot time, the
maximum frequency available to the cpu is then used to calculate the capacity
value internally used by the kernel.

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. If cpufreq is not
available, final capacities are calculated by directly using capacity-dmips-
mhz values (normalized w.r.t. the highest value found while parsing the DT).
"

So it already clearly says two things:
- If all CPUs don't have this property, we fallback to default capacity for
  every CPU.
- And the OS may also normalize the capacity based on the maximum frequency.

What more do we want to add here ?

-- 
viresh

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Rob Herring]

On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
> In the case of asymmetric 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 computation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> In order to fix this situation, allocate 'raw_capacity' so the pointer
> is set and the init_cpu_capacity_callback() function can be called.
> 
> 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 
> Cc: Quentin Perret 
> Cc: Viresh Kumar 
> Cc: Amit Kucheria 
> Cc: Nicolas Dechesne 
> Cc: Niklas Cassel 
> Reviewed-by: Viresh Kumar 
> Signed-off-by: Daniel Lezcano 
> ---
>  Documentation/devicetree/bindings/arm/cpu-capacity.txt |  6 ++

Acked-by: Rob Herring 

>  drivers/base/arch_topology.c   | 13 -
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> index 84262cd..f53a3c9 100644
> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
> cpufreq is not
>  available, final capacities are calculated by directly using capacity-dmips-
>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>  
> +If capacity-dmips-mhz is not specified or if the parsing fails, the
> +default capacity value will be computed against the highest frequency.
> +When all CPUs have the same OPP, they will have the same capacity
> +value otherwise the capacity will be scaled down for CPUs having lower
> +frequencies.
> +
>  ===
>  4 - Examples
>  ===
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fd5325b..e0c5b60 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);
> +
> + raw_capacity = kmalloc_array(num_possible_cpus(),
> +  sizeof(*raw_capacity), GFP_KERNEL);
> + if (!raw_capacity)
> + return -ENOMEM;
> + }
> +
>   if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>   pr_err("cpu_capacity: failed to allocate memory for 
> cpus_to_visit\n");
>   return -ENOMEM;
> -- 
> 2.7.4
> 

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Rob Herring]

On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
> In the case of asymmetric 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 computation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> In order to fix this situation, allocate 'raw_capacity' so the pointer
> is set and the init_cpu_capacity_callback() function can be called.
> 
> 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 
> Cc: Quentin Perret 
> Cc: Viresh Kumar 
> Cc: Amit Kucheria 
> Cc: Nicolas Dechesne 
> Cc: Niklas Cassel 
> Reviewed-by: Viresh Kumar 
> Signed-off-by: Daniel Lezcano 
> ---
>  Documentation/devicetree/bindings/arm/cpu-capacity.txt |  6 ++

Acked-by: Rob Herring 

>  drivers/base/arch_topology.c   | 13 -
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpu-capacity.txt 
> b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> index 84262cd..f53a3c9 100644
> --- a/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> +++ b/Documentation/devicetree/bindings/arm/cpu-capacity.txt
> @@ -54,6 +54,12 @@ fall back to the default capacity value for every CPU. If 
> cpufreq is not
>  available, final capacities are calculated by directly using capacity-dmips-
>  mhz values (normalized w.r.t. the highest value found while parsing the DT).
>  
> +If capacity-dmips-mhz is not specified or if the parsing fails, the
> +default capacity value will be computed against the highest frequency.
> +When all CPUs have the same OPP, they will have the same capacity
> +value otherwise the capacity will be scaled down for CPUs having lower
> +frequencies.
> +
>  ===
>  4 - Examples
>  ===
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fd5325b..e0c5b60 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);
> +
> + raw_capacity = kmalloc_array(num_possible_cpus(),
> +  sizeof(*raw_capacity), GFP_KERNEL);
> + if (!raw_capacity)
> + return -ENOMEM;
> + }
> +
>   if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>   pr_err("cpu_capacity: failed to allocate memory for 
> cpus_to_visit\n");
>   return -ENOMEM;
> -- 
> 2.7.4
> 

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

Hi Greg,

thanks for the review.

On 26/11/2018 16:06, Greg Kroah-Hartman wrote:
> On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -243,9 +243,20 @@ 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);
> 
> Why the extra blank line?
> 
> And what is userspace going to do with this noise?  Is this an error?
> Just normal operation?  A device should never be saying anything to the
> log for normal boot functionality.  When is this called?

It is not an error but a fallback path. It is called at init time when
the cpufreq notifier is called and when either the DT read failed or
nothing is specified. I agree this is noise, I will remove the trace.

> And no need for the "cpu_capacity:" right?  Shouldn't the pr_info() line
> handle the prefix for you?

Ah, right I did not pay attention to the prefix and blindly copied the
line from somewhere else. I think it is better to drop this trace in any
case.

I will provide a patch setting the pr_fmt in a separate series.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

Hi Greg,

thanks for the review.

On 26/11/2018 16:06, Greg Kroah-Hartman wrote:
> On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -243,9 +243,20 @@ 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);
> 
> Why the extra blank line?
> 
> And what is userspace going to do with this noise?  Is this an error?
> Just normal operation?  A device should never be saying anything to the
> log for normal boot functionality.  When is this called?

It is not an error but a fallback path. It is called at init time when
the cpufreq notifier is called and when either the DT read failed or
nothing is specified. I agree this is noise, I will remove the trace.

> And no need for the "cpu_capacity:" right?  Shouldn't the pr_info() line
> handle the prefix for you?

Ah, right I did not pay attention to the prefix and blindly copied the
line from somewhere else. I think it is better to drop this trace in any
case.

I will provide a patch setting the pr_fmt in a separate series.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Greg Kroah-Hartman]

On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);

Why the extra blank line?

And what is userspace going to do with this noise?  Is this an error?
Just normal operation?  A device should never be saying anything to the
log for normal boot functionality.  When is this called?

And no need for the "cpu_capacity:" right?  Shouldn't the pr_info() line
handle the prefix for you?

thanks,

greg k-h

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Greg Kroah-Hartman]

On Mon, Nov 26, 2018 at 01:20:43PM +0100, Daniel Lezcano wrote:
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);

Why the extra blank line?

And what is userspace going to do with this noise?  Is this an error?
Just normal operation?  A device should never be saying anything to the
log for normal boot functionality.  When is this called?

And no need for the "cpu_capacity:" right?  Shouldn't the pr_info() line
handle the prefix for you?

thanks,

greg k-h

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 26/11/2018 13:48, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 13:20:43 (+0100), Daniel Lezcano wrote:
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index fd5325b..e0c5b60 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -243,9 +243,20 @@ 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);
>> +
>> +raw_capacity = kmalloc_array(num_possible_cpus(),
>> + sizeof(*raw_capacity), GFP_KERNEL);
>> +if (!raw_capacity)
>> +return -ENOMEM;
>> +}
>> +
>>  if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>>  pr_err("cpu_capacity: failed to allocate memory for 
>> cpus_to_visit\n");
>>  return -ENOMEM;
> 
> I just tried on my Juno by removing the capacity-dmips-mhz values from
> the DT and got the following:
> 
>   $ cat /sys/devices/system/cpu/cpufreq/policy*/scaling_available_frequencies
>   45 575000 70 775000 85
>   45 625000 80 95 110
>   $ cat /sys/devices/system/cpu/cpu*/cpu_capacity
>   791
>   1024
>   1024
>   791
>   791
>   791
> 
> Same thing with a partially-filled DT (which is the expected behaviour
> now). So feel free to add:
> 
> Tested-by: Quentin Perret 

Thanks for testing!


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 26/11/2018 13:48, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 13:20:43 (+0100), Daniel Lezcano wrote:
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index fd5325b..e0c5b60 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -243,9 +243,20 @@ 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);
>> +
>> +raw_capacity = kmalloc_array(num_possible_cpus(),
>> + sizeof(*raw_capacity), GFP_KERNEL);
>> +if (!raw_capacity)
>> +return -ENOMEM;
>> +}
>> +
>>  if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>>  pr_err("cpu_capacity: failed to allocate memory for 
>> cpus_to_visit\n");
>>  return -ENOMEM;
> 
> I just tried on my Juno by removing the capacity-dmips-mhz values from
> the DT and got the following:
> 
>   $ cat /sys/devices/system/cpu/cpufreq/policy*/scaling_available_frequencies
>   45 575000 70 775000 85
>   45 625000 80 95 110
>   $ cat /sys/devices/system/cpu/cpu*/cpu_capacity
>   791
>   1024
>   1024
>   791
>   791
>   791
> 
> Same thing with a partially-filled DT (which is the expected behaviour
> now). So feel free to add:
> 
> Tested-by: Quentin Perret 

Thanks for testing!


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Monday 26 Nov 2018 at 13:20:43 (+0100), Daniel Lezcano wrote:
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fd5325b..e0c5b60 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);
> +
> + raw_capacity = kmalloc_array(num_possible_cpus(),
> +  sizeof(*raw_capacity), GFP_KERNEL);
> + if (!raw_capacity)
> + return -ENOMEM;
> + }
> +
>   if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>   pr_err("cpu_capacity: failed to allocate memory for 
> cpus_to_visit\n");
>   return -ENOMEM;

I just tried on my Juno by removing the capacity-dmips-mhz values from
the DT and got the following:

  $ cat /sys/devices/system/cpu/cpufreq/policy*/scaling_available_frequencies
  45 575000 70 775000 85
  45 625000 80 95 110
  $ cat /sys/devices/system/cpu/cpu*/cpu_capacity
  791
  1024
  1024
  791
  791
  791

Same thing with a partially-filled DT (which is the expected behaviour
now). So feel free to add:

Tested-by: Quentin Perret 

Thanks,
Quentin

Re: [PATCH V4 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Monday 26 Nov 2018 at 13:20:43 (+0100), Daniel Lezcano wrote:
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fd5325b..e0c5b60 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ 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);
> +
> + raw_capacity = kmalloc_array(num_possible_cpus(),
> +  sizeof(*raw_capacity), GFP_KERNEL);
> + if (!raw_capacity)
> + return -ENOMEM;
> + }
> +
>   if (!alloc_cpumask_var(_to_visit, GFP_KERNEL)) {
>   pr_err("cpu_capacity: failed to allocate memory for 
> cpus_to_visit\n");
>   return -ENOMEM;

I just tried on my Juno by removing the capacity-dmips-mhz values from
the DT and got the following:

  $ cat /sys/devices/system/cpu/cpufreq/policy*/scaling_available_frequencies
  45 575000 70 775000 85
  45 625000 80 95 110
  $ cat /sys/devices/system/cpu/cpu*/cpu_capacity
  791
  1024
  1024
  791
  791
  791

Same thing with a partially-filled DT (which is the expected behaviour
now). So feel free to add:

Tested-by: Quentin Perret 

Thanks,
Quentin