Quoting Scott Wood (2016-07-06 21:13:23)

On Wed, 2016-07-06 at 18:30 -0700, Michael Turquette wrote:

quoted

Quoting Scott Wood (2016-06-15 23:21:25)

quoted

-static struct device_node *cpu_to_clk_node(int cpu)
+static struct clk *cpu_to_clk(int cpu)
 {
-       struct device_node *np, *clk_np;
+       struct device_node *np;
+       struct clk *clk;
 
        if (!cpu_present(cpu))
                return NULL;

@@ -112,37 +80,28 @@ static struct device_node *cpu_to_clk_node(int cpu)

        if (!np)
                return NULL;
 
-       clk_np = of_parse_phandle(np, "clocks", 0);
-       if (!clk_np)
-               return NULL;
-
+       clk = of_clk_get(np, 0);

Why not use devm_clk_get here?

devm_clk_get() is a wrapper around clk_get() which is not the same as
of_clk_get().  What device would you pass to devm_clk_get(), and what name
would you pass?

I'm fuzzy on whether or not you get a struct device from a cpufreq
driver. If so, then that would be the one to use. I would hope that
cpufreq drivers model cpus as devices, but I'm really not sure without
looking into the code.

Regards,
Mike

quoted

quoted

@@ -221,17 +180,12 @@ static int qoriq_cpufreq_cpu_init(struct

cpufreq_policy *policy)
                goto err_nomem2;
        }
 
-       pnode = of_parse_phandle(np, "clocks", 0);
-       if (!pnode) {
-               pr_err("%s: could not get clock information\n", __func__);
-               goto err_nomem2;
-       }
+       count = clk_get_num_parents(policy->clk);

We already have of_clk_get_parent_count. This is found in
clk-provider.h, which doesn't fit perfectly here since the cpufreq
driver is not a clock provider, but instead a consumer.

It's also a device tree function, and the clock parents in question aren't in
the device tree.

quoted

quoted

@@ -240,23 +194,11 @@ static int qoriq_cpufreq_cpu_init(struct

cpufreq_policy *policy)
                goto err_pclk;
        }
 
-       if (fmask)
-               mask = fmask[get_cpu_physical_id(cpu)];
-       else
-               mask = 0x0;
-
        for (i = 0; i < count; i++) {
-               clk = of_clk_get(pnode, i);
+               clk = clk_get_parent_by_index(policy->clk, i);
                data->pclk[i] = clk;
                freq = clk_get_rate(clk);
-               /*
-                * the clock is valid if its frequency is not masked
-                * and large than minimum allowed frequency.
-                */
-               if (freq < min_cpufreq || (mask & (1 << i)))
-                       table[i].frequency = CPUFREQ_ENTRY_INVALID;
-               else
-                       table[i].frequency = freq / 1000;
+               table[i].frequency = freq / 1000;

Hmm, so you change cpu clock rate by selecting different clock sources
from a cpu clock mux, right?

Yes.  You'd think such a simple thing would be more straightforward.

quoted

 I wonder if you can just have a child mux
clock that selects parents from .set_rate (via a .determine_rate
callback)? Then you could just call clk_set_rate() on your cpu mux clock
and maybe skip most of the stuff this driver does?

"Most of the stuff this driver does" is dealing with the cpufreq subsystem
(ask the cpufreq maintainers why it requires so much boilerplate), associating
clock muxes with cpus, etc.  It is also not obvious to me how to use
determine_rate() or that the end result would be any simpler or better.  It
seems like the implementation would just be reimplementing logic that already
exists in cpufreq, and the cpufreq driver would still need to be able to get a
list of possible frequencies, because cpufreq wants a table of them.

After nearly a year of non-response to these patches[1], a request to
completely rearchitect this driver[2] just to avoid exposing a couple
straightforward informational functions to clock consumers[3] was not quite
what I was hoping for.  What is wrong with clock consumers being able to query
the parent list, given that clock consumers have the ability to request a
particular parent?

-Scott

[1] Original versions:
http://www.spinics.net/lists/linux-clk/msg03069.html
http://www.spinics.net/lists/linux-clk/msg03070.html


[2] The only reason I'm touching this driver at all is because it currently
makes bad assumptions about clock provider internals (and clock provider
device tree structure) that are broken by the new bindings enabled by
commit 0dfc86b3173fee ("clk: qoriq: Move chip-specific knowledge into
driver").

[3] I initially discussed adding consumer APIs for this patchset in 
http://lkml.iu.edu/hypermail/linux/kernel/1509.2/02728.html