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

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

[Joe Komlodi]

Changelog:
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 (3):
  hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity
  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 | 176 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 161 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Joe Komlodi]

QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
This matches the behavior of the other bits in the NVCFG register.

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f..4255a6a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
         s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
         s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
         s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
-        if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
+        if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
             s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
         }
-        if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
+        if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
             s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
         }
         if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
-- 
2.7.4

[PATCH v3 2/3] 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 | 132 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 119 insertions(+), 13 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 4255a6a..8a1b684 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,70 @@ static void reset_memory(Flash *s)
     trace_m25p80_reset_done(s);
 }
 
+static uint8_t numonyx_get_mode(Flash *s)
+{
+    uint8_t mode;
+
+    if (s->enh_volatile_cfg & EVCFG_QUAD_IO_ENABLED) {
+        mode = MODE_QIO;
+    } else if (s->enh_volatile_cfg & EVCFG_DUAL_IO_ENABLED) {
+        mode = MODE_DIO;
+    } else {
+        mode = MODE_STD;
+    }
+
+    return mode;
+}
+
+static bool numonyx_check_cmd_mode(Flash *s)
+{
+    uint8_t mode;
+    assert(get_man(s) == MAN_NUMONYX);
+
+    mode = numonyx_get_mode(s);
+
+    switch (mode) {
+    case MODE_STD:
+        return true;
+    case MODE_DIO:
+        switch (s->cmd_in_progress) {
+        case QOR:
+        case QOR4:
+        case QIOR:
+        case QIOR4:
+        case QPP:
+        case QPP_4:
+        case PP4_4:
+        case JEDEC_READ:
+        case READ:
+        case READ4:
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "DIO mode\n", s->cmd_in_progress);
+            return false;
+        default:
+            return true;
+        }
+    case MODE_QIO:
+        switch (s->cmd_in_progress) {
+        case DOR:
+        case DOR4:
+        case DIOR:
+        case DIOR4:
+        case DPP:
+        case JEDEC_READ:
+        case READ:
+        case READ4:
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
+                          "QIO mode\n", s->cmd_in_progress);
+            return false;
+        default:
+            return true;
+        }
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static void decode_fast_read_cmd(Flash *s)
 {
     s->needed_bytes = get_addr_length(s);
@@ -827,9 +897,13 @@ 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);
+        if (numonyx_check_cmd_mode(s)) {
+            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 +911,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 +935,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 +943,13 @@ 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);
+        if (numonyx_check_cmd_mode(s)) {
+            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 +963,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 +981,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 +989,13 @@ 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);
+        if (numonyx_check_cmd_mode(s)) {
+            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 +1009,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,6 +1038,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case ERASE4_32K:
     case ERASE_SECTOR:
     case ERASE4_SECTOR:
+    case DIE_ERASE:
+    case RDID_90:
+    case RDID_AB:
+        s->needed_bytes = get_addr_length(s);
+        s->pos = 0;
+        s->len = 0;
+        s->state = STATE_COLLECTING_DATA;
+        break;
+
     case READ:
     case READ4:
     case DPP:
@@ -958,13 +1055,19 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case PP:
     case PP4:
     case PP4_4:
-    case DIE_ERASE:
-    case RDID_90:
-    case RDID_AB:
-        s->needed_bytes = get_addr_length(s);
-        s->pos = 0;
-        s->len = 0;
-        s->state = STATE_COLLECTING_DATA;
+        if (get_man(s) == MAN_NUMONYX) {
+            if (numonyx_check_cmd_mode(s)) {
+                s->needed_bytes = get_addr_length(s);
+                s->pos = 0;
+                s->len = 0;
+                s->state = STATE_COLLECTING_DATA;
+            }
+        } else {
+            s->needed_bytes = get_addr_length(s);
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+        }
         break;
 
     case FAST_READ:
@@ -1035,6 +1138,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
 
     case JEDEC_READ:
+        if (get_man(s) == MAN_NUMONYX && !numonyx_check_cmd_mode(s)) {
+            break;
+        }
         trace_m25p80_populated_jedec(s);
         for (i = 0; i < s->pi->id_len; i++) {
             s->data[i] = s->pi->id[i];
-- 
2.7.4

[PATCH v3 3/3] 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 | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8a1b684..a2cdfb6 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -841,6 +841,43 @@ static uint8_t numonyx_get_mode(Flash *s)
     return mode;
 }
 
+static uint8_t numonyx_extract_cfg_num_dummies(Flash *s)
+{
+    uint8_t cycle_count;
+    uint8_t num_dummies;
+    uint8_t mode;
+    uint8_t cycle_table[0x100][3] = {
+        [FAST_READ] = {8, 8, 10},
+        [FAST_READ4] = {8, 8, 10},
+        [DOR] = {8, 8, 0xff},
+        [DOR4] = {8, 8, 0xff},
+        [QOR] = {8, 0xff, 10},
+        [QOR4] = {8, 0xff, 10},
+        [DIOR] = {8, 8, 0xff},
+        [DIOR4] = {8, 8, 0xff},
+        [QIOR] = {10, 0xff, 10},
+        [QIOR4] = {10, 0xff, 10},
+    };
+    assert(get_man(s) == MAN_NUMONYX);
+
+    mode = numonyx_get_mode(s);
+
+    cycle_count = extract32(s->volatile_cfg, 4, 4);
+    if (cycle_count == 0x0 || cycle_count == 0xf) {
+        num_dummies = cycle_table[s->cmd_in_progress][mode];
+    } else {
+        num_dummies = cycle_count;
+    }
+
+    /*
+     * Validation if the command can be executed should be done outside of
+     * this function. e.g. trying to execute DIOR in QIO mode.
+     */
+    assert(num_dummies != 0xff);
+
+    return num_dummies;
+}
+
 static bool numonyx_check_cmd_mode(Flash *s)
 {
     uint8_t mode;
@@ -901,7 +938,7 @@ static void decode_fast_read_cmd(Flash *s)
         break;
     case MAN_NUMONYX:
         if (numonyx_check_cmd_mode(s)) {
-            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
             s->state = STATE_COLLECTING_DATA;
         }
         break;
@@ -947,7 +984,7 @@ static void decode_dio_read_cmd(Flash *s)
         break;
     case MAN_NUMONYX:
         if (numonyx_check_cmd_mode(s)) {
-            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
             s->state = STATE_COLLECTING_DATA;
         }
         break;
@@ -993,7 +1030,7 @@ static void decode_qio_read_cmd(Flash *s)
         break;
     case MAN_NUMONYX:
         if (numonyx_check_cmd_mode(s)) {
-            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
+            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
             s->state = STATE_COLLECTING_DATA;
         }
         break;
-- 
2.7.4

Re: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Philippe Mathieu-Daudé]

On 11/6/20 2:32 AM, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

Is this material for the 5.2 release?

> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>          s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>          s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>          s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -        if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>          }
> -        if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> 

