[PATCH 5/7] clk/samsung: add support for pll2550xx

[Rahul Sharma]

Hi Tomasz,

On 19 December 2013 17:31, Tomasz Figa <t.figa at samsung.com> wrote:
> Hi Pankaj, Rahul, Arun,
>
> On Friday 06 of December 2013 21:26:29 Rahul Sharma wrote:
>> From: Pankaj Dubey <pankaj.dubey at samsung.com>
>>
>> exynos5260 use pll2520xx and it has different bitfields
>> for P,M,S values as compared to pll2550xx. Support for
>> pll2520xx is added here.
>>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey at samsung.com>
>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>> Signed-off-by: Arun Kumar K <arun.kk at samsung.com>
>> ---
>>  drivers/clk/samsung/clk-pll.c |  107 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/samsung/clk-pll.h |    1 +
>>  2 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
>> index e8e8953..237a889 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -710,6 +710,107 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
>>       return clk;
>>  }
>>
>> +/*
>> + * PLL2550xx Clock Type
>> + */
>> +
>> +/* Maximum lock time can be 270 * PDIV cycles */
>> +#define PLL2550XX_LOCK_FACTOR (270)
>> +
>> +#define PLL2550XX_MDIV_MASK          (0x3FF)
>> +#define PLL2550XX_PDIV_MASK          (0x3F)
>> +#define PLL2550XX_SDIV_MASK          (0x7)
>> +#define PLL2550XX_LOCK_STAT_MASK     (0x1)
>> +#define PLL2550XX_MDIV_SHIFT         (9)
>> +#define PLL2550XX_PDIV_SHIFT         (3)
>> +#define PLL2550XX_SDIV_SHIFT         (0)
>> +#define PLL2550XX_LOCK_STAT_SHIFT    (21)
>> +
>> +static unsigned long samsung_pll2550xx_recalc_rate(struct clk_hw *hw,
>> +                             unsigned long parent_rate)
>> +{
>> +     struct samsung_clk_pll *pll = to_clk_pll(hw);
>> +     u32 mdiv, pdiv, sdiv, pll_con;
>> +     u64 fvco = parent_rate;
>> +
>> +     pll_con = __raw_readl(pll->con_reg);
>> +     mdiv = (pll_con >> PLL2550XX_MDIV_SHIFT) & PLL2550XX_MDIV_MASK;
>> +     pdiv = (pll_con >> PLL2550XX_PDIV_SHIFT) & PLL2550XX_PDIV_MASK;
>> +     sdiv = (pll_con >> PLL2550XX_SDIV_SHIFT) & PLL2550XX_SDIV_MASK;
>> +
>> +     fvco *= mdiv;
>> +     do_div(fvco, (pdiv << sdiv));
>> +
>> +     return (unsigned long)fvco;
>> +}
>> +
>> +static inline bool samsung_pll2550xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
>> +{
>> +     if ((mdiv != ((pll_con >> PLL2550XX_MDIV_SHIFT) &
>> +                             PLL2550XX_MDIV_MASK)) ||
>> +             (pdiv != ((pll_con >> PLL2550XX_PDIV_SHIFT) &
>> +                             PLL2550XX_PDIV_MASK)))
>> +             return 1;
>> +     else
>> +             return 0;
>
> This doesn't look too good. Can you make this consistent with
> implementations of this helper for other PLLs, such as
> samsung_pll35xx_mp_change()?

I have changed this in V2.

>
>> +}
>> +
>> +static int samsung_pll2550xx_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 = __raw_readl(pll->con_reg);
>> +
>> +     if (!(samsung_pll2550xx_mp_change(rate->mdiv, rate->pdiv, tmp))) {
>> +             /* If only s change, change just s value only*/
>> +             tmp &= ~(PLL2550XX_SDIV_MASK << PLL2550XX_SDIV_SHIFT);
>> +             tmp |= rate->sdiv << PLL2550XX_SDIV_SHIFT;
>> +             __raw_writel(tmp, pll->con_reg);
>> +     } else {
>
> Please make coding style of this function consistent with implementations
> of this operation for other PLLs, such as samsung_pll35xx_set_rate().
>
> Otherwise the patch looks fine.

Ok.

Regards,
Rahul Sharma.

>
> Best regards,
> Tomasz
>