diff mbox series

[1/3] cpufreq: dt: Allow platform specific suspend/resume callbacks

Message ID 031b87c0dd05625c83ac920a8da661e72afb4b02.1524549644.git.viresh.kumar@linaro.org
State Superseded
Headers show
Series cpufreq: dt: Allow platforms to provide suspend/resume hooks | expand

Commit Message

Viresh Kumar April 24, 2018, 6:07 a.m. UTC 
Platforms may need to implement platform specific suspend/resume hooks.
Update cpufreq-dt driver's platform data to contain those for such
platforms.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/cpufreq/cpufreq-dt.c | 10 ++++++++--
 drivers/cpufreq/cpufreq-dt.h |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.15.0.194.g9af6a3dea062

Comments

Miquel Raynal April 24, 2018, 9:14 a.m. UTC | #1 
Hi Viresh,

On Tue, 24 Apr 2018 11:37:34 +0530, Viresh Kumar
<viresh.kumar@linaro.org> wrote:

> Platforms may need to implement platform specific suspend/resume hooks.

> Update cpufreq-dt driver's platform data to contain those for such

> platforms.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  drivers/cpufreq/cpufreq-dt.c | 10 ++++++++--

>  drivers/cpufreq/cpufreq-dt.h |  5 +++++

>  2 files changed, 13 insertions(+), 2 deletions(-)

> 

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

> index 190ea0dccb79..0a9ebf00be46 100644

> --- a/drivers/cpufreq/cpufreq-dt.c

> +++ b/drivers/cpufreq/cpufreq-dt.c

> @@ -346,8 +346,14 @@ static int dt_cpufreq_probe(struct platform_device *pdev)

>  	if (ret)

>  		return ret;

>  

> -	if (data && data->have_governor_per_policy)

> -		dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> +	if (data) {

> +		if (data->have_governor_per_policy)

> +			dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> +

> +		dt_cpufreq_driver.resume = data->resume;

> +		if (data->suspend)

> +			dt_cpufreq_driver.suspend = data->suspend;


I wonder why testing data->suspend and not data->resume? I suppose this
condition is not actually needed, right?

> +	}

>  

>  	ret = cpufreq_register_driver(&dt_cpufreq_driver);

>  	if (ret)

> diff --git a/drivers/cpufreq/cpufreq-dt.h b/drivers/cpufreq/cpufreq-dt.h

> index 54d774e46c43..d5aeea13433e 100644

> --- a/drivers/cpufreq/cpufreq-dt.h

> +++ b/drivers/cpufreq/cpufreq-dt.h

> @@ -12,8 +12,13 @@

>  

>  #include <linux/types.h>

>  

> +struct cpufreq_policy;

> +

>  struct cpufreq_dt_platform_data {

>  	bool have_governor_per_policy;

> +

> +	int (*suspend)(struct cpufreq_policy *policy);

> +	int (*resume)(struct cpufreq_policy *policy);

>  };

>  

>  #endif /* __CPUFREQ_DT_H__ */



Thank you,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Viresh Kumar April 24, 2018, 9:19 a.m. UTC | #2 
On 24-04-18, 11:14, Miquel Raynal wrote:
> Hi Viresh,

> 

> On Tue, 24 Apr 2018 11:37:34 +0530, Viresh Kumar

> <viresh.kumar@linaro.org> wrote:

> 

> > Platforms may need to implement platform specific suspend/resume hooks.

> > Update cpufreq-dt driver's platform data to contain those for such

> > platforms.

> > 

> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > ---

> >  drivers/cpufreq/cpufreq-dt.c | 10 ++++++++--

> >  drivers/cpufreq/cpufreq-dt.h |  5 +++++

> >  2 files changed, 13 insertions(+), 2 deletions(-)

> > 

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

> > index 190ea0dccb79..0a9ebf00be46 100644

> > --- a/drivers/cpufreq/cpufreq-dt.c

> > +++ b/drivers/cpufreq/cpufreq-dt.c

> > @@ -346,8 +346,14 @@ static int dt_cpufreq_probe(struct platform_device *pdev)

> >  	if (ret)

> >  		return ret;

> >  

> > -	if (data && data->have_governor_per_policy)

