Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543986 > unrolled thread

Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Chanwoo Choi <cwchoi00@gmail.com> - 2016-12-17 16:00 +0100
    Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Tobias Jakobi <tjakobi@math.uni-bielefeld.de> - 2016-12-17 16:20 +0100
      Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Chanwoo Choi <cwchoi00@gmail.com> - 2016-12-17 17:50 +0100
        Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Tobias Jakobi <tjakobi@math.uni-bielefeld.de> - 2016-12-17 19:20 +0100
          Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Chanwoo Choi <cwchoi00@gmail.com> - 2016-12-17 23:10 +0100

#1543986 — Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support

Hi Lin,

2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
> Hi Lin,
>
> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> On 2016년 11월 24일 17:34, hl wrote:
>>> Hi Chanwoo Choi,
>>>
>>>
>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>> Hi Chanwoo Choi,
>>>>>
>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>
>>>>> &dmc_opp_table {
>>>>>      opp06 {
>>>>>          opp-suspend;
>>>>>      };
>>>>> };
>>>> Two approaches use the 'opp-suspend' property.
>>>>
>>>> I think that the method to support suspend-opp have to
>>>> guarantee following conditions:
>>>> - Support the all of devfreq's governors.
>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>> which will ingore governor.
>>
>> Other approach already support the all of governors.
>> Before calling the mail, I discussed with Myungjoo Ham.
>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>
> It is not correct expression. We need to wait the reply from Myungjoo
> to clarify this.
>
>>
>> To Myungjoo,
>> Please add your opinion how to support the suspend frequency.
>
>>
>>>> - Devfreq framework have the responsibility to change the
>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>    how to support the suspend-opp. Just the developer of each
>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>
>> No, the frequency should be handled by governor or framework.
>> The each devfreq device has no any responsibility of next frequency/voltage.
>> The governor and core of devfreq can decide the next frequency/voltage.
>> You can refer to the cpufreq subsystem.
>>
>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>
>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>> It think this way is easy.
>>
>> But,
>> If the each devfreq device want to decide the next frequency/voltage only for
>> suspend state. We can check the cpufreq subsystem.
>>
>> If specific devfreq device want to handle the suspend frequency,
>> each devfreq will add the own suspend/resume functions as following:
>>
>>       struct devfreq_dev_profile {
>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>       } a_profile;
>>
>>       a_profile = devfreq_generic_suspend;
>>
>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>                       ...
>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>                       ...
>>               }
>>
>>       or
>>
>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>
>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>       instead of devfreq->profile->target();
>>
>>       The devfreq call the 'devfreq->profile->suspend()'
>>       to support the suspend frequency.
>>
>> Regards,
>> Chanwoo Choi
>
> The key difference between two approaches:
>
> Your approach:
> - The each developer should add the 'opp-suspend' property to the dts file.
> - The each devfreq should call the devfreq_suspend_device()
>   to support the suspend frequency.
>
>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>   can support the suspend frequency.
>
> Other approach:
> - The each developer only should add the 'opp-suspend' property to the dts file
>   without the additional behavior.
>
> In the cpufreq subsystem,
> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
> without the additional behavior.

I'm missing the use-case when using the devfreq_suspend_device()
before entering the suspend mode. We should consider the case when
devfreq device
calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
is exported function, each devfreq device call this function on the fly
without entering the suspend mode.

I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
So, I make the following patch. This patch set the suspend frequency
in devfreq_suspend_device() after stoping the governor.
It consider the all governors of devfreq.

