[PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

[PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

To reduce the suspend/resume latency, CPU's max supported frequency
should be used during low level suspend/resume phase, "opp-suspend"
property is NOT feasible since OPP defined in DT could be NOT supported
according to speed garding and market segment fuse settings. So we
can assign the cpufreq policy's suspend_freq with max available
frequency provided by cpufreq driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/cpufreq/imx-cpufreq-dt.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpufreq/imx-cpufreq-dt.c b/drivers/cpufreq/imx-cpufreq-dt.c
index 4f85f31..b6607e8 100644
--- a/drivers/cpufreq/imx-cpufreq-dt.c
+++ b/drivers/cpufreq/imx-cpufreq-dt.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/cpu.h>
+#include <linux/cpufreq.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -84,6 +85,16 @@ static int imx_cpufreq_dt_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __init imx_cpufreq_dt_setup_suspend_opp(void)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_get(0);
+
+	policy->suspend_freq = cpufreq_quick_get_max(0);
+
+	return 0;
+}
+late_initcall(imx_cpufreq_dt_setup_suspend_opp);
+
 static struct platform_driver imx_cpufreq_dt_driver = {
 	.probe = imx_cpufreq_dt_probe,
 	.remove = imx_cpufreq_dt_remove,
-- 
2.7.4

Re: [PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

[Leonard Crestez]

On 7/8/2019 10:55 AM, Anson.Huang@nxp.com wrote:
> To reduce the suspend/resume latency, CPU's max supported frequency
> should be used during low level suspend/resume phase, "opp-suspend"
> property is NOT feasible since OPP defined in DT could be NOT supported
> according to speed garding and market segment fuse settings. So we
> can assign the cpufreq policy's suspend_freq with max available
> frequency provided by cpufreq driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

> diff --git a/drivers/cpufreq/imx-cpufreq-dt.c b/drivers/cpufreq/imx-cpufreq-dt.c

> +static int __init imx_cpufreq_dt_setup_suspend_opp(void)
> +{
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(0);
> +
> +	policy->suspend_freq = cpufreq_quick_get_max(0);
> +
> +	return 0;
> +}
> +late_initcall(imx_cpufreq_dt_setup_suspend_opp);

The imx-cpufreq-dt driver is built as a module by default and this patch 
produces an error:

In file included from ../drivers/cpufreq/imx-cpufreq-dt.c:11:
../include/linux/module.h:131:42: error: redefinition of ‘__inittest’
   static inline initcall_t __maybe_unused __inittest(void)  \
                                           ^~~~~~~~~~
../include/linux/device.h:1656:1: note: in expansion of macro ‘module_init’
  module_init(__driver##_init); \
  ^~~~~~~~~~~

As far as I can tell late_initcall is not supported for modules.

Viresh: "max freq as suspend freq" is something that could be useful for 
other SOC families. The hardware can suspend at any freq; it's just that 
the highest one makes sense because it makes suspend/resume slightly faster.

Could this behavior be pushed to cpufreq-dt as a bool flag inside struct 
cpufreq_dt_platform_data?

Only a few other platforms use this, most others pass NULL like imx. But 
passing custom SOC-specific flags to cpufreq-dt makes a lot of sense

--
Regards,
Leonard

RE: [PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

[Anson Huang]

Hi, Leonard

> On 7/8/2019 10:55 AM, Anson.Huang@nxp.com wrote:
> > To reduce the suspend/resume latency, CPU's max supported frequency
> > should be used during low level suspend/resume phase, "opp-suspend"
> > property is NOT feasible since OPP defined in DT could be NOT
> > supported according to speed garding and market segment fuse settings.
> > So we can assign the cpufreq policy's suspend_freq with max available
> > frequency provided by cpufreq driver.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> > diff --git a/drivers/cpufreq/imx-cpufreq-dt.c
> > b/drivers/cpufreq/imx-cpufreq-dt.c
> 
> > +static int __init imx_cpufreq_dt_setup_suspend_opp(void)
> > +{
> > +	struct cpufreq_policy *policy = cpufreq_cpu_get(0);
> > +
> > +	policy->suspend_freq = cpufreq_quick_get_max(0);
> > +
> > +	return 0;
> > +}
> > +late_initcall(imx_cpufreq_dt_setup_suspend_opp);
> 
> The imx-cpufreq-dt driver is built as a module by default and this patch
> produces an error:
> 
> In file included from ../drivers/cpufreq/imx-cpufreq-dt.c:11:
> ../include/linux/module.h:131:42: error: redefinition of ‘__inittest’
>    static inline initcall_t __maybe_unused __inittest(void)  \
>                                            ^~~~~~~~~~
> ../include/linux/device.h:1656:1: note: in expansion of macro ‘module_init’
>   module_init(__driver##_init); \
>   ^~~~~~~~~~~
> 
> As far as I can tell late_initcall is not supported for modules.

Ah, yes, I have NOT test the module build, I ONLY tested the built-in case, thanks for
reminder.

> 
> Viresh: "max freq as suspend freq" is something that could be useful for
> other SOC families. The hardware can suspend at any freq; it's just that the
> highest one makes sense because it makes suspend/resume slightly faster.

Agree.

> 
> Could this behavior be pushed to cpufreq-dt as a bool flag inside struct
> cpufreq_dt_platform_data?
> 
> Only a few other platforms use this, most others pass NULL like imx. But
> passing custom SOC-specific flags to cpufreq-dt makes a lot of sense

Although we have other methods to make it work for i.MX platforms, like setting
the suspend freq as this patch did but ONLY be called before suspend, but if common
cpufreq-dt can handle this case, that would benefit us and other platforms a lot, waiting
for your opinion.

Thanks,
Anson

RE: [PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

[Anson Huang]

> > On 7/8/2019 10:55 AM, Anson.Huang@nxp.com wrote:
> > > To reduce the suspend/resume latency, CPU's max supported frequency
> > > should be used during low level suspend/resume phase, "opp-suspend"
> > > property is NOT feasible since OPP defined in DT could be NOT
> > > supported according to speed garding and market segment fuse settings.
> > > So we can assign the cpufreq policy's suspend_freq with max
> > > available frequency provided by cpufreq driver.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > > diff --git a/drivers/cpufreq/imx-cpufreq-dt.c
> > > b/drivers/cpufreq/imx-cpufreq-dt.c
> >
> > > +static int __init imx_cpufreq_dt_setup_suspend_opp(void)
> > > +{
> > > +	struct cpufreq_policy *policy = cpufreq_cpu_get(0);
> > > +
> > > +	policy->suspend_freq = cpufreq_quick_get_max(0);
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall(imx_cpufreq_dt_setup_suspend_opp);
> >
> > The imx-cpufreq-dt driver is built as a module by default and this
> > patch produces an error:
> >
> > In file included from ../drivers/cpufreq/imx-cpufreq-dt.c:11:
> > ../include/linux/module.h:131:42: error: redefinition of ‘__inittest’
> >    static inline initcall_t __maybe_unused __inittest(void)  \
> >                                            ^~~~~~~~~~
> > ../include/linux/device.h:1656:1: note: in expansion of macro ‘module_init’
> >   module_init(__driver##_init); \
> >   ^~~~~~~~~~~
> >
> > As far as I can tell late_initcall is not supported for modules.
> 
> Ah, yes, I have NOT test the module build, I ONLY tested the built-in case,
> thanks for reminder.
> 
> >
> > Viresh: "max freq as suspend freq" is something that could be useful
> > for other SOC families. The hardware can suspend at any freq; it's
> > just that the highest one makes sense because it makes suspend/resume
> slightly faster.
> 
> Agree.
> 
> >
> > Could this behavior be pushed to cpufreq-dt as a bool flag inside
> > struct cpufreq_dt_platform_data?
> >
> > Only a few other platforms use this, most others pass NULL like imx.
> > But passing custom SOC-specific flags to cpufreq-dt makes a lot of
> > sense
> 
> Although we have other methods to make it work for i.MX platforms, like
> setting the suspend freq as this patch did but ONLY be called before suspend,
> but if common cpufreq-dt can handle this case, that would benefit us and
> other platforms a lot, waiting for your opinion.

Please ignore this patch, based on the discussion, I have sent out a new patch
series to support this feature. Please review them:
https://patchwork.kernel.org/patch/11036505/

Thanks,
Anson