mbox series

[RFC,v3,0/2] Add Krait Cache Scaling support

Message ID 20200821140026.19643-1-ansuelsmth@gmail.com
Headers show
Series Add Krait Cache Scaling support | expand

Message

Christian Marangi Aug. 21, 2020, 2 p.m. UTC
This adds Krait Cache scaling support using the cpufreq notifier.
I have some doubt about where this should be actually placed (clk or cpufreq)?
Also the original idea was to create a dedicated cpufreq driver (like it's done in
the codeaurora qcom repo) by copying the cpufreq-dt driver and adding the cache
scaling logic but i still don't know what is better. Have a very similar driver or
add a dedicated driver only for the cache using the cpufreq notifier and do the
scale on every freq transition.
Thanks to everyone who will review or answer these questions.

v3:
* Use opp infrastructure
* Update documentation

v2:
* Fix Documentation error reported by bot
* Rework code to fail probe on missing required params
* Optimize notifier callback to reduce CPU cycle

Ansuel Smith (2):
  cpufreq: qcom: Add Krait Cache Scaling support
  dt-bindings: cpufreq: Document Krait CPU Cache scaling

 .../bindings/cpufreq/krait-cache-scale.yaml   |  79 ++++++
 drivers/cpufreq/Kconfig.arm                   |   9 +
 drivers/cpufreq/Makefile                      |   1 +
 drivers/cpufreq/krait-cache.c                 | 232 ++++++++++++++++++
 4 files changed, 321 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/krait-cache-scale.yaml
 create mode 100644 drivers/cpufreq/krait-cache.c

Comments

Christian Marangi Aug. 31, 2020, 7:41 a.m. UTC | #1
> -----Messaggio originale-----
> Da: sibis=codeaurora.org@mg.codeaurora.org
> <sibis=codeaurora.org@mg.codeaurora.org> Per conto di Sibi Sankar
> Inviato: lunedì 31 agosto 2020 07:46
> A: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Ansuel Smith <ansuelsmth@gmail.com>; vincent.guittot@linaro.org;
> saravanak@google.com; Sudeep Holla <sudeep.holla@arm.com>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Rob Herring <robh+dt@kernel.org>; linux-
> pm@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Oggetto: Re: [RFC PATCH v3 0/2] Add Krait Cache Scaling support
> 
> On 2020-08-24 16:10, Viresh Kumar wrote:
> > +Vincent/Saravana/Sibi
> >
> > On 21-08-20, 16:00, Ansuel Smith wrote:
> >> This adds Krait Cache scaling support using the cpufreq notifier.
> >> I have some doubt about where this should be actually placed (clk or
> >> cpufreq)?
> >> Also the original idea was to create a dedicated cpufreq driver (like
> >> it's done in
> >> the codeaurora qcom repo) by copying the cpufreq-dt driver and adding
> >> the cache
> >> scaling logic but i still don't know what is better. Have a very
> >> similar driver or
> >> add a dedicated driver only for the cache using the cpufreq notifier
> >> and do the
> >> scale on every freq transition.
> >> Thanks to everyone who will review or answer these questions.
> >
> > Saravana was doing something with devfreq to solve such issues if I
> > wasn't mistaken.
> >
> > Sibi ?
> 
> IIRC the final plan was to create a devfreq device
> and devfreq-cpufreq based governor to scale them, this
> way one can switch to a different governor if required.

So in this case I should convert this patch to a devfreq driver- 
Isn't overkill to use a governor for such a task?
(3 range based on the cpufreq?)

