In am335x_dmtpps, use a spin mutex to interlock between PPS capture and PPS

ioctl(2) handling.  This allows doing the pps_event() work in the polling
routine, instead of using a taskqueue task to do that work.

Also, add PNPINFO, and switch to using make_dev_s() to create the cdev.

Using a spin mutex and calling pps_event() from the polling function works
around the situation which requires more than 2 sets of timecounter
timehands in a single-core system to get reliable PPS capture.  That problem
would happen when a single-core system is idle in cpu_idle() then gets woken
up with an event timer event which was scheduled to handle a hardclock tick.
That processing path would end up calling tc_windup 3 or 4 times between
when the tc polling function was called and when the taskqueue task would
eventually run, and with only two sets of timehands, the th_generation count
would always be too old to allow the captured PPS data to be used.

diff --git a/sys/arm/ti/am335x/am335x_dmtpps.c b/sys/arm/ti/am335x/am335x_dmtpps.c
index db4de8a7760c591da0c4c1737cd4625a4aea47c1..c718f582702f3a777f42fc81198366d3f50b6a3b 100644 (file)
--- a/sys/arm/ti/am335x/am335x_dmtpps.c
+++ b/sys/arm/ti/am335x/am335x_dmtpps.c
@@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
 #include <sys/malloc.h>
 #include <sys/mutex.h>
 #include <sys/rman.h>
-#include <sys/taskqueue.h>
 #include <sys/timepps.h>
 #include <sys/timetc.h>
 #include <machine/bus.h>
@@ -79,7 +78,6 @@ struct dmtpps_softc {
        uint32_t                tclr;           /* Cached TCLR register. */
        struct timecounter      tc;
        int                     pps_curmode;    /* Edge mode now set in hw. */
-       struct task             pps_task;       /* For pps_event handling. */
        struct cdev *           pps_cdev;
        struct pps_state        pps_state;
        struct mtx              pps_mtx;
@@ -93,6 +91,7 @@ static struct ofw_compat_data compat_data[] = {
        {"ti,am335x-timer-1ms", 1},
        {NULL,                  0},
 };
+SIMPLEBUS_PNP_INFO(compat_data);
 
 /*
  * A table relating pad names to the hardware timer number they can be mux'd to.
@@ -285,46 +284,27 @@ dmtpps_poll(struct timecounter *tc)
         * populates it from the current DMT_TCRR register) with the latched
         * value from the TCAR1 register.
         *
-        * There is no locking here, by design.  pps_capture() writes into an
-        * area of struct pps_state which is read only by pps_event().  The
-        * synchronization of access to that area is temporal rather than
-        * interlock based... we write in this routine and trigger the task that
-        * will read the data, so no simultaneous access can occur.
-        *
         * Note that we don't have the TCAR interrupt enabled, but the hardware
         * still provides the status bits in the "RAW" status register even when
         * they're masked from generating an irq.  However, when clearing the
         * TCAR status to re-arm the capture for the next second, we have to
         * write to the IRQ status register, not the RAW register.  Quirky.
+        *
+        * We do not need to hold a lock while capturing the pps data, because
+        * it is captured into an area of the pps_state struct which is read
+        * only by pps_event().  We do need to hold a lock while calling
+        * pps_event(), because it manipulates data which is also accessed from
+        * the ioctl(2) context by userland processes.
         */
        if (DMTIMER_READ4(sc, DMT_IRQSTATUS_RAW) & DMT_IRQ_TCAR) {
                pps_capture(&sc->pps_state);
                sc->pps_state.capcount = DMTIMER_READ4(sc, DMT_TCAR1);
                DMTIMER_WRITE4(sc, DMT_IRQSTATUS, DMT_IRQ_TCAR);
-               taskqueue_enqueue(taskqueue_fast, &sc->pps_task);
-       }
-}
-
-static void
-dmtpps_event(void *arg, int pending)
-{
-       struct dmtpps_softc *sc;
-
-       sc = arg;
 
-       /* This is the task function that gets enqueued by poll_pps.  Once the
-        * time has been captured by the timecounter polling code which runs in
-        * primary interrupt context, the remaining (more expensive) work to
-        * process the event is done later in a threaded context.
-        *
-        * Here there is an interlock that protects the event data in struct
-        * pps_state.  That data can be accessed at any time from userland via
-        * ioctl() calls so we must ensure that there is no read access to
-        * partially updated data while pps_event() does its work.
-        */
-       mtx_lock(&sc->pps_mtx);
-       pps_event(&sc->pps_state, PPS_CAPTUREASSERT);
-       mtx_unlock(&sc->pps_mtx);
+               mtx_lock_spin(&sc->pps_mtx);
+               pps_event(&sc->pps_state, PPS_CAPTUREASSERT);
+               mtx_unlock_spin(&sc->pps_mtx);
+       }
 }
 
 static int
@@ -374,9 +354,9 @@ dmtpps_ioctl(struct cdev *dev, u_long cmd, caddr_t data,
        sc = dev->si_drv1;
 
        /* Let the kernel do the heavy lifting for ioctl. */
-       mtx_lock(&sc->pps_mtx);
+       mtx_lock_spin(&sc->pps_mtx);
        err = pps_ioctl(cmd, data, &sc->pps_state);
-       mtx_unlock(&sc->pps_mtx);
+       mtx_unlock_spin(&sc->pps_mtx);
        if (err != 0)
                return (err);
 
@@ -436,6 +416,7 @@ static int
 dmtpps_attach(device_t dev)
 {
        struct dmtpps_softc *sc;
+       struct make_dev_args mda;
        clk_ident_t timer_id;
        int err, sysclk_freq;
 
@@ -502,22 +483,27 @@ dmtpps_attach(device_t dev)
         * now, just say we can only capture assert events (the positive-going
         * edge of the pulse).
         */
-       mtx_init(&sc->pps_mtx, "dmtpps", NULL, MTX_DEF);
+       mtx_init(&sc->pps_mtx, "dmtpps", NULL, MTX_SPIN);
+       sc->pps_state.flags = PPSFLAG_MTX_SPIN;
        sc->pps_state.ppscap = PPS_CAPTUREASSERT;
        sc->pps_state.driver_abi = PPS_ABI_VERSION;
        sc->pps_state.driver_mtx = &sc->pps_mtx;
        pps_init_abi(&sc->pps_state);
 
-       /*
-        * Init the task that does deferred pps_event() processing after
-        * the polling routine has captured a pps pulse time.
-        */
-       TASK_INIT(&sc->pps_task, 0, dmtpps_event, sc);
-
        /* Create the PPS cdev. */
-       sc->pps_cdev = make_dev(&dmtpps_cdevsw, 0, UID_ROOT, GID_WHEEL, 0600,
-           PPS_CDEV_NAME);
-       sc->pps_cdev->si_drv1 = sc;
+       make_dev_args_init(&mda);
+       mda.mda_flags = MAKEDEV_WAITOK;
+       mda.mda_devsw = &dmtpps_cdevsw;
+       mda.mda_cr = NULL;
+       mda.mda_uid = UID_ROOT;
+       mda.mda_gid = GID_WHEEL;
+       mda.mda_mode = 0600;
+       mda.mda_unit = device_get_unit(dev);
+       mda.mda_si_drv1 = sc;
+       if ((err = make_dev_s(&mda, &sc->pps_cdev, PPS_CDEV_NAME)) != 0) {
+               device_printf(dev, "Failed to create cdev %s\n", PPS_CDEV_NAME);
+               return (err);
+       }
 
        if (bootverbose)
                device_printf(sc->dev, "Using %s for PPS device /dev/%s\n",