[PATCH v4 0/4] hw/block/m25p80: Numonyx: Fix dummy cycles and check for SPI mode on cmds

[PATCH v4 0/4] hw/block/m25p80: Numonyx: Fix dummy cycles and check for SPI mode on cmds

[Joe Komlodi]

Changelog:
v3 -> v4
 - 1/4: Patch changed to change names of register fields to be more accurate.
 - 1/4: Revert polarity change from v3.
 - 2/4: Added, fixes polarity of VCFG XIP mode when copied from NVCFG.
 - 3/4: Removed check_cmd_mode function, each command check is done in decode_new_cmd instead.
 - 3/4: Add guest error print if JEDEC read is executed in QIO or DIO mode.
 - 3/4: Don't check PP and PP4, they work regardless of mode. PP4_4 is left as is.
 - 3/4: Simplify get_mode function.
 - 4/4: Simplify extract_cfg_num_dummies function.
 - 4/4: Use switch statement instead of table for cycle retrieving.

v2 -> v3
 - 1/3: Added, Fixes NVCFG polarity for DIO/QIO.
 - 2/3: Added, Checks if we can execute the current command in standard/DIO/QIO mode.
 - 3/3: Was 1/1 in v2.  Added cycle counts for DIO/QIO mode.

v1 -> v2
 - 1/2: Change function name to be more accurate
 - 2/2: Dropped

Hi all,

The series fixes the behavior of the dummy cycle register for Numonyx flashes so
it's closer to how hardware behaves.
It also checks if a command can be executed in the current SPI mode
(standard, DIO, or QIO) before extracting dummy cycles for the command.

On hardware, the dummy cycles for fast read commands are set to a specific value
(8 or 10) if the register is all 0s or 1s.
If the register value isn't all 0s or 1s, then the flash expects the amount of
cycles sent to be equal to the count in the register.

Thanks!
Joe

Joe Komlodi (4):
  hw/block/m25p80: Make Numonyx config field names more accurate
  hw/block/m25p80: Fix when VCFG XIP bit is set for Numonyx
  hw/block/m25p80: Check SPI mode before running some Numonyx commands
  hw/block/m25p80: Fix Numonyx fast read dummy cycle count

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

-- 
2.7.4

[PATCH v4 1/4] hw/block/m25p80: Make Numonyx config field names more accurate

[Joe Komlodi]

