[RFC] clk: rockchip: rk3399: support pll setting automatically

Commit Message

zhengxing Aug. 2, 2016, 1:34 p.m. UTC

From: Elaine Zhang <[e-mail]>

The goal is that we can configure the most suitable pll params automatically.

If setting freq is not supported in rockchip_pll_rate_table rk3399_pll_rates[],
we can set pll params automatically.

Signed-off-by: Elaine Zhang <[e-mail]>
Signed-off-by: Xing Zheng <[e-mail]>
---

 drivers/clk/rockchip/clk-pll.c |  213 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 200 insertions(+), 13 deletions(-)

Comments

Heiko Stübner Aug. 28, 2016, 3:21 p.m. UTC | #1

Hi Xing, Elaine,

Am Dienstag, 2. August 2016, 21:34:12 schrieb Xing Zheng:
> From: Elaine Zhang <[e-mail]>
> 
> The goal is that we can configure the most suitable pll params
> automatically.
> 
> If setting freq is not supported in rockchip_pll_rate_table
> rk3399_pll_rates[], we can set pll params automatically.
> 
> Signed-off-by: Elaine Zhang <[e-mail]>
> Signed-off-by: Xing Zheng <[e-mail]>

first off, I really like that idea of calculating the generic pll frequencies 
instead of duplicating these entries in every soc clock driver.

Please also provide a follow up patch, removing the then unneeded frequencies 
from the rate tables.

I still have some comments inline below:

