linux-kernel - Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 22 Apr 2015 09:33:52 -0500
From:	Josh Cartwright <joshc@...com>
To:	Pi-Cheng Chen <pi-cheng.chen@...aro.org>
Cc:	Viresh Kumar <viresh.kumar@...aro.org>,
	Mike Turquette <mturquette@...aro.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Chen Fan <fan.chen@...iatek.com>,
	Howard Chen <ibanezchen@...il.com>,
	linux-mediatek@...ts.infradead.org,
	"Joe.C" <yingjoe.chen@...iatek.com>,
	Eddie Huang <eddie.huang@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver

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

Content of type "application/pgp-signature" skipped