On Thu, Mar 11, 2021 at 1:37 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:
>
> - 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>
> Reviewed-by: David Edmondson <david.edmondson@oracle.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();
Duplicated trace calls
> + /*
> + * 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);
This adds a memory_region_rom_device_set_romd() call compared to the
original codes. Please double check if this is correct.
> /* 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);
ditto
> 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.
Regards,
Bin