Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751553AbbEGJmt (ORCPT ); Thu, 7 May 2015 05:42:49 -0400 Received: from mail-la0-f42.google.com ([209.85.215.42]:33772 "EHLO mail-la0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751135AbbEGJmq (ORCPT ); Thu, 7 May 2015 05:42:46 -0400 MIME-Version: 1.0 In-Reply-To: <20150424125521.GU6325@pengutronix.de> References: <1429522047-16675-1-git-send-email-pi-cheng.chen@linaro.org> <1429522047-16675-2-git-send-email-pi-cheng.chen@linaro.org> <20150423120152.GM6325@pengutronix.de> <20150424125521.GU6325@pengutronix.de> Date: Thu, 7 May 2015 17:42:44 +0800 Message-ID: Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver From: Pi-Cheng Chen To: Sascha Hauer Cc: Viresh Kumar , Mike Turquette , Matthias Brugger , "Joe.C" , Eddie Huang , Howard Chen , Chen Fan , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , "linux-pm@vger.kernel.org" , Linaro Kernel Mailman List , linux-mediatek@lists.infradead.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4075 Lines: 96 On Fri, Apr 24, 2015 at 8:55 PM, Sascha Hauer wrote: > 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 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 >> >> --- >> >> 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); > } Thanks for your explanation. I'll try it to get rid of those functions to find out the exact voltages. Best Regards, Pi-Cheng > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/