Re: moving Tegra30 to the common clock framework

[Saravana Kannan <skannan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 05/09/2012 03:36 AM, Peter De Schrijver wrote:
> On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
>> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
>>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>   wrote:
>>>> Hi Mike,
>>>> Could you please explain more details about how to implement a
>>>> re-parenting operation as part of it's .set_rate implementation?
>>>
>>> Sure.
>>>
>>>> As far as I know, we can not call clk_set_parent in .set_rate function
>>>> directly, since clk_set_rate and clk_set_parent are using the same
>>>> prepare_lock.
>>>
>>> That is correct.
>>>
>>>> Any other interface can be used to implement it?
>>>
>>> You have two options available to you.
>>>
>>> 1) __clk_reparent can be used from your .set_rate callback today to
>>> reflect changes made to the tree topology.  OMAP uses this in our PLL
>>> .set_rate implementation: depending on the re-lock frequency the PLL
>>> may switch parents dynamically.  __clk_reparent does the
>>> framework-level cleanup needed for this (that function is also used
>>> when populating the clock tree with new clock nodes).
>>>
>>> 2) __clk_set_parent could be made non-static if you needed this (I've
>>> been meaning to talk to Saravana about this since I think MSM needs
>>> something like this).
>>
>> Thanks!
>>
>> I don't think I need (2). But I don't think I can use (1) as is either.
>> I can use (1) with some additional code in my set rate op.
>>
>> While set rate is in progress, both the parents might need to stay
>> enabled for a short duration. So, in my internal set rate, I need to
>> check if my clock is prepared/enabled and call prepare/enable on the
>> "old parent", call __clk_reparent (which will reduce the ref count for
>> the old parents and increase it for the new parents), finish the
>> reparent in HW and then unprepare/disable the old parent if I have
>> prepared/enabled them earlier.
>>
>> It might be beneficial to provide something like a
>> __clk_reparent_start(new_parent, *scratch_pointer) and
>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
>> than just MSM. Based on this email, I would guess that Tegra would want
>> something similar too.
>>
>
> We also need to reparent clocks using a pll if we want to change the PLLs rate
> while the users are active.

Peter,

Is this reparent permanent (as in, stays reparented when you return from 
clk_set_rate()) or is it a reparenting for just a short duration inside 
the set_rate ops?

If it's the latter, I don't think you would need any helper code in 
clock framework other than the already existing __clk_prepare() and 
clk_enable(). Just turn on the temp parent, reparent to it, do whatever 
you need to do, and go back to your original parent.

If it's the former (permanent reparent), can I assume that the 
CLK_SET_RATE_PARENT flag won't be set for this clock? Otherwise, it's 
going to be quite yucky/convoluted. I'm not sure how well a reparent 
would work with the code in common clock framwework that walks up the 
parents to propagate the rate change to them. I wouldn't say that it's 
wrong to only want to propagate the rate for certain rates and for 
certain parents, but I will have to stare at the common clock code for a 
while.

Mike,

I was looking at the code to make the changes and I noticed this snippet 
(reformatted for email) in clk_change_rate():

        if (clk->ops->set_rate)
                 clk->ops->set_rate(clk->hw, clk->new_rate,
					clk->parent->rate);


         if (clk->ops->recalc_rate)
                 clk->rate = clk->ops->recalc_rate(clk->hw,
                                 clk->parent->rate);
         else
                 clk->rate = clk->parent->rate;

I'm a bit confused. I thought recalc_rates was optional. But if I don't 
implement it, the clocks rate will get set to parent's rate? Or is that 
a bug in the code?

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?

Would you mind adding more comments inside clk_calc_new_rates() and 
clk_change_rate() trying to explain what cases you are trying to account 
for?

Also, in clk_calc_new_rates(),

         if (!clk->ops->round_rate) {
                 top = clk_calc_new_rates(clk->parent, rate);
                 new_rate = clk->parent->new_rate;

                 goto out;
         }

Is the code assuming that if there is no round rate ops that that clock 
node is only a gating clock (as in, can't change frequency the input 
freq)? Just trying to understand the assumptions made in the code.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.