> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@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.krzemin...@nokia.com> wrote: > > > > > > W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze: > >> On Wed, Dec 16, 2015 at 4:57 AM, <marcin.krzemin...@nokia.com> > wrote: > >>> From: Marcin Krzeminski <marcin.krzemin...@nokia.com> > >>> > >>> Signed-off-by: Marcin Krzeminski <marcin.krzemin...@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 > >>> > >>> > >> > >> > >