From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756982AbcHDAdh (ORCPT ); Wed, 3 Aug 2016 20:33:37 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:51333 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752199AbcHDAdf convert rfc822-to-8bit (ORCPT ); Wed, 3 Aug 2016 20:33:35 -0400 X-AuditID: cbfee68e-f79cb6d000006cfe-1c-57a28d5d5dc2 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <57A28D5C.40507@samsung.com> Date: Thu, 04 Aug 2016 09:33:32 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: hl , heiko@sntech.de Cc: tixy@linaro.org, typ@rock-chips.com, airlied@linux.ie, mturquette@baylibre.com, dbasehore@chromium.org, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, dianders@chromium.org, linux-rockchip@lists.infradead.org, kyungmin.park@samsung.com, myungjoo.ham@samsung.com, linux-arm-kernel@lists.infradead.org, mark.yao@rock-chips.com Subject: Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc References: <1469779021-10426-1-git-send-email-hl@rock-chips.com> <1469779021-10426-7-git-send-email-hl@rock-chips.com> <579F2445.1020200@samsung.com> <579FF164.2000907@rock-chips.com> <57A01FD2.2060806@samsung.com> <57A19F60.7090700@rock-chips.com> In-reply-to: <57A19F60.7090700@rock-chips.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupmleLIzCtJLcpLzFFi42JZI2JSqBvbuyjc4MJTDovecyeZLF5t3sNm cXbZQTaLK1/fs1n8f/Sa1eLHhlPMFmeb3rBbbHp8jdXi8q45bBafHvxnttgx5QCTxcVTrha3 G1ewWfw4081isXD+fXaL2avrHAQ83t9oZfeY3XCRxeNyXy+Tx51re9g8tn97wOpxv/s4k8fm JfUef2ftZ/Ho27KK0WP7tXnMHp83yQVwR3HZpKTmZJalFunbJXBlvH19hrHgiWfFydbDTA2M Cyy7GDk4JARMJBquJ3YxcgKZYhIX7q1nA7GFBFYwSqy5HAkRN5E4OfUgYxcjF1B8KaNEd+c9 RpAEr4CgxI/J91hA5jALqEtMmZILYYpILLvPCVLBLKAtsWzha2aI1geMEidffWeBaNWQWPbi FpjNIqAqMeveK7C9bAJaEvtf3ACz+QUUJa7+eMwIMlNUIEKi+0QlSFgEaOapp+eZQGYyC/xg kpjXtwvsHGGBcInPR78wQSy7zCRxc10XWIJTQE/i8cY9bCAJCYETHBKzz75ihNgsIPFt8iEW SEDISmw6wAzxsKTEwRU3WCYwSsxC8uYshDdnIbw5C8mbCxhZVjGKphYkFxQnpRcZ6RUn5haX 5qXrJefnbmIEppHT/5717WC8ecD6EKMAB6MSDy9H+qJwIdbEsuLK3EOMpkD3TGSWEk3OByar vJJ4Q2MzIwtTE1NjI3NLMyVx3gSpn8FCAumJJanZqakFqUXxRaU5qcWHGJk4OKUaGKN/f0xc MVmqWvrd5AtPuTzbbhX/vlHbue9K5j3Dz7MOC6pFnDmz4/pG8Zp33/bbyBYzC/bNn+4ReMXK O/qjaWtD7dMXz40D5E0LU8TfTjIznXBg1xGHVwu1r890Ttt6o+8Tn0JD8KMr/K3nDp22kV37 oOdbzvEtLMrPYxpmCM0W5UwyeHZ0w2IlluKMREMt5qLiRABSHrZpHgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrAKsWRmVeSWpSXmKPExsVy+t9jAd3Y3kXhBlNKLHrPnWSyeLV5D5vF 2WUH2SyufH3PZvH/0WtWix8bTjFbnG16w26x6fE1VovLu+awWXx68J/ZYseUA0wWF0+5Wtxu XMFm8eNMN4vFwvn32S1mr65zEPB4f6OV3WN2w0UWj8t9vUwed67tYfPY/u0Bq8f97uNMHpuX 1Hv8nbWfxaNvyypGj+3X5jF7fN4kF8Ad1cBok5GamJJapJCal5yfkpmXbqvkHRzvHG9qZmCo a2hpYa6kkJeYm2qr5OIToOuWmQP0jpJCWWJOKVAoILG4WEnfDtOE0BA3XQuYxghd35AguB4j AzSQsIYxY8LBmYwF9zwqtl4FOmiWRRcjJ4eEgInEyakHGSFsMYkL99azdTFycQgJLGWU6O68 B5bgFRCU+DH5HksXIwcHs4C8xJFL2RCmusSUKbkQ5Q8YJU6++s4CUa4hsezFLTCbRUBVYta9 V2wgNpuAlsT+FzfAbH4BRYmrPx4zgswRFYiQ6D5RCRIWEdCWOPX0PBPITGaBH0wS8/p2gZ0g LBAu8fnoFyaIZZeZJG6u6wJLcAroSTzeuIdtAqPgLCSnzkI4dRbCqQsYmVcxSqQWJBcUJ6Xn GuallusVJ+YWl+al6yXn525iBKeeZ1I7GA/ucj/EKMDBqMTDe8F0UbgQa2JZcWXuIUYJDmYl Ed4pXUAh3pTEyqrUovz4otKc1OJDjKZAv05klhJNzgemxbySeENjEzMjSyNzQwsjY3Mlcd7H /9eFCQmkJ5akZqemFqQWwfQxcXBKNTBOUEp813hE0oNRdvEVnjgX7eRtlz3sls9e3Ph5z88r b2TzQmYIJyZqC53PkDu4N/qzLZ/u0+Dr7U6aIU/LTd/FHa/+6rSv0iRSPrUi38k8xWn9+/wb hRzGbYzvtTbqm/04UrVsl+T/NTaPTsS/uOGVu871RJnusXA5F/9ruQ3rOO7O3bBh0iklluKM REMt5qLiRAC47nNxUwMAAA== 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 Hi Lin, On 2016년 08월 03일 16:38, hl wrote: > > Hi Chanwoo Choi, > On 2016年08月02日 12:21, Chanwoo Choi wrote: >> Hi Lin, >> >> On the next version, I'd like you to add the 'linux-pm@vger.kernel.org' >> because devfreq is a subsystem of power management. > Sure, will do it next version. >> On 2016년 08월 02일 10:03, hl wrote: >>> Hi Chanwoo Choi, >>> >>> Thanks for reviewing so carefully. And i have some question: >>> >>> On 2016年08月01日 18:28, Chanwoo Choi wrote: >>>> Hi Lin, >>>> >>>> As I mentioned on patch5, you better to make the documentation as following: >>>> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt >>>> And, I add the comments. >>>> >>>> >>>> On 2016년 07월 29일 16:57, Lin Huang wrote: >>>>> base on dfi result, we do ddr frequency scaling, register >>>>> dmc driver to devfreq framework, and use simple-ondemand >>>>> policy. >>>>> >>>>> Signed-off-by: Lin Huang >>>>> --- >>>>> Changes in v4: >>>>> - use arm_smccc_smc() function talk to bl31 >>>>> - delete rockchip_dmc.c file and config >>>>> - delete dmc_notify >>>>> - adjust probe order >>>>> Changes in v3: >>>>> - operate dram setting through sip call >>>>> - imporve set rate flow >>>>> >>>>> Changes in v2: >>>>> - None >>>>> Changes in v1: >>>>> - move dfi controller to event >>>>> - fix set voltage sequence when set rate fail >>>>> - change Kconfig type from tristate to bool >>>>> - move unuse EXPORT_SYMBOL_GPL() >>>>> >>>>> drivers/devfreq/Kconfig | 1 + >>>>> drivers/devfreq/Makefile | 1 + >>>>> drivers/devfreq/rockchip/Kconfig | 8 + >>>>> drivers/devfreq/rockchip/Makefile | 1 + >>>>> drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++ >>>>> 5 files changed, 484 insertions(+) >>>>> create mode 100644 drivers/devfreq/rockchip/Kconfig >>>>> create mode 100644 drivers/devfreq/rockchip/Makefile >>>>> create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c >>>>> >> [snip] >> >>>>> + >>>>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, >>>>> + u32 flags) >>>>> +{ >>>>> + struct platform_device *pdev = container_of(dev, struct platform_device, >>>>> + dev); >>>>> + struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev); >>>> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'. >>>> >>>> struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev); >>>> >>>>> + struct dev_pm_opp *opp; >>>>> + unsigned long old_clk_rate = dmcfreq->rate; >>>>> + unsigned long target_volt, target_rate; >>>>> + int err; >>>>> + >>>>> + rcu_read_lock(); >>>>> + opp = devfreq_recommended_opp(dev, freq, flags); >>>>> + if (IS_ERR(opp)) { >>>>> + rcu_read_unlock(); >>>>> + return PTR_ERR(opp); >>>>> + } >>>>> + >>>>> + target_rate = dev_pm_opp_get_freq(opp); >>>>> + target_volt = dev_pm_opp_get_voltage(opp); >>>>> + opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags); >>>>> + if (IS_ERR(opp)) { >>>>> + rcu_read_unlock(); >>>>> + return PTR_ERR(opp); >>>>> + } >>>>> + dmcfreq->volt = dev_pm_opp_get_voltage(opp); >>>> If you add the 'curr_opp' variable to struct rk3399_dmcfreq, >>>> you can remove the calling of devfreq_recommended_opp(). >>>> dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp); >>>> dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp); >>>> >>>> Because the current rate and voltage is already decided on previous polling cycle, >>>> So we don't need to get the opp with devfreq_recommended_opp(). >>> I prefer the way now use, since we get the dmcfreq->rate use clk_get_rate() after, >>> Base on that, i do not care the set_rate success or fail. use curr_opp i need to >>> care about set_rate status, when fail, i must set some rate, when success i must >>> set other rate. >> I think that it is not good to get the alrady decided opp >> by devfreq_recommended_opp(). Usually, devfreq_recommended_opp() is used >> to get the proper opp which get the close frequency (dmcfreq->rate). >> >> Also, When finishing the rk3399_dmcfreq_target(), the rk3399_dmc.c >> have to know the current opp or rate without any finding sequence. >> The additional finding procedure is un-needed. >> >>>>> + rcu_read_unlock(); >>>>> + >>>>> + if (dmcfreq->rate == target_rate) >>>>> + return 0; >>>>> + >>>>> + mutex_lock(&dmcfreq->lock); >>>>> + >>>>> + /* >>>>> + * if frequency scaling from low to high, adjust voltage first; >>>>> + * if frequency scaling from high to low, adjuset frequency first; >>>>> + */ >>>> s/adjuset/adjust >>>> >>>> I recommend that you use a captital letter for first character and use the '.' >>>> instead of ';'. >>>> >>>>> + if (old_clk_rate < target_rate) { >>>>> + err = regulator_set_voltage(dmcfreq->vdd_center, target_volt, >>>>> + target_volt); >>>>> + if (err) { >>>>> + dev_err(dev, "Unable to set vol %lu\n", target_volt); >>>> To readability, you better to use the corrent word to pass the precise the log message. >>>> - s/vol/voltage >>>> >>>> And, this patch uses the 'Unable to' or 'Cannot' to show the error log. >>>> I recommend that you use the consistent expression if there is not any specific reason. >>>> >>>> dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt); >>>> >>>>> + goto out; >>>>> + } >>>>> + } >>>>> + dmcfreq->wait_dcf_flag = 1; >>>>> + err = clk_set_rate(dmcfreq->dmc_clk, target_rate); >>>>> + if (err) { >>>>> + dev_err(dev, >>>>> + "Unable to set freq %lu. Current freq %lu. Error %d\n", >>>>> + target_rate, old_clk_rate, err); >>>> dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err); >>>> >>>>> + regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt, >>>>> + dmcfreq->volt); >>>>> + goto out; >>>>> + } >>>>> + >>>>> + /* >>>>> + * wait until bcf irq happen, it means freq scaling finish in bl31, >>>> ditto. >>>> >>>>> + * use 100ms as timeout time >>>> s/time/time. >>>> >>>>> + */ >>>>> + if (!wait_event_timeout(dmcfreq->wait_dcf_queue, >>>>> + !dmcfreq->wait_dcf_flag, HZ / 10)) >>>>> + dev_warn(dev, "Timeout waiting for dcf irq\n"); >>>> If the timeout happen, are there any problem? >>> When timeout happen , may be we miss interrupt, but it do not affect this >>> process, since we will check the rate whether success later. >> OK. But I'd like you to modify the warning message. >> >> One more thing, is the dcf interrupt related to the change of clock rate? >> When the clock rate is changed, the dcf interrupt happen? > Yes, when clock rate changed sucessful, it will trigger a irq in bl31. OK. >> >>>> After setting the frequency and voltage, store the current opp entry on .curr_opp. >>>> dmcfreq->curr_opp = opp; >>>> >>>>> + /* >>>>> + * check the dpll rate >>>>> + * there only two result we will get, >>>>> + * 1. ddr frequency scaling fail, we still get the old rate >>>>> + * 2, ddr frequency scaling sucessful, we get the rate we set >>>>> + */ >>>>> + dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk); >>>>> + >>>>> + /* if get the incorrect rate, set voltage to old value */ >>>>> + if (dmcfreq->rate != target_rate) { >>>>> + dev_err(dev, "get wrong ddr frequency, Request freq %lu,\ >>>>> + Current freq %lu\n", target_rate, dmcfreq->rate); >>>>> + regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt, >>>>> + dmcfreq->volt); >>>> [Without force, it is just my opion about this code:] >>>> I think that this checking code it is un-needed. >>>> If this case occur, the rk3399_dmc.c never set the specific frequency >>>> because the rk3399 clock don't support the specific frequency for rk3399 dmc. >>>> >>>> If you want to set the correct frequency, >>>> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver. >>>> >>>> Basically, if the the clock driver don't support the correct same frequency , >>>> CCF(Common Clock Framework) set the close frequency. It is not a bad thing. >>> May be i should remove the regulator_set_voltage() here, but still need to >>> check whether we get the right frequency, since if we do not get the right frequency, >> When calling clk_set_rate(), the final frequency only depend on the rk3399 clock driver. >> But, if you want to check the new rate, I think that you should move this code >> right after clk_set_rate() when there is any dependency of dcf interrupt. > it should be after the wait_event, since it indicate the clk_set_rate finish, OK. >> >>> we should send a warn message, to remind that maybe you pass a frequency which >>> do not support in bl31. >> Also, I'd like you to explain the 'bl31' and add the description on next version. >> >> [snip] >> >> Regards, >> Chanwoo Choi Regards, Chanwoo Choi