What do you think?
If you are ok, I'll send this patch with your author.

 int devfreq_suspend_device(struct devfreq *devfreq)
 {
+      int ret = 0;
+
        if (!devfreq)
        return -EINVAL;

        if (!devfreq->governor)
             return 0;

-      return devfreq->governor->event_handler(devfreq,
+     ret = devfreq->governor->event_handler(devfreq,
                    DEVFREQ_GOV_SUSPEND, NULL);
+      if (ret < 0) {
+           dev_err(devfreq->dev.parent, "failed to suspend governor\n");
+           return ret;
+      }
+
+      if (devfreq->suspend_freq) {
+           ret = devfreq->profile->target(devfreq->dev.parent,
+                                      &devfreq->suspend_freq, 0);
+           if (ret < 0) {
+                  dev_err(devfreq->dev.parent,
+                             "failed to set suspend-freq\n");
+                  return ret;
+           }
+           dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
+                          devfreq->suspend_freq);
+      }
+
+      return 0;
 }
 EXPORT_SYMBOL(devfreq_suspend_device);

-- 
Best Regards,
Chanwoo Choi

[toc] | [next] | [standalone]

#1543993

Hey guys,

Chanwoo Choi wrote:
> Hi Lin,
> 
> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
>> Hi Lin,
>>
>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> On 2016년 11월 24일 17:34, hl wrote:
>>>> Hi Chanwoo Choi,
>>>>
>>>>
>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>> Hi Lin,
>>>>>
>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>> Hi Chanwoo Choi,
>>>>>>
>>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>
>>>>>> &dmc_opp_table {
>>>>>>      opp06 {
>>>>>>          opp-suspend;
>>>>>>      };
>>>>>> };
>>>>> Two approaches use the 'opp-suspend' property.
>>>>>
>>>>> I think that the method to support suspend-opp have to
>>>>> guarantee following conditions:
>>>>> - Support the all of devfreq's governors.
>>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>> which will ingore governor.
>>>
>>> Other approach already support the all of governors.
>>> Before calling the mail, I discussed with Myungjoo Ham.
>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>
>> It is not correct expression. We need to wait the reply from Myungjoo
>> to clarify this.
>>
>>>
>>> To Myungjoo,
>>> Please add your opinion how to support the suspend frequency.
>>
>>>
>>>>> - Devfreq framework have the responsibility to change the
>>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>>    how to support the suspend-opp. Just the developer of each
>>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>
>>> No, the frequency should be handled by governor or framework.
>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>> The governor and core of devfreq can decide the next frequency/voltage.
>>> You can refer to the cpufreq subsystem.
>>>
>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>
>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>> It think this way is easy.
>>>
>>> But,
>>> If the each devfreq device want to decide the next frequency/voltage only for
>>> suspend state. We can check the cpufreq subsystem.
>>>
>>> If specific devfreq device want to handle the suspend frequency,
>>> each devfreq will add the own suspend/resume functions as following:
>>>
>>>       struct devfreq_dev_profile {
>>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>>       } a_profile;
>>>
>>>       a_profile = devfreq_generic_suspend;
>>>
>>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>>                       ...
>>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>>                       ...
>>>               }
>>>
>>>       or
>>>
>>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>
>>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>       instead of devfreq->profile->target();
>>>
>>>       The devfreq call the 'devfreq->profile->suspend()'
>>>       to support the suspend frequency.
>>>
>>> Regards,
>>> Chanwoo Choi
>>
>> The key difference between two approaches:
>>
>> Your approach:
>> - The each developer should add the 'opp-suspend' property to the dts file.
>> - The each devfreq should call the devfreq_suspend_device()
>>   to support the suspend frequency.
>>
>>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>   can support the suspend frequency.
>>
>> Other approach:
>> - The each developer only should add the 'opp-suspend' property to the dts file
>>   without the additional behavior.
>>
>> In the cpufreq subsystem,
>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>> without the additional behavior.
> 
> I'm missing the use-case when using the devfreq_suspend_device()
> before entering the suspend mode. We should consider the case when
> devfreq device
> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
> is exported function, each devfreq device call this function on the fly
> without entering the suspend mode.
> 
> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
> So, I make the following patch. This patch set the suspend frequency
> in devfreq_suspend_device() after stoping the governor.
> It consider the all governors of devfreq.
> 
> What do you think?
> If you are ok, I'll send this patch with your author.
The problem I see here is that we need to keep track of the suspended
state when suspending the (entire) devfreq subsystem. When doing that,
we don't know if any device driver has previously called
devfreq_suspend_device() and might end up calling it twice.

