Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

Hi,

This isn't a very big patchset and this patch is very much required to
understand other patches and so please cc all people from other
list here as well..

On Wed, Apr 30, 2014 at 11:58 AM, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
> In the frequency table dts file, the frequencies are arranged in

Improve your logs a bit. Which dts file are you talking about here ?
How would anybody know that you are talking about exynos here?

Also, you shouldn't mention that here, just tell the kind of requirement
platforms may have. i.e. people may want to keep the opp list in the
same order in which it came from DT.

> descending order which maps 1 to 1 with other frequency parameter to
> be calculated and programmed in some registers.
> But the OPP library works by generating the frequencies in ascending
> order which breaks the above logic.
> So added OPP_TABLE_ORDER_DESCEND flag to consider descending order.

So, create three flags:
OPP_TABLE_ORDER_ASCENDING              0
OPP_TABLE_ORDER_DESCENDING            1
OPP_TABLE_ORDER_ORIGINAL                  2 (And use this for your case.)

> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
> ---
>  drivers/base/power/opp.c |   17 ++++++++++++++++-
>  include/linux/pm_opp.h   |    7 +++++--
>  2 files changed, 21 insertions(+), 3 deletions(-)

You are changing prototype of a function and so all other files which are using
this routine will break after this patch and we can't afford it as we
want git bisect
to work properly.

So, fix all platforms here in this patch only, i.e. part of 2/3 and
complete 3/3 should
have been merged into this one.

> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 2553867..ec7d553 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -18,6 +18,7 @@
>  #include <linux/cpufreq.h>
>  #include <linux/device.h>
>  #include <linux/list.h>
> +#include <linux/list_sort.h>
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
>  #include <linux/pm_opp.h>
> @@ -597,10 +598,21 @@ int dev_pm_opp_disable(struct device *dev, unsigned
> long freq)
>  EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
>
>  #ifdef CONFIG_CPU_FREQ
> +
> +static int opp_descend_cmp(void *priv, struct list_head *a,
> +                                        struct list_head *b)
> +{
> +        struct dev_pm_opp *ra = list_entry(a, struct dev_pm_opp, node);
> +        struct dev_pm_opp *rb = list_entry(b, struct dev_pm_opp, node);
> +
> +        return rb->rate - ra->rate;
> +}
> +
>  /**
>   * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device
>   * @dev:       device for which we do this operation
>   * @table:     Cpufreq table returned back to caller
> + * @flags:     OPP_TABLE_ORDER_DESCEND or zero
>   *
>   * Generate a cpufreq table for a provided device- this assumes that the
>   * opp list is already initialized and ready for usage.
> @@ -622,7 +634,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
>   * or in contexts where mutex locking cannot be used.
>   */
>  int dev_pm_opp_init_cpufreq_table(struct device *dev,
> -                           struct cpufreq_frequency_table **table)
> +               struct cpufreq_frequency_table **table, unsigned char flags)

You are targeting the wrong routine. Fix of_init_opp_table() instead and
things would work automatically then..

And please don't change prototype of dev_pm_opp_add() for now and
just define __dev_pm_opp_add() which will be called from
dev_pm_opp_add() and of_init_opp_table() with 'int order' parameter.

RE: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Jonghwan Choi]

Hi. Viresh Kumar
Your reply is so fast like Usain Bolt.

> So, create three flags:
> OPP_TABLE_ORDER_ASCENDING              0
> OPP_TABLE_ORDER_DESCENDING            1
> OPP_TABLE_ORDER_ORIGINAL                  2 (And use this for your case.)

-> Actually, I want to use OPP_TABLE_ORDER_DESCENDING.(Not
OPP_TABLE_ORDER_ORIGINAL.)
I think that it is enough to support both descending and ascending ordering
only.
The meaning of "ORIGIANL" Amit, said, when he(and I) writes a frequency in
dts file with ordering(Ascending or Descending). He(and I) want the
frequency to be register according to ordering.(Ascending or Descending).

I concerned that if we use ORIGINAL ordering, opp_find_freq_ceil/foor can be
broken.
(example, 1GH - 500MH - 800MHz - 200MHz - 600MHz)

Thanks~

Best Regars

