Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

On 1 July 2017 at 03:22, Stephen Boyd  wrote:
> On 06/30, Chunyan Zhang wrote:
>> Hi Stephen,
>>
>> On 30 June 2017 at 09:44, Stephen Boyd  wrote:
>> > On 06/22, Chunyan Zhang wrote:
>> >> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>> >> > On 06/18, Chunyan Zhang wrote:
>> >> >> + 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.
>> >>
>> >
>> > Rename is fine, but pll can still be marked const?
>>
>> Oh, sorry I misunderstood :)
>> You mean mark the input parameter "pll" const, right?
>
> Yes.
>
>> >> >> +
>> >> >> +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.
>> >
>> > Does the hardware width change? Still not clear to me what's
>> > going on here.
>>
>> I heard from my colleague, that because the calculation precision on
>> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs,  when the
>> width of the value of PLL_KINT is larger than 21, the value is too
>> large to be multiplied on 32-bit Spreadtrum's SoCs.
>
> It sounds like you're saying that the clk hardware is not
> changing, but the sizeof(long) is different on 64-bit and 32-bit
> CPUs so you've added this ifndef here for that.
>

I finally figure out that this #ifndef is indeed not needed, thanks to
your querying :)

They told me that on Spreadtrum's 32-bit SoCs, only 20-bit of PLL_KINT
is used and 3-bit is reserved in order to keep in line with the PLLs
on 64-bit SoC.
And we can get the same effect only if we define PLL with the specific
'width' and 'shift' for PLL_KINT.

I'll remove this statement and 'i' from here and
sprd_pll_helper_recalc_rate() function.

>>
>> i = width < 21 ? 0 : i - 20;
>>
>> Here ' i ' is used to adjust 'shift' rather than 'width'  like below (
>> wrote the code back for convenience of 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

On 1 July 2017 at 03:22, Stephen Boyd  wrote:
> On 06/30, Chunyan Zhang wrote:
>> Hi Stephen,
>>
>> On 30 June 2017 at 09:44, Stephen Boyd  wrote:
>> > On 06/22, Chunyan Zhang wrote:
>> >> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>> >> > On 06/18, Chunyan Zhang wrote:
>> >> >> + 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.
>> >>
>> >
>> > Rename is fine, but pll can still be marked const?
>>
>> Oh, sorry I misunderstood :)
>> You mean mark the input parameter "pll" const, right?
>
> Yes.
>
>> >> >> +
>> >> >> +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.
>> >
>> > Does the hardware width change? Still not clear to me what's
>> > going on here.
>>
>> I heard from my colleague, that because the calculation precision on
>> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs,  when the
>> width of the value of PLL_KINT is larger than 21, the value is too
>> large to be multiplied on 32-bit Spreadtrum's SoCs.
>
> It sounds like you're saying that the clk hardware is not
> changing, but the sizeof(long) is different on 64-bit and 32-bit
> CPUs so you've added this ifndef here for that.
>

I finally figure out that this #ifndef is indeed not needed, thanks to
your querying :)

They told me that on Spreadtrum's 32-bit SoCs, only 20-bit of PLL_KINT
is used and 3-bit is reserved in order to keep in line with the PLLs
on 64-bit SoC.
And we can get the same effect only if we define PLL with the specific
'width' and 'shift' for PLL_KINT.

I'll remove this statement and 'i' from here and
sprd_pll_helper_recalc_rate() function.

