[PATCH v2 1/3] clk: fractional-divider: Export approximation algo to the CCF users

[PATCH v2 1/3] clk: fractional-divider: Export approximation algo to the CCF users

[Andy Shevchenko]

At least one user currently duplicates some functions that are provided
by fractional divider module. Let's export approximation algo and replace
the open-coded variant.

As a bonus the exported function will get better documentation in place.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: fixed compilation error (LKP), successfully compile-tested on x86
 drivers/clk/clk-fractional-divider.c | 11 +++++++----
 drivers/clk/clk-fractional-divider.h |  9 +++++++++
 drivers/clk/rockchip/clk.c           | 17 +++--------------
 3 files changed, 19 insertions(+), 18 deletions(-)
 create mode 100644 drivers/clk/clk-fractional-divider.h

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index b1e556f20911..535d299af646 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -14,6 +14,8 @@
 #include <linux/slab.h>
 #include <linux/rational.h>
 
+#include "clk-fractional-divider.h"
+
 static inline u32 clk_fd_readl(struct clk_fractional_divider *fd)
 {
 	if (fd->flags & CLK_FRAC_DIVIDER_BIG_ENDIAN)
@@ -68,9 +70,10 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static void clk_fd_general_approximation(struct clk_hw *hw, unsigned long rate,
-					 unsigned long *parent_rate,
-					 unsigned long *m, unsigned long *n)
+void clk_fractional_divider_general_approximation(struct clk_hw *hw,
+						  unsigned long rate,
+						  unsigned long *parent_rate,
+						  unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long scale;
@@ -102,7 +105,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	if (fd->approximation)
 		fd->approximation(hw, rate, parent_rate, &m, &n);
 	else
-		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
+		clk_fractional_divider_general_approximation(hw, rate, parent_rate, &m, &n);
 
 	ret = (u64)*parent_rate * m;
 	do_div(ret, n);
diff --git a/drivers/clk/clk-fractional-divider.h b/drivers/clk/clk-fractional-divider.h
new file mode 100644
index 000000000000..4fa359a12ef4
--- /dev/null
+++ b/drivers/clk/clk-fractional-divider.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+struct clk_hw;
+
+void clk_fractional_divider_general_approximation(struct clk_hw *hw,
+						  unsigned long rate,
+						  unsigned long *parent_rate,
+						  unsigned long *m,
+						  unsigned long *n);
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 049e5e0b64f6..b7be7e11b0df 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -22,6 +22,8 @@
 #include <linux/regmap.h>
 #include <linux/reboot.h>
 #include <linux/rational.h>
+
+#include "../clk-fractional-divider.h"
 #include "clk.h"
 
 /*
@@ -178,10 +180,8 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 		unsigned long rate, unsigned long *parent_rate,
 		unsigned long *m, unsigned long *n)
 {
-	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long p_rate, p_parent_rate;
 	struct clk_hw *p_parent;
-	unsigned long scale;
 
 	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
 	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
@@ -190,18 +190,7 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 		*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.
-	 */
-	scale = fls_long(*parent_rate / rate - 1);
-	if (scale > fd->nwidth)
-		rate <<= scale - fd->nwidth;
-
-	rational_best_approximation(rate, *parent_rate,
-			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
-			m, n);
+	clk_fractional_divider_general_approximation(hw, rate, parent_rate, m, n);
 }
 
 static struct clk *rockchip_clk_register_frac_branch(
-- 
2.30.2

[PATCH v2 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Andy Shevchenko]

The newly introduced flag, when set, makes the flow to skip
the assumption that the caller will use an additional 2^scale
prescaler to get the desired clock rate.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: moved entire snipped under new flag check (Liu)
 drivers/clk/clk-fractional-divider.c | 10 ++++++----
 include/linux/clk-provider.h         |  5 +++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 535d299af646..5f4b6a8aef67 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -76,16 +76,18 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 						  unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
-	unsigned long scale;
 
 	/*
 	 * 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.
 	 */
-	scale = fls_long(*parent_rate / rate - 1);
-	if (scale > fd->nwidth)
-		rate <<= scale - fd->nwidth;
+	if (!(fd->flags & CLK_FRAC_DIVIDER_NO_PRESCALER)) {
+		unsigned long scale = fls_long(*parent_rate / rate - 1);
+
+		if (scale > fd->nwidth)
+			rate <<= scale - fd->nwidth;
+	}
 
 	rational_best_approximation(rate, *parent_rate,
 			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index d83b829305c0..f74d0afe275f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1001,6 +1001,10 @@ struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
  * CLK_FRAC_DIVIDER_BIG_ENDIAN - By default little endian register accesses are
  *	used for the divider register.  Setting this flag makes the register
  *	accesses big endian.
+ * CLK_FRAC_DIVIDER_NO_PRESCALER - By default the resulting rate may be shifted
+ *	left by a few bits in case when the asked one is quite small to satisfy
+ *	the desired range of denominator. If the caller wants to get the best
+ *	rate without using an additional prescaler, this flag may be set.
  */
 struct clk_fractional_divider {
 	struct clk_hw	hw;
@@ -1022,6 +1026,7 @@ struct clk_fractional_divider {
 
 #define CLK_FRAC_DIVIDER_ZERO_BASED		BIT(0)
 #define CLK_FRAC_DIVIDER_BIG_ENDIAN		BIT(1)
+#define CLK_FRAC_DIVIDER_NO_PRESCALER		BIT(2)
 
 extern const struct clk_ops clk_fractional_divider_ops;
 struct clk *clk_register_fractional_divider(struct device *dev,
-- 
2.30.2

[PATCH v2 3/3] clk: fractional-divider: Document the arithmetics used behind the code

[Andy Shevchenko]

It appears that some code lines raise the question why they are needed
and how they are participated in the calculus of the resulting values.

Document this in a form of the top comment in the module file.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: renamed variables in formulas to follow the code, added floor()
 drivers/clk/clk-fractional-divider.c | 34 +++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 5f4b6a8aef67..7f7f688f8de5 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -3,8 +3,38 @@
  * Copyright (C) 2014 Intel Corporation
  *
  * Adjustable fractional divider clock implementation.
- * Output rate = (m / n) * parent_rate.
  * Uses rational best approximation algorithm.
+ *
+ * Output is calculated as
+ *
+ *	rate = (m / n) * parent_rate				(1)
+ *
+ * This is useful when on die we have a prescaler block which asks for
+ * m (numerator) and n (denominator) values to be provided to satisfy
+ * the (1) as much as possible.
+ *
+ * Since m and n have the limitation by a range, e.g.
+ *
+ *	n >= 1, n < N_width, where N_width = 2^nwidth		(2)
+ *
+ * for some cases the output may be saturated. Hence, from (1) and (2),
+ * assuming the worst case when m = 1, the inequality
+ *
+ *	floor(log2(parent_rate / rate)) <= nwidth		(3)
+ *
+ * may be derived. Thus, in cases when
+ *
+ *	(parent_rate / rate) >> N_width				(4)
+ *
+ * we scale up the rate by 2^scale, where
+ *
+ *	scale = floor(log2(parent_rate / rate)) - nwidth	(5)
+ *
+ * and assume that the IP, that needs m and n, has also its own
+ * prescaler, which is capable to divide by 2^scale. In this way
+ * we get the denominator to satisfy the desired range (2) and
+ * at the same time much much better result of m and n than simple
+ * saturated values.
  */
 
 #include <linux/clk-provider.h>
@@ -81,6 +111,8 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 	 * 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.
+	 *
+	 * For the detailed explanation see the top comment in this file.
 	 */
 	if (!(fd->flags & CLK_FRAC_DIVIDER_NO_PRESCALER)) {
 		unsigned long scale = fls_long(*parent_rate / rate - 1);
-- 
2.30.2

Re: [PATCH v2 1/3] clk: fractional-divider: Export approximation algo to the CCF users

[Heiko Stübner]

Am Freitag, 16. Juli 2021, 15:34:46 CEST schrieb Andy Shevchenko:
> At least one user currently duplicates some functions that are provided
> by fractional divider module. Let's export approximation algo and replace
> the open-coded variant.
> 
> As a bonus the exported function will get better documentation in place.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

on Rockchip rk3288 and rk3399
Tested-by: Heiko Stuebner <heiko@sntech.de>

Also for dropping the Rockchip-specific copy in favor of the main
implementation
Acked-by: Heiko Stuebner <heiko@sntech.de>



> ---
> v2: fixed compilation error (LKP), successfully compile-tested on x86
>  drivers/clk/clk-fractional-divider.c | 11 +++++++----
>  drivers/clk/clk-fractional-divider.h |  9 +++++++++
>  drivers/clk/rockchip/clk.c           | 17 +++--------------
>  3 files changed, 19 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/clk/clk-fractional-divider.h
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index b1e556f20911..535d299af646 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -14,6 +14,8 @@
>  #include <linux/slab.h>
>  #include <linux/rational.h>
>  
> +#include "clk-fractional-divider.h"
> +
>  static inline u32 clk_fd_readl(struct clk_fractional_divider *fd)
>  {
>  	if (fd->flags & CLK_FRAC_DIVIDER_BIG_ENDIAN)
> @@ -68,9 +70,10 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
>  	return ret;
>  }
>  
> -static void clk_fd_general_approximation(struct clk_hw *hw, unsigned long rate,
> -					 unsigned long *parent_rate,
> -					 unsigned long *m, unsigned long *n)
> +void clk_fractional_divider_general_approximation(struct clk_hw *hw,
> +						  unsigned long rate,
> +						  unsigned long *parent_rate,
> +						  unsigned long *m, unsigned long *n)
>  {
>  	struct clk_fractional_divider *fd = to_clk_fd(hw);
>  	unsigned long scale;
> @@ -102,7 +105,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>  	if (fd->approximation)
>  		fd->approximation(hw, rate, parent_rate, &m, &n);
>  	else
> -		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
> +		clk_fractional_divider_general_approximation(hw, rate, parent_rate, &m, &n);
>  
>  	ret = (u64)*parent_rate * m;
>  	do_div(ret, n);
> diff --git a/drivers/clk/clk-fractional-divider.h b/drivers/clk/clk-fractional-divider.h
> new file mode 100644
> index 000000000000..4fa359a12ef4
> --- /dev/null
> +++ b/drivers/clk/clk-fractional-divider.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +struct clk_hw;
> +
> +void clk_fractional_divider_general_approximation(struct clk_hw *hw,
> +						  unsigned long rate,
> +						  unsigned long *parent_rate,
> +						  unsigned long *m,
> +						  unsigned long *n);
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 049e5e0b64f6..b7be7e11b0df 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -22,6 +22,8 @@
>  #include <linux/regmap.h>
>  #include <linux/reboot.h>
>  #include <linux/rational.h>
> +
> +#include "../clk-fractional-divider.h"
>  #include "clk.h"
>  
>  /*
> @@ -178,10 +180,8 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
>  		unsigned long rate, unsigned long *parent_rate,
>  		unsigned long *m, unsigned long *n)
>  {
> -	struct clk_fractional_divider *fd = to_clk_fd(hw);
>  	unsigned long p_rate, p_parent_rate;
>  	struct clk_hw *p_parent;
> -	unsigned long scale;
>  
>  	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
>  	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
> @@ -190,18 +190,7 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
>  		*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.
> -	 */
> -	scale = fls_long(*parent_rate / rate - 1);
> -	if (scale > fd->nwidth)
> -		rate <<= scale - fd->nwidth;
> -
> -	rational_best_approximation(rate, *parent_rate,
> -			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
> -			m, n);
> +	clk_fractional_divider_general_approximation(hw, rate, parent_rate, m, n);
>  }
>  
>  static struct clk *rockchip_clk_register_frac_branch(
> 

Re: [PATCH v2 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Liu Ying]

On Fri, 2021-07-16 at 16:34 +0300, Andy Shevchenko wrote:
> The newly introduced flag, when set, makes the flow to skip
> the assumption that the caller will use an additional 2^scale
> prescaler to get the desired clock rate.

As I mentioned in v1 comment, it seems to be good to decouple the
prescaler knowledge from this common fractional divider clk driver.
This way, we'll make it simpler and easier to maintain. Also, then, the
NO_PRESCALER flag is not needed at all.  However, it seems that two
Intel drivers which use the frational divider drivers will be affected
and rate negotiation logics need to be implemented for them.  Please
consider if it's doable or not.

If we ultimately keep the prescaler knowledge here, please consider to
add the NO_PRESCALER flag for i.MX7ulp as it hasn't the prescaler IIUC.

Regards,
Liu Ying

> 
> Reported-by: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: moved entire snipped under new flag check (Liu)
>  drivers/clk/clk-fractional-divider.c | 10 ++++++----
>  include/linux/clk-provider.h         |  5 +++++
>  2 files changed, 11 insertions(+), 4 deletions(-)

Re: [PATCH v2 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Andy Shevchenko]

On Thu, Jul 22, 2021 at 12:33 PM Liu Ying <victor.liu@nxp.com> wrote:
> On Fri, 2021-07-16 at 16:34 +0300, Andy Shevchenko wrote:
> > The newly introduced flag, when set, makes the flow to skip
> > the assumption that the caller will use an additional 2^scale
> > prescaler to get the desired clock rate.
>
> As I mentioned in v1 comment, it seems to be good to decouple the
> prescaler knowledge from this common fractional divider clk driver.
> This way, we'll make it simpler and easier to maintain. Also, then, the
> NO_PRESCALER flag is not needed at all.  However, it seems that two
> Intel drivers which use the frational divider drivers will be affected
> and rate negotiation logics need to be implemented for them.  Please
> consider if it's doable or not.

The current driver works for the certain hardware without this change.
If you think it's better, submit a proposal we will discuss.

> If we ultimately keep the prescaler knowledge here, please consider to
> add the NO_PRESCALER flag for i.MX7ulp as it hasn't the prescaler IIUC.

You mean there is a code which is currently using this driver w/o
taking into account this prescaller flavour? Can you, please, point
out, I'll definitely update it. Thanks for the catch!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Liu Ying]

On Thu, 2021-07-22 at 12:38 +0300, Andy Shevchenko wrote:
> On Thu, Jul 22, 2021 at 12:33 PM Liu Ying <victor.liu@nxp.com> wrote:
> > On Fri, 2021-07-16 at 16:34 +0300, Andy Shevchenko wrote:
> > > The newly introduced flag, when set, makes the flow to skip
> > > the assumption that the caller will use an additional 2^scale
> > > prescaler to get the desired clock rate.
> > 
> > As I mentioned in v1 comment, it seems to be good to decouple the
> > prescaler knowledge from this common fractional divider clk driver.
> > This way, we'll make it simpler and easier to maintain. Also, then, the
> > NO_PRESCALER flag is not needed at all.  However, it seems that two
> > Intel drivers which use the frational divider drivers will be affected
> > and rate negotiation logics need to be implemented for them.  Please
> > consider if it's doable or not.
> 
> The current driver works for the certain hardware without this change.
> If you think it's better, submit a proposal we will discuss.

Well, I'm not afford to do so. Just share an idea. I haven't got the
intel HW to test.  As I mentioned in v1 comment, it seems that you have
experience on relevent drivers and HW to test, may I encourage you to
do that :-) Or forget that if you really think you won't do that.

> 
> > If we ultimately keep the prescaler knowledge here, please consider to
> > add the NO_PRESCALER flag for i.MX7ulp as it hasn't the prescaler IIUC.
> 
> You mean there is a code which is currently using this driver w/o
> taking into account this prescaller flavour? Can you, please, point
> out, I'll definitely update it. Thanks for the catch!

drivers/clk/imx/clk-composite-7ulp.c

Regards,
Liu Ying

> 

Re: [PATCH v2 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Andy Shevchenko]

On Thu, Jul 22, 2021 at 05:43:49PM +0800, Liu Ying wrote:
> On Thu, 2021-07-22 at 12:38 +0300, Andy Shevchenko wrote:
> > On Thu, Jul 22, 2021 at 12:33 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > On Fri, 2021-07-16 at 16:34 +0300, Andy Shevchenko wrote:
> > > > The newly introduced flag, when set, makes the flow to skip
> > > > the assumption that the caller will use an additional 2^scale
> > > > prescaler to get the desired clock rate.
> > > 
> > > As I mentioned in v1 comment, it seems to be good to decouple the
> > > prescaler knowledge from this common fractional divider clk driver.
> > > This way, we'll make it simpler and easier to maintain. Also, then, the
> > > NO_PRESCALER flag is not needed at all.  However, it seems that two
> > > Intel drivers which use the frational divider drivers will be affected
> > > and rate negotiation logics need to be implemented for them.  Please
> > > consider if it's doable or not.
> > 
> > The current driver works for the certain hardware without this change.
> > If you think it's better, submit a proposal we will discuss.
> 
> Well, I'm not afford to do so. Just share an idea. I haven't got the
> intel HW to test.  As I mentioned in v1 comment, it seems that you have
> experience on relevent drivers and HW to test, may I encourage you to
> do that :-) Or forget that if you really think you won't do that.

Noted. I am in support of this idea, and will help with testing on Intel HW
if anyone submits the code.

> > > If we ultimately keep the prescaler knowledge here, please consider to
> > > add the NO_PRESCALER flag for i.MX7ulp as it hasn't the prescaler IIUC.
> > 
> > You mean there is a code which is currently using this driver w/o
> > taking into account this prescaller flavour? Can you, please, point
> > out, I'll definitely update it. Thanks for the catch!
> 
> drivers/clk/imx/clk-composite-7ulp.c

Thanks, I'll update in v3. Can you, please, look the other patch(es)?

-- 
With Best Regards,
Andy Shevchenko