> -----Original Message-----
> From: viresh.linux@gmail.com [mailto:viresh.linux@gmail.com] On Behalf Of
> Viresh Kumar
> Sent: Wednesday, April 30, 2014 5:25 PM
> To: Jonghwan Choi; Linux PM list
> Cc: open list; Rafael J. Wysocki; Len Brown; Amit Daniel Kachhap
> Subject: Re: [PATCH 1/3] PM / OPP: Add support for descending order for
> cpufreq table
> 
> Hi,
> 
> This isn't a very big patchset and this patch is very much required to
> understand other patches and so please cc all people from other list here
> as well..
> 
> On Wed, Apr 30, 2014 at 11:58 AM, Jonghwan Choi <jhbird.choi@samsung.com>
> wrote:
> > In the frequency table dts file, the frequencies are arranged in
> 
> Improve your logs a bit. Which dts file are you talking about here ?
> How would anybody know that you are talking about exynos here?
> 
> Also, you shouldn't mention that here, just tell the kind of requirement
> platforms may have. i.e. people may want to keep the opp list in the same
> order in which it came from DT.
> 
> > descending order which maps 1 to 1 with other frequency parameter to
> > be calculated and programmed in some registers.
> > But the OPP library works by generating the frequencies in ascending
> > order which breaks the above logic.
> > So added OPP_TABLE_ORDER_DESCEND flag to consider descending order.
> 
> So, create three flags:
> OPP_TABLE_ORDER_ASCENDING              0
> OPP_TABLE_ORDER_DESCENDING            1
> OPP_TABLE_ORDER_ORIGINAL                  2 (And use this for your case.)
> 
> > Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
> > ---
> >  drivers/base/power/opp.c |   17 ++++++++++++++++-
> >  include/linux/pm_opp.h   |    7 +++++--
> >  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> You are changing prototype of a function and so all other files which are
> using this routine will break after this patch and we can't afford it as
> we want git bisect to work properly.
> 
> So, fix all platforms here in this patch only, i.e. part of 2/3 and
> complete 3/3 should have been merged into this one.
> 
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index
> > 2553867..ec7d553 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/cpufreq.h>
> >  #include <linux/device.h>
> >  #include <linux/list.h>
> > +#include <linux/list_sort.h>
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/pm_opp.h>
> > @@ -597,10 +598,21 @@ int dev_pm_opp_disable(struct device *dev,
> > unsigned long freq)  EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
> >
> >  #ifdef CONFIG_CPU_FREQ
> > +
> > +static int opp_descend_cmp(void *priv, struct list_head *a,
> > +                                        struct list_head *b) {
> > +        struct dev_pm_opp *ra = list_entry(a, struct dev_pm_opp, node);
> > +        struct dev_pm_opp *rb = list_entry(b, struct dev_pm_opp,
> > +node);
> > +
> > +        return rb->rate - ra->rate;
> > +}
> > +
> >  /**
> >   * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a
> device
> >   * @dev:       device for which we do this operation
> >   * @table:     Cpufreq table returned back to caller
> > + * @flags:     OPP_TABLE_ORDER_DESCEND or zero
> >   *
> >   * Generate a cpufreq table for a provided device- this assumes that
> the
> >   * opp list is already initialized and ready for usage.
> > @@ -622,7 +634,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
> >   * or in contexts where mutex locking cannot be used.
> >   */
> >  int dev_pm_opp_init_cpufreq_table(struct device *dev,
> > -                           struct cpufreq_frequency_table **table)
> > +               struct cpufreq_frequency_table **table, unsigned char
> > + flags)
> 
> You are targeting the wrong routine. Fix of_init_opp_table() instead and
> things would work automatically then..
> 
> And please don't change prototype of dev_pm_opp_add() for now and just
> define __dev_pm_opp_add() which will be called from
> dev_pm_opp_add() and of_init_opp_table() with 'int order' parameter.

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 3 May 2014 05:46, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
> Hi. Viresh Kumar
> Your reply is so fast like Usain Bolt.

Heh, that's not true.. See how slow I was this time :)

>> So, create three flags:
>> OPP_TABLE_ORDER_ASCENDING              0
>> OPP_TABLE_ORDER_DESCENDING            1
>> OPP_TABLE_ORDER_ORIGINAL                  2 (And use this for your case.)
>
> -> Actually, I want to use OPP_TABLE_ORDER_DESCENDING.(Not
> OPP_TABLE_ORDER_ORIGINAL.)
> I think that it is enough to support both descending and ascending ordering
> only.
> The meaning of "ORIGIANL" Amit, said, when he(and I) writes a frequency in
> dts file with ordering(Ascending or Descending). He(and I) want the
> frequency to be register according to ordering.(Ascending or Descending).

But what if somebody doesn't have a ascending or descending list there? And
want to preserve the original list? That's why I recommended it.

> I concerned that if we use ORIGINAL ordering, opp_find_freq_ceil/foor can be
> broken.

I completely missed that earlier :) ..
It would be broken with descending one as well..

To skip the complexity of finding right freq associated with
"ORIGINAL" ordering,
lets concentrate on Ascending/Descending ordering for now.

So, this is what I would recommend now:
- Create two macros: OPP_TABLE_ORDER_ASCENDING and
  OPP_TABLE_ORDER_DESCENDING
- create of_init_opp_table_ordered(**, int order), order would be one of the
above two macros
- rename dev_pm_opp_add to __dev_pm_opp_add() and create a wrapper
over it: dev_pm_opp_add(), which would pass
OPP_TABLE_ORDER_ASCENDING to it by default and call it from
of_init_opp_table_ordered() like this: __dev_pm_opp_add(***, order)..

- Fix ceil/floor routines for these two cases.

