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

[Heinrich Schuchardt]

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.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
>>>>
> 
> 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