SAMA5: Fix bugs in timer/counter interrupts and one-shot timer

9 files changed, 118 insertions, 53 deletions

diff --git a/nuttx/arch/arm/src/sama5/sam_freerun.c b/nuttx/arch/arm/src/sama5/sam_freerun.c
index f7fd95650..1c742ec49 100644
--- a/nuttx/arch/arm/src/sama5/sam_freerun.c
+++ b/nuttx/arch/arm/src/sama5/sam_freerun.c
@@ -69,6 +69,8 @@
 #ifdef CONFIG_SAMA5_TC_DEBUG
 #  define tcdbg    dbg
 #  define tcvdbg   vdbg
+#  define tcdbg    lldbg
+#  define tcvdbg   llvdbg
 #  define tclldbg  lldbg
 #  define tcllvdbg llvdbg
 #else
diff --git a/nuttx/arch/arm/src/sama5/sam_oneshot.c b/nuttx/arch/arm/src/sama5/sam_oneshot.c
index 552cee10d..a1ff9474a 100644
--- a/nuttx/arch/arm/src/sama5/sam_oneshot.c
+++ b/nuttx/arch/arm/src/sama5/sam_oneshot.c
@@ -102,32 +102,44 @@
  *   level up.
  *
  * Input Parameters:
- *   handle - The handle that represents the timer state
- *   arg    - An opaque argument provided when the interrupt was registered
- *   sr     - The value of the timer interrupt status register at the time
- *            that the interrupt occurred.
+ *   tch - The handle that represents the timer state
+ *   arg - An opaque argument provided when the interrupt was registered
+ *   sr  - The value of the timer interrupt status register at the time
+ *         that the interrupt occurred.
  *
  * Returned Value:
  *   None
  *
  ****************************************************************************/
 
-static void sam_oneshot_handler(TC_HANDLE handle, void *arg, uint32_t sr)
+static void sam_oneshot_handler(TC_HANDLE tch, void *arg, uint32_t sr)
 {
   struct sam_oneshot_s *oneshot = (struct sam_oneshot_s *)arg;
+  oneshot_handler_t oneshot_handler;
+  void *oneshot_arg;
+
+  tcllvdbg("Expired...\n");
   DEBUGASSERT(oneshot && oneshot->handler);
 
   /* The clock was stopped, but not disabled when the RC match occurred.
    * Disable the TC now and disable any further interrupts.
    */
 
-  sam_tc_attach(oneshot->handler, NULL, NULL, 0);
-  sam_tc_stop(oneshot->handle);
+  sam_tc_attach(oneshot->tch, NULL, NULL, 0);
+  sam_tc_stop(oneshot->tch);
 
-  /* Forward the event */
+  /* The timer is no longer running */
 
   oneshot->running = false;
-  oneshot->handler(oneshot->arg);
+
+  /* Forward the event, clearing out any vestiges */
+
+  oneshot_handler  = oneshot->handler;
+  oneshot->handler = NULL;
+  oneshot_arg      = oneshot->arg;
+  oneshot->arg     = NULL;
+
+  oneshot_handler(oneshot_arg);
 }
 
 /****************************************************************************
@@ -210,8 +222,8 @@ int sam_oneshot_initialize(struct sam_oneshot_s *oneshot, int chan,
           TC_CMR_ASWTRG_NONE | TC_CMR_BCPB_NONE   |    TC_CMR_BCPC_NONE  |
           TC_CMR_BEEVT_NONE  | TC_CMR_BSWTRG_NONE);
 
-  oneshot->handle = sam_tc_allocate(chan, cmr);
-  if (!oneshot->handle)
+  oneshot->tch = sam_tc_allocate(chan, cmr);
+  if (!oneshot->tch)
     {
       tcdbg("ERROR: Failed to allocate timer channel %d\n", chan);
       return -EBUSY;
@@ -270,6 +282,11 @@ int sam_oneshot_start(struct sam_oneshot_s *oneshot, oneshot_handler_t handler,
       (void)sam_oneshot_cancel(oneshot, NULL);
     }
 
+  /* Save the new handler and its argument */
+
+  oneshot->handler = handler;
+  oneshot->arg     = arg;
+
   /* We configured the counter to run with an LSB of the specified
    * resolution.  We now must need need to set RC to the number
    * of resolution units corresponding to the requested delay.
@@ -278,23 +295,24 @@ int sam_oneshot_start(struct sam_oneshot_s *oneshot, oneshot_handler_t handler,
   usec   = (uint64_t)ts->tv_sec * 1000000 + (uint64_t)(ts->tv_nsec / 1000);
   regval = usec / oneshot->resolution;
 
-  tcdbg("usec=%llu regval=%08llx\n", usec, regval);
+  tcdbg("usec=%lu regval=%08lx\n",
+        (unsigned long)usec, (unsigned long)regval);
   DEBUGASSERT(regval <= UINT32_MAX);
 
   /* Set up to receive the callback when the interrupt occurs */
 
