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
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