Same thing on devfreq subsystem resume.

I've prepared a new RFC of my series (going to send it shortly), but I'm
not so happy with the current design. I think it would be much cleaner
to keep some suspend_refcount in struct devfreq so that I can call
devfreq_suspend_device() multiple times, while keeping a sane internal
state.

Something like devfreq_device_runtime_{put,get}() perhaps?

- Tobias



> 
>  int devfreq_suspend_device(struct devfreq *devfreq)
>  {
> +      int ret = 0;
> +
>         if (!devfreq)
>         return -EINVAL;
> 
>         if (!devfreq->governor)
>              return 0;
> 
> -      return devfreq->governor->event_handler(devfreq,
> +     ret = devfreq->governor->event_handler(devfreq,
>                     DEVFREQ_GOV_SUSPEND, NULL);
> +      if (ret < 0) {
> +           dev_err(devfreq->dev.parent, "failed to suspend governor\n");
> +           return ret;
> +      }
> +
> +      if (devfreq->suspend_freq) {
> +           ret = devfreq->profile->target(devfreq->dev.parent,
> +                                      &devfreq->suspend_freq, 0);
> +           if (ret < 0) {
> +                  dev_err(devfreq->dev.parent,
> +                             "failed to set suspend-freq\n");
> +                  return ret;
> +           }
> +           dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
> +                          devfreq->suspend_freq);
> +      }
> +
> +      return 0;
>  }
>  EXPORT_SYMBOL(devfreq_suspend_device);
> 

[toc] | [prev] | [next] | [standalone]

#1544010

2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hey guys,
>
> Chanwoo Choi wrote:
>> Hi Lin,
>>
>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
>>> Hi Lin,
>>>
>>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> On 2016년 11월 24일 17:34, hl wrote:
>>>>> Hi Chanwoo Choi,
>>>>>
>>>>>
>>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>>> Hi Lin,
>>>>>>
>>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>>> Hi Chanwoo Choi,
>>>>>>>
>>>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>>
>>>>>>> &dmc_opp_table {
>>>>>>>      opp06 {
>>>>>>>          opp-suspend;
>>>>>>>      };
>>>>>>> };
>>>>>> Two approaches use the 'opp-suspend' property.
>>>>>>
>>>>>> I think that the method to support suspend-opp have to
>>>>>> guarantee following conditions:
>>>>>> - Support the all of devfreq's governors.
>>>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>>> which will ingore governor.
>>>>
>>>> Other approach already support the all of governors.
>>>> Before calling the mail, I discussed with Myungjoo Ham.
>>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>>
>>> It is not correct expression. We need to wait the reply from Myungjoo
>>> to clarify this.
>>>
>>>>
>>>> To Myungjoo,
>>>> Please add your opinion how to support the suspend frequency.
>>>
>>>>
>>>>>> - Devfreq framework have the responsibility to change the
>>>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>>>    how to support the suspend-opp. Just the developer of each
>>>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>>
>>>> No, the frequency should be handled by governor or framework.
>>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>>> The governor and core of devfreq can decide the next frequency/voltage.
>>>> You can refer to the cpufreq subsystem.
>>>>
>>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>>
>>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>>> It think this way is easy.
>>>>
>>>> But,
>>>> If the each devfreq device want to decide the next frequency/voltage only for
>>>> suspend state. We can check the cpufreq subsystem.
>>>>
>>>> If specific devfreq device want to handle the suspend frequency,
>>>> each devfreq will add the own suspend/resume functions as following:
>>>>
>>>>       struct devfreq_dev_profile {
>>>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>>>       } a_profile;
>>>>
>>>>       a_profile = devfreq_generic_suspend;
>>>>
>>>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>>>                       ...
>>>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>>>                       ...
>>>>               }
>>>>
>>>>       or
>>>>
>>>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>>
>>>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>>       instead of devfreq->profile->target();
>>>>
>>>>       The devfreq call the 'devfreq->profile->suspend()'
>>>>       to support the suspend frequency.
>>>>
>>>> Regards,
>>>> Chanwoo Choi
>>>
>>> The key difference between two approaches:
>>>
>>> Your approach:
>>> - The each developer should add the 'opp-suspend' property to the dts file.
>>> - The each devfreq should call the devfreq_suspend_device()
>>>   to support the suspend frequency.
>>>
>>>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>>   can support the suspend frequency.
>>>
>>> Other approach:
>>> - The each developer only should add the 'opp-suspend' property to the dts file
>>>   without the additional behavior.
>>>
>>> In the cpufreq subsystem,
>>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>>> without the additional behavior.
>>
>> I'm missing the use-case when using the devfreq_suspend_device()
>> before entering the suspend mode. We should consider the case when
>> devfreq device
>> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
>> is exported function, each devfreq device call this function on the fly
>> without entering the suspend mode.
>>
>> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
>> So, I make the following patch. This patch set the suspend frequency
>> in devfreq_suspend_device() after stoping the governor.
>> It consider the all governors of devfreq.
>>
>> What do you think?
>> If you are ok, I'll send this patch with your author.
> The problem I see here is that we need to keep track of the suspended
> state when suspending the (entire) devfreq subsystem. When doing that,
> we don't know if any device driver has previously called
> devfreq_suspend_device() and might end up calling it twice.
>
> Same thing on devfreq subsystem resume.
>
> I've prepared a new RFC of my series (going to send it shortly), but I'm
> not so happy with the current design. I think it would be much cleaner
> to keep some suspend_refcount in struct devfreq so that I can call
> devfreq_suspend_device() multiple times, while keeping a sane internal
> state.

