[v1,3/3] PM / devfreq: Add required OPPs support to passive governor
diff mbox series

Message ID 20190622003449.33707-4-saravanak@google.com
State Superseded
Headers show
Series
  • Add required-opps support to devfreq passive gov
Related show

Commit Message

Saravana Kannan June 22, 2019, 12:34 a.m. UTC
Look at the required OPPs of the "parent" device to determine the OPP that
is required from the slave device managed by the passive governor. This
allows having mappings between a parent device and a slave device even when
they don't have the same number of OPPs.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/devfreq/governor_passive.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Chanwoo Choi June 22, 2019, noon UTC | #1
Hi,

Absolutely, I agree this approach.
But, I add some comments on below. please check them.

2019년 6월 22일 (토) 오전 9:36, Saravana Kannan <saravanak@google.com>님이 작성:
>
> Look at the required OPPs of the "parent" device to determine the OPP that
> is required from the slave device managed by the passive governor. This
> allows having mappings between a parent device and a slave device even when
> they don't have the same number of OPPs.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/devfreq/governor_passive.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e..bd4a98bb15b1 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -21,8 +21,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>         struct devfreq_passive_data *p_data
>                         = (struct devfreq_passive_data *)devfreq->data;
>         struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> +       struct opp_table *opp_table = NULL, *c_opp_table = NULL;

In this function, the base device is passive devfreq device.
So, I think that better to define the 'parent_opp_table' instead of 'opp_table'
indicating the OPP table of parent devfreq device. And better to define
just 'opp_table' instead of 'c_opp_table' indicating the passive devfreq device.
- opp_table -> parent_opp_table
- c_opp_table -> opp_table

>         unsigned long child_freq = ULONG_MAX;
> -       struct dev_pm_opp *opp;
> +       struct dev_pm_opp *opp = NULL, *c_opp = NULL;

Ditto. I think that better to define the variables as following:
- opp -> parent_opp
- c_cpp -> opp

