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