[PATCH v3 2/2] cpufreq: qoriq: Don't look at clock implementation details

[Scott Wood]

On Wed, 2016-07-06 at 18:30 -0700, Michael Turquette wrote:
> Quoting Scott Wood (2016-06-15 23:21:25)
> > 
> > -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?

> > 
> > @@ -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.

> > @@ -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.

>  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