| Message ID | 20211220003240.1081986-1-venture@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/nvram: at24 return 0xff if 1 byte address | expand |
Hi Patrick, On 12/20/21 01:32, Patrick Venture wrote: > The at24 eeproms are 2 byte devices that return 0xff when they are read > from with a partial (1-byte) address written. This distinction was > found comparing model behavior to real hardware testing. > > Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next > byte > > Signed-off-by: Patrick Venture <venture@google.com> > --- > hw/nvram/eeprom_at24c.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c > index a9e3702b00..184fac9702 100644 > --- a/hw/nvram/eeprom_at24c.c > +++ b/hw/nvram/eeprom_at24c.c > @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event) > case I2C_START_SEND: > case I2C_START_RECV: > case I2C_FINISH: > - ee->haveaddr = 0; > + if (event != I2C_START_RECV) { > + ee->haveaddr = 0; > + } Alternatively (matter of taste, I find it easier to read): case I2C_START_SEND: case I2C_FINISH: ee->haveaddr = 0; /* fallthrough */ case I2C_START_RECV: > DPRINTK("clear\n"); > if (ee->blk && ee->changed) { > int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0); > @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) > EEPROMState *ee = AT24C_EE(s); > uint8_t ret; > > + if (ee->haveaddr == 1) { > + return 0xff; Don't we need to increase ee->haveaddr? > + } > + > ret = ee->mem[ee->cur]; > > ee->cur = (ee->cur + 1u) % ee->rsize; Here for parity with send, what about: if (ee->haveaddr < 2) { ret = 0xff; ee->haveaddr++; } else { ret = ee->mem[ee->cur]; ee->cur = (ee->cur + 1u) % ee->rsize; } ?
On Mon, Dec 20, 2021 at 1:12 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Patrick, > > On 12/20/21 01:32, Patrick Venture wrote: > > The at24 eeproms are 2 byte devices that return 0xff when they are read > > from with a partial (1-byte) address written. This distinction was > > found comparing model behavior to real hardware testing. > > > > Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next > > byte > > > > Signed-off-by: Patrick Venture <venture@google.com> > > --- > > hw/nvram/eeprom_at24c.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c > > index a9e3702b00..184fac9702 100644 > > --- a/hw/nvram/eeprom_at24c.c > > +++ b/hw/nvram/eeprom_at24c.c > > @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event > event) > > case I2C_START_SEND: > > case I2C_START_RECV: > > case I2C_FINISH: > > - ee->haveaddr = 0; > > + if (event != I2C_START_RECV) { > > + ee->haveaddr = 0; > > + } > > Alternatively (matter of taste, I find it easier to read): > > case I2C_START_SEND: > case I2C_FINISH: > ee->haveaddr = 0; > /* fallthrough */ > case I2C_START_RECV: > That may be easier to read :) I'm not sure, but I'm willing to bend and change my patch to behave this way. Sometimes the fallthrough things leads to compiler annoyances in my experience. We might need __attribute__(fallthrough) or the like to convince the system that's what we really want. > > > DPRINTK("clear\n"); > > if (ee->blk && ee->changed) { > > int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0); > > @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) > > EEPROMState *ee = AT24C_EE(s); > > uint8_t ret; > > > > + if (ee->haveaddr == 1) { > > + return 0xff; > > Don't we need to increase ee->haveaddr? > We don't because the call to recv doesn't set any addr bytes. This patch is primarily a behavioral fix to handle the device being treated as 8-bit addressable. This is typically tested by writing a 1 byte address and then trying to read. The chip itself will not have enough address bytes and reject this read by returning 0xff. The haveaddr variable is strictly updated when they've written another byte to the address, or they've changed states in such a way that should clear any previously written address. You can read from an eeprom by just reading or by setting an address and then reading. > > > + } > > + > > ret = ee->mem[ee->cur]; > > > > ee->cur = (ee->cur + 1u) % ee->rsize; > > Here for parity with send, what about: > > if (ee->haveaddr < 2) { > ret = 0xff; > ee->haveaddr++; > } else { > ret = ee->mem[ee->cur]; > ee->cur = (ee->cur + 1u) % ee->rsize; > } > > ? > >
On 12/20/21 16:32, Patrick Venture wrote: > On Mon, Dec 20, 2021 at 1:12 AM Philippe Mathieu-Daudé > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote: > > Hi Patrick, > > On 12/20/21 01:32, Patrick Venture wrote: > > The at24 eeproms are 2 byte devices that return 0xff when they are > read > > from with a partial (1-byte) address written. This distinction was > > found comparing model behavior to real hardware testing. > > > > Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next > > byte > > > > Signed-off-by: Patrick Venture <venture@google.com > <mailto:venture@google.com>> > > --- > > hw/nvram/eeprom_at24c.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c > > index a9e3702b00..184fac9702 100644 > > --- a/hw/nvram/eeprom_at24c.c > > +++ b/hw/nvram/eeprom_at24c.c > > @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum > i2c_event event) > > case I2C_START_SEND: > > case I2C_START_RECV: > > case I2C_FINISH: > > - ee->haveaddr = 0; > > + if (event != I2C_START_RECV) { > > + ee->haveaddr = 0; > > + } > > Alternatively (matter of taste, I find it easier to read): > > case I2C_START_SEND: > case I2C_FINISH: > ee->haveaddr = 0; > /* fallthrough */ > case I2C_START_RECV: > > > That may be easier to read :) I'm not sure, but I'm willing to bend and > change my patch to behave this way. Sometimes the fallthrough things > leads to compiler annoyances in my experience. We might need > __attribute__(fallthrough) or the like to convince the system that's > what we really want. OK then. > > > > DPRINTK("clear\n"); > > if (ee->blk && ee->changed) { > > int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0); > > @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) > > EEPROMState *ee = AT24C_EE(s); > > uint8_t ret; > > > > + if (ee->haveaddr == 1) { > > + return 0xff; > > Don't we need to increase ee->haveaddr? > > > We don't because the call to recv doesn't set any addr bytes. This > patch is primarily a behavioral fix to handle the device being treated > as 8-bit addressable. This is typically tested by writing a 1 byte > address and then trying to read. The chip itself will not have enough > address bytes and reject this read by returning 0xff. The > haveaddr variable is strictly updated when they've written another byte > to the address, or they've changed states in such a way that should > clear any previously written address. You can read from an eeprom by > just reading or by setting an address and then reading. Yes. And your approach is simple enough. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks, Phil.
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c index a9e3702b00..184fac9702 100644 --- a/hw/nvram/eeprom_at24c.c +++ b/hw/nvram/eeprom_at24c.c @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event) case I2C_START_SEND: case I2C_START_RECV: case I2C_FINISH: - ee->haveaddr = 0; + if (event != I2C_START_RECV) { + ee->haveaddr = 0; + } DPRINTK("clear\n"); if (ee->blk && ee->changed) { int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0); @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) EEPROMState *ee = AT24C_EE(s); uint8_t ret; + if (ee->haveaddr == 1) { + return 0xff; + } + ret = ee->mem[ee->cur]; ee->cur = (ee->cur + 1u) % ee->rsize;
The at24 eeproms are 2 byte devices that return 0xff when they are read from with a partial (1-byte) address written. This distinction was found comparing model behavior to real hardware testing. Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next byte Signed-off-by: Patrick Venture <venture@google.com> --- hw/nvram/eeprom_at24c.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)