| Message ID | 1375219237-9594-2-git-send-email-marex@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Jul 30, 2013 at 11:20:36PM +0200, 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> Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl have used these patches. Don't be shy :) > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Christoph Baumann <cb@sgoc.de> > Cc: Fabio Estevam <r49496@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Torsten Fleischer <to-fleischer@t-online.de> > Cc: Wolfram Sang <wsa@the-dreams.de> > --- > drivers/i2c/busses/i2c-mxs.c | 263 ++++++++++++++++++++++++++---------------- > 1 file changed, 166 insertions(+), 97 deletions(-) > > V2: - Fix the data register shift computation algorithm and document that > some more. The original algo failed for short 1-byte writes as seen > in http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg12812.html > - Add me a copyright/authorship line, since according to git blame, I > have quite a lot of authored lines in the driver. Wolfram, I brought > this up some time ago already, but finally got to it. You're OK with > this, right? Sure. One question: Wouldn't it be more logical to have this patch first (fix pio) and then squash patches 1 and 3 as one on the top (add mx23 to now working pio)? I am not pushing too hard if this means a lot of work, but it sounds a bit more logical to me. > 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. > + */ > + BUG_ON(msg->len > 4); How could this happen? There is a check in mxs_i2c_xfer_msg.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 05/08/2013 12:36, Wolfram Sang wrote: > On Tue, Jul 30, 2013 at 11:20:36PM +0200, 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> > > Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl > have used these patches. Don't be shy :) > > It was on my todo list until now. Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> - -- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBCAAGBQJR/4hNAAoJEKbNnwlvZCyzv0gP/1/ED0u/HwbJFU6mxi3ALpLB NJ/RYMlmU+u+4FlNzlAEYBPSIp/2IFhgmTLnf+t1E3VsvjZ0mchYZwp/3IH/Z4X5 HAx5X0/mJ19Nqlq7ARuQvKaF+i7IQxOY61GybxX8W7gvHOL2FXufZFdVh+KjRFDY rOnxgugCF473iZ7YUA5p+qJo9H/uJKaUcHQO56yUJkA3BOM4MVWztMUI0wuICg9H Khq4zKuL6/qXsL1fz3sT5L1pUUYUZFUt75pz1zIYvovEOg+Bk3kSHz2xRQPYHXJ2 UzTDANNnaIEkh9ZzuNoVywOb3Qme9QLufD/ju29QYmOZvVBuTo9oSgdxuZGZ8Wxl u6y02wp3y1zWXhHc2nvOAyWOWYiYvIJrn7dkvMhLmiJbc941gyJdnOgutsKZlBnu atPOVt1muj0sz0wTHe9UvpSqQf18T8kKdtjh+VyI8px5OWfjc7FmJVOPHTUwZ/UV obzNskq/MAFoib/hd6hU3raDpRDQYBmgVet1HHyhWHObn2SC8hRLTwIoZ5YHo+pH SZ62nGJmeVm1Bfk10jUrmWQ7NEMiDEJR0wnGiCmZm3x/fwXe4Gu8eiDKZxYeLCJO zytXpNq3yW+2KuxMvwfsiVJ3kHx6GGHWVLtSLuCV1Nqmw4bpXdOuGGPdLLjgu7Nx w8pZ+BZpzDDmXtZGPXNk =3+JD -----END PGP SIGNATURE----- -- 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 Wolfram Sang, > On Tue, Jul 30, 2013 at 11:20:36PM +0200, 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> > > Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl > have used these patches. Don't be shy :) > > > Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Christoph Baumann <cb@sgoc.de> > > Cc: Fabio Estevam <r49496@freescale.com> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Torsten Fleischer <to-fleischer@t-online.de> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > --- > > > > drivers/i2c/busses/i2c-mxs.c | 263 > > ++++++++++++++++++++++++++---------------- 1 file changed, 166 > > insertions(+), 97 deletions(-) > > > > V2: - Fix the data register shift computation algorithm and document that > > > > some more. The original algo failed for short 1-byte writes as seen > > in > > http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg12812.htm > > l > > > > - Add me a copyright/authorship line, since according to git blame, I > > > > have quite a lot of authored lines in the driver. Wolfram, I > > brought this up some time ago already, but finally got to it. > > You're OK with this, right? > > Sure. > > One question: Wouldn't it be more logical to have this patch first (fix > pio) and then squash patches 1 and 3 as one on the top (add mx23 to now > working pio)? I am not pushing too hard if this means a lot of work, but > it sounds a bit more logical to me. I would prefer to keep Juergens' patch separate from mine if you don't mind to be clear on the authorship. > > 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. > > + */ > > + BUG_ON(msg->len > 4); > > How could this happen? There is a check in mxs_i2c_xfer_msg. It cannot happen, I added the check here to make sure when someone becomes adventurous enough to start messing with these constants, it will stop him early enough. 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
On Mon, Aug 05, 2013 at 12:36:47PM +0200, Wolfram Sang wrote: > Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl > have used these patches. Don't be shy :) Tested-by: Shawn Guo <shawn.guo@linaro.org> -- 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
> > One question: Wouldn't it be more logical to have this patch first (fix > > pio) and then squash patches 1 and 3 as one on the top (add mx23 to now > > working pio)? I am not pushing too hard if this means a lot of work, but > > it sounds a bit more logical to me. > > I would prefer to keep Juergens' patch separate from mine if you don't mind to > be clear on the authorship. If you add both SoB, everything should be fine. We often work on someone else's patches, no? > > > 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. > > > + */ > > > + BUG_ON(msg->len > 4); > > > > How could this happen? There is a check in mxs_i2c_xfer_msg. > > It cannot happen, I added the check here to make sure when someone becomes > adventurous enough to start messing with these constants, it will stop him early > enough. Then, add the comment to the check so ppl will notice there? I like to keep BUG_ON sparse, since it is hard to tell if the occasion is really worth stopping the machine.
Dear Wolfram Sang, > > > One question: Wouldn't it be more logical to have this patch first (fix > > > pio) and then squash patches 1 and 3 as one on the top (add mx23 to now > > > working pio)? I am not pushing too hard if this means a lot of work, > > > but it sounds a bit more logical to me. > > > > I would prefer to keep Juergens' patch separate from mine if you don't > > mind to be clear on the authorship. > > If you add both SoB, everything should be fine. We often work on someone > else's patches, no? I agree, but I still don't like squashing the two patches together. I forgot to mention it last time, but please, look at the patches one more time. Jurgens' does the DT change and mine fixes the PIO on MX23, I'd like to keep these changes separate. > > > > 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. > > > > + */ > > > > + BUG_ON(msg->len > 4); > > > > > > How could this happen? There is a check in mxs_i2c_xfer_msg. > > > > It cannot happen, I added the check here to make sure when someone > > becomes adventurous enough to start messing with these constants, it > > will stop him early enough. > > Then, add the comment to the check so ppl will notice there? If I could add a big red flashing sign into the code stating "If you go over 4 bytes here, a kitten will die", then by all means, I would add it. Unfortunatelly, there is no such thing possible. > I like to > keep BUG_ON sparse, since it is hard to tell if the occasion is really > worth stopping the machine. While I agree with you on this, I would also like for the kernel to check if someone accidentally screwed up and thoroughly warn him. We cannot continue anyway, because if more than 4 bytes are requested, the controller would simply not give us more than the last 4 bytes and this would result in an undefined behavior and data corruption during reception. We simply do not want that. Maybe WARN_ONCE and return with error might just work? 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
> I agree, but I still don't like squashing the two patches together. I forgot to > mention it last time, but please, look at the patches one more time. Jurgens' > does the DT change and mine fixes the PIO on MX23, I'd like to keep these > changes separate. Ok, as said, I am not pushing hard on this. > > Then, add the comment to the check so ppl will notice there? > > If I could add a big red flashing sign into the code stating "If you go over 4 > bytes here, a kitten will die", then by all means, I would add it. > Unfortunatelly, there is no such thing possible. I'd say if somebody changes the code with that comment on top of it, it is really deserved... > Maybe WARN_ONCE and return with error might just work? ... but if you insist, I can accept this. Thanks, Wolfram
Dear Wolfram Sang, > > I agree, but I still don't like squashing the two patches together. I > > forgot to mention it last time, but please, look at the patches one more > > time. Jurgens' does the DT change and mine fixes the PIO on MX23, I'd > > like to keep these changes separate. > > Ok, as said, I am not pushing hard on this. Thanks! > > > Then, add the comment to the check so ppl will notice there? > > > > If I could add a big red flashing sign into the code stating "If you go > > over 4 bytes here, a kitten will die", then by all means, I would add > > it. Unfortunatelly, there is no such thing possible. > > I'd say if somebody changes the code with that comment on top of it, it > is really deserved... You mean the kitten ... ? ;-) > > Maybe WARN_ONCE and return with error might just work? > > ... but if you insist, I can accept this. Thinking about it harder, WARN might be more appropriate. Maybe I'll just remove it altogether, I'll think about it some more. 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
> Thinking about it harder, WARN might be more appropriate. Maybe I'll just remove > it altogether, I'll think about it some more. And when resending, please rebase on top of "i2c: i2c-mxs: Use DMA mode even for small transfers" which I pushed out a few minutes ago. Thanks.
Dear Wolfram Sang, > > Thinking about it harder, WARN might be more appropriate. Maybe I'll just > > remove it altogether, I'll think about it some more. > > And when resending, please rebase on top of "i2c: i2c-mxs: Use DMA > mode even for small transfers" which I pushed out a few minutes ago. OK, thanks for the heads up. 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, > + > + 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 ? */ checking for the NAK at this point is really necessary. I've tested the patches by writing to the EEPROM using the at24 driver. If the data to write cross the EEPROM's page boundary then the at24 driver splits the I2C write commands. After the first I2C write command is done the at24 driver immediately tries to send the next I2C write command. Normally the EEPROM signals a NAK, because the previous write operation is internally still in progress. The NAK will be handled by the at24 driver. It later retries the I2C write command. But if the the EEPROM signals a NAK and the I2C write command has more than 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!" appears, because the loop is not exited due to the NAK. 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
Dear Torsten Fleischer, > Hi Marek, > > > + > > + 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 ? */ > > checking for the NAK at this point is really necessary. > > I've tested the patches by writing to the EEPROM using the at24 driver. > If the data to write cross the EEPROM's page boundary then the at24 driver > splits the I2C write commands. > > After the first I2C write command is done the at24 driver immediately tries > to send the next I2C write command. Normally the EEPROM signals a NAK, > because the previous write operation is internally still in progress. The > NAK will be handled by the at24 driver. It later retries the I2C write > command. > > But if the the EEPROM signals a NAK and the I2C write command has more than > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!" > appears, because the loop is not exited due to the NAK. Ok, so we check HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we "goto cleanup" with ret = -ENXIO . How does it sound to you? Wolfram, is -ENXIO the right one here? 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, > > > + > > > + /* Check NAK here ? */ > > > > checking for the NAK at this point is really necessary. > > > > I've tested the patches by writing to the EEPROM using the at24 driver. > > If the data to write cross the EEPROM's page boundary then the at24 driver > > splits the I2C write commands. > > > > After the first I2C write command is done the at24 driver immediately > > tries > > to send the next I2C write command. Normally the EEPROM signals a NAK, > > because the previous write operation is internally still in progress. The > > NAK will be handled by the at24 driver. It later retries the I2C write > > command. > > > > But if the the EEPROM signals a NAK and the I2C write command has more > > than > > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!" > > appears, because the loop is not exited due to the NAK. > > Ok, so we check > > HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we > "goto cleanup" with ret = -ENXIO . How does it sound to you? this sounds good to me. 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
Dear Torsten Fleischer, > Hi Marek, > > > > > + > > > > + /* Check NAK here ? */ > > > > > > checking for the NAK at this point is really necessary. > > > > > > I've tested the patches by writing to the EEPROM using the at24 driver. > > > If the data to write cross the EEPROM's page boundary then the at24 > > > driver splits the I2C write commands. > > > > > > After the first I2C write command is done the at24 driver immediately > > > tries > > > to send the next I2C write command. Normally the EEPROM signals a NAK, > > > because the previous write operation is internally still in progress. > > > The NAK will be handled by the at24 driver. It later retries the I2C > > > write command. > > > > > > But if the the EEPROM signals a NAK and the I2C write command has more > > > than > > > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE > > > cmd!" appears, because the loop is not exited due to the NAK. > > > > Ok, so we check > > > > HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we > > "goto cleanup" with ret = -ENXIO . How does it sound to you? > > this sounds good to me. Does it fix your EEPROM issue then ? 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, > > > > > > > + > > > > > + /* Check NAK here ? */ > > > > > > > > checking for the NAK at this point is really necessary. > > > > > > > > I've tested the patches by writing to the EEPROM using the at24 > > > > driver. > > > > If the data to write cross the EEPROM's page boundary then the at24 > > > > driver splits the I2C write commands. > > > > > > > > After the first I2C write command is done the at24 driver immediately > > > > tries > > > > to send the next I2C write command. Normally the EEPROM signals a NAK, > > > > because the previous write operation is internally still in progress. > > > > The NAK will be handled by the at24 driver. It later retries the I2C > > > > write command. > > > > > > > > But if the the EEPROM signals a NAK and the I2C write command has more > > > > than > > > > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE > > > > cmd!" appears, because the loop is not exited due to the NAK. > > > > > > Ok, so we check > > > > > > HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we > > > "goto cleanup" with ret = -ENXIO . How does it sound to you? > > > > this sounds good to me. > > Does it fix your EEPROM issue then ? yes, this fix solves the EEPROM issue. I have tested it. 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 ca54ac4..741e5dc 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -1,6 +1,7 @@ /* * Freescale MXS I2C bus driver * + * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de> * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K. * * based on a (non-working) driver which was: @@ -295,48 +296,11 @@ write_init_pio_fail: return -EINVAL; } -static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c) +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_DEBUG0) & - MXS_I2C_DEBUG0_DMAREQ)) { - 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); - - /* - * We do not use interrupts in the PIO mode. Due to the - * maximum transfer length being 8 bytes in PIO mode, the - * overhead of interrupt would be too large and this would - * neglect the gain from using the PIO mode. - */ - - 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); - - /* - * When ending a transfer with a stop, we have to wait for the bus to - * go idle before we report the transfer as completed. Otherwise the - * start of the next transfer may race with the end of the current one. - */ - while (last && (readl(i2c->regs + MXS_I2C_STAT) & - (MXS_I2C_STAT_BUS_BUSY | MXS_I2C_STAT_CLK_GEN_BUSY))) { + while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) { if (time_after(jiffies, timeout)) return -ETIMEDOUT; cond_resched(); @@ -374,106 +338,201 @@ 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); +} + 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, xmit = 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. + */ + BUG_ON(msg->len > 4); + addr_data |= I2C_SMBUS_READ; /* SELECT command. */ - mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT); + mxs_i2c_pio_trigger_write_cmd(i2c, MXS_CMD_I2C_SELECT, addr_data); - 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); - - 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, MXS_CMD_I2C_READ | flags | MXS_I2C_CTRL0_XFER_COUNT(msg->len)); + ret = mxs_i2c_pio_wait_xfer_end(i2c); + if (ret) { + dev_err(i2c->dev, + "PIO: Failed to send SELECT command!\n"); + goto cleanup; + } + + data = readl(i2c->regs + MXS_I2C_DATA); 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); - 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); + + xmit = 0; + + /* This is the last transfer of the message. */ + if (i + 1 == msg->len) { + /* Add optional STOP flag. */ + start |= flags; + /* Remove RETAIN_CLOCK bit. */ + start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK; + xmit = 1; } - } - 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); + /* Four bytes are ready in the "data" variable. */ + if ((i & 3) == 2) + xmit = 1; + + /* Nothing interesting happened, continue stuffing. */ + if (!xmit) + continue; + + /* + * Compute the size of the transfer and shift the + * data accordingly. + * + * i = (4k + 0) .... xlen = 2 + * i = (4k + 1) .... xlen = 3 + * i = (4k + 2) .... xlen = 4 + * i = (4k + 3) .... xlen = 1 + */ + + if ((i % 4) == 3) + xlen = 1; + else + xlen = (i % 4) + 2; + + data >>= (4 - xlen) * 8; + + 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; } /* @@ -485,6 +544,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; @@ -495,16 +555,24 @@ 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. - * Note: this special feature only works on i.MX28 SoC + * 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; + + /* Disable PIO on MX23. */ + if (i2c->dev_type == MXS_I2C_V1) + use_pio = 0; + i2c->cmd_err = 0; - if (i2c->dev_type == MXS_I2C_V2 && 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); @@ -516,9 +584,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. @@ -527,8 +597,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; @@ -775,6 +843,7 @@ static void __exit mxs_i2c_exit(void) } module_exit(mxs_i2c_exit); +MODULE_AUTHOR("Marek Vasut <marex@denx.de>"); MODULE_AUTHOR("Wolfram Sang <w.sang@pengutronix.de>"); MODULE_DESCRIPTION("MXS I2C Bus Driver"); MODULE_LICENSE("GPL");
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: Christoph Baumann <cb@sgoc.de> Cc: Fabio Estevam <r49496@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: Torsten Fleischer <to-fleischer@t-online.de> Cc: Wolfram Sang <wsa@the-dreams.de> --- drivers/i2c/busses/i2c-mxs.c | 263 ++++++++++++++++++++++++++---------------- 1 file changed, 166 insertions(+), 97 deletions(-) V2: - Fix the data register shift computation algorithm and document that some more. The original algo failed for short 1-byte writes as seen in http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg12812.html - Add me a copyright/authorship line, since according to git blame, I have quite a lot of authored lines in the driver. Wolfram, I brought this up some time ago already, but finally got to it. You're OK with this, right?