diff mbox series

[v2,3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()

Message ID 20190702001301.4768-4-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/block/pflash_cfi01: Add DeviceReset() handler | expand

Commit Message

Philippe Mathieu-Daudé July 2, 2019, 12:12 a.m. UTC
The same pattern is used when setting the flash in READ_ARRAY mode:
- Set the state machine command to READ_ARRAY
- Reset the write_cycle counter
- Reset the memory region in ROMD

Refactor the current code by extracting this pattern.
It is used twice:
- On a write access (on command failure, error, or explicitly asked)
- When the device is initialized. Here the ROMD mode is hidden
  by the memory_region_init_rom_device() call.

Rename the 'reset_flash' as 'mode_read_array' to make explicit we
do not reset the device, we simply set its internal state machine
in the READ_ARRAY mode. We do not reset the status register error
bits, as a device reset would do.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++----------------
 hw/block/trace-events   |  1 +
 2 files changed, 21 insertions(+), 16 deletions(-)

Comments

John Snow July 2, 2019, 3:01 a.m. UTC | #1
On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote:
> The same pattern is used when setting the flash in READ_ARRAY mode:
> - Set the state machine command to READ_ARRAY
> - Reset the write_cycle counter
> - Reset the memory region in ROMD
> 
> Refactor the current code by extracting this pattern.
> It is used twice:
> - On a write access (on command failure, error, or explicitly asked)
> - When the device is initialized. Here the ROMD mode is hidden
>   by the memory_region_init_rom_device() call.
> 
> Rename the 'reset_flash' as 'mode_read_array' to make explicit we
> do not reset the device, we simply set its internal state machine
> in the READ_ARRAY mode. We do not reset the status register error
> bits, as a device reset would do.
> 

The rename does make it less confusing; but as of this patch it doesn't
seem obvious why you need this as a function, it's only really used
about "1.5 times" as of yet -- the realize call as you mention doesn't
quite exactly utilize it.

I will assume this comes in handy later, so:

Reviewed-by: John Snow <jsnow@redhat.com>

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++----------------
>  hw/block/trace-events   |  1 +
>  2 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 743b5d5794..33c77f6569 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_mode_read_array();
> +    pfl->cmd = 0xff; /* Read Array */
> +    pfl->wcycle = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
>  /* Perform a CFI query based on the bank width of the flash.
>   * If this code is called we know we have a device_width set for
>   * this flash.
> @@ -470,7 +478,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>          case 0x50: /* Clear status bits */
>              DPRINTF("%s: Clear status bits\n", __func__);
>              pfl->status = 0x0;
> -            goto reset_flash;
> +            goto mode_read_array;
>          case 0x60: /* Block (un)lock */
>              DPRINTF("%s: Block unlock\n", __func__);
>              break;
> @@ -495,10 +503,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              break;
>          case 0xf0: /* Probe for AMD flash */
>              DPRINTF("%s: Probe for AMD flash\n", __func__);
> -            goto reset_flash;
> +            goto mode_read_array;
>          case 0xff: /* Read array mode */
>              DPRINTF("%s: Read array mode\n", __func__);
> -            goto reset_flash;
> +            goto mode_read_array;
>          default:
>              goto error_flash;
>          }
> @@ -525,7 +533,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
>              } else if (cmd == 0xff) { /* Read Array */
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else
>                  goto error_flash;
>  
> @@ -552,15 +560,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
>              } else if (cmd == 0xff) { /* read array mode */
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> -                goto reset_flash;
> +                goto mode_read_array;
>              }
>              break;
>          case 0x98:
>              if (cmd == 0xff) {
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else {
>                  DPRINTF("%s: leaving query mode\n", __func__);
>              }
> @@ -620,7 +628,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                      " the data is already written to storage!\n"
>                      "Flash device reset into READ mode.\n",
>                      __func__);
> -                goto reset_flash;
> +                goto mode_read_array;
>              }
>              break;
>          default:
> @@ -630,7 +638,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      default:
>          /* Should never happen */
>          DPRINTF("%s: invalid write state\n",  __func__);
> -        goto reset_flash;
> +        goto mode_read_array;
>      }
>      return;
>  
> @@ -639,11 +647,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value 0x%x)"
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
> - reset_flash:
> -    trace_pflash_reset();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0xff;
> + mode_read_array:
> +    pflash_mode_read_array(pfl);
>  }
>  
>  
> @@ -758,8 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0xff;
> +    pflash_mode_read_array(pfl);
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 97a17838ed..d627cfc3f5 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -7,6 +7,7 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
>  # pflash_cfi02.c
>  # pflash_cfi01.c
>  pflash_reset(void) "reset"
> +pflash_mode_read_array(void) "mode: read array"
>  pflash_read(uint64_t offset, uint8_t cmd, int width, uint8_t wcycle) "offset:0x%04"PRIx64" cmd:0x%02x width:%d wcycle:%u"
>  pflash_write(uint64_t offset, uint32_t value, int width, uint8_t wcycle) "offset:0x%04"PRIx64" value:0x%03x width:%d wcycle:%u"
>  pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
>
Alistair Francis July 2, 2019, 4:01 p.m. UTC | #2
On Mon, Jul 1, 2019 at 5:14 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> The same pattern is used when setting the flash in READ_ARRAY mode:
> - Set the state machine command to READ_ARRAY
> - Reset the write_cycle counter
> - Reset the memory region in ROMD
>
> Refactor the current code by extracting this pattern.
> It is used twice:
> - On a write access (on command failure, error, or explicitly asked)
> - When the device is initialized. Here the ROMD mode is hidden
>   by the memory_region_init_rom_device() call.
>
> Rename the 'reset_flash' as 'mode_read_array' to make explicit we
> do not reset the device, we simply set its internal state machine
> in the READ_ARRAY mode. We do not reset the status register error
> bits, as a device reset would do.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++----------------
>  hw/block/trace-events   |  1 +
>  2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 743b5d5794..33c77f6569 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_mode_read_array();
> +    pfl->cmd = 0xff; /* Read Array */
> +    pfl->wcycle = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
>  /* Perform a CFI query based on the bank width of the flash.
>   * If this code is called we know we have a device_width set for
>   * this flash.
> @@ -470,7 +478,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>          case 0x50: /* Clear status bits */
>              DPRINTF("%s: Clear status bits\n", __func__);
>              pfl->status = 0x0;
> -            goto reset_flash;
> +            goto mode_read_array;
>          case 0x60: /* Block (un)lock */
>              DPRINTF("%s: Block unlock\n", __func__);
>              break;
> @@ -495,10 +503,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              break;
>          case 0xf0: /* Probe for AMD flash */
>              DPRINTF("%s: Probe for AMD flash\n", __func__);
> -            goto reset_flash;
> +            goto mode_read_array;
>          case 0xff: /* Read array mode */
>              DPRINTF("%s: Read array mode\n", __func__);
> -            goto reset_flash;
> +            goto mode_read_array;
>          default:
>              goto error_flash;
>          }
> @@ -525,7 +533,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
>              } else if (cmd == 0xff) { /* Read Array */
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else
>                  goto error_flash;
>
> @@ -552,15 +560,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
>              } else if (cmd == 0xff) { /* read array mode */
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> -                goto reset_flash;
> +                goto mode_read_array;
>              }
>              break;
>          case 0x98:
>              if (cmd == 0xff) {
> -                goto reset_flash;
> +                goto mode_read_array;
>              } else {
>                  DPRINTF("%s: leaving query mode\n", __func__);
>              }
> @@ -620,7 +628,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                      " the data is already written to storage!\n"
>                      "Flash device reset into READ mode.\n",
>                      __func__);
> -                goto reset_flash;
> +                goto mode_read_array;
>              }
>              break;
>          default:
> @@ -630,7 +638,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      default:
>          /* Should never happen */
>          DPRINTF("%s: invalid write state\n",  __func__);
> -        goto reset_flash;
> +        goto mode_read_array;
>      }
>      return;
>
> @@ -639,11 +647,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value 0x%x)"
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
> - reset_flash:
> -    trace_pflash_reset();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0xff;
> + mode_read_array:
> +    pflash_mode_read_array(pfl);
>  }
>
>
> @@ -758,8 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0xff;
> +    pflash_mode_read_array(pfl);
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 97a17838ed..d627cfc3f5 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -7,6 +7,7 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
>  # pflash_cfi02.c
>  # pflash_cfi01.c
>  pflash_reset(void) "reset"
> +pflash_mode_read_array(void) "mode: read array"
>  pflash_read(uint64_t offset, uint8_t cmd, int width, uint8_t wcycle) "offset:0x%04"PRIx64" cmd:0x%02x width:%d wcycle:%u"
>  pflash_write(uint64_t offset, uint32_t value, int width, uint8_t wcycle) "offset:0x%04"PRIx64" value:0x%03x width:%d wcycle:%u"
>  pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
> --
> 2.20.1
>
diff mbox series