The previous naming of the configuration registers made it sound like that if
the bits were set the settings would be enabled, while the opposite is true.

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f..452d252 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -136,7 +136,7 @@ typedef struct FlashPartInfo {
 #define VCFG_WRAP_SEQUENTIAL 0x2
 #define NVCFG_XIP_MODE_DISABLED (7 << 9)
 #define NVCFG_XIP_MODE_MASK (7 << 9)
-#define VCFG_XIP_MODE_ENABLED (1 << 3)
+#define VCFG_XIP_MODE_DISABLED (1 << 3)
 #define CFG_DUMMY_CLK_LEN 4
 #define NVCFG_DUMMY_CLK_POS 12
 #define VCFG_DUMMY_CLK_POS 4
@@ -144,9 +144,9 @@ typedef struct FlashPartInfo {
 #define EVCFG_VPP_ACCELERATOR (1 << 3)
 #define EVCFG_RESET_HOLD_ENABLED (1 << 4)
 #define NVCFG_DUAL_IO_MASK (1 << 2)
-#define EVCFG_DUAL_IO_ENABLED (1 << 6)
+#define EVCFG_DUAL_IO_DISABLED (1 << 6)
 #define NVCFG_QUAD_IO_MASK (1 << 3)
-#define EVCFG_QUAD_IO_ENABLED (1 << 7)
+#define EVCFG_QUAD_IO_DISABLED (1 << 7)
 #define NVCFG_4BYTE_ADDR_MASK (1 << 0)
 #define NVCFG_LOWER_SEGMENT_MASK (1 << 1)
 
@@ -769,7 +769,7 @@ static void reset_memory(Flash *s)
         s->volatile_cfg |= VCFG_WRAP_SEQUENTIAL;
         if ((s->nonvolatile_cfg & NVCFG_XIP_MODE_MASK)
                                 != NVCFG_XIP_MODE_DISABLED) {
-            s->volatile_cfg |= VCFG_XIP_MODE_ENABLED;
+            s->volatile_cfg |= VCFG_XIP_MODE_DISABLED;
         }
         s->volatile_cfg |= deposit32(s->volatile_cfg,
                             VCFG_DUMMY_CLK_POS,
@@ -784,10 +784,10 @@ static void reset_memory(Flash *s)
         s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
         s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
         if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
-            s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
+            s->enh_volatile_cfg |= EVCFG_DUAL_IO_DISABLED;
         }
         if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
-            s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
+            s->enh_volatile_cfg |= EVCFG_QUAD_IO_DISABLED;
         }
         if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
             s->four_bytes_address_mode = true;
-- 
2.7.4

Re: [PATCH v4 1/4] hw/block/m25p80: Make Numonyx config field names more accurate

[Francisco Iglesias]

On Thu, Nov 12, 2020 at 07:10:52PM -0800, Joe Komlodi wrote:
> The previous naming of the configuration registers made it sound like that if
> the bits were set the settings would be enabled, while the opposite is true.
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@xilinx.com>

> ---
>  hw/block/m25p80.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..452d252 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -136,7 +136,7 @@ typedef struct FlashPartInfo {
>  #define VCFG_WRAP_SEQUENTIAL 0x2
>  #define NVCFG_XIP_MODE_DISABLED (7 << 9)
>  #define NVCFG_XIP_MODE_MASK (7 << 9)
> -#define VCFG_XIP_MODE_ENABLED (1 << 3)
> +#define VCFG_XIP_MODE_DISABLED (1 << 3)
>  #define CFG_DUMMY_CLK_LEN 4
>  #define NVCFG_DUMMY_CLK_POS 12
>  #define VCFG_DUMMY_CLK_POS 4
> @@ -144,9 +144,9 @@ typedef struct FlashPartInfo {
>  #define EVCFG_VPP_ACCELERATOR (1 << 3)
>  #define EVCFG_RESET_HOLD_ENABLED (1 << 4)
>  #define NVCFG_DUAL_IO_MASK (1 << 2)
> -#define EVCFG_DUAL_IO_ENABLED (1 << 6)
> +#define EVCFG_DUAL_IO_DISABLED (1 << 6)
>  #define NVCFG_QUAD_IO_MASK (1 << 3)
> -#define EVCFG_QUAD_IO_ENABLED (1 << 7)
> +#define EVCFG_QUAD_IO_DISABLED (1 << 7)
>  #define NVCFG_4BYTE_ADDR_MASK (1 << 0)
>  #define NVCFG_LOWER_SEGMENT_MASK (1 << 1)
>  
> @@ -769,7 +769,7 @@ static void reset_memory(Flash *s)
>          s->volatile_cfg |= VCFG_WRAP_SEQUENTIAL;
>          if ((s->nonvolatile_cfg & NVCFG_XIP_MODE_MASK)
>                                  != NVCFG_XIP_MODE_DISABLED) {
> -            s->volatile_cfg |= VCFG_XIP_MODE_ENABLED;
> +            s->volatile_cfg |= VCFG_XIP_MODE_DISABLED;
>          }
>          s->volatile_cfg |= deposit32(s->volatile_cfg,
>                              VCFG_DUMMY_CLK_POS,
> @@ -784,10 +784,10 @@ static void reset_memory(Flash *s)
>          s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>          s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
>          if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> -            s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
> +            s->enh_volatile_cfg |= EVCFG_DUAL_IO_DISABLED;
>          }
>          if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> -            s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
> +            s->enh_volatile_cfg |= EVCFG_QUAD_IO_DISABLED;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
>              s->four_bytes_address_mode = true;
> -- 
> 2.7.4
> 

[PATCH v4 2/4] hw/block/m25p80: Fix when VCFG XIP bit is set for Numonyx

[Joe Komlodi]

VCFG XIP is set (disabled) when the NVCFG XIP bits are all set (disabled).

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 452d252..eb6539f 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -768,7 +768,7 @@ static void reset_memory(Flash *s)
         s->volatile_cfg |= VCFG_DUMMY;
         s->volatile_cfg |= VCFG_WRAP_SEQUENTIAL;
         if ((s->nonvolatile_cfg & NVCFG_XIP_MODE_MASK)
-                                != NVCFG_XIP_MODE_DISABLED) {
+                                == NVCFG_XIP_MODE_DISABLED) {
             s->volatile_cfg |= VCFG_XIP_MODE_DISABLED;
         }
         s->volatile_cfg |= deposit32(s->volatile_cfg,
-- 
2.7.4

Re: [PATCH v4 2/4] hw/block/m25p80: Fix when VCFG XIP bit is set for Numonyx

[Francisco Iglesias]

On Thu, Nov 12, 2020 at 07:10:53PM -0800, Joe Komlodi wrote:
> VCFG XIP is set (disabled) when the NVCFG XIP bits are all set (disabled).
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@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 452d252..eb6539f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -768,7 +768,7 @@ static void reset_memory(Flash *s)
>          s->volatile_cfg |= VCFG_DUMMY;
>          s->volatile_cfg |= VCFG_WRAP_SEQUENTIAL;
>          if ((s->nonvolatile_cfg & NVCFG_XIP_MODE_MASK)
> -                                != NVCFG_XIP_MODE_DISABLED) {
> +                                == NVCFG_XIP_MODE_DISABLED) {
>              s->volatile_cfg |= VCFG_XIP_MODE_DISABLED;
>          }
>          s->volatile_cfg |= deposit32(s->volatile_cfg,
> -- 
> 2.7.4
> 

[PATCH v4 3/4] hw/block/m25p80: Check SPI mode before running some Numonyx commands

[Joe Komlodi]

Some Numonyx flash commands cannot be executed in DIO and QIO mode, such as
trying to do DPP or DOR when in QIO mode.

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index eb6539f..2552f2c 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -413,6 +413,12 @@ typedef enum {
     MAN_GENERIC,
 } Manufacturer;
 
+typedef enum {
+    MODE_STD = 0,
+    MODE_DIO = 1,
+    MODE_QIO = 2
+} SPIMode;
+
 #define M25P80_INTERNAL_DATA_BUFFER_SZ 16
 
 struct Flash {
@@ -820,6 +826,17 @@ static void reset_memory(Flash *s)
     trace_m25p80_reset_done(s);
 }
 
+static uint8_t numonyx_get_mode(Flash *s)
+{
+    if (!(s->enh_volatile_cfg & EVCFG_QUAD_IO_DISABLED)) {
+        return MODE_QIO;
+    } else if (!(s->enh_volatile_cfg & EVCFG_DUAL_IO_DISABLED)) {
+        return MODE_DIO;
+    } else {
+        return MODE_STD;
+    }
+}
+
 static void decode_fast_read_cmd(Flash *s)
 {
     s->needed_bytes = get_addr_length(s);
@@ -827,9 +844,11 @@ static void decode_fast_read_cmd(Flash *s)
     /* Dummy cycles - modeled with bytes writes instead of bits */
     case MAN_WINBOND:
         s->needed_bytes += 8;
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
         s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
         if (extract32(s->volatile_cfg, 6, 2) == 1) {
@@ -837,19 +856,21 @@ static void decode_fast_read_cmd(Flash *s)
         } else {
             s->needed_bytes += 8;
         }
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_SPANSION:
         s->needed_bytes += extract32(s->spansion_cr2v,
                                     SPANSION_DUMMY_CLK_POS,
                                     SPANSION_DUMMY_CLK_LEN
                                     );
+        s->state = STATE_COLLECTING_DATA;
         break;
     default:
+        s->state = STATE_COLLECTING_DATA;
         break;
     }
     s->pos = 0;
     s->len = 0;
-    s->state = STATE_COLLECTING_DATA;
 }
 
 static void decode_dio_read_cmd(Flash *s)
@@ -859,6 +880,7 @@ static void decode_dio_read_cmd(Flash *s)
     switch (get_man(s)) {
     case MAN_WINBOND:
         s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_SPANSION:
         s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
@@ -866,9 +888,11 @@ static void decode_dio_read_cmd(Flash *s)
                                     SPANSION_DUMMY_CLK_POS,
                                     SPANSION_DUMMY_CLK_LEN
                                     );
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
         s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
         switch (extract32(s->volatile_cfg, 6, 2)) {
@@ -882,13 +906,14 @@ static void decode_dio_read_cmd(Flash *s)
             s->needed_bytes += 4;
             break;
         }
+        s->state = STATE_COLLECTING_DATA;
         break;
     default:
+        s->state = STATE_COLLECTING_DATA;
         break;
     }
     s->pos = 0;
     s->len = 0;
-    s->state = STATE_COLLECTING_DATA;
 }
 
 static void decode_qio_read_cmd(Flash *s)
@@ -899,6 +924,7 @@ static void decode_qio_read_cmd(Flash *s)
     case MAN_WINBOND:
         s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
         s->needed_bytes += 4;
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_SPANSION:
         s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
@@ -906,9 +932,11 @@ static void decode_qio_read_cmd(Flash *s)
                                     SPANSION_DUMMY_CLK_POS,
                                     SPANSION_DUMMY_CLK_LEN
                                     );
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
         s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
         switch (extract32(s->volatile_cfg, 6, 2)) {
@@ -922,13 +950,14 @@ static void decode_qio_read_cmd(Flash *s)
             s->needed_bytes += 6;
             break;
         }
+        s->state = STATE_COLLECTING_DATA;
         break;
     default:
+        s->state = STATE_COLLECTING_DATA;
         break;
     }
     s->pos = 0;
     s->len = 0;
-    s->state = STATE_COLLECTING_DATA;
 }
 
 static void decode_new_cmd(Flash *s, uint32_t value)
@@ -950,14 +979,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case ERASE4_32K:
     case ERASE_SECTOR:
     case ERASE4_SECTOR:
-    case READ:
-    case READ4:
-    case DPP:
-    case QPP:
-    case QPP_4:
     case PP:
     case PP4:
-    case PP4_4:
     case DIE_ERASE:
     case RDID_90:
     case RDID_AB:
@@ -966,24 +989,85 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         s->len = 0;
         s->state = STATE_COLLECTING_DATA;
         break;
+    case READ:
+    case READ4:
+        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
+            numonyx_get_mode(s) != MODE_QIO)) {
+            s->needed_bytes = get_addr_length(s);
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "DIO or QIO mode\n", s->cmd_in_progress);
+        }
+        break;
+    case DPP:
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
+            s->needed_bytes = get_addr_length(s);
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "QIO mode\n", s->cmd_in_progress);
+        }
+        break;
+    case QPP:
+    case QPP_4:
+    case PP4_4:
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
+            s->needed_bytes = get_addr_length(s);
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "DIO mode\n", s->cmd_in_progress);
+        }
+        break;
 
     case FAST_READ:
     case FAST_READ4:
