[PATCH] clk: k210: Fix error calculation on 32bit

[Heinrich Schuchardt]

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.schuchardt@canonical.com/
>>
>> Signed-off-by: Michal Suchanek <msuchanek at 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
>>