[2/2] cpufreq: cpu0: Extend support beyond CPU0

Commit Message

Stephen Boyd June 27, 2014, 12:06 a.m. UTC

On 06/26/14 03:52, Viresh Kumar wrote:
> On 26 June 2014 00:32, Stephen Boyd <[e-mail]> wrote:
>> I don't think this driver should be using regulator_get_optional() (Mark
>> B. please correct me if I'm wrong). I doubt a supply is actually
>> optional for CPUs, just some DTs aren't specifying them. In those cases,
>> the regulator core will insert a dummy supply and the code will work
>> without having to check for probe defer and error pointers.
> Hi Stephen,
>
> Thanks for your comments.
>
> Leaving the above one, I have tried to fix all you mentioned. And it surely
> looks much better now.
>
> I would like to wait for a day or two before sending V2, as people might
> be reviewing it and the above issue is still wide open..
>
> But in case you wanna test it (completely changed I must say, but
> for good), its here:
>
> git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/cpu0-krait-v2

I gave it a spin. It looks mostly good except for the infinite loop:


which I hope is ok.

Finally, checking for equivalent pointers from clk_get() will work now,
but it isn't future-proof if/when the clock framework starts returning
dynamically allocated clock pointers for each clk_get() invocation.
Maybe we need a function in the common clock framework that tells us if
the clocks are the same either via DT or by taking two clock pointers?

Comments

Mike Turquette June 27, 2014, 1:53 a.m. UTC | #1

Quoting Stephen Boyd (2014-06-26 17:06:00)
> Finally, checking for equivalent pointers from clk_get() will work now,

Please don't do that. Even though it works for the current
implementation, comparing those pointers from a driver violates how
clkdev is supposed to work. The pointer returned by clk_get should only
be dereferenced by a driver to check if it is an error code. Anything
besides an error code is no business of the driver.

> but it isn't future-proof if/when the clock framework starts returning
> dynamically allocated clock pointers for each clk_get() invocation.
> Maybe we need a function in the common clock framework that tells us if
> the clocks are the same either via DT or by taking two clock pointers?

I looked through the patch briefly and did not see why we would need to
do this. Any hint?

Thanks,
Mike

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar June 27, 2014, 2:15 a.m. UTC | #2

On 27 June 2014 07:23, Mike Turquette <[e-mail]> wrote:
>> but it isn't future-proof if/when the clock framework starts returning
>> dynamically allocated clock pointers for each clk_get() invocation.
>> Maybe we need a function in the common clock framework that tells us if
>> the clocks are the same either via DT or by taking two clock pointers?
>
> I looked through the patch briefly and did not see why we would need to
> do this. Any hint?

We want to know which CPUs are sharing clock line, so that we can
fill affected-cpus field of cpufreq core.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar June 27, 2014, 2:26 a.m. UTC | #3

On 27 June 2014 05:36, Stephen Boyd <[e-mail]> wrote:
> I gave it a spin. It looks mostly good except for the infinite loop:

Great !!

> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> index b7ee67c4d1c0..6744321ae33d 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -138,8 +138,10 @@ try_again:
>                 }
>
>                 /* Try with "cpu-supply" */
> -               if (reg == reg_cpu0)
> +               if (reg == reg_cpu0) {
> +                       reg = reg_cpu;
>                         goto try_again;
> +               }
>
>                 dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n",
>                          cpu, PTR_ERR(cpu_reg));

Awesome code, I wrote it. Thanks for fixing it.

> and I think we just want reg_cpu to be "cpu", not "cpu-supply" because I
> think the regulator core adds in the "-supply" part already.

Okay.

> After fixing that I can get cpufreq going. I'm currently working on
> populating the OPPs at runtime without relying on DT. So eventually I'll
> need this patch:

I see..

> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> index b7ee67c4d1c0..6744321ae33d 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -239,11 +241,6 @@ static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
>         }
>
>         ret = of_init_opp_table(cpu_dev);
> -       if (ret) {
> -               dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
> -               goto out_put_node;
> -       }
> -
>         ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
>         if (ret) {
>                 dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
>
> which I hope is ok.

I need to see details of these routines once again, but will surely make
it work for you.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar June 30, 2014, 7:57 a.m. UTC | #4

On 27 June 2014 07:45, Viresh Kumar <[e-mail]> wrote:
> On 27 June 2014 07:23, Mike Turquette <[e-mail]> wrote:
>>> but it isn't future-proof if/when the clock framework starts returning
>>> dynamically allocated clock pointers for each clk_get() invocation.
>>> Maybe we need a function in the common clock framework that tells us if
>>> the clocks are the same either via DT or by taking two clock pointers?
>>
>> I looked through the patch briefly and did not see why we would need to
>> do this. Any hint?
>
> We want to know which CPUs are sharing clock line, so that we can
> fill affected-cpus field of cpufreq core.

What about comparing "clocks" property in cpu DT nodes?

@Rob/Grant: I tried looking for an existing routine to do that, but couldn't
find it. And so wrote one.

I am not good at DT stuff and so I do hope there would be few correction
required here. Let me know if this can be added to drivers/of/base.c :

+/**
+ * of_property_match - Match property in two nodes
+ * @np1, np2: Nodes to match
+ * @list_name: property to match
+ *
+ * Returns 1 on match, 0 on no match, and error for missing property.
+ */
+static int of_property_match(const struct device_node *np1,
+                             const struct device_node *np2,
+                             const char *list_name)
+{
+       const __be32 *list1, *list2, *list1_end;
+       int size1, size2;
+       phandle phandle1, phandle2;
+
+       /* Retrieve the list property */
+       list1 = of_get_property(np1, list_name, &size1);
+       if (!list1)
+               return -ENOENT;
+
+       list2 = of_get_property(np2, list_name, &size2);
+       if (!list2)
+               return -ENOENT;
+
+       if (size1 != size2)
+               return 0;
+
+       list1_end = list1 + size1 / sizeof(*list1);
+
+       /* Loop over the phandles */
+       while (list1 < list1_end) {
+               phandle1 = be32_to_cpup(list1++);
+               phandle2 = be32_to_cpup(list2++);
+
+               if (phandle1 != phandle2)
+                       return 0;
+       }
+
+       return 1;
+}

@Stephen: I have updated my tree with this change, in case you wanna
try.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring June 30, 2014, 6:33 p.m. UTC | #5

On Mon, Jun 30, 2014 at 2:57 AM, Viresh Kumar <[e-mail]> wrote:
> On 27 June 2014 07:45, Viresh Kumar <[e-mail]> wrote:
>> On 27 June 2014 07:23, Mike Turquette <[e-mail]> wrote:
>>>> but it isn't future-proof if/when the clock framework starts returning
>>>> dynamically allocated clock pointers for each clk_get() invocation.
>>>> Maybe we need a function in the common clock framework that tells us if
>>>> the clocks are the same either via DT or by taking two clock pointers?
>>>
>>> I looked through the patch briefly and did not see why we would need to
>>> do this. Any hint?
>>
>> We want to know which CPUs are sharing clock line, so that we can
>> fill affected-cpus field of cpufreq core.
>
> What about comparing "clocks" property in cpu DT nodes?

What if a different clock is selected for some reason. I think a clock
api function would be better.

That being said, I don't really have any issue with such a function.
Some comments on the implementation.

>
> @Rob/Grant: I tried looking for an existing routine to do that, but couldn't
> find it. And so wrote one.
>
> I am not good at DT stuff and so I do hope there would be few correction
> required here. Let me know if this can be added to drivers/of/base.c :
>
> +/**
> + * of_property_match - Match property in two nodes
> + * @np1, np2: Nodes to match
> + * @list_name: property to match
> + *
> + * Returns 1 on match, 0 on no match, and error for missing property.
> + */
> +static int of_property_match(const struct device_node *np1,
> +                             const struct device_node *np2,
> +                             const char *list_name)
> +{
> +       const __be32 *list1, *list2, *list1_end;

s/list/prop/

Everywhere.

> +       int size1, size2;
> +       phandle phandle1, phandle2;
> +
> +       /* Retrieve the list property */
> +       list1 = of_get_property(np1, list_name, &size1);
> +       if (!list1)
> +               return -ENOENT;
> +
> +       list2 = of_get_property(np2, list_name, &size2);
> +       if (!list2)
> +               return -ENOENT;
> +
> +       if (size1 != size2)
> +               return 0;
> +
> +       list1_end = list1 + size1 / sizeof(*list1);
> +
> +       /* Loop over the phandles */
> +       while (list1 < list1_end) {
> +               phandle1 = be32_to_cpup(list1++);
> +               phandle2 = be32_to_cpup(list2++);
> +
> +               if (phandle1 != phandle2)
> +                       return 0;
> +       }

You can just do a memcmp here.

This is wrong anyway because you don't know #clock-cells size.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar July 1, 2014, 11:14 a.m. UTC | #6

On 1 July 2014 00:03, Rob Herring <[e-mail]> wrote:
>> What about comparing "clocks" property in cpu DT nodes?
>
> What if a different clock is selected for some reason.

I don't know why that will happen for CPUs sharing clock line.

> I think a clock api function would be better.

@Mike: What do you think? I think we can get a clock API for
this.

> That being said, I don't really have any issue with such a function.
> Some comments on the implementation.

>> +static int of_property_match(const struct device_node *np1,
>> +                             const struct device_node *np2,
>> +                             const char *list_name)
>> +{
>> +       const __be32 *list1, *list2, *list1_end;
>
> s/list/prop/
>
> Everywhere.

Ok.

>> +       int size1, size2;
>> +       phandle phandle1, phandle2;
>> +
>> +       /* Retrieve the list property */
>> +       list1 = of_get_property(np1, list_name, &size1);
>> +       if (!list1)
>> +               return -ENOENT;
>> +
>> +       list2 = of_get_property(np2, list_name, &size2);
>> +       if (!list2)
>> +               return -ENOENT;
>> +
>> +       if (size1 != size2)
>> +               return 0;
>> +
>> +       list1_end = list1 + size1 / sizeof(*list1);
>> +
>> +       /* Loop over the phandles */
>> +       while (list1 < list1_end) {
>> +               phandle1 = be32_to_cpup(list1++);
>> +               phandle2 = be32_to_cpup(list2++);
>> +
>> +               if (phandle1 != phandle2)
>> +                       return 0;
>> +       }
>
> You can just do a memcmp here.

Yeah, that would be much better.

> This is wrong anyway because you don't know #clock-cells size.

I was actually comparing all the clock-cells, whatever there number
is to make sure "clocks" properties are exactly same. Anyway
memcmp will still guarantee that.

Thanks for your review.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Turquette July 1, 2014, 10 p.m. UTC | #7

Quoting Viresh Kumar (2014-07-01 04:14:04)
> On 1 July 2014 00:03, Rob Herring <[e-mail]> wrote:
> >> What about comparing "clocks" property in cpu DT nodes?
> >
> > What if a different clock is selected for some reason.
> 
> I don't know why that will happen for CPUs sharing clock line.
> 
> > I think a clock api function would be better.
> 
> @Mike: What do you think? I think we can get a clock API for
> this.

I can't help but think this is a pretty ugly solution. Why not specify
the nature of the cpu clock(s) in DT directly? There was a thread
already that discussed adding such a property to the CPU DT binding but
it seems to have gone cold[1]. Furthermore my mailer sucks and I see now
that my response to that thread never hit the list due to mangled
headers. Here is a copy/paste of my response to the aforementioned
thread:

"""
I'll join the bikeshedding.

The hardware property that matters for cpufreq-cpu0 users is that a
multi-core CPU uses a single clock input to scale frequency across all
of the cores in that cluster. So an accurate description is:

scaling-method = "clock-ganged"; //hardware-people-speak

Or,

scaling-method = "clock-shared"; //software-people-speak

Versus independently scalable CPUs in an SMP cluster:

scaling-method = "independent"; //x86, Krait, etc.

Or perhaps instead of "independent" at the parent "cpus" node we would
put the following in each cpu@N node:

scaling-method = "clock";

Or "psci" or "acpi" or whatever.

Thought exercise: for Hyperthreaded(tm) CPUs with 2 virtual cores for
every hard CPU (and multiple CPUs in a cluster):

scaling-method = "paired";

Or more simply, "hyperthreaded".
"""

Regards,
Mike

[1] http://www.spinics.net/lists/cpufreq/msg10034.html

> 
> > That being said, I don't really have any issue with such a function.
> > Some comments on the implementation.
> 
> >> +static int of_property_match(const struct device_node *np1,
> >> +                             const struct device_node *np2,
> >> +                             const char *list_name)
> >> +{
> >> +       const __be32 *list1, *list2, *list1_end;
> >
> > s/list/prop/
> >
> > Everywhere.
> 
> Ok.
> 
> >> +       int size1, size2;
> >> +       phandle phandle1, phandle2;
> >> +
> >> +       /* Retrieve the list property */
> >> +       list1 = of_get_property(np1, list_name, &size1);
> >> +       if (!list1)
> >> +               return -ENOENT;
> >> +
> >> +       list2 = of_get_property(np2, list_name, &size2);
> >> +       if (!list2)
> >> +               return -ENOENT;
> >> +
> >> +       if (size1 != size2)
> >> +               return 0;
> >> +
> >> +       list1_end = list1 + size1 / sizeof(*list1);
> >> +
> >> +       /* Loop over the phandles */
> >> +       while (list1 < list1_end) {
> >> +               phandle1 = be32_to_cpup(list1++);
> >> +               phandle2 = be32_to_cpup(list2++);
> >> +
> >> +               if (phandle1 != phandle2)
> >> +                       return 0;
> >> +       }
> >
> > You can just do a memcmp here.
> 
> Yeah, that would be much better.
> 
> > This is wrong anyway because you don't know #clock-cells size.
> 
> I was actually comparing all the clock-cells, whatever there number
> is to make sure "clocks" properties are exactly same. Anyway
> memcmp will still guarantee that.
> 
> Thanks for your review.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Viresh Kumar July 2, 2014, 3:32 a.m. UTC | #8

On 2 July 2014 03:30, Mike Turquette <[e-mail]> wrote:
> I can't help but think this is a pretty ugly solution. Why not specify

Thanks :)

> the nature of the cpu clock(s) in DT directly? There was a thread
> already that discussed adding such a property to the CPU DT binding but
> it seems to have gone cold[1]. Furthermore my mailer sucks and I see now
> that my response to that thread never hit the list due to mangled
> headers. Here is a copy/paste of my response to the aforementioned
> thread:

Atleast I received it.

Yes, I do agree that we need to get this from the DT in more elegant
way but it is going to take some time in doing that, and probably some
people are working on it as that might be used in scheduler-cpufreq
coordination as well..

For now we can go ahead and make it workable, even if it isn't that
elegant and update it later on.

Thanks for your inputs.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index b7ee67c4d1c0..6744321ae33d 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -138,8 +138,10 @@  try_again:
                }
 
                /* Try with "cpu-supply" */
-               if (reg == reg_cpu0)
+               if (reg == reg_cpu0) {
+                       reg = reg_cpu;
                        goto try_again;
+               }
 
                dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n",
                         cpu, PTR_ERR(cpu_reg));

and I think we just want reg_cpu to be "cpu", not "cpu-supply" because I
think the regulator core adds in the "-supply" part already.

After fixing that I can get cpufreq going. I'm currently working on
populating the OPPs at runtime without relying on DT. So eventually I'll
need this patch:

diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index b7ee67c4d1c0..6744321ae33d 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -239,11 +241,6 @@  static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
        }
 
        ret = of_init_opp_table(cpu_dev);
-       if (ret) {
-               dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
-               goto out_put_node;
-       }
-
        ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
        if (ret) {
                dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);