hw/nvram/eeprom_at24c.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
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;
+ }
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;
--
2.34.1.173.g76aa8bc2d0-goog
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.
© 2016 - 2024 Red Hat, Inc.