diff mbox series

[2/2] PM / devfreq: Use exclusively PM QoS to determine frequency limits

Message ID 20200110094913.2.Ie8eacf976ce7a13e421592f5c1ab8dbdc537da5c@changeid (mailing list archive)
State Rejected
Delegated to: Chanwoo Choi
Headers show
Series [1/2] thermal: devfreq_cooling: Use PM QoS to set frequency limits | expand

Commit Message

Matthias Kaehlcke Jan. 10, 2020, 5:49 p.m. UTC
Traditionally devfreq cooling devices dynamically disabled OPPs
that shouldn't be used because of thermal pressure. Devfreq cooling
devices now use PM QoS to set frequency limits, hence the devfreq
code dealing that deals with disabled OPPs can be removed.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 drivers/devfreq/devfreq.c | 75 +++++----------------------------------
 include/linux/devfreq.h   |  4 ---
 2 files changed, 8 insertions(+), 71 deletions(-)

Comments

Chanwoo Choi Jan. 13, 2020, 7:31 a.m. UTC | #1
Hi,


Any device driver except for devfreq_cooling.c might
use dev_pm_opp_enable/disable interface. 
So, don't need to remove the devfreq->scaling_max_freq
and devfreq->scaling_min_freq for supporting OPP interface.

Regards,
Chanwoo Choi