> ---
> 
>  drivers/clk/rockchip/clk-pll.c |  213
> +++++++++++++++++++++++++++++++++++++--- 1 file changed, 200 insertions(+),
> 13 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index 35994e8..08979f9 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -23,6 +23,7 @@
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/clk.h>
> +#include <linux/gcd.h>
>  #include "clk.h"
> 
>  #define PLL_MODE_MASK		0x3
> @@ -54,6 +55,200 @@ struct rockchip_clk_pll {
>  #define to_rockchip_clk_pll_nb(nb) \
>  			container_of(nb, struct rockchip_clk_pll, clk_nb)
> 
> +#define MHZ			(1000UL * 1000UL)
> +#define KHZ			(1000UL)
> +
> +/* CLK_PLL_TYPE_RK3066_AUTO type ops */

wouldn't that better be 
/* rk3066 pll frequency ranges */

> +#define PLL_FREF_MIN		(269 * KHZ)
> +#define PLL_FREF_MAX		(2200 * MHZ)
> +
> +#define PLL_FVCO_MIN		(440 * MHZ)
> +#define PLL_FVCO_MAX		(2200 * MHZ)
> +
> +#define PLL_FOUT_MIN		(27500 * KHZ)
> +#define PLL_FOUT_MAX		(2200 * MHZ)
> +
> +#define PLL_NF_MAX		(4096)
> +#define PLL_NR_MAX		(64)
> +#define PLL_NO_MAX		(16)

instead of using a comment, please prefix these constants accordingly, like
	RK3066_PLL_FVCO_MIN
etc, especially as some of those contstraints actually _are different_ between 
implementations. See rk3188 vs. rk3288 for example. With the current static 
rate table we just have matching values already, so never had to care about 
that.

Another idea might be to have the constraints as table in the soc clock-
drivers, similar to how we transfer we handle the rate tables right now.

> +
> +/* CLK_PLL_TYPE_RK3036/3366/3399_AUTO type ops */
> +#define MIN_FOUTVCO_FREQ	(800 * MHZ)
> +#define MAX_FOUTVCO_FREQ	(2000 * MHZ)

same here, RK3036_PLL_FOUTVCO_MIN etc.

> +
> +static struct rockchip_pll_rate_table auto_table;
> +
> +static struct rockchip_pll_rate_table *rk_pll_rate_table_get(void)
> +{
> +	return &auto_table;
> +}
> +
> +static int rockchip_pll_clk_set_postdiv(unsigned long fout_hz,

the rk3036, rk3366, rk3399 pll type won't be the last one Rockchip will use, 
so please add a prefix. Same for everything else below :-) .

> +					u32 *postdiv1,
> +					u32 *postdiv2,
> +					u32 *foutvco)
> +{
> +	unsigned long freq;
> +
> +	if (fout_hz < MIN_FOUTVCO_FREQ) {
> +		for (*postdiv1 = 1; *postdiv1 <= 7; (*postdiv1)++) {
> +			for (*postdiv2 = 1; *postdiv2 <= 7; (*postdiv2)++) {
> +				freq = fout_hz * (*postdiv1) * (*postdiv2);
> +				if (freq >= MIN_FOUTVCO_FREQ &&
> +				    freq <= MAX_FOUTVCO_FREQ) {
> +					*foutvco = freq;
> +					return 0;
> +				}
> +			}
> +			pr_err("CANNOT FIND postdiv1/2 to make fout in range from 800M to
> 2000M,fout = %lu\n", +			       fout_hz);
> +		}
> +	} else {
> +		*postdiv1 = 1;
> +		*postdiv2 = 1;
> +	}
> +	return 0;
> +}
> +
> +static struct rockchip_pll_rate_table *
> +rockchip_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
> +			     unsigned long fin_hz,
> +			     unsigned long fout_hz)
> +{
> +	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
> +	/* FIXME set postdiv1/2 always 1*/
> +	u32 foutvco = fout_hz;
> +	u64 fin_64, frac_64;
> +	u32 f_frac, postdiv1, postdiv2;
> +	unsigned long clk_gcd = 0;
> +
> +	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
> +		return NULL;
> +
> +	rockchip_pll_clk_set_postdiv(fout_hz, &postdiv1, &postdiv2, &foutvco);
> +	rate_table->postdiv1 = postdiv1;
> +	rate_table->postdiv2 = postdiv2;
> +	rate_table->dsmpd = 1;
> +
> +	if (fin_hz / MHZ * MHZ == fin_hz && fout_hz / MHZ * MHZ == fout_hz) {
> +		fin_hz /= MHZ;
> +		foutvco /= MHZ;
> +		clk_gcd = gcd(fin_hz, foutvco);
> +		rate_table->refdiv = fin_hz / clk_gcd;
> +		rate_table->fbdiv = foutvco / clk_gcd;
> +
> +		rate_table->frac = 0;
> +
> +		pr_debug("fin = %lu, fout = %lu, clk_gcd = %lu, refdiv = %u, fbdiv = 
%u,
> postdiv1 = %u, postdiv2 = %u, frac = %u\n", +			 fin_hz, fout_hz, 
clk_gcd,
> rate_table->refdiv,
> +			 rate_table->fbdiv, rate_table->postdiv1,
> +			 rate_table->postdiv2, rate_table->frac);
> +	} else {
> +		pr_debug("frac div running, fin_hz = %lu, fout_hz = %lu, fin_INT_mhz =
> %lu, fout_INT_mhz = %lu\n", +			 fin_hz, fout_hz,
> +			 fin_hz / MHZ * MHZ,
> +			 fout_hz / MHZ * MHZ);
> +		pr_debug("frac get postdiv1 = %u,  postdiv2 = %u, foutvco = %u\n",
> +			 rate_table->postdiv1, rate_table->postdiv2, foutvco);
> +		clk_gcd = gcd(fin_hz / MHZ, foutvco / MHZ);
> +		rate_table->refdiv = fin_hz / MHZ / clk_gcd;
> +		rate_table->fbdiv = foutvco / MHZ / clk_gcd;
> +		pr_debug("frac get refdiv = %u,  fbdiv = %u\n",
> +			 rate_table->refdiv, rate_table->fbdiv);
> +
> +		rate_table->frac = 0;
> +
> +		f_frac = (foutvco % MHZ);
> +		fin_64 = fin_hz;
> +		do_div(fin_64, (u64)rate_table->refdiv);
> +		frac_64 = (u64)f_frac << 24;
> +		do_div(frac_64, fin_64);
> +		rate_table->frac = (u32)frac_64;
> +		if (rate_table->frac > 0)
> +			rate_table->dsmpd = 0;
> +		pr_debug("frac = %x\n", rate_table->frac);
> +	}
> +	return rate_table;
> +}
> +
> +static struct rockchip_pll_rate_table *
> +rockchip_rk3066_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
> +				    unsigned long fin_hz,
> +				    unsigned long fout_hz)
> +{
> +	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
> +	u32 nr, nf, no, nonr;
> +	u32 nr_out, nf_out, no_out;
> +	u32 n;
> +	u32 numerator, denominator;
> +	u64 fref, fvco, fout;
> +	unsigned long clk_gcd = 0;
> +
> +	nr_out = PLL_NR_MAX + 1;
> +	nf_out = 0;
> +	no_out = 0;
> +
> +	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
> +		return NULL;
> +
> +	clk_gcd = gcd(fin_hz, fout_hz);
> +
> +	numerator = fout_hz / clk_gcd;
> +	denominator = fin_hz / clk_gcd;
> +
> +	for (n = 1;; n++) {
> +		nf = numerator * n;
> +		nonr = denominator * n;
> +		if (nf > PLL_NF_MAX || nonr > (PLL_NO_MAX * PLL_NR_MAX))
> +			break;
> +
> +		for (no = 1; no <= PLL_NO_MAX; no++) {
> +			if (!(no == 1 || !(no % 2)))
> +				continue;
> +
> +			if (nonr % no)
> +				continue;
> +			nr = nonr / no;
> +
> +			if (nr > PLL_NR_MAX)
> +				continue;
> +
> +			fref = fin_hz / nr;
> +			if (fref < PLL_FREF_MIN || fref > PLL_FREF_MAX)
> +				continue;
> +
> +			fvco = fref * nf;
> +			if (fvco < PLL_FVCO_MIN || fvco > PLL_FVCO_MAX)
> +				continue;
> +
> +			fout = fvco / no;
> +			if (fout < PLL_FOUT_MIN || fout > PLL_FOUT_MAX)
> +				continue;
> +
> +			/* select the best from all available PLL settings */
> +			if ((no > no_out) ||
> +			    ((no == no_out) && (nr < nr_out))) {
> +				nr_out = nr;
> +				nf_out = nf;
> +				no_out = no;
> +			}
> +		}
> +	}
> +
> +	/* output the best PLL setting */
> +	if ((nr_out <= PLL_NR_MAX) && (no_out > 0)) {
> +		if (rate_table->nr && rate_table->nf && rate_table->no) {
> +			rate_table->nr = nr_out;
> +			rate_table->nf = nf_out;
> +			rate_table->no = no_out;
> +		}
> +	} else {
> +		return NULL;
> +	}
> +
> +	return rate_table;
> +}
> +
>  static const struct rockchip_pll_rate_table *rockchip_get_pll_settings(
>  			    struct rockchip_clk_pll *pll, unsigned long rate)
>  {
> @@ -65,24 +260,16 @@ static const struct rockchip_pll_rate_table
> *rockchip_get_pll_settings( return &rate_table[i];
>  	}
> 
> -	return NULL;
> +	if (pll->type == pll_rk3066)

this should be a switch as well, to prepare for other pll types.

> +		return rockchip_rk3066_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
> +	else
> +		return rockchip_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
>  }
> 

Heiko
--
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

Heiko Stübner Aug. 29, 2016, 8:02 a.m. UTC | #2

Hi Xing, Elaine,

Am Dienstag, 2. August 2016, 21:34:12 schrieb Xing Zheng:
> From: Elaine Zhang <[e-mail]>
> 
> The goal is that we can configure the most suitable pll params
> automatically.
> 
> If setting freq is not supported in rockchip_pll_rate_table
> rk3399_pll_rates[], we can set pll params automatically.
> 
> Signed-off-by: Elaine Zhang <[e-mail]>
> Signed-off-by: Xing Zheng <[e-mail]>

first off, I really like that idea of calculating the generic pll frequencies 
instead of duplicating these entries in every soc clock driver.

Please also provide a follow up patch, removing the then unneeded frequencies 
from the rate tables.

I still have some comments inline below:

> ---
> 
>  drivers/clk/rockchip/clk-pll.c |  213
> +++++++++++++++++++++++++++++++++++++--- 1 file changed, 200 insertions(+),
> 13 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index 35994e8..08979f9 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -23,6 +23,7 @@
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/clk.h>
> +#include <linux/gcd.h>
>  #include "clk.h"
> 
>  #define PLL_MODE_MASK		0x3
> @@ -54,6 +55,200 @@ struct rockchip_clk_pll {
>  #define to_rockchip_clk_pll_nb(nb) \
>  			container_of(nb, struct rockchip_clk_pll, clk_nb)
> 
> +#define MHZ			(1000UL * 1000UL)
> +#define KHZ			(1000UL)
> +
> +/* CLK_PLL_TYPE_RK3066_AUTO type ops */

wouldn't that better be 
/* rk3066 pll frequency ranges */

> +#define PLL_FREF_MIN		(269 * KHZ)
> +#define PLL_FREF_MAX		(2200 * MHZ)
> +
> +#define PLL_FVCO_MIN		(440 * MHZ)
> +#define PLL_FVCO_MAX		(2200 * MHZ)
> +
> +#define PLL_FOUT_MIN		(27500 * KHZ)
> +#define PLL_FOUT_MAX		(2200 * MHZ)
> +
> +#define PLL_NF_MAX		(4096)
> +#define PLL_NR_MAX		(64)
> +#define PLL_NO_MAX		(16)

instead of using a comment, please prefix these constants accordingly, like
	RK3066_PLL_FVCO_MIN
etc, especially as some of those contstraints actually _are different_ between 
implementations. See rk3188 vs. rk3288 for example. With the current static 
rate table we just have matching values already, so never had to care about 
that.

Another idea might be to have the constraints as table in the soc clock-
drivers, similar to how we transfer we handle the rate tables right now.

> +
> +/* CLK_PLL_TYPE_RK3036/3366/3399_AUTO type ops */
> +#define MIN_FOUTVCO_FREQ	(800 * MHZ)
> +#define MAX_FOUTVCO_FREQ	(2000 * MHZ)

same here, RK3036_PLL_FOUTVCO_MIN etc.

> +
> +static struct rockchip_pll_rate_table auto_table;
> +
> +static struct rockchip_pll_rate_table *rk_pll_rate_table_get(void)
> +{
> +	return &auto_table;
> +}
> +
> +static int rockchip_pll_clk_set_postdiv(unsigned long fout_hz,

the rk3036, rk3366, rk3399 pll type won't be the last one Rockchip will use, 
so please add a prefix. Same for everything else below :-) .

> +					u32 *postdiv1,
> +					u32 *postdiv2,
> +					u32 *foutvco)
> +{
> +	unsigned long freq;
> +
> +	if (fout_hz < MIN_FOUTVCO_FREQ) {
> +		for (*postdiv1 = 1; *postdiv1 <= 7; (*postdiv1)++) {
> +			for (*postdiv2 = 1; *postdiv2 <= 7; (*postdiv2)++) {
> +				freq = fout_hz * (*postdiv1) * (*postdiv2);
> +				if (freq >= MIN_FOUTVCO_FREQ &&
> +				    freq <= MAX_FOUTVCO_FREQ) {
> +					*foutvco = freq;
> +					return 0;
> +				}
> +			}
> +			pr_err("CANNOT FIND postdiv1/2 to make fout in range from 800M to
> 2000M,fout = %lu\n", +			       fout_hz);
> +		}
> +	} else {
> +		*postdiv1 = 1;
> +		*postdiv2 = 1;
> +	}
> +	return 0;
> +}
> +
> +static struct rockchip_pll_rate_table *
> +rockchip_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
> +			     unsigned long fin_hz,
> +			     unsigned long fout_hz)
> +{
> +	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
> +	/* FIXME set postdiv1/2 always 1*/
> +	u32 foutvco = fout_hz;
> +	u64 fin_64, frac_64;
> +	u32 f_frac, postdiv1, postdiv2;
> +	unsigned long clk_gcd = 0;
> +
> +	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
> +		return NULL;
> +
> +	rockchip_pll_clk_set_postdiv(fout_hz, &postdiv1, &postdiv2, &foutvco);
> +	rate_table->postdiv1 = postdiv1;
> +	rate_table->postdiv2 = postdiv2;
> +	rate_table->dsmpd = 1;
> +
> +	if (fin_hz / MHZ * MHZ == fin_hz && fout_hz / MHZ * MHZ == fout_hz) {
> +		fin_hz /= MHZ;
> +		foutvco /= MHZ;
> +		clk_gcd = gcd(fin_hz, foutvco);
> +		rate_table->refdiv = fin_hz / clk_gcd;
> +		rate_table->fbdiv = foutvco / clk_gcd;
> +
> +		rate_table->frac = 0;
> +
> +		pr_debug("fin = %lu, fout = %lu, clk_gcd = %lu, refdiv = %u, fbdiv = 
%u,
> postdiv1 = %u, postdiv2 = %u, frac = %u\n", +			 fin_hz, fout_hz, 
clk_gcd,
> rate_table->refdiv,
> +			 rate_table->fbdiv, rate_table->postdiv1,
> +			 rate_table->postdiv2, rate_table->frac);
> +	} else {
> +		pr_debug("frac div running, fin_hz = %lu, fout_hz = %lu, fin_INT_mhz =
> %lu, fout_INT_mhz = %lu\n", +			 fin_hz, fout_hz,
> +			 fin_hz / MHZ * MHZ,
> +			 fout_hz / MHZ * MHZ);
> +		pr_debug("frac get postdiv1 = %u,  postdiv2 = %u, foutvco = %u\n",
> +			 rate_table->postdiv1, rate_table->postdiv2, foutvco);
> +		clk_gcd = gcd(fin_hz / MHZ, foutvco / MHZ);
> +		rate_table->refdiv = fin_hz / MHZ / clk_gcd;
> +		rate_table->fbdiv = foutvco / MHZ / clk_gcd;
> +		pr_debug("frac get refdiv = %u,  fbdiv = %u\n",
> +			 rate_table->refdiv, rate_table->fbdiv);
> +
> +		rate_table->frac = 0;
> +
> +		f_frac = (foutvco % MHZ);
> +		fin_64 = fin_hz;
> +		do_div(fin_64, (u64)rate_table->refdiv);
> +		frac_64 = (u64)f_frac << 24;
> +		do_div(frac_64, fin_64);
> +		rate_table->frac = (u32)frac_64;
> +		if (rate_table->frac > 0)
> +			rate_table->dsmpd = 0;
> +		pr_debug("frac = %x\n", rate_table->frac);
> +	}
> +	return rate_table;
> +}
> +
> +static struct rockchip_pll_rate_table *
> +rockchip_rk3066_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
> +				    unsigned long fin_hz,
> +				    unsigned long fout_hz)
> +{
> +	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
> +	u32 nr, nf, no, nonr;
> +	u32 nr_out, nf_out, no_out;
> +	u32 n;
> +	u32 numerator, denominator;
> +	u64 fref, fvco, fout;
> +	unsigned long clk_gcd = 0;
> +
> +	nr_out = PLL_NR_MAX + 1;
> +	nf_out = 0;
> +	no_out = 0;
> +
> +	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
> +		return NULL;
> +
> +	clk_gcd = gcd(fin_hz, fout_hz);
> +
> +	numerator = fout_hz / clk_gcd;
> +	denominator = fin_hz / clk_gcd;
> +
> +	for (n = 1;; n++) {
> +		nf = numerator * n;
> +		nonr = denominator * n;
> +		if (nf > PLL_NF_MAX || nonr > (PLL_NO_MAX * PLL_NR_MAX))
> +			break;
> +
> +		for (no = 1; no <= PLL_NO_MAX; no++) {
> +			if (!(no == 1 || !(no % 2)))
> +				continue;
> +
> +			if (nonr % no)
> +				continue;
> +			nr = nonr / no;
> +
> +			if (nr > PLL_NR_MAX)
> +				continue;
> +
> +			fref = fin_hz / nr;
> +			if (fref < PLL_FREF_MIN || fref > PLL_FREF_MAX)
> +				continue;
> +
> +			fvco = fref * nf;
> +			if (fvco < PLL_FVCO_MIN || fvco > PLL_FVCO_MAX)
> +				continue;
> +
> +			fout = fvco / no;
> +			if (fout < PLL_FOUT_MIN || fout > PLL_FOUT_MAX)
> +				continue;
> +
> +			/* select the best from all available PLL settings */
> +			if ((no > no_out) ||
> +			    ((no == no_out) && (nr < nr_out))) {
> +				nr_out = nr;
> +				nf_out = nf;
> +				no_out = no;
> +			}
> +		}
> +	}
> +
> +	/* output the best PLL setting */
> +	if ((nr_out <= PLL_NR_MAX) && (no_out > 0)) {
> +		if (rate_table->nr && rate_table->nf && rate_table->no) {
> +			rate_table->nr = nr_out;
> +			rate_table->nf = nf_out;
> +			rate_table->no = no_out;
> +		}
> +	} else {
> +		return NULL;
> +	}
> +
> +	return rate_table;
> +}
> +
>  static const struct rockchip_pll_rate_table *rockchip_get_pll_settings(
>  			    struct rockchip_clk_pll *pll, unsigned long rate)
>  {
> @@ -65,24 +260,16 @@ static const struct rockchip_pll_rate_table
> *rockchip_get_pll_settings( return &rate_table[i];
>  	}
> 
> -	return NULL;
> +	if (pll->type == pll_rk3066)

this should be a switch as well, to prepare for other pll types.

> +		return rockchip_rk3066_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
> +	else
> +		return rockchip_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
>  }
> 