>>
>> i = width < 21 ? 0 : i - 20;
>>
>> Here ' i ' is used to adjust 'shift' rather than 'width'  like below (
>> wrote the code back for convenience of understanding)
>>
>> +   kint = DIV_ROUND_CLOSEST(((fvco - refin * nint 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

On 06/30, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 30 June 2017 at 09:44, Stephen Boyd  wrote:
> > On 06/22, Chunyan Zhang wrote:
> >> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
> >> > On 06/18, Chunyan Zhang wrote:
> >> >> + 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.
> >>
> >
> > Rename is fine, but pll can still be marked const?
> 
> Oh, sorry I misunderstood :)
> You mean mark the input parameter "pll" const, right?

Yes.

> >> >> +
> >> >> +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.
> >
> > Does the hardware width change? Still not clear to me what's
> > going on here.
> 
> I heard from my colleague, that because the calculation precision on
> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs,  when the
> width of the value of PLL_KINT is larger than 21, the value is too
> large to be multiplied on 32-bit Spreadtrum's SoCs.

It sounds like you're saying that the clk hardware is not
changing, but the sizeof(long) is different on 64-bit and 32-bit
CPUs so you've added this ifndef here for that.

> 
> i = width < 21 ? 0 : i - 21;
> 
> Here ' i ' is used to adjust 'shift' rather than 'width'  like below (
> wrote the code back for convenience of understanding)
> 
> +   kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
> +   ((mask >> (shift + i)) + 1), refin * 100) << i;
> 

Having the types for all these variables would also be helpful.

  u32 mask, shift, width, kint, nint;
  unsigned long refin, fvco;

Why don't we do 64-bit math here instead of 32-bit math? And use
DIV_ROUND_CLOSEST_ULL?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

On 06/30, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 30 June 2017 at 09:44, Stephen Boyd  wrote:
> > On 06/22, Chunyan Zhang wrote:
> >> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
> >> > On 06/18, Chunyan Zhang wrote:
> >> >> + 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.
> >>
> >
> > Rename is fine, but pll can still be marked const?
> 
> Oh, sorry I misunderstood :)
> You mean mark the input parameter "pll" const, right?

Yes.

> >> >> +
> >> >> +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.
> >
> > Does the hardware width change? Still not clear to me what's
> > going on here.
> 
> I heard from my colleague, that because the calculation precision on
> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs,  when the
> width of the value of PLL_KINT is larger than 21, the value is too
> large to be multiplied on 32-bit Spreadtrum's SoCs.

It sounds like you're saying that the clk hardware is not
changing, but the sizeof(long) is different on 64-bit and 32-bit
CPUs so you've added this ifndef here for that.

> 
> i = width < 21 ? 0 : i - 21;
> 
> Here ' i ' is used to adjust 'shift' rather than 'width'  like below (
> wrote the code back for convenience of understanding)
> 
> +   kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
> +   ((mask >> (shift + i)) + 1), refin * 100) << i;
> 

Having the types for all these variables would also be helpful.

  u32 mask, shift, width, kint, nint;
  unsigned long refin, fvco;

Why don't we do 64-bit math here instead of 32-bit math? And use
DIV_ROUND_CLOSEST_ULL?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

Hi Stephen,

On 30 June 2017 at 09:44, Stephen Boyd  wrote:
> On 06/22, Chunyan Zhang wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
>> >> +#include 
>> >
>> > Is this include used? Should be clk-provider?
>>
>> Right, will remove it.
>>
>> >
>> >> +#include 
>> >> +#include 
>> >> +
>> >> +#include "ccu_pll.h"
>> >> +
>> >> +#define CCU_PLL_1M   100
>> >> +#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.
>>
>
> Rename is fine, but pll can still be marked const?

Oh, sorry I misunderstood :)
You mean mark the input parameter "pll" const, right?

>>
>> >
>> >> + 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 = 1;
>> >> + 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;
>> 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

Hi Stephen,

On 30 June 2017 at 09:44, Stephen Boyd  wrote:
> On 06/22, Chunyan Zhang wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
>> >> +#include 
>> >
>> > Is this include used? Should be clk-provider?
>>
>> Right, will remove it.
>>
>> >
>> >> +#include 
>> >> +#include 
>> >> +
>> >> +#include "ccu_pll.h"
>> >> +
>> >> +#define CCU_PLL_1M   100
>> >> +#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.
>>
>
> Rename is fine, but pll can still be marked const?

Oh, sorry I misunderstood :)
You mean mark the input parameter "pll" const, right?

>>
>> >
>> >> + 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 = 1;
>> >> + 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;
>> >> +
>> 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