> (I don't see if ^^ applies well for l2 though). In the
> interim until such a solution is acked on the list we
> just scale the resources directly from the cpufreq

In this case for this SoC we can't really scale the L2 freq
with the cpu since we observed a bug and we need to switch
back to the idle freq sometimes. Also this SoC use the generic
cpufreq-dt driver and doesn't have a dedicated driver. So we
must use a notifier.

> driver. On SDM845/SC7180 SoCs, L3 is modeled as a
> interconnect provider and is directly scaled from the
> cpufreq-hw driver.
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project.
Sibi Sankar Sept. 3, 2020, 7:13 a.m. UTC | #2
On 2020-09-03 12:23, Viresh Kumar wrote:
> On 31-08-20, 09:41, ansuelsmth@gmail.com wrote:
>> On 31-08-20, Sibi wrote:
>> > On 2020-08-24 16:10, Viresh Kumar wrote:
>> > > +Vincent/Saravana/Sibi
>> > >
>> > > On 21-08-20, 16:00, Ansuel Smith wrote:
>> > >> This adds Krait Cache scaling support using the cpufreq notifier.
>> > >> I have some doubt about where this should be actually placed (clk or
>> > >> cpufreq)?
>> > >> Also the original idea was to create a dedicated cpufreq driver (like
>> > >> it's done in
>> > >> the codeaurora qcom repo) by copying the cpufreq-dt driver and adding
>> > >> the cache
>> > >> scaling logic but i still don't know what is better. Have a very
>> > >> similar driver or
>> > >> add a dedicated driver only for the cache using the cpufreq notifier
>> > >> and do the
>> > >> scale on every freq transition.
>> > >> Thanks to everyone who will review or answer these questions.
>> > >
>> > > Saravana was doing something with devfreq to solve such issues if I
>> > > wasn't mistaken.
>> > >
>> > > Sibi ?
>> >
>> > IIRC the final plan was to create a devfreq device
>> > and devfreq-cpufreq based governor to scale them, this
>> > way one can switch to a different governor if required.
>> 
>> So in this case I should convert this patch to a devfreq driver-
> 
> I think this should happen nevertheless. You are doing DVFS for a
> device which isn't a CPU and devfreq looks to be the right place of
> doing so.
> 
>> Isn't overkill to use a governor for such a task?
>> (3 range based on the cpufreq?)
> 
> I am not sure about the governor part here, maybe it won't be required
> ?

Yeah I don't see it being needed in ^^
case as well. I just mentioned them as
an advantage in case you wanted to switch
to a different governor in the future.

https://lore.kernel.org/lkml/d0bc8877-6d41-f54e-1c4c-2fadbb9dcd0b@samsung.com/

A devfreq governor tracking cpufreq was
generally accepted but using a cpufreq
notifier to achieve that was discouraged.
Christian Marangi Sept. 3, 2020, 11 a.m. UTC | #3
> -----Messaggio originale-----
> Da: sibis=codeaurora.org@mg.codeaurora.org
> <sibis=codeaurora.org@mg.codeaurora.org> Per conto di Sibi Sankar
> Inviato: giovedì 3 settembre 2020 09:13
> A: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: ansuelsmth@gmail.com; vincent.guittot@linaro.org;
> saravanak@google.com; 'Sudeep Holla' <sudeep.holla@arm.com>; 'Rafael J.
> Wysocki' <rjw@rjwysocki.net>; 'Rob Herring' <robh+dt@kernel.org>; linux-
> pm@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Oggetto: Re: R: [RFC PATCH v3 0/2] Add Krait Cache Scaling support
> 
> On 2020-09-03 12:23, Viresh Kumar wrote:
> > On 31-08-20, 09:41, ansuelsmth@gmail.com wrote:
> >> On 31-08-20, Sibi wrote:
> >> > On 2020-08-24 16:10, Viresh Kumar wrote:
> >> > > +Vincent/Saravana/Sibi
> >> > >
> >> > > On 21-08-20, 16:00, Ansuel Smith wrote:
> >> > >> This adds Krait Cache scaling support using the cpufreq notifier.
> >> > >> I have some doubt about where this should be actually placed (clk
> or
> >> > >> cpufreq)?
> >> > >> Also the original idea was to create a dedicated cpufreq driver
(like
> >> > >> it's done in
> >> > >> the codeaurora qcom repo) by copying the cpufreq-dt driver and
> adding
> >> > >> the cache
> >> > >> scaling logic but i still don't know what is better. Have a very
> >> > >> similar driver or
> >> > >> add a dedicated driver only for the cache using the cpufreq
notifier
> >> > >> and do the
> >> > >> scale on every freq transition.
> >> > >> Thanks to everyone who will review or answer these questions.
> >> > >
> >> > > Saravana was doing something with devfreq to solve such issues if I
> >> > > wasn't mistaken.
> >> > >
> >> > > Sibi ?
> >> >
> >> > IIRC the final plan was to create a devfreq device
> >> > and devfreq-cpufreq based governor to scale them, this
> >> > way one can switch to a different governor if required.
> >>
> >> So in this case I should convert this patch to a devfreq driver-
> >
> > I think this should happen nevertheless. You are doing DVFS for a
> > device which isn't a CPU and devfreq looks to be the right place of
> > doing so.
> >
> >> Isn't overkill to use a governor for such a task?
> >> (3 range based on the cpufreq?)
> >
> > I am not sure about the governor part here, maybe it won't be required
> > ?
> 
> Yeah I don't see it being needed in ^^
> case as well. I just mentioned them as
> an advantage in case you wanted to switch
> to a different governor in the future.
> 
> https://lore.kernel.org/lkml/d0bc8877-6d41-f54e-1c4c-
> 2fadbb9dcd0b@samsung.com/
> 
> A devfreq governor tracking cpufreq was
> generally accepted but using a cpufreq
> notifier to achieve that was discouraged.
> 

I read the patch discussion and it looks like at the very end they
lost interest in pushing it. That would very fit what I need here so
I'm asking how should I proceed? Keep the cpufreq notifier?
Introduce a dedicated governor? Ask them to resume the pushing or
try to include the changes to the passive governor by myself? 

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project.