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;
}
?