Re: [PATCHV2 2/2] OMAP3630: Clock: Fixing HSDivider Limitation

[Mike Turquette <mturquette@ti.com>]

Kauppi Ari (EXT-Ixonos/Oulu) wrote:
> On Mon, 2010-01-18 at 19:04 +0100, ext Paul Walmsley wrote:
>> Vijayakumar, Mike,
>>> Without this workaround the divider value goes
>>> to its reset state after the corresponding PWRDN bit is set from the
>>> CM_CLKEN_PLL register. The correct divider value can be restored by
>>> writing a dummy value to the register different than what is in there, and
>>> then re-writing the desired value.
>>> This Work around is applicable for below HS Dividers.
>>>  1. DPLL3_M3
>>>  2. DPLL4_M2
>>>  3. DPLL4_M3
>>>  4. DPLL4_M4
>>>  5. DPLL4_M5
>>>  6. DPLL4_M6
>>>
>>> Signed-off-by: Mike Turquette <mturquette@ti.com>
>>> Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
>>> Signed-off-by: Vijaykumar GN <vijaykumar.gn@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/clock34xx.c      |   36 ++++++++++++++++++++++++++++++++++
>>>  arch/arm/mach-omap2/clock34xx.h      |    1 +
>>>  arch/arm/mach-omap2/clock34xx_data.c |   15 ++++++++++++++
>>>  3 files changed, 52 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
>>> index 0d30e53..e5213f8 100644
>>> --- a/arch/arm/mach-omap2/clock34xx.c
>>> +++ b/arch/arm/mach-omap2/clock34xx.c
>>> @@ -146,6 +146,42 @@ const struct clkops clkops_omap3430es2_hsotgusb_wait = {
>>>  	.find_companion = omap2_clk_dflt_find_companion,
>>>  };
>>>  
>>> +/** omap3_pwrdn_clk_enable_with_hsdiv_restore - enable clocks suffering
>>> + *         from HSDivider problem.
>> The '/**' needs to be on its own line.  I appreciate the useful comments, 
>> but please make sure they conform to 
>> Documentation/kernel-doc-nano-HOWTO.txt
>>
>>> + * @clk: DPLL output struct clk
>>> + *
>>> + * 3630 only: dpll3_m3_ck, dpll4_m2_ck, dpll4_m3_ck, dpll4_m4_ck, dpll4_m5_ck
>>> + * & dpll4_m6_ck dividers get lost after their respective PWRDN bits are set.
>>> + * Any write to the corresponding CM_CLKSEL register will refresh the
>>> + * dividers.  Only x2 clocks are affected, so it is safe to trust the parent
>>> + * clock information to refresh the CM_CLKSEL registers.
>>> + */
>>> +int omap3_pwrdn_clk_enable_with_hsdiv_restore(struct clk *clk)
>>> +{
>>> +	u32 v;
>>> +	int ret;
>>> +
>>> +	/* enable the clock */
>>> +	ret = omap2_dflt_clk_enable(clk);
>>> +
>>> +	/* Restore the dividers */
>>> +	if (!ret) {
>>> +		v = __raw_readl(clk->parent->clksel_reg);
>>> +		v += (1 << clk->parent->clksel_shift);
>> Using += here could affect many bits in the register if the add carries.  
>> This doesn't seem like what you want.
>>
>> Also, isn't there a risk of side-effects here, that if this bit was not 
>> already set, that everything further down the clock tree from this could 
>> be affected?  Wouldn't it be best to just rewrite the correct clksel 
>> value?
> 
> The assumption is that the divider is correct in the register so I guess
> the clock tree should be fine? The proper clksel/divider has been
> earlier set but for some reason HW loses the divider if PWRDN is
> toggled.

In my testing that assumption was always correct.  In fact that is what 
made this bug very difficult to track down: the register values were 
always the correct value programmed initially but the actual clocks were 
wrong.

> Our experiments with this issue show that just read/write of same value
> is not enough to refresh the HW. The clksel has to be written with a
> different divider first and then write the original divider back.

ACK to the above comment.  Registers need to be changed to a different 
value and back.

Mike

> --
> Ari
>