[PATCH v2 1/4] clk: k210: Fix checking if ulongs are less than 0

[PATCH v2 1/4] clk: k210: Fix checking if ulongs are less than 0

[Sean Anderson]

The PLL functions take ulong arguments for rate, but still check if that
rate is negative (which is never true). The correct way to handle this is
to use IS_ERR_VALUE (like is already done in k210_clk_set_rate). While
we're at it, we can move the error checking up into the caller of the pll
set/get rate functions.  This also protects our other calculations from
using bogus values for rate.

Fixes: 609bd60b94 ("clk: k210: Rewrite to remove CCF")
Reported-by: Coverity Scan <scan-admin@coverity.com>
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Reworked patch to use IS_ERR_VALUE instead of changing arguments to long

 drivers/clk/clk_kendryte.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c
index 3148756968..2caa21aec9 100644
--- a/drivers/clk/clk_kendryte.c
+++ b/drivers/clk/clk_kendryte.c
@@ -849,9 +849,6 @@ static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,
 	u32 reg;
 	ulong calc_rate;
 
-	if (rate_in < 0)
-		return rate_in;
-
 	err = k210_pll_calc_config(rate, rate_in, &config);
 	if (err)
 		return err;
@@ -895,7 +892,7 @@ static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id,
 	u64 r, f, od;
 	u32 reg = readl(priv->base + k210_plls[id].off);
 
-	if (rate_in < 0 || (reg & K210_PLL_BYPASS))
+	if (reg & K210_PLL_BYPASS)
 		return rate_in;
 
 	if (!(reg & K210_PLL_PWRD))
@@ -1029,6 +1026,8 @@ static ulong do_k210_clk_get_rate(struct k210_clk_priv *priv, int id)
 
 	parent = k210_clk_get_parent(priv, id);
 	parent_rate = do_k210_clk_get_rate(priv, parent);
+	if (IS_ERR_VALUE(parent_rate))
+		return parent_rate;
 
 	if (k210_clks[id].flags & K210_CLKF_PLL)
 		return k210_pll_get_rate(priv, k210_clks[id].pll, parent_rate);
@@ -1099,6 +1098,8 @@ static ulong k210_clk_set_rate(struct clk *clk, unsigned long rate)
 
 	parent = k210_clk_get_parent(priv, clk->id);
 	rate_in = do_k210_clk_get_rate(priv, parent);
+	if (IS_ERR_VALUE(rate_in))
+		return rate_in;
 
 	log_debug("id=%ld rate=%lu rate_in=%lu\n", clk->id, rate, rate_in);
 
-- 
2.33.0

[PATCH v2 2/4] k210: clk: Refactor out_of_spec tests

[Sean Anderson]

Everything here sits in a while (true) loop. However, this introduces a
couple of layers of indentation. We can simplify the code by introducing a
single goto instead of using continue/break. This will also make adding
loops in the next patch easier.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/clk/clk_kendryte.c | 105 ++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 53 deletions(-)

diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c
index 2caa21aec9..69691c4a04 100644
--- a/drivers/clk/clk_kendryte.c
+++ b/drivers/clk/clk_kendryte.c
@@ -709,6 +709,10 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
 		 * Whether we swapped r and od while enforcing frequency limits
 		 */
 		bool swapped = false;
+		/*
+		 * Whether the intermediate frequencies are out-of-spec
+		 */
+		bool out_of_spec;
 		u64 last_od = od;
 		u64 last_r = r;
 
@@ -767,76 +771,71 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
 		 * aren't in spec, try swapping r and od. If everything is
 		 * in-spec, calculate the relative error.
 		 */