Heiko
--
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

Doug Anderson Aug. 29, 2016, 5:35 p.m. UTC | #3

Hi,

On Sun, Aug 28, 2016 at 8:21 AM, Heiko Stübner <[e-mail]> wrote:
> Hi Xing, Elaine,
>
> Am Dienstag, 2. August 2016, 21:34:12 schrieb Xing Zheng:
>> From: Elaine Zhang <[e-mail]>
>>
>> The goal is that we can configure the most suitable pll params
>> automatically.
>>
>> If setting freq is not supported in rockchip_pll_rate_table
>> rk3399_pll_rates[], we can set pll params automatically.
>>
>> Signed-off-by: Elaine Zhang <[e-mail]>
>> Signed-off-by: Xing Zheng <[e-mail]>
>
> first off, I really like that idea of calculating the generic pll frequencies
> instead of duplicating these entries in every soc clock driver.

I probably won't have time to look at this any time soon, but when I
saw this patch fly by I was a little hesitant because:

* Not all PLL settings produce the same jitter.  On rk3288 I spent
loads of time searching for PLL settings that would be low jitter
enough to make a pixel clock that was happy with lots of different TVs
/ monitors.  It's true that there were certain heuristics that could
be followed but certainly my predictions didn't always match what
happened.

* Depending on what's using the PLL, you might want very different
settings.  One can optimize for lock time, power usage, jitter,
accuracy, etc.  For instance for a CPU clock that you change a lot you
probably don't care about hitting an exact frequency and you probably
don't care overly much about jitter, but you want low lock times.  For
HDMI you don't care about lock times and and probably don't care about
power usage, but you care about meeting an exact clock and really
really care about jitter.

