From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933140AbbLODdM (ORCPT ); Mon, 14 Dec 2015 22:33:12 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:33064 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933027AbbLODdI (ORCPT ); Mon, 14 Dec 2015 22:33:08 -0500 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfee690-f79646d000001316-5b-566f89f0ea8d Content-transfer-encoding: 8BIT Message-id: <566F89E7.30607@samsung.com> Date: Tue, 15 Dec 2015 12:32:55 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: myungjoo.ham@samsung.com, =?UTF-8?B?7YGs7Ims7Iuc7Yag7ZSE?= , "kgene@kernel.org" Cc: =?UTF-8?B?67CV6rK966+8?= , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "linux@arm.linux.org.uk" , "tjakobi@math.uni-bielefeld.de" , "linux.amoon@gmail.com" , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH v4 01/20] PM / devfreq: exynos: Add generic exynos bus frequency driver References: <989487615.658131450081722610.JavaMail.weblogic@epmlwas07b> In-reply-to: <989487615.658131450081722610.JavaMail.weblogic@epmlwas07b> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrFIsWRmVeSWpSXmKPExsWyRsSkUPdDZ36YwfZ7Fhbzj5xjteh/s5DV 4tyrlYwWr18YWvQ/fs1scbbpDbvF5V1z2Cw+9x5htJhxfh+TxbqNt9gtbl/mtVh6/SKTxe3G FWwWE6avZbFo3XuE3aJt9QdWBwGPNfPWMHq0NPeweVzu62Xy2DnrLrvHyuVf2Dw2repk8/h3 jN2jb8sqRo/Pm+QCOKO4bFJSczLLUov07RK4Mp707WYpaDOrODptBVMD4wPtLkYODgkBE4lz 65m7GDmBTDGJC/fWs3UxcnEICaxglHjx+jAjTM3EHQkgNUICsxgluteUgdi8AoISPybfYwEp YRaQlzhyKRvCVJeYMiUXYsoDRok729YyQZRrSLRO/QNmswioSsyc1sEKYrMJaEnsf3GDDaRX VCBCovtEJUiviEAzo8Tniz0sIDXMAk9YJVZ/ALtYWCBWYvriEIhr3CX2HfwFNpJTwENi0udF LCC9EgIrOSQ+f5rGCrFLQOLb5EMsEJ/ISmw6APWtpMTBFTdYJjCKzULyzCyEZ2YhPLOAkXkV o2hqQXJBcVJ6kYlecWJucWleul5yfu4mRmCUn/73bMIOxnsHrA8xCnAwKvHwLmDODxNiTSwr rsw9xGgKdMNEZinR5HxgKskriTc0NjOyMDUxNTYytzRTEud9LfUzWEggPbEkNTs1tSC1KL6o NCe1+BAjEwenVANjlB3nzVkXM1tOhPJyvZFg3nrZ+HfgEl8G2Z6IOA8fxyc+W7znbA4Wbw44 fONInYh93DvJAG7mTx26Vl/c5i19cTTarNTvoVF1fTBrevjheE6HRy2CP/7NKfZq89vH8Ugs UojB7MTJg7LbXeNS62xv9n+c/2vxOvPiLUzN2497r1k1w2tpfKcSS3FGoqEWc1FxIgBB2f8J 7QIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBKsWRmVeSWpSXmKPExsVy+t9jQd0PnflhBq8OGVrMP3KO1aL/zUJW i3OvVjJavH5haNH/+DWzxdmmN+wWl3fNYbP43HuE0WLG+X1MFus23mK3uH2Z12Lp9YtMFrcb V7BZTJi+lsWide8Rdou21R9YHQQ81sxbw+jR0tzD5nG5r5fJY+esu+weK5d/YfPYtKqTzePf MXaPvi2rGD0+b5IL4IxqYLTJSE1MSS1SSM1Lzk/JzEu3VfIOjneONzUzMNQ1tLQwV1LIS8xN tVVy8QnQdcvMAfpCSaEsMacUKBSQWFyspG+HaUJoiJuuBUxjhK5vSBBcj5EBGkhYw5jxpG83 S0GbWcXRaSuYGhgfaHcxcnBICJhITNyR0MXICWSKSVy4t54NxBYSmMUo0b2mDMTmFRCU+DH5 HgtIObOAvMSRS9kQprrElCm5XYxcQNUPGCXubFvLBFGuIdE69Q+YzSKgKjFzWgcriM0moCWx /8UNNpBeUYEIie4TlSC9IgLNjBKfL/awgNQwCzxhlVj9AewyYYFYiemLQyCucZfYd/AX2EhO AQ+JSZ8XsUxgBDoR4bhZCMfNQjhuASPzKkaJ1ILkguKk9FzDvNRyveLE3OLSvHS95PzcTYzg VPJMagfjwV3uhxgFOBiVeHgXMOeHCbEmlhVX5h5ilOBgVhLhLbYACvGmJFZWpRblxxeV5qQW H2I0BfpuIrOUaHI+MM3llcQbGpuYGVkamRtaGBmbK4nz1l6KDBMSSE8sSc1OTS1ILYLpY+Lg lGpgrLQru+tyl2f2Fp6920WXzFlwMuU7z23u4IIvrf+yUvl3t3KX98jqHWI3YrN5qHhmqvge 2w+p7BeU6gQNf8/sWxhxwTJgigvfW+mU8NY45yns62dI31Q0z+8+EVMwY1eIgsui9lNNvOZv PM5MOjlNMTPZjlNfXG1T6oqS3K4Ulq6N2rFVVcJKLMUZiYZazEXFiQBdLDOpOwMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015년 12월 14일 17:28, MyungJoo Ham wrote: >> >> This patch adds the generic exynos bus frequency driver for AMBA AXI bus >> of sub-blocks in exynos SoC with DEVFREQ framework. The Samsung Exynos SoC >> have the common architecture for bus between DRAM and sub-blocks in SoC. >> This driver can support the generic bus frequency driver for Exynos SoCs. >> >> In devicetree, Each bus block has a bus clock, regulator, operation-point >> and devfreq-event devices which measure the utilization of each bus block. >> >> Signed-off-by: Chanwoo Choi >> [linux.amoon: Tested on Odroid U3] >> Tested-by: Anand Moon >> > > Chanwoo, could you please show me testing this set of patches in your site? > Please let me know when is ok to visit you. > (I do not have exynos machines right now.) > >> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile >> index 5134f9ee983d..375ebbb4fcfb 100644 >> --- a/drivers/devfreq/Makefile >> +++ b/drivers/devfreq/Makefile >> @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o >> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >> >> # DEVFREQ Drivers >> +obj-$(CONFIG_ARCH_EXYNOS) += exynos/ >> obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ) += exynos/ >> obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ) += exynos/ > > CONFIG_ARCH_EXYNOS is true if > CONFIG_ARM_EXYNOS4_BUS_DEVFREQ is true > or > CONFIG_ARM_EXYNOS5_BUS_DEVFREQ is true > Thus, the two lines after you've added have become useless. (dead code) > > Please delete them. In this series, patch11 deletes all of both exynos4_bus.c and exynos5_bus.c. > > [] >> --- /dev/null >> +++ b/drivers/devfreq/exynos/exynos-bus.c > [] >> +static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags) >> +{ >> + struct exynos_bus *bus = dev_get_drvdata(dev); >> + struct dev_pm_opp *new_opp; >> + unsigned long old_freq, new_freq, old_volt, new_volt; >> + int ret = 0; >> + >> + /* Get new opp-bus instance according to new bus clock */ >> + rcu_read_lock(); >> + new_opp = devfreq_recommended_opp(dev, freq, flags); >> + if (IS_ERR_OR_NULL(new_opp)) { >> + dev_err(dev, "failed to get recommed opp instance\n"); >> + rcu_read_unlock(); >> + return PTR_ERR(new_opp); >> + } >> + >> + new_freq = dev_pm_opp_get_freq(new_opp); >> + new_volt = dev_pm_opp_get_voltage(new_opp); >> + old_freq = dev_pm_opp_get_freq(bus->curr_opp); >> + old_volt = dev_pm_opp_get_voltage(bus->curr_opp); >> + rcu_read_unlock(); >> + >> + if (old_freq == new_freq) >> + return 0; >> + >> + /* Change voltage and frequency according to new OPP level */ >> + mutex_lock(&bus->lock); >> + >> + if (old_freq < new_freq) { >> + ret = regulator_set_voltage(bus->regulator, new_volt, new_volt); > > Setting the maximum volt same as the minimum volt is not recommended. > Especially for any DVFS mechanisms, I recommend to set values as: > min_volt = minimum voltage that does not harm the stability > max_volt = maximum voltage that does not break the circuit > > Please refer to /include/linux/regulator/driver.h > "@set_voltage" comments. > > For the rest of regulator_set_voltage usages, I'd say the same. OK. I'll add the 'voltage-tolerance' property as cpufreq-dt.c driver. The cpufreq-dt.c get the percentage value by using 'voltage-tolerance' devicetree property. For example, if (of_property_read_u32(np, "exynos,voltage-tolerance", &bus->voltage_tolerance)) bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE; tol = new_volt * bus->voltage_tolerance / 100; regulator_set_voltage_tol(regulator, new_volt, tol); > > [] >> +static int exynos_bus_get_dev_status(struct device *dev, >> + struct devfreq_dev_status *stat) >> +{ >> + struct exynos_bus *bus = dev_get_drvdata(dev); >> + struct devfreq_event_data edata; >> + int ret; >> + >> + rcu_read_lock(); >> + stat->current_frequency = dev_pm_opp_get_freq(bus->curr_opp); >> + rcu_read_unlock(); >> + >> + ret = exynos_bus_get_event(bus, &edata); >> + if (ret < 0) { >> + stat->total_time = stat->busy_time = 0; >> + goto err; >> + } >> + >> + stat->busy_time = (edata.load_count * 100) / bus->ratio; >> + stat->total_time = edata.total_count; >> + >> + dev_dbg(dev, "Usage of devfreq-event : %ld/%ld\n", stat->busy_time, >> + stat->total_time); > > These two values are unsigned long. OK. I'll modify it (%ld -> %lu) > > [] >> +static int exynos_bus_parse_of(struct device_node *np, >> + struct exynos_bus *bus) >> +{ >> + struct device *dev = bus->dev; >> + unsigned long rate; >> + int i, ret, count, size; >> + >> + /* Get the clock to provide each bus with source clock */ >> + bus->clk = devm_clk_get(dev, "bus"); >> + if (IS_ERR(bus->clk)) { >> + dev_err(dev, "failed to get bus clock\n"); >> + return PTR_ERR(bus->clk); >> + } >> + >> + ret = clk_prepare_enable(bus->clk); >> + if (ret < 0) { >> + dev_err(dev, "failed to get enable clock\n"); >> + return ret; >> + } > > [] > >> +err_regulator: >> + regulator_disable(bus->regulator); >> +err_opp: >> + dev_pm_opp_of_remove_table(dev); >> + >> + return ret; > > No clk_disable_unprepare() somewhere in the error handling routines? OK. I'll handle the error of clock control. > > [] > >> +#ifdef CONFIG_PM_SLEEP >> +static int exynos_bus_resume(struct device *dev) >> +{ > [] >> + ret = regulator_enable(bus->regulator); > [] >> +} >> + >> +static int exynos_bus_suspend(struct device *dev) >> +{ > [] >> + regulator_disable(bus->regulator); > [] >> +} >> +#endif > > Isn't there any possibility that you should not disable at suspend callbacks? > If I remember correctly, we should not disable VDD-INT/VDD-MIF of Exynos4412 > for suspend-to-RAM although it is "mostly" ok to do so, but not "always" ok. Yes. It is not always same. I'll pass the role of handling the VDD_INT/VDD_MIF regulator to regulator framework. The regulator framework handles the state in suspend state by using 'regulator-off-in-suspend' property as following: For example, in arch/arm/boot/dts/exynos4412-trats.dts: buck1_reg: BUCK1 { regulator-name = "vdd_mif"; regulator-min-microvolt = <850000>; regulator-max-microvolt = <1100000>; regulator-always-on; regulator-boot-on; regulator-state-mem { regulator-off-in-suspend; }; }; buck3_reg: BUCK3 { regulator-name = "vdd_int"; regulator-min-microvolt = <850000>; regulator-max-microvolt = <1150000>; regulator-always-on; regulator-boot-on; regulator-state-mem { regulator-off-in-suspend; }; }; > > In such cases, I guess it should be "selectively" disabled for suspend. > (some regulators support special "low power if suspended" modes for such cases) Thanks, Chanwoo Choi