[PATCH v1 0/2] set specific ddr frequency when stop ddr dvfs

[PATCH v1 0/2] set specific ddr frequency when stop ddr dvfs

[Lin Huang]

We need ddr run a specific frequency when ddr dvfs stop working. So we
implement get suspend frequency function in devfreq framework, and call
it in rk3399 dmc driver.

Lin Huang (2):
  PM/devfreq: add suspend frequency support
  PM/devfreq: rk3399: set specific ddr frequency when stop ddr dvfs

 drivers/devfreq/devfreq.c    | 17 ++++++++++++++++-
 drivers/devfreq/rk3399_dmc.c |  2 +-
 include/linux/devfreq.h      |  9 +++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.6.6

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

[Lin Huang]

Add suspend frequency support and if needed set it to
the frequency obtained from the suspend opp (can be defined
using opp-v2 bindings and is optional).

Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
 include/linux/devfreq.h   |  9 +++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..d1152eb 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
 		return;
 	}
 
-	devfreq_update_status(devfreq, devfreq->previous_freq);
+	if (devfreq->suspend_freq)
+		devfreq_update_status(devfreq, devfreq->suspend_freq);
+	else
+		devfreq_update_status(devfreq, devfreq->previous_freq);
 	devfreq->stop_polling = true;
 	mutex_unlock(&devfreq->lock);
 	cancel_delayed_work_sync(&devfreq->work);
@@ -1251,6 +1254,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 }
 EXPORT_SYMBOL(devfreq_recommended_opp);
 
