Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680994 > unrolled thread

Re: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 16:10 +0200
    RE: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support "A.s. Dong" <aisheng.dong@nxp.com> - 2017-07-04 16:30 +0200
      RE: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 16:50 +0200
        RE: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support "A.s. Dong" <aisheng.dong@nxp.com> - 2017-07-04 17:20 +0200
          RE: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 17:40 +0200

#1680994 — Re: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support

On Tue, 4 Jul 2017, Dong Aisheng wrote:

> IMX Timer/PWM Module (TPM) supports both timer and pwm function while
> this patch only adds the timer support. PWM would be added later.
> 
> The TPM counter, compare and capture registers are clocked by an
> asynchronous clock that can remain enabled in low power modes.
> 
> Due to the possible bus fabric contention, the CNT write may
> take a few more cycles and we need add ETIME check in case current
> delta event program gets missed.
> 
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Anson Huang <Anson.Huang@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> ---
> ChangeLog:
> v2->v3:
>  * address all comments from Daniel Lezcano
>  * add more explaination on ETIME check in commit message

Actually the logic wants to be explained in a comment inside the function
as well.

I'm really impressed, that 10 years after we discovered the HPET disaster
(See comment in arch/x86/kernel/hpet.c::hpet_next_event) the same hardware
idiocy comes around again....

Thanks,

	tglx

[toc] | [next] | [standalone]

#1681021

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Tuesday, July 04, 2017 10:10 PM
> To: A.s. Dong
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> daniel.lezcano@linaro.org; shawnguo@kernel.org; Jacky Bai; Anson Huang;
> dongas86@gmail.com; kernel@pengutronix.de; Arnd Bergmann; Anson Huang
> Subject: Re: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support
> 
> On Tue, 4 Jul 2017, Dong Aisheng wrote:
> 
> > IMX Timer/PWM Module (TPM) supports both timer and pwm function while
> > this patch only adds the timer support. PWM would be added later.
> >
> > The TPM counter, compare and capture registers are clocked by an
> > asynchronous clock that can remain enabled in low power modes.
> >
> > Due to the possible bus fabric contention, the CNT write may take a
> > few more cycles and we need add ETIME check in case current delta
> > event program gets missed.
> >
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Anson Huang <Anson.Huang@nxp.com>
> > Cc: Bai Ping <ping.bai@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > ---
> > ChangeLog:
> > v2->v3:
> >  * address all comments from Daniel Lezcano
> >  * add more explaination on ETIME check in commit message
> 
> Actually the logic wants to be explained in a comment inside the function
> as well.
> 

Good suggestion, will add them inside function as well.

> I'm really impressed, that 10 years after we discovered the HPET disaster
> (See comment in arch/x86/kernel/hpet.c::hpet_next_event) the same
> hardware idiocy comes around again....
> 

Not quite sure but seems a bit different issue.
The issue is still uncertain but the test shows it's related to fabric priority
Configuration, if increase the A7 core priority higher than GPU, the issue
is very hard to be seen. But we don't want to change the default priority,
we use ETIME check to fix it.

Probably I would be better add a FIXME prefix before the comments in code
as well because it's still uncertain.

Regards
Dong Aisheng

> Thanks,
> 
> 	tglx
> 

[toc] | [prev] | [next] | [standalone]

#1681027

On Tue, 4 Jul 2017, A.s. Dong wrote:
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > I'm really impressed, that 10 years after we discovered the HPET disaster
> > (See comment in arch/x86/kernel/hpet.c::hpet_next_event) the same
> > hardware idiocy comes around again....
> > 
> 
> Not quite sure but seems a bit different issue.
> The issue is still uncertain but the test shows it's related to fabric priority
> Configuration, if increase the A7 core priority higher than GPU, the issue
> is very hard to be seen. But we don't want to change the default priority,
> we use ETIME check to fix it.

Well, whether it's hard to be observed or not is not the question. The
point is, that with match equal registers you always have:

      now = read_counter();
      match = now + delta;
      write_match(match);

If the counter advanced past match before the write hits the match
register, then the next interrupt will come after the wrap around of the
counter, which might be close to eternity depending on the counter
frequency and bit width.

This advancement can be caused by a gazillion of reasons:

     - Fabric delays
     - TLB/cache misses
     - .....

The probability might be low, but this can and will happen. And there is
nothing you can do about it. No FIXME in the world will change that
behaviour except that the FIXME actually changes the hardware.

Match equal registers are simply crap in such a context and should never be
used for timers. That's not a new finding, that's well known since 40+
years. But sure, hardware folks are always smarter.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1681037

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Tuesday, July 04, 2017 10:43 PM
> To: A.s. Dong
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> daniel.lezcano@linaro.org; shawnguo@kernel.org; Jacky Bai; Anson Huang;
> dongas86@gmail.com; kernel@pengutronix.de; Arnd Bergmann; Anson Huang
> Subject: RE: [PATCH V3 2/2] timer: imx-tpm: add imx tpm timer support
> 
> On Tue, 4 Jul 2017, A.s. Dong wrote:
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de] I'm really
> > > impressed, that 10 years after we discovered the HPET disaster (See
> > > comment in arch/x86/kernel/hpet.c::hpet_next_event) the same
> > > hardware idiocy comes around again....
> > >
> >
> > Not quite sure but seems a bit different issue.
> > The issue is still uncertain but the test shows it's related to fabric
> > priority Configuration, if increase the A7 core priority higher than
> > GPU, the issue is very hard to be seen. But we don't want to change
> > the default priority, we use ETIME check to fix it.
> 
> Well, whether it's hard to be observed or not is not the question. The
> point is, that with match equal registers you always have:
> 
>       now = read_counter();
>       match = now + delta;
>       write_match(match);
> 
> If the counter advanced past match before the write hits the match
> register, then the next interrupt will come after the wrap around of the
> counter, which might be close to eternity depending on the counter
> frequency and bit width.
> 

Yes we did observe that.
TPM CNT is 32 bit width and working at 3Mhz, it takes about 23 seconds
to wrap around to trigger the next event.
And due to it's single core, RCU stall can't trap it. The kernel
Seems have no idea about the wrap around and just resume and keep run.

> This advancement can be caused by a gazillion of reasons:
> 
>      - Fabric delays
>      - TLB/cache misses
>      - .....
> 
> The probability might be low, but this can and will happen. And there is
> nothing you can do about it. No FIXME in the world will change that
> behaviour except that the FIXME actually changes the hardware.
> 

That's Right.

> Match equal registers are simply crap in such a context and should never
> be used for timers. That's not a new finding, that's well known since 40+
> years. But sure, hardware folks are always smarter.
> 

Thanks for the detailed explanation.

I planned to add the following explanation in function for this issue.
diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
index 4716746..c13a8de 100644
--- a/drivers/clocksource/timer-imx-tpm.c
+++ b/drivers/clocksource/timer-imx-tpm.c
@@ -99,6 +99,12 @@ static int tpm_set_next_event(unsigned long delta,
        writel(next, timer_base + TPM_C0V);
        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;
 }

Do you think it's ok?

Regards
Dong Aisheng

> Thanks,
> 
> 	tglx
> 

[toc] | [prev] | [next] | [standalone]

#1681061

On Tue, 4 Jul 2017, A.s. Dong wrote:
> +       /*
> +        * 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;
>  }
> 
> Do you think it's ok?

Looks about right.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web