On Mon, Apr 24, 2017 at 1:35 PM, Sylwester Nawrocki
<s.nawrocki@xxxxxxxxxxx> wrote:
> (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@xxxxxxxxxxx> 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 for explanation.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
