* [PATCH v2 0/4] i2c : i2c-ibm-iic : use interrupts to perform the data transfer
@ 2013-11-22 8:58 jean-jacques hiblot
[not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: jean-jacques hiblot @ 2013-11-22 8:58 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot
This patch set aims to improve the performance of the driver for the IBM iic
controller by implementing the data transfer in the interrupt handler.
Using interrupts to trigger the data transfer reduces and make more
deterministic the latencies between indivdual bytes, and consequently reduces
the total transfer time,
In our test environement with multiple masters, this significantly reduces the
rate of i2c errors.
Changes since v1:
- split the patch in 4 individual patches. The code has been refactored a bit
to make the diff easier to read.
- changed some dev_dbg in dev_err or dev_warn when more appropriate
jean-jacques hiblot (4):
i2c: i2c-ibm-iic: cleanup.
i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
i2c: i2c-ibm-iic: Implements transfer abortion
i2c: i2c-ibm-iic: Implements a polling mode
drivers/i2c/busses/i2c-ibm_iic.c | 469 +++++++++++++++++++++++----------------
drivers/i2c/busses/i2c-ibm_iic.h | 20 +-
2 files changed, 293 insertions(+), 196 deletions(-)
--
1.8.4.2
^ permalink raw reply [flat|nested] 10+ messages in thread[parent not found: <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>]
* [PATCH v2 1/4] i2c: i2c-ibm-iic: cleanup. [not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> @ 2013-11-22 8:58 ` jean-jacques hiblot [not found] ` <1385110686-4226-2-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 2013-11-22 8:58 ` [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler jean-jacques hiblot ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: jean-jacques hiblot @ 2013-11-22 8:58 UTC (permalink / raw) To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot, jean-jacques hiblot * removed unneeded 'volatile' qualifiers and casts * use the dev_dbg, dev_err etc. instead of printk * removed unneeded members for the driver's private data Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- drivers/i2c/busses/i2c-ibm_iic.c | 109 ++++++++++++++++++++------------------- drivers/i2c/busses/i2c-ibm_iic.h | 9 ++-- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index ff3caa0..9cdef65 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -69,21 +69,23 @@ MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)"); #endif #if DBG_LEVEL > 0 -# define DBG(f,x...) printk(KERN_DEBUG "ibm-iic" f, ##x) +# define DBG(dev, f, x...) dev_dbg(dev->adap.dev.parent, f, ##x) #else -# define DBG(f,x...) ((void)0) +# define DBG(dev, f, x...) ((void)0) #endif #if DBG_LEVEL > 1 -# define DBG2(f,x...) DBG(f, ##x) +# define DBG2(dev, f, x...) DBG(dev, f, ##x) #else -# define DBG2(f,x...) ((void)0) +# define DBG2(dev, f, x...) ((void)0) #endif #if DBG_LEVEL > 2 static void dump_iic_regs(const char* header, struct ibm_iic_private* dev) { - volatile struct iic_regs __iomem *iic = dev->vaddr; - printk(KERN_DEBUG "ibm-iic%d: %s\n", dev->idx, header); - printk(KERN_DEBUG + struct iic_regs __iomem *iic = dev->vaddr; + struct device *device = dev.adap.dev.parent; + + dev_dbg(device, ": %s\n", header); + dev_dbg(device, " cntl = 0x%02x, mdcntl = 0x%02x\n" " sts = 0x%02x, extsts = 0x%02x\n" " clkdiv = 0x%02x, xfrcnt = 0x%02x\n" @@ -133,9 +135,9 @@ static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable) */ static void iic_dev_init(struct ibm_iic_private* dev) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; - DBG("%d: init\n", dev->idx); + DBG(dev, "init\n"); /* Clear master address */ out_8(&iic->lmadr, 0); @@ -178,11 +180,11 @@ static void iic_dev_init(struct ibm_iic_private* dev) */ static void iic_dev_reset(struct ibm_iic_private* dev) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; int i; u8 dc; - DBG("%d: soft reset\n", dev->idx); + DBG(dev, "soft reset\n"); DUMP_REGS("reset", dev); /* Place chip in the reset state */ @@ -191,7 +193,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) /* Check if bus is free */ dc = in_8(&iic->directcntl); if (!DIRCTNL_FREE(dc)){ - DBG("%d: trying to regain bus control\n", dev->idx); + DBG(dev, "trying to regain bus control\n"); /* Try to set bus free state */ out_8(&iic->directcntl, DIRCNTL_SDAC | DIRCNTL_SCC); @@ -226,7 +228,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) */ /* Wait for SCL and/or SDA to be high */ -static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask) +static int iic_dc_wait(struct iic_regs __iomem *iic, u8 mask) { unsigned long x = jiffies + HZ / 28 + 2; while ((in_8(&iic->directcntl) & mask) != mask){ @@ -239,19 +241,18 @@ static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask) static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; const struct i2c_timings* t = &timings[dev->fast_mode ? 1 : 0]; u8 mask, v, sda; int i, res; /* Only 7-bit addresses are supported */ if (unlikely(p->flags & I2C_M_TEN)){ - DBG("%d: smbus_quick - 10 bit addresses are not supported\n", - dev->idx); + DBG(dev, "smbus_quick - 10 bit addresses are not supported\n"); return -EINVAL; } - DBG("%d: smbus_quick(0x%02x)\n", dev->idx, p->addr); + DBG(dev, "smbus_quick(0x%02x)\n", p->addr); /* Reset IIC interface */ out_8(&iic->xtcntlss, XTCNTLSS_SRST); @@ -304,7 +305,7 @@ static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p) ndelay(t->buf); - DBG("%d: smbus_quick -> %s\n", dev->idx, res ? "NACK" : "ACK"); + DBG(dev, "smbus_quick -> %s\n", res ? "NACK" : "ACK"); out: /* Remove reset */ out_8(&iic->xtcntlss, 0); @@ -314,7 +315,7 @@ out: return res; err: - DBG("%d: smbus_quick - bus is stuck\n", dev->idx); + DBG(dev, "smbus_quick - bus is stuck\n"); res = -EREMOTEIO; goto out; } @@ -325,10 +326,10 @@ err: static irqreturn_t iic_handler(int irq, void *dev_id) { struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id; - volatile struct iic_regs __iomem *iic = dev->vaddr; + 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)); + 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); @@ -343,10 +344,10 @@ static irqreturn_t iic_handler(int irq, void *dev_id) */ static int iic_xfer_result(struct ibm_iic_private* dev) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; if (unlikely(in_8(&iic->sts) & STS_ERR)){ - DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx, + DBG(dev, "xfer error, EXTSTS = 0x%02x\n", in_8(&iic->extsts)); /* Clear errors and possible pending IRQs */ @@ -362,7 +363,7 @@ static int iic_xfer_result(struct ibm_iic_private* dev) * 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); + DBG(dev, "bus is stuck, resetting\n"); iic_dev_reset(dev); } return -EREMOTEIO; @@ -376,10 +377,10 @@ static int iic_xfer_result(struct ibm_iic_private* dev) */ static void iic_abort_xfer(struct ibm_iic_private* dev) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; unsigned long x; - DBG("%d: iic_abort_xfer\n", dev->idx); + DBG(dev, "iic_abort_xfer\n"); out_8(&iic->cntl, CNTL_HMT); @@ -390,7 +391,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) 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); + DBG(dev, "abort timeout, resetting...\n"); iic_dev_reset(dev); return; } @@ -408,7 +409,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) */ static int iic_wait_for_tc(struct ibm_iic_private* dev){ - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; int ret = 0; if (dev->irq >= 0){ @@ -417,9 +418,9 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ !(in_8(&iic->sts) & STS_PT), dev->adap.timeout); if (unlikely(ret < 0)) - DBG("%d: wait interrupted\n", dev->idx); + DBG(dev, "wait interrupted\n"); else if (unlikely(in_8(&iic->sts) & STS_PT)){ - DBG("%d: wait timeout\n", dev->idx); + DBG(dev, "wait timeout\n"); ret = -ETIMEDOUT; } } @@ -429,13 +430,13 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ while (in_8(&iic->sts) & STS_PT){ if (unlikely(time_after(jiffies, x))){ - DBG("%d: poll timeout\n", dev->idx); + DBG(dev, "poll timeout\n"); ret = -ETIMEDOUT; break; } if (unlikely(signal_pending(current))){ - DBG("%d: poll interrupted\n", dev->idx); + DBG(dev, "poll interrupted\n"); ret = -ERESTARTSYS; break; } @@ -448,7 +449,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ else ret = iic_xfer_result(dev); - DBG2("%d: iic_wait_for_tc -> %d\n", dev->idx, ret); + DBG2(dev, "iic_wait_for_tc -> %d\n", ret); return ret; } @@ -459,7 +460,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, int combined_xfer) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; char* buf = pm->buf; int i, j, loops, ret = 0; int len = pm->len; @@ -475,14 +476,14 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, if (!(cntl & CNTL_RW)) for (j = 0; j < count; ++j) - out_8((void __iomem *)&iic->mdbuf, *buf++); + out_8(&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); + DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd); /* Start transfer */ out_8(&iic->cntl, cmd); @@ -493,8 +494,8 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, if (unlikely(ret < 0)) break; else if (unlikely(ret != count)){ - DBG("%d: xfer_bytes, requested %d, transferred %d\n", - dev->idx, count, ret); + DBG(dev, "xfer_bytes, requested %d, transferred %d\n", + count, ret); /* If it's not a last part of xfer, abort it */ if (combined_xfer || (i < loops - 1)) @@ -506,7 +507,7 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, if (cntl & CNTL_RW) for (j = 0; j < count; ++j) - *buf++ = in_8((void __iomem *)&iic->mdbuf); + *buf++ = in_8(&iic->mdbuf); } return ret > 0 ? 0 : ret; @@ -517,10 +518,10 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, */ static inline void iic_address(struct ibm_iic_private* dev, struct i2c_msg* msg) { - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; u16 addr = msg->addr; - DBG2("%d: iic_address, 0x%03x (%d-bit)\n", dev->idx, + DBG2(dev, "iic_address, 0x%03x (%d-bit)\n", addr, msg->flags & I2C_M_TEN ? 10 : 7); if (msg->flags & I2C_M_TEN){ @@ -553,10 +554,10 @@ static inline int iic_address_neq(const struct i2c_msg* p1, static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct ibm_iic_private* dev = (struct ibm_iic_private*)(i2c_get_adapdata(adap)); - volatile struct iic_regs __iomem *iic = dev->vaddr; + struct iic_regs __iomem *iic = dev->vaddr; int i, ret = 0; - DBG2("%d: iic_xfer, %d msg(s)\n", dev->idx, num); + DBG2(dev, "iic_xfer, %d msg(s)\n", num); if (!num) return 0; @@ -565,7 +566,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) * Uhh, generic i2c layer is more suitable place for such code... */ if (unlikely(iic_invalid_address(&msgs[0]))){ - DBG("%d: invalid address 0x%03x (%d-bit)\n", dev->idx, + DBG(dev, "invalid address 0x%03x (%d-bit)\n", msgs[0].addr, msgs[0].flags & I2C_M_TEN ? 10 : 7); return -EINVAL; } @@ -578,19 +579,19 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) */ return iic_smbus_quick(dev, &msgs[0]); } - DBG("%d: invalid len %d in msg[%d]\n", dev->idx, + DBG(dev, "invalid len %d in msg[%d]\n", msgs[i].len, i); return -EINVAL; } if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))){ - DBG("%d: invalid addr in msg[%d]\n", dev->idx, i); + DBG(dev, "invalid addr in msg[%d]\n", i); return -EINVAL; } } /* Check bus state */ if (unlikely((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE)){ - DBG("%d: iic_xfer, bus is not free\n", dev->idx); + DBG(dev, "iic_xfer, bus is not free\n"); /* Usually it means something serious has happened. * We *cannot* have unfinished previous transfer @@ -603,7 +604,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) iic_dev_reset(dev); if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ - DBG("%d: iic_xfer, bus is still not free\n", dev->idx); + DBG(dev, "iic_xfer, bus is still not free\n"); return -EREMOTEIO; } } @@ -635,15 +636,17 @@ static const struct i2c_algorithm iic_algo = { /* * Calculates IICx_CLCKDIV value for a specific OPB clock frequency */ -static inline u8 iic_clckdiv(unsigned int opb) +static inline u8 iic_clckdiv(struct ibm_iic_private *dev, unsigned int opb) { + struct device *device = dev->adap.dev.parent; + /* Compatibility kludge, should go away after all cards * are fixed to fill correct value for opbfreq. * Previous driver version used hardcoded divider value 4, * it corresponds to OPB frequency from the range (40, 50] MHz */ if (!opb){ - printk(KERN_WARNING "ibm-iic: using compatibility value for OPB freq," + dev_warn(device, "iic_clckdiv: using compatibility value for OPB freq," " fix your board specific setup\n"); opb = 50000000; } @@ -652,7 +655,7 @@ static inline u8 iic_clckdiv(unsigned int opb) opb /= 1000000; if (opb < 20 || opb > 150){ - printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n", + dev_warn(device, "iic_clckdiv: invalid OPB clock frequency %u MHz\n", opb); opb = opb < 20 ? 20 : 150; } @@ -733,7 +736,7 @@ static int iic_probe(struct platform_device *ofdev) } } - dev->clckdiv = iic_clckdiv(*freq); + dev->clckdiv = iic_clckdiv(dev, *freq); dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv); /* Initialize IIC interface */ diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h index fdaa482..1efce5d 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.h +++ b/drivers/i2c/busses/i2c-ibm_iic.h @@ -25,8 +25,10 @@ #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,9 +46,8 @@ struct iic_regs { struct ibm_iic_private { struct i2c_adapter adap; - volatile struct iic_regs __iomem *vaddr; + struct iic_regs __iomem *vaddr; wait_queue_head_t wq; - int idx; int irq; int fast_mode; u8 clckdiv; -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1385110686-4226-2-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>]
* Re: [PATCH v2 1/4] i2c: i2c-ibm-iic: cleanup. [not found] ` <1385110686-4226-2-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> @ 2013-11-22 15:15 ` Gregory CLEMENT 0 siblings, 0 replies; 10+ messages in thread From: Gregory CLEMENT @ 2013-11-22 15:15 UTC (permalink / raw) To: jean-jacques hiblot, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot Hi Jean-Jacques, On 22/11/2013 09:58, jean-jacques hiblot wrote: > * removed unneeded 'volatile' qualifiers and casts > * use the dev_dbg, dev_err etc. instead of printk > * removed unneeded members for the driver's private data > > Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> This one was easy to review, and I didn't find any thing to say, so you can add my: Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Thanks, Gregory > --- > drivers/i2c/busses/i2c-ibm_iic.c | 109 ++++++++++++++++++++------------------- > drivers/i2c/busses/i2c-ibm_iic.h | 9 ++-- > 2 files changed, 61 insertions(+), 57 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c > index ff3caa0..9cdef65 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.c > +++ b/drivers/i2c/busses/i2c-ibm_iic.c > @@ -69,21 +69,23 @@ MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)"); > #endif > > #if DBG_LEVEL > 0 > -# define DBG(f,x...) printk(KERN_DEBUG "ibm-iic" f, ##x) > +# define DBG(dev, f, x...) dev_dbg(dev->adap.dev.parent, f, ##x) > #else > -# define DBG(f,x...) ((void)0) > +# define DBG(dev, f, x...) ((void)0) > #endif > #if DBG_LEVEL > 1 > -# define DBG2(f,x...) DBG(f, ##x) > +# define DBG2(dev, f, x...) DBG(dev, f, ##x) > #else > -# define DBG2(f,x...) ((void)0) > +# define DBG2(dev, f, x...) ((void)0) > #endif > #if DBG_LEVEL > 2 > static void dump_iic_regs(const char* header, struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > - printk(KERN_DEBUG "ibm-iic%d: %s\n", dev->idx, header); > - printk(KERN_DEBUG > + struct iic_regs __iomem *iic = dev->vaddr; > + struct device *device = dev.adap.dev.parent; > + > + dev_dbg(device, ": %s\n", header); > + dev_dbg(device, > " cntl = 0x%02x, mdcntl = 0x%02x\n" > " sts = 0x%02x, extsts = 0x%02x\n" > " clkdiv = 0x%02x, xfrcnt = 0x%02x\n" > @@ -133,9 +135,9 @@ static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable) > */ > static void iic_dev_init(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > > - DBG("%d: init\n", dev->idx); > + DBG(dev, "init\n"); > > /* Clear master address */ > out_8(&iic->lmadr, 0); > @@ -178,11 +180,11 @@ static void iic_dev_init(struct ibm_iic_private* dev) > */ > static void iic_dev_reset(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int i; > u8 dc; > > - DBG("%d: soft reset\n", dev->idx); > + DBG(dev, "soft reset\n"); > DUMP_REGS("reset", dev); > > /* Place chip in the reset state */ > @@ -191,7 +193,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) > /* Check if bus is free */ > dc = in_8(&iic->directcntl); > if (!DIRCTNL_FREE(dc)){ > - DBG("%d: trying to regain bus control\n", dev->idx); > + DBG(dev, "trying to regain bus control\n"); > > /* Try to set bus free state */ > out_8(&iic->directcntl, DIRCNTL_SDAC | DIRCNTL_SCC); > @@ -226,7 +228,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) > */ > > /* Wait for SCL and/or SDA to be high */ > -static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask) > +static int iic_dc_wait(struct iic_regs __iomem *iic, u8 mask) > { > unsigned long x = jiffies + HZ / 28 + 2; > while ((in_8(&iic->directcntl) & mask) != mask){ > @@ -239,19 +241,18 @@ static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask) > > static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > const struct i2c_timings* t = &timings[dev->fast_mode ? 1 : 0]; > u8 mask, v, sda; > int i, res; > > /* Only 7-bit addresses are supported */ > if (unlikely(p->flags & I2C_M_TEN)){ > - DBG("%d: smbus_quick - 10 bit addresses are not supported\n", > - dev->idx); > + DBG(dev, "smbus_quick - 10 bit addresses are not supported\n"); > return -EINVAL; > } > > - DBG("%d: smbus_quick(0x%02x)\n", dev->idx, p->addr); > + DBG(dev, "smbus_quick(0x%02x)\n", p->addr); > > /* Reset IIC interface */ > out_8(&iic->xtcntlss, XTCNTLSS_SRST); > @@ -304,7 +305,7 @@ static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p) > > ndelay(t->buf); > > - DBG("%d: smbus_quick -> %s\n", dev->idx, res ? "NACK" : "ACK"); > + DBG(dev, "smbus_quick -> %s\n", res ? "NACK" : "ACK"); > out: > /* Remove reset */ > out_8(&iic->xtcntlss, 0); > @@ -314,7 +315,7 @@ out: > > return res; > err: > - DBG("%d: smbus_quick - bus is stuck\n", dev->idx); > + DBG(dev, "smbus_quick - bus is stuck\n"); > res = -EREMOTEIO; > goto out; > } > @@ -325,10 +326,10 @@ err: > static irqreturn_t iic_handler(int irq, void *dev_id) > { > struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id; > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + 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)); > + 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); > @@ -343,10 +344,10 @@ static irqreturn_t iic_handler(int irq, void *dev_id) > */ > static int iic_xfer_result(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > > if (unlikely(in_8(&iic->sts) & STS_ERR)){ > - DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx, > + DBG(dev, "xfer error, EXTSTS = 0x%02x\n", > in_8(&iic->extsts)); > > /* Clear errors and possible pending IRQs */ > @@ -362,7 +363,7 @@ static int iic_xfer_result(struct ibm_iic_private* dev) > * 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); > + DBG(dev, "bus is stuck, resetting\n"); > iic_dev_reset(dev); > } > return -EREMOTEIO; > @@ -376,10 +377,10 @@ static int iic_xfer_result(struct ibm_iic_private* dev) > */ > static void iic_abort_xfer(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > unsigned long x; > > - DBG("%d: iic_abort_xfer\n", dev->idx); > + DBG(dev, "iic_abort_xfer\n"); > > out_8(&iic->cntl, CNTL_HMT); > > @@ -390,7 +391,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) > 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); > + DBG(dev, "abort timeout, resetting...\n"); > iic_dev_reset(dev); > return; > } > @@ -408,7 +409,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) > */ > static int iic_wait_for_tc(struct ibm_iic_private* dev){ > > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int ret = 0; > > if (dev->irq >= 0){ > @@ -417,9 +418,9 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > !(in_8(&iic->sts) & STS_PT), dev->adap.timeout); > > if (unlikely(ret < 0)) > - DBG("%d: wait interrupted\n", dev->idx); > + DBG(dev, "wait interrupted\n"); > else if (unlikely(in_8(&iic->sts) & STS_PT)){ > - DBG("%d: wait timeout\n", dev->idx); > + DBG(dev, "wait timeout\n"); > ret = -ETIMEDOUT; > } > } > @@ -429,13 +430,13 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > > while (in_8(&iic->sts) & STS_PT){ > if (unlikely(time_after(jiffies, x))){ > - DBG("%d: poll timeout\n", dev->idx); > + DBG(dev, "poll timeout\n"); > ret = -ETIMEDOUT; > break; > } > > if (unlikely(signal_pending(current))){ > - DBG("%d: poll interrupted\n", dev->idx); > + DBG(dev, "poll interrupted\n"); > ret = -ERESTARTSYS; > break; > } > @@ -448,7 +449,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > else > ret = iic_xfer_result(dev); > > - DBG2("%d: iic_wait_for_tc -> %d\n", dev->idx, ret); > + DBG2(dev, "iic_wait_for_tc -> %d\n", ret); > > return ret; > } > @@ -459,7 +460,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > int combined_xfer) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > char* buf = pm->buf; > int i, j, loops, ret = 0; > int len = pm->len; > @@ -475,14 +476,14 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > > if (!(cntl & CNTL_RW)) > for (j = 0; j < count; ++j) > - out_8((void __iomem *)&iic->mdbuf, *buf++); > + out_8(&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); > + DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd); > > /* Start transfer */ > out_8(&iic->cntl, cmd); > @@ -493,8 +494,8 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > if (unlikely(ret < 0)) > break; > else if (unlikely(ret != count)){ > - DBG("%d: xfer_bytes, requested %d, transferred %d\n", > - dev->idx, count, ret); > + DBG(dev, "xfer_bytes, requested %d, transferred %d\n", > + count, ret); > > /* If it's not a last part of xfer, abort it */ > if (combined_xfer || (i < loops - 1)) > @@ -506,7 +507,7 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > > if (cntl & CNTL_RW) > for (j = 0; j < count; ++j) > - *buf++ = in_8((void __iomem *)&iic->mdbuf); > + *buf++ = in_8(&iic->mdbuf); > } > > return ret > 0 ? 0 : ret; > @@ -517,10 +518,10 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > */ > static inline void iic_address(struct ibm_iic_private* dev, struct i2c_msg* msg) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > u16 addr = msg->addr; > > - DBG2("%d: iic_address, 0x%03x (%d-bit)\n", dev->idx, > + DBG2(dev, "iic_address, 0x%03x (%d-bit)\n", > addr, msg->flags & I2C_M_TEN ? 10 : 7); > > if (msg->flags & I2C_M_TEN){ > @@ -553,10 +554,10 @@ static inline int iic_address_neq(const struct i2c_msg* p1, > static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > { > struct ibm_iic_private* dev = (struct ibm_iic_private*)(i2c_get_adapdata(adap)); > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int i, ret = 0; > > - DBG2("%d: iic_xfer, %d msg(s)\n", dev->idx, num); > + DBG2(dev, "iic_xfer, %d msg(s)\n", num); > > if (!num) > return 0; > @@ -565,7 +566,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > * Uhh, generic i2c layer is more suitable place for such code... > */ > if (unlikely(iic_invalid_address(&msgs[0]))){ > - DBG("%d: invalid address 0x%03x (%d-bit)\n", dev->idx, > + DBG(dev, "invalid address 0x%03x (%d-bit)\n", > msgs[0].addr, msgs[0].flags & I2C_M_TEN ? 10 : 7); > return -EINVAL; > } > @@ -578,19 +579,19 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > */ > return iic_smbus_quick(dev, &msgs[0]); > } > - DBG("%d: invalid len %d in msg[%d]\n", dev->idx, > + DBG(dev, "invalid len %d in msg[%d]\n", > msgs[i].len, i); > return -EINVAL; > } > if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))){ > - DBG("%d: invalid addr in msg[%d]\n", dev->idx, i); > + DBG(dev, "invalid addr in msg[%d]\n", i); > return -EINVAL; > } > } > > /* Check bus state */ > if (unlikely((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE)){ > - DBG("%d: iic_xfer, bus is not free\n", dev->idx); > + DBG(dev, "iic_xfer, bus is not free\n"); > > /* Usually it means something serious has happened. > * We *cannot* have unfinished previous transfer > @@ -603,7 +604,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > iic_dev_reset(dev); > > if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ > - DBG("%d: iic_xfer, bus is still not free\n", dev->idx); > + DBG(dev, "iic_xfer, bus is still not free\n"); > return -EREMOTEIO; > } > } > @@ -635,15 +636,17 @@ static const struct i2c_algorithm iic_algo = { > /* > * Calculates IICx_CLCKDIV value for a specific OPB clock frequency > */ > -static inline u8 iic_clckdiv(unsigned int opb) > +static inline u8 iic_clckdiv(struct ibm_iic_private *dev, unsigned int opb) > { > + struct device *device = dev->adap.dev.parent; > + > /* Compatibility kludge, should go away after all cards > * are fixed to fill correct value for opbfreq. > * Previous driver version used hardcoded divider value 4, > * it corresponds to OPB frequency from the range (40, 50] MHz > */ > if (!opb){ > - printk(KERN_WARNING "ibm-iic: using compatibility value for OPB freq," > + dev_warn(device, "iic_clckdiv: using compatibility value for OPB freq," > " fix your board specific setup\n"); > opb = 50000000; > } > @@ -652,7 +655,7 @@ static inline u8 iic_clckdiv(unsigned int opb) > opb /= 1000000; > > if (opb < 20 || opb > 150){ > - printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n", > + dev_warn(device, "iic_clckdiv: invalid OPB clock frequency %u MHz\n", > opb); > opb = opb < 20 ? 20 : 150; > } > @@ -733,7 +736,7 @@ static int iic_probe(struct platform_device *ofdev) > } > } > > - dev->clckdiv = iic_clckdiv(*freq); > + dev->clckdiv = iic_clckdiv(dev, *freq); > dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv); > > /* Initialize IIC interface */ > diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h > index fdaa482..1efce5d 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.h > +++ b/drivers/i2c/busses/i2c-ibm_iic.h > @@ -25,8 +25,10 @@ > #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,9 +46,8 @@ struct iic_regs { > > struct ibm_iic_private { > struct i2c_adapter adap; > - volatile struct iic_regs __iomem *vaddr; > + struct iic_regs __iomem *vaddr; > wait_queue_head_t wq; > - int idx; > int irq; > int fast_mode; > u8 clckdiv; > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler [not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 2013-11-22 8:58 ` [PATCH v2 1/4] i2c: i2c-ibm-iic: cleanup jean-jacques hiblot @ 2013-11-22 8:58 ` jean-jacques hiblot [not found] ` <1385110686-4226-3-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 2013-11-22 8:58 ` [PATCH v2 3/4] i2c: i2c-ibm-iic: Implements transfer abortion jean-jacques hiblot 2013-11-22 8:58 ` [PATCH v2 4/4] i2c: i2c-ibm-iic: Implements a polling mode jean-jacques hiblot 3 siblings, 1 reply; 10+ messages in thread From: jean-jacques hiblot @ 2013-11-22 8:58 UTC (permalink / raw) To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot, jean-jacques hiblot 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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) + #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 ? + */ + 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; + 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"); 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); /* 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"); /* 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 */ -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1385110686-4226-3-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>]
* Re: [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler [not found] ` <1385110686-4226-3-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> @ 2013-11-22 15:16 ` Gregory CLEMENT [not found] ` <528F7547.2070308-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Gregory CLEMENT @ 2013-11-22 15:16 UTC (permalink / raw) To: jean-jacques hiblot, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <528F7547.2070308-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler [not found] ` <528F7547.2070308-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2013-11-25 8:24 ` Jean-Jacques Hiblot 0 siblings, 0 replies; 10+ messages in thread From: Jean-Jacques Hiblot @ 2013-11-25 8:24 UTC (permalink / raw) To: Gregory CLEMENT, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot Hi Gregory, Thank you for the review. Le 22/11/2013 16:16, Gregory CLEMENT a écrit : > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > > 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 > I'll take those comments in account for a 3rd version of the patches. > >> --- >> 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] i2c: i2c-ibm-iic: Implements transfer abortion [not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 2013-11-22 8:58 ` [PATCH v2 1/4] i2c: i2c-ibm-iic: cleanup jean-jacques hiblot 2013-11-22 8:58 ` [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler jean-jacques hiblot @ 2013-11-22 8:58 ` jean-jacques hiblot [not found] ` <1385110686-4226-4-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 2013-11-22 8:58 ` [PATCH v2 4/4] i2c: i2c-ibm-iic: Implements a polling mode jean-jacques hiblot 3 siblings, 1 reply; 10+ messages in thread From: jean-jacques hiblot @ 2013-11-22 8:58 UTC (permalink / raw) To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot, jean-jacques hiblot Clean-up properly when a transfer fails for whatever reason. Cancel the transfer when the process is signaled. Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- drivers/i2c/busses/i2c-ibm_iic.c | 144 +++++++++------------------------------ drivers/i2c/busses/i2c-ibm_iic.h | 2 +- 2 files changed, 35 insertions(+), 111 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index 2bb55b3..a3f3f1b 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -334,119 +334,25 @@ static irqreturn_t iic_handler(int irq, void *dev_id) } /* - * 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) -{ - struct iic_regs __iomem *iic = dev->vaddr; - - if (unlikely(in_8(&iic->sts) & STS_ERR)){ - DBG(dev, "xfer error, EXTSTS = 0x%02x\n", - in_8(&iic->extsts)); - - /* Clear errors and possible pending IRQs */ - out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | - EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA); - - /* Flush master data buffer */ - out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB); - - /* 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(dev, "bus is stuck, resetting\n"); - iic_dev_reset(dev); - } - 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) +static void iic_abort_xfer(struct ibm_iic_private *dev) { - struct iic_regs __iomem *iic = dev->vaddr; - unsigned long x; - - DBG(dev, "iic_abort_xfer\n"); + struct device *device = dev->adap.dev.parent; + unsigned long end; - out_8(&iic->cntl, CNTL_HMT); + DBG(dev, "aborting transfer\n"); + /* transfer should be aborted within 10ms */ + end = jiffies + 10; + dev->abort = 1; - /* - * 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(dev, "abort timeout, resetting...\n"); - iic_dev_reset(dev); - return; - } + while (time_after(end, jiffies) && !dev->transfer_complete) schedule(); - } - /* Just to clear errors */ - iic_xfer_result(dev); -} - -/* - * 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) - */ -static int iic_wait_for_tc(struct ibm_iic_private* dev){ - - 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(dev, "wait interrupted\n"); - else if (unlikely(in_8(&iic->sts) & STS_PT)){ - DBG(dev, "wait timeout\n"); - 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(dev, "poll timeout\n"); - ret = -ETIMEDOUT; - break; - } - - if (unlikely(signal_pending(current))){ - DBG(dev, "poll interrupted\n"); - ret = -ERESTARTSYS; - break; - } - schedule(); - } + if (!dev->transfer_complete) { + dev_err(device, "abort operation failed\n"); + iic_dev_reset(dev); } - - if (unlikely(ret < 0)) - iic_abort_xfer(dev); - else - ret = iic_xfer_result(dev); - - DBG2(dev, "iic_wait_for_tc -> %d\n", ret); - - return ret; } /* @@ -470,6 +376,13 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) EXTSTS_ICT | EXTSTS_XFRA); out_8(&iic->sts, STS_IRQA | STS_SCMP); + if (dev->status == -ECANCELED) { + DBG(dev, "abort completed\n"); + dev->transfer_complete = 1; + complete(&dev->iic_compl); + return dev->status; + } + if ((status & STS_ERR) || (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) { DBG(dev, "status 0x%x\n", status); @@ -571,7 +484,14 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) /* actually start the transfer of the current data chunk */ out_8(&iic->cntl, cntl | CNTL_PT); - return 0; + /* The transfer must be aborted. */ + if (dev->abort) { + DBG(dev, "aborting\n"); + out_8(&iic->cntl, CNTL_HMT); + dev->status = -ECANCELED; + } + + return dev->status; } /* @@ -673,6 +593,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) dev->current_byte = dev->current_msg = dev->current_byte_rx = 0; dev->transfer_complete = 0; dev->status = 0; + dev->abort = 0; dev->msgs = msgs; dev->num_msgs = num; @@ -710,8 +631,9 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) /* 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); + /* we don't mask the interrupts here because we may + * need them to abort the transfer gracefully + */ } if (ret == 0) { @@ -720,11 +642,15 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) } else if (ret < 0) { if (ret == -ERESTARTSYS) ERR(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; } @@ -821,8 +747,6 @@ static int iic_probe(struct platform_device *ofdev) goto error_cleanup; } - init_waitqueue_head(&dev->wq); - dev->irq = iic_request_irq(ofdev, dev); if (!dev->irq) dev_info(&ofdev->dev, "using polling mode\n"); diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h index 76c476a..0ee28a9 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.h +++ b/drivers/i2c/busses/i2c-ibm_iic.h @@ -47,7 +47,6 @@ struct iic_regs { struct ibm_iic_private { struct i2c_adapter adap; struct iic_regs __iomem *vaddr; - wait_queue_head_t wq; int irq; int fast_mode; u8 clckdiv; @@ -58,6 +57,7 @@ struct ibm_iic_private { int current_byte_rx; int transfer_complete; int status; + int abort; struct completion iic_compl; }; -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1385110686-4226-4-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>]
* Re: [PATCH v2 3/4] i2c: i2c-ibm-iic: Implements transfer abortion [not found] ` <1385110686-4226-4-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> @ 2013-11-29 21:38 ` Gregory CLEMENT 0 siblings, 0 replies; 10+ messages in thread From: Gregory CLEMENT @ 2013-11-29 21:38 UTC (permalink / raw) To: jean-jacques hiblot, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot Hi Jean-Jacques, I have just found these emails in may draft box, and I thought I have already sent them. However I didn't find anything relevant about the driver itslef, but only few nitpick. So either your change are goods, or I missed the important points. The final judgment belongs ton Wolfram :) On 22/11/2013 09:58, jean-jacques hiblot wrote: > Clean-up properly when a transfer fails for whatever reason. > Cancel the transfer when the process is signaled. > > Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > drivers/i2c/busses/i2c-ibm_iic.c | 144 +++++++++------------------------------ > drivers/i2c/busses/i2c-ibm_iic.h | 2 +- > 2 files changed, 35 insertions(+), 111 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c > index 2bb55b3..a3f3f1b 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.c > +++ b/drivers/i2c/busses/i2c-ibm_iic.c > @@ -334,119 +334,25 @@ static irqreturn_t iic_handler(int irq, void *dev_id) > } > > /* > - * 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) > -{ > - struct iic_regs __iomem *iic = dev->vaddr; > - > - if (unlikely(in_8(&iic->sts) & STS_ERR)){ > - DBG(dev, "xfer error, EXTSTS = 0x%02x\n", > - in_8(&iic->extsts)); > - > - /* Clear errors and possible pending IRQs */ > - out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD | > - EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA); > - > - /* Flush master data buffer */ > - out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB); > - > - /* 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(dev, "bus is stuck, resetting\n"); > - iic_dev_reset(dev); > - } > - 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) > +static void iic_abort_xfer(struct ibm_iic_private *dev) > { > - struct iic_regs __iomem *iic = dev->vaddr; > - unsigned long x; > - > - DBG(dev, "iic_abort_xfer\n"); > + struct device *device = dev->adap.dev.parent; > + unsigned long end; > > - out_8(&iic->cntl, CNTL_HMT); > + DBG(dev, "aborting transfer\n"); > + /* transfer should be aborted within 10ms */ > + end = jiffies + 10; > + dev->abort = 1; > > - /* > - * 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(dev, "abort timeout, resetting...\n"); > - iic_dev_reset(dev); > - return; > - } > + while (time_after(end, jiffies) && !dev->transfer_complete) > schedule(); > - } > > - /* Just to clear errors */ > - iic_xfer_result(dev); > -} > - > -/* > - * 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) > - */ > -static int iic_wait_for_tc(struct ibm_iic_private* dev){ > - > - 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(dev, "wait interrupted\n"); > - else if (unlikely(in_8(&iic->sts) & STS_PT)){ > - DBG(dev, "wait timeout\n"); > - 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(dev, "poll timeout\n"); > - ret = -ETIMEDOUT; > - break; > - } > - > - if (unlikely(signal_pending(current))){ > - DBG(dev, "poll interrupted\n"); > - ret = -ERESTARTSYS; > - break; > - } > - schedule(); > - } > + if (!dev->transfer_complete) { > + dev_err(device, "abort operation failed\n"); What about using the ERR macro you introduce in the previous patch? > + iic_dev_reset(dev); > } > - > - if (unlikely(ret < 0)) > - iic_abort_xfer(dev); > - else > - ret = iic_xfer_result(dev); > - > - DBG2(dev, "iic_wait_for_tc -> %d\n", ret); > - > - return ret; > } > > /* > @@ -470,6 +376,13 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) > EXTSTS_ICT | EXTSTS_XFRA); > out_8(&iic->sts, STS_IRQA | STS_SCMP); > > + if (dev->status == -ECANCELED) { > + DBG(dev, "abort completed\n"); > + dev->transfer_complete = 1; > + complete(&dev->iic_compl); > + return dev->status; > + } > + > if ((status & STS_ERR) || > (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) { > DBG(dev, "status 0x%x\n", status); > @@ -571,7 +484,14 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) > /* actually start the transfer of the current data chunk */ > out_8(&iic->cntl, cntl | CNTL_PT); > > - return 0; > + /* The transfer must be aborted. */ > + if (dev->abort) { > + DBG(dev, "aborting\n"); > + out_8(&iic->cntl, CNTL_HMT); > + dev->status = -ECANCELED; > + } > + > + return dev->status; > } > > /* > @@ -673,6 +593,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > dev->current_byte = dev->current_msg = dev->current_byte_rx = 0; > dev->transfer_complete = 0; > dev->status = 0; > + dev->abort = 0; > dev->msgs = msgs; > dev->num_msgs = num; > > @@ -710,8 +631,9 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > /* 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); > + /* we don't mask the interrupts here because we may > + * need them to abort the transfer gracefully > + */ nitpick:wrong multiline comment style > } > > if (ret == 0) { > @@ -720,11 +642,15 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > } else if (ret < 0) { > if (ret == -ERESTARTSYS) > ERR(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; > } > > @@ -821,8 +747,6 @@ static int iic_probe(struct platform_device *ofdev) > goto error_cleanup; > } > > - init_waitqueue_head(&dev->wq); > - > dev->irq = iic_request_irq(ofdev, dev); > if (!dev->irq) > dev_info(&ofdev->dev, "using polling mode\n"); > diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h > index 76c476a..0ee28a9 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.h > +++ b/drivers/i2c/busses/i2c-ibm_iic.h > @@ -47,7 +47,6 @@ struct iic_regs { > struct ibm_iic_private { > struct i2c_adapter adap; > struct iic_regs __iomem *vaddr; > - wait_queue_head_t wq; > int irq; > int fast_mode; > u8 clckdiv; > @@ -58,6 +57,7 @@ struct ibm_iic_private { > int current_byte_rx; > int transfer_complete; > int status; > + int abort; > struct completion iic_compl; > }; > > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] i2c: i2c-ibm-iic: Implements a polling mode [not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> ` (2 preceding siblings ...) 2013-11-22 8:58 ` [PATCH v2 3/4] i2c: i2c-ibm-iic: Implements transfer abortion jean-jacques hiblot @ 2013-11-22 8:58 ` jean-jacques hiblot [not found] ` <1385110686-4226-5-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> 3 siblings, 1 reply; 10+ messages in thread From: jean-jacques hiblot @ 2013-11-22 8:58 UTC (permalink / raw) To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot, jean-jacques hiblot When no valid interrupt is defined for the controller, use polling to handle the transfers. The polling mode can also be forced with the "iic_force_poll" module parameter. Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- drivers/i2c/busses/i2c-ibm_iic.c | 89 ++++++++++++++++++++++++++++++++-------- drivers/i2c/busses/i2c-ibm_iic.h | 1 + 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c index a3f3f1b..1dde6e1 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.c +++ b/drivers/i2c/busses/i2c-ibm_iic.c @@ -334,11 +334,45 @@ static irqreturn_t iic_handler(int irq, void *dev_id) } /* + * Polling used when interrupt can't be used + */ +static int poll_for_end_of_transfer(struct ibm_iic_private *dev, u32 timeout) +{ + struct iic_regs __iomem *iic = dev->vaddr; + 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, + * 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 (signal_pending(current)) + return -ERESTARTSYS; + + if (dev->transfer_complete == 0) + return 0; + + return 1; +} + +/* * Try to abort active transfer. */ static void iic_abort_xfer(struct ibm_iic_private *dev) { struct device *device = dev->adap.dev.parent; + struct iic_regs __iomem *iic = dev->vaddr; unsigned long end; DBG(dev, "aborting transfer\n"); @@ -346,8 +380,17 @@ static void iic_abort_xfer(struct ibm_iic_private *dev) end = jiffies + 10; dev->abort = 1; - while (time_after(end, jiffies) && !dev->transfer_complete) - schedule(); + 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 (dev->transfer_complete == 0) + schedule(); + } if (!dev->transfer_complete) { dev_err(device, "abort operation failed\n"); @@ -379,7 +422,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) if (dev->status == -ECANCELED) { DBG(dev, "abort completed\n"); dev->transfer_complete = 1; - complete(&dev->iic_compl); + if (!dev->use_polling) + complete(&dev->iic_compl); return dev->status; } @@ -398,7 +442,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) dev->status = -EIO; dev->transfer_complete = 1; - complete(&dev->iic_compl); + if (!dev->use_polling) + complete(&dev->iic_compl); return dev->status; } @@ -426,7 +471,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) if (dev->current_msg == dev->num_msgs) { DBG2(dev, "end of transfer\n"); dev->transfer_complete = 1; - complete(&dev->iic_compl); + if (!dev->use_polling) + complete(&dev->iic_compl); return dev->status; } pm++; @@ -617,23 +663,28 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) /* Load slave address */ iic_address(dev, &msgs[0]); - init_completion(&dev->iic_compl); + if (!dev->use_polling) + 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); - /* we don't mask the interrupts here because we may - * need them to abort the transfer gracefully - */ + 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 + */ + } } if (ret == 0) { @@ -699,6 +750,8 @@ static int iic_request_irq(struct platform_device *ofdev, struct device_node *np = ofdev->dev.of_node; int irq; + dev->use_polling = 1; + if (iic_force_poll) return 0; @@ -718,6 +771,8 @@ static int iic_request_irq(struct platform_device *ofdev, return 0; } + dev->use_polling = 0; + return irq; } diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h index 0ee28a9..523cfc1 100644 --- a/drivers/i2c/busses/i2c-ibm_iic.h +++ b/drivers/i2c/busses/i2c-ibm_iic.h @@ -58,6 +58,7 @@ struct ibm_iic_private { int transfer_complete; int status; int abort; + int use_polling; struct completion iic_compl; }; -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <1385110686-4226-5-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>]
* Re: [PATCH v2 4/4] i2c: i2c-ibm-iic: Implements a polling mode [not found] ` <1385110686-4226-5-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org> @ 2013-11-29 21:39 ` Gregory CLEMENT 0 siblings, 0 replies; 10+ messages in thread From: Gregory CLEMENT @ 2013-11-29 21:39 UTC (permalink / raw) To: jean-jacques hiblot, linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: jean-jacques hiblot On 22/11/2013 09:58, jean-jacques hiblot wrote: > When no valid interrupt is defined for the controller, use polling to handle > the transfers. > The polling mode can also be forced with the "iic_force_poll" module parameter. > > Signed-off-by: jean-jacques hiblot <jjhiblot-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > drivers/i2c/busses/i2c-ibm_iic.c | 89 ++++++++++++++++++++++++++++++++-------- > drivers/i2c/busses/i2c-ibm_iic.h | 1 + > 2 files changed, 73 insertions(+), 17 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c > index a3f3f1b..1dde6e1 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.c > +++ b/drivers/i2c/busses/i2c-ibm_iic.c > @@ -334,11 +334,45 @@ static irqreturn_t iic_handler(int irq, void *dev_id) > } > > /* > + * Polling used when interrupt can't be used > + */ > +static int poll_for_end_of_transfer(struct ibm_iic_private *dev, u32 timeout) > +{ > + struct iic_regs __iomem *iic = dev->vaddr; > + 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 */ occurred > + if ((status & (STS_ERR | STS_SCMP)) || !(status & STS_PT)) > + iic_xfer_bytes(dev); > + /* The transfer is not complete, > + * calling schedule relaxes the CPU load and allows to know > + * if the process is being signaled (for abortion) > + */ nitpick: wrong muliline comment style > + if (dev->transfer_complete == 0) > + schedule(); > + } > + > + if (signal_pending(current)) > + return -ERESTARTSYS; > + > + if (dev->transfer_complete == 0) > + return 0; > + > + return 1; > +} > + > +/* > * Try to abort active transfer. > */ > static void iic_abort_xfer(struct ibm_iic_private *dev) > { > struct device *device = dev->adap.dev.parent; > + struct iic_regs __iomem *iic = dev->vaddr; > unsigned long end; > > DBG(dev, "aborting transfer\n"); > @@ -346,8 +380,17 @@ static void iic_abort_xfer(struct ibm_iic_private *dev) > end = jiffies + 10; > dev->abort = 1; > > - while (time_after(end, jiffies) && !dev->transfer_complete) > - schedule(); > + 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 (dev->transfer_complete == 0) > + schedule(); > + } > > if (!dev->transfer_complete) { > dev_err(device, "abort operation failed\n"); > @@ -379,7 +422,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) > if (dev->status == -ECANCELED) { > DBG(dev, "abort completed\n"); > dev->transfer_complete = 1; > - complete(&dev->iic_compl); > + if (!dev->use_polling) > + complete(&dev->iic_compl); > return dev->status; > } > > @@ -398,7 +442,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) > > dev->status = -EIO; > dev->transfer_complete = 1; > - complete(&dev->iic_compl); > + if (!dev->use_polling) > + complete(&dev->iic_compl); > return dev->status; > } > > @@ -426,7 +471,8 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev) > if (dev->current_msg == dev->num_msgs) { > DBG2(dev, "end of transfer\n"); > dev->transfer_complete = 1; > - complete(&dev->iic_compl); > + if (!dev->use_polling) > + complete(&dev->iic_compl); > return dev->status; > } > pm++; > @@ -617,23 +663,28 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > /* Load slave address */ > iic_address(dev, &msgs[0]); > > - init_completion(&dev->iic_compl); > + if (!dev->use_polling) > + 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); > - /* we don't mask the interrupts here because we may > - * need them to abort the transfer gracefully > - */ > + 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 > + */ > + } > } > > if (ret == 0) { > @@ -699,6 +750,8 @@ static int iic_request_irq(struct platform_device *ofdev, > struct device_node *np = ofdev->dev.of_node; > int irq; > > + dev->use_polling = 1; > + > if (iic_force_poll) > return 0; > > @@ -718,6 +771,8 @@ static int iic_request_irq(struct platform_device *ofdev, > return 0; > } > > + dev->use_polling = 0; > + > return irq; > } > > diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h > index 0ee28a9..523cfc1 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.h > +++ b/drivers/i2c/busses/i2c-ibm_iic.h > @@ -58,6 +58,7 @@ struct ibm_iic_private { > int transfer_complete; > int status; > int abort; > + int use_polling; > struct completion iic_compl; > }; > > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-29 21:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-22 8:58 [PATCH v2 0/4] i2c : i2c-ibm-iic : use interrupts to perform the data transfer jean-jacques hiblot
[not found] ` <1385110686-4226-1-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
2013-11-22 8:58 ` [PATCH v2 1/4] i2c: i2c-ibm-iic: cleanup jean-jacques hiblot
[not found] ` <1385110686-4226-2-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
2013-11-22 15:15 ` Gregory CLEMENT
2013-11-22 8:58 ` [PATCH v2 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler jean-jacques hiblot
[not found] ` <1385110686-4226-3-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
2013-11-22 15:16 ` Gregory CLEMENT
[not found] ` <528F7547.2070308-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-11-25 8:24 ` Jean-Jacques Hiblot
2013-11-22 8:58 ` [PATCH v2 3/4] i2c: i2c-ibm-iic: Implements transfer abortion jean-jacques hiblot
[not found] ` <1385110686-4226-4-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
2013-11-29 21:38 ` Gregory CLEMENT
2013-11-22 8:58 ` [PATCH v2 4/4] i2c: i2c-ibm-iic: Implements a polling mode jean-jacques hiblot
[not found] ` <1385110686-4226-5-git-send-email-jean-jacques.hiblot-qU79poH1Kb0@public.gmane.org>
2013-11-29 21:39 ` Gregory CLEMENT
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.