[v2,18/18] cpufreq: scmi: add support for fast frequency switching
diff mbox

Message ID 1501857104-11279-19-git-send-email-sudeep.holla@arm.com
State Deferred
Headers show

Commit Message

Sudeep Holla Aug. 4, 2017, 2:31 p.m. UTC
The cpufreq core provides option for drivers to implement fast_switch
callback which is invoked for frequency switching from interrupt context.

This patch adds support for fast_switch callback in SCMI cpufreq driver
by making use of polling based SCMI transfer. It also sets the flag
fast_switch_possible.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/scmi-cpufreq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Viresh Kumar Aug. 9, 2017, 4:28 a.m. UTC | #1
On 04-08-17, 15:31, Sudeep Holla wrote:
> The cpufreq core provides option for drivers to implement fast_switch
> callback which is invoked for frequency switching from interrupt context.
> 
> This patch adds support for fast_switch callback in SCMI cpufreq driver
> by making use of polling based SCMI transfer. It also sets the flag
> fast_switch_possible.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 034359cafea5..cb1084cb1ef1 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -61,6 +61,19 @@ scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
>  	return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false);
>  }
>  
> +static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> +					     unsigned int target_freq)
> +{
> +	struct scmi_data *priv = policy->driver_data;
> +	struct scmi_perf_ops *perf_ops = priv->handle->perf_ops;
> +
> +	if (!perf_ops->freq_set(priv->handle, priv->domain_id,
> +				target_freq * 1000, true))
> +		return target_freq;
> +
> +	return CPUFREQ_ENTRY_INVALID;
> +}

This is very much similar to the target routine, perhaps we should write another
local routine which is used by both target and fast switch.

Do we guarantee that the frequency is changed by the time this routine returns?
Or we just send a SCMI request and return back ?

If we just send the request and don't wait for freq to get changed, what
protects another scmi_cpufreq_fast_switch() to get called before the first one
is finished? And what will happen on that call ?

>  static int
>  scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>  {
> @@ -164,6 +177,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  
>  	policy->cpuinfo.transition_latency = latency;
>  
> +	policy->fast_switch_possible = true;
>  	return 0;
>  
>  out_free_cpufreq_table:
> @@ -180,6 +194,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
>  {
>  	struct scmi_data *priv = policy->driver_data;
>  
> +	policy->fast_switch_possible = false;
>  	cpufreq_cooling_unregister(priv->cdev);
>  	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
>  	dev_pm_opp_cpumask_remove_table(policy->related_cpus);
> @@ -228,6 +243,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
>  	.init			= scmi_cpufreq_init,
>  	.exit			= scmi_cpufreq_exit,
>  	.ready			= scmi_cpufreq_ready,
> +	.fast_switch		= scmi_cpufreq_fast_switch,

Maybe add it right after target_index ?
Sudeep Holla Aug. 9, 2017, 10:09 a.m. UTC | #2
On 09/08/17 05:28, Viresh Kumar wrote:
> On 04-08-17, 15:31, Sudeep Holla wrote:
>> The cpufreq core provides option for drivers to implement fast_switch
>> callback which is invoked for frequency switching from interrupt context.
>>
>> This patch adds support for fast_switch callback in SCMI cpufreq driver
>> by making use of polling based SCMI transfer. It also sets the flag
>> fast_switch_possible.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/cpufreq/scmi-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index 034359cafea5..cb1084cb1ef1 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -61,6 +61,19 @@ scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
>>  	return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false);
>>  }
>>  
>> +static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
>> +					     unsigned int target_freq)
>> +{
>> +	struct scmi_data *priv = policy->driver_data;
>> +	struct scmi_perf_ops *perf_ops = priv->handle->perf_ops;
>> +
>> +	if (!perf_ops->freq_set(priv->handle, priv->domain_id,
>> +				target_freq * 1000, true))
>> +		return target_freq;
>> +
>> +	return CPUFREQ_ENTRY_INVALID;
>> +}
> 
> This is very much similar to the target routine, perhaps we should write another
> local routine which is used by both target and fast switch.
> 

