| Message ID | 031b87c0dd05625c83ac920a8da661e72afb4b02.1524549644.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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.
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?
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.
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
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__ */
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(-)