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

[Jacky Bai]

Hi Daniel,

> Subject: Re: [PATCH] clocksource: imx-tpm: Wait for CnV write to take effect
> 
> On 09/10/2023 10:42, Jacky Bai wrote:
> > The value write into the TPM CnV can only be updated into the HW when
> > CNT increase. Additional writes to the CnV write buffer are ignored
> > until the register has been updated. So we need to check if the CnV
> > has been updated before continue. Wait for 1 CNT cycle in worst case.
> 
> Is that a fix for an erratic observed behavior on the platform ?

It is an IP requirement that we overlooked before. This driver originally used by NXP
i.MX7ULP platform, now we use the same driver on NXP i.MX8ULP(arm64). due to the SoC
arch and bus fabric(bus speed, core speed etc) difference, we meet the issue that
if two consecutive 'tpm_set_next_event' call happens in one TPM CNT cycle, the
secondary write will be ignored by the HW. So we need to wait at least one CNT cycle
to make sure the first write has been updated successfully.

> 
> > Additionally, current return check is not correct, if a max_delta need
> > be set, it will return '-ETIME' wrongly due to the 'int' type cast, so
> > refine the check logic to fix it.
> 
> Please put that in a separate patch and double check the fix (now -
> prev) which seems to be always nearly one CNT
> 

Ok, will split it int a separate patch. It is expected, we need to make sure
That now - prev should be less than the delta we want to set, otherwise
It means the TPM will lost the compare event as the TPM can only generate
Irq when CNT=C0V, if CNT > C0V, no irq. An ideal design should be IRQ assert when
CNT >= C0V, but it is not the case for this IP.

> 
> Fixes tag missing

Ok, will be fixed.

> 
> > Signed-off-by: Jacky Bai <ping.bai at nxp.com>
> > ---
> >   drivers/clocksource/timer-imx-tpm.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-imx-tpm.c
> b/drivers/clocksource/timer-imx-tpm.c
> > index bd64a8a8427f..92c025b70eb6 100644
> > --- a/drivers/clocksource/timer-imx-tpm.c
> > +++ b/drivers/clocksource/timer-imx-tpm.c
> > @@ -83,20 +83,28 @@ static u64 notrace tpm_read_sched_clock(void)
> >   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.

> > +		while (now == tpm_read_counter())
> > +			;
> 
> IIUC, we can rely on one of these condition:
> 
> Check against timer_base + TPM_C0V or tpm_read_counter()
> 
> We can use the following routine:
> 	while (readl(timer_base + TPM_C0V) != next);
> 
> because as soon as 1 CNT cycle happened, TPM_C0V is updated.

C0V does not update based the cnt cycle. it is static.

> 
> Other version could be:
> 
> 	while (now == tpm_read_counter());
> 
Same as the originally changes, but with ';' and 'while' in the same line?

> Or simply:
> 
> 	usleep_range(min, max);
> 

This function is called by the clock event framework with irq disable,
Sleep procedure may be not suitable?

BR
> Right?
> 
> >   	/*
> >   	 * NOTE: We observed in a very small probability, the bus fabric
> >   	 * contention between GPU and A7 may results a few cycles delay
> >   	 * of writing CNT registers which may cause the min_delta event
> got
> >   	 * missed, so we need add a ETIME check here in case it happened.
> >   	 */
> > -	return (int)(next - now) <= 0 ? -ETIME : 0;
> > +	return (now - prev) >= delta ? -ETIME : 0;
> >   }
> >
> >   static int tpm_set_state_oneshot(struct clock_event_device *evt)
> 
> --