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
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>
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.
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.
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. >
© 2016 - 2026 Red Hat, Inc.