[PATCH 0/9] hw/block/pflash: Refactors around setting the device in read-array mode

[PATCH 0/9] hw/block/pflash: Refactors around setting the device in read-array mode

[Philippe Mathieu-Daudé]

I remembered this almost 2 years old series while reviewing
David Edmondson's patches... (which I plan to apply on top).

Basically we move things around to make the code easier to maintain.

Please review :)

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  hw/block/pflash_cfi: Fix code style for checkpatch.pl
  hw/block/pflash_cfi01: Extract pflash_cfi01_fill_cfi_table()
  hw/block/pflash_cfi02: Extract pflash_cfi02_fill_cfi_table()
  hw/block/pflash_cfi02: Set rom_mode to true in pflash_setup_mappings()
  hw/block/pflash_cfi02: Open-code pflash_register_memory(rom=false)
  hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array
  hw/block/pflash_cfi02: Factor out DeviceReset method
  hw/block/pflash_cfi01: Clarify trace events
  hw/block/pflash_cfi01: Extract pflash_mode_read_array()

 hw/block/pflash_cfi01.c | 201 +++++++++++++++++----------------
 hw/block/pflash_cfi02.c | 238 +++++++++++++++++++++-------------------
 hw/block/trace-events   |   1 +
 3 files changed, 235 insertions(+), 205 deletions(-)

-- 
2.26.2

[PATCH 1/9] hw/block/pflash_cfi: Fix code style for checkpatch.pl

[Philippe Mathieu-Daudé]

We are going to move this code, fix its style first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++++++------------
 hw/block/pflash_cfi02.c |  9 ++++++---
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 22287a1522e..b6919bbe474 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -115,7 +115,8 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
-/* Perform a CFI query based on the bank width of the flash.
+/*
+ * 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.
  */
@@ -125,7 +126,8 @@ static uint32_t pflash_cfi_query(PFlashCFI01 *pfl, hwaddr offset)
     uint32_t resp = 0;
     hwaddr boff;
 
-    /* Adjust incoming offset to match expected device-width
+    /*
+     * Adjust incoming offset to match expected device-width
      * addressing. CFI query addresses are always specified in terms of
      * the maximum supported width of the device.  This means that x8
      * devices and x8/x16 devices in x8 mode behave differently.  For
@@ -141,7 +143,8 @@ static uint32_t pflash_cfi_query(PFlashCFI01 *pfl, hwaddr offset)
     if (boff >= sizeof(pfl->cfi_table)) {
         return 0;
     }
-    /* Now we will construct the CFI response generated by a single
+    /*
+     * Now we will construct the CFI response generated by a single
      * device, then replicate that for all devices that make up the
      * bus.  For wide parts used in x8 mode, CFI query responses
      * are different than native byte-wide parts.
@@ -185,7 +188,8 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr offset)
     uint32_t resp;
     hwaddr boff;
 
-    /* Adjust incoming offset to match expected device-width
+    /*
+     * Adjust incoming offset to match expected device-width
      * addressing. Device ID read addresses are always specified in
      * terms of the maximum supported width of the device.  This means
      * that x8 devices and x8/x16 devices in x8 mode behave
@@ -198,7 +202,8 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr offset)
     boff = offset >> (ctz32(pfl->bank_width) +
                       ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
 
-    /* Mask off upper bits which may be used in to query block
+    /*
+     * Mask off upper bits which may be used in to query block
      * or sector lock status at other addresses.
      * Offsets 2/3 are block lock status, is not emulated.
      */
@@ -297,7 +302,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
     case 0x60: /* Block /un)lock */
     case 0x70: /* Status Register */
     case 0xe8: /* Write block */
-        /* Status register read.  Return status from each device in
+        /*
+         * Status register read.  Return status from each device in
          * bank.
          */
         ret = pfl->status;
