| Message ID | 20231009084235.1944917-1-ping.bai@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource: imx-tpm: Wait for CnV write to take effect | expand |
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@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)
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@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) > > --
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@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) >> >> --
> 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@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
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?
> 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? BR > > > --
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.
> 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 > > > > > --
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)) + while (now == tpm_read_counter()) + ; + /* * 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)
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. 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. Signed-off-by: Jacky Bai <ping.bai@nxp.com> --- drivers/clocksource/timer-imx-tpm.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)