+        decode_fast_read_cmd(s);
+        break;
     case DOR:
     case DOR4:
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
+            decode_fast_read_cmd(s);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "QIO mode\n", s->cmd_in_progress);
+        }
+        break;
     case QOR:
     case QOR4:
-        decode_fast_read_cmd(s);
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
+            decode_fast_read_cmd(s);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "DIO mode\n", s->cmd_in_progress);
+        }
         break;
 
     case DIOR:
     case DIOR4:
-        decode_dio_read_cmd(s);
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
+            decode_dio_read_cmd(s);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "QIO mode\n", s->cmd_in_progress);
+        }
         break;
 
     case QIOR:
     case QIOR4:
-        decode_qio_read_cmd(s);
+        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
+            decode_qio_read_cmd(s);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "DIO mode\n", s->cmd_in_progress);
+        }
         break;
 
     case WRSR:
@@ -1035,17 +1119,23 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
 
     case JEDEC_READ:
-        trace_m25p80_populated_jedec(s);
-        for (i = 0; i < s->pi->id_len; i++) {
-            s->data[i] = s->pi->id[i];
-        }
-        for (; i < SPI_NOR_MAX_ID_LEN; i++) {
-            s->data[i] = 0;
-        }
+        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
+            numonyx_get_mode(s) != MODE_QIO)) {
+            trace_m25p80_populated_jedec(s);
+            for (i = 0; i < s->pi->id_len; i++) {
+                s->data[i] = s->pi->id[i];
+            }
+            for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+                s->data[i] = 0;
+            }
 
