Re: [PATCH v5 20/20] cpufreq: scmi: add support for fast frequency switching

[Sudeep Holla]

On 04/01/18 22:10, Alexey Klimov wrote:
> Hi Sudeep,
> 
> On Tue, Jan 2, 2018 at 2:42 PM, Sudeep Holla <sudeep.holla@xxxxxxx> 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: linux-pm@xxxxxxxxxxxxxxx
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>> ---
>>  drivers/cpufreq/scmi-cpufreq.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index 0ee9335d0063..d0a82d7c6fd4 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -64,6 +64,19 @@ scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
>>         return perf_ops->freq_set(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 = handle->perf_ops;
>> +
>> +       if (!perf_ops->freq_set(handle, priv->domain_id,
>> +                               target_freq * 1000, true))
>> +               return target_freq;
>> +
>> +       return 0;
>> +}
> 
> Could you please explain how it's supposed to work for purpose of fast
> frequency switching?
> 
> I am trying to track down ->freq_set.
> So it looks like this will fire an scmi perf level set command and
> will poll for this command to complete without asking for firmware to
> send command completion irq.
> 
> scmi_perf_level_set() will call the following functions:
> 
> scmi_one_xfer_init();
> scmi_do_xfer(handle, t);
> scmi_one_xfer_put(handle, t);
> 
> 
> The first function in the list calls scmi_one_xfer_get() which has
> this in the description (I guess because of down_timeout()):
> "This function can sleep depending on pending requests already in the system
> for the SCMI entity. Further, this also holds a spinlock to maintain
> integrity of internal data structures."
> 
> So it can sleep.
> 

Indeed, I can drop the whole semaphore story and expect the caller to
retry in case buffer is full which is very rare condition.

> As far as I see description of fast frequency switching it's required
> for fast_switch to not sleep:
> (file Documentation/cpu-freq/cpu-drivers.txt)
> 
> "This function is used for frequency switching from scheduler's context.
> Not all drivers are expected to implement it, as sleeping from within
> this callback isn't allowed. This callback must be highly optimized to
> do switching as fast as possible."
> 
> 
> The other questions to this implementation of fast switching:
> 
> 1) Fast switching callback must be highly optimized. Is it now? I see
> few spinlocks (in scmi mbox client and in the mailbox framework) there
> and polling functionality with udelay(5) inside that will timeout (if
> my calculations are correct) after 0.5 ms.

Do you have any alternate ideas to avoid that and still achieve fast
switching ?

> 2) Is it highly dependent on transport? If mailbox transport
> ->send_data() may sleep or hrtimer-based polling in mailbox framework
> will be used, then this fast switch won't work, right?
> 

Yes.

> I am still looking into that: I can be wrong and just trying to
> understand if it is all okay.
> 

Thanks for taking a look at this.

-- 
Regards,
Sudeep