Re: [PATCH RFC 1/7] clk: samsung: Add enable/disable operation for PLL36XX clocks

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

(dropping unrelated addresses from Cc list)

On 04/24/2017 01:18 PM, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 1:12 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote:
>>>> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw
>>>> *hw, unsigned long drate,
>>>>         writel_relaxed(pll_con1, pll->con_reg + 4);
>>>>
>>>>         /* wait_lock_time */
>>>> -       do {
>>>> -               cpu_relax();
>>>> -               tmp = readl_relaxed(pll->con_reg);
>>>> -       } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT)));
>>
>>
>> I will add a comment here like:
>> /* Wait until the PLL is locked if it is enabled. */
>>
>>>> +       if (pll_con0 & BIT(pll->enable_offs)) {
>>>
>>>
>>> Why this additional if() is needed?
>>
>>
>> The PLL will never transition to a locked state if it is disabled, without
>> this test we would end up with an indefinite loop below.
>
> Hmmm... shouldn't this be split from this change? Or maybe this is
> needed only because you added enable/disable for pl36xx and previously
> this was not existing?

Yes, it looks like previously the PLLs were always on after were enabled.
I wouldn't be separating that part, as it's all logically dependent.
Just the commit message might need some improvement.

-- 
Thanks,
Sylwester