On 1/11/20 2:49 AM, Matthias Kaehlcke wrote:
> Traditionally devfreq cooling devices dynamically disabled OPPs
> that shouldn't be used because of thermal pressure. Devfreq cooling
> devices now use PM QoS to set frequency limits, hence the devfreq
> code dealing that deals with disabled OPPs can be removed.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  drivers/devfreq/devfreq.c | 75 +++++----------------------------------
>  include/linux/devfreq.h   |  4 ---
>  2 files changed, 8 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 57f6944d65a6..ec66e2c27cc4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -73,34 +73,6 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>  	return ERR_PTR(-ENODEV);
>  }
>  
> -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);
> -
> -	return min_freq;
> -}
> -
> -static unsigned long find_available_max_freq(struct devfreq *devfreq)
> -{
> -	struct dev_pm_opp *opp;
> -	unsigned long max_freq = ULONG_MAX;
> -
> -	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
> -	if (IS_ERR(opp))
> -		max_freq = 0;
> -	else
> -		dev_pm_opp_put(opp);
> -
> -	return max_freq;
> -}
> -
>  /**
>   * get_freq_range() - Get the current freq range
>   * @devfreq:	the devfreq instance
> @@ -141,10 +113,6 @@ static void get_freq_range(struct devfreq *devfreq,
>  		*max_freq = min(*max_freq,
>  				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>  
> -	/* Apply constraints from OPP interface */
> -	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
> -	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> -
>  	if (*min_freq > *max_freq)
>  		*min_freq = *max_freq;
>  }
> @@ -610,23 +578,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  				 void *devp)
>  {
>  	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> -	int err = -EINVAL;
> +	int err;
>  
>  	mutex_lock(&devfreq->lock);
> -
> -	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> -	if (!devfreq->scaling_min_freq)
> -		goto out;
> -
> -	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq) {
> -		devfreq->scaling_max_freq = ULONG_MAX;
> -		goto out;
> -	}
> -
>  	err = update_devfreq(devfreq);
> -
> -out:
>  	mutex_unlock(&devfreq->lock);
>  	if (err)
>  		dev_err(devfreq->dev.parent,
> @@ -781,19 +736,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		mutex_lock(&devfreq->lock);
>  	}
>  
> -	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> -	if (!devfreq->scaling_min_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		err = -EINVAL;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
>  		goto err_dev;
> -	}
> -
> -	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		err = -EINVAL;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY,
> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> +	if (err < 0)
>  		goto err_dev;
> -	}
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
> @@ -834,16 +785,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  	mutex_unlock(&devfreq->lock);
>  
> -	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> -				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> -	if (err < 0)
> -		goto err_devfreq;
> -	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> -				     DEV_PM_QOS_MAX_FREQUENCY,
> -				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> -	if (err < 0)
> -		goto err_devfreq;
> -
>  	devfreq->nb_min.notifier_call = qos_min_notifier_call;
>  	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>  				      DEV_PM_QOS_MIN_FREQUENCY);
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fb376b5b7281..cb75f23ad2f4 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -126,8 +126,6 @@ struct devfreq_dev_profile {
>   *		touch this.
>   * @user_min_freq_req:	PM QoS minimum frequency request from user (via sysfs)
>   * @user_max_freq_req:	PM QoS maximum frequency request from user (via sysfs)
> - * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
> - * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -166,8 +164,6 @@ struct devfreq {
>  
>  	struct dev_pm_qos_request user_min_freq_req;
>  	struct dev_pm_qos_request user_max_freq_req;
> -	unsigned long scaling_min_freq;
> -	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;
>
Leonard Crestez Jan. 14, 2020, 4:08 p.m. UTC | #2
On 13.01.2020 09:24, Chanwoo Choi wrote:
> Hi,
> 
> Any device driver except for devfreq_cooling.c might
> use dev_pm_opp_enable/disable interface.
> So, don't need to remove the devfreq->scaling_max_freq
> and devfreq->scaling_min_freq for supporting OPP interface.

It seems that devfreq_cooling was the only upstream user of 
dev_pm_opp_enable and the remaining callers of dev_pm_opp_disable are 
probe-time checks.

> Regards,
> Chanwoo Choi
> 
> On 1/11/20 2:49 AM, Matthias Kaehlcke wrote:
>> Traditionally devfreq cooling devices dynamically disabled OPPs
>> that shouldn't be used because of thermal pressure. Devfreq cooling
>> devices now use PM QoS to set frequency limits, hence the devfreq
>> code dealing that deals with disabled OPPs can be removed.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>
>>   drivers/devfreq/devfreq.c | 75 +++++----------------------------------
>>   include/linux/devfreq.h   |  4 ---
>>   2 files changed, 8 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 57f6944d65a6..ec66e2c27cc4 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -73,34 +73,6 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>>   	return ERR_PTR(-ENODEV);
>>   }
>>   
>> -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);
>> -
>> -	return min_freq;
>> -}
>> -
>> -static unsigned long find_available_max_freq(struct devfreq *devfreq)
>> -{
>> -	struct dev_pm_opp *opp;
>> -	unsigned long max_freq = ULONG_MAX;
>> -
>> -	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
>> -	if (IS_ERR(opp))
>> -		max_freq = 0;
>> -	else
>> -		dev_pm_opp_put(opp);
>> -
>> -	return max_freq;
>> -}
>> -
>>   /**
>>    * get_freq_range() - Get the current freq range
>>    * @devfreq:	the devfreq instance
>> @@ -141,10 +113,6 @@ static void get_freq_range(struct devfreq *devfreq,
>>   		*max_freq = min(*max_freq,
>>   				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>>   
>> -	/* Apply constraints from OPP interface */
>> -	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>> -	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> -
>>   	if (*min_freq > *max_freq)
>>   		*min_freq = *max_freq;
>>   }
>> @@ -610,23 +578,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>   				 void *devp)
>>   {
>>   	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
>> -	int err = -EINVAL;
>> +	int err;
>>   
>>   	mutex_lock(&devfreq->lock);
>> -
>> -	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> -	if (!devfreq->scaling_min_freq)
>> -		goto out;
>> -
>> -	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> -	if (!devfreq->scaling_max_freq) {
>> -		devfreq->scaling_max_freq = ULONG_MAX;
>> -		goto out;
>> -	}
>> -
>>   	err = update_devfreq(devfreq);
>> -
>> -out:
>>   	mutex_unlock(&devfreq->lock);
>>   	if (err)
>>   		dev_err(devfreq->dev.parent,
>> @@ -781,19 +736,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   		mutex_lock(&devfreq->lock);
>>   	}
>>   
>> -	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> -	if (!devfreq->scaling_min_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		err = -EINVAL;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0)
>>   		goto err_dev;
>> -	}
>> -
>> -	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> -	if (!devfreq->scaling_max_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		err = -EINVAL;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> +				     DEV_PM_QOS_MAX_FREQUENCY,
>> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>> +	if (err < 0)
>>   		goto err_dev;
>> -	}
>>   
>>   	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>>   	atomic_set(&devfreq->suspend_count, 0);
>> @@ -834,16 +785,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   
>>   	mutex_unlock(&devfreq->lock);
>>   
>> -	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> -				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> -	if (err < 0)
>> -		goto err_devfreq;
>> -	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> -				     DEV_PM_QOS_MAX_FREQUENCY,
>> -				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>> -	if (err < 0)
>> -		goto err_devfreq;
>> -