+void devfreq_opp_get_suspend_opp(struct device *dev, struct devfreq *devfreq)
+{
+	struct dev_pm_opp *suspend_opp;
+
+	rcu_read_lock();
+	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
+	if (suspend_opp)
+		devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(devfreq_opp_get_suspend_opp);
+
 /**
  * devfreq_register_opp_notifier() - Helper function to get devfreq notified
  *				   for any changes in the OPP availability
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2de4e2e..c785ad9 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -88,6 +88,7 @@ struct devfreq_dev_status {
  */
 struct devfreq_dev_profile {
 	unsigned long initial_freq;
+	unsigned long suspend_freq;
 	unsigned int polling_ms;
 
 	int (*target)(struct device *dev, unsigned long *freq, u32 flags);
@@ -172,6 +173,7 @@ struct devfreq {
 	struct delayed_work work;
 
 	unsigned long previous_freq;
+	unsigned long suspend_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
@@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
 /* Helper functions for devfreq user device driver with OPP. */
 extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 					   unsigned long *freq, u32 flags);
+extern void devfreq_opp_get_suspend_opp(struct device *dev,
+					struct devfreq *devfreq);
 extern int devfreq_register_opp_notifier(struct device *dev,
 					 struct devfreq *devfreq);
 extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 	return ERR_PTR(-EINVAL);
 }
 
+static inline void devfreq_opp_get_suspend_opp(struct device *dev,
+					       struct devfreq *devfreq)
+{
+}
+
 static inline int devfreq_register_opp_notifier(struct device *dev,
 					 struct devfreq *devfreq)
 {
-- 
2.6.6

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

[dbasehore .]

On Tue, Nov 1, 2016 at 1:47 AM, Lin Huang <hl@rock-chips.com> wrote:
> Add suspend frequency support and if needed set it to
> the frequency obtained from the suspend opp (can be defined
> using opp-v2 bindings and is optional).
>
> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>  drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>  include/linux/devfreq.h   |  9 +++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..d1152eb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>                 return;
>         }
>
> -       devfreq_update_status(devfreq, devfreq->previous_freq);
> +       if (devfreq->suspend_freq)
> +               devfreq_update_status(devfreq, devfreq->suspend_freq);
> +       else
> +               devfreq_update_status(devfreq, devfreq->previous_freq);
>         devfreq->stop_polling = true;
>         mutex_unlock(&devfreq->lock);
>         cancel_delayed_work_sync(&devfreq->work);
> @@ -1251,6 +1254,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>  }
>  EXPORT_SYMBOL(devfreq_recommended_opp);
>
> +void devfreq_opp_get_suspend_opp(struct device *dev, struct devfreq *devfreq)
> +{
> +       struct dev_pm_opp *suspend_opp;
> +
> +       rcu_read_lock();
> +       suspend_opp = dev_pm_opp_get_suspend_opp(dev);
> +       if (suspend_opp)
> +               devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
> +       rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(devfreq_opp_get_suspend_opp);
> +

Instead of exporting a new function to do this, can it be done in the
devfreq_add_device function?

>  /**
>   * devfreq_register_opp_notifier() - Helper function to get devfreq notified
>   *                                for any changes in the OPP availability
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2de4e2e..c785ad9 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -88,6 +88,7 @@ struct devfreq_dev_status {
>   */
>  struct devfreq_dev_profile {
>         unsigned long initial_freq;
> +       unsigned long suspend_freq;
>         unsigned int polling_ms;
>
>         int (*target)(struct device *dev, unsigned long *freq, u32 flags);
> @@ -172,6 +173,7 @@ struct devfreq {
>         struct delayed_work work;
>
>         unsigned long previous_freq;
> +       unsigned long suspend_freq;
>         struct devfreq_dev_status last_status;
>
>         void *data; /* private data for governors */
> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
>  /* Helper functions for devfreq user device driver with OPP. */
>  extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>                                            unsigned long *freq, u32 flags);
> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
> +                                       struct devfreq *devfreq);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>                                          struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>         return ERR_PTR(-EINVAL);
>  }
>
> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
> +                                              struct devfreq *devfreq)
> +{
> +}
> +
>  static inline int devfreq_register_opp_notifier(struct device *dev,
>                                          struct devfreq *devfreq)
>  {
> --
> 2.6.6
>

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

[hl]

Hi Derek,

On 2016年11月01日 17:06, dbasehore . wrote:
> On Tue, Nov 1, 2016 at 1:47 AM, Lin Huang <hl@rock-chips.com> wrote:
>> Add suspend frequency support and if needed set it to
>> the frequency obtained from the suspend opp (can be defined
>> using opp-v2 bindings and is optional).
>>
>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>>   drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>   include/linux/devfreq.h   |  9 +++++++++
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..d1152eb 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>>                  return;
>>          }
>>
>> -       devfreq_update_status(devfreq, devfreq->previous_freq);
>> +       if (devfreq->suspend_freq)
>> +               devfreq_update_status(devfreq, devfreq->suspend_freq);
>> +       else
>> +               devfreq_update_status(devfreq, devfreq->previous_freq);
>>          devfreq->stop_polling = true;
>>          mutex_unlock(&devfreq->lock);
>>          cancel_delayed_work_sync(&devfreq->work);
>> @@ -1251,6 +1254,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>   }
>>   EXPORT_SYMBOL(devfreq_recommended_opp);
>>
>> +void devfreq_opp_get_suspend_opp(struct device *dev, struct devfreq *devfreq)
>> +{
>> +       struct dev_pm_opp *suspend_opp;
>> +
>> +       rcu_read_lock();
>> +       suspend_opp = dev_pm_opp_get_suspend_opp(dev);
>> +       if (suspend_opp)
>> +               devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
>> +       rcu_read_unlock();
>> +}
>> +EXPORT_SYMBOL(devfreq_opp_get_suspend_opp);
>> +
> Instead of exporting a new function to do this, can it be done in the
> devfreq_add_device function?
Seems all the devfreq opp function export a new fucntion, to be consistent,
i export devfreq_opp_get_suspend_opp().
>>   /**
>>    * devfreq_register_opp_notifier() - Helper function to get devfreq notified
>>    *                                for any changes in the OPP availability
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2de4e2e..c785ad9 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -88,6 +88,7 @@ struct devfreq_dev_status {
>>    */
>>   struct devfreq_dev_profile {
>>          unsigned long initial_freq;
>> +       unsigned long suspend_freq;
>>          unsigned int polling_ms;
>>
>>          int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>> @@ -172,6 +173,7 @@ struct devfreq {
>>          struct delayed_work work;
>>
>>          unsigned long previous_freq;
>> +       unsigned long suspend_freq;
>>          struct devfreq_dev_status last_status;
>>
>>          void *data; /* private data for governors */
>> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
>>   /* Helper functions for devfreq user device driver with OPP. */
>>   extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>                                             unsigned long *freq, u32 flags);
>> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
>> +                                       struct devfreq *devfreq);
>>   extern int devfreq_register_opp_notifier(struct device *dev,
>>                                           struct devfreq *devfreq);
>>   extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>          return ERR_PTR(-EINVAL);
>>   }
>>
>> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
>> +                                              struct devfreq *devfreq)
>> +{
>> +}
>> +
>>   static inline int devfreq_register_opp_notifier(struct device *dev,
>>                                           struct devfreq *devfreq)
>>   {
>> --
>> 2.6.6
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

-- 
Lin Huang

[PATCH v1 2/2] PM/devfreq: rk3399: set specific ddr frequency when stop ddr dvfs

[Lin Huang]

We need ddr run a specific frequency when ddr dvfs stop
working. For example: if we enable two monitor, then we will
stop ddr dvfs, but we hope ddr can run in highest frequency
obviously.

Change-Id: I02460f0becbf0e1a732e2a55b8e529f53c01109c
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 drivers/devfreq/rk3399_dmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index e24b73d..9309941 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -443,7 +443,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 	if (IS_ERR(data->devfreq))
 		return PTR_ERR(data->devfreq);
 	devm_devfreq_register_opp_notifier(dev, data->devfreq);
-
+	devfreq_opp_get_suspend_opp(dev, data->devfreq);
 	data->dev = dev;
 	platform_set_drvdata(pdev, data);
 
-- 
2.6.6

RE: [PATCH v1 1/2] PM/devfreq: add suspend frequency support

[MyungJoo Ham]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

> Add suspend frequency support and if needed set it to
> the frequency obtained from the suspend opp (can be defined
> using opp-v2 bindings and is optional).
> 
> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>  drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>  include/linux/devfreq.h   |  9 +++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..d1152eb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>  		return;
>  	}
>  
> -	devfreq_update_status(devfreq, devfreq->previous_freq);
> +	if (devfreq->suspend_freq)
> +		devfreq_update_status(devfreq, devfreq->suspend_freq);
> +	else
> +		devfreq_update_status(devfreq, devfreq->previous_freq);

