Re: Role of PLL_ENABLE_BIT

["Andreas Färber" <afaerber@suse.de>]

Hi Humberto,

Somehow the original message didn't arrive on linux-samsung-soc...

Am 31.07.2014 06:51, schrieb Yadwinder Singh Brar:
>> ------- Original Message -------
>> Sender : Humberto Naves<hsnaves@gmail.com>
>> 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);

It appears tmp is a u32. Does that make 1 << 31 a safe expression wrt
signedness, or does it need to be 1u << PLL35XX_ENABLE_SHIFT?
Did you verify the result?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg