On Mon, May 14, 2012 at 9:20 PM, Saravana Kannan [off-list ref] wrote:

On 05/14/2012 07:00 PM, Mike Turquette wrote:
I agree the with the need for recalc_rates you mentioned above (parent
changes rate and it will affect children). But I still don't get why you
have to recalc the rate of the clock that you just set. Can you please give
an example?

I don't have a clock that needs "immediate recalculation after setting
it's rate".  However the code is not incorrect as-is, just suboptimal.
 I'm not currently looking into improving this since I have bigger
problems to solve.  Patches are welcome, otherwise I'll toss it on the
back-burner and fix it up some day (not for initial 3.5 merge window).

quoted

I'm willing to discuss removing the (sometimes) redundant .recalc_rate
calls immediately following .set_rate

Yes, please. This was one of the main points of my previous email.

Same as above.

quoted

(since we basically perform a
preemptive .recalc_rate in clk_calc_subtree, called by clk_calc_rates).

Do you do a recalc on the clock that the clk_set_rate() is called on? You
only seem to make the preemptive calls on the children. Which makes sense
(But I have one concern. I will get to it at the end).

Preemptive calls happen once only, in a cascade starting from the
"top" clock (which may or may not be the original clock in the
clk_set_rate call due to upwards parent propagation).

However we do recalc the rate for each clock (even after calling its
.set_rate callback) in clk_change_rate.  This could be improved, but
I'm not looking at it right now.

...

For example, there are several PLLs in MSM that get their input from a fixed
crystal oscillator. There's no point in implementing recalc_rate for them
(except for figuring out the rate during init).

I still feel that, fundamentally, any clock which can output a
different rate than is fed into must implement .recalc_rates.  The set
of clocks which can output a different rate than their input often
exceeds the set of clocks that implement .set_rate (as evidenced by my
fixed-divide-by-2 example).  But forcing a check for .recalc_rate on
clocks that implement .set_rate at least helps us catch some which
should implement and do not.

For your PLLs which are fed from a fixed input clock, we can optimize
the code to not _call_ .recalc_rates unnecessarily.  But some day when
your chip changes and you have multiple inputs to your PLLs or an
adjustable rate parent you'll be glad you had .recalc_rates
implemented and ready to go.  It's simply a matter of correctness.

Which brings me to another point:
I think we should split out the "figure out the clock's rate during
boot/init" to a separate op. That operation by definition has to go through
many registers, and if it's a rate settable clock, go through all the
possible frequencies to figure out the rate. That seems too expensive for
something that's done often like .recalc_rate. In pretty much every other
call to .recalc_rate, it doesn't really need to check the hardware. It just
needs to recompute the rate based on the software model of that clock. If we
do add this new op (say, .sync), the expense of calling .recalc_rate after
calling .set_rate would be much much lower and I won't really complain much
about it (would still be nice to not do it).

This is also worth thinking about, but it changes the existing
interfaces for clock code to implement and it's yet another
optimization.  So patches are welcome, otherwise I'll toss it onto the
stack somewhere.

Regards,
Mike