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

[Heinrich Schuchardt]

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.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.
>>
>> 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.schuchardt@canonical.com/

https://patchwork.ozlabs.org/project/uboot/patch/20221013203429.15200-1-msuchanek@suse.de/

Best regards

Heinrich