@@ -308,7 +314,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
                 shift += pfl->device_width * 8;
             }
         } else if (!pfl->device_width && width > 2) {
-            /* Handle 32 bit flash cases where device width is not
+            /*
+             * Handle 32 bit flash cases where device width is not
              * set. (Existing behavior before device width added.)
              */
             ret |= pfl->status << 16;
@@ -340,7 +347,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
                 break;
             }
         } else {
-            /* If we have a read larger than the bank_width, combine multiple
+            /*
+             * If we have a read larger than the bank_width, combine multiple
              * manufacturer/device ID queries into a single response.
              */
             int i;
@@ -367,7 +375,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
                 ret = 0;
             }
         } else {
-            /* If we have a read larger than the bank_width, combine multiple
+            /*
+             * If we have a read larger than the bank_width, combine multiple
              * CFI queries into a single response.
              */
             int i;
@@ -544,7 +553,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
 
             break;
         case 0xe8:
-            /* Mask writeblock size based on device width, or bank width if
+            /*
+             * Mask writeblock size based on device width, or bank width if
              * device width not specified.
              */
             /* FIXME check @offset, @width */
@@ -718,7 +728,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 
     total_len = pfl->sector_len * pfl->nb_blocs;
 
-    /* These are only used to expose the parameters of each device
+    /*
+     * These are only used to expose the parameters of each device
      * in the cfi_table[].
      */
     num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
@@ -763,7 +774,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    /* Default to devices being used at their maximum device width. This was
+    /*
+     * Default to devices being used at their maximum device width. This was
      * assumed before the device_width support was added.
      */
     if (!pfl->max_device_width) {
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 7962cff7455..fa981465e12 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -100,7 +100,8 @@ struct PFlashCFI02 {
     uint16_t unlock_addr1;
     uint8_t cfi_table[0x4d];
     QEMUTimer timer;
-    /* The device replicates the flash memory across its memory space.  Emulate
+    /*
+     * The device replicates the flash memory across its memory space.  Emulate
      * that by having a container (.mem) filled with an array of aliases
      * (.mem_mappings) pointing to the flash memory (.orig_mem).
      */
@@ -884,8 +885,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x28] = 0x02;
     pfl->cfi_table[0x29] = 0x00;
     /* Max number of bytes in multi-bytes write */
-    /* XXX: disable buffered write as it's not supported */
-    //    pfl->cfi_table[0x2A] = 0x05;
+    /*
+     * XXX: disable buffered write as it's not supported
+     * pfl->cfi_table[0x2A] = 0x05;
+     */
     pfl->cfi_table[0x2A] = 0x00;
     pfl->cfi_table[0x2B] = 0x00;
     /* Number of erase block regions */
-- 
2.26.2

[PATCH 2/9] hw/block/pflash_cfi01: Extract pflash_cfi01_fill_cfi_table()

[Philippe Mathieu-Daudé]

Fill the CFI table in out of DeviceRealize() in a new function:
pflash_cfi01_fill_cfi_table().

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

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index b6919bbe474..03472ea5b64 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -704,30 +704,11 @@ static const MemoryRegionOps pflash_cfi01_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
+static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
 {
-    ERRP_GUARD();
-    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
-    uint64_t total_len;
-    int ret;
     uint64_t blocks_per_device, sector_len_per_device, device_len;
     int num_devices;
 
-    if (pfl->sector_len == 0) {
-        error_setg(errp, "attribute \"sector-length\" not specified or zero.");
-        return;
-    }
-    if (pfl->nb_blocs == 0) {
-        error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
-        return;
-    }
-    if (pfl->name == NULL) {
-        error_setg(errp, "attribute \"name\" not specified.");
-        return;
-    }
-
-    total_len = pfl->sector_len * pfl->nb_blocs;
-
     /*
      * These are only used to expose the parameters of each device
      * in the cfi_table[].
@@ -742,53 +723,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
     device_len = sector_len_per_device * blocks_per_device;
 
-    memory_region_init_rom_device(
-        &pfl->mem, OBJECT(dev),
-        &pflash_cfi01_ops,
-        pfl,
-        pfl->name, total_len, errp);
-    if (*errp) {
-        return;
-    }
-
-    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
-
-    if (pfl->blk) {
-        uint64_t perm;
-        pfl->ro = !blk_supports_write_perm(pfl->blk);
-        perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
-        ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
-        if (ret < 0) {
-            return;
-        }
-    } else {
-        pfl->ro = 0;
-    }
-
-    if (pfl->blk) {
-        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
-                                         errp)) {
-            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
-            return;
-        }
-    }
-
-    /*
-     * Default to devices being used at their maximum device width. This was
-     * assumed before the device_width support was added.
-     */
-    if (!pfl->max_device_width) {
-        pfl->max_device_width = pfl->device_width;
-    }
-
-    pfl->wcycle = 0;
-    /*
-     * The command 0x00 is not assigned by the CFI open standard,
-     * but QEMU historically uses it for the READ_ARRAY command (0xff).
-     */
-    pfl->cmd = 0x00;
-    pfl->status = 0x80; /* WSM ready */
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
@@ -876,6 +810,78 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 }
 
+static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
+    uint64_t total_len;
+    int ret;
+
+    if (pfl->sector_len == 0) {
+        error_setg(errp, "attribute \"sector-length\" not specified or zero.");
+        return;
+    }
+    if (pfl->nb_blocs == 0) {
+        error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
+        return;
+    }
+    if (pfl->name == NULL) {
+        error_setg(errp, "attribute \"name\" not specified.");
+        return;
+    }
+
+    total_len = pfl->sector_len * pfl->nb_blocs;
+
+    memory_region_init_rom_device(
+        &pfl->mem, OBJECT(dev),
+        &pflash_cfi01_ops,
+        pfl,
+        pfl->name, total_len, errp);
+    if (*errp) {
+        return;
+    }
+
+    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
+
+    if (pfl->blk) {
+        uint64_t perm;
+        pfl->ro = !blk_supports_write_perm(pfl->blk);
+        perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
+        ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
+        if (ret < 0) {
+            return;
+        }
+    } else {
+        pfl->ro = 0;
+    }
+
+    if (pfl->blk) {
+        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
+                                         errp)) {
+            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
+            return;
+        }
+    }
+
+    /*
+     * Default to devices being used at their maximum device width. This was
+     * assumed before the device_width support was added.
+     */
+    if (!pfl->max_device_width) {
+        pfl->max_device_width = pfl->device_width;
+    }
+
+    pfl->wcycle = 0;
+    /*
+     * The command 0x00 is not assigned by the CFI open standard,
+     * but QEMU historically uses it for the READ_ARRAY command (0xff).
+     */
+    pfl->cmd = 0x00;
+    pfl->status = 0x80; /* WSM ready */
+    pflash_cfi01_fill_cfi_table(pfl);
+}
+
 static void pflash_cfi01_system_reset(DeviceState *dev)
 {
     PFlashCFI01 *pfl = PFLASH_CFI01(dev);
-- 
2.26.2

[PATCH 3/9] hw/block/pflash_cfi02: Extract pflash_cfi02_fill_cfi_table()

[Philippe Mathieu-Daudé]

Fill the CFI table in out of DeviceRealize() in a new function:
pflash_cfi02_fill_cfi_table().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 193 +++++++++++++++++++++-------------------
 1 file changed, 99 insertions(+), 94 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index fa981465e12..845f50ed99b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -724,6 +724,104 @@ static const MemoryRegionOps pflash_cfi02_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void pflash_cfi02_fill_cfi_table(PFlashCFI02 *pfl, int nb_regions)
+{
+    /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
+    const uint16_t pri_ofs = 0x40;
+    /* Standard "QRY" string */
+    pfl->cfi_table[0x10] = 'Q';
+    pfl->cfi_table[0x11] = 'R';
+    pfl->cfi_table[0x12] = 'Y';
+    /* Command set (AMD/Fujitsu) */
+    pfl->cfi_table[0x13] = 0x02;
+    pfl->cfi_table[0x14] = 0x00;
+    /* Primary extended table address */
+    pfl->cfi_table[0x15] = pri_ofs;
+    pfl->cfi_table[0x16] = pri_ofs >> 8;
+    /* Alternate command set (none) */
+    pfl->cfi_table[0x17] = 0x00;
+    pfl->cfi_table[0x18] = 0x00;
+    /* Alternate extended table (none) */
+    pfl->cfi_table[0x19] = 0x00;
+    pfl->cfi_table[0x1A] = 0x00;
+    /* Vcc min */
+    pfl->cfi_table[0x1B] = 0x27;
+    /* Vcc max */
+    pfl->cfi_table[0x1C] = 0x36;
+    /* Vpp min (no Vpp pin) */
+    pfl->cfi_table[0x1D] = 0x00;
+    /* Vpp max (no Vpp pin) */
+    pfl->cfi_table[0x1E] = 0x00;
+    /* Timeout per single byte/word write (128 ms) */
+    pfl->cfi_table[0x1F] = 0x07;
+    /* Timeout for min size buffer write (NA) */
+    pfl->cfi_table[0x20] = 0x00;
+    /* Typical timeout for block erase (512 ms) */
+    pfl->cfi_table[0x21] = 0x09;
+    /* Typical timeout for full chip erase (4096 ms) */
+    pfl->cfi_table[0x22] = 0x0C;
+    /* Reserved */
+    pfl->cfi_table[0x23] = 0x01;
+    /* Max timeout for buffer write (NA) */
+    pfl->cfi_table[0x24] = 0x00;
+    /* Max timeout for block erase */
+    pfl->cfi_table[0x25] = 0x0A;
+    /* Max timeout for chip erase */
+    pfl->cfi_table[0x26] = 0x0D;
+    /* Device size */
+    pfl->cfi_table[0x27] = ctz32(pfl->chip_len);
+    /* Flash device interface (8 & 16 bits) */
+    pfl->cfi_table[0x28] = 0x02;
+    pfl->cfi_table[0x29] = 0x00;
+    /* Max number of bytes in multi-bytes write */
+    /*
+     * XXX: disable buffered write as it's not supported
+     * pfl->cfi_table[0x2A] = 0x05;
+     */
+    pfl->cfi_table[0x2A] = 0x00;
+    pfl->cfi_table[0x2B] = 0x00;
+    /* Number of erase block regions */
+    pfl->cfi_table[0x2c] = nb_regions;
+    /* Erase block regions */
+    for (int i = 0; i < nb_regions; ++i) {
+        uint32_t sector_len_per_device = pfl->sector_len[i];
+        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
+        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
+        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
+        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
+    }
+    assert(0x2c + 4 * nb_regions < pri_ofs);
+
+    /* Extended */
+    pfl->cfi_table[0x00 + pri_ofs] = 'P';
+    pfl->cfi_table[0x01 + pri_ofs] = 'R';
+    pfl->cfi_table[0x02 + pri_ofs] = 'I';
+
+    /* Extended version 1.0 */
+    pfl->cfi_table[0x03 + pri_ofs] = '1';
+    pfl->cfi_table[0x04 + pri_ofs] = '0';
+
+    /* Address sensitive unlock required. */
+    pfl->cfi_table[0x05 + pri_ofs] = 0x00;
+    /* Erase suspend to read/write. */
+    pfl->cfi_table[0x06 + pri_ofs] = 0x02;
+    /* Sector protect not supported. */
+    pfl->cfi_table[0x07 + pri_ofs] = 0x00;
+    /* Temporary sector unprotect not supported. */
+    pfl->cfi_table[0x08 + pri_ofs] = 0x00;
+
+    /* Sector protect/unprotect scheme. */
+    pfl->cfi_table[0x09 + pri_ofs] = 0x00;
+
+    /* Simultaneous operation not supported. */
+    pfl->cfi_table[0x0a + pri_ofs] = 0x00;
+    /* Burst mode not supported. */
+    pfl->cfi_table[0x0b + pri_ofs] = 0x00;
+    /* Page mode not supported. */
+    pfl->cfi_table[0x0c + pri_ofs] = 0x00;
+    assert(0x0c + pri_ofs < ARRAY_SIZE(pfl->cfi_table));
+}
+
 static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
 {
     ERRP_GUARD();
@@ -837,100 +935,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->cmd = 0;
     pfl->status = 0;
 
-    /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
-    const uint16_t pri_ofs = 0x40;
-    /* Standard "QRY" string */
-    pfl->cfi_table[0x10] = 'Q';
-    pfl->cfi_table[0x11] = 'R';
-    pfl->cfi_table[0x12] = 'Y';
-    /* Command set (AMD/Fujitsu) */
-    pfl->cfi_table[0x13] = 0x02;
-    pfl->cfi_table[0x14] = 0x00;
-    /* Primary extended table address */
-    pfl->cfi_table[0x15] = pri_ofs;
-    pfl->cfi_table[0x16] = pri_ofs >> 8;
-    /* Alternate command set (none) */
-    pfl->cfi_table[0x17] = 0x00;
-    pfl->cfi_table[0x18] = 0x00;
-    /* Alternate extended table (none) */
-    pfl->cfi_table[0x19] = 0x00;
-    pfl->cfi_table[0x1A] = 0x00;
-    /* Vcc min */
-    pfl->cfi_table[0x1B] = 0x27;
-    /* Vcc max */
-    pfl->cfi_table[0x1C] = 0x36;
-    /* Vpp min (no Vpp pin) */
-    pfl->cfi_table[0x1D] = 0x00;
-    /* Vpp max (no Vpp pin) */
-    pfl->cfi_table[0x1E] = 0x00;
-    /* Timeout per single byte/word write (128 ms) */
-    pfl->cfi_table[0x1F] = 0x07;
-    /* Timeout for min size buffer write (NA) */
-    pfl->cfi_table[0x20] = 0x00;
-    /* Typical timeout for block erase (512 ms) */
-    pfl->cfi_table[0x21] = 0x09;
-    /* Typical timeout for full chip erase (4096 ms) */
-    pfl->cfi_table[0x22] = 0x0C;
-    /* Reserved */
-    pfl->cfi_table[0x23] = 0x01;
-    /* Max timeout for buffer write (NA) */
-    pfl->cfi_table[0x24] = 0x00;
-    /* Max timeout for block erase */
-    pfl->cfi_table[0x25] = 0x0A;
-    /* Max timeout for chip erase */
-    pfl->cfi_table[0x26] = 0x0D;
-    /* Device size */
-    pfl->cfi_table[0x27] = ctz32(pfl->chip_len);
-    /* Flash device interface (8 & 16 bits) */
-    pfl->cfi_table[0x28] = 0x02;
-    pfl->cfi_table[0x29] = 0x00;
-    /* Max number of bytes in multi-bytes write */
-    /*
-     * XXX: disable buffered write as it's not supported
-     * pfl->cfi_table[0x2A] = 0x05;
-     */
-    pfl->cfi_table[0x2A] = 0x00;
-    pfl->cfi_table[0x2B] = 0x00;
-    /* Number of erase block regions */
-    pfl->cfi_table[0x2c] = nb_regions;
-    /* Erase block regions */
-    for (int i = 0; i < nb_regions; ++i) {
-        uint32_t sector_len_per_device = pfl->sector_len[i];
-        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
-        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
-        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
-        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
-    }
-    assert(0x2c + 4 * nb_regions < pri_ofs);
-
-    /* Extended */
-    pfl->cfi_table[0x00 + pri_ofs] = 'P';
-    pfl->cfi_table[0x01 + pri_ofs] = 'R';
-    pfl->cfi_table[0x02 + pri_ofs] = 'I';
-
-    /* Extended version 1.0 */
-    pfl->cfi_table[0x03 + pri_ofs] = '1';
-    pfl->cfi_table[0x04 + pri_ofs] = '0';
-
-    /* Address sensitive unlock required. */
-    pfl->cfi_table[0x05 + pri_ofs] = 0x00;
-    /* Erase suspend to read/write. */
-    pfl->cfi_table[0x06 + pri_ofs] = 0x02;
-    /* Sector protect not supported. */
-    pfl->cfi_table[0x07 + pri_ofs] = 0x00;
-    /* Temporary sector unprotect not supported. */
-    pfl->cfi_table[0x08 + pri_ofs] = 0x00;
-
-    /* Sector protect/unprotect scheme. */
-    pfl->cfi_table[0x09 + pri_ofs] = 0x00;
-
-    /* Simultaneous operation not supported. */
-    pfl->cfi_table[0x0a + pri_ofs] = 0x00;
-    /* Burst mode not supported. */
-    pfl->cfi_table[0x0b + pri_ofs] = 0x00;
-    /* Page mode not supported. */
-    pfl->cfi_table[0x0c + pri_ofs] = 0x00;
-    assert(0x0c + pri_ofs < ARRAY_SIZE(pfl->cfi_table));
+    pflash_cfi02_fill_cfi_table(pfl, nb_regions);
 }
 
 static Property pflash_cfi02_properties[] = {
-- 
2.26.2

[PATCH 4/9] hw/block/pflash_cfi02: Set rom_mode to true in pflash_setup_mappings()

[Philippe Mathieu-Daudé]

There is only one call to pflash_setup_mappings(). Convert 'rom_mode'
to boolean and set it to true directly within pflash_setup_mappings().

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

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 845f50ed99b..5f949b4c792 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -108,7 +108,7 @@ struct PFlashCFI02 {
     MemoryRegion mem;
     MemoryRegion *mem_mappings;    /* array; one per mapping */
     MemoryRegion orig_mem;
-    int rom_mode;
+    bool rom_mode;
     int read_counter; /* used for lazy switch-back to rom mode */
     int sectors_to_erase;
     uint64_t erase_time_remaining;
@@ -181,6 +181,7 @@ static void pflash_setup_mappings(PFlashCFI02 *pfl)
                                  "pflash-alias", &pfl->orig_mem, 0, size);
         memory_region_add_subregion(&pfl->mem, i * size, &pfl->mem_mappings[i]);
     }
+    pfl->rom_mode = true;
 }
 
 static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
