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

[Michael Turquette]

Quoting Scott Wood (2016-07-06 21:13:23)
> 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?

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

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