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

Wed Oct 11 03:13:01 PDT 2023

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

'next' upper 32bit may not be '0's.

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

Ooh, sorry, my fault. I was confused by myself. You are right.^_^

> 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

Prefer to use this one without the if statement. ^_^

BR
>   - 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.l/
> inaro.org%2F&data=05%7C01%7Cping.bai%40nxp.com%7C1b02ea0d079e49
> 9f9a9308dbca3d9293%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638326138021983821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=02ldeCYZ3pwlcnOHc4xgELypgZEzNnCfGEm4jD%2BDEkQ%3D
> &reserved=0> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:
> <http://www.f/
> acebook.com%2Fpages%2FLinaro&data=05%7C01%7Cping.bai%40nxp.com%
> 7C1b02ea0d079e499f9a9308dbca3d9293%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C638326138021983821%7CUnknown%7CTWFpbGZsb3
> d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7C3000%7C%7C%7C&sdata=5cz2Em4IV5bmONAR5wQHcvA40%2BQp
> mg0%2FEoSpVbun%2FpE%3D&reserved=0> Facebook |
> <http://twitter/
> .com%2F%23!%2Flinaroorg&data=05%7C01%7Cping.bai%40nxp.com%7C1b0
> 2ea0d079e499f9a9308dbca3d9293%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C638326138021983821%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&sdata=RJLU2AYZnWs93ofblnVZasCP7Nppj7VApu0dASri
> ySI%3D&reserved=0> Twitter |
> <http://www.l/
> inaro.org%2Flinaro-blog%2F&data=05%7C01%7Cping.bai%40nxp.com%7C1b
> 02ea0d079e499f9a9308dbca3d9293%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C638326138021983821%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&sdata=4LzQ9ONPIJ4g%2BekyIr%2FKWMa%2BQPBqf
> 9ncsilrNF4hEIQ%3D&reserved=0> Blog