| Message ID | 1385971379-3096-5-git-send-email-dantesu@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Heiko Schocher |
| Headers | show |
On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote: > From: Kuo-Jung Su <dantesu@faraday-tech.com> > diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c > index 02539c4..3924805 100644 > --- a/common/cmd_eeprom.c > +++ b/common/cmd_eeprom.c > @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt > #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) > spi_read (addr, alen, buffer, len); > #else > - if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0) > + if (i2c_read(addr[0], offset, alen - 1, buffer, len)) > rcode = 1; > #endif > buffer += len; I think this change is whether incomplete or improper. Let's look at source code above line 161: ============================= 125 #if CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1 && !defined(CONFIG_SPI_X) 126 uchar addr[2]; 127 128 blk_off = offset & 0xFF; /* block offset */ 129 130 addr[0] = offset >> 8; /* block number */ 131 addr[1] = blk_off; /* block offset */ 132 alen = 2; 133 #else 134 uchar addr[3]; 135 136 blk_off = offset & 0xFF; /* block offset */ 137 138 addr[0] = offset >> 16; /* block number */ 139 addr[1] = offset >> 8; /* upper address octet */ 140 addr[2] = blk_off; /* lower address octet */ 141 alen = 3; 142 #endif /* CONFIG_SYS_I2C_EEPROM_ADDR_LEN, CONFIG_SPI_X */ 143 144 addr[0] |= dev_addr; /* insert device address */ ============================= From these line you see that "addr[0]" is set like this: =========== If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1": addr[0] = offset >> 8; If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1": addr[0] = offset >> 16; And in both cases then OR with "dev_addr": addr[0] |= dev_addr; =========== In other words it gets both real I2S slave address + MSB bits of offset. But note that "offset" value stays unchanged. So if you pass both "addr[0]" (which already has MSB bits of "offset") and "offset" itself then you'll get completely incorrect I2C command. > @@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn > /* Write is enabled ... now write eeprom value. > */ > #endif > - if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0) > + if (i2c_write(addr[0], offset, alen - 1, buffer, len)) > rcode = 1; > > #endif Same goes to "eeprom_write". Moreover I'd say that this address/offset tricks are very EEPROM-specific and because of this we'd better keep it here and don't modify generic I2C code. Regards, Alexey
2013/12/2 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote: >> From: Kuo-Jung Su <dantesu@faraday-tech.com> > >> diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c >> index 02539c4..3924805 100644 >> --- a/common/cmd_eeprom.c >> +++ b/common/cmd_eeprom.c >> @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt >> #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) >> spi_read (addr, alen, buffer, len); >> #else >> - if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0) >> + if (i2c_read(addr[0], offset, alen - 1, buffer, len)) >> rcode = 1; >> #endif >> buffer += len; > > I think this change is whether incomplete or improper. > Let's look at source code above line 161: > ============================= > 125 #if CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1 && !defined(CONFIG_SPI_X) > 126 uchar addr[2]; > 127 > 128 blk_off = offset & 0xFF; /* block offset */ > 129 > 130 addr[0] = offset >> 8; /* block number */ > 131 addr[1] = blk_off; /* block offset */ > 132 alen = 2; > 133 #else > 134 uchar addr[3]; > 135 > 136 blk_off = offset & 0xFF; /* block offset */ > 137 > 138 addr[0] = offset >> 16; /* block number */ > 139 addr[1] = offset >> 8; /* upper address > octet */ > 140 addr[2] = blk_off; /* lower address > octet */ > 141 alen = 3; > 142 #endif /* CONFIG_SYS_I2C_EEPROM_ADDR_LEN, CONFIG_SPI_X */ > 143 > 144 addr[0] |= dev_addr; /* insert device > address */ > ============================= > > From these line you see that "addr[0]" is set like this: > =========== > If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1": > addr[0] = offset >> 8; > > If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1": > addr[0] = offset >> 16; > > And in both cases then OR with "dev_addr": > addr[0] |= dev_addr; > =========== > This is the reason why I said: CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW is always enabled inside cmd_eeprom.c so everything is O.K. > In other words it gets both real I2S slave address + MSB bits of offset. > But note that "offset" value stays unchanged. > The comment bellow clearly explain the issue here. soft_i2c.c: line 351 ~ 367: #ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW /* * EEPROM chips that implement "address overflow" are ones * like Catalyst 24WC04/08/16 which has 9/10/11 bits of * address and the extra bits end up in the "chip address" * bit slots. This makes a 24WC08 (1Kbyte) chip look like * four 256 byte chips. * * Note that we consider the length of the address field to * still be one byte because the extra address bits are * hidden in the chip address. */ chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW); PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n", chip, addr); #endif > So if you pass both "addr[0]" (which already has MSB bits of "offset") > and "offset" itself then you'll get completely incorrect I2C command. > >> @@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn >> /* Write is enabled ... now write eeprom value. >> */ >> #endif >> - if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0) >> + if (i2c_write(addr[0], offset, alen - 1, buffer, len)) >> rcode = 1; >> >> #endif > > Same goes to "eeprom_write". > > Moreover I'd say that this address/offset tricks are very > EEPROM-specific and because of this we'd better keep it here and don't > modify generic I2C code. > Yes,the address/offset tricks are device specific (not only EEPROM, it also applies to Audio Codecs..etc.) But this code was there over a decade. And if everything works just fine, why bother ? > Regards, > Alexey >
On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote: > The comment bellow clearly explain the issue here. > > soft_i2c.c: line 351 ~ 367: > > #ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW > /* > * EEPROM chips that implement "address overflow" are ones > * like Catalyst 24WC04/08/16 which has 9/10/11 bits of > * address and the extra bits end up in the "chip address" > * bit slots. This makes a 24WC08 (1Kbyte) chip look like > * four 256 byte chips. > * > * Note that we consider the length of the address field to > * still be one byte because the extra address bits are > * hidden in the chip address. > */ > chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW); > > PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n", > chip, addr); > #endif Indeed comment is pretty clear. But IMHO this code is very generic (how is it bound to any specific device driver?) and because of this I believe it should be in common I2C sources but not in device-specific ones. Otherwise do we need to copy-paste this snippet to all I2C drivers? I do like the code above for modification of slave address ("chip") - for me it looks very clear and CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW makes perfect sense. So why don't we try to push this in generic "eeprom_{read|write}"? > Yes,the address/offset tricks are device specific (not only EEPROM, it > also applies to Audio Codecs..etc.) Saying "device specific" I meant not "I2C driver specific" but "attached I2C slave specific". As you correctly stated this kind of tricky addressing is used by EEPROMs, audio codecs etc. So when we need to work with EEPPROM with "ADDR_OVERFLOW" I expect to enable CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW in configuration. But imagine if the same I2C bus has another slave which expects "normal" I2C addressing. So how then you're going to configure your I2C driver so it correctly works with both slaves? My vision of resolution is like this: I2C driver always work in "normal addressing" mode - just uses "chip" and "address" values as they are, but in higher level code like in ours "cmd_eeprom" we may modify both "chip" and "offset" values for each particular type of attached I2C device. > But this code was there over a decade. And if everything works just > fine, why bother ? Well as it turned out not everything worked that fine. As I discovered "dw_i2c" didn't work because of missing address re-calculation. Indeed I may agree with your previous patch: ===== if (i2c_write(dev_addr, offset, alen - 1, buffer, len)) ===== and implement address modification in "dw_i2c" driver. But still there're questions: 1. Which other drivers will require update? and who's going to check/fix it there? 2. Why do we need all this address modification part in "cmd_eeprom.c"? And if we don't need - who's going to clean this up? BTW what I cannot understand is why "soft_i2c_read" has this "chip" modification part while "soft_i2c_write" doesn't? Is it done on purpose? And how it actually works at all then? Regards, Alexey
Hello Kuo-jung, Am 02.12.2013 09:02, schrieb Kuo-Jung Su: > From: Kuo-Jung Su<dantesu@faraday-tech.com> > > The local pointer of address (i.e., addr) only gets > referenced under SPI mode, and it won't be appropriate > to pass only 1-byte addr[1] to i2c_read/i2c_write while > CONFIG_SYS_I2C_EEPROM_ADDR_LEN> 1. > > 1. In U-boot's I2C model, the address would be re-assembled > to a byte string in MSB order inside I2C controller drivers. > > 2. The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' option which could > be found at soft_i2c.c is always turned on in cmd_eeprom.c, > the addr[0] always contains the device address with overflowed > MSB address bits. > > Signed-off-by: Kuo-Jung Su<dantesu@faraday-tech.com> > Cc: Alexey Brodkin<abrodkin@synopsys.com> > Cc: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com> > cc: Peter Tyser<ptyser@xes-inc.com> > Cc: Heiko Schocher<hs@denx.de> > Cc: Wolfgang Denk<wd@denx.de> > Cc: Stefan Roese<sr@denx.de> > Cc: Mischa Jonker<mjonker@synopsys.com> > > --- > Changes for v3: > - It turns out that what we did before 2013-11-13 > (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM) > is still the best one, this patch simply rollback to it with coding > style fix. > > Changes for v2: > - Initial release > > common/cmd_eeprom.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Applied to u-boot.i2c.git, thanks! bye, Heiko
Hi Heiko, On Mon, 2013-12-09 at 07:56 +0100, Heiko Schocher wrote: > Applied to u-boot.i2c.git, thanks! > I'm wondering if you've seen a discussion between me and Kuo-jung regarding this patch and consequences of it being applied. Do you mind to comment on questions we discussed there? My main concern is how to keep underlying I2C drivers working because as I wrote some of them will not function any more correctly. Regards, Alexey
Hello Alexey, Am 09.12.2013 11:35, schrieb Alexey Brodkin: > Hi Heiko, > > On Mon, 2013-12-09 at 07:56 +0100, Heiko Schocher wrote: > >> Applied to u-boot.i2c.git, thanks! >> > > I'm wondering if you've seen a discussion between me and Kuo-jung > regarding this patch and consequences of it being applied. > > Do you mind to comment on questions we discussed there? Sorry, seems I missed this ... > My main concern is how to keep underlying I2C drivers working because as > I wrote some of them will not function any more correctly. I thought the v3 patch just rolls things back as patch comment states: ------------------------ Changes for v3: - It turns out that what we did before 2013-11-13 (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM) is still the best one, this patch simply rollback to it with coding style fix. ------------------------ Without this roll back, I think, some boards are broken now in current mainline... so first we should go this step back. Next step, drivers who do not work, should be fixed if needed, or: You wrote: on Alexey Brodkin - Dec. 3, 2013, 7:42 a.m. wrote: > On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote: >> The comment bellow clearly explain the issue here. > [...] >> But this code was there over a decade. And if everything works just >> fine, why bother ? > > Well as it turned out not everything worked that fine. As I discovered > "dw_i2c" didn't work because of missing address re-calculation. > > Indeed I may agree with your previous patch: > ===== > if (i2c_write(dev_addr, offset, alen - 1, buffer, len)) > ===== > and implement address modification in "dw_i2c" driver. Hmm.. this is real ancient code ... seems, until now, nobody had problems with it ... if you see problems now in dw_i2c driver, you have two possibilites: - fix the dw_i2c driver - The best way would really be to rework this, as you stated: "My vision of resolution is like this: I2C driver always work in "normal addressing" mode - just uses "chip" and "address" values as they are, but in higher level code like in ours "cmd_eeprom" we may modify both "chip" and "offset" values for each particular type of attached I2C device." This touches I think a lot of boards ... the problem would be, how to test this change... Nevertheless, this should be done in a new thread ... > But still there're questions: > 1. Which other drivers will require update? and who's going to check/fix > it there? Yes thats the problem. For the "old state" this should be done, if new boards need it and fix the i2c driver. If you want to clean up this, as you mentioned above, this touches maybe a lot of current i2c drivers (drivers who do this address modification) So, this must be done carefully, and you/we need some board maintainers which can test it on their boards. > 2. Why do we need all this address modification part in "cmd_eeprom.c"? > And if we don't need - who's going to clean this up? The first who have issues with it? Or you volunteer to send a cleanup patch? > BTW what I cannot understand is why "soft_i2c_read" has this "chip" > modification part while "soft_i2c_write" doesn't? Is it done on purpose? > And how it actually works at all then? Good question ... maybe currently only used on i2c reads ? bye, Heiko
On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote: > I thought the v3 patch just rolls things back as patch comment states: > ------------------------ > Changes for v3: > - It turns out that what we did before 2013-11-13 > (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM) > is still the best one, this patch simply rollback to it with coding > style fix. > ------------------------ > > Without this roll back, I think, some boards are broken now in current > mainline... so first we should go this step back. I'd say it's very questionable. For example if you look at I2C driver that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch applied (the one you've just rolled back) it will work flawlessly because: 1. "chip" address there has double applied MSB bits of offset (the first time it gets applied in "cmd_eeprom") 2. Only LSB byte of offset gets written in I2C device if addr_len = 1. This one makes IMHO much more sense because it excludes an extra "chip" address modification - so it's clear that it will be done by particular I2C driver so people won't be confused with their expectations. > > BTW what I cannot understand is why "soft_i2c_read" has this "chip" > > modification part while "soft_i2c_write" doesn't? Is it done on purpose? > > And how it actually works at all then? > > Good question ... maybe currently only used on i2c reads ? Frankly I have only 1 supposition regarding this strange state of things in "soft_i2c_write". Because without any doubts the same modification of "chip" address is applicable to both "read" and "write" because it is an integral part of data addressing. But due to discussed a lot in this thread "double chip address modification" (application of MSB bits of offset) proper support of "chip" address was never implemented in "soft_i2c_write". As you correctly mentioned - "real ancient code" works and nobody has problems with it. Well, just because we have current implementation of "eeprom_write" that does "chip" address modification "soft_i2c" may not have this feature in both "read" and "write". So I'd prefer to go with previous version of patch sent by Kuo-Jung http://patchwork.ozlabs.org/patch/294733/ And indeed this will "break" functionality of currently incomplete implementation of "soft_i2c_write". Regards, Alexey
2013/12/10 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote: >> I thought the v3 patch just rolls things back as patch comment states: >> ------------------------ >> Changes for v3: >> - It turns out that what we did before 2013-11-13 >> (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM) >> is still the best one, this patch simply rollback to it with coding >> style fix. >> ------------------------ >> >> Without this roll back, I think, some boards are broken now in current >> mainline... so first we should go this step back. > > I'd say it's very questionable. For example if you look at I2C driver > that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch > applied (the one you've just rolled back) it will work flawlessly > because: > 1. "chip" address there has double applied MSB bits of offset (the first > time it gets applied in "cmd_eeprom") > 2. Only LSB byte of offset gets written in I2C device if addr_len = 1. But if addr_len = 2, everything goes wrong. This is the real problem here... > > This one makes IMHO much more sense because it excludes an extra "chip" > address modification - so it's clear that it will be done by particular > I2C driver so people won't be confused with their expectations. > >> > BTW what I cannot understand is why "soft_i2c_read" has this "chip" >> > modification part while "soft_i2c_write" doesn't? Is it done on purpose? >> > And how it actually works at all then? >> >> Good question ... maybe currently only used on i2c reads ? > > Frankly I have only 1 supposition regarding this strange state of things > in "soft_i2c_write". Because without any doubts the same modification of > "chip" address is applicable to both "read" and "write" because it is an > integral part of data addressing. > > But due to discussed a lot in this thread "double chip address > modification" (application of MSB bits of offset) proper support of > "chip" address was never implemented in "soft_i2c_write". As you > correctly mentioned - "real ancient code" works and nobody has problems > with it. Well, just because we have current implementation of > "eeprom_write" that does "chip" address modification "soft_i2c" may not > have this feature in both "read" and "write". > > So I'd prefer to go with previous version of patch sent by Kuo-Jung > http://patchwork.ozlabs.org/patch/294733/ > > And indeed this will "break" functionality of currently incomplete > implementation of "soft_i2c_write". > No, it will works just fine. 1. eeprom_read: The overflowed address was shifted to chip address by cmd_eeprom.c, and then the same operations would be repeated by soft_i2c.c, but it's no harm to do a OR operation twice on the same bits, so everything is all right. 2. eeprom_write: The overflowed address was shifted to chip address by cmd_eeprom.c, and get passed to soft_i2c.c without further modification. So everything is all right. The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' in soft_i2c.c is actually a redundant code snippet. And it would be better to move/copied its comment to cmd_eeprom.c. Which means if we don't rollback, none of the EEPROMs with addr_len > 1 will work on u-boot. And this rollback actually does no harm to the EEPROMs with addr_len = 1 or 2. P.S: The designware_i2c.c should work just fine with this rollback, too....
diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c index 02539c4..3924805 100644 --- a/common/cmd_eeprom.c +++ b/common/cmd_eeprom.c @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C) spi_read (addr, alen, buffer, len); #else - if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0) + if (i2c_read(addr[0], offset, alen - 1, buffer, len)) rcode = 1; #endif buffer += len; @@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn /* Write is enabled ... now write eeprom value. */ #endif - if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0) + if (i2c_write(addr[0], offset, alen - 1, buffer, len)) rcode = 1; #endif