On 06/22, Chunyan Zhang wrote:
> On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
> >> +#include 
> >
> > Is this include used? Should be clk-provider?
> 
> Right, will remove it.
> 
> >
> >> +#include 
> >> +#include 
> >> +
> >> +#include "ccu_pll.h"
> >> +
> >> +#define CCU_PLL_1M   100
> >> +#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.
> 

Rename is fine, but pll can still be marked const?
> 
> >
> >> + 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 = 1;
> >> + 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);
> >> + 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

On 06/22, Chunyan Zhang wrote:
> On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
> >> +#include 
> >
> > Is this include used? Should be clk-provider?
> 
> Right, will remove it.
> 
> >
> >> +#include 
> >> +#include 
> >> +
> >> +#include "ccu_pll.h"
> >> +
> >> +#define CCU_PLL_1M   100
> >> +#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.
> 

Rename is fine, but pll can still be marked const?
> 
> >
> >> + 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 = 1;
> >> + 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, 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

Hi Arnd,

On 22 June 2017 at 19:15, Arnd Bergmann  wrote:
> On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang  wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>>> On 06/18, Chunyan Zhang wrote:
>
 + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
 + ((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.
>
> I think you have to do both: normally the writel() is not guaranteed
> to arrive at the device until you read back from an address in the
> same device, so the delay must happen after the readl(), or you won't
> know how long to wait for.

I got it, will add the readl() in the next iterator.

Thanks,
Chunyan

>
>Arnd

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

Hi Arnd,

On 22 June 2017 at 19:15, Arnd Bergmann  wrote:
> On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang  wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>>> On 06/18, Chunyan Zhang wrote:
>
 + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
 + ((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.
>
> I think you have to do both: normally the writel() is not guaranteed
> to arrive at the device until you read back from an address in the
> same device, so the delay must happen after the readl(), or you won't
> know how long to wait for.

I got it, will add the readl() in the next iterator.

Thanks,
Chunyan

>
>Arnd

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Arnd Bergmann]

On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang  wrote:
> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>> On 06/18, Chunyan Zhang wrote:

>>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
>>> + ((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.

I think you have to do both: normally the writel() is not guaranteed
to arrive at the device until you read back from an address in the
same device, so the delay must happen after the readl(), or you won't
know how long to wait for.

   Arnd

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Arnd Bergmann]

On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang  wrote:
> On 20 June 2017 at 09:37, Stephen Boyd  wrote:
>> On 06/18, Chunyan Zhang wrote:

>>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/1) *
>>> + ((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.

I think you have to do both: normally the writel() is not guaranteed
to arrive at the device until you read back from an address in the
same device, so the delay must happen after the readl(), or you won't
know how long to wait for.

   Arnd

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
>> +#include 
>
> Is this include used? Should be clk-provider?

Right, will remove it.

>
>> +#include 
>> +#include 
>> +
>> +#include "ccu_pll.h"
>> +
>> +#define CCU_PLL_1M   100
>> +#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 = 1;
>> + 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 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Chunyan Zhang]

On 20 June 2017 at 09:37, Stephen Boyd  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 000..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 
>> +#include 
>
> Is this include used? Should be clk-provider?

Right, will remove it.

>
>> +#include 
>> +#include 
>> +
>> +#include "ccu_pll.h"
>> +
>> +#define CCU_PLL_1M   100
>> +#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 = 1;
>> + 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 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

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 000..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 
> +#include 

Is this include used? Should be clk-provider?

> +#include 
> +#include 
> +
> +#include "ccu_pll.h"
> +
> +#define CCU_PLL_1M   100
> +#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?

> +{
> + 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.

> + 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 = 1;
> + 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, 

Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support

[Stephen Boyd]

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 000..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 
> +#include 

Is this include used? Should be clk-provider?

> +#include 
> +#include 
> +
> +#include "ccu_pll.h"
> +
> +#define CCU_PLL_1M   100
> +#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?

> +{
> + 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.

> + 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 = 1;
> + 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,