| Message ID | 1373312807-13227-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi, > Analyze and rework the PIO mode operation. The PIO mode operation > was unreliable on MX28, by analyzing the bus with LA, the checks > for when data were available or were to be sent were wrong. > > The PIO WRITE has to be completely reworked as it multiple problems. > The MX23 datasheet helped here, see comments in the code for details. > The problems boil down to: > - RUN bit in CTRL0 must be set after DATA register was written > - The PIO transfer must be 4 bytes long tops, otherwise use > clock stretching. > Both of these fixes are implemented. > > The PIO READ operation can only be done for up to four bytes as > we are unable to read out the data from the DATA register fast > enough. > > This patch also tries to document the investigation within the > code. Please test the patch, it'd be really nice to see how it behaves now. Note the I2C is still broken on MX23. For some reason, even the PIO doesn't work well with EEPROM. When writing EEPROM on MX23, I see it sends the first 3-byte WRITE command (address + 2 bytes of in-array address) , then the payload , but after both transfers, there is one more 0x00 byte sent , followed by NAK+STOP. No idea what that one final byte is. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marek, Your patch seems to be working fine on my platform and it is using PIO mode. However, I get the following warnings at compile time: drivers/i2c/busses/i2c-mxs.c:293:12: warning: 'mxs_i2c_pio_wait_dmareq' defined but not used [-Wunused-function] drivers/i2c/busses/i2c-mxs.c:320:12: warning: 'mxs_i2c_pio_wait_cplt' defined but not used [-Wunused-function] Regards, On 08/07/2013 21:46, Marek Vasut wrote: > Analyze and rework the PIO mode operation. The PIO mode operation > was unreliable on MX28, by analyzing the bus with LA, the checks > for when data were available or were to be sent were wrong. > > The PIO WRITE has to be completely reworked as it multiple problems. > The MX23 datasheet helped here, see comments in the code for details. > The problems boil down to: > - RUN bit in CTRL0 must be set after DATA register was written > - The PIO transfer must be 4 bytes long tops, otherwise use > clock stretching. > Both of these fixes are implemented. > > The PIO READ operation can only be done for up to four bytes as > we are unable to read out the data from the DATA register fast > enough. > > This patch also tries to document the investigation within the > code. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Fabio Estevam <r49496@freescale.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: <to-fleischer@t-online.de> > --- > drivers/i2c/busses/i2c-mxs.c | 202 ++++++++++++++++++++++++++++++------------ > 1 file changed, 147 insertions(+), 55 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c > index df8ff5a..c176aa8 100644 > --- a/drivers/i2c/busses/i2c-mxs.c > +++ b/drivers/i2c/busses/i2c-mxs.c > @@ -1,5 +1,5 @@ > /* > - * Freescale MXS I2C bus driver > + * Freescale MX28 I2C bus driver > * > * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K. > * > @@ -7,6 +7,8 @@ > * > * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved. > * > + * WARNING: This driver does NOT yet support i.MX23. > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > * the Free Software Foundation; either version 2 of the License, or > @@ -301,6 +303,19 @@ static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) > return 0; > } > > +static int mxs_i2c_pio_wait_xfer_end(struct mxs_i2c_dev *i2c) > +{ > + unsigned long timeout = jiffies + msecs_to_jiffies(1000); > + > + while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) { > + if (time_after(jiffies, timeout)) > + return -ETIMEDOUT; > + cond_resched(); > + } > + > + return 0; > +} > + > static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last) > { > unsigned long timeout = jiffies + msecs_to_jiffies(1000); > @@ -366,36 +381,76 @@ static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd) > writel(reg, i2c->regs + MXS_I2C_CTRL0); > } > > +/* > + * Start WRITE transaction on the I2C bus. By studying i.MX23 datasheet, > + * CTRL0::PIO_MODE bit description clarifies the order in which the registers > + * must be written during PIO mode operation. First, the CTRL0 register has > + * to be programmed with all the necessary bits but the RUN bit. Then the > + * payload has to be written into the DATA register. Finally, the transmission > + * is executed by setting the RUN bit in CTRL0. > + */ > +static void mxs_i2c_pio_trigger_write_cmd(struct mxs_i2c_dev *i2c, u32 cmd, u32 data) > +{ > + writel(cmd, i2c->regs + MXS_I2C_CTRL0); > + writel(data, i2c->regs + MXS_I2C_DATA); > + writel(MXS_I2C_CTRL0_RUN, i2c->regs + MXS_I2C_CTRL0_SET); > +} > + > +#include <linux/delay.h> > 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; > + int i, ret, xlen = 0; > + uint32_t start; > > /* Mute IRQs coming from this block. */ > writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR); > > + /* > + * MX23 idea: > + * - Enable CTRL0::PIO_MODE (1 << 24) > + * - Enable CTRL1::ACK_MODE (1 << 27) > + * > + * WARNING! The MX23 is broken in some way, even if it claims > + * to support PIO, when we try to transfer any amount of data > + * that is not aligned to 4 bytes, the DMA engine will have > + * bits in DEBUG1::DMA_BYTES_ENABLES still set even after the > + * transfer. This in turn will mess up the next transfer as > + * the block it emit one byte write onto the bus terminated > + * with a NAK+STOP. A possible workaround is to reset the IP > + * block after every PIO transmission, which might just work. > + * > + * NOTE: The CTRL0::PIO_MODE description is important, since > + * it outlines how the PIO mode is really supposed to work. > + */ > + > if (msg->flags & I2C_M_RD) { > + /* > + * PIO READ transfer: > + * > + * This transfer MUST be limited to 4 bytes maximum. It is not > + * possible to transfer more than four bytes via PIO, since we > + * can not in any way make sure we can read the data from the > + * DATA register fast enough. Besides, the RX FIFO is only four > + * bytes deep, thus we can only really read up to four bytes at > + * time. Finally, there is no bit indicating us that new data > + * arrived at the FIFO and can thus be fetched from the DATA > + * register. > + */ > addr_data |= I2C_SMBUS_READ; > > /* SELECT command. */ > - mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT); > - > - ret = mxs_i2c_pio_wait_dmareq(i2c); > - if (ret) > - return ret; > - > - writel(addr_data, i2c->regs + MXS_I2C_DATA); > - writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); > + mxs_i2c_pio_trigger_write_cmd(i2c, MXS_CMD_I2C_SELECT, addr_data); > > - ret = mxs_i2c_pio_wait_cplt(i2c, 0); > - if (ret) > - return ret; > - > - if (mxs_i2c_pio_check_error_state(i2c)) > + ret = mxs_i2c_pio_wait_xfer_end(i2c); > + if (ret) { > + dev_err(i2c->dev, > + "PIO: Failed to send SELECT command!\n"); > goto cleanup; > + } > > /* READ command. */ > mxs_i2c_pio_trigger_cmd(i2c, > @@ -404,68 +459,99 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, > > for (i = 0; i < msg->len; i++) { > if ((i & 3) == 0) { > - ret = mxs_i2c_pio_wait_dmareq(i2c); > - if (ret) > - return ret; > + /* > + * Wait a bit until the data arrive in the > + * DATA register so we can read them. > + */ > + mdelay(5); > data = readl(i2c->regs + MXS_I2C_DATA); > - writel(MXS_I2C_DEBUG0_DMAREQ, > - i2c->regs + MXS_I2C_DEBUG0_CLR); > } > msg->buf[i] = data & 0xff; > data >>= 8; > } > } else { > + /* > + * PIO WRITE transfer: > + * > + * The code below implements clock stretching to circumvent > + * the possibility of kernel not being able to supply data > + * fast enough. It is possible to transfer arbitrary amount > + * of data using PIO write. > + */ > addr_data |= I2C_SMBUS_WRITE; > > - /* WRITE command. */ > - mxs_i2c_pio_trigger_cmd(i2c, > - MXS_CMD_I2C_WRITE | flags | > - MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1)); > - > /* > * The LSB of data buffer is the first byte blasted across > * the bus. Higher order bytes follow. Thus the following > * filling schematic. > */ > + > data = addr_data << 24; > + > + /* Start the transfer with START condition. */ > + start = MXS_I2C_CTRL0_PRE_SEND_START; > + > + /* If the transfer is long, use clock stretching. */ > + if (msg->len > 3) > + start |= MXS_I2C_CTRL0_RETAIN_CLOCK; > + > 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); > - writel(MXS_I2C_DEBUG0_DMAREQ, > - i2c->regs + MXS_I2C_DEBUG0_CLR); > + > + if (i + 1 == msg->len) { > + /* This is the last transfer. */ > + start |= flags; > + start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK; > + xlen = (i + 2) % 4; > + data >>= (4 - xlen) * 8; > + } else if ((i & 3) == 2) { > + /* Regular transfer. */ > + xlen = 4; > + } else { > + /* Just stuff data. */ > + continue; > } > - } > > - 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); > + dev_dbg(i2c->dev, > + "PIO: len=%i pos=%i total=%i [W%s%s%s]\n", > + xlen, i, msg->len, > + start & MXS_I2C_CTRL0_PRE_SEND_START ? "S": "", > + start & MXS_I2C_CTRL0_POST_SEND_STOP ? "E": "", > + start & MXS_I2C_CTRL0_RETAIN_CLOCK ? "C": ""); > + > writel(MXS_I2C_DEBUG0_DMAREQ, > i2c->regs + MXS_I2C_DEBUG0_CLR); > + > + mxs_i2c_pio_trigger_write_cmd(i2c, > + start | MXS_I2C_CTRL0_MASTER_MODE | > + MXS_I2C_CTRL0_DIRECTION | > + MXS_I2C_CTRL0_XFER_COUNT(xlen), data); > + > + /* The START condition is sent only once. */ > + start &= ~MXS_I2C_CTRL0_PRE_SEND_START; > + > + /* Wait for the end of the transfer. */ > + ret = mxs_i2c_pio_wait_xfer_end(i2c); > + if (ret) { > + dev_err(i2c->dev, > + "PIO: Failed to finish WRITE cmd!\n"); > + break; > + } > + > + /* Check NAK here ? */ > } > } > > - ret = mxs_i2c_pio_wait_cplt(i2c, flags & MXS_I2C_CTRL0_POST_SEND_STOP); > - if (ret) > - return ret; > - > /* make sure we capture any occurred error into cmd_err */ > - mxs_i2c_pio_check_error_state(i2c); > + ret = mxs_i2c_pio_check_error_state(i2c); > > cleanup: > /* 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; > + return ret; > } > > /* > @@ -477,6 +563,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap); > int ret; > int flags; > + int use_pio = 0; > > flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0; > > @@ -487,15 +574,20 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > return -EINVAL; > > /* > - * The current boundary to select between PIO/DMA transfer method > - * is set to 8 bytes, transfers shorter than 8 bytes are transfered > - * using PIO mode while longer transfers use DMA. The 8 byte border is > - * based on this empirical measurement and a lot of previous frobbing. > + * The MX28 I2C IP block can only do PIO READ for transfer of to up > + * 4 bytes of length. The write transfer is not limited as it can use > + * clock stretching to avoid FIFO underruns. > */ > + if ((msg->flags & I2C_M_RD) && (msg->len <= 4)) > + use_pio = 1; > + if (!(msg->flags & I2C_M_RD) && (msg->len < 7)) > + use_pio = 1; > + > i2c->cmd_err = 0; > - if (msg->len < 8) { > + if (use_pio) { > ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); > - if (ret) > + /* No need to reset the block if NAK was received. */ > + if (ret && (ret != -ENXIO)) > mxs_i2c_reset(i2c); > } else { > INIT_COMPLETION(i2c->cmd_complete); > @@ -507,9 +599,11 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > msecs_to_jiffies(1000)); > if (ret == 0) > goto timeout; > + > + ret = i2c->cmd_err; > } > > - if (i2c->cmd_err == -ENXIO) { > + if (ret == -ENXIO) { > /* > * If the transfer fails with a NAK from the slave the > * controller halts until it gets told to return to idle state. > @@ -518,8 +612,6 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > i2c->regs + MXS_I2C_CTRL1_SET); > } > > - ret = i2c->cmd_err; > - > dev_dbg(i2c->dev, "Done with err=%d\n", ret); > > return ret;
Hi, > Analyze and rework the PIO mode operation. The PIO mode operation > was unreliable on MX28, by analyzing the bus with LA, the checks > for when data were available or were to be sent were wrong. > > The PIO WRITE has to be completely reworked as it multiple problems. > The MX23 datasheet helped here, see comments in the code for details. > The problems boil down to: > - RUN bit in CTRL0 must be set after DATA register was written > - The PIO transfer must be 4 bytes long tops, otherwise use > clock stretching. > Both of these fixes are implemented. > > The PIO READ operation can only be done for up to four bytes as > we are unable to read out the data from the DATA register fast > enough. > > This patch also tries to document the investigation within the > code. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Fabio Estevam <r49496@freescale.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: <to-fleischer@t-online.de> I so far got confirmation from ALexandre this patch works. Can someone else test it please? I'd like to roll out a final version to close this issue. One question remain though -- shall we have the PIO as an experimental feature and disable it by default OR not ? Thanks! Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marek, Am Montag, den 15.07.2013, 04:05 +0200 schrieb Marek Vasut: > Hi, > > > Analyze and rework the PIO mode operation. The PIO mode operation > > was unreliable on MX28, by analyzing the bus with LA, the checks > > for when data were available or were to be sent were wrong. > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > The MX23 datasheet helped here, see comments in the code for details. > > The problems boil down to: > > - RUN bit in CTRL0 must be set after DATA register was written > > - The PIO transfer must be 4 bytes long tops, otherwise use > > clock stretching. > > Both of these fixes are implemented. > > > > The PIO READ operation can only be done for up to four bytes as > > we are unable to read out the data from the DATA register fast > > enough. > > > > This patch also tries to document the investigation within the > > code. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Fabio Estevam <r49496@freescale.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Cc: <to-fleischer@t-online.de> > > I so far got confirmation from ALexandre this patch works. Can someone else test > it please? I'd like to roll out a final version to close this issue. > Sorry, I wasn't able to test it until now. I now have the respective hardware at my desk again, so I'll get you a report by latest day after tomorrow. I'll post some comments to the patch itself in the meantime. > One question remain though -- shall we have the PIO as an experimental feature > and disable it by default OR not ? > > Thanks! > > Best regards, > Marek Vasut
Hi Lucas, > Hi Marek, > > Am Montag, den 15.07.2013, 04:05 +0200 schrieb Marek Vasut: > > Hi, > > > > > Analyze and rework the PIO mode operation. The PIO mode operation > > > was unreliable on MX28, by analyzing the bus with LA, the checks > > > for when data were available or were to be sent were wrong. > > > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > > The MX23 datasheet helped here, see comments in the code for details. > > > The problems boil down to: > > > - RUN bit in CTRL0 must be set after DATA register was written > > > - The PIO transfer must be 4 bytes long tops, otherwise use > > > > > > clock stretching. > > > > > > Both of these fixes are implemented. > > > > > > The PIO READ operation can only be done for up to four bytes as > > > we are unable to read out the data from the DATA register fast > > > enough. > > > > > > This patch also tries to document the investigation within the > > > code. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > Cc: Fabio Estevam <r49496@freescale.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Shawn Guo <shawn.guo@linaro.org> > > > Cc: Wolfram Sang <wsa@the-dreams.de> > > > Cc: <to-fleischer@t-online.de> > > > > I so far got confirmation from ALexandre this patch works. Can someone > > else test it please? I'd like to roll out a final version to close this > > issue. > > Sorry, I wasn't able to test it until now. I now have the respective > hardware at my desk again, so I'll get you a report by latest day after > tomorrow. Thanks > I'll post some comments to the patch itself in the meantime. Consider the patch RFC, I know there are about two unused functions and maybe some compiler warnings. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
As promised some comments without a real test, yet. Am Montag, den 08.07.2013, 21:46 +0200 schrieb Marek Vasut: > Analyze and rework the PIO mode operation. The PIO mode operation > was unreliable on MX28, by analyzing the bus with LA, the checks > for when data were available or were to be sent were wrong. > > The PIO WRITE has to be completely reworked as it multiple problems. > The MX23 datasheet helped here, see comments in the code for details. > The problems boil down to: > - RUN bit in CTRL0 must be set after DATA register was written > - The PIO transfer must be 4 bytes long tops, otherwise use > clock stretching. > Both of these fixes are implemented. > > The PIO READ operation can only be done for up to four bytes as > we are unable to read out the data from the DATA register fast > enough. > > This patch also tries to document the investigation within the > code. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Fabio Estevam <r49496@freescale.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: <to-fleischer@t-online.de> > --- > drivers/i2c/busses/i2c-mxs.c | 202 ++++++++++++++++++++++++++++++------------ > 1 file changed, 147 insertions(+), 55 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c > index df8ff5a..c176aa8 100644 > --- a/drivers/i2c/busses/i2c-mxs.c > +++ b/drivers/i2c/busses/i2c-mxs.c > @@ -1,5 +1,5 @@ > /* > - * Freescale MXS I2C bus driver > + * Freescale MX28 I2C bus driver > * > * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K. > * > @@ -7,6 +7,8 @@ > * > * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved. > * > + * WARNING: This driver does NOT yet support i.MX23. > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > * the Free Software Foundation; either version 2 of the License, or > @@ -301,6 +303,19 @@ static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) > return 0; > } > The comment above is wrong. Only PIO mode doesn't work on MX23. We have customer hardware here at Pengutronix, which is working just fine with MX23 i2c when only using the DMA mode. If your patch doesn't fix MX23, please pick up Juergens fix to differentiate between the chip types in the driver and disable PIO accordingly. > +static int mxs_i2c_pio_wait_xfer_end(struct mxs_i2c_dev *i2c) > +{ > + unsigned long timeout = jiffies + msecs_to_jiffies(1000); > + > + while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) { > + if (time_after(jiffies, timeout)) > + return -ETIMEDOUT; > + cond_resched(); > + } > + > + return 0; > +} > + > static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last) > { > unsigned long timeout = jiffies + msecs_to_jiffies(1000); > @@ -366,36 +381,76 @@ static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd) > writel(reg, i2c->regs + MXS_I2C_CTRL0); > } > > +/* > + * Start WRITE transaction on the I2C bus. By studying i.MX23 datasheet, > + * CTRL0::PIO_MODE bit description clarifies the order in which the registers > + * must be written during PIO mode operation. First, the CTRL0 register has > + * to be programmed with all the necessary bits but the RUN bit. Then the > + * payload has to be written into the DATA register. Finally, the transmission > + * is executed by setting the RUN bit in CTRL0. > + */ > +static void mxs_i2c_pio_trigger_write_cmd(struct mxs_i2c_dev *i2c, u32 cmd, u32 data) > +{ > + writel(cmd, i2c->regs + MXS_I2C_CTRL0); > + writel(data, i2c->regs + MXS_I2C_DATA); > + writel(MXS_I2C_CTRL0_RUN, i2c->regs + MXS_I2C_CTRL0_SET); > +} > + > +#include <linux/delay.h> > 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; > + int i, ret, xlen = 0; > + uint32_t start; > > /* Mute IRQs coming from this block. */ > writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR); > > + /* > + * MX23 idea: > + * - Enable CTRL0::PIO_MODE (1 << 24) > + * - Enable CTRL1::ACK_MODE (1 << 27) > + * > + * WARNING! The MX23 is broken in some way, even if it claims > + * to support PIO, when we try to transfer any amount of data > + * that is not aligned to 4 bytes, the DMA engine will have > + * bits in DEBUG1::DMA_BYTES_ENABLES still set even after the > + * transfer. This in turn will mess up the next transfer as > + * the block it emit one byte write onto the bus terminated > + * with a NAK+STOP. A possible workaround is to reset the IP > + * block after every PIO transmission, which might just work. > + * > + * NOTE: The CTRL0::PIO_MODE description is important, since > + * it outlines how the PIO mode is really supposed to work. > + */ > + > if (msg->flags & I2C_M_RD) { > + /* > + * PIO READ transfer: > + * > + * This transfer MUST be limited to 4 bytes maximum. It is not > + * possible to transfer more than four bytes via PIO, since we > + * can not in any way make sure we can read the data from the > + * DATA register fast enough. Besides, the RX FIFO is only four > + * bytes deep, thus we can only really read up to four bytes at > + * time. Finally, there is no bit indicating us that new data > + * arrived at the FIFO and can thus be fetched from the DATA > + * register. > + */ > addr_data |= I2C_SMBUS_READ; > > /* SELECT command. */ > - mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT); > - > - ret = mxs_i2c_pio_wait_dmareq(i2c); > - if (ret) > - return ret; > - > - writel(addr_data, i2c->regs + MXS_I2C_DATA); > - writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); > + mxs_i2c_pio_trigger_write_cmd(i2c, MXS_CMD_I2C_SELECT, addr_data); > > - ret = mxs_i2c_pio_wait_cplt(i2c, 0); > - if (ret) > - return ret; > - > - if (mxs_i2c_pio_check_error_state(i2c)) > + ret = mxs_i2c_pio_wait_xfer_end(i2c); > + if (ret) { > + dev_err(i2c->dev, > + "PIO: Failed to send SELECT command!\n"); > goto cleanup; > + } > > /* READ command. */ > mxs_i2c_pio_trigger_cmd(i2c, > @@ -404,68 +459,99 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, > > for (i = 0; i < msg->len; i++) { > if ((i & 3) == 0) { > - ret = mxs_i2c_pio_wait_dmareq(i2c); > - if (ret) > - return ret; > + /* > + * Wait a bit until the data arrive in the > + * DATA register so we can read them. > + */ > + mdelay(5); This is more than a bit, 5msecs seems excessive. Aside from possibly nullifying any benefits from the PIO mode, hogging the CPU for this duration just to wait for data to arrive is crazy. If this is really needed AND you still see benefits from PIO mode, consider sleeping here. > data = readl(i2c->regs + MXS_I2C_DATA); > - writel(MXS_I2C_DEBUG0_DMAREQ, > - i2c->regs + MXS_I2C_DEBUG0_CLR); > } > msg->buf[i] = data & 0xff; > data >>= 8; > } > } else { > + /* > + * PIO WRITE transfer: > + * > + * The code below implements clock stretching to circumvent > + * the possibility of kernel not being able to supply data > + * fast enough. It is possible to transfer arbitrary amount > + * of data using PIO write. > + */ > addr_data |= I2C_SMBUS_WRITE; > > - /* WRITE command. */ > - mxs_i2c_pio_trigger_cmd(i2c, > - MXS_CMD_I2C_WRITE | flags | > - MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1)); > - > /* > * The LSB of data buffer is the first byte blasted across > * the bus. Higher order bytes follow. Thus the following > * filling schematic. > */ > + > data = addr_data << 24; > + > + /* Start the transfer with START condition. */ > + start = MXS_I2C_CTRL0_PRE_SEND_START; > + > + /* If the transfer is long, use clock stretching. */ > + if (msg->len > 3) > + start |= MXS_I2C_CTRL0_RETAIN_CLOCK; > + > 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); > - writel(MXS_I2C_DEBUG0_DMAREQ, > - i2c->regs + MXS_I2C_DEBUG0_CLR); > + > + if (i + 1 == msg->len) { > + /* This is the last transfer. */ > + start |= flags; > + start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK; > + xlen = (i + 2) % 4; > + data >>= (4 - xlen) * 8; > + } else if ((i & 3) == 2) { > + /* Regular transfer. */ > + xlen = 4; > + } else { > + /* Just stuff data. */ > + continue; > } > - } > > - 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); > + dev_dbg(i2c->dev, > + "PIO: len=%i pos=%i total=%i [W%s%s%s]\n", > + xlen, i, msg->len, > + start & MXS_I2C_CTRL0_PRE_SEND_START ? "S": "", > + start & MXS_I2C_CTRL0_POST_SEND_STOP ? "E": "", > + start & MXS_I2C_CTRL0_RETAIN_CLOCK ? "C": ""); > + > writel(MXS_I2C_DEBUG0_DMAREQ, > i2c->regs + MXS_I2C_DEBUG0_CLR); > + > + mxs_i2c_pio_trigger_write_cmd(i2c, > + start | MXS_I2C_CTRL0_MASTER_MODE | > + MXS_I2C_CTRL0_DIRECTION | > + MXS_I2C_CTRL0_XFER_COUNT(xlen), data); > + > + /* The START condition is sent only once. */ > + start &= ~MXS_I2C_CTRL0_PRE_SEND_START; > + > + /* Wait for the end of the transfer. */ > + ret = mxs_i2c_pio_wait_xfer_end(i2c); > + if (ret) { > + dev_err(i2c->dev, > + "PIO: Failed to finish WRITE cmd!\n"); > + break; > + } > + > + /* Check NAK here ? */ > } > } > > - ret = mxs_i2c_pio_wait_cplt(i2c, flags & MXS_I2C_CTRL0_POST_SEND_STOP); > - if (ret) > - return ret; > - > /* make sure we capture any occurred error into cmd_err */ > - mxs_i2c_pio_check_error_state(i2c); > + ret = mxs_i2c_pio_check_error_state(i2c); > > cleanup: > /* 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; > + return ret; > } > > /* > @@ -477,6 +563,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap); > int ret; > int flags; > + int use_pio = 0; > > flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0; > > @@ -487,15 +574,20 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > return -EINVAL; > > /* > - * The current boundary to select between PIO/DMA transfer method > - * is set to 8 bytes, transfers shorter than 8 bytes are transfered > - * using PIO mode while longer transfers use DMA. The 8 byte border is > - * based on this empirical measurement and a lot of previous frobbing. > + * The MX28 I2C IP block can only do PIO READ for transfer of to up > + * 4 bytes of length. The write transfer is not limited as it can use > + * clock stretching to avoid FIFO underruns. > */ > + if ((msg->flags & I2C_M_RD) && (msg->len <= 4)) > + use_pio = 1; > + if (!(msg->flags & I2C_M_RD) && (msg->len < 7)) > + use_pio = 1; > + Why do you need to split this out? I think the statement is reasonable sized to go into the if(), especially with the comment above it. No need to use a local variable + you don't loose lazy evaluation. > i2c->cmd_err = 0; > - if (msg->len < 8) { > + if (use_pio) { > ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); > - if (ret) > + /* No need to reset the block if NAK was received. */ > + if (ret && (ret != -ENXIO)) > mxs_i2c_reset(i2c); > } else { > INIT_COMPLETION(i2c->cmd_complete); > @@ -507,9 +599,11 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > msecs_to_jiffies(1000)); > if (ret == 0) > goto timeout; > + > + ret = i2c->cmd_err; All those error code swaps between ret and cmd_err are really confusing. Perhaps we should record the error in _one_ of both only and then assign one to the other at the very end of this function. > } > > - if (i2c->cmd_err == -ENXIO) { > + if (ret == -ENXIO) { > /* > * If the transfer fails with a NAK from the slave the > * controller halts until it gets told to return to idle state. > @@ -518,8 +612,6 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, > i2c->regs + MXS_I2C_CTRL1_SET); > } > > - ret = i2c->cmd_err; > - > dev_dbg(i2c->dev, "Done with err=%d\n", ret); > > return ret;
On Mon, Jul 15, 2013 at 04:05:52AM +0200, Marek Vasut wrote: > I so far got confirmation from ALexandre this patch works. Can someone else test > it please? I'd like to roll out a final version to close this issue. I just confirmed that it fixes sgtl5000 register write failure for me. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lucas, > As promised some comments without a real test, yet. > > Am Montag, den 08.07.2013, 21:46 +0200 schrieb Marek Vasut: > > Analyze and rework the PIO mode operation. The PIO mode operation > > was unreliable on MX28, by analyzing the bus with LA, the checks > > for when data were available or were to be sent were wrong. > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > The MX23 datasheet helped here, see comments in the code for details. > > The problems boil down to: > > - RUN bit in CTRL0 must be set after DATA register was written > > - The PIO transfer must be 4 bytes long tops, otherwise use > > > > clock stretching. > > > > Both of these fixes are implemented. > > > > The PIO READ operation can only be done for up to four bytes as > > we are unable to read out the data from the DATA register fast > > enough. > > > > This patch also tries to document the investigation within the > > code. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Fabio Estevam <r49496@freescale.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Cc: <to-fleischer@t-online.de> > > --- > > > > drivers/i2c/busses/i2c-mxs.c | 202 > > ++++++++++++++++++++++++++++++------------ 1 file changed, 147 > > insertions(+), 55 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c > > index df8ff5a..c176aa8 100644 > > --- a/drivers/i2c/busses/i2c-mxs.c > > +++ b/drivers/i2c/busses/i2c-mxs.c > > @@ -1,5 +1,5 @@ > > > > /* > > > > - * Freescale MXS I2C bus driver > > + * Freescale MX28 I2C bus driver > > > > * > > * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K. > > * > > > > @@ -7,6 +7,8 @@ > > > > * > > * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights > > Reserved. * > > > > + * WARNING: This driver does NOT yet support i.MX23. > > + * > > > > * This program is free software; you can redistribute it and/or modify > > * it under the terms of the GNU General Public License as published by > > * the Free Software Foundation; either version 2 of the License, or > > > > @@ -301,6 +303,19 @@ static int mxs_i2c_pio_wait_dmareq(struct > > mxs_i2c_dev *i2c) > > > > return 0; > > > > } > > The comment above is wrong. Only PIO mode doesn't work on MX23. We have > customer hardware here at Pengutronix, which is working just fine with > MX23 i2c when only using the DMA mode. Check the bus with an LA, I think I saw some trouble on MX23EVK. It'd be nice to verify if what you see on the bus is really clean. > If your patch doesn't fix MX23, > please pick up Juergens fix to differentiate between the chip types in > the driver and disable PIO accordingly. I suspect we will need Jurgens' patch anyway, since the PIO is either FUBAR'd on MX23 or I do not know yet how to operate it. On the other hand, the MX23 manual does mention some PIO operation (the PIO bit in CTRL register). [...] > > @@ -404,68 +459,99 @@ static int mxs_i2c_pio_setup_xfer(struct > > i2c_adapter *adap, > > > > for (i = 0; i < msg->len; i++) { > > > > if ((i & 3) == 0) { > > > > - ret = mxs_i2c_pio_wait_dmareq(i2c); > > - if (ret) > > - return ret; > > + /* > > + * Wait a bit until the data arrive in the > > + * DATA register so we can read them. > > + */ > > + mdelay(5); > > This is more than a bit, 5msecs seems excessive. Aside from possibly > nullifying any benefits from the PIO mode, hogging the CPU for this > duration just to wait for data to arrive is crazy. If this is really > needed AND you still see benefits from PIO mode, consider sleeping here. This needs to be fixed. I just didn't find a bit that the I2C block asserts to signalize it has all the data. I have an idea, but I'll need to verify my idea. [...] > > - * The current boundary to select between PIO/DMA transfer method > > - * is set to 8 bytes, transfers shorter than 8 bytes are transfered > > - * using PIO mode while longer transfers use DMA. The 8 byte border is > > - * based on this empirical measurement and a lot of previous frobbing. > > + * The MX28 I2C IP block can only do PIO READ for transfer of to up > > + * 4 bytes of length. The write transfer is not limited as it can use > > + * clock stretching to avoid FIFO underruns. > > > > */ > > > > + if ((msg->flags & I2C_M_RD) && (msg->len <= 4)) > > + use_pio = 1; > > + if (!(msg->flags & I2C_M_RD) && (msg->len < 7)) > > + use_pio = 1; > > + > > Why do you need to split this out? I think the statement is reasonable > sized to go into the if(), especially with the comment above it. No need > to use a local variable + you don't loose lazy evaluation. The compiler does optimize this anyway, or do you see it does not? Anyway, you might be right about the condition being simple enough for an ugly two-liner. > > i2c->cmd_err = 0; > > > > - if (msg->len < 8) { > > + if (use_pio) { > > > > ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); > > > > - if (ret) > > + /* No need to reset the block if NAK was received. */ > > + if (ret && (ret != -ENXIO)) > > > > mxs_i2c_reset(i2c); > > > > } else { > > > > INIT_COMPLETION(i2c->cmd_complete); > > > > @@ -507,9 +599,11 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter > > *adap, struct i2c_msg *msg, > > > > msecs_to_jiffies(1000)); > > > > if (ret == 0) > > > > goto timeout; > > > > + > > + ret = i2c->cmd_err; > > All those error code swaps between ret and cmd_err are really confusing. > Perhaps we should record the error in _one_ of both only and then assign > one to the other at the very end of this function. The error reporting is horrible, it needs fixing. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Shawn Guo, > On Mon, Jul 15, 2013 at 04:05:52AM +0200, Marek Vasut wrote: > > I so far got confirmation from ALexandre this patch works. Can someone > > else test it please? I'd like to roll out a final version to close this > > issue. > > I just confirmed that it fixes sgtl5000 register write failure for me. Thanks, I'll roll out the V2 this week. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > > > Analyze and rework the PIO mode operation. The PIO mode operation > > was unreliable on MX28, by analyzing the bus with LA, the checks for > > when data were available or were to be sent were wrong. > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > The MX23 datasheet helped here, see comments in the code for > > details. > > The problems boil down to: > > - RUN bit in CTRL0 must be set after DATA register was written - The > > PIO transfer must be 4 bytes long tops, otherwise use > > clock stretching. > > Both of these fixes are implemented. > > > > The PIO READ operation can only be done for up to four bytes as we > > are unable to read out the data from the DATA register fast enough. > > > > This patch also tries to document the investigation within the code. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Fabio Estevam <r49496@freescale.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Cc: <to-fleischer@t-online.de> > > > > I so far got confirmation from ALexandre this patch works. Can someone > else test it please? I'd like to roll out a final version to close > this issue. > sorry, I couldn't test the patch until today on my iMX283 board. It works when reading the registers from the temperature sensor TCN75A. But it fails when writing less than 7 bytes to the EEPROM (24LC32). Then I always get a 'Connection timed out' error in the user space. Best regards Torsten Fleischer -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Torsten, > Hi, > > > > Analyze and rework the PIO mode operation. The PIO mode operation > > > was unreliable on MX28, by analyzing the bus with LA, the checks for > > > when data were available or were to be sent were wrong. > > > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > > The MX23 datasheet helped here, see comments in the code for > > > details. > > > The problems boil down to: > > > - RUN bit in CTRL0 must be set after DATA register was written - The > > > PIO transfer must be 4 bytes long tops, otherwise use > > > clock stretching. > > > Both of these fixes are implemented. > > > > > > The PIO READ operation can only be done for up to four bytes as we > > > are unable to read out the data from the DATA register fast enough. > > > > > > This patch also tries to document the investigation within the code. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > Cc: Fabio Estevam <r49496@freescale.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Shawn Guo <shawn.guo@linaro.org> > > > Cc: Wolfram Sang <wsa@the-dreams.de> > > > Cc: <to-fleischer@t-online.de> > > > > I so far got confirmation from ALexandre this patch works. Can someone > > else test it please? I'd like to roll out a final version to close > > this issue. > > sorry, I couldn't test the patch until today on my iMX283 board. No problem, I was busy too. > It works when reading the registers from the temperature sensor TCN75A. OK > But it fails when writing less than 7 bytes to the EEPROM (24LC32). Then > I always get a 'Connection timed out' error in the user space. This works just fine for me, even the LA shows no weird stuff on the I2C, would you be able to trace where this comes from please? > Best regards > Torsten Fleischer Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear to-fleischer@t-online.de, > Hi, > > > > Analyze and rework the PIO mode operation. The PIO mode operation > > > was unreliable on MX28, by analyzing the bus with LA, the checks for > > > when data were available or were to be sent were wrong. > > > > > > The PIO WRITE has to be completely reworked as it multiple problems. > > > The MX23 datasheet helped here, see comments in the code for > > > details. > > > The problems boil down to: > > > - RUN bit in CTRL0 must be set after DATA register was written - The > > > PIO transfer must be 4 bytes long tops, otherwise use > > > clock stretching. > > > Both of these fixes are implemented. > > > > > > The PIO READ operation can only be done for up to four bytes as we > > > are unable to read out the data from the DATA register fast enough. > > > > > > This patch also tries to document the investigation within the code. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > Cc: Fabio Estevam <r49496@freescale.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Shawn Guo <shawn.guo@linaro.org> > > > Cc: Wolfram Sang <wsa@the-dreams.de> > > > Cc: <to-fleischer@t-online.de> > > > > I so far got confirmation from ALexandre this patch works. Can someone > > else test it please? I'd like to roll out a final version to close > > this issue. > > sorry, I couldn't test the patch until today on my iMX283 board. > It works when reading the registers from the temperature sensor TCN75A. > But it fails when writing less than 7 bytes to the EEPROM (24LC32). Then > I always get a 'Connection timed out' error in the user space. Thinking about this, did you correctly configure the EEPROM write sector size? I think it's 32 bytes for this device. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marek, > > Thinking about this, did you correctly configure the EEPROM write sector > size? I think it's 32 bytes for this device. > The EEPROM is correctly configured, i.e. page size is set to 32. I made a test by writing one byte (0x01) to the address 0x0100 of the EEPROM. The EEPROM's I2C bus address is 0x54. The test has only one transfer with a message length of 3 bytes. I figured out that the value written to the MXS_I2C_DATA register should be 0x010001a8. But instead of that a value of 0x00000000 is written to the register. I think that the reason for that can be found in the following code section: if (i + 1 == msg->len) { /* This is the last transfer. */ start |= flags; start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK; xlen = (i + 2) % 4; data >>= (4 - xlen) * 8; } else if ((i & 3) == 2) { /* Regular transfer. */ xlen = 4; } else { /* Just stuff data. */ continue; } After the last byte is stuffed into the variable data (i = 2) xlen is calculated (with xlen = (i + 2) % 4) to zero. Thus the following shift operation (data >>= (4 - xlen) * 8) shifts all the bits out of data that already contains 0x010001a8. Best regards Torsten Fleischer -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index df8ff5a..c176aa8 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -1,5 +1,5 @@ /* - * Freescale MXS I2C bus driver + * Freescale MX28 I2C bus driver * * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K. * @@ -7,6 +7,8 @@ * * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved. * + * WARNING: This driver does NOT yet support i.MX23. + * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or @@ -301,6 +303,19 @@ static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) return 0; } +static int mxs_i2c_pio_wait_xfer_end(struct mxs_i2c_dev *i2c) +{ + unsigned long timeout = jiffies + msecs_to_jiffies(1000); + + while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) { + if (time_after(jiffies, timeout)) + return -ETIMEDOUT; + cond_resched(); + } + + return 0; +} + static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last) { unsigned long timeout = jiffies + msecs_to_jiffies(1000); @@ -366,36 +381,76 @@ static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd) writel(reg, i2c->regs + MXS_I2C_CTRL0); } +/* + * Start WRITE transaction on the I2C bus. By studying i.MX23 datasheet, + * CTRL0::PIO_MODE bit description clarifies the order in which the registers + * must be written during PIO mode operation. First, the CTRL0 register has + * to be programmed with all the necessary bits but the RUN bit. Then the + * payload has to be written into the DATA register. Finally, the transmission + * is executed by setting the RUN bit in CTRL0. + */ +static void mxs_i2c_pio_trigger_write_cmd(struct mxs_i2c_dev *i2c, u32 cmd, u32 data) +{ + writel(cmd, i2c->regs + MXS_I2C_CTRL0); + writel(data, i2c->regs + MXS_I2C_DATA); + writel(MXS_I2C_CTRL0_RUN, i2c->regs + MXS_I2C_CTRL0_SET); +} + +#include <linux/delay.h> 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; + int i, ret, xlen = 0; + uint32_t start; /* Mute IRQs coming from this block. */ writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR); + /* + * MX23 idea: + * - Enable CTRL0::PIO_MODE (1 << 24) + * - Enable CTRL1::ACK_MODE (1 << 27) + * + * WARNING! The MX23 is broken in some way, even if it claims + * to support PIO, when we try to transfer any amount of data + * that is not aligned to 4 bytes, the DMA engine will have + * bits in DEBUG1::DMA_BYTES_ENABLES still set even after the + * transfer. This in turn will mess up the next transfer as + * the block it emit one byte write onto the bus terminated + * with a NAK+STOP. A possible workaround is to reset the IP + * block after every PIO transmission, which might just work. + * + * NOTE: The CTRL0::PIO_MODE description is important, since + * it outlines how the PIO mode is really supposed to work. + */ + if (msg->flags & I2C_M_RD) { + /* + * PIO READ transfer: + * + * This transfer MUST be limited to 4 bytes maximum. It is not + * possible to transfer more than four bytes via PIO, since we + * can not in any way make sure we can read the data from the + * DATA register fast enough. Besides, the RX FIFO is only four + * bytes deep, thus we can only really read up to four bytes at + * time. Finally, there is no bit indicating us that new data + * arrived at the FIFO and can thus be fetched from the DATA + * register. + */ addr_data |= I2C_SMBUS_READ; /* SELECT command. */ - mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT); - - ret = mxs_i2c_pio_wait_dmareq(i2c); - if (ret) - return ret; - - writel(addr_data, i2c->regs + MXS_I2C_DATA); - writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); + mxs_i2c_pio_trigger_write_cmd(i2c, MXS_CMD_I2C_SELECT, addr_data); - ret = mxs_i2c_pio_wait_cplt(i2c, 0); - if (ret) - return ret; - - if (mxs_i2c_pio_check_error_state(i2c)) + ret = mxs_i2c_pio_wait_xfer_end(i2c); + if (ret) { + dev_err(i2c->dev, + "PIO: Failed to send SELECT command!\n"); goto cleanup; + } /* READ command. */ mxs_i2c_pio_trigger_cmd(i2c, @@ -404,68 +459,99 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, for (i = 0; i < msg->len; i++) { if ((i & 3) == 0) { - ret = mxs_i2c_pio_wait_dmareq(i2c); - if (ret) - return ret; + /* + * Wait a bit until the data arrive in the + * DATA register so we can read them. + */ + mdelay(5); data = readl(i2c->regs + MXS_I2C_DATA); - writel(MXS_I2C_DEBUG0_DMAREQ, - i2c->regs + MXS_I2C_DEBUG0_CLR); } msg->buf[i] = data & 0xff; data >>= 8; } } else { + /* + * PIO WRITE transfer: + * + * The code below implements clock stretching to circumvent + * the possibility of kernel not being able to supply data + * fast enough. It is possible to transfer arbitrary amount + * of data using PIO write. + */ addr_data |= I2C_SMBUS_WRITE; - /* WRITE command. */ - mxs_i2c_pio_trigger_cmd(i2c, - MXS_CMD_I2C_WRITE | flags | - MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1)); - /* * The LSB of data buffer is the first byte blasted across * the bus. Higher order bytes follow. Thus the following * filling schematic. */ + data = addr_data << 24; + + /* Start the transfer with START condition. */ + start = MXS_I2C_CTRL0_PRE_SEND_START; + + /* If the transfer is long, use clock stretching. */ + if (msg->len > 3) + start |= MXS_I2C_CTRL0_RETAIN_CLOCK; + 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); - writel(MXS_I2C_DEBUG0_DMAREQ, - i2c->regs + MXS_I2C_DEBUG0_CLR); + + if (i + 1 == msg->len) { + /* This is the last transfer. */ + start |= flags; + start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK; + xlen = (i + 2) % 4; + data >>= (4 - xlen) * 8; + } else if ((i & 3) == 2) { + /* Regular transfer. */ + xlen = 4; + } else { + /* Just stuff data. */ + continue; } - } - 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); + dev_dbg(i2c->dev, + "PIO: len=%i pos=%i total=%i [W%s%s%s]\n", + xlen, i, msg->len, + start & MXS_I2C_CTRL0_PRE_SEND_START ? "S": "", + start & MXS_I2C_CTRL0_POST_SEND_STOP ? "E": "", + start & MXS_I2C_CTRL0_RETAIN_CLOCK ? "C": ""); + writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR); + + mxs_i2c_pio_trigger_write_cmd(i2c, + start | MXS_I2C_CTRL0_MASTER_MODE | + MXS_I2C_CTRL0_DIRECTION | + MXS_I2C_CTRL0_XFER_COUNT(xlen), data); + + /* The START condition is sent only once. */ + start &= ~MXS_I2C_CTRL0_PRE_SEND_START; + + /* Wait for the end of the transfer. */ + ret = mxs_i2c_pio_wait_xfer_end(i2c); + if (ret) { + dev_err(i2c->dev, + "PIO: Failed to finish WRITE cmd!\n"); + break; + } + + /* Check NAK here ? */ } } - ret = mxs_i2c_pio_wait_cplt(i2c, flags & MXS_I2C_CTRL0_POST_SEND_STOP); - if (ret) - return ret; - /* make sure we capture any occurred error into cmd_err */ - mxs_i2c_pio_check_error_state(i2c); + ret = mxs_i2c_pio_check_error_state(i2c); cleanup: /* 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; + return ret; } /* @@ -477,6 +563,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap); int ret; int flags; + int use_pio = 0; flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0; @@ -487,15 +574,20 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, return -EINVAL; /* - * The current boundary to select between PIO/DMA transfer method - * is set to 8 bytes, transfers shorter than 8 bytes are transfered - * using PIO mode while longer transfers use DMA. The 8 byte border is - * based on this empirical measurement and a lot of previous frobbing. + * The MX28 I2C IP block can only do PIO READ for transfer of to up + * 4 bytes of length. The write transfer is not limited as it can use + * clock stretching to avoid FIFO underruns. */ + if ((msg->flags & I2C_M_RD) && (msg->len <= 4)) + use_pio = 1; + if (!(msg->flags & I2C_M_RD) && (msg->len < 7)) + use_pio = 1; + i2c->cmd_err = 0; - if (msg->len < 8) { + if (use_pio) { ret = mxs_i2c_pio_setup_xfer(adap, msg, flags); - if (ret) + /* No need to reset the block if NAK was received. */ + if (ret && (ret != -ENXIO)) mxs_i2c_reset(i2c); } else { INIT_COMPLETION(i2c->cmd_complete); @@ -507,9 +599,11 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, msecs_to_jiffies(1000)); if (ret == 0) goto timeout; + + ret = i2c->cmd_err; } - if (i2c->cmd_err == -ENXIO) { + if (ret == -ENXIO) { /* * If the transfer fails with a NAK from the slave the * controller halts until it gets told to return to idle state. @@ -518,8 +612,6 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, i2c->regs + MXS_I2C_CTRL1_SET); } - ret = i2c->cmd_err; - dev_dbg(i2c->dev, "Done with err=%d\n", ret); return ret;
Analyze and rework the PIO mode operation. The PIO mode operation was unreliable on MX28, by analyzing the bus with LA, the checks for when data were available or were to be sent were wrong. The PIO WRITE has to be completely reworked as it multiple problems. The MX23 datasheet helped here, see comments in the code for details. The problems boil down to: - RUN bit in CTRL0 must be set after DATA register was written - The PIO transfer must be 4 bytes long tops, otherwise use clock stretching. Both of these fixes are implemented. The PIO READ operation can only be done for up to four bytes as we are unable to read out the data from the DATA register fast enough. This patch also tries to document the investigation within the code. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> Cc: Fabio Estevam <r49496@freescale.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: <to-fleischer@t-online.de> --- drivers/i2c/busses/i2c-mxs.c | 202 ++++++++++++++++++++++++++++++------------ 1 file changed, 147 insertions(+), 55 deletions(-)