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