[Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model

[Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model

[marcin.krzeminski]

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

This series introduce some improvememnt targeting mt25qu01g.

Marcin Krzeminski (2):
  block: m25p80: Add Quad Page Program 4byte version op
  block: m25p80: Introduce Die Erase command

 hw/block/m25p80.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op

[marcin.krzeminski]

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

Some flash chips has additional page program opcode that
takes only 4 byte address. This commit adds support
for such command in Qemu.

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index d29ff4c..2bc7028 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -325,6 +325,7 @@ typedef enum {
     PP4_4 = 0x3e,
     DPP = 0xa2,
     QPP = 0x32,
+    QPP_4 = 0x34,
 
     ERASE_4K = 0x20,
     ERASE4_4K = 0x21,
@@ -573,6 +574,7 @@ static inline int get_addr_length(Flash *s)
    switch (s->cmd_in_progress) {
    case PP4:
    case PP4_4:
+   case QPP_4:
    case READ4:
    case QIOR4:
    case ERASE4_4K:
@@ -606,6 +608,7 @@ static void complete_collecting_data(Flash *s)
     switch (s->cmd_in_progress) {
     case DPP:
     case QPP:
+    case QPP_4:
     case PP:
     case PP4:
     case PP4_4:
@@ -873,6 +876,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case READ4:
     case DPP:
     case QPP:
+    case QPP_4:
     case PP:
     case PP4:
     case PP4_4:
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command

[marcin.krzeminski]

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

Big flash chips (like mt25qu01g) are consisted from dies.
Because of that some manufactures remove support for Chip
Erase giving Die Erase command instead.To avoid unnecessary
code complication, support for chip erase for mt25qu01g
is not removed.

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 2bc7028..0bc1fbf 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = {
     { 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) },
-    { INFO("mt25ql01g",   0x20ba21,      0,  64 << 10, 2048, ER_4K) },
-    { INFO("mt25qu01g",   0x20bb21,      0,  64 << 10, 2048, ER_4K) },
+    { INFO("mt25ql01g",   0x20ba21, 0x1040,  64 << 10, 2048, ER_4K) },
+    { INFO("mt25qu01g",   0x20bb21, 0x1040,  64 << 10, 2048, ER_4K) },
 
     /* Spansion -- single (large) sector size only, at least
      * for the chips listed here (without boot sectors).
@@ -358,6 +358,8 @@ typedef enum {
 
     REVCR = 0x65,
     WEVCR = 0x61,
+
+    DIE_ERASE = 0xC4,
 } FlashCMD;
 
 typedef enum {
@@ -411,6 +413,7 @@ typedef struct Flash {
     bool reset_enable;
     bool quad_enable;
     uint8_t ear;
+    uint32_t die_cnt;
 
     int64_t dirty_page;
 
@@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
 
 static void flash_erase(Flash *s, int offset, FlashCMD cmd)
 {
-    uint32_t len;
+    uint32_t len = 0;
     uint8_t capa_to_assert = 0;
 
     switch (cmd) {
@@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
     case BULK_ERASE:
         len = s->size;
         break;
+    case DIE_ERASE:
+        if (s->die_cnt) {
+            len = s->size / s->die_cnt;
+            offset = offset & (~(len-1));
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by"
+                          " device\n", len);
+            return;
+        }
+        break;
     default:
         abort();
     }
@@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s)
     case ERASE4_32K:
     case ERASE_SECTOR:
     case ERASE4_SECTOR:
+    case DIE_ERASE:
         flash_erase(s, s->cur_addr, s->cmd_in_progress);
         break;
     case WRSR:
@@ -684,6 +698,7 @@ static void reset_memory(Flash *s)
     s->write_enable = false;
     s->reset_enable = false;
     s->quad_enable = false;
+    s->die_cnt = 0;
 
     switch (get_man(s)) {
     case MAN_NUMONYX:
@@ -716,7 +731,15 @@ static void reset_memory(Flash *s)
             s->four_bytes_address_mode = true;
         }
         if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) {
-            s->ear = s->size / MAX_3BYTES_SIZE - 1;
+            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0);
+        }
+        /* 1GiB devices */
+        if (s->pi->id[2] == 0x21) {
+            /* MT25Q00 has 2 dies N25Q00 has 4 */
+            if (s->pi->id[4] & BIT(6))
+                s->die_cnt = 2;
+            else
+                s->die_cnt = 4;
         }
         break;
     case MAN_MACRONIX:
@@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case PP:
     case PP4:
     case PP4_4:
+    case DIE_ERASE:
         s->needed_bytes = get_addr_length(s);
         s->pos = 0;
         s->len = 0;
@@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT8(spansion_cr2nv, Flash),
         VMSTATE_UINT8(spansion_cr3nv, Flash),
         VMSTATE_UINT8(spansion_cr4nv, Flash),
+        VMSTATE_UINT32(die_cnt, Flash),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model
Message-id: 1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com -> patchew/1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com
Switched to a new branch 'test'
44ae298 block: m25p80: Introduce Die Erase command
1c50e2e block: m25p80: Add Quad Page Program 4byte version op

=== OUTPUT BEGIN ===
Checking PATCH 1/2: block: m25p80: Add Quad Page Program 4byte version op...
Checking PATCH 2/2: block: m25p80: Introduce Die Erase command...
ERROR: spaces required around that '-' (ctx:VxV)
#63: FILE: hw/block/m25p80.c:522:
+            offset = offset & (~(len-1));
                                     ^

WARNING: line over 80 characters
#65: FILE: hw/block/m25p80.c:524:
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by"

WARNING: line over 80 characters
#94: FILE: hw/block/m25p80.c:734:
+            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0);

ERROR: space prohibited after that open parenthesis '('
#94: FILE: hw/block/m25p80.c:734:
+            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0);

ERROR: braces {} are necessary for all arms of this statement
#99: FILE: hw/block/m25p80.c:739:
+            if (s->pi->id[4] & BIT(6))
[...]
+            else
[...]

total: 3 errors, 2 warnings, 93 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op

[Edgar E. Iglesias]

On Fri, Dec 16, 2016 at 02:27:41PM +0100, marcin.krzeminski@nokia.com wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> Some flash chips has additional page program opcode that
> takes only 4 byte address. This commit adds support
> for such command in Qemu.
> 
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/block/m25p80.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index d29ff4c..2bc7028 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -325,6 +325,7 @@ typedef enum {
>      PP4_4 = 0x3e,
>      DPP = 0xa2,
>      QPP = 0x32,
> +    QPP_4 = 0x34,
>  
>      ERASE_4K = 0x20,
>      ERASE4_4K = 0x21,
> @@ -573,6 +574,7 @@ static inline int get_addr_length(Flash *s)
>     switch (s->cmd_in_progress) {
>     case PP4:
>     case PP4_4:
> +   case QPP_4:
>     case READ4:
>     case QIOR4:
>     case ERASE4_4K:
> @@ -606,6 +608,7 @@ static void complete_collecting_data(Flash *s)
>      switch (s->cmd_in_progress) {
>      case DPP:
>      case QPP:
> +    case QPP_4:
>      case PP:
>      case PP4:
>      case PP4_4:
> @@ -873,6 +876,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case READ4:
>      case DPP:
>      case QPP:
> +    case QPP_4:
>      case PP:
>      case PP4:
>      case PP4_4:
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command

[Edgar E. Iglesias]

On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> Big flash chips (like mt25qu01g) are consisted from dies.
> Because of that some manufactures remove support for Chip
> Erase giving Die Erase command instead.To avoid unnecessary
> code complication, support for chip erase for mt25qu01g
> is not removed.
> 
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 2bc7028..0bc1fbf 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = {
>      { 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) },
> -    { INFO("mt25ql01g",   0x20ba21,      0,  64 << 10, 2048, ER_4K) },
> -    { INFO("mt25qu01g",   0x20bb21,      0,  64 << 10, 2048, ER_4K) },
> +    { INFO("mt25ql01g",   0x20ba21, 0x1040,  64 << 10, 2048, ER_4K) },
> +    { INFO("mt25qu01g",   0x20bb21, 0x1040,  64 << 10, 2048, ER_4K) },
>  
>      /* Spansion -- single (large) sector size only, at least
>       * for the chips listed here (without boot sectors).
> @@ -358,6 +358,8 @@ typedef enum {
>  
>      REVCR = 0x65,
>      WEVCR = 0x61,
> +
> +    DIE_ERASE = 0xC4,
>  } FlashCMD;
>  
>  typedef enum {
> @@ -411,6 +413,7 @@ typedef struct Flash {
>      bool reset_enable;
>      bool quad_enable;
>      uint8_t ear;
> +    uint32_t die_cnt;
>  
>      int64_t dirty_page;
>  
> @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
>  
>  static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>  {
> -    uint32_t len;
> +    uint32_t len = 0;

Do you really need this?

>      uint8_t capa_to_assert = 0;
>  
>      switch (cmd) {
> @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>      case BULK_ERASE:
>          len = s->size;
>          break;
> +    case DIE_ERASE:
> +        if (s->die_cnt) {
> +            len = s->size / s->die_cnt;
> +            offset = offset & (~(len-1));
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by"
> +                          " device\n", len);
> +            return;
> +        }
> +        break;
>      default:
>          abort();
>      }
> @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s)
>      case ERASE4_32K:
>      case ERASE_SECTOR:
>      case ERASE4_SECTOR:
> +    case DIE_ERASE:
>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>          break;
>      case WRSR:
> @@ -684,6 +698,7 @@ static void reset_memory(Flash *s)
>      s->write_enable = false;
>      s->reset_enable = false;
>      s->quad_enable = false;
> +    s->die_cnt = 0;
>  
>      switch (get_man(s)) {
>      case MAN_NUMONYX:
> @@ -716,7 +731,15 @@ static void reset_memory(Flash *s)
>              s->four_bytes_address_mode = true;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) {
> -            s->ear = s->size / MAX_3BYTES_SIZE - 1;
> +            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0);
> +        }
> +        /* 1GiB devices */
> +        if (s->pi->id[2] == 0x21) {
> +            /* MT25Q00 has 2 dies N25Q00 has 4 */
> +            if (s->pi->id[4] & BIT(6))
> +                s->die_cnt = 2;
> +            else
> +                s->die_cnt = 4;

Decoding of die_cnt should probably be done in realize().

>          }
>          break;
>      case MAN_MACRONIX:
> @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case PP:
>      case PP4:
>      case PP4_4:
> +    case DIE_ERASE:
>          s->needed_bytes = get_addr_length(s);
>          s->pos = 0;
>          s->len = 0;
> @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT8(spansion_cr2nv, Flash),
>          VMSTATE_UINT8(spansion_cr3nv, Flash),
>          VMSTATE_UINT8(spansion_cr4nv, Flash),
> +        VMSTATE_UINT32(die_cnt, Flash),

die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be part of VMSTATE.

>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command

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

> -----Original Message-----
> From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com]
> Sent: Friday, December 16, 2016 5:36 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw-
> patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org
> Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase
> command
> 
> On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com
> wrote:
> > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >
> > Big flash chips (like mt25qu01g) are consisted from dies.
> > Because of that some manufactures remove support for Chip Erase giving
> > Die Erase command instead.To avoid unnecessary code complication,
> > support for chip erase for mt25qu01g is not removed.
> >
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > ---
> >  hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> > 2bc7028..0bc1fbf 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = {
> >      { 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) },
> > -    { INFO("mt25ql01g",   0x20ba21,      0,  64 << 10, 2048, ER_4K) },
> > -    { INFO("mt25qu01g",   0x20bb21,      0,  64 << 10, 2048, ER_4K) },
> > +    { INFO("mt25ql01g",   0x20ba21, 0x1040,  64 << 10, 2048, ER_4K) },
> > +    { INFO("mt25qu01g",   0x20bb21, 0x1040,  64 << 10, 2048, ER_4K) },
> >
> >      /* Spansion -- single (large) sector size only, at least
> >       * for the chips listed here (without boot sectors).
> > @@ -358,6 +358,8 @@ typedef enum {
> >
> >      REVCR = 0x65,
> >      WEVCR = 0x61,
> > +
> > +    DIE_ERASE = 0xC4,
> >  } FlashCMD;
> >
> >  typedef enum {
> > @@ -411,6 +413,7 @@ typedef struct Flash {
> >      bool reset_enable;
> >      bool quad_enable;
> >      uint8_t ear;
> > +    uint32_t die_cnt;
> >
> >      int64_t dirty_page;
> >
> > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s,
> > int64_t off, int64_t len)
> >
> >  static void flash_erase(Flash *s, int offset, FlashCMD cmd)  {
> > -    uint32_t len;
> > +    uint32_t len = 0;
> 
> Do you really need this?

Compilation warning.
> 
> >      uint8_t capa_to_assert = 0;
> >
> >      switch (cmd) {
> > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >      case BULK_ERASE:
> >          len = s->size;
> >          break;
> > +    case DIE_ERASE:
> > +        if (s->die_cnt) {
> > +            len = s->size / s->die_cnt;
> > +            offset = offset & (~(len-1));
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not
> supported by"
> > +                          " device\n", len);
> > +            return;
> > +        }
> > +        break;
> >      default:
> >          abort();
> >      }
> > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s)
> >      case ERASE4_32K:
> >      case ERASE_SECTOR:
> >      case ERASE4_SECTOR:
> > +    case DIE_ERASE:
> >          flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >          break;
> >      case WRSR:
> > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s)
> >      s->write_enable = false;
> >      s->reset_enable = false;
> >      s->quad_enable = false;
> > +    s->die_cnt = 0;
> >
> >      switch (get_man(s)) {
> >      case MAN_NUMONYX:
> > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s)
> >              s->four_bytes_address_mode = true;
> >          }
> >          if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) {
> > -            s->ear = s->size / MAX_3BYTES_SIZE - 1;
> > +            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE
> - 1 : 0);
> > +        }
> > +        /* 1GiB devices */
> > +        if (s->pi->id[2] == 0x21) {
> > +            /* MT25Q00 has 2 dies N25Q00 has 4 */
> > +            if (s->pi->id[4] & BIT(6))
> > +                s->die_cnt = 2;
> > +            else
> > +                s->die_cnt = 4;
> 
> Decoding of die_cnt should probably be done in realize().
> 
> >          }
> >          break;
> >      case MAN_MACRONIX:
> > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t
> value)
> >      case PP:
> >      case PP4:
> >      case PP4_4:
> > +    case DIE_ERASE:
> >          s->needed_bytes = get_addr_length(s);
> >          s->pos = 0;
> >          s->len = 0;
> > @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80
> = {
> >          VMSTATE_UINT8(spansion_cr2nv, Flash),
> >          VMSTATE_UINT8(spansion_cr3nv, Flash),
> >          VMSTATE_UINT8(spansion_cr4nv, Flash),
> > +        VMSTATE_UINT32(die_cnt, Flash),
> 
> die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be
> part of VMSTATE.

Yes, you are right. Even more, number of a dies is fixed attribute of chip, so it could
Be a part of FlashPartInfo. I'll up v2.

Thanks,
Marcin

> 
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > --
> > 2.7.4
> >
> >

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command

[Edgar E. Iglesias]

On Mon, Dec 19, 2016 at 06:21:13AM +0000, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
> > -----Original Message-----
> > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com]
> > Sent: Friday, December 16, 2016 5:36 PM
> > To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> > <marcin.krzeminski@nokia.com>
> > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw-
> > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org
> > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase
> > command
> > 
> > On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com
> > wrote:
> > > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > >
> > > Big flash chips (like mt25qu01g) are consisted from dies.
> > > Because of that some manufactures remove support for Chip Erase giving
> > > Die Erase command instead.To avoid unnecessary code complication,
> > > support for chip erase for mt25qu01g is not removed.
> > >
> > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > > ---
> > >  hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++----
> > >  1 file changed, 29 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> > > 2bc7028..0bc1fbf 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = {
> > >      { 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) },
> > > -    { INFO("mt25ql01g",   0x20ba21,      0,  64 << 10, 2048, ER_4K) },
> > > -    { INFO("mt25qu01g",   0x20bb21,      0,  64 << 10, 2048, ER_4K) },
> > > +    { INFO("mt25ql01g",   0x20ba21, 0x1040,  64 << 10, 2048, ER_4K) },
> > > +    { INFO("mt25qu01g",   0x20bb21, 0x1040,  64 << 10, 2048, ER_4K) },
> > >
> > >      /* Spansion -- single (large) sector size only, at least
> > >       * for the chips listed here (without boot sectors).
> > > @@ -358,6 +358,8 @@ typedef enum {
> > >
> > >      REVCR = 0x65,
> > >      WEVCR = 0x61,
> > > +
> > > +    DIE_ERASE = 0xC4,
> > >  } FlashCMD;
> > >
> > >  typedef enum {
> > > @@ -411,6 +413,7 @@ typedef struct Flash {
> > >      bool reset_enable;
> > >      bool quad_enable;
> > >      uint8_t ear;
> > > +    uint32_t die_cnt;
> > >
> > >      int64_t dirty_page;
> > >
> > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s,
> > > int64_t off, int64_t len)
> > >
> > >  static void flash_erase(Flash *s, int offset, FlashCMD cmd)  {
> > > -    uint32_t len;
> > > +    uint32_t len = 0;
> > 
> > Do you really need this?
> 
> Compilation warning.

Of course, because you are logging len in a code path that
doesn't use the variable.

Let me be more clear below:

> > 
> > >      uint8_t capa_to_assert = 0;
> > >
> > >      switch (cmd) {
> > > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset,
> > FlashCMD cmd)
> > >      case BULK_ERASE:
> > >          len = s->size;
> > >          break;
> > > +    case DIE_ERASE:
> > > +        if (s->die_cnt) {
> > > +            len = s->size / s->die_cnt;
> > > +            offset = offset & (~(len-1));
> > > +        } else {
> > > +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not
> > supported by"
> > > +                          " device\n", len);

Don't log len here...



> > > +            return;
> > > +        }
> > > +        break;
> > >      default:
> > >          abort();
> > >      }
> > > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s)
> > >      case ERASE4_32K:
> > >      case ERASE_SECTOR:
> > >      case ERASE4_SECTOR:
> > > +    case DIE_ERASE:
> > >          flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > >          break;
> > >      case WRSR:
> > > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s)
> > >      s->write_enable = false;
> > >      s->reset_enable = false;
> > >      s->quad_enable = false;
> > > +    s->die_cnt = 0;
> > >
> > >      switch (get_man(s)) {
> > >      case MAN_NUMONYX:
> > > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s)
> > >              s->four_bytes_address_mode = true;
> > >          }
> > >          if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) {
> > > -            s->ear = s->size / MAX_3BYTES_SIZE - 1;
> > > +            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE
> > - 1 : 0);
> > > +        }
> > > +        /* 1GiB devices */
> > > +        if (s->pi->id[2] == 0x21) {
> > > +            /* MT25Q00 has 2 dies N25Q00 has 4 */
> > > +            if (s->pi->id[4] & BIT(6))
> > > +                s->die_cnt = 2;
> > > +            else
> > > +                s->die_cnt = 4;
> > 
> > Decoding of die_cnt should probably be done in realize().
> > 
> > >          }
> > >          break;
> > >      case MAN_MACRONIX:
> > > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t
> > value)
> > >      case PP:
> > >      case PP4:
> > >      case PP4_4:
> > > +    case DIE_ERASE:
> > >          s->needed_bytes = get_addr_length(s);
> > >          s->pos = 0;
> > >          s->len = 0;
> > > @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80
> > = {
> > >          VMSTATE_UINT8(spansion_cr2nv, Flash),
> > >          VMSTATE_UINT8(spansion_cr3nv, Flash),
> > >          VMSTATE_UINT8(spansion_cr4nv, Flash),
> > > +        VMSTATE_UINT32(die_cnt, Flash),
> > 
> > die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be
> > part of VMSTATE.
> 
> Yes, you are right. Even more, number of a dies is fixed attribute of chip, so it could
> Be a part of FlashPartInfo. I'll up v2.
> 
> Thanks,
> Marcin
> 
> > 
> > >          VMSTATE_END_OF_LIST()
> > >      }
> > >  };
> > > --
> > > 2.7.4
> > >
> > >

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command

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

> -----Original Message-----
> From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com]
> Sent: Monday, December 19, 2016 8:28 AM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw-
> patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org
> Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase
> command
> 
> On Mon, Dec 19, 2016 at 06:21:13AM +0000, Krzeminski, Marcin (Nokia -
> PL/Wroclaw) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com]
> > > Sent: Friday, December 16, 2016 5:36 PM
> > > To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> > > <marcin.krzeminski@nokia.com>
> > > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw-
> > > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org
> > > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die
> > > Erase command
> > >
> > > On Fri, Dec 16, 2016 at 02:27:42PM +0100,
> > > marcin.krzeminski@nokia.com
> > > wrote:
> > > > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > > >
> > > > Big flash chips (like mt25qu01g) are consisted from dies.
> > > > Because of that some manufactures remove support for Chip Erase
> > > > giving Die Erase command instead.To avoid unnecessary code
> > > > complication, support for chip erase for mt25qu01g is not removed.
> > > >
> > > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > > > ---
> > > >  hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++----
> > > >  1 file changed, 29 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> > > > 2bc7028..0bc1fbf 100644
> > > > --- a/hw/block/m25p80.c
> > > > +++ b/hw/block/m25p80.c
> > > > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = {
> > > >      { 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) },
> > > > -    { INFO("mt25ql01g",   0x20ba21,      0,  64 << 10, 2048, ER_4K) },
> > > > -    { INFO("mt25qu01g",   0x20bb21,      0,  64 << 10, 2048, ER_4K) },
> > > > +    { INFO("mt25ql01g",   0x20ba21, 0x1040,  64 << 10, 2048, ER_4K) },
> > > > +    { INFO("mt25qu01g",   0x20bb21, 0x1040,  64 << 10, 2048, ER_4K) },
> > > >
> > > >      /* Spansion -- single (large) sector size only, at least
> > > >       * for the chips listed here (without boot sectors).
> > > > @@ -358,6 +358,8 @@ typedef enum {
> > > >
> > > >      REVCR = 0x65,
> > > >      WEVCR = 0x61,
> > > > +
> > > > +    DIE_ERASE = 0xC4,
> > > >  } FlashCMD;
> > > >
> > > >  typedef enum {
> > > > @@ -411,6 +413,7 @@ typedef struct Flash {
> > > >      bool reset_enable;
> > > >      bool quad_enable;
> > > >      uint8_t ear;
> > > > +    uint32_t die_cnt;
> > > >
> > > >      int64_t dirty_page;
> > > >
> > > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s,
> > > > int64_t off, int64_t len)
> > > >
> > > >  static void flash_erase(Flash *s, int offset, FlashCMD cmd)  {
> > > > -    uint32_t len;
> > > > +    uint32_t len = 0;
> > >
> > > Do you really need this?
> >
> > Compilation warning.
> 
> Of course, because you are logging len in a code path that doesn't use the
> variable.
> 
> Let me be more clear below:
> 
> > >
> > > >      uint8_t capa_to_assert = 0;
> > > >
> > > >      switch (cmd) {
> > > > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset,
> > > FlashCMD cmd)
> > > >      case BULK_ERASE:
> > > >          len = s->size;
> > > >          break;
> > > > +    case DIE_ERASE:
> > > > +        if (s->die_cnt) {
> > > > +            len = s->size / s->die_cnt;
> > > > +            offset = offset & (~(len-1));
> > > > +        } else {
> > > > +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase
> > > > + not
> > > supported by"
> > > > +                          " device\n", len);
> 
> Don't log len here...

Sure, I did not get your intention :)

Thanks,
Marcin
> 
> 
> 
> > > > +            return;
> > > > +        }
> > > > +        break;
> > > >      default:
> > > >          abort();
> > > >      }
> > > > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s)
> > > >      case ERASE4_32K:
> > > >      case ERASE_SECTOR:
> > > >      case ERASE4_SECTOR:
> > > > +    case DIE_ERASE:
> > > >          flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > > >          break;
> > > >      case WRSR:
> > > > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s)
> > > >      s->write_enable = false;
> > > >      s->reset_enable = false;
> > > >      s->quad_enable = false;
> > > > +    s->die_cnt = 0;
> > > >
> > > >      switch (get_man(s)) {
> > > >      case MAN_NUMONYX:
> > > > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s)
> > > >              s->four_bytes_address_mode = true;
> > > >          }
> > > >          if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) {
> > > > -            s->ear = s->size / MAX_3BYTES_SIZE - 1;
> > > > +            s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size /
> > > > + MAX_3BYTES_SIZE
> > > - 1 : 0);
> > > > +        }
> > > > +        /* 1GiB devices */
> > > > +        if (s->pi->id[2] == 0x21) {
> > > > +            /* MT25Q00 has 2 dies N25Q00 has 4 */
> > > > +            if (s->pi->id[4] & BIT(6))
> > > > +                s->die_cnt = 2;
> > > > +            else
> > > > +                s->die_cnt = 4;
> > >
> > > Decoding of die_cnt should probably be done in realize().
> > >
> > > >          }
> > > >          break;
> > > >      case MAN_MACRONIX:
> > > > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t
> > > value)
> > > >      case PP:
> > > >      case PP4:
> > > >      case PP4_4:
> > > > +    case DIE_ERASE:
> > > >          s->needed_bytes = get_addr_length(s);
> > > >          s->pos = 0;
> > > >          s->len = 0;
> > > > @@ -1217,6 +1241,7 @@ static const VMStateDescription
> > > > vmstate_m25p80
> > > = {
> > > >          VMSTATE_UINT8(spansion_cr2nv, Flash),
> > > >          VMSTATE_UINT8(spansion_cr3nv, Flash),
> > > >          VMSTATE_UINT8(spansion_cr4nv, Flash),
> > > > +        VMSTATE_UINT32(die_cnt, Flash),
> > >
> > > die_cnt is fixed per instance. Since it doesn't change, it doesn't
> > > need to be part of VMSTATE.
> >
> > Yes, you are right. Even more, number of a dies is fixed attribute of
> > chip, so it could Be a part of FlashPartInfo. I'll up v2.
> >
> > Thanks,
> > Marcin
> >
> > >
> > > >          VMSTATE_END_OF_LIST()
> > > >      }
> > > >  };
> > > > --
> > > > 2.7.4
> > > >
> > > >