Re: [Qemu-devel] [PATCH 02/12] Added reset-pin emulation in model.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:address@hidden
> Sent: Monday, February 01, 2016 7:29 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> Cc: Peter Maydell; address@hidden 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) <address@hidden> wrote:
> >
> >
> > W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze:
> >> On Wed, Dec 16, 2015 at 4:57 AM,  <address@hidden>
> wrote:
> >>> From: Marcin Krzeminski <address@hidden>
> >>>
> >>> Signed-off-by: Marcin Krzeminski <address@hidden>
> >>> ---
> >>>  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
> >>>
> >>>
> >>
> >>
> >