Re: [PATCH] arm64: topology: Don't support AMU without cpufreq

From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH] arm64: topology: Don't support AMU without cpufreq
Date: Thu, 9 Jul 2020 13:46:30 +0100	[thread overview]
Message-ID: <20200709124630.GB15342@arm.com> (raw)
In-Reply-To: <20200709104048.emwuquj2qkyascb3@vireshk-i7>

Hey,

On Thursday 09 Jul 2020 at 16:10:48 (+0530), Viresh Kumar wrote:
[..]
> > I agree that this happening is a cornercase and a reason for which
> > cpufreq_get_hw_max_freq() was made weak. If some platform has entirely
> > firmware driven frequency control, but it enables CONFIG_CPU_FREQ
> > (as is the default) and it defines its own cpufreq_get_hw_max_freq(),
> > it could benefit from AMU use.
> > 
> > So I did believe it was best for these checks to be decoupled, for this
> > reason, and potential other reasons in the future, involving more
> > decoupling from cpufreq.
> > 
> > I do have code in progress to clean the overall interaction between
> > cpufreq and AMUs, started at [1]. Bear with me on this, it is all
> > connected :).
> 
> Of course I missed few things here.
> 
> - I didn't realize that cpufreq_get_hw_max_freq() is defined weak :(
> 
>   I understand that we want to support everything that is possible,
>   but there is no need to support cases which we may never have
>   actually. We have seen code going in the kernel, which no one ever
>   ends up using.
> 
>   Do we see a case in near future where someone is going to override
>   this weak implementation ? If we don't have an actual target for it
>   at the moment, then we should probably remove the weak attribute and
>   simplify the code.
> 

I saw this case during FVP testing, although I acknowledge the 'virtual'
part of that platform [1]. But allowing this does enable AMU testing on
an AEM FVP.

While I completely understand the reasoning behind avoiding to introduce
large changes for small corner-case gains, the arguments for this
support was:
 - (1) AMUs are a new feature and it will take some time until we see the
   real usecases. That's always the case with early support for a
   feature - we want to add it early to enable its use and testing, but
   it will take some time to establish the true usecases.
 - (2) It literally needed 2 lines of code + the weak cpufreq function
   to support this.

> - I understood earlier that, we don't pick up AMU support unless all
>   CPUs of a policy are supported by AMUs, but forgot that later while
>   writing the patch. What is the thing with AMUs? Why would some
>   platform add it only for some CPUs out of a policy ? Do we have such
>   platforms already or in queue ?
> 

Given that I can't guarantee what hardware will or won't do, and given
that AMUs are an optional feature, I controlled the only thing I could:
the software :). By not making assumptions about the hardware, I ensured
that the code does not break the interaction between cpufreq use or AMU
use for frequency invariance.

This will be nicer in the new code as the control will be at CPU level,
rather than policy level.

[1]
https://developer.arm.com/tools-and-software/simulation-models/fixed-virtual-platforms

Regards,
Ionela.

> Lets discuss more after we have settled on the first point here.
> 
> Thanks for review Ionela.
> 
> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel