[lkml]   [2020]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance
Hi Viresh,

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.

On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote:
> Hello,
> CPPC cpufreq driver is used for ARM servers and this patch series tries
> to provide frequency invariance support for them. The same is also
> provided using a specific hardware extension, known as AMU (Activity
> Monitors Unit), but that is optional for platforms and at least few of
> them don't have it.
> This patchset allows multiple parts of the kernel to provide the same
> functionality, by registering with the topology core.
> This is tested with some hacks, as I didn't have access to the right
> hardware, on the ARM64 hikey board to check the overall functionality
> and that works fine.
> Ionela/Peter Puhov, it would be nice if you guys can give this a shot.

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().

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() or even the more invasive wrapping of the
counter read functions is not working. I believe those solutions
would brings only a fraction of the complexity added through this

- (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.

- (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.

While I'm keen on having invariance support for CPPC when lacking part
or full support for AMUs, I think we should explore alternative designs.
I can try to come up with some code suggestions, but it will take a few
days as I have many other things in the air :).

I'm happy to test this when the design is agreed on.

Hope it helps,

 \ /
  Last update: 2020-07-09 14:44    [W:0.084 / U:2.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site