[PATCH] i2c: i2c-imx: Use correct function to write to register

[PATCH] i2c: i2c-imx: Use correct function to write to register

[Michail Georgios Etairidis]

The i2c-imx driver incorrectly uses readb()/writeb() to read and write to the appropriate registers when performing a repeated start. The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be used instead. Performing a repeated start results in a kernel panic.

I am resubmitting this patch as the previous one was erroneous. This patch is based on the current upstream kernel code.

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 95ed171..54a47b4 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		 * the first read operation, otherwise the first read cost
 		 * one extra clock cycle.
 		 */
-		temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 		temp |= I2CR_MTX;
-		writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 	}
 	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 
@@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				 * the first read operation, otherwise the first read cost
 				 * one extra clock cycle.
 				 */
-				temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 				temp |= I2CR_MTX;
-				writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 			}
 		} else if (i == (msgs->len - 2)) {
 			dev_dbg(&i2c_imx->adapter.dev,

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Wolfram Sang]

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

On Fri, Jun 09, 2017 at 03:28:12PM +0200, Michail Georgios Etairidis wrote:

> The i2c-imx driver incorrectly uses readb()/writeb() to read and write
> to the appropriate registers when performing a repeated start. The
> appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be
> used instead. Performing a repeated start results in a kernel panic.

On what platform? It has a different regshift, I assume. Anyone from PTX
or Freescale can confirm?

> I am resubmitting this patch as the previous one was erroneous. This
> patch is based on the current upstream kernel code.

There is no Signed-off-by. I need that. Read
Documentation/process/submit-checklist.rst if you don't know about it
yet.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Uwe Kleine-König]

On Fri, Jun 09, 2017 at 03:28:12PM +0200, Michail Georgios Etairidis wrote:
> The i2c-imx driver incorrectly uses readb()/writeb() to read and write to the appropriate registers when performing a repeated start. The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be used instead. Performing a repeated start results in a kernel panic.

please limit lines in the commit to 76 chars.

> I am resubmitting this patch as the previous one was erroneous. This patch is based on the current upstream kernel code.
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 95ed171..54a47b4 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  		 * the first read operation, otherwise the first read cost
>  		 * one extra clock cycle.
>  		 */
> -		temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  		temp |= I2CR_MTX;
> -		writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);

These were introduced by commit ce1a78840ff7a authored by Yao Yuan.

>  	}
>  	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>  
> @@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
>  				 * the first read operation, otherwise the first read cost
>  				 * one extra clock cycle.
>  				 */
> -				temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> +				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  				temp |= I2CR_MTX;
> -				writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);

These were introduced by commit 054b62d9f25c9 authored by Fugang Duan.

I added these two people to Cc:, maybe they are still at Freescale/NXP
and can comment.

	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);

results in

	readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));

where .regshift is 2 for imx and 0 for vf610. So the patch makes a
difference on the former platforms only.

Best regards
Uwe

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

RE: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Andy Duan]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Tuesday, June 20, 2017 2:54 AM
>On Fri, Jun 09, 2017 at 03:28:12PM +0200, Michail Georgios Etairidis wrote:
>> The i2c-imx driver incorrectly uses readb()/writeb() to read and write to the
>appropriate registers when performing a repeated start. The appropriate
>imx_i2c_read_reg()/imx_i2c_write_reg() functions should be used instead.
>Performing a repeated start results in a kernel panic.
>
>please limit lines in the commit to 76 chars.
>
>> I am resubmitting this patch as the previous one was erroneous. This patch
>is based on the current upstream kernel code.
>>
>> diff --git a/drivers/i2c/busses/i2c-imx.c
>> b/drivers/i2c/busses/i2c-imx.c index 95ed171..54a47b4 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct
>*i2c_imx,
>>  		 * the first read operation, otherwise the first read cost
>>  		 * one extra clock cycle.
>>  		 */
>> -		temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>>  		temp |= I2CR_MTX;
>> -		writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>
>These were introduced by commit ce1a78840ff7a authored by Yao Yuan.
>
>>  	}
>>  	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>>
>> @@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct
>*i2c_imx, struct i2c_msg *msgs, bo
>>  				 * the first read operation, otherwise the first
>read cost
>>  				 * one extra clock cycle.
>>  				 */
>> -				temp = readb(i2c_imx->base + IMX_I2C_I2CR);
>> +				temp = imx_i2c_read_reg(i2c_imx,
>IMX_I2C_I2CR);
>>  				temp |= I2CR_MTX;
>> -				writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
>> +				imx_i2c_write_reg(temp, i2c_imx,
>IMX_I2C_I2CR);
>
>These were introduced by commit 054b62d9f25c9 authored by Fugang Duan.
>
>I added these two people to Cc:, maybe they are still at Freescale/NXP and
>can comment.
>
>	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>
>results in
>
>	readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
>
>where .regshift is 2 for imx and 0 for vf610. So the patch makes a difference
>on the former platforms only.
>
>Best regards
>Uwe
>
The patch seems fine. It should use .imx_i2c_read_reg() instead of readb().

