Problem with system time in lpc176x bsp

Joel Sherrill joel at rtems.org
Mon Jan 11 18:44:36 UTC 2016


On Mon, Jan 11, 2016 at 12:28 PM, Marcos Díaz <
marcos.diaz at tallertechnologies.com> wrote:

> I will issue a ticket. But I noticed that in my patch I include changes to
> common code that Sebastian suggested, and this will break any other BSP
> that uses rtems timecounter simple downcounter or upcounter, since these
> function's signatures changed. Should we update all BSPs? Or make changes
> more locally to our BSP?  Meanwhile please do not apply the patch I sent.
>
>
I would prefer not to ship BSPs with bugs but fixing them comes with risks.
So I would lean to the same fix on the master and 4.11 branch fixing it
where it needs to be fixed.

What BSPs does this change impact?

--joel


>
> On Mon, Jan 11, 2016 at 3:16 PM, Joel Sherrill <joel at rtems.org> wrote:
>
>> 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
>>>
>>
>>
>
>
> --
>
> ______________________________
>
> <http://www.tallertechnologies.com>
>
>
> Marcos Díaz
>
> Software Engineer
>
>
> San Lorenzo 47, 3rd Floor, Office 5
>
> Córdoba, Argentina
>
>
> Phone: +54 351 4217888 / +54 351 4218211/ +54 351 7617452
>
> Skype: markdiaz22
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160111/428fc755/attachment-0002.html>


More information about the devel mailing list