Patchwork [02/17] hw/onenand: Qdevify

login
register
mail settings
Submitter Peter Maydell
Date Aug. 25, 2011, 8:04 p.m.
Message ID <1314302711-20498-3-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/111668/
State New
Headers show

Comments

Peter Maydell - Aug. 25, 2011, 8:04 p.m.
From: Juha Riihimäki <juha.riihimaki@nokia.com>

Qdevify the ONENAND device.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
[Riku Voipio: Fixes and restructuring patchset]
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
[Peter Maydell: More fixes and cleanups for upstream submission]
Signed-off-by:  Peter Maydell <peter.maydell@linaro.org>
---
 hw/flash.h   |   10 ++--
 hw/nseries.c |    9 ++-
 hw/onenand.c |  165 +++++++++++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 138 insertions(+), 46 deletions(-)
Edgar Iglesias - Aug. 27, 2011, 5:38 p.m.
On Thu, Aug 25, 2011 at 09:04:56PM +0100, Peter Maydell wrote:
> From: Juha Riihimäki <juha.riihimaki@nokia.com>
> 
> Qdevify the ONENAND device.
> 
> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> [Riku Voipio: Fixes and restructuring patchset]
> Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
> [Peter Maydell: More fixes and cleanups for upstream submission]
> Signed-off-by:  Peter Maydell <peter.maydell@linaro.org>


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
Peter Maydell - Aug. 28, 2011, 1:21 p.m.
On 27 August 2011 18:38, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Aug 25, 2011 at 09:04:56PM +0100, Peter Maydell wrote:
>> @@ -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?

Are you objecting to the change itself or the fact it's in this
patch rather than its own patch? (I'm happy to split it out into
a separate patch if you prefer.)

I think the change itself is right -- in a qdev device these
functions are basically methods on the qdev object, and it
doesn't make sense to pass a method one of the object's own
fields as a method argument. So either we should have
   onenand_command(s, value);
and make onenand_command do the setting of s->command;
or we do what this patch does and let onenand_command()
read the member field.

>> +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.

I think they're really just utility functions which are working around
the problem qdev has that instantiating, configuring and wiring up
a qdev model is so verbose. But I'm happy to lose this one, especially
since it's only used once. (well, twice once I get the n900 model in
a submittable state...)

Thanks for the rapid review of this patchset.

-- PMM
Edgar Iglesias - Aug. 28, 2011, 2:16 p.m.
On Sun, Aug 28, 2011 at 02:21:29PM +0100, Peter Maydell wrote:
> On 27 August 2011 18:38, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Thu, Aug 25, 2011 at 09:04:56PM +0100, Peter Maydell wrote:
> >> @@ -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?
> 
> Are you objecting to the change itself or the fact it's in this
> patch rather than its own patch? (I'm happy to split it out into
> a separate patch if you prefer.)
> 
> I think the change itself is right -- in a qdev device these
> functions are basically methods on the qdev object, and it
> doesn't make sense to pass a method one of the object's own
> fields as a method argument. So either we should have
>    onenand_command(s, value);
> and make onenand_command do the setting of s->command;
> or we do what this patch does and let onenand_command()
> read the member field.

OK thanks, I see them more like stylistic changes and would have
probably left them out for the sake of reviewability. But it doesn't
matter, my main concern was that I was missing something more important
here. No need to respin just for this..


> >> +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.
> 
> I think they're really just utility functions which are working around
> the problem qdev has that instantiating, configuring and wiring up
> a qdev model is so verbose. But I'm happy to lose this one, especially
> since it's only used once. (well, twice once I get the n900 model in
> a submittable state...)

Thanks. Please note that I'm not opposed to the helpers per se, but more
with the placement of them. It was shortly discussed here (a long while ago):
http://lists.gnu.org/archive/html/qemu-devel/2009-09/msg00843.html

Cheers

Patch

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;
     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;
+}
 
-    return s->otp;
+void *onenand_raw_otp(DeviceState *onenand_device)
+{
+    return FROM_SYSBUS(OneNANDState, sysbus_from_qdev(onenand_device))->otp;
 }
+
+device_init(onenand_register_device)