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

[Daniel Lezcano]

On 11/10/2023 10:44, Jacky Bai wrote:
> 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.

But next is unsigned long and readl returns a long, no ?

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

The description says it is not possible to write multiple value to the 
TPM_C0V register within a CNT cycle.

The code writes to the register: writel(next, timer_base + TPM_C0V);

and then read the value back to check if it is different. That assumes, 
the cycle did not happen yet and we wait for this cycle. Am I correct?

Given the initial statement, IMO that could be simplified to:

  - read the TPM_C0V until the value is equal to the one set
or
  - read the tpm_read_counter() until one CNT happened
or
  - wait a duration corresponding to the cycle with udelay (which should 
be equivalent to the tpm_read_counter()

IOW, it is just about adding a tempo after writing the TPM_C0V register

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

No, both propositions are without the if statement

>> Or simply:
>>
>> 	usleep_range(min, max);
>>
> 
> This function is called by the clock event framework with irq disable,
> Sleep procedure may be not suitable?

Yes, you are right. udelay() could be used.

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

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