[PATCH] i2c: mxs: disable QUEUE when sending is done

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Wolfram Sang]

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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---

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(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 4c37347..76b8af4 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -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);
-- 
1.7.10

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Wolfram Sang]

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 <w.sang@pengutronix.de>
---

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(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 4c37347..76b8af4 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -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);
-- 
1.7.10

Re: [PATCH] i2c: mxs: disable QUEUE when sending is done

[Fabio Estevam]

Hi Wolfram,

On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>        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

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Fabio Estevam]

Hi Wolfram,

On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> ? ? ? ?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

Re: [PATCH] i2c: mxs: disable QUEUE when sending is done

[Uwe Kleine-König]

On Fri, Apr 27, 2012 at 11:51:56AM -0300, Fabio Estevam wrote:
> Hi Wolfram,
> 
> On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> 
> >        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.
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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Uwe Kleine-König]

On Fri, Apr 27, 2012 at 11:51:56AM -0300, Fabio Estevam wrote:
> Hi Wolfram,
> 
> On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> 
> > ? ? ? ?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.
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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] i2c: mxs: disable QUEUE when sending is done

[Marek Vasut]

Dear Uwe Kleine-König,

> On Fri, Apr 27, 2012 at 11:51:56AM -0300, Fabio Estevam wrote:
> > Hi Wolfram,
> > 
> > On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang@pengutronix.de> 
wrote:
> > >        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.
> 
> 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.

> 
> Best regards
> Uwe

Best regards,
Marek Vasut

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Marek Vasut]

Dear Uwe Kleine-K?nig,

> On Fri, Apr 27, 2012 at 11:51:56AM -0300, Fabio Estevam wrote:
> > Hi Wolfram,
> > 
> > On Fri, Apr 27, 2012 at 11:23 AM, Wolfram Sang <w.sang@pengutronix.de> 
wrote:
> > >        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.
> 
> 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.

> 
> Best regards
> Uwe

Best regards,
Marek Vasut

Re: [PATCH] i2c: mxs: disable QUEUE when sending is done

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]


> > > 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.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Wolfram Sang]

> > > 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.

-- 
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>

Re: [PATCH] i2c: mxs: disable QUEUE when sending is done

[Marek Vasut]

Dear Wolfram Sang,

> > > > 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.

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

Best regards,
Marek Vasut

[PATCH] i2c: mxs: disable QUEUE when sending is done

[Marek Vasut]

Dear Wolfram Sang,

> > > > 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.

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

Best regards,
Marek Vasut