> > -		dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> > +	if (data) {

> > +		if (data->have_governor_per_policy)

> > +			dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> > +

> > +		dt_cpufreq_driver.resume = data->resume;

> > +		if (data->suspend)

> > +			dt_cpufreq_driver.suspend = data->suspend;

> 

> I wonder why testing data->suspend and not data->resume? I suppose this

> condition is not actually needed, right?


That's because dt_cpufreq_driver.suspend is already set while the
structure is defined. We shouldn't overwrite that unconditionally. But
resume isn't set there and so just overriding it is fine.

-- 
viresh
Miquel Raynal April 24, 2018, 9:27 a.m. UTC | #3 
Hi Viresh,

On Tue, 24 Apr 2018 14:49:24 +0530, Viresh Kumar
<viresh.kumar@linaro.org> wrote:

> On 24-04-18, 11:14, Miquel Raynal wrote:

> > Hi Viresh,

> > 

> > On Tue, 24 Apr 2018 11:37:34 +0530, Viresh Kumar

> > <viresh.kumar@linaro.org> wrote:

> >   

> > > Platforms may need to implement platform specific suspend/resume hooks.

> > > Update cpufreq-dt driver's platform data to contain those for such

> > > platforms.

> > > 

> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > ---

> > >  drivers/cpufreq/cpufreq-dt.c | 10 ++++++++--

> > >  drivers/cpufreq/cpufreq-dt.h |  5 +++++

> > >  2 files changed, 13 insertions(+), 2 deletions(-)

> > > 

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

> > > index 190ea0dccb79..0a9ebf00be46 100644

> > > --- a/drivers/cpufreq/cpufreq-dt.c

> > > +++ b/drivers/cpufreq/cpufreq-dt.c

> > > @@ -346,8 +346,14 @@ static int dt_cpufreq_probe(struct platform_device *pdev)

> > >  	if (ret)

> > >  		return ret;

> > >  

> > > -	if (data && data->have_governor_per_policy)

> > > -		dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> > > +	if (data) {

> > > +		if (data->have_governor_per_policy)

> > > +			dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;

> > > +

> > > +		dt_cpufreq_driver.resume = data->resume;

> > > +		if (data->suspend)

> > > +			dt_cpufreq_driver.suspend = data->suspend;  

> > 

> > I wonder why testing data->suspend and not data->resume? I suppose this

> > condition is not actually needed, right?  

> 

> That's because dt_cpufreq_driver.suspend is already set while the

> structure is defined. We shouldn't overwrite that unconditionally. But

> resume isn't set there and so just overriding it is fine.

> 


Ok, I did not check that.

In this case we probable should not set the resume function neither? In
the armada37xx case, let's say the suspend hook was already defined, it
would mean that the ->resume hook could be called while the "suspended
state" structure is still not initialized: this could freeze the board
I guess.

I don't know exactly in which case the ->suspend hook could be defined
though, maybe this situation does not apply?

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Viresh Kumar April 24, 2018, 9:30 a.m. UTC | #4 
On 24-04-18, 11:27, Miquel Raynal wrote:
> Ok, I did not check that.

> 

> In this case we probable should not set the resume function neither? In

> the armada37xx case, let's say the suspend hook was already defined, it


But we are overwriting this in your case. The default suspend hook is
used to run a generic suspend routine, which takes down the CPUs to a
specific OPP (may be lowest frequency) before taking it down.

> would mean that the ->resume hook could be called while the "suspended

> state" structure is still not initialized: this could freeze the board

> I guess.


You should set the suspend hook as well, its a bug otherwise. There
can be cases where people want the generic suspend routine to run at
suspend but they want to reinitialize something during resume and so
this code may make sense.

-- 
viresh
Miquel Raynal April 24, 2018, 9:38 a.m. UTC | #5 
Hi Viresh,

On Tue, 24 Apr 2018 15:00:58 +0530, Viresh Kumar
<viresh.kumar@linaro.org> wrote:

> On 24-04-18, 11:27, Miquel Raynal wrote:

> > Ok, I did not check that.

> > 

> > In this case we probable should not set the resume function neither? In

> > the armada37xx case, let's say the suspend hook was already defined, it  

> 

> But we are overwriting this in your case. The default suspend hook is

> used to run a generic suspend routine, which takes down the CPUs to a

> specific OPP (may be lowest frequency) before taking it down.

> 

> > would mean that the ->resume hook could be called while the "suspended

> > state" structure is still not initialized: this could freeze the board

> > I guess.  

> 

> You should set the suspend hook as well, its a bug otherwise. There

> can be cases where people want the generic suspend routine to run at

> suspend but they want to reinitialize something during resume and so

> this code may make sense.

> 


Ok then, it makes sense.

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 190ea0dccb79..0a9ebf00be46 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -346,8 +346,14 @@  static int dt_cpufreq_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (data && data->have_governor_per_policy)
-		dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;
+	if (data) {
+		if (data->have_governor_per_policy)
+			dt_cpufreq_driver.flags |= CPUFREQ_HAVE_GOVERNOR_PER_POLICY;
+
+		dt_cpufreq_driver.resume = data->resume;
+		if (data->suspend)
+			dt_cpufreq_driver.suspend = data->suspend;
+	}
 
 	ret = cpufreq_register_driver(&dt_cpufreq_driver);
 	if (ret)
diff --git a/drivers/cpufreq/cpufreq-dt.h b/drivers/cpufreq/cpufreq-dt.h
index 54d774e46c43..d5aeea13433e 100644
--- a/drivers/cpufreq/cpufreq-dt.h
+++ b/drivers/cpufreq/cpufreq-dt.h
@@ -12,8 +12,13 @@ 
 
 #include <linux/types.h>
 
+struct cpufreq_policy;
+
 struct cpufreq_dt_platform_data {
 	bool have_governor_per_policy;
+
+	int (*suspend)(struct cpufreq_policy *policy);
+	int (*resume)(struct cpufreq_policy *policy);
 };
 
 #endif /* __CPUFREQ_DT_H__ */