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

Commit Message

Chunyan Zhang June 18, 2017, 1:58 a.m. UTC

Introduced a common adjustable pll clock driver for Spreadtrum SoCs.

Original-by: Xiaolong Zhang <[e-mail]>
Signed-off-by: Chunyan Zhang <[e-mail]>
---
 drivers/clk/sprd/Makefile  |   2 +-
 drivers/clk/sprd/ccu_pll.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/sprd/ccu_pll.h | 123 +++++++++++++++++++++++
 3 files changed, 365 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sprd/ccu_pll.c
 create mode 100644 drivers/clk/sprd/ccu_pll.h

Comments

Stephen Boyd June 20, 2017, 1:37 a.m. UTC | #1

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?

> +#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?

> +{
> +	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 = 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?

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

> +	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?

> +	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
need the bit field thing.

> +	u64 fvco;
> +	u16 fflag;
> +
> +	struct ccu_common	common;

Chunyan Zhang June 22, 2017, 10:17 a.m. UTC | #2

On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arnd Bergmann June 22, 2017, 11:15 a.m. UTC | #3

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

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chunyan Zhang June 22, 2017, 12:06 p.m. UTC | #4

Hi Arnd,

On 22 June 2017 at 19:15, Arnd Bergmann <[e-mail]> wrote:
> On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang <[e-mail]> wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> wrote:
>>> On 06/18, Chunyan Zhang wrote:
>
>>>> +     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.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 30, 2017, 1:44 a.m. UTC | #5

On 06/22, Chunyan Zhang wrote:
> On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> 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.
> 

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 = 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.

Does the hardware width change? Still not clear to me what's
going on here.

Chunyan Zhang June 30, 2017, 7:55 a.m. UTC | #6

Hi Stephen,

On 30 June 2017 at 09:44, Stephen Boyd <[e-mail]> wrote:
> On 06/22, Chunyan Zhang wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> 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.
>>
>
> 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 = 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.
>
> 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.

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)/10000) *
+       ((mask >> (shift + i)) + 1), refin * 100) << i;


Thanks for your review,
Chunyan

>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 30, 2017, 7:22 p.m. UTC | #7

On 06/30, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 30 June 2017 at 09:44, Stephen Boyd <[e-mail]> wrote:
> > On 06/22, Chunyan Zhang wrote:
> >> On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> 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)/10000) *
> +       ((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?

Chunyan Zhang July 3, 2017, 7:41 a.m. UTC | #8

On 1 July 2017 at 03:22, Stephen Boyd <[e-mail]> wrote:
> On 06/30, Chunyan Zhang wrote:
>> Hi Stephen,
>>
>> On 30 June 2017 at 09:44, Stephen Boyd <[e-mail]> wrote:
>> > On 06/22, Chunyan Zhang wrote:
>> >> On 20 June 2017 at 09:37, Stephen Boyd <[e-mail]> 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 * CCU_PLL_1M)/10000) *
>> +       ((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?

Agree, we should use 64-bit math, will address this.

Thanks,
Chunyan

>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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>
+#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)
+{
+	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 {
+		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
+	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);
+	}
+
+	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,
+	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
+ *
+ * @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;
+	u64 fvco;
+	u16 fflag;
+
+	struct ccu_common	common;
+};
+
+#define SPRD_CCU_PLL_WITH_ITABLE_FVCO(_struct, _name, _parent, _reg,	\
+				      _regs, _itable, _udelay,		\
+				      _factors,	_fvco, _fflag)		\
+	struct ccu_pll _struct = {					\
+		.regs		= _regs,				\
+		.itable		= _itable,				\
+		.udelay		= _udelay,				\
+		.factors	= _factors,				\
+		.fvco		= _fvco,				\
+		.fflag		= _fflag,				\
+		.common		= {					\
+			.reg		= _reg,				\
+			.hw.init	= CLK_HW_INIT(_name,		\
+						      _parent,		\
+						      &ccu_pll_ops,	\
+						CLK_IGNORE_UNUSED),	\
+		},							\
+	}
+
+#define SPRD_CCU_PLL_WITH_ITABLE(_struct, _name, _parent, _reg,		\
+				 _regs, _itable, _udelay,		\
+				 _factors)				\
+	SPRD_CCU_PLL_WITH_ITABLE_FVCO(_struct, _name, _parent, _reg,	\
+				      _regs, _itable, _udelay,		\
+				      _factors, 0, 0)
+
+static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_pll, common);
+}
+
+static inline u32 ccu_pll_readl(struct ccu_pll *pll, u8 index)
+{
+	struct ccu_common *common = &pll->common;
+
+	if (WARN_ON(index >= pll->regs[0]))
+		return 0;
+
+	return readl(common->base + pll->regs[index + 1]);
+}
+
+static inline void ccu_pll_writel(struct ccu_pll *pll, u8 index,
+				  u32 val, u32 msk)
+{
+	struct ccu_common *common = &pll->common;
+	void __iomem *addr;
+	u32 reg;
+
+	if (WARN_ON(index >= pll->regs[0]))
+		return;
+
+	addr = common->base + pll->regs[index + 1];
+	reg = readl(addr);
+	writel((reg & ~msk) | val, addr);
+}
+
+extern const struct clk_ops ccu_pll_ops;
+
+#endif /* _CCU_PLL_H_ */