I agree the devfreq need  reference count for devfreq_suspend/resume_device.
This patch focus on when changing the suspend frequency.

>
> Something like devfreq_device_runtime_{put,get}() perhaps?

Why do devfreq need new additional functions?
I think the devfreq_suspend/resume_device are enough.

Thanks,
Chanwoo Choi

>
> - Tobias
>
>
>
>>
>>  int devfreq_suspend_device(struct devfreq *devfreq)
>>  {
>> +      int ret = 0;
>> +
>>         if (!devfreq)
>>         return -EINVAL;
>>
>>         if (!devfreq->governor)
>>              return 0;
>>
>> -      return devfreq->governor->event_handler(devfreq,
>> +     ret = devfreq->governor->event_handler(devfreq,
>>                     DEVFREQ_GOV_SUSPEND, NULL);
>> +      if (ret < 0) {
>> +           dev_err(devfreq->dev.parent, "failed to suspend governor\n");
>> +           return ret;
>> +      }
>> +
>> +      if (devfreq->suspend_freq) {
>> +           ret = devfreq->profile->target(devfreq->dev.parent,
>> +                                      &devfreq->suspend_freq, 0);
>> +           if (ret < 0) {
>> +                  dev_err(devfreq->dev.parent,
>> +                             "failed to set suspend-freq\n");
>> +                  return ret;
>> +           }
>> +           dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
>> +                          devfreq->suspend_freq);
>> +      }
>> +
>> +      return 0;
>>  }
>>  EXPORT_SYMBOL(devfreq_suspend_device);
>>
>

[toc] | [prev] | [next] | [standalone]

#1544027

Hey Chanwoo,


