From mboxrd@z Thu Jan 1 00:00:00 1970 From: hl Subject: Re: [RFC PATCH 3/4] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc Date: Thu, 2 Jun 2016 09:54:32 +0800 Message-ID: <574F91D8.70606@rock-chips.com> References: <1464773739-18152-1-git-send-email-hl@rock-chips.com> <1464773739-18152-4-git-send-email-hl@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-clk-owner@vger.kernel.org To: myungjoo.ham@gmail.com Cc: =?UTF-8?Q?Heiko_St=c3=bcbner?= , airlied@linux.ie, mturquette@baylibre.com, dbasehore@chromium.org, Stephen Boyd , LKML , dri-devel@lists.freedesktop.org, dianders@chromium.org, linux-rockchip@lists.infradead.org, Kyungmin Park , linux-clk@vger.kernel.org, linux-arm-kernel , mark.yao@rock-chips.com List-Id: linux-rockchip.vger.kernel.org Hi Myungloo Ham, On 2016=E5=B9=B406=E6=9C=8801=E6=97=A5 18:47, MyungJoo Ham wrote: > On Wed, Jun 1, 2016 at 6:35 PM, Lin Huang wrote: >> there is dfi controller on rk3399 platform, it can monitor >> ddr load, register this controller to devfreq framework, and >> default to use simple_ondeamnd policy, and do ddr frequency >> scaling base on this result. >> >> Signed-off-by: Lin Huang >> --- >> drivers/devfreq/Kconfig | 2 +- >> drivers/devfreq/Makefile | 1 + >> drivers/devfreq/rockchip/Kconfig | 14 + >> drivers/devfreq/rockchip/Makefile | 2 + >> drivers/devfreq/rockchip/rk3399_dmc.c | 438 ++++++++++++++++++++= ++++++++++++ >> drivers/devfreq/rockchip/rockchip_dmc.c | 132 ++++++++++ >> include/soc/rockchip/rockchip_dmc.h | 44 ++++ >> 7 files changed, 632 insertions(+), 1 deletion(-) >> create mode 100644 drivers/devfreq/rockchip/Kconfig >> create mode 100644 drivers/devfreq/rockchip/Makefile >> create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c >> create mode 100644 drivers/devfreq/rockchip/rockchip_dmc.c >> create mode 100644 include/soc/rockchip/rockchip_dmc.h >> >> + >> + /* check the rate we get whether correct */ >> + dmcfreq->rate =3D clk_get_rate(dmcfreq->dmc_clk); >> + if (dmcfreq->rate !=3D target_rate) { >> + dev_err(dev, "get wrong ddr frequency, Request freq = %lu,\ >> + Current freq %lu\n", target_rate, dmcfreq->r= ate); >> + regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->= volt, >> + dmcfreq->volt); > Why do you need to check this and > more importantly, why do you assume that dmvfreq->volt > will be safe in this case? (what if the new dmcfreq->rate is > larger than the old dmcfreq->rate after clk_get_rate?) > Note that you didn't update dmcfreq->volt after clk_get_rate() Now we will set the ddr clock rate in bl31 use dcf controller(as i= =20 show in the patch cover letter), and there only two result we can get, set clock rate fail(ddr=20 clock still in old rate) or set clock rate sucessful(use the new ddr rate), if the set rate fail, we should=20 keep the voltage old value. Ah, you remind me, i should check clock rate before: if (old_clk_rate > target_rate) otherwise there have chance set high rate fail and use low voltage= =2E And about the dmcfreq->volt, i will update this value when into th= e=20 function, throuth opp =3D devfreq_recommended_opp(dev, &dmcfreq->rate, flags); if (IS_ERR(opp)) { rcu_read_unlock(); return PTR_ERR(opp); } dmcfreq->volt =3D dev_pm_opp_get_voltage(opp); > >> +EXPORT_SYMBOL_GPL(dmc_event); >> +EXPORT_SYMBOL_GPL(rockchip_dmc_enabled); >> +EXPORT_SYMBOL_GPL(rockchip_dmc_enable); >> +EXPORT_SYMBOL_GPL(rockchip_dmc_disable); >> +EXPORT_SYMBOL_GPL(dmc_register_notifier); >> +EXPORT_SYMBOL_GPL(dmc_unregister_notifier); >> +EXPORT_SYMBOL_GPL(rockchip_dmc_get); >> +EXPORT_SYMBOL_GPL(rockchip_dmc_put); > Do you really need to export all these device driver specific > functions? Looks like a design flaw here. > there is some situation we need to disable ddr frequency=20 scaling(connect two panel etc) on rk3399 platform, and it should be also needed on other rockchip= =20 platform, so i move these funticon as separate file. > > Cheers, > MyungJoo > > --=20 Lin Huang