-        s->len = SPI_NOR_MAX_ID_LEN;
-        s->pos = 0;
-        s->state = STATE_READING_DATA;
+            s->len = SPI_NOR_MAX_ID_LEN;
+            s->pos = 0;
+            s->state = STATE_READING_DATA;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute JEDEC read "
+                          "in DIO or QIO mode\n");
+        }
         break;
 
     case RDCR:
-- 
2.7.4

Re: [PATCH v4 3/4] hw/block/m25p80: Check SPI mode before running some Numonyx commands

[Francisco Iglesias]

Hi Joe,

On Thu, Nov 12, 2020 at 07:10:54PM -0800, Joe Komlodi wrote:
> Some Numonyx flash commands cannot be executed in DIO and QIO mode, such as
> trying to do DPP or DOR when in QIO mode.
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 134 +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 112 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index eb6539f..2552f2c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -413,6 +413,12 @@ typedef enum {
>      MAN_GENERIC,
>  } Manufacturer;
>  
> +typedef enum {
> +    MODE_STD = 0,
> +    MODE_DIO = 1,
> +    MODE_QIO = 2
> +} SPIMode;
> +
>  #define M25P80_INTERNAL_DATA_BUFFER_SZ 16
>  
>  struct Flash {
> @@ -820,6 +826,17 @@ static void reset_memory(Flash *s)
>      trace_m25p80_reset_done(s);
>  }
>  
> +static uint8_t numonyx_get_mode(Flash *s)
> +{
> +    if (!(s->enh_volatile_cfg & EVCFG_QUAD_IO_DISABLED)) {
> +        return MODE_QIO;
> +    } else if (!(s->enh_volatile_cfg & EVCFG_DUAL_IO_DISABLED)) {
> +        return MODE_DIO;
> +    } else {
> +        return MODE_STD;
> +    }
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)
>  {
>      s->needed_bytes = get_addr_length(s);
> @@ -827,9 +844,11 @@ static void decode_fast_read_cmd(Flash *s)
>      /* Dummy cycles - modeled with bytes writes instead of bits */
>      case MAN_WINBOND:
>          s->needed_bytes += 8;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          if (extract32(s->volatile_cfg, 6, 2) == 1) {
> @@ -837,19 +856,21 @@ static void decode_fast_read_cmd(Flash *s)
>          } else {
>              s->needed_bytes += 8;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += extract32(s->spansion_cr2v,
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;

Above change in this function and the similar ones in below two functions
don't seem to be needed anymore (s->state = STATE_COLLECTING_DATA is being done
in all cases).

>  }
>  
>  static void decode_dio_read_cmd(Flash *s)
> @@ -859,6 +880,7 @@ static void decode_dio_read_cmd(Flash *s)
>      switch (get_man(s)) {
>      case MAN_WINBOND:
>          s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> @@ -866,9 +888,11 @@ static void decode_dio_read_cmd(Flash *s)
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> @@ -882,13 +906,14 @@ static void decode_dio_read_cmd(Flash *s)
>              s->needed_bytes += 4;
>              break;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;
>  }
>  
>  static void decode_qio_read_cmd(Flash *s)
> @@ -899,6 +924,7 @@ static void decode_qio_read_cmd(Flash *s)
>      case MAN_WINBOND:
>          s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
>          s->needed_bytes += 4;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> @@ -906,9 +932,11 @@ static void decode_qio_read_cmd(Flash *s)
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> @@ -922,13 +950,14 @@ static void decode_qio_read_cmd(Flash *s)
>              s->needed_bytes += 6;
>              break;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;
>  }
>  
>  static void decode_new_cmd(Flash *s, uint32_t value)
> @@ -950,14 +979,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case ERASE4_32K:
>      case ERASE_SECTOR:
>      case ERASE4_SECTOR:
> -    case READ:
> -    case READ4:
> -    case DPP:
> -    case QPP:
> -    case QPP_4:
>      case PP:
>      case PP4:
> -    case PP4_4:
>      case DIE_ERASE:
>      case RDID_90:
>      case RDID_AB:
> @@ -966,24 +989,85 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
>          break;
> +    case READ:
> +    case READ4:
> +        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
> +            numonyx_get_mode(s) != MODE_QIO)) {

Above seems easier to check if we are in the correct mode here instead:

if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) {

> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO or QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
> +    case DPP:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
> +    case QPP:
> +    case QPP_4:
> +    case PP4_4:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
>  
>      case FAST_READ:
>      case FAST_READ4:
> +        decode_fast_read_cmd(s);
> +        break;
>      case DOR:
>      case DOR4:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            decode_fast_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
>      case QOR:
>      case QOR4:
> -        decode_fast_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            decode_fast_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case DIOR:
>      case DIOR4:
> -        decode_dio_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            decode_dio_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case QIOR:
>      case QIOR4:
> -        decode_qio_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            decode_qio_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case WRSR:
> @@ -1035,17 +1119,23 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>  
>      case JEDEC_READ:
> -        trace_m25p80_populated_jedec(s);
> -        for (i = 0; i < s->pi->id_len; i++) {
> -            s->data[i] = s->pi->id[i];
> -        }
> -        for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -            s->data[i] = 0;
> -        }
> +        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
> +            numonyx_get_mode(s) != MODE_QIO)) {

The same here as above:
if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) {

Best regards,
Francisco Iglesias


> +            trace_m25p80_populated_jedec(s);
> +            for (i = 0; i < s->pi->id_len; i++) {
> +                s->data[i] = s->pi->id[i];
> +            }
> +            for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +                s->data[i] = 0;
> +            }
>  
> -        s->len = SPI_NOR_MAX_ID_LEN;
> -        s->pos = 0;
> -        s->state = STATE_READING_DATA;
> +            s->len = SPI_NOR_MAX_ID_LEN;
> +            s->pos = 0;
> +            s->state = STATE_READING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute JEDEC read "
> +                          "in DIO or QIO mode\n");
> +        }
>          break;
>  
>      case RDCR:
> -- 
> 2.7.4
> 