@@ -927,7 +928,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->sector_erase_map = bitmap_new(pfl->total_sectors);
 
     pflash_setup_mappings(pfl);
-    pfl->rom_mode = 1;
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
-- 
2.26.2

[PATCH 5/9] hw/block/pflash_cfi02: Open-code pflash_register_memory(rom=false)

[Philippe Mathieu-Daudé]

There is only one call to pflash_register_memory() with
rom_mode == false. As we want to modify pflash_register_memory()
in the next patch, open-code this trivial function in place for
the 'rom_mode == false' case.

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

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 5f949b4c792..4efbae2f0c9 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -467,8 +467,10 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
     switch (pfl->wcycle) {
     case 0:
         /* Set the device in I/O access mode if required */
-        if (pfl->rom_mode)
-            pflash_register_memory(pfl, 0);
+        if (pfl->rom_mode) {
+            pfl->rom_mode = false;
+            memory_region_rom_device_set_romd(&pfl->orig_mem, false);
+        }
         pfl->read_counter = 0;
         /* We're in read mode */
     check_unlock0:
-- 
2.26.2

[PATCH 6/9] hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array

[Philippe Mathieu-Daudé]

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 three times:

- When the timer expires and not in bypass mode

- On a read access (on invalid command).

- When the device is initialized. Here the ROMD mode is hidden
  by the memory_region_init_rom_device() call.

pflash_register_memory(rom_mode=true) already sets the ROM device
in "read array" mode (from I/O device to ROM one). Explicit that
by renaming the function as pflash_mode_read_array(), adding
a trace event and resetting wcycle.

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

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4efbae2f0c9..2ba77a0171b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -184,10 +184,13 @@ static void pflash_setup_mappings(PFlashCFI02 *pfl)
     pfl->rom_mode = true;
 }
 
-static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
+static void pflash_mode_read_array(PFlashCFI02 *pfl)
 {
-    memory_region_rom_device_set_romd(&pfl->orig_mem, rom_mode);
-    pfl->rom_mode = rom_mode;
+    trace_pflash_mode_read_array();
+    pfl->cmd = 0x00;
+    pfl->wcycle = 0;
+    pfl->rom_mode = true;
+    memory_region_rom_device_set_romd(&pfl->orig_mem, true);
 }
 
 static size_t pflash_regions_count(PFlashCFI02 *pfl)
@@ -249,11 +252,10 @@ static void pflash_timer(void *opaque)
     toggle_dq7(pfl);
     if (pfl->bypass) {
         pfl->wcycle = 2;
+        pfl->cmd = 0;
     } else {
-        pflash_register_memory(pfl, 1);
-        pfl->wcycle = 0;
+        pflash_mode_read_array(pfl);
     }
