Hi Jean-Jacques,
On 22/11/2013 09:58, jean-jacques hiblot wrote:
> The current implementation uses the interrupt only to wakeup the process doing
> the data transfer. While this is working, it introduces indesirable latencies.
> This patch implements the data transfer in the interrupt handler. It keeps the
> latency between individual bytes low and the jitter on the total transfer time
> is reduced.
>
> Note: transfer abortion and polling mode are not working and will be supported
> in further patches
>
> This was tested on a custom board built around a PPC460EX with several
> different I2C devices (including EEPROMs and smart batteries)
>
> Signed-off-by: jean-jacques hiblot <jjhiblot@xxxxxxxxx>
Reviewed-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
I didn't see anything scary on this patch but my last interaction with this i2c
controller was a long time ago, so I can't really comment the heart of this patch.
However if it worked on your board at least it is not too buggy ;)
I still made a few trivial comments
> ---
> drivers/i2c/busses/i2c-ibm_iic.c | 233 ++++++++++++++++++++++++++++-----------
> drivers/i2c/busses/i2c-ibm_iic.h | 8 ++
> 2 files changed, 177 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
> index 9cdef65..2bb55b3 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -68,6 +68,8 @@ MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)");
> #undef DBG2
> #endif
>
> +# define ERR(dev, f, x...) dev_err(dev->adap.dev.parent, f, ##x)
> +
This chunk should be part of the previous patch
> #if DBG_LEVEL > 0
> # define DBG(dev, f, x...) dev_dbg(dev->adap.dev.parent, f, ##x)
> #else
> @@ -123,6 +125,7 @@ static struct i2c_timings {
> .high = 600,
> .buf = 1300,
> }};
> +static int iic_xfer_bytes(struct ibm_iic_private *dev);
>
> /* Enable/disable interrupt generation */
> static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable)
> @@ -165,8 +168,8 @@ static void iic_dev_init(struct ibm_iic_private* dev)
> /* Clear control register */
> out_8(&iic->cntl, 0);
>
> - /* Enable interrupts if possible */
> - iic_interrupt_mode(dev, dev->irq >= 0);
> + /* Start with each individual interrupt masked*/
> + iic_interrupt_mode(dev, 0);
>
> /* Set mode control */
> out_8(&iic->mdcntl, MDCNTL_FMDB | MDCNTL_EINT | MDCNTL_EUBS
> @@ -325,16 +328,8 @@ err:
> */
> static irqreturn_t iic_handler(int irq, void *dev_id)
> {
> - struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
> - struct iic_regs __iomem *iic = dev->vaddr;
> -
> - DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
> - in_8(&iic->sts), in_8(&iic->extsts));
> -
> - /* Acknowledge IRQ and wakeup iic_wait_for_tc */
> - out_8(&iic->sts, STS_IRQA | STS_SCMP);
> - wake_up_interruptible(&dev->wq);
> -
> + struct ibm_iic_private *dev = (struct ibm_iic_private *) dev_id;
> + iic_xfer_bytes(dev);
> return IRQ_HANDLED;
> }
>
> @@ -457,60 +452,126 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){
> /*
> * Low level master transfer routine
> */
> -static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
> - int combined_xfer)
> +static int iic_xfer_bytes(struct ibm_iic_private *dev)
> {
> - struct iic_regs __iomem *iic = dev->vaddr;
> - char* buf = pm->buf;
> - int i, j, loops, ret = 0;
> - int len = pm->len;
> -
> - u8 cntl = (in_8(&iic->cntl) & CNTL_AMD) | CNTL_PT;
> - if (pm->flags & I2C_M_RD)
> - cntl |= CNTL_RW;
> + struct i2c_msg *pm = &(dev->msgs[dev->current_msg]);
> + struct iic_regs *iic = dev->vaddr;
> + u8 cntl = in_8(&iic->cntl) & CNTL_AMD;
> + int count;
> + u32 status;
> + u32 ext_status;
> +
> + /* First check the status register */
> + status = in_8(&iic->sts);
> + ext_status = in_8(&iic->extsts);
> +
> + /* and acknowledge the interrupt */
> + out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | EXTSTS_LA |
> + EXTSTS_ICT | EXTSTS_XFRA);
> + out_8(&iic->sts, STS_IRQA | STS_SCMP);
>
> - loops = (len + 3) / 4;
> - for (i = 0; i < loops; ++i, len -= 4){
> - int count = len > 4 ? 4 : len;
> - u8 cmd = cntl | ((count - 1) << CNTL_TCT_SHIFT);
> + if ((status & STS_ERR) ||
> + (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
> + DBG(dev, "status 0x%x\n", status);
> + DBG(dev, "extended status 0x%x\n", ext_status);
> + if (status & STS_ERR)
> + ERR(dev, "Error detected\n");
> + if (ext_status & EXTSTS_LA)
> + DBG(dev, "Lost arbitration\n");
> + if (ext_status & EXTSTS_ICT)
> + ERR(dev, "Incomplete transfer\n");
> + if (ext_status & EXTSTS_XFRA)
> + ERR(dev, "Transfer aborted\n");
> +
> + dev->status = -EIO;
> + dev->transfer_complete = 1;
> + complete(&dev->iic_compl);
> + return dev->status;
> + }
>
> - if (!(cntl & CNTL_RW))
> - for (j = 0; j < count; ++j)
> - out_8(&iic->mdbuf, *buf++);
> + if (dev->msgs == NULL) {
> + DBG(dev, "spurious !!!!!\n");
> + dev->status = -EINVAL;
> + return dev->status;
> + }
>
> - if (i < loops - 1)
> - cmd |= CNTL_CHT;
> - else if (combined_xfer)
> - cmd |= CNTL_RPST;
> + /* check if there's any data to read from the fifo */
> + if (pm->flags & I2C_M_RD) {
> + while (dev->current_byte_rx < dev->current_byte) {
> + u8 *buf = pm->buf + dev->current_byte_rx;
> + *buf = in_8(&iic->mdbuf);
> + dev->current_byte_rx++;
> + DBG2(dev, "read 0x%x\n", *buf);
> + }
> + }
> + /* check if we must go with the next tranfer */
> + if (pm->len == dev->current_byte) {
> + DBG2(dev, "going to next transfer\n");
> + dev->current_byte = 0;
> + dev->current_byte_rx = 0;
> + dev->current_msg++;
> + if (dev->current_msg == dev->num_msgs) {
> + DBG2(dev, "end of transfer\n");
> + dev->transfer_complete = 1;
> + complete(&dev->iic_compl);
> + return dev->status;
> + }
> + pm++;
> + }
>
> - DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd);
> + /* take care of the direction */
> + if (pm->flags & I2C_M_RD)
> + cntl |= CNTL_RW;
>
> - /* Start transfer */
> - out_8(&iic->cntl, cmd);
> + /* how much data are we going to transfer this time ?
> + * (up to 4 bytes at once)
> + */
> + count = pm->len - dev->current_byte;
> + count = (count > 4) ? 4 : count;
> + cntl |= (count - 1) << CNTL_TCT_SHIFT;
> +
> + if ((pm->flags & I2C_M_RD) == 0) {
> + /* This is a write. we must fill the fifo with the data */
> + int i;
> + u8 *buf = pm->buf + dev->current_byte;
> +
> + for (i = 0; i < count; i++) {
> + out_8(&iic->mdbuf, buf[i]);
> + DBG2(dev, "write : 0x%x\n", buf[i]);
> + }
> + }
>
> - /* Wait for completion */
> - ret = iic_wait_for_tc(dev);
> + /* will the current transfer complete with this chunk of data ? */
> + if (pm->len > dev->current_byte + 4) {
> + /* we're not done with the current transfer.
> + * Don't send a repeated start
> + */
> + cntl |= CNTL_CHT;
> + }
> + /* This transfer will be complete with this chunk of data
> + * BUT is this the last transfer of the list ?
> + */
It's really a nitpick but the style for long (multi-line) comments
this should be:
/*
* This transfer will be complete with this chunk of data
* BUT is this the last transfer of the list ?
*/
The style you used was for files in net/ and drivers/net/,
see "Chapter 8: Commenting" of the Documentation/CodingStyle file
> + else if (dev->current_msg != (dev->num_msgs-1)) {
> + /* not the last tranfer */
> + cntl |= CNTL_RPST; /* Do not send a STOP condition */
> + /* check if the NEXT transfer needs a repeated start */
> + if (pm[1].flags & I2C_M_NOSTART)
> + cntl |= CNTL_CHT;
> + }
>
> - if (unlikely(ret < 0))
> - break;
> - else if (unlikely(ret != count)){
> - DBG(dev, "xfer_bytes, requested %d, transferred %d\n",
> - count, ret);
> + if ((cntl & CNTL_RPST) == 0)
> + DBG2(dev, "STOP required\n");
>
> - /* If it's not a last part of xfer, abort it */
> - if (combined_xfer || (i < loops - 1))
> - iic_abort_xfer(dev);
> + if ((cntl & CNTL_CHT) == 0)
> + DBG2(dev, "next transfer will begin with START\n");
>
> - ret = -EREMOTEIO;
> - break;
> - }
> + /* keep track of the amount of data transfered (RX and TX) */
> + dev->current_byte += count;
>
> - if (cntl & CNTL_RW)
> - for (j = 0; j < count; ++j)
> - *buf++ = in_8(&iic->mdbuf);
> - }
> + /* actually start the transfer of the current data chunk */
> + out_8(&iic->cntl, cntl | CNTL_PT);
>
> - return ret > 0 ? 0 : ret;
> + return 0;
> }
>
> /*
> @@ -608,19 +669,63 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> return -EREMOTEIO;
> }
> }
> - else {
> - /* Flush master data buffer (just in case) */
> - out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
> +
> + dev->current_byte = dev->current_msg = dev->current_byte_rx = 0;
> + dev->transfer_complete = 0;
> + dev->status = 0;
> + dev->msgs = msgs;
> + dev->num_msgs = num;
> +
> + /* clear the buffers */
> + out_8(&iic->mdcntl, MDCNTL_FMDB);
> + i = 100;
please use a define for it
something like
#define FLUSH_TIMEOUT 100
> + while (in_8(&iic->mdcntl) & MDCNTL_FMDB) {
> + if (i-- < 0) {
> + DBG(dev, "iic_xfer, unable to flush\n");
> + return -EREMOTEIO;
> + }
> }
>
> + /* clear all interrupts masks */
> + out_8(&iic->intmsk, 0x0);
> + /* clear any status */
> + out_8(&iic->sts, STS_IRQA | STS_SCMP);
> + out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | EXTSTS_LA |
> + EXTSTS_ICT | EXTSTS_XFRA);
> +
> /* Load slave address */
> iic_address(dev, &msgs[0]);
>
> - /* Do real transfer */
> - for (i = 0; i < num && !ret; ++i)
> - ret = iic_xfer_bytes(dev, &msgs[i], i < num - 1);
> + init_completion(&dev->iic_compl);
> +
> + /* start the transfer */
> + ret = iic_xfer_bytes(dev);
> +
> + if (ret == 0) {
> + /* enable the interrupts */
> + out_8(&iic->mdcntl, MDCNTL_EINT);
> + /* unmask the interrupts */
> + out_8(&iic->intmsk, INTRMSK_EIMTC | INTRMSK_EITA |
> + INTRMSK_EIIC | INTRMSK_EIHE);
> + /* wait for the transfer to complete */
> + ret = wait_for_completion_interruptible_timeout(
> + &dev->iic_compl, num * HZ);
> + /* mask the interrupts */
> + out_8(&iic->intmsk, 0);
> + }
>
> - return ret < 0 ? ret : num;
> + if (ret == 0) {
> + ERR(dev, "transfer timed out\n");
> + ret = -ETIMEDOUT;
> + } else if (ret < 0) {
> + if (ret == -ERESTARTSYS)
> + ERR(dev, "transfer interrupted\n");
> + } else {
> + /* Transfer is complete */
> + ret = (dev->status) ? dev->status : num;
> + }
> +
> + return ret;
> }
>
> static u32 iic_func(struct i2c_adapter *adap)
> @@ -673,7 +778,7 @@ static int iic_request_irq(struct platform_device *ofdev,
>
> irq = irq_of_parse_and_map(np, 0);
> if (!irq) {
> - dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
> + ERR(dev, "irq_of_parse_and_map failed\n");
This chunk should be part of the previous patch
> return 0;
> }
>
> @@ -682,7 +787,7 @@ static int iic_request_irq(struct platform_device *ofdev,
> */
> iic_interrupt_mode(dev, 0);
> if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
> - dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
> + ERR(dev, "request_irq %d failed\n", irq);
This chunk should be part of the previous patch
> /* Fallback to the polling mode */
> return 0;
> }
> @@ -720,7 +825,7 @@ static int iic_probe(struct platform_device *ofdev)
>
> dev->irq = iic_request_irq(ofdev, dev);
> if (!dev->irq)
> - dev_warn(&ofdev->dev, "using polling mode\n");
> + dev_info(&ofdev->dev, "using polling mode\n");
This chunk should be part of the previous patch
>
> /* Board specific settings */
> if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
> index 1efce5d..76c476a 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.h
> +++ b/drivers/i2c/busses/i2c-ibm_iic.h
> @@ -51,6 +51,14 @@ struct ibm_iic_private {
> int irq;
> int fast_mode;
> u8 clckdiv;
> + struct i2c_msg *msgs;
> + int num_msgs;
> + int current_msg;
> + int current_byte;
> + int current_byte_rx;
> + int transfer_complete;
> + int status;
> + struct completion iic_compl;
> };
>
> /* IICx_CNTL register */
>
Thanks,
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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
