| Message ID | 1450270635-27080-3-git-send-email-marcin.krzeminski@nokia.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 16, 2015 at 4:57 AM, <marcin.krzeminski@nokia.com> wrote: > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > --- > hw/block/m25p80.c | 38 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index bfd493f..bcb66a5 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -258,6 +258,8 @@ typedef struct Flash { > uint8_t cmd_in_progress; > uint64_t cur_addr; > bool write_enable; > + bool initialized; > + uint8_t reset_pin; > > int64_t dirty_page; > > @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s) > } > } > > +static void reset_memory(Flash *s) > +{ > + s->cmd_in_progress = NOP; > + s->cur_addr = 0; > + s->len = 0; > + s->needed_bytes = 0; > + s->pos = 0; > + s->state = STATE_IDLE; > + s->write_enable = false; > + > + DB_PRINT_L(0, "Reset done.\n"); > +} > + > static void decode_new_cmd(Flash *s, uint32_t value) > { > s->cmd_in_progress = value; > @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss) > s->size = s->pi->sector_size * s->pi->n_sectors; > s->dirty_page = -1; > > + s->initialized = false; > + Init functions should never set runtime state. > /* FIXME use a qdev drive property instead of drive_get_next() */ > dinfo = drive_get_next(IF_MTD); > > @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque) > flash_sync_dirty((Flash *)opaque, -1); > } > > +static void m25p80_reset(DeviceState *d) > +{ > + Flash *s = M25P80(d); > + > + if (s->reset_pin || !s->initialized) { > + reset_memory(s); > + s->initialized = true; Bus I think the real issue here is that is trying to model something other than a power-on-reset. Looks like a a soft reset. the dc->reset should be a power-on-reset and not be conditional on anything. Only the non-volatile state (the storage data) should persist. Is your board using qemu_system_reset_request() by any chance for something that is not supposed to reset the whole board? If this is the case, you need to limit the scope of that reset logic and just set your board up to not use the centralized all-devices reset (which is pretty broken for these modern boards/SoCs that have multiple reset domains). Regards, Peter > + } > +} > + > +static Property m25p80_properties[] = { > + DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static const VMStateDescription vmstate_m25p80 = { > .name = "xilinx_spi", > - .version_id = 1, > + .version_id = 2, > .minimum_version_id = 1, > .pre_save = m25p80_pre_save, > .fields = (VMStateField[]) { > @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = { > VMSTATE_UINT8(cmd_in_progress, Flash), > VMSTATE_UINT64(cur_addr, Flash), > VMSTATE_BOOL(write_enable, Flash), > + VMSTATE_BOOL(initialized, Flash), > + VMSTATE_UINT8(reset_pin, Flash), > VMSTATE_END_OF_LIST() > } > }; > @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void *data) > k->set_cs = m25p80_cs; > k->cs_polarity = SSI_CS_LOW; > dc->vmsd = &vmstate_m25p80; > + dc->reset = &m25p80_reset; > + dc->props = m25p80_properties; > mc->pi = data; > } > > -- > 2.5.0 > >
W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze: > On Wed, Dec 16, 2015 at 4:57 AM, <marcin.krzeminski@nokia.com> wrote: >> From: Marcin Krzeminski <marcin.krzeminski@nokia.com> >> >> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> >> --- >> hw/block/m25p80.c | 38 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index bfd493f..bcb66a5 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -258,6 +258,8 @@ typedef struct Flash { >> uint8_t cmd_in_progress; >> uint64_t cur_addr; >> bool write_enable; >> + bool initialized; >> + uint8_t reset_pin; >> >> int64_t dirty_page; >> >> @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s) >> } >> } >> >> +static void reset_memory(Flash *s) >> +{ >> + s->cmd_in_progress = NOP; >> + s->cur_addr = 0; >> + s->len = 0; >> + s->needed_bytes = 0; >> + s->pos = 0; >> + s->state = STATE_IDLE; >> + s->write_enable = false; >> + >> + DB_PRINT_L(0, "Reset done.\n"); >> +} >> + >> static void decode_new_cmd(Flash *s, uint32_t value) >> { >> s->cmd_in_progress = value; >> @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss) >> s->size = s->pi->sector_size * s->pi->n_sectors; >> s->dirty_page = -1; >> >> + s->initialized = false; >> + > > Init functions should never set runtime state. > >> /* FIXME use a qdev drive property instead of drive_get_next() */ >> dinfo = drive_get_next(IF_MTD); >> >> @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque) >> flash_sync_dirty((Flash *)opaque, -1); >> } >> >> +static void m25p80_reset(DeviceState *d) >> +{ >> + Flash *s = M25P80(d); >> + >> + if (s->reset_pin || !s->initialized) { >> + reset_memory(s); >> + s->initialized = true; > > Bus I think the real issue here is that is trying to model something > other than a power-on-reset. Looks like a a soft reset. the dc->reset > should be a power-on-reset and not be conditional on anything. Only > the non-volatile state (the storage data) should persist. > > Is your board using qemu_system_reset_request() by any chance for > something that is not supposed to reset the whole board? If this is > the case, you need to limit the scope of that reset logic and just set > your board up to not use the centralized all-devices reset (which is > pretty broken for these modern boards/SoCs that have multiple reset > domains). > > Regards, > Peter That is not exactly my case. I want to emulate device that have a RESET signal functionality (eg. n25q128 has such option). The other think that I want to have is possibility to use this signal by qemu model (eg. pin is connected on board to reset IC, or it is unused). The use case: qemu boots us powered-up board, then I issue qemu_system_reset_request(). Then if the pin is connected (propeti is set, flash wil also reset, in other case will not). I don't see why it against reset signal implementation. Regards, Marcin > >> + } >> +} >> + >> +static Property m25p80_properties[] = { >> + DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static const VMStateDescription vmstate_m25p80 = { >> .name = "xilinx_spi", >> - .version_id = 1, >> + .version_id = 2, >> .minimum_version_id = 1, >> .pre_save = m25p80_pre_save, >> .fields = (VMStateField[]) { >> @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = { >> VMSTATE_UINT8(cmd_in_progress, Flash), >> VMSTATE_UINT64(cur_addr, Flash), >> VMSTATE_BOOL(write_enable, Flash), >> + VMSTATE_BOOL(initialized, Flash), >> + VMSTATE_UINT8(reset_pin, Flash), >> VMSTATE_END_OF_LIST() >> } >> }; >> @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void *data) >> k->set_cs = m25p80_cs; >> k->cs_polarity = SSI_CS_LOW; >> dc->vmsd = &vmstate_m25p80; >> + dc->reset = &m25p80_reset; >> + dc->props = m25p80_properties; >> mc->pi = data; >> } >> >> -- >> 2.5.0 >> >> > >
On Mon, Dec 21, 2015 at 5:39 AM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > > W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze: >> On Wed, Dec 16, 2015 at 4:57 AM, <marcin.krzeminski@nokia.com> wrote: >>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com> >>> >>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> >>> --- >>> hw/block/m25p80.c | 38 +++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 37 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >>> index bfd493f..bcb66a5 100644 >>> --- a/hw/block/m25p80.c >>> +++ b/hw/block/m25p80.c >>> @@ -258,6 +258,8 @@ typedef struct Flash { >>> uint8_t cmd_in_progress; >>> uint64_t cur_addr; >>> bool write_enable; >>> + bool initialized; >>> + uint8_t reset_pin; >>> >>> int64_t dirty_page; >>> >>> @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s) >>> } >>> } >>> >>> +static void reset_memory(Flash *s) >>> +{ >>> + s->cmd_in_progress = NOP; >>> + s->cur_addr = 0; >>> + s->len = 0; >>> + s->needed_bytes = 0; >>> + s->pos = 0; >>> + s->state = STATE_IDLE; >>> + s->write_enable = false; >>> + >>> + DB_PRINT_L(0, "Reset done.\n"); >>> +} >>> + >>> static void decode_new_cmd(Flash *s, uint32_t value) >>> { >>> s->cmd_in_progress = value; >>> @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss) >>> s->size = s->pi->sector_size * s->pi->n_sectors; >>> s->dirty_page = -1; >>> >>> + s->initialized = false; >>> + >> >> Init functions should never set runtime state. >> >>> /* FIXME use a qdev drive property instead of drive_get_next() */ >>> dinfo = drive_get_next(IF_MTD); >>> >>> @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque) >>> flash_sync_dirty((Flash *)opaque, -1); >>> } >>> >>> +static void m25p80_reset(DeviceState *d) >>> +{ >>> + Flash *s = M25P80(d); >>> + >>> + if (s->reset_pin || !s->initialized) { >>> + reset_memory(s); >>> + s->initialized = true; >> >> Bus I think the real issue here is that is trying to model something >> other than a power-on-reset. Looks like a a soft reset. the dc->reset >> should be a power-on-reset and not be conditional on anything. Only >> the non-volatile state (the storage data) should persist. >> >> Is your board using qemu_system_reset_request() by any chance for >> something that is not supposed to reset the whole board? If this is >> the case, you need to limit the scope of that reset logic and just set >> your board up to not use the centralized all-devices reset (which is >> pretty broken for these modern boards/SoCs that have multiple reset >> domains). >> >> Regards, >> Peter > That is not exactly my case. > I want to emulate device that have a RESET signal functionality (eg. n25q128 has such option). > The other think that I want to have is possibility to use this signal by qemu model > (eg. pin is connected on board to reset IC, or it is unused). > The use case: qemu boots us powered-up board, then I issue qemu_system_reset_request(). > Then if the pin is connected (propeti is set, flash wil also reset, in other case will not). The "will not" case is what is divergent I think. If the flash does not reset itself through qemu_system_reset_request, it is not a power-on reset. I would avoid using qemu_system_reset_request() in the case you mention and model the DC::reset as application of power to the flash chip in isolation. The signal-driven reset you want to model can share code with it but there should be no reliance on the init function setting initial state or the reset changing behaviour based on volatile state. Regards, Peter > I don't see why it against reset signal implementation. > > Regards, > Marcin > >> >>> + } >>> +} >>> + >>> +static Property m25p80_properties[] = { >>> + DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> static const VMStateDescription vmstate_m25p80 = { >>> .name = "xilinx_spi", >>> - .version_id = 1, >>> + .version_id = 2, >>> .minimum_version_id = 1, >>> .pre_save = m25p80_pre_save, >>> .fields = (VMStateField[]) { >>> @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = { >>> VMSTATE_UINT8(cmd_in_progress, Flash), >>> VMSTATE_UINT64(cur_addr, Flash), >>> VMSTATE_BOOL(write_enable, Flash), >>> + VMSTATE_BOOL(initialized, Flash), >>> + VMSTATE_UINT8(reset_pin, Flash), >>> VMSTATE_END_OF_LIST() >>> } >>> }; >>> @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void *data) >>> k->set_cs = m25p80_cs; >>> k->cs_polarity = SSI_CS_LOW; >>> dc->vmsd = &vmstate_m25p80; >>> + dc->reset = &m25p80_reset; >>> + dc->props = m25p80_properties; >>> mc->pi = data; >>> } >>> >>> -- >>> 2.5.0 >>> >>> >> >> >
> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > Sent: Monday, February 01, 2016 7:29 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > Cc: Peter Maydell; qemu-devel@nongnu.org Developers; Lenkow, Pawel > (Nokia - PL/Wroclaw) > Subject: Re: [Qemu-devel] [PATCH 02/12] Added reset-pin emulation in > model. > > On Mon, Dec 21, 2015 at 5:39 AM, Krzeminski, Marcin (Nokia - > PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > > > > > W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze: > >> On Wed, Dec 16, 2015 at 4:57 AM, <marcin.krzeminski@nokia.com> > wrote: > >>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > >>> > >>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > >>> --- > >>> hw/block/m25p80.c | 38 > +++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 37 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > >>> bfd493f..bcb66a5 100644 > >>> --- a/hw/block/m25p80.c > >>> +++ b/hw/block/m25p80.c > >>> @@ -258,6 +258,8 @@ typedef struct Flash { > >>> uint8_t cmd_in_progress; > >>> uint64_t cur_addr; > >>> bool write_enable; > >>> + bool initialized; > >>> + uint8_t reset_pin; > >>> > >>> int64_t dirty_page; > >>> > >>> @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s) > >>> } > >>> } > >>> > >>> +static void reset_memory(Flash *s) > >>> +{ > >>> + s->cmd_in_progress = NOP; > >>> + s->cur_addr = 0; > >>> + s->len = 0; > >>> + s->needed_bytes = 0; > >>> + s->pos = 0; > >>> + s->state = STATE_IDLE; > >>> + s->write_enable = false; > >>> + > >>> + DB_PRINT_L(0, "Reset done.\n"); } > >>> + > >>> static void decode_new_cmd(Flash *s, uint32_t value) { > >>> s->cmd_in_progress = value; > >>> @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss) > >>> s->size = s->pi->sector_size * s->pi->n_sectors; > >>> s->dirty_page = -1; > >>> > >>> + s->initialized = false; > >>> + > >> > >> Init functions should never set runtime state. > >> > >>> /* FIXME use a qdev drive property instead of drive_get_next() */ > >>> dinfo = drive_get_next(IF_MTD); > >>> > >>> @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque) > >>> flash_sync_dirty((Flash *)opaque, -1); } > >>> > >>> +static void m25p80_reset(DeviceState *d) { > >>> + Flash *s = M25P80(d); > >>> + > >>> + if (s->reset_pin || !s->initialized) { > >>> + reset_memory(s); > >>> + s->initialized = true; > >> > >> Bus I think the real issue here is that is trying to model something > >> other than a power-on-reset. Looks like a a soft reset. the dc->reset > >> should be a power-on-reset and not be conditional on anything. Only > >> the non-volatile state (the storage data) should persist. > >> > >> Is your board using qemu_system_reset_request() by any chance for > >> something that is not supposed to reset the whole board? If this is > >> the case, you need to limit the scope of that reset logic and just > >> set your board up to not use the centralized all-devices reset (which > >> is pretty broken for these modern boards/SoCs that have multiple > >> reset domains). > >> > >> Regards, > >> Peter > > That is not exactly my case. > > I want to emulate device that have a RESET signal functionality (eg. n25q128 > has such option). > > The other think that I want to have is possibility to use this signal > > by qemu model (eg. pin is connected on board to reset IC, or it is unused). > > The use case: qemu boots us powered-up board, then I issue > qemu_system_reset_request(). > > Then if the pin is connected (propeti is set, flash wil also reset, in other case > will not). > > The "will not" case is what is divergent I think. If the flash does not reset itself > through qemu_system_reset_request, it is not a power-on reset. I would > avoid using qemu_system_reset_request() in the case you mention and > model the DC::reset as application of power to the flash chip in isolation. The > signal-driven reset you want to model can share code with it but there > should be no reliance on the init function setting initial state or the reset > changing behaviour based on volatile state. Ok, fair enough. For next version I will model typical reset then. Thanks, Marcin > > Regards, > Peter > > > I don't see why it against reset signal implementation. > > > > Regards, > > Marcin > > > >> > >>> + } > >>> +} > >>> + > >>> +static Property m25p80_properties[] = { > >>> + DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0), > >>> + DEFINE_PROP_END_OF_LIST(), > >>> +}; > >>> + > >>> static const VMStateDescription vmstate_m25p80 = { > >>> .name = "xilinx_spi", > >>> - .version_id = 1, > >>> + .version_id = 2, > >>> .minimum_version_id = 1, > >>> .pre_save = m25p80_pre_save, > >>> .fields = (VMStateField[]) { > >>> @@ -664,6 +696,8 @@ static const VMStateDescription > vmstate_m25p80 = { > >>> VMSTATE_UINT8(cmd_in_progress, Flash), > >>> VMSTATE_UINT64(cur_addr, Flash), > >>> VMSTATE_BOOL(write_enable, Flash), > >>> + VMSTATE_BOOL(initialized, Flash), > >>> + VMSTATE_UINT8(reset_pin, Flash), > >>> VMSTATE_END_OF_LIST() > >>> } > >>> }; > >>> @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, > void *data) > >>> k->set_cs = m25p80_cs; > >>> k->cs_polarity = SSI_CS_LOW; > >>> dc->vmsd = &vmstate_m25p80; > >>> + dc->reset = &m25p80_reset; > >>> + dc->props = m25p80_properties; > >>> mc->pi = data; > >>> } > >>> > >>> -- > >>> 2.5.0 > >>> > >>> > >> > >> > >
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index bfd493f..bcb66a5 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -258,6 +258,8 @@ typedef struct Flash { uint8_t cmd_in_progress; uint64_t cur_addr; bool write_enable; + bool initialized; + uint8_t reset_pin; int64_t dirty_page; @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s) } } +static void reset_memory(Flash *s) +{ + s->cmd_in_progress = NOP; + s->cur_addr = 0; + s->len = 0; + s->needed_bytes = 0; + s->pos = 0; + s->state = STATE_IDLE; + s->write_enable = false; + + DB_PRINT_L(0, "Reset done.\n"); +} + static void decode_new_cmd(Flash *s, uint32_t value) { s->cmd_in_progress = value; @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss) s->size = s->pi->sector_size * s->pi->n_sectors; s->dirty_page = -1; + s->initialized = false; + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_MTD); @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque) flash_sync_dirty((Flash *)opaque, -1); } +static void m25p80_reset(DeviceState *d) +{ + Flash *s = M25P80(d); + + if (s->reset_pin || !s->initialized) { + reset_memory(s); + s->initialized = true; + } +} + +static Property m25p80_properties[] = { + DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0), + DEFINE_PROP_END_OF_LIST(), +}; + static const VMStateDescription vmstate_m25p80 = { .name = "xilinx_spi", - .version_id = 1, + .version_id = 2, .minimum_version_id = 1, .pre_save = m25p80_pre_save, .fields = (VMStateField[]) { @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = { VMSTATE_UINT8(cmd_in_progress, Flash), VMSTATE_UINT64(cur_addr, Flash), VMSTATE_BOOL(write_enable, Flash), + VMSTATE_BOOL(initialized, Flash), + VMSTATE_UINT8(reset_pin, Flash), VMSTATE_END_OF_LIST() } }; @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void *data) k->set_cs = m25p80_cs; k->cs_polarity = SSI_CS_LOW; dc->vmsd = &vmstate_m25p80; + dc->reset = &m25p80_reset; + dc->props = m25p80_properties; mc->pi = data; }