Re: [PATCH] clk: k210: Fix error calculation on 32bit
Hello, On Sun, Oct 16, 2022 at 05:02:38PM +0200, Heinrich Schuchardt wrote: > > > On 10/16/22 16:57, Sean Anderson wrote: > > On 10/16/22 10:50, Heinrich Schuchardt wrote: > > > > > > > > > On 10/16/22 16:48, Sean Anderson wrote: > > > > On 10/16/22 10:40, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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. > > > > > > > > It's still enabled for sandbox64, so it should still be executed there. > > > > > > No, the sandbox64 is also to be compiled on ilp32. It models > > > physical address extension (PAE). > > > > Do we have a config for word size? PHYS_64BIT seems to be for the > > address size. > > phys_addr_t is what is used to address physical memory which may be 64bit on > a 32bit PAE system (CONFIG_PHYS_64BIT=y). > > Compiling on armv7 yields 32bit pointers. > > There are two alternative patches in review removing HOST_32BIT and > HOST_64BIT: > > https://patchwork.ozlabs.org/project/uboot/patch/20221014064052.5592-1-heinrich.schucha...@canonical.com/ > > https://patchwork.ozlabs.org/project/uboot/patch/20221013203429.15200-1-msucha...@suse.de/ I don't think that we have a config option that can be used to detect that the build is 64bit. Such option may exit per platform but drivers that can be built for multiple platforms will have hard time detecting it. I think that Heinrich's suggestion to use BIT_ULL instead of BIT and abs64 instead of abs is non-controversial. It's technically more correct way to write the expression although relevant only for 32bit. Thanks Michal
Re: [PATCH] clk: k210: Fix error calculation on 32bit
On 10/16/22 16:57, Sean Anderson wrote:
On 10/16/22 10:50, Heinrich Schuchardt wrote:
On 10/16/22 16:48, Sean Anderson wrote:
On 10/16/22 10:40, Heinrich Schuchardt wrote:
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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.
It's still enabled for sandbox64, so it should still be executed there.
No, the sandbox64 is also to be compiled on ilp32. It models physical
address extension (PAE).
Do we have a config for word size? PHYS_64BIT seems to be for the
address size.
phys_addr_t is what is used to address physical memory which may be
64bit on a 32bit PAE system (CONFIG_PHYS_64BIT=y).
Compiling on armv7 yields 32bit pointers.
There are two alternative patches in review removing HOST_32BIT and
HOST_64BIT:
https://patchwork.ozlabs.org/project/uboot/patch/20221014064052.5592-1-heinrich.schucha...@canonical.com/
https://patchwork.ozlabs.org/project/uboot/patch/20221013203429.15200-1-msucha...@suse.de/
Best regards
Heinrich
Re: [PATCH] clk: k210: Fix error calculation on 32bit
On 10/16/22 10:50, Heinrich Schuchardt wrote:
On 10/16/22 16:48, Sean Anderson wrote:
On 10/16/22 10:40, Heinrich Schuchardt wrote:
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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.
It's still enabled for sandbox64, so it should still be executed there.
No, the sandbox64 is also to be compiled on ilp32. It models physical address
extension (PAE).
Do we have a config for word size? PHYS_64BIT seems to be for the address size.
--Sean
Re: [PATCH] clk: k210: Fix error calculation on 32bit
On 10/16/22 16:48, Sean Anderson wrote:
On 10/16/22 10:40, Heinrich Schuchardt wrote:
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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.
It's still enabled for sandbox64, so it should still be executed there.
No, the sandbox64 is also to be compiled on ilp32. It models physical
address extension (PAE).
Best regards
Heinrich
Re: [PATCH] clk: k210: Fix error calculation on 32bit
On 10/16/22 10:40, Heinrich Schuchardt wrote:
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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.
It's still enabled for sandbox64, so it should still be executed there.
--Sean
Re: [PATCH] clk: k210: Fix error calculation on 32bit
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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
Re: [PATCH] clk: k210: Fix error calculation on 32bit
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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.
--Sean
Re: [PATCH] clk: k210: Fix error calculation on 32bit
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
---
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 = 0x
before
error - BIT(32) = -4294967297 = 0xfffe
abs(error - BIT(32)) = 4294967297 = 0x10001
abs(error - BIT(32)) >> 16 = 65536 = 0x1
after
error >> 16 = -1 = 0x
(error >> 16) - BIT(32 - 16) = -65537 = 0xfffe
abs((error >> 16) - BIT(32 - 16)) = 65537 = 0x10001
using BIT_ULL
abs(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 0x11170
before
error - BIT(32) = -4294897296 = 0x00011170
abs(error - BIT(32)) = 4294897296 = 0xfffeee90
abs(error - BIT(32)) >> 16 = 65534 = 0xfffe
after
error >> 16 = 1 = 0x1
(error >> 16) - BIT(32 - 16) = -65535 = 0x0001
abs((error >> 16) - BIT(32 - 16)) = 65535 = 0x
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 = 0x
abs(error - BIT_ULL(32)) >> 16 = 0 = 0x0
abs64(error - BIT_ULL(32)) >> 16 = 65536 = 0x1
error = 7 = 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
Re: [PATCH] clk: k210: Fix error calculation on 32bit
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
> ---
>
> 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));
>
> 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
>
[PATCH] clk: k210: Fix error calculation on 32bit
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.
Signed-off-by: Michal Suchanek
---
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));
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
