| Message ID | 1543325060-1599-2-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Daniel, +cc: Russell King <linux@armlinux.org.uk> On 11/27/18 2:24 PM, 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. [...] I'm afraid that this change is incompatible with the still existing cpu_efficiency interface we have in Arm32 for A15/A7 systems like Arm TC2: In case you specify clock-frequency dt properties per cpu for such a system, the cpu_capacity values are determined via the cpu_efficiency code in arch/arm/kernel/topology.c. So on Arm TC2 with clock-frequency = <1000000000> [A15] and <800000000> [A7] you get: root@linaro-nano:~# cat /sys/devices/system/cpu/cpu*/cpu_capacity 606 1441 1441 606 606 With your patches on top (cpu_capacity functionality in drivers/base/arch_topology.c does not have to be switched on by specifying capacity-dmips-mhz dt properties anymore) we end up scaling by max frequency again: root@linaro-nano:~# cat /sys/devices/system/cpu/cpu*/cpu_capacity 358 1024 1024 358 358 I tried to remove the cpu_efficiency based API a year ago but Russell pointed out that the compatibility has to be maintained for longer: https://lore.kernel.org/lkml/20171024102718.16113-1-dietmar.eggemann@arm.com/ I assume that the capacity-dmips-mhz dt property is like a switch to turn this functionality on for big.Little and so called gold/silver platforms, which have cores with the same uArch but in frequency domains with different max frequency values. So what's wrong with specifying capacity-dmips-mhz = <1024> for all cores for those gold/silver platforms? I don't expect that there will be so many of them. And normal SMP platforms (w/o frequency domains w/o different max frequency values) don't have to execute this code. IMHO, at least we should remove the cpu_efficiency bits before we do this change. [...]
Hi Dietmar, thanks for the review and spotting this. On 03/12/2018 14:46, Dietmar Eggemann wrote: > Hi Daniel, > > +cc: Russell King <linux@armlinux.org.uk> > > On 11/27/18 2:24 PM, 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. > > [...] > > I'm afraid that this change is incompatible with the still existing > cpu_efficiency interface we have in Arm32 for A15/A7 systems like Arm TC2: > > In case you specify clock-frequency dt properties per cpu for such a > system, the cpu_capacity values are determined via the cpu_efficiency > code in arch/arm/kernel/topology.c. > > So on Arm TC2 with clock-frequency = <1000000000> [A15] and <800000000> > [A7] you get: > > root@linaro-nano:~# cat /sys/devices/system/cpu/cpu*/cpu_capacity > 606 > 1441 > 1441 > 606 > 606 > > With your patches on top (cpu_capacity functionality in > drivers/base/arch_topology.c does not have to be switched on by > specifying capacity-dmips-mhz dt properties anymore) we end up scaling > by max frequency again: > > root@linaro-nano:~# cat /sys/devices/system/cpu/cpu*/cpu_capacity > 358 > 1024 > 1024 > 358 > 358 > > I tried to remove the cpu_efficiency based API a year ago but Russell > pointed out that the compatibility has to be maintained for longer: > > https://lore.kernel.org/lkml/20171024102718.16113-1-dietmar.eggemann@arm.com/ > > > I assume that the capacity-dmips-mhz dt property is like a switch to > turn this functionality on for big.Little and so called gold/silver > platforms, which have cores with the same uArch but in frequency domains > with different max frequency values. > > So what's wrong with specifying capacity-dmips-mhz = <1024> for all > cores for those gold/silver platforms? There is nothing wrong, I just don't like to specify in a DT a default values. > I don't expect that there will be > so many of them. And normal SMP platforms (w/o frequency domains w/o > different max frequency values) don't have to execute this code. Fair enough, I will send a DT change, I'm tired of playing mikado with this code. Thanks again for the review. -- Daniel -- <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
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..696cea5 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -243,9 +243,16 @@ 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) { + raw_capacity = kmalloc_array(num_possible_cpus(), + sizeof(*raw_capacity), GFP_KERNEL); + if (!raw_capacity) + return -ENOMEM; + } + if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) { pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n"); return -ENOMEM;