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

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

On 07/10/2017 07:05 PM, Heiko Stuebner wrote:
> Hi Elaine,
>
> Am Freitag, 7. Juli 2017, 10:52:23 CEST schrieb Elaine Zhang:
>> add clk_fractional_divider_special_ops for rockchip specific requirements,
>> 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.igned-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> ---
>>   drivers/clk/clk-fractional-divider.c | 32 ++++++++++++++++++++++++++++++++
>>   drivers/clk/rockchip/clk.c           |  2 +-
>>   include/linux/clk-provider.h         |  1 +
>>   3 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
>> index aab904618eb6..3107b33327f9 100644
>> --- a/drivers/clk/clk-fractional-divider.c
>> +++ b/drivers/clk/clk-fractional-divider.c
>> @@ -158,6 +158,38 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider);
>>
>> +static long clk_fd_round_rate_special(struct clk_hw *hw, unsigned long rate,
>> +				      unsigned long *parent_rate)
>> +{
>
> this obviously still encodes Rockchip-specific things into the generic
> fractional-divider driver. And it's of course only special for Rockchip
> fractional dividers and will end it chaos if every implementation wants
> to add a "special" function there.
>
> Did you have a look at the patch I added to the last mail (for real this time)?
>

I think your patch is not most appropriate.
Because I think the rational_best_approximation is work well, I modified 
is parent_rate,
I just need to make sure the parent_rate > frac_rate * 20.
And I modify this like: (It is more appropriate?)

(I tested in RK SoCs. It's work well.)
diff --git a/drivers/clk/clk-fractional-divider.c 
b/drivers/clk/clk-fractional-divider.c
index e7a315e840e6..ccc9a98e53ce 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -62,6 +62,9 @@ static long clk_fd_round_rate(struct clk_hw *hw, 
unsigned long rate,
  	if (!rate || rate >= *parent_rate)
  		return *parent_rate;

+	if (fd->approx)
+		fd->approx(hw, rate, 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
@@ -147,6 +150,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 = NULL;
  	fd->hw.init = &init;

  	hw = &fd->hw;
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index e96a2e187862..a8ed48ffd531 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -160,6 +160,21 @@ static int rockchip_clk_frac_notifier_cb(struct 
notifier_block *nb,
  	return notifier_from_errno(ret);
  }

+void rockchip_fractional_special_approx(struct clk_hw *hw,
+					unsigned long rate,
+					unsigned long *parent_rate)
+{
+	struct clk_hw *p_parent;
+	unsigned long p_rate, p_parent_rate;
+
+	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;
+	}
+}
+
  static struct clk *rockchip_clk_register_frac_branch(
  		struct rockchip_clk_provider *ctx, const char *name,
  		const char *const *parent_names, u8 num_parents,
@@ -206,6 +221,7 @@ static struct clk *rockchip_clk_register_frac_branch(
  	div->nwidth = 16;
  	div->nmask = GENMASK(div->nwidth - 1, 0) << div->nshift;
  	div->lock = lock;
+	div->approx = rockchip_fractional_special_approx;
  	div_ops = &clk_fractional_divider_ops;

  	clk = clk_register_composite(NULL, name, parent_names, num_parents,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f70440a4edd7..6e4f6940e39e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -513,6 +513,9 @@ struct clk_fractional_divider {
  	u8		nwidth;
  	u32		nmask;
  	u8		flags;
+	void            (*approx)(struct clk_hw *hw,
+				  unsigned long rate,
+				  unsigned long *parent_rate);
  	spinlock_t	*lock;
  };



>
> Heiko
>
>
>
>