Just one difference, fast switch uses polling based mailbox while
target_index uses interrupt based. I thought initially to reuse, but
it's comes done to just perf_ops->freq_set, so dropped that idea.

> Do we guarantee that the frequency is changed by the time this routine returns?

No, firmware may return acknowledging the request not it's completion.

> Or we just send a SCMI request and return back ?
> 

Yes, exactly.

> If we just send the request and don't wait for freq to get changed, what
> protects another scmi_cpufreq_fast_switch() to get called before the first one
> is finished? And what will happen on that call ?

Firmware needs to serialize or override based on the timing of the two
consecutive requests.

> 
>>  static int
>>  scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>>  {
>> @@ -164,6 +177,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>  
>>  	policy->cpuinfo.transition_latency = latency;
>>  
>> +	policy->fast_switch_possible = true;
>>  	return 0;
>>  
>>  out_free_cpufreq_table:
>> @@ -180,6 +194,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
>>  {
>>  	struct scmi_data *priv = policy->driver_data;
>>  
>> +	policy->fast_switch_possible = false;
>>  	cpufreq_cooling_unregister(priv->cdev);
>>  	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
>>  	dev_pm_opp_cpumask_remove_table(policy->related_cpus);
>> @@ -228,6 +243,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
>>  	.init			= scmi_cpufreq_init,
>>  	.exit			= scmi_cpufreq_exit,
>>  	.ready			= scmi_cpufreq_ready,
>> +	.fast_switch		= scmi_cpufreq_fast_switch,
> 
> Maybe add it right after target_index ?
> 

Done
Viresh Kumar Aug. 9, 2017, 10:13 a.m. UTC | #3
On 09-08-17, 11:09, Sudeep Holla wrote:
> Firmware needs to serialize or override based on the timing of the two
> consecutive requests.

Maybe add a comment over that routine and detail its working a bit? Like its not
synchronous, etc. I expect that you would also add a callback with SCMI, so that
the cpufreq driver is notified when frequency is changed ?
Sudeep Holla Aug. 9, 2017, 10:17 a.m. UTC | #4
On 09/08/17 11:13, Viresh Kumar wrote:
> On 09-08-17, 11:09, Sudeep Holla wrote:
>> Firmware needs to serialize or override based on the timing of the two
>> consecutive requests.
> 
> Maybe add a comment over that routine and detail its working a bit? Like its not
> synchronous, etc. I expect that you would also add a callback with SCMI, so that
> the cpufreq driver is notified when frequency is changed ?
> 

OK, will do. Yes, I don't have firmware implementing notifications yet,
so waiting until I can test it.

Patch
diff mbox

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 034359cafea5..cb1084cb1ef1 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -61,6 +61,19 @@  scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
 	return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false);
 }
 
+static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
+					     unsigned int target_freq)
+{
+	struct scmi_data *priv = policy->driver_data;
+	struct scmi_perf_ops *perf_ops = priv->handle->perf_ops;
+
+	if (!perf_ops->freq_set(priv->handle, priv->domain_id,
+				target_freq * 1000, true))
+		return target_freq;
+
+	return CPUFREQ_ENTRY_INVALID;
+}
+
 static int
 scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 {
@@ -164,6 +177,7 @@  static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = latency;
 
+	policy->fast_switch_possible = true;
 	return 0;
 
 out_free_cpufreq_table:
@@ -180,6 +194,7 @@  static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct scmi_data *priv = policy->driver_data;
 
+	policy->fast_switch_possible = false;
 	cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_cpumask_remove_table(policy->related_cpus);
@@ -228,6 +243,7 @@  static struct cpufreq_driver scmi_cpufreq_driver = {
 	.init			= scmi_cpufreq_init,
 	.exit			= scmi_cpufreq_exit,
 	.ready			= scmi_cpufreq_ready,
+	.fast_switch		= scmi_cpufreq_fast_switch,
 };
 
 static int scmi_cpufreq_probe(struct platform_device *pdev)