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

[Jacky Bai]

> Subject: Re: [PATCH] clocksource: imx-tpm: Wait for CnV write to take effect
> 
> 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.

Great thx. I will refine the changes as you suggested in V2.

BR
> 
> 
> 
> 
> --