RE: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Joe Komlodi]

Hi Philippe,

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com> 
Sent: Monday, November 9, 2020 6:21 AM
To: Joe Komlodi <komlodi@xilinx.com>; qemu-devel@nongnu.org; Cédric Le Goater <clg@kaod.org>; Edgar Iglesias <edgari@xilinx.com>
Cc: Francisco Eduardo Iglesias <figlesia@xilinx.com>; alistair@alistair23.me; philippe.mathieu.daude@gmail.com; qemu-block@nongnu.org; mreitz@redhat.com
Subject: Re: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

On 11/6/20 2:32 AM, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

Is this material for the 5.2 release?

[Joe] If possible, yeah.  Otherwise if it's too late for 5.2 I have no problems pushing it to a later release.
Thanks!
Joe

> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 
> 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>          s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>          s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>          s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -        if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>          }
> -        if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> 

Re: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Francisco Iglesias]

Hi Joe,

On Thu, Nov 05, 2020 at 05:32:56PM -0800, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

The enhanced register has the bits with the same polarities, meaning that the
value should be propagated as it was before (0 - enabled and 1 - disabled). I
do see though that the define naming is not perfect and can make you believe
otherwise.

Best regards,
Francisco Iglesias

> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>          s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>          s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>          s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -        if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>          }
> -        if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> -- 
> 2.7.4
> 

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

