linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephan Gerhold <stephan@gerhold.net>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: 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: Thu, 21 May 2020 21:18:09 +0200	[thread overview]
Message-ID: <20200521191809.GA253181@gerhold.net> (raw)
In-Reply-To: <20200507104603.GA581328@gerhold.net>

On Thu, May 07, 2020 at 12:46:08PM +0200, Stephan Gerhold wrote:
> On Wed, May 06, 2020 at 11:18:07PM +0200, Stephan Gerhold wrote:
> > Hi,
> > 
> > On Sun, Apr 26, 2020 at 02:31:48PM +0200, Stephan Gerhold wrote:
> > > On Wed, Apr 22, 2020 at 09:55:06PM -0700, Bjorn Andersson wrote:
> > > > On Fri 03 Apr 11:00 PDT 2020, Stephan Gerhold wrote:
> > > > 
> > > > > On Fri, Apr 03, 2020 at 12:09:25PM +0200, Stephan Gerhold wrote:
> > > > > > On Thu, Apr 02, 2020 at 06:31:19PM -0700, Bjorn Andersson wrote:
> > > > > > > On Thu 02 Apr 01:13 PDT 2020, Stephan Gerhold wrote:
> > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Wed, Apr 01, 2020 at 07:50:59PM +0200, Loic Poulain wrote:
> > > > > > > > > The highest cpu frequency opps have been dropped because CPR is not
> > > > > > > > > supported. However, we can simply specify operating voltage so that
> > > > > > > > > they match the max corner voltages for each freq. With that, we can
> > > > > > > > > support up to 1.36Ghz. Ideally, msm8916 CPR should be implemented to
> > > > > > > > > fine tune operating voltages and optimize power consumption.
> > > > > > > > 
> > > > > > > > Thanks for the patch! I was wondering how to enable the higher CPU
> > > > > > > > frequencies for a while now...
> > > > > > > > 
> > > > > > > > I was actually quite excited to see CPR being mainlined for QCS404.
> > > > > > > > If we are trying to add such a workaround (rather than CPR) for MSM8916
> > > > > > > > now, does that mean it's unlikely to see CPR working for MSM8916
> > > > > > > > anytime soon?
> > > > > > > > 
> > > > > > > > AFAICT, there is a WIP branch from Niklas Cassel here:
> > > > > > > > https://git.linaro.org/people/nicolas.dechesne/niklas.cassel/kernel.git/log/?h=cpr-msm8916-mainline
> > > > > > > > but it hasn't been updated for a while. (Not sure if it was working
> > > > > > > > already...)
> > > > > > > > 
> > > > > > > > Can someone explain what is missing to make CPR work for MSM8916?
> > > > > > > > 
> > > > > > > 
> > > > > > > CPR needs to control 3 things; VDD_APC, VDD_MX and MEMACC. On QCS404 it
> > > > > > > seems we don't have to adjust VDD_MX, so the code for this is missing
> > > > > > > from the driver.
> > > > > > > 
> > > > > > > So, afaict, what's missing is that rpmpd.c needs to gain support for
> > > > > > > 8916, then the CPR driver needs to gain a cpr_acc_desc and compatible
> > > > > > > for 8916, it needs to reference the VDDMX power domain and before/after
> > > > > > > we're adjusting the corner of the CPR we need to adjust the MX according
> > > > > > > to the mapping specified in the downstream kernel (i.e.  1->4, 2->5 and
> > > > > > > 3->7).
> > > > > > > 
> > > > > > > 
> > > > > > > Unfortunately, the requirement that VDDMX (VDD_MEM I presume) must be
> > > > > > > higher than VDD_APC most likely needs to be taken into consideration for
> > > > > > > Loic's proposed static voltage scaling as well. Unless VDD_MEM is left
> > > > > > > in Turbo mode from the boot loader I think we need to take VDDMX to
> > > > > > > corner 7 for speeds 998MHz and above (i.e. where we pull VDD_APC to
> > > > > > > 1.35V).
> > > > > > > 
> > > > > > 
> > > > > > I see! I wonder how hard it would be to add MSM8916 to rpmpd,
> > > > > > looking at previous commits it's mainly setting up a few defines?
> > > > > > 
> > > > > > If I understand it correctly, the OPPs from rpmpd could then be
> > > > > > referenced as "required-opps" in the CPU OPP table so that VDD_MX is
> > > > > > scaled together with the CPU frequency, and doesn't need to stay at
> > > > > > turbo mode (like in v3 from Loic) the whole time.
> > > > > > 
> > > > > 
> > > > > I have been thinking about this some more and I think I came up with
> > > > > some changes that make sense (but not entirely sure).
> > > > > 
> > > > > Based on the available downstream sources I guessed the defines to add
> > > > > for MSM8916 to the rpmpd driver. Then I added the VDD_MX OPPs as
> > > > > "required-opps" to the CPU OPP table so it would vote for the appopriate
> > > > > corners (with the mapping you mentioned above).
> > > > > 
> > > > 
> > > > I was not aware it was possible to describe the dependency between the
> > > > CPU opp table and MX in this fashion. If that's the case then this looks
> > > > really good and it should be straight forward to add MSM8916 support to
> > > > the CPR driver as well.
> > > > 
> > > 
> > > Indeed!
> > > 
> > > > > I haven't tested it yet, maybe I can get some feedback first if the code
> > > > > seems reasonable or if I'm missing something obvious? :)
> > > > > 
> > > > 
> > > > Have you tested this yet?
> > > > 
> > > 
> > > I just did. It does not fully work, yet:
> > > 
> > > rpmpd_set_performance() is indeed called as necessary when switching the
> > > CPU frequency. When I set 200 MHz it sets corner 3, with 533 MHz corner 4
> > > and starting with 998 MHz corner 6. So far so good :)
> > > 
> > > However, there is never actually anything sent to the RPM. :(
> > > It bails out in rpmpd_set_performance() before calling rpmpd_aggregate_corner():
> > > 
> > > 	/* Always send updates for vfc and vfl */
> > > 	if (!pd->enabled && pd->key != KEY_FLOOR_CORNER &&
> > > 	    pd->key != KEY_FLOOR_LEVEL)
> > > 		goto out;
> > > 
> > > Seems like we just try to set performance states, but never actually
> > > enable (rpmpd_power_on()) the power domain (pd->enabled == false).
> > > 
> > > I'm not sure which of the components involved here should handle that.
> > > The OPP core when setting required OPPs, the genpd core etc.
> > >
> > > Any ideas?
> > > 
> > > Thanks,
> > > Stephan
> > > 
> > 
> > For now I have added something to qcom-cpufreq-nvmem.c to power on/enable
> > the power domain (not really sure if it belongs there...):
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index a1b8238872a2..ed352ead037e 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> >  #include <linux/soc/qcom/smem.h>
> >  
> > @@ -370,10 +371,13 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> >  		}
> >  
> >  		if (drv->data->genpd_names) {
> > +			struct device **pd_dev;
> > +			const char **name = drv->data->genpd_names;
> > +
> >  			drv->genpd_opp_tables[cpu] =
> >  				dev_pm_opp_attach_genpd(cpu_dev,
> >  							drv->data->genpd_names,
> > -							NULL);
> > +							&pd_dev);
> >  			if (IS_ERR(drv->genpd_opp_tables[cpu])) {
> >  				ret = PTR_ERR(drv->genpd_opp_tables[cpu]);
> >  				if (ret != -EPROBE_DEFER)
> > @@ -382,6 +386,12 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> >  						ret);
> >  				goto free_genpd_opp;
> >  			}
> > +
> > +			while (*name) {
> > +				pm_runtime_get_sync(*pd_dev);
> > +				name++;
> > +				pd_dev++;
> > +			}
> >  		}
> >  	}
> > 
> > I really wonder why this doesn't affect CPR (or does it?).
> > So far I was not able to find anything that would power on/enable
> > the "cpr" power domain either. But I don't have a qcs404 to verify.
> >  
> > As far as I can tell from the log, the corner votes are correctly sent
> > to the RPM now when the CPU frequency is changed.
> > 
> > However...
> > 
> > > > > Also: Is there a good way to validate these changes?
> > > > > I suppose I could check the genpd state but that wouldn't tell me if the
> > > > > corner was applied correctly. Maybe I can check the actual voltage
> > > > > through the SPMI interface, hm...
> > > > > 
> > > > 
> > > > Validating that S2 and VDD_MX changes appropriately in Linux would be a
> > > > pretty good test.
> > > > 
> > 
> > Unfortunately I was not able to see any change in the voltage of L3 yet.
> > On samsung-a5u, /sys/class/regulator/<l3 spmi regulator>/microvolts
> > permanently reports 1300000 uV, even after a few different corner votes.
> > 
> > I'm not sure if:
> > 
> >   - This is normal (maybe some other remoteproc has a higher vote?)
> >     - I tried to disable wcnss, venus and hexagon without difference
> > 
> >   - I'm just missing something in the code
> > 
> >   - This is some peculiarity of the RPM firmware on samsung-a5u.
> >     (Although that seems quite unlikely to me...)
> > 
> > Any ideas? :/
> > 
> 
> I just tried the same on the downstream kernel for samsung-a5u.
> There the L3 voltage doesn't change either, it stays on 1300000 uV
> permanently. So more likely would be either "this is normal" or
> "This is some peculiarity of the RPM firmware on samsung-a5u".
> 
> I don't have a DB410c available for testing, but I'm curious if it would
> work correctly there. So far I have been unable to find any obvious
> mistake in my changes.
> 
> I'm attaching the full diff (including Loic's changes) in case someone
> wants to test this on the DB410c (or some other MSM8916 device).
> Basically you need to:
> 
>   1. CONFIG_QCOM_RPMPD=y
>      CONFIG_ARM_QCOM_CPUFREQ_NVMEM=y
>   2. Run it and change CPU frequencies a bit
>      (either with an automatic CPU governor, or by setting speeds
>       manually with powersave/performance/userspace)
>   3. There should be "set performance state" messages in dmesg
>      for the VDD_MX corner votes
>   4. Identify the spmi l3 regulator using /sys/class/regulator/*/name
>   5. Check the reported voltage in /sys/class/regulator/.../microvolts
> 
> For me it stays on 1300000 uV, permanently. :(
> 

I recently bought another device that does not have signature checks
enabled. For now I have tested with the original firmware there:

With or without my changes it stays at 1287500 uV permanently.
At least this seems somewhat more realistic since this is the
"super turbo" voltage.

I tried disabling various things in case they affect this somehow:
WCNSS in particular sets 1287500 uV for pm8916_l3.
But even with WCNSS disabled, even with the regulator constraints
removed for pm8916_l3 and all changes reverted - it keeps reporting
1287500 uV through the SPMI interface, no matter which CPU frequency
is selected.

This is even more confusing because l3 is still at nominal voltage
(1150000 uV) in the lk bootloader. (I ported a part of the mainline
spmi_regulator driver to lk so I can dump the voltages from there...)
Somehow it gets changed when Linux is booted. Very confusing.

Unlike on my previous test device, testing on downstream was successful:

  - CPU frequency 200MHz - 800MHz results into 1150000 uV
    reported as voltage for l3 through the SPMI interface (corner 4/5)
  - Higher CPU frequencies result into 1287500 uV (corner 7)

Again I ported the spmi_regulator driver from mainline to downstream,
so the way the voltage is read should be the same.

Clearly something must be different/missing in my changes,
but I still have no idea what exactly.
Any suggestions would be highly appreciated :)

Thanks,
Stephan

PS: Does someone know if the RPM firmware on MSM8916 is usually
device-specific? After some funny re-partitioning I managed to get
PSCI working on this device by flashing the HYP/TZ firmware from DB410c.

I was considering also flashing the RPM firmware from DB410c,
but not entirely sure if that would be safe. (Any voltages hardcoded there?)

  reply	other threads:[~2020-05-21 19:18 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 [this message]
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
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=20200521191809.GA253181@gerhold.net \
    --to=stephan@gerhold.net \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    /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).