This is incorrect.

devfreq_update_status is to record the statistics.
This code intends to record the current status before entering suspend;
thus you should not record "suspend_freq" here.

If you really need to record the frequency during suspended state
for the statistics, you need to do that at resume; however, I object to
that idea either.

If you really want to set a predefined suspend-to-RAM mode frequency,
(probably because of HW instability during resume process)
you need to update, not udpate_status (statistics).




Cheers,
MyungJoo

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

[hl]

Hi MyungJoo Ham,

On 2016年11月02日 14:35, MyungJoo Ham wrote:
>> Add suspend frequency support and if needed set it to
>> the frequency obtained from the suspend opp (can be defined
>> using opp-v2 bindings and is optional).
>>
>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>>   drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>   include/linux/devfreq.h   |  9 +++++++++
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..d1152eb 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>>   		return;
>>   	}
>>   
>> -	devfreq_update_status(devfreq, devfreq->previous_freq);
>> +	if (devfreq->suspend_freq)
>> +		devfreq_update_status(devfreq, devfreq->suspend_freq);
>> +	else
>> +		devfreq_update_status(devfreq, devfreq->previous_freq);
> This is incorrect.
>
> devfreq_update_status is to record the statistics.
> This code intends to record the current status before entering suspend;
> thus you should not record "suspend_freq" here.
>
> If you really need to record the frequency during suspended state
> for the statistics, you need to do that at resume; however, I object to
> that idea either.
>
> If you really want to set a predefined suspend-to-RAM mode frequency,
> (probably because of HW instability during resume process)
> you need to update, not udpate_status (statistics).
>
Thanks for pointing it, but if i use update_devfreq(), it can not 
specify the
frequency, it call the get_target_freq() to get the frequency and setting,
use now devfreq framework, it seems hard to set the specific freqeuency
when suspend. Do you have any idea, thanks.
>
>
> Cheers,
> MyungJoo
>

-- 
Lin Huang

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

[MyungJoo Ham]

On Thu, Nov 3, 2016 at 4:03 PM, hl <hl@rock-chips.com> wrote:
> Hi MyungJoo Ham,
>
>
> On 2016年11月02日 14:35, MyungJoo Ham wrote:
>>>
>>> Add suspend frequency support and if needed set it to
>>> the frequency obtained from the suspend opp (can be defined
>>> using opp-v2 bindings and is optional).
>>>
>>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>>   include/linux/devfreq.h   |  9 +++++++++
>>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index bf3ea76..d1152eb 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq
>>> *devfreq)
>>>                 return;
>>>         }
>>>   -     devfreq_update_status(devfreq, devfreq->previous_freq);
>>> +       if (devfreq->suspend_freq)
>>> +               devfreq_update_status(devfreq, devfreq->suspend_freq);
>>> +       else
>>> +               devfreq_update_status(devfreq, devfreq->previous_freq);
>>
>> This is incorrect.
>>
>> devfreq_update_status is to record the statistics.
>> This code intends to record the current status before entering suspend;
>> thus you should not record "suspend_freq" here.
>>
>> If you really need to record the frequency during suspended state
>> for the statistics, you need to do that at resume; however, I object to
>> that idea either.
>>
>> If you really want to set a predefined suspend-to-RAM mode frequency,
>> (probably because of HW instability during resume process)
>> you need to update, not udpate_status (statistics).
>>
> Thanks for pointing it, but if i use update_devfreq(), it can not specify
> the
> frequency, it call the get_target_freq() to get the frequency and setting,
> use now devfreq framework, it seems hard to set the specific freqeuency
> when suspend. Do you have any idea, thanks.