[Francisco Iglesias]

Hi Joe,

On Thu, Nov 05, 2020 at 05:32:57PM -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 | 132 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 119 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 4255a6a..8a1b684 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,70 @@ static void reset_memory(Flash *s)
>      trace_m25p80_reset_done(s);
>  }
>  
> +static uint8_t numonyx_get_mode(Flash *s)
> +{
> +    uint8_t mode;

We need to swap the polarities in below if checks. If you would like to get rid
of some lines you can also just return the enum directly instead (and remove
the 'mode' variable). 

> +
> +    if (s->enh_volatile_cfg & EVCFG_QUAD_IO_ENABLED) {
> +        mode = MODE_QIO;
> +    } else if (s->enh_volatile_cfg & EVCFG_DUAL_IO_ENABLED) {
> +        mode = MODE_DIO;
> +    } else {
> +        mode = MODE_STD;
> +    }
> +
> +    return mode;
> +}
> +
> +static bool numonyx_check_cmd_mode(Flash *s)
> +{
> +    uint8_t mode;
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    mode = numonyx_get_mode(s);
> +
> +    switch (mode) {
> +    case MODE_STD:
> +        return true;
> +    case MODE_DIO:
> +        switch (s->cmd_in_progress) {
> +        case QOR:
> +        case QOR4:
> +        case QIOR:
> +        case QIOR4:
> +        case QPP:
> +        case QPP_4:
> +        case PP4_4:
> +        case JEDEC_READ:
> +        case READ:
> +        case READ4:
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "DIO mode\n", s->cmd_in_progress);
> +            return false;
> +        default:
> +            return true;
> +        }
> +    case MODE_QIO:
> +        switch (s->cmd_in_progress) {
> +        case DOR:
> +        case DOR4:
> +        case DIOR:
> +        case DIOR4:
> +        case DPP:
> +        case JEDEC_READ:
> +        case READ:
> +        case READ4:
> +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in "
> +                          "QIO mode\n", s->cmd_in_progress);
> +            return false;
> +        default:
> +            return true;
> +        }
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  static void decode_fast_read_cmd(Flash *s)
>  {
>      s->needed_bytes = get_addr_length(s);
> @@ -827,9 +897,13 @@ 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);
> +        if (numonyx_check_cmd_mode(s)) {
> +            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 +911,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 +935,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 +943,13 @@ 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);
> +        if (numonyx_check_cmd_mode(s)) {
> +            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 +963,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 +981,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 +989,13 @@ 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);
> +        if (numonyx_check_cmd_mode(s)) {

Instead of creating the numonyx_check_cmd_mode function I think it is
better to chop up the switch in decode_new_cmd further and entering the
decode_qio/dio/fast_read_cmd functions already checked for correct mode (the
commands are already switch into there).

> +            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 +1009,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,6 +1038,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case ERASE4_32K:
>      case ERASE_SECTOR:
>      case ERASE4_SECTOR:
> +    case DIE_ERASE:
> +    case RDID_90:
> +    case RDID_AB:
> +        s->needed_bytes = get_addr_length(s);
> +        s->pos = 0;
> +        s->len = 0;
> +        s->state = STATE_COLLECTING_DATA;
> +        break;
> +
>      case READ:
>      case READ4:
>      case DPP:
> @@ -958,13 +1055,19 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case PP:
>      case PP4:
>      case PP4_4:
> -    case DIE_ERASE:
> -    case RDID_90:
> -    case RDID_AB:

Similar as above comment, instead of creating numonyx_check_cmd_mode I think it
is better to do the mode check here (and switch out more cmds if needed).
Something like:

...
case QPP:
case QPP_4:
case PPP4_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: Attempting Quad PP while in DIO mode....!\n");
    }
    break;
    

