Re: [PATCH] arm64: topology: Cleanup init_amu_fie() a bit

[Ionela Voinescu]

Hi,

On Thursday 10 Dec 2020 at 12:48:20 (+0530), Viresh Kumar wrote:
> Every time I have stumbled upon this routine, I get confused with the
> way 'have_policy' is used and I have to dig in to understand why is it
> so.
> 

I'm first of all replying to discuss the need of this policy analysis
that enable_policy_freq_counters() does which results in the setting of
have_policy.

Basically, that's functions purpose is only to make sure that invariance
at the level of the policy is consistent: either all CPUs in a policy
support counters and counters will be used for the scale factor, or
either none or only some support counters and therefore the default
cpufreq implementation will be used (arch_set_freq_scale()) for all CPUs
in a policy.

If we find that cpufreq policies are not present at all, we only enable
counter use if all CPUs support them.

This being said, there is a very important part of your patches in this
set:

+	/* Disallow partial use of counters for frequency invariance */
+	if (!cpumask_equal(amu_fie_cpus, cpu_present_mask))
+		goto free_valid_mask;


If this is agreed upon, there is a lot more that can be removed from the
initialisation: enable_policy_freq_counters() can entirely go away plus
all the checks around it.

I completely agree that all of this will be more clear if we decided to
"Disallow partial use of counters for frequency invariance", but I think
we might have to add it back in again when systems with partial support
for counters show up.

I don't agree to not support these systems from the get go as it's
likely that the first big.LITTLE systems will only have partial support
for AMUs, but it's only an assumption at this point.

I'm happy to hear the opinion of other arm64 folk about this.

Many thanks for looking over the code,
Ionela.