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

Sylwester Nawrocki
2017-04-24T11:12:22Z

On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote:
[...]
>> +static void samsung_pll3xxx_disable(struct clk_hw *hw)
>> +{
>> + struct samsung_clk_pll *pll = to_clk_pll(hw);
>> + u32 tmp;
>> +
>> + tmp = readl_relaxed(pll->con_reg);
>> + tmp |= BIT(pll->enable_offs);
>
> I think you meant here:
> tmp &= ~BIT()

Yes, I messed it up while copy/pasting from samsung_pll3xxx_enable().

>> + writel_relaxed(tmp, pll->con_reg);
>> +}

>> static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw,
>> unsigned long parent_rate)
>> @@ -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.

>> + do {
>> + cpu_relax();
>> + tmp = readl_relaxed(pll->con_reg);
>> + } while (!(tmp & BIT(PLL36XX_LOCK_STAT_SHIFT)));
>
> To be consistent:
> BIT(pll->lock_offs)?

Yes, fixed.

Thanks for your review!

--
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org