> -        s->needed_bytes = get_addr_length(s);
> -        s->pos = 0;
> -        s->len = 0;
> -        s->state = STATE_COLLECTING_DATA;
> +        if (get_man(s) == MAN_NUMONYX) {
> +            if (numonyx_check_cmd_mode(s)) {
> +                s->needed_bytes = get_addr_length(s);
> +                s->pos = 0;
> +                s->len = 0;
> +                s->state = STATE_COLLECTING_DATA;
> +            }
> +        } else {
> +            s->needed_bytes = get_addr_length(s);
> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +        }
>          break;
>  
>      case FAST_READ:
> @@ -1035,6 +1138,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>  
>      case JEDEC_READ:
> +        if (get_man(s) == MAN_NUMONYX && !numonyx_check_cmd_mode(s)) {

A log message here could help out when debugging (and also check mode directly
as above).  

Best regards,
Francisco Iglesias

> +            break;
> +        }
>          trace_m25p80_populated_jedec(s);
>          for (i = 0; i < s->pi->id_len; i++) {
>              s->data[i] = s->pi->id[i];
> -- 
> 2.7.4
> 

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

[Francisco Iglesias]

Hi Joe,

On Thu, Nov 05, 2020 at 05:32:58PM -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]
> 
> [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 | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8a1b684..a2cdfb6 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -841,6 +841,43 @@ static uint8_t numonyx_get_mode(Flash *s)
>      return mode;
>  }
>  
> +static uint8_t numonyx_extract_cfg_num_dummies(Flash *s)
> +{
> +    uint8_t cycle_count;

We can remove some lines and shrink the function if we remove above variable
and work on num_dummies directly instead.

> +    uint8_t num_dummies;
> +    uint8_t mode;
> +    uint8_t cycle_table[0x100][3] = {
> +        [FAST_READ] = {8, 8, 10},
> +        [FAST_READ4] = {8, 8, 10},
> +        [DOR] = {8, 8, 0xff},
> +        [DOR4] = {8, 8, 0xff},
> +        [QOR] = {8, 0xff, 10},
> +        [QOR4] = {8, 0xff, 10},
> +        [DIOR] = {8, 8, 0xff},
> +        [DIOR4] = {8, 8, 0xff},
> +        [QIOR] = {10, 0xff, 10},
> +        [QIOR4] = {10, 0xff, 10},
> +    };
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    mode = numonyx_get_mode(s);
> +
> +    cycle_count = extract32(s->volatile_cfg, 4, 4);
> +    if (cycle_count == 0x0 || cycle_count == 0xf) {
> +        num_dummies = cycle_table[s->cmd_in_progress][mode];

Instead of the table i think it is easier to do a switch here:

switch (s->cmd_in_progress) {
case QIOR:
case QIOR4:
    num_dummies = 10;
    break;
default:
    num_dummies = (mode == MODE_QIO) = 10 : 8;
    break;

};

Best regards,
Francisco Iglesias

> +    } else {
> +        num_dummies = cycle_count;
> +    }
> +
> +    /*
> +     * Validation if the command can be executed should be done outside of
> +     * this function. e.g. trying to execute DIOR in QIO mode.
> +     */
> +    assert(num_dummies != 0xff);
> +
> +    return num_dummies;
> +}
> +
>  static bool numonyx_check_cmd_mode(Flash *s)
>  {
>      uint8_t mode;
> @@ -901,7 +938,7 @@ static void decode_fast_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> @@ -947,7 +984,7 @@ static void decode_dio_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> @@ -993,7 +1030,7 @@ static void decode_qio_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> -- 
> 2.7.4
> 

RE: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Joe Komlodi]

Hi Francisco,

-----Original Message-----
From: Francisco Iglesias <francisco.iglesias@xilinx.com> 
Sent: Wednesday, November 11, 2020 6:33 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 v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

Hi Joe,

On Thu, Nov 05, 2020 at 05:32:56PM -0800, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

The enhanced register has the bits with the same polarities, meaning that the value should be propagated as it was before (0 - enabled and 1 - disabled). I do see though that the define naming is not perfect and can make you believe otherwise.

[Joe] Ah, you're right.  I'll fix that and change the bit checks in the other patches accordingly.
I'll probably also add a patch to this series to change the define names to make them more accurate.

Thanks!
Joe

Best regards,
Francisco Iglesias

> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 
> 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>          s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>          s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>          s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -        if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>          }
> -        if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +        if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>              s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>          }
>          if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> --
> 2.7.4
>