[PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Marek Vasut]

Properly terminate the DMA transfer in case the DMA PIO transfer
or setup fails for any reason.

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Tim Michals <tcmichals-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 286ca19..0670da7 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -287,12 +287,14 @@ read_init_dma_fail:
 select_init_dma_fail:
 	dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
 select_init_pio_fail:
+	dmaengine_terminate_all(i2c->dmach);
 	return -EINVAL;
 
 /* Write failpath. */
 write_init_dma_fail:
 	dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
 write_init_pio_fail:
+	dmaengine_terminate_all(i2c->dmach);
 	return -EINVAL;
 }
 
-- 
1.7.10.4

[PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Marek Vasut]

There is no reason to disable the I2C SMBus quick mode on this
IP block. Enable it. This essentially fixes the problem with the
"i2c-detect" command for probing the bus.

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Tim Michals <tcmichals-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 0670da7..6ed53da 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -359,7 +359,7 @@ static int mxs_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 
 static u32 mxs_i2c_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }
 
 static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
-- 
1.7.10.4

Re: [PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Marek Vasut]

Dear Tim Michals,

> Mr. Marek Vasut,

Ugh, I feel so old ;-D

[...]

> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 286ca19..0670da7 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -287,12 +287,14 @@ read_init_dma_fail:
>  select_init_dma_fail:
>         dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
>  select_init_pio_fail:
> +       dmaengine_terminate_all(i2c->dmach);
>         return -EINVAL;
> 
>  /* Write failpath. */
>  write_init_dma_fail:
>         dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
>  write_init_pio_fail:
> +       dmaengine_terminate_all(i2c->dmach);
>         return -EINVAL;
>  }
> 
> I tested both patches using Linux version 3.7.0-rc4 with the following
> command:

Did you test it on mx28 or on mx23? I tested it on mx28 with i2cdetect -y 0 and 
it worked.

> #  i2cdetect -r 0 20 30
[...]

Attaching my results on MX28 (and note, I also attach dmesg and I re-ran the 
test to proove it's replicable -- aka. the DMA engine doesn't stall).

~ # /i2cdetect.static -r 0 20 30
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0 using read byte commands.
I will probe address range 0x14-0x1e.
Continue? [Y/n] 
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                                                 
10:             -- -- -- -- -- -- -- -- -- -- --    
20:                                                 
30:                                                 
40:                                                 
50:                                                 
60:                                                 
70:                                                 
~ # dmesg | tail -n 12
[    6.960000] Freeing init memory: 1348K
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
[   29.500000] mxs-dma 80024000.dma-apbx: mxs_dma_int_handler: error in channel 
6
~ # /i2cdetect.static -r 0 20 30
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0 using read byte commands.
I will probe address range 0x14-0x1e.
Continue? [Y/n] 
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                                                 
10:             -- -- -- -- -- -- -- -- -- -- --    
20:                                                 
30:                                                 
40:                                                 
50:                                                 
60:                                                 
70:

Best regards,
Marek Vasut

Re: [PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Marek Vasut]

Dear Tim Michals,

[...]

> > ~ # /i2cdetect.static -r 0 20 30
> > WARNING! This program can confuse your I2C bus, cause data loss and
> > worse! I will probe file /dev/i2c-0 using read byte commands.
> > I will probe address range 0x14-0x1e.
> > Continue? [Y/n]
> > 
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 
> > 00:
> > 10:             -- -- -- -- -- -- -- -- -- -- --
> > 20:
> > 30:
> > 40:
> > 50:
> > 60:
> > 70:
> > 
> > Best regards,
> > Marek Vasut
> 
> I'm using a imx233, I don't know what would be different or cause this
> issue.

The register layout differs. You might want to check if all the register 
programing is correct. Make sure the placement of various bits is correct. Also 
see how the DMA operates with the transfer length on MX23 and MX28 ... there are 
quite a few differences and I didn't test the driver on MX23 like that. I recall 
someone testing it on a different MX23 board but only by some "standard" usage.

Re: [PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Marek Vasut]

Dear Tim Michals,

[...]

> > The register layout differs. You might want to check if all the register
> > programing is correct. Make sure the placement of various bits is
> > correct. Also
> > see how the DMA operates with the transfer length on MX23 and MX28 ...
> > there are
> > quite a few differences and I didn't test the driver on MX23 like that. I
> > recall
> > someone testing it on a different MX23 board but only by some "standard"
> > usage.
> 
> OK, will report back in a couple of weeks due holidays and current
> assignment load.

Ok, it might be better to apply these as they actually fix it on MX28 until you 
have a proper fix ready. We don't officially support MX23 with this driver 
anyway.

Best regards,
Marek Vasut

Re: [PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Fabio Estevam]

Tim,

