Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Leonard Crestez <leonard.crestez@nxp.com>
To: Chanwoo Choi <cw00.choi@samsung.com>
Cc: "Matthias Kaehlcke" <mka@chromium.org>,
	"MyungJoo Ham" <myungjoo.ham@samsung.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Artur Świgoń" <a.swigon@partner.samsung.com>,
	"Saravana Kannan" <saravanak@google.com>,
	"Krzysztof Kozlowski" <krzk@kernel.org>,
	"Alexandre Bailon" <abailon@baylibre.com>,
	"Georgi Djakov" <georgi.djakov@linaro.org>,
	"Abel Vesa" <abel.vesa@nxp.com>, "Jacky Bai" <ping.bai@nxp.com>,
	"Viresh Kumar" <viresh.kumar@linaro.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper
Date: Thu, 31 Oct 2019 13:12:58 +0000
Message-ID: <VI1PR04MB702322FBAD1EB3F7C9AB8B9CEE630@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <a4b4c295-b230-5f15-61a2-d0e826584393@samsung.com>

On 31.10.2019 04:37, Chanwoo Choi wrote:
> On 19. 10. 3. 오전 4:25, Leonard Crestez wrote:
>> Moving handling of min/max freq to a single function and call it from
>> update_devfreq and for printing min/max freq values in sysfs.
>>
>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>> is now done at evaluation time. This means that if an out-of-range
>> constraint is imposed by sysfs and it later becomes valid then it will
>> be enforced.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
>>   1 file changed, 62 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 87eff789ce24..2d63692903ff 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -96,10 +96,53 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>   		dev_pm_opp_put(opp);
>>   
>>   	return max_freq;
>>   }
>>   
>> +/**
>> + * get_freq_range() - Get the current freq range
>> + * @devfreq:	the devfreq instance
>> + * @min_freq:	the min frequency
>> + * @max_freq:	the max frequency
>> + *
>> + * This takes into consideration all constraints.
>> + */
>> +static void get_freq_range(struct devfreq *devfreq,
>> +			   unsigned long *min_freq,
>> +			   unsigned long *max_freq)
>> +{
>> +	unsigned long *freq_table = devfreq->profile->freq_table;
>> +
>> +	lockdep_assert_held(&devfreq->lock);
>> +
>> +	/*
>> +	 * Initialize minimum/maximum frequency from freq table.
>> +	 * The devfreq drivers can initialize this in either ascending or
>> +	 * descending order and devfreq core supports both.
>> +	 */
>> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
>> +		*min_freq = freq_table[0];
>> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
>> +	} else {
>> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
>> +		*max_freq = freq_table[0];
>> +	}
>> +
>> +	/* Apply constraints from sysfs */
>> +	*min_freq = max(*min_freq, devfreq->min_freq);
>> +	*max_freq = min(*max_freq, devfreq->max_freq);
>> +
>> +	/* Apply constraints from OPP interface */
>> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>> +	/* scaling_max_freq can be zero on error */
>> +	if (devfreq->scaling_max_freq)
> 
> nitpick:
> This if statement is necessary?
> 
> The patch3 in this series initializes the 'devfreq->scaling_max_freq'
> is ULONG_MAX if failed to get the available frequency from find_available_max_freq.

Ok, seem I forgot to drop the if statement.