--
viresh

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On 05/05/2014 12:54 AM, Viresh Kumar wrote:
> On 3 May 2014 05:46, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
>> Hi. Viresh Kumar
>> Your reply is so fast like Usain Bolt.
> 
> Heh, that's not true.. See how slow I was this time :)
> 
>>> So, create three flags:
>>> OPP_TABLE_ORDER_ASCENDING              0
>>> OPP_TABLE_ORDER_DESCENDING            1
>>> OPP_TABLE_ORDER_ORIGINAL                  2 (And use this for your case.)
>>
>> -> Actually, I want to use OPP_TABLE_ORDER_DESCENDING.(Not
>> OPP_TABLE_ORDER_ORIGINAL.)
>> I think that it is enough to support both descending and ascending ordering
>> only.
>> The meaning of "ORIGIANL" Amit, said, when he(and I) writes a frequency in
>> dts file with ordering(Ascending or Descending). He(and I) want the
>> frequency to be register according to ordering.(Ascending or Descending).
> 
> But what if somebody doesn't have a ascending or descending list there? And
> want to preserve the original list? That's why I recommended it.
> 
>> I concerned that if we use ORIGINAL ordering, opp_find_freq_ceil/foor can be
>> broken.
> 
> I completely missed that earlier :) ..
> It would be broken with descending one as well..
> 
> To skip the complexity of finding right freq associated with
> "ORIGINAL" ordering,
> lets concentrate on Ascending/Descending ordering for now.
> 
> So, this is what I would recommend now:
> - Create two macros: OPP_TABLE_ORDER_ASCENDING and
>   OPP_TABLE_ORDER_DESCENDING
> - create of_init_opp_table_ordered(**, int order), order would be one of the
> above two macros
> - rename dev_pm_opp_add to __dev_pm_opp_add() and create a wrapper
> over it: dev_pm_opp_add(), which would pass
> OPP_TABLE_ORDER_ASCENDING to it by default and call it from
> of_init_opp_table_ordered() like this: __dev_pm_opp_add(***, order)..
> 
> - Fix ceil/floor routines for these two cases.

With the brief history of the patch in linux-pm, I am unable to
understand why not just use ceil/floor routines to pick up data the
way you need it. It should not matter if we use an ordered list, or
some other weird organization inside the storage. There are already
accessors functions meant to precisely help with the case that is
being tried here.


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 5 May 2014 19:08, Nishanth Menon <nm@ti.com> wrote:
> With the brief history of the patch in linux-pm, I am unable to
> understand why not just use ceil/floor routines to pick up data the
> way you need it. It should not matter if we use an ordered list, or
> some other weird organization inside the storage. There are already
> accessors functions meant to precisely help with the case that is
> being tried here.

To be precise, for exynos they need the position of a frequency when
it is arranged in descending order. And they will simply write this position
in their clock controller later. For example, if frequencies are:
100 MHz, 200, 300, 400, 500, 600

Then they need to write 1 for 600, 2 for 500, 3 for 400, and so on..
I am not able to imaging how ceil/floor would help here.

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On Mon, May 5, 2014 at 9:14 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> To be precise, for exynos they need the position of a frequency when
> it is arranged in descending order. And they will simply write this position
> in their clock controller later. For example, if frequencies are:
> 100 MHz, 200, 300, 400, 500, 600
>
> Then they need to write 1 for 600, 2 for 500, 3 for 400, and so on..
> I am not able to imaging how ceil/floor would help here.

ceil and floor allows us to walk down the opp entries the direction we
want it to.
one can convert that data any way one wants it - especially when custom
mapping such as this is desired.

Regards,
Nishanth Menon

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 5 May 2014 19:53, Nishanth Menon <nm@ti.com> wrote:
> ceil and floor allows us to walk down the opp entries the direction we
> want it to.
> one can convert that data any way one wants it - especially when custom
> mapping such as this is desired.

Yeah, but doing that for every frequency transition is not right.
Otherwise they already have a solution, where they reverse order
of frequencies for their driver. exynos_sort_descend_freq_table().

Probably that's a better solution then :)

But, Jonghwan was probably trying to get this solved in the framework
only, in case anybody else needs it.

--
viresh

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On Mon, May 5, 2014 at 9:38 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 5 May 2014 19:53, Nishanth Menon <nm@ti.com> wrote:
>> ceil and floor allows us to walk down the opp entries the direction we
>> want it to.
>> one can convert that data any way one wants it - especially when custom
>> mapping such as this is desired.
>
> Yeah, but doing that for every frequency transition is not right.
> Otherwise they already have a solution, where they reverse order
> of frequencies for their driver. exynos_sort_descend_freq_table().
>
> Probably that's a better solution then :)
Such a map should probably be done at probe, unless we have strong
reasons why not to.

>
> But, Jonghwan was probably trying to get this solved in the framework
> only, in case anybody else needs it.
>
Lets keep things simple for the time being. ideally users of a
framework should not depend on internal organization of data.

