Re: [Qemu-devel] [PATCH v2 3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()
On Mon, Jul 1, 2019 at 5:14 PM Philippe Mathieu-Daudé wrote:
>
> The same pattern is used when setting the flash in READ_ARRAY mode:
> - Set the state machine command to READ_ARRAY
> - Reset the write_cycle counter
> - Reset the memory region in ROMD
>
> Refactor the current code by extracting this pattern.
> It is used twice:
> - On a write access (on command failure, error, or explicitly asked)
> - When the device is initialized. Here the ROMD mode is hidden
> by the memory_region_init_rom_device() call.
>
> Rename the 'reset_flash' as 'mode_read_array' to make explicit we
> do not reset the device, we simply set its internal state machine
> in the READ_ARRAY mode. We do not reset the status register error
> bits, as a device reset would do.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> hw/block/pflash_cfi01.c | 36
> hw/block/trace-events | 1 +
> 2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 743b5d5794..33c77f6569 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
>
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +trace_pflash_mode_read_array();
> +pfl->cmd = 0xff; /* Read Array */
> +pfl->wcycle = 0;
> +memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
> /* Perform a CFI query based on the bank width of the flash.
> * If this code is called we know we have a device_width set for
> * this flash.
> @@ -470,7 +478,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> case 0x50: /* Clear status bits */
> DPRINTF("%s: Clear status bits\n", __func__);
> pfl->status = 0x0;
> -goto reset_flash;
> +goto mode_read_array;
> case 0x60: /* Block (un)lock */
> DPRINTF("%s: Block unlock\n", __func__);
> break;
> @@ -495,10 +503,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr
> offset,
> break;
> case 0xf0: /* Probe for AMD flash */
> DPRINTF("%s: Probe for AMD flash\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> case 0xff: /* Read array mode */
> DPRINTF("%s: Read array mode\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> default:
> goto error_flash;
> }
> @@ -525,7 +533,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> pfl->wcycle = 0;
> pfl->status |= 0x80;
> } else if (cmd == 0xff) { /* Read Array */
> -goto reset_flash;
> +goto mode_read_array;
> } else
> goto error_flash;
>
> @@ -552,15 +560,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr
> offset,
> pfl->wcycle = 0;
> pfl->status |= 0x80;
> } else if (cmd == 0xff) { /* read array mode */
> -goto reset_flash;
> +goto mode_read_array;
> } else {
> DPRINTF("%s: Unknown (un)locking command\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> break;
> case 0x98:
> if (cmd == 0xff) {
> -goto reset_flash;
> +goto mode_read_array;
> } else {
> DPRINTF("%s: leaving query mode\n", __func__);
> }
> @@ -620,7 +628,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> " the data is already written to storage!\n"
> "Flash device reset into READ mode.\n",
> __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> break;
> default:
> @@ -630,7 +638,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> default:
> /* Should never happen */
> DPRINTF("%s: invalid write state\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> return;
>
> @@ -639,11 +647,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>"(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value
> 0x%x)"
>"\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
> - reset_flash:
> -trace_pflash_reset();
> -memory_region_rom_device_set_romd(&pfl->mem, true);
> -pfl->wcycle = 0;
> -pfl->cmd = 0xff;
> + mode_read_array:
> +pflash_mode_read_array(pfl);
> }
>
>
> @@ -758,8 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
> **errp)
> pfl->max_device_width = pfl->device_width;
> }
>
> -pfl->wcyc
Re: [Qemu-devel] [PATCH v2 3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()
On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote:
> The same pattern is used when setting the flash in READ_ARRAY mode:
> - Set the state machine command to READ_ARRAY
> - Reset the write_cycle counter
> - Reset the memory region in ROMD
>
> Refactor the current code by extracting this pattern.
> It is used twice:
> - On a write access (on command failure, error, or explicitly asked)
> - When the device is initialized. Here the ROMD mode is hidden
> by the memory_region_init_rom_device() call.
>
> Rename the 'reset_flash' as 'mode_read_array' to make explicit we
> do not reset the device, we simply set its internal state machine
> in the READ_ARRAY mode. We do not reset the status register error
> bits, as a device reset would do.
>
The rename does make it less confusing; but as of this patch it doesn't
seem obvious why you need this as a function, it's only really used
about "1.5 times" as of yet -- the realize call as you mention doesn't
quite exactly utilize it.
I will assume this comes in handy later, so:
Reviewed-by: John Snow
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/block/pflash_cfi01.c | 36
> hw/block/trace-events | 1 +
> 2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 743b5d5794..33c77f6569 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
>
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +trace_pflash_mode_read_array();
> +pfl->cmd = 0xff; /* Read Array */
> +pfl->wcycle = 0;
> +memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
> /* Perform a CFI query based on the bank width of the flash.
> * If this code is called we know we have a device_width set for
> * this flash.
> @@ -470,7 +478,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> case 0x50: /* Clear status bits */
> DPRINTF("%s: Clear status bits\n", __func__);
> pfl->status = 0x0;
> -goto reset_flash;
> +goto mode_read_array;
> case 0x60: /* Block (un)lock */
> DPRINTF("%s: Block unlock\n", __func__);
> break;
> @@ -495,10 +503,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr
> offset,
> break;
> case 0xf0: /* Probe for AMD flash */
> DPRINTF("%s: Probe for AMD flash\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> case 0xff: /* Read array mode */
> DPRINTF("%s: Read array mode\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> default:
> goto error_flash;
> }
> @@ -525,7 +533,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> pfl->wcycle = 0;
> pfl->status |= 0x80;
> } else if (cmd == 0xff) { /* Read Array */
> -goto reset_flash;
> +goto mode_read_array;
> } else
> goto error_flash;
>
> @@ -552,15 +560,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr
> offset,
> pfl->wcycle = 0;
> pfl->status |= 0x80;
> } else if (cmd == 0xff) { /* read array mode */
> -goto reset_flash;
> +goto mode_read_array;
> } else {
> DPRINTF("%s: Unknown (un)locking command\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> break;
> case 0x98:
> if (cmd == 0xff) {
> -goto reset_flash;
> +goto mode_read_array;
> } else {
> DPRINTF("%s: leaving query mode\n", __func__);
> }
> @@ -620,7 +628,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> " the data is already written to storage!\n"
> "Flash device reset into READ mode.\n",
> __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> break;
> default:
> @@ -630,7 +638,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> default:
> /* Should never happen */
> DPRINTF("%s: invalid write state\n", __func__);
> -goto reset_flash;
> +goto mode_read_array;
> }
> return;
>
> @@ -639,11 +647,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>"(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value
> 0x%x)"
>"\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
> - reset_flash:
> -trace_pflash_reset();
> -memory_region_rom_device_set_romd(&pfl->mem, true);
> -pfl->wcycle
[Qemu-devel] [PATCH v2 3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()
The same pattern is used when setting the flash in READ_ARRAY mode:
- Set the state machine command to READ_ARRAY
- Reset the write_cycle counter
- Reset the memory region in ROMD
Refactor the current code by extracting this pattern.
It is used twice:
- On a write access (on command failure, error, or explicitly asked)
- When the device is initialized. Here the ROMD mode is hidden
by the memory_region_init_rom_device() call.
Rename the 'reset_flash' as 'mode_read_array' to make explicit we
do not reset the device, we simply set its internal state machine
in the READ_ARRAY mode. We do not reset the status register error
bits, as a device reset would do.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/pflash_cfi01.c | 36
hw/block/trace-events | 1 +
2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 743b5d5794..33c77f6569 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
}
};
+static void pflash_mode_read_array(PFlashCFI01 *pfl)
+{
+trace_pflash_mode_read_array();
+pfl->cmd = 0xff; /* Read Array */
+pfl->wcycle = 0;
+memory_region_rom_device_set_romd(&pfl->mem, true);
+}
+
/* Perform a CFI query based on the bank width of the flash.
* If this code is called we know we have a device_width set for
* this flash.
@@ -470,7 +478,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
case 0x50: /* Clear status bits */
DPRINTF("%s: Clear status bits\n", __func__);
pfl->status = 0x0;
-goto reset_flash;
+goto mode_read_array;
case 0x60: /* Block (un)lock */
DPRINTF("%s: Block unlock\n", __func__);
break;
@@ -495,10 +503,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
break;
case 0xf0: /* Probe for AMD flash */
DPRINTF("%s: Probe for AMD flash\n", __func__);
-goto reset_flash;
+goto mode_read_array;
case 0xff: /* Read array mode */
DPRINTF("%s: Read array mode\n", __func__);
-goto reset_flash;
+goto mode_read_array;
default:
goto error_flash;
}
@@ -525,7 +533,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
pfl->wcycle = 0;
pfl->status |= 0x80;
} else if (cmd == 0xff) { /* Read Array */
-goto reset_flash;
+goto mode_read_array;
} else
goto error_flash;
@@ -552,15 +560,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
pfl->wcycle = 0;
pfl->status |= 0x80;
} else if (cmd == 0xff) { /* read array mode */
-goto reset_flash;
+goto mode_read_array;
} else {
DPRINTF("%s: Unknown (un)locking command\n", __func__);
-goto reset_flash;
+goto mode_read_array;
}
break;
case 0x98:
if (cmd == 0xff) {
-goto reset_flash;
+goto mode_read_array;
} else {
DPRINTF("%s: leaving query mode\n", __func__);
}
@@ -620,7 +628,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
" the data is already written to storage!\n"
"Flash device reset into READ mode.\n",
__func__);
-goto reset_flash;
+goto mode_read_array;
}
break;
default:
@@ -630,7 +638,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
default:
/* Should never happen */
DPRINTF("%s: invalid write state\n", __func__);
-goto reset_flash;
+goto mode_read_array;
}
return;
@@ -639,11 +647,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
"(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value
0x%x)"
"\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
- reset_flash:
-trace_pflash_reset();
-memory_region_rom_device_set_romd(&pfl->mem, true);
-pfl->wcycle = 0;
-pfl->cmd = 0xff;
+ mode_read_array:
+pflash_mode_read_array(pfl);
}
@@ -758,8 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
pfl->max_device_width = pfl->device_width;
}
-pfl->wcycle = 0;
-pfl->cmd = 0xff;
+pflash_mode_read_array(pfl);
pfl->status = 0;
/* Hardcoded CFI table */
/* Standard "QRY" string */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 97a17838ed..d627cfc3f5 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -7,6 +7,7 @@ fdc_ioport_write(uint8_t reg, uint8_t
