Problem with system time in lpc176x bsp
Sebastian Huber
sebastian.huber at embedded-brains.de
Mon Jan 11 19:18:21 UTC 2016
Hello,
I already created a ticket which blocks the 4.11 release for this bug:
https://devel.rtems.org/ticket/2502
I sent a new version of the patch to one of your previous threads some days ago:
https://lists.rtems.org/pipermail/devel/2016-January/013289.html
There is still a bug in it, since we must disable interrupts in the is pending function, but the general approach should work.
----- Am 11. Jan 2016 um 20:05 schrieb Marcos Díaz marcos.diaz at tallertechnologies.com:
> I made a fast search:
> These BSPs use *rtems_timecounter_simple_downcounter*:
> arm/shared/armv7m
> m68k/mcf52235
> m68k/mcf5225x
> m68k/mcf5329
> m68k/uC5282
> powerpc/mpc55xxevb
> sparc/erc32
> sparc/leon2
> sparc/leon3
> sparc/shared
>
> These use *rtems_timecounter_simple_upcounter*:
> powerpc/mpc55xxevb
> arm/lpc22xx
>
> We should search a little more for the other changes but at least these
> BSPs will be affected. Maybe we should wait Sebastian's opinion, since he
> suggested the change in timecounter's signatures.
>
> On Mon, Jan 11, 2016 at 3:44 PM, Joel Sherrill <joel at rtems.org> wrote:
>
>>
>>
>> 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
>>>
>>>
>>
>
>
> --
>
> ______________________________
>
> <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
More information about the devel
mailing list