-    pfl->cmd = 0;
 }
 
 /*
@@ -315,7 +317,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
     /* Lazy reset to ROMD mode after a certain amount of read accesses */
     if (!pfl->rom_mode && pfl->wcycle == 0 &&
         ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
-        pflash_register_memory(pfl, 1);
+        pflash_mode_read_array(pfl);
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;
@@ -933,8 +935,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
     pfl->status = 0;
 
     pflash_cfi02_fill_cfi_table(pfl, nb_regions);
diff --git a/hw/block/trace-events b/hw/block/trace-events
index d32475c3989..f16d6e90cfd 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_cfi01.c
 # pflash_cfi02.c
 pflash_reset(void) "reset"
+pflash_mode_read_array(void) "mode: read array"
 pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
 pflash_io_read(uint64_t offset, unsigned size, uint32_t value, uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" size:%u value:0x%04x cmd:0x%02x wcycle:%u"
 pflash_io_write(uint64_t offset, unsigned size, uint32_t value, uint8_t wcycle) "offset:0x%04"PRIx64" size:%u value:0x%04x wcycle:%u"
-- 
2.26.2

[PATCH 7/9] hw/block/pflash_cfi02: Factor out DeviceReset method

[Philippe Mathieu-Daudé]

There is multiple places doing a device reset. Factor that
out in a common method which matches the DeviceReset prototype,
so we can also remove the reset code from the DeviceRealize()
handler. Explicit the device is set in "read array" mode on
reset.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2ba77a0171b..484b056b898 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -193,6 +193,14 @@ static void pflash_mode_read_array(PFlashCFI02 *pfl)
     memory_region_rom_device_set_romd(&pfl->orig_mem, true);
 }
 
+static void pflash_cfi02_reset(DeviceState *dev)
+{
+    PFlashCFI02 *pfl = PFLASH_CFI02(dev);
+
+    trace_pflash_reset();
+    pflash_mode_read_array(pfl);
+}
+
 static size_t pflash_regions_count(PFlashCFI02 *pfl)
 {
     return pfl->cfi_table[0x2c];
@@ -330,8 +338,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
     default:
         /* 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;
+        pflash_cfi02_reset(DEVICE(pfl));
         /* fall through to the read code */
     case 0x80: /* Erase (unlock) */
         /* We accept reads during second unlock sequence... */
@@ -710,10 +717,8 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
 
     /* Reset flash */
  reset_flash:
-    trace_pflash_reset();
     pfl->bypass = 0;
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pflash_cfi02_reset(DEVICE(pfl));
     return;
 
  do_bypass:
@@ -977,6 +982,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pflash_cfi02_realize;
+    dc->reset = pflash_cfi02_reset;
     dc->unrealize = pflash_cfi02_unrealize;
     device_class_set_props(dc, pflash_cfi02_properties);
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-- 
2.26.2

[PATCH 8/9] hw/block/pflash_cfi01: Clarify trace events

[Philippe Mathieu-Daudé]

Use the 'mode_read_array' event when we set the device in such
mode, and use the 'reset' event in DeviceReset handler.

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

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 03472ea5b64..2618e00926d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -663,7 +663,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
  mode_read_array:
-    trace_pflash_reset();
+    trace_pflash_mode_read_array();
     memory_region_rom_device_set_romd(&pfl->mem, true);
     pfl->wcycle = 0;
     pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
@@ -886,6 +886,7 @@ static void pflash_cfi01_system_reset(DeviceState *dev)
 {
     PFlashCFI01 *pfl = PFLASH_CFI01(dev);
 
+    trace_pflash_reset();
     /*
      * The command 0x00 is not assigned by the CFI open standard,
      * but QEMU historically uses it for the READ_ARRAY command (0xff).
-- 
2.26.2

[PATCH 9/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()

[Philippe Mathieu-Daudé]

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 three times:

- On a read access (on invalid command).

- 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.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 2618e00926d..32c9b289715 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -115,6 +115,19 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_mode_read_array(PFlashCFI01 *pfl)
+{
+    trace_pflash_mode_read_array();
+    /*
+     * The command 0x00 is not assigned by the CFI open standard,
+     * but QEMU historically uses it for the READ_ARRAY command (0xff).
+     */
+    trace_pflash_mode_read_array();
+    pfl->cmd = 0x00;
+    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
@@ -283,12 +296,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
     default:
         /* This should never happen : reset state & treat it as a read */
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
-        pfl->wcycle = 0;
-        /*
-         * The command 0x00 is not assigned by the CFI open standard,
-         * but QEMU historically uses it for the READ_ARRAY command (0xff).
-         */
-        pfl->cmd = 0x00;
+        pflash_mode_read_array(pfl);
         /* fall through to read code */
     case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
         /* Flash area read */
@@ -663,10 +671,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
  mode_read_array:
-    trace_pflash_mode_read_array();
-    memory_region_rom_device_set_romd(&pfl->mem, true);
-    pfl->wcycle = 0;
-    pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
+    pflash_mode_read_array(pfl);
 }
 
 
@@ -872,13 +877,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    /*
-     * The command 0x00 is not assigned by the CFI open standard,
-     * but QEMU historically uses it for the READ_ARRAY command (0xff).
-     */
-    pfl->cmd = 0x00;
     pfl->status = 0x80; /* WSM ready */
+    pflash_mode_read_array(pfl);
     pflash_cfi01_fill_cfi_table(pfl);
 }
 
@@ -887,13 +887,7 @@ static void pflash_cfi01_system_reset(DeviceState *dev)
     PFlashCFI01 *pfl = PFLASH_CFI01(dev);
 
     trace_pflash_reset();
-    /*
-     * The command 0x00 is not assigned by the CFI open standard,
-     * but QEMU historically uses it for the READ_ARRAY command (0xff).
-     */
-    pfl->cmd = 0x00;
-    pfl->wcycle = 0;
-    memory_region_rom_device_set_romd(&pfl->mem, true);
+    pflash_mode_read_array(pfl);
     /*
      * The WSM ready timer occurs at most 150ns after system reset.
      * This model deliberately ignores this delay.
-- 
2.26.2

Re: [PATCH 1/9] hw/block/pflash_cfi: Fix code style for checkpatch.pl

[Bin Meng]

On Wed, Mar 10, 2021 at 7:50 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> We are going to move this code, fix its style first.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++++++------------
>  hw/block/pflash_cfi02.c |  9 ++++++---
>  2 files changed, 30 insertions(+), 15 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 2/9] hw/block/pflash_cfi01: Extract pflash_cfi01_fill_cfi_table()

[Bin Meng]

On Wed, Mar 10, 2021 at 7:51 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Fill the CFI table in out of DeviceRealize() in a new function:
> pflash_cfi01_fill_cfi_table().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 140 +++++++++++++++++++++-------------------
>  1 file changed, 73 insertions(+), 67 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 3/9] hw/block/pflash_cfi02: Extract pflash_cfi02_fill_cfi_table()

[Bin Meng]

On Wed, Mar 10, 2021 at 7:52 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Fill the CFI table in out of DeviceRealize() in a new function:
> pflash_cfi02_fill_cfi_table().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 193 +++++++++++++++++++++-------------------
>  1 file changed, 99 insertions(+), 94 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 4/9] hw/block/pflash_cfi02: Set rom_mode to true in pflash_setup_mappings()

[Bin Meng]

On Wed, Mar 10, 2021 at 7:54 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> There is only one call to pflash_setup_mappings(). Convert 'rom_mode'
> to boolean and set it to true directly within pflash_setup_mappings().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 5/9] hw/block/pflash_cfi02: Open-code pflash_register_memory(rom=false)

[Bin Meng]

On Wed, Mar 10, 2021 at 7:51 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> There is only one call to pflash_register_memory() with
> rom_mode == false. As we want to modify pflash_register_memory()
> in the next patch, open-code this trivial function in place for
> the 'rom_mode == false' case.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 6/9] hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array

[Bin Meng]

On Wed, Mar 10, 2021 at 7:53 AM 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 three times:
>
> - When the timer expires and not in bypass mode
>
> - On a read access (on invalid command).
>
> - When the device is initialized. Here the ROMD mode is hidden
>   by the memory_region_init_rom_device() call.
>
> pflash_register_memory(rom_mode=true) already sets the ROM device
> in "read array" mode (from I/O device to ROM one). Explicit that
> by renaming the function as pflash_mode_read_array(), adding
> a trace event and resetting wcycle.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 18 +++++++++---------
>  hw/block/trace-events   |  1 +
>  2 files changed, 10 insertions(+), 9 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 7/9] hw/block/pflash_cfi02: Factor out DeviceReset method

[Bin Meng]

On Wed, Mar 10, 2021 at 7:55 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> There is multiple places doing a device reset. Factor that
> out in a common method which matches the DeviceReset prototype,
> so we can also remove the reset code from the DeviceRealize()
> handler. Explicit the device is set in "read array" mode on
> reset.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 2ba77a0171b..484b056b898 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -193,6 +193,14 @@ static void pflash_mode_read_array(PFlashCFI02 *pfl)
>      memory_region_rom_device_set_romd(&pfl->orig_mem, true);
>  }
>
> +static void pflash_cfi02_reset(DeviceState *dev)
> +{
> +    PFlashCFI02 *pfl = PFLASH_CFI02(dev);
> +
> +    trace_pflash_reset();
> +    pflash_mode_read_array(pfl);
> +}
> +
>  static size_t pflash_regions_count(PFlashCFI02 *pfl)
>  {
>      return pfl->cfi_table[0x2c];
> @@ -330,8 +338,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>      default:
>          /* 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;
> +        pflash_cfi02_reset(DEVICE(pfl));
>          /* fall through to the read code */
>      case 0x80: /* Erase (unlock) */
>          /* We accept reads during second unlock sequence... */
> @@ -710,10 +717,8 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>
>      /* Reset flash */
>   reset_flash:
> -    trace_pflash_reset();
>      pfl->bypass = 0;
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_cfi02_reset(DEVICE(pfl));

The old codes did not set pfl->rom_mode to true, but the new codes
pflash_cfi02_reset() do. Is this correct?

>      return;
>
>   do_bypass:
> @@ -977,6 +982,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->realize = pflash_cfi02_realize;
> +    dc->reset = pflash_cfi02_reset;
>      dc->unrealize = pflash_cfi02_unrealize;
>      device_class_set_props(dc, pflash_cfi02_properties);
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> --

Regards,
Bin

Re: [PATCH 8/9] hw/block/pflash_cfi01: Clarify trace events

[Bin Meng]

On Wed, Mar 10, 2021 at 7:53 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Use the 'mode_read_array' event when we set the device in such
> mode, and use the 'reset' event in DeviceReset handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 1/9] hw/block/pflash_cfi: Fix code style for checkpatch.pl

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:20 +01, Philippe Mathieu-Daudé wrote:

