[PATCH 0/2] hw/block/m25p80: Fix Numonyx flash dummy cycle register behavior

[PATCH 0/2] hw/block/m25p80: Fix Numonyx flash dummy cycle register behavior

[Joe Komlodi]

Hi all,

This series addresses a couple issues with dummy cycle counts with Numonyx
flashes.

The first patch fixes the behavior of the dummy cycle register so it's closer to
how hardware behaves.
As a consequence, it also corrects the amount of dummy cycles QIOR and QIOR4
commands need by default.

The second patch changes the default value of the nvcfg register so it
matches what would be in hardware from the factory.

Thanks!
Joe

Joe Komlodi (2):
  hw/block/m25p80: Fix Numonyx dummy cycle register behavior
  hw/block/m25p80: Fix nonvolatile-cfg property default value

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

-- 
2.7.4

[PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register behavior

[Joe Komlodi]

Numonyx chips determine the number of cycles to wait based on bits 7:4 in the
volatile configuration register.

However, if these bits are 0x0 or 0xF, the number of dummy cycles to wait is
10 on a QIOR or QIOR4 command, or 8 on any other currently supported
fast read command. [1]

[1] http://www.micron.com/-/media/client/global/documents/products/
data-sheet/nor-flash/serial-nor/n25q/n25q_512mb_1_8v_65nm.pdf

Page 22 note 2, and page 30 notes 5 and 10.

Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
---
 hw/block/m25p80.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f..43830c9 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -820,6 +820,26 @@ static void reset_memory(Flash *s)
     trace_m25p80_reset_done(s);
 }
 
