[V2,2/2] cpufreq: dt: Implement online/offline() callbacks
diff mbox series

Message ID 8030961ef762018fc286e4d3d3a010638e900e53.1549955027.git.viresh.kumar@linaro.org
State Awaiting Upstream
Delegated to: Rafael Wysocki
Headers show
Series
  • cpufreq: Allow light-weight tear down and bring up of CPUs
Related show

Commit Message

Viresh Kumar Feb. 12, 2019, 7:06 a.m. UTC
Implement the light-weight tear down and bring up helpers to reduce the
amount of work to do on CPU offline/online operation.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Rafael J. Wysocki Feb. 12, 2019, 9:43 a.m. UTC | #1
On Tue, Feb 12, 2019 at 8:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Implement the light-weight tear down and bring up helpers to reduce the
> amount of work to do on CPU offline/online operation.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 7ba392911cd0..1aefaa1b0ca2 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>         return ret;
>  }
>
> +static int cpufreq_online(struct cpufreq_policy *policy)
> +{
> +       /* We did light-weight tear down earlier, nothing to do here */
> +       return 0;
> +}

I think you could avoid having to add this empty stub if the core
checked for both online and offline, that is

if (driver->offline || driver->online) {
    ret = driver->online ? driver->online(policy) : 0;
} else {
    ret = driver->init(policy);
}

or similar.

> +
> +static int cpufreq_offline(struct cpufreq_policy *policy)
> +{
> +       /*
> +        * Preserve policy->driver_data and don't free resources on light-weight
> +        * tear down.
> +        */
> +       return 0;
> +}
> +
>  static int cpufreq_exit(struct cpufreq_policy *policy)
>  {
>         struct private_data *priv = policy->driver_data;
> @@ -319,6 +334,8 @@ static struct cpufreq_driver dt_cpufreq_driver = {
>         .get = cpufreq_generic_get,
>         .init = cpufreq_init,
>         .exit = cpufreq_exit,
> +       .online = cpufreq_online,
> +       .offline = cpufreq_offline,
>         .name = "cpufreq-dt",
>         .attr = cpufreq_dt_attr,
>         .suspend = cpufreq_generic_suspend,
> --
> 2.20.1.321.g9e740568ce00
>
Viresh Kumar Feb. 12, 2019, 9:49 a.m. UTC | #2
On 12-02-19, 10:43, Rafael J. Wysocki wrote:
> On Tue, Feb 12, 2019 at 8:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Implement the light-weight tear down and bring up helpers to reduce the
> > amount of work to do on CPU offline/online operation.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> > index 7ba392911cd0..1aefaa1b0ca2 100644
> > --- a/drivers/cpufreq/cpufreq-dt.c
> > +++ b/drivers/cpufreq/cpufreq-dt.c
> > @@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >         return ret;
> >  }
> >
> > +static int cpufreq_online(struct cpufreq_policy *policy)
> > +{
> > +       /* We did light-weight tear down earlier, nothing to do here */
> > +       return 0;
> > +}
> 
> I think you could avoid having to add this empty stub if the core
> checked for both online and offline, that is
> 
> if (driver->offline || driver->online) {

This doesn't look great as all we should care about here is ->online()
and checking for offline as well looks a bit hacky.

>     ret = driver->online ? driver->online(policy) : 0;
> } else {
>     ret = driver->init(policy);
> }
> 
> or similar.

I also thought of a new flag like: CPUFREQ_LIGHT_WEIGHT_OFFLINE and
then we can get rid of both online/offline dummies but then thought of
starting with the dummy routines to begin with :)
Rafael J. Wysocki Feb. 12, 2019, 10:26 a.m. UTC | #3
On Tue, Feb 12, 2019 at 10:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-02-19, 10:43, Rafael J. Wysocki wrote:
> > On Tue, Feb 12, 2019 at 8:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Implement the light-weight tear down and bring up helpers to reduce the
> > > amount of work to do on CPU offline/online operation.
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >  drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> > > index 7ba392911cd0..1aefaa1b0ca2 100644
> > > --- a/drivers/cpufreq/cpufreq-dt.c
> > > +++ b/drivers/cpufreq/cpufreq-dt.c
> > > @@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> > >         return ret;
> > >  }
> > >
> > > +static int cpufreq_online(struct cpufreq_policy *policy)
> > > +{
> > > +       /* We did light-weight tear down earlier, nothing to do here */
> > > +       return 0;
> > > +}
> >
> > I think you could avoid having to add this empty stub if the core
> > checked for both online and offline, that is
> >
> > if (driver->offline || driver->online) {
>
> This doesn't look great as all we should care about here is ->online()
> and checking for offline as well looks a bit hacky.

That's a matter of the driver I/F definition.

You can require online to be there if offline is set and the other way
around (but then you should check if that's the case while registering
the driver) or you can allow just one of them to be provided for the
lightweight online/offline to be done.  In the latter case the
presence of the other callback plays the role of a flag.

Both are basically fine by me, but if you go for the former, please
add the checks to cpufreq_register_driver().

Patch
diff mbox series

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 7ba392911cd0..1aefaa1b0ca2 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -295,6 +295,21 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	return ret;
 }
 
+static int cpufreq_online(struct cpufreq_policy *policy)
+{
+	/* We did light-weight tear down earlier, nothing to do here */
+	return 0;
+}
+
+static int cpufreq_offline(struct cpufreq_policy *policy)
+{
+	/*
+	 * Preserve policy->driver_data and don't free resources on light-weight
+	 * tear down.
+	 */
+	return 0;
+}
+
 static int cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct private_data *priv = policy->driver_data;
@@ -319,6 +334,8 @@  static struct cpufreq_driver dt_cpufreq_driver = {
 	.get = cpufreq_generic_get,
 	.init = cpufreq_init,
 	.exit = cpufreq_exit,
+	.online = cpufreq_online,
+	.offline = cpufreq_offline,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_dt_attr,
 	.suspend = cpufreq_generic_suspend,