Acked-by: Fugang Duan <fugang.duan@nxp.com>

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

AW: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Michail Georgios Etairidis]

The i2c-imx driver incorrectly uses readb()/writeb() to read and 
write to the appropriate registers when performing a repeated start. 
The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions 
should be used instead. Performing a repeated start results in 
a kernel panic. The platform is imx.

Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com>

---

I am resubmitting this patch to add the required Signed-off-by 
information.

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 95ed171..54a47b4 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		 * the first read operation, otherwise the first read cost
 		 * one extra clock cycle.
 		 */
-		temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 		temp |= I2CR_MTX;
-		writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 	}
 	msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 
@@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				 * the first read operation, otherwise the first read cost
 				 * one extra clock cycle.
 				 */
-				temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 				temp |= I2CR_MTX;
-				writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 			}
 		} else if (i == (msgs->len - 2)) {
 			dev_dbg(&i2c_imx->adapter.dev,

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Uwe Kleine-König]

Cc += Andy Duan

On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote:
> 
> The i2c-imx driver incorrectly uses readb()/writeb() to read and 
> write to the appropriate registers when performing a repeated start. 
> The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions 
> should be used instead. Performing a repeated start results in 
> a kernel panic. The platform is imx.

I really wonder why this didn't pop up earlier, maybe repeated start
just isn't that usual.

> Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com>
Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Fixes: 054b62d9f25c ("i2c: imx: fix the i2c bus hang issue when do repeat restart")
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

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

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Wolfram Sang]

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

On Tue, Jun 20, 2017 at 10:28:35AM +0200, Uwe Kleine-König wrote:
> Cc += Andy Duan
> 
> On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote:
> > 
> > The i2c-imx driver incorrectly uses readb()/writeb() to read and 
> > write to the appropriate registers when performing a repeated start. 
> > The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions 
> > should be used instead. Performing a repeated start results in 
> > a kernel panic. The platform is imx.
> 
> I really wonder why this didn't pop up earlier, maybe repeated start
> just isn't that usual.

Quite the contrary, read a register or something from an EEPROM and you
should have the write (for the memory pointer) and the read (actual
data) connected with a repeated start.

The readb() calls are only accessed when a read message is not the last
message. Since most transfers look like what I described above, this is
not so super often. Add to that that not everyone reports such patches
back upstream, this might go unnoticed for a while.

So, thanks Michail for the patch and keeping at it!

Note: I found there is some code duplication in the driver. Both
'if (is_lastmsg)' blocks look like copied to me...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Wolfram Sang]

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

On Tue, Jun 20, 2017 at 10:20:42AM +0200, Michail Georgios Etairidis wrote:
> 
> The i2c-imx driver incorrectly uses readb()/writeb() to read and 
> write to the appropriate registers when performing a repeated start. 
> The appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions 
> should be used instead. Performing a repeated start results in 
> a kernel panic. The platform is imx.
> 
> Signed-off-by: Michail G Etairidis <m.etairidis@beck-ipc.com>
> 

Applied to for-current with the acks and fixes tags added, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Michail Georgios Etairidis]

Hello Lucas,

I am sorry about the HTML.

The patch is based on version 4.11.4.

As far as I can see, the prototype fits:

static inline void imx_i2c_write_reg(unsigned int val,
		                        struct imx_i2c_struct *i2c_imx, unsigned int reg)

...and...

static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
		                                 unsigned int reg)

Those two functions are also used everywhere else in the driver in the same manner to access the registers.

Regards,

Michail