On Mon, Nov 19, 2012 at 1:05 PM, Tim Michals <tcmichals-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I tested both patches using Linux version 3.7.0-rc4 with the following
> command:
> #  i2cdetect -r 0 20 30
> Usage: i2cdetect [-y] [-a] [-q|-r] I2CBUS [FIRST LAST]
>        i2cdetect -F I2CBUS
>        i2cdetect -l
>   I2CBUS is an integer or an I2C bus name
>   If provided, FIRST and LAST limit the probing range.
> # i2cdetect -r 0 20 30
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-0 using read byte commands.
> I will probe address range 0x14-0x1e.
> Continue? [Y/n] y
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:             -- [   31.830000] mxs-i2c 80058000.i2c: Failed to get PIO
> reg. .
> -- -- [   32.830000] mxs-i2c 80058000.i2c: Failed to get PIO reg. write
> descrip.
> -- -- [   33.830000] mxs-i2c 80058000.i2c: Failed to get PIO reg. write
> descrip.
> -- -- [   34.830000] mxs-i2c 80058000.i2c: Failed to get PIO reg. write
> descrip.
> -- -- [   35.830000] mxs-i2c 80058000.i2c: Failed to get PIO reg. write
> descrip.

Which I2C device you have in the bus? Can you share your dts?

Regards,

Fabio Estevam

Re: [PATCH 2/2] i2c: mxs: Do not disable the I2C SMBus quick mode

[Wolfram Sang]

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

On Sun, Nov 18, 2012 at 06:25:08AM +0100, Marek Vasut wrote:
> There is no reason to disable the I2C SMBus quick mode on this
> IP block. Enable it. This essentially fixes the problem with the
> "i2c-detect" command for probing the bus.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Applied to for-next, thanks!

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

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

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Wolfram Sang]

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

On Sun, Nov 18, 2012 at 06:25:07AM +0100, Marek Vasut wrote:
> Properly terminate the DMA transfer in case the DMA PIO transfer
> or setup fails for any reason.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Tim Michals <tcmichals-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

I liked the one patch approach better; but mainly still waiting for
Tim's response.

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

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

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Marek Vasut]

Dear Wolfram Sang,

> On Sun, Nov 18, 2012 at 06:25:07AM +0100, Marek Vasut wrote:
> > Properly terminate the DMA transfer in case the DMA PIO transfer
> > or setup fails for any reason.
> > 
> > Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > Cc: Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Tim Michals <tcmichals-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> I liked the one patch approach better; but mainly still waiting for
> Tim's response.

Oh well ... you now have both, pick whichever you like ;-)

Agreed, we should wait for Tim. I thought he said he'd send the patch on friday, 
no ?

btw. I guess we won't be able to squeeze these into 3.7 anymore, right? Even if 
I'd say these are fixes.

Best regards,
Marek Vasut

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Wolfram Sang]

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


> btw. I guess we won't be able to squeeze these into 3.7 anymore, right? Even if 
> I'd say these are fixes.

After the outcoming of the discussion with Tim, I will apply the single
patch. But I won't apply Fabio's patch to enable I2C for MX23 since
there seem to be issues.

Since the driver currently advertises properly that it cannot do
SMBUS_QUICK, the addition of that is not a bugfix but a feature
addition, so I'll apply it to for-next.

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

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

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Marek Vasut]

Dear Wolfram Sang,

> > btw. I guess we won't be able to squeeze these into 3.7 anymore, right?
> > Even if I'd say these are fixes.
> 
> After the outcoming of the discussion with Tim, I will apply the single
> patch. But I won't apply Fabio's patch to enable I2C for MX23 since
> there seem to be issues.

Sounds good.

> Since the driver currently advertises properly that it cannot do
> SMBUS_QUICK, the addition of that is not a bugfix but a feature
> addition, so I'll apply it to for-next.

Actually, I'd say apply the DMA termination fix to stable and the SMBUS_QUICK 
enabling to -next if you want to be completely correct. You're right, one part 
of the patch is a bugfix, the rest if feature addition.

Best regards,
Marek Vasut

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Wolfram Sang]

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


> > Since the driver currently advertises properly that it cannot do
> > SMBUS_QUICK, the addition of that is not a bugfix but a feature
> > addition, so I'll apply it to for-next.
> 
> Actually, I'd say apply the DMA termination fix to stable and the SMBUS_QUICK 
> enabling to -next if you want to be completely correct. You're right, one part 
> of the patch is a bugfix, the rest if feature addition.

OK then.

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

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

Re: [PATCH 1/2] i2c: mxs: Handle i2c DMA failure properly

[Wolfram Sang]

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

On Sun, Nov 18, 2012 at 06:25:07AM +0100, Marek Vasut wrote:
> Properly terminate the DMA transfer in case the DMA PIO transfer
> or setup fails for any reason.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Applied to for-current, thanks!

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

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