-		while (true) {
+again:
+		out_of_spec = false;
+		if (r > max_r) {
+			out_of_spec = true;
+		} else {
 			/*
-			 * Whether the intermediate frequencies are out-of-spec
+			 * There is no way to only divide once; we need
+			 * to examine the frequency with and without the
+			 * effect of od.
 			 */
-			bool out_of_spec = false;
+			u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
 
-			if (r > max_r) {
+			if (vco > 1750000000 || vco < 340000000)
 				out_of_spec = true;
-			} else {
-				/*
-				 * There is no way to only divide once; we need
-				 * to examine the frequency with and without the
-				 * effect of od.
-				 */
-				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
+		}
 
-				if (vco > 1750000000 || vco < 340000000)
-					out_of_spec = true;
+		if (out_of_spec) {
+			u64 new_r, new_od;
+
+			if (!swapped) {
+				u64 tmp = r;
+
+				r = od;
+				od = tmp;
+				swapped = true;
+				goto again;
 			}
 
-			if (out_of_spec) {
-				if (!swapped) {
-					u64 tmp = r;
-
-					r = od;
-					od = tmp;
-					swapped = true;
-					continue;
-				} else {
-					/*
-					 * Try looking ahead to see if there are
-					 * additional factors for the same
-					 * product.
-					 */
-					if (i + 1 < ARRAY_SIZE(factors)) {
-						u64 new_r, new_od;
-
-						i++;
-						new_r = UNPACK_R(factors[i]);
-						new_od = UNPACK_OD(factors[i]);
-						if (r * od == new_r * new_od) {
-							r = new_r;
-							od = new_od;
-							swapped = false;
-							continue;
-						}
-						i--;
-					}
-					break;
+			/*
+			 * Try looking ahead to see if there are additional
+			 * factors for the same product.
+			 */
+			if (i + 1 < ARRAY_SIZE(factors)) {
+				i++;
+				new_r = UNPACK_R(factors[i]);
+				new_od = UNPACK_OD(factors[i]);
+				if (r * od == new_r * new_od) {
+					r = new_r;
+					od = new_od;
+					swapped = false;
+					goto again;
 				}
+				i--;
 			}
 
-			error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od);
-			/* The lower 16 bits are spurious */
-			error = abs((error - BIT(32))) >> 16;
+			/* We ran out of things to try */
+			continue;
+		}
 
-			if (error < best_error) {
-				best->r = r;
-				best->f = f;
-				best->od = od;
-				best_error = error;
-			}
-			break;
+		error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od);
+		/* The lower 16 bits are spurious */
+		error = abs((error - BIT(32))) >> 16;
+
+		if (error < best_error) {
+			best->r = r;
+			best->f = f;
+			best->od = od;
+			best_error = error;
 		}
 	} while (f < 64 && i + 1 < ARRAY_SIZE(factors) && error != 0);
 
+	log_debug("best error %lld\n", best_error);
 	if (best_error == S64_MAX)
 		return -EINVAL;
 
-	log_debug("best error %lld\n", best_error);
 	return 0;
 }
 
-- 
2.33.0

Re: [PATCH v2 2/4] k210: clk: Refactor out_of_spec tests

[Leo Liang]

On Sat, Sep 11, 2021 at 01:20:01PM -0400, Sean Anderson wrote:
> Everything here sits in a while (true) loop. However, this introduces a
> couple of layers of indentation. We can simplify the code by introducing a
> single goto instead of using continue/break. This will also make adding
> loops in the next patch easier.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> (no changes since v1)
>
>  drivers/clk/clk_kendryte.c | 105 ++++++++++++++++++-------------------
>  1 file changed, 52 insertions(+), 53 deletions(-)
>

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.

[PATCH v2 3/4] test: dm: k210: Reduce duplication in test cases

[Sean Anderson]

Having to copy-paste the same 3 lines makes adding new test cases
error-prone. Use a macro.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 test/dm/k210_pll.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/test/dm/k210_pll.c b/test/dm/k210_pll.c
index 54764f269c..5574ac96fa 100644
--- a/test/dm/k210_pll.c
+++ b/test/dm/k210_pll.c
@@ -69,27 +69,21 @@ static int dm_test_k210_pll(struct unit_test_state *uts)
 						  &theirs));
 	ut_asserteq(-EINVAL, k210_pll_calc_config(1500000000, 20000000,
 						  &theirs));
+	ut_asserteq(-EINVAL, k210_pll_calc_config(1750000000, 13300000,
+						  &theirs));
 
 	/* Verify we get the same output with brute-force */
-	ut_assertok(dm_test_k210_pll_calc_config(390000000, 26000000, &ours));
-	ut_assertok(k210_pll_calc_config(390000000, 26000000, &theirs));
-	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs));
+#define compare(rate, rate_in) do { \
+	ut_assertok(dm_test_k210_pll_calc_config(rate, rate_in, &ours)); \
+	ut_assertok(k210_pll_calc_config(rate, rate_in, &theirs)); \
+	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs)); \
+} while (0)
 