It appears that we need to set the frequency directly with target() function
at suspend/resume after it is assured that related routines are
already suspended.

Plus, you might need to consider using devfreq_suspend/resume_device instead
of monitor. monitor functions are for the governor-specific behaviors.
You'll need such capabilities regardless of governors. (even userspace-governor
needs this)

Cheers,
MyungJoo

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
MyungJoo Ham, Ph.D.
Frontier CS Lab, S/W Center, Samsung Electronics

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

[hl]

On 2016年11月22日 20:48, MyungJoo Ham wrote:
> On Thu, Nov 3, 2016 at 4:03 PM, hl <hl@rock-chips.com> wrote:
>> Hi MyungJoo Ham,
>>
>>
>> On 2016年11月02日 14:35, MyungJoo Ham wrote:
>>>> Add suspend frequency support and if needed set it to
>>>> the frequency obtained from the suspend opp (can be defined
>>>> using opp-v2 bindings and is optional).
>>>>
>>>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>>>    include/linux/devfreq.h   |  9 +++++++++
>>>>    2 files changed, 25 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index bf3ea76..d1152eb 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq
>>>> *devfreq)
>>>>                  return;
>>>>          }
>>>>    -     devfreq_update_status(devfreq, devfreq->previous_freq);
>>>> +       if (devfreq->suspend_freq)
>>>> +               devfreq_update_status(devfreq, devfreq->suspend_freq);
>>>> +       else
>>>> +               devfreq_update_status(devfreq, devfreq->previous_freq);
>>> This is incorrect.
>>>
>>> devfreq_update_status is to record the statistics.
>>> This code intends to record the current status before entering suspend;
>>> thus you should not record "suspend_freq" here.
>>>
>>> If you really need to record the frequency during suspended state
>>> for the statistics, you need to do that at resume; however, I object to
>>> that idea either.
>>>
>>> If you really want to set a predefined suspend-to-RAM mode frequency,
>>> (probably because of HW instability during resume process)
>>> you need to update, not udpate_status (statistics).
>>>
>> Thanks for pointing it, but if i use update_devfreq(), it can not specify
>> the
>> frequency, it call the get_target_freq() to get the frequency and setting,
>> use now devfreq framework, it seems hard to set the specific freqeuency
>> when suspend. Do you have any idea, thanks.
>
> It appears that we need to set the frequency directly with target() function
> at suspend/resume after it is assured that related routines are
> already suspended.
>
> Plus, you might need to consider using devfreq_suspend/resume_device instead
> of monitor. monitor functions are for the governor-specific behaviors.
> You'll need such capabilities regardless of governors. (even userspace-governor
> needs this)
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, &freq);
>
> Cheers,
> MyungJoo
>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

-- 
Lin Huang



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

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

[hl]

Hi MyungJoo Ham,

