On 05/14/2012 07:00 PM, Mike Turquette wrote:
On 20120514-16:48, Saravana Kannan wrote:
On 05/14/2012 02:36 PM, Turquette, Mike wrote:
On Fri, May 11, 2012 at 7:58 PM, Saravana Kannan<skannan@xxxxxxxxxxxxxx> wrote:
Mike,
<snip>
The take-away is that a clock that can adjust its rate (eg: implements a
.set_rate callback) must also implement .recalc_rate and .round_rate
callbacks.
I get the round_rate ops part. But I don't see a need to force a
recalc_rate ops. Can we just check for the existence of .set_rate()
to figure out if the clock will take up the rate of the parent or
will output a different rate?
I think you are forgetting the case where a clock can adjust its output
rate but doesn't always have its .set_rate callback called. The trivial
example of this is an adjustable divider that is downstream from some
parent clock whose rate is being changed. In any such case it is quite
necessary to have a .recalc_rate callback on the downstream clock.
>
The sanity checks in __clk_init (and the documenation) make it clear
that .recalc_rate callbacks must exist due to this exact scenario. I
guess we could leave it up to the implementor to "get it right" and only
provide .recalc_rate callbacks for clocks whose parents' rates might
change, but why take that risk?
My comment about forcing recalc rates is limited to "a clock that can
adjust its rate (eg: implements a .set_rate callback) must also
implement .recalc_rate.". As you mention, this is really a function of
whether the parent rate can change or not.
I stand corrected on my orthogonal point that "if .set_rate is not
implemented, we can assume the rate matches the parent".
Also, if the clock's rate was just set with set_rate, why do we need to
recalc the rate by reading hardware? I'm a bit confused. Can you please
clarify what's going on here?
This is simply being very cautious. For platforms adjusting dividers
with direct register writes this might feel unnecessary. However this
strict checking is in anticipation of clock hardware that might not
actually output the precise rate passed into .set_rate. In principal
this isn't different from how CPUfreq and devfreq drivers inspect rates
after requesting them.
Sorry, this still doesn't make much sense to me. This essentially
means we can't trust the HW to do what we are asking it to do?
My bit about being "cautious" above is not the driving force behind the
requirement for implementing .recalc_rate. I thought that you were
specifically complaining about calling .recalc_rate AFTER we called
.set_rate, in which case my point above stands from the perspective of
being future-proof.
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?
Your hardware even has a feature to sample it's own frequency at
run-time... does that mean your hardware doesn't trust itself?
No. This is for debugging the software.
<snip>
but you also feel that we
should not have a requirement to implement .recalc_rate on clocks that
can adjust their rate again.
I never questioned the need for recalc rates. I also didn't make the
above blanket statement -- in fact, I was saying the opposite.
My point was whether we needed to check for .recalc_rates or if we can
just look for .set_rate to figure out if the clock will take the
parent's rate. Your fixed factor div-2 example corrected me on that point.
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.
(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).
But to assert that we can entirely remove the requirement to implement
.recalc_rates on clocks that support .set_rate sounds insane to me.
Please let me know if I've misunderstood what you meant by the
statement, "I don't see a need to force a recalc_rate ops".
Yeah, I think you mistook my comment that was specific to clocks that
implement set rate as applying to all clocks.
<snip>
I hope that after
reading this email you agree that anyone implementing .set_rate must
also implement .recalc_rate.
This is the original point I raised. To be clear, I'm not denying the
need for .recalc_rate. I'm just saying that it's not related to
.set_rate(). As you mentioned in your response above, this is not really
a function of whether a clock can set it's rate or not. It's a function
of whether a parent's rate can change. So, I don't see why we should
arbitrarily tie it to .set_rate.
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).
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).
Regards,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