Chanwoo Choi wrote:
> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
>> Hey guys,
>>
>> Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
>>>> Hi Lin,
>>>>
>>>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>>>> Hi Lin,
>>>>>
>>>>> On 2016년 11월 24일 17:34, hl wrote:
>>>>>> Hi Chanwoo Choi,
>>>>>>
>>>>>>
>>>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>>>> Hi Lin,
>>>>>>>
>>>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>>>> Hi Chanwoo Choi,
>>>>>>>>
>>>>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>>>
>>>>>>>> &dmc_opp_table {
>>>>>>>>      opp06 {
>>>>>>>>          opp-suspend;
>>>>>>>>      };
>>>>>>>> };
>>>>>>> Two approaches use the 'opp-suspend' property.
>>>>>>>
>>>>>>> I think that the method to support suspend-opp have to
>>>>>>> guarantee following conditions:
>>>>>>> - Support the all of devfreq's governors.
>>>>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>>>> which will ingore governor.
>>>>>
>>>>> Other approach already support the all of governors.
>>>>> Before calling the mail, I discussed with Myungjoo Ham.
>>>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>>>
>>>> It is not correct expression. We need to wait the reply from Myungjoo
>>>> to clarify this.
>>>>
>>>>>
>>>>> To Myungjoo,
>>>>> Please add your opinion how to support the suspend frequency.
>>>>
>>>>>
>>>>>>> - Devfreq framework have the responsibility to change the
>>>>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>>>>    how to support the suspend-opp. Just the developer of each
>>>>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>>>
>>>>> No, the frequency should be handled by governor or framework.
>>>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>>>> The governor and core of devfreq can decide the next frequency/voltage.
>>>>> You can refer to the cpufreq subsystem.
>>>>>
>>>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>>>
>>>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>>>> It think this way is easy.
>>>>>
>>>>> But,
>>>>> If the each devfreq device want to decide the next frequency/voltage only for
>>>>> suspend state. We can check the cpufreq subsystem.
>>>>>
>>>>> If specific devfreq device want to handle the suspend frequency,
>>>>> each devfreq will add the own suspend/resume functions as following:
>>>>>
>>>>>       struct devfreq_dev_profile {
>>>>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>>>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>>>>       } a_profile;
>>>>>
>>>>>       a_profile = devfreq_generic_suspend;
>>>>>
>>>>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>>>>                       ...
>>>>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>>>>                       ...
>>>>>               }
>>>>>
>>>>>       or
>>>>>
>>>>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>>>
>>>>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>>>       instead of devfreq->profile->target();
>>>>>
>>>>>       The devfreq call the 'devfreq->profile->suspend()'
>>>>>       to support the suspend frequency.
>>>>>
>>>>> Regards,
>>>>> Chanwoo Choi
>>>>
>>>> The key difference between two approaches:
>>>>
>>>> Your approach:
>>>> - The each developer should add the 'opp-suspend' property to the dts file.
>>>> - The each devfreq should call the devfreq_suspend_device()
>>>>   to support the suspend frequency.
>>>>
>>>>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>>>   can support the suspend frequency.
>>>>
>>>> Other approach:
>>>> - The each developer only should add the 'opp-suspend' property to the dts file
>>>>   without the additional behavior.
>>>>
>>>> In the cpufreq subsystem,
>>>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>>>> without the additional behavior.
>>>
>>> I'm missing the use-case when using the devfreq_suspend_device()
>>> before entering the suspend mode. We should consider the case when
>>> devfreq device
>>> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
>>> is exported function, each devfreq device call this function on the fly
>>> without entering the suspend mode.
>>>
>>> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
>>> So, I make the following patch. This patch set the suspend frequency
>>> in devfreq_suspend_device() after stoping the governor.
>>> It consider the all governors of devfreq.
>>>
>>> What do you think?
>>> If you are ok, I'll send this patch with your author.
>> The problem I see here is that we need to keep track of the suspended
>> state when suspending the (entire) devfreq subsystem. When doing that,
>> we don't know if any device driver has previously called
>> devfreq_suspend_device() and might end up calling it twice.
>>
>> Same thing on devfreq subsystem resume.
>>
>> I've prepared a new RFC of my series (going to send it shortly), but I'm
>> not so happy with the current design. I think it would be much cleaner
>> to keep some suspend_refcount in struct devfreq so that I can call
>> devfreq_suspend_device() multiple times, while keeping a sane internal
>> state.
> 
> I agree the devfreq need  reference count for devfreq_suspend/resume_device.
> This patch focus on when changing the suspend frequency.
> 
>>
>> Something like devfreq_device_runtime_{put,get}() perhaps?
> 
> Why do devfreq need new additional functions?
> I think the devfreq_suspend/resume_device are enough.
Just thinking out loud here. I would prefer a naming that implies that
some refcounting is going on. When I see a pair of function with
put/get, then I usually know what is going on.

