From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932164AbbLNIpP (ORCPT ); Mon, 14 Dec 2015 03:45:15 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:49556 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752578AbbLNIpJ (ORCPT ); Mon, 14 Dec 2015 03:45:09 -0500 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfee68d-f79646d000001355-ab-566e81932497 Content-transfer-encoding: 8BIT Message-id: <566E8189.7040505@samsung.com> Date: Mon, 14 Dec 2015 17:44:57 +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+NgFrrDIsWRmVeSWpSXmKPExsWyRsSkQHdyY16Ywbzrehbzj5xjteh/s5DV 4tyrlYwWr18YWvQ/fs1scbbpDbvF5V1z2Cw+9x5htJhxfh+TxbqNt9gtbl/mtVh6/SKTxe3G FWwWE6avZbFo3XuE3aJt9QdWBwGPNfPWMHq0NPeweVzu62Xy2DnrLrvHyuVf2Dw2repk8/h3 jN2jb8sqRo/Pm+QCOKO4bFJSczLLUov07RK4Mk7ev8VesFSvYv2bn+wNjFdVuxg5OSQETCQW XnrBCmGLSVy4t56ti5GLQ0hgBaPEkQWzWWGKrh0+DJVYyihxa1ovG0iCV0BQ4sfkeyxdjBwc zALyEkcuZUOY6hJTpuRClD9glLgM9A5EuZbE+c4FYDaLgKrE9O6zLCA2G1B8/4sbbCC9ogIR Et0nKkF6RQSaGSU+X+wBq2EWeMIqsfqDNkiNsECsxPTFISBhIQF3iX0Hf4GN5BTwkJj0eREL xMkrOSR6F0ZCrBKQ+Db5ENiVEgKyEpsOMEOUSEocXHGDZQKj2Cwkv8xC+GUWwi8LGJlXMYqm FiQXFCelFxnqFSfmFpfmpesl5+duYgRG+ul/z3p3MN4+YH2IUYCDUYmHN2NZbpgQa2JZcWXu IUZToBsmMkuJJucD00leSbyhsZmRhamJqbGRuaWZkjivotTPYCGB9MSS1OzU1ILUovii0pzU 4kOMTBycUg2M7Pt35KZvXaDwmbexhW1zwe8suwMFfy+sbWad9L7rooKSU0J0eFjWXU2ztsKp 1s1Hb2yxTH7ccH3nlcUa2yQcZG8uCNSQrNpXH1qja2izeFntFeWv2kaXVpyuPfhK4NrNnqz3 Hmv8OzfbJnBYfT0gGflT2ny7Z5+N/64i1pXn4nu4z1/8y1unxFKckWioxVxUnAgAxHtdTu8C AAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrFKsWRmVeSWpSXmKPExsVy+t9jAd3JjXlhBrueq1nMP3KO1aL/zUJW i3OvVjJavH5haNH/+DWzxdmmN+wWl3fNYbP43HuE0WLG+X1MFus23mK3uH2Z12Lp9YtMFrcb V7BZTJi+lsWide8Rdou21R9YHQQ81sxbw+jR0tzD5nG5r5fJY+esu+weK5d/YfPYtKqTzePf MXaPvi2rGD0+b5IL4IxqYLTJSE1MSS1SSM1Lzk/JzEu3VfIOjneONzUzMNQ1tLQwV1LIS8xN tVVy8QnQdcvMAfpCSaEsMacUKBSQWFyspG+HaUJoiJuuBUxjhK5vSBBcj5EBGkhYw5hx8v4t 9oKlehXr3/xkb2C8qtrFyMkhIWAice3wYTYIW0ziwr31QDYXh5DAUkaJW9N6wRK8AoISPybf Y+li5OBgFpCXOHIpG8JUl5gyJRei/AGjxGVgaEGUa0mc71wAZrMIqEpM7z7LAmKzAcX3v7jB BtIrKhAh0X2iEqRXRKCZUeLzxR6wGmaBJ6wSqz9og9QIC8RKTF8cAhIWEnCX2HfwF9hITgEP iUmfF7FMYBSYheS4WQjHzUI4bgEj8ypGidSC5ILipPRcw7zUcr3ixNzi0rx0veT83E2M4HTy TGoH48Fd7ocYBTgYlXh4M5flhgmxJpYVV+YeYpTgYFYS4U2wygsT4k1JrKxKLcqPLyrNSS0+ xGgK9N1EZinR5HxgqssriTc0NjEzsjQyN7QwMjZXEuetvRQZJiSQnliSmp2aWpBaBNPHxMEp 1cBo8JdjxZYtje3tByby81u+cmBe8rj4Iof8nL1c3bqTxdosNXRE3ed9/rPw76GkrdmXtIv1 gub/v/3t4pxnxpl5Ok+OMvGcKe+JXVYp0xQkGtC+Me1NwBe+DxuSPfUk4/wMK/j1Q8MnaBd8 W8+3e/3SC/fNnZv/PN0dIv+ZbWHSWhG/OP4J+YZKLMUZiYZazEXFiQAGTQkqPQMAAA== 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.) Sure. I can show it tomorrow whenever you want. Before visiting to me, just let me know. I'll prepare the demonstartion. Regards, Chanwo oChoi > >> 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. > > [] >> --- /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. > > [] >> +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. > > [] >> +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? > > [] > >> +#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. > > In such cases, I guess it should be "selectively" disabled for suspend. > (some regulators support special "low power if suspended" modes for such cases) > > [] > N�����r��y���b�X��ǧv�^�)޺{.n�+����{�����x,�ȧ���ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z��w���?����&�)ߢfl=== >