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

From: Robin Murphy <robin.murphy@arm.com>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: linux-kernel@vger.kernel.org,
	Brian Norris <briannorris@chromium.org>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Douglas Anderson <dianders@chromium.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH v1] arch_topology: Adjust initial CPU capacities with current freq
Date: Fri, 10 Jan 2020 12:28:46 +0000	[thread overview]
Message-ID: <15ab46e5-a2b4-eb96-1217-2b2ef8827f64@arm.com> (raw)
In-Reply-To: <5475692c-e72b-74c1-bd6e-95278703249b@arm.com>

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@rock-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
> ...
> 
> 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 :(

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>;
> 