-	ut_assertok(dm_test_k210_pll_calc_config(26000000, 390000000, &ours));
-	ut_assertok(k210_pll_calc_config(26000000, 390000000, &theirs));
-	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs));
-
-	ut_assertok(dm_test_k210_pll_calc_config(400000000, 26000000, &ours));
-	ut_assertok(k210_pll_calc_config(400000000, 26000000, &theirs));
-	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs));
-
-	ut_assertok(dm_test_k210_pll_calc_config(27000000, 26000000, &ours));
-	ut_assertok(k210_pll_calc_config(27000000, 26000000, &theirs));
-	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs));
-
-	ut_assertok(dm_test_k210_pll_calc_config(26000000, 27000000, &ours));
-	ut_assertok(k210_pll_calc_config(26000000, 27000000, &theirs));
-	ut_assertok(dm_test_k210_pll_compare(&ours, &theirs));
+	compare(390000000, 26000000);
+	compare(26000000, 390000000);
+	compare(400000000, 26000000);
+	compare(27000000, 26000000);
+	compare(26000000, 27000000);
 
 	return 0;
 }
-- 
2.33.0

Re: [PATCH v2 3/4] test: dm: k210: Reduce duplication in test cases

[Leo Liang]

On Sat, Sep 11, 2021 at 01:20:02PM -0400, Sean Anderson wrote:
> Having to copy-paste the same 3 lines makes adding new test cases
> error-prone. Use a macro.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  test/dm/k210_pll.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
>
Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.

[PATCH v2 4/4] clk: k210: Try harder to get the best config

[Sean Anderson]

In some cases, the best config cannot be used because the VCO would be
out-of-spec. In these cases, we may need to try a worse combination of r/od
in order to find the best representable config. This also adds a few test
cases to catch this and other (possible) unlikely errors.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/clk/clk_kendryte.c | 24 ++++++++++++++++++++++++
 test/dm/k210_pll.c         |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c
index 69691c4a04..97efda5b6f 100644
--- a/drivers/clk/clk_kendryte.c
+++ b/drivers/clk/clk_kendryte.c
@@ -816,6 +816,30 @@ again:
 				i--;
 			}
 
+			/*
+			 * Try looking back to see if there is a worse ratio
+			 * that we could try anyway
+			 */
+			while (i > 0) {
+				i--;
+				new_r = UNPACK_R(factors[i]);
+				new_od = UNPACK_OD(factors[i]);
+				/*
+				 * Don't loop over factors for the same product
+				 * to avoid getting stuck because of the above
+				 * clause
+				 */
+				if (r * od != new_r * new_od) {
+					if (new_r * new_od > last_r * last_od) {
+						r = new_r;
+						od = new_od;
+						swapped = false;
+						goto again;
+					}
+					break;
+				}
+			}
+
 			/* We ran out of things to try */
 			continue;
 		}
diff --git a/test/dm/k210_pll.c b/test/dm/k210_pll.c
index 5574ac96fa..f55379f336 100644
--- a/test/dm/k210_pll.c
+++ b/test/dm/k210_pll.c
@@ -84,6 +84,10 @@ static int dm_test_k210_pll(struct unit_test_state *uts)
 	compare(400000000, 26000000);
 	compare(27000000, 26000000);
 	compare(26000000, 27000000);
+	compare(13300000 * 64, 13300000);
+	compare(21250000, 21250000 * 70);
+	compare(21250000, 1750000000);
+	compare(1750000000, 1750000000);
 
 	return 0;
 }
-- 
2.33.0

Re: [PATCH v2 4/4] clk: k210: Try harder to get the best config

[Simon Glass]

On Sat, 11 Sept 2021 at 11:20, Sean Anderson <seanga2@gmail.com> wrote:
>
> In some cases, the best config cannot be used because the VCO would be
> out-of-spec. In these cases, we may need to try a worse combination of r/od
> in order to find the best representable config. This also adds a few test
> cases to catch this and other (possible) unlikely errors.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> (no changes since v1)
>
>  drivers/clk/clk_kendryte.c | 24 ++++++++++++++++++++++++
>  test/dm/k210_pll.c         |  4 ++++
>  2 files changed, 28 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 1/4] clk: k210: Fix checking if ulongs are less than 0

[Leo Liang]

On Sat, Sep 11, 2021 at 01:20:00PM -0400, Sean Anderson wrote:
> The PLL functions take ulong arguments for rate, but still check if that
> rate is negative (which is never true). The correct way to handle this is
> to use IS_ERR_VALUE (like is already done in k210_clk_set_rate). While
> we're at it, we can move the error checking up into the caller of the pll
> set/get rate functions.  This also protects our other calculations from
> using bogus values for rate.
>
> Fixes: 609bd60b94 ("clk: k210: Rewrite to remove CCF")
> Reported-by: Coverity Scan <scan-admin@coverity.com>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v2:
> - Reworked patch to use IS_ERR_VALUE instead of changing arguments to long
>
>  drivers/clk/clk_kendryte.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.