diff mbox series

[PATCHv6] i2c: cadence: Fix the hold bit setting

Message ID 1549365173-2271-1-git-send-email-shubhrajyoti.datta@gmail.com
State Accepted
Headers show
Series [PATCHv6] i2c: cadence: Fix the hold bit setting | expand

Commit Message

Shubhrajyoti Datta Feb. 5, 2019, 11:12 a.m. UTC 
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

In case the hold bit is not needed we are carrying the old values.
Fix the same by resetting the bit when not needed.

Fixes the sporadic i2c bus lockups on National Instruments
Zynq-based devices.


Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
Acked-by: Michal Simek <michal.simek@xilinx.com>
Reported-and-tested-by: Kyle Roeschley <kyle.roeschley@ni.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
v5:
fix some typos
v4:
update fixes tag
v3:
Add fixes and Ack
v2:
Update commit log
Also check if the hold_flag

 drivers/i2c/busses/i2c-cadence.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Wolfram Sang Feb. 5, 2019, 12:32 p.m. UTC | #1 
On Tue, Feb 05, 2019 at 04:42:53PM +0530, shubhrajyoti.datta@gmail.com wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> In case the hold bit is not needed we are carrying the old values.
> Fix the same by resetting the bit when not needed.
> 
> Fixes the sporadic i2c bus lockups on National Instruments
> Zynq-based devices.
> 
> 
> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> Reported-and-tested-by: Kyle Roeschley <kyle.roeschley@ni.com>

Since code was changed since v4, I'd like to ask Kyle to retest this
version of the patch, too.

> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
> v5:
> fix some typos

This is too lazy. Please describe exactly what you changed. Otherwise me
or the reviewers have to look it up. No need to resend because of this,
but I am getting a little annoyed with this patch because each version
feels rushed in some way wasting others people time.
Kyle Roeschley Feb. 5, 2019, 2:25 p.m. UTC | #2 
On Tue, Feb 05, 2019 at 01:32:51PM +0100, Wolfram Sang wrote:
> On Tue, Feb 05, 2019 at 04:42:53PM +0530, shubhrajyoti.datta@gmail.com wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > 
> > In case the hold bit is not needed we are carrying the old values.
> > Fix the same by resetting the bit when not needed.
> > 
> > Fixes the sporadic i2c bus lockups on National Instruments
> > Zynq-based devices.
> > 
> > 
> > Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> > Acked-by: Michal Simek <michal.simek@xilinx.com>
> > Reported-and-tested-by: Kyle Roeschley <kyle.roeschley@ni.com>
> 
> Since code was changed since v4, I'd like to ask Kyle to retest this
> version of the patch, too.
> 

I re-tested and this still fixes our bus timeouts and system hang.

> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > ---
> > v5:
> > fix some typos
> 
> This is too lazy. Please describe exactly what you changed. Otherwise me
> or the reviewers have to look it up. No need to resend because of this,
> but I am getting a little annoyed with this patch because each version
> feels rushed in some way wasting others people time.
> 
> [...]
> > -       if (id->send_count > CDNS_I2C_FIFO_DEPTH)
> > +       if ((id->send_count > CDNS_I2C_FIFO_DEPTH) || id->bus_hold_flag)
> >                 ctrl_reg |= CDNS_I2C_CR_HOLD;
> > +       else
> > +               ctrl_reg = ctrl_reg & ~CDNS_I2C_CR_HOLD;

We should use &= to be consistent. Peter's comment on v5 already mentioned
this, but after v6 was sent.
Wolfram Sang Feb. 15, 2019, 8:44 a.m. UTC | #3 
On Tue, Feb 05, 2019 at 04:42:53PM +0530, shubhrajyoti.datta@gmail.com wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> In case the hold bit is not needed we are carrying the old values.
> Fix the same by resetting the bit when not needed.
> 
> Fixes the sporadic i2c bus lockups on National Instruments
> Zynq-based devices.
> 
> 
> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> Reported-and-tested-by: Kyle Roeschley <kyle.roeschley@ni.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

Applied to for-current, thanks to everyone involved!

The mentioned update with "&=" can be sent incrementally. Most important
now, that this issue gets fixes.

Note that there are other patches pending for the cadence driver:

http://patchwork.ozlabs.org/project/linux-i2c/list/?series=&submitter=&state=&q=cadence&archive=&delegate=
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 9260574..42c38fd 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -628,8 +628,10 @@  static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	 * Check for the message size against FIFO depth and set the
 	 * 'hold bus' bit if it is greater than FIFO depth.
 	 */
-	if (id->recv_count > CDNS_I2C_FIFO_DEPTH)
+	if ((id->recv_count > CDNS_I2C_FIFO_DEPTH)  || id->bus_hold_flag)
 		ctrl_reg |= CDNS_I2C_CR_HOLD;
+	else
+		ctrl_reg = ctrl_reg & ~CDNS_I2C_CR_HOLD;
 
 	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
 
@@ -686,8 +688,11 @@  static void cdns_i2c_msend(struct cdns_i2c *id)
 	 * Check for the message size against FIFO depth and set the
 	 * 'hold bus' bit if it is greater than FIFO depth.
 	 */
-	if (id->send_count > CDNS_I2C_FIFO_DEPTH)
+	if ((id->send_count > CDNS_I2C_FIFO_DEPTH) || id->bus_hold_flag)
 		ctrl_reg |= CDNS_I2C_CR_HOLD;
+	else
+		ctrl_reg = ctrl_reg & ~CDNS_I2C_CR_HOLD;
+
 	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
 
 	/* Clear the interrupts in interrupt status register. */