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

[Chanwoo Choi]

2016-12-18 3:19 GMT+09:00 Tobias Jakobi :
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
>>> Hey guys,
>>>
>>> Chanwoo Choi wrote:
 Hi Lin,

 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
> 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:
>
> _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 

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

[Chanwoo Choi]

2016-12-18 3:19 GMT+09:00 Tobias Jakobi :
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
>>> Hey guys,
>>>
>>> Chanwoo Choi wrote:
 Hi Lin,

 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
> 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:
>
> _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 

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

[Tobias Jakobi]

Hey Chanwoo,


Chanwoo Choi wrote:
> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
>> Hey guys,
>>
>> Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
 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:

 _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 

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

[Tobias Jakobi]

Hey Chanwoo,


Chanwoo Choi wrote:
> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
>> Hey guys,
>>
>> Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
 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:

 _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 

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

[Chanwoo Choi]

2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
> Hey guys,
>
> Chanwoo Choi wrote:
>> Hi Lin,
>>
>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
>>> 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:
>>>
>>> _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 

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

[Chanwoo Choi]

2016-12-18 0:13 GMT+09:00 Tobias Jakobi :
> Hey guys,
>
> Chanwoo Choi wrote:
>> Hi Lin,
>>
>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
>>> 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:
>>>
>>> _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 

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

[Tobias Jakobi]

Hey guys,

Chanwoo Choi wrote:
> Hi Lin,
> 
> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
>> 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:
>>
>> _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 

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

[Tobias Jakobi]

Hey guys,

Chanwoo Choi wrote:
> Hi Lin,
> 
> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
>> 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:
>>
>> _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 

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

[Chanwoo Choi]

Hi Lin,

2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
> 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:
>
> _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 

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

[Chanwoo Choi]

Hi Lin,

2016-11-24 18:54 GMT+09:00 Chanwoo Choi :
> 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:
>
> _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 

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

[Chanwoo Choi]

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:

 _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.

Regards,
Chanwoo Choi

> 
>>> Best Regards,
>>> Chanwoo Choi
>>>
 so i think my way semm more simple.

 On 2016年11月24日 15:10, Chanwoo Choi wrote:
> + Tobias Jakobi,
>
> Hi Lin,
>
> We need to discuss how to support the suspend-opp of devfreq device.
> Now, there are two patch thread for suspend-opp of devfreq.
>
> The Lin's approach modify the devfreq_suspend_device() to support 
> suspend-opp.
> The Tobias's approach[1] add new devfreq_suspend() and then call it on 
> dpm_suspend()
> when entering the suspend state.
>
> [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
> - https://patchwork.kernel.org/patch/9443323/
> - https://patchwork.kernel.org/patch/9443325/
> - https://patchwork.kernel.org/patch/9443329/
> - https://patchwork.kernel.org/patch/9443331/
>

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

[Chanwoo Choi]

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:

 _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.

Regards,
Chanwoo Choi

> 
>>> Best Regards,
>>> Chanwoo Choi
>>>
 so i think my way semm more simple.

 On 2016年11月24日 15:10, Chanwoo Choi wrote:
> + Tobias Jakobi,
>
> Hi Lin,
>
> We need to discuss how to support the suspend-opp of devfreq device.
> Now, there are two patch thread for suspend-opp of devfreq.
>
> The Lin's approach modify the devfreq_suspend_device() to support 
> suspend-opp.
> The Tobias's approach[1] add new devfreq_suspend() and then call it on 
> dpm_suspend()
> when entering the suspend state.
>
> [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
> - https://patchwork.kernel.org/patch/9443323/
> - https://patchwork.kernel.org/patch/9443325/
> - https://patchwork.kernel.org/patch/9443329/
> - https://patchwork.kernel.org/patch/9443331/
>

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

[Chanwoo Choi]

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:
>>>
>>> _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().

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

>> Best Regards,
>> Chanwoo Choi
>>
>>> so i think my way semm more simple.
>>>
>>> On 2016年11月24日 15:10, Chanwoo Choi wrote:
 + Tobias Jakobi,

 Hi Lin,

 We need to discuss how to support the suspend-opp of devfreq device.
 Now, there are two patch thread for suspend-opp of devfreq.

 The Lin's approach modify the devfreq_suspend_device() to support 
 suspend-opp.
 The Tobias's approach[1] add new devfreq_suspend() and then call it on 
 dpm_suspend()
 when entering the suspend state.

 [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
 - https://patchwork.kernel.org/patch/9443323/
 - https://patchwork.kernel.org/patch/9443325/
 - https://patchwork.kernel.org/patch/9443329/
 - https://patchwork.kernel.org/patch/9443331/

 I think we need to discuss it together.

 Regards,
 Chanwoo Choi

 On 2016년 11월 24일 15:45, hl wrote:
> Hi MyungJoo Ham,
>
> On 2016年11月24日 14:14, MyungJoo Ham wrote:
>> On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
>>> Hi MyungJoo Ham,
>> []
 We still need to sync the all status even i call target() in
 devfreq_suspend/resume_device
 directly, so still need update_devfreq() other setp except
 devfreq->governor->get_target_freq(devfreq, );
>>> And i think it better to be governor behaviors, for userspace they may 
>>> not
>>> want to change
>>> the suspend frequency like other governor, the frequency should decide 
>>> by
>>> the user, if they
>>> want this function, they should like other governor to rigister a
>>> devfreq_monitor_suspend().
>>> What do you think about my rev6 patch?
>> 

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

[Chanwoo Choi]

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:
>>>
>>> _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().

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

>> Best Regards,
>> Chanwoo Choi
>>
>>> so i think my way semm more simple.
>>>
>>> On 2016年11月24日 15:10, Chanwoo Choi wrote:
 + Tobias Jakobi,

 Hi Lin,

 We need to discuss how to support the suspend-opp of devfreq device.
 Now, there are two patch thread for suspend-opp of devfreq.

 The Lin's approach modify the devfreq_suspend_device() to support 
 suspend-opp.
 The Tobias's approach[1] add new devfreq_suspend() and then call it on 
 dpm_suspend()
 when entering the suspend state.

 [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
 - https://patchwork.kernel.org/patch/9443323/
 - https://patchwork.kernel.org/patch/9443325/
 - https://patchwork.kernel.org/patch/9443329/
 - https://patchwork.kernel.org/patch/9443331/

 I think we need to discuss it together.

 Regards,
 Chanwoo Choi

 On 2016년 11월 24일 15:45, hl wrote:
> Hi MyungJoo Ham,
>
> On 2016年11月24日 14:14, MyungJoo Ham wrote:
>> On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
>>> Hi MyungJoo Ham,
>> []
 We still need to sync the all status even i call target() in
 devfreq_suspend/resume_device
 directly, so still need update_devfreq() other setp except
 devfreq->governor->get_target_freq(devfreq, );
>>> And i think it better to be governor behaviors, for userspace they may 
>>> not
>>> want to change
>>> the suspend frequency like other governor, the frequency should decide 
>>> by
>>> the user, if they
>>> want this function, they should like other governor to rigister a
>>> devfreq_monitor_suspend().
>>> What do you think about my rev6 patch?
>> If I understand the 

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

[hl]

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:

_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.

- 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
specific driver, i think the voltage should handle in the 
devfreq->profile->target();

Best Regards,
Chanwoo Choi


so i think my way semm more simple.

On 2016年11月24日 15:10, Chanwoo Choi wrote:

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:

Hi MyungJoo Ham,

On 2016年11月24日 14:14, MyungJoo Ham wrote:

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:

Hi MyungJoo Ham,

[]

We still need to sync the all status even i call target() in
devfreq_suspend/resume_device
directly, so still need update_devfreq() other setp except
devfreq->governor->get_target_freq(devfreq, );

And i think it better to be governor behaviors, for userspace they may not
want to change
the suspend frequency like other governor, the frequency should decide by
the user, if they
want this function, they should like other governor to rigister a
devfreq_monitor_suspend().
What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
  /* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
 return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.


Thanks for your suggestion, i will update the new version soon.

Cheers,
MyungJoo




___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

--
Lin Huang










--
Lin Huang

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

[hl]

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:

_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.

- 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
specific driver, i think the voltage should handle in the 
devfreq->profile->target();

Best Regards,
Chanwoo Choi


so i think my way semm more simple.

On 2016年11月24日 15:10, Chanwoo Choi wrote:

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:

Hi MyungJoo Ham,

On 2016年11月24日 14:14, MyungJoo Ham wrote:

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:

Hi MyungJoo Ham,

[]

We still need to sync the all status even i call target() in
devfreq_suspend/resume_device
directly, so still need update_devfreq() other setp except
devfreq->governor->get_target_freq(devfreq, );

And i think it better to be governor behaviors, for userspace they may not
want to change
the suspend frequency like other governor, the frequency should decide by
the user, if they
want this function, they should like other governor to rigister a
devfreq_monitor_suspend().
What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
  /* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
 return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.


Thanks for your suggestion, i will update the new version soon.

Cheers,
MyungJoo




___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

--
Lin Huang










--
Lin Huang

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

[Chanwoo Choi]

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:
> 
> _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.
- 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.

Best Regards,
Chanwoo Choi

> 
> so i think my way semm more simple.
> 
> On 2016年11月24日 15:10, Chanwoo Choi wrote:
>> + Tobias Jakobi,
>>
>> Hi Lin,
>>
>> We need to discuss how to support the suspend-opp of devfreq device.
>> Now, there are two patch thread for suspend-opp of devfreq.
>>
>> The Lin's approach modify the devfreq_suspend_device() to support 
>> suspend-opp.
>> The Tobias's approach[1] add new devfreq_suspend() and then call it on 
>> dpm_suspend()
>> when entering the suspend state.
>>
>> [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
>> - https://patchwork.kernel.org/patch/9443323/
>> - https://patchwork.kernel.org/patch/9443325/
>> - https://patchwork.kernel.org/patch/9443329/
>> - https://patchwork.kernel.org/patch/9443331/
>>
>> I think we need to discuss it together.
>>
>> Regards,
>> Chanwoo Choi
>>
>> On 2016년 11월 24일 15:45, hl wrote:
>>> Hi MyungJoo Ham,
>>>
>>> On 2016年11月24日 14:14, MyungJoo Ham wrote:
 On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
> Hi MyungJoo Ham,
 []
>> We still need to sync the all status even i call target() in
>> devfreq_suspend/resume_device
>> directly, so still need update_devfreq() other setp except
>> devfreq->governor->get_target_freq(devfreq, );
> And i think it better to be governor behaviors, for userspace they may not
> want to change
> the suspend frequency like other governor, the frequency should decide by
> the user, if they
> want this function, they should like other governor to rigister a
> devfreq_monitor_suspend().
> What do you think about my rev6 patch?
 If I understand the intention correctly, this is for the stability of
 the device due to the behavior or bootloader/SoC-initializer, which
 has nothing to do with governors.

 Even if users are using userspace, as long as they set the custom
 frequencies lower than the default, they have the possibility of
 being unstable as ondemand is going to have.


 To reuse the update_devfreq() code, you may do something like:

 static int _update_freq(struct devfreq *devfreq, bool is_suspending)
 {
  /* original contents of update_freq with if statement with 
 is_suspending wrapping get_target_freq */
 }
 int update_freq(struct devfreq *devfreq)
 {
 return _update_freq(devfreq, false);
 }


 There should be other good non-invasive methods that are not 
 governoe-specific as well.

>>> Thanks for your suggestion, i will update the new version soon.
 Cheers,
 MyungJoo




 ___
 Linux-rockchip mailing list
 linux-rockc...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>> -- 
>>> Lin Huang
>>>
>>
>>
>>
> 

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

[Chanwoo Choi]

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:
> 
> _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.
- 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.

Best Regards,
Chanwoo Choi

> 
> so i think my way semm more simple.
> 
> On 2016年11月24日 15:10, Chanwoo Choi wrote:
>> + Tobias Jakobi,
>>
>> Hi Lin,
>>
>> We need to discuss how to support the suspend-opp of devfreq device.
>> Now, there are two patch thread for suspend-opp of devfreq.
>>
>> The Lin's approach modify the devfreq_suspend_device() to support 
>> suspend-opp.
>> The Tobias's approach[1] add new devfreq_suspend() and then call it on 
>> dpm_suspend()
>> when entering the suspend state.
>>
>> [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
>> - https://patchwork.kernel.org/patch/9443323/
>> - https://patchwork.kernel.org/patch/9443325/
>> - https://patchwork.kernel.org/patch/9443329/
>> - https://patchwork.kernel.org/patch/9443331/
>>
>> I think we need to discuss it together.
>>
>> Regards,
>> Chanwoo Choi
>>
>> On 2016년 11월 24일 15:45, hl wrote:
>>> Hi MyungJoo Ham,
>>>
>>> On 2016年11月24日 14:14, MyungJoo Ham wrote:
 On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
> Hi MyungJoo Ham,
 []
>> We still need to sync the all status even i call target() in
>> devfreq_suspend/resume_device
>> directly, so still need update_devfreq() other setp except
>> devfreq->governor->get_target_freq(devfreq, );
> And i think it better to be governor behaviors, for userspace they may not
> want to change
> the suspend frequency like other governor, the frequency should decide by
> the user, if they
> want this function, they should like other governor to rigister a
> devfreq_monitor_suspend().
> What do you think about my rev6 patch?
 If I understand the intention correctly, this is for the stability of
 the device due to the behavior or bootloader/SoC-initializer, which
 has nothing to do with governors.

 Even if users are using userspace, as long as they set the custom
 frequencies lower than the default, they have the possibility of
 being unstable as ondemand is going to have.


 To reuse the update_devfreq() code, you may do something like:

 static int _update_freq(struct devfreq *devfreq, bool is_suspending)
 {
  /* original contents of update_freq with if statement with 
 is_suspending wrapping get_target_freq */
 }
 int update_freq(struct devfreq *devfreq)
 {
 return _update_freq(devfreq, false);
 }


 There should be other good non-invasive methods that are not 
 governoe-specific as well.

>>> Thanks for your suggestion, i will update the new version soon.
 Cheers,
 MyungJoo




 ___
 Linux-rockchip mailing list
 linux-rockc...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>> -- 
>>> Lin Huang
>>>
>>
>>
>>
> 

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

[hl]

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:

_opp_table {
opp06 {
opp-suspend;
};
};

so i think my way semm more simple.

On 2016年11月24日 15:10, Chanwoo Choi wrote:

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:

Hi MyungJoo Ham,

On 2016年11月24日 14:14, MyungJoo Ham wrote:

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:

Hi MyungJoo Ham,

[]

We still need to sync the all status even i call target() in
devfreq_suspend/resume_device
directly, so still need update_devfreq() other setp except
devfreq->governor->get_target_freq(devfreq, );

And i think it better to be governor behaviors, for userspace they may not
want to change
the suspend frequency like other governor, the frequency should decide by
the user, if they
want this function, they should like other governor to rigister a
devfreq_monitor_suspend().
What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
 /* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.


Thanks for your suggestion, i will update the new version soon.

Cheers,
MyungJoo




___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

--
Lin Huang







--
Lin Huang

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

[hl]

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:

_opp_table {
opp06 {
opp-suspend;
};
};

so i think my way semm more simple.

On 2016年11月24日 15:10, Chanwoo Choi wrote:

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:

Hi MyungJoo Ham,

On 2016年11月24日 14:14, MyungJoo Ham wrote:

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:

Hi MyungJoo Ham,

[]

We still need to sync the all status even i call target() in
devfreq_suspend/resume_device
directly, so still need update_devfreq() other setp except
devfreq->governor->get_target_freq(devfreq, );

And i think it better to be governor behaviors, for userspace they may not
want to change
the suspend frequency like other governor, the frequency should decide by
the user, if they
want this function, they should like other governor to rigister a
devfreq_monitor_suspend().
What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
 /* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.


Thanks for your suggestion, i will update the new version soon.

Cheers,
MyungJoo




___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

--
Lin Huang







--
Lin Huang

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

[Chanwoo Choi]

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:
> Hi MyungJoo Ham,
> 
> On 2016年11月24日 14:14, MyungJoo Ham wrote:
>> On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
>>> Hi MyungJoo Ham,
>> []
 We still need to sync the all status even i call target() in
 devfreq_suspend/resume_device
 directly, so still need update_devfreq() other setp except
 devfreq->governor->get_target_freq(devfreq, );
>>> And i think it better to be governor behaviors, for userspace they may not
>>> want to change
>>> the suspend frequency like other governor, the frequency should decide by
>>> the user, if they
>>> want this function, they should like other governor to rigister a
>>> devfreq_monitor_suspend().
>>
>>> What do you think about my rev6 patch?
>> If I understand the intention correctly, this is for the stability of
>> the device due to the behavior or bootloader/SoC-initializer, which
>> has nothing to do with governors.
>>
>> Even if users are using userspace, as long as they set the custom
>> frequencies lower than the default, they have the possibility of
>> being unstable as ondemand is going to have.
>>
>>
>> To reuse the update_devfreq() code, you may do something like:
>>
>> static int _update_freq(struct devfreq *devfreq, bool is_suspending)
>> {
>> /* original contents of update_freq with if statement with is_suspending 
>> wrapping get_target_freq */
>> }
>> int update_freq(struct devfreq *devfreq)
>> {
>>  return _update_freq(devfreq, false);
>> }
>>
>>
>> There should be other good non-invasive methods that are not 
>> governoe-specific as well.
>>
> Thanks for your suggestion, i will update the new version soon.
>>
>> Cheers,
>> MyungJoo
>>
>>
>>
>>
>> ___
>> Linux-rockchip mailing list
>> linux-rockc...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> -- 
> Lin Huang
> 

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

[Chanwoo Choi]

+ Tobias Jakobi,

Hi Lin,

We need to discuss how to support the suspend-opp of devfreq device.
Now, there are two patch thread for suspend-opp of devfreq.

The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
The Tobias's approach[1] add new devfreq_suspend() and then call it on 
dpm_suspend()
when entering the suspend state.

[1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
- https://patchwork.kernel.org/patch/9443323/
- https://patchwork.kernel.org/patch/9443325/
- https://patchwork.kernel.org/patch/9443329/
- https://patchwork.kernel.org/patch/9443331/

I think we need to discuss it together.

Regards,
Chanwoo Choi

On 2016년 11월 24일 15:45, hl wrote:
> Hi MyungJoo Ham,
> 
> On 2016年11月24日 14:14, MyungJoo Ham wrote:
>> On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
>>> Hi MyungJoo Ham,
>> []
 We still need to sync the all status even i call target() in
 devfreq_suspend/resume_device
 directly, so still need update_devfreq() other setp except
 devfreq->governor->get_target_freq(devfreq, );
>>> And i think it better to be governor behaviors, for userspace they may not
>>> want to change
>>> the suspend frequency like other governor, the frequency should decide by
>>> the user, if they
>>> want this function, they should like other governor to rigister a
>>> devfreq_monitor_suspend().
>>
>>> What do you think about my rev6 patch?
>> If I understand the intention correctly, this is for the stability of
>> the device due to the behavior or bootloader/SoC-initializer, which
>> has nothing to do with governors.
>>
>> Even if users are using userspace, as long as they set the custom
>> frequencies lower than the default, they have the possibility of
>> being unstable as ondemand is going to have.
>>
>>
>> To reuse the update_devfreq() code, you may do something like:
>>
>> static int _update_freq(struct devfreq *devfreq, bool is_suspending)
>> {
>> /* original contents of update_freq with if statement with is_suspending 
>> wrapping get_target_freq */
>> }
>> int update_freq(struct devfreq *devfreq)
>> {
>>  return _update_freq(devfreq, false);
>> }
>>
>>
>> There should be other good non-invasive methods that are not 
>> governoe-specific as well.
>>
> Thanks for your suggestion, i will update the new version soon.
>>
>> Cheers,
>> MyungJoo
>>
>>
>>
>>
>> ___
>> Linux-rockchip mailing list
>> linux-rockc...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> -- 
> Lin Huang
> 

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

[MyungJoo Ham]

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
> Hi MyungJoo Ham,
[]
>>
>> We still need to sync the all status even i call target() in
>> devfreq_suspend/resume_device
>> directly, so still need update_devfreq() other setp except
>> devfreq->governor->get_target_freq(devfreq, );
>
> And i think it better to be governor behaviors, for userspace they may not
> want to change
> the suspend frequency like other governor, the frequency should decide by
> the user, if they
> want this function, they should like other governor to rigister a
> devfreq_monitor_suspend().


> What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
/* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.



Cheers,
MyungJoo

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

[MyungJoo Ham]

On Thu, Nov 24, 2016 at 11:18 AM, hl  wrote:
> Hi MyungJoo Ham,
[]
>>
>> We still need to sync the all status even i call target() in
>> devfreq_suspend/resume_device
>> directly, so still need update_devfreq() other setp except
>> devfreq->governor->get_target_freq(devfreq, );
>
> And i think it better to be governor behaviors, for userspace they may not
> want to change
> the suspend frequency like other governor, the frequency should decide by
> the user, if they
> want this function, they should like other governor to rigister a
> devfreq_monitor_suspend().


> What do you think about my rev6 patch?

If I understand the intention correctly, this is for the stability of
the device due to the behavior or bootloader/SoC-initializer, which
has nothing to do with governors.

Even if users are using userspace, as long as they set the custom
frequencies lower than the default, they have the possibility of
being unstable as ondemand is going to have.


To reuse the update_devfreq() code, you may do something like:

static int _update_freq(struct devfreq *devfreq, bool is_suspending)
{
/* original contents of update_freq with if statement with is_suspending 
wrapping get_target_freq */
}
int update_freq(struct devfreq *devfreq)
{
return _update_freq(devfreq, false);
}


There should be other good non-invasive methods that are not governoe-specific 
as well.



Cheers,
MyungJoo