> We are going to move this code, fix its style first.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++++++------------
>  hw/block/pflash_cfi02.c |  9 ++++++---
>  2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 22287a1522e..b6919bbe474 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -115,7 +115,8 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> -/* Perform a CFI query based on the bank width of the flash.
> +/*
> + * 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.
>   */
> @@ -125,7 +126,8 @@ static uint32_t pflash_cfi_query(PFlashCFI01 *pfl, hwaddr offset)
>      uint32_t resp = 0;
>      hwaddr boff;
>  
> -    /* Adjust incoming offset to match expected device-width
> +    /*
> +     * Adjust incoming offset to match expected device-width
>       * addressing. CFI query addresses are always specified in terms of
>       * the maximum supported width of the device.  This means that x8
>       * devices and x8/x16 devices in x8 mode behave differently.  For
> @@ -141,7 +143,8 @@ static uint32_t pflash_cfi_query(PFlashCFI01 *pfl, hwaddr offset)
>      if (boff >= sizeof(pfl->cfi_table)) {
>          return 0;
>      }
> -    /* Now we will construct the CFI response generated by a single
> +    /*
> +     * Now we will construct the CFI response generated by a single
>       * device, then replicate that for all devices that make up the
>       * bus.  For wide parts used in x8 mode, CFI query responses
>       * are different than native byte-wide parts.
> @@ -185,7 +188,8 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr offset)
>      uint32_t resp;
>      hwaddr boff;
>  
> -    /* Adjust incoming offset to match expected device-width
> +    /*
> +     * Adjust incoming offset to match expected device-width
>       * addressing. Device ID read addresses are always specified in
>       * terms of the maximum supported width of the device.  This means
>       * that x8 devices and x8/x16 devices in x8 mode behave
> @@ -198,7 +202,8 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr offset)
>      boff = offset >> (ctz32(pfl->bank_width) +
>                        ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
>  
> -    /* Mask off upper bits which may be used in to query block
> +    /*
> +     * Mask off upper bits which may be used in to query block
>       * or sector lock status at other addresses.
>       * Offsets 2/3 are block lock status, is not emulated.
>       */
> @@ -297,7 +302,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>      case 0x60: /* Block /un)lock */
>      case 0x70: /* Status Register */
>      case 0xe8: /* Write block */
> -        /* Status register read.  Return status from each device in
> +        /*
> +         * Status register read.  Return status from each device in
>           * bank.
>           */
>          ret = pfl->status;
> @@ -308,7 +314,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>                  shift += pfl->device_width * 8;
>              }
>          } else if (!pfl->device_width && width > 2) {
> -            /* Handle 32 bit flash cases where device width is not
> +            /*
> +             * Handle 32 bit flash cases where device width is not
>               * set. (Existing behavior before device width added.)
>               */
>              ret |= pfl->status << 16;
> @@ -340,7 +347,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>                  break;
>              }
>          } else {
> -            /* If we have a read larger than the bank_width, combine multiple
> +            /*
> +             * If we have a read larger than the bank_width, combine multiple
>               * manufacturer/device ID queries into a single response.
>               */
>              int i;
> @@ -367,7 +375,8 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>                  ret = 0;
>              }
>          } else {
> -            /* If we have a read larger than the bank_width, combine multiple
> +            /*
> +             * If we have a read larger than the bank_width, combine multiple
>               * CFI queries into a single response.
>               */
>              int i;
> @@ -544,7 +553,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>  
>              break;
>          case 0xe8:
> -            /* Mask writeblock size based on device width, or bank width if
> +            /*
> +             * Mask writeblock size based on device width, or bank width if
>               * device width not specified.
>               */
>              /* FIXME check @offset, @width */
> @@ -718,7 +728,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>  
>      total_len = pfl->sector_len * pfl->nb_blocs;
>  
> -    /* These are only used to expose the parameters of each device
> +    /*
> +     * These are only used to expose the parameters of each device
>       * in the cfi_table[].
>       */
>      num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
> @@ -763,7 +774,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    /* Default to devices being used at their maximum device width. This was
> +    /*
> +     * Default to devices being used at their maximum device width. This was
>       * assumed before the device_width support was added.
>       */
>      if (!pfl->max_device_width) {
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 7962cff7455..fa981465e12 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -100,7 +100,8 @@ struct PFlashCFI02 {
>      uint16_t unlock_addr1;
>      uint8_t cfi_table[0x4d];
>      QEMUTimer timer;
> -    /* The device replicates the flash memory across its memory space.  Emulate
> +    /*
> +     * The device replicates the flash memory across its memory space.  Emulate
>       * that by having a container (.mem) filled with an array of aliases
>       * (.mem_mappings) pointing to the flash memory (.orig_mem).
>       */
> @@ -884,8 +885,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->cfi_table[0x28] = 0x02;
>      pfl->cfi_table[0x29] = 0x00;
>      /* Max number of bytes in multi-bytes write */
> -    /* XXX: disable buffered write as it's not supported */
> -    //    pfl->cfi_table[0x2A] = 0x05;
> +    /*
> +     * XXX: disable buffered write as it's not supported
> +     * pfl->cfi_table[0x2A] = 0x05;
> +     */
>      pfl->cfi_table[0x2A] = 0x00;
>      pfl->cfi_table[0x2B] = 0x00;
>      /* Number of erase block regions */
> -- 
> 2.26.2

dme.
-- 
You're like my yo-yo, that glowed in the dark.

Re: [PATCH 2/9] hw/block/pflash_cfi01: Extract pflash_cfi01_fill_cfi_table()

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:21 +01, Philippe Mathieu-Daudé wrote:

> Fill the CFI table in out of DeviceRealize() in a new function:
> pflash_cfi01_fill_cfi_table().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi01.c | 140 +++++++++++++++++++++-------------------
>  1 file changed, 73 insertions(+), 67 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index b6919bbe474..03472ea5b64 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -704,30 +704,11 @@ static const MemoryRegionOps pflash_cfi01_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> +static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
>  {
> -    ERRP_GUARD();
> -    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
> -    uint64_t total_len;
> -    int ret;
>      uint64_t blocks_per_device, sector_len_per_device, device_len;
>      int num_devices;
>  
> -    if (pfl->sector_len == 0) {
> -        error_setg(errp, "attribute \"sector-length\" not specified or zero.");
> -        return;
> -    }
> -    if (pfl->nb_blocs == 0) {
> -        error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
> -        return;
> -    }
> -    if (pfl->name == NULL) {
> -        error_setg(errp, "attribute \"name\" not specified.");
> -        return;
> -    }
> -
> -    total_len = pfl->sector_len * pfl->nb_blocs;
> -
>      /*
>       * These are only used to expose the parameters of each device
>       * in the cfi_table[].
> @@ -742,53 +723,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>      device_len = sector_len_per_device * blocks_per_device;
>  
> -    memory_region_init_rom_device(
> -        &pfl->mem, OBJECT(dev),
> -        &pflash_cfi01_ops,
> -        pfl,
> -        pfl->name, total_len, errp);
> -    if (*errp) {
> -        return;
> -    }
> -
> -    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> -
> -    if (pfl->blk) {
> -        uint64_t perm;
> -        pfl->ro = !blk_supports_write_perm(pfl->blk);
> -        perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
> -        ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
> -        if (ret < 0) {
> -            return;
> -        }
> -    } else {
> -        pfl->ro = 0;
> -    }
> -
> -    if (pfl->blk) {
> -        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
> -                                         errp)) {
> -            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> -            return;
> -        }
> -    }
> -
> -    /*
> -     * Default to devices being used at their maximum device width. This was
> -     * assumed before the device_width support was added.
> -     */
> -    if (!pfl->max_device_width) {
> -        pfl->max_device_width = pfl->device_width;
> -    }
> -
> -    pfl->wcycle = 0;
> -    /*
> -     * The command 0x00 is not assigned by the CFI open standard,
> -     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -     */
> -    pfl->cmd = 0x00;
> -    pfl->status = 0x80; /* WSM ready */
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> @@ -876,6 +810,78 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>  }
>  
> +static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
> +    uint64_t total_len;
> +    int ret;
> +
> +    if (pfl->sector_len == 0) {
> +        error_setg(errp, "attribute \"sector-length\" not specified or zero.");
> +        return;
> +    }
> +    if (pfl->nb_blocs == 0) {
> +        error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
> +        return;
> +    }
> +    if (pfl->name == NULL) {
> +        error_setg(errp, "attribute \"name\" not specified.");
> +        return;
> +    }
> +
> +    total_len = pfl->sector_len * pfl->nb_blocs;
> +
> +    memory_region_init_rom_device(
> +        &pfl->mem, OBJECT(dev),
> +        &pflash_cfi01_ops,
> +        pfl,
> +        pfl->name, total_len, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> +
> +    if (pfl->blk) {
> +        uint64_t perm;
> +        pfl->ro = !blk_supports_write_perm(pfl->blk);
> +        perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
> +        ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
> +        if (ret < 0) {
> +            return;
> +        }
> +    } else {
> +        pfl->ro = 0;
> +    }
> +
> +    if (pfl->blk) {
> +        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
> +                                         errp)) {
> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> +            return;
> +        }
> +    }
> +
> +    /*
> +     * Default to devices being used at their maximum device width. This was
> +     * assumed before the device_width support was added.
> +     */
> +    if (!pfl->max_device_width) {
> +        pfl->max_device_width = pfl->device_width;
> +    }
> +
> +    pfl->wcycle = 0;
> +    /*
> +     * The command 0x00 is not assigned by the CFI open standard,
> +     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +     */
> +    pfl->cmd = 0x00;
> +    pfl->status = 0x80; /* WSM ready */
> +    pflash_cfi01_fill_cfi_table(pfl);
> +}
> +
>  static void pflash_cfi01_system_reset(DeviceState *dev)
>  {
>      PFlashCFI01 *pfl = PFLASH_CFI01(dev);
> -- 
> 2.26.2

dme.
-- 
She's as sweet as Tupelo honey, she's an angel of the first degree.

Re: [PATCH 3/9] hw/block/pflash_cfi02: Extract pflash_cfi02_fill_cfi_table()

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:22 +01, Philippe Mathieu-Daudé wrote:

> Fill the CFI table in out of DeviceRealize() in a new function:
> pflash_cfi02_fill_cfi_table().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi02.c | 193 +++++++++++++++++++++-------------------
>  1 file changed, 99 insertions(+), 94 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index fa981465e12..845f50ed99b 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -724,6 +724,104 @@ static const MemoryRegionOps pflash_cfi02_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +static void pflash_cfi02_fill_cfi_table(PFlashCFI02 *pfl, int nb_regions)
> +{
> +    /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> +    const uint16_t pri_ofs = 0x40;
> +    /* Standard "QRY" string */
> +    pfl->cfi_table[0x10] = 'Q';
> +    pfl->cfi_table[0x11] = 'R';
> +    pfl->cfi_table[0x12] = 'Y';
> +    /* Command set (AMD/Fujitsu) */
> +    pfl->cfi_table[0x13] = 0x02;
> +    pfl->cfi_table[0x14] = 0x00;
> +    /* Primary extended table address */
> +    pfl->cfi_table[0x15] = pri_ofs;
> +    pfl->cfi_table[0x16] = pri_ofs >> 8;
> +    /* Alternate command set (none) */
> +    pfl->cfi_table[0x17] = 0x00;
> +    pfl->cfi_table[0x18] = 0x00;
> +    /* Alternate extended table (none) */
> +    pfl->cfi_table[0x19] = 0x00;
> +    pfl->cfi_table[0x1A] = 0x00;
> +    /* Vcc min */
> +    pfl->cfi_table[0x1B] = 0x27;
> +    /* Vcc max */
> +    pfl->cfi_table[0x1C] = 0x36;
> +    /* Vpp min (no Vpp pin) */
> +    pfl->cfi_table[0x1D] = 0x00;
> +    /* Vpp max (no Vpp pin) */
> +    pfl->cfi_table[0x1E] = 0x00;
> +    /* Timeout per single byte/word write (128 ms) */
> +    pfl->cfi_table[0x1F] = 0x07;
> +    /* Timeout for min size buffer write (NA) */
> +    pfl->cfi_table[0x20] = 0x00;
> +    /* Typical timeout for block erase (512 ms) */
> +    pfl->cfi_table[0x21] = 0x09;
> +    /* Typical timeout for full chip erase (4096 ms) */
> +    pfl->cfi_table[0x22] = 0x0C;
> +    /* Reserved */
> +    pfl->cfi_table[0x23] = 0x01;
> +    /* Max timeout for buffer write (NA) */
> +    pfl->cfi_table[0x24] = 0x00;
> +    /* Max timeout for block erase */
> +    pfl->cfi_table[0x25] = 0x0A;
> +    /* Max timeout for chip erase */
> +    pfl->cfi_table[0x26] = 0x0D;
> +    /* Device size */
> +    pfl->cfi_table[0x27] = ctz32(pfl->chip_len);
> +    /* Flash device interface (8 & 16 bits) */
> +    pfl->cfi_table[0x28] = 0x02;
> +    pfl->cfi_table[0x29] = 0x00;
> +    /* Max number of bytes in multi-bytes write */
> +    /*
> +     * XXX: disable buffered write as it's not supported
> +     * pfl->cfi_table[0x2A] = 0x05;
> +     */
> +    pfl->cfi_table[0x2A] = 0x00;
> +    pfl->cfi_table[0x2B] = 0x00;
> +    /* Number of erase block regions */
> +    pfl->cfi_table[0x2c] = nb_regions;
> +    /* Erase block regions */
> +    for (int i = 0; i < nb_regions; ++i) {
> +        uint32_t sector_len_per_device = pfl->sector_len[i];
> +        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
> +        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
> +        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
> +        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
> +    }
> +    assert(0x2c + 4 * nb_regions < pri_ofs);
> +
> +    /* Extended */
> +    pfl->cfi_table[0x00 + pri_ofs] = 'P';
> +    pfl->cfi_table[0x01 + pri_ofs] = 'R';
> +    pfl->cfi_table[0x02 + pri_ofs] = 'I';
> +
> +    /* Extended version 1.0 */
> +    pfl->cfi_table[0x03 + pri_ofs] = '1';
> +    pfl->cfi_table[0x04 + pri_ofs] = '0';
> +
> +    /* Address sensitive unlock required. */
> +    pfl->cfi_table[0x05 + pri_ofs] = 0x00;
> +    /* Erase suspend to read/write. */
> +    pfl->cfi_table[0x06 + pri_ofs] = 0x02;
> +    /* Sector protect not supported. */
> +    pfl->cfi_table[0x07 + pri_ofs] = 0x00;
> +    /* Temporary sector unprotect not supported. */
> +    pfl->cfi_table[0x08 + pri_ofs] = 0x00;
> +
> +    /* Sector protect/unprotect scheme. */
> +    pfl->cfi_table[0x09 + pri_ofs] = 0x00;
> +
> +    /* Simultaneous operation not supported. */
> +    pfl->cfi_table[0x0a + pri_ofs] = 0x00;
> +    /* Burst mode not supported. */
> +    pfl->cfi_table[0x0b + pri_ofs] = 0x00;
> +    /* Page mode not supported. */
> +    pfl->cfi_table[0x0c + pri_ofs] = 0x00;
> +    assert(0x0c + pri_ofs < ARRAY_SIZE(pfl->cfi_table));
> +}
> +
>  static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -837,100 +935,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->cmd = 0;
>      pfl->status = 0;
>  
> -    /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> -    const uint16_t pri_ofs = 0x40;
> -    /* Standard "QRY" string */
> -    pfl->cfi_table[0x10] = 'Q';
> -    pfl->cfi_table[0x11] = 'R';
> -    pfl->cfi_table[0x12] = 'Y';
> -    /* Command set (AMD/Fujitsu) */
> -    pfl->cfi_table[0x13] = 0x02;
> -    pfl->cfi_table[0x14] = 0x00;
> -    /* Primary extended table address */
> -    pfl->cfi_table[0x15] = pri_ofs;
> -    pfl->cfi_table[0x16] = pri_ofs >> 8;
> -    /* Alternate command set (none) */
> -    pfl->cfi_table[0x17] = 0x00;
> -    pfl->cfi_table[0x18] = 0x00;
> -    /* Alternate extended table (none) */
> -    pfl->cfi_table[0x19] = 0x00;
> -    pfl->cfi_table[0x1A] = 0x00;
> -    /* Vcc min */
> -    pfl->cfi_table[0x1B] = 0x27;
> -    /* Vcc max */
> -    pfl->cfi_table[0x1C] = 0x36;
> -    /* Vpp min (no Vpp pin) */
> -    pfl->cfi_table[0x1D] = 0x00;
> -    /* Vpp max (no Vpp pin) */
> -    pfl->cfi_table[0x1E] = 0x00;
> -    /* Timeout per single byte/word write (128 ms) */
> -    pfl->cfi_table[0x1F] = 0x07;
> -    /* Timeout for min size buffer write (NA) */
> -    pfl->cfi_table[0x20] = 0x00;
> -    /* Typical timeout for block erase (512 ms) */
> -    pfl->cfi_table[0x21] = 0x09;
> -    /* Typical timeout for full chip erase (4096 ms) */
> -    pfl->cfi_table[0x22] = 0x0C;
> -    /* Reserved */
> -    pfl->cfi_table[0x23] = 0x01;
> -    /* Max timeout for buffer write (NA) */
> -    pfl->cfi_table[0x24] = 0x00;
> -    /* Max timeout for block erase */
> -    pfl->cfi_table[0x25] = 0x0A;
> -    /* Max timeout for chip erase */
> -    pfl->cfi_table[0x26] = 0x0D;
> -    /* Device size */
> -    pfl->cfi_table[0x27] = ctz32(pfl->chip_len);
> -    /* Flash device interface (8 & 16 bits) */
> -    pfl->cfi_table[0x28] = 0x02;
> -    pfl->cfi_table[0x29] = 0x00;
> -    /* Max number of bytes in multi-bytes write */
> -    /*
> -     * XXX: disable buffered write as it's not supported
> -     * pfl->cfi_table[0x2A] = 0x05;
> -     */
> -    pfl->cfi_table[0x2A] = 0x00;
> -    pfl->cfi_table[0x2B] = 0x00;
> -    /* Number of erase block regions */
> -    pfl->cfi_table[0x2c] = nb_regions;
> -    /* Erase block regions */
> -    for (int i = 0; i < nb_regions; ++i) {
> -        uint32_t sector_len_per_device = pfl->sector_len[i];
> -        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
> -        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
> -        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
> -        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
> -    }
> -    assert(0x2c + 4 * nb_regions < pri_ofs);
> -
> -    /* Extended */
> -    pfl->cfi_table[0x00 + pri_ofs] = 'P';
> -    pfl->cfi_table[0x01 + pri_ofs] = 'R';
> -    pfl->cfi_table[0x02 + pri_ofs] = 'I';
> -
> -    /* Extended version 1.0 */
> -    pfl->cfi_table[0x03 + pri_ofs] = '1';
> -    pfl->cfi_table[0x04 + pri_ofs] = '0';
> -
> -    /* Address sensitive unlock required. */
> -    pfl->cfi_table[0x05 + pri_ofs] = 0x00;
> -    /* Erase suspend to read/write. */
> -    pfl->cfi_table[0x06 + pri_ofs] = 0x02;
> -    /* Sector protect not supported. */
> -    pfl->cfi_table[0x07 + pri_ofs] = 0x00;
> -    /* Temporary sector unprotect not supported. */
> -    pfl->cfi_table[0x08 + pri_ofs] = 0x00;
> -
> -    /* Sector protect/unprotect scheme. */
> -    pfl->cfi_table[0x09 + pri_ofs] = 0x00;
> -
> -    /* Simultaneous operation not supported. */
> -    pfl->cfi_table[0x0a + pri_ofs] = 0x00;
> -    /* Burst mode not supported. */
> -    pfl->cfi_table[0x0b + pri_ofs] = 0x00;
> -    /* Page mode not supported. */
> -    pfl->cfi_table[0x0c + pri_ofs] = 0x00;
> -    assert(0x0c + pri_ofs < ARRAY_SIZE(pfl->cfi_table));
> +    pflash_cfi02_fill_cfi_table(pfl, nb_regions);
>  }
>  
>  static Property pflash_cfi02_properties[] = {
> -- 
> 2.26.2

dme.
-- 
You took the words right out of my mouth.

Re: [PATCH 4/9] hw/block/pflash_cfi02: Set rom_mode to true in pflash_setup_mappings()

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:23 +01, Philippe Mathieu-Daudé wrote:

> There is only one call to pflash_setup_mappings(). Convert 'rom_mode'
> to boolean and set it to true directly within pflash_setup_mappings().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 845f50ed99b..5f949b4c792 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -108,7 +108,7 @@ struct PFlashCFI02 {
>      MemoryRegion mem;
>      MemoryRegion *mem_mappings;    /* array; one per mapping */
>      MemoryRegion orig_mem;
> -    int rom_mode;
> +    bool rom_mode;

Given this, doesn't the second argument to pflash_register_memory() need
to change to bool, affecting its callers?

>      int read_counter; /* used for lazy switch-back to rom mode */
>      int sectors_to_erase;
>      uint64_t erase_time_remaining;
> @@ -181,6 +181,7 @@ static void pflash_setup_mappings(PFlashCFI02 *pfl)
>                                   "pflash-alias", &pfl->orig_mem, 0, size);
>          memory_region_add_subregion(&pfl->mem, i * size, &pfl->mem_mappings[i]);
>      }
> +    pfl->rom_mode = true;
>  }
>  
>  static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
> @@ -927,7 +928,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->sector_erase_map = bitmap_new(pfl->total_sectors);
>  
>      pflash_setup_mappings(pfl);
> -    pfl->rom_mode = 1;
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>  
>      timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> -- 
> 2.26.2

