Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1238694 > unrolled thread

Regression: at24 eeprom writing

Started byPeter Rosin <peda@lysator.liu.se>
First post2015-10-03 01:10 +0200
Last post2015-10-05 18:00 +0200
Articles 9 — 5 participants

Back to article view | Back to linux.kernel

Contents

  Regression: at24 eeprom writing Peter Rosin <peda@lysator.liu.se> - 2015-10-03 01:10 +0200
    Re: Regression: at24 eeprom writing Peter Rosin <peda@lysator.liu.se> - 2015-10-04 22:00 +0200
      Re: Regression: at24 eeprom writing Christian Gmeiner <christian.gmeiner@gmail.com> - 2015-10-05 08:50 +0200
    Re: Regression: at24 eeprom writing Peter Rosin <peda@lysator.liu.se> - 2015-10-05 10:50 +0200
      Re: Re: Regression: at24 eeprom writing kbuild test robot <lkp@intel.com> - 2015-10-05 11:10 +0200
      Re: Regression: at24 eeprom writing Ludovic Desroches <ludovic.desroches@atmel.com> - 2015-10-05 17:10 +0200
        Re: Regression: at24 eeprom writing Peter Rosin <peda@lysator.liu.se> - 2015-10-05 17:10 +0200
      Re: Regression: at24 eeprom writing Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-10-05 17:30 +0200
        Re: Regression: at24 eeprom writing Peter Rosin <peda@lysator.liu.se> - 2015-10-05 18:00 +0200

#1238694 — Regression: at24 eeprom writing

FromPeter Rosin <peda@lysator.liu.se>
Date2015-10-03 01:10 +0200
SubjectRegression: at24 eeprom writing
Message-ID<qfhCG-P8-19@gated-at.bofh.it>
Hi!

I recently upgraded from the atmel linux-3.18-at91 kernel to vanilla 4.2
and everything seemed fine. Until I tried to write to the little eeprom
chip. I then tried the linux-4.1-at91 kernel and that suffers too.

The symptoms are that it seems like writes get interrupted, and restarted
again without properly initializing everything again. Inspecting the i2c
bus during these fails gets me something like this (int hex) when I

echo abcdefghijklmnopqr > /sys/bus/i2c/devices/0-0050/eeprom

S a0 00 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 P
S a0 10 (clk and data low for a "long" time) 10 71 72 0a P

Notice how the address byte in the second chunk (10) is repeated after
the strange event on the i2c bus.

I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
"eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
eeprom writing starts working again.

AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
i2c_transfer code path both with that patch and with it reverted,
so I sadly don't see why the patch makes a difference.

I'm on a board that is based on the sama5d31 evaluation kit, with a
NXP SE97BTP,547 chip and this in the devicetree:

			i2c0: i2c@f0014000 {
				status = "okay";

				jc42@18 {
					compatible = "jc42";
					reg = <0x18>;
				};

				eeprom@50 {
					compatible = "24c02";
					reg = <0x50>;
					pagesize = <16>;
				};
			};

Any ideas?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1239200

FromPeter Rosin <peda@lysator.liu.se>
Date2015-10-04 22:00 +0200
Message-ID<qfXBU-1IF-9@gated-at.bofh.it>
In reply to#1238694
On 2015-10-03 01:05, Peter Rosin wrote:
> I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
> "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
> eeprom writing starts working again.
> 
> AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
> i2c_transfer code path both with that patch and with it reverted,
> so I sadly don't see why the patch makes a difference.

And now when I retry the same thing, that patch is no longer affecting things.
I must have confused myself over what kernel was actually running. Christian,
please accept my deepest apologies for implicating you in this regression.

But the regression is still there. In short, linux-3.18-at91 from the
linux4sam tree works, linux-4.1-at91 from the same tree does not, and
vanilla 4.2 also doesn't work. I have a hard time bisecting this thing
though, since the last known good version has a long list of atmel
patches that I refuse to even try to rebase...

Ideas still welcome of course.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239304

