Re: [Qemu-devel] [PATCH 02/17] hw/onenand: Qdevify

[Edgar E. Iglesias]

On Thu, Aug 25, 2011 at 09:04:56PM +0100, Peter Maydell wrote:
> From: Juha Riihimäki <address@hidden>
> 
> Qdevify the ONENAND device.
> 
> Signed-off-by: Juha Riihimäki <address@hidden>
> [Riku Voipio: Fixes and restructuring patchset]
> Signed-off-by: Riku Voipio <address@hidden>
> [Peter Maydell: More fixes and cleanups for upstream submission]
> Signed-off-by:  Peter Maydell <address@hidden>


Hello



> ---
>  hw/flash.h   |   10 ++--
>  hw/nseries.c |    9 ++-
>  hw/onenand.c |  165 
> +++++++++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 138 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/flash.h b/hw/flash.h
> index 7fb012b..de3c5c3 100644
> --- a/hw/flash.h
> +++ b/hw/flash.h
> @@ -42,12 +42,10 @@ uint32_t nand_getbuswidth(DeviceState *dev);
>  #define NAND_MFR_MICRON              0x2c
>  
>  /* onenand.c */
> -void onenand_base_update(void *opaque, target_phys_addr_t new);
> -void onenand_base_unmap(void *opaque);
> -void *onenand_init(BlockDriverState *bdrv,
> -                uint16_t man_id, uint16_t dev_id, uint16_t ver_id,
> -                int regshift, qemu_irq irq);
> -void *onenand_raw_otp(void *opaque);
> +DeviceState *onenand_init(BlockDriverState *bdrv,
> +                          uint16_t man_id, uint16_t dev_id, uint16_t ver_id,
> +                          int regshift, qemu_irq irq);
> +void *onenand_raw_otp(DeviceState *onenand_device);
>  
>  /* ecc.c */
>  typedef struct {
> diff --git a/hw/nseries.c b/hw/nseries.c
> index f7aae7a..e61014c 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -33,6 +33,7 @@
>  #include "loader.h"
>  #include "blockdev.h"
>  #include "tusb6010.h"
> +#include "sysbus.h"
>  
>  /* Nokia N8x0 support */
>  struct n800_s {
> @@ -52,7 +53,7 @@ struct n800_s {
>      TUSBState *usb;
>      void *retu;
>      void *tahvo;
> -    void *nand;
> +    DeviceState *nand;
>  };
>  
>  /* GPIO pins */
> @@ -172,8 +173,10 @@ static void n8x0_nand_setup(struct n800_s *s)
>      s->nand = onenand_init(dinfo ? dinfo->bdrv : 0,
>                      NAND_MFR_SAMSUNG, 0x48, 0, 1,
>                      qdev_get_gpio_in(s->cpu->gpio, N8X0_ONENAND_GPIO));
> -    omap_gpmc_attach(s->cpu->gpmc, N8X0_ONENAND_CS, 0, onenand_base_update,
> -                    onenand_base_unmap, s->nand);
> +    omap_gpmc_attach(s->cpu->gpmc, N8X0_ONENAND_CS,
> +                     sysbus_mmio_get_region(sysbus_from_qdev(s->nand), 0),
> +                     NULL, NULL,
> +                     s->nand);
>      otp_region = onenand_raw_otp(s->nand);
>  
>      memcpy(otp_region + 0x000, n8x0_cal_wlan_mac, sizeof(n8x0_cal_wlan_mac));
> diff --git a/hw/onenand.c b/hw/onenand.c
> index 00276a0..15fffc8 100644
> --- a/hw/onenand.c
> +++ b/hw/onenand.c
> @@ -25,6 +25,7 @@
>  #include "blockdev.h"
>  #include "memory.h"
>  #include "exec-memory.h"
> +#include "sysbus.h"
>  
>  /* 11 for 2kB-page OneNAND ("2nd generation") and 10 for 1kB-page chips */
>  #define PAGE_SHIFT   11
> @@ -33,6 +34,7 @@
>  #define BLOCK_SHIFT  (PAGE_SHIFT + 6)
>  
>  typedef struct {
> +    SysBusDevice busdev;
>      struct {
>          uint16_t man;
>          uint16_t dev;
> @@ -49,6 +51,7 @@ typedef struct {
>      uint8_t *current;
>      MemoryRegion ram;
>      MemoryRegion mapped_ram;
> +    uint8_t current_direction;
>      uint8_t *boot[2];
>      uint8_t *data[2][2];
>      MemoryRegion iomem;
> @@ -120,27 +123,72 @@ static void onenand_mem_setup(OneNANDState *s)
>                                          1);
>  }
>  
> -void onenand_base_update(void *opaque, target_phys_addr_t new)
> +static void onenand_intr_update(OneNANDState *s)
>  {
> -    OneNANDState *s = (OneNANDState *) opaque;
> -
> -    s->base = new;
> -
> -    memory_region_add_subregion(get_system_memory(), s->base, &s->container);
> +    qemu_set_irq(s->intr, ((s->intstatus >> 15) ^ (~s->config[0] >> 6)) & 1);
>  }
>  
> -void onenand_base_unmap(void *opaque)
> +static void onenand_pre_save(void *opaque)
>  {
> -    OneNANDState *s = (OneNANDState *) opaque;
> -
> -    memory_region_del_subregion(get_system_memory(), &s->container);
> +    OneNANDState *s = opaque;
> +    if (s->current == s->otp) {
> +        s->current_direction = 1;
> +    } else if (s->current == s->image) {
> +        s->current_direction = 2;
> +    } else {
> +        s->current_direction = 0;
> +    }
>  }
>  
> -static void onenand_intr_update(OneNANDState *s)
> +static int onenand_post_load(void *opaque, int version_id)
>  {
> -    qemu_set_irq(s->intr, ((s->intstatus >> 15) ^ (~s->config[0] >> 6)) & 1);
> +    OneNANDState *s = opaque;
> +    switch (s->current_direction) {
> +    case 0:
> +        break;
> +    case 1:
> +        s->current = s->otp;
> +        break;
> +    case 2:
> +        s->current = s->image;
> +        break;
> +    default:
> +        return -1;
> +    }
> +    onenand_intr_update(s);
> +    return 0;
>  }
>  
> +static const VMStateDescription vmstate_onenand = {
> +    .name = "onenand",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = onenand_pre_save,
> +    .post_load = onenand_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(current_direction, OneNANDState),
> +        VMSTATE_INT32(cycle, OneNANDState),
> +        VMSTATE_INT32(otpmode, OneNANDState),
> +        VMSTATE_UINT16_ARRAY(addr, OneNANDState, 8),
> +        VMSTATE_UINT16_ARRAY(unladdr, OneNANDState, 8),
> +        VMSTATE_INT32(bufaddr, OneNANDState),
> +        VMSTATE_INT32(count, OneNANDState),
> +        VMSTATE_UINT16(command, OneNANDState),
> +        VMSTATE_UINT16_ARRAY(config, OneNANDState, 2),
> +        VMSTATE_UINT16(status, OneNANDState),
> +        VMSTATE_UINT16(intstatus, OneNANDState),
> +        VMSTATE_UINT16(wpstatus, OneNANDState),
> +        VMSTATE_INT32(secs_cur, OneNANDState),
> +        VMSTATE_PARTIAL_VBUFFER(blockwp, OneNANDState, blocks),
> +        VMSTATE_UINT8(ecc.cp, OneNANDState),
> +        VMSTATE_UINT16_ARRAY(ecc.lp, OneNANDState, 2),
> +        VMSTATE_UINT16(ecc.count, OneNANDState),
> +        VMSTATE_BUFFER_UNSAFE(otp, OneNANDState, 0, ((64 + 2) << 
> PAGE_SHIFT)),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  /* Hot reset (Reset OneNAND command) or warm reset (RP pin low) */
>  static void onenand_reset(OneNANDState *s, int cold)
>  {
> @@ -167,11 +215,17 @@ static void onenand_reset(OneNANDState *s, int cold)
>          /* Lock the whole flash */
>          memset(s->blockwp, ONEN_LOCK_LOCKED, s->blocks);
>  
> -        if (s->bdrv && bdrv_read(s->bdrv, 0, s->boot[0], 8) < 0)
> -            hw_error("%s: Loading the BootRAM failed.\n", __FUNCTION__);
> +        if (s->bdrv_cur && bdrv_read(s->bdrv_cur, 0, s->boot[0], 8) < 0) {
> +            hw_error("%s: Loading the BootRAM failed.\n", __func__);
> +        }
>      }
>  }
>  
> +static void onenand_system_reset(DeviceState *dev)
> +{
> +    onenand_reset(FROM_SYSBUS(OneNANDState, sysbus_from_qdev(dev)), 1);
> +}
> +
>  static inline int onenand_load_main(OneNANDState *s, int sec, int secn,
>                  void *dest)
>  {
> @@ -326,7 +380,7 @@ fail:
>      return 1;
>  }
>  
> -static void onenand_command(OneNANDState *s, int cmd)
> +static void onenand_command(OneNANDState *s)
>  {
>      int b;
>      int sec;
> @@ -346,7 +400,7 @@ static void onenand_command(OneNANDState *s, int cmd)
>              s->data[(s->bufaddr >> 2) & 1][1] : s->boot[1];  \
>      buf += (s->bufaddr & 3) << 4;
>  
> -    switch (cmd) {
> +    switch (s->command) {
>      case 0x00:       /* Load single/multiple sector data unit into buffer */
>          SETADDR(ONEN_BUF_BLOCK, ONEN_BUF_PAGE)
>  
> @@ -527,7 +581,7 @@ static void onenand_command(OneNANDState *s, int cmd)
>          s->status |= ONEN_ERR_CMD;
>          s->intstatus |= ONEN_INT;
>          fprintf(stderr, "%s: unknown OneNAND command %x\n",
> -                        __FUNCTION__, cmd);
> +                        __func__, s->command);
>      }
>  
>      onenand_intr_update(s);
> @@ -659,7 +713,7 @@ static void onenand_write(void *opaque, 
> target_phys_addr_t addr,
>          if (s->intstatus & (1 << 15))
>              break;
>          s->command = value;
> -        onenand_command(s, s->command);
> +        onenand_command(s);
>          break;


This s->command change doesnt seem related, is there a reason for it that
I'm missing?



>      case 0xf221:     /* System Configuration 1 */
>          s->config[0] = value;
> @@ -700,30 +754,25 @@ static const MemoryRegionOps onenand_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -void *onenand_init(BlockDriverState *bdrv,
> -                uint16_t man_id, uint16_t dev_id, uint16_t ver_id,
> -                int regshift, qemu_irq irq)
> +static int onenand_initfn(SysBusDevice *dev)
>  {
> -    OneNANDState *s = (OneNANDState *) g_malloc0(sizeof(*s));
> -    uint32_t size = 1 << (24 + ((dev_id >> 4) & 7));
> +    OneNANDState *s = (OneNANDState *)dev;
> +    uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
>      void *ram;
> -
> -    s->shift = regshift;
> -    s->intr = irq;
> +    s->base = (target_phys_addr_t)-1;
>      s->rdy = NULL;
> -    s->id.man = man_id;
> -    s->id.dev = dev_id;
> -    s->id.ver = ver_id;
>      s->blocks = size >> BLOCK_SHIFT;
>      s->secs = size >> 9;
>      s->blockwp = g_malloc(s->blocks);
> -    s->density_mask = (dev_id & 0x08) ? (1 << (6 + ((dev_id >> 4) & 7))) : 0;
> +    s->density_mask = (s->id.dev & 0x08)
> +        ? (1 << (6 + ((s->id.dev >> 4) & 7))) : 0;
>      memory_region_init_io(&s->iomem, &onenand_ops, s, "onenand",
>                            0x10000 << s->shift);
> -    s->bdrv = bdrv;
>      if (!s->bdrv) {
>          s->image = memset(g_malloc(size + (size >> 5)),
> -                        0xff, size + (size >> 5));
> +                          0xff, size + (size >> 5));
> +    } else {
> +        s->bdrv_cur = s->bdrv;
>      }
>      s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT),
>                      0xff, (64 + 2) << PAGE_SHIFT);
> @@ -736,15 +785,57 @@ void *onenand_init(BlockDriverState *bdrv,
>      s->data[1][0] = ram + ((0x0200 + (1 << (PAGE_SHIFT - 1))) << s->shift);
>      s->data[1][1] = ram + ((0x8010 + (1 << (PAGE_SHIFT - 6))) << s->shift);
>      onenand_mem_setup(s);
> +    sysbus_init_irq(dev, &s->intr);
> +    sysbus_init_mmio_region(dev, &s->container);
> +    vmstate_register(&dev->qdev,
> +                     ((s->shift & 0x7f) << 24)
> +                     | ((s->id.man & 0xff) << 16)
> +                     | ((s->id.dev & 0xff) << 8)
> +                     | (s->id.ver & 0xff),
> +                     &vmstate_onenand, s);
> +    return 0;
> +}
>  
> -    onenand_reset(s, 1);
> +static SysBusDeviceInfo onenand_info = {
> +    .init = onenand_initfn,
> +    .qdev.name = "onenand",
> +    .qdev.size = sizeof(OneNANDState),
> +    .qdev.reset = onenand_system_reset,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT16("manufacturer_id", OneNANDState, id.man, 0),
> +        DEFINE_PROP_UINT16("device_id", OneNANDState, id.dev, 0),
> +        DEFINE_PROP_UINT16("version_id", OneNANDState, id.ver, 0),
> +        DEFINE_PROP_INT32("shift", OneNANDState, shift, 0),
> +        DEFINE_PROP_DRIVE("drive", OneNANDState, bdrv),
> +        DEFINE_PROP_END_OF_LIST()
> +    }
> +};
>  
> -    return s;
> +static void onenand_register_device(void)
> +{
> +    sysbus_register_withprop(&onenand_info);
>  }
>  
> -void *onenand_raw_otp(void *opaque)
> +DeviceState *onenand_init(BlockDriverState *bdrv,
> +                          uint16_t man_id, uint16_t dev_id, uint16_t ver_id,
> +                          int regshift, qemu_irq irq)
>  {
> -    OneNANDState *s = (OneNANDState *) opaque;
> +    DeviceState *dev = qdev_create(NULL, "onenand");
> +    qdev_prop_set_uint16(dev, "manufacturer_id", man_id);
> +    qdev_prop_set_uint16(dev, "device_id", dev_id);
> +    qdev_prop_set_uint16(dev, "version_id", ver_id);
> +    qdev_prop_set_int32(dev, "shift", regshift);
> +    if (bdrv) {
> +        qdev_prop_set_drive_nofail(dev, "drive", bdrv);
> +    }
> +    qdev_init_nofail(dev);
> +    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
> +    return dev;
> +}

Personally Im not a fan of having code that conceptually runs above Qdev,
embedded in qdev models. But there seems to be a lack of agreement on this
and its commonly done elsewere. I'm not NAKing but if you agree, and would
like to move it out, I'd appreciate it.

This looks good otherwise.

Cheers