dme.
-- 
And you can't hold me down, 'cause I belong to the hurricane.

Re: [PATCH 6/9] hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:25 +01, 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 three times:
>
> - When the timer expires and not in bypass mode
>
> - On a read access (on invalid command).
>
> - When the device is initialized. Here the ROMD mode is hidden
>   by the memory_region_init_rom_device() call.
>
> pflash_register_memory(rom_mode=true) already sets the ROM device
> in "read array" mode (from I/O device to ROM one). Explicit that
> by renaming the function as pflash_mode_read_array(), adding
> a trace event and resetting wcycle.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

Okay, I see that pflash_register_memory() was going to lose its second
argument anyway, so perhaps no need to fix it in the previous patch.

> ---
>  hw/block/pflash_cfi02.c | 18 +++++++++---------
>  hw/block/trace-events   |  1 +
>  2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 4efbae2f0c9..2ba77a0171b 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -184,10 +184,13 @@ static void pflash_setup_mappings(PFlashCFI02 *pfl)
>      pfl->rom_mode = true;
>  }
>  
> -static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
> +static void pflash_mode_read_array(PFlashCFI02 *pfl)
>  {
> -    memory_region_rom_device_set_romd(&pfl->orig_mem, rom_mode);
> -    pfl->rom_mode = rom_mode;
> +    trace_pflash_mode_read_array();
> +    pfl->cmd = 0x00;
> +    pfl->wcycle = 0;
> +    pfl->rom_mode = true;
> +    memory_region_rom_device_set_romd(&pfl->orig_mem, true);
>  }
>  
>  static size_t pflash_regions_count(PFlashCFI02 *pfl)
> @@ -249,11 +252,10 @@ static void pflash_timer(void *opaque)
>      toggle_dq7(pfl);
>      if (pfl->bypass) {
>          pfl->wcycle = 2;
> +        pfl->cmd = 0;
>      } else {
> -        pflash_register_memory(pfl, 1);
> -        pfl->wcycle = 0;
> +        pflash_mode_read_array(pfl);
>      }
> -    pfl->cmd = 0;
>  }
>  
>  /*
> @@ -315,7 +317,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>      /* Lazy reset to ROMD mode after a certain amount of read accesses */
>      if (!pfl->rom_mode && pfl->wcycle == 0 &&
>          ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
> -        pflash_register_memory(pfl, 1);
> +        pflash_mode_read_array(pfl);
>      }
>      offset &= pfl->chip_len - 1;
>      boff = offset & 0xFF;
> @@ -933,8 +935,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>  
>      timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
>      pfl->status = 0;
>  
>      pflash_cfi02_fill_cfi_table(pfl, nb_regions);
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index d32475c3989..f16d6e90cfd 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_cfi01.c
>  # pflash_cfi02.c
>  pflash_reset(void) "reset"
> +pflash_mode_read_array(void) "mode: read array"
>  pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
>  pflash_io_read(uint64_t offset, unsigned size, uint32_t value, uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" size:%u value:0x%04x cmd:0x%02x wcycle:%u"
>  pflash_io_write(uint64_t offset, unsigned size, uint32_t value, uint8_t wcycle) "offset:0x%04"PRIx64" size:%u value:0x%04x wcycle:%u"
> -- 
> 2.26.2

dme.
-- 
Ah, oh your hair is beautiful.

Re: [PATCH 7/9] hw/block/pflash_cfi02: Factor out DeviceReset method

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:26 +01, Philippe Mathieu-Daudé wrote:

> There is multiple places doing a device reset. Factor that
> out in a common method which matches the DeviceReset prototype,
> so we can also remove the reset code from the DeviceRealize()
> handler. Explicit the device is set in "read array" mode on
> reset.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi02.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 2ba77a0171b..484b056b898 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -193,6 +193,14 @@ static void pflash_mode_read_array(PFlashCFI02 *pfl)
>      memory_region_rom_device_set_romd(&pfl->orig_mem, true);
>  }
>  
> +static void pflash_cfi02_reset(DeviceState *dev)
> +{
> +    PFlashCFI02 *pfl = PFLASH_CFI02(dev);
> +
> +    trace_pflash_reset();
> +    pflash_mode_read_array(pfl);
> +}
> +
>  static size_t pflash_regions_count(PFlashCFI02 *pfl)
>  {
>      return pfl->cfi_table[0x2c];
> @@ -330,8 +338,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>      default:
>          /* 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;
> +        pflash_cfi02_reset(DEVICE(pfl));
>          /* fall through to the read code */
>      case 0x80: /* Erase (unlock) */
>          /* We accept reads during second unlock sequence... */
> @@ -710,10 +717,8 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>  
>      /* Reset flash */
>   reset_flash:
> -    trace_pflash_reset();
>      pfl->bypass = 0;
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_cfi02_reset(DEVICE(pfl));
>      return;
>  
>   do_bypass:
> @@ -977,6 +982,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pflash_cfi02_realize;
> +    dc->reset = pflash_cfi02_reset;
>      dc->unrealize = pflash_cfi02_unrealize;
>      device_class_set_props(dc, pflash_cfi02_properties);
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -- 
> 2.26.2

dme.
-- 
The band is just fantastic, that is really what I think.

Re: [PATCH 8/9] hw/block/pflash_cfi01: Clarify trace events

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:27 +01, Philippe Mathieu-Daudé wrote:

> Use the 'mode_read_array' event when we set the device in such
> mode, and use the 'reset' event in DeviceReset handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi01.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 03472ea5b64..2618e00926d 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -663,7 +663,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
>   mode_read_array:
> -    trace_pflash_reset();
> +    trace_pflash_mode_read_array();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
>      pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
> @@ -886,6 +886,7 @@ static void pflash_cfi01_system_reset(DeviceState *dev)
>  {
>      PFlashCFI01 *pfl = PFLASH_CFI01(dev);
>  
> +    trace_pflash_reset();
>      /*
>       * The command 0x00 is not assigned by the CFI open standard,
>       * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -- 
> 2.26.2

dme.
-- 
At least they're not lonely.

Re: [PATCH 9/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()

[David Edmondson]

On Wednesday, 2021-03-10 at 00:50:28 +01, 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 three times:
>
> - On a read access (on invalid command).
>
> - 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.
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  hw/block/pflash_cfi01.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 2618e00926d..32c9b289715 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -115,6 +115,19 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_mode_read_array();
> +    /*
> +     * The command 0x00 is not assigned by the CFI open standard,
> +     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +     */
> +    trace_pflash_mode_read_array();
> +    pfl->cmd = 0x00;
> +    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
> @@ -283,12 +296,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>      default:
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
> -        pfl->wcycle = 0;
> -        /*
> -         * The command 0x00 is not assigned by the CFI open standard,
> -         * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -         */
> -        pfl->cmd = 0x00;
> +        pflash_mode_read_array(pfl);
>          /* fall through to read code */
>      case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
>          /* Flash area read */
> @@ -663,10 +671,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
>   mode_read_array:
> -    trace_pflash_mode_read_array();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
> +    pflash_mode_read_array(pfl);
>  }
>  
>  
> @@ -872,13 +877,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    /*
> -     * The command 0x00 is not assigned by the CFI open standard,
> -     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -     */
> -    pfl->cmd = 0x00;
>      pfl->status = 0x80; /* WSM ready */
> +    pflash_mode_read_array(pfl);
>      pflash_cfi01_fill_cfi_table(pfl);
>  }
>  
> @@ -887,13 +887,7 @@ static void pflash_cfi01_system_reset(DeviceState *dev)
>      PFlashCFI01 *pfl = PFLASH_CFI01(dev);
>  
>      trace_pflash_reset();
> -    /*
> -     * The command 0x00 is not assigned by the CFI open standard,
> -     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -     */
> -    pfl->cmd = 0x00;
> -    pfl->wcycle = 0;
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> +    pflash_mode_read_array(pfl);
>      /*
>       * The WSM ready timer occurs at most 150ns after system reset.
>       * This model deliberately ignores this delay.
> -- 
> 2.26.2

dme.
-- 
Would you offer your throat to the wolf with the red roses?

Re: [PATCH 6/9] hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array

[Philippe Mathieu-Daudé]

On 3/10/21 11:59 AM, David Edmondson wrote:
> On Wednesday, 2021-03-10 at 00:50:25 +01, 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 three times:
>>
>> - When the timer expires and not in bypass mode
>>
>> - On a read access (on invalid command).
>>
>> - When the device is initialized. Here the ROMD mode is hidden
>>   by the memory_region_init_rom_device() call.
>>
>> pflash_register_memory(rom_mode=true) already sets the ROM device
>> in "read array" mode (from I/O device to ROM one). Explicit that
>> by renaming the function as pflash_mode_read_array(), adding
>> a trace event and resetting wcycle.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> 
> Okay, I see that pflash_register_memory() was going to lose its second
> argument anyway, so perhaps no need to fix it in the previous patch.

It makes the previous patch more complex, so I'll keep it that way.

Thanks for your review!

Phil.

Re: [PATCH 4/9] hw/block/pflash_cfi02: Set rom_mode to true in pflash_setup_mappings()

[Philippe Mathieu-Daudé]

On 3/10/21 11:58 AM, David Edmondson wrote:
> On Wednesday, 2021-03-10 at 00:50:23 +01, Philippe Mathieu-Daudé wrote:
> 
>> There is only one call to pflash_setup_mappings(). Convert 'rom_mode'
>> to boolean and set it to true directly within pflash_setup_mappings().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 845f50ed99b..5f949b4c792 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -108,7 +108,7 @@ struct PFlashCFI02 {
>>      MemoryRegion mem;
>>      MemoryRegion *mem_mappings;    /* array; one per mapping */
>>      MemoryRegion orig_mem;
>> -    int rom_mode;
>> +    bool rom_mode;
> 
> Given this, doesn't the second argument to pflash_register_memory() need
> to change to bool, affecting its callers?