Patch

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 743b5d5794..33c77f6569 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -112,6 +112,14 @@  static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_mode_read_array(PFlashCFI01 *pfl)
+{
+    trace_pflash_mode_read_array();
+    pfl->cmd = 0xff; /* Read Array */
+    pfl->wcycle = 0;
+    memory_region_rom_device_set_romd(&pfl->mem, true);
+}
+
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -470,7 +478,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         case 0x50: /* Clear status bits */
             DPRINTF("%s: Clear status bits\n", __func__);
             pfl->status = 0x0;
-            goto reset_flash;
+            goto mode_read_array;
         case 0x60: /* Block (un)lock */
             DPRINTF("%s: Block unlock\n", __func__);
             break;
@@ -495,10 +503,10 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             break;
         case 0xf0: /* Probe for AMD flash */
             DPRINTF("%s: Probe for AMD flash\n", __func__);
-            goto reset_flash;
+            goto mode_read_array;
         case 0xff: /* Read array mode */
             DPRINTF("%s: Read array mode\n", __func__);
-            goto reset_flash;
+            goto mode_read_array;
         default:
             goto error_flash;
         }
@@ -525,7 +533,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
             } else if (cmd == 0xff) { /* Read Array */
-                goto reset_flash;
+                goto mode_read_array;
             } else
                 goto error_flash;
 