Regards,
Nishanth Menon

RE: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Jonghwan Choi]

Hi  

My holiday is finished.

I implemented another cpufreq driver. And that driver also have to use exynos_sort_descend_freq_table().
Then exynos5440 and new cpufreq have a duplicate function.(exynos_sort_descend_freq_table().
So I want to solve it.

Thanks.~

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Monday, May 05, 2014 11:39 PM
> To: Nishanth Menon
> Cc: Jonghwan Choi; Linux PM list; open list; Rafael J. Wysocki; Len Brown;
> Amit Daniel Kachhap
> Subject: Re: [PATCH 1/3] PM / OPP: Add support for descending order for
> cpufreq table
> 
> On 5 May 2014 19:53, Nishanth Menon <nm@ti.com> wrote:
> > ceil and floor allows us to walk down the opp entries the direction we
> > want it to.
> > one can convert that data any way one wants it - especially when
> > custom mapping such as this is desired.
> 
> Yeah, but doing that for every frequency transition is not right.
> Otherwise they already have a solution, where they reverse order of
> frequencies for their driver. exynos_sort_descend_freq_table().
> 
> Probably that's a better solution then :)
> 
> But, Jonghwan was probably trying to get this solved in the framework only,
> in case anybody else needs it.
> 
> --
> viresh

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On Tue, May 6, 2014 at 6:43 PM, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
> Hi

Please dont top post. it is usually frowned upon.

>
> My holiday is finished.
>
> I implemented another cpufreq driver. And that driver also have to use exynos_sort_descend_freq_table().
> Then exynos5440 and new cpufreq have a duplicate function.(exynos_sort_descend_freq_table().
> So I want to solve it.

As discussed in the thread, creating stuff that are common into a
common file, and even isolating this into cpufreq specific solution
might be good.

[1] now moves that entire logic of table creation to be cpufreq
specific - we could consider modifier functions to them.

In some quick tests by reversing table [2], I cant see any difference
in behavior in ascending[3] or descending[4] order of the cpufreq
table.

So, we could do [2] as default as well, if it is determined to impact
no one else making any form of assumptions on table ordering - but it
might be preferable for drivers not to depend on framework ordering of
data as things could change in the future.

[1] https://patchwork.kernel.org/patch/4115141/ +
https://patchwork.kernel.org/patch/4115101/
[2] http://slexy.org/view/s21HyCUhXK
[3] http://slexy.org/view/s202xTUG59
[4] http://slexy.org/view/s20ewFa6PW


Regards,
Nishanth Menon

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 7 May 2014 06:30, Nishanth Menon <nm@ti.com> wrote:
> So, we could do [2] as default as well, if it is determined to impact
> no one else making any form of assumptions on table ordering - but it
> might be preferable for drivers not to depend on framework ordering of
> data as things could change in the future.

Exactly and that's what we discussed earlier. I don't want to change the
default behavior at all, as somebody may request the ascending order
tomorrow :)

@Jonghwan: Please consider doing this:
- Don't play with the order of frequencies in table.
- Instead initialize .driver_data filed with values that you need to write
in the registers for all frequencies. i.e. 0 for highest frequency and
FREQ_COUNT-1 for lowest one.

RE: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Jonghwan Choi]

> @Jonghwan: Please consider doing this:
> - Don't play with the order of frequencies in table.
> - Instead initialize .driver_data filed with values that you need to write
> in the registers for all frequencies. i.e. 0 for highest frequency and
> FREQ_COUNT-1 for lowest one.

-> For that, I changed like this.
For initializing .driver_data, I changed dev_pm_opp_init_cpufreq_table function().


--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -622,12 +622,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
  * or in contexts where mutex locking cannot be used.
  */
 int dev_pm_opp_init_cpufreq_table(struct device *dev,
-                           struct cpufreq_frequency_table **table)
+               struct cpufreq_frequency_table **table, int order)
 {
        struct device_opp *dev_opp;
        struct dev_pm_opp *opp;
        struct cpufreq_frequency_table *freq_table;
-       int i = 0;
+       int i = 0, index = 0;

        /* Pretend as if I am an updater */
        mutex_lock(&dev_opp_list_lock);
@@ -649,16 +649,22 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
                return -ENOMEM;
        }

+       if (OPP_TABLE_ORDER_DESCENDING == order)
+               index = dev_pm_opp_get_opp_count(dev) - 1;
+
        list_for_each_entry(opp, &dev_opp->opp_list, node) {
                if (opp->available) {
-                       freq_table[i].driver_data = i;
+                       if (OPP_TABLE_ORDER_DESCENDING == order)
+                               freq_table[i].driver_data = index--;
+                       else
+                               freq_table[i].driver_data = index++;
                        freq_table[i].frequency = opp->rate / 1000;
                        i++;
                }
        }
        mutex_unlock(&dev_opp_list_lock);

