[PATCH v3 3/6] clk: samsung: Add set_rate() clk_ops for PLL35xx

Yadwinder Singh Brar yadi.brar01 at gmail.com
Wed Jun 12 10:51:27 EDT 2013


On Sat, Jun 8, 2013 at 5:42 PM, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> On Friday 31 of May 2013 18:01:33 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar at samsung.com>
>>
>> This patch add set_rate() and round_rate() for PLL35xx
>>
>> Reviewed-by: Doug Anderson <dianders at chromium.org>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar at samsung.com>
>> ---
>>  drivers/clk/samsung/clk-pll.c |  103
>> ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 102
>> insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-pll.c
>> b/drivers/clk/samsung/clk-pll.c index 8226528..9591560 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -27,6 +27,36 @@ struct samsung_clk_pll {
>>  #define pll_writel(pll, val, offset)                                 \
>>               __raw_writel(val, (void __iomem *)(pll->base + (offset)));
>>
>> +static const struct samsung_pll_rate_table *samsung_get_pll_settings(
>> +                             struct samsung_clk_pll *pll, unsigned long
> rate)
>> +{
>> +     const struct samsung_pll_rate_table  *rate_table = pll-
>>rate_table;
>> +     int i;
>> +
>> +     for (i = 0; i < pll->rate_count; i++) {
>> +             if (rate == rate_table[i].rate)
>> +                     return &rate_table[i];
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static long samsung_pll_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate)
>> +{
>> +     struct samsung_clk_pll *pll = to_clk_pll(hw);
>> +     const struct samsung_pll_rate_table *rate_table = pll->rate_table;
>> +     int i;
>> +
>> +     /* Assumming rate_table is in descending order */
>> +     for (i = 0; i < pll->rate_count; i++) {
>> +             if (drate >= rate_table[i].rate)
>> +                     return rate_table[i].rate;
>> +     }
>> +
>> +     /* return minimum supported value */
>> +     return rate_table[i - 1].rate;
>> +}
>>  /*
>>   * PLL35xx Clock Type
>>   */
>> @@ -34,12 +64,17 @@ struct samsung_clk_pll {
>>  #define PLL35XX_CON0_OFFSET  (0x100)
>>  #define PLL35XX_CON1_OFFSET  (0x104)
>>
>> +/* 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_LOCK_STAT_MASK  (0x1)
>>  #define PLL35XX_MDIV_SHIFT      (16)
>>  #define PLL35XX_PDIV_SHIFT      (8)
>>  #define PLL35XX_SDIV_SHIFT      (0)
>> +#define PLL35XX_LOCK_STAT_SHIFT      (29)
>>
>>  static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw,
>>                               unsigned long parent_rate)
>> @@ -59,8 +94,70 @@ static unsigned long
>> samsung_pll35xx_recalc_rate(struct clk_hw *hw, return (unsigned
>> long)fvco;
>>  }
>>
>> +static inline bool samsung_pll35xx_mp_change(u32 mdiv, u32 pdiv, u32
>> pll_con) +{
>> +     if ((mdiv != ((pll_con >> PLL35XX_MDIV_SHIFT) &
> PLL35XX_MDIV_MASK)) ||
>> +             (pdiv != ((pll_con >> PLL35XX_PDIV_SHIFT) &
> PLL35XX_PDIV_MASK)))
>> +             return 1;
>> +     else
>> +             return 0;
>
> Readability of this function could be improved by moving some code out of
> the if clause, like:
>
> static inline bool samsung_pll35xx_mp_change(u32 mdiv,
>                                                 u32 pdiv, u32 pll_con)
> {
>         u32 old_mdiv, old_pdiv;
>
>         old_mdiv = (pll_con >> PLL35XX_MDIV_SHIFT) & PLL35XX_MDIV_MASK;
>         old_pdiv = (pll_con >> PLL35XX_PDIV_SHIFT) & PLL35XX_PDIV_MASK;
>
>         return (mdiv != old_mdiv || pdiv != old_pdiv);
> }
>

Yes, it looks neater, I have modified it V4.

>> +}
>> +
>> +static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long
>> drate, +                                      unsigned long prate)
>> +{
>> +     struct samsung_clk_pll *pll = to_clk_pll(hw);
>> +     const struct samsung_pll_rate_table *rate;
>> +     u32 tmp;
>> +
>> +     /* Get required rate settings from table */
>> +     rate = samsung_get_pll_settings(pll, drate);
>> +     if (!rate) {
>> +             pr_err("%s: Invalid rate : %lu for pll clk %s\n",
> __func__,
>> +                     drate, __clk_get_name(hw->clk));
>> +             return -EINVAL;
>> +     }
>> +
>> +     tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
>> +
>> +     if (!(samsung_pll35xx_mp_change(rate->mdiv, rate->pdiv, tmp))) {
>> +             /* If only s change, change just s value only*/
>> +             tmp &= ~(PLL35XX_SDIV_MASK << PLL35XX_SDIV_SHIFT);
>> +             tmp |= rate->sdiv << PLL35XX_SDIV_SHIFT;
>> +             pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
>
> To improve readability of this code, return 0 could be added here and
> following code could be moved out of the else clause.
>

Hmm... I can't see much difference but I have taken this also.

Thanks,
Yadwinder

> Best regards,
> Tomasz
>
>> +     } else {
>> +             /* Set PLL lock time. */
>> +             pll_writel(pll, rate->pdiv * PLL35XX_LOCK_FACTOR,
>> +                             PLL35XX_LOCK_OFFSET);
>> +
>> +             /* Change PLL PMS values */
>> +             tmp &= ~((PLL35XX_MDIV_MASK << PLL35XX_MDIV_SHIFT) |
>> +                             (PLL35XX_PDIV_MASK << PLL35XX_PDIV_SHIFT)
> |
>> +                             (PLL35XX_SDIV_MASK <<
> PLL35XX_SDIV_SHIFT));
>> +             tmp |= (rate->mdiv << PLL35XX_MDIV_SHIFT) |
>> +                             (rate->pdiv << PLL35XX_PDIV_SHIFT) |
>> +                             (rate->sdiv << PLL35XX_SDIV_SHIFT);
>> +             pll_writel(pll, tmp, PLL35XX_CON0_OFFSET);
>> +
>> +             /* wait_lock_time */
>> +             do {
>> +                     cpu_relax();
>> +                     tmp = pll_readl(pll, PLL35XX_CON0_OFFSET);
>> +             } while (!(tmp & (PLL35XX_LOCK_STAT_MASK
>> +                             << PLL35XX_LOCK_STAT_SHIFT)));
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  static const struct clk_ops samsung_pll35xx_clk_ops = {
>>       .recalc_rate = samsung_pll35xx_recalc_rate,
>> +     .round_rate = samsung_pll_round_rate,
>> +     .set_rate = samsung_pll35xx_set_rate,
>> +};
>> +
>> +static const struct clk_ops samsung_pll35xx_clk_min_ops = {
>> +     .recalc_rate = samsung_pll35xx_recalc_rate,
>>  };
>>
>>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
>> @@ -79,11 +176,15 @@ struct clk * __init
>> samsung_clk_register_pll35xx(const char *name, }
>>
>>       init.name = name;
>> -     init.ops = &samsung_pll35xx_clk_ops;
>>       init.flags = CLK_GET_RATE_NOCACHE;
>>       init.parent_names = &pname;
>>       init.num_parents = 1;
>>
>> +     if (rate_table && rate_count)
>> +             init.ops = &samsung_pll35xx_clk_ops;
>> +     else
>> +             init.ops = &samsung_pll35xx_clk_min_ops;
>> +
>>       pll->hw.init = &init;
>>       pll->base = base;
>>       pll->rate_table = rate_table;
>
>



More information about the linux-arm-kernel mailing list