LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Josh Cartwright <joshc@ni.com>
To: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	Mike Turquette <mturquette@linaro.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Linaro Kernel Mailman List <linaro-kernel@lists.linaro.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Chen Fan <fan.chen@mediatek.com>,
	Howard Chen <ibanezchen@gmail.com>,
	linux-mediatek@lists.infradead.org,
	"Joe.C" <yingjoe.chen@mediatek.com>,
	Eddie Huang <eddie.huang@mediatek.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver
Date: Wed, 22 Apr 2015 09:33:52 -0500	[thread overview]
Message-ID: <20150422143352.GJ27115@jcartwri.amer.corp.natinst.com> (raw)
In-Reply-To: <CALx668W6nJkYR325BhxOXMF1z=i7mkYbh3pyBFDNoZC9NZ0dVw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

On Wed, Apr 22, 2015 at 11:11:34AM +0800, Pi-Cheng Chen wrote:
[..]
> >> +config ARM_MT8173_CPUFREQ
> >> +     bool "Mediatek MT8173 CPUFreq support"
> >> +     depends on ARCH_MEDIATEK && REGULATOR
> >
> > I think you want to 'select REGULATOR' here; because REGULATOR isn't
> > a user-visible option.
> 
> I am not sure but I need it to be "depends on" as other SoC cpufreq
> drivers. Please check
> ARM_S3C2416_CPUFREQ_VCORESCALE in drivers/cpufreq/Kconfig.arm
> By the way, I would like to know more details about the visibility of
> these configurable
> options, would you kindly point me out some documents about it?

Paul pointed out that I was wrong, so I'll defer to him.  My knowledge
has likely been outdated.

[..]
> >> +/* OPP table for LITTLE cores of MT8173 */
> >> +struct mtk_cpu_opp mt8173_l_opp[] = {
> >
> > static const?
> 
> Yes. I miss "static" here. But I need those two array to be non-const
> so that I could
> fix up the exact voltage values by querying the supported voltages of
> regulators.
> Please check the mt8173_cpufreq_cpu_opp_fixup() function below.

Indeed.  Thanks.

[..]
> >> +static int mtk_cpufreq_voltage_trace(struct cpu_dvfs_info *info,
> >> +                                  struct mtk_cpu_opp *opp)
> >> +{
> >> +     struct regulator *proc_reg = info->proc_reg;
> >> +     struct regulator *sram_reg = info->sram_reg;
> >> +     int old_vproc, new_vproc, old_vsram, new_vsram, vsram, vproc, ret;
> >> +
> >> +     old_vproc = regulator_get_voltage(proc_reg);
> >> +     old_vsram = regulator_get_voltage(sram_reg);
> >> +
> >> +     new_vproc = opp->vproc;
> >> +     new_vsram = opp->vsram;
> >> +
> >> +     /*
> >> +      * In the case the voltage is going to be scaled up, Vsram and Vproc
> >> +      * need to be scaled up step by step. In each step, Vsram needs to be
> >> +      * set to (Vproc + 200mV) first, then Vproc is set to (Vsram - 100mV).
> >> +      * Repeat the step until Vsram and Vproc are set to target voltage.
> >> +      */
> >> +     if (old_vproc < new_vproc) {
> >> +next_up_step:
> >> +             old_vsram = regulator_get_voltage(sram_reg);
> >> +
> >> +             vsram = (new_vsram - old_vproc < MAX_VOLT_SHIFT) ?
> >> +                     new_vsram : old_vproc + MAX_VOLT_SHIFT;
> >> +             vsram = get_regulator_voltage_floor(sram_reg, vsram);
> >> +
> >> +             ret = regulator_set_voltage(sram_reg, vsram, vsram);
> >> +             if (ret)
> >> +                     return ret;
> >> +
> >> +             vproc = (new_vsram == vsram) ?
> >> +                     new_vproc : vsram - MIN_VOLT_SHIFT;
> >> +             vproc = get_regulator_voltage_ceil(proc_reg, vproc);
> >> +
> >> +             ret = regulator_set_voltage(proc_reg, vproc, vproc);
> >> +             if (ret) {
> >> +                     regulator_set_voltage(sram_reg, old_vsram, old_vsram);
> >> +                     return ret;
> >> +             }
> >> +
> >> +             if (new_vproc == vproc && new_vsram == vsram)
> >> +                     return 0;
> >> +
> >> +             old_vproc = vproc;
> >> +             goto next_up_step;
> >
> > Perhaps a naive question: but, is this the correct place to do this?  I
> > would expect this stepping behavior to be implemented in the driver
> > controlling the regulator you are consuming.  It seems strange to do it
> > here.
> 
> This was already discussed in the last round of this series of patches.
> Please check the discussion[1]. Any suggestion would be welcomed.
> Thanks.

Interesting, thanks.  Sorry for rehashing already-covered territory!

  Josh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

  reply	other threads:[~2015-04-22 14:34 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-20  9:27 [PATCH v3 0/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
2015-04-20  9:27 ` [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver pi-cheng.chen
2015-04-20 14:17   ` Josh Cartwright
2015-04-20 18:31     ` Paul Bolle
2015-04-22  3:11     ` Pi-Cheng Chen
2015-04-22 14:33       ` Josh Cartwright [this message]
2015-04-20 18:28   ` Paul Bolle
2015-04-22  3:14     ` Pi-Cheng Chen
2015-04-23 12:01   ` Sascha Hauer
2015-04-24  6:46     ` Pi-Cheng Chen
2015-04-24 12:55       ` Sascha Hauer
2015-05-07  9:42         ` Pi-Cheng Chen
2015-04-23 12:56   ` Mark Rutland
2015-04-24  6:50     ` Pi-Cheng Chen
2015-04-29  6:06       ` Viresh Kumar
2015-04-30  7:42   ` Sascha Hauer
2015-05-07  9:40     ` Pi-Cheng Chen
2015-04-20  9:27 ` [PATCH 2/2] ARM64: mt8173: dts Add MT8173 cpufreq driver support pi-cheng.chen
2015-04-23 12:53   ` Mark Rutland
2015-04-24  7:09     ` Pi-Cheng Chen
2015-05-18 13:29       ` Pi-Cheng Chen

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=20150422143352.GJ27115@jcartwri.amer.corp.natinst.com \
    --to=joshc@ni.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eddie.huang@mediatek.com \
    --cc=fan.chen@mediatek.com \
    --cc=ibanezchen@gmail.com \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mturquette@linaro.org \
    --cc=pi-cheng.chen@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=yingjoe.chen@mediatek.com \
    --subject='Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver' \
    /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

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