Hi Gregory, On 20/11/2013 16:53, Gregory CLEMENT wrote:
Polling mode is supported if no valid interrupt is defined in the device tree.Hi Jean-Jacques, On 20/11/2013 16:08, 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 a sometimes indesirable blanks during the transfer. 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. Polling mode is still supported.
I saw that you added a flag named use_polling, but I didn't see how a user can set it (nothing was added in the device tree). Could you explain what do you meant by "Polling mode is still supported" ?This was tested on a custom board built around a PPC460EX with several different I2C devices (including EEPROMs and smart batteries)Could you wrap the commit log at ~80 columns?
Ok
I made only a few comments because your patch is hard to review. Could you split your patch in smaller and incremental part?
Ok
However I made few comments inlineSigned-off-by: jean-jacques hiblot <jjhiblot@xxxxxxxxx> --- drivers/i2c/busses/i2c-ibm_iic.c | 411 +++++++++++++++++++++++---------------- drivers/i2c/busses/i2c-ibm_iic.h | 21 +- 2 files changed, 263 insertions(+), 169 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index ff3caa0..77df857 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -163,8 +163,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); + /* disable interrupts*/ + iic_interrupt_mode(dev, 0); /* Set mode control */ out_8(&iic->mdcntl, MDCNTL_FMDB | MDCNTL_EINT | MDCNTL_EUBS @@ -319,197 +319,218 @@ err: goto out; } -/* - * IIC interrupt handler - */ -static irqreturn_t iic_handler(int irq, void *dev_id) +static int iic_xfer_bytes(struct ibm_iic_private *dev) { - struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id; - volatile struct iic_regs __iomem *iic = dev->vaddr; - - DBG2("%d: irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n", - dev->idx, in_8(&iic->sts), in_8(&iic->extsts)); - - /* Acknowledge IRQ and wakeup iic_wait_for_tc */ + 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); - wake_up_interruptible(&dev->wq); - return IRQ_HANDLED; -} + if (dev->status == -ECANCELED) { + dev_dbg(dev->dev, "abort completed\n"); + dev->transfer_complete = 1; + if (!dev->use_polling) + complete(&dev->iic_compl); + return dev->status; + } -/* - * Get master transfer result and clear errors if any. - * Returns the number of actually transferred bytes or error (<0) - */ -static int iic_xfer_result(struct ibm_iic_private* dev) -{ - volatile struct iic_regs __iomem *iic = dev->vaddr; + if ((status & STS_ERR) || + (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) { + dev_dbg(dev->dev, "status 0x%x\n", status); + dev_dbg(dev->dev, "extended status 0x%x\n", ext_status); + if (status & STS_ERR) + dev_dbg(dev->dev, "Error detected\n");Is it really a debug message? shouldn't it be an error message?
Indeed I'll make it a dev_err
+ if (ext_status & EXTSTS_LA) + dev_dbg(dev->dev, "Lost arbitration\n");Ditto
I don't know if this is really an error. Arbitration loss wan be very common in a multi master environement. What about dev_warn ou dev_info
+ if (ext_status & EXTSTS_ICT) + dev_dbg(dev->dev, "Incomplete transfer\n");Ditto+ if (ext_status & EXTSTS_XFRA) + dev_dbg(dev->dev, "Transfer aborted\n");Ditto
I'll use dev_err instead for those two.
+ + dev->status = -EIO; + dev->transfer_complete = 1; + if (!dev->use_polling) + complete(&dev->iic_compl); + } - if (unlikely(in_8(&iic->sts) & STS_ERR)){ - DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx, - in_8(&iic->extsts)); + if (dev->msgs == NULL) { + dev_dbg(dev->dev, "spurious !!!!!\n"); + dev->status = -EINVAL; + return dev->status; + } - /* Clear errors and possible pending IRQs */ - out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | - EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA); + /* 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++; + dev_dbg(dev->dev, "read 0x%x\n", *buf); + } + } + /* check if we must go with the next tranfer */ + if (pm->len == dev->current_byte) { + dev_dbg(dev->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) { + dev_dbg(dev->dev, "end of transfer\n"); + dev->transfer_complete = 1; + if (!dev->use_polling) + complete(&dev->iic_compl); + return dev->status; + } + pm++; + } - /* Flush master data buffer */ - out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB); + /* take care of the direction */ + if (pm->flags & I2C_M_RD) + cntl |= CNTL_RW; - /* Is bus free? - * If error happened during combined xfer - * IIC interface is usually stuck in some strange - * state, the only way out - soft reset. - */ - if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ - DBG("%d: bus is stuck, resetting\n", dev->idx); - iic_dev_reset(dev); + /* 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]); + dev_dbg(dev->dev, "write : 0x%x\n", buf[i]); } - return -EREMOTEIO; } - else - return in_8(&iic->xfrcnt) & XFRCNT_MTC_MASK; -} -/* - * Try to abort active transfer. - */ -static void iic_abort_xfer(struct ibm_iic_private* dev) -{ - volatile struct iic_regs __iomem *iic = dev->vaddr; - unsigned long x; + /* 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 ? + */ + else if (dev->current_msg != (dev->num_msgs-1)) { + /* not the last tranfer */ + cntl |= CNTL_BCL; /* 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; + } - DBG("%d: iic_abort_xfer\n", dev->idx); + if ((cntl & CNTL_BCL) == 0) + dev_dbg(dev->dev, "STOP required\n"); - out_8(&iic->cntl, CNTL_HMT); + if ((cntl & CNTL_CHT) == 0) + dev_dbg(dev->dev, "next transfer will begin with START\n"); - /* - * Wait for the abort command to complete. - * It's not worth to be optimized, just poll (timeout >= 1 tick) - */ - x = jiffies + 2; - while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ - if (time_after(jiffies, x)){ - DBG("%d: abort timeout, resetting...\n", dev->idx); - iic_dev_reset(dev); - return; - } - schedule(); + /* keep track of the amount of data transfered (RX and TX) */ + dev->current_byte += count; + + /* actually start the transfer of the current data chunk */ + out_8(&iic->cntl, cntl | CNTL_PT); + + /* The transfer must be aborted. */ + if (dev->abort) { + dev_dbg(dev->dev, "aborting\n"); + out_8(&iic->cntl, CNTL_HMT); + dev->status = -ECANCELED; } - /* Just to clear errors */ - iic_xfer_result(dev); + return dev->status; } /* - * Wait for master transfer to complete. - * It puts current process to sleep until we get interrupt or timeout expires. - * Returns the number of transferred bytes or error (<0) + * IIC interrupt handler */ -static int iic_wait_for_tc(struct ibm_iic_private* dev){ +static irqreturn_t iic_handler(int irq, void *dev_id) +{ + struct ibm_iic_private *dev = (struct ibm_iic_private *) dev_id; + iic_xfer_bytes(dev);Shouldn't you do something with the return value of this function?
Not really. We don't really care of the return value. except when transmiting the first byte of the global transfer.
I needed the pointer to the struct device for the dev_err, dev_dbg etc., so I stored it in the private structure. If it's better I can modify the driver to get it from adap->dev.parent when needed.+ return IRQ_HANDLED; +} +/* + * Polling used when interrupt can't be used + */ +static int poll_for_end_of_transfer(struct ibm_iic_private *dev, u32 timeout) +{ volatile struct iic_regs __iomem *iic = dev->vaddr; - int ret = 0; - - if (dev->irq >= 0){ - /* Interrupt mode */ - ret = wait_event_interruptible_timeout(dev->wq, - !(in_8(&iic->sts) & STS_PT), dev->adap.timeout); - - if (unlikely(ret < 0)) - DBG("%d: wait interrupted\n", dev->idx); - else if (unlikely(in_8(&iic->sts) & STS_PT)){ - DBG("%d: wait timeout\n", dev->idx); - ret = -ETIMEDOUT; - } - } - else { - /* Polling mode */ - unsigned long x = jiffies + dev->adap.timeout; - - while (in_8(&iic->sts) & STS_PT){ - if (unlikely(time_after(jiffies, x))){ - DBG("%d: poll timeout\n", dev->idx); - ret = -ETIMEDOUT; - break; - } - - if (unlikely(signal_pending(current))){ - DBG("%d: poll interrupted\n", dev->idx); - ret = -ERESTARTSYS; - break; - } + u32 status; + unsigned long end = jiffies + timeout; + + while ((dev->transfer_complete == 0) && + time_after(end, jiffies) && + !signal_pending(current)) { + status = in_8(&iic->sts); + /* check if the transfer is done or an error occured */ + if ((status & (STS_ERR | STS_SCMP)) || !(status & STS_PT)) + iic_xfer_bytes(dev); + /* The transfer is not complete, + * let's wait a bit to give time to the controller to update + * its registers. + * calling schedule relaxes the CPU load and allows to know + * if the process is being signaled (for abortion) + */ + if (dev->transfer_complete == 0) schedule(); - } } - if (unlikely(ret < 0)) - iic_abort_xfer(dev); - else - ret = iic_xfer_result(dev); + if (signal_pending(current)) + return -ERESTARTSYS; - DBG2("%d: iic_wait_for_tc -> %d\n", dev->idx, ret); + if (dev->transfer_complete == 0) + return 0; - return ret; + return 1; } /* - * Low level master transfer routine + * Try to abort active transfer. */ -static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, - int combined_xfer) +static void iic_abort_xfer(struct ibm_iic_private *dev) { - volatile 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; - - 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 (!(cntl & CNTL_RW)) - for (j = 0; j < count; ++j) - out_8((void __iomem *)&iic->mdbuf, *buf++); - - if (i < loops - 1) - cmd |= CNTL_CHT; - else if (combined_xfer) - cmd |= CNTL_RPST; - - DBG2("%d: xfer_bytes, %d, CNTL = 0x%02x\n", dev->idx, count, cmd); - - /* Start transfer */ - out_8(&iic->cntl, cmd); - - /* Wait for completion */ - ret = iic_wait_for_tc(dev); - - if (unlikely(ret < 0)) - break; - else if (unlikely(ret != count)){ - DBG("%d: xfer_bytes, requested %d, transferred %d\n", - dev->idx, count, ret); - - /* If it's not a last part of xfer, abort it */ - if (combined_xfer || (i < loops - 1)) - iic_abort_xfer(dev); - - ret = -EREMOTEIO; - break; + struct iic_regs *iic = dev->vaddr; + unsigned long end; + + dev_dbg(dev->dev, "aborting transfer\n"); + /* transfer should be aborted within 10ms */ + end = jiffies + 10; + dev->abort = 1; + + while (time_after(end, jiffies) && !dev->transfer_complete) { + u32 sts; + if (dev->use_polling) { + sts = in_8(&iic->sts); + /* check if the transfer is done or an error occured */ + if ((sts & (STS_ERR | STS_SCMP)) || !(sts & STS_PT)) + iic_xfer_bytes(dev); } - - if (cntl & CNTL_RW) - for (j = 0; j < count; ++j) - *buf++ = in_8((void __iomem *)&iic->mdbuf); + if (dev->transfer_complete == 0) + schedule(); } - return ret > 0 ? 0 : ret; + if (!dev->transfer_complete) { + dev_err(dev->dev, "abort operation failed\n"); + iic_dev_reset(dev); + } } /* @@ -607,19 +628,74 @@ 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->msgs = msgs; + dev->num_msgs = num; + dev->transfer_complete = 0; + dev->status = 0; + dev->abort = 0; + + /* clear the buffers */ + out_8(&iic->mdcntl, MDCNTL_FMDB); + i = 100; + while (in_8(&iic->mdcntl) & MDCNTL_FMDB) { + if (i-- < 0) { + DBG("%d: iic_xfer, unable to flush\n", dev->idx); + 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); + if (!dev->use_polling) + init_completion(&dev->iic_compl); + + /* start the transfer */ + ret = iic_xfer_bytes(dev); + + if (ret == 0) { + if (dev->use_polling) { + ret = poll_for_end_of_transfer(dev, num * HZ); + } else { + /* 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); + /* we don't mask the interrupts here because we may + * need them to abort the transfer gracefully + */ + } + } - return ret < 0 ? ret : num; + if (ret == 0) { + dev_err(dev->dev, "transfer timed out\n"); + ret = -ETIMEDOUT; + } else if (ret < 0) { + if (ret == -ERESTARTSYS) + dev_err(dev->dev, "transfer interrupted\n"); + iic_abort_xfer(dev); + } else { + /* Transfer is complete */ + ret = (dev->status) ? dev->status : num; + } + + /* mask the interrupts */ + out_8(&iic->intmsk, 0); + + return ret; } static u32 iic_func(struct i2c_adapter *adap) @@ -668,6 +744,7 @@ static int iic_request_irq(struct platform_device *ofdev, if (iic_force_poll) return 0; + dev->use_polling = 1; irq = irq_of_parse_and_map(np, 0); if (!irq) { dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n"); @@ -677,13 +754,14 @@ static int iic_request_irq(struct platform_device *ofdev, /* Disable interrupts until we finish initialization, assumes * level-sensitive IRQ setup... */ + init_completion(&dev->iic_compl); 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); /* Fallback to the polling mode */ return 0; } - + dev->use_polling = 0; return irq; } @@ -705,6 +783,7 @@ static int iic_probe(struct platform_device *ofdev) } platform_set_drvdata(ofdev, dev); + dev->dev = &ofdev->dev;Is it related to the topic of this patch?
>
dev->vaddr = of_iomap(np, 0); if (dev->vaddr == NULL) { diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h index fdaa482..8fa2b6b 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.h +++ b/drivers/i2c/busses/i2c-ibm_iic.h @@ -22,11 +22,14 @@ #ifndef __I2C_IBM_IIC_H_ #define __I2C_IBM_IIC_H_ +#include <linux/device.h> #include <linux/i2c.h> struct iic_regs { - u16 mdbuf; - u16 sbbuf; + u8 mdbuf; + u8 reserved1; + u8 sbbuf; + u8 reserved2; u8 lmadr; u8 hmadr; u8 cntl; @@ -44,12 +47,23 @@ struct iic_regs { struct ibm_iic_private { struct i2c_adapter adap; - volatile struct iic_regs __iomem *vaddr; + struct iic_regs *vaddr; wait_queue_head_t wq; int idx; 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 use_polling; + int status; + int abort; + struct completion iic_compl; + struct device *dev; }; /* IICx_CNTL register */ @@ -58,6 +72,7 @@ struct ibm_iic_private { #define CNTL_TCT_MASK 0x30 #define CNTL_TCT_SHIFT 4 #define CNTL_RPST 0x08 +#define CNTL_BCL CNTL_RPST #define CNTL_CHT 0x04 #define CNTL_RW 0x02 #define CNTL_PT 0x01
-- 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