FromChristian Gmeiner <christian.gmeiner@gmail.com>
Date2015-10-05 08:50 +0200
Message-ID<qg7KW-7Yq-3@gated-at.bofh.it>
In reply to#1239200
Hi Peter.

Sorry for the late answer - I am currently on my way to Dublin. Maybe
it helps if you enable
I2C_DEBUG_CORE and I2C_DEBUG_BUS. In theory you should see a little
bit better what
happens on the bus.

Greets
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner


2015-10-04 21:50 GMT+02:00 Peter Rosin <peda@lysator.liu.se>:
> On 2015-10-03 01:05, Peter Rosin wrote:
>> I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
>> "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
>> eeprom writing starts working again.
>>
>> AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
>> i2c_transfer code path both with that patch and with it reverted,
>> so I sadly don't see why the patch makes a difference.
>
> And now when I retry the same thing, that patch is no longer affecting things.
> I must have confused myself over what kernel was actually running. Christian,
> please accept my deepest apologies for implicating you in this regression.
>
> But the regression is still there. In short, linux-3.18-at91 from the
> linux4sam tree works, linux-4.1-at91 from the same tree does not, and
> vanilla 4.2 also doesn't work. I have a hard time bisecting this thing
> though, since the last known good version has a long list of atmel
> patches that I refuse to even try to rebase...
>
> Ideas still welcome of course.
>
> Cheers,
> Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239360

FromPeter Rosin <peda@lysator.liu.se>
Date2015-10-05 10:50 +0200
Message-ID<qg9D4-2es-17@gated-at.bofh.it>
In reply to#1238694

[Multipart message — attachments visible in raw view] — view raw

On 2015-10-03 01:05, Peter Rosin wrote:
> Hi!
> 
> I recently upgraded from the atmel linux-3.18-at91 kernel to vanilla 4.2
> and everything seemed fine. Until I tried to write to the little eeprom
> chip. I then tried the linux-4.1-at91 kernel and that suffers too.
> 
> The symptoms are that it seems like writes get interrupted, and restarted
> again without properly initializing everything again. Inspecting the i2c
> bus during these fails gets me something like this (int hex) when I
> 
> echo abcdefghijklmnopqr > /sys/bus/i2c/devices/0-0050/eeprom
> 
> S a0 00 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 P
> S a0 10 (clk and data low for a "long" time) 10 71 72 0a P
> 
> Notice how the address byte in the second chunk (10) is repeated after
> the strange event on the i2c bus.
> 
> I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
> "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
> eeprom writing starts working again.
> 
> AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
> i2c_transfer code path both with that patch and with it reverted,
> so I sadly don't see why the patch makes a difference.
> 
> I'm on a board that is based on the sama5d31 evaluation kit, with a
> NXP SE97BTP,547 chip and this in the devicetree:
> 
> 			i2c0: i2c@f0014000 {
> 				status = "okay";
> 
> 				jc42@18 {
> 					compatible = "jc42";
> 					reg = <0x18>;
> 				};
> 
> 				eeprom@50 {
> 					compatible = "24c02";
> 					reg = <0x50>;
> 					pagesize = <16>;
> 				};
> 			};

Ok, I found the culprit, and I double and triple checked it this time...

If I move to the very latest on the linux-3.18-at91 branch, the bug is
there too. Which made it vastly more palatable to bisect the bug.

The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b
"i2c: at91: fix a race condition when using the DMA controller"
which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list.

If I add that patch on top of my previously working tree, it behaves just
as newer kernels, i.e. equally bad. The patch doesn't revert cleanly, but
reverting the patch and quick-n-dirty-fixing the conflict on vanilla 4.2
makes the problem go away.

I have attached what I actually reverted.

Cheers,
Peter

[toc] | [prev] | [next] | [standalone]

#1239373

Fromkbuild test robot <lkp@intel.com>
Date2015-10-05 11:10 +0200
Message-ID<qg9Wr-2Qs-33@gated-at.bofh.it>
In reply to#1239360

[Multipart message — attachments visible in raw view] — view raw

