From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932121AbbLNI2v (ORCPT ); Mon, 14 Dec 2015 03:28:51 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:53264 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752542AbbLNI2t (ORCPT ); Mon, 14 Dec 2015 03:28:49 -0500 X-AuditID: cbfee68f-f793a6d000001364-bc-566e7dbf4816 Date: Mon, 14 Dec 2015 08:28:47 +0000 (GMT) From: MyungJoo Ham Subject: Re: [PATCH v4 01/20] PM / devfreq: exynos: Add generic exynos bus frequency driver To: =?utf-8?Q?=EC=B5=9C=EC=B0=AC=EC=9A=B0?= , =?utf-8?Q?=ED=81=AC=EC=89=AC=EC=8B=9C=ED=86=A0=ED=94=84?= , "kgene@kernel.org" Cc: =?utf-8?Q?=EB=B0=95=EA=B2=BD=EB=AF=BC?= , "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" Reply-to: myungjoo.ham@samsung.com MIME-version: 1.0 X-MTR: 20151214082836191@myungjoo.ham Msgkey: 20151214082836191@myungjoo.ham X-EPLocale: ko_KR.utf-8 X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-MLAttribute: X-RootMTR: 20151214082836191@myungjoo.ham X-ParentMTR: X-ArchiveUser: X-CPGSPASS: N X-ConfirmMail: N,general Content-type: text/plain; charset=utf-8 MIME-version: 1.0 Message-id: <989487615.658131450081722610.JavaMail.weblogic@epmlwas07b> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrCIsWRmVeSWpSXmKPExsWyRsSkWHd/bV6YwfvlohaXd81hc2D0+LxJ LoAxissmJTUnsyy1SN8ugSvj75VFzAU9FhXvTz9lbWC8YNbFyMkhJKAusWjJSTYQW0LAROL6 jZNMELaYxIV764HiXEA1Sxklts9vZu9i5AArurAxAiI+h1Fi37vJrCANLAKqEjNnb2UGqWET 0JOY+TkZJCwsEC2xbeUTRpB6EYHjjBLHrmwAc5gFvrJKLHzwnw3iCiWJNftesYDYvAKCEidn PmGBuEJV4tyXfmaIuJrEzudHWSHi4hIX5l5ih7B5JWa0P4Wql5OY9nUNM4QtLXF+FsgyiG8W f38MFeeXOHZ7B9SXAhJTzxyEqtGSOLLyPzQk+CTWLHzLAlO/69RyZphd97fMheqVkNja8gTs HmYBRYkp3Q/BAcQsoCmxfpc+uld4BdwlHtzdwATyu4TAXA6JyztXsk1gVJqFpG4WklGzEEYh K1nAyLKKUTS1ILmgOCm9yFivODG3uDQvXS85P3cTIzAxnP73rH8H490D1ocYBTgYlXh4M5bl hgmxJpYVV+YeYjQFRtNEZinR5Hxg+skriTc0NjOyMDUxNTYytzRTEuddKPUzWEggPbEkNTs1 tSC1KL6oNCe1+BAjEwenVAOj9ZWd8u66k22a5NykNxoYizS496/pyr80ocnKaNe9L332bgrb Tav+/1PoPSSl2a+q8Nb0yyq2V5v3/Qo02OouXvn43+bDjvuO6gmuDHFO5e8QnS0hl9yWNe2w uMY/zVt5mzgt5G4o6/eUhVkemXk0U6vyQkvPSsf4GOOMT2K7jae3PV6Z4q3EUpyRaKjFXFSc CADqygM+BwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrKKsWRmVeSWpSXmKPExsVy+t/tPt39tXlhBpOWCFhc3jWHzYHR4/Mm uQDGqDSbjNTElNQihdS85PyUzLx0WyXv4HjneFMzA0NdQ0sLcyWFvMTcVFslF58AXbfMHKCh SgpliTmlQKGAxOJiJX07m6L80pJUhYz84hJbpWhDcyM9IwM9UyM9Q+NYK0MDAyNToJqEtIy/ VxYxF/RYVLw//ZS1gfGCWRcjJ4eQgLrEoiUn2boYOTgkBEwkLmyMAAlLCIhJXLi3HijMBVQy h1Fi37vJrCAJFgFViZmztzKD1LMJ6EnM/JwMEhYWiJbYtvIJI0i9iMBxRoljVzaAOcwCX1kl Fj74zwaxTElizb5XLCA2r4CgxMmZT1ggtqlKnPvSzwwRV5PY+fwoK0RcXOLC3EvsEDavxIz2 p1D1chLTvq5hhrClJc7PAlkGcfXi74+h4vwSx27vYIKwBSSmnjkIVaMlcWQlxD0SAnwSaxa+ ZYGp33VqOTPMrvtb5kL1SkhsbXkCdg+zgKLElO6H7CDPMwtoSqzfpY/uFV4Bd4kHdzcwTWCU nYUkNQtJ9yyEbmQlCxhZVjGKphYkFxQnpVcY6xUn5haX5qXrJefnbmIEJ6Fni3cw/j9vfYhR gINRiYc3Y1lumBBrYllxZe4hRgkOZiUR3gSrvDAh3pTEyqrUovz4otKc1OJDjKbASJvILCWa nA9MkHkl8YbGxiZmJqaWJhYGpuZK4ry39/mFCQmkJ5akZqemFqQWwfQxcXBKNTBmJ3hoPynW XaijvM7F5Pjz26cUWL2ui/H/KQ79276xQjbn3uolbK9sL1u2sWpt1/t9fosF85dJT2rdVs91 5Gux3i8/d+66jKiV+cteiDUcT9p/zPDbFq/ExS/bXv2In7Nirecs0WqVY5vPfLvrqzT9UKKj jNtyM6kJ0QYem05e3yFceb919dcuJZbijERDLeai4kQA61R0VVgDAAA= DLP-Filter: Pass X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tBE8SuaF023761 > > 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. [] > --- /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++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I