Date:   Wed, 04 Jul 2018 15:41:46 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Matthias Kaehlcke <mka@...omium.org>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     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,

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.


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


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);


> +
> +	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
> +				DEVFREQ_ADJUST, policy);
> +
>  	/* Reevaluate the proper frequency */
>  	err = policy->governor->get_target_freq(devfreq, &freq);
>  	if (err)
>  		return err;
>  
> -	/*
> -	 * Adjust the frequency with user freq, QoS and available freq.
> -	 *
> -	 * List from the highest priority
> -	 * max_freq
> -	 * min_freq
> -	 */
> -	max_freq = MIN(policy->devinfo.max_freq, policy->user.max_freq);
> -	min_freq = MAX(policy->devinfo.min_freq, policy->user.min_freq);
> +	/* Adjust the frequency */
> +
> +	max_freq = MIN(policy->max, policy->user.max_freq);
> +	min_freq = MAX(policy->min, policy->user.min_freq);
>  
>  	if (freq < min_freq) {
>  		freq = min_freq;
> @@ -645,6 +661,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->last_stat_updated = jiffies;
>  
>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
> +	srcu_init_notifier_head(&devfreq->policy_notifier_list);
>  
>  	mutex_unlock(&devfreq->lock);
>  
> @@ -1445,7 +1462,7 @@ EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
>   * devfreq_register_notifier() - Register a driver with devfreq
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to register.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  int devfreq_register_notifier(struct devfreq *devfreq,
>  				struct notifier_block *nb,
> @@ -1461,6 +1478,10 @@ int devfreq_register_notifier(struct devfreq *devfreq,
>  		ret = srcu_notifier_chain_register(
>  				&devfreq->transition_notifier_list, nb);
>  		break;
> +	case DEVFREQ_POLICY_NOTIFIER:
> +		ret = srcu_notifier_chain_register(
> +				&devfreq->policy_notifier_list, nb);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -1473,7 +1494,7 @@ EXPORT_SYMBOL(devfreq_register_notifier);
>   * devfreq_unregister_notifier() - Unregister a driver with devfreq
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to be unregistered.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  int devfreq_unregister_notifier(struct devfreq *devfreq,
>  				struct notifier_block *nb,
> @@ -1489,6 +1510,11 @@ int devfreq_unregister_notifier(struct devfreq *devfreq,
>  		ret = srcu_notifier_chain_unregister(
>  				&devfreq->transition_notifier_list, nb);
>  		break;
> +	case DEVFREQ_POLICY_NOTIFIER:
> +		ret = srcu_notifier_chain_unregister(
> +				&devfreq->policy_notifier_list, nb);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  	}
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 9bf23b976f4d..7c8dce96db73 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -33,6 +33,10 @@
>  #define	DEVFREQ_PRECHANGE		(0)
>  #define DEVFREQ_POSTCHANGE		(1)
>  
> +#define DEVFREQ_POLICY_NOTIFIER		1
> +
> +#define	DEVFREQ_ADJUST			0
> +
>  struct devfreq;
>  struct devfreq_governor;
>  
> @@ -121,12 +125,16 @@ struct devfreq_freq_limits {
>  
>  /**
>   * struct devfreq_policy - Devfreq policy
> + * @min:	minimum frequency (adjustable by policy notifiers)
> + * @min:	maximum frequency (adjustable by policy notifiers)
>   * @user:	frequency limits requested by the user
>   * @devinfo:	frequency limits of the device (available OPPs)
>   * @governor:	method how to choose frequency based on the usage.
>   * @governor_name:	devfreq governor name for use with this devfreq
>   */
>  struct devfreq_policy {
> +	unsigned long min;
> +	unsigned long max;
>  	struct devfreq_freq_limits user;
>  	struct devfreq_freq_limits devinfo;
>  	const struct devfreq_governor *governor;
> @@ -155,6 +163,7 @@ struct devfreq_policy {
>   * @time_in_state:	Statistics of devfreq states
>   * @last_stat_updated:	The last time stat updated
>   * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
> + * @policy_notifier_list: list head of DEVFREQ_POLICY_NOTIFIER notifier
>   *
>   * This structure stores the devfreq information for a give device.
>   *
> @@ -188,6 +197,7 @@ struct devfreq {
>  	unsigned long last_stat_updated;
>  
>  	struct srcu_notifier_head transition_notifier_list;
> +	struct srcu_notifier_head policy_notifier_list;
>  };
>  
>  struct devfreq_freqs {
> @@ -240,6 +250,45 @@ extern void devm_devfreq_unregister_notifier(struct device *dev,
>  extern struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev,
>  						int index);
>  
> +/**
> + * devfreq_verify_within_limits() - Adjust a devfreq policy if needed to make
> + *                                  sure its min/max values are within a
> + *                                  specified range.
> + * @policy:	the policy
> + * @min:	the minimum frequency
> + * @max:	the maximum frequency
> + */
> +static inline void devfreq_verify_within_limits(struct devfreq_policy *policy,
> +		unsigned int min, unsigned int max)
> +{
> +	if (policy->min < min)
> +		policy->min = min;
> +	if (policy->max < min)
> +		policy->max = min;
> +	if (policy->min > max)
> +		policy->min = max;
> +	if (policy->max > max)
> +		policy->max = max;
> +	if (policy->min > policy->max)
> +		policy->min = policy->max;
> +}
> +
> +/**
> + * devfreq_verify_within_dev_limits() - Adjust a devfreq policy if needed to
> + *                                      make sure its min/max values are within
> + *                                      the frequency range supported by the
> + *                                      device.
> + * @policy:	the policy
> + */
> +static inline void
> +devfreq_verify_within_dev_limits(struct devfreq_policy *policy)
> +{
> +	devfreq_verify_within_limits(policy, policy->devinfo.min_freq,
> +			policy->devinfo.max_freq);
> +}
> +
> +struct devfreq *dev_to_devfreq(struct device *dev);
> +
>  #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
>  /**
>   * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
> @@ -394,10 +443,26 @@ static inline struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev,
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +static inline void devfreq_verify_within_limits(struct devfreq_policy *policy,
> +		unsigned int min, unsigned int max)
> +{
> +}
> +
> +static inline void
> +devfreq_verify_within_dev_limits(struct devfreq_policy *policy)
> +{
> +}
> +
>  static inline int devfreq_update_stats(struct devfreq *df)
>  {
>  	return -EINVAL;
>  }
> +
> +static inline struct devfreq *dev_to_devfreq(struct device *dev)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_PM_DEVFREQ */
>  
>  #endif /* __LINUX_DEVFREQ_H__ */
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists