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

[Elaine Zhang]

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@xxxxxxxxxxxxxx>
> >
> > Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
> > ---
> >   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