Re: [Qemu-devel] [PATCH v4 02/10] block/pflash_cfi02: Refactor, NFC intended

[Philippe Mathieu-Daudé]

On 4/26/19 6:26 PM, Stephen Checkoway wrote:
> Simplify and refactor for upcoming commits. In particular, pull out all
> of the code to modify the status into simple helper functions. Status
> handling becomes more complex once multiple chips are interleaved to
> produce a single device.
> 
> No change in functionality is intended with this commit.

As this patch is hard to digest, I splitted it in various atomic changes
in another series:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00975.html

> 
> Signed-off-by: Stephen Checkoway <address@hidden>
> ---
>  hw/block/pflash_cfi02.c | 221 +++++++++++++++++-----------------------
>  1 file changed, 95 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index f2c6201f81..4b7af71806 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -46,18 +46,19 @@
>  #include "hw/sysbus.h"
>  #include "trace.h"
>  
> -//#define PFLASH_DEBUG
> -#ifdef PFLASH_DEBUG
> +#define PFLASH_DEBUG false
>  #define DPRINTF(fmt, ...)                                  \
>  do {                                                       \
> -    fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);       \
> +    if (PFLASH_DEBUG) {                                    \
> +        fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
> +    }                                                      \
>  } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do { } while (0)
> -#endif
>  
>  #define PFLASH_LAZY_ROMD_THRESHOLD 42
>  
> +/* Special write cycle for CFI queries. */
> +#define WCYCLE_CFI 7
> +
>  struct PFlashCFI02 {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -97,6 +98,31 @@ struct PFlashCFI02 {
>      void *storage;
>  };
>  
> +/*
> + * Toggle status bit DQ7.
> + */
> +static inline void toggle_dq7(PFlashCFI02 *pfl)
> +{
> +    pfl->status ^= 0x80;
> +}
> +
> +/*
> + * Set status bit DQ7 to bit 7 of value.
> + */
> +static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
> +{
> +    pfl->status &= 0x7F;
> +    pfl->status |= value & 0x80;
> +}
> +
> +/*
> + * Toggle status bit DQ6.
> + */
> +static inline void toggle_dq6(PFlashCFI02 *pfl)
> +{
> +    pfl->status ^= 0x40;
> +}
> +
>  /*
>   * Set up replicated mappings of the same region.
>   */
> @@ -126,7 +152,7 @@ static void pflash_timer (void *opaque)
>  
>      trace_pflash_timer_expired(pfl->cmd);
>      /* Reset flash */
> -    pfl->status ^= 0x80;
> +    toggle_dq7(pfl);
>      if (pfl->bypass) {
>          pfl->wcycle = 2;
>      } else {
> @@ -136,12 +162,34 @@ static void pflash_timer (void *opaque)
>      pfl->cmd = 0;
>  }
>  
> -static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
> -                            int width, int be)
> +/*
> + * Read data from flash.
> + */
> +static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
> +                                 unsigned int width)
>  {
> +    uint8_t *p = (uint8_t *)pfl->storage + offset;
> +    uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
> +    /* XXX: Need a trace_pflash_data_read(offset, ret, width) */
> +    switch (width) {
> +    case 1:
> +        trace_pflash_data_read8(offset, ret);
> +        break;
> +    case 2:
> +        trace_pflash_data_read16(offset, ret);
> +        break;
> +    case 4:
> +        trace_pflash_data_read32(offset, ret);
> +        break;
> +    }
> +    return ret;
> +}
> +
> +static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
> +{
> +    PFlashCFI02 *pfl = opaque;
>      hwaddr boff;
> -    uint32_t ret;
> -    uint8_t *p;
> +    uint64_t ret;
>  
>      ret = -1;
>      trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
> @@ -166,39 +214,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
> offset,
>      case 0x80:
>          /* We accept reads during second unlock sequence... */
>      case 0x00:
> -    flash_read:
>          /* Flash area read */
> -        p = pfl->storage;
> -        switch (width) {
> -        case 1:
> -            ret = p[offset];
> -            trace_pflash_data_read8(offset, ret);
> -            break;
> -        case 2:
> -            if (be) {
> -                ret = p[offset] << 8;
> -                ret |= p[offset + 1];
> -            } else {
> -                ret = p[offset];
> -                ret |= p[offset + 1] << 8;
> -            }
> -            trace_pflash_data_read16(offset, ret);
> -            break;
> -        case 4:
> -            if (be) {
> -                ret = p[offset] << 24;
> -                ret |= p[offset + 1] << 16;
> -                ret |= p[offset + 2] << 8;
> -                ret |= p[offset + 3];
> -            } else {
> -                ret = p[offset];
> -                ret |= p[offset + 1] << 8;
> -                ret |= p[offset + 2] << 16;
> -                ret |= p[offset + 3] << 24;
> -            }
> -            trace_pflash_data_read32(offset, ret);
> -            break;
> -        }
> +        ret = pflash_data_read(pfl, offset, width);
>          break;
>      case 0x90:
>          /* flash ID read */
> @@ -213,23 +230,23 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
> offset,
>          case 0x0E:
>          case 0x0F:
>              ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
> -            if (ret == (uint8_t)-1) {
> -                goto flash_read;
> +            if (ret != (uint8_t)-1) {
> +                break;
>              }
> -            break;
> +            /* Fall through to data read. */
>          default:
> -            goto flash_read;
> +            ret = pflash_data_read(pfl, offset, width);
>          }
> -        DPRINTF("%s: ID " TARGET_FMT_plx " %x\n", __func__, boff, ret);
> +        DPRINTF("%s: ID " TARGET_FMT_plx " %" PRIx64 "\n", __func__, boff, 
> ret);
>          break;
>      case 0xA0:
>      case 0x10:
>      case 0x30:
>          /* Status register read */
>          ret = pfl->status;
> -        DPRINTF("%s: status %x\n", __func__, ret);
> +        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
>          /* Toggle bit 6 */
> -        pfl->status ^= 0x40;
> +        toggle_dq6(pfl);
>          break;
>      case 0x98:
>          /* CFI query mode */
> @@ -245,8 +262,7 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
> offset,
>  }
>  
>  /* update flash content on disk */
> -static void pflash_update(PFlashCFI02 *pfl, int offset,
> -                          int size)
> +static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
>  {
>      int offset_end;
>      if (pfl->blk) {
> @@ -259,9 +275,10 @@ static void pflash_update(PFlashCFI02 *pfl, int offset,
>      }
>  }
>  
> -static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
> -                         uint32_t value, int width, int be)
> +static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
> +                         unsigned int width)
>  {
> +    PFlashCFI02 *pfl = opaque;
>      hwaddr boff;
>      uint8_t *p;
>      uint8_t cmd;
> @@ -277,7 +294,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>      trace_pflash_write(offset, value, width, pfl->wcycle);
>      offset &= pfl->chip_len - 1;
>  
> -    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d\n", __func__,
> +    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
>              offset, value, width);
>      boff = offset & (pfl->sector_len - 1);
>      if (pfl->width == 2)
> @@ -295,7 +312,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>          if (boff == 0x55 && cmd == 0x98) {
>          enter_CFI_mode:
>              /* Enter CFI query mode */
> -            pfl->wcycle = 7;
> +            pfl->wcycle = WCYCLE_CFI;
>              pfl->cmd = 0x98;
>              return;
>          }
> @@ -345,40 +362,22 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr 
> offset,
>              goto check_unlock0;
>          case 0xA0:
>              trace_pflash_data_write(offset, value, width, 0);
> -            p = pfl->storage;
>              if (!pfl->ro) {
> -                switch (width) {
> -                case 1:
> -                    p[offset] &= value;
> -                    pflash_update(pfl, offset, 1);
> -                    break;
> -                case 2:
> -                    if (be) {
> -                        p[offset] &= value >> 8;
> -                        p[offset + 1] &= value;
> -                    } else {
> -                        p[offset] &= value;
> -                        p[offset + 1] &= value >> 8;
> -                    }
> -                    pflash_update(pfl, offset, 2);
> -                    break;
> -                case 4:
> -                    if (be) {
> -                        p[offset] &= value >> 24;
> -                        p[offset + 1] &= value >> 16;
> -                        p[offset + 2] &= value >> 8;
> -                        p[offset + 3] &= value;
> -                    } else {
> -                        p[offset] &= value;
> -                        p[offset + 1] &= value >> 8;
> -                        p[offset + 2] &= value >> 16;
> -                        p[offset + 3] &= value >> 24;
> -                    }
> -                    pflash_update(pfl, offset, 4);
> -                    break;
> +                p = (uint8_t *)pfl->storage + offset;
> +                if (pfl->be) {
> +                    uint64_t current = ldn_be_p(p, width);
> +                    stn_be_p(p, width, current & value);
> +                } else {
> +                    uint64_t current = ldn_le_p(p, width);
> +                    stn_le_p(p, width, current & value);
>                  }
> +                pflash_update(pfl, offset, width);
>              }
> -            pfl->status = 0x00 | ~(value & 0x80);
> +            /*
> +             * While programming, status bit DQ7 should hold the opposite
> +             * value from how it was programmed.
> +             */
> +            set_dq7(pfl, ~value);
>              /* Let's pretend write is immediate */
>              if (pfl->bypass)
>                  goto do_bypass;
> @@ -426,7 +425,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>                  memset(pfl->storage, 0xFF, pfl->chip_len);
>                  pflash_update(pfl, 0, pfl->chip_len);
>              }
> -            pfl->status = 0x00;
> +            set_dq7(pfl, 0x00);
>              /* Let's wait 5 seconds before chip erase is done */
>              timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND * 5));
> @@ -441,7 +440,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>                  memset(p + offset, 0xFF, pfl->sector_len);
>                  pflash_update(pfl, offset, pfl->sector_len);
>              }
> -            pfl->status = 0x00;
> +            set_dq7(pfl, 0x00);
>              /* Let's wait 1/2 second before sector erase is done */
>              timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND / 2));
> @@ -467,7 +466,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>              goto reset_flash;
>          }
>          break;
> -    case 7: /* Special value for CFI queries */
> +    case WCYCLE_CFI: /* Special value for CFI queries */
>          DPRINTF("%s: invalid write in CFI query mode\n", __func__);
>          goto reset_flash;
>      default:
> @@ -492,39 +491,9 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>      pfl->cmd = 0;
>  }
>  
> -static uint64_t pflash_be_readfn(void *opaque, hwaddr addr, unsigned size)
> -{
> -    return pflash_read(opaque, addr, size, 1);
> -}
> -
> -static void pflash_be_writefn(void *opaque, hwaddr addr,
> -                              uint64_t value, unsigned size)
> -{
> -    pflash_write(opaque, addr, value, size, 1);
> -}
> -
> -static uint64_t pflash_le_readfn(void *opaque, hwaddr addr, unsigned size)
> -{
> -    return pflash_read(opaque, addr, size, 0);
> -}
> -
> -static void pflash_le_writefn(void *opaque, hwaddr addr,
> -                              uint64_t value, unsigned size)
> -{
> -    pflash_write(opaque, addr, value, size, 0);
> -}
> -
> -static const MemoryRegionOps pflash_cfi02_ops_be = {
> -    .read = pflash_be_readfn,
> -    .write = pflash_be_writefn,
> -    .valid.min_access_size = 1,
> -    .valid.max_access_size = 4,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> -};
> -
> -static const MemoryRegionOps pflash_cfi02_ops_le = {
> -    .read = pflash_le_readfn,
> -    .write = pflash_le_writefn,
> +static const MemoryRegionOps pflash_cfi02_ops = {
> +    .read = pflash_read,
> +    .write = pflash_write,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> @@ -552,9 +521,9 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
> **errp)
>  
>      chip_len = pfl->sector_len * pfl->nb_blocs;
>  
> -    memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
> -                                  &pflash_cfi02_ops_be : 
> &pflash_cfi02_ops_le,
> -                                  pfl, pfl->name, chip_len, &local_err);
> +    memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
> +                                  &pflash_cfi02_ops, pfl, pfl->name,
> +                                  chip_len, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>