>         int i, count, ret = 0;
>
>         /*
> @@ -65,7 +66,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>                 goto out;
>         }
>
> -       dev_pm_opp_put(opp);
> +       opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);

devfreq_passive_get_target_freq() is called frequently for DVFS support.
I think that you have to add 'struct opp_table *opp_table' instance to
'struct devfreq'
and then get 'opp_table' instance in the devfreq_add_device().

devfreq_add_device() already get the OPP information by using
dev_pm_opp_get_suspend_opp_freq().
You can add following code nearby dev_pm_opp_get_suspend_opp_freq() in
devfreq_add_device().
- devfreq->opp_table = dev_pm_opp_get_opp_table(dev);


> +       if (IS_ERR_OR_NULL(opp_table)) {
> +               ret = PTR_ERR(opp_table);
> +               goto out;
> +       }
> +
> +       c_opp_table = dev_pm_opp_get_opp_table(devfreq->dev.parent);
> +       if (!IS_ERR_OR_NULL(c_opp_table))
> +               c_opp = dev_pm_opp_xlate_opp(opp_table, c_opp_table, opp);
> +       if (c_opp) {
> +               *freq = dev_pm_opp_get_freq(c_opp);
> +               dev_pm_opp_put(c_opp);
> +               goto out;
> +       }
>
>         /*
>          * Get the OPP table's index of decided freqeuncy by governor
> @@ -92,6 +106,13 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>         *freq = child_freq;
>
>  out:
> +       if (!IS_ERR_OR_NULL(opp_table))
> +               dev_pm_opp_put_opp_table(opp_table);
> +       if (!IS_ERR_OR_NULL(c_opp_table))
> +               dev_pm_opp_put_opp_table(c_opp_table);
> +       if (!IS_ERR_OR_NULL(opp))
> +               dev_pm_opp_put(opp);
> +
>         return ret;
>  }
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
Saravana Kannan June 22, 2019, 9:45 p.m. UTC | #2
On Sat, Jun 22, 2019 at 5:01 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> Absolutely, I agree this approach.

Thanks!

> But, I add some comments on below. please check them.
>
> 2019년 6월 22일 (토) 오전 9:36, Saravana Kannan <saravanak@google.com>님이 작성:
> >
> > Look at the required OPPs of the "parent" device to determine the OPP that
> > is required from the slave device managed by the passive governor. This
> > allows having mappings between a parent device and a slave device even when
> > they don't have the same number of OPPs.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/devfreq/governor_passive.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> > index 3bc29acbd54e..bd4a98bb15b1 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -21,8 +21,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> >         struct devfreq_passive_data *p_data
> >                         = (struct devfreq_passive_data *)devfreq->data;
> >         struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> > +       struct opp_table *opp_table = NULL, *c_opp_table = NULL;
>
> In this function, the base device is passive devfreq device.
> So, I think that better to define the 'parent_opp_table' instead of 'opp_table'
> indicating the OPP table of parent devfreq device. And better to define
> just 'opp_table' instead of 'c_opp_table' indicating the passive devfreq device.
> - opp_table -> parent_opp_table
> - c_opp_table -> opp_table

Sounds good. I did it that way at first, but I wanted to keep the diff
simple in my first patch series. So renamed it. I can do the rename
that you suggested. Makes sense to me.

> >         unsigned long child_freq = ULONG_MAX;
> > -       struct dev_pm_opp *opp;
> > +       struct dev_pm_opp *opp = NULL, *c_opp = NULL;
>
> Ditto. I think that better to define the variables as following:
> - opp -> parent_opp
> - c_cpp -> opp

Will do.

>
> >         int i, count, ret = 0;
> >
> >         /*
> > @@ -65,7 +66,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> >                 goto out;
> >         }
> >
> > -       dev_pm_opp_put(opp);
> > +       opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);
>
> devfreq_passive_get_target_freq() is called frequently for DVFS support.
> I think that you have to add 'struct opp_table *opp_table' instance to
> 'struct devfreq'
> and then get 'opp_table' instance in the devfreq_add_device().

Sounds good. I had wanted to do that anyway, but didn't think it was
part of this series. But I can add that change to this series.

> devfreq_add_device() already get the OPP information by using
> dev_pm_opp_get_suspend_opp_freq().
> You can add following code nearby dev_pm_opp_get_suspend_opp_freq() in
> devfreq_add_device().
> - devfreq->opp_table = dev_pm_opp_get_opp_table(dev);

Will do something like that.

I'll send out an updated patch series on Monday or Tuesday. Hopefully
Viresh would have replied by then to give his opinion on whether this
is okay by him.

Thanks,
Saravana

Patch
diff mbox series

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 3bc29acbd54e..bd4a98bb15b1 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -21,8 +21,9 @@  static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	struct devfreq_passive_data *p_data
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
+	struct opp_table *opp_table = NULL, *c_opp_table = NULL;
 	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp = NULL, *c_opp = NULL;
 	int i, count, ret = 0;
 
 	/*
@@ -65,7 +66,20 @@  static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 		goto out;
 	}
 
-	dev_pm_opp_put(opp);
+	opp_table = dev_pm_opp_get_opp_table(parent_devfreq->dev.parent);
+	if (IS_ERR_OR_NULL(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto out;
+	}
+
+	c_opp_table = dev_pm_opp_get_opp_table(devfreq->dev.parent);
+	if (!IS_ERR_OR_NULL(c_opp_table))
+		c_opp = dev_pm_opp_xlate_opp(opp_table, c_opp_table, opp);
+	if (c_opp) {
+		*freq = dev_pm_opp_get_freq(c_opp);
+		dev_pm_opp_put(c_opp);
+		goto out;
+	}
 
 	/*
 	 * Get the OPP table's index of decided freqeuncy by governor
@@ -92,6 +106,13 @@  static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	*freq = child_freq;
 
 out:
+	if (!IS_ERR_OR_NULL(opp_table))
+		dev_pm_opp_put_opp_table(opp_table);
+	if (!IS_ERR_OR_NULL(c_opp_table))
+		dev_pm_opp_put_opp_table(c_opp_table);
+	if (!IS_ERR_OR_NULL(opp))
+		dev_pm_opp_put(opp);
+
 	return ret;
 }