> 
>> +		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> +
>> +	if (*min_freq > *max_freq)
>> +		*min_freq = *max_freq;
>> +}
>> +
>>   /**
>>    * devfreq_get_freq_level() - Lookup freq_table for the frequency
>>    * @devfreq:	the devfreq instance
>>    * @freq:	the target frequency
>>    */
>> @@ -348,20 +391,11 @@ int update_devfreq(struct devfreq *devfreq)
>>   
>>   	/* Reevaluate the proper frequency */
>>   	err = devfreq->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(devfreq->scaling_max_freq, devfreq->max_freq);
>> -	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
>> +	get_freq_range(devfreq, &min_freq, &max_freq);
>>   
>>   	if (freq < min_freq) {
>>   		freq = min_freq;
>>   		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>   	}
>> @@ -1281,40 +1315,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>   	ret = sscanf(buf, "%lu", &value);
>>   	if (ret != 1)
>>   		return -EINVAL;
>>   
>>   	mutex_lock(&df->lock);
>> -
>> -	if (value) {
>> -		if (value > df->max_freq) {
>> -			ret = -EINVAL;
>> -			goto unlock;
>> -		}
>> -	} else {
>> -		unsigned long *freq_table = df->profile->freq_table;
>> -
>> -		/* Get minimum frequency according to sorting order */
>> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
>> -			value = freq_table[0];
>> -		else
>> -			value = freq_table[df->profile->max_state - 1];
>> -	}
>> -
>>   	df->min_freq = value;
>>   	update_devfreq(df);
>> -	ret = count;
>> -unlock:
>>   	mutex_unlock(&df->lock);
>> -	return ret;
>> +
>> +	return count;
>>   }
>>   
>>   static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>   			     char *buf)
>>   {
>>   	struct devfreq *df = to_devfreq(dev);
>> +	unsigned long min_freq, max_freq;
>>   
>> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
>> +	mutex_lock(&df->lock);
>> +	get_freq_range(df, &min_freq, &max_freq);
>> +	mutex_unlock(&df->lock);
>> +
>> +	return sprintf(buf, "%lu\n", min_freq);
>>   }
>>   
>>   static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>   			      const char *buf, size_t count)
>>   {
>> @@ -1326,40 +1348,32 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>   	if (ret != 1)
>>   		return -EINVAL;
>>   
>>   	mutex_lock(&df->lock);
>>   
>> -	if (value) {
>> -		if (value < df->min_freq) {
>> -			ret = -EINVAL;
>> -			goto unlock;
>> -		}
>> -	} else {
>> -		unsigned long *freq_table = df->profile->freq_table;
>> -
>> -		/* Get maximum frequency according to sorting order */
>> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
>> -			value = freq_table[df->profile->max_state - 1];
>> -		else
>> -			value = freq_table[0];
>> -	}
>> +	if (!value)
>> +		value = ULONG_MAX;
>>   
>>   	df->max_freq = value;
>>   	update_devfreq(df);
>> -	ret = count;
>> -unlock:
>>   	mutex_unlock(&df->lock);
>> -	return ret;
>> +
>> +	return count;
>>   }
>>   static DEVICE_ATTR_RW(min_freq);
>>   
>>   static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>   			     char *buf)
>>   {
>>   	struct devfreq *df = to_devfreq(dev);
>> +	unsigned long min_freq, max_freq;
>> +
>> +	mutex_lock(&df->lock);
>> +	get_freq_range(df, &min_freq, &max_freq);
>> +	mutex_unlock(&df->lock);
>>   
>> -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
>> +	return sprintf(buf, "%lu\n", max_freq);
>>   }
>>   static DEVICE_ATTR_RW(max_freq);
>>   
>>   static ssize_t available_frequencies_show(struct device *d,
>>   					  struct device_attribute *attr,
>>
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 


  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-02 19:25 [PATCH v9 0/8] PM / devfreq: Add dev_pm_qos support Leonard Crestez
2019-10-02 19:25 ` [PATCH v9 1/8] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
2019-10-02 19:25 ` [PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno Leonard Crestez
2019-10-02 21:24   ` Matthias Kaehlcke
2019-10-02 23:47     ` Leonard Crestez
2019-10-31  2:29   ` Chanwoo Choi
2019-10-02 19:25 ` [PATCH v9 3/8] PM / devfreq: Set scaling_max_freq to max on OPP notifier error Leonard Crestez
2019-10-02 21:33   ` Matthias Kaehlcke
2019-10-31  2:21   ` Chanwoo Choi
2019-10-02 19:25 ` [PATCH v9 4/8] PM / devfreq: Move more initialization before registration Leonard Crestez
2019-10-31  3:15   ` Chanwoo Choi
2019-10-31 13:31     ` Leonard Crestez
2019-11-01  8:31       ` Chanwoo Choi
2019-11-01 14:36         ` Leonard Crestez
2019-10-02 19:25 ` [PATCH v9 5/8] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
2019-10-02 19:25 ` [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper Leonard Crestez
2019-10-03 18:19   ` Matthias Kaehlcke
2019-10-03 19:16     ` Leonard Crestez
2019-10-03 19:36       ` Matthias Kaehlcke
2019-10-11 18:29     ` Matthias Kaehlcke
2019-10-31  2:44       ` Chanwoo Choi
2019-10-31  2:42   ` Chanwoo Choi
2019-10-31 13:12     ` Leonard Crestez [this message]
2019-10-02 19:25 ` [PATCH v9 7/8] PM / devfreq: Add PM QoS support Leonard Crestez
2019-10-02 22:22   ` Matthias Kaehlcke
2019-10-04 17:04   ` Matthias Kaehlcke
2019-10-31  3:01   ` Chanwoo Choi
2019-10-31 13:21     ` Leonard Crestez
2019-10-02 19:25 ` [PATCH v9 8/8] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
2019-10-04 17:05   ` Matthias Kaehlcke
2019-10-31  3:10   ` Chanwoo Choi
2019-10-23 14:06 ` [PATCH v9 0/8] PM / devfreq: Add dev_pm_qos support Leonard Crestez
2019-10-23 16:34   ` Matthias Kaehlcke
2019-10-24 16:21     ` Leonard Crestez

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR04MB702322FBAD1EB3F7C9AB8B9CEE630@VI1PR04MB7023.eurprd04.prod.outlook.com \
    --to=leonard.crestez@nxp.com \
    --cc=a.swigon@partner.samsung.com \
    --cc=abailon@baylibre.com \
    --cc=abel.vesa@nxp.com \
    --cc=cw00.choi@samsung.com \
    --cc=georgi.djakov@linaro.org \
    --cc=krzk@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-pm@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=ping.bai@nxp.com \
    --cc=saravanak@google.com \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git