Performing PM QoS initialization under devfreq->lock triggers lockdep 
warnings for me. The warnings seem to be legitimate:

1) At init time &dev_pm_qos_mtx is taken under &devfreq->lock;
2) At update time &devfreq->lock is called under &dev_pm_qos_mtx (it's 
held while notifiers are called).

It's not clear why you moved dev_pm_qos_add_request higher?

>>   	devfreq->nb_min.notifier_call = qos_min_notifier_call;
>>   	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>   				      DEV_PM_QOS_MIN_FREQUENCY);
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index fb376b5b7281..cb75f23ad2f4 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -126,8 +126,6 @@ struct devfreq_dev_profile {
>>    *		touch this.
>>    * @user_min_freq_req:	PM QoS minimum frequency request from user (via sysfs)
>>    * @user_max_freq_req:	PM QoS maximum frequency request from user (via sysfs)
>> - * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>> - * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>>    * @stop_polling:	 devfreq polling status of a device.
>>    * @suspend_freq:	 frequency of a device set during suspend phase.
>>    * @resume_freq:	 frequency of a device set in resume phase.
>> @@ -166,8 +164,6 @@ struct devfreq {
>>   
>>   	struct dev_pm_qos_request user_min_freq_req;
>>   	struct dev_pm_qos_request user_max_freq_req;
>> -	unsigned long scaling_min_freq;
>> -	unsigned long scaling_max_freq;
>>   	bool stop_polling;
>>   
>>   	unsigned long suspend_freq;
>>
>
Chanwoo Choi Jan. 14, 2020, 5:35 p.m. UTC | #3
On Wed, Jan 15, 2020 at 1:08 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 13.01.2020 09:24, Chanwoo Choi wrote:
> > Hi,
> >
> > Any device driver except for devfreq_cooling.c might
> > use dev_pm_opp_enable/disable interface.
> > So, don't need to remove the devfreq->scaling_max_freq
> > and devfreq->scaling_min_freq for supporting OPP interface.
>
> It seems that devfreq_cooling was the only upstream user of
> dev_pm_opp_enable and the remaining callers of dev_pm_opp_disable are
> probe-time checks.

OPP interface has still dev_pm_opp_enable and dev_pm_opp_disable
function. As long as remains them, any device driver related to devfreq
could call them at some time. The devfreq supports the OPP interface,
not just for only devfreq_cooling.

>
> > Regards,
> > Chanwoo Choi
> >
> > On 1/11/20 2:49 AM, Matthias Kaehlcke wrote:
> >> Traditionally devfreq cooling devices dynamically disabled OPPs
> >> that shouldn't be used because of thermal pressure. Devfreq cooling
> >> devices now use PM QoS to set frequency limits, hence the devfreq
> >> code dealing that deals with disabled OPPs can be removed.
> >>
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>
> >>   drivers/devfreq/devfreq.c | 75 +++++----------------------------------
> >>   include/linux/devfreq.h   |  4 ---
> >>   2 files changed, 8 insertions(+), 71 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index 57f6944d65a6..ec66e2c27cc4 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -73,34 +73,6 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> >>      return ERR_PTR(-ENODEV);
> >>   }
> >>
> >> -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);
> >> -
> >> -    return min_freq;
> >> -}
> >> -
> >> -static unsigned long find_available_max_freq(struct devfreq *devfreq)
> >> -{
> >> -    struct dev_pm_opp *opp;
> >> -    unsigned long max_freq = ULONG_MAX;
> >> -
> >> -    opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
> >> -    if (IS_ERR(opp))
> >> -            max_freq = 0;
> >> -    else
> >> -            dev_pm_opp_put(opp);
> >> -
> >> -    return max_freq;
> >> -}
> >> -
> >>   /**
> >>    * get_freq_range() - Get the current freq range
> >>    * @devfreq:       the devfreq instance
> >> @@ -141,10 +113,6 @@ static void get_freq_range(struct devfreq *devfreq,
> >>              *max_freq = min(*max_freq,
> >>                              (unsigned long)HZ_PER_KHZ * qos_max_freq);
> >>
> >> -    /* Apply constraints from OPP interface */
> >> -    *min_freq = max(*min_freq, devfreq->scaling_min_freq);
> >> -    *max_freq = min(*max_freq, devfreq->scaling_max_freq);
> >> -
> >>      if (*min_freq > *max_freq)
> >>              *min_freq = *max_freq;
> >>   }
> >> @@ -610,23 +578,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
> >>                               void *devp)
> >>   {
> >>      struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> >> -    int err = -EINVAL;
> >> +    int err;
> >>
> >>      mutex_lock(&devfreq->lock);
> >> -
> >> -    devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> >> -    if (!devfreq->scaling_min_freq)
> >> -            goto out;
> >> -
> >> -    devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> >> -    if (!devfreq->scaling_max_freq) {
> >> -            devfreq->scaling_max_freq = ULONG_MAX;
> >> -            goto out;
> >> -    }
> >> -
> >>      err = update_devfreq(devfreq);
> >> -
> >> -out:
> >>      mutex_unlock(&devfreq->lock);
> >>      if (err)
> >>              dev_err(devfreq->dev.parent,
> >> @@ -781,19 +736,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>              mutex_lock(&devfreq->lock);
> >>      }
> >>
> >> -    devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> >> -    if (!devfreq->scaling_min_freq) {
> >> -            mutex_unlock(&devfreq->lock);
> >> -            err = -EINVAL;
> >> +    err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> >> +                                 DEV_PM_QOS_MIN_FREQUENCY, 0);
> >> +    if (err < 0)
> >>              goto err_dev;
> >> -    }
> >> -
> >> -    devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> >> -    if (!devfreq->scaling_max_freq) {
> >> -            mutex_unlock(&devfreq->lock);
> >> -            err = -EINVAL;
> >> +    err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> >> +                                 DEV_PM_QOS_MAX_FREQUENCY,
> >> +                                 PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> >> +    if (err < 0)
> >>              goto err_dev;
> >> -    }
> >>
> >>      devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> >>      atomic_set(&devfreq->suspend_count, 0);
> >> @@ -834,16 +785,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>
> >>      mutex_unlock(&devfreq->lock);
> >>
> >> -    err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> >> -                                 DEV_PM_QOS_MIN_FREQUENCY, 0);
> >> -    if (err < 0)
> >> -            goto err_devfreq;
> >> -    err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> >> -                                 DEV_PM_QOS_MAX_FREQUENCY,
> >> -                                 PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> >> -    if (err < 0)
> >> -            goto err_devfreq;
> >> -
>
> Performing PM QoS initialization under devfreq->lock triggers lockdep
> warnings for me. The warnings seem to be legitimate:
>
> 1) At init time &dev_pm_qos_mtx is taken under &devfreq->lock;
> 2) At update time &devfreq->lock is called under &dev_pm_qos_mtx (it's
> held while notifiers are called).
>
> It's not clear why you moved dev_pm_qos_add_request higher?
>
> >>      devfreq->nb_min.notifier_call = qos_min_notifier_call;
> >>      err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
> >>                                    DEV_PM_QOS_MIN_FREQUENCY);
> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> >> index fb376b5b7281..cb75f23ad2f4 100644
> >> --- a/include/linux/devfreq.h
> >> +++ b/include/linux/devfreq.h
> >> @@ -126,8 +126,6 @@ struct devfreq_dev_profile {
> >>    *         touch this.
> >>    * @user_min_freq_req:     PM QoS minimum frequency request from user (via sysfs)
> >>    * @user_max_freq_req:     PM QoS maximum frequency request from user (via sysfs)
> >> - * @scaling_min_freq:       Limit minimum frequency requested by OPP interface
> >> - * @scaling_max_freq:       Limit maximum frequency requested by OPP interface
> >>    * @stop_polling:   devfreq polling status of a device.
> >>    * @suspend_freq:   frequency of a device set during suspend phase.
> >>    * @resume_freq:    frequency of a device set in resume phase.
> >> @@ -166,8 +164,6 @@ struct devfreq {
> >>
> >>      struct dev_pm_qos_request user_min_freq_req;
> >>      struct dev_pm_qos_request user_max_freq_req;
> >> -    unsigned long scaling_min_freq;
> >> -    unsigned long scaling_max_freq;
> >>      bool stop_polling;
> >>
> >>      unsigned long suspend_freq;
> >>
> >
>
Matthias Kaehlcke Jan. 14, 2020, 7:13 p.m. UTC | #4
On Wed, Jan 15, 2020 at 02:35:48AM +0900, Chanwoo Choi wrote:
> On Wed, Jan 15, 2020 at 1:08 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> >
> > On 13.01.2020 09:24, Chanwoo Choi wrote:
> > > Hi,
> > >
> > > Any device driver except for devfreq_cooling.c might
> > > use dev_pm_opp_enable/disable interface.
> > > So, don't need to remove the devfreq->scaling_max_freq
> > > and devfreq->scaling_min_freq for supporting OPP interface.
> >
> > It seems that devfreq_cooling was the only upstream user of
> > dev_pm_opp_enable and the remaining callers of dev_pm_opp_disable are
> > probe-time checks.
> 
> OPP interface has still dev_pm_opp_enable and dev_pm_opp_disable
> function. As long as remains them, any device driver related to devfreq
> could call them at some time. The devfreq supports the OPP interface,
> not just for only devfreq_cooling.

I would like to remove the disabled OPP handling since no devfreq device
makes use of dev_pm_opp_enable/disable, but I fear you are right that
we have to keep it as long as the API is available.
diff mbox series

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 57f6944d65a6..ec66e2c27cc4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -73,34 +73,6 @@  static struct devfreq *find_device_devfreq(struct device *dev)
 	return ERR_PTR(-ENODEV);
 }
 
-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);
-
-	return min_freq;
-}
-
-static unsigned long find_available_max_freq(struct devfreq *devfreq)
-{
-	struct dev_pm_opp *opp;
-	unsigned long max_freq = ULONG_MAX;
-
-	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
-	if (IS_ERR(opp))
-		max_freq = 0;
-	else
-		dev_pm_opp_put(opp);
-
-	return max_freq;
-}
-
 /**
  * get_freq_range() - Get the current freq range
  * @devfreq:	the devfreq instance
@@ -141,10 +113,6 @@  static void get_freq_range(struct devfreq *devfreq,
 		*max_freq = min(*max_freq,
 				(unsigned long)HZ_PER_KHZ * qos_max_freq);
 
-	/* Apply constraints from OPP interface */
-	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
-	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
-
 	if (*min_freq > *max_freq)
 		*min_freq = *max_freq;
 }
