Dear Wolfram Sang,
> Hi Marek,
>
> On Wed, Nov 21, 2012 at 06:51:28AM +0100, Marek Vasut wrote:
> > Add support for the PIO mode and mixed PIO/DMA mode support. The mixed
> > PIO/DMA is the default mode of operation. This shall leverage overhead
> > that the driver creates due to setting up DMA descriptors even for very
> > short transfers.
> >
> > The current boundary between PIO/DMA 8 bytes, transfers shorter than 8
> > bytes are transfered by PIO, longer transfers use DMA. The performance
> > of write transfers remains unchanged, while there is a minor improvement
> > of read performance. Reading 16KB EEPROM with DMA-only operations gives
> > a read speed of 39.5KB/s, while with then new mixed-mode the speed is
> > blazing 40.6KB/s.
> >
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > Cc: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx>
> > Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > Cc: Wolfram Sang <w.sang@xxxxxxxxxxxxxx>
> > ---
> >
> > drivers/i2c/busses/i2c-mxs.c | 163
> > ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 150
> > insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> > index 47d5d53..9048f34 100644
> > --- a/drivers/i2c/busses/i2c-mxs.c
> > +++ b/drivers/i2c/busses/i2c-mxs.c
> > @@ -39,6 +39,7 @@
> >
> > #define MXS_I2C_CTRL0_SET (0x04)
> >
> > #define MXS_I2C_CTRL0_SFTRST 0x80000000
> >
> > +#define MXS_I2C_CTRL0_RUN 0x20000000
> >
> > #define MXS_I2C_CTRL0_SEND_NAK_ON_LAST 0x02000000
> > #define MXS_I2C_CTRL0_RETAIN_CLOCK 0x00200000
> > #define MXS_I2C_CTRL0_POST_SEND_STOP 0x00100000
> >
> > @@ -64,6 +65,13 @@
> >
> > #define MXS_I2C_CTRL1_SLAVE_STOP_IRQ 0x02
> > #define MXS_I2C_CTRL1_SLAVE_IRQ 0x01
> >
> > +#define MXS_I2C_DATA (0xa0)
> > +
> > +#define MXS_I2C_DEBUG0 (0xb0)
> > +#define MXS_I2C_DEBUG0_CLR (0xb8)
> > +
> > +#define MXS_I2C_DEBUG0_DMAREQ 0x80000000
> > +
> >
> > #define MXS_I2C_IRQ_MASK (MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ | \
> >
> > MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ | \
> > MXS_I2C_CTRL1_EARLY_TERM_IRQ | \
> >
> > @@ -298,6 +306,128 @@ write_init_pio_fail:
> > return -EINVAL;
> >
> > }
> >
> > +static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c)
> > +{
> > + unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> > +
> > + while (!(readl(i2c->regs + MXS_I2C_DEBUG0) &
> > + MXS_I2C_DEBUG0_DMAREQ)) {
> > + if (time_after(jiffies, timeout))
> > + return -ETIMEDOUT;
> > + cond_resched();
> > + }
> > +
> > + writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR);
> > +
> > + return 0;
> > +}
> > +
> > +static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
> > +{
> > + unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> > +
> > + while (!(readl(i2c->regs + MXS_I2C_CTRL1) &
> > + MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ)) {
> > + if (time_after(jiffies, timeout))
> > + return -ETIMEDOUT;
> > + cond_resched();
> > + }
> > +
> > + writel(MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ,
> > + i2c->regs + MXS_I2C_CTRL1_CLR);
> > +
> > + return 0;
> > +}
>
> That's a lot of polling. Didn't interrupts work?
Oh they do work, but check below, it transfers 8 bytes of data max via PIO. The
overhead of interrupt handling would be much larger. Thus, polling.
> > +static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msg, uint32_t flags)
> > +{
> > + struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
> > + uint32_t addr_data = msg->addr << 1;
> > + uint32_t data = 0;
> > + int i, shifts_left, ret;
> > +
> > + /* Mute IRQs coming from this block. */
> > + writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR);
> > +
> > + if (msg->flags & I2C_M_RD) {
> > + addr_data |= I2C_SMBUS_READ;
> > +
> > + /* SELECT command. */
> > + writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_SELECT,
> > + i2c->regs + MXS_I2C_CTRL0);
> > +
> > + ret = mxs_i2c_pio_wait_dmareq(i2c);
> > + if (ret)
> > + return ret;
> > +
> > + writel(addr_data, i2c->regs + MXS_I2C_DATA);
> > +
> > + ret = mxs_i2c_pio_wait_cplt(i2c);
> > + if (ret)
> > + return ret;
> > +
> > + /* READ command. */
> > + writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags |
> > + MXS_I2C_CTRL0_XFER_COUNT(msg->len),
> > + i2c->regs + MXS_I2C_CTRL0);
> > +
> > + for (i = 0; i < msg->len; i++) {
> > + if ((i & 3) == 0) {
> > + ret = mxs_i2c_pio_wait_dmareq(i2c);
> > + if (ret)
> > + return ret;
> > + data = readl(i2c->regs + MXS_I2C_DATA);
> > + }
> > + msg->buf[i] = data & 0xff;
> > + data >>= 8;
> > + }
> > + } else {
> > + addr_data |= I2C_SMBUS_WRITE;
> > +
> > + /* WRITE command. */
> > + writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE | flags |
> > + MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1),
> > + i2c->regs + MXS_I2C_CTRL0);
> > +
> > + /*
> > + * The LSB of data buffer is the first byte blasted across
> > + * the bus. Higher order bytes follow. Thus the following
> > + * filling schematic.
> > + */
>
> Ah, my old code again :) What was wrong with my comment?
You're right, I recycled this portion from somewhere, I think from the old PIOQ
code. I just shortened it.
> > + data = addr_data << 24;
> > + for (i = 0; i < msg->len; i++) {
> > + data >>= 8;
> > + data |= (msg->buf[i] << 24);
> > + if ((i & 3) == 2) {
> > + ret = mxs_i2c_pio_wait_dmareq(i2c);
> > + if (ret)
> > + return ret;
> > + writel(data, i2c->regs + MXS_I2C_DATA);
> > + }
> > + }
> > +
> > + shifts_left = 24 - (i & 3) * 8;
> > + if (shifts_left) {
> > + data >>= shifts_left;
> > + ret = mxs_i2c_pio_wait_dmareq(i2c);
> > + if (ret)
> > + return ret;
> > + writel(data, i2c->regs + MXS_I2C_DATA);
> > + }
> > + }
> > +
> > + ret = mxs_i2c_pio_wait_cplt(i2c);
> > + if (ret)
> > + return ret;
> > +
> > + /* Clear any dangling IRQs and re-enable interrupts. */
> > + writel(MXS_I2C_IRQ_MASK, i2c->regs + MXS_I2C_CTRL1_CLR);
> > + writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
> > +
> > + return 0;
> > +}
> > +
> >
> > /*
> >
> > * Low level master read/write transaction.
> > */
> >
> > @@ -316,24 +446,31 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter
> > *adap, struct i2c_msg *msg,
> >
> > if (msg->len == 0)
> >
> > return -EINVAL;
> >
> > - INIT_COMPLETION(i2c->cmd_complete);
> > - i2c->cmd_err = 0;
> > -
> > - ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
> > - if (ret)
> > - return ret;
> > + if (msg->len < 8) {
>
> Some THRESHOLD define would be nice here.
True, lemme roll out a V2.
> > + ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
> > + if (ret)
> > + mxs_i2c_reset(i2c);
> > + } else {
> > + i2c->cmd_err = 0;
> > + INIT_COMPLETION(i2c->cmd_complete);
> > + ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
> > + if (ret)
> > + return ret;
> >
> > - ret = wait_for_completion_timeout(&i2c->cmd_complete,
> > + ret = wait_for_completion_timeout(&i2c->cmd_complete,
> >
> > msecs_to_jiffies(1000));
> >
> > - if (ret == 0)
> > - goto timeout;
> > + if (ret == 0)
> > + goto timeout;
> > +
> > + if (i2c->cmd_err == -ENXIO)
> > + mxs_i2c_reset(i2c);
> >
> > - if (i2c->cmd_err == -ENXIO)
> > - mxs_i2c_reset(i2c);
> > + ret = i2c->cmd_err;
> > + }
> >
> > - dev_dbg(i2c->dev, "Done with err=%d\n", i2c->cmd_err);
> > + dev_dbg(i2c->dev, "Done with err=%d\n", ret);
> >
> > - return i2c->cmd_err;
> > + return ret;
> >
> > timeout:
> > dev_dbg(i2c->dev, "Timeout!\n");
>
> Regards,
>
> Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
