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

[Peter Crosthwaite]

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.

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
>>>
>>>
>>
>>
>