@@ -610,23 +578,10 @@  static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 				 void *devp)
 {
 	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
-	int err = -EINVAL;
+	int err;
 
 	mutex_lock(&devfreq->lock);
-
-	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
-	if (!devfreq->scaling_min_freq)
-		goto out;
-
-	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
-	if (!devfreq->scaling_max_freq) {
-		devfreq->scaling_max_freq = ULONG_MAX;
-		goto out;
-	}
-
 	err = update_devfreq(devfreq);
-
-out:
 	mutex_unlock(&devfreq->lock);
 	if (err)
 		dev_err(devfreq->dev.parent,
@@ -781,19 +736,15 @@  struct devfreq *devfreq_add_device(struct device *dev,
 		mutex_lock(&devfreq->lock);
 	}
 
-	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
-	if (!devfreq->scaling_min_freq) {
-		mutex_unlock(&devfreq->lock);
-		err = -EINVAL;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
 		goto err_dev;
-	}
-
-	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
-	if (!devfreq->scaling_max_freq) {
-		mutex_unlock(&devfreq->lock);
-		err = -EINVAL;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY,
+				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+	if (err < 0)
 		goto err_dev;
-	}
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
@@ -834,16 +785,6 @@  struct devfreq *devfreq_add_device(struct device *dev,
 
 	mutex_unlock(&devfreq->lock);
 
-	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
-				     DEV_PM_QOS_MIN_FREQUENCY, 0);
-	if (err < 0)
-		goto err_devfreq;
-	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
-				     DEV_PM_QOS_MAX_FREQUENCY,
-				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
-	if (err < 0)
-		goto err_devfreq;
-
 	devfreq->nb_min.notifier_call = qos_min_notifier_call;
 	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
 				      DEV_PM_QOS_MIN_FREQUENCY);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index fb376b5b7281..cb75f23ad2f4 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -126,8 +126,6 @@  struct devfreq_dev_profile {
  *		touch this.
  * @user_min_freq_req:	PM QoS minimum frequency request from user (via sysfs)
  * @user_max_freq_req:	PM QoS maximum frequency request from user (via sysfs)
- * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
- * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -166,8 +164,6 @@  struct devfreq {
 
 	struct dev_pm_qos_request user_min_freq_req;
 	struct dev_pm_qos_request user_max_freq_req;
-	unsigned long scaling_min_freq;
-	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;