[PATCH] clocksource: imx-tpm: Wait for CnV write to take effect

[Daniel Lezcano]

On 12/10/2023 08:44, Jacky Bai wrote:
>> Subject: Re: [PATCH] clocksource: imx-tpm: Wait for CnV write to take effect
>>
>> On 11/10/2023 12:13, Jacky Bai wrote:
>>>> Subject: Re: [PATCH] clocksource: imx-tpm: Wait for CnV write to take
>>>> effect
>>
>> [ ... ]
>>
>>>>>>>      static int tpm_set_next_event(unsigned long delta,
>>>>>>>                                     struct clock_event_device
>> *evt)
>>>>>>>      {
>>>>>>> - unsigned long next, now;
>>>>>>> + unsigned long next, prev, now;
>>>>>>>
>>>>>>> - next = tpm_read_counter();
>>>>>>> - next += delta;
>>>>>>> + prev = tpm_read_counter();
>>>>>>> + next = prev + delta;
>>>>>>>             writel(next, timer_base + TPM_C0V);
>>>>>>>             now = tpm_read_counter();
>>>>>>>
>>>>>>> + /*
>>>>>>> +  * Need to wait CNT increase at least 1 cycle to make sure
>>>>>>> +  * the C0V has been updated into HW.
>>>>>>> +  */
>>>>>>> + if ((next & 0xffffffff) != readl(timer_base + TPM_C0V))
>>>>>>
>>>>>> Why do you use a mask for 'next' ?
>>>>>>
>>>>>
>>>>> 'unsigned long' is 64 bit on arm64 platform, while the register is
>>>>> 32bit, so the upper 32bit cleared before comparing.
>>>>
>>>> But next is unsigned long and readl returns a long, no ?
>>>
>>> 'next' upper 32bit may not be '0's.
>>
>> Is it possible to have 'next' greater than INT_MAX?
> 
> may has the possibility if prev + next > 0xffffffff?

So there is a couple of things.

1.

The init function called clockevents_config_and_register() with the 
register width and the max_delta.

The underlying framework is not supposed to call "'set_next_event' where 
the next event will be greater than the register width.

   now + delta is < UINT_MAX (or max_delta if I'm not wrong)

It detects timer wrapping and then counts the number of time has to wrap 
before applying a recomputed delta.

2.

unsigned long a = ULONG_MAX;
unsigned int b = UINT_MAX;

a = b;

printf("%lu\n", a);

Gives : 4294967295 , UINT_MAX

Said simply, (next & 0xffffffff) is not needed AFAICT.




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog