From 95ff0a0888174341bf1de85c3da14d6754f2f429 Mon Sep 17 00:00:00 2001 From: Chris Kay Date: Mon, 16 Jul 2018 11:13:17 +0100 Subject: [PATCH 1/4] fwk/list: Add missing const-qualifications to generics Change-Id: I890942badbaa4676651e142c09cf03752bda77e4 --- framework/include/fwk_list.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/framework/include/fwk_list.h b/framework/include/fwk_list.h index 6e6d916c3..0ccef8047 100644 --- a/framework/include/fwk_list.h +++ b/framework/include/fwk_list.h @@ -61,9 +61,11 @@ */ #define fwk_list_head(list) \ ((void *)_Generic((list), \ + const struct fwk_slist * : __fwk_slist_head, \ + const struct fwk_dlist * : __fwk_slist_head, \ struct fwk_slist * : __fwk_slist_head, \ struct fwk_dlist * : __fwk_slist_head \ - )((struct fwk_slist *)list)) + )((const struct fwk_slist *)list)) /*! * \brief Test whether a linked list is empty or not. @@ -79,7 +81,7 @@ const struct fwk_dlist * : __fwk_slist_is_empty, \ struct fwk_slist * : __fwk_slist_is_empty, \ struct fwk_dlist * : __fwk_slist_is_empty \ - )((struct fwk_slist *)list) + )((const struct fwk_slist *)list) /*! * \brief Add a new node to the head of a linked list. @@ -141,9 +143,11 @@ */ #define fwk_list_next(list, node) \ ((void *) _Generic((list), \ + const struct fwk_slist * : __fwk_slist_next, \ + const struct fwk_dlist * : __fwk_slist_next, \ struct fwk_slist * : __fwk_slist_next, \ struct fwk_dlist * : __fwk_slist_next \ - )((struct fwk_slist *)list, (struct fwk_slist_node *)node)) + )((const struct fwk_slist *)list, (const struct fwk_slist_node *)node)) /*! * \brief Remove a node from a linked list. @@ -191,9 +195,11 @@ */ #define fwk_list_contains(list, node) \ _Generic((list), \ + const struct fwk_slist * : __fwk_slist_contains, \ + const struct fwk_dlist * : __fwk_slist_contains, \ struct fwk_slist * : __fwk_slist_contains, \ struct fwk_dlist * : __fwk_slist_contains \ - )((struct fwk_slist *)list, (struct fwk_slist_node *)node) + )((const struct fwk_slist *)list, (const struct fwk_slist_node *)node) /*! * @} -- GitLab From f64ffabad8547c260e4f3c30cb7e8874e5df3a0d Mon Sep 17 00:00:00 2001 From: Elieva Pignat Date: Mon, 25 Jun 2018 15:00:19 +0100 Subject: [PATCH 2/4] timer: fix comments Change-Id: I8e6f54ac5afff0479d1be8252b1d2f25df2eaecf Signed-off-by: Elieva Pignat --- module/timer/include/mod_timer.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/module/timer/include/mod_timer.h b/module/timer/include/mod_timer.h index 7bf3567a5..3ba4a525a 100644 --- a/module/timer/include/mod_timer.h +++ b/module/timer/include/mod_timer.h @@ -171,7 +171,7 @@ struct mod_timer_api { * \param microseconds The amount of time, given in microseconds, to delay. * * \retval FWK_SUCCESS Operation succeeded. - * \retval One of the other specific error codes described by the module. + * \retval One of the other specific error codes described by the framework. */ int (*delay)(fwk_id_t dev_id, uint32_t microseconds); @@ -194,7 +194,7 @@ struct mod_timer_api { * elapsed. * \retval FWK_E_TIMEOUT The timeout period elapsed before the condition was * met. - * \retval One of the other specific error codes described by the module. + * \retval One of the other specific error codes described by the framework. */ int (*wait)(fwk_id_t dev_id, uint32_t microseconds, @@ -216,7 +216,7 @@ struct mod_timer_api { * * \retval FWK_SUCCESS Operation succeeded. * \retval FWK_E_PARAM The remaining_ticks pointer was invalid. - * \retval One of the other specific error codes described by the module. + * \retval One of the other specific error codes described by the framework. * * \note remaining_ticks is also a timestamp. */ -- GitLab From 4eab094595adf1064709ce0e7834a36442589988 Mon Sep 17 00:00:00 2001 From: Elieva Pignat Date: Thu, 14 Jun 2018 17:45:50 +0100 Subject: [PATCH 3/4] timer: add function to get the remaining ticks of next alarm Change-Id: I339715bf64ba38b6c1b0473419e55eed4e330e68 Signed-off-by: Elieva Pignat --- module/timer/include/mod_timer.h | 21 ++++++++++ module/timer/src/mod_timer.c | 70 ++++++++++++++++++++++++++++---- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/module/timer/include/mod_timer.h b/module/timer/include/mod_timer.h index 3ba4a525a..1caca8021 100644 --- a/module/timer/include/mod_timer.h +++ b/module/timer/include/mod_timer.h @@ -223,6 +223,27 @@ struct mod_timer_api { int (*remaining)(fwk_id_t dev_id, uint64_t timestamp, uint64_t *remaining_ticks); + + /*! + * \brief Get the number of ticks before the next alarm trigger of a given + * timer. + * + * \warning If the timer has no active alarm, \p remaining_ticks is not + * initialized. + * + * \param dev_id Element identifier that identifies the timer device. + * \param [out] has_alarm \c true if the timer has an active alarm, + * otherwise \c false. + * \param [out] remaining_ticks Number of ticks between now and the next + * alarm trigger of the timer identified by \p dev_id. + * + * \retval FWK_SUCCESS Operation succeeded. + * \retval FWK_E_PARAM One of the parameters is invalid. + * \return One of the other specific error codes described by the framework. + */ + int (*get_next_alarm_remaining)(fwk_id_t dev_id, + bool *has_alarm, + uint64_t *remaining_ticks); }; /*! diff --git a/module/timer/src/mod_timer.c b/module/timer/src/mod_timer.c index 2b61cd260..a80e4901e 100644 --- a/module/timer/src/mod_timer.c +++ b/module/timer/src/mod_timer.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -119,6 +120,29 @@ static int _timestamp_from_now(struct dev_ctx *ctx, return FWK_SUCCESS; } +static int _remaining(const struct dev_ctx *ctx, + uint64_t timestamp, + uint64_t *remaining_ticks) +{ + int status; + uint64_t counter; + + fwk_assert(ctx != NULL); + fwk_assert(remaining_ticks != NULL); + + status = ctx->driver->get_counter(ctx->driver_dev_id, &counter); + if (!fwk_expect(status == FWK_SUCCESS)) + return status; + + /* If timestamp is in the past, remaining_ticks is set to zero. */ + if (timestamp < counter) + *remaining_ticks = 0; + else + *remaining_ticks = timestamp - counter; + + return FWK_SUCCESS; +} + static void _configure_timer_with_next_alarm(struct dev_ctx *ctx) { struct alarm_ctx *alarm_head; @@ -309,7 +333,6 @@ static int remaining(fwk_id_t dev_id, { struct dev_ctx *ctx; int status; - uint64_t counter; status = fwk_module_check_call(dev_id); if (status != FWK_SUCCESS) @@ -320,16 +343,48 @@ static int remaining(fwk_id_t dev_id, if (remaining_ticks == NULL) return FWK_E_PARAM; - status = ctx->driver->get_counter(ctx->driver_dev_id, &counter); + return _remaining(ctx, timestamp, remaining_ticks); +} + +static int get_next_alarm_remaining(fwk_id_t dev_id, + bool *has_alarm, + uint64_t *remaining_ticks) +{ + int status; + const struct dev_ctx *ctx; + const struct alarm_ctx *alarm_ctx; + const struct fwk_dlist_node *alarm_ctx_node; + + status = fwk_module_check_call(dev_id); if (status != FWK_SUCCESS) return status; - if (timestamp <= counter) - *remaining_ticks = 0; - else - *remaining_ticks = timestamp - counter; + if (has_alarm == NULL) + return FWK_E_PARAM; - return FWK_SUCCESS; + if (remaining_ticks == NULL) + return FWK_E_PARAM; + + ctx = &ctx_table[fwk_id_get_element_idx(dev_id)]; + + /* + * The timer interrupt is disabled to ensure that the alarm list is not + * modified while we are trying to read it below. + */ + ctx->driver->disable(ctx->driver_dev_id); + + *has_alarm = !fwk_list_is_empty(&ctx->alarms_active); + + if (*has_alarm) { + alarm_ctx_node = fwk_list_head(&ctx->alarms_active); + alarm_ctx = FWK_LIST_GET(alarm_ctx_node, struct alarm_ctx, node); + + status = _remaining(ctx, alarm_ctx->timestamp, remaining_ticks); + } + + ctx->driver->enable(ctx->driver_dev_id); + + return status; } static const struct mod_timer_api timer_api = { @@ -339,6 +394,7 @@ static const struct mod_timer_api timer_api = { .delay = delay, .wait = wait, .remaining = remaining, + .get_next_alarm_remaining = get_next_alarm_remaining, }; /* -- GitLab From b8abfa4e83b43e672a788f752f814253e1be2077 Mon Sep 17 00:00:00 2001 From: Elieva Pignat Date: Thu, 14 Jun 2018 17:47:16 +0100 Subject: [PATCH 4/4] timer: add a minimum timestamp for the alarms When running on FVP, alarms set within a very short time period can be missed. This patch defines a minimum timestamp workaround. Change-Id: Icb7ca816d94c5b3467633e9931d7205d569b1f2a Signed-off-by: Elieva Pignat --- module/gtimer/src/mod_gtimer.c | 65 ++++++++++++++++++++++------------ module/timer/src/mod_timer.c | 43 +--------------------- 2 files changed, 44 insertions(+), 64 deletions(-) diff --git a/module/gtimer/src/mod_gtimer.c b/module/gtimer/src/mod_gtimer.c index 74845169f..ed772b395 100644 --- a/module/gtimer/src/mod_gtimer.c +++ b/module/gtimer/src/mod_gtimer.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,7 @@ #define GTIMER_FREQUENCY_MIN_HZ UINT32_C(1) #define GTIMER_FREQUENCY_MAX_HZ UINT32_C(1000000000) +#define GTIMER_MIN_TIMESTAMP 2000 /* Device content */ struct dev_ctx { @@ -70,10 +72,12 @@ static int disable(fwk_id_t dev_id) return FWK_SUCCESS; } -static int set_timer(fwk_id_t dev_id, uint64_t timestamp) +static int get_counter(fwk_id_t dev_id, uint64_t *value) { - struct dev_ctx *ctx; + const struct dev_ctx *ctx; int status; + uint32_t counter_low; + uint32_t counter_high; status = fwk_module_check_call(dev_id); if (status != FWK_SUCCESS) @@ -81,37 +85,56 @@ static int set_timer(fwk_id_t dev_id, uint64_t timestamp) ctx = ctx_table + fwk_id_get_element_idx(dev_id); - ctx->hw_timer->P_CVALL = timestamp & 0xFFFFFFFF; - ctx->hw_timer->P_CVALH = timestamp >> 32; + /* + * To avoid race conditions where the high half of the counter increments + * after it has been sampled but before the low half is sampled, the values + * are resampled until the high half has stabilized. This assumes that the + * loop is faster than the high half incrementation. + */ + do { + counter_high = ctx->hw_timer->PCTH; + counter_low = ctx->hw_timer->PCTL; + } while (counter_high != ctx->hw_timer->PCTH); + + *value = ((uint64_t)counter_high << 32) | counter_low; return FWK_SUCCESS; } -static int get_timer(fwk_id_t dev_id, uint64_t *timestamp) +static int set_timer(fwk_id_t dev_id, uint64_t timestamp) { struct dev_ctx *ctx; + uint64_t counter; int status; status = fwk_module_check_call(dev_id); if (status != FWK_SUCCESS) return status; - ctx = ctx_table + fwk_id_get_element_idx(dev_id); + status = get_counter(dev_id, &counter); + if (status != FWK_SUCCESS) + return status; - struct { - uint32_t low; - uint32_t high; - } value; + /* + * If an alarm's period is very small, the timer device could be configured + * to interrupt on a timestamp that is "in the past". In this case, with the + * current FVP implementation an interrupt will not be generated. To avoid + * this issue, the minimum timestamp is GTIMER_MIN_TIMESTAMP ticks from now. + * + * It is assumed here that the 64-bit counter will never loop back during + * the course of the execution (@1GHz it would loop back after ~585 years). + */ + timestamp = FWK_MAX(counter + GTIMER_MIN_TIMESTAMP, timestamp); - /* Read 64-bit timer value */ - value.low = ctx->hw_timer->P_CVALL; - value.high = ctx->hw_timer->P_CVALH; + ctx = ctx_table + fwk_id_get_element_idx(dev_id); + + ctx->hw_timer->P_CVALL = timestamp & 0xFFFFFFFF; + ctx->hw_timer->P_CVALH = timestamp >> 32; - *timestamp = *(uint64_t *)&value; return FWK_SUCCESS; } -static int get_counter(fwk_id_t dev_id, uint64_t *value) +static int get_timer(fwk_id_t dev_id, uint64_t *timestamp) { struct dev_ctx *ctx; int status; @@ -125,15 +148,13 @@ static int get_counter(fwk_id_t dev_id, uint64_t *value) struct { uint32_t low; uint32_t high; - } counter; + } value; - /* Read 64-bit counter value */ - do { - counter.high = ctx->hw_timer->PCTH; - counter.low = ctx->hw_timer->PCTL; - } while (counter.high != ctx->hw_timer->PCTH); + /* Read 64-bit timer value */ + value.low = ctx->hw_timer->P_CVALL; + value.high = ctx->hw_timer->P_CVALH; - *value = *(uint64_t *)&counter; + *timestamp = *(uint64_t *)&value; return FWK_SUCCESS; } diff --git a/module/timer/src/mod_timer.c b/module/timer/src/mod_timer.c index a80e4901e..9da41fbca 100644 --- a/module/timer/src/mod_timer.c +++ b/module/timer/src/mod_timer.c @@ -146,28 +146,11 @@ static int _remaining(const struct dev_ctx *ctx, static void _configure_timer_with_next_alarm(struct dev_ctx *ctx) { struct alarm_ctx *alarm_head; - uint64_t counter = 0; - int status; assert(ctx != NULL); alarm_head = (struct alarm_ctx *)fwk_list_head(&ctx->alarms_active); if (alarm_head != NULL) { - /* - * If an alarm's period is very small, the timer device could be - * configured to interrupt on a timestamp that is "in the past" by the - * time interrupts are enabled. In this case, the interrupt will not be - * generated due to a model bug. This code can be deleted once this bug - * has been fixed. - * - * If this alarm occurs very soon, process it immediately to avoid - * potentially missing the interrupt and waiting forever. - */ - status = ctx->driver->get_counter(ctx->driver_dev_id, &counter); - if ((status == FWK_SUCCESS) && - (counter + 2000 >= alarm_head->timestamp)) - timer_isr((uintptr_t)ctx); - /* Configure timer device */ ctx->driver->set_timer(ctx->driver_dev_id, alarm_head->timestamp); ctx->driver->enable(ctx->driver_dev_id); @@ -486,10 +469,8 @@ static void timer_isr(uintptr_t ctx_ptr) { int status; struct alarm_ctx *alarm; - struct alarm_ctx *alarm_head; struct dev_ctx *ctx = (struct dev_ctx *)ctx_ptr; uint64_t timestamp = 0; - uint64_t counter = 0; struct fwk_event event; assert(ctx != NULL); @@ -498,7 +479,6 @@ static void timer_isr(uintptr_t ctx_ptr) ctx->driver->disable(ctx->driver_dev_id); fwk_interrupt_clear_pending(ctx->config->timer_irq); -process_alarm: alarm = (struct alarm_ctx *)fwk_list_pop_head(&ctx->alarms_active); if (alarm == NULL) { @@ -535,28 +515,7 @@ process_alarm: "back into queue.\n"); } - alarm_head = FWK_LIST_GET(fwk_list_head(&ctx->alarms_active), - struct alarm_ctx, - node); - if (alarm_head != NULL) { - /* - * If successive alarm item timestamps are very close together, the - * timer device could be configured to interrupt on a timestamp that is - * "in the past". In this case, the interrupt will not be generated due - * to a model bug. This code can be deleted once this bug has been - * fixed. - * - * If the next alarm occurs very soon, process it immidiately to avoid - * potentially missing the interrupt and waiting forever. - */ - status = ctx->driver->get_counter(ctx->driver_dev_id, &counter); - if ((status == FWK_SUCCESS) && - (counter + 2000 >= alarm_head->timestamp)) - goto process_alarm; - - ctx->driver->set_timer(ctx->driver_dev_id, alarm_head->timestamp); - ctx->driver->enable(ctx->driver_dev_id); - } + _configure_timer_with_next_alarm(ctx); } /* -- GitLab