Message-id: <5399c191-e140-e2b8-629b-72ddfbf99b0f@samsung.com>
Date:   Thu, 12 Jul 2018 17:44:33 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Brian Norris <briannorris@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Lee Jones <lee.jones@...aro.org>,
        Benson Leung <bleung@...omium.org>,
        Olof Johansson <olof@...om.net>
Subject: Re: [PATCH v5 05/12] PM / devfreq: Add support for policy notifiers

Hi Matthias,

On 2018년 07월 07일 02:53, Matthias Kaehlcke wrote:
> Hi Chanwoo,
> 
> On Wed, Jul 04, 2018 at 03:41:46PM +0900, Chanwoo Choi wrote:
> 
>> Firstly,
>> I'm not sure why devfreq needs the devfreq_verify_within_limits() function.
>>
>> devfreq already used the OPP interface as default. It means that
>> the outside of 'drivers/devfreq' can disable/enable the frequency
>> such as drivers/thermal/devfreq_cooling.c. Also, when some device
>> drivers disable/enable the specific frequency, the devfreq core
>> consider them.
>>
>> So, devfreq doesn't need to devfreq_verify_within_limits() because
>> already support some interface to change the minimum/maximum frequency
>> of devfreq device. 
>>
>> In case of cpufreq subsystem, cpufreq only provides 'cpufreq_verify_with_limits()'
>> to change the minimum/maximum frequency of cpu. some device driver cannot
>> change the minimum/maximum frequency through OPP interface.
>>
>> But, in case of devfreq subsystem, as I explained already, devfreq support
>> the OPP interface as default way. devfreq subsystem doesn't need to add
>> other way to change the minimum/maximum frequency.
> 
> Using the OPP interface exclusively works as long as a
> enabling/disabling of OPPs is limited to a single driver
> (drivers/thermal/devfreq_cooling.c). When multiple drivers are
> involved you need a way to resolve conflicts, that's the purpose of
> devfreq_verify_within_limits(). Please let me know if there are
> existing mechanisms for conflict resolution that I overlooked.
> 
> Possibly drivers/thermal/devfreq_cooling.c could be migrated to use
> devfreq_verify_within_limits() instead of the OPP interface if
> desired, however this seems beyond the scope of this series.

Actually, if we uses this approach, it doesn't support the multiple drivers too.
If non throttler drivers uses devfreq_verify_within_limits(), the conflict
happen.

To resolve the conflict for multiple device driver, maybe OPP interface
have to support 'usage_count' such as clk_enable/disable().

