[lkml]   [2009]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
Hello Russell,

On Sun, 8 Feb 2009, Russell King - ARM Linux wrote:

> On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote:
> > For upcoming notifier support, modify the rate recalculation code to
> > take parent rate and rate storage parameters. The goal here is to
> > allow the clock code to determine what the clock's rate would be after
> > a parent change or a rate change, without actually changing the
> > hardware registers. This is used by the upcoming notifier patches to
> > pass a clock's current and planned rates to the notifier callbacks.
> NAK. I'm not sure whether you've realised, but this is exactly what the
> 'round_rate' method is supposed to be doing. It provides a method by
> which you can find out what the clock rate would be if you asked for
> 'set_rate' to set the hardware to the requested rate.

There's been some miscommunication about the purpose of this patch - most
likely my fault for not grouping this patch with the clock notifier patch
set, which has not yet been submitted for upstream merging. Most of this
patch is not needed until notifiers are implemented, so I'd propose that
we defer consideration of most of this patch until the clock notifiers are
submitted. What would be useful, though, is to split out the
RECALC_ON_ENABLE portion of this patch and merge that. That is a bug fix,
and I should have sent that as a separate patch.

For the sake of the current discussion though, here's what this patch was
intended to accomplish. We have some patches under development that
implement clock rate change notifiers, allowing drivers to register for
pre- and post-notification on clock rate changes. The notifier callbacks
are passed the old clock rate and the new clock rate. Since a rate change
on a clock in the middle of the clock tree (e.g., dpll3_m2_ck) can affect
all clocks downstream, the clock code must compute the new rate for not
only dpll3_m2_ck, but also all downstream clocks, before clk->rate is
updated and the hardware registers are actually changed.

This presents a problem with the current code. Consider the
pre-notification case, in which the clock code needs to compute the target
clock rate without actually updating clk->rate. But the round_rate()
functions assume that the parent clock's current rate is what should be
used. If the parent clock's rate would also change as part of the
clk_set_rate(), the target rate computation will return an incorrect rate.

So this patch allows temporary rates to be computed for all of the clocks
that would be affected by the rate or parent change, so the correct target
rates can be passed to each clock's notifier (assuming one exists for the

N.B., this temporary rate computation is also the motivation behind the
recursive propagate_rate() calls that we discussed earlier.

It also may turn out that it's unnecessary to pass the new rate to the
notifiers, in which case most of this code won't need to reappear.

> A far better way to approach this would be to split the set_rate/recalc
> functionality into two parts:
> 1. a method which returns the both the new clock rate and the hardware
> programming information
> 2. a method to commit the hardware programming information to the registers
> (1) can be used for implementing clk_round_rate() (which is totally lacking
> in OMAP2+, yet is implemented in OMAP1), clk_set_rate(), clk_set_parent()
> etc.

The OMAP2/3 clock code does implement clk_round_rate() - most commonly via
mach-omap2/clock.c:omap2_clksel_round_rate(). There are a few special
cases which use other round_rate() functions.

> (2) can be used when it's required to actually reprogram the hardware.
> So, rather than the current situation where we have recalc, round_rate
> and set_rate methods, we have calc_rate() and commit() methods instead
> and have the core clock code sort out calling these.

I haven't yet spent much time thinking about this new arrangement -- it
seems to be orthogonal to the problem that this patch is intended to solve
-- but it might make sense for a different purpose. Many of our
set_rate() functions wind up calling round_rate() first to determine
whether the passed-in rate is valid and to return the hardware programming
data from the clksel structures. This extra call to round_rate() just
wastes cycles. So this new layout might be more efficient, although it
appears that calc_rate() is called twice in the new arrangement as well.

I appreciate the ongoing technical review,

- Paul

 \ /
  Last update: 2009-02-11 08:55    [W:0.185 / U:18.444 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site