* [PATCH 2/4] k210: clk: Refactor out_of_spec tests
2021-07-27 3:51 [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Sean Anderson
@ 2021-07-27 3:51 ` Sean Anderson
2021-07-27 3:51 ` [PATCH 3/4] test: dm: k210: Reduce duplication in test cases Sean Anderson
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Sean Anderson @ 2021-07-27 3:51 UTC (permalink / raw)
To: u-boot, Leo Liang; +Cc: Lukasz Majewski, Damien Le Moal, 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>
---
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 37bd624eca..d2cee2cf97 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.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] test: dm: k210: Reduce duplication in test cases
2021-07-27 3:51 [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Sean Anderson
2021-07-27 3:51 ` [PATCH 2/4] k210: clk: Refactor out_of_spec tests Sean Anderson
@ 2021-07-27 3:51 ` Sean Anderson
2021-07-31 16:59 ` Simon Glass
2021-07-27 3:51 ` [PATCH 4/4] clk: k210: Try harder to get the best config Sean Anderson
2021-07-27 8:15 ` [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Damien Le Moal
3 siblings, 1 reply; 7+ messages in thread
From: Sean Anderson @ 2021-07-27 3:51 UTC (permalink / raw)
To: u-boot, Leo Liang
Cc: Lukasz Majewski, Damien Le Moal, Sean Anderson, Simon Glass
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>
---
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.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] clk: k210: Try harder to get the best config
2021-07-27 3:51 [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Sean Anderson
2021-07-27 3:51 ` [PATCH 2/4] k210: clk: Refactor out_of_spec tests Sean Anderson
2021-07-27 3:51 ` [PATCH 3/4] test: dm: k210: Reduce duplication in test cases Sean Anderson
@ 2021-07-27 3:51 ` Sean Anderson
2021-07-27 8:15 ` [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Damien Le Moal
3 siblings, 0 replies; 7+ messages in thread
From: Sean Anderson @ 2021-07-27 3:51 UTC (permalink / raw)
To: u-boot, Leo Liang
Cc: Lukasz Majewski, Damien Le Moal, Sean Anderson, Simon Glass
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>
---
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 d2cee2cf97..e90550ef33 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.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0
2021-07-27 3:51 [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Sean Anderson
` (2 preceding siblings ...)
2021-07-27 3:51 ` [PATCH 4/4] clk: k210: Try harder to get the best config Sean Anderson
@ 2021-07-27 8:15 ` Damien Le Moal
2021-07-27 14:01 ` Sean Anderson
3 siblings, 1 reply; 7+ messages in thread
From: Damien Le Moal @ 2021-07-27 8:15 UTC (permalink / raw)
To: Sean Anderson, u-boot, Leo Liang; +Cc: Lukasz Majewski, Coverity Scan
On 2021/07/27 12:51, Sean Anderson wrote:
> Some clock functions return ulong but still have "negative" errors. To deal
> with this, cast the relevant arguments to long.
>
> Fixes: 609bd60b94 ("clk: k210: Rewrite to remove CCF")
> Reported-by: Coverity Scan <scan-admin@coverity.com>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> drivers/clk/clk_kendryte.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c
> index 3148756968..37bd624eca 100644
> --- a/drivers/clk/clk_kendryte.c
> +++ b/drivers/clk/clk_kendryte.c
> @@ -439,7 +439,7 @@ static const struct k210_clk_params k210_clks[] = {
> #ifdef CONFIG_CLK_K210_SET_RATE
> static int k210_pll_enable(struct k210_clk_priv *priv, int id);
> static int k210_pll_disable(struct k210_clk_priv *priv, int id);
> -static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, ulong rate_in);
> +static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, long rate_in);
>
> /*
> * The PLL included with the Kendryte K210 appears to be a True Circuits, Inc.
> @@ -841,7 +841,7 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
> }
>
> static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,
Shouldn't this one return a long, in case of error ? It seems that the commit
messages hints at such a change, but you are changing the argument type instead.
A little confusing. What am I missing ?
> - ulong rate_in)
> + long rate_in)
> {
> int err;
> const struct k210_pll_params *pll = &k210_plls[id];
> @@ -890,7 +890,7 @@ static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,
> #endif /* CONFIG_CLK_K210_SET_RATE */
>
> static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id,
Same here ?
> - ulong rate_in)
> + long rate_in)
I would assume that these functions are called if the rate_in argument is
correct, so I do not really understand why the argument type needs to be changed...
> {
> u64 r, f, od;
> u32 reg = readl(priv->base + k210_plls[id].off);
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0
2021-07-27 8:15 ` [PATCH 1/4] clk: k210: Fix checking if ulongs are less than 0 Damien Le Moal
@ 2021-07-27 14:01 ` Sean Anderson
0 siblings, 0 replies; 7+ messages in thread
From: Sean Anderson @ 2021-07-27 14:01 UTC (permalink / raw)
To: Damien Le Moal, u-boot, Leo Liang; +Cc: Lukasz Majewski, Coverity Scan
On 7/27/21 4:15 AM, Damien Le Moal wrote:
> On 2021/07/27 12:51, Sean Anderson wrote:
>> Some clock functions return ulong but still have "negative" errors. To deal
>> with this, cast the relevant arguments to long.
>>
>> Fixes: 609bd60b94 ("clk: k210: Rewrite to remove CCF")
>> Reported-by: Coverity Scan <scan-admin@coverity.com>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> drivers/clk/clk_kendryte.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk_kendryte.c b/drivers/clk/clk_kendryte.c
>> index 3148756968..37bd624eca 100644
>> --- a/drivers/clk/clk_kendryte.c
>> +++ b/drivers/clk/clk_kendryte.c
>> @@ -439,7 +439,7 @@ static const struct k210_clk_params k210_clks[] = {
>> #ifdef CONFIG_CLK_K210_SET_RATE
>> static int k210_pll_enable(struct k210_clk_priv *priv, int id);
>> static int k210_pll_disable(struct k210_clk_priv *priv, int id);
>> -static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, ulong rate_in);
>> +static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id, long rate_in);
>>
>> /*
>> * The PLL included with the Kendryte K210 appears to be a True Circuits, Inc.
>> @@ -841,7 +841,7 @@ TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>> }
>>
>> static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,
>
> Shouldn't this one return a long, in case of error ? It seems that the commit
> messages hints at such a change, but you are changing the argument type instead.
> A little confusing. What am I missing ?
Perhaps they should return long, but these are basically matching the
prototypes in include/clk-uclass.h. And there, get_rate and set_rate
take ulong arguments and return ulongs.
>
>> - ulong rate_in)
>> + long rate_in)
>> {
>> int err;
>> const struct k210_pll_params *pll = &k210_plls[id];
>> @@ -890,7 +890,7 @@ static ulong k210_pll_set_rate(struct k210_clk_priv *priv, int id, ulong rate,
>> #endif /* CONFIG_CLK_K210_SET_RATE */
>>
>> static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id,
>
> Same here ?
>
>> - ulong rate_in)
>> + long rate_in)
>
> I would assume that these functions are called if the rate_in argument is
> correct, so I do not really understand why the argument type needs to be changed...
Hm, I suppose the better patch would be to check the return of
get_rate and set_rate when we call them. I think my intent here was to
allow subsequent functions to be no-ops in case of error, but it looks
like I act on these values directly. Will fix in v2.
--Sean
>
>> {
>> u64 r, f, od;
>> u32 reg = readl(priv->base + k210_plls[id].off);
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread