Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965415AbbDVOea (ORCPT ); Wed, 22 Apr 2015 10:34:30 -0400 Received: from skprod2.natinst.com ([130.164.80.23]:38887 "EHLO ni.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932238AbbDVOeY (ORCPT ); Wed, 22 Apr 2015 10:34:24 -0400 Date: Wed, 22 Apr 2015 09:33:52 -0500 From: Josh Cartwright To: Pi-Cheng Chen Cc: Viresh Kumar , Mike Turquette , Matthias Brugger , "devicetree@vger.kernel.org" , Linaro Kernel Mailman List , "linux-pm@vger.kernel.org" , Linux Kernel Mailing List , Chen Fan , Howard Chen , linux-mediatek@lists.infradead.org, "Joe.C" , Eddie Huang , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver Message-ID: <20150422143352.GJ27115@jcartwri.amer.corp.natinst.com> References: <1429522047-16675-1-git-send-email-pi-cheng.chen@linaro.org> <1429522047-16675-2-git-send-email-pi-cheng.chen@linaro.org> <20150420141719.GF27115@jcartwri.amer.corp.natinst.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-MIMETrack: Itemize by SMTP Server on US-AUS-MGWOut2/AUS/H/NIC(Release 8.5.3FP6|November 21, 2013) at 04/22/2015 09:33:52 AM, Serialize by Router on US-AUS-MGWOut2/AUS/H/NIC(Release 8.5.3FP6|November 21, 2013) at 04/22/2015 09:33:53 AM, Serialize complete at 04/22/2015 09:33:53 AM Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="a+b56+3nqLzpiR9O" Content-Disposition: inline X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2015-04-22_04:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4589 Lines: 127 --a+b56+3nqLzpiR9O Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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[] =3D { > > > > static const? >=20 > 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 =3D info->proc_reg; > >> + struct regulator *sram_reg =3D info->sram_reg; > >> + int old_vproc, new_vproc, old_vsram, new_vsram, vsram, vproc, re= t; > >> + > >> + old_vproc =3D regulator_get_voltage(proc_reg); > >> + old_vsram =3D regulator_get_voltage(sram_reg); > >> + > >> + new_vproc =3D opp->vproc; > >> + new_vsram =3D opp->vsram; > >> + > >> + /* > >> + * In the case the voltage is going to be scaled up, Vsram and V= proc > >> + * 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 - 1= 00mV). > >> + * Repeat the step until Vsram and Vproc are set to target volta= ge. > >> + */ > >> + if (old_vproc < new_vproc) { > >> +next_up_step: > >> + old_vsram =3D regulator_get_voltage(sram_reg); > >> + > >> + vsram =3D (new_vsram - old_vproc < MAX_VOLT_SHIFT) ? > >> + new_vsram : old_vproc + MAX_VOLT_SHIFT; > >> + vsram =3D get_regulator_voltage_floor(sram_reg, vsram); > >> + > >> + ret =3D regulator_set_voltage(sram_reg, vsram, vsram); > >> + if (ret) > >> + return ret; > >> + > >> + vproc =3D (new_vsram =3D=3D vsram) ? > >> + new_vproc : vsram - MIN_VOLT_SHIFT; > >> + vproc =3D get_regulator_voltage_ceil(proc_reg, vproc); > >> + > >> + ret =3D regulator_set_voltage(proc_reg, vproc, vproc); > >> + if (ret) { > >> + regulator_set_voltage(sram_reg, old_vsram, old_v= sram); > >> + return ret; > >> + } > >> + > >> + if (new_vproc =3D=3D vproc && new_vsram =3D=3D vsram) > >> + return 0; > >> + > >> + old_vproc =3D 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. >=20 > 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 --a+b56+3nqLzpiR9O Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJVN7FJAAoJEKp7ZBKwQFArnV4IAKNBOH7QabPrJpHVqNQcAgFP yUalVICfm1deZ8eodT7T2J6+AVO9lmBrPYusKYeEoc54SsC0DNOSGcNSClJIRag3 Afu9LHCurNe8Vqz8SGpq+/uD3vewwmP2plXXxK59wdT8jDLd1h0MgfAkkadOJjmz OZ3v2nTSE30zSq2lgh7SqLjjfoj+tM6nMmiUnhK+XyiZgS3/hnj4GnoGE+d5qH5L xcQx8vrNAFFQLGPBX516T/aId4V3dbtDFBWwJvII0qA9Co8cRqao9MhylOSB2JZu /ELxDt2dkwdLQZraAyejz5F2EJ0+vzrNT18hWTwqETjG4aDomRIhb2nydarRmjI= =O6Ib -----END PGP SIGNATURE----- --a+b56+3nqLzpiR9O-- -- 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/