Hi Peter,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: arm-at91_dt_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-at91.c: In function 'at91_twi_read_data_dma_callback':
>> drivers/i2c/busses/i2c-at91.c:378:11: warning: unused variable 'ier' [-Wunused-variable]
     unsigned ier = AT91_TWI_TXCOMP;
              ^

vim +/ier +378 drivers/i2c/busses/i2c-at91.c

75b81f33 Marek Roszko      2014-08-20  362  			dev->buf_len = 1;
75b81f33 Marek Roszko      2014-08-20  363  		}
fac368a0 Nikolaus Voss     2011-11-08  364  	}
fac368a0 Nikolaus Voss     2011-11-08  365  
fac368a0 Nikolaus Voss     2011-11-08  366  	/* send stop if second but last byte has been read */
0ef6f321 Cyrille Pitchen   2015-06-09  367  	if (!dev->pdata->has_alt_cmd && dev->buf_len == 1)
fac368a0 Nikolaus Voss     2011-11-08  368  		at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
fac368a0 Nikolaus Voss     2011-11-08  369  
fac368a0 Nikolaus Voss     2011-11-08  370  	dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
fac368a0 Nikolaus Voss     2011-11-08  371  
fac368a0 Nikolaus Voss     2011-11-08  372  	++dev->buf;
fac368a0 Nikolaus Voss     2011-11-08  373  }
fac368a0 Nikolaus Voss     2011-11-08  374  
60937b2c Ludovic Desroches 2012-11-23  375  static void at91_twi_read_data_dma_callback(void *data)
60937b2c Ludovic Desroches 2012-11-23  376  {
60937b2c Ludovic Desroches 2012-11-23  377  	struct at91_twi_dev *dev = (struct at91_twi_dev *)data;
0ef6f321 Cyrille Pitchen   2015-06-09 @378  	unsigned ier = AT91_TWI_TXCOMP;
60937b2c Ludovic Desroches 2012-11-23  379  
5e3cfc6c Cyrille Pitchen   2015-06-09  380  	dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg[0]),
28772ac8 Wolfram Sang      2014-07-21  381  			 dev->buf_len, DMA_FROM_DEVICE);
60937b2c Ludovic Desroches 2012-11-23  382  
60937b2c Ludovic Desroches 2012-11-23  383  	/* The last two bytes have to be read without using dma */
60937b2c Ludovic Desroches 2012-11-23  384  	dev->buf += dev->buf_len - 2;
60937b2c Ludovic Desroches 2012-11-23  385  	dev->buf_len = 2;
e0b440c7 Peter Rosin       2015-10-05  386  	at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY);

:::::: The code at line 378 was first introduced by commit
:::::: 0ef6f3213dac48bbc71c98b5a2752db556f3e649 i2c: at91: add support for new alternative command mode

:::::: TO: Cyrille Pitchen <cyrille.pitchen@atmel.com>
:::::: CC: Wolfram Sang <wsa@the-dreams.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1239636

FromLudovic Desroches <ludovic.desroches@atmel.com>
Date2015-10-05 17:10 +0200
Message-ID<qgfyO-2sl-3@gated-at.bofh.it>
In reply to#1239360
Hi Peter

On Mon, Oct 05, 2015 at 10:45:29AM +0200, Peter Rosin wrote:
> On 2015-10-03 01:05, Peter Rosin wrote:

[...]

> Ok, I found the culprit, and I double and triple checked it this time...
> 
> If I move to the very latest on the linux-3.18-at91 branch, the bug is
> there too. Which made it vastly more palatable to bisect the bug.
> 
> The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b
> "i2c: at91: fix a race condition when using the DMA controller"
> which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list.
> 

Thanks for the bisecting effort. I am currently at ELCE where I have
met someone with the same kind of issue. Is it easily reproducible? It
doesn't seem to be the case for him.

I'll have a look once back.

> If I add that patch on top of my previously working tree, it behaves just
> as newer kernels, i.e. equally bad. The patch doesn't revert cleanly, but
> reverting the patch and quick-n-dirty-fixing the conflict on vanilla 4.2
> makes the problem go away.
> 
> I have attached what I actually reverted.
> 
> Cheers,
> Peter
> 

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239642

