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?
I made only a few comments because your patch is hard to review.
Could you split your patch in smaller and incremental part?
However I made few comments inline
>
> Signed-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?
> + if (ext_status & EXTSTS_LA)
> + dev_dbg(dev->dev, "Lost arbitration\n");
Ditto
> + 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
> +
> + 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?
> + 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
>
--
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
