diff mbox series

[v2,2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

Message ID 20190702001301.4768-3-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
In the "Read Array Flowchart" the command has a value of 0xFF.

In the document [*] the "Read Array Flowchart", the READ_ARRAY
command has a value of 0xff.

Use the correct value in the pflash model.

There is no change of behavior in the guest, because:
- when the guest were sending 0xFF, the reset_flash label
  was setting the command value as 0x00
- 0x00 was used internally for READ_ARRAY

[*] "Common Flash Interface (CFI) and Command Sets"
    (Intel Application Note 646)
    Appendix B "Basic Command Set"

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

John Snow July 2, 2019, 2:54 a.m. UTC | #1
On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote:
> In the "Read Array Flowchart" the command has a value of 0xFF.
> 
> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> command has a value of 0xff.
> 
> Use the correct value in the pflash model.
> 
> There is no change of behavior in the guest, because:
> - when the guest were sending 0xFF, the reset_flash label
>   was setting the command value as 0x00
> - 0x00 was used internally for READ_ARRAY
> 
> [*] "Common Flash Interface (CFI) and Command Sets"
>     (Intel Application Note 646)
>     Appendix B "Basic Command Set"
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I'm assuming you tested a guest and saw it sending 0xFF; and based
entirely on the assumption that you tested this and it is not now worse:

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

> ---
>  hw/block/pflash_cfi01.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index dcc9885bf0..743b5d5794 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -280,10 +280,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>          pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pfl->cmd = 0xff;
>          /* fall through to read code */
> -    case 0x00:
> -        /* Flash area read */
> +    case 0xff: /* Read Array */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
>      case 0x10: /* Single byte program */
> @@ -449,8 +448,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> -            goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
>              DPRINTF("%s: Single Byte Program\n", __func__);
> @@ -527,7 +524,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              if (cmd == 0xd0) { /* confirm */
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) { /* read array mode */
> +            } else if (cmd == 0xff) { /* Read Array */
>                  goto reset_flash;
>              } else
>                  goto error_flash;
> @@ -554,7 +551,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              } else if (cmd == 0x01) {
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) {
> +            } else if (cmd == 0xff) { /* read array mode */
>                  goto reset_flash;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> @@ -646,7 +643,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      trace_pflash_reset();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>  }
>  
>  
> @@ -762,7 +759,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>
Alistair Francis July 2, 2019, 3:59 p.m. UTC | #2
On Mon, Jul 1, 2019 at 5:14 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> In the "Read Array Flowchart" the command has a value of 0xFF.
>
> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> command has a value of 0xff.
>
> Use the correct value in the pflash model.
>
> There is no change of behavior in the guest, because:
> - when the guest were sending 0xFF, the reset_flash label
>   was setting the command value as 0x00
> - 0x00 was used internally for READ_ARRAY
>
> [*] "Common Flash Interface (CFI) and Command Sets"
>     (Intel Application Note 646)
>     Appendix B "Basic Command Set"
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

Alistair

> ---
>  hw/block/pflash_cfi01.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index dcc9885bf0..743b5d5794 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -280,10 +280,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>          pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pfl->cmd = 0xff;
>          /* fall through to read code */
> -    case 0x00:
> -        /* Flash area read */
> +    case 0xff: /* Read Array */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
>      case 0x10: /* Single byte program */
> @@ -449,8 +448,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> -            goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
>              DPRINTF("%s: Single Byte Program\n", __func__);
> @@ -527,7 +524,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              if (cmd == 0xd0) { /* confirm */
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) { /* read array mode */
> +            } else if (cmd == 0xff) { /* Read Array */
>                  goto reset_flash;
>              } else
>                  goto error_flash;
> @@ -554,7 +551,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              } else if (cmd == 0x01) {
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) {
> +            } else if (cmd == 0xff) { /* read array mode */
>                  goto reset_flash;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> @@ -646,7 +643,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      trace_pflash_reset();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>  }
>
>
> @@ -762,7 +759,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> --
> 2.20.1
>
diff mbox series

Patch

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index dcc9885bf0..743b5d5794 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -280,10 +280,9 @@  static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
         /* This should never happen : reset state & treat it as a read */
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
         pfl->wcycle = 0;
-        pfl->cmd = 0;
+        pfl->cmd = 0xff;
         /* fall through to read code */
-    case 0x00:
-        /* Flash area read */
+    case 0xff: /* Read Array */
         ret = pflash_data_read(pfl, offset, width, be);
         break;
     case 0x10: /* Single byte program */
@@ -449,8 +448,6 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     case 0:
         /* read mode */
         switch (cmd) {
-        case 0x00: /* ??? */
-            goto reset_flash;
         case 0x10: /* Single Byte Program */
         case 0x40: /* Single Byte Program */
             DPRINTF("%s: Single Byte Program\n", __func__);
@@ -527,7 +524,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             if (cmd == 0xd0) { /* confirm */
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
-            } else if (cmd == 0xff) { /* read array mode */
+            } else if (cmd == 0xff) { /* Read Array */
                 goto reset_flash;
             } else
                 goto error_flash;
@@ -554,7 +551,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             } else if (cmd == 0x01) {
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
-            } else if (cmd == 0xff) {
+            } else if (cmd == 0xff) { /* read array mode */
                 goto reset_flash;
             } else {
                 DPRINTF("%s: Unknown (un)locking command\n", __func__);
@@ -646,7 +643,7 @@  static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     trace_pflash_reset();
     memory_region_rom_device_set_romd(&pfl->mem, true);
     pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pfl->cmd = 0xff;
 }
 
 
@@ -762,7 +759,7 @@  static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pfl->cmd = 0xff;
     pfl->status = 0;
     /* Hardcoded CFI table */
     /* Standard "QRY" string */