| Message ID | 1334677065-6620-1-git-send-email-to-fleischer@t-online.de |
|---|---|
| State | Accepted |
| Delegated to: | Heiko Schocher |
| Headers | show |
Dear Torsten Fleischer, > Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. > These devices require that the high byte of the internal address has to be > written first. > The mxs_i2c driver currently writes the address' low byte first. > > The following patch fixes the byte order of the internal address that > should be written to the I2C device. > > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de> Acked-by: Marek Vasut <marex@denx.de> > > CC: Marek Vasut <marex@denx.de> > CC: Stefano Babic <sbabic@denx.de> > CC: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/i2c/mxs_i2c.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c > index c8fea32..48aaaa6 100644 > --- a/drivers/i2c/mxs_i2c.c > +++ b/drivers/i2c/mxs_i2c.c > @@ -97,7 +97,7 @@ void mxs_i2c_write(uchar chip, uint addr, int alen, > > for (i = 0; i < alen; i++) { > data >>= 8; > - data |= ((char *)&addr)[i] << 24; > + data |= ((char *)&addr)[alen - i - 1] << 24; > if ((i & 3) == 2) > writel(data, &i2c_regs->hw_i2c_data); > } Best regards, Marek Vasut
Dear Torsten Fleischer, > Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. > These devices require that the high byte of the internal address has to be > written first. > The mxs_i2c driver currently writes the address' low byte first. Are you sure about it? Are you sure what you're seeing isn't FIFO overrun? Basically, how does your problem manifest? How did you test this? We've been having similar issues in Linux recently. > > The following patch fixes the byte order of the internal address that > should be written to the I2C device. > > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de> > > CC: Marek Vasut <marex@denx.de> > CC: Stefano Babic <sbabic@denx.de> > CC: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/i2c/mxs_i2c.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c > index c8fea32..48aaaa6 100644 > --- a/drivers/i2c/mxs_i2c.c > +++ b/drivers/i2c/mxs_i2c.c > @@ -97,7 +97,7 @@ void mxs_i2c_write(uchar chip, uint addr, int alen, > > for (i = 0; i < alen; i++) { > data >>= 8; > - data |= ((char *)&addr)[i] << 24; > + data |= ((char *)&addr)[alen - i - 1] << 24; > if ((i & 3) == 2) > writel(data, &i2c_regs->hw_i2c_data); > } Best regards, Marek Vasut
Dear Marek Vasut, > > Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. > > These devices require that the high byte of the internal address has to > > be written first. > > The mxs_i2c driver currently writes the address' low byte first. > > Are you sure about it? Are you sure what you're seeing isn't FIFO overrun? Yes, because I used 'i2c md' and 'i2c mw' for the test and these commands do only a small write transfer. They write 3 and 4 bytes respectively. This is in both cases only one FIFO word. > Basically, how does your problem manifest? How did you test this? First I wrote different bytes into the addresses 0x00 and 0x01 of the EEPROM using 'i2c mw', for example: => i2c mw 54 0.2 12 => i2c mw 54 1.2 34 Then I read the address range 0x00 - 0x01 and got: => i2c md 54 0.2 2 0000: 12 ff .. That's not what I had expected. On address 0x01 there should be '34' instead of 'ff'. But when I read directly from the address 0x01 the command returned the correct value: => i2c md 54 1.2 1 0001: 34 4 Thereupon I checked the I2C frame of the read and write commands to the address 0x01 with an oscilloscope. The scope showed that after the device's address the bytes 0x01 followed by 0x00 were sent to the EEPROM. But the two bytes must be in reverse order. After inspecting the source code I swapped the address bytes, i.e. I used '100.2' instead of '1.2'. With it the read/write command accessed the address 0x01 of the EEPROM. I verified this with the scope. Reading the range 0x00 - 0x01 returned then the expected content: => i2c md 54 0.2 2 0000: 12 34 .4 I also tested the address byte swapping - before and after modifying the driver - with other addresses (e.g. 0x0180 and 0x0fff) by checking the I2C frames with the scope. After fixing the driver, I additionally read the whole EEPROM using 'i2c md' and verified the written addresses. Best Regards Torsten Fleischer
Dear Torsten Fleischer, > Dear Marek Vasut, > > > > Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. > > > These devices require that the high byte of the internal address has to > > > be written first. > > > The mxs_i2c driver currently writes the address' low byte first. > > > > Are you sure about it? Are you sure what you're seeing isn't FIFO > > overrun? > > Yes, because I used 'i2c md' and 'i2c mw' for the test and these commands > do only a small write transfer. They write 3 and 4 bytes respectively. > This is in both cases only one FIFO word. > > > Basically, how does your problem manifest? How did you test this? > > First I wrote different bytes into the addresses 0x00 and 0x01 of the > EEPROM using 'i2c mw', for example: > > => i2c mw 54 0.2 12 > => i2c mw 54 1.2 34 > > Then I read the address range 0x00 - 0x01 and got: > > => i2c md 54 0.2 2 > 0000: 12 ff .. > > That's not what I had expected. On address 0x01 there should be '34' > instead of 'ff'. > But when I read directly from the address 0x01 the command returned the > correct value: > > => i2c md 54 1.2 1 > 0001: 34 4 > > Thereupon I checked the I2C frame of the read and write commands to the > address 0x01 with an oscilloscope. The scope showed that after the device's > address the bytes 0x01 followed by 0x00 were sent to the EEPROM. But the > two bytes must be in reverse order. > > After inspecting the source code I swapped the address bytes, i.e. I used > '100.2' instead of '1.2'. With it the read/write command accessed the > address 0x01 of the EEPROM. I verified this with the scope. > > Reading the range 0x00 - 0x01 returned then the expected content: > > => i2c md 54 0.2 2 > 0000: 12 34 .4 > > I also tested the address byte swapping - before and after modifying the > driver - with other addresses (e.g. 0x0180 and 0x0fff) by checking the I2C > frames with the scope. > After fixing the driver, I additionally read the whole EEPROM using 'i2c > md' and verified the written addresses. Oh this is very nice! Thanks a lot! Obviously: Acked-by: Marek Vasut <marex@denx.de> > Best Regards > Torsten Fleischer Best regards, Marek Vasut
On 17/04/2012 17:37, Torsten Fleischer wrote: > Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. > These devices require that the high byte of the internal address has to be > written first. > The mxs_i2c driver currently writes the address' low byte first. > > The following patch fixes the byte order of the internal address that should > be written to the I2C device. > > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de> > > CC: Marek Vasut <marex@denx.de> > CC: Stefano Babic <sbabic@denx.de> > CC: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/i2c/mxs_i2c.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c > index c8fea32..48aaaa6 100644 > --- a/drivers/i2c/mxs_i2c.c > +++ b/drivers/i2c/mxs_i2c.c > @@ -97,7 +97,7 @@ void mxs_i2c_write(uchar chip, uint addr, int alen, > > for (i = 0; i < alen; i++) { > data >>= 8; > - data |= ((char *)&addr)[i] << 24; > + data |= ((char *)&addr)[alen - i - 1] << 24; > if ((i & 3) == 2) > writel(data, &i2c_regs->hw_i2c_data); > } I forwarded your patch to Heiko (I2C custodian), but here my: Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c index c8fea32..48aaaa6 100644 --- a/drivers/i2c/mxs_i2c.c +++ b/drivers/i2c/mxs_i2c.c @@ -97,7 +97,7 @@ void mxs_i2c_write(uchar chip, uint addr, int alen, for (i = 0; i < alen; i++) { data >>= 8; - data |= ((char *)&addr)[i] << 24; + data |= ((char *)&addr)[alen - i - 1] << 24; if ((i & 3) == 2) writel(data, &i2c_regs->hw_i2c_data); }
Large EEPROMs, e.g. 24lc32, need 2 byte to address the internal memory. These devices require that the high byte of the internal address has to be written first. The mxs_i2c driver currently writes the address' low byte first. The following patch fixes the byte order of the internal address that should be written to the I2C device. Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de> CC: Marek Vasut <marex@denx.de> CC: Stefano Babic <sbabic@denx.de> CC: Fabio Estevam <fabio.estevam@freescale.com> --- drivers/i2c/mxs_i2c.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)