Problem with system time in lpc176x bsp
Marcos Díaz
marcos.diaz at tallertechnologies.com
Mon Jan 11 19:05:22 UTC 2016
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160111/64bebf8e/attachment-0002.html>
More information about the devel
mailing list