[PATCH V2 RESEND 0/4] PM / devfreq: a few small fixes and improvements

[PATCH V2 RESEND 0/4] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

A few small fixes and improvements

ChangeLog:
v2 Resend:
 * rebase to devfreq-next
 * drop original patch 1 & 5.
   Patch 5 will be re-sent later when dependent patches merged.
v1->v2:
 * squash a few patches
 * rebase to devfreq-testing


Dong Aisheng (4):
  PM / devfreq: Use more accurate returned new_freq as resume_freq
  PM / devfreq: Remove the invalid description for get_target_freq
  PM / devfreq: bail out early if no freq changes in devfreq_set_target
  PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

 Documentation/ABI/testing/sysfs-class-devfreq |  5 +----
 drivers/devfreq/devfreq.c                     | 11 +++++++----
 drivers/devfreq/governor.h                    |  2 --
 drivers/devfreq/imx8m-ddrc.c                  | 14 --------------
 4 files changed, 8 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH V2 RESEND 1/4] PM / devfreq: Use more accurate returned new_freq as resume_freq

[Dong Aisheng]

Use the more accurate returned new_freq as resume_freq.
It's the same as how devfreq->previous_freq was updated.

Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
devfreq device")
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b6d3e7db0b09..85927bd7ee76 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -388,7 +388,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 	devfreq->previous_freq = new_freq;
 
 	if (devfreq->suspend_freq)
-		devfreq->resume_freq = cur_freq;
+		devfreq->resume_freq = new_freq;
 
 	return err;
 }
-- 
2.25.1

[PATCH V2 RESEND 2/4] PM / devfreq: Remove the invalid description for get_target_freq

[Dong Aisheng]

First of all, no_central_polling was removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")
Secondly, get_target_freq() is not only called only with update_devfreq()
notified by OPP now, but also min/max freq qos notifier.

So remove this invalid description now to avoid confusing.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/ABI/testing/sysfs-class-devfreq | 5 +----
 drivers/devfreq/governor.h                    | 2 --
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
index 386bc230a33d..5e6b74f30406 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -97,10 +97,7 @@ Description:
 		object. The values are represented in ms. If the value is
 		less than 1 jiffy, it is considered to be 0, which means
 		no polling. This value is meaningless if the governor is
-		not polling; thus. If the governor is not using
-		devfreq-provided central polling
-		(/sys/class/devfreq/.../central_polling is 0), this value
-		may be useless.
+		not polling.
 
 		A list of governors that support the node:
 		- simple_ondmenad
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 244634465170..2d69a0ce6291 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -57,8 +57,6 @@
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the
  *			status of the device (load = busy_time / total_time).
- *			If no_central_polling is set, this callback is called
- *			only with update_devfreq() notified by OPP.
  * @event_handler:      Callback for devfreq core framework to notify events
  *                      to governors. Events include per device governor
  *                      init and exit, opp changes out of devfreq, suspend
-- 
2.25.1

[PATCH V2 RESEND 3/4] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

It's unnecessary to set the same freq again and run notifier calls.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/devfreq/devfreq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 85927bd7ee76..a6ee91dd17bd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -352,13 +352,16 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 {
 	struct devfreq_freqs freqs;
 	unsigned long cur_freq;
-	int err = 0;
+	int err;
 
 	if (devfreq->profile->get_cur_freq)
 		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
 	else
 		cur_freq = devfreq->previous_freq;
 
+	if (new_freq == cur_freq)
+		return 0;
+
 	freqs.old = cur_freq;
 	freqs.new = new_freq;
 	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
@@ -375,7 +378,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 	 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
 	 * change order of between devfreq device and passive devfreq device.
 	 */
-	if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+	if (trace_devfreq_frequency_enabled())
 		trace_devfreq_frequency(devfreq, new_freq, cur_freq);
 
 	freqs.new = new_freq;
@@ -390,7 +393,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 	if (devfreq->suspend_freq)
 		devfreq->resume_freq = new_freq;
 