FromPeter Rosin <peda@lysator.liu.se>
Date2015-10-05 17:10 +0200
Message-ID<qgfyO-2sl-21@gated-at.bofh.it>
In reply to#1239636
On 2015-10-05 17:00, Ludovic Desroches wrote:
> Hi Peter
> 
> On Mon, Oct 05, 2015 at 10:45:29AM +0200, Peter Rosin wrote:
>> On 2015-10-03 01:05, Peter Rosin wrote:
> 
> [...]
> 
>> Ok, I found the culprit, and I double and triple checked it this time...
>>
>> If I move to the very latest on the linux-3.18-at91 branch, the bug is
>> there too. Which made it vastly more palatable to bisect the bug.
>>
>> The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b
>> "i2c: at91: fix a race condition when using the DMA controller"
>> which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list.
>>
> 
> Thanks for the bisecting effort. I am currently at ELCE where I have
> met someone with the same kind of issue. Is it easily reproducible? It
> doesn't seem to be the case for him.

Yes, easy as pie, happens on every eeprom write of 256 bytes so far...

> I'll have a look once back.

Ok good, to further help understanding, I'm seeing this on the i2c bus (I
hope you understand the notation, or just ask):

Working (4.2 + the revert from my previous message)
===================================================

S W50 0x00 "product = 1-776-" P   S W50 NACK P   S W50 NACK P
delay 15.2 ms
S W50 0x10 "3.0\n" P
delay 19.5 ms
S W50 0x10 "3.0\n" P   S W50 NACK P   S W50 NACK P   S W50 NACK P
delay 19.0 ms
S W50 0x14 "serial = 380" P
delay 18.8 ms
S W50 0x14 "serial = 380" P   S W50 NACK P   S W50 NACK P
delay 18.4 ms
S W50 0x20 "000002\n" P
delay 19.2 ms
S W50 0x20 "000002\n" P
delay 10.8 ms
S W50 0x27 "         " P   S W50 NACK P (repeated 5 times)
delay 16.7 ms
S W50 0x30 "                " P
delay 18.4 ms
S W50 0x30 "                " P   S W50 NACK P (repeated 3 times)
delay 17.9 ms
S W50 0x40 "                " P

etc

I.e. every write (but the first) seems to fail the first time and is
then retried, even if the i2c bus shows no failure indication (at
least that I can find).


Not working (vanilla 4.2)
=========================

S W50 0x00 "product = 1-776-" P   S W50 NACK P   S W50 NACK P
delay 17.3 ms
S W50 0x10 ACK...
delay 19.8 with both SDA and SCL low
...ACK 0x10 "3.0\n" P   S W50 NACK P   S W50 NACK P
delay 19.3 ms
S W50 0x14 "serial = 380" P   S W50 NACK P   S W50 NACK P
delay 18.5 ms
S W50 0x20 ACK...
delay 19.9 with both SDA and SCL low
...ACK 0x20 "000002\n" P   S W50 NACK P   S W50 NACK P
delay 18.9 ms
S W50 0x27 "         " P   S W50 NACK P   S W50 NACK P
delay 19.2 ms
S W50 0x30 "                " P   S W50 NACK P   S W50 NACK P
delay 17.6 ms
S W50 0x40 "                " P   S W50 NACK P   S W50 NACK P

etc

I.e. when there is a disturbance (the long ACks) the recovery
mechanism appears to attempt to heal it by resending only the
failing byte, but the eeprom appears to not see the failure and
takes both bytes instead of just the resend.



It seems dangerous to attempt to fix apparent trouble with an
i2c command by anything less than a full retry, like the working
version appears to do. No?