> 
>> Secondly,
>> This patch send the 'struct devfreq_policy' instance as the data
>> when sending the notification as following:
>>
>> 	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
>> 			DEVFREQ_ADJUST, policy);
>>
>> But, I think that if devfreq core sends the 'struct devfreq_freq_limits'
>> instance instead of 'struct devfreq_policy', it is enough.
>> Because receiver of DEVFREQ_ADJUST just will use the min_freq/max_freq variables.
>>
>> So, I tried to find the cpufreq's case. The some device drivers using
>> CPUFREQ_POLICY_NOTIFIER uses following variables of 'struct cpufreq_policy'.
>> It means that receiver of CPUFREQ_POLICY_NOTIFIER don't need to other
>> information/variables except for min/max frequency.
>>
>> - policy->min
>> - policy->max
>> - policy->cpuinfo.max_freq
>> - policy->cpuinfo.min_freq
>> - policy->cpu : not related to devfreq)
>> - policy->related_cpus : not related to devfreq)
>>
>> - list of device drivers using CPUFREQ_POLICY_NOTIFIER (linux kernel is v4.18-rc1)
>> $ grep -rn "CPUFREQ_POLICY_NOTIFIER" .
>> ./drivers/macintosh/windfarm_cpufreq_clamp.c
>> ./drivers/thermal/cpu_cooling.c
>> ./drivers/thermal/cpu_cooling.c
>> ./drivers/acpi/processor_thermal.c
>> ./drivers/acpi/processor_thermal.c
>> ./drivers/acpi/processor_perflib.c
>> ./drivers/acpi/processor_perflib.c
>> ./drivers/base/arch_topology.c
>> ./drivers/base/arch_topology.c
>> ./drivers/video/fbdev/sa1100fb.c
>> ./drivers/video/fbdev/pxafb.c
>> ./drivers/cpufreq/ppc_cbe_cpufreq_pmi.c
>> ./drivers/cpufreq/cpufreq.c
>> ./drivers/cpufreq/cpufreq.c
>> ./drivers/cpufreq/cpufreq.c
>> ./drivers/cpufreq/cpufreq.c
> 
> Thanks for your investigation.
> 
> I decided to mirror the cpufreq interface for consistency, but I agree
> that 'struct devfreq_freq_limits' could be passed instead of the
> policy object. I'm fine with changing that.
> 
>> On 2018년 07월 04일 08:46, Matthias Kaehlcke wrote:
>>> Policy notifiers are called before a frequency change and may narrow
>>> the min/max frequency range in devfreq_policy, which is used to adjust
>>> the target frequency if it is beyond this range.
>>>
>>> Also add a few helpers:
>>>  - devfreq_verify_within_[dev_]limits()
>>>     - should be used by the notifiers for policy adjustments.
>>>  - dev_to_devfreq()
>>>     - lookup a devfreq strict from a device pointer
>>>
>>> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
>>> Reviewed-by: Brian Norris <briannorris@...omium.org>
>>> ---
>>> Changes in v5:
>>> - none
>>>
>>> Changes in v4:
>>> - Fixed typo in commit message: devfreg => devfreq
>>> - added 'Reviewed-by: Brian Norris <briannorris@...omium.org>' tag
>>>
>>> Changes in v3:
>>> - devfreq.h: fixed misspelling of struct devfreq_policy
>>>
>>> Changes in v2:
>>> - performance, powersave and simpleondemand governors don't need changes
>>>   with "PM / devfreq: Don't adjust to user limits in governors"
>>> - formatting fixes
>>> ---
>>>  drivers/devfreq/devfreq.c | 48 ++++++++++++++++++++++-------
>>>  include/linux/devfreq.h   | 65 +++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 102 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 21604d6ae2b8..4cbaa7ad1972 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -72,6 +72,21 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>>>  	return ERR_PTR(-ENODEV);
>>>  }
>>>  
>>> +/**
>>> + * dev_to_devfreq() - find devfreq struct using device pointer
>>> + * @dev:	device pointer used to lookup device devfreq.
>>> + */
>>> +struct devfreq *dev_to_devfreq(struct device *dev)
>>> +{
>>> +	struct devfreq *devfreq;
>>> +
>>> +	mutex_lock(&devfreq_list_lock);
>>> +	devfreq = find_device_devfreq(dev);
>>> +	mutex_unlock(&devfreq_list_lock);
>>> +
>>> +	return devfreq;
>>> +}
>>> +
>>>  static unsigned long find_available_min_freq(struct devfreq *devfreq)
>>>  {
>>>  	struct dev_pm_opp *opp;
>>> @@ -269,20 +284,21 @@ int update_devfreq(struct devfreq *devfreq)
>>>  	if (!policy->governor)
>>>  		return -EINVAL;
>>>  
>>> +	policy->min = policy->devinfo.min_freq;
>>> +	policy->max = policy->devinfo.max_freq;
>>
>> Why don't you consider 'policy->user.max/min_freq' as following?
>> As I already commented, I think that 'struct devfreq_freq_limits' is enough
>> instead of 'struct devfreq_policy'.
>>
>> 	->max_freq = MIN(policy->devinfo.max_freq, policy->user.max_freq);
>> 	->min_freq = MAX(policy->devinfo.min_freq, policy->user.min_freq);
> 
> You mean limiting the frequency range with user.min/max before
> DEVFREQ_ADJUST instead of adjusting it afterwards? That's fine with
> me.
> 
> Thanks
> 
> Matthias
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists