linux-kernel - Re: [PATCH v1] arch_topology: Adjust initial CPU capacities with current freq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5E193828.1070000@rock-chips.com>
Date:   Sat, 11 Jan 2020 10:51:20 +0800
From:   JeffyChen <jeffy.chen@...k-chips.com>
To:     Robin Murphy <robin.murphy@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Sudeep Holla <sudeep.holla@....com>
CC:     linux-kernel@...r.kernel.org,
        Brian Norris <briannorris@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Douglas Anderson <dianders@...omium.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v1] arch_topology: Adjust initial CPU capacities with
 current freq

Hi Robin,

Thanks for the clarification :)

On 01/10/2020 08:28 PM, Robin Murphy wrote:
> On 2020-01-10 12:01 pm, Dietmar Eggemann wrote:
>> On 10/01/2020 12:37, Sudeep Holla wrote:
>>> On Thu, Jan 09, 2020 at 03:52:14PM +0800, Jeffy Chen wrote:
>>>> The CPU freqs are not supposed to change before cpufreq policies
>>>> properly registered, meaning that they should be used to calculate the
>>>> initial CPU capacities.
>>>>
>>>> Doing this helps choosing the best CPU during early boot, especially
>>>> for the initramfs decompressing.
>>>>
>>>> Signed-off-by: Jeffy Chen <jeffy.chen@...k-chips.com>
>>>
>>> [...]
>>>
>>>> @@ -146,10 +153,15 @@ bool __init topology_parse_cpu_capacity(struct
>>>> device_node *cpu_node, int cpu)
>>>>                   return false;
>>>>               }
>>>>           }
>>>> -        capacity_scale = max(cpu_capacity, capacity_scale);
>>>>           raw_capacity[cpu] = cpu_capacity;
>>>>           pr_debug("cpu_capacity: %pOF cpu_capacity=%u (raw)\n",
>>>>               cpu_node, raw_capacity[cpu]);
>>>> +
>>>> +        cpu_clk = of_clk_get(cpu_node, 0);
>>>> +        if (!PTR_ERR_OR_ZERO(cpu_clk))
>>>> +            per_cpu(max_freq, cpu) = clk_get_rate(cpu_clk) / 1000;
>>>> +
>>>> +        clk_put(cpu_clk);
>>>
>>> I don't like to assume DVFS to be supplied only using 'clk'. So NACK!
>>> We have other non-clk mechanism for CPU DVFS and this needs to simply
>>> use cpufreq APIs to get frequency value if required.
>>
>> To support this, it's failing on my Arm64 Juno board.
>>
>> ...
>> [    0.084858] CPU1 cpu_clk=-517
>> [    0.087961] CPU2 cpu_clk=-517
>> [    0.091005] CPU0 cpu_clk=-517
>> [    0.094121] CPU3 cpu_clk=-517
>> [    0.097248] CPU4 cpu_clk=-517
>> [    0.100415] CPU5 cpu_clk=-517

It there any other way to get the initial cpu capacity for this case?

Or can we just assuming all the cores running at the same freq here?

>> ...
>>
>> Since you're on a big.LITTLE platform, did you specify
>> 'capacity-dmips-mhz' for CPUs to be able to distinguish big and little
>> CPUs before CPUfreq kicks in?
>
> Indeed, and that's the "problem" - the capacities are there, but with
> the broken firmware the kernel starts with the little (boot) cluster
> clocked at either 400 or 200MHz, but the big cluster at just 12MHz. At
> that speed, a full distro config can take about 3 minutes to get to the
> point of loading cpufreq as a module, and I've seen at least one distro
> reverting 97df3aa76b4a to 'fix' the symptom :(

Right, for the big cluster, the bootrom(maskrom) will init the clock to 
24MHz, and if the bootloader(u-boot for example) doesn't bump it, it 
would become 12MHz after kernel initialized the whole clk tree.

And in rockchip's BSP 4.4 kernel, there are hacks to bump it to 
800MHz(higher freq might require regulator changing) in clk tree 
initialization, the BSP u-boot also added that recently.

The chromeos's coreboot looks fine, but upstream u-boot seems missing 
that part too, i'll try to send a patch for that :)

>
> Robin.
>
>>
>> $ grep capacity-dmips-mhz ./arch/arm64/boot/dts/arm/juno.dts
>>             capacity-dmips-mhz = <1024>;
>>             capacity-dmips-mhz = <1024>;
>>             capacity-dmips-mhz = <578>;
>>             capacity-dmips-mhz = <578>;
>>             capacity-dmips-mhz = <578>;
>>             capacity-dmips-mhz = <578>;
>>
>
>
>
>