-       freq_table[i].driver_data = i;
+       freq_table[i].driver_data = index;
        freq_table[i].frequency = CPUFREQ_TABLE_END;

        *table = &freq_table[0];


Is it acceptiable?

Thanks

Best Regards

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On Wed, May 7, 2014 at 8:22 PM, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
>> @Jonghwan: Please consider doing this:
>> - Don't play with the order of frequencies in table.
>> - Instead initialize .driver_data filed with values that you need to write
>> in the registers for all frequencies. i.e. 0 for highest frequency and
>> FREQ_COUNT-1 for lowest one.
>
> -> For that, I changed like this.
> For initializing .driver_data, I changed dev_pm_opp_init_cpufreq_table function().
>
>
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -622,12 +622,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
>   * or in contexts where mutex locking cannot be used.
>   */
>  int dev_pm_opp_init_cpufreq_table(struct device *dev,
> -                           struct cpufreq_frequency_table **table)
> +               struct cpufreq_frequency_table **table, int order)
>  {
>         struct device_opp *dev_opp;
>         struct dev_pm_opp *opp;
>         struct cpufreq_frequency_table *freq_table;
> -       int i = 0;
> +       int i = 0, index = 0;
>
>         /* Pretend as if I am an updater */
>         mutex_lock(&dev_opp_list_lock);
> @@ -649,16 +649,22 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
>                 return -ENOMEM;
>         }
>
> +       if (OPP_TABLE_ORDER_DESCENDING == order)
> +               index = dev_pm_opp_get_opp_count(dev) - 1;
> +
>         list_for_each_entry(opp, &dev_opp->opp_list, node) {
>                 if (opp->available) {
> -                       freq_table[i].driver_data = i;
> +                       if (OPP_TABLE_ORDER_DESCENDING == order)
> +                               freq_table[i].driver_data = index--;
> +                       else
> +                               freq_table[i].driver_data = index++;
>                         freq_table[i].frequency = opp->rate / 1000;
>                         i++;
>                 }
>         }
>         mutex_unlock(&dev_opp_list_lock);
>
> -       freq_table[i].driver_data = i;
> +       freq_table[i].driver_data = index;
>         freq_table[i].frequency = CPUFREQ_TABLE_END;
>
>         *table = &freq_table[0];
>
>
> Is it acceptiable?

Personally, I feel that filling up driver_data should be left to the
driver(caller of dev_pm_opp_init_cpufreq_table). for example providing
a function pointer which decides what that value should be (be it
index or some magical register value).. Viresh might have better
opinions.

Regards,
Nishanth Menon

RE: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Jonghwan Choi]

I believe that 3 item is required for DVFS. Those are frequency, voltage, divider value.
Currently OPP only supports voltage and frequency. 
So some cpufreq and devfreq driver get a divider value from struct divider table.

How about adding that divider value into struct dev_pm_opp like this;

struct dev_pm_opp {
        struct list_head node;

        bool available;
        unsigned long rate;
        unsigned long u_volt;
        unsigned int ctl[2]; // Added

        struct device_opp *dev_opp;
        struct rcu_head head;
};
In my test, it works very wel..

I got a this idea from _PCT in acpi spec.

Then we can remove a lot of code related to divide table. And we also can solve this problem.

Thanks

Best Regarfs.