> Hi Michail,
> 
> which kernel is this based on? The change doesn't seem to match the
> prototype of imx_i2c_read_reg() and imx_i2c_write_reg() of the upstream
> kernel.
> 
> Also please fix your mailer to send patches in plain text instead of
> HTML.
> 
> Regards,
> Lucas
> 
> Am Donnerstag, den 08.06.2017, 18:34 +0200 schrieb Michail Georgios
> Etairidis:
> > The i2c-imx driver incorrectly uses readb()/writeb() to read and write
> > to the appropriate registers when performing a repeated start. The
> > appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be
> > used instead. Performing a repeated start results to a kernel panic.
> > 
> >  
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c
> > 
> > index 95ed1718..988bb620 100644
> > 
> > --- a/drivers/i2c/busses/i2c-imx.c
> > 
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > 
> > @@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct
> > *i2c_imx,
> > 
> >                                 * the first read operation, otherwise
> > the first read cost
> > 
> >                                 * one extra clock cycle.
> > 
> >                                 */
> > 
> > -                              temp = readb(i2c_imx->base +
> > IMX_I2C_I2CR);
> > 
> > +                             temp = imx_i2c_read_reg(i2c_imx->base +
> > IMX_I2C_I2CR);
> > 
> >                                temp |= I2CR_MTX;
> > 
> > -                              writeb(temp, i2c_imx->base +
> > IMX_I2C_I2CR);
> > 
> > +                             imx_i2c_write_reg(temp, i2c_imx->base +
> > IMX_I2C_I2CR);
> > 
> >                }
> > 
> >                msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx,
> > IMX_I2C_I2DR);
> > 
> > @@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct
> > *i2c_imx, struct i2c_msg *msgs, bo
> > 
> >                                                                 * the
> > first read operation, otherwise the first read cost
> > 
> >                                                                 * one
> > extra clock cycle.
> > 
> >                                                                 */
> > 
> > -                                                              temp =
> > readb(i2c_imx->base + IMX_I2C_I2CR);
> > 
> > +                                                             temp =
> > imx_i2c_read_reg(i2c_imx->base + IMX_I2C_I2CR);
> > 
> >                                                                temp |=
> > I2CR_MTX;
> > 
> > -
> > writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> > 
> > +
> > imx_i2c_write_reg(temp, i2c_imx->base + IMX_I2C_I2CR);
> > 
> >                                                }
> > 
> >                                } else if (i == (msgs->len - 2)) {
> > 
> > 
> > dev_dbg(&i2c_imx->adapter.dev,
> > 
> >

Re: [PATCH] i2c: i2c-imx: Use correct function to write to register

[Lucas Stach]

Hi Michail,

which kernel is this based on? The change doesn't seem to match the
prototype of imx_i2c_read_reg() and imx_i2c_write_reg() of the upstream
kernel.

Also please fix your mailer to send patches in plain text instead of
HTML.

Regards,
Lucas

Am Donnerstag, den 08.06.2017, 18:34 +0200 schrieb Michail Georgios
Etairidis:
> The i2c-imx driver incorrectly uses readb()/writeb() to read and write
> to the appropriate registers when performing a repeated start. The
> appropriate imx_i2c_read_reg()/imx_i2c_write_reg() functions should be
> used instead. Performing a repeated start results to a kernel panic.
> 
>  
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c
> b/drivers/i2c/busses/i2c-imx.c
> 
> index 95ed1718..988bb620 100644
> 
> --- a/drivers/i2c/busses/i2c-imx.c
> 
> +++ b/drivers/i2c/busses/i2c-imx.c
> 
> @@ -734,9 +734,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct
> *i2c_imx,
> 
>                                 * the first read operation, otherwise
> the first read cost
> 
>                                 * one extra clock cycle.
> 
>                                 */
> 
> -                              temp = readb(i2c_imx->base +
> IMX_I2C_I2CR);
> 
> +                             temp = imx_i2c_read_reg(i2c_imx->base +
> IMX_I2C_I2CR);
> 
>                                temp |= I2CR_MTX;
> 
> -                              writeb(temp, i2c_imx->base +
> IMX_I2C_I2CR);
> 
> +                             imx_i2c_write_reg(temp, i2c_imx->base +
> IMX_I2C_I2CR);
> 
>                }
> 
>                msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx,
> IMX_I2C_I2DR);
> 
> @@ -857,9 +857,9 @@ static int i2c_imx_read(struct imx_i2c_struct
> *i2c_imx, struct i2c_msg *msgs, bo
> 
>                                                                 * the
> first read operation, otherwise the first read cost
> 
>                                                                 * one
> extra clock cycle.
> 
>                                                                 */
> 
> -                                                              temp =
> readb(i2c_imx->base + IMX_I2C_I2CR);
> 
> +                                                             temp =
> imx_i2c_read_reg(i2c_imx->base + IMX_I2C_I2CR);
> 
>                                                                temp |=
> I2CR_MTX;
> 
> -
> writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> 
> +
> imx_i2c_write_reg(temp, i2c_imx->base + IMX_I2C_I2CR);
> 
>                                                }
> 
>                                } else if (i == (msgs->len - 2)) {
> 
> 
> dev_dbg(&i2c_imx->adapter.dev,
> 
>