Here I would have to look at the actual implementation to realize, at
the moment, that I have to be careful calling these functions twice.

-Tobias


> 
> Thanks,
> Chanwoo Choi
> 
>>
>> - Tobias
>>
>>
>>
>>>
>>>  int devfreq_suspend_device(struct devfreq *devfreq)
>>>  {
>>> +      int ret = 0;
>>> +
>>>         if (!devfreq)
>>>         return -EINVAL;
>>>
>>>         if (!devfreq->governor)
>>>              return 0;
>>>
>>> -      return devfreq->governor->event_handler(devfreq,
>>> +     ret = devfreq->governor->event_handler(devfreq,
>>>                     DEVFREQ_GOV_SUSPEND, NULL);
>>> +      if (ret < 0) {
>>> +           dev_err(devfreq->dev.parent, "failed to suspend governor\n");
>>> +           return ret;
>>> +      }
>>> +
>>> +      if (devfreq->suspend_freq) {
>>> +           ret = devfreq->profile->target(devfreq->dev.parent,
>>> +                                      &devfreq->suspend_freq, 0);
>>> +           if (ret < 0) {
>>> +                  dev_err(devfreq->dev.parent,
>>> +                             "failed to set suspend-freq\n");
>>> +                  return ret;
>>> +           }
>>> +           dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
>>> +                          devfreq->suspend_freq);
>>> +      }
>>> +
>>> +      return 0;
>>>  }
>>>  EXPORT_SYMBOL(devfreq_suspend_device);
>>>
>>

[toc] | [prev] | [next] | [standalone]

#1544048