> -----Original Message-----
> From: menon.nishanth@gmail.com [mailto:menon.nishanth@gmail.com] On Behalf
> Of Nishanth Menon
> Sent: Thursday, May 08, 2014 10:56 AM
> To: Jonghwan Choi
> Cc: Viresh Kumar; Linux PM list; open list; Rafael J. Wysocki; Len Brown;
> Amit Daniel Kachhap
> Subject: Re: [PATCH 1/3] PM / OPP: Add support for descending order for
> cpufreq table
> 
> On Wed, May 7, 2014 at 8:22 PM, Jonghwan Choi <jhbird.choi@samsung.com>
> wrote:
> >> @Jonghwan: Please consider doing this:
> >> - Don't play with the order of frequencies in table.
> >> - Instead initialize .driver_data filed with values that you need to
> >> write in the registers for all frequencies. i.e. 0 for highest
> >> frequency and
> >> FREQ_COUNT-1 for lowest one.
> >
> > -> For that, I changed like this.
> > For initializing .driver_data, I changed dev_pm_opp_init_cpufreq_table
> function().
> >
> >
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -622,12 +622,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
> >   * or in contexts where mutex locking cannot be used.
> >   */
> >  int dev_pm_opp_init_cpufreq_table(struct device *dev,
> > -                           struct cpufreq_frequency_table **table)
> > +               struct cpufreq_frequency_table **table, int order)
> >  {
> >         struct device_opp *dev_opp;
> >         struct dev_pm_opp *opp;
> >         struct cpufreq_frequency_table *freq_table;
> > -       int i = 0;
> > +       int i = 0, index = 0;
> >
> >         /* Pretend as if I am an updater */
> >         mutex_lock(&dev_opp_list_lock); @@ -649,16 +649,22 @@ int
> > dev_pm_opp_init_cpufreq_table(struct device *dev,
> >                 return -ENOMEM;
> >         }
> >
> > +       if (OPP_TABLE_ORDER_DESCENDING == order)
> > +               index = dev_pm_opp_get_opp_count(dev) - 1;
> > +
> >         list_for_each_entry(opp, &dev_opp->opp_list, node) {
> >                 if (opp->available) {
> > -                       freq_table[i].driver_data = i;
> > +                       if (OPP_TABLE_ORDER_DESCENDING == order)
> > +                               freq_table[i].driver_data = index--;
> > +                       else
> > +                               freq_table[i].driver_data = index++;
> >                         freq_table[i].frequency = opp->rate / 1000;
> >                         i++;
> >                 }
> >         }
> >         mutex_unlock(&dev_opp_list_lock);
> >
> > -       freq_table[i].driver_data = i;
> > +       freq_table[i].driver_data = index;
> >         freq_table[i].frequency = CPUFREQ_TABLE_END;
> >
> >         *table = &freq_table[0];
> >
> >
> > Is it acceptiable?
> 
> Personally, I feel that filling up driver_data should be left to the
> driver(caller of dev_pm_opp_init_cpufreq_table). for example providing a
> function pointer which decides what that value should be (be it index or
> some magical register value).. Viresh might have better opinions.
> 
> Regards,
> Nishanth Menon

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 8 May 2014 07:37, Jonghwan Choi <jhbird.choi@samsung.com> wrote:

As asked earlier by Nishanth:

- Avoid top-posting (the practice of putting your answer above the quoted
  text you are responding to).  It makes your response harder to read and
  makes a poor impression.

Reference: Documentation/development-process/2.Process

> I believe that 3 item is required for DVFS. Those are frequency, voltage, divider value.

Not necessarily. People may need a multiplier as well or some other
configuration and so this stuff was left for drivers to implement.

> How about adding that divider value into struct dev_pm_opp like this;

Wouldn't work for all and so NAK.

> struct dev_pm_opp {
>         struct list_head node;
>
>         bool available;
>         unsigned long rate;
>         unsigned long u_volt;
>         unsigned int ctl[2]; // Added
>
>         struct device_opp *dev_opp;
>         struct rcu_head head;
> };

Always paste a diff, its impossible to read this :(

> In my test, it works very wel..

Working isn't enough :)

You don't have a complicated list of dividers, these are simple
values from 0 to total-num-of-freq -1 and that can be handled very
easily in your code.. Please do it there.

RE: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Jonghwan Choi]

On 8 May 2014 2:56 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> 
> Not necessarily. People may need a multiplier as well or some other
> configuration and so this stuff was left for drivers to implement.

-> In exynos cpufreq driver, if we want to support more frequency, then 
we have to describe frequency information in dts file and have to change exynos cpufreq
 driver file also(For adding divider value). 
But if dev_pm_opp has a divider value for DVFS,when we want to support more frequency 
we have only to change the dts file.

I think that it is easy to maintain cpufreq driver and very convenient.
(Can be applied to devfreq)

This is a example (exynos4210 doesn't support DT.)

--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -64,6 +64,7 @@ struct dev_pm_opp {
        bool available;
        unsigned long rate;
        unsigned long u_volt;
+       unsigned int ctl[2];

        struct device_opp *dev_opp;
        struct rcu_head head;
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index cacf614..007a411 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -81,6 +81,24 @@
                interrupts = <2 2>, <3 2>;
        };

+       cpufreq@160000 {
+               compatible = "samsung,exynos7842-cpufreq";
+               reg = <0x160000 0x1000>;
+               interrupts = <0 57 0>;
+               operating-points = <
+                               /* KHz    uV    clkdiv0 clkdiv1 */
+                               1500000 1100000 0x11111 0x11111
+                               1400000 1075000 0x22223 0x22222
+                               1300000 1050000 0x33333 0x33333
+                               1200000 1025000 0x44444 0x44444
+                               1100000 1000000 0x55555 0x55555
+                               1000000 975000  0x66666 0x66666
+                               900000  950000  0x77777 0x77777
+                               800000  925000  0x88888 0x88888
+               >;
+       };
+
+
        pinctrl_0: pinctrl@11400000 {
                compatible = "samsung,exynos4210-pinctrl";
                reg = <0x11400000 0x1000>;


Thanks

Best Regards.

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 9 May 2014 06:39, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
> -> In exynos cpufreq driver, if we want to support more frequency, then

Don't add "->" to your replies, it doesn't make it more readable but less.

> we have to describe frequency information in dts file and have to change exynos cpufreq
>  driver file also(For adding divider value).

Why? So, as far as I got it your dividers are nothing but 0,1,2...
i.e.
Freqs: 400  500  600  700  800
div:      4      3      2      1      0

right? That's what you are doing in exynos5440. So just add this in your
probe after doing: dev_pm_opp_init_cpufreq_table

for(i = 0; all-available-freqs; i++)
    dvfs_info->freq_table[i].driver_data = dvfs_info->freq_count - i;

And this will work with changes in dts files.

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[jonghwan Choi]

On Thu, May 8, 2014 at 11:00 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> Why? So, as far as I got it your dividers are nothing but 0,1,2...
> i.e.
> Freqs: 400  500  600  700  800
> div:      4      3      2      1      0
>
> right? That's what you are doing in exynos5440. So just add this in your
> probe after doing: dev_pm_opp_init_cpufreq_table
>
> for(i = 0; all-available-freqs; i++)
>     dvfs_info->freq_table[i].driver_data = dvfs_info->freq_count - i;
>
> And this will work with changes in dts files.

I am sorry
I couldn’t provide detailed information about this suggestion.
This suggestion is not for exynos5440. This is for exynos4210,
exynos4x12 and exynos5250.
(But this can be applied to exynos5440 also)
I want to make exynos cpufreq driver simple.
There are exynos-cpufreq.c, exynos4210-cpufreq.c exynos4x12-cpufreq.
exynos5250-cpufreq.c for exynos soc.
And exynos4210-cpufreq.c, exynos4x12 and exynos5250-cpufreq. c has a
clk divider table for each frequency.

example) exynos4210-cpufreq.c
static struct apll_freq apll_freq_4210[] = {
        /*
         * values:
         * freq
         * clock divider for CORE, COREM0, COREM1, PERIPH, ATB,
PCLK_DBG, APLL, RESERVED
         * clock divider for COPY, HPM, RESERVED
         * PLL M, P, S
         */
        APLL_FREQ(1200, 0, 3, 7, 3, 4, 1, 7, 0, 5, 0, 0, 150, 3, 1),
        APLL_FREQ(1000, 0, 3, 7, 3, 4, 1, 7, 0, 4, 0, 0, 250, 6, 1),
        APLL_FREQ(800,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 200, 6, 1),
        APLL_FREQ(500,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 250, 6, 2),
        APLL_FREQ(200,  0, 1, 3, 1, 3, 1, 0, 0, 3, 0, 0, 200, 6, 3),
};

If we can pass this clk divider value to exynos cpufreq driver through
DT, we can remove most of exynosxxxx-cpufreq.c files/codes. And when
new frequency is added/removed or new soc is released, for supporting
dvfs we have only to describe frequency, voltage and divider value in
dts file.

Thanks

Best Regards.

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Nishanth Menon]

On 05/09/2014 06:59 AM, jonghwan Choi wrote:
> On Thu, May 8, 2014 at 11:00 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
>> Why? So, as far as I got it your dividers are nothing but 0,1,2...
>> i.e.
>> Freqs: 400  500  600  700  800
>> div:      4      3      2      1      0
>>
>> right? That's what you are doing in exynos5440. So just add this in your
>> probe after doing: dev_pm_opp_init_cpufreq_table
>>
>> for(i = 0; all-available-freqs; i++)
>>     dvfs_info->freq_table[i].driver_data = dvfs_info->freq_count - i;
>>
>> And this will work with changes in dts files.
> 
> I am sorry
> I couldn’t provide detailed information about this suggestion.
> This suggestion is not for exynos5440. This is for exynos4210,
> exynos4x12 and exynos5250.
> (But this can be applied to exynos5440 also)
> I want to make exynos cpufreq driver simple.
> There are exynos-cpufreq.c, exynos4210-cpufreq.c exynos4x12-cpufreq.
> exynos5250-cpufreq.c for exynos soc.
> And exynos4210-cpufreq.c, exynos4x12 and exynos5250-cpufreq. c has a
> clk divider table for each frequency.
> 
> example) exynos4210-cpufreq.c
> static struct apll_freq apll_freq_4210[] = {
>         /*
>          * values:
>          * freq
>          * clock divider for CORE, COREM0, COREM1, PERIPH, ATB,
> PCLK_DBG, APLL, RESERVED
>          * clock divider for COPY, HPM, RESERVED
>          * PLL M, P, S
>          */
>         APLL_FREQ(1200, 0, 3, 7, 3, 4, 1, 7, 0, 5, 0, 0, 150, 3, 1),
>         APLL_FREQ(1000, 0, 3, 7, 3, 4, 1, 7, 0, 4, 0, 0, 250, 6, 1),
>         APLL_FREQ(800,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 200, 6, 1),
>         APLL_FREQ(500,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 250, 6, 2),
>         APLL_FREQ(200,  0, 1, 3, 1, 3, 1, 0, 0, 3, 0, 0, 200, 6, 3),
> };
> 
> If we can pass this clk divider value to exynos cpufreq driver through
> DT, we can remove most of exynosxxxx-cpufreq.c files/codes. And when
> new frequency is added/removed or new soc is released, for supporting
> dvfs we have only to describe frequency, voltage and divider value in
> dts file.