RE: [PATCH v4 3/4] hw/block/m25p80: Check SPI mode before running some Numonyx commands

[Joe Komlodi]

Hi Francisco,

-----Original Message-----
From: Francisco Iglesias <francisco.iglesias@xilinx.com> 
Sent: Monday, November 16, 2020 7:59 AM
To: Joe Komlodi <komlodi@xilinx.com>
Cc: qemu-devel@nongnu.org; philippe.mathieu.daude@gmail.com; Francisco Eduardo Iglesias <figlesia@xilinx.com>; alistair@alistair23.me; qemu-block@nongnu.org; mreitz@redhat.com
Subject: Re: [PATCH v4 3/4] hw/block/m25p80: Check SPI mode before running some Numonyx commands

Hi Joe,

On Thu, Nov 12, 2020 at 07:10:54PM -0800, Joe Komlodi wrote:
> Some Numonyx flash commands cannot be executed in DIO and QIO mode, 
> such as trying to do DPP or DOR when in QIO mode.
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 134 
> +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 112 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 
> eb6539f..2552f2c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -413,6 +413,12 @@ typedef enum {
>      MAN_GENERIC,
>  } Manufacturer;
>  
> +typedef enum {
> +    MODE_STD = 0,
> +    MODE_DIO = 1,
> +    MODE_QIO = 2
> +} SPIMode;
> +
>  #define M25P80_INTERNAL_DATA_BUFFER_SZ 16
>  
>  struct Flash {
> @@ -820,6 +826,17 @@ static void reset_memory(Flash *s)
>      trace_m25p80_reset_done(s);
>  }
>  
> +static uint8_t numonyx_get_mode(Flash *s) {
> +    if (!(s->enh_volatile_cfg & EVCFG_QUAD_IO_DISABLED)) {
> +        return MODE_QIO;
> +    } else if (!(s->enh_volatile_cfg & EVCFG_DUAL_IO_DISABLED)) {
> +        return MODE_DIO;
> +    } else {
> +        return MODE_STD;
> +    }
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)  {
>      s->needed_bytes = get_addr_length(s); @@ -827,9 +844,11 @@ static 
> void decode_fast_read_cmd(Flash *s)
>      /* Dummy cycles - modeled with bytes writes instead of bits */
>      case MAN_WINBOND:
>          s->needed_bytes += 8;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          if (extract32(s->volatile_cfg, 6, 2) == 1) { @@ -837,19 
> +856,21 @@ static void decode_fast_read_cmd(Flash *s)
>          } else {
>              s->needed_bytes += 8;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += extract32(s->spansion_cr2v,
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;

Above change in this function and the similar ones in below two functions don't seem to be needed anymore (s->state = STATE_COLLECTING_DATA is being done in all cases).
[Joe] Oops, I'll simplify that.

>  }
>  
>  static void decode_dio_read_cmd(Flash *s) @@ -859,6 +880,7 @@ static 
> void decode_dio_read_cmd(Flash *s)
>      switch (get_man(s)) {
>      case MAN_WINBOND:
>          s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> @@ -866,9 +888,11 @@ static void decode_dio_read_cmd(Flash *s)
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) { @@ -882,13 
> +906,14 @@ static void decode_dio_read_cmd(Flash *s)
>              s->needed_bytes += 4;
>              break;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;
>  }
>  
>  static void decode_qio_read_cmd(Flash *s) @@ -899,6 +924,7 @@ static 
> void decode_qio_read_cmd(Flash *s)
>      case MAN_WINBOND:
>          s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
>          s->needed_bytes += 4;
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> @@ -906,9 +932,11 @@ static void decode_qio_read_cmd(Flash *s)
>                                      SPANSION_DUMMY_CLK_POS,
>                                      SPANSION_DUMMY_CLK_LEN
>                                      );
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
>          s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) { @@ -922,13 
> +950,14 @@ static void decode_qio_read_cmd(Flash *s)
>              s->needed_bytes += 6;
>              break;
>          }
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      default:
> +        s->state = STATE_COLLECTING_DATA;
>          break;
>      }
>      s->pos = 0;
>      s->len = 0;
> -    s->state = STATE_COLLECTING_DATA;
>  }
>  
>  static void decode_new_cmd(Flash *s, uint32_t value) @@ -950,14 
> +979,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case ERASE4_32K:
>      case ERASE_SECTOR:
>      case ERASE4_SECTOR:
> -    case READ:
> -    case READ4:
> -    case DPP:
> -    case QPP:
> -    case QPP_4:
>      case PP:
>      case PP4:
> -    case PP4_4:
>      case DIE_ERASE:
>      case RDID_90:
>      case RDID_AB:
> @@ -966,24 +989,85 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
>          break;
> +    case READ:
> +    case READ4:
> +        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
> +            numonyx_get_mode(s) != MODE_QIO)) {

Above seems easier to check if we are in the correct mode here instead:

if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) {

> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO or QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
> +    case DPP:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
> +    case QPP:
> +    case QPP_4:
> +    case PP4_4:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
>  
>      case FAST_READ:
>      case FAST_READ4:
> +        decode_fast_read_cmd(s);
> +        break;
>      case DOR:
>      case DOR4:
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            decode_fast_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
> +        break;
>      case QOR:
>      case QOR4:
> -        decode_fast_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            decode_fast_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case DIOR:
>      case DIOR4:
> -        decode_dio_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) {
> +            decode_dio_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case QIOR:
>      case QIOR4:
> -        decode_qio_read_cmd(s);
> +        if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) {
> +            decode_qio_read_cmd(s);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +        }
>          break;
>  
>      case WRSR:
> @@ -1035,17 +1119,23 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>  
>      case JEDEC_READ:
> -        trace_m25p80_populated_jedec(s);
> -        for (i = 0; i < s->pi->id_len; i++) {
> -            s->data[i] = s->pi->id[i];
> -        }
> -        for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -            s->data[i] = 0;
> -        }
> +        if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO &&
> +            numonyx_get_mode(s) != MODE_QIO)) {

The same here as above:
if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) {

[Joe] Agreed, that's much easier and keeps it on one line, not sure how I missed that...
Thanks!
Joe

Best regards,
Francisco Iglesias


> +            trace_m25p80_populated_jedec(s);
> +            for (i = 0; i < s->pi->id_len; i++) {
> +                s->data[i] = s->pi->id[i];
> +            }
> +            for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +                s->data[i] = 0;
> +            }
>  
> -        s->len = SPI_NOR_MAX_ID_LEN;
> -        s->pos = 0;
> -        s->state = STATE_READING_DATA;
> +            s->len = SPI_NOR_MAX_ID_LEN;
> +            s->pos = 0;
> +            s->state = STATE_READING_DATA;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute JEDEC read "
> +                          "in DIO or QIO mode\n");
> +        }
>          break;
>  
>      case RDCR:
> --
> 2.7.4
> 

