Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670161 > unrolled thread

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

Started byStephen Boyd <sboyd@codeaurora.org>
First post2017-06-20 03:40 +0200
Last post2017-07-03 09:50 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:40 +0200
    Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:20 +0200
      Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Arnd Bergmann <arnd@arndb.de> - 2017-06-22 13:20 +0200
        Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 14:10 +0200
      Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:50 +0200
        Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-30 10:00 +0200
          Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 21:30 +0200
            Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-07-03 09:50 +0200

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

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-20 03:40 +0200
SubjectRe: [PATCH V1 7/9] clk: sprd: add adjustable pll support
Message-ID<tUgj8-20K-19@gated-at.bofh.it>
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;

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

[toc] | [next] | [standalone]

#1672500

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

Right, will remove it.

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

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

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

Sure.

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

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

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

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

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

OK.

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

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

Thanks for your review,
Chunyan

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

[toc] | [prev] | [next] | [standalone]

#1672547

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-22 13:20 +0200
Message-ID<tV8jw-3Vn-19@gated-at.bofh.it>
In reply to#1672500
On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> 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

[toc] | [prev] | [next] | [standalone]

#1672570

FromChunyan Zhang <zhang.lyra@gmail.com>
Date2017-06-22 14:10 +0200
Message-ID<tV95U-4vV-25@gated-at.bofh.it>
In reply to#1672547
Hi Arnd,

On 22 June 2017 at 19:15, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> 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

[toc] | [prev] | [next] | [standalone]

#1678385

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-30 03:50 +0200
Message-ID<tXTeh-7bI-11@gated-at.bofh.it>
In reply to#1672500
On 06/22, Chunyan Zhang wrote:
> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/18, Chunyan Zhang wrote:
> >> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> >> index 83232e5..c593a93 100644
> >> --- a/drivers/clk/sprd/Makefile
> >> +++ b/drivers/clk/sprd/Makefile
> >> @@ -1,3 +1,3 @@
> >>  ifneq ($(CONFIG_OF),)
> >> -obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
> >> +obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
> >>  endif
> >> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
> >> new file mode 100644
> >> index 0000000..6c908e4
> >> --- /dev/null
> >> +++ b/drivers/clk/sprd/ccu_pll.c
> >> @@ -0,0 +1,241 @@
> >> +/*
> >> + * Spreadtrum pll clock driver
> >> + *
> >> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/clk.h>
> >
> > Is this include used? Should be clk-provider?
> 
> Right, will remove it.
> 
> >
> >> +#include <linux/err.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#include "ccu_pll.h"
> >> +
> >> +#define CCU_PLL_1M   1000000
> >> +#define CCU_PLL_10M  (CCU_PLL_1M * 10)
> >> +
> >> +#define pindex(pll, member)          \
> >> +     (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
> >> +
> >> +#define pshift(pll, member)          \
> >> +     (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
> >> +
> >> +#define pwidth(pll, member)          \
> >> +     pll->factors[member].width
> >> +
> >> +#define pmask(pll, member)                                   \
> >> +     ((pwidth(pll, member)) ?                                \
> >> +     GENMASK(pwidth(pll, member) + pshift(pll, member) - 1,  \
> >> +     pshift(pll, member)) : 0)
> >> +
> >> +#define pinternal(pll, cfg, member)  \
> >> +     (cfg[pindex(pll, member)] & pmask(pll, member))
> >> +
> >> +#define pinternal_val(pll, cfg, member)      \
> >> +     (pinternal(pll, cfg, member) >> pshift(pll, member))
> >> +
> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
> >
> > pll could be const?
> 
> What this function returns is a factor used to calculate the pll rate
> later, I will rename this function in the next iterator.
> 

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.

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

[toc] | [prev] | [next] | [standalone]

#1678576

FromChunyan Zhang <zhang.lyra@gmail.com>
Date2017-06-30 10:00 +0200
Message-ID<tXZ0m-2EL-25@gated-at.bofh.it>
In reply to#1678385
Hi Stephen,

On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/22, Chunyan Zhang wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/18, Chunyan Zhang wrote:
>> >> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> >> index 83232e5..c593a93 100644
>> >> --- a/drivers/clk/sprd/Makefile
>> >> +++ b/drivers/clk/sprd/Makefile
>> >> @@ -1,3 +1,3 @@
>> >>  ifneq ($(CONFIG_OF),)
>> >> -obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
>> >> +obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
>> >>  endif
>> >> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
>> >> new file mode 100644
>> >> index 0000000..6c908e4
>> >> --- /dev/null
>> >> +++ b/drivers/clk/sprd/ccu_pll.c
>> >> @@ -0,0 +1,241 @@
>> >> +/*
>> >> + * Spreadtrum pll clock driver
>> >> + *
>> >> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> >> + *
>> >> + * SPDX-License-Identifier: GPL-2.0
>> >> + */
>> >> +
>> >> +#include <linux/delay.h>
>> >> +#include <linux/clk.h>
>> >
>> > Is this include used? Should be clk-provider?
>>
>> Right, will remove it.
>>
>> >
>> >> +#include <linux/err.h>
>> >> +#include <linux/slab.h>
>> >> +
>> >> +#include "ccu_pll.h"
>> >> +
>> >> +#define CCU_PLL_1M   1000000
>> >> +#define CCU_PLL_10M  (CCU_PLL_1M * 10)
>> >> +
>> >> +#define pindex(pll, member)          \
>> >> +     (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
>> >> +
>> >> +#define pshift(pll, member)          \
>> >> +     (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
>> >> +
>> >> +#define pwidth(pll, member)          \
>> >> +     pll->factors[member].width
>> >> +
>> >> +#define pmask(pll, member)                                   \
>> >> +     ((pwidth(pll, member)) ?                                \
>> >> +     GENMASK(pwidth(pll, member) + pshift(pll, member) - 1,  \
>> >> +     pshift(pll, member)) : 0)
>> >> +
>> >> +#define pinternal(pll, cfg, member)  \
>> >> +     (cfg[pindex(pll, member)] & pmask(pll, member))
>> >> +
>> >> +#define pinternal_val(pll, cfg, member)      \
>> >> +     (pinternal(pll, cfg, member) >> pshift(pll, member))
>> >> +
>> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
>> >
>> > pll could be const?
>>
>> What this function returns is a factor used to calculate the pll rate
>> later, I will rename this function in the next iterator.
>>
>
> 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

[toc] | [prev] | [next] | [standalone]

#1679077

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-30 21:30 +0200
Message-ID<tY9M7-13z-35@gated-at.bofh.it>
In reply to#1678576
On 06/30, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/22, Chunyan Zhang wrote:
> >> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> 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?

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

[toc] | [prev] | [next] | [standalone]

#1679709

FromChunyan Zhang <zhang.lyra@gmail.com>
Date2017-07-03 09:50 +0200
Message-ID<tZ4hj-6qv-1@gated-at.bofh.it>
In reply to#1679077
On 1 July 2017 at 03:22, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/30, Chunyan Zhang wrote:
>> Hi Stephen,
>>
>> On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/22, Chunyan Zhang wrote:
>> >> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web