Message-ID: <CAAfSe-sMy3aM9=LwpVivvHKy4hVw3GEd3U-hZe17pXM5fJRxPg@mail.gmail.com>
Date:   Thu, 22 Jun 2017 18:17:26 +0800
From:   Chunyan Zhang <zhang.lyra@...il.com>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     Chunyan Zhang <chunyan.zhang@...eadtrum.com>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-clk@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Arnd Bergmann <arnd@...db.de>, Mark Brown <broonie@...nel.org>,
        Xiaolong Zhang <xiaolong.zhang@...eadtrum.com>,
        Orson Zhai <orson.zhai@...eadtrum.com>,
        Geng Ren <geng.ren@...eadtrum.com>,
        Ben Li <ben.li@...eadtrum.com>
Subject: Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

On 20 June 2017 at 09:37, Stephen Boyd <sboyd@...eaurora.org> wrote:
> On 06/18, Chunyan Zhang wrote:
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> index 83232e5..c593a93 100644
>> --- a/drivers/clk/sprd/Makefile
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -1,3 +1,3 @@
>>  ifneq ($(CONFIG_OF),)
>> -obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
>> +obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
>>  endif
>> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
>> new file mode 100644
>> index 0000000..6c908e4
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_pll.c
>> @@ -0,0 +1,241 @@
>> +/*
>> + * Spreadtrum pll clock driver
>> + *
>> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/clk.h>
>
> Is this include used? Should be clk-provider?

Right, will remove it.

>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +
>> +#include "ccu_pll.h"
>> +
>> +#define CCU_PLL_1M   1000000
>> +#define CCU_PLL_10M  (CCU_PLL_1M * 10)
>> +
>> +#define pindex(pll, member)          \
>> +     (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
>> +
>> +#define pshift(pll, member)          \
>> +     (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
>> +
>> +#define pwidth(pll, member)          \
>> +     pll->factors[member].width
>> +
>> +#define pmask(pll, member)                                   \
>> +     ((pwidth(pll, member)) ?                                \
>> +     GENMASK(pwidth(pll, member) + pshift(pll, member) - 1,  \
>> +     pshift(pll, member)) : 0)
>> +
>> +#define pinternal(pll, cfg, member)  \
>> +     (cfg[pindex(pll, member)] & pmask(pll, member))
>> +
>> +#define pinternal_val(pll, cfg, member)      \
>> +     (pinternal(pll, cfg, member) >> pshift(pll, member))
>> +
>> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
>
> pll could be const?

What this function returns is a factor used to calculate the pll rate
later, I will rename this function in the next iterator.

>
>> +{
>> +     u8 shift, index, refin_id = 3;
>> +     u32 mask;
>> +     const unsigned long refin[4] = { 2, 4, 13, 26 };
>> +
>> +     if (pwidth(pll, PLL_REFIN)) {
>> +             index = pindex(pll, PLL_REFIN);
>> +             shift = pshift(pll, PLL_REFIN);
>> +             mask = pmask(pll, PLL_REFIN);
>> +             refin_id = (ccu_pll_readl(pll, index) & mask) >> shift;
>> +             if (refin_id > 3)
>> +                     refin_id = 3;
>> +     }
>> +
>> +     return refin[refin_id];
>> +}
>> +
>> +static u8 pll_get_ibias(unsigned long rate, const u64 *table)
>> +{
>> +     u64 i;
>> +     u8 num = table[0];
>> +
>> +     for (i = 0; i < num; i++)
>> +             if (rate <= table[i + 1])
>> +                     break;
>> +
>> +     return i == num ? num - 1 : i;
>> +}
>> +
>> +static unsigned long ccu_pll_helper_recalc_rate(struct ccu_pll *pll,
>> +                                             unsigned long parent_rate)
>> +{
>> +     unsigned long rate, refin, k1, k2;
>> +     unsigned long kint = 0, nint;
>> +     u32 reg_num = pll->regs[0];
>> +     u32 *cfg;
>> +     u32 i;
>> +     u32 mask;
>> +
>> +     cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> +     if (!cfg)
>> +             return -ENOMEM;
>> +
>> +     for (i = 0; i < reg_num; i++)
>> +             cfg[i] = ccu_pll_readl(pll, i);
>> +
>> +     refin = pll_get_refin_rate(pll);
>> +
>> +     if (pinternal(pll, cfg, PLL_PREDIV))
>> +             refin = refin * 2;
>> +
>> +     if (pwidth(pll, PLL_POSTDIV) &&
>> +         ((pll->fflag == 1 && pinternal(pll, cfg, PLL_POSTDIV)) ||
>> +          (!pll->fflag && !pinternal(pll, cfg, PLL_POSTDIV))))
>> +             refin = refin / 2;
>> +
>> +     if (!pinternal(pll, cfg, PLL_DIV_S))
>> +             rate = refin * pinternal_val(pll, cfg, PLL_N) * CCU_PLL_10M;
>> +     else {
>
> Please include braces on the if as well when another branch has them.

Sure.

>
>> +             nint = pinternal_val(pll, cfg, PLL_NINT);
>> +             if (pinternal(pll, cfg, PLL_SDM_EN))
>> +                     kint = pinternal_val(pll, cfg, PLL_KINT);
>> +
>> +             mask = pmask(pll, PLL_KINT);
>> +#ifdef CONFIG_64BIT
>> +             k1 = 1000;
>> +             k2 = 1000;
>> +             rate = DIV_ROUND_CLOSEST(refin * kint * k1,
>> +                                      ((mask >> __ffs(mask)) + 1)) *
>> +                                      k2 + refin * nint * CCU_PLL_1M;
>> +#else
>> +             k1 = 100;
>> +             k2 = 10000;
>> +             i = pwidth(pll, PLL_KINT);
>> +             i = i < 21 ? 0 : i - 21;
>> +             rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
>> +                                      ((mask >> (__ffs(mask) + i)) + 1)) *
>> +                                      k2 + refin * nint * CCU_PLL_1M;
>> +#endif
>> +     }
>> +
>> +     return rate;
>> +}
>> +
>> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
>> +                                unsigned long rate,
>> +                                unsigned long parent_rate)
>> +{
>> +     u32 mask, shift, width, ibias_val, index, kint, nint;
>> +     u32 reg_num = pll->regs[0], i = 0;
>> +     unsigned long refin, fvco = rate;
>> +     struct reg_cfg *cfg;
>> +
>> +     cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> +     if (!cfg)
>> +             return -ENOMEM;
>> +
>> +     refin = pll_get_refin_rate(pll);
>> +
>> +     mask = pmask(pll, PLL_PREDIV);
>> +     index = pindex(pll, PLL_PREDIV);
>> +     width = pwidth(pll, PLL_PREDIV);
>> +     if (width && (ccu_pll_readl(pll, index) & mask))
>> +             refin = refin * 2;
>> +
>> +     mask = pmask(pll, PLL_POSTDIV);
>> +     index = pindex(pll, PLL_POSTDIV);
>> +     width = pwidth(pll, PLL_POSTDIV);
>> +     cfg[index].msk = mask;
>> +     if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
>> +                   (pll->fflag == 0 && fvco > pll->fvco)))
>> +             cfg[index].val |= mask;
>> +
>> +     if (width && fvco <= pll->fvco)
>> +             fvco = fvco * 2;
>> +
>> +     mask = pmask(pll, PLL_DIV_S);
>> +     index = pindex(pll, PLL_DIV_S);
>> +     cfg[index].val |= mask;
>> +     cfg[index].msk |= mask;
>> +
>> +     mask = pmask(pll, PLL_SDM_EN);
>> +     index = pindex(pll, PLL_SDM_EN);
>> +     cfg[index].val |= mask;
>> +     cfg[index].msk |= mask;
>> +
>> +     nint  = fvco/(refin * CCU_PLL_1M);
>> +
>> +     mask = pmask(pll, PLL_NINT);
>> +     index = pindex(pll, PLL_NINT);
>> +     shift = pshift(pll, PLL_NINT);
>> +     cfg[index].val |= (nint << shift) & mask;
>> +     cfg[index].msk |= mask;
>> +
>> +     mask = pmask(pll, PLL_KINT);
>> +     index = pindex(pll, PLL_KINT);
>> +     width = pwidth(pll, PLL_KINT);
>> +     shift = pshift(pll, PLL_KINT);
>> +#ifndef CONFIG_64BIT
>> +     i = width < 21 ? 0 : i - 21;
>> +#endif
>
> What's this? Why do we depend on CONFIG_64BIT?

On 32-bit SoCs, the largest width we can support is limited due to the
limitation of calculation precision.

>
>> +     kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
>> +     ((mask >> (shift + i)) + 1), refin * 100) << i;
>> +     cfg[index].val |= (kint << shift) & mask;
>> +     cfg[index].msk |= mask;
>> +
>> +     ibias_val = pll_get_ibias(fvco, pll->itable);
>> +
>> +     mask = pmask(pll, PLL_IBIAS);
>> +     index = pindex(pll, PLL_IBIAS);
>> +     shift = pshift(pll, PLL_IBIAS);
>> +     cfg[index].val |= ibias_val << shift & mask;
>> +     cfg[index].msk |= mask;
>> +
>> +     for (i = 0; i < reg_num; i++) {
>> +             if (cfg[i].msk)
>> +                     ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
>> +     }
>> +
>
> Are we waiting for the writel() to go through above? If so we
> need a readl() of the same register to make sure the write has
> completed before delaying.

After writing these configuration registers, we have to wait a certain
time to make sure the pll has worked as we configured.  This depends
on other circuit part, so we use udelay rather than reading the same
register.

>
>> +     udelay(pll->udelay);
>> +
>> +     return 0;
>> +}
>> +
>> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> +                                      unsigned long parent_rate)
>> +{
>> +     struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> +     return ccu_pll_helper_recalc_rate(pll, parent_rate);
>> +}
>> +
>> +static int ccu_pll_set_rate(struct clk_hw *hw,
>> +                         unsigned long rate,
>> +                         unsigned long parent_rate)
>> +{
>> +     struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> +     return ccu_pll_helper_set_rate(pll, rate, parent_rate);
>> +}
>> +
>> +static int ccu_pll_clk_prepare(struct clk_hw *hw)
>> +{
>> +     struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> +     udelay(pll->udelay);
>> +
>> +     return 0;
>> +}
>> +
>> +static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                            unsigned long *prate)
>> +{
>> +     return rate;
>> +}
>> +
>> +const struct clk_ops ccu_pll_ops = {
>> +     .prepare = ccu_pll_clk_prepare,
>> +     .recalc_rate = ccu_pll_recalc_rate,
>> +     .round_rate = ccu_pll_round_rate,
>> +     .set_rate = ccu_pll_set_rate,
>> +};
>> diff --git a/drivers/clk/sprd/ccu_pll.h b/drivers/clk/sprd/ccu_pll.h
>> new file mode 100644
>> index 0000000..66fe5d1
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_pll.h
>> @@ -0,0 +1,123 @@
>> +/*
>> + * Spreadtrum clock pll configurations
>> + *
>> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#ifndef _CCU_PLL_H_
>> +#define _CCU_PLL_H_
>> +
>> +#include "ccu_common.h"
>> +
>> +struct reg_cfg {
>> +     u32 val;
>> +     u32 msk;
>> +};
>> +
>> +struct ccu_bit_field {
>> +     u8 shift;
>> +     u8 width;
>> +};
>> +
>> +enum {
>> +     PLL_LOCK_DONE = 0,
>
> Drop the = 0 please unless it's needed for something?

OK.

>
>> +     PLL_DIV_S,
>> +     PLL_MOD_EN,
>> +     PLL_SDM_EN,
>> +     PLL_REFIN,
>> +     PLL_IBIAS,
>> +     PLL_N,
>> +     PLL_NINT,
>> +     PLL_KINT,
>> +     PLL_PREDIV,
>> +     PLL_POSTDIV,
>> +
>> +     PLL_FACT_MAX
>> +};
>> +
>> +/*
>> + * struct ccu_pll - defination of adjustable pll clock
>
> s/defination/definition/
>
>> + *
>> + * @reg:     registers used to set the configuration of pll clock,
>> + *           reg[0] shows how many registers this pll clock uses.
>> + * @itable:  pll ibias table, itable[0] means how many items this
>> + *           table includes
>> + * @udelay   delay time after setting rate
>> + * @factors  used to calculate the pll clock rate
>> + * @fvco:    fvco threshold rate
>> + * @fflag:   fvco flag
>> + */
>> +struct ccu_pll {
>> +     const u32 *regs;
>> +     const u64 *itable;
>> +     u16 udelay;
>> +     const struct ccu_bit_field *factors;
>
> Does this change across the different PLLs? Would be nice to not

Yes, different PLL has diferent bit field arragement, even on the same SoC.

Thanks for your review,
Chunyan

> need the bit field thing.
>
>> +     u64 fvco;
>> +     u16 fflag;
>> +
>> +     struct ccu_common       common;
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Powered by blists - more mailing lists