diff mbox series

cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency

Message ID 20190708074624.910-1-Anson.Huang@nxp.com (mailing list archive)
State Changes Requested, archived
Headers show
Series cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency | expand

Commit Message

Anson Huang July 8, 2019, 7:46 a.m. UTC 
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(+)

Comments

Leonard Crestez July 8, 2019, 11:04 a.m. UTC | #1 
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
Anson Huang July 9, 2019, 3:09 a.m. UTC | #2 
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
Anson Huang July 9, 2019, 7:24 a.m. UTC | #3 
> > 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
diff mbox series

Patch

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,