Problem with system time in lpc176x bsp

Joel Sherrill joel at rtems.org
Mon Jan 11 18:16:34 UTC 2016


At this point, a ticket is needed for anything applied to 4.11 that is not
release
mechanics related.

--joel

On Mon, Jan 11, 2016 at 11:47 AM, Marcos Diaz <
marcos.diaz at tallertechnologies.com> wrote:

> Hi Sebastian,
>
> we did some more testing and found out what's causing the problem. Based
> on what
> I posted at
> https://lists.rtems.org/pipermail/devel/2015-December/013235.html,
> the problem arises when the ticker interrupt occurs while a task is
> executing
> one of the instructions that make up the following line:
>
> is_count_pending = (systick->csr & ARMV7M_SYSTICK_CSR_COUNTFLAG) != 0;
>
> which compiling with -O3 are:
>
> ldr.w r3, [r9]
> ubfx r5, r3, #16, #1
> strb.w r5, [r8, #64]
>
> If the SysTick counts to 0 and the ldr executes before the interrupt
> occurs,
> R3 will have the CSR.COUNTFLAG bit set. Even though _ARMV7M_TC_at_tick will
> clear the is_count_pending boolean, the task will still see it as true.
>
> The solution we found is to simply disable interrupts while reading that
> flag.
> We also use a local variable inside _ARMV7M_TC_is_pending instead of
> directly
> returning the current value of the is_count_pending global, just in case.
>
> Here's the patch that fixes it; this applies to the 4.11 branch.
> Do we have to open a new thread for it to be applied to the mainline, or
> will
> this suffice? Should we open a ticket for 4.11?
>
> ---
>  .../arm/shared/armv7m/clock/armv7m-clock-config.c  | 38
> +++++++++++++++-------
>  c/src/lib/libbsp/shared/clockdrv_shell.h           | 16 ++++++++-
>  cpukit/rtems/src/clocktick.c                       |  6 +++-
>  cpukit/sapi/include/rtems/timecounter.h            | 35
> ++++++++++++++++----
>  cpukit/score/include/rtems/score/timecounter.h     | 37
> +++++++++++++++++++--
>  cpukit/score/src/kern_tc.c                         | 16 ++++-----
>  doc/bsp_howto/clock.t                              | 13 +++++++-
>  7 files changed, 130 insertions(+), 31 deletions(-)
>
> diff --git
> a/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c
> b/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c
> index e78684c..53bd7cf 100644
> --- a/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c
> +++ b/c/src/lib/libbsp/arm/shared/armv7m/clock/armv7m-clock-config.c
> @@ -25,6 +25,8 @@ static void Clock_isr(void *arg);
>
>  static rtems_timecounter_simple _ARMV7M_TC;
>
> +static bool is_count_pending = false;
> +
>  static uint32_t _ARMV7M_TC_get(rtems_timecounter_simple *tc)
>  {
>    volatile ARMV7M_Systick *systick = _ARMV7M_Systick;
> @@ -34,9 +36,20 @@ static uint32_t _ARMV7M_TC_get(rtems_timecounter_simple
> *tc)
>
>  static bool _ARMV7M_TC_is_pending(rtems_timecounter_simple *tc)
>  {
> -  volatile ARMV7M_SCB *scb = _ARMV7M_SCB;
> +  volatile ARMV7M_Systick *systick = _ARMV7M_Systick;
> +  ISR_lock_Context lock_context;
> +
> +  _Timecounter_Acquire( &lock_context );
> +  /* We use this bool since the hardware flag is reset each time it is
> read. */
> +  if (!is_count_pending)
> +  {
> +    is_count_pending = ((systick->csr & ARMV7M_SYSTICK_CSR_COUNTFLAG) !=
> 0 );
> +  }
>
> -  return ((scb->icsr & ARMV7M_SCB_ICSR_PENDSTSET) != 0);
> +  const bool is_pending_local = is_count_pending;
> +
> +  _Timecounter_Release( &lock_context );
> +  return is_pending_local;
>  }
>
>  static uint32_t _ARMV7M_TC_get_timecount(struct timecounter *tc)
> @@ -48,19 +61,23 @@ static uint32_t _ARMV7M_TC_get_timecount(struct
> timecounter *tc)
>    );
>  }
>
> -static void _ARMV7M_TC_tick(void)
> -{
> -  rtems_timecounter_simple_downcounter_tick(&_ARMV7M_TC, _ARMV7M_TC_get);
> -}
> -
> -static void _ARMV7M_Systick_at_tick(void)
> +static void _ARMV7M_TC_at_tick(rtems_timecounter_simple *tc)
>  {
>    volatile ARMV7M_Systick *systick = _ARMV7M_Systick;
> -
> +  is_count_pending = false;
>    /* Clear COUNTFLAG */
>    systick->csr;
>  }
>
> +static void _ARMV7M_TC_tick(void)
> +{
> +  rtems_timecounter_simple_downcounter_tick(
> +    &_ARMV7M_TC,
> +    _ARMV7M_TC_get,
> +    _ARMV7M_TC_at_tick
> +  );
> +}
> +
>  static void _ARMV7M_Systick_handler(void)
>  {
>    _ARMV7M_Interrupt_service_enter();
> @@ -111,9 +128,6 @@ static void _ARMV7M_Systick_cleanup(void)
>
>  #define Clock_driver_timecounter_tick() _ARMV7M_TC_tick()
>
> -#define Clock_driver_support_at_tick() \
> -  _ARMV7M_Systick_at_tick()
> -
>  #define Clock_driver_support_initialize_hardware() \
>    _ARMV7M_Systick_initialize()
>
> diff --git a/c/src/lib/libbsp/shared/clockdrv_shell.h
> b/c/src/lib/libbsp/shared/clockdrv_shell.h
> index d546fb8..96b962f 100644
> --- a/c/src/lib/libbsp/shared/clockdrv_shell.h
> +++ b/c/src/lib/libbsp/shared/clockdrv_shell.h
> @@ -44,6 +44,13 @@
>    #define Clock_driver_support_find_timer()
>  #endif
>
> +/**
> + * @brief Do nothing by default.
> + */
> +#ifndef Clock_driver_support_at_tick
> +  #define Clock_driver_support_at_tick()
> +#endif
> +
>  /*
>   * A specialized clock driver may use for example
> rtems_timecounter_tick_simple()
>   * instead of the default.
> @@ -108,7 +115,14 @@ rtems_isr Clock_isr(
>            && _Thread_Executing->Start.entry_point
>              == (Thread_Entry) rtems_configuration_get_idle_task()
>        ) {
> -        _Timecounter_Tick_simple(interval, (*tc->tc_get_timecount)(tc));
> +        ISR_lock_Context lock_context;
> +
> +        _Timecounter_Acquire(&lock_context);
> +        _Timecounter_Tick_simple(
> +          interval,
> +          (*tc->tc_get_timecount)(tc),
> +          &lock_context
> +        );
>        }
>
>        Clock_driver_support_at_tick();
> diff --git a/cpukit/rtems/src/clocktick.c b/cpukit/rtems/src/clocktick.c
> index e2cd35f..a6bf26d 100644
> --- a/cpukit/rtems/src/clocktick.c
> +++ b/cpukit/rtems/src/clocktick.c
> @@ -23,9 +23,13 @@
>
>  rtems_status_code rtems_clock_tick( void )
>  {
> +  ISR_lock_Context lock_context;
> +
> +  _Timecounter_Acquire( &lock_context );
>    _Timecounter_Tick_simple(
>      rtems_configuration_get_microseconds_per_tick(),
> -    0
> +    0,
> +    &lock_context
>    );
>
>    return RTEMS_SUCCESSFUL;
> diff --git a/cpukit/sapi/include/rtems/timecounter.h
> b/cpukit/sapi/include/rtems/timecounter.h
> index 04bc534..06b3973 100644
> --- a/cpukit/sapi/include/rtems/timecounter.h
> +++ b/cpukit/sapi/include/rtems/timecounter.h
> @@ -94,6 +94,13 @@ typedef struct {
>  } rtems_timecounter_simple;
>
>  /**
> + * @brief At tick handling done under protection of the timecounter lock.
> + */
> +typedef uint32_t rtems_timecounter_simple_at_tick(
> +  rtems_timecounter_simple *tc
> +);
> +
> +/**
>   * @brief Returns the current value of a simple timecounter.
>   */
>  typedef uint32_t rtems_timecounter_simple_get(
> @@ -199,20 +206,28 @@ RTEMS_INLINE_ROUTINE uint32_t
> rtems_timecounter_simple_scale(
>   *
>   * @param[in] tc The simple timecounter.
>   * @param[in] get The method to get the value of the simple timecounter.
> + * @param[in] at_tick The method to perform work under timecounter lock
> + * protection at this tick, e.g. clear a pending flag.
>   */
>  RTEMS_INLINE_ROUTINE void rtems_timecounter_simple_downcounter_tick(
> -  rtems_timecounter_simple     *tc,
> -  rtems_timecounter_simple_get  get
> +  rtems_timecounter_simple         *tc,
> +  rtems_timecounter_simple_get      get,
> +  rtems_timecounter_simple_at_tick  at_tick
>  )
>  {
> +  ISR_lock_Context lock_context;
>    uint32_t current;
>
> +  _Timecounter_Acquire( &lock_context );
> +
> +  ( *at_tick )( tc );
> +
>    current = rtems_timecounter_simple_scale(
>      tc,
>      tc->real_interval - ( *get )( tc )
>    );
>
> -  _Timecounter_Tick_simple( tc->binary_interval, current );
> +  _Timecounter_Tick_simple( tc->binary_interval, current, &lock_context );
>  }
>
>  /**
> @@ -220,17 +235,25 @@ RTEMS_INLINE_ROUTINE void
> rtems_timecounter_simple_downcounter_tick(
>   *
>   * @param[in] tc The simple timecounter.
>   * @param[in] get The method to get the value of the simple timecounter.
> + * @param[in] at_tick The method to perform work under timecounter lock
> + * protection at this tick, e.g. clear a pending flag.
>   */
>  RTEMS_INLINE_ROUTINE void rtems_timecounter_simple_upcounter_tick(
> -  rtems_timecounter_simple     *tc,
> -  rtems_timecounter_simple_get  get
> +  rtems_timecounter_simple         *tc,
> +  rtems_timecounter_simple_get      get,
> +  rtems_timecounter_simple_at_tick  at_tick
>  )
>  {
> +  ISR_lock_Context lock_context;
>    uint32_t current;
>
> +  _Timecounter_Acquire( &lock_context );
> +
> +  ( *at_tick )( tc );
> +
>    current = rtems_timecounter_simple_scale( tc, ( *get )( tc ) );
>
> -  _Timecounter_Tick_simple( tc->binary_interval, current );
> +  _Timecounter_Tick_simple( tc->binary_interval, current, &lock_context );
>  }
>
>  /**
> diff --git a/cpukit/score/include/rtems/score/timecounter.h
> b/cpukit/score/include/rtems/score/timecounter.h
> index 0d17cc7..0e611b5 100644
> --- a/cpukit/score/include/rtems/score/timecounter.h
> +++ b/cpukit/score/include/rtems/score/timecounter.h
> @@ -26,6 +26,8 @@
>  #include <sys/time.h>
>  #include <sys/timetc.h>
>
> +#include <rtems/score/isrlock.h>
> +
>  #ifdef __cplusplus
>  extern "C" {
>  #endif /* __cplusplus */
> @@ -161,6 +163,31 @@ void _Timecounter_Install( struct timecounter *tc );
>  void _Timecounter_Tick( void );
>
>  /**
> + * @brief Lock to protect the timecounter mechanic.
> + */
> +ISR_LOCK_DECLARE( extern, _Timecounter_Lock )
> +
> +/**
> + * @brief Acquires the timecounter lock.
> + *
> + * @param[in] lock_context The lock context.
> + *
> + * See _Timecounter_Tick_simple().
> + */
> +#define _Timecounter_Acquire( lock_context ) \
> +  _ISR_lock_ISR_disable_and_acquire( &_Timecounter_Lock, lock_context )
> +
> +/**
> + * @brief Releases the timecounter lock.
> + *
> + * @param[in] lock_context The lock context.
> + *
> + * See _Timecounter_Tick_simple().
> + */
> +#define _Timecounter_Release(lock_context) \
> +  _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, lock_context)
> +
> +/**
>   * @brief Performs a simple timecounter tick.
>   *
>   * This is a special purpose tick function for simple timecounter to
> support
> @@ -169,8 +196,14 @@ void _Timecounter_Tick( void );
>   * @param[in] delta The time in timecounter ticks elapsed since the last
> call
>   * to _Timecounter_Tick_simple().
>   * @param[in] offset The current value of the timecounter.
> - */
> -void _Timecounter_Tick_simple( uint32_t delta, uint32_t offset );
> + * @param[in] lock_context The lock context of the corresponding
> + * _Timecounter_Acquire().
> + */
> +void _Timecounter_Tick_simple(
> +  uint32_t delta,
> +  uint32_t offset,
> +  ISR_lock_Context *lock_context
> +);
>
>  /**
>   * @brief The wall clock time in seconds.
> diff --git a/cpukit/score/src/kern_tc.c b/cpukit/score/src/kern_tc.c
> index f6a1136..4257533 100644
> --- a/cpukit/score/src/kern_tc.c
> +++ b/cpukit/score/src/kern_tc.c
> @@ -64,7 +64,9 @@ __FBSDID("$FreeBSD r284178 2015-06-09T11:49:56Z$");
>  #ifdef __rtems__
>  #include <limits.h>
>  #include <rtems.h>
> -ISR_LOCK_DEFINE(static, _Timecounter_Lock, "Timecounter");
> +ISR_LOCK_DEFINE(, _Timecounter_Lock, "Timecounter")
> +#define _Timecounter_Release(lock_context) \
> +  _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock, lock_context)
>  #define hz rtems_clock_get_ticks_per_second()
>  #define printf(...)
>  #define log(...)
> @@ -1383,7 +1385,7 @@ tc_windup(void)
>  #ifdef __rtems__
>         ISR_lock_Context lock_context;
>
> -       _ISR_lock_ISR_disable_and_acquire(&_Timecounter_Lock,
> &lock_context);
> +       _Timecounter_Acquire(&lock_context);
>  #endif /* __rtems__ */
>
>         /*
> @@ -1538,7 +1540,7 @@ tc_windup(void)
>         timekeep_push_vdso();
>  #endif /* __rtems__ */
>  #ifdef __rtems__
> -       _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock,
> &lock_context);
> +       _Timecounter_Release(&lock_context);
>  #endif /* __rtems__ */
>  }
>
> @@ -1963,14 +1965,12 @@ _Timecounter_Tick(void)
>  }
>  #ifdef __rtems__
>  void
> -_Timecounter_Tick_simple(uint32_t delta, uint32_t offset)
> +_Timecounter_Tick_simple(uint32_t delta, uint32_t offset,
> +    ISR_lock_Context *lock_context)
>  {
>         struct bintime bt;
>         struct timehands *th;
>         uint32_t ogen;
> -       ISR_lock_Context lock_context;
> -
> -       _ISR_lock_ISR_disable_and_acquire(&_Timecounter_Lock,
> &lock_context);
>
>         th = timehands;
>         ogen = th->th_generation;
> @@ -1997,7 +1997,7 @@ _Timecounter_Tick_simple(uint32_t delta, uint32_t
> offset)
>         time_second = th->th_microtime.tv_sec;
>         time_uptime = th->th_offset.sec;
>
> -       _ISR_lock_Release_and_ISR_enable(&_Timecounter_Lock,
> &lock_context);
> +       _Timecounter_Release(lock_context);
>
>         _Watchdog_Tick();
>  }
> diff --git a/doc/bsp_howto/clock.t b/doc/bsp_howto/clock.t
> index f58b898..2891bb2 100644
> --- a/doc/bsp_howto/clock.t
> +++ b/doc/bsp_howto/clock.t
> @@ -89,6 +89,13 @@ static uint32_t some_tc_get( rtems_timecounter_simple
> *tc )
>    return some.counter;
>  @}
>
> +static void some_tc_at_tick( rtems_timecounter_simple *tc )
> +@{
> +  /*
> +   * Do work necessary at the clock tick interrupt, e.g. clear a pending
> flag.
> +   */
> +@}
> +
>  static bool some_tc_is_pending( rtems_timecounter_simple *tc )
>  @{
>    return some.is_pending;
> @@ -105,7 +112,11 @@ static uint32_t some_tc_get_timecount( struct
> timecounter *tc )
>
>  static void some_tc_tick( void )
>  @{
> -  rtems_timecounter_simple_downcounter_tick( &some_tc, some_tc_get );
> +  rtems_timecounter_simple_downcounter_tick(
> +    &some_tc,
> +    some_tc_get,
> +    some_tc_at_tick
> +  );
>  @}
>
>  static void some_support_initialize_hardware( void )
> --
> 2.7.0
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160111/b175e7e8/attachment-0002.html>


More information about the devel mailing list