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
