i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

PATCH New Gregory CLEMENT 6 participants 22 messages Feb 7, 2014 – Mar 10, 2014

Subsystems (2)

I2C subsystem host drivers I2C MV64XXX Marvell and Allwinner driver

lore

Share & tools

lore

Gregory CLEMENT

author

2014-02-07 10:55 UTC #0 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.

In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.

This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.

This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.

Cc stable at vger.kernel.org # v3.12+

Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-mv64xxx.c +8 -1

index b8c5187b9ee0..a1700c62d955 100644
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,

Signature · 1 line

1.8.1.2

Jason Cooper

2014-02-07 15:09 UTC #1 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

Quoted 12 lines · Gregory CLEMENT

Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.
In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.
This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.

Quoted 7 lines

Cc stable at vger.kernel.org # v3.12+
Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
Signed-off-by Gregory CLEMENT <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

I'd like to get a few tested-by's on this before this is pushed. We've had quite a bit of fixes this round :( Please test both multi_v7 and mvebu defconfigs.

Kevin, I know you're busy with a lot more than us, but if you could confirm that this fixes the bus hangs you reported, that would be great.

thx,

Jason.

Quoted 25 lines

drivers/i2c/busses/i2c-mv64xxx.c +8 -1

index b8c5187b9ee0..a1700c62d955 100644
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,

Signature · 1 line

1.8.1.2

Gregory CLEMENT

author

2014-02-07 15:18 UTC #2 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

Quoted 16 lines · Jason Cooper

Quoted 15 lines · Gregory CLEMENT
Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.
In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.
This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.
Cc stable at vger.kernel.org # v3.12+
Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)

Quoted 8 lines

Quoted 4 lines
Signed-off-by Gregory CLEMENT <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
I'd like to get a few tested-by's on this before this is pushed. We've had quite a bit of fixes this round :( Please test both multi_v7 and mvebu defconfigs.

Well of course I have tested with multi_v7 and mvebu defconfigs, but I think it is not my tested-by that you want! :)

An for the test we only have to test the Armada XP B0 based board, because in the other case the offload is disable, so the code I modified never doesn't run.

Thanks,

Gregory

Quoted 33 lines

Kevin, I know you're busy with a lot more than us, but if you could confirm that this fixes the bus hangs you reported, that would be great.

thx,

Jason.

Quoted 25 lines

drivers/i2c/busses/i2c-mv64xxx.c +8 -1

index b8c5187b9ee0..a1700c62d955 100644
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,

Signature · 1 line

1.8.1.2

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Kevin Hilman

2014-02-07 18:09 UTC #3 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

In reply to Jason Cooper

Quoted 21 lines · Jason Cooper

Quoted 12 lines · Gregory CLEMENT
Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.
In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.
This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.
Quoted 7 lines
Cc stable at vger.kernel.org # v3.12+
Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
Signed-off-by Gregory CLEMENT <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Quoted 6 lines

I'd like to get a few tested-by's on this before this is pushed. We've had quite a bit of fixes this round :( Please test both multi_v7 and mvebu defconfigs.
Kevin, I know you're busy with a lot more than us, but if you could confirm that this fixes the bus hangs you reported, that would be great.

I applied this patch on top of next-20140207 and tested this on the armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.

I confirm it fixes the timeouts I was seeing.

Tested-by Kevin Hilman <[email protected]>

Kevin

Jason Cooper

2014-02-07 18:13 UTC #4 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

Quoted 30 lines · Kevin Hilman

Quoted 21 lines · Jason Cooper
Quoted 12 lines · Gregory CLEMENT
Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.
In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.
This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.
Quoted 7 lines
Cc stable at vger.kernel.org # v3.12+
Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
Signed-off-by Gregory CLEMENT <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Quoted 6 lines
I'd like to get a few tested-by's on this before this is pushed. We've had quite a bit of fixes this round :( Please test both multi_v7 and mvebu defconfigs.
Kevin, I know you're busy with a lot more than us, but if you could confirm that this fixes the bus hangs you reported, that would be great.

Quoted 6 lines

I applied this patch on top of next-20140207 and tested this on the armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.
I confirm it fixes the timeouts I was seeing.
Tested-by Kevin Hilman <[email protected]>

Thanks, Kevin!

thx,

Jason.

Jason Cooper

2014-02-08 17:01 UTC #5 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

In reply to Gregory CLEMENT

Quoted 12 lines · Gregory CLEMENT

Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow.
In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading.
This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board.

Quoted 7 lines

Cc stable at vger.kernel.org # v3.12+
Fixes 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
Signed-off-by Gregory CLEMENT <[email protected]>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Acked-by Jason Cooper <[email protected]>

thx,

Jason.

Wolfram Sang

2014-02-13 09:41 UTC #6 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

In reply to Gregory CLEMENT

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 464

Quoted 8 lines

drivers/i2c/busses/i2c-mv64xxx.c +1 -1

@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {

Here you break the coding style...

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 472

Quoted 8 lines

drivers/i2c/busses/i2c-mv64xxx.c +7 -0

 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}

... but more importantly, NACK. The code is already hard to follow which is the cause for this bug. This snipplet makes the code even harder to read, so it needs some simplification IMO. I'll fire up a counterpatch in a minute to explain what I mean.

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +0 -0

 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,

Signature · 1 line

1.8.1.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140213/cee40674/attachment.sig>

Gregory CLEMENT

author

2014-02-13 14:23 UTC #7 lore raw

PATCH 1/1 i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

Hi Wolfram,

Thanks for you review,

Quoted 11 lines · Wolfram Sang

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 464
Quoted 8 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -1
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
Here you break the coding style...

Quoted 13 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 472
Quoted 8 lines
drivers/i2c/busses/i2c-mv64xxx.c +7 -0
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
... but more importantly, NACK. The code is already hard to follow which is the cause for this bug. This snipplet makes the code even harder to read, so it needs some simplification IMO. I'll fire up a counterpatch in a minute to explain what I mean.

OK, as long as we have a fix for this issue.

I am waiting for your patch.

Thanks,

Gregory

Quoted 7 lines

Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -0
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
Signature · 1 line
1.8.1.2

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Wolfram Sang

2014-02-13 20:36 UTC #8 lore raw

cover mv64xxx updates

In reply to Wolfram Sang

So, this is a series I came up with trying to fix the issue found by Kevin. Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW). Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5 are further cleanup possibilities. And there is still more potential, I mainly wanted to give some inspiration and awareness that the driver could need some more love. Please test at least 1+2, comments to 3-5 very welcome.

Sorry for the delay, I got distracted by an NMI.

Wolfram Sang (5):
i2c: mv64xxx: put offload check into offload prepare function
i2c: mv64xxx: refactor message start to ensure proper initialization
i2c: mv64xxx: refactor send_start
i2c: mv64xxx: directly call send_start when initializing transfer
i2c: mv64xxx: refactor initialization for new msgs

 drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

Signature · 1 line

1.8.5.1

Wolfram Sang

2014-02-13 20:36 UTC #9 lore raw

PATCH 1/5 i2c: mv64xxx: put offload check into offload prepare function

It makes code simpler to read and prepares a reorganization needed for a bugfix.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +4 -2

index b8c5187..ba64978 100644
@@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	unsigned long ctrl_reg;
 	struct i2c_msg *msg = drv_data->msgs;
 
+	if (!drv_data->offload_enabled)
+		return -EOPNOTSUPP;
+
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;

@@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (!(drv_data->offload_enabled &&
-				mv64xxx_i2c_offload_msg(drv_data))) {
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
 			writel(drv_data->cntl_bits,
 			drv_data->reg_base + drv_data->reg_offsets.control);

Signature · 1 line

1.8.5.1

Gregory CLEMENT

author

2014-02-14 11:38 UTC #10 lore raw

PATCH 1/5 i2c: mv64xxx: put offload check into offload prepare function

Quoted 4 lines · Wolfram Sang

It makes code simpler to read and prepares a reorganization needed for a bugfix.
Signed-off-by Wolfram Sang <[email protected]>

Tested-by Gregory CLEMENT <[email protected]>

As expected it didn't fix the issue I had, but it didn't introduce any visible regression too.

Quoted 29 lines

---
 drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +4 -2

index b8c5187..ba64978 100644
@@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	unsigned long ctrl_reg;
 	struct i2c_msg *msg = drv_data->msgs;
 
+	if (!drv_data->offload_enabled)
+		return -EOPNOTSUPP;
+
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;

@@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (!(drv_data->offload_enabled &&
-				mv64xxx_i2c_offload_msg(drv_data))) {
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
 			writel(drv_data->cntl_bits,
 			drv_data->reg_base + drv_data->reg_offsets.control);

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Wolfram Sang

2014-02-13 20:36 UTC #11 lore raw

PATCH 2/5 i2c: mv64xxx: refactor message start to ensure proper initialization

In reply to Wolfram Sang

Because the offload mechanism can fall back to a standard transfer, having two seperate initialization states is unfortunate. Let's just have one state which does things consistently. This fixes a bug where some preparation was missing when the fallback happened. And it makes the code much easier to follow.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +10 ~~-17~~

index ba64978..d52d849 100644
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,

@@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
-		if (!mv64xxx_i2c_offload_msg(drv_data))
-			break;
-		else
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
-		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		/* Can we offload this msg ? */
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+			/* No, switch to standard path */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+				drv_data->reg_base + drv_data->reg_offsets.control);
+		}
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:

@@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	if (drv_data->offload_enabled) {
-		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	} else {
-		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	}
+	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);

Signature · 1 line

1.8.5.1

Gregory CLEMENT

author

2014-02-14 11:39 UTC #12 lore raw

PATCH 2/5 i2c: mv64xxx: refactor message start to ensure proper initialization

Quoted 7 lines · Wolfram Sang

Because the offload mechanism can fall back to a standard transfer, having two seperate initialization states is unfortunate. Let's just have one state which does things consistently. This fixes a bug where some preparation was missing when the fallback happened. And it makes the code much easier to follow.
Signed-off-by Wolfram Sang <[email protected]>

Tested-by Gregory CLEMENT <[email protected]>

With this one the bug we observed was fixed.

Thanks,

Gregory

Quoted 59 lines

---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +10 ~~-17~~

index ba64978..d52d849 100644
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,

@@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
-		if (!mv64xxx_i2c_offload_msg(drv_data))
-			break;
-		else
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
-		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		/* Can we offload this msg ? */
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+			/* No, switch to standard path */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+				drv_data->reg_base + drv_data->reg_offsets.control);
+		}
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:

@@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	if (drv_data->offload_enabled) {
-		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	} else {
-		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	}
+	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Wolfram Sang

2014-02-15 14:44 UTC #13 lore raw

PATCH 2/5 i2c: mv64xxx: refactor message start to ensure proper initialization

Quoted 7 lines · Gregory CLEMENT

Quoted 6 lines · Wolfram Sang
Because the offload mechanism can fall back to a standard transfer, having two seperate initialization states is unfortunate. Let's just have one state which does things consistently. This fixes a bug where some preparation was missing when the fallback happened. And it makes the code much easier to follow.

Quoted 6 lines

Quoted 1 line
Signed-off-by Wolfram Sang <[email protected]>
Tested-by Gregory CLEMENT <[email protected]>
With this one the bug we observed was fixed.

I squashed patches 1+2 (easier handling for stable) and applied to for-current, thanks!

Wolfram Sang

2014-02-13 20:36 UTC #14 lore raw

PATCH 3/5 i2c: mv64xxx: refactor send_start

In reply to Wolfram Sang

For start and restart, we are doing the same thing. Let's consolidate that.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +16 ~~-14~~

index d52d849..9c37b59 100644
@@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 	}
 }
 
+static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
+{
+	/* Can we offload this msg ? */
+	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+		/* No, switch to standard path */
+		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+			drv_data->reg_base + drv_data->reg_offsets.control);
+	}
+}
+
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {

@@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-			writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		// CHECKME: Does it work? Order of writel and prepare_for_io is
+		// exchanged. Also, do we need to change cntl_bits in drv_data
+		// with |= MV64XXX_I2C_REG_CONTROL_START?
+		mv64xxx_i2c_send_start(drv_data);
 
-			/* Setup for the next message */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-		}
 		if (drv_data->errata_delay)
 			udelay(5);

@@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_START:
-		/* Can we offload this msg ? */
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			/* No, switch to standard path */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-				drv_data->reg_base + drv_data->reg_offsets.control);
-		}
+		mv64xxx_i2c_send_start(drv_data);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:

Signature · 1 line

1.8.5.1

Gregory CLEMENT

author

2014-02-14 11:42 UTC #15 lore raw

PATCH 3/5 i2c: mv64xxx: refactor send_start

Quoted 4 lines · Wolfram Sang

For start and restart, we are doing the same thing. Let's consolidate that.
Signed-off-by Wolfram Sang <[email protected]>

With my first tests it continue to work with this change, but I want to have a closer look on it

Thanks,

Gregory

Quoted 61 lines

---
 drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +16 ~~-14~~

index d52d849..9c37b59 100644
@@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 	}
 }
 
+static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
+{
+	/* Can we offload this msg ? */
+	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+		/* No, switch to standard path */
+		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+			drv_data->reg_base + drv_data->reg_offsets.control);
+	}
+}
+
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {

@@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-			writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		// CHECKME: Does it work? Order of writel and prepare_for_io is
+		// exchanged. Also, do we need to change cntl_bits in drv_data
+		// with |= MV64XXX_I2C_REG_CONTROL_START?
+		mv64xxx_i2c_send_start(drv_data);
 
-			/* Setup for the next message */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-		}
 		if (drv_data->errata_delay)
 			udelay(5);

@@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_START:
-		/* Can we offload this msg ? */
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			/* No, switch to standard path */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-				drv_data->reg_base + drv_data->reg_offsets.control);
-		}
+		mv64xxx_i2c_send_start(drv_data);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Wolfram Sang

2014-02-13 20:36 UTC #16 lore raw

PATCH 4/5 i2c: mv64xxx: directly call send_start when initializing transfer

In reply to Wolfram Sang

Calling the state machine with a definite state which is only used in this context is superfluous. Do it directly.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +1 -7

index 9c37b59..3af5266 100644
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,

@@ -467,10 +466,6 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_SEND_START:
-		mv64xxx_i2c_send_start(drv_data);
-		break;
-
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
 		writel(drv_data->addr1,
 			drv_data->reg_base + drv_data->reg_offsets.data);

@@ -628,12 +623,11 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 
 	spin_lock_irqsave(&drv_data->lock, flags);
 
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
 	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
 
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
-	mv64xxx_i2c_do_action(drv_data);
+	mv64xxx_i2c_send_start(drv_data);
 	spin_unlock_irqrestore(&drv_data->lock, flags);
 
 	mv64xxx_i2c_wait_for_completion(drv_data);

Signature · 1 line

1.8.5.1

Wolfram Sang

2014-02-13 20:36 UTC #17 lore raw

PATCH 5/5 i2c: mv64xxx: refactor initialization for new msgs

In reply to Wolfram Sang

We now have a central place to put this code to.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +6 ~~-10~~

index 3af5266..be4b0a1 100644
@@ -175,11 +175,6 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 {
 	u32	dir = 0;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
 		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;

@@ -205,11 +200,6 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	if (!drv_data->offload_enabled)
 		return -EOPNOTSUPP;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	/* Only regular transactions can be offloaded */
 	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
 		return -EINVAL;

@@ -420,6 +410,12 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 
 static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
 {
+	drv_data->msg = drv_data->msgs;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = drv_data->msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+
 	/* Can we offload this msg ? */
 	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 		/* No, switch to standard path */

Signature · 1 line

1.8.5.1

Gregory CLEMENT

author

2014-02-13 20:41 UTC #18 lore raw

cover mv64xxx updates

In reply to Wolfram Sang

Hi Wolfram,

Quoted 9 lines · Wolfram Sang

So, this is a series I came up with trying to fix the issue found by Kevin. Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW). Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5 are further cleanup possibilities. And there is still more potential, I mainly wanted to give some inspiration and awareness that the driver could need some more love. Please test at least 1+2, comments to 3-5 very welcome.
Sorry for the delay, I got distracted by an NMI.

Thanks for this series, indeed the code looks better.

I will test it tomorrow and let you know if it fixed the bug. I will also take time to review the RFC patches.

Greogry

Quoted 10 lines

Wolfram Sang (5):
i2c: mv64xxx: put offload check into offload prepare function
i2c: mv64xxx: refactor message start to ensure proper initialization
i2c: mv64xxx: refactor send_start
i2c: mv64xxx: directly call send_start when initializing transfer
i2c: mv64xxx: refactor initialization for new msgs
 drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Wolfram Sang

2014-03-10 16:25 UTC #19 lore raw

cover mv64xxx updates

Quoted 3 lines

I will test it tomorrow and let you know if it fixed the bug. I will also take time to review the RFC patches.

I applied the other three patches now.

Thomas Petazzoni

2014-02-13 20:51 UTC #20 lore raw

cover mv64xxx updates

In reply to Wolfram Sang

Dear Wolfram Sang,

Quoted 8 lines · Wolfram Sang

So, this is a series I came up with trying to fix the issue found by Kevin. Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW). Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5 are further cleanup possibilities. And there is still more potential, I mainly wanted to give some inspiration and awareness that the driver could need some more love. Please test at least 1+2, comments to 3-5 very welcome.
Sorry for the delay, I got distracted by an NMI.

I'm adding Maxime Ripard in Cc, since he is the maintainer of the Allwinner platform, which also uses this I2C driver. So I guess he would like to be notified of such changes in order to test that the driver still works for him.

Thanks!

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Maxime Ripard

2014-02-18 11:29 UTC #21 lore raw

cover mv64xxx updates

Hi,

Quoted 10 lines · Thomas Petazzoni

Dear Wolfram Sang,
Quoted 7 lines · Wolfram Sang
So, this is a series I came up with trying to fix the issue found by Kevin. Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW). Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5 are further cleanup possibilities. And there is still more potential, I mainly wanted to give some inspiration and awareness that the driver could need some more love. Please test at least 1+2, comments to 3-5 very welcome.

Quoted 6 lines

Quoted 1 line
Sorry for the delay, I got distracted by an NMI.
I'm adding Maxime Ripard in Cc, since he is the maintainer of the Allwinner platform, which also uses this I2C driver. So I guess he would like to be notified of such changes in order to test that the driver still works for him.

I just gave a try to these patches, on my A31 board with the extra patches sent previously.

Tested-by Maxime Ripard <[email protected]>

Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140218/3a3f8b4c/attachment.sig>