RE: [PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC
Ben Gardiner wrote: > In "i2c-davinci: Fix race when setting up for TX" and "i2c-davinci: > Fix TX setup for more SoCs" you mention testing on DM355 -- is there > any you have hardware on which the recovery procedure is executed on > occasion and that you would be available to test modifications to the > current implementation? Nope, sorry. I've never seen the bus lock up. -- Jon Povey jon.po...@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC
Ben Gardiner wrote: >>> When creating this series I noticed that there are obvious >>> similarities between the existing recovery routine implemented by >>> Philby John and John Povey > I'm not sure how the 20us in the existing method was derived -- I > wonder if Philby John or John Povey could comment? I've been a little bemused about why I am getting credited for I2C bus recovery work. I don't remember doing any work on that. I did a couple of patches to fix a race when setting up a TX, but those are, afaik, unrelated. All I know about the bus recovery stuff is looking at it a while back and thinking hmm, that seems to wiggle gpio without changing the pinmuxing, so it can't possibly work. -- Jon Povey jon.po...@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4] i2c: davinci: Fix race when setting up for TX
Kevin Hilman wrote: > I just noticed that it has already been pulled and is part of -rc7. > > Jon, care to submit a new patch with v4 diff and including > the acks and > tested-bys? Done and sent: Message-Id: <1286858825-21540-1-git-send-email-jon.po...@racelogic.co.uk> -- Jon Povey jon.po...@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: davinci: Fix TX setup for more SoCs
This patch is an improvement to 4bba0fd8d1c6d405df666e2573e1a1f917098be0
which got to mainline a little early.
Sudhakar Rajashekhara explains that at least OMAP-L138 requires MDR mode
settings before DXR for correct behaviour, so load MDR first with
STT cleared and later load again with STT set.
Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
Signed-off-by: Jon Povey
Acked-by: Troy Kisky
Tested-by: Sudhakar Rajashekhara
Acked-by: Kevin Hilman
---
This patches to what was v4 of the original patch.
The original patch which made it to 2.6.36-rc7 will as I understand it have
introduced a regression for OMAP-L138 so this patch is a regression fix and
wants to get into 2.6.36.
drivers/i2c/busses/i2c-davinci.c | 24 +++-
1 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8feac5..5795c83 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -331,21 +331,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
INIT_COMPLETION(dev->cmd_complete);
dev->cmd_err = 0;
- /* Take I2C out of reset, configure it as master and set the
-* start bit */
- flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST | DAVINCI_I2C_MDR_STT;
+ /* Take I2C out of reset and configure it as master */
+ flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST;
/* if the slave address is ten bit address, enable XA bit */
if (msg->flags & I2C_M_TEN)
flag |= DAVINCI_I2C_MDR_XA;
if (!(msg->flags & I2C_M_RD))
flag |= DAVINCI_I2C_MDR_TRX;
- if (stop)
- flag |= DAVINCI_I2C_MDR_STP;
- if (msg->len == 0) {
+ if (msg->len == 0)
flag |= DAVINCI_I2C_MDR_RM;
- flag &= ~DAVINCI_I2C_MDR_STP;
- }
/* Enable receive or transmit interrupts */
w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
@@ -358,17 +353,28 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->terminate = 0;
/*
+* Write mode register first as needed for correct behaviour
+* on OMAP-L138, but don't set STT yet to avoid a race with XRDY
+* occuring before we have loaded DXR
+*/
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+
+ /*
* First byte should be set here, not after interrupt,
* because transmit-data-ready interrupt can come before
* NACK-interrupt during sending of previous message and
* ICDXR may have wrong data
+* It also saves us one interrupt, slightly faster
*/
if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
dev->buf_len--;
}
- /* write the data into mode register; start transmitting */
+ /* Set STT to begin transmit now DXR is loaded */
+ flag |= DAVINCI_I2C_MDR_STT;
+ if (stop && msg->len != 0)
+ flag |= DAVINCI_I2C_MDR_STP;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4] i2c: davinci: Fix race when setting up for TX
Hi Ben, I am not on the i2c list but noticed this pull request: http://www.spinics.net/linux/lists/linux-i2c/msg04022.html I think you have the wrong (old) version of this patch in that branch, http://git.fluff.org/gitweb?p=bjdooks/linux.git;a=commitdiff;h=4bba0fd8d1c6d405df666e2573e1a1f917098be0 The correct v4 one from the start of this thread has more lines of patch and this commit message: >>>>> When setting up to transmit, a race exists between the ISR and >>>>> i2c_davinci_xfer_msg() trying to load the first byte and adjust >>>>> counters. This is mostly visible for transmits > 1 byte long. >>>>> >>>>> The hardware starts sending immediately that MDR.STT is set. IMR >>>>> trickery doesn't work because if we start sending, finish the >>>>> first byte and an XRDY event occurs before we load IMR to unmask >>>>> it, we never get an interrupt, and we timeout. >>>>> >>>>> Sudhakar Rajashekhara explains that at least OMAP-L138 requires >>>>> MDR mode settings before DXR for correct behaviour, so load MDR >>>>> first with STT cleared and later load again with STT set. >>>>> >>>>> Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985 >>>>> >>>>> Signed-off-by: Jon Povey >>>>> CC: Sudhakar Rajashekhara >>>>> CC: Troy Kisky It also has some more acks and a tested, via Kevin: > Acked-by: Troy Kisky > Tested-by: Sudhakar Rajashekhara > Acked-by: Kevin Hilman -- Jon Povey jon.po...@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] i2c: davinci: Fix race when setting up for TX
When setting up to transmit, a race exists between the ISR and
i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
This is mostly visible for transmits > 1 byte long.
The hardware starts sending immediately that MDR.STT is set. IMR trickery
doesn't work because if we start sending, finish the first byte and an
XRDY event occurs before we load IMR to unmask it, we never get an
interrupt, and we timeout.
Sudhakar Rajashekhara explains that at least OMAP-L138 requires MDR mode
settings before DXR for correct behaviour, so load MDR first with
STT cleared and later load again with STT set.
Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
Signed-off-by: Jon Povey
CC: Sudhakar Rajashekhara
CC: Troy Kisky
---
Reworked after comments by Troy and Sudhakar.
Looking at the datasheet it seemed like setting STP without STT early
might cause a stray STOP to be generated, so I moved it into the second
MDR load.
This passes a quick smoke test but I can't do much more testing right at
the moment. Sudhakar, your comments would be welcomed.
drivers/i2c/busses/i2c-davinci.c | 24 +++-
1 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c87..5795c83 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -331,21 +331,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
INIT_COMPLETION(dev->cmd_complete);
dev->cmd_err = 0;
- /* Take I2C out of reset, configure it as master and set the
-* start bit */
- flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST | DAVINCI_I2C_MDR_STT;
+ /* Take I2C out of reset and configure it as master */
+ flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST;
/* if the slave address is ten bit address, enable XA bit */
if (msg->flags & I2C_M_TEN)
flag |= DAVINCI_I2C_MDR_XA;
if (!(msg->flags & I2C_M_RD))
flag |= DAVINCI_I2C_MDR_TRX;
- if (stop)
- flag |= DAVINCI_I2C_MDR_STP;
- if (msg->len == 0) {
+ if (msg->len == 0)
flag |= DAVINCI_I2C_MDR_RM;
- flag &= ~DAVINCI_I2C_MDR_STP;
- }
/* Enable receive or transmit interrupts */
w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
@@ -357,7 +352,11 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->terminate = 0;
- /* write the data into mode register */
+ /*
+* Write mode register first as needed for correct behaviour
+* on OMAP-L138, but don't set STT yet to avoid a race with XRDY
+* occuring before we have loaded DXR
+*/
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
/*
@@ -365,12 +364,19 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
* because transmit-data-ready interrupt can come before
* NACK-interrupt during sending of previous message and
* ICDXR may have wrong data
+* It also saves us one interrupt, slightly faster
*/
if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
dev->buf_len--;
}
+ /* Set STT to begin transmit now DXR is loaded */
+ flag |= DAVINCI_I2C_MDR_STT;
+ if (stop && msg->len != 0)
+ flag |= DAVINCI_I2C_MDR_STP;
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
dev->adapter.timeout);
if (r == 0) {
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] i2c: davinci: Fix race when setting up for TX
Troy Kisky wrote:
> On 9/17/2010 6:09 AM, Sudhakar Rajashekhara wrote:
>> Hi,
Seems I didn't get Sudhakar's email.. I wonder why?
>> On Fri, Sep 17, 2010 at 08:32:11, Jon Povey wrote:
>>> Move the MDR load after DXR,IMR loads to avoid this race without
>>> locking.
>>
>> I remember I had some issues on OMAP-L138 with this fix, that's when
>> I reverted to configuring ICMDR before writing to DXR (Please see
>> here: https://patchwork.kernel.org/patch/75262/). I checked the BIOS
>> I2C driver code for OMAP-L138 and there also we are configuring MDR
>> before accessing DXR.
Ah :/
> How about killing the lines from commit
> c6c7c729a22bfeb8e63eafce48dbaeea20e68703
> ---
> /*
> * First byte should be set here, not after interrupt,
> * because transmit-data-ready interrupt can come before
> * NACK-interrupt during sending of previous message and
> * ICDXR may have wrong data
> */
> if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
> dev->buf_len--; }
> --
>
> and resetting the i2c upon a NAK interrupt (after the stop)
> to clear the bad fifo data?
I can't really comment on how valid that would be and can't easily test it.
However preloading DXR does save one interrupt on transmit so the whole
operation is faster and more efficient.
Maybe v1 of my patch, with locking, is the best option?
--
Jon Povey
jon.po...@racelogic.co.uk
Racelogic is a limited company registered in England. Registered number 2743719
.
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks,
MK18 1TB .
The information contained in this electronic mail transmission is intended by
Racelogic Ltd for the use of the named individual or entity to which it is
directed and may contain information that is confidential or privileged. If you
have received this electronic mail transmission in error, please delete it from
your system without copying or forwarding it, and notify the sender of the
error by reply email so that the sender's address records can be corrected. The
views expressed by the sender of this communication do not necessarily
represent those of Racelogic Ltd. Please note that Racelogic reserves the right
to monitor e-mail communications passing through its network
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] i2c: davinci: Fix race when setting up for TX
When setting up to transmit, a race exists between the ISR and
i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
This is mostly visible for transmits > 1 byte long.
The hardware starts sending immediately that MDR is loaded. IMR trickery
doesn't work because if we start sending, finish the first byte and an
XRDY event occurs before we load IMR to unmask it, we never get an
interrupt, and we timeout.
Move the MDR load after DXR,IMR loads to avoid this race without locking.
Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
Signed-off-by: Jon Povey
---
Oops, v2 was based on the wrong version. Rebased so this should apply
against mainline.
Troy, thanks for the input. I tried reordering the IMR load before but got
occasional timeouts. Went back to the logic analyser today and worked out
why, see above comment in the commit message.
Moving the MDR load seems to fix things without locking, much neater and
tiny patch.
As I understand it we can be confident inside i2c_davinci_xfer_msg() that
the peripheral is not busy sending or receiving something else, I had a
look at the other interrupt sources in the datasheet and this seems safe
enough.
I'm not sure what the correct thing to do for followup message IDs here..
My original email? Troy's? Clues welcome for next time.
drivers/i2c/busses/i2c-davinci.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c87..b8feac5 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -357,9 +357,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->terminate = 0;
- /* write the data into mode register */
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-
/*
* First byte should be set here, not after interrupt,
* because transmit-data-ready interrupt can come before
@@ -371,6 +368,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->buf_len--;
}
+ /* write the data into mode register; start transmitting */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
dev->adapter.timeout);
if (r == 0) {
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] i2c: davinci: Fix race when setting up for TX
When setting up to transmit, a race exists between the ISR and
i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
This is mostly visible for transmits > 1 byte long.
The hardware starts sending immediately that MDR is loaded. IMR trickery
doesn't work because if we start sending, finish the first byte and an
XRDY event occurs before we load IMR to unmask it, we never get an
interrupt, and we timeout.
Move the MDR load after DXR,IMR loads to avoid this race without locking.
Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
Signed-off-by: Jon Povey
---
Troy, thanks for the input. I tried reordering the IMR load before but got
occasional timeouts. Went back to the logic analyser today and worked out
why, see above comment in the commit message.
Moving the MDR load seems to fix things without locking, much neater and
tiny patch.
As I understand it we can be confident inside i2c_davinci_xfer_msg() that
the peripheral is not busy sending or receiving something else, I had a
look at the other interrupt sources in the datasheet and this seems safe
enough.
I'm not sure what the correct thing to do for followup message IDs here..
My original email? Troy's? Clues welcome for next time.
drivers/i2c/busses/i2c-davinci.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 72df4af..baa5209 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -349,9 +349,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->terminate = 0;
- /* write the data into mode register */
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-
/*
* First byte should be set here, not after interrupt,
* because transmit-data-ready interrupt can come before
@@ -371,6 +368,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
w |= DAVINCI_I2C_IMR_XRDY;
davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
+ /* write the data into mode register; start transmitting */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
dev->adapter.timeout);
if (r == 0) {
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: davinci: Fix race when setting up for TX
When setting up to transmit, a race exists between the ISR and
i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
This is mostly visible for transmits > 1 byte long.
The ISR may run at any time after the mode register has been set.
While we are setting up and loading the first byte, protect this critical
section from the ISR with a spinlock.
The RX path or zero-length transmits do not need this locking.
Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
Signed-off-by: Jon Povey
---
I suspect this hasn't shown up for others using single-byte transmits as the
interrupt tends to either run entirely before or entirely after this block
in i2c_davinci_xfer_msg():
/*
* First byte should be set here, not after interrupt,
* because transmit-data-ready interrupt can come before
* NACK-interrupt during sending of previous message and
* ICDXR may have wrong data
*/
if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
dev->buf_len--;
}
Often the entire message would be sent before that test was executed
(observed with LED wiggling and a logic analyser), so dev->buf_len would
be untrue and things merrily went on their way. That seems to be counter
to the intent in the comment.
I tried some fiddling around reordering the register loads but couldn't
get things reliable so stuck in a spinlock. Better solutions welcome.
P.S.: Having run into the the bus reset code a lot during testing, I
am pretty sure that that generic_i2c_clock_pulse() does NOTHING due to
pinmuxing, at least on DM355, it is misleading and may be better
replaced with a comment saying "It would be great to toggle SCL here".
drivers/i2c/busses/i2c-davinci.c | 21 -
1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c87..43aa55d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -107,6 +107,7 @@ struct davinci_i2c_dev {
u8 *buf;
size_t buf_len;
int irq;
+ spinlock_t lock;
int stop;
u8 terminate;
struct i2c_adapter adapter;
@@ -312,6 +313,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
u32 flag;
u16 w;
int r;
+ unsigned long flags;
+ int preload = 0;
if (!pdata)
pdata = &davinci_i2c_platform_data_default;
@@ -347,6 +350,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
flag &= ~DAVINCI_I2C_MDR_STP;
}
+ /*
+* When transmitting, lock ISR out to avoid it racing on the buffer and
+* DXR register before we are done
+*/
+ if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
+ preload = 1;
+ spin_lock_irqsave(&dev->lock, flags);
+ }
+
/* Enable receive or transmit interrupts */
w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
if (msg->flags & I2C_M_RD)
@@ -366,13 +378,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
* NACK-interrupt during sending of previous message and
* ICDXR may have wrong data
*/
- if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
+ if (preload) {
davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
dev->buf_len--;
+ spin_unlock_irqrestore(&dev->lock, flags);
}
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
dev->adapter.timeout);
+
if (r == 0) {
dev_err(dev->dev, "controller timed out\n");
i2c_recover_bus(dev);
@@ -490,6 +504,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
int count = 0;
u16 w;
+ spin_lock(&dev->lock);
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -579,6 +595,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void
*dev_id)
}
}
+ spin_unlock(&dev->lock);
+
return count ? IRQ_HANDLED : IRQ_NONE;
}
@@ -662,6 +680,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_release_region;
}
+ spin_lock_init(&dev->lock);
init_completion(&dev->
[PATCH] i2c: davinci: Fix race when setting up for TX
Move the interrupt enable after the first byte load for TX, as it
was racing with the interrupt handler and corrupting dev->buf_len
for messages > 1 byte.
Tested on DM355.
Signed-off-by: Jon Povey
---
The race seems to have been introduced by
869e6692d527983958216f13c3c8e095791909df
This fixes the problem for me, but someone from TI might want to comment
on the validity of starting the transfer before enabling the RDY interrupts.
Any possibility of losing interrupts?
Considered a spinlock but if this reordering is OK then it's neater.
I'm guessing not many people have been sending multi-byte messages..
drivers/i2c/busses/i2c-davinci.c | 16
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c87..72df4af 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -347,14 +347,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
flag &= ~DAVINCI_I2C_MDR_STP;
}
- /* Enable receive or transmit interrupts */
- w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
- if (msg->flags & I2C_M_RD)
- w |= DAVINCI_I2C_IMR_RRDY;
- else
- w |= DAVINCI_I2C_IMR_XRDY;
- davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
-
dev->terminate = 0;
/* write the data into mode register */
@@ -371,6 +363,14 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->buf_len--;
}
+ /* Enable receive or transmit interrupts */
+ w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
+ if (msg->flags & I2C_M_RD)
+ w |= DAVINCI_I2C_IMR_RRDY;
+ else
+ w |= DAVINCI_I2C_IMR_XRDY;
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
+
r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
dev->adapter.timeout);
if (r == 0) {
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
