Re: [RFC 1/4] PM / devfreq: Add DevFreq subsystem suspend/resume

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

Hey Chanwoo,

first of all, thanks for the help!


Chanwoo Choi wrote:
> Hi Tobias,
> 
> On 2016년 11월 24일 10:34, Chanwoo Choi wrote:
>> Hi Tobias,
>>
>> I like your suggestion. But I have some comment on below.
>>
>> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>>> This suspend/resume operations works analogous to the
>>> cpufreq_{suspend,resume}() calls in the CPUFreq subsystem.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/devfreq.h   | 10 +++++++
>>>  2 files changed, 85 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index bf3ea76..2f1aa83 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -785,6 +785,81 @@ int devfreq_resume_device(struct devfreq *devfreq)
>>>  EXPORT_SYMBOL(devfreq_resume_device);
>>>  
>>>  /**
>>> + * devfreq_suspend() - Suspend DevFreq governors
>>> + *
>>> + * Called during system wide Suspend/Hibernate cycles for suspending governors
>>> + * in the same fashion as cpufreq_suspend().
>>> + */
>>> +void devfreq_suspend(void)
>>> +{
>>> +	struct devfreq *devfreq;
>>> +	unsigned long freq;
>>> +	int ret;
>>> +
>>> +	mutex_lock(&devfreq_list_lock);
>>> +
>>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>>> +		if (!devfreq->suspend_freq)
>>> +			continue;
>>> +
>>> +		ret = devfreq_suspend_device(devfreq);
> 
> The devfreq_suspend_device() stop the monitoring of governors.
> But, this function is exported. It means that each devfreq device.
> can call the devfreq_suspend/resume_device() (e.g., int drivers/scsi/ufs/ufshcd.c)
> 
> So, you should consider following case:
> - case :Before calling the devfreq_suspend() and specific devfreq already call
>   the devfreq_suspend_device(), how to support it?
>   In this case, the specific devfreq device don't want to call
>   the devfreq_resume_device() in the devfreq_resume().
How about this idea?

Introduce a boolean 'devfreq_suspended' (again similar to CPUFreq) and
set it to true (atomically?) once we have entered devfreq_suspend().

At the end of devfreq_suspend() we set it to false again.

We just need to check in devfreq_{suspend,resume}_device() for
'devfreq_suspended' and return some error code (-EBUSY maybe?) to the
caller.

Of course I would inline the devfreq->governor->event_handler() call
into devfreq_{suspend,resume}() first.

Does this look sane?

Also, do you see any other exported calls which we might have to
'protect' during suspended state?


With best wishes,
Tobias


>>> +		if (ret < 0) {
>>> +			dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
>>> +			continue;
>>> +		}
>>> +
>>> +		devfreq->resume_freq = 0;
>>> +		if (devfreq->profile->get_cur_freq) {
>>> +			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
>>> +			if (ret >= 0)
>>> +				devfreq->resume_freq = freq;
>>> +		}
>>> +
>>> +		freq = devfreq->suspend_freq;
>>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
>>
>>
>> DEVFREQ subsystem has the passive governor[2] and devfreq device using passive[1]
>> governor depends on the parent devfreq device. The passive governor uses
>> 'DEVFREQ_TRANSITION_NOTIFIER' notifier to track the changed state of
>> parent devfreq device.
>>
>> When changing the clock/voltage of parent devfreq device,
>> DEVFREQ subsystem have to notify the changed frequency
>> to the passive devfreq device.
>>
>> So, you should call the devfreq_notifiy_transition()
>> before/after 'devfreq->profile->target' as following:
>> You can refer the update_devfreq() function 
>> that is how to use devfreq_notifiy_transition().
>>
>> For example,
>> 		struct devfreq_freq freqs;
>>  
>> 		if (devfreq->profile->get_cur_freq) {
>> 			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
>> 			if (ret >= 0)
>> 				devfreq->resume_freq = freq;
>> 		} else {
>> 			devfreq->resume_freq = devfreq_previous_freq;
>> 		} 
>>
>> 		freqs.old = devfreq->resume_freq;
>> 		freqs.new = devfreq->suspend_freq;
>> 		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
>>
>>                 freq = devfreq->suspend_freq;
>>                 ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
>>  
>> 		freqs.new = freq;
>> 		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>
>>                 if (ret < 0)
>>                         dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>>         }
>>
>> [1] DEVFREQ_TRANSITION_NOTIFIER notifier
>> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0fe3a66410a3ba96679be903f1e287d7a0a264a9
>> [2] DEVFREQ passive governor
>> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=996133119f57334c38b020dbfaaac5b5eb127e29
>>
>>> +
>>> +		if (ret < 0)
>>> +			dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>>> +	}
>>> +
>>> +	mutex_unlock(&devfreq_list_lock);
>>> +}
>>> +
>>> +/**
>>> + * devfreq_resume() - Resume DevFreq governors
>>> + *
>>> + * Called during system wide Suspend/Hibernate cycle for resuming governors that
>>> + * are suspended with devfreq_suspend().
>>> + */
>>> +void devfreq_resume(void)
>>> +{
>>> +	struct devfreq *devfreq;
>>> +	unsigned long freq;
>>> +	int ret;
>>> +
>>> +	mutex_lock(&devfreq_list_lock);
>>> +
>>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>>> +		if (!devfreq->suspend_freq)
>>> +			continue;
>>> +
>>> +		freq = devfreq->resume_freq;
>>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
>>
>> ditto.
>>
>>> +
>>> +		if (ret < 0) {
>>> +			dev_warn(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
>>> +			continue;
>>> +		}
>>> +
>>> +		ret = devfreq_resume_device(devfreq);
>>> +		if (ret < 0)
>>> +			dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
>>> +	}
>>> +
>>> +	mutex_unlock(&devfreq_list_lock);
>>> +}
>>> +
>>> +/**
>>>   * devfreq_add_governor() - Add devfreq governor
>>>   * @governor:	the devfreq governor to be added
>>>   */
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index 2de4e2e..555d024 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -171,6 +171,9 @@ struct devfreq {
>>>  	struct notifier_block nb;
>>>  	struct delayed_work work;
>>>  
>>> +	unsigned long suspend_freq; /* freq during devfreq suspend */
>>> +	unsigned long resume_freq; /* freq restored after suspend cycle */
>>> +
>>>  	unsigned long previous_freq;
>>>  	struct devfreq_dev_status last_status;
>>>  
>>> @@ -211,6 +214,10 @@ extern void devm_devfreq_remove_device(struct device *dev,
>>>  extern int devfreq_suspend_device(struct devfreq *devfreq);
>>>  extern int devfreq_resume_device(struct devfreq *devfreq);
>>>  
>>> +/* Suspend/resume the entire Devfreq subsystem. */
>>> +void devfreq_suspend(void);
>>> +void devfreq_resume(void);
>>> +
>>>  /* Helper functions for devfreq user device driver with OPP. */
>>>  extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>>  					   unsigned long *freq, u32 flags);
>>> @@ -410,6 +417,9 @@ static inline int devfreq_update_stats(struct devfreq *df)
>>>  {
>>>  	return -EINVAL;
>>>  }
>>> +
>>> +static inline void devfreq_suspend(void) {}
>>> +static inline void devfreq_resume(void) {}
>>>  #endif /* CONFIG_PM_DEVFREQ */
>>>  
>>>  #endif /* __LINUX_DEVFREQ_H__ */
>>>
>>
>>
> Best Regards,
> Chanwoo Choi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>