-  (void)sam_tc_attach(oneshot->handle, sam_oneshot_handler, oneshot,
+  (void)sam_tc_attach(oneshot->tch, sam_oneshot_handler, oneshot,
                       TC_INT_CPCS);
 
   /* Set RC so that an event will be triggered when TC_CV register counts
    * up to RC.
    */
 
-  sam_tc_setregister(oneshot->handle, TC_REGC, regval);
+  sam_tc_setregister(oneshot->tch, TC_REGC, (uint32_t)regval);
 
   /* Start the counter */
 
-  sam_tc_start(oneshot->handle);
+  sam_tc_start(oneshot->tch);
 
   /* Enable interrupts.  We should get the callback when the interrupt
    * occurs.
@@ -311,16 +329,21 @@ int sam_oneshot_start(struct sam_oneshot_s *oneshot, oneshot_handler_t handler,
  * Description:
  *   Cancel the oneshot timer and return the time remaining on the timer.
  *
+ *   NOTE: This function may execute at a high rate with no timer running (as
+ *   when pre-emption is enabled and disabled).
+ *
  * Input Parameters:
  *   oneshot Caller allocated instance of the oneshot state structure.  This
  *           structure must have been previously initialized via a call to
  *           sam_oneshot_initialize();
  *   ts      The location in which to return the time remaining on the
- *           oneshot timer.
+ *           oneshot timer.  A time of zero is returned if the timer is
+ *           not running.
  *
  * Returned Value:
- *   Zero (OK) is returned on success; a negated errno value is returned
- *   on failure.
+ *   Zero (OK) is returned on success.  A call to up_timer_cancel() when
+ *   the timer is not active should also return success; a negated errno
+ *   value is returned on any failure.
  *
  ****************************************************************************/
 
@@ -332,16 +355,35 @@ int sam_oneshot_cancel(struct sam_oneshot_s *oneshot, struct timespec *ts)
   uint32_t usec;
   uint32_t sec;
 
-  /* Get the timer counter and rc registers and stop the counter.  If the
-   * counter expires while we are doing this, the counter clock will be
+  /* Was the timer running? */
+
+  flags = irqsave();
+  if (!oneshot->running)
+    {
+      /* No.. Just return zero timer remaining and successful cancellation.
+       * This function may execute at a high rate with no timer running
+       * (as when pre-emption is enabled and disabled).
+       */
+
+      ts->tv_sec  = 0;
+      ts->tv_nsec = 0;
+      irqrestore(flags);
+      return OK;
+    }
+
+  /* Yes.. Get the timer counter and rc registers and stop the counter.  If
+   * the counter expires while we are doing this, the counter clock will be
    * stopped, but the clock will not be disabled.
    *
-   * REVISIT: Will the counter value be reset to zero?
+   * NOTE: This is not documented, but I have observed in this case that the
+   * counter register freezes at a value equal to the RC register.  The
+   * following logic depends on this fact.
    */
 
-  flags = irqsave();
-  count = sam_tc_getcounter(oneshot->handle);
-  rc    = sam_tc_getregister(oneshot->handle, TC_REGC);
+  tcvdbg("Canceling...\n");
+
+  count = sam_tc_getcounter(oneshot->tch);
+  rc    = sam_tc_getregister(oneshot->tch, TC_REGC);
 
   /* Now we can disable the interrupt and stop the timer.
    *
@@ -350,11 +392,13 @@ int sam_oneshot_cancel(struct sam_oneshot_s *oneshot, struct timespec *ts)
    * clock will be disabled, so I am hoping not.
    */
 
-  DEBUGASSERT(count > 0 || (sam_tc_getpending(oneshot->handle) & TC_INT_CPCS) == 0);
-  sam_tc_attach(oneshot->handle, NULL, NULL, 0);
-  sam_tc_stop(oneshot->handle);
+  DEBUGASSERT(count > 0 || (sam_tc_getpending(oneshot->tch) & TC_INT_CPCS) == 0);
+  sam_tc_attach(oneshot->tch, NULL, NULL, 0);
+  sam_tc_stop(oneshot->tch);
 
   oneshot->running = false;
+  oneshot->handler = NULL;
+  oneshot->arg     = NULL;
   irqrestore(flags);
 
   /* The total time remaining is the difference */
diff --git a/nuttx/arch/arm/src/sama5/sam_oneshot.h b/nuttx/arch/arm/src/sama5/sam_oneshot.h
index dd3bfdf42..144e95eb4 100644
--- a/nuttx/arch/arm/src/sama5/sam_oneshot.h
+++ b/nuttx/arch/arm/src/sama5/sam_oneshot.h
@@ -73,13 +73,15 @@ typedef void (*oneshot_handler_t)(void *arg);
 
 struct sam_oneshot_s
 {
-  uint8_t chan;              /* The timer/counter in use */
-  bool running;              /* True: the timer is running */
-  uint16_t resolution;       /* Timer resolution in microseconds */
-  uint32_t divisor;          /* TC divisor derived from resolution */
-  TC_HANDLE handle;          /* Handle returned by sam_tc_initialize() */
-  oneshot_handler_t handler; /* Oneshot expiration callback */
-  void *arg;                 /* The argument that will accompany the callback */
+  uint8_t chan;                       /* The timer/counter in use */
+  volatile bool running;              /* True: the timer is running */
+  uint16_t resolution;                /* Timer resolution in microseconds */
+  uint32_t divisor;                   /* TC divisor derived from resolution */
+  TC_HANDLE tch;                      /* Handle returned by
+                                       * sam_tc_initialize() */
+  volatile oneshot_handler_t handler; /* Oneshot expiration callback */
+  volatile void *arg;                 /* The argument that will accompany
+                                       * the callback */
 };
 
 /****************************************************************************
@@ -151,16 +153,21 @@ int sam_oneshot_start(struct sam_oneshot_s *oneshot, oneshot_handler_t handler,
  * Description:
  *   Cancel the oneshot timer and return the time remaining on the timer.
  *
+ *   NOTE: This function may execute at a high rate with no timer running (as
+ *   when pre-emption is enabled and disabled).
+ *
  * Input Parameters:
  *   oneshot Caller allocated instance of the oneshot state structure.  This
  *           structure must have been previously initialized via a call to
  *           sam_oneshot_initialize();
  *   ts      The location in which to return the time remaining on the
- *           oneshot timer.
+ *           oneshot timer.  A time of zero is returned if the timer is
+ *           not running.
  *
  * Returned Value:
- *   Zero (OK) is returned on success; a negated errno value is returned
- *   on failure.
+ *   Zero (OK) is returned on success.  A call to up_timer_cancel() when
+ *   the timer is not active should also return success; a negated errno
+ *   value is returned on any failure.
  *
  ****************************************************************************/
 
diff --git a/nuttx/arch/arm/src/sama5/sam_tc.c b/nuttx/arch/arm/src/sama5/sam_tc.c
index 942f1912b..c55398546 100644
--- a/nuttx/arch/arm/src/sama5/sam_tc.c
+++ b/nuttx/arch/arm/src/sama5/sam_tc.c
@@ -1264,8 +1264,8 @@ tc_handler_t sam_tc_attach(TC_HANDLE handle, tc_handler_t handler,
 
   /* Remember the old interrupt handler and set the new handler */
 
-  flags = irqsave();
-  oldhandler = handler;
+  flags         = irqsave();
+  oldhandler    = chan->handler;
   chan->handler = handler;
 
   /* Don't enable interrupt if we are detaching no matter what the caller
@@ -1278,6 +1278,8 @@ tc_handler_t sam_tc_attach(TC_HANDLE handle, tc_handler_t handler,
       mask = 0;
     }
 
+  chan->arg = arg;
+
   /* Now enable interrupt as requested */
 
   sam_chan_putreg(chan, SAM_TC_IDR_OFFSET, TC_INT_ALL & ~mask);
diff --git a/nuttx/arch/arm/src/sama5/sam_tc.h b/nuttx/arch/arm/src/sama5/sam_tc.h
index 6aa7cb653..c7175347c 100644
--- a/nuttx/arch/arm/src/sama5/sam_tc.h
+++ b/nuttx/arch/arm/src/sama5/sam_tc.h
@@ -81,13 +81,13 @@ typedef void *TC_HANDLE;
 /* Timer interrupt callback.  When a timer interrupt expires, the client will
  * receive:
  *
- *   handle - The handle that represents the timer state
- *   arg    - An opaque argument provided when the interrupt was registered
- *   sr     - The value of the timer interrupt status register at the time
- *            that the interrupt occurred.
+ *   tch - The handle that represents the timer state
+ *   arg - An opaque argument provided when the interrupt was registered
+ *   sr  - The value of the timer interrupt status register at the time
+ *         that the interrupt occurred.
  */
 
-typedef void (*tc_handler_t)(TC_HANDLE handle, void *arg, uint32_t sr);
+typedef void (*tc_handler_t)(TC_HANDLE tch, void *arg, uint32_t sr);
 
 /****************************************************************************
  * Public Data
diff --git a/nuttx/arch/arm/src/sama5/sam_tickless.c b/nuttx/arch/arm/src/sama5/sam_tickless.c
index 36b1759f8..cbe8262e9 100644
--- a/nuttx/arch/arm/src/sama5/sam_tickless.c
+++ b/nuttx/arch/arm/src/sama5/sam_tickless.c
@@ -151,6 +151,8 @@
 #ifdef CONFIG_SAMA5_TC_DEBUG
 #  define tcdbg    dbg
 #  define tcvdbg   vdbg
+#  define tcdbg    lldbg
+#  define tcvdbg   llvdbg
 #  define tclldbg  lldbg
 #  define tcllvdbg llvdbg
 #else
@@ -204,6 +206,7 @@ struct sam_tickless_s g_tickless;
 
 static void sam_oneshot_handler(void *arg)
 {
+  tcllvdbg("Expired...\n");
   sched_timer_expiration();
 }
 
@@ -315,6 +318,9 @@ int up_timer_gettime(FAR struct timespec *ts)
  *   that up_timer_start() and the remaining time of zero should be
  *   returned.
  *
+ *   NOTE: This function may execute at a high rate with no timer running (as
+ *   when pre-emption is enabled and disabled).
+ *
  *   Provided by platform-specific code and called from the RTOS base code.
  *
  * Input Parameters:
@@ -322,8 +328,9 @@ int up_timer_gettime(FAR struct timespec *ts)
  *        if the timer is not active.
  *
  * Returned Value:
- *   Zero (OK) is returned on success; a negated errno value is returned on
- *   any failure.
+ *   Zero (OK) is returned on success.  A call to up_timer_cancel() when
+ *   the timer is not active should also return success; a negated errno
+ *   value is returned on any failure.
  *
  * Assumptions:
  *   May be called from interrupt level handling or from the normal tasking
diff --git a/nuttx/drivers/serial/serial.c b/nuttx/drivers/serial/serial.c
index 9754bf391..b2b4cccc2 100644
--- a/nuttx/drivers/serial/serial.c
+++ b/nuttx/drivers/serial/serial.c
@@ -435,8 +435,8 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer,
            *
            * OXTABS - primarily a full-screen terminal optimisation
            * ONOEOT - Unix interoperability hack
-           * OLCUC - Not specified by Posix
-           * ONOCR - low-speed interactive optimisation
+           * OLCUC  - Not specified by POSIX
+           * ONOCR  - low-speed interactive optimisation
            */
         }
 
@@ -447,7 +447,6 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer,
         {
           ret = uart_putxmitchar(dev, '\r', oktoblock);
         }
-
 #endif
 
       /* Put the character into the transmit buffer */
diff --git a/nuttx/include/nuttx/arch.h b/nuttx/include/nuttx/arch.h
index 5a7ee87e6..d25171796 100644
--- a/nuttx/include/nuttx/arch.h
+++ b/nuttx/include/nuttx/arch.h
@@ -1042,6 +1042,9 @@ int up_timer_gettime(FAR struct timespec *ts);
  *   that up_timer_start() and the remaining time of zero should be
  *   returned.
  *
+ *   NOTE: This function may execute at a high rate with no timer running (as
+ *   when pre-emption is enabled and disabled).
+ *
  *   Provided by platform-specific code and called from the RTOS base code.
  *
  * Input Parameters:
@@ -1049,8 +1052,9 @@ int up_timer_gettime(FAR struct timespec *ts);
  *        if the timer is not active.
  *
  * Returned Value:
- *   Zero (OK) is returned on success; a negated errno value is returned on
- *   any failure.
+ *   Zero (OK) is returned on success.  A call to up_timer_cancel() when
+ *   the timer is not active should also return success; a negated errno
+ *   value is returned on any failure.
  *
  * Assumptions:
  *   May be called from interrupt level handling or from the normal tasking
diff --git a/nuttx/libc/stdio/lib_rawsostream.c b/nuttx/libc/stdio/lib_rawsostream.c
index 489a0fe9b..7d971a36a 100644
--- a/nuttx/libc/stdio/lib_rawsostream.c
+++ b/nuttx/libc/stdio/lib_rawsostream.c
@@ -73,8 +73,8 @@ static void rawoutstream_putc(FAR struct lib_outstream_s *this, int ch)
         }
 
       /* The only expected error is EINTR, meaning that the write operation
-       * was awakened by a signal.  Zero would not be a valid return value
-       * from write().
+       * was awakened by a signal.  Zero or values > 1 would not be valid
+       * return values from write().
        */
 
       DEBUGASSERT(nwritten < 0);