Re: [Qemu-devel] [PATCH 1/5] hw/block/pflash_cfi01: Removed an unused timer

[Laszlo Ersek <lersek@redhat.com>]

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
> timing modelled. One year later, the CFI02 model was introduced
> (commit 05ee37ebf630) based on the CFI01 model. As noted in the

You got those commit references backwards, I believe:

* Commit 29133e9a0fff ("AMD NOR flash device support (initial patch by
Jocelyn Mayer)", 2006-06-25) introduced "hw/pflash_cfi02.c".

* Commit 05ee37ebf630 ("Gumstix 'connex' board support by Thorsten
Zitterell.", 2007-11-17) introduced "hw/pflash_cfi01.c".

> header, "It does not support timings". 12 years later, we never
> had to model the device timings. Time to remove the unused timer,
> we can still add it back if required.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Yes, I plan to model those timings later. Actually I have a series
> working, but I'd rather first
> 1/ refactor common code between the both CFI implementations,

Good idea.

> 2/ discuss on list whether or not use timings for the Virt flash.

What would the timer buy us (specifically wrt. cfi01 / OVMF / ArmVirt)?

Being faithful to actual hardware is always good... except when it runs
a significant risk of regressions. :) By that I don't mean "programming
errors"; I mean that guest code would now have to conform to various
timeouts, and that always makes me a bit concerned.


For this patch, with the commit references fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> ---
>  hw/block/pflash_cfi01.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 16dfae14b80..6dc04f156a7 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -42,7 +42,6 @@
>  #include "hw/block/flash.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/error.h"
> -#include "qemu/timer.h"
>  #include "qemu/bitops.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
> @@ -86,7 +85,6 @@ struct PFlashCFI01 {
>      uint8_t cfi_table[0x52];
>      uint64_t counter;
>      unsigned int writeblock_size;
> -    QEMUTimer *timer;
>      MemoryRegion mem;
>      char *name;
>      void *storage;
> @@ -110,18 +108,6 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> -static void pflash_timer (void *opaque)
> -{
> -    PFlashCFI01 *pfl = opaque;
> -
> -    trace_pflash_timer_expired(pfl->cmd);
> -    /* Reset flash */
> -    pfl->status ^= 0x80;
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -}
> -
>  /* 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.
> @@ -771,7 +757,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
>