linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Niklas Cassel <nks@flawful.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	agross@kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps
Date: Tue, 26 May 2020 17:54:20 +0200	[thread overview]
Message-ID: <20200526155419.GA9977@flawful.org> (raw)
In-Reply-To: <20200526085948.GA1329@gerhold.net>

On Tue, May 26, 2020 at 10:59:48AM +0200, Stephan Gerhold wrote:
> > Considering that CPR is not an actual power domain, CPR gives
> > adjustments to VDD_APC, but I don't know of any other device
> > connected to VDD_APC, other than the CPU, so in hindsight the CPR
> > driver probably should have been implemented using .target_index(),
> > rather than as a power domain provider using performance states.
> 
> I suppose having CPR, MEMACC etc as power domain providers is a bit
> overkill, given there is just one consumer. However, at least the
> "performance state" part fits quite well in my opinion. At the end
> all these requirements represent some performance state that must be
> set when the CPU frequency is changed.
> 

For MX, it makes sense to model it as a power domain provider, and for
it to have its own OPP table, since this actually is a power domain.

For CPR, I think that the target_index() model of just giving an index
in a frequency table is much better, the OPP library can still be used
to get the frequencies/frequency_table.
Since at least for Qualcom CPU's, the corner (opp-level) is defined as
an increasing number 1,2,3,4, without skips.

Even if it wasn't always without skips, we could just put opp-level in
the CPU opp table, and get it from there.

The only thing that the corner is used for really, is to use it as an
index the local drv->corner array, which is where the (current) VDD_APC
voltage is stored for each index/corner.

For CPR, the .target_index() in cpufreq-dt.c gets called, which is
supplied with an index, but the index gets converted to a frequency.
This frequency is then sent to the OPP library, and is then converted
back to an index of the same value (just increased by one), before
cpr_set_performance_state() is called (which then has to subtract one).
In this case, all the extra overhead of going via genpd is totally
unnecessary.

This is totally correct when setting a performance state on a power
domain like MX, since for an actual power domain you might have
multiple consumers, so you need to go via genpd.

Considering that CPR is not a power domain, I wish the driver wasn't
designed around performance states, which, _for the CPR case_,
is misleading, unnecessary, and adds extra overhead for no reason.

I realize the irony of me criticizing my own code.
I simply know better now, and wish I had designed it differently :)


Kind regards,
Niklas

  reply	other threads:[~2020-05-26 15:54 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-01 17:50 [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps Loic Poulain
2020-04-01 23:46 ` Bjorn Andersson
2020-04-02  8:13 ` Stephan Gerhold
2020-04-02  9:58   ` Loic Poulain
2020-04-03  1:31   ` Bjorn Andersson
2020-04-03 10:09     ` Stephan Gerhold
2020-04-03 18:00       ` Stephan Gerhold
2020-04-23  4:55         ` Bjorn Andersson
2020-04-26 12:31           ` Stephan Gerhold
2020-05-06 21:18             ` Stephan Gerhold
2020-05-07  5:34               ` Bjorn Andersson
2020-05-08 12:08                 ` Ulf Hansson
2020-05-08 13:42                   ` Stephan Gerhold
2020-05-11  5:29                   ` Viresh Kumar
2020-05-07 10:46               ` Stephan Gerhold
2020-05-21 19:18                 ` Stephan Gerhold
2020-05-23 12:08                   ` Stephan Gerhold
2020-05-27 20:47                     ` Georgi Djakov
2020-05-25 15:32           ` Niklas Cassel
2020-05-25 16:36             ` Stephan Gerhold
2020-05-25 19:44               ` Niklas Cassel
2020-05-26  8:59                 ` Stephan Gerhold
2020-05-26 15:54                   ` Niklas Cassel [this message]
2020-05-26 20:54                     ` Stephan Gerhold
2020-05-27 10:39                       ` Niklas Cassel
2020-05-27 12:04                         ` Stephan Gerhold
2020-05-27 12:59                           ` Niklas Cassel
2020-05-27 20:56                             ` Stephan Gerhold
2020-05-27 23:10                               ` Niklas Cassel
2020-05-28 13:32                                 ` Stephan Gerhold
2020-05-28  4:44                           ` Viresh Kumar
2020-04-28 20:04 ` Amit Kucheria

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200526155419.GA9977@flawful.org \
    --to=nks@flawful.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=stephan@gerhold.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).