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 clock). 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