[PATCH v4 4/4] hw/block/m25p80: Fix Numonyx fast read dummy cycle count

[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]
https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf?rev=9b167fbf2b3645efba6385949a72e453

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 2552f2c..0c78015 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -837,6 +837,30 @@ static uint8_t numonyx_get_mode(Flash *s)
     }
 }
 
+static uint8_t numonyx_extract_cfg_num_dummies(Flash *s)
+{
+    uint8_t num_dummies;
+    uint8_t mode;
+    assert(get_man(s) == MAN_NUMONYX);
+
+    mode = numonyx_get_mode(s);
+    num_dummies = extract32(s->volatile_cfg, 4, 4);
+
+    if (num_dummies == 0x0 || num_dummies == 0xf) {
+        switch (s->cmd_in_progress) {
+        case QIOR:
+        case QIOR4:
+            num_dummies = 10;
+            break;
+        default:
+            num_dummies = (mode == MODE_QIO) ? 10 : 8;
+            break;
+        }
+    }
+
+    return num_dummies;
+}
+
 static void decode_fast_read_cmd(Flash *s)
 {
     s->needed_bytes = get_addr_length(s);
@@ -847,7 +871,7 @@ static void decode_fast_read_cmd(Flash *s)
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
@@ -891,7 +915,7 @@ static void decode_dio_read_cmd(Flash *s)
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
@@ -935,7 +959,7 @@ static void decode_qio_read_cmd(Flash *s)
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_NUMONYX:
-        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
         s->state = STATE_COLLECTING_DATA;
         break;
     case MAN_MACRONIX:
-- 
2.7.4

Re: [PATCH v4 4/4] hw/block/m25p80: Fix Numonyx fast read dummy cycle count

[Francisco Iglesias]

Hi Joe,

On Thu, Nov 12, 2020 at 07:10:55PM -0800, 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]