+static uint8_t numonyx_fast_read_num_dummies(Flash *s)
+{
+    uint8_t cycle_count;
+    uint8_t num_dummies;
+    assert(get_man(s) == MAN_NUMONYX);
+
+    cycle_count = extract32(s->volatile_cfg, 4, 4);
+    if (cycle_count == 0x0 || cycle_count == 0x0F) {
+        if (s->cmd_in_progress == QIOR || s->cmd_in_progress == QIOR4) {
+            num_dummies = 10;
+        } else {
+            num_dummies = 8;
+        }
+    } else {
+        num_dummies = cycle_count;
+    }
+
+    return num_dummies;
+}
+
 static void decode_fast_read_cmd(Flash *s)
 {
     s->needed_bytes = get_addr_length(s);
@@ -829,7 +849,7 @@ static void decode_fast_read_cmd(Flash *s)
         s->needed_bytes += 8;
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_fast_read_num_dummies(s);
         break;
     case MAN_MACRONIX:
         if (extract32(s->volatile_cfg, 6, 2) == 1) {
@@ -868,7 +888,7 @@ static void decode_dio_read_cmd(Flash *s)
                                     );
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_fast_read_num_dummies(s);
         break;
     case MAN_MACRONIX:
         switch (extract32(s->volatile_cfg, 6, 2)) {
@@ -908,7 +928,7 @@ static void decode_qio_read_cmd(Flash *s)
                                     );
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_fast_read_num_dummies(s);
         break;
     case MAN_MACRONIX:
         switch (extract32(s->volatile_cfg, 6, 2)) {
-- 
2.7.4

[PATCH 2/2] hw/block/m25p80: Fix nonvolatile-cfg property default value

[Joe Komlodi]

The nvcfg registers are all 1s, unless previously modified.

Signed-off-by: Joe Komlodi <komlodi@xilinx.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 43830c9..69c88d4 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1334,7 +1334,7 @@ static int m25p80_pre_save(void *opaque)
 
 static Property m25p80_properties[] = {
     /* This is default value for Micron flash */
-    DEFINE_PROP_UINT32("nonvolatile-cfg", Flash, nonvolatile_cfg, 0x8FFF),
+    DEFINE_PROP_UINT32("nonvolatile-cfg", Flash, nonvolatile_cfg, 0xFFFF),
     DEFINE_PROP_UINT8("spansion-cr1nv", Flash, spansion_cr1nv, 0x0),
     DEFINE_PROP_UINT8("spansion-cr2nv", Flash, spansion_cr2nv, 0x8),
     DEFINE_PROP_UINT8("spansion-cr3nv", Flash, spansion_cr3nv, 0x2),
-- 
2.7.4

Re: [PATCH 0/2] hw/block/m25p80: Fix Numonyx flash dummy cycle register behavior

[no-reply]

Patchew URL: https://patchew.org/QEMU/1601425716-204629-1-git-send-email-komlodi@xilinx.com/



Hi,

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

N/A. Internal error while reading log file



The full log is available at
http://patchew.org/logs/1601425716-204629-1-git-send-email-komlodi@xilinx.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register behavior

[Francisco Iglesias]

Hi Joe,

On Tue, Sep 29, 2020 at 05:28:35PM -0700, Joe Komlodi wrote:
> Numonyx chips determine the number of cycles to wait based on bits 7:4 in the
> volatile configuration register.
> 
> However, if these bits are 0x0 or 0xF, the number of dummy cycles to wait is
> 10 on a QIOR or QIOR4 command, or 8 on any other currently supported
> fast read command. [1]
> 
> [1] http://www.micron.com/-/media/client/global/documents/products/
> data-sheet/nor-flash/serial-nor/n25q/n25q_512mb_1_8v_65nm.pdf
> 
> Page 22 note 2, and page 30 notes 5 and 10.
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..43830c9 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -820,6 +820,26 @@ static void reset_memory(Flash *s)
>      trace_m25p80_reset_done(s);
>  }
>  
> +static uint8_t numonyx_fast_read_num_dummies(Flash *s)

Should we rename the function to something like
'numonyx_extract_cfg_num_dummies' (since it is not only used inside
'decode_fast_read_cmd')?

> +{
> +    uint8_t cycle_count;
> +    uint8_t num_dummies;
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    cycle_count = extract32(s->volatile_cfg, 4, 4);
> +    if (cycle_count == 0x0 || cycle_count == 0x0F) {
> +        if (s->cmd_in_progress == QIOR || s->cmd_in_progress == QIOR4) {

QOR and QOR4 also has 10 dummy cycles on default so we will have to check for
those aswell, perhaps something similar like below migth work:  

uint8_t n_dummies = extract32(s->volatile_cfg, 4, 4);

if (!n_dummies || n_dummies == 0xF) {
    switch(s->cmd_in_progress){
    case QOR:
    case QOR4
    case QIOR:
    case QIOR4:
        n_dummies = 10;
        break;
    default:
        n_dummies = 8;
        break;
    }
}

return n_dummies;

Best regards,
Francisco Iglesias

> +            num_dummies = 10;
> +        } else {
> +            num_dummies = 8;
> +        }
> +    } else {
> +        num_dummies = cycle_count;
> +    }
> +
> +    return num_dummies;
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)
>  {
>      s->needed_bytes = get_addr_length(s);
> @@ -829,7 +849,7 @@ static void decode_fast_read_cmd(Flash *s)
>          s->needed_bytes += 8;
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          if (extract32(s->volatile_cfg, 6, 2) == 1) {
> @@ -868,7 +888,7 @@ static void decode_dio_read_cmd(Flash *s)
>                                      );
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> @@ -908,7 +928,7 @@ static void decode_qio_read_cmd(Flash *s)
>                                      );
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> -- 
> 2.7.4
> 

RE: [PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register behavior

[Joe Komlodi]

Hi Francisco,

Comments marked with [Joe]

-----Original Message-----
From: Francisco Iglesias <francisco.iglesias@xilinx.com> 
Sent: Tuesday, October 20, 2020 6:50 AM
To: Joe Komlodi <komlodi@xilinx.com>
Cc: qemu-devel@nongnu.org; alistair@alistair23.me; kwolf@redhat.com; mreitz@redhat.com; qemu-block@nongnu.org
Subject: Re: [PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register behavior

Hi Joe,

On Tue, Sep 29, 2020 at 05:28:35PM -0700, Joe Komlodi wrote:
> Numonyx chips determine the number of cycles to wait based on bits 7:4 
> in the volatile configuration register.
> 
> However, if these bits are 0x0 or 0xF, the number of dummy cycles to 
> wait is
> 10 on a QIOR or QIOR4 command, or 8 on any other currently supported 
> fast read command. [1]
> 
> [1] http://www.micron.com/-/media/client/global/documents/products/
> data-sheet/nor-flash/serial-nor/n25q/n25q_512mb_1_8v_65nm.pdf
> 
> Page 22 note 2, and page 30 notes 5 and 10.
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 
> 483925f..43830c9 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -820,6 +820,26 @@ static void reset_memory(Flash *s)
>      trace_m25p80_reset_done(s);
>  }
>  
> +static uint8_t numonyx_fast_read_num_dummies(Flash *s)

Should we rename the function to something like 'numonyx_extract_cfg_num_dummies' (since it is not only used inside 'decode_fast_read_cmd')?

[Joe] Yeah, that name makes more sense.

> +{
> +    uint8_t cycle_count;
> +    uint8_t num_dummies;
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    cycle_count = extract32(s->volatile_cfg, 4, 4);
> +    if (cycle_count == 0x0 || cycle_count == 0x0F) {
> +        if (s->cmd_in_progress == QIOR || s->cmd_in_progress == 
> + QIOR4) {

QOR and QOR4 also has 10 dummy cycles on default so we will have to check for those aswell, perhaps something similar like below migth work:  

uint8_t n_dummies = extract32(s->volatile_cfg, 4, 4);

if (!n_dummies || n_dummies == 0xF) {
    switch(s->cmd_in_progress){
    case QOR:
    case QOR4
    case QIOR:
    case QIOR4:
        n_dummies = 10;
        break;
    default:
        n_dummies = 8;
        break;
    }
}

return n_dummies;

[Joe] As talked about offline, the datasheet in the commit message just has confusing wording.
8 dummies for QOR seems to be correct, and I'll update the datasheet in the commit message with one that's more clear.

Thanks!
Joe

Best regards,
Francisco Iglesias

> +            num_dummies = 10;
> +        } else {
> +            num_dummies = 8;
> +        }
> +    } else {
> +        num_dummies = cycle_count;
> +    }
> +
> +    return num_dummies;
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)  {
>      s->needed_bytes = get_addr_length(s); @@ -829,7 +849,7 @@ static 
> void decode_fast_read_cmd(Flash *s)
>          s->needed_bytes += 8;
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          if (extract32(s->volatile_cfg, 6, 2) == 1) { @@ -868,7 +888,7 
> @@ static void decode_dio_read_cmd(Flash *s)
>                                      );
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) { @@ -908,7 +928,7 
> @@ static void decode_qio_read_cmd(Flash *s)
>                                      );
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> --
> 2.7.4
>