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@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