With above changed to:

"
However, if these bits are 0x0 or 0xF, the number of dummy cycles to wait
is 10 on a QIOR or QIOR4 command or when in QIO mode and else 8 for the
currently supported fast read commands. [1]
"

Reviewed-by: Francisco Iglesias <francisco.iglesias@xilinx.com>

Best regards,
Francisco Iglesias


> 
> [1]
> https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf?rev=9b167fbf2b3645efba6385949a72e453
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 2552f2c..0c78015 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -837,6 +837,30 @@ static uint8_t numonyx_get_mode(Flash *s)
>      }
>  }
>  
> +static uint8_t numonyx_extract_cfg_num_dummies(Flash *s)
> +{
> +    uint8_t num_dummies;
> +    uint8_t mode;
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    mode = numonyx_get_mode(s);
> +    num_dummies = extract32(s->volatile_cfg, 4, 4);
> +
> +    if (num_dummies == 0x0 || num_dummies == 0xf) {
> +        switch (s->cmd_in_progress) {
> +        case QIOR:
> +        case QIOR4:
> +            num_dummies = 10;
> +            break;
> +        default:
> +            num_dummies = (mode == MODE_QIO) ? 10 : 8;
> +            break;
> +        }
> +    }
> +
> +    return num_dummies;
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)
>  {
>      s->needed_bytes = get_addr_length(s);
> @@ -847,7 +871,7 @@ static void decode_fast_read_cmd(Flash *s)
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
> @@ -891,7 +915,7 @@ static void decode_dio_read_cmd(Flash *s)
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
> @@ -935,7 +959,7 @@ static void decode_qio_read_cmd(Flash *s)
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_NUMONYX:
> -        s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +        s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>          s->state = STATE_COLLECTING_DATA;
>          break;
>      case MAN_MACRONIX:
> -- 
> 2.7.4
>