On Sat, Jun 8, 2013 at 5:42 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
> On Friday 31 of May 2013 18:01:33 Vikas Sajjan wrote:
>> From: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx>
>>
>> This patch add set_rate() and round_rate() for PLL35xx
>>
>> Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx>
>> ---
>> 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;
>
>
--
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