* I know that in the past the way I found to make some of the best
rates for HDMI was to have the PLL make a rate that was several times
what I wanted and then activate a divider later in the clock tree.
That gave me the best jitter / clock accuracy.  I have no idea how to
do that automatically, but in the past I relied on the PLL only having
a fixed set of rates it could make so it would fail to make the lower
clock rate (higher jitter) and eventually end up with the better one.


Anyway, I don't know how to solve all of the above, but I just wanted
to bring it up as a concern.


-Doug
--
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

Heiko Stübner Aug. 29, 2016, 5:54 p.m. UTC | #4

Am Montag, 29. August 2016, 10:35:44 schrieb Doug Anderson:
> Hi,
> 
> On Sun, Aug 28, 2016 at 8:21 AM, Heiko Stübner <[e-mail]> wrote:
> > Hi Xing, Elaine,
> > 
> > Am Dienstag, 2. August 2016, 21:34:12 schrieb Xing Zheng:
> >> From: Elaine Zhang <[e-mail]>
> >> 
> >> The goal is that we can configure the most suitable pll params
> >> automatically.
> >> 
> >> If setting freq is not supported in rockchip_pll_rate_table
> >> rk3399_pll_rates[], we can set pll params automatically.
> >> 
> >> Signed-off-by: Elaine Zhang <[e-mail]>
> >> Signed-off-by: Xing Zheng <[e-mail]>
> > 
> > first off, I really like that idea of calculating the generic pll
> > frequencies instead of duplicating these entries in every soc clock
> > driver.
> 
> I probably won't have time to look at this any time soon, but when I
> saw this patch fly by I was a little hesitant because:
> 
> * Not all PLL settings produce the same jitter.  On rk3288 I spent
> loads of time searching for PLL settings that would be low jitter
> enough to make a pixel clock that was happy with lots of different TVs
> / monitors.  It's true that there were certain heuristics that could
> be followed but certainly my predictions didn't always match what
> happened.
> 
> * Depending on what's using the PLL, you might want very different
> settings.  One can optimize for lock time, power usage, jitter,
> accuracy, etc.  For instance for a CPU clock that you change a lot you
> probably don't care about hitting an exact frequency and you probably
> don't care overly much about jitter, but you want low lock times.  For
> HDMI you don't care about lock times and and probably don't care about
> power usage, but you care about meeting an exact clock and really
> really care about jitter.
> 
> * I know that in the past the way I found to make some of the best
> rates for HDMI was to have the PLL make a rate that was several times
> what I wanted and then activate a divider later in the clock tree.
> That gave me the best jitter / clock accuracy.  I have no idea how to
> do that automatically, but in the past I relied on the PLL only having
> a fixed set of rates it could make so it would fail to make the lower
> clock rate (higher jitter) and eventually end up with the better one.
> 
> 
> Anyway, I don't know how to solve all of the above, but I just wanted
> to bring it up as a concern.

after talking with Doug on irc, I realized that this really can become a 
problem, as right now we have a list with known-good rates, can adjust those 
as needed and have the clock framework select the best for a certain target 
rate.

But with the automatic calculation it would open up nearly unlimited different 
rates with possible negative effects and no way to eliminate or even test most 
of them.

So it may very well be best to stay with static rates, in a component as core 
as the PLLs are?


Heiko
--
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

zhangqing Aug. 30, 2016, 1:30 a.m. UTC | #5

On 08/30/2016 01:54 AM, Heiko Stübner wrote:
> Am Montag, 29. August 2016, 10:35:44 schrieb Doug Anderson:
>> Hi,
>>
>> On Sun, Aug 28, 2016 at 8:21 AM, Heiko Stübner <[e-mail]> wrote:
>>> Hi Xing, Elaine,
>>>
>>> Am Dienstag, 2. August 2016, 21:34:12 schrieb Xing Zheng:
>>>> From: Elaine Zhang <[e-mail]>
>>>>
>>>> The goal is that we can configure the most suitable pll params
>>>> automatically.
>>>>
>>>> If setting freq is not supported in rockchip_pll_rate_table
>>>> rk3399_pll_rates[], we can set pll params automatically.
>>>>
>>>> Signed-off-by: Elaine Zhang <[e-mail]>
>>>> Signed-off-by: Xing Zheng <[e-mail]>
>>>
>>> first off, I really like that idea of calculating the generic pll
>>> frequencies instead of duplicating these entries in every soc clock
>>> driver.
>>
>> I probably won't have time to look at this any time soon, but when I
>> saw this patch fly by I was a little hesitant because:
>>
>> * Not all PLL settings produce the same jitter.  On rk3288 I spent
>> loads of time searching for PLL settings that would be low jitter
>> enough to make a pixel clock that was happy with lots of different TVs
>> / monitors.  It's true that there were certain heuristics that could
>> be followed but certainly my predictions didn't always match what
>> happened.
>>
>> * Depending on what's using the PLL, you might want very different
>> settings.  One can optimize for lock time, power usage, jitter,
>> accuracy, etc.  For instance for a CPU clock that you change a lot you
>> probably don't care about hitting an exact frequency and you probably
>> don't care overly much about jitter, but you want low lock times.  For
>> HDMI you don't care about lock times and and probably don't care about
>> power usage, but you care about meeting an exact clock and really
>> really care about jitter.
>>
>> * I know that in the past the way I found to make some of the best
>> rates for HDMI was to have the PLL make a rate that was several times
>> what I wanted and then activate a divider later in the clock tree.
>> That gave me the best jitter / clock accuracy.  I have no idea how to
>> do that automatically, but in the past I relied on the PLL only having
>> a fixed set of rates it could make so it would fail to make the lower
>> clock rate (higher jitter) and eventually end up with the better one.
>>
>>
>> Anyway, I don't know how to solve all of the above, but I just wanted
>> to bring it up as a concern.
>
> after talking with Doug on irc, I realized that this really can become a
> problem, as right now we have a list with known-good rates, can adjust those
> as needed and have the clock framework select the best for a certain target
> rate.
>
> But with the automatic calculation it would open up nearly unlimited different
> rates with possible negative effects and no way to eliminate or even test most
> of them.
>
> So it may very well be best to stay with static rates, in a component as core
> as the PLLs are?

