Quoting Viresh Kumar (2014-07-02 19:44:04)

On 3 July 2014 06:54, Stephen Boyd [off-list ref] wrote:

quoted

I gave it a spin. It works so you can have my

Tested-by: Stephen Boyd <redacted>

Thanks, all suggested improvements are made and pushed again with
your Tested-by..

quoted

I'm still concerned about the patch where we figure out if the clocks
are shared. I worry about a configuration where there are different
clocks for on/off (i.e. gates) that are per-cpu but they all source from
the same divider or something that is per-cluster. In DT this may be
described as different clock provider outputs for the gates and in the
cpu node we would have different clock specifiers but in reality all the
CPUs in that cluster are affected by the same frequency scaling.

Yeah, this is probably what matches with Rob's doubt. These can
actually be different. Good point.

quoted

In this case we'll need to get help from the clock framework to
determine that those gates clocks don't have any .set_rate() callback so
they can't actually change rate independently, and then walk up the tree
to their parents to see if they have a common ancestor that does change
rates. That's where it becomes useful to have a clock framework API for
this, like clk_shares_rate(struct clk *clk, struct clk *peer) or
something that can hide all this from cpufreq. Here's what I think it
would look like (totally untested/uncompiled):

static struct clk *find_rate_changer(struct clk *clk)
{

        if (!clk)
                return NULL;

        do {
                /* Rate could change by changing parents */
                if (clk->num_parents > 1)
                        return clk;

                /* Rate could change by calling clk_set_rate() */
                if (clk->ops->set_rate)
                        return clk;

                /*
                 * This is odd, clk_set_rate() doesn't propagate
                 * and this clock can't change rate or parents
                 * so we must be at the root and the clock we
                 * started at can't change rates. Just return the
                 * root so that we can see if the other clock shares
                 * the same root although CPUfreq should never care.
                 */
                if (!(clk->flags & CLK_SET_RATE_PARENT))
                        return clk;
        } while ((clk = clk->parent))

        return NULL;
}

bool clk_shares_rate(struct clk *clk, struct clk *peer)
{
        struct clk *p1, *p2;

        p1 = find_rate_changer(clk);
        p2 = find_rate_changer(peer)

        return p1 == p2;
}

I find it much better then doing what I did initially, simply matching clk_get()
outputs. Lets see what Mike has to say..

Sorry for being dense, but I still do not get why trying to dynamically
discover a shared rate-changeable clock is a better approach than simply
describing the hardware in DT?

Is adding a property to the CPU binding that describes how the CPUs in a
cluster expect to use a clock somehow a non-starter? It is certainly a
win for readability when staring at DT and trying to understand how DVFS
on that CPU is meant to work (as opposed to hiding that knowledge behind
a tree walk).

Regards,
Mike

@Mike: Is this less ugly ? :)