Indeed, thanks.

Re: [PATCH 6/9] hw/block/pflash_cfi02: Rename register_memory(true) as mode_read_array

[Philippe Mathieu-Daudé]

On 3/10/21 5:31 PM, Philippe Mathieu-Daudé wrote:
> On 3/10/21 11:59 AM, David Edmondson wrote:
>> On Wednesday, 2021-03-10 at 00:50:25 +01, 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 three times:
>>>
>>> - When the timer expires and not in bypass mode
>>>
>>> - On a read access (on invalid command).
>>>
>>> - When the device is initialized. Here the ROMD mode is hidden
>>>   by the memory_region_init_rom_device() call.
>>>
>>> pflash_register_memory(rom_mode=true) already sets the ROM device
>>> in "read array" mode (from I/O device to ROM one). Explicit that
>>> by renaming the function as pflash_mode_read_array(), adding
>>> a trace event and resetting wcycle.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>>
>> Okay, I see that pflash_register_memory() was going to lose its second
>> argument anyway, so perhaps no need to fix it in the previous patch.
> 
> It makes the previous patch more complex, so I'll keep it that way.

OK by previous I understood #5 but now I read your comment on patch #4.
I'll update #4 then.

> 
> Thanks for your review!
> 
> Phil.
> 

Re: [PATCH 7/9] hw/block/pflash_cfi02: Factor out DeviceReset method