Have you considered the option of having a clock driver which can
decide the divider (based on dts OR index or whatever)?

example: you could do clk_set_rate(apll, rate);
and instead of implementing clock divider programmation inside cpufreq
driver, you let corresponding clock driver do it for you. that allows
you to reuse clock driver with various parameters needed for your SoC
variations. IMHO, we are trying to solve a problem meant to be solved
in clock framework instead of within cpufreq.

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[jonghwan Choi]

On Fri, May 9, 2014 at 6:23 AM, Nishanth Menon <nm@ti.com> wrote:
> Have you considered the option of having a clock driver which can
> decide the divider (based on dts OR index or whatever)?
>
> example: you could do clk_set_rate(apll, rate);
> and instead of implementing clock divider programmation inside cpufreq
> driver, you let corresponding clock driver do it for you. that allows
> you to reuse clock driver with various parameters needed for your SoC
> variations. IMHO, we are trying to solve a problem meant to be solved
> in clock framework instead of within cpufreq.


I already considered it.
(But it only passes on  what cpufreq driver has to do to clock framework.
For changing clock rate, if changing operation just divides a rate of
parent it can be solved easily
But exycpufreq driver is  more complicated.

Previously, to change frequency, pll value and clk divider value were
changed in cpufreq driver.
Later someone moved the code which changes pll value to clock framework.
In there, pll values are maintained as table per frequency. And if
frequency is added/removed, values of
pll table should be changed.
when we change the pll value through clk_set_rate, internally  to find
proper pll value,  pll table is searched.
If proper pll value is found, that value is written into the register)