2016-12-18 3:19 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
>>> Hey guys,
>>>
>>> Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
>>>>> Hi Lin,
>>>>>
>>>>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>>>>> Hi Lin,
>>>>>>
>>>>>> On 2016년 11월 24일 17:34, hl wrote:
>>>>>>> Hi Chanwoo Choi,
>>>>>>>
>>>>>>>
>>>>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>>>>> Hi Lin,
>>>>>>>>
>>>>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>>>>> Hi Chanwoo Choi,
>>>>>>>>>
>>>>>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>>>>
>>>>>>>>> &dmc_opp_table {
>>>>>>>>>      opp06 {
>>>>>>>>>          opp-suspend;
>>>>>>>>>      };
>>>>>>>>> };
>>>>>>>> Two approaches use the 'opp-suspend' property.
>>>>>>>>
>>>>>>>> I think that the method to support suspend-opp have to
>>>>>>>> guarantee following conditions:
>>>>>>>> - Support the all of devfreq's governors.
>>>>>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>>>>> which will ingore governor.
>>>>>>
>>>>>> Other approach already support the all of governors.
>>>>>> Before calling the mail, I discussed with Myungjoo Ham.
>>>>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>>>>
>>>>> It is not correct expression. We need to wait the reply from Myungjoo
>>>>> to clarify this.
>>>>>
>>>>>>
>>>>>> To Myungjoo,
>>>>>> Please add your opinion how to support the suspend frequency.
>>>>>
>>>>>>
>>>>>>>> - Devfreq framework have the responsibility to change the
>>>>>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>>>>>    how to support the suspend-opp. Just the developer of each
>>>>>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>>>>
>>>>>> No, the frequency should be handled by governor or framework.
>>>>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>>>>> The governor and core of devfreq can decide the next frequency/voltage.
>>>>>> You can refer to the cpufreq subsystem.
>>>>>>
>>>>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>>>>
>>>>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>>>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>>>>> It think this way is easy.
>>>>>>
>>>>>> But,
>>>>>> If the each devfreq device want to decide the next frequency/voltage only for
>>>>>> suspend state. We can check the cpufreq subsystem.
>>>>>>
>>>>>> If specific devfreq device want to handle the suspend frequency,
>>>>>> each devfreq will add the own suspend/resume functions as following:
>>>>>>
>>>>>>       struct devfreq_dev_profile {
>>>>>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>>>>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>>>>>       } a_profile;
>>>>>>
>>>>>>       a_profile = devfreq_generic_suspend;
>>>>>>
>>>>>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>>>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>>>>>                       ...
>>>>>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>>>>>                       ...
>>>>>>               }
>>>>>>
>>>>>>       or
>>>>>>
>>>>>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>>>>
>>>>>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>>>>       instead of devfreq->profile->target();
>>>>>>
>>>>>>       The devfreq call the 'devfreq->profile->suspend()'
>>>>>>       to support the suspend frequency.
>>>>>>
>>>>>> Regards,
>>>>>> Chanwoo Choi
>>>>>
>>>>> The key difference between two approaches:
>>>>>
>>>>> Your approach:
>>>>> - The each developer should add the 'opp-suspend' property to the dts file.
>>>>> - The each devfreq should call the devfreq_suspend_device()
>>>>>   to support the suspend frequency.
>>>>>
>>>>>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>>>>   can support the suspend frequency.
>>>>>
>>>>> Other approach:
>>>>> - The each developer only should add the 'opp-suspend' property to the dts file
>>>>>   without the additional behavior.
>>>>>
>>>>> In the cpufreq subsystem,
>>>>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>>>>> without the additional behavior.
>>>>
>>>> I'm missing the use-case when using the devfreq_suspend_device()
>>>> before entering the suspend mode. We should consider the case when
>>>> devfreq device
>>>> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
>>>> is exported function, each devfreq device call this function on the fly
>>>> without entering the suspend mode.
>>>>
>>>> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
>>>> So, I make the following patch. This patch set the suspend frequency
>>>> in devfreq_suspend_device() after stoping the governor.
>>>> It consider the all governors of devfreq.
>>>>
>>>> What do you think?
>>>> If you are ok, I'll send this patch with your author.
>>> The problem I see here is that we need to keep track of the suspended
>>> state when suspending the (entire) devfreq subsystem. When doing that,
>>> we don't know if any device driver has previously called
>>> devfreq_suspend_device() and might end up calling it twice.
>>>
>>> Same thing on devfreq subsystem resume.
>>>
>>> I've prepared a new RFC of my series (going to send it shortly), but I'm
>>> not so happy with the current design. I think it would be much cleaner
>>> to keep some suspend_refcount in struct devfreq so that I can call
>>> devfreq_suspend_device() multiple times, while keeping a sane internal
>>> state.
>>
>> I agree the devfreq need  reference count for devfreq_suspend/resume_device.
>> This patch focus on when changing the suspend frequency.
>>
>>>
>>> Something like devfreq_device_runtime_{put,get}() perhaps?
>>
>> Why do devfreq need new additional functions?
>> I think the devfreq_suspend/resume_device are enough.
> Just thinking out loud here. I would prefer a naming that implies that
> some refcounting is going on. When I see a pair of function with
> put/get, then I usually know what is going on.

The suspend/resume name are already used as pair function name.
I think that devfreq_suspend/resume_device() are appropriate.

Usually, '_runtime_put/get' naming means the 'runtime PM' callback function
which handle the all of resource (e.g., clock, regulator, register and so on).

>
> Here I would have to look at the actual implementation to realize, at
> the moment, that I have to be careful calling these functions twice.

Sure. I'm waiting your patch.

[snip]

Thanks,
Chanwoo Choi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web