But what trouble does the i2c bus driver see? Admittedly I only
have a simple logic level bus viewer, and not a full-blown
oscilloscope, so there might be something analogue going on?
I don't think so though, those signals looked fine last time we
looked (but we obviously didn't have these issues then, and
didn't really look that closely). I'll see if I can recheck
with a real scope too.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239675

FromCyrille Pitchen <cyrille.pitchen@atmel.com>
Date2015-10-05 17:30 +0200
Message-ID<qgfSb-2Pr-51@gated-at.bofh.it>
In reply to#1239360
Le 05/10/2015 10:45, Peter Rosin a écrit :
> On 2015-10-03 01:05, Peter Rosin wrote:
>> Hi!
>>
>> I recently upgraded from the atmel linux-3.18-at91 kernel to vanilla 4.2
>> and everything seemed fine. Until I tried to write to the little eeprom
>> chip. I then tried the linux-4.1-at91 kernel and that suffers too.
>>
>> The symptoms are that it seems like writes get interrupted, and restarted
>> again without properly initializing everything again. Inspecting the i2c
>> bus during these fails gets me something like this (int hex) when I
>>
>> echo abcdefghijklmnopqr > /sys/bus/i2c/devices/0-0050/eeprom
>>
>> S a0 00 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 P
>> S a0 10 (clk and data low for a "long" time) 10 71 72 0a P
>>
>> Notice how the address byte in the second chunk (10) is repeated after
>> the strange event on the i2c bus.
>>
>> I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
>> "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
>> eeprom writing starts working again.
>>
>> AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
>> i2c_transfer code path both with that patch and with it reverted,
>> so I sadly don't see why the patch makes a difference.
>>
>> I'm on a board that is based on the sama5d31 evaluation kit, with a
>> NXP SE97BTP,547 chip and this in the devicetree:
>>
>> 			i2c0: i2c@f0014000 {
>> 				status = "okay";
>>
>> 				jc42@18 {
>> 					compatible = "jc42";
>> 					reg = <0x18>;
>> 				};
>>
>> 				eeprom@50 {
>> 					compatible = "24c02";
>> 					reg = <0x50>;
>> 					pagesize = <16>;
>> 				};
>> 			};
> 
> Ok, I found the culprit, and I double and triple checked it this time...
> 
> If I move to the very latest on the linux-3.18-at91 branch, the bug is
> there too. Which made it vastly more palatable to bisect the bug.
> 
> The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b
> "i2c: at91: fix a race condition when using the DMA controller"
> which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list.
> 
> If I add that patch on top of my previously working tree, it behaves just
> as newer kernels, i.e. equally bad. The patch doesn't revert cleanly, but
> reverting the patch and quick-n-dirty-fixing the conflict on vanilla 4.2
> makes the problem go away.
> 
> I have attached what I actually reverted.
> 
> Cheers,
> Peter
> 

Hi Peter,

Can you tell me whether your device tree sets the I2C controller i2c0 to use
dma channels, especially the "tx" one. I guess so but it is just to confirm 
hence we look in the right direction.

Then I think we should look at this part of the original patch:

        } else {
                if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
+                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
                        at91_twi_write_data_dma(dev);
-                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
                } else {
                        at91_twi_write_next_byte(dev);
                        at91_twi_write(dev, AT91_TWI_IER,

Here, for DMA TX transfers, we enable the NACK interrupt instead of the TXCOMP
one. This is the actual fix of the DMA race. Indeed there were two issues when
using TXCOMP to detect NACK conditions.

As written in the datasheet and confirmed by the IP designer, the TXCOMP bit is
set in the Status Register when both the Transmit Holding Register (THR) and
its internal shifter are empty and the STOP condition has been sent.
So when a first transfer successfully completes, the TXCOMP bit is set. Then
this bit remains set until the next write into THR.

The first issue is the race condition:
at91_twi_write_data_dma(dev);
at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);

The first line prepares a DMA transfer but when we execute the second line to
enable the TXCOMP interrupt, we have no mean to know whether the DMA has
already performed a first write access into THR, which also clears the TXCOMP
bit in the Status Register. If the DMA controller hasn't completed this first
write yet, the TXCOMP bit is still set in the Status Register. Hence the
interrupt handler is executed immediately after the TXCOMP interrupt has been
enabled. If the interrupt handler reads the Status Register before the DMA
controller has written into the THR, the TXCOMP bit is still set. Consequently,
the interrupt handler calls complete(&dev->cmd_complete) thinking the transfer
has completed though it actually has not even started.


The second issue is about the detection of NACK condition when using the DMA
controller. Before the patch, the driver relied on the TXCOMP interrupt to
detect NACK condition. It is true that the TXCOMP bit is set in the Status
Register when a NACK condition occurs. However if the I2C controller has
already triggered the DMA controller before it detects a NACK condition and
sets the TXCOMP bit, the DMA controller writes into the THR right after, hence
clears the TXCOMP bit in the Status Register. when the interrupt handler is
executed, it reads the Status Register but fails to detect the NACK condition
since the TXCOMP bit has been cleared: The driver misses the NACK condition.
This is why we should rely on the NACK interrupt instead. the NACK bit is
cleared on read in the Status Register, the NACK condition is properly
detected.

So instead of reverting the patch, maybe you could try to add the single line
which used to enable the TXCOMP interrupt after having scheduled the TX DMA
transfer:

        } else {
                if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
                        at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
                        at91_twi_write_data_dma(dev);
+                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
                } else {
                        at91_twi_write_next_byte(dev);
                        at91_twi_write(dev, AT91_TWI_IER,

I don't know whether this would "fix" your issue. Anyway if it does, this is
not a proper fix but it may help us to understand what is going on.

On my side, I will try to reproduce your issue on a sama5d3x board.


Best Regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239694

FromPeter Rosin <peda@lysator.liu.se>
Date2015-10-05 18:00 +0200
Message-ID<qgglc-3nE-5@gated-at.bofh.it>
In reply to#1239675
On 2015-10-05 17:28, Cyrille Pitchen wrote:
> Le 05/10/2015 10:45, Peter Rosin a écrit :
>> On 2015-10-03 01:05, Peter Rosin wrote:
>>> Hi!
>>>
>>> I recently upgraded from the atmel linux-3.18-at91 kernel to vanilla 4.2
>>> and everything seemed fine. Until I tried to write to the little eeprom
>>> chip. I then tried the linux-4.1-at91 kernel and that suffers too.
>>>
>>> The symptoms are that it seems like writes get interrupted, and restarted
>>> again without properly initializing everything again. Inspecting the i2c
>>> bus during these fails gets me something like this (int hex) when I
>>>
>>> echo abcdefghijklmnopqr > /sys/bus/i2c/devices/0-0050/eeprom
>>>
>>> S a0 00 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 P
>>> S a0 10 (clk and data low for a "long" time) 10 71 72 0a P
>>>
>>> Notice how the address byte in the second chunk (10) is repeated after
>>> the strange event on the i2c bus.
>>>
>>> I looked around and found that if I revert a839ce663b3183209fdf7b1fc4796bfe2a4679c3
>>> "eeprom: at24: extend driver to allow writing via i2c_smbus_write_byte_data"
>>> eeprom writing starts working again.
>>>
>>> AFAICT, the i2c-at91 bus driver makes the eeprom driver use the
>>> i2c_transfer code path both with that patch and with it reverted,
>>> so I sadly don't see why the patch makes a difference.
>>>
>>> I'm on a board that is based on the sama5d31 evaluation kit, with a
>>> NXP SE97BTP,547 chip and this in the devicetree:
>>>
>>> 			i2c0: i2c@f0014000 {
>>> 				status = "okay";
>>>
>>> 				jc42@18 {
>>> 					compatible = "jc42";
>>> 					reg = <0x18>;
>>> 				};
>>>
>>> 				eeprom@50 {
>>> 					compatible = "24c02";
>>> 					reg = <0x50>;
>>> 					pagesize = <16>;
>>> 				};
>>> 			};
>>
>> Ok, I found the culprit, and I double and triple checked it this time...
>>
>> If I move to the very latest on the linux-3.18-at91 branch, the bug is
>> there too. Which made it vastly more palatable to bisect the bug.
>>
>> The offender (in the 4.2 kernel) is 93563a6a71bb69dd324fc7354c60fb05f84aae6b
>> "i2c: at91: fix a race condition when using the DMA controller"
>> which is far more understandable. Ao, adding Cyrille Pitchen to the Cc list.
>>
>> If I add that patch on top of my previously working tree, it behaves just
>> as newer kernels, i.e. equally bad. The patch doesn't revert cleanly, but
>> reverting the patch and quick-n-dirty-fixing the conflict on vanilla 4.2
>> makes the problem go away.
>>
>> I have attached what I actually reverted.
>>
>> Cheers,
>> Peter
>>
> 
> Hi Peter,
> 
> Can you tell me whether your device tree sets the I2C controller i2c0 to use
> dma channels, especially the "tx" one. I guess so but it is just to confirm 
> hence we look in the right direction.

I think yes, I'm including sama5d3.dtsi and am not overriding anything interesting
in that area.
 
> Then I think we should look at this part of the original patch:
> 
>         } else {
>                 if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
> +                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
>                         at91_twi_write_data_dma(dev);
> -                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
>                 } else {
>                         at91_twi_write_next_byte(dev);
>                         at91_twi_write(dev, AT91_TWI_IER,
> 
> Here, for DMA TX transfers, we enable the NACK interrupt instead of the TXCOMP
> one. This is the actual fix of the DMA race. Indeed there were two issues when
> using TXCOMP to detect NACK conditions.
> 
> As written in the datasheet and confirmed by the IP designer, the TXCOMP bit is
> set in the Status Register when both the Transmit Holding Register (THR) and
> its internal shifter are empty and the STOP condition has been sent.
> So when a first transfer successfully completes, the TXCOMP bit is set. Then
> this bit remains set until the next write into THR.
> 
> The first issue is the race condition:
> at91_twi_write_data_dma(dev);
> at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
> 
> The first line prepares a DMA transfer but when we execute the second line to
> enable the TXCOMP interrupt, we have no mean to know whether the DMA has
> already performed a first write access into THR, which also clears the TXCOMP
> bit in the Status Register. If the DMA controller hasn't completed this first
> write yet, the TXCOMP bit is still set in the Status Register. Hence the
> interrupt handler is executed immediately after the TXCOMP interrupt has been
> enabled. If the interrupt handler reads the Status Register before the DMA
> controller has written into the THR, the TXCOMP bit is still set. Consequently,
> the interrupt handler calls complete(&dev->cmd_complete) thinking the transfer
> has completed though it actually has not even started.
> 
> 
> The second issue is about the detection of NACK condition when using the DMA
> controller. Before the patch, the driver relied on the TXCOMP interrupt to
> detect NACK condition. It is true that the TXCOMP bit is set in the Status
> Register when a NACK condition occurs. However if the I2C controller has
> already triggered the DMA controller before it detects a NACK condition and
> sets the TXCOMP bit, the DMA controller writes into the THR right after, hence
> clears the TXCOMP bit in the Status Register. when the interrupt handler is
> executed, it reads the Status Register but fails to detect the NACK condition
> since the TXCOMP bit has been cleared: The driver misses the NACK condition.
> This is why we should rely on the NACK interrupt instead. the NACK bit is
> cleared on read in the Status Register, the NACK condition is properly
> detected.
> 
> So instead of reverting the patch, maybe you could try to add the single line
> which used to enable the TXCOMP interrupt after having scheduled the TX DMA
> transfer:
> 
>         } else {
>                 if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
>                         at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
>                         at91_twi_write_data_dma(dev);
> +                       at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
>                 } else {
>                         at91_twi_write_next_byte(dev);
>                         at91_twi_write(dev, AT91_TWI_IER,
> 
> I don't know whether this would "fix" your issue. Anyway if it does, this is
> not a proper fix but it may help us to understand what is going on.

Nope, no change, and the bus also looks like in the other message I sent in
response to Ludovic, with some ~20ms long ACKs after the eeprom address which
is then repeated.

> On my side, I will try to reproduce your issue on a sama5d3x board.

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web