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:	Fri, 24 Apr 2015 14:55:21 +0200
From:	Sascha Hauer <s.hauer@...gutronix.de>
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>,
	"Joe.C" <yingjoe.chen@...iatek.com>,
	Eddie Huang <eddie.huang@...iatek.com>,
	Howard Chen <ibanezchen@...il.com>,
	Chen Fan <fan.chen@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver

On Fri, Apr 24, 2015 at 02:46:25PM +0800, Pi-Cheng Chen wrote:
> Hi Sascha,
> 
> Thanks for reviewing.
> 
> On Thu, Apr 23, 2015 at 8:01 PM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
> > On Mon, Apr 20, 2015 at 05:27:26PM +0800, pi-cheng.chen wrote:
> >> This patch implements MT8173 specific cpufreq driver with OPP table defined
> >> in the driver code.
> >>
> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@...aro.org>
> >> ---
> >>  drivers/cpufreq/Kconfig.arm      |   6 +
> >>  drivers/cpufreq/Makefile         |   1 +
> >>  drivers/cpufreq/mt8173-cpufreq.c | 509 +++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 516 insertions(+)
> >>  create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
> >>
> >> +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;
> >
> > This introspecting the regulators for possible voltages looks hacky and
> > unnecessary. regulator_set_voltage() can be passed minimum and maximum
> > values, why don't you use it to increase the voltage within sensible
> > limit, like
> >
> >         regulator_set_voltage(sram_reg, old_vsram + 100000, old_vsram + 200000);
> >
> > or similar?
> 
> I am sorry I don't understand how could I do it. Would you elaborate?

You try to set the OPPs to the exact voltages, then next use functions
to determine the next exact higher voltage and set the regulator voltage
to an exact value. Instead of doing this you can let the ability to
specify a voltage range work for you, something like:

	int tolerance = 50000;

	while (vproc < new_vproc) {
		int next = min(new_vproc - vproc, 200000);
		int next_sram = next + 100000;

		regulator_set_voltage(sram_reg, next_sram - tolerance, next_sram + tolerance);
		regulator_set_voltage(vproc_reg, next - tolerance, next + tolerance);
		vproc = regulator_get_voltage(vproc_reg);
	}

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/