[PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass

[Rajendra Nayak]

On Thursday 28 March 2013 10:02 PM, Mike Turquette wrote:
> Quoting Rajendra Nayak (2013-03-28 03:59:41)
>> omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
>> as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
>> whether the .set_rate results in the DPLL being locked or put in bypass.
>> Early at boot, while some of these DPLLs are programmed and locked
>> (using .set_rate for the DPLL), this causes an ordering issue.
>>
>> For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
>> If a .set_rate of USB DPLL which programmes the M,N and locks it is called
>> before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
>> then attempts to lock the ABE DPLL and fails as the M,N values for ABE
>> are yet to be programmed.
>>
>> To get rid of this ordering needs, enable bypass clk for a DPLL as part
>> of its .set_rate only when its being put in bypass, and only enable the
>> ref clk when its locked.
>>
> 
> Hi Rajendra,
> 
> Another way to solve this problem would be to model the DPLLs as mux
> clocks with a list of possible parents (e.g. clk->parents[2]).  Then set
> the CLK_SET_RATE_PARENT flag on the USB DPLL which will allow to

But isn't this applicable only to the _current_ parent? The bypass_clk isn't
the parent of DPLL USB when I try a .set_rate on it. Its the ref_clk.

> propagate the rate request up to the ABE DPLL.  This should set the MN
> dividers appropriately.
> 
>> Reported-by: Roger Quadros <rogerq at ti.com>
>> Signed-off-by: Rajendra Nayak <rnayak at ti.com>
>> ---
>>  arch/arm/mach-omap2/dpll3xxx.c |   19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
>> index 3aed4b0..6e9873f 100644
>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>> @@ -480,20 +480,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>>         if (!dd)
>>                 return -EINVAL;
>>  
>> -       __clk_prepare(dd->clk_bypass);
>> -       clk_enable(dd->clk_bypass);
>> -       __clk_prepare(dd->clk_ref);
>> -       clk_enable(dd->clk_ref);
> 
> Is this safe?  I always thought that the programming sequence in the TRM
> was to enable both the ref and bypass clocks during DPLL reprogramming.
> Maybe I am misremembering or assuming that the code strictly followed
> the TRM.

hmm, I didn't bother to check either. Will check, it would be strange though
if such a sequence is indeed needed.

regards,
Rajendra

> 
> Regards,
> Mike
> 
>> -
>>         if (__clk_get_rate(dd->clk_bypass) == rate &&
>>             (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) {
>>                 pr_debug("%s: %s: set rate: entering bypass.\n",
>>                          __func__, __clk_get_name(hw->clk));
>>  
>> +               __clk_prepare(dd->clk_bypass);
>> +               clk_enable(dd->clk_bypass);
>>                 ret = _omap3_noncore_dpll_bypass(clk);
>>                 if (!ret)
>>                         new_parent = dd->clk_bypass;
>> +               clk_disable(dd->clk_bypass);
>> +               __clk_unprepare(dd->clk_bypass);
>>         } else {
>> +               __clk_prepare(dd->clk_ref);
>> +               clk_enable(dd->clk_ref);
>> +
>>                 if (dd->last_rounded_rate != rate)
>>                         rate = __clk_round_rate(hw->clk, rate);
>>  
>> @@ -514,6 +516,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>>                 ret = omap3_noncore_dpll_program(clk, freqsel);
>>                 if (!ret)
>>                         new_parent = dd->clk_ref;
>> +               clk_disable(dd->clk_ref);
>> +               __clk_unprepare(dd->clk_ref);
>>         }
>>         /*
>>         * FIXME - this is all wrong.  common code handles reparenting and
>> @@ -525,11 +529,6 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>>         if (!ret)
>>                 __clk_reparent(hw->clk, new_parent);
>>  
>> -       clk_disable(dd->clk_ref);
>> -       __clk_unprepare(dd->clk_ref);
>> -       clk_disable(dd->clk_bypass);
>> -       __clk_unprepare(dd->clk_bypass);
>> -
>>         return 0;
>>  }
>>  
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html