[Qemu-devel] [PATCH 00/12] Support for new flash devices/4bytes commands

[Qemu-devel] [PATCH 00/12] Support for new flash devices/4bytes commands

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Hello,

This patch series adds support for flash devices:
N25Q256A,N25Q512A,MX66U51235 and S25FL512S,
with needed 4bytes commands.
Additionally support for serial eeproms AT25128A/AT25256A
has been added.
Since this patchset has more functionality than first version,
it is not v2.


Marcin Krzeminski (12):
  Removed unused variable.
  Added reset-pin emulation in model.
  Reset enable and reset memory commands support.
  Changed variable type to allow serial eeprom emulation (changing
    0->1).
  Added support for serial eeproms - AT25128A/AT25256A
  4byte address mode support added.
  Added support for extend address mode commands.
  Support for N25Q256A/N25Q512A
  Support for 6Bytes jdec.
  Support for quad commands.
  Support for mx66u51235 and s25fl512s
  Read flag status register command support added.

 hw/block/m25p80.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 251 insertions(+), 31 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 01/12] Removed unused variable.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index efc43dd..bfd493f 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -244,8 +244,6 @@ typedef enum {
 typedef struct Flash {
     SSISlave parent_obj;
 
-    uint32_t r;
-
     BlockBackend *blk;
 
     uint8_t *storage;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 01/12] Removed unused variable.

[Peter Crosthwaite]

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>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/block/m25p80.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index efc43dd..bfd493f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -244,8 +244,6 @@ typedef enum {
>  typedef struct Flash {
>      SSISlave parent_obj;
>
> -    uint32_t r;
> -
>      BlockBackend *blk;
>
>      uint8_t *storage;
> --
> 2.5.0
>
>

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

[marcin.krzeminski]

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;
+
     /* 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;
 }
 
-- 
2.5.0

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

[Peter Crosthwaite]

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

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

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

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

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

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

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

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

[Qemu-devel] [PATCH 03/12] Reset enable and reset memory commands support.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index bcb66a5..5e07b57 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -231,6 +231,10 @@ typedef enum {
     ERASE_4K = 0x20,
     ERASE_32K = 0x52,
     ERASE_SECTOR = 0xd8,
+
+    RESET_ENABLE = 0x66,
+    RESET_MEMORY = 0x99,
+
 } FlashCMD;
 
 typedef enum {
@@ -258,6 +262,7 @@ typedef struct Flash {
     uint8_t cmd_in_progress;
     uint64_t cur_addr;
     bool write_enable;
+    bool reset_enable;
     bool initialized;
     uint8_t reset_pin;
 
@@ -441,6 +446,7 @@ static void reset_memory(Flash *s)
     s->pos = 0;
     s->state = STATE_IDLE;
     s->write_enable = false;
+    s->reset_enable = false;
 
     DB_PRINT_L(0, "Reset done.\n");
 }
@@ -554,6 +560,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
     case NOP:
         break;
+    case RESET_ENABLE:
+        s->reset_enable = true;
+        break;
+    case RESET_MEMORY:
+        if (s->reset_enable) {
+            reset_memory(s);
+        }
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
         break;
@@ -696,6 +710,7 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT8(cmd_in_progress, Flash),
         VMSTATE_UINT64(cur_addr, Flash),
         VMSTATE_BOOL(write_enable, Flash),
+        VMSTATE_BOOL(reset_enable, Flash),
         VMSTATE_BOOL(initialized, Flash),
         VMSTATE_UINT8(reset_pin, Flash),
         VMSTATE_END_OF_LIST()
-- 
2.5.0

Re: [Qemu-devel] [PATCH 03/12] Reset enable and reset memory commands support.

[Peter Crosthwaite]

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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index bcb66a5..5e07b57 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -231,6 +231,10 @@ typedef enum {
>      ERASE_4K = 0x20,
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> +
> +    RESET_ENABLE = 0x66,
> +    RESET_MEMORY = 0x99,
> +

Extra blank not needed.

>  } FlashCMD;
>
>  typedef enum {
> @@ -258,6 +262,7 @@ typedef struct Flash {
>      uint8_t cmd_in_progress;
>      uint64_t cur_addr;
>      bool write_enable;
> +    bool reset_enable;
>      bool initialized;
>      uint8_t reset_pin;
>
> @@ -441,6 +446,7 @@ static void reset_memory(Flash *s)
>      s->pos = 0;
>      s->state = STATE_IDLE;
>      s->write_enable = false;
> +    s->reset_enable = false;
>
>      DB_PRINT_L(0, "Reset done.\n");
>  }
> @@ -554,6 +560,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>      case NOP:
>          break;
> +    case RESET_ENABLE:
> +        s->reset_enable = true;
> +        break;
> +    case RESET_MEMORY:
> +        if (s->reset_enable) {
> +            reset_memory(s);
> +        }
> +        break;

Reading an n25q256 datasheet, the RESET_ENABLE state is cancelled by
commands other that RESET_ENABLE or by bouncing the CS. A way to
implement this would be something like

if (value != RESET_MEMORY) {
    s->reset_enable = false;
}

before the switch of value as well as clearing reset_enable in m25p80_cs.

Regards,
Peter

>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>          break;
> @@ -696,6 +710,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT8(cmd_in_progress, Flash),
>          VMSTATE_UINT64(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
> +        VMSTATE_BOOL(reset_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
>          VMSTATE_END_OF_LIST()
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 03/12] Reset enable and reset memory commands support.

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

W dniu 21.12.2015 o 12:18, 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 | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index bcb66a5..5e07b57 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -231,6 +231,10 @@ typedef enum {
>>      ERASE_4K = 0x20,
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>> +
>> +    RESET_ENABLE = 0x66,
>> +    RESET_MEMORY = 0x99,
>> +
>
> Extra blank not needed.
Ok.
>
>
>>  } FlashCMD;
>>
>>  typedef enum {
>> @@ -258,6 +262,7 @@ typedef struct Flash {
>>      uint8_t cmd_in_progress;
>>      uint64_t cur_addr;
>>      bool write_enable;
>> +    bool reset_enable;
>>      bool initialized;
>>      uint8_t reset_pin;
>>
>> @@ -441,6 +446,7 @@ static void reset_memory(Flash *s)
>>      s->pos = 0;
>>      s->state = STATE_IDLE;
>>      s->write_enable = false;
>> +    s->reset_enable = false;
>>
>>      DB_PRINT_L(0, "Reset done.\n");
>>  }
>> @@ -554,6 +560,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>      case NOP:
>>          break;
>> +    case RESET_ENABLE:
>> +        s->reset_enable = true;
>> +        break;
>> +    case RESET_MEMORY:
>> +        if (s->reset_enable) {
>> +            reset_memory(s);
>> +        }
>> +        break;
>
> Reading an n25q256 datasheet, the RESET_ENABLE state is cancelled by
> commands other that RESET_ENABLE or by bouncing the CS. A way to
> implement this would be something like
>
> if (value != RESET_MEMORY) {
>     s->reset_enable = false;
> }
>
> before the switch of value as well as clearing reset_enable in m25p80_cs.
>
> Regards,
> Peter
Agree.

Thanks,
Marcin
>
>>      default:
>>          qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>>          break;
>> @@ -696,6 +710,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_UINT8(cmd_in_progress, Flash),
>>          VMSTATE_UINT64(cur_addr, Flash),
>>          VMSTATE_BOOL(write_enable, Flash),
>> +        VMSTATE_BOOL(reset_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>>          VMSTATE_END_OF_LIST()
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 04/12] Changed variable type to allow serial eeprom emulation (changing 0->1).

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 5e07b57..fbbfd1d 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -60,7 +60,7 @@ typedef struct FlashPartInfo {
     uint32_t sector_size;
     uint32_t n_sectors;
     uint32_t page_size;
-    uint8_t flags;
+    uint16_t flags;
 } FlashPartInfo;
 
 /* adapted from linux */
-- 
2.5.0

Re: [Qemu-devel] [PATCH 04/12] Changed variable type to allow serial eeprom emulation (changing 0->1).

[Peter Crosthwaite]

Your commit message subject line should be a little shorter and
lengthier explanations of the patch content go here as a paragraph.
You should also have subsystem prefixes to patch subject lines. This
patch would be something like:

block: m25p80: widen flags variable

Extend the width of the flags variable to support the already existing
(but unused) WR_1 flag, which is above the range of 8 bits. This is
turn allows support of EEPROM emulation which requires the WR_1
feature.

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>

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> ---
>  hw/block/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 5e07b57..fbbfd1d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -60,7 +60,7 @@ typedef struct FlashPartInfo {
>      uint32_t sector_size;
>      uint32_t n_sectors;
>      uint32_t page_size;
> -    uint8_t flags;
> +    uint16_t flags;
>  } FlashPartInfo;
>
>  /* adapted from linux */
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 04/12] Changed variable type to allow serial eeprom emulation (changing 0->1).

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

W dniu 21.12.2015 o 12:23, Peter Crosthwaite pisze:
> Your commit message subject line should be a little shorter and
> lengthier explanations of the patch content go here as a paragraph.
> You should also have subsystem prefixes to patch subject lines. This
> patch would be something like:
>
> block: m25p80: widen flags variable
>
> Extend the width of the flags variable to support the already existing
> (but unused) WR_1 flag, which is above the range of 8 bits. This is
> turn allows support of EEPROM emulation which requires the WR_1
> feature.
Thanks,
Marcin
>
> 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>
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Regards,
> Peter
>
>> ---
>>  hw/block/m25p80.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 5e07b57..fbbfd1d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -60,7 +60,7 @@ typedef struct FlashPartInfo {
>>      uint32_t sector_size;
>>      uint32_t n_sectors;
>>      uint32_t page_size;
>> -    uint8_t flags;
>> +    uint16_t flags;
>>  } FlashPartInfo;
>>
>>  /* adapted from linux */
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 05/12] Added support for serial eeproms - AT25128A/AT25256A

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index fbbfd1d..1a547ae 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -94,6 +94,11 @@ static const FlashPartInfo known_devices[] = {
 
     { INFO("at45db081d",  0x1f2500,      0,  64 << 10,  16, ER_4K) },
 
+    /* Atmel EEPROMS - it is assumed, that don't care bit in command */
+    /* is set to 0. Block protection is not supported */
+    { INFO("at25128a-nonjedec", 0x0,     0,         1, 131072, WR_1) },
+    { INFO("at25256a-nonjedec", 0x0,     0,         1, 262144, WR_1) },
+
     /* EON -- en25xxx */
     { INFO("en25f32",     0x1c3116,      0,  64 << 10,  64, ER_4K) },
     { INFO("en25p32",     0x1c2016,      0,  64 << 10,  64, 0) },
-- 
2.5.0

Re: [Qemu-devel] [PATCH 05/12] Added support for serial eeproms - AT25128A/AT25256A

[Peter Crosthwaite]

Subsystem prefix needed in commit subject (block: m25p80). Bring the
mention of specific parts to the paragraph to shorten the subject.

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 | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index fbbfd1d..1a547ae 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -94,6 +94,11 @@ static const FlashPartInfo known_devices[] = {
>
>      { INFO("at45db081d",  0x1f2500,      0,  64 << 10,  16, ER_4K) },
>
> +    /* Atmel EEPROMS - it is assumed, that don't care bit in command */

Multi-line comment should not stop and start again (no */).

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> +    /* is set to 0. Block protection is not supported */
> +    { INFO("at25128a-nonjedec", 0x0,     0,         1, 131072, WR_1) },
> +    { INFO("at25256a-nonjedec", 0x0,     0,         1, 262144, WR_1) },
> +
>      /* EON -- en25xxx */
>      { INFO("en25f32",     0x1c3116,      0,  64 << 10,  64, ER_4K) },
>      { INFO("en25p32",     0x1c2016,      0,  64 << 10,  64, 0) },
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 05/12] Added support for serial eeproms - AT25128A/AT25256A

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

W dniu 21.12.2015 o 12:28, Peter Crosthwaite pisze:
> Subsystem prefix needed in commit subject (block: m25p80). Bring the
> mention of specific parts to the paragraph to shorten the subject.
I accidentally removed word m25p80 from all patches subject.
Sorry.
>
>
> 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 | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index fbbfd1d..1a547ae 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -94,6 +94,11 @@ static const FlashPartInfo known_devices[] = {
>>
>>      { INFO("at45db081d",  0x1f2500,      0,  64 << 10,  16, ER_4K) },
>>
>> +    /* Atmel EEPROMS - it is assumed, that don't care bit in command */
>
> Multi-line comment should not stop and start again (no */).
Ok.
Thanks,
Marcin
>
>
> Otherwise,
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Regards,
> Peter
>
>> +    /* is set to 0. Block protection is not supported */
>> +    { INFO("at25128a-nonjedec", 0x0,     0,         1, 131072, WR_1) },
>> +    { INFO("at25256a-nonjedec", 0x0,     0,         1, 262144, WR_1) },
>> +
>>      /* EON -- en25xxx */
>>      { INFO("en25f32",     0x1c3116,      0,  64 << 10,  64, ER_4K) },
>>      { INFO("en25p32",     0x1c2016,      0,  64 << 10,  64, 0) },
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 06/12] 4byte address mode support added.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 1a547ae..6d5d90d 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -237,6 +237,9 @@ typedef enum {
     ERASE_32K = 0x52,
     ERASE_SECTOR = 0xd8,
 
+    EN_4BYTE_ADDR = 0xB7,
+    EX_4BYTE_ADDR = 0xE9,
+
     RESET_ENABLE = 0x66,
     RESET_MEMORY = 0x99,
 
@@ -267,6 +270,7 @@ typedef struct Flash {
     uint8_t cmd_in_progress;
     uint64_t cur_addr;
     bool write_enable;
+    bool four_bytes_address_mode;
     bool reset_enable;
     bool initialized;
     uint8_t reset_pin;
@@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
     s->dirty_page = page;
 }
 
+static inline int is_4bytes(Flash *s)
+{
+       return s->four_bytes_address_mode;
+   }
+}
+
 static void complete_collecting_data(Flash *s)
 {
-    s->cur_addr = s->data[0] << 16;
-    s->cur_addr |= s->data[1] << 8;
-    s->cur_addr |= s->data[2];
+    if (is_4bytes(s)) {
+        s->cur_addr = s->data[0] << 24;
+        s->cur_addr |= s->data[1] << 16;
+        s->cur_addr |= s->data[2] << 8;
+        s->cur_addr |= s->data[3];
+    } else {
+        s->cur_addr = s->data[0] << 16;
+        s->cur_addr |= s->data[1] << 8;
+        s->cur_addr |= s->data[2];
+    }
 
     s->state = STATE_IDLE;
 
@@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
 {
     s->cmd_in_progress = NOP;
     s->cur_addr = 0;
+    s->four_bytes_address_mode = false;
     s->len = 0;
     s->needed_bytes = 0;
     s->pos = 0;
@@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
     case NOP:
         break;
+    case EN_4BYTE_ADDR:
+        s->four_bytes_address_mode = true;
+        break;
+    case EX_4BYTE_ADDR:
+        s->four_bytes_address_mode = false;
+        break;
     case RESET_ENABLE:
         s->reset_enable = true;
         break;
@@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT8(cmd_in_progress, Flash),
         VMSTATE_UINT64(cur_addr, Flash),
         VMSTATE_BOOL(write_enable, Flash),
+        VMSTATE_BOOL(four_bytes_address_mode, Flash),
         VMSTATE_BOOL(reset_enable, Flash),
         VMSTATE_BOOL(initialized, Flash),
         VMSTATE_UINT8(reset_pin, Flash),
-- 
2.5.0

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

[Peter Crosthwaite]

Commit subject subsystem prefix (block: m25p80:) needed.

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 | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 1a547ae..6d5d90d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -237,6 +237,9 @@ typedef enum {
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
>
> +    EN_4BYTE_ADDR = 0xB7,
> +    EX_4BYTE_ADDR = 0xE9,
> +
>      RESET_ENABLE = 0x66,
>      RESET_MEMORY = 0x99,
>
> @@ -267,6 +270,7 @@ typedef struct Flash {
>      uint8_t cmd_in_progress;
>      uint64_t cur_addr;
>      bool write_enable;
> +    bool four_bytes_address_mode;
>      bool reset_enable;
>      bool initialized;
>      uint8_t reset_pin;
> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>      s->dirty_page = page;
>  }
>
> +static inline int is_4bytes(Flash *s)
> +{
> +       return s->four_bytes_address_mode;
> +   }
> +}
> +
>  static void complete_collecting_data(Flash *s)
>  {
> -    s->cur_addr = s->data[0] << 16;
> -    s->cur_addr |= s->data[1] << 8;
> -    s->cur_addr |= s->data[2];
> +    if (is_4bytes(s)) {
> +        s->cur_addr = s->data[0] << 24;
> +        s->cur_addr |= s->data[1] << 16;
> +        s->cur_addr |= s->data[2] << 8;
> +        s->cur_addr |= s->data[3];
> +    } else {
> +        s->cur_addr = s->data[0] << 16;
> +        s->cur_addr |= s->data[1] << 8;
> +        s->cur_addr |= s->data[2];
> +    }

Avoid the duped code with a loop:

s->cur_addr = 0;
for (i = 0; i < (is_4bytes(s) ? 4 : 3); i++) {
    s->cur_addr <<= 8;
    s->cur_addr |= s->data[i];
}

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>
>      s->state = STATE_IDLE;
>
> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>  {
>      s->cmd_in_progress = NOP;
>      s->cur_addr = 0;
> +    s->four_bytes_address_mode = false;
>      s->len = 0;
>      s->needed_bytes = 0;
>      s->pos = 0;
> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>      case NOP:
>          break;
> +    case EN_4BYTE_ADDR:
> +        s->four_bytes_address_mode = true;
> +        break;
> +    case EX_4BYTE_ADDR:
> +        s->four_bytes_address_mode = false;
> +        break;
>      case RESET_ENABLE:
>          s->reset_enable = true;
>          break;
> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT8(cmd_in_progress, Flash),
>          VMSTATE_UINT64(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
> +        VMSTATE_BOOL(four_bytes_address_mode, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 06/12] 4byte address mode support added.

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

W dniu 21.12.2015 o 12:35, Peter Crosthwaite pisze:
> Commit subject subsystem prefix (block: m25p80:) needed.
>
> 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 | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 1a547ae..6d5d90d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -237,6 +237,9 @@ typedef enum {
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>>
>> +    EN_4BYTE_ADDR = 0xB7,
>> +    EX_4BYTE_ADDR = 0xE9,
>> +
>>      RESET_ENABLE = 0x66,
>>      RESET_MEMORY = 0x99,
>>
>> @@ -267,6 +270,7 @@ typedef struct Flash {
>>      uint8_t cmd_in_progress;
>>      uint64_t cur_addr;
>>      bool write_enable;
>> +    bool four_bytes_address_mode;
>>      bool reset_enable;
>>      bool initialized;
>>      uint8_t reset_pin;
>> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>>      s->dirty_page = page;
>>  }
>>
>> +static inline int is_4bytes(Flash *s)
>> +{
>> +       return s->four_bytes_address_mode;
>> +   }
>> +}
>> +
>>  static void complete_collecting_data(Flash *s)
>>  {
>> -    s->cur_addr = s->data[0] << 16;
>> -    s->cur_addr |= s->data[1] << 8;
>> -    s->cur_addr |= s->data[2];
>> +    if (is_4bytes(s)) {
>> +        s->cur_addr = s->data[0] << 24;
>> +        s->cur_addr |= s->data[1] << 16;
>> +        s->cur_addr |= s->data[2] << 8;
>> +        s->cur_addr |= s->data[3];
>> +    } else {
>> +        s->cur_addr = s->data[0] << 16;
>> +        s->cur_addr |= s->data[1] << 8;
>> +        s->cur_addr |= s->data[2];
>> +    }
>
> Avoid the duped code with a loop:
>
> s->cur_addr = 0;
> for (i = 0; i < (is_4bytes(s) ? 4 : 3); i++) {
>     s->cur_addr <<= 8;
>     s->cur_addr |= s->data[i];
> }
Ok.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Regards,
> Peter
>
>>
>>      s->state = STATE_IDLE;
>>
>> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>>  {
>>      s->cmd_in_progress = NOP;
>>      s->cur_addr = 0;
>> +    s->four_bytes_address_mode = false;
>>      s->len = 0;
>>      s->needed_bytes = 0;
>>      s->pos = 0;
>> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>      case NOP:
>>          break;
>> +    case EN_4BYTE_ADDR:
>> +        s->four_bytes_address_mode = true;
>> +        break;
>> +    case EX_4BYTE_ADDR:
>> +        s->four_bytes_address_mode = false;
>> +        break;
>>      case RESET_ENABLE:
>>          s->reset_enable = true;
>>          break;
>> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_UINT8(cmd_in_progress, Flash),
>>          VMSTATE_UINT64(cur_addr, Flash),
>>          VMSTATE_BOOL(write_enable, Flash),
>> +        VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>          VMSTATE_BOOL(reset_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

[Cédric Le Goater]

Hello Marcin,


On 12/16/2015 01:57 PM, 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 | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 1a547ae..6d5d90d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -237,6 +237,9 @@ typedef enum {
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> 
> +    EN_4BYTE_ADDR = 0xB7,
> +    EX_4BYTE_ADDR = 0xE9,
> +
>      RESET_ENABLE = 0x66,
>      RESET_MEMORY = 0x99,
> 
> @@ -267,6 +270,7 @@ typedef struct Flash {
>      uint8_t cmd_in_progress;
>      uint64_t cur_addr;
>      bool write_enable;
> +    bool four_bytes_address_mode;
>      bool reset_enable;
>      bool initialized;
>      uint8_t reset_pin;
> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>      s->dirty_page = page;
>  }
> 
> +static inline int is_4bytes(Flash *s)
> +{
> +       return s->four_bytes_address_mode;
> +   }
> +}
> +
>  static void complete_collecting_data(Flash *s)
>  {
> -    s->cur_addr = s->data[0] << 16;
> -    s->cur_addr |= s->data[1] << 8;
> -    s->cur_addr |= s->data[2];
> +    if (is_4bytes(s)) {
> +        s->cur_addr = s->data[0] << 24;
> +        s->cur_addr |= s->data[1] << 16;
> +        s->cur_addr |= s->data[2] << 8;
> +        s->cur_addr |= s->data[3];
> +    } else {
> +        s->cur_addr = s->data[0] << 16;
> +        s->cur_addr |= s->data[1] << 8;
> +        s->cur_addr |= s->data[2];
> +    }
> 
>      s->state = STATE_IDLE;


Don't we need to also change 'needed_bytes' in the decode_new_cmd() routine
to increase the number of bytes expected by some commands ? 

If so, we could add a width attribute to 'struct Flash' and to something like :

	@@ -260,6 +263,7 @@ typedef struct Flash {
	     uint8_t cmd_in_progress;
	     uint64_t cur_addr;
	     bool write_enable;
	+    uint8_t width;
 
	     int64_t dirty_page;
 
	@@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
	     s->cur_addr |= s->data[1] << 8;
	     s->cur_addr |= s->data[2];
	 
	+    if (s->width == 4) {
	+        s->cur_addr = s->cur_addr << 8 | s->data[4];
	+    }
	+
	     s->state = STATE_IDLE;
 
	     switch (s->cmd_in_progress) {
	@@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
	     case DPP:
	     case QPP:
	     case PP:
	-        s->needed_bytes = 3;
	+        s->needed_bytes = s->width;
	         s->pos = 0;
	         s->len = 0;
	         s->state = STATE_COLLECTING_DATA;
	@@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss)
	         memset(s->storage, 0xFF, s->size);
	     }
 
	+    s->width = 3;
	     return 0;
	 }
 


QOR, DIOR, QIOR command also need a check. I suppose an address and some 
number of dummy bytes are expected before the fast read command is done. 
I would need to check the datasheets.

Cheers,

C.

> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>  {
>      s->cmd_in_progress = NOP;
>      s->cur_addr = 0;
> +    s->four_bytes_address_mode = false;
>      s->len = 0;
>      s->needed_bytes = 0;
>      s->pos = 0;
> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>      case NOP:
>          break;
> +    case EN_4BYTE_ADDR:
> +        s->four_bytes_address_mode = true;
> +        break;
> +    case EX_4BYTE_ADDR:
> +        s->four_bytes_address_mode = false;
> +        break;
>      case RESET_ENABLE:
>          s->reset_enable = true;
>          break;
> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT8(cmd_in_progress, Flash),
>          VMSTATE_UINT64(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
> +        VMSTATE_BOOL(four_bytes_address_mode, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
> 

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

[Peter Crosthwaite]

On Tue, Dec 22, 2015 at 10:41 AM, Cédric Le Goater <clg@fr.ibm.com> wrote:
> Hello Marcin,
>
>
> On 12/16/2015 01:57 PM, 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 | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 1a547ae..6d5d90d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -237,6 +237,9 @@ typedef enum {
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>>
>> +    EN_4BYTE_ADDR = 0xB7,
>> +    EX_4BYTE_ADDR = 0xE9,
>> +
>>      RESET_ENABLE = 0x66,
>>      RESET_MEMORY = 0x99,
>>
>> @@ -267,6 +270,7 @@ typedef struct Flash {
>>      uint8_t cmd_in_progress;
>>      uint64_t cur_addr;
>>      bool write_enable;
>> +    bool four_bytes_address_mode;
>>      bool reset_enable;
>>      bool initialized;
>>      uint8_t reset_pin;
>> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>>      s->dirty_page = page;
>>  }
>>
>> +static inline int is_4bytes(Flash *s)
>> +{
>> +       return s->four_bytes_address_mode;
>> +   }
>> +}
>> +
>>  static void complete_collecting_data(Flash *s)
>>  {
>> -    s->cur_addr = s->data[0] << 16;
>> -    s->cur_addr |= s->data[1] << 8;
>> -    s->cur_addr |= s->data[2];
>> +    if (is_4bytes(s)) {
>> +        s->cur_addr = s->data[0] << 24;
>> +        s->cur_addr |= s->data[1] << 16;
>> +        s->cur_addr |= s->data[2] << 8;
>> +        s->cur_addr |= s->data[3];
>> +    } else {
>> +        s->cur_addr = s->data[0] << 16;
>> +        s->cur_addr |= s->data[1] << 8;
>> +        s->cur_addr |= s->data[2];
>> +    }
>>
>>      s->state = STATE_IDLE;
>
>
> Don't we need to also change 'needed_bytes' in the decode_new_cmd() routine
> to increase the number of bytes expected by some commands ?
>

I think you are right, and it may be solved later in the series, from patch 10:

     case QPP:
     case PP:
-        s->needed_bytes = 3;
+       s->needed_bytes = is_4bytes(s) ? 4 : 3;
         s->pos = 0;
         s->len = 0;
         s->state = STATE_COLLECTING_DATA;

This hunk should be brought forward to this patch.

> If so, we could add a width attribute to 'struct Flash' and to something like :
>
>         @@ -260,6 +263,7 @@ typedef struct Flash {
>              uint8_t cmd_in_progress;
>              uint64_t cur_addr;
>              bool write_enable;
>         +    uint8_t width;
>
>              int64_t dirty_page;
>
>         @@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
>              s->cur_addr |= s->data[1] << 8;
>              s->cur_addr |= s->data[2];
>
>         +    if (s->width == 4) {
>         +        s->cur_addr = s->cur_addr << 8 | s->data[4];
>         +    }
>         +
>              s->state = STATE_IDLE;
>
>              switch (s->cmd_in_progress) {
>         @@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
>              case DPP:
>              case QPP:
>              case PP:
>         -        s->needed_bytes = 3;
>         +        s->needed_bytes = s->width;
>                  s->pos = 0;
>                  s->len = 0;
>                  s->state = STATE_COLLECTING_DATA;
>         @@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss)
>                  memset(s->storage, 0xFF, s->size);
>              }
>
>         +    s->width = 3;
>              return 0;
>          }
>
>
>
> QOR, DIOR, QIOR command also need a check. I suppose an address and some
> number of dummy bytes are expected before the fast read command is done.
> I would need to check the datasheets.
>

I just checked an n25q256 datasheet, and yes you are right. The same
logic as in the hunk above needs to apply to these commands. CC
Xilinx, this bug is in their tree as well I think.

https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c

Where PP, READ and friends have the 4 byte correction logic based on
addr_4b but QIOR does not.

Nice catch :)

Regards,
Peter

> Cheers,
>
> C.
>
>> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>>  {
>>      s->cmd_in_progress = NOP;
>>      s->cur_addr = 0;
>> +    s->four_bytes_address_mode = false;
>>      s->len = 0;
>>      s->needed_bytes = 0;
>>      s->pos = 0;
>> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>      case NOP:
>>          break;
>> +    case EN_4BYTE_ADDR:
>> +        s->four_bytes_address_mode = true;
>> +        break;
>> +    case EX_4BYTE_ADDR:
>> +        s->four_bytes_address_mode = false;
>> +        break;
>>      case RESET_ENABLE:
>>          s->reset_enable = true;
>>          break;
>> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_UINT8(cmd_in_progress, Flash),
>>          VMSTATE_UINT64(cur_addr, Flash),
>>          VMSTATE_BOOL(write_enable, Flash),
>> +        VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>          VMSTATE_BOOL(reset_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>>
>
>
>

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

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

> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Tuesday, December 22, 2015 10:29 PM
> To: Cédric Le Goater; git@xilinx.com
> Cc: Krzeminski, Marcin (Nokia - PL/Wroclaw); qemu-devel@nongnu.org
> Developers; Lenkow, Pawel (Nokia - PL/Wroclaw)
> Subject: Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support
> added.
> 
> On Tue, Dec 22, 2015 at 10:41 AM, Cédric Le Goater <clg@fr.ibm.com> wrote:
> > Hello Marcin,
> >
> >
> > On 12/16/2015 01:57 PM, 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 | 31 ++++++++++++++++++++++++++++---
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> >> 1a547ae..6d5d90d 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -237,6 +237,9 @@ typedef enum {
> >>      ERASE_32K = 0x52,
> >>      ERASE_SECTOR = 0xd8,
> >>
> >> +    EN_4BYTE_ADDR = 0xB7,
> >> +    EX_4BYTE_ADDR = 0xE9,
> >> +
> >>      RESET_ENABLE = 0x66,
> >>      RESET_MEMORY = 0x99,
> >>
> >> @@ -267,6 +270,7 @@ typedef struct Flash {
> >>      uint8_t cmd_in_progress;
> >>      uint64_t cur_addr;
> >>      bool write_enable;
> >> +    bool four_bytes_address_mode;
> >>      bool reset_enable;
> >>      bool initialized;
> >>      uint8_t reset_pin;
> >> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr,
> uint8_t data)
> >>      s->dirty_page = page;
> >>  }
> >>
> >> +static inline int is_4bytes(Flash *s) {
> >> +       return s->four_bytes_address_mode;
> >> +   }
> >> +}
> >> +
> >>  static void complete_collecting_data(Flash *s)  {
> >> -    s->cur_addr = s->data[0] << 16;
> >> -    s->cur_addr |= s->data[1] << 8;
> >> -    s->cur_addr |= s->data[2];
> >> +    if (is_4bytes(s)) {
> >> +        s->cur_addr = s->data[0] << 24;
> >> +        s->cur_addr |= s->data[1] << 16;
> >> +        s->cur_addr |= s->data[2] << 8;
> >> +        s->cur_addr |= s->data[3];
> >> +    } else {
> >> +        s->cur_addr = s->data[0] << 16;
> >> +        s->cur_addr |= s->data[1] << 8;
> >> +        s->cur_addr |= s->data[2];
> >> +    }
> >>
> >>      s->state = STATE_IDLE;
> >
> >
> > Don't we need to also change 'needed_bytes' in the decode_new_cmd()
> > routine to increase the number of bytes expected by some commands ?
> >
> 
> I think you are right, and it may be solved later in the series, from patch 10:
> 
>      case QPP:
>      case PP:
> -        s->needed_bytes = 3;
> +       s->needed_bytes = is_4bytes(s) ? 4 : 3;
>          s->pos = 0;
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
> 
> This hunk should be brought forward to this patch.
> 
> > If so, we could add a width attribute to 'struct Flash' and to something like :
> >
> >         @@ -260,6 +263,7 @@ typedef struct Flash {
> >              uint8_t cmd_in_progress;
> >              uint64_t cur_addr;
> >              bool write_enable;
> >         +    uint8_t width;
> >
> >              int64_t dirty_page;
> >
> >         @@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
> >              s->cur_addr |= s->data[1] << 8;
> >              s->cur_addr |= s->data[2];
> >
> >         +    if (s->width == 4) {
> >         +        s->cur_addr = s->cur_addr << 8 | s->data[4];
> >         +    }
> >         +
> >              s->state = STATE_IDLE;
> >
> >              switch (s->cmd_in_progress) {
> >         @@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
> >              case DPP:
> >              case QPP:
> >              case PP:
> >         -        s->needed_bytes = 3;
> >         +        s->needed_bytes = s->width;
> >                  s->pos = 0;
> >                  s->len = 0;
> >                  s->state = STATE_COLLECTING_DATA;
> >         @@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss)
> >                  memset(s->storage, 0xFF, s->size);
> >              }
> >
> >         +    s->width = 3;
> >              return 0;
> >          }
> >
> >
> >
> > QOR, DIOR, QIOR command also need a check. I suppose an address and
> > some number of dummy bytes are expected before the fast read
> command is done.
> > I would need to check the datasheets.
> >
> 
> I just checked an n25q256 datasheet, and yes you are right. The same logic as
> in the hunk above needs to apply to these commands. CC Xilinx, this bug is in
> their tree as well I think.
> 
> https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c
> 
> Where PP, READ and friends have the 4 byte correction logic based on
> addr_4b but QIOR does not.
> 
> Nice catch :)
> 
> Regards,
> Peter
> 

Hello Cedric,

Sorry for late response.
As peter has responded, needed bytes for 4bytes address mode/cmd length is handled partially (not for all commands).
Dummy cycles are not handled since my QSPI controller model had a bug so I missed this feature.
Thanks for finding - it will be covered in v2.

Regards,
Marcin
> > Cheers,
> >
> > C.
> >
> >> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)  {
> >>      s->cmd_in_progress = NOP;
> >>      s->cur_addr = 0;
> >> +    s->four_bytes_address_mode = false;
> >>      s->len = 0;
> >>      s->needed_bytes = 0;
> >>      s->pos = 0;
> >> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t
> value)
> >>          break;
> >>      case NOP:
> >>          break;
> >> +    case EN_4BYTE_ADDR:
> >> +        s->four_bytes_address_mode = true;
> >> +        break;
> >> +    case EX_4BYTE_ADDR:
> >> +        s->four_bytes_address_mode = false;
> >> +        break;
> >>      case RESET_ENABLE:
> >>          s->reset_enable = true;
> >>          break;
> >> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80
> = {
> >>          VMSTATE_UINT8(cmd_in_progress, Flash),
> >>          VMSTATE_UINT64(cur_addr, Flash),
> >>          VMSTATE_BOOL(write_enable, Flash),
> >> +        VMSTATE_BOOL(four_bytes_address_mode, Flash),
> >>          VMSTATE_BOOL(reset_enable, Flash),
> >>          VMSTATE_BOOL(initialized, Flash),
> >>          VMSTATE_UINT8(reset_pin, Flash),
> >>
> >
> >
> >

[Qemu-devel] [PATCH 07/12] Added support for extend address mode commands.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 6d5d90d..f0f637e 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -47,6 +47,9 @@
  */
 #define WR_1 0x100
 
+/* 16 MiB max in 3 byte address mode */
+#define MAX_3BYTES_SIZE 0x1000000
+
 typedef struct FlashPartInfo {
     const char *part_name;
     /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
@@ -240,6 +243,9 @@ typedef enum {
     EN_4BYTE_ADDR = 0xB7,
     EX_4BYTE_ADDR = 0xE9,
 
+    EXTEND_ADDR_READ = 0xC8,
+    EXTEND_ADDR_WRITE = 0xC5,
+
     RESET_ENABLE = 0x66,
     RESET_MEMORY = 0x99,
 
@@ -274,6 +280,7 @@ typedef struct Flash {
     bool reset_enable;
     bool initialized;
     uint8_t reset_pin;
+    uint8_t ear;
 
     int64_t dirty_page;
 
@@ -426,6 +433,8 @@ static void complete_collecting_data(Flash *s)
         s->cur_addr = s->data[0] << 16;
         s->cur_addr |= s->data[1] << 8;
         s->cur_addr |= s->data[2];
+
+        s->cur_addr += (s->ear&0x3)*MAX_3BYTES_SIZE;
     }
 
     s->state = STATE_IDLE;
@@ -454,6 +463,9 @@ static void complete_collecting_data(Flash *s)
             s->write_enable = false;
         }
         break;
+    case EXTEND_ADDR_WRITE:
+        s->ear = s->data[0];
+        break;
     default:
         break;
     }
@@ -463,6 +475,7 @@ static void reset_memory(Flash *s)
 {
     s->cmd_in_progress = NOP;
     s->cur_addr = 0;
+    s->ear = 0;
     s->four_bytes_address_mode = false;
     s->len = 0;
     s->needed_bytes = 0;
@@ -589,6 +602,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case EX_4BYTE_ADDR:
         s->four_bytes_address_mode = false;
         break;
+    case EXTEND_ADDR_READ:
+        s->data[0] = s->ear;
+        s->pos = 0;
+        s->len = 1;
+        s->state = STATE_READING_DATA;
+        break;
+    case EXTEND_ADDR_WRITE:
+        if (s->write_enable) {
+            s->needed_bytes = 1;
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+        }
+        break;
     case RESET_ENABLE:
         s->reset_enable = true;
         break;
@@ -740,6 +767,7 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT64(cur_addr, Flash),
         VMSTATE_BOOL(write_enable, Flash),
         VMSTATE_BOOL(four_bytes_address_mode, Flash),
+        VMSTATE_UINT8(ear, Flash),
         VMSTATE_BOOL(reset_enable, Flash),
         VMSTATE_BOOL(initialized, Flash),
         VMSTATE_UINT8(reset_pin, Flash),
-- 
2.5.0

Re: [Qemu-devel] [PATCH 07/12] Added support for extend address mode commands.

[Peter Crosthwaite]

Subsystem prefix.

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 | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 6d5d90d..f0f637e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -47,6 +47,9 @@
>   */
>  #define WR_1 0x100
>
> +/* 16 MiB max in 3 byte address mode */
> +#define MAX_3BYTES_SIZE 0x1000000
> +
>  typedef struct FlashPartInfo {
>      const char *part_name;
>      /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
> @@ -240,6 +243,9 @@ typedef enum {
>      EN_4BYTE_ADDR = 0xB7,
>      EX_4BYTE_ADDR = 0xE9,
>
> +    EXTEND_ADDR_READ = 0xC8,
> +    EXTEND_ADDR_WRITE = 0xC5,
> +
>      RESET_ENABLE = 0x66,
>      RESET_MEMORY = 0x99,
>
> @@ -274,6 +280,7 @@ typedef struct Flash {
>      bool reset_enable;
>      bool initialized;
>      uint8_t reset_pin;
> +    uint8_t ear;
>
>      int64_t dirty_page;
>
> @@ -426,6 +433,8 @@ static void complete_collecting_data(Flash *s)
>          s->cur_addr = s->data[0] << 16;
>          s->cur_addr |= s->data[1] << 8;
>          s->cur_addr |= s->data[2];
> +
> +        s->cur_addr += (s->ear&0x3)*MAX_3BYTES_SIZE;

The reset of the file puts spaces around all arithmetic operators so
the same should be for the & and * here.

Following on for the loop suggestion in prev. patch this would then be
in its own if for !is_4byte after the loop.

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>      }
>
>      s->state = STATE_IDLE;
> @@ -454,6 +463,9 @@ static void complete_collecting_data(Flash *s)
>              s->write_enable = false;
>          }
>          break;
> +    case EXTEND_ADDR_WRITE:
> +        s->ear = s->data[0];
> +        break;
>      default:
>          break;
>      }
> @@ -463,6 +475,7 @@ static void reset_memory(Flash *s)
>  {
>      s->cmd_in_progress = NOP;
>      s->cur_addr = 0;
> +    s->ear = 0;
>      s->four_bytes_address_mode = false;
>      s->len = 0;
>      s->needed_bytes = 0;
> @@ -589,6 +602,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case EX_4BYTE_ADDR:
>          s->four_bytes_address_mode = false;
>          break;
> +    case EXTEND_ADDR_READ:
> +        s->data[0] = s->ear;
> +        s->pos = 0;
> +        s->len = 1;
> +        s->state = STATE_READING_DATA;
> +        break;
> +    case EXTEND_ADDR_WRITE:
> +        if (s->write_enable) {
> +            s->needed_bytes = 1;
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        }
> +        break;
>      case RESET_ENABLE:
>          s->reset_enable = true;
>          break;
> @@ -740,6 +767,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT64(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
> +        VMSTATE_UINT8(ear, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 07/12] Added support for extend address mode commands.

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

W dniu 21.12.2015 o 12:41, Peter Crosthwaite pisze:
> Subsystem prefix.
>
> 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 | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 6d5d90d..f0f637e 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -47,6 +47,9 @@
>>   */
>>  #define WR_1 0x100
>>
>> +/* 16 MiB max in 3 byte address mode */
>> +#define MAX_3BYTES_SIZE 0x1000000
>> +
>>  typedef struct FlashPartInfo {
>>      const char *part_name;
>>      /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
>> @@ -240,6 +243,9 @@ typedef enum {
>>      EN_4BYTE_ADDR = 0xB7,
>>      EX_4BYTE_ADDR = 0xE9,
>>
>> +    EXTEND_ADDR_READ = 0xC8,
>> +    EXTEND_ADDR_WRITE = 0xC5,
>> +
>>      RESET_ENABLE = 0x66,
>>      RESET_MEMORY = 0x99,
>>
>> @@ -274,6 +280,7 @@ typedef struct Flash {
>>      bool reset_enable;
>>      bool initialized;
>>      uint8_t reset_pin;
>> +    uint8_t ear;
>>
>>      int64_t dirty_page;
>>
>> @@ -426,6 +433,8 @@ static void complete_collecting_data(Flash *s)
>>          s->cur_addr = s->data[0] << 16;
>>          s->cur_addr |= s->data[1] << 8;
>>          s->cur_addr |= s->data[2];
>> +
>> +        s->cur_addr += (s->ear&0x3)*MAX_3BYTES_SIZE;
>
> The reset of the file puts spaces around all arithmetic operators so
> the same should be for the & and * here.
>
> Following on for the loop suggestion in prev. patch this would then be
> in its own if for !is_4byte after the loop.
Ok.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Regards,
> Peter
>
>>      }
>>
>>      s->state = STATE_IDLE;
>> @@ -454,6 +463,9 @@ static void complete_collecting_data(Flash *s)
>>              s->write_enable = false;
>>          }
>>          break;
>> +    case EXTEND_ADDR_WRITE:
>> +        s->ear = s->data[0];
>> +        break;
>>      default:
>>          break;
>>      }
>> @@ -463,6 +475,7 @@ static void reset_memory(Flash *s)
>>  {
>>      s->cmd_in_progress = NOP;
>>      s->cur_addr = 0;
>> +    s->ear = 0;
>>      s->four_bytes_address_mode = false;
>>      s->len = 0;
>>      s->needed_bytes = 0;
>> @@ -589,6 +602,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>      case EX_4BYTE_ADDR:
>>          s->four_bytes_address_mode = false;
>>          break;
>> +    case EXTEND_ADDR_READ:
>> +        s->data[0] = s->ear;
>> +        s->pos = 0;
>> +        s->len = 1;
>> +        s->state = STATE_READING_DATA;
>> +        break;
>> +    case EXTEND_ADDR_WRITE:
>> +        if (s->write_enable) {
>> +            s->needed_bytes = 1;
>> +            s->pos = 0;
>> +            s->len = 0;
>> +            s->state = STATE_COLLECTING_DATA;
>> +        }
>> +        break;
>>      case RESET_ENABLE:
>>          s->reset_enable = true;
>>          break;
>> @@ -740,6 +767,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_UINT64(cur_addr, Flash),
>>          VMSTATE_BOOL(write_enable, Flash),
>>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>> +        VMSTATE_UINT8(ear, Flash),
>>          VMSTATE_BOOL(reset_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 08/12] Support for N25Q256A/N25Q512A

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index f0f637e..a41c2f1 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -214,6 +214,8 @@ static const FlashPartInfo known_devices[] = {
 
     /* Numonyx -- n25q128 */
     { INFO("n25q128",      0x20ba18,      0,  64 << 10, 256, 0) },
+    { INFO("n25q256a",     0x20ba19,      0,  64 << 10, 512, ER_4K) },
+    { INFO("n25q512a",     0x20ba20,      0,  64 << 10, 1024, ER_4K) },
 };
 
 typedef enum {
-- 
2.5.0

Re: [Qemu-devel] [PATCH 08/12] Support for N25Q256A/N25Q512A

[Peter Crosthwaite]

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 | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index f0f637e..a41c2f1 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -214,6 +214,8 @@ static const FlashPartInfo known_devices[] = {
>
>      /* Numonyx -- n25q128 */

Comment needs update to qXXX or just drop the part name altogether for:

/* Numonyx */

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>      { INFO("n25q128",      0x20ba18,      0,  64 << 10, 256, 0) },
> +    { INFO("n25q256a",     0x20ba19,      0,  64 << 10, 512, ER_4K) },
> +    { INFO("n25q512a",     0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>  };
>
>  typedef enum {
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 08/12] Support for N25Q256A/N25Q512A

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

W dniu 21.12.2015 o 11:29, 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 | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index f0f637e..a41c2f1 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -214,6 +214,8 @@ static const FlashPartInfo known_devices[] = {
>>
>>      /* Numonyx -- n25q128 */
>
> Comment needs update to qXXX or just drop the part name altogether for:
>
> /* Numonyx */
Will be dropped.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Regards,
> Peter
>
>>      { INFO("n25q128",      0x20ba18,      0,  64 << 10, 256, 0) },
>> +    { INFO("n25q256a",     0x20ba19,      0,  64 << 10, 512, ER_4K) },
>> +    { INFO("n25q512a",     0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>>  };
>>
>>  typedef enum {
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 09/12] Support for 6Bytes jdec.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
---
 hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index a41c2f1..6fc55a3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -50,12 +50,17 @@
 /* 16 MiB max in 3 byte address mode */
 #define MAX_3BYTES_SIZE 0x1000000
 
+#define SPI_NOR_MAX_ID_LEN    6
+
 typedef struct FlashPartInfo {
     const char *part_name;
-    /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
-    uint32_t jedec;
-    /* extended jedec code */
-    uint16_t ext_jedec;
+    /*
+     * This array stores the ID bytes.
+     * The first three bytes are the JEDIC ID.
+     * JEDEC ID zero means "no ID" (mostly older chips).
+     */
+    uint8_t id[SPI_NOR_MAX_ID_LEN];
+    uint8_t id_len;
     /* there is confusion between manufacturers as to what a sector is. In this
      * device model, a "sector" is the size that is erased by the ERASE_SECTOR
      * command (opcode 0xd8).
@@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
 } FlashPartInfo;
 
 /* adapted from linux */
-
-#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, _flags)\
-    .part_name = (_part_name),\
-    .jedec = (_jedec),\
-    .ext_jedec = (_ext_jedec),\
+/* Used when the "_ext_id" is two bytes at most */
+#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
+    .part_name = _part_name,\
+    .id = {\
+        ((_jedec_id) >> 16) & 0xff,\
+        ((_jedec_id) >> 8) & 0xff,\
+        (_jedec_id) & 0xff,\
+        ((_ext_id) >> 8) & 0xff,\
+        (_ext_id) & 0xff,\
+          },\
+    .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
+    .sector_size = (_sector_size),\
+    .n_sectors = (_n_sectors),\
+    .page_size = 256,\
+    .flags = (_flags),
+
+#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
+    .part_name = _part_name,\
+    .id = {\
+        ((_jedec_id) >> 16) & 0xff,\
+        ((_jedec_id) >> 8) & 0xff,\
+        (_jedec_id) & 0xff,\
+        ((_ext_id) >> 16) & 0xff,\
+        ((_ext_id) >> 8) & 0xff,\
+        (_ext_id) & 0xff,\
+          },\
+    .id_len = 6,\
     .sector_size = (_sector_size),\
     .n_sectors = (_n_sectors),\
     .page_size = 256,\
@@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
 
     case DIOR:
-        switch ((s->pi->jedec >> 16) & 0xFF) {
+        switch (s->pi->id[0]) {
         case JEDEC_WINBOND:
         case JEDEC_SPANSION:
             s->needed_bytes = 4;
@@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
 
     case QIOR:
-        switch ((s->pi->jedec >> 16) & 0xFF) {
+        switch (s->pi->id[0]) {
         case JEDEC_WINBOND:
         case JEDEC_SPANSION:
             s->needed_bytes = 6;
@@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 
     case JEDEC_READ:
         DB_PRINT_L(0, "populated jedec code\n");
-        s->data[0] = (s->pi->jedec >> 16) & 0xff;
-        s->data[1] = (s->pi->jedec >> 8) & 0xff;
-        s->data[2] = s->pi->jedec & 0xff;
-        if (s->pi->ext_jedec) {
-            s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
-            s->data[4] = s->pi->ext_jedec & 0xff;
-            s->len = 5;
-        } else {
-            s->len = 3;
+        for (i = 0; i < s->pi->id_len; i++) {
+            s->data[i] = s->pi->id[i];
         }
+        s->len = s->pi->id_len;
         s->pos = 0;
         s->state = STATE_READING_DATA;
         break;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 09/12] Support for 6Bytes jdec.

[Peter Crosthwaite]

You need to add more notes in commit messages about what you have
changed. Specifically here, you need to say something about the
arrayification of the Jedec ID. You are also correcting terminology by
changing ext_jedec to ext_id which shuld be mentioned.

On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>

Who is the Author? The patch authorship is Marcin, but the signature
is Pawel. The original author should be the author (the From:) as well
as have the Signed-off-by. Extra people (who resend, queue or commit
the patch) add their SOB. So if this is Pawel's code you need to do a
git commit --amend --author with Pawel's name. But either way, as
sender your own (Marcin) SoB should be here.

> ---
>  hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index a41c2f1..6fc55a3 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -50,12 +50,17 @@
>  /* 16 MiB max in 3 byte address mode */
>  #define MAX_3BYTES_SIZE 0x1000000
>
> +#define SPI_NOR_MAX_ID_LEN    6
> +
>  typedef struct FlashPartInfo {
>      const char *part_name;
> -    /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
> -    uint32_t jedec;
> -    /* extended jedec code */
> -    uint16_t ext_jedec;
> +    /*
> +     * This array stores the ID bytes.

Comment should be of form

/* some text wrapping
 * to multiple lines
 */

for consistency with surrounding code.

> +     * The first three bytes are the JEDIC ID.

"JEDEC"

> +     * JEDEC ID zero means "no ID" (mostly older chips).
> +     */
> +    uint8_t id[SPI_NOR_MAX_ID_LEN];
> +    uint8_t id_len;

This arrayification is a very nice change.

>      /* there is confusion between manufacturers as to what a sector is. In this
>       * device model, a "sector" is the size that is erased by the ERASE_SECTOR
>       * command (opcode 0xd8).
> @@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
>  } FlashPartInfo;
>
>  /* adapted from linux */
> -

Keep this blank.

> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, _flags)\
> -    .part_name = (_part_name),\
> -    .jedec = (_jedec),\
> -    .ext_jedec = (_ext_jedec),\
> +/* Used when the "_ext_id" is two bytes at most */
> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
> +    .part_name = _part_name,\
> +    .id = {\
> +        ((_jedec_id) >> 16) & 0xff,\
> +        ((_jedec_id) >> 8) & 0xff,\
> +        (_jedec_id) & 0xff,\
> +        ((_ext_id) >> 8) & 0xff,\
> +        (_ext_id) & 0xff,\
> +          },\
> +    .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
> +    .sector_size = (_sector_size),\
> +    .n_sectors = (_n_sectors),\
> +    .page_size = 256,\
> +    .flags = (_flags),
> +
> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\

I am not sure we need the extra macro. Can you just make the 6th byte
an extra _flags flag and use the original INFO()? If you do need the
extra macro, one macro should call the other, or both should call a a
common macro (accepting _id_len as a macro arg) to avoid the dup of
the majority of the macro body.

Regards,
Peter

> +    .part_name = _part_name,\
> +    .id = {\
> +        ((_jedec_id) >> 16) & 0xff,\
> +        ((_jedec_id) >> 8) & 0xff,\
> +        (_jedec_id) & 0xff,\
> +        ((_ext_id) >> 16) & 0xff,\
> +        ((_ext_id) >> 8) & 0xff,\
> +        (_ext_id) & 0xff,\
> +          },\
> +    .id_len = 6,\
>      .sector_size = (_sector_size),\
>      .n_sectors = (_n_sectors),\
>      .page_size = 256,\
> @@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>
>      case DIOR:
> -        switch ((s->pi->jedec >> 16) & 0xFF) {
> +        switch (s->pi->id[0]) {
>          case JEDEC_WINBOND:
>          case JEDEC_SPANSION:
>              s->needed_bytes = 4;
> @@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>
>      case QIOR:
> -        switch ((s->pi->jedec >> 16) & 0xFF) {
> +        switch (s->pi->id[0]) {
>          case JEDEC_WINBOND:
>          case JEDEC_SPANSION:
>              s->needed_bytes = 6;
> @@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>
>      case JEDEC_READ:
>          DB_PRINT_L(0, "populated jedec code\n");
> -        s->data[0] = (s->pi->jedec >> 16) & 0xff;
> -        s->data[1] = (s->pi->jedec >> 8) & 0xff;
> -        s->data[2] = s->pi->jedec & 0xff;
> -        if (s->pi->ext_jedec) {
> -            s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
> -            s->data[4] = s->pi->ext_jedec & 0xff;
> -            s->len = 5;
> -        } else {
> -            s->len = 3;
> +        for (i = 0; i < s->pi->id_len; i++) {
> +            s->data[i] = s->pi->id[i];
>          }
> +        s->len = s->pi->id_len;
>          s->pos = 0;
>          s->state = STATE_READING_DATA;
>          break;
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 09/12] Support for 6Bytes jdec.

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

W dniu 21.12.2015 o 11:47, Peter Crosthwaite pisze:
> You need to add more notes in commit messages about what you have
> changed. Specifically here, you need to say something about the
> arrayification of the Jedec ID. You are also correcting terminology by
> changing ext_jedec to ext_id which shuld be mentioned.
>
> On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>
>> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
>
> Who is the Author? The patch authorship is Marcin, but the signature
> is Pawel. The original author should be the author (the From:) as well
> as have the Signed-off-by. Extra people (who resend, queue or commit
> the patch) add their SOB. So if this is Pawel's code you need to do a
> git commit --amend --author with Pawel's name. But either way, as
> sender your own (Marcin) SoB should be here.
My understanding was that SOB is enough to identify an author.
In all this patches whee Pawel is SOB, he is also the author.
I will correct it.
Thanks,
Marcin
>
>
>> ---
>>  hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index a41c2f1..6fc55a3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -50,12 +50,17 @@
>>  /* 16 MiB max in 3 byte address mode */
>>  #define MAX_3BYTES_SIZE 0x1000000
>>
>> +#define SPI_NOR_MAX_ID_LEN    6
>> +
>>  typedef struct FlashPartInfo {
>>      const char *part_name;
>> -    /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
>> -    uint32_t jedec;
>> -    /* extended jedec code */
>> -    uint16_t ext_jedec;
>> +    /*
>> +     * This array stores the ID bytes.
>
> Comment should be of form
>
> /* some text wrapping
>  * to multiple lines
>  */
>
> for consistency with surrounding code.
>
>> +     * The first three bytes are the JEDIC ID.
>
> "JEDEC"
>
>> +     * JEDEC ID zero means "no ID" (mostly older chips).
>> +     */
>> +    uint8_t id[SPI_NOR_MAX_ID_LEN];
>> +    uint8_t id_len;
>
> This arrayification is a very nice change.
>
>>      /* there is confusion between manufacturers as to what a sector is. In this
>>       * device model, a "sector" is the size that is erased by the ERASE_SECTOR
>>       * command (opcode 0xd8).
>> @@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
>>  } FlashPartInfo;
>>
>>  /* adapted from linux */
>> -
>
> Keep this blank.
>
>> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, _flags)\
>> -    .part_name = (_part_name),\
>> -    .jedec = (_jedec),\
>> -    .ext_jedec = (_ext_jedec),\
>> +/* Used when the "_ext_id" is two bytes at most */
>> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
>> +    .part_name = _part_name,\
>> +    .id = {\
>> +        ((_jedec_id) >> 16) & 0xff,\
>> +        ((_jedec_id) >> 8) & 0xff,\
>> +        (_jedec_id) & 0xff,\
>> +        ((_ext_id) >> 8) & 0xff,\
>> +        (_ext_id) & 0xff,\
>> +          },\
>> +    .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
>> +    .sector_size = (_sector_size),\
>> +    .n_sectors = (_n_sectors),\
>> +    .page_size = 256,\
>> +    .flags = (_flags),
>> +
>> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
>
> I am not sure we need the extra macro. Can you just make the 6th byte
> an extra _flags flag and use the original INFO()? If you do need the
> extra macro, one macro should call the other, or both should call a a
> common macro (accepting _id_len as a macro arg) to avoid the dup of
> the majority of the macro body.
>
> Regards,
> Peter
>
>> +    .part_name = _part_name,\
>> +    .id = {\
>> +        ((_jedec_id) >> 16) & 0xff,\
>> +        ((_jedec_id) >> 8) & 0xff,\
>> +        (_jedec_id) & 0xff,\
>> +        ((_ext_id) >> 16) & 0xff,\
>> +        ((_ext_id) >> 8) & 0xff,\
>> +        (_ext_id) & 0xff,\
>> +          },\
>> +    .id_len = 6,\
>>      .sector_size = (_sector_size),\
>>      .n_sectors = (_n_sectors),\
>>      .page_size = 256,\
>> @@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>
>>      case DIOR:
>> -        switch ((s->pi->jedec >> 16) & 0xFF) {
>> +        switch (s->pi->id[0]) {
>>          case JEDEC_WINBOND:
>>          case JEDEC_SPANSION:
>>              s->needed_bytes = 4;
>> @@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>
>>      case QIOR:
>> -        switch ((s->pi->jedec >> 16) & 0xFF) {
>> +        switch (s->pi->id[0]) {
>>          case JEDEC_WINBOND:
>>          case JEDEC_SPANSION:
>>              s->needed_bytes = 6;
>> @@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>
>>      case JEDEC_READ:
>>          DB_PRINT_L(0, "populated jedec code\n");
>> -        s->data[0] = (s->pi->jedec >> 16) & 0xff;
>> -        s->data[1] = (s->pi->jedec >> 8) & 0xff;
>> -        s->data[2] = s->pi->jedec & 0xff;
>> -        if (s->pi->ext_jedec) {
>> -            s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
>> -            s->data[4] = s->pi->ext_jedec & 0xff;
>> -            s->len = 5;
>> -        } else {
>> -            s->len = 3;
>> +        for (i = 0; i < s->pi->id_len; i++) {
>> +            s->data[i] = s->pi->id[i];
>>          }
>> +        s->len = s->pi->id_len;
>>          s->pos = 0;
>>          s->state = STATE_READING_DATA;
>>          break;
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] [PATCH 10/12] Support for quad commands.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
---
 hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 6fc55a3..25ec666 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -255,19 +255,24 @@ typedef enum {
     BULK_ERASE = 0xc7,
 
     READ = 0x3,
+    READ4 = 0x13,
     FAST_READ = 0xb,
     DOR = 0x3b,
     QOR = 0x6b,
     DIOR = 0xbb,
     QIOR = 0xeb,
+    QIOR4 = 0xec,
 
     PP = 0x2,
+    PP4 = 0x12,
     DPP = 0xa2,
     QPP = 0x32,
 
     ERASE_4K = 0x20,
+    ERASE4_4K = 0x21,
     ERASE_32K = 0x52,
     ERASE_SECTOR = 0xd8,
+    ERASE4_SECTOR = 0xdc,
 
     EN_4BYTE_ADDR = 0xB7,
     EX_4BYTE_ADDR = 0xE9,
@@ -307,6 +312,7 @@ typedef struct Flash {
     bool write_enable;
     bool four_bytes_address_mode;
     bool reset_enable;
+    bool quad_enable;
     bool initialized;
     uint8_t reset_pin;
     uint8_t ear;
@@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
 
     switch (cmd) {
     case ERASE_4K:
+    case ERASE4_4K:
         len = 4 << 10;
         capa_to_assert = ER_4K;
         break;
@@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
         capa_to_assert = ER_32K;
         break;
     case ERASE_SECTOR:
+    case ERASE4_SECTOR:
         len = s->pi->sector_size;
         break;
     case BULK_ERASE:
@@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
 
 static inline int is_4bytes(Flash *s)
 {
+   switch (s->cmd_in_progress) {
+   case PP4:
+   case READ4:
+   case QIOR4:
+   case ERASE4_4K:
+   case ERASE4_SECTOR:
+       return 1;
+   default:
        return s->four_bytes_address_mode;
    }
 }
@@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
     case DPP:
     case QPP:
     case PP:
+    case PP4:
         s->state = STATE_PAGE_PROGRAM;
         break;
     case READ:
+    case READ4:
     case FAST_READ:
     case DOR:
     case QOR:
     case DIOR:
     case QIOR:
+    case QIOR4:
         s->state = STATE_READ;
         break;
     case ERASE_4K:
+    case ERASE4_4K:
     case ERASE_32K:
     case ERASE_SECTOR:
+    case ERASE4_SECTOR:
         flash_erase(s, s->cur_addr, s->cmd_in_progress);
         break;
     case WRSR:
@@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
     s->state = STATE_IDLE;
     s->write_enable = false;
     s->reset_enable = false;
+    s->quad_enable = false;
 
     DB_PRINT_L(0, "Reset done.\n");
 }
@@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
 static void decode_new_cmd(Flash *s, uint32_t value)
 {
     s->cmd_in_progress = value;
+    int i;
     DB_PRINT_L(0, "decoded new command:%x\n", value);
 
     switch (value) {
 
     case ERASE_4K:
+    case ERASE4_4K:
     case ERASE_32K:
     case ERASE_SECTOR:
+    case ERASE4_SECTOR:
     case READ:
+    case READ4:
     case DPP:
     case QPP:
     case PP:
-        s->needed_bytes = 3;
+    case PP4:
+        s->needed_bytes = is_4bytes(s) ? 4 : 3;
         s->pos = 0;
         s->len = 0;
         s->state = STATE_COLLECTING_DATA;
@@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         s->len = 0;
         s->state = STATE_COLLECTING_DATA;
         break;
-
+    case QIOR4:
+        /* 4 address + 1 dummy */
+        s->needed_bytes = 5;
+        s->pos = 0;
+        s->len = 0;
+        s->state = STATE_COLLECTING_DATA;
+        break;
     case WRSR:
         if (s->write_enable) {
             s->needed_bytes = 1;
@@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_BOOL(four_bytes_address_mode, Flash),
         VMSTATE_UINT8(ear, Flash),
         VMSTATE_BOOL(reset_enable, Flash),
+        VMSTATE_BOOL(quad_enable, Flash),
         VMSTATE_BOOL(initialized, Flash),
         VMSTATE_UINT8(reset_pin, Flash),
         VMSTATE_END_OF_LIST()
-- 
2.5.0

Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>

Same comments as for earlier patches. Need to clarify authorship and
add subsystem prefix and commit paragraph.

> ---
>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 6fc55a3..25ec666 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -255,19 +255,24 @@ typedef enum {
>      BULK_ERASE = 0xc7,
>
>      READ = 0x3,
> +    READ4 = 0x13,
>      FAST_READ = 0xb,
>      DOR = 0x3b,
>      QOR = 0x6b,
>      DIOR = 0xbb,
>      QIOR = 0xeb,
> +    QIOR4 = 0xec,
>
>      PP = 0x2,
> +    PP4 = 0x12,
>      DPP = 0xa2,
>      QPP = 0x32,
>
>      ERASE_4K = 0x20,
> +    ERASE4_4K = 0x21,
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> +    ERASE4_SECTOR = 0xdc,
>
>      EN_4BYTE_ADDR = 0xB7,
>      EX_4BYTE_ADDR = 0xE9,
> @@ -307,6 +312,7 @@ typedef struct Flash {
>      bool write_enable;
>      bool four_bytes_address_mode;
>      bool reset_enable;
> +    bool quad_enable;

What is this used for?

>      bool initialized;
>      uint8_t reset_pin;
>      uint8_t ear;
> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>
>      switch (cmd) {
>      case ERASE_4K:
> +    case ERASE4_4K:
>          len = 4 << 10;
>          capa_to_assert = ER_4K;
>          break;
> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>          capa_to_assert = ER_32K;
>          break;
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          len = s->pi->sector_size;
>          break;
>      case BULK_ERASE:
> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>
>  static inline int is_4bytes(Flash *s)
>  {
> +   switch (s->cmd_in_progress) {
> +   case PP4:
> +   case READ4:
> +   case QIOR4:
> +   case ERASE4_4K:
> +   case ERASE4_SECTOR:
> +       return 1;
> +   default:
>         return s->four_bytes_address_mode;
>     }
>  }
> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>      case DPP:
>      case QPP:
>      case PP:
> +    case PP4:
>          s->state = STATE_PAGE_PROGRAM;
>          break;
>      case READ:
> +    case READ4:
>      case FAST_READ:
>      case DOR:
>      case QOR:
>      case DIOR:
>      case QIOR:
> +    case QIOR4:
>          s->state = STATE_READ;
>          break;
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>          break;
>      case WRSR:
> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>      s->state = STATE_IDLE;
>      s->write_enable = false;
>      s->reset_enable = false;
> +    s->quad_enable = false;
>
>      DB_PRINT_L(0, "Reset done.\n");
>  }
> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>      s->cmd_in_progress = value;
> +    int i;

I can't see where i is used?

>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>
>      switch (value) {
>
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>      case READ:
> +    case READ4:
>      case DPP:
>      case QPP:
>      case PP:
> -        s->needed_bytes = 3;
> +    case PP4:
> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
>          s->pos = 0;
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
>          break;
> -
> +    case QIOR4:
> +        /* 4 address + 1 dummy */
> +        s->needed_bytes = 5;
> +        s->pos = 0;
> +        s->len = 0;
> +        s->state = STATE_COLLECTING_DATA;
> +        break;

Blank line needed here. The blank is omitted between between logical
groupings of commands (such as read-write pairs or different types of
the same op) but there is no grouping between the data RW ops and
WRSR.

Regards,
Peter

>      case WRSR:
>          if (s->write_enable) {
>              s->needed_bytes = 1;
> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>          VMSTATE_UINT8(ear, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
> +        VMSTATE_BOOL(quad_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
>          VMSTATE_END_OF_LIST()
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 10/12] Support for quad commands.

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

W dniu 21.12.2015 o 12:52, 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: Pawel Lenkow <pawel.lenkow@nokia.com>
>
> Same comments as for earlier patches. Need to clarify authorship and
> add subsystem prefix and commit paragraph.
>
>> ---
>>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 6fc55a3..25ec666 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -255,19 +255,24 @@ typedef enum {
>>      BULK_ERASE = 0xc7,
>>
>>      READ = 0x3,
>> +    READ4 = 0x13,
>>      FAST_READ = 0xb,
>>      DOR = 0x3b,
>>      QOR = 0x6b,
>>      DIOR = 0xbb,
>>      QIOR = 0xeb,
>> +    QIOR4 = 0xec,
>>
>>      PP = 0x2,
>> +    PP4 = 0x12,
>>      DPP = 0xa2,
>>      QPP = 0x32,
>>
>>      ERASE_4K = 0x20,
>> +    ERASE4_4K = 0x21,
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>> +    ERASE4_SECTOR = 0xdc,
>>
>>      EN_4BYTE_ADDR = 0xB7,
>>      EX_4BYTE_ADDR = 0xE9,
>> @@ -307,6 +312,7 @@ typedef struct Flash {
>>      bool write_enable;
>>      bool four_bytes_address_mode;
>>      bool reset_enable;
>> +    bool quad_enable;
>
> What is this used for?
>
>>      bool initialized;
>>      uint8_t reset_pin;
>>      uint8_t ear;
>> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>
>>      switch (cmd) {
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>          len = 4 << 10;
>>          capa_to_assert = ER_4K;
>>          break;
>> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>          capa_to_assert = ER_32K;
>>          break;
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>          len = s->pi->sector_size;
>>          break;
>>      case BULK_ERASE:
>> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>>
>>  static inline int is_4bytes(Flash *s)
>>  {
>> +   switch (s->cmd_in_progress) {
>> +   case PP4:
>> +   case READ4:
>> +   case QIOR4:
>> +   case ERASE4_4K:
>> +   case ERASE4_SECTOR:
>> +       return 1;
>> +   default:
>>         return s->four_bytes_address_mode;
>>     }
>>  }
>> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>>      case DPP:
>>      case QPP:
>>      case PP:
>> +    case PP4:
>>          s->state = STATE_PAGE_PROGRAM;
>>          break;
>>      case READ:
>> +    case READ4:
>>      case FAST_READ:
>>      case DOR:
>>      case QOR:
>>      case DIOR:
>>      case QIOR:
>> +    case QIOR4:
>>          s->state = STATE_READ;
>>          break;
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>      case ERASE_32K:
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>>          break;
>>      case WRSR:
>> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>>      s->state = STATE_IDLE;
>>      s->write_enable = false;
>>      s->reset_enable = false;
>> +    s->quad_enable = false;
>>
>>      DB_PRINT_L(0, "Reset done.\n");
>>  }
>> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>>  static void decode_new_cmd(Flash *s, uint32_t value)
>>  {
>>      s->cmd_in_progress = value;
>> +    int i;
>
> I can't see where i is used?
Should be in patch 09/12.
I will correct this.
Thanks,
Marcin
>
>
>>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>>
>>      switch (value) {
>>
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>      case ERASE_32K:
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>      case READ:
>> +    case READ4:
>>      case DPP:
>>      case QPP:
>>      case PP:
>> -        s->needed_bytes = 3;
>> +    case PP4:
>> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
>>          s->pos = 0;
>>          s->len = 0;
>>          s->state = STATE_COLLECTING_DATA;
>> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          s->len = 0;
>>          s->state = STATE_COLLECTING_DATA;
>>          break;
>> -
>> +    case QIOR4:
>> +        /* 4 address + 1 dummy */
>> +        s->needed_bytes = 5;
>> +        s->pos = 0;
>> +        s->len = 0;
>> +        s->state = STATE_COLLECTING_DATA;
>> +        break;
>
> Blank line needed here. The blank is omitted between between logical
> groupings of commands (such as read-write pairs or different types of
> the same op) but there is no grouping between the data RW ops and
> WRSR.
>
> Regards,
> Peter
>
>>      case WRSR:
>>          if (s->write_enable) {
>>              s->needed_bytes = 1;
>> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>          VMSTATE_UINT8(ear, Flash),
>>          VMSTATE_BOOL(reset_enable, Flash),
>> +        VMSTATE_BOOL(quad_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>>          VMSTATE_END_OF_LIST()
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
> ---
>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 6fc55a3..25ec666 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -255,19 +255,24 @@ typedef enum {
>      BULK_ERASE = 0xc7,
>
>      READ = 0x3,
> +    READ4 = 0x13,
>      FAST_READ = 0xb,
>      DOR = 0x3b,
>      QOR = 0x6b,
>      DIOR = 0xbb,
>      QIOR = 0xeb,
> +    QIOR4 = 0xec,
>
>      PP = 0x2,
> +    PP4 = 0x12,
>      DPP = 0xa2,
>      QPP = 0x32,
>
>      ERASE_4K = 0x20,
> +    ERASE4_4K = 0x21,
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> +    ERASE4_SECTOR = 0xdc,
>
>      EN_4BYTE_ADDR = 0xB7,
>      EX_4BYTE_ADDR = 0xE9,
> @@ -307,6 +312,7 @@ typedef struct Flash {
>      bool write_enable;
>      bool four_bytes_address_mode;
>      bool reset_enable;
> +    bool quad_enable;
>      bool initialized;
>      uint8_t reset_pin;
>      uint8_t ear;
> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>
>      switch (cmd) {
>      case ERASE_4K:
> +    case ERASE4_4K:
>          len = 4 << 10;
>          capa_to_assert = ER_4K;
>          break;
> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>          capa_to_assert = ER_32K;
>          break;
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          len = s->pi->sector_size;
>          break;
>      case BULK_ERASE:
> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>
>  static inline int is_4bytes(Flash *s)
>  {
> +   switch (s->cmd_in_progress) {
> +   case PP4:
> +   case READ4:
> +   case QIOR4:
> +   case ERASE4_4K:
> +   case ERASE4_SECTOR:
> +       return 1;
> +   default:
>         return s->four_bytes_address_mode;
>     }
>  }
> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>      case DPP:
>      case QPP:
>      case PP:
> +    case PP4:
>          s->state = STATE_PAGE_PROGRAM;
>          break;
>      case READ:
> +    case READ4:
>      case FAST_READ:
>      case DOR:
>      case QOR:
>      case DIOR:
>      case QIOR:
> +    case QIOR4:
>          s->state = STATE_READ;
>          break;
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>          break;
>      case WRSR:
> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>      s->state = STATE_IDLE;
>      s->write_enable = false;
>      s->reset_enable = false;
> +    s->quad_enable = false;
>
>      DB_PRINT_L(0, "Reset done.\n");
>  }
> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>      s->cmd_in_progress = value;
> +    int i;
>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>
>      switch (value) {
>
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>      case READ:
> +    case READ4:
>      case DPP:
>      case QPP:
>      case PP:
> -        s->needed_bytes = 3;
> +    case PP4:
> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
>          s->pos = 0;
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
>          break;
> -
> +    case QIOR4:
> +        /* 4 address + 1 dummy */
> +        s->needed_bytes = 5;

So looking at the current QIOR handling, the number of "dummy bytes"
is calculated as:

num_dummy_cycles X 4.

This makes sense, as both the address and data-phase on either side of
the dummy phase is done on four-wires, so it is cleaner that the dummy
phase is also on 4 wires. Ideally we implement Dual and Quad mode
awareness and cycle accuracy on the SSI layer itself to handle these
cases, but until then we stuck with this strange fixed policy.

For numonyx, this gives 1 command + 3 address + 4 dummies ( == 8
cycles) = 8 needed_bytes.

So this would be 9 for the extra address byte in 4b mode.

> +        s->pos = 0;
> +        s->len = 0;
> +        s->state = STATE_COLLECTING_DATA;
> +        break;

Following on from, Cedric's spot, should this be a fallthrough to
regular QIOR, which then uses is_4bytes to implement the +1 on needed
bytes?

Regards,
Peter

>      case WRSR:
>          if (s->write_enable) {
>              s->needed_bytes = 1;
> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>          VMSTATE_UINT8(ear, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
> +        VMSTATE_BOOL(quad_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
>          VMSTATE_END_OF_LIST()
> --
> 2.5.0
>
>

Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Peter Crosthwaite]

On Tue, Dec 22, 2015 at 1:40 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>
>> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
>> ---
>>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 6fc55a3..25ec666 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -255,19 +255,24 @@ typedef enum {
>>      BULK_ERASE = 0xc7,
>>
>>      READ = 0x3,
>> +    READ4 = 0x13,
>>      FAST_READ = 0xb,
>>      DOR = 0x3b,
>>      QOR = 0x6b,
>>      DIOR = 0xbb,
>>      QIOR = 0xeb,
>> +    QIOR4 = 0xec,
>>
>>      PP = 0x2,
>> +    PP4 = 0x12,
>>      DPP = 0xa2,
>>      QPP = 0x32,
>>
>>      ERASE_4K = 0x20,
>> +    ERASE4_4K = 0x21,
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>> +    ERASE4_SECTOR = 0xdc,
>>
>>      EN_4BYTE_ADDR = 0xB7,
>>      EX_4BYTE_ADDR = 0xE9,
>> @@ -307,6 +312,7 @@ typedef struct Flash {
>>      bool write_enable;
>>      bool four_bytes_address_mode;
>>      bool reset_enable;
>> +    bool quad_enable;
>>      bool initialized;
>>      uint8_t reset_pin;
>>      uint8_t ear;
>> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>
>>      switch (cmd) {
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>          len = 4 << 10;
>>          capa_to_assert = ER_4K;
>>          break;
>> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>          capa_to_assert = ER_32K;
>>          break;
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>          len = s->pi->sector_size;
>>          break;
>>      case BULK_ERASE:
>> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>>
>>  static inline int is_4bytes(Flash *s)
>>  {
>> +   switch (s->cmd_in_progress) {
>> +   case PP4:
>> +   case READ4:
>> +   case QIOR4:
>> +   case ERASE4_4K:
>> +   case ERASE4_SECTOR:
>> +       return 1;
>> +   default:
>>         return s->four_bytes_address_mode;
>>     }
>>  }
>> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>>      case DPP:
>>      case QPP:
>>      case PP:
>> +    case PP4:
>>          s->state = STATE_PAGE_PROGRAM;
>>          break;
>>      case READ:
>> +    case READ4:
>>      case FAST_READ:
>>      case DOR:
>>      case QOR:
>>      case DIOR:
>>      case QIOR:
>> +    case QIOR4:
>>          s->state = STATE_READ;
>>          break;
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>      case ERASE_32K:
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>>          break;
>>      case WRSR:
>> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>>      s->state = STATE_IDLE;
>>      s->write_enable = false;
>>      s->reset_enable = false;
>> +    s->quad_enable = false;
>>
>>      DB_PRINT_L(0, "Reset done.\n");
>>  }
>> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>>  static void decode_new_cmd(Flash *s, uint32_t value)
>>  {
>>      s->cmd_in_progress = value;
>> +    int i;
>>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>>
>>      switch (value) {
>>
>>      case ERASE_4K:
>> +    case ERASE4_4K:
>>      case ERASE_32K:
>>      case ERASE_SECTOR:
>> +    case ERASE4_SECTOR:
>>      case READ:
>> +    case READ4:
>>      case DPP:
>>      case QPP:
>>      case PP:
>> -        s->needed_bytes = 3;
>> +    case PP4:
>> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
>>          s->pos = 0;
>>          s->len = 0;
>>          s->state = STATE_COLLECTING_DATA;
>> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          s->len = 0;
>>          s->state = STATE_COLLECTING_DATA;
>>          break;
>> -
>> +    case QIOR4:
>> +        /* 4 address + 1 dummy */
>> +        s->needed_bytes = 5;
>
> So looking at the current QIOR handling, the number of "dummy bytes"
> is calculated as:
>
> num_dummy_cycles X 4.
>
> This makes sense, as both the address and data-phase on either side of
> the dummy phase is done on four-wires, so it is cleaner that the dummy
> phase is also on 4 wires. Ideally we implement Dual and Quad mode
> awareness and cycle accuracy on the SSI layer itself to handle these
> cases, but until then we stuck with this strange fixed policy.
>
> For numonyx, this gives 1 command + 3 address + 4 dummies ( == 8
> cycles) = 8 needed_bytes.
>

Correction, there is no command byte in the needed bytes, rather there
a 5 dummies (for 10 cycles).

Regards,
Peter

> So this would be 9 for the extra address byte in 4b mode.
>
>> +        s->pos = 0;
>> +        s->len = 0;
>> +        s->state = STATE_COLLECTING_DATA;
>> +        break;
>
> Following on from, Cedric's spot, should this be a fallthrough to
> regular QIOR, which then uses is_4bytes to implement the +1 on needed
> bytes?
>
> Regards,
> Peter
>
>>      case WRSR:
>>          if (s->write_enable) {
>>              s->needed_bytes = 1;
>> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>          VMSTATE_UINT8(ear, Flash),
>>          VMSTATE_BOOL(reset_enable, Flash),
>> +        VMSTATE_BOOL(quad_enable, Flash),
>>          VMSTATE_BOOL(initialized, Flash),
>>          VMSTATE_UINT8(reset_pin, Flash),
>>          VMSTATE_END_OF_LIST()
>> --
>> 2.5.0
>>
>>

Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Lenkow, Pawel (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Wednesday, December 23, 2015 3:25 AM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw); Cédric Le Goater
> Cc: qemu-devel@nongnu.org Developers; Lenkow, Pawel (Nokia -
> PL/Wroclaw)
> Subject: Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.
> 
> On Tue, Dec 22, 2015 at 1:40 PM, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
> > On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
> >> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>
> >> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
> >> ---
> >>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++-
> -
> >>  1 file changed, 36 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 6fc55a3..25ec666 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -255,19 +255,24 @@ typedef enum {
> >>      BULK_ERASE = 0xc7,
> >>
> >>      READ = 0x3,
> >> +    READ4 = 0x13,
> >>      FAST_READ = 0xb,
> >>      DOR = 0x3b,
> >>      QOR = 0x6b,
> >>      DIOR = 0xbb,
> >>      QIOR = 0xeb,
> >> +    QIOR4 = 0xec,
> >>
> >>      PP = 0x2,
> >> +    PP4 = 0x12,
> >>      DPP = 0xa2,
> >>      QPP = 0x32,
> >>
> >>      ERASE_4K = 0x20,
> >> +    ERASE4_4K = 0x21,
> >>      ERASE_32K = 0x52,
> >>      ERASE_SECTOR = 0xd8,
> >> +    ERASE4_SECTOR = 0xdc,
> >>
> >>      EN_4BYTE_ADDR = 0xB7,
> >>      EX_4BYTE_ADDR = 0xE9,
> >> @@ -307,6 +312,7 @@ typedef struct Flash {
> >>      bool write_enable;
> >>      bool four_bytes_address_mode;
> >>      bool reset_enable;
> >> +    bool quad_enable;
> >>      bool initialized;
> >>      uint8_t reset_pin;
> >>      uint8_t ear;
> >> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >>
> >>      switch (cmd) {
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>          len = 4 << 10;
> >>          capa_to_assert = ER_4K;
> >>          break;
> >> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >>          capa_to_assert = ER_32K;
> >>          break;
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>          len = s->pi->sector_size;
> >>          break;
> >>      case BULK_ERASE:
> >> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t
> data)
> >>
> >>  static inline int is_4bytes(Flash *s)
> >>  {
> >> +   switch (s->cmd_in_progress) {
> >> +   case PP4:
> >> +   case READ4:
> >> +   case QIOR4:
> >> +   case ERASE4_4K:
> >> +   case ERASE4_SECTOR:
> >> +       return 1;
> >> +   default:
> >>         return s->four_bytes_address_mode;
> >>     }
> >>  }
> >> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
> >>      case DPP:
> >>      case QPP:
> >>      case PP:
> >> +    case PP4:
> >>          s->state = STATE_PAGE_PROGRAM;
> >>          break;
> >>      case READ:
> >> +    case READ4:
> >>      case FAST_READ:
> >>      case DOR:
> >>      case QOR:
> >>      case DIOR:
> >>      case QIOR:
> >> +    case QIOR4:
> >>          s->state = STATE_READ;
> >>          break;
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>      case ERASE_32K:
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >>          break;
> >>      case WRSR:
> >> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
> >>      s->state = STATE_IDLE;
> >>      s->write_enable = false;
> >>      s->reset_enable = false;
> >> +    s->quad_enable = false;
> >>
> >>      DB_PRINT_L(0, "Reset done.\n");
> >>  }
> >> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
> >>  static void decode_new_cmd(Flash *s, uint32_t value)
> >>  {
> >>      s->cmd_in_progress = value;
> >> +    int i;
> >>      DB_PRINT_L(0, "decoded new command:%x\n", value);
> >>
> >>      switch (value) {
> >>
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>      case ERASE_32K:
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>      case READ:
> >> +    case READ4:
> >>      case DPP:
> >>      case QPP:
> >>      case PP:
> >> -        s->needed_bytes = 3;
> >> +    case PP4:
> >> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
> >>          s->pos = 0;
> >>          s->len = 0;
> >>          s->state = STATE_COLLECTING_DATA;
> >> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t
> value)
> >>          s->len = 0;
> >>          s->state = STATE_COLLECTING_DATA;
> >>          break;
> >> -
> >> +    case QIOR4:
> >> +        /* 4 address + 1 dummy */
> >> +        s->needed_bytes = 5;
> >
> > So looking at the current QIOR handling, the number of "dummy bytes"
> > is calculated as:
> >
> > num_dummy_cycles X 4.
> >
> > This makes sense, as both the address and data-phase on either side of
> > the dummy phase is done on four-wires, so it is cleaner that the dummy
> > phase is also on 4 wires. Ideally we implement Dual and Quad mode
> > awareness and cycle accuracy on the SSI layer itself to handle these
> > cases, but until then we stuck with this strange fixed policy.
> >
> > For numonyx, this gives 1 command + 3 address + 4 dummies ( == 8
> > cycles) = 8 needed_bytes.
> >
> 
> Correction, there is no command byte in the needed bytes, rather there
> a 5 dummies (for 10 cycles).
> 
> Regards,
> Peter
> 

Yes you are right, I will fixed it.
Micron has by default 10 cycles for QIOR so it will be 5 bytes extra.
I need to check to Spansion datasheets, because there is no clear for me how
many dummy cycles are required. 


> > So this would be 9 for the extra address byte in 4b mode.
> >
> >> +        s->pos = 0;
> >> +        s->len = 0;
> >> +        s->state = STATE_COLLECTING_DATA;
> >> +        break;
> >
> > Following on from, Cedric's spot, should this be a fallthrough to
> > regular QIOR, which then uses is_4bytes to implement the +1 on needed
> > bytes?


Yes, good idea.
Br,

Pawel Lenkow


> >
> > Regards,
> > Peter
> >
> >>      case WRSR:
> >>          if (s->write_enable) {
> >>              s->needed_bytes = 1;
> >> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80
> = {
> >>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
> >>          VMSTATE_UINT8(ear, Flash),
> >>          VMSTATE_BOOL(reset_enable, Flash),
> >> +        VMSTATE_BOOL(quad_enable, Flash),
> >>          VMSTATE_BOOL(initialized, Flash),
> >>          VMSTATE_UINT8(reset_pin, Flash),
> >>          VMSTATE_END_OF_LIST()
> >> --
> >> 2.5.0
> >>
> >>

[Qemu-devel] [PATCH 11/12] Support for mx66u51235 and s25fl512s

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
---
 hw/block/m25p80.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 25ec666..fadd6ec 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -107,6 +107,7 @@ typedef struct FlashPartInfo {
 #define JEDEC_NUMONYX 0x20
 #define JEDEC_WINBOND 0xEF
 #define JEDEC_SPANSION 0x01
+#define JEDEC_MACRONIX 0xC2
 
 static const FlashPartInfo known_devices[] = {
     /* Atmel -- some are (confusingly) marketed as "DataFlash" */
@@ -157,6 +158,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
     { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
     { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
+    { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
 
     /* Micron */
     { INFO("n25q032a11",  0x20bb16,      0,  64 << 10,  64, ER_4K) },
@@ -175,7 +177,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("s25sl064p",   0x010216, 0x4d00,  64 << 10, 128, ER_4K) },
     { INFO("s25fl256s0",  0x010219, 0x4d00, 256 << 10, 128, 0) },
     { INFO("s25fl256s1",  0x010219, 0x4d01,  64 << 10, 512, 0) },
-    { INFO("s25fl512s",   0x010220, 0x4d00, 256 << 10, 256, 0) },
+    { INFO6("s25fl512s",   0x010220, 0x4d0080, 256 << 10, 256, 0) },
     { INFO("s70fl01gs",   0x010221, 0x4d00, 256 << 10, 256, 0) },
     { INFO("s25sl12800",  0x012018, 0x0300, 256 << 10,  64, 0) },
     { INFO("s25sl12801",  0x012018, 0x0301,  64 << 10, 256, 0) },
@@ -189,6 +191,9 @@ static const FlashPartInfo known_devices[] = {
     { INFO("s25fl016k",   0xef4015,      0,  64 << 10,  32, ER_4K | ER_32K) },
     { INFO("s25fl064k",   0xef4017,      0,  64 << 10, 128, ER_4K | ER_32K) },
 
+    /* Spansion --  boot sectors support  */
+    { INFO6("s25fs512s",    0x010220, 0x4d0081, 256 << 10, 256, 0) },
+
     /* SST -- large erase sizes are "overlays", "sectors" are 4<< 10 */
     { INFO("sst25vf040b", 0xbf258d,      0,  64 << 10,   8, ER_4K) },
     { INFO("sst25vf080b", 0xbf258e,      0,  64 << 10,  16, ER_4K) },
@@ -283,6 +288,12 @@ typedef enum {
     RESET_ENABLE = 0x66,
     RESET_MEMORY = 0x99,
 
+    /*
+     * Micron: 0x35 - enable QPI
+     * Spansion: 0x35 - read control register
+     */
+    RDCR_QPIEN = 0x35,
+
 } FlashCMD;
 
 typedef enum {
@@ -509,6 +520,16 @@ static void complete_collecting_data(Flash *s)
         flash_erase(s, s->cur_addr, s->cmd_in_progress);
         break;
     case WRSR:
+        switch (s->pi->id[0]) {
+        case JEDEC_SPANSION:
+            s->quad_enable = !!(s->data[1] & 0x02);
+            break;
+        case JEDEC_MACRONIX:
+            s->quad_enable = !!(s->data[0] & 0x40);
+            break;
+        default:
+            break;
+        }
         if (s->write_enable) {
             s->write_enable = false;
         }
@@ -610,7 +631,19 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
     case WRSR:
         if (s->write_enable) {
-            s->needed_bytes = 1;
+            switch (s->pi->id[0]) {
+            case JEDEC_SPANSION:
+            /* FIXME:
+             * needed_bytes fixed to 2 because Spansion
+             * supports access to 2nd register
+             * in one WRSR command and Linux is using it.
+             * Real needed_bytes should depend on CS line.
+             */
+                s->needed_bytes = 2;
+                break;
+            default:
+                s->needed_bytes = 1;
+            }
             s->pos = 0;
             s->len = 0;
             s->state = STATE_COLLECTING_DATA;
@@ -626,6 +659,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 
     case RDSR:
         s->data[0] = (!!s->write_enable) << 1;
+        if (s->pi->id[0] == JEDEC_MACRONIX) {
+            s->data[0] |= (!!s->quad_enable) << 6;
+        }
         s->pos = 0;
         s->len = 1;
         s->state = STATE_READING_DATA;
@@ -680,6 +716,18 @@ static void decode_new_cmd(Flash *s, uint32_t value)
             reset_memory(s);
         }
         break;
+    case RDCR_QPIEN:
+        switch (s->pi->id[0]) {
+        case JEDEC_SPANSION:
+            s->data[0] = (!!s->quad_enable) << 1;
+            s->pos = 0;
+            s->len = 1;
+            s->state = STATE_READING_DATA;
+            break;
+        default:
+            break;
+        }
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
         break;
-- 
2.5.0

[Qemu-devel] [PATCH 12/12] Read flag status register command support added.

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index fadd6ec..ef05ad3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -258,6 +258,7 @@ typedef enum {
     WREN = 0x6,
     JEDEC_READ = 0x9f,
     BULK_ERASE = 0xc7,
+    READ_FSR = 0x70,
 
     READ = 0x3,
     READ4 = 0x13,
@@ -667,6 +668,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         s->state = STATE_READING_DATA;
         break;
 
+    case READ_FSR:
+        s->data[0] = (1<<7); /*Indicates flash is ready */
+        s->pos = 0;
+        s->len = 1;
+        s->state = STATE_READING_DATA;
+        break;
+
     case JEDEC_READ:
         DB_PRINT_L(0, "populated jedec code\n");
         for (i = 0; i < s->pi->id_len; i++) {
-- 
2.5.0

Re: [Qemu-devel] [PATCH 12/12] Read flag status register command support added.

[Peter Crosthwaite]

Subsystem prefix needed and reverse commit subject to start with verb:

"block: m25p80: Add Read Flag Status command"

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>

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Thanks to you and Pawel for this work.

Regards,
Peter

> ---
>  hw/block/m25p80.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index fadd6ec..ef05ad3 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -258,6 +258,7 @@ typedef enum {
>      WREN = 0x6,
>      JEDEC_READ = 0x9f,
>      BULK_ERASE = 0xc7,
> +    READ_FSR = 0x70,
>
>      READ = 0x3,
>      READ4 = 0x13,
> @@ -667,6 +668,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->state = STATE_READING_DATA;
>          break;
>
> +    case READ_FSR:
> +        s->data[0] = (1<<7); /*Indicates flash is ready */
> +        s->pos = 0;
> +        s->len = 1;
> +        s->state = STATE_READING_DATA;
> +        break;
> +
>      case JEDEC_READ:
>          DB_PRINT_L(0, "populated jedec code\n");
>          for (i = 0; i < s->pi->id_len; i++) {
> --
> 2.5.0
>
>

[Qemu-devel] ODP: [PATCH 12/12] Read flag status register command support added.

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

W dniu 21.12.2015 o 12:54, Peter Crosthwaite pisze:
> Subsystem prefix needed and reverse commit subject to start with verb:
>
> "block: m25p80: Add Read Flag Status command"
>
> 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>
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> Thanks to you and Pawel for this work.
Thanks for review and sorry for many technical issues in patches.

Thanks,
Marcin
>
>
> Regards,
> Peter
>
>> ---
>>  hw/block/m25p80.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index fadd6ec..ef05ad3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -258,6 +258,7 @@ typedef enum {
>>      WREN = 0x6,
>>      JEDEC_READ = 0x9f,
>>      BULK_ERASE = 0xc7,
>> +    READ_FSR = 0x70,
>>
>>      READ = 0x3,
>>      READ4 = 0x13,
>> @@ -667,6 +668,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          s->state = STATE_READING_DATA;
>>          break;
>>
>> +    case READ_FSR:
>> +        s->data[0] = (1<<7); /*Indicates flash is ready */
>> +        s->pos = 0;
>> +        s->len = 1;
>> +        s->state = STATE_READING_DATA;
>> +        break;
>> +
>>      case JEDEC_READ:
>>          DB_PRINT_L(0, "populated jedec code\n");
>>          for (i = 0; i < s->pi->id_len; i++) {
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH 00/12] Support for new flash devices/4bytes commands

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

Corrected Peters address ( it seem that maintainers file has wrong email address).

> -----Original Message-----
> From: marcin.krzeminski@nokia.com [mailto:marcin.krzeminski@nokia.com]
> Sent: Wednesday, December 16, 2015 1:57 PM
> To: qemu-devel@nongnu.org
> Cc: Lenkow, Pawel (Nokia - PL/Wroclaw); Krzeminski, Marcin (Nokia -
> PL/Wroclaw); peter.crosthwaite@xilinx.com
> Subject: [PATCH 00/12] Support for new flash devices/4bytes commands
> 
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> Hello,
> 
> This patch series adds support for flash devices:
> N25Q256A,N25Q512A,MX66U51235 and S25FL512S, with needed 4bytes
> commands.
> Additionally support for serial eeproms AT25128A/AT25256A has been
> added.
> Since this patchset has more functionality than first version, it is not v2.
> 
> 
> Marcin Krzeminski (12):
>   Removed unused variable.
>   Added reset-pin emulation in model.
>   Reset enable and reset memory commands support.
>   Changed variable type to allow serial eeprom emulation (changing
>     0->1).
>   Added support for serial eeproms - AT25128A/AT25256A
>   4byte address mode support added.
>   Added support for extend address mode commands.
>   Support for N25Q256A/N25Q512A
>   Support for 6Bytes jdec.
>   Support for quad commands.
>   Support for mx66u51235 and s25fl512s
>   Read flag status register command support added.
> 
>  hw/block/m25p80.c | 282
> ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 251 insertions(+), 31 deletions(-)
> 
> --
> 2.5.0