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

[Elaine Zhang <zhangqing@rock-chips.com>]

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