Date: Fri, 5 Jan 2018 19:21:45 +0000 (UTC) From: Ian Lepore <ian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r327594 - stable/11/sys/arm/broadcom/bcm2835 Message-ID: <201801051921.w05JLjs9041014@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ian Date: Fri Jan 5 19:21:45 2018 New Revision: 327594 URL: https://svnweb.freebsd.org/changeset/base/327594 Log: MFC r324169: Work around bcm283x silicon bugs to make i2c repeat-start work for the most common case where it's needed -- a write followed by a read to the same slave. The i2c controller in this chip only performs complete transfers, it does not provide control over start/repeat-start/stop operations on the bus. Thus, we have gotten a full stop/start sequence rather than a repeat-start when doing a typical i2c slave access of "write address, read data". Some i2c slave devices require a repeat-start to work correctly. These changes cause the controller to do a repeat-start by pre-staging the read parameters in the controller registers immediate after the controller has latched the values for the initial write operation, but before any bytes are actually written. With the values pre-staged, when the write portion of the transfer completes, the state machine in the silicon sees a new start operation already staged and that causes it to perform a repeat-start. The key to tricking the buggy hardware into doing this is to avoid prefilling any output data in the transmit FIFO so that it is possible to catch the silicon in the state where transmit values are latched but the transmit isn't completed yet. Modified: stable/11/sys/arm/broadcom/bcm2835/bcm2835_bsc.c stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscreg.h stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscvar.h Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/arm/broadcom/bcm2835/bcm2835_bsc.c ============================================================================== --- stable/11/sys/arm/broadcom/bcm2835/bcm2835_bsc.c Fri Jan 5 19:15:24 2018 (r327593) +++ stable/11/sys/arm/broadcom/bcm2835/bcm2835_bsc.c Fri Jan 5 19:21:45 2018 (r327594) @@ -2,6 +2,7 @@ * Copyright (c) 2001 Tsubai Masanari. * Copyright (c) 2012 Oleksandr Tymoshenko <gonzo@freebsd.org> * Copyright (c) 2013 Luiz Otavio O Souza <loos@freebsd.org> + * Copyright (c) 2017 Ian Lepore <ian@freebsd.org> * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -29,6 +30,57 @@ #include <sys/cdefs.h> __FBSDID("$FreeBSD$"); +/* + * Driver for bcm2835 i2c-compatible two-wire bus, named 'BSC' on this SoC. + * + * This controller can only perform complete transfers, it does not provide + * low-level control over sending start/repeat-start/stop sequences on the bus. + * In addition, bugs in the silicon make it somewhat difficult to perform a + * repeat-start, and limit the repeat-start to a read following a write on + * the same slave device. (The i2c protocol allows a repeat start to change + * direction or not, and change slave address or not at any time.) + * + * The repeat-start bug and workaround are described in a problem report at + * https://github.com/raspberrypi/linux/issues/254 with the crucial part being + * in a comment block from a fragment of a GPU i2c driver, containing this: + * + * ----------------------------------------------------------------------------- + * - See i2c.v: The I2C peripheral samples the values for rw_bit and xfer_count + * - in the IDLE state if start is set. + * - + * - We want to generate a ReSTART not a STOP at the end of the TX phase. In + * - order to do that we must ensure the state machine goes RACK1 -> RACK2 -> + * - SRSTRT1 (not RACK1 -> RACK2 -> SSTOP1). + * - + * - So, in the RACK2 state when (TX) xfer_count==0 we must therefore have + * - already set, ready to be sampled: + * - READ ; rw_bit <= I2CC bit 0 -- must be "read" + * - ST; start <= I2CC bit 7 -- must be "Go" in order to not issue STOP + * - DLEN; xfer_count <= I2CDLEN -- must be equal to our read amount + * - + * - The plan to do this is: + * - 1. Start the sub-address write, but don't let it finish + * - (keep xfer_count > 0) + * - 2. Populate READ, DLEN and ST in preparation for ReSTART read sequence + * - 3. Let TX finish (write the rest of the data) + * - 4. Read back data as it arrives + * ----------------------------------------------------------------------------- + * + * The transfer function below scans the list of messages passed to it, looking + * for a read following a write to the same slave. When it finds that, it + * starts the write without prefilling the tx fifo, which holds xfer_count>0, + * then presets the direction, length, and start command for the following read, + * as described above. Then the tx fifo is filled and the rest of the transfer + * proceeds as normal, with the controller automatically supplying a + * repeat-start on the bus when the write operation finishes. + * + * XXX I suspect the controller may be able to do a repeat-start on any + * write->read or write->write transition, even when the slave addresses differ. + * It's unclear whether the slave address can be prestaged along with the + * direction and length while the write xfer_count is being held at zero. In + * fact, if it can't do this, then it couldn't be used to read EDID data. + */ + #include <sys/param.h> #include <sys/systm.h> #include <sys/kernel.h> @@ -58,6 +110,14 @@ static struct ofw_compat_data compat_data[] = { {NULL, 0} }; +#define DEVICE_DEBUGF(sc, lvl, fmt, args...) \ + if ((lvl) <= (sc)->sc_debug) \ + device_printf((sc)->sc_dev, fmt, ##args) + +#define DEBUGF(sc, lvl, fmt, args...) \ + if ((lvl) <= (sc)->sc_debug) \ + printf(fmt, ##args) + static void bcm_bsc_intr(void *); static int bcm_bsc_detach(device_t); @@ -197,6 +257,9 @@ bcm_bsc_sysctl_init(struct bcm_bsc_softc *sc) SYSCTL_ADD_PROC(ctx, tree, OID_AUTO, "rise_edge_delay", CTLFLAG_RW | CTLTYPE_UINT, sc, sizeof(*sc), bcm_bsc_rise_proc, "IU", "I2C BUS rising edge delay"); + SYSCTL_ADD_INT(ctx, tree, OID_AUTO, "debug", + CTLFLAG_RWTUN, &sc->sc_debug, 0, + "Enable debug; 1=reads/writes, 2=add starts/stops"); } static void @@ -322,6 +385,8 @@ bcm_bsc_detach(device_t dev) bus_generic_detach(dev); sc = device_get_softc(dev); + if (sc->sc_iicbus != NULL) + device_delete_child(dev, sc->sc_iicbus); mtx_destroy(&sc->sc_mtx); if (sc->sc_intrhand) bus_teardown_intr(dev, sc->sc_irq_res, sc->sc_intrhand); @@ -334,6 +399,76 @@ bcm_bsc_detach(device_t dev) } static void +bcm_bsc_empty_rx_fifo(struct bcm_bsc_softc *sc) +{ + uint32_t status; + + /* Assumes sc_totlen > 0 and BCM_BSC_STATUS_RXD is asserted on entry. */ + do { + if (sc->sc_resid == 0) { + sc->sc_data = sc->sc_curmsg->buf; + sc->sc_dlen = sc->sc_curmsg->len; + sc->sc_resid = sc->sc_dlen; + ++sc->sc_curmsg; + } + do { + *sc->sc_data = BCM_BSC_READ(sc, BCM_BSC_DATA); + DEBUGF(sc, 1, "0x%02x ", *sc->sc_data); + ++sc->sc_data; + --sc->sc_resid; + --sc->sc_totlen; + status = BCM_BSC_READ(sc, BCM_BSC_STATUS); + } while (sc->sc_resid > 0 && (status & BCM_BSC_STATUS_RXD)); + } while (sc->sc_totlen > 0 && (status & BCM_BSC_STATUS_RXD)); +} + +static void +bcm_bsc_fill_tx_fifo(struct bcm_bsc_softc *sc) +{ + uint32_t status; + + /* Assumes sc_totlen > 0 and BCM_BSC_STATUS_TXD is asserted on entry. */ + do { + if (sc->sc_resid == 0) { + sc->sc_data = sc->sc_curmsg->buf; + sc->sc_dlen = sc->sc_curmsg->len; + sc->sc_resid = sc->sc_dlen; + ++sc->sc_curmsg; + } + do { + BCM_BSC_WRITE(sc, BCM_BSC_DATA, *sc->sc_data); + DEBUGF(sc, 1, "0x%02x ", *sc->sc_data); + ++sc->sc_data; + --sc->sc_resid; + --sc->sc_totlen; + status = BCM_BSC_READ(sc, BCM_BSC_STATUS); + } while (sc->sc_resid > 0 && (status & BCM_BSC_STATUS_TXD)); + /* + * If a repeat-start was pending and we just hit the end of a tx + * buffer, see if it's also the end of the writes that preceeded + * the repeat-start. If so, log the repeat-start and the start + * of the following read, and return because we're not writing + * anymore (and TXD will be true because there's room to write + * in the fifo). + */ + if (sc->sc_replen > 0 && sc->sc_resid == 0) { + sc->sc_replen -= sc->sc_dlen; + if (sc->sc_replen == 0) { + DEBUGF(sc, 1, " err=0\n"); + DEVICE_DEBUGF(sc, 2, "rstart 0x%02x\n", + sc->sc_curmsg->slave | 0x01); + DEVICE_DEBUGF(sc, 1, + "read 0x%02x len %d: ", + sc->sc_curmsg->slave | 0x01, + sc->sc_totlen); + sc->sc_flags |= BCM_I2C_READ; + return; + } + } + } while (sc->sc_totlen > 0 && (status & BCM_BSC_STATUS_TXD)); +} + +static void bcm_bsc_intr(void *arg) { struct bcm_bsc_softc *sc; @@ -350,35 +485,28 @@ bcm_bsc_intr(void *arg) } status = BCM_BSC_READ(sc, BCM_BSC_STATUS); + DEBUGF(sc, 4, " <intrstatus=0x%08x> ", status); - /* Check for errors. */ - if (status & (BCM_BSC_STATUS_CLKT | BCM_BSC_STATUS_ERR)) { - /* Disable interrupts. */ - bcm_bsc_reset(sc); - sc->sc_flags |= BCM_I2C_ERROR; - wakeup(sc->sc_dev); - BCM_BSC_UNLOCK(sc); - return; - } + /* RXD and DONE can assert together, empty fifo before checking done. */ + if ((sc->sc_flags & BCM_I2C_READ) && (status & BCM_BSC_STATUS_RXD)) + bcm_bsc_empty_rx_fifo(sc); - if (sc->sc_flags & BCM_I2C_READ) { - while (sc->sc_resid > 0 && (status & BCM_BSC_STATUS_RXD)) { - *sc->sc_data++ = BCM_BSC_READ(sc, BCM_BSC_DATA); - sc->sc_resid--; - status = BCM_BSC_READ(sc, BCM_BSC_STATUS); - } - } else { - while (sc->sc_resid > 0 && (status & BCM_BSC_STATUS_TXD)) { - BCM_BSC_WRITE(sc, BCM_BSC_DATA, *sc->sc_data++); - sc->sc_resid--; - status = BCM_BSC_READ(sc, BCM_BSC_STATUS); - } - } - - if (status & BCM_BSC_STATUS_DONE) { + /* Check for completion. */ + if (status & (BCM_BSC_STATUS_ERRBITS | BCM_BSC_STATUS_DONE)) { + sc->sc_flags |= BCM_I2C_DONE; + if (status & BCM_BSC_STATUS_ERRBITS) + sc->sc_flags |= BCM_I2C_ERROR; /* Disable interrupts. */ bcm_bsc_reset(sc); - wakeup(sc->sc_dev); + wakeup(sc); + } else if (!(sc->sc_flags & BCM_I2C_READ)) { + /* + * Don't check for TXD until after determining whether the + * transfer is complete; TXD will be asserted along with ERR or + * DONE if there is room in the fifo. + */ + if (status & BCM_BSC_STATUS_TXD) + bcm_bsc_fill_tx_fifo(sc); } BCM_BSC_UNLOCK(sc); @@ -388,8 +516,11 @@ static int bcm_bsc_transfer(device_t dev, struct iic_msg *msgs, uint32_t nmsgs) { struct bcm_bsc_softc *sc; - uint32_t intr, read, status; - int i, err; + struct iic_msg *endmsgs, *nxtmsg; + uint32_t readctl, status; + int err; + uint16_t curlen; + uint8_t curisread, curslave, nxtisread, nxtslave; sc = device_get_softc(dev); BCM_BSC_LOCK(sc); @@ -401,53 +532,156 @@ bcm_bsc_transfer(device_t dev, struct iic_msg *msgs, u /* Now we have control over the BSC controller. */ sc->sc_flags = BCM_I2C_BUSY; + DEVICE_DEBUGF(sc, 3, "Transfer %d msgs\n", nmsgs); + /* Clear the FIFO and the pending interrupts. */ bcm_bsc_reset(sc); + /* + * Perform all the transfers requested in the array of msgs. Note that + * it is bcm_bsc_empty_rx_fifo() and bcm_bsc_fill_tx_fifo() that advance + * sc->sc_curmsg through the array of messages, as the data from each + * message is fully consumed, but it is this loop that notices when we + * have no more messages to process. + */ err = 0; - for (i = 0; i < nmsgs; i++) { + sc->sc_resid = 0; + sc->sc_curmsg = msgs; + endmsgs = &msgs[nmsgs]; + while (sc->sc_curmsg < endmsgs) { + readctl = 0; + curslave = sc->sc_curmsg->slave >> 1; + curisread = sc->sc_curmsg->flags & IIC_M_RD; + sc->sc_replen = 0; + sc->sc_totlen = sc->sc_curmsg->len; + /* + * Scan for scatter/gather IO (same slave and direction) or + * repeat-start (read following write for the same slave). + */ + for (nxtmsg = sc->sc_curmsg + 1; nxtmsg < endmsgs; ++nxtmsg) { + nxtslave = nxtmsg->slave >> 1; + if (curslave == nxtslave) { + nxtisread = nxtmsg->flags & IIC_M_RD; + if (curisread == nxtisread) { + /* + * Same slave and direction, this + * message will be part of the same + * transfer as the previous one. + */ + sc->sc_totlen += nxtmsg->len; + continue; + } else if (curisread == IIC_M_WR) { + /* + * Read after write to same slave means + * repeat-start, remember how many bytes + * come before the repeat-start, switch + * the direction to IIC_M_RD, and gather + * up following reads to the same slave. + */ + curisread = IIC_M_RD; + sc->sc_replen = sc->sc_totlen; + sc->sc_totlen += nxtmsg->len; + continue; + } + } + break; + } + /* + * curslave and curisread temporaries from above may refer to + * the after-repstart msg, reset them to reflect sc_curmsg. + */ + curisread = (sc->sc_curmsg->flags & IIC_M_RD) ? 1 : 0; + curslave = sc->sc_curmsg->slave | curisread; + /* Write the slave address. */ - BCM_BSC_WRITE(sc, BCM_BSC_SLAVE, msgs[i].slave >> 1); + BCM_BSC_WRITE(sc, BCM_BSC_SLAVE, curslave >> 1); - /* Write the data length. */ - BCM_BSC_WRITE(sc, BCM_BSC_DLEN, msgs[i].len); + DEVICE_DEBUGF(sc, 2, "start 0x%02x\n", curslave); - sc->sc_data = msgs[i].buf; - sc->sc_resid = msgs[i].len; - if ((msgs[i].flags & IIC_M_RD) == 0) { - /* Fill up the TX FIFO. */ - status = BCM_BSC_READ(sc, BCM_BSC_STATUS); - while (sc->sc_resid > 0 && - (status & BCM_BSC_STATUS_TXD)) { - BCM_BSC_WRITE(sc, BCM_BSC_DATA, *sc->sc_data); - sc->sc_data++; - sc->sc_resid--; - status = BCM_BSC_READ(sc, BCM_BSC_STATUS); + /* + * Either set up read length and direction variables for a + * simple transfer or get the hardware started on the first + * piece of a transfer that involves a repeat-start and set up + * the read length and direction vars for the second piece. + */ + if (sc->sc_replen == 0) { + DEVICE_DEBUGF(sc, 1, "%-6s 0x%02x len %d: ", + (curisread) ? "readctl" : "write", curslave, + sc->sc_totlen); + curlen = sc->sc_totlen; + if (curisread) { + readctl = BCM_BSC_CTRL_READ; + sc->sc_flags |= BCM_I2C_READ; + } else { + readctl = 0; + sc->sc_flags &= ~BCM_I2C_READ; } - read = 0; - intr = BCM_BSC_CTRL_INTT; - sc->sc_flags &= ~BCM_I2C_READ; } else { - sc->sc_flags |= BCM_I2C_READ; - read = BCM_BSC_CTRL_READ; - intr = BCM_BSC_CTRL_INTR; + DEVICE_DEBUGF(sc, 1, "%-6s 0x%02x len %d: ", + (curisread) ? "readctl" : "write", curslave, + sc->sc_replen); + + /* + * Start the write transfer with an empty fifo and wait + * for the 'transfer active' status bit to light up; + * that indicates that the hardware has latched the + * direction and length for the write, and we can safely + * reload those registers and issue the start for the + * following read; interrupts are not enabled here. + */ + BCM_BSC_WRITE(sc, BCM_BSC_DLEN, sc->sc_replen); + BCM_BSC_WRITE(sc, BCM_BSC_CTRL, BCM_BSC_CTRL_I2CEN | + BCM_BSC_CTRL_ST); + do { + status = BCM_BSC_READ(sc, BCM_BSC_STATUS); + if (status & BCM_BSC_STATUS_ERR) { + /* no ACK on slave addr */ + err = EIO; + goto xfer_done; + } + } while ((status & BCM_BSC_STATUS_TA) == 0); + /* + * Set curlen and readctl for the repeat-start read that + * we need to set up below, but set sc_flags to write, + * because that is the operation in progress right now. + */ + curlen = sc->sc_totlen - sc->sc_replen; + readctl = BCM_BSC_CTRL_READ; + sc->sc_flags &= ~BCM_I2C_READ; } - intr |= BCM_BSC_CTRL_INTD; - /* Start the transfer. */ - BCM_BSC_WRITE(sc, BCM_BSC_CTRL, BCM_BSC_CTRL_I2CEN | - BCM_BSC_CTRL_ST | read | intr); + /* + * Start the transfer with interrupts enabled, then if doing a + * write, fill the tx fifo. Not prefilling the fifo until after + * this start command is the key workaround for making + * repeat-start work, and it's harmless to do it in this order + * for a regular write too. + */ + BCM_BSC_WRITE(sc, BCM_BSC_DLEN, curlen); + BCM_BSC_WRITE(sc, BCM_BSC_CTRL, readctl | BCM_BSC_CTRL_I2CEN | + BCM_BSC_CTRL_ST | BCM_BSC_CTRL_INT_ALL); - /* Wait for the transaction to complete. */ - err = mtx_sleep(dev, &sc->sc_mtx, 0, "bsciow", hz); + if (!(sc->sc_curmsg->flags & IIC_M_RD)) { + bcm_bsc_fill_tx_fifo(sc); + } + /* Wait for the transaction to complete. */ + while (err == 0 && !(sc->sc_flags & BCM_I2C_DONE)) { + err = mtx_sleep(sc, &sc->sc_mtx, 0, "bsciow", hz); + } /* Check for errors. */ if (err == 0 && (sc->sc_flags & BCM_I2C_ERROR)) err = EIO; +xfer_done: + DEBUGF(sc, 1, " err=%d\n", err); + DEVICE_DEBUGF(sc, 2, "stop\n"); if (err != 0) break; } + + /* Disable interrupts, clean fifo, etc. */ + bcm_bsc_reset(sc); /* Clean the controller flags. */ sc->sc_flags = 0; Modified: stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscreg.h ============================================================================== --- stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscreg.h Fri Jan 5 19:15:24 2018 (r327593) +++ stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscreg.h Fri Jan 5 19:21:45 2018 (r327594) @@ -40,6 +40,9 @@ #define BCM_BSC_CTRL_CLEAR1 (1 << 5) #define BCM_BSC_CTRL_CLEAR0 (1 << 4) #define BCM_BSC_CTRL_READ (1 << 0) +#define BCM_BSC_CTRL_INT_ALL \ + (BCM_BSC_CTRL_INTR | BCM_BSC_CTRL_INTT | BCM_BSC_CTRL_INTD) + #define BCM_BSC_STATUS 0x04 #define BCM_BSC_STATUS_CLKT (1 << 9) #define BCM_BSC_STATUS_ERR (1 << 8) @@ -51,6 +54,9 @@ #define BCM_BSC_STATUS_TXW (1 << 2) #define BCM_BSC_STATUS_DONE (1 << 1) #define BCM_BSC_STATUS_TA (1 << 0) +#define BCM_BSC_STATUS_ERRBITS \ + (BCM_BSC_STATUS_CLKT | BCM_BSC_STATUS_ERR) + #define BCM_BSC_DLEN 0x08 #define BCM_BSC_SLAVE 0x0c #define BCM_BSC_DATA 0x10 Modified: stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscvar.h ============================================================================== --- stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscvar.h Fri Jan 5 19:15:24 2018 (r327593) +++ stable/11/sys/arm/broadcom/bcm2835/bcm2835_bscvar.h Fri Jan 5 19:21:45 2018 (r327594) @@ -40,23 +40,31 @@ struct { }; #define BCM_BSC_BASE_MASK 0x00ffffff +struct iic_msg; + struct bcm_bsc_softc { device_t sc_dev; device_t sc_iicbus; struct mtx sc_mtx; struct resource * sc_mem_res; struct resource * sc_irq_res; + void * sc_intrhand; + struct iic_msg * sc_curmsg; bus_space_tag_t sc_bst; bus_space_handle_t sc_bsh; + int sc_debug; + uint16_t sc_replen; + uint16_t sc_totlen; uint16_t sc_resid; - uint8_t *sc_data; + uint16_t sc_dlen; + uint8_t * sc_data; uint8_t sc_flags; - void * sc_intrhand; }; #define BCM_I2C_BUSY 0x01 #define BCM_I2C_READ 0x02 #define BCM_I2C_ERROR 0x04 +#define BCM_I2C_DONE 0x08 #define BCM_BSC_WRITE(_sc, _off, _val) \ bus_space_write_4((_sc)->sc_bst, (_sc)->sc_bsh, _off, _val)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201801051921.w05JLjs9041014>