On 10/16/22 16:19, Sean Anderson wrote:
On 10/16/22 07:46, Heinrich Schuchardt wrote:
On 10/16/22 09:51, Michal Suchánek wrote:
On Thu, Oct 13, 2022 at 10:34:29PM +0200, Michal Suchanek wrote:
k210 is 64bit but the driver and tests are also built in sandbox, and
that can be built 32bit.
BIT(32) does not work on 32bit, shift before subtraction to fit into
32bit integer with BIT values.
Also see
https://patchwork.ozlabs.org/project/uboot/patch/20221016071035.461454-1-heinrich.schucha...@canonical.com/
Signed-off-by: Michal Suchanek <msucha...@suse.de>
---
drivers/clk/clk_k210.c | 2 +-
test/dm/k210_pll.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk_k210.c b/drivers/clk/clk_k210.c
index 1961efaa5e..e85f8ae14a 100644
--- a/drivers/clk/clk_k210.c
+++ b/drivers/clk/clk_k210.c
@@ -846,7 +846,7 @@ again:
error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od);
/* The lower 16 bits are spurious */
- error = abs((error - BIT(32))) >> 16;
+ error = abs((error >> 16) - BIT(32 - 16));
Your patch changes the results. Replacing BIT() by BIT_ULL() does not
change the result:
error = -1 = 0xffffffffffffffff
before
error - BIT(32) = -4294967297 = 0xfffffffeffffffff
abs(error - BIT(32)) = 4294967297 = 0x100000001
abs(error - BIT(32)) >> 16 = 65536 = 0x10000
after
error >> 16 = -1 = 0xffffffffffffffff
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffffffffffeffff
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x10000
error = 70000 = 0x11170
before
error - BIT(32) = -4294897296 = 0xffffffff00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0xffffffffffff0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0xffff
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65534 = 0xfffe
I assume this change of results is unintended.
We also replace abs() by abs64() to get correct results on a 32bit
system:
error = -1 = 0xffffffffffffffff
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x10000
error = 70000 = 0x11170
abs(error - BIT_ULL(32)) >> 16 = 1 = 0x1
abs64(error - BIT_ULL(32)) >> 16 = 65534 = 0xfffe
Best regards
Heinrich
if (error < best_error) {
best->r = r;
diff --git a/test/dm/k210_pll.c b/test/dm/k210_pll.c
index a0cc84c396..622b1c9bef 100644
--- a/test/dm/k210_pll.c
+++ b/test/dm/k210_pll.c
@@ -33,7 +33,7 @@ static int dm_test_k210_pll_calc_config(u32 rate,
u32 rate_in,
error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio,
r * od);
/* The lower 16 bits are spurious */
- error = abs((error - BIT(32))) >> 16;
+ error = abs((error >> 16) - BIT(32 - 16));
if (error < best_error) {
best->r = r;
best->f = f;
--
2.37.3
IMO the driver should just be changed to depend on 64-bit. The k210 is
64-bit,
and I didn't write anything with 32-bit in mind.
Michal, Simon, and I are striving to get the sandbox working on ilp32
systems.
Do you suggest to remove the driver from sandbox_defconfig?
This would imply that the unit test is not executed on Gitlab CI. You
will still be able to execute it on the actual hardware.
Best regards
Heinrich