I also thought of this question, and really cant't solve all of the above.
According to before product experience,if the clk test jitter is not 
accept,we can add this freq to the static freq table.
And others use the pll setting automatically.

>
>
> Heiko
>
>
>

--
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/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 35994e8..08979f9 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -23,6 +23,7 @@ 
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
 #include <linux/clk.h>
+#include <linux/gcd.h>
 #include "clk.h"
 
 #define PLL_MODE_MASK		0x3
@@ -54,6 +55,200 @@  struct rockchip_clk_pll {
 #define to_rockchip_clk_pll_nb(nb) \
 			container_of(nb, struct rockchip_clk_pll, clk_nb)
 
+#define MHZ			(1000UL * 1000UL)
+#define KHZ			(1000UL)
+
+/* CLK_PLL_TYPE_RK3066_AUTO type ops */
+#define PLL_FREF_MIN		(269 * KHZ)
+#define PLL_FREF_MAX		(2200 * MHZ)
+
+#define PLL_FVCO_MIN		(440 * MHZ)
+#define PLL_FVCO_MAX		(2200 * MHZ)
+
+#define PLL_FOUT_MIN		(27500 * KHZ)
+#define PLL_FOUT_MAX		(2200 * MHZ)
+
+#define PLL_NF_MAX		(4096)
+#define PLL_NR_MAX		(64)
+#define PLL_NO_MAX		(16)
+
+/* CLK_PLL_TYPE_RK3036/3366/3399_AUTO type ops */
+#define MIN_FOUTVCO_FREQ	(800 * MHZ)
+#define MAX_FOUTVCO_FREQ	(2000 * MHZ)
+
+static struct rockchip_pll_rate_table auto_table;
+
+static struct rockchip_pll_rate_table *rk_pll_rate_table_get(void)
+{
+	return &auto_table;
+}
+
+static int rockchip_pll_clk_set_postdiv(unsigned long fout_hz,
+					u32 *postdiv1,
+					u32 *postdiv2,
+					u32 *foutvco)
+{
+	unsigned long freq;
+
+	if (fout_hz < MIN_FOUTVCO_FREQ) {
+		for (*postdiv1 = 1; *postdiv1 <= 7; (*postdiv1)++) {
+			for (*postdiv2 = 1; *postdiv2 <= 7; (*postdiv2)++) {
+				freq = fout_hz * (*postdiv1) * (*postdiv2);
+				if (freq >= MIN_FOUTVCO_FREQ &&
+				    freq <= MAX_FOUTVCO_FREQ) {
+					*foutvco = freq;
+					return 0;
+				}
+			}
+			pr_err("CANNOT FIND postdiv1/2 to make fout in range from 800M to 2000M,fout = %lu\n",
+			       fout_hz);
+		}
+	} else {
+		*postdiv1 = 1;
+		*postdiv2 = 1;
+	}
+	return 0;
+}
+
+static struct rockchip_pll_rate_table *
+rockchip_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
+			     unsigned long fin_hz,
+			     unsigned long fout_hz)
+{
+	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
+	/* FIXME set postdiv1/2 always 1*/
+	u32 foutvco = fout_hz;
+	u64 fin_64, frac_64;
+	u32 f_frac, postdiv1, postdiv2;
+	unsigned long clk_gcd = 0;
+
+	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
+		return NULL;
+
+	rockchip_pll_clk_set_postdiv(fout_hz, &postdiv1, &postdiv2, &foutvco);
+	rate_table->postdiv1 = postdiv1;
+	rate_table->postdiv2 = postdiv2;
+	rate_table->dsmpd = 1;
+
+	if (fin_hz / MHZ * MHZ == fin_hz && fout_hz / MHZ * MHZ == fout_hz) {
+		fin_hz /= MHZ;
+		foutvco /= MHZ;
+		clk_gcd = gcd(fin_hz, foutvco);
+		rate_table->refdiv = fin_hz / clk_gcd;
+		rate_table->fbdiv = foutvco / clk_gcd;
+
+		rate_table->frac = 0;
+
+		pr_debug("fin = %lu, fout = %lu, clk_gcd = %lu, refdiv = %u, fbdiv = %u, postdiv1 = %u, postdiv2 = %u, frac = %u\n",
+			 fin_hz, fout_hz, clk_gcd, rate_table->refdiv,
+			 rate_table->fbdiv, rate_table->postdiv1,
+			 rate_table->postdiv2, rate_table->frac);
+	} else {
+		pr_debug("frac div running, fin_hz = %lu, fout_hz = %lu, fin_INT_mhz = %lu, fout_INT_mhz = %lu\n",
+			 fin_hz, fout_hz,
+			 fin_hz / MHZ * MHZ,
+			 fout_hz / MHZ * MHZ);
+		pr_debug("frac get postdiv1 = %u,  postdiv2 = %u, foutvco = %u\n",
+			 rate_table->postdiv1, rate_table->postdiv2, foutvco);
+		clk_gcd = gcd(fin_hz / MHZ, foutvco / MHZ);
+		rate_table->refdiv = fin_hz / MHZ / clk_gcd;
+		rate_table->fbdiv = foutvco / MHZ / clk_gcd;
+		pr_debug("frac get refdiv = %u,  fbdiv = %u\n",
+			 rate_table->refdiv, rate_table->fbdiv);
+
+		rate_table->frac = 0;
+
+		f_frac = (foutvco % MHZ);
+		fin_64 = fin_hz;
+		do_div(fin_64, (u64)rate_table->refdiv);
+		frac_64 = (u64)f_frac << 24;
+		do_div(frac_64, fin_64);
+		rate_table->frac = (u32)frac_64;
+		if (rate_table->frac > 0)
+			rate_table->dsmpd = 0;
+		pr_debug("frac = %x\n", rate_table->frac);
+	}
+	return rate_table;
+}
+
+static struct rockchip_pll_rate_table *
+rockchip_rk3066_pll_clk_set_by_auto(struct rockchip_clk_pll *pll,
+				    unsigned long fin_hz,
+				    unsigned long fout_hz)
+{
+	struct rockchip_pll_rate_table *rate_table = rk_pll_rate_table_get();
+	u32 nr, nf, no, nonr;
+	u32 nr_out, nf_out, no_out;
+	u32 n;
+	u32 numerator, denominator;
+	u64 fref, fvco, fout;
+	unsigned long clk_gcd = 0;
+
+	nr_out = PLL_NR_MAX + 1;
+	nf_out = 0;
+	no_out = 0;
+
+	if (fin_hz == 0 || fout_hz == 0 || fout_hz == fin_hz)
+		return NULL;
+
+	clk_gcd = gcd(fin_hz, fout_hz);
+
+	numerator = fout_hz / clk_gcd;
+	denominator = fin_hz / clk_gcd;
+
+	for (n = 1;; n++) {
+		nf = numerator * n;
+		nonr = denominator * n;
+		if (nf > PLL_NF_MAX || nonr > (PLL_NO_MAX * PLL_NR_MAX))
+			break;
+
+		for (no = 1; no <= PLL_NO_MAX; no++) {
+			if (!(no == 1 || !(no % 2)))
+				continue;
+
+			if (nonr % no)
+				continue;
+			nr = nonr / no;
+
+			if (nr > PLL_NR_MAX)
+				continue;
+
+			fref = fin_hz / nr;
+			if (fref < PLL_FREF_MIN || fref > PLL_FREF_MAX)
+				continue;
+
+			fvco = fref * nf;
+			if (fvco < PLL_FVCO_MIN || fvco > PLL_FVCO_MAX)
+				continue;
+
+			fout = fvco / no;
+			if (fout < PLL_FOUT_MIN || fout > PLL_FOUT_MAX)
+				continue;
+
+			/* select the best from all available PLL settings */
+			if ((no > no_out) ||
+			    ((no == no_out) && (nr < nr_out))) {
+				nr_out = nr;
+				nf_out = nf;
+				no_out = no;
+			}
+		}
+	}
+
+	/* output the best PLL setting */
+	if ((nr_out <= PLL_NR_MAX) && (no_out > 0)) {
+		if (rate_table->nr && rate_table->nf && rate_table->no) {
+			rate_table->nr = nr_out;
+			rate_table->nf = nf_out;
+			rate_table->no = no_out;
+		}
+	} else {
+		return NULL;
+	}
+
+	return rate_table;
+}
+
 static const struct rockchip_pll_rate_table *rockchip_get_pll_settings(
 			    struct rockchip_clk_pll *pll, unsigned long rate)
 {
@@ -65,24 +260,16 @@  static const struct rockchip_pll_rate_table *rockchip_get_pll_settings(
 			return &rate_table[i];
 	}
 
-	return NULL;
+	if (pll->type == pll_rk3066)
+		return rockchip_rk3066_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
+	else
+		return rockchip_pll_clk_set_by_auto(pll, 24 * MHZ, rate);
 }
 
 static long rockchip_pll_round_rate(struct clk_hw *hw,
 			    unsigned long drate, unsigned long *prate)
 {
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate_table = pll->rate_table;
-	int i;
-
-	/* Assumming rate_table is in descending order */
-	for (i = 0; i < pll->rate_count; i++) {
-		if (drate >= rate_table[i].rate)
-			return rate_table[i].rate;
-	}
-
-	/* return minimum supported value */
-	return rate_table[i - 1].rate;
+	return drate;
 }
 
 /*