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

[Daniel Lezcano]

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 ?

> 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


Fixes tag missing

> 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' ?

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

Other version could be:

	while (now == tpm_read_counter());	

Or simply:

	usleep_range(min, max);

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)

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