@@ -552,15 +560,15 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
             } else if (cmd == 0xff) { /* read array mode */
-                goto reset_flash;
+                goto mode_read_array;
             } else {
                 DPRINTF("%s: Unknown (un)locking command\n", __func__);
-                goto reset_flash;
+                goto mode_read_array;
             }
             break;
         case 0x98:
             if (cmd == 0xff) {
-                goto reset_flash;
+                goto mode_read_array;
             } else {
                 DPRINTF("%s: leaving query mode\n", __func__);
             }
@@ -620,7 +628,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                     " the data is already written to storage!\n"
                     "Flash device reset into READ mode.\n",
                     __func__);
-                goto reset_flash;
+                goto mode_read_array;
             }
             break;
         default:
@@ -630,7 +638,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     default:
         /* Should never happen */
         DPRINTF("%s: invalid write state\n",  __func__);
-        goto reset_flash;
+        goto mode_read_array;
     }
     return;
 
@@ -639,11 +647,8 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value 0x%x)"
                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
- reset_flash:
-    trace_pflash_reset();
-    memory_region_rom_device_set_romd(&pfl->mem, true);
-    pfl->wcycle = 0;
-    pfl->cmd = 0xff;
+ mode_read_array:
+    pflash_mode_read_array(pfl);
 }
 
 
@@ -758,8 +763,7 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    pfl->cmd = 0xff;
+    pflash_mode_read_array(pfl);
     pfl->status = 0;
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 97a17838ed..d627cfc3f5 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -7,6 +7,7 @@  fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
 # pflash_cfi02.c
 # pflash_cfi01.c
 pflash_reset(void) "reset"
+pflash_mode_read_array(void) "mode: read array"
 pflash_read(uint64_t offset, uint8_t cmd, int width, uint8_t wcycle) "offset:0x%04"PRIx64" cmd:0x%02x width:%d wcycle:%u"
 pflash_write(uint64_t offset, uint32_t value, int width, uint8_t wcycle) "offset:0x%04"PRIx64" value:0x%03x width:%d wcycle:%u"
 pflash_timer_expired(uint8_t cmd) "command 0x%02x done"