[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/5] hw/nvram/eeprom_at24c: Make reset behavior more like
From: |
Peter Delevoryas |
Subject: |
Re: [PATCH v2 5/5] hw/nvram/eeprom_at24c: Make reset behavior more like hardware |
Date: |
Tue, 17 Jan 2023 10:29:52 -0800 |
On Tue, Jan 17, 2023 at 08:42:46AM +0100, Cédric Le Goater wrote:
> On 1/17/23 00:56, Peter Delevoryas wrote:
> > EEPROM's are a form of non-volatile memory. After power-cycling an EEPROM,
> > I would expect the I2C state machine to be reset to default values, but I
> > wouldn't really expect the memory to change at all.
>
>
> >
> > The current implementation of the at24c EEPROM resets its internal memory on
> > reset. This matches the specification in docs/devel/reset.rst:
> >
> > Cold reset is supported by every resettable object. In QEMU, it means we
> > reset
> > to the initial state corresponding to the start of QEMU; this might
> > differ
> > from what is a real hardware cold reset. It differs from other resets
> > (like
> > warm or bus resets) which may keep certain parts untouched.
> >
> > But differs from my intuition. For example, if someone writes some
> > information
> > to an EEPROM, then AC power cycles their board, they would expect the
> > EEPROM to
> > retain that information. It's very useful to be able to test things like
> > this
> > in QEMU as well, to verify software instrumentation like determining the
> > cause
> > of a reboot.
>
> Yes. should we take into account the "writable" property ? It is not set to
> false in any model but it could.
We're already using "writable" in at24c_eeprom_send to control writes, do we
need to use it anywhere else? I would assume we shouldn't use it in init/reset,
but maybe you have something specific in mind.
>
> Thanks,
>
> C.
>
> > Fixes: 5d8424dbd3e8 ("nvram: add AT24Cx i2c eeprom")
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > ---
> > hw/nvram/eeprom_at24c.c | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> > index bb9ee75864fe..6bcded7b496c 100644
> > --- a/hw/nvram/eeprom_at24c.c
> > +++ b/hw/nvram/eeprom_at24c.c
> > @@ -185,18 +185,6 @@ static void at24c_eeprom_realize(DeviceState *dev,
> > Error **errp)
> > }
> > ee->mem = g_malloc0(ee->rsize);
> > -
> > -}
> > -
> > -static
> > -void at24c_eeprom_reset(DeviceState *state)
> > -{
> > - EEPROMState *ee = AT24C_EE(state);
> > -
> > - ee->changed = false;
> > - ee->cur = 0;
> > - ee->haveaddr = 0;
> > -
> > memset(ee->mem, 0, ee->rsize);
> > if (ee->blk) {
> > @@ -214,6 +202,16 @@ void at24c_eeprom_reset(DeviceState *state)
> > }
> > }
> > +static
> > +void at24c_eeprom_reset(DeviceState *state)
> > +{
> > + EEPROMState *ee = AT24C_EE(state);
> > +
> > + ee->changed = false;
> > + ee->cur = 0;
> > + ee->haveaddr = 0;
> > +}
> > +
> > static Property at24c_eeprom_props[] = {
> > DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
> > DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
>