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

[Sean Anderson]

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

It's still enabled for sandbox64, so it should still be executed there.

--Sean