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@xxxxxxxxxxxx>
> Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> ---
> Changes in v5:
> - none
>
> Changes in v4:
> - Fixed typo in commit message: devfreg => devfreq
> - added 'Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>' 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
