[PATCH v1] clk: fractional-divider: fix up the fractional clk's jitter

[PATCH v1] clk: fractional-divider: fix up the fractional clk's jitter

[Elaine Zhang]

fractional divider must set that denominator is 20 times larger than
numerator to generate precise clock frequency.
Otherwise the CLK jitter is very big, poor quality of the clock signal.

RK document description:
3.1.9  Fractional divider usage
To get specific frequency, clocks of I2S, SPDIF, UARTcan be generated by
fractional divider. Generally you must set that denominator is 20 times
larger than numerator to generate precise clock frequency. So the
fractional divider applies only to generate low frequency clock like
I2S, UART.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/clk/clk-fractional-divider.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index aab904618eb6..1c29d6f5ffd8 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -56,11 +56,24 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long scale;
 	unsigned long m, n;
 	u64 ret;
+	struct clk_hw *p_parent;
+	unsigned long p_rate, p_parent_rate;
 
 	if (!rate || rate >= *parent_rate)
 		return *parent_rate;
 
 	/*
+	 * fractional divider must set that denominator is 20 times larger than
+	 * numerator to generate precise clock frequency.
+	 */
+	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
+		p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
+		p_parent_rate = clk_hw_get_rate(p_parent);
+		*parent_rate = p_parent_rate;
+	}
+
+	/*
 	 * Get rate closer to *parent_rate to guarantee there is no overflow
 	 * for m and n. In the result it will be the nearest rate left shifted
 	 * by (scale - fd->nwidth) bits.
-- 
1.9.1

Re: [PATCH v1] clk: fractional-divider: fix up the fractional clk's jitter

[Heiko Stübner]

Hi Elaine,

Am Donnerstag, 6. Juli 2017, 16:28:34 CEST schrieb Elaine Zhang:
> fractional divider must set that denominator is 20 times larger than
> numerator to generate precise clock frequency.
> Otherwise the CLK jitter is very big, poor quality of the clock signal.
> 
> RK document description:
> 3.1.9  Fractional divider usage
> To get specific frequency, clocks of I2S, SPDIF, UARTcan be generated by
> fractional divider. Generally you must set that denominator is 20 times
> larger than numerator to generate precise clock frequency. So the
> fractional divider applies only to generate low frequency clock like
> I2S, UART.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  drivers/clk/clk-fractional-divider.c | 13 +++++++++++++

As I said in our previous private mails, having soc-specific quirks in
the generic driver won't work.

I just realized that while I mentioned a different approach in one my
mails, I seem to have forgotten to attach the patch showcasing my idea.
[Just realized that while looking at the old mails] Sorry about that.

So the idea would be allowing clk-fractional users to just provide their
own approximation function, which you could then provide one
for the rockchip-specific requirements. That change was only compile-
tested at the time, so you might want to check if it actually works,


For the below you would "just" need to create an approximation
function and adapt the rockchip_clk_register_frac_branch
to set div->approx to your approximation function.


Heiko


---------
From: Heiko Stuebner <heiko@sntech.de>
Date: Fri, 2 Jun 2017 13:42:51 +0200
Subject: [PATCH] clk: fractional-divider: allow clk-specific approximation
 functions

Fractional dividers may have special requirements concerning numerator
and denominator selection that differ from just getting the best
approximation.

For example on Rockchip socs the denominator must be at least 20 times
larger than the numerator to generate precise clock frequencies.

Therefore add the ability to provide custom approximation functions
but fall back to rational_best_approximation in the default case.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk-fractional-divider.c | 42 ++++++++++++++++++++++++++++++++----
 include/linux/clk-provider.h         | 17 +++++++++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index aab904618eb6..2a066d8182e1 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -69,7 +69,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	if (scale > fd->nwidth)
 		rate <<= scale - fd->nwidth;
 
-	rational_best_approximation(rate, *parent_rate,
+	fd->approx(rate, *parent_rate,
 			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
 			&m, &n);
 
@@ -87,7 +87,7 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long m, n;
 	u32 val;
 
-	rational_best_approximation(rate, parent_rate,
+	fd->approx(rate, parent_rate,
 			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
 			&m, &n);
 
@@ -116,10 +116,13 @@ const struct clk_ops clk_fractional_divider_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
 
-struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
+struct clk_hw *clk_hw_register_fractional_divider_custom(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
-		u8 clk_divider_flags, spinlock_t *lock)
+		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
+				(unsigned long gnum, unsigned long gdenom,
+				 unsigned long mnum, unsigned long mdenom,
+				 unsigned long *bnum, unsigned long *bdenom))
 {
 	struct clk_fractional_divider *fd;
 	struct clk_init_data init;
@@ -145,6 +148,7 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
 	fd->nmask = GENMASK(nwidth - 1, 0) << nshift;
 	fd->flags = clk_divider_flags;
 	fd->lock = lock;
+	fd->approx = approx;
 	fd->hw.init = &init;
 
 	hw = &fd->hw;
@@ -156,8 +160,38 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
 
 	return hw;
 }
+EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider_custom);
+
+struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock)
+{
+	return clk_hw_register_fractional_divider_custom(dev, name, parent_name,
+		flags, reg, mshift, mwidth, nshift, nwidth, clk_divider_flags,
+		lock, rational_best_approximation);
+}
 EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider);
 
+struct clk *clk_register_fractional_divider_custom(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
+				(unsigned long gnum, unsigned long gdenom,
+				 unsigned long mnum, unsigned long mdenom,
+				 unsigned long *bnum, unsigned long *bdenom))
+{
+	struct clk_hw *hw;
+
+	hw = clk_hw_register_fractional_divider_custom(dev, name, parent_name,
+			flags, reg, mshift, mwidth, nshift, nwidth,
+			clk_divider_flags, lock, approx);
+	if (IS_ERR(hw))
+		return ERR_CAST(hw);
+	return hw->clk;
+}
+EXPORT_SYMBOL_GPL(clk_register_fractional_divider_custom);
+
 struct clk *clk_register_fractional_divider(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a428aec36ace..5bf2bc49ccb7 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -564,16 +564,33 @@ struct clk_fractional_divider {
 	u8		nwidth;
 	u32		nmask;
 	u8		flags;
+	void		(*approx)(unsigned long gnum, unsigned long gdenom,
+				  unsigned long mnum, unsigned long mdenom,
+				  unsigned long *bnum, unsigned long *bdenom);
 	spinlock_t	*lock;
 };
 
 #define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
 
 extern const struct clk_ops clk_fractional_divider_ops;
+struct clk *clk_register_fractional_divider_custom(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
+				(unsigned long gnum, unsigned long gdenom,
+				 unsigned long mnum, unsigned long mdenom,
+				 unsigned long *bnum, unsigned long *bdenom));
 struct clk *clk_register_fractional_divider(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
 		u8 clk_divider_flags, spinlock_t *lock);
+struct clk_hw *clk_hw_register_fractional_divider_custom(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
+				(unsigned long gnum, unsigned long gdenom,
+				 unsigned long mnum, unsigned long mdenom,
+				 unsigned long *bnum, unsigned long *bdenom));
 struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
-- 
2.11.0

Re: [PATCH v1] clk: fractional-divider: fix up the fractional clk's jitter

[Elaine Zhang]

On 07/06/2017 06:01 PM, Heiko Stübner wrote:
> Hi Elaine,
>
> Am Donnerstag, 6. Juli 2017, 16:28:34 CEST schrieb Elaine Zhang:
>> fractional divider must set that denominator is 20 times larger than
>> numerator to generate precise clock frequency.
>> Otherwise the CLK jitter is very big, poor quality of the clock signal.
>>
>> RK document description:
>> 3.1.9  Fractional divider usage
>> To get specific frequency, clocks of I2S, SPDIF, UARTcan be generated by
>> fractional divider. Generally you must set that denominator is 20 times
>> larger than numerator to generate precise clock frequency. So the
>> fractional divider applies only to generate low frequency clock like
>> I2S, UART.
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> ---
>>   drivers/clk/clk-fractional-divider.c | 13 +++++++++++++
>
> As I said in our previous private mails, having soc-specific quirks in
> the generic driver won't work.
>
> I just realized that while I mentioned a different approach in one my
> mails, I seem to have forgotten to attach the patch showcasing my idea.
> [Just realized that while looking at the old mails] Sorry about that.
>
> So the idea would be allowing clk-fractional users to just provide their
> own approximation function, which you could then provide one
> for the rockchip-specific requirements. That change was only compile-
> tested at the time, so you might want to check if it actually works,
>
>
> For the below you would "just" need to create an approximation
> function and adapt the rockchip_clk_register_frac_branch
> to set div->approx to your approximation function.
>
>
> Heiko
>
>
> ---------
> From: Heiko Stuebner <heiko@sntech.de>
> Date: Fri, 2 Jun 2017 13:42:51 +0200
> Subject: [PATCH] clk: fractional-divider: allow clk-specific approximation
>   functions
>
> Fractional dividers may have special requirements concerning numerator
> and denominator selection that differ from just getting the best
> approximation.
>
> For example on Rockchip socs the denominator must be at least 20 times
> larger than the numerator to generate precise clock frequencies.
>
> Therefore add the ability to provide custom approximation functions
> but fall back to rational_best_approximation in the default case.

The custom approximation functions is not need.
The rational_best_approximation is work well, I modified is parent_rate,
I just need to make sure the parent_rate > frac_rate * 20.
According to your patch, I can't correction the parent_rate.

In clk_fd_round_rate() function:
fd->approx(rate, *parent_rate,
    			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
    			&m, &n);
I just need to set the *parent_rate >= 20* rate.

>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/clk/clk-fractional-divider.c | 42 ++++++++++++++++++++++++++++++++----
>   include/linux/clk-provider.h         | 17 +++++++++++++++
>   2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index aab904618eb6..2a066d8182e1 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -69,7 +69,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>   	if (scale > fd->nwidth)
>   		rate <<= scale - fd->nwidth;
>
> -	rational_best_approximation(rate, *parent_rate,
> +	fd->approx(rate, *parent_rate,
>   			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
>   			&m, &n);
>
> @@ -87,7 +87,7 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
>   	unsigned long m, n;
>   	u32 val;
>
> -	rational_best_approximation(rate, parent_rate,
> +	fd->approx(rate, parent_rate,
>   			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
>   			&m, &n);
>
> @@ -116,10 +116,13 @@ const struct clk_ops clk_fractional_divider_ops = {
>   };
>   EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
>
> -struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
> +struct clk_hw *clk_hw_register_fractional_divider_custom(struct device *dev,
>   		const char *name, const char *parent_name, unsigned long flags,
>   		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> -		u8 clk_divider_flags, spinlock_t *lock)
> +		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
> +				(unsigned long gnum, unsigned long gdenom,
> +				 unsigned long mnum, unsigned long mdenom,
> +				 unsigned long *bnum, unsigned long *bdenom))
>   {
>   	struct clk_fractional_divider *fd;
>   	struct clk_init_data init;
> @@ -145,6 +148,7 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
>   	fd->nmask = GENMASK(nwidth - 1, 0) << nshift;
>   	fd->flags = clk_divider_flags;
>   	fd->lock = lock;
> +	fd->approx = approx;
>   	fd->hw.init = &init;
>
>   	hw = &fd->hw;
> @@ -156,8 +160,38 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
>
>   	return hw;
>   }
> +EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider_custom);
> +
> +struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +		u8 clk_divider_flags, spinlock_t *lock)
> +{
> +	return clk_hw_register_fractional_divider_custom(dev, name, parent_name,
> +		flags, reg, mshift, mwidth, nshift, nwidth, clk_divider_flags,
> +		lock, rational_best_approximation);
> +}
>   EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider);
>
> +struct clk *clk_register_fractional_divider_custom(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
> +				(unsigned long gnum, unsigned long gdenom,
> +				 unsigned long mnum, unsigned long mdenom,
> +				 unsigned long *bnum, unsigned long *bdenom))
> +{
> +	struct clk_hw *hw;
> +
> +	hw = clk_hw_register_fractional_divider_custom(dev, name, parent_name,
> +			flags, reg, mshift, mwidth, nshift, nwidth,
> +			clk_divider_flags, lock, approx);
> +	if (IS_ERR(hw))
> +		return ERR_CAST(hw);
> +	return hw->clk;
> +}
> +EXPORT_SYMBOL_GPL(clk_register_fractional_divider_custom);
> +
>   struct clk *clk_register_fractional_divider(struct device *dev,
>   		const char *name, const char *parent_name, unsigned long flags,
>   		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index a428aec36ace..5bf2bc49ccb7 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -564,16 +564,33 @@ struct clk_fractional_divider {
>   	u8		nwidth;
>   	u32		nmask;
>   	u8		flags;
> +	void		(*approx)(unsigned long gnum, unsigned long gdenom,
> +				  unsigned long mnum, unsigned long mdenom,
> +				  unsigned long *bnum, unsigned long *bdenom);
>   	spinlock_t	*lock;
>   };
>
>   #define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
>
>   extern const struct clk_ops clk_fractional_divider_ops;
> +struct clk *clk_register_fractional_divider_custom(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
> +				(unsigned long gnum, unsigned long gdenom,
> +				 unsigned long mnum, unsigned long mdenom,
> +				 unsigned long *bnum, unsigned long *bdenom));
>   struct clk *clk_register_fractional_divider(struct device *dev,
>   		const char *name, const char *parent_name, unsigned long flags,
>   		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
>   		u8 clk_divider_flags, spinlock_t *lock);
> +struct clk_hw *clk_hw_register_fractional_divider_custom(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +		u8 clk_divider_flags, spinlock_t *lock, void (*approx)
> +				(unsigned long gnum, unsigned long gdenom,
> +				 unsigned long mnum, unsigned long mdenom,
> +				 unsigned long *bnum, unsigned long *bdenom));
>   struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
>   		const char *name, const char *parent_name, unsigned long flags,
>   		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
>