On 2016年11月23日 10:01, hl wrote:
>
>
> On 2016年11月22日 20:48, MyungJoo Ham wrote:
>> On Thu, Nov 3, 2016 at 4:03 PM, hl <hl@rock-chips.com> wrote:
>>> Hi MyungJoo Ham,
>>>
>>>
>>> On 2016年11月02日 14:35, MyungJoo Ham wrote:
>>>>> Add suspend frequency support and if needed set it to
>>>>> the frequency obtained from the suspend opp (can be defined
>>>>> using opp-v2 bindings and is optional).
>>>>>
>>>>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>> ---
>>>>>    drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>>>>>    include/linux/devfreq.h   |  9 +++++++++
>>>>>    2 files changed, 25 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index bf3ea76..d1152eb 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq
>>>>> *devfreq)
>>>>>                  return;
>>>>>          }
>>>>>    -     devfreq_update_status(devfreq, devfreq->previous_freq);
>>>>> +       if (devfreq->suspend_freq)
>>>>> +               devfreq_update_status(devfreq, 
>>>>> devfreq->suspend_freq);
>>>>> +       else
>>>>> +               devfreq_update_status(devfreq, 
>>>>> devfreq->previous_freq);
>>>> This is incorrect.
>>>>
>>>> devfreq_update_status is to record the statistics.
>>>> This code intends to record the current status before entering 
>>>> suspend;
>>>> thus you should not record "suspend_freq" here.
>>>>
>>>> If you really need to record the frequency during suspended state
>>>> for the statistics, you need to do that at resume; however, I 
>>>> object to
>>>> that idea either.
>>>>
>>>> If you really want to set a predefined suspend-to-RAM mode frequency,
>>>> (probably because of HW instability during resume process)
>>>> you need to update, not udpate_status (statistics).
>>>>
>>> Thanks for pointing it, but if i use update_devfreq(), it can not 
>>> specify
>>> the
>>> frequency, it call the get_target_freq() to get the frequency and 
>>> setting,
>>> use now devfreq framework, it seems hard to set the specific freqeuency
>>> when suspend. Do you have any idea, thanks.
>>
>> It appears that we need to set the frequency directly with target() 
>> function
>> at suspend/resume after it is assured that related routines are
>> already suspended.
>>
>> Plus, you might need to consider using devfreq_suspend/resume_device 
>> instead
>> of monitor. monitor functions are for the governor-specific behaviors.
>> You'll need such capabilities regardless of governors. (even 
>> userspace-governor
>> needs this)
> 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, &freq);
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?

>>
>> Cheers,
>> MyungJoo
>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>

-- 
Lin Huang

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

[MyungJoo Ham]

[-- Attachment #1: Type: text/plain, Size: 2803 bytes --]



On Wed, Nov 23, 2016 at 11:01 AM, hl <hl@rock-chips.com> wrote:
>
>
> On 2016年11月22日 20:48, MyungJoo Ham wrote:
>>
>> On Thu, Nov 3, 2016 at 4:03 PM, hl <hl@rock-chips.com> wrote:
>>>
>>> Hi MyungJoo Ham,
>>>
>>>
>>> On 2016年11月02日 14:35, MyungJoo Ham wrote:
>>>>>
>>>>> Add suspend frequency support and if needed set it to
>>>>> the frequency obtained from the suspend opp (can be defined
>>>>> using opp-v2 bindings and is optional).
>>>>>
>>>>> +       if (devfreq->suspend_freq)
>>>>> +               devfreq_update_status(devfreq, devfreq->suspend_freq);
>>>>> +       else
>>>>> +               devfreq_update_status(devfreq, devfreq->previous_freq);
>>>>
>>>> This is incorrect.
>>>>
>>>> devfreq_update_status is to record the statistics.
>>>> This code intends to record the current status before entering suspend;
>>>> thus you should not record "suspend_freq" here.
>>>>
>>>> If you really need to record the frequency during suspended state
>>>> for the statistics, you need to do that at resume; however, I object to
>>>> that idea either.
>>>>
>>>> If you really want to set a predefined suspend-to-RAM mode frequency,
>>>> (probably because of HW instability during resume process)
>>>> you need to update, not udpate_status (statistics).
>>>>
>>> Thanks for pointing it, but if i use update_devfreq(), it can not specify
>>> the
>>> frequency, it call the get_target_freq() to get the frequency and
>>> setting,
>>> use now devfreq framework, it seems hard to set the specific freqeuency
>>> when suspend. Do you have any idea, thanks.
>>
>>
>> It appears that we need to set the frequency directly with target()
>> function
>> at suspend/resume after it is assured that related routines are
>> already suspended.
>>
>> Plus, you might need to consider using devfreq_suspend/resume_device
>> instead
>> of monitor. monitor functions are for the governor-specific behaviors.
>> You'll need such capabilities regardless of governors. (even
>> userspace-governor
>> needs this)
>
> 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, &freq);

The devfreq_update() there, which is to invoke corresponding
governor to decide next frequency (get_target_freq).

In order to change the frequency ignoring whatever governors might say
(i.e., we are going to suspend / resume now), you need to talk to
the driver directly without invoking governors.

You only need to be careful on the sequence of notification chains and
status updating (you will probably be writing a much simpler version of
update_devfreq() for suspend/resume)

 

Cheers,
MyungJoo