[Philippe Mathieu-Daudé]

On 3/10/21 9:48 AM, Bin Meng wrote:
> On Wed, Mar 10, 2021 at 7:55 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> There is multiple places doing a device reset. Factor that
>> out in a common method which matches the DeviceReset prototype,
>> so we can also remove the reset code from the DeviceRealize()
>> handler. Explicit the device is set in "read array" mode on
>> reset.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 2ba77a0171b..484b056b898 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -193,6 +193,14 @@ static void pflash_mode_read_array(PFlashCFI02 *pfl)
>>      memory_region_rom_device_set_romd(&pfl->orig_mem, true);
>>  }
>>
>> +static void pflash_cfi02_reset(DeviceState *dev)
>> +{
>> +    PFlashCFI02 *pfl = PFLASH_CFI02(dev);
>> +
>> +    trace_pflash_reset();
>> +    pflash_mode_read_array(pfl);
>> +}
>> +
>>  static size_t pflash_regions_count(PFlashCFI02 *pfl)
>>  {
>>      return pfl->cfi_table[0x2c];
>> @@ -330,8 +338,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>>      default:
>>          /* 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;
>> +        pflash_cfi02_reset(DEVICE(pfl));
>>          /* fall through to the read code */
>>      case 0x80: /* Erase (unlock) */
>>          /* We accept reads during second unlock sequence... */
>> @@ -710,10 +717,8 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>>
>>      /* Reset flash */
>>   reset_flash:
>> -    trace_pflash_reset();
>>      pfl->bypass = 0;
>> -    pfl->wcycle = 0;
>> -    pfl->cmd = 0;
>> +    pflash_cfi02_reset(DEVICE(pfl));
> 
> The old codes did not set pfl->rom_mode to true, but the new codes
> pflash_cfi02_reset() do. Is this correct?

Hmmm let's be precautious indeed. I'll better split this change.