On 12/20/21 16:32, Patrick Venture wrote:
> On Mon, Dec 20, 2021 at 1:12 AM Philippe Mathieu-Daudé
> <phi...@redhat.com <mailto:phi...@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 <vent...@google.com
> <mailto:vent...@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é <phi...@redhat.com>
Thanks,
Phil.