My suggestion is that all these change details should be removed
according to adding/removing frequency.
I believe that cpufreq driver just writes a specific value per
frequency  into the register for dvfs(Maybe other work is also needed)

If we just describe the specific value per frequency in dts file, the
driver will get that information through DT, and use it for DVFS.)
Then when a new chip is  released(if the chip has the same h/w
interface - register map), we only have to do as above.


Thanks

Best Regards

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On 11 May 2014 17:08, jonghwan Choi <jhbird.choi@gmail.com> wrote:
> I already considered it.
> (But it only passes on  what cpufreq driver has to do to clock framework.
> For changing clock rate, if changing operation just divides a rate of
> parent it can be solved easily
> But exycpufreq driver is  more complicated.
>
> Previously, to change frequency, pll value and clk divider value were
> changed in cpufreq driver.
> Later someone moved the code which changes pll value to clock framework.
> In there, pll values are maintained as table per frequency. And if
> frequency is added/removed, values of
> pll table should be changed.
> when we change the pll value through clk_set_rate, internally  to find
> proper pll value,  pll table is searched.
> If proper pll value is found, that value is written into the register)
>
> My suggestion is that all these change details should be removed
> according to adding/removing frequency.
> I believe that cpufreq driver just writes a specific value per
> frequency  into the register for dvfs(Maybe other work is also needed)
>
> If we just describe the specific value per frequency in dts file, the
> driver will get that information through DT, and use it for DVFS.)
> Then when a new chip is  released(if the chip has the same h/w
> interface - register map), we only have to do as above.

We also want to make your life simple, but adding this field to OPP
table isn't the right approach for sure.

Can't you calculate the divider values at run time based on a frequency?
I think it should work. That way you can just code these calculations
in clock driver and things would work smoothly..

If there are problems, tell us what they are and we will try to find some
solution for you. .

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Viresh Kumar]

On Thu, May 8, 2014 at 7:25 AM, Nishanth Menon <nm@ti.com> wrote:
>> Is it acceptiable?
>
> Personally, I feel that filling up driver_data should be left to the
> driver(caller of dev_pm_opp_init_cpufreq_table).

Exactly, and I never advised Jonghwan to update the common routine
for this. I wanted him to handle this in his driver only :)

Re: [PATCH 1/3] PM / OPP: Add support for descending order for cpufreq table

[Sudeep Holla]

On Mon, May 5, 2014 at 3:14 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 5 May 2014 19:08, Nishanth Menon <nm@ti.com> wrote:
>> With the brief history of the patch in linux-pm, I am unable to
>> understand why not just use ceil/floor routines to pick up data the
>> way you need it. It should not matter if we use an ordered list, or
>> some other weird organization inside the storage. There are already
>> accessors functions meant to precisely help with the case that is
>> being tried here.
>
> To be precise, for exynos they need the position of a frequency when
> it is arranged in descending order. And they will simply write this position
> in their clock controller later. For example, if frequencies are:
> 100 MHz, 200, 300, 400, 500, 600
>
> Then they need to write 1 for 600, 2 for 500, 3 for 400, and so on..
> I am not able to imaging how ceil/floor would help here.

It's better to have these mapping in the users of this framework IMO.
Assuming these OPPs come from DT, what happens if entries in the DT are
updated (added/removed), won't the mapping go wrong then ?

Regards,
Sudeep