Message ID | 20230118024214.14413-6-peter@pjd.dev |
---|---|
State | New |
Headers | show |
Series | hw/nvram/eeprom_at24c: Cleanup + FRUID EEPROM init example | expand |
On 1/18/23 03:42, 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. > > Fixes: 5d8424dbd3e8 ("nvram: add AT24Cx i2c eeprom") > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > Reviewed-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > 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 f8d751fa278d..5074776bff04 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->init_rom) { > @@ -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),
On Tue, Jan 17, 2023 at 06:42:14PM -0800, 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. Yes, I agree, I was actually wondering about this reviewing earlier changes. Thanks for fixing this. Reviewed-by: Corey Minyard <cminyard@mvista.com> > > 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. > > Fixes: 5d8424dbd3e8 ("nvram: add AT24Cx i2c eeprom") > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > 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 f8d751fa278d..5074776bff04 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->init_rom) { > @@ -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), > -- > 2.39.0 > >
On Wed, Jan 25, 2023 at 03:41:30PM -0600, Corey Minyard wrote: > On Tue, Jan 17, 2023 at 06:42:14PM -0800, 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. > > Yes, I agree, I was actually wondering about this reviewing earlier > changes. Thanks for fixing this. Oh great! I'm glad everyone has agreed with this so far. - Peter > > Reviewed-by: Corey Minyard <cminyard@mvista.com> > > > > > 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. > > > > Fixes: 5d8424dbd3e8 ("nvram: add AT24Cx i2c eeprom") > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > --- > > 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 f8d751fa278d..5074776bff04 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->init_rom) { > > @@ -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), > > -- > > 2.39.0 > > > >
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c index f8d751fa278d..5074776bff04 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->init_rom) { @@ -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),