From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chanwoo Choi Subject: Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table Date: Tue, 24 Apr 2018 16:26:18 +0900 Message-ID: <5ADEDC1A.2000101@samsung.com> References: <20180424002016.9205-1-bjorn.andersson@linaro.org> <20180424002016.9205-2-bjorn.andersson@linaro.org> <5ADE9AE6.9090601@samsung.com> <20180424052916.GD2052@tuxbook-pro> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Return-path: In-reply-to: <20180424052916.GD2052@tuxbook-pro> Sender: linux-kernel-owner@vger.kernel.org To: Bjorn Andersson Cc: MyungJoo Ham , Kyungmin Park , Vinayak Holikatti , "Martin K. Petersen" , linux-scsi@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Vivek Gautam List-Id: linux-scsi@vger.kernel.org Hi, On 2018년 04월 24일 14:29, Bjorn Andersson wrote: > On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote: > >> Hi, >> >> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote: >>> The code in devfreq_add_device() handles the case where a freq_table is >>> passed by the client, but then requests min and max frequences from >>> the, in this case absent, opp tables. >>> >>> Read the min and max frequencies from the frequency table, which has >>> been built from the opp table if one exists, instead of querying the >>> opp table. >>> >>> Signed-off-by: Bjorn Andersson >>> --- >>> >>> An alternative approach is to clarify in the devfreq code that it's not >>> possible to pass a freq_table and then in patch 3 create an opp table for the >>> device in runtime; although the error handling of this becomes non-trivial. >>> >>> Transitioning the UFSHCD to use opp tables directly is hindered by the fact >>> that the Qualcomm UFS hardware has two different clocks that needs to be >>> running at different rates, so we would need a way to describe the two rates in >>> the opp table. (And would force us to change the DT binding) >>> >>> drivers/devfreq/devfreq.c | 22 ++++------------------ >>> 1 file changed, 4 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index fe2af6aa88fc..086ced50a13d 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct device *dev) >>> >>> static unsigned long find_available_min_freq(struct devfreq *devfreq) >>> { >>> - struct dev_pm_opp *opp; >>> - unsigned long min_freq = 0; >>> - >>> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq); >>> - if (IS_ERR(opp)) >>> - min_freq = 0; >>> - else >>> - dev_pm_opp_put(opp); >>> + struct devfreq_dev_profile *profile = devfreq->profile; >>> >>> - return min_freq; >>> + return profile->freq_table[0]; >> >> It is wrong. The thermal framework support the devfreq-cooling device >> which uses the dev_pm_opp_enable/disable(). >> > > Okay, that makes sense. So rather than registering a custom freq_table I > should register the min and max frequency using dev_pm_opp_add(). Thanks. > >> In order to find the correct available min frequency, >> the devfreq have to use the OPP function instead of using the first entry >> of the freq_table array. >> > > Based on this there seems to be room for cleaning out the freq_table > from devfreq, to reduce the confusion. I will review this further. Actually, devfreq must need to have the freq_table[] array. But, freq_table[] array should be handled in the devfreq core. Now, the devfreq device drivers can touch the freq_table. I think it is not good. There is a reason why we have to maintain the freq_table[] as the internal variable. OPP doesn't provide the OPP API which get the all registered frequencies. If devfreq-cooling device disables the specific frequency by using dev_pm_oppdisable(), the user of OPP interface can not get the disabled frequency list. So, I maintain the freq_table even if using the OPP interface. And, devfreq-cooling device uses the freq_table directly because released MALi driver from ARM initializes the freq_table list directly. I have no any objection for refactoring. Just I'm sharing the issue and current status. > > Thanks, > Bjorn > > > -- Best Regards, Chanwoo Choi Samsung Electronics