-	return err;
+	return 0;
 }
 
 /**
-- 
2.25.1

[PATCH V2 RESEND 4/4] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Dong Aisheng]

Current driver actually does not support simple ondemand governor
as it's unable to provide device load information. So removing
the unnecessary callback to avoid confusing.
Right now the driver is using userspace governor by default.

polling_ms was also dropped as it's not needed for non-ondemand
governor.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/devfreq/imx8m-ddrc.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index bc82d3653bff..ecb9375aa877 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
 	return 0;
 }
 
-static int imx8m_ddrc_get_dev_status(struct device *dev,
-				     struct devfreq_dev_status *stat)
-{
-	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-
-	stat->busy_time = 0;
-	stat->total_time = 0;
-	stat->current_frequency = clk_get_rate(priv->dram_core);
-
-	return 0;
-}
-
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
 	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
@@ -429,9 +417,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
-	priv->profile.polling_ms = 1000;
 	priv->profile.target = imx8m_ddrc_target;
-	priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
 	priv->profile.exit = imx8m_ddrc_exit;
 	priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
 	priv->profile.initial_freq = clk_get_rate(priv->dram_core);
-- 
2.25.1

Re: [PATCH V2 RESEND 1/4] PM / devfreq: Use more accurate returned new_freq as resume_freq

[Chanwoo Choi]

On 21. 3. 23. 오후 4:20, Dong Aisheng wrote:
> Use the more accurate returned new_freq as resume_freq.
> It's the same as how devfreq->previous_freq was updated.
> 
> Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
> devfreq device")
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   drivers/devfreq/devfreq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index b6d3e7db0b09..85927bd7ee76 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -388,7 +388,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>   	devfreq->previous_freq = new_freq;
>   
>   	if (devfreq->suspend_freq)
> -		devfreq->resume_freq = cur_freq;
> +		devfreq->resume_freq = new_freq;
>   
>   	return err;
>   }
> 

Applied it.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH V2 RESEND 2/4] PM / devfreq: Remove the invalid description for get_target_freq

[Chanwoo Choi]

On 21. 3. 23. 오후 4:20, Dong Aisheng wrote:
> First of all, no_central_polling was removed since
> commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
> which can idle")
> Secondly, get_target_freq() is not only called only with update_devfreq()
> notified by OPP now, but also min/max freq qos notifier.
> 
> So remove this invalid description now to avoid confusing.
> 
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   Documentation/ABI/testing/sysfs-class-devfreq | 5 +----
>   drivers/devfreq/governor.h                    | 2 --
>   2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
> index 386bc230a33d..5e6b74f30406 100644
> --- a/Documentation/ABI/testing/sysfs-class-devfreq
> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
> @@ -97,10 +97,7 @@ Description:
>   		object. The values are represented in ms. If the value is
>   		less than 1 jiffy, it is considered to be 0, which means
>   		no polling. This value is meaningless if the governor is
> -		not polling; thus. If the governor is not using
> -		devfreq-provided central polling
> -		(/sys/class/devfreq/.../central_polling is 0), this value
> -		may be useless.
> +		not polling.
>   
>   		A list of governors that support the node:
>   		- simple_ondmenad
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index 244634465170..2d69a0ce6291 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -57,8 +57,6 @@
>    *			Basically, get_target_freq will run
>    *			devfreq_dev_profile.get_dev_status() to get the
>    *			status of the device (load = busy_time / total_time).
> - *			If no_central_polling is set, this callback is called
> - *			only with update_devfreq() notified by OPP.
>    * @event_handler:      Callback for devfreq core framework to notify events
>    *                      to governors. Events include per device governor
>    *                      init and exit, opp changes out of devfreq, suspend
> 

Applied it. Thanks.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH V2 RESEND 4/4] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Chanwoo Choi]

On 21. 3. 23. 오후 4:20, Dong Aisheng wrote:
> Current driver actually does not support simple ondemand governor
> as it's unable to provide device load information. So removing
> the unnecessary callback to avoid confusing.
> Right now the driver is using userspace governor by default.
> 
> polling_ms was also dropped as it's not needed for non-ondemand
> governor.
> 
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   drivers/devfreq/imx8m-ddrc.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> index bc82d3653bff..ecb9375aa877 100644
> --- a/drivers/devfreq/imx8m-ddrc.c
> +++ b/drivers/devfreq/imx8m-ddrc.c
> @@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>   	return 0;
>   }
>   
> -static int imx8m_ddrc_get_dev_status(struct device *dev,
> -				     struct devfreq_dev_status *stat)
> -{
> -	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> -
> -	stat->busy_time = 0;
> -	stat->total_time = 0;
> -	stat->current_frequency = clk_get_rate(priv->dram_core);
> -
> -	return 0;
> -}
> -
>   static int imx8m_ddrc_init_freq_info(struct device *dev)
>   {
>   	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> @@ -429,9 +417,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		goto err;
>   
> -	priv->profile.polling_ms = 1000;
>   	priv->profile.target = imx8m_ddrc_target;
> -	priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
>   	priv->profile.exit = imx8m_ddrc_exit;
>   	priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
>   	priv->profile.initial_freq = clk_get_rate(priv->dram_core);
> 

Applied it. Thanks.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH V2 RESEND 3/4] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Chanwoo Choi]

On 21. 3. 23. 오후 4:20, Dong Aisheng wrote:
> It's unnecessary to set the same freq again and run notifier calls.
> 
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   drivers/devfreq/devfreq.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 85927bd7ee76..a6ee91dd17bd 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -352,13 +352,16 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>   {
>   	struct devfreq_freqs freqs;
>   	unsigned long cur_freq;
> -	int err = 0;
> +	int err;
>   
>   	if (devfreq->profile->get_cur_freq)
>   		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>   	else
>   		cur_freq = devfreq->previous_freq;
>   
> +	if (new_freq == cur_freq)
> +		return 0;

cur_freq is one of the OPP frequencies. But, new_freq is calculated from
governor algorithm. It means that new_freq is not one of the
frequencies. Actually, it is not efficient.

After devfreq->profile->target() which almost uses
devfreq_recommended_opp(), new_freq is one of OPP frequencies.

> +
>   	freqs.old = cur_freq;
>   	freqs.new = new_freq;
>   	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
> @@ -375,7 +378,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>   	 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
>   	 * change order of between devfreq device and passive devfreq device.
>   	 */
> -	if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> +	if (trace_devfreq_frequency_enabled())
>   		trace_devfreq_frequency(devfreq, new_freq, cur_freq);
>   
>   	freqs.new = new_freq;
> @@ -390,7 +393,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>   	if (devfreq->suspend_freq)
>   		devfreq->resume_freq = new_freq;
>   
> -	return err;
> +	return 0;
>   }
>   
>   /**
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH V2 RESEND 3/4] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

On Tue, Mar 23, 2021 at 11:01 PM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> On 21. 3. 23. 오후 4:20, Dong Aisheng wrote:
> > It's unnecessary to set the same freq again and run notifier calls.
> >
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >   drivers/devfreq/devfreq.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 85927bd7ee76..a6ee91dd17bd 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -352,13 +352,16 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> >   {
> >       struct devfreq_freqs freqs;
> >       unsigned long cur_freq;
> > -     int err = 0;
> > +     int err;
> >
> >       if (devfreq->profile->get_cur_freq)
> >               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
> >       else
> >               cur_freq = devfreq->previous_freq;
> >
> > +     if (new_freq == cur_freq)
> > +             return 0;
>
> cur_freq is one of the OPP frequencies. But, new_freq is calculated from
> governor algorithm. It means that new_freq is not one of the
> frequencies. Actually, it is not efficient.
>
> After devfreq->profile->target() which almost uses
> devfreq_recommended_opp(), new_freq is one of OPP frequencies.
>

Yes, but i feel at least when the desired new_freq is equal to cur_freq
which is the last successfully set rate,  it is sufficient to bail out early as
it's meaningless to re-set the same rate as the last one and notify the
an unchanged rate transition in HW.
Does that make sense?

Regards
Aisheng

> > +
> >       freqs.old = cur_freq;
> >       freqs.new = new_freq;
> >       devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
> > @@ -375,7 +378,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> >        * and DEVFREQ_POSTCHANGE because for showing the correct frequency
> >        * change order of between devfreq device and passive devfreq device.
> >        */
> > -     if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> > +     if (trace_devfreq_frequency_enabled())
> >               trace_devfreq_frequency(devfreq, new_freq, cur_freq);
> >
> >       freqs.new = new_freq;
> > @@ -390,7 +393,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> >       if (devfreq->suspend_freq)
> >               devfreq->resume_freq = new_freq;
> >
> > -     return err;
> > +     return 0;
> >   }
> >
> >   /**
> >
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi