Skip to contentSkip to conversation

i2c: mxs: disable QUEUE when sending is done

PATCH New Wolfram Sang 4 participants 6 messages Apr 27, 2012 – Apr 29, 2012

Subsystems (2)
Arm i.MX and Layerscape I2C subsystem host drivers
lore
Share & tools
lore

Wolfram Sang

author
#0 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Since the last fixes to this driver ensure now the queue termination is done correctly, we can finally disable the queue after a transfer without problems. The gain is that it will only be reenabled after the next transfer is fully set up. Before, the queue was running all the time and if the setup of the next message was interrupted by another thread, an incomplete buffer could have been sent, padded with zeroes.

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

---

In case you are testing this patch, please donate Tested-by tags. Regular I2C usage should do, a bit of stress testing (putting I2C bus and system under load) would be great.

 drivers/i2c/busses/i2c-mxs.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-mxs.c +3 -2
index 4c37347..76b8af4 100644
@@ -253,6 +253,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 	if (i2c->cmd_err == -ENXIO)
 		mxs_i2c_reset(i2c);
+	else
+		writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+				i2c->regs + MXS_I2C_QUEUECTRL_CLR);
 
 	dev_dbg(i2c->dev, "Done with err=%d\n", i2c->cmd_err);
 

@@ -383,8 +386,6 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev)
 	if (ret)
 		return -EBUSY;
 
-	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
-			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
 	writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET);
 
 	platform_set_drvdata(pdev, NULL);
Signature · 1 line
1.7.10
reviewer
#1 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Hi Wolfram,

Quoted 5 lines · Wolfram Sang

? ? ? ?if (i2c->cmd_err == -ENXIO)
? ? ? ? ? ? ? ?mxs_i2c_reset(i2c);
+ ? ? ? else
+ ? ? ? ? ? ? ? writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i2c->regs + MXS_I2C_QUEUECTRL_CLR);

When setting the QUEUE_RUN, do we really want to clear all the other bits of QUEUECTRL_CLR register?

I am wondering if we should only set QUEUE_RUN bit here.

My mx28evk does not come with i2c eeprom. I hope I will solder one soon so I can test it.

Thanks,

Fabio Estevam

#2 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Quoted 10 lines · Fabio Estevam

Hi Wolfram,

Quoted 5 lines · Wolfram Sang

? ? ? ?if (i2c->cmd_err == -ENXIO)
? ? ? ? ? ? ? ?mxs_i2c_reset(i2c);
+ ? ? ? else
+ ? ? ? ? ? ? ? writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i2c->regs + MXS_I2C_QUEUECTRL_CLR);

Quoted 4 lines

When setting the QUEUE_RUN, do we really want to clear all the other bits of QUEUECTRL_CLR register?

I am wondering if we should only set QUEUE_RUN bit here.

I didn't check the manual, but I guess writing to MXS_I2C_QUEUECTRL_CLR just clears the QUEUE_RUN bit and nothing else?!

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#3 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Dear Uwe Kleine-K?nig,

Quoted 4 lines · Fabio Estevam
Quoted 3 lines

Hi Wolfram,

On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <[email protected]>

Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 5 lines
       if (i2c->cmd_err == -ENXIO)
               mxs_i2c_reset(i2c);
drivers/i2c/busses/i2c-mxs.c +3 -0
+       else
+               writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+                               i2c->regs + MXS_I2C_QUEUECTRL_CLR);
Quoted 7 lines
Quoted 4 lines

When setting the QUEUE_RUN, do we really want to clear all the other bits of QUEUECTRL_CLR register?

I am wondering if we should only set QUEUE_RUN bit here.

I didn't check the manual, but I guess writing to MXS_I2C_QUEUECTRL_CLR just clears the QUEUE_RUN bit and nothing else?!

I think the RUN bit will be cleared by the controller anyway, so this patch is pointless.

Quoted 3 lines

Best regards
Uwe

Best regards,
Marek Vasut

author
#4 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

I am wondering if we should only set QUEUE_RUN bit here.

I didn't check the manual, but I guess writing to MXS_I2C_QUEUECTRL_CLR just clears the QUEUE_RUN bit and nothing else?!

I think the RUN bit will be cleared by the controller anyway, so this patch is pointless.

How do you get this idea? Given 27.3.2 and our experiences, I'd be very surprised. Try putting the system under heavy load without this patch and you'll see that FIFOs might be sent half filled when interrupted while filling.

Signature · 9 lines
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120429/67ca761f/attachment.sig>
#5 lore raw

PATCH 1/1 i2c: mxs: disable QUEUE when sending is done

Dear Wolfram Sang,

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line

I am wondering if we should only set QUEUE_RUN bit here.

I didn't check the manual, but I guess writing to MXS_I2C_QUEUECTRL_CLR just clears the QUEUE_RUN bit and nothing else?!

Quoted 7 lines
Quoted 2 lines

I think the RUN bit will be cleared by the controller anyway, so this patch is pointless.

How do you get this idea? Given 27.3.2 and our experiences, I'd be very surprised. Try putting the system under heavy load without this patch and you'll see that FIFOs might be sent half filled when interrupted while filling.

All right, I won't argue here ;-)

Best regards,
Marek Vasut