Thanks for the quick reply Ionela.

On 09-07-20, 13:43, Ionela Voinescu wrote:

I'll put all my comments here for now, as they refer more to the design
of the solution.

I hope it won't be too repetitive compared to what we previously discussed
offline.

I understand you want to get additional points of view.

Not necessarily, I knew you would be one of the major reviewers here
:)

I posted so you don't need to review in private anymore and then the
code is somewhat updated since the previous time.

On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
I believe the code is unnecessarily invasive for the functionality it
tries to introduce and it does break existing functionality.


 - (1) From code readability and design point of view, this switching
       between an architectural method and a driver method complicates
       an already complicated situation. We already have code that
       chooses between a cpufreq-based method and a counter based method
       for frequency invariance. This would basically introduce a choice
       between a cpufreq-based method through arch_set_freq_scale(), an
       architectural counter-based method through arch_set_freq_tick(),
       and another cpufreq-based method that piggy-backs on the
       architectural arch_set_freq_tick().

I agree.

       As discussed offline, before I even try to begin accepting the
       possibility of this complicated mix, I would like to know why
       methods of obtaining the same thing by using the cpufreq
       arch_set_freq_scale()

The problem is same as that was in case of x86, we don't know the real
frequency the CPU may be running at and we need something that fires
up periodically in a guaranteed way to capture the freq-scale.

Though I am thinking now if we can trust the target_index() helper and
keep updating the freq-scale based on the delta between last call to
it and the latest call. I am not sure if it will be sufficient.

       or even the more invasive wrapping of the
       counter read functions is not working.

I am not sure I understood this one.

 - (2) For 1/3, the presence of AMU counters does not guarantee their
       usability for frequency invariance. I know you wanted to avoid
       the complications of AMUs being marked as supporting invariance
       after the cpufreq driver init function, but this breaks the
       scenario in which the maximum frequency is invalid.

Is that really a scenario ? i.e. Invalid maximum frequency ? Why would
that ever happen ?

And I am not sure if this breaks anything which already exists,
because all we are doing in this case now is not registering cppc for
FI, which should be fine.

 - (3) For 2/3, currently we support platforms that have partial support
       for AMUs, while this would not be supported here. The suggestions
       at (1) would give us this for free.

As both were counter based mechanisms, I thought it would be better
and more consistent if only one of them is picked. Though partial
support of AMUs would still work without the CPPC driver.

-- 
viresh