Hi Humberto, > ------- Original Message ------- > Sender : Humberto Naves<hsnaves@xxxxxxxxx> > Date : Jul 31, 2014 00:16 (GMT+09:00) > Title : Role of PLL_ENABLE_BIT > > Hi, > > > I am using an ODROID-XU board, and I was trying to change the rates of some PLL clocks for a while, but I could not. After a while, I realized that the cpu was trapped in an infinite loop waiting for the PLL35XX_LOCK_STAT bit to be set. Furthermore, I discovered that the reason was that the the PLL35XX_ENABLE_BIT was not set. > > > I wonder if this is really a driver problem, since in the set_rate functions, the ENABLE_BIT is not set. Can someone confirm if this is indeed the expected behavior of the driver? > hmm .. actually it doesn't clear also ENABLE_BIT, I wonder how its getting clear in your case since we don't have enable/disable() implemented yet. > > > By the way, In order to solve this issue, I changed the files according to the following diff > > > diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c > index b07fad2..9300274 100644 > --- a/drivers/clk/samsung/clk-pll.c > +++ b/drivers/clk/samsung/clk-pll.c > @@ -131,14 +131,15 @@ static const struct clk_ops samsung_pll3000_clk_ops = { > /* Maximum lock time can be 270 * PDIV cycles */ > #define PLL35XX_LOCK_FACTOR (270) > > -#define PLL35XX_MDIV_MASK (0x3FF) > -#define PLL35XX_PDIV_MASK (0x3F) > -#define PLL35XX_SDIV_MASK (0x7) > +#define PLL35XX_MDIV_MASK (0x3FF) > +#define PLL35XX_PDIV_MASK (0x3F) > +#define PLL35XX_SDIV_MASK (0x7) > #define PLL35XX_LOCK_STAT_MASK (0x1) > -#define PLL35XX_MDIV_SHIFT (16) > -#define PLL35XX_PDIV_SHIFT (8) > -#define PLL35XX_SDIV_SHIFT (0) > +#define PLL35XX_MDIV_SHIFT (16) > +#define PLL35XX_PDIV_SHIFT (8) > +#define PLL35XX_SDIV_SHIFT (0) > #define PLL35XX_LOCK_STAT_SHIFT (29) > +#define PLL35XX_ENABLE_SHIFT (31) > > static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > @@ -190,6 +191,7 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, > /* If only s change, change just s value only*/ > tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT); > tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT; > + tmp |= (1 << PLL35XX_ENABLE_SHIFT); > __raw_writel(tmp, pll->con_reg); > > return 0; > @@ -206,6 +208,7 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, > tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) | > (rate->pdiv << PLL35XX_PDIV_SHIFT) | > (rate->sdiv << PLL35XX_SDIV_SHIFT); > + tmp |= (1 << PLL35XX_ENABLE_SHIFT); Ideally tmp should already have this bit set since we are not clearing it after doing tmp = __raw_readl(tmp); Can you please check value of tmp here ? and Is there anything else in your kernel clearing this bit ? Regards, Yadwinder > __raw_writel(tmp, pll->con_reg); > > /* wait_lock_time */ > @@ -242,6 +245,7 @@ static const struct clk_ops samsung_pll35xx_clk_min_ops = { > #define PLL36XX_SDIV_SHIFT (0) > #define PLL36XX_KDIV_SHIFT (0) > #define PLL36XX_LOCK_STAT_SHIFT (29) > +#define PLL36XX_ENABLE_SHIFT (31) > > static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > @@ -299,6 +303,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, > /* If only s change, change just s value only*/ > pll_con0 &= ~(PLL36XX_SDIV_MASK << PLL36XX_SDIV_SHIFT); > pll_con0 |= (rate->sdiv << PLL36XX_SDIV_SHIFT); > + pll_con0 |= (1 << PLL36XX_ENABLE_SHIFT); > __raw_writel(pll_con0, pll->con_reg); > > return 0; > @@ -314,6 +319,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, > pll_con0 |= (rate->mdiv << PLL36XX_MDIV_SHIFT) | > (rate->pdiv << PLL36XX_PDIV_SHIFT) | > (rate->sdiv << PLL36XX_SDIV_SHIFT); > + pll_con0 |= (1 << PLL36XX_ENABLE_SHIFT); > __raw_writel(pll_con0, pll->con_reg); > > pll_con1 &= ~(PLL36XX_KDIV_MASK << PLL36XX_KDIV_SHIFT); -- 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