[src/trunk]: src/sys/arch/arm/nvidia Use a separate lock (not the i2c bus loc...

[thorpej <thorpej%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/6ad1a5820dbf
branches:  trunk
changeset: 466466:6ad1a5820dbf
user:      thorpej <thorpej%NetBSD.org@localhost>
date:      Sun Dec 22 23:40:49 2019 +0000

description:
Use a separate lock (not the i2c bus lock) to synchronize with the
interrupt handler.  This in all liklihood fixes a deadlock bug that
necessitated forcing I2C_F_POLL in tegra_i2c_exec() (someone who has
the hardware should test removing that line).

Also includes the changes for:

Cleanup i2c bus acquire / release, centralizing all of the logic into
iic_acquire_bus() / iic_release_bus().  "acquire" and "release" hooks
no longer need to be provided by back-end controller drivers (only if
they need special handling, e.g. powering on the i2c controller).
This results in the removal of a bunch of rendundant code from each
back-end controller driver.

Assert that we are not in hard interrupt context in iic_acquire_bus(),
iic_exec(), and iic_release_bus().

diffstat:

 sys/arch/arm/nvidia/tegra_i2c.c |  67 +++++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 39 deletions(-)

diffs (153 lines):

diff -r 36b7fe8ee49e -r 6ad1a5820dbf sys/arch/arm/nvidia/tegra_i2c.c
--- a/sys/arch/arm/nvidia/tegra_i2c.c   Sun Dec 22 23:24:56 2019 +0000
+++ b/sys/arch/arm/nvidia/tegra_i2c.c   Sun Dec 22 23:40:49 2019 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: tegra_i2c.c,v 1.22 2018/09/25 22:23:22 jmcneill Exp $ */
+/* $NetBSD: tegra_i2c.c,v 1.23 2019/12/22 23:40:49 thorpej Exp $ */
 
 /*-
  * Copyright (c) 2015 Jared D. McNeill <jmcneill%invisible.ca@localhost>
@@ -27,7 +27,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: tegra_i2c.c,v 1.22 2018/09/25 22:23:22 jmcneill Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tegra_i2c.c,v 1.23 2019/12/22 23:40:49 thorpej Exp $");
 
 #include <sys/param.h>
 #include <sys/bus.h>
@@ -63,15 +63,13 @@
        u_int                   sc_cid;
 
        struct i2c_controller   sc_ic;
-       kmutex_t                sc_lock;
-       kcondvar_t              sc_cv;
+       kmutex_t                sc_intr_lock;
+       kcondvar_t              sc_intr_wait;
 };
 
 static void    tegra_i2c_init(struct tegra_i2c_softc *);
 static int     tegra_i2c_intr(void *);
 
-static int     tegra_i2c_acquire_bus(void *, int);
-static void    tegra_i2c_release_bus(void *, int);
 static int     tegra_i2c_exec(void *, i2c_op_t, i2c_addr_t, const void *,
                               size_t, void *, size_t, int);
 
@@ -140,8 +138,8 @@
                    addr, error);
                return;
        }
-       mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_VM);
-       cv_init(&sc->sc_cv, device_xname(self));
+       mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_VM);
+       cv_init(&sc->sc_intr_wait, device_xname(self));
 
        aprint_naive("\n");
        aprint_normal(": I2C\n");
@@ -177,11 +175,12 @@
        }
        fdtbus_reset_deassert(sc->sc_rst);
 
+       mutex_enter(&sc->sc_intr_lock);
        tegra_i2c_init(sc);
+       mutex_exit(&sc->sc_intr_lock);
 
+       iic_tag_init(&sc->sc_ic);
        sc->sc_ic.ic_cookie = sc;
-       sc->sc_ic.ic_acquire_bus = tegra_i2c_acquire_bus;
-       sc->sc_ic.ic_release_bus = tegra_i2c_release_bus;
        sc->sc_ic.ic_exec = tegra_i2c_exec;
 
        fdtbus_register_i2c_controller(self, phandle, &tegra_i2c_funcs);
@@ -236,48 +235,34 @@
                return 0;
        I2C_WRITE(sc, I2C_INTERRUPT_STATUS_REG, istatus);
 
-       mutex_enter(&sc->sc_lock);
-       cv_broadcast(&sc->sc_cv);
-       mutex_exit(&sc->sc_lock);
+       mutex_enter(&sc->sc_intr_lock);
+       cv_broadcast(&sc->sc_intr_wait);
+       mutex_exit(&sc->sc_intr_lock);
 
        return 1;
 }
 
 static int
-tegra_i2c_acquire_bus(void *priv, int flags)
-{
-       struct tegra_i2c_softc * const sc = priv;
-
-       mutex_enter(&sc->sc_lock);
-
-       return 0;
-}
-
-static void
-tegra_i2c_release_bus(void *priv, int flags)
-{
-       struct tegra_i2c_softc * const sc = priv;
-
-       mutex_exit(&sc->sc_lock);
-}
-
-static int
 tegra_i2c_exec(void *priv, i2c_op_t op, i2c_addr_t addr, const void *cmdbuf,
     size_t cmdlen, void *buf, size_t buflen, int flags)
 {
        struct tegra_i2c_softc * const sc = priv;
        int retry, error;
 
-#if notyet
-       if (cold)
-#endif
-               flags |= I2C_F_POLL;
-
-       KASSERT(mutex_owned(&sc->sc_lock));
+       /*
+        * XXXJRT This is probably no longer necessary?  Before these
+        * changes, the bus lock was also used for the interrupt handler,
+        * and there would be a deadlock when the interrupt handler tried to
+        * acquire it again.  The bus lock is now owned by the mid-layer and
+        * we have our own interrupt lock.
+        */
+       flags |= I2C_F_POLL;
 
        if (buflen == 0 && cmdlen == 0)
                return EINVAL;
 
+       mutex_enter(&sc->sc_intr_lock);
+
        if ((flags & I2C_F_POLL) == 0) {
                I2C_WRITE(sc, I2C_INTERRUPT_MASK_REG,
                    I2C_INTERRUPT_MASK_NOACK | I2C_INTERRUPT_MASK_ARB_LOST |
@@ -296,6 +281,7 @@
                delay(1);
        }
        if (retry == 0) {
+               mutex_exit(&sc->sc_intr_lock);
                device_printf(sc->sc_dev, "timeout flushing FIFO\n");
                return EIO;
        }
@@ -325,6 +311,8 @@
                tegra_i2c_init(sc);
        }
 
+       mutex_exit(&sc->sc_intr_lock);
+
        return error;
 }
 
@@ -338,8 +326,9 @@
 
        while (--retry > 0) {
                if ((flags & I2C_F_POLL) == 0) {
-                       error = cv_timedwait_sig(&sc->sc_cv, &sc->sc_lock,
-                           uimax(mstohz(10), 1));
+                       error = cv_timedwait_sig(&sc->sc_intr_wait,
+                                                &sc->sc_intr_lock,
+                                                uimax(mstohz(10), 1));
                        if (error) {
                                return error;
                        }