[Qemu-devel] [PATCH v2 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate

[Qemu-devel] [PATCH v2 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate

[Peter Maydell]

This patchset updates the ancient pxa2xx_mmci device to something
resembling modern standards for devices. In particular it makes
it a proper sysbus device and switches to VMStateDescription structs.

The only change here (apart from a trivial rebase) is that since
I sent the first version, commit 5ec911c30ff433 has updated sd_init()
so you can now call it with an already-attached-to-a-device block
backend. This means that the ugliness in the first version with a
pointer property has gone away and we now have a DEFINE_PROP_DRIVE.

(Question: when should a device use DEFINE_PROP_DRIVE and when should
it use DEFINE_BLOCK_PROPERTIES to get the PROP_DRIVE and some other
stuff? We seem to have examples of both...)

There are clearly further cleanup opportunities for this device,
like making the sd callbacks into sysbus gpio input lines rather
than having an ad-hoc pxa2xx_mmci_handlers() function to set them,
but one thing at a time.

Changes v1->v2:
 * (1/3) use an actual DEFINE_PROP_DRIVE property for the drive

Peter Maydell (3):
  hw/sd/pxa2xx_mmci: convert to SysBusDevice object
  hw/sd/pxa2xx_mmci: Convert to VMStateDescription
  hw/sd/pxa2xx_mmci: Add reset function

 hw/sd/pxa2xx_mmci.c | 254 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 152 insertions(+), 102 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

Convert the pxa2xx_mmci device to be a sysbus device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 89 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 17 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index b217080..39ca309 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -11,16 +11,24 @@
  */
 
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/arm/pxa.h"
 #include "hw/sd/sd.h"
 #include "hw/qdev.h"
+#include "hw/qdev-properties.h"
+
+#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
+#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
+
+typedef struct PXA2xxMMCIState {
+    SysBusDevice parent_obj;
 
-struct PXA2xxMMCIState {
     MemoryRegion iomem;
     qemu_irq irq;
     qemu_irq rx_dma;
     qemu_irq tx_dma;
 
+    BlockBackend *blk;
     SDState *card;
 
     uint32_t status;
@@ -48,7 +56,7 @@ struct PXA2xxMMCIState {
     int resp_len;
 
     int cmdreq;
-};
+} PXA2xxMMCIState;
 
 #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
 #define MMC_STAT	0x04	/* MMC Status register */
@@ -474,31 +482,78 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 BlockBackend *blk, qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma)
 {
-    PXA2xxMMCIState *s;
+    DeviceState *dev;
+    SysBusDevice *sbd;
+
+    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
+    qdev_prop_set_drive_nofail(dev, "drive", blk);
+    qdev_init_nofail(dev);
+    sbd = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(sbd, 0, base);
+    sysbus_connect_irq(sbd, 0, irq);
+    sysbus_connect_irq(sbd, 1, rx_dma);
+    sysbus_connect_irq(sbd, 2, tx_dma);
+    return PXA2XX_MMCI(dev);
+}
 
-    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
-    s->irq = irq;
-    s->rx_dma = rx_dma;
-    s->tx_dma = tx_dma;
+void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
+                qemu_irq coverswitch)
+{
+    sd_set_cb(s->card, readonly, coverswitch);
+}
+
+static void pxa2xx_mmci_instance_init(Object *obj)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
+    memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s,
                           "pxa2xx-mmci", 0x00100000);
-    memory_region_add_subregion(sysmem, base, &s->iomem);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+    sysbus_init_irq(sbd, &s->rx_dma);
+    sysbus_init_irq(sbd, &s->tx_dma);
+
+    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
+                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+}
+
+static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
 
     /* Instantiate the actual storage */
-    s->card = sd_init(blk, false);
+    s->card = sd_init(s->blk, false);
     if (s->card == NULL) {
-        exit(1);
+        error_setg(errp, "Could not initialize SD card with specified drive");
+        return;
     }
+}
 
-    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+static Property pxa2xx_mmci_properties[] = {
+    DEFINE_PROP_DRIVE("drive", PXA2xxMMCIState, blk),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
-    return s;
+static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pxa2xx_mmci_realize;
+    dc->props = pxa2xx_mmci_properties;
 }
 
-void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
-                qemu_irq coverswitch)
+static const TypeInfo pxa2xx_mmci_info = {
+    .name = TYPE_PXA2XX_MMCI,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PXA2xxMMCIState),
+    .class_init = pxa2xx_mmci_class_init,
+    .instance_init = pxa2xx_mmci_instance_init,
+};
+
+static void pxa2xx_mmci_register_types(void)
 {
-    sd_set_cb(s->card, readonly, coverswitch);
+    type_register_static(&pxa2xx_mmci_info);
 }
+
+type_init(pxa2xx_mmci_register_types)
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Maydell]

Convert the pxa2xx_mmci device from manual save/load
functions to a VMStateDescription structure.

This is a migration compatibility break.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 149 ++++++++++++++++++++--------------------------------
 1 file changed, 57 insertions(+), 92 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 39ca309..fd76ffe 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -37,27 +37,72 @@ typedef struct PXA2xxMMCIState {
     uint32_t cmdat;
     uint32_t resp_tout;
     uint32_t read_tout;
-    int blklen;
-    int numblk;
+    int32_t blklen;
+    int32_t numblk;
     uint32_t intmask;
     uint32_t intreq;
-    int cmd;
+    int32_t cmd;
     uint32_t arg;
 
-    int active;
-    int bytesleft;
+    int32_t active;
+    int32_t bytesleft;
     uint8_t tx_fifo[64];
-    int tx_start;
-    int tx_len;
+    uint32_t tx_start;
+    uint32_t tx_len;
     uint8_t rx_fifo[32];
-    int rx_start;
-    int rx_len;
+    uint32_t rx_start;
+    uint32_t rx_len;
     uint16_t resp_fifo[9];
-    int resp_len;
+    uint32_t resp_len;
 
-    int cmdreq;
+    int32_t cmdreq;
 } PXA2xxMMCIState;
 
+static bool pxa2xx_mmci_vmstate_validate(void *opaque, int version_id)
+{
+    PXA2xxMMCIState *s = opaque;
+
+    return s->tx_start < sizeof(s->tx_fifo)
+        && s->rx_start < sizeof(s->rx_fifo)
+        && s->tx_len <= sizeof(s->tx_fifo)
+        && s->rx_len <= sizeof(s->rx_fifo)
+        && s->resp_len <= sizeof(s->resp_fifo);
+}
+
+
+static const VMStateDescription vmstate_pxa2xx_mmci = {
+    .name = "pxa2xx-mmci",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(status, PXA2xxMMCIState),
+        VMSTATE_UINT32(clkrt, PXA2xxMMCIState),
+        VMSTATE_UINT32(spi, PXA2xxMMCIState),
+        VMSTATE_UINT32(cmdat, PXA2xxMMCIState),
+        VMSTATE_UINT32(resp_tout, PXA2xxMMCIState),
+        VMSTATE_UINT32(read_tout, PXA2xxMMCIState),
+        VMSTATE_INT32(blklen, PXA2xxMMCIState),
+        VMSTATE_INT32(numblk, PXA2xxMMCIState),
+        VMSTATE_UINT32(intmask, PXA2xxMMCIState),
+        VMSTATE_UINT32(intreq, PXA2xxMMCIState),
+        VMSTATE_INT32(cmd, PXA2xxMMCIState),
+        VMSTATE_UINT32(arg, PXA2xxMMCIState),
+        VMSTATE_INT32(cmdreq, PXA2xxMMCIState),
+        VMSTATE_INT32(active, PXA2xxMMCIState),
+        VMSTATE_INT32(bytesleft, PXA2xxMMCIState),
+        VMSTATE_UINT32(tx_start, PXA2xxMMCIState),
+        VMSTATE_UINT32(tx_len, PXA2xxMMCIState),
+        VMSTATE_UINT32(rx_start, PXA2xxMMCIState),
+        VMSTATE_UINT32(rx_len, PXA2xxMMCIState),
+        VMSTATE_UINT32(resp_len, PXA2xxMMCIState),
+        VMSTATE_VALIDATE("fifo size incorrect", pxa2xx_mmci_vmstate_validate),
+        VMSTATE_UINT8_ARRAY(tx_fifo, PXA2xxMMCIState, 64),
+        VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxMMCIState, 32),
+        VMSTATE_UINT16_ARRAY(resp_fifo, PXA2xxMMCIState, 9),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
 #define MMC_STAT	0x04	/* MMC Status register */
 #define MMC_CLKRT	0x08	/* MMC Clock Rate register */
@@ -399,84 +444,6 @@ static const MemoryRegionOps pxa2xx_mmci_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pxa2xx_mmci_save(QEMUFile *f, void *opaque)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    int i;
-
-    qemu_put_be32s(f, &s->status);
-    qemu_put_be32s(f, &s->clkrt);
-    qemu_put_be32s(f, &s->spi);
-    qemu_put_be32s(f, &s->cmdat);
-    qemu_put_be32s(f, &s->resp_tout);
-    qemu_put_be32s(f, &s->read_tout);
-    qemu_put_be32(f, s->blklen);
-    qemu_put_be32(f, s->numblk);
-    qemu_put_be32s(f, &s->intmask);
-    qemu_put_be32s(f, &s->intreq);
-    qemu_put_be32(f, s->cmd);
-    qemu_put_be32s(f, &s->arg);
-    qemu_put_be32(f, s->cmdreq);
-    qemu_put_be32(f, s->active);
-    qemu_put_be32(f, s->bytesleft);
-
-    qemu_put_byte(f, s->tx_len);
-    for (i = 0; i < s->tx_len; i ++)
-        qemu_put_byte(f, s->tx_fifo[(s->tx_start + i) & 63]);
-
-    qemu_put_byte(f, s->rx_len);
-    for (i = 0; i < s->rx_len; i ++)
-        qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 31]);
-
-    qemu_put_byte(f, s->resp_len);
-    for (i = s->resp_len; i < 9; i ++)
-        qemu_put_be16s(f, &s->resp_fifo[i]);
-}
-
-static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    int i;
-
-    qemu_get_be32s(f, &s->status);
-    qemu_get_be32s(f, &s->clkrt);
-    qemu_get_be32s(f, &s->spi);
-    qemu_get_be32s(f, &s->cmdat);
-    qemu_get_be32s(f, &s->resp_tout);
-    qemu_get_be32s(f, &s->read_tout);
-    s->blklen = qemu_get_be32(f);
-    s->numblk = qemu_get_be32(f);
-    qemu_get_be32s(f, &s->intmask);
-    qemu_get_be32s(f, &s->intreq);
-    s->cmd = qemu_get_be32(f);
-    qemu_get_be32s(f, &s->arg);
-    s->cmdreq = qemu_get_be32(f);
-    s->active = qemu_get_be32(f);
-    s->bytesleft = qemu_get_be32(f);
-
-    s->tx_len = qemu_get_byte(f);
-    s->tx_start = 0;
-    if (s->tx_len >= sizeof(s->tx_fifo) || s->tx_len < 0)
-        return -EINVAL;
-    for (i = 0; i < s->tx_len; i ++)
-        s->tx_fifo[i] = qemu_get_byte(f);
-
-    s->rx_len = qemu_get_byte(f);
-    s->rx_start = 0;
-    if (s->rx_len >= sizeof(s->rx_fifo) || s->rx_len < 0)
-        return -EINVAL;
-    for (i = 0; i < s->rx_len; i ++)
-        s->rx_fifo[i] = qemu_get_byte(f);
-
-    s->resp_len = qemu_get_byte(f);
-    if (s->resp_len > 9 || s->resp_len < 0)
-        return -EINVAL;
-    for (i = s->resp_len; i < 9; i ++)
-         qemu_get_be16s(f, &s->resp_fifo[i]);
-
-    return 0;
-}
-
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 hwaddr base,
                 BlockBackend *blk, qemu_irq irq,
@@ -513,9 +480,6 @@ static void pxa2xx_mmci_instance_init(Object *obj)
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_irq(sbd, &s->rx_dma);
     sysbus_init_irq(sbd, &s->tx_dma);
-
-    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
 }
 
 static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
@@ -541,6 +505,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pxa2xx_mmci_realize;
     dc->props = pxa2xx_mmci_properties;
+    dc->vmsd = &vmstate_pxa2xx_mmci;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/3] hw/sd/pxa2xx_mmci: Add reset function

[Peter Maydell]

Add a reset function to the pxa2xx_mmci device; previously it had
no handling for system reset at all.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index fd76ffe..3f21021 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -469,6 +469,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
     sd_set_cb(s->card, readonly, coverswitch);
 }
 
+static void pxa2xx_mmci_reset(DeviceState *d)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(d);
+
+    s->status = 0;
+    s->clkrt = 0;
+    s->spi = 0;
+    s->cmdat = 0;
+    s->resp_tout = 0;
+    s->read_tout = 0;
+    s->blklen = 0;
+    s->numblk = 0;
+    s->intmask = 0;
+    s->intreq = 0;
+    s->cmd = 0;
+    s->arg = 0;
+    s->active = 0;
+    s->bytesleft = 0;
+    s->tx_start = 0;
+    s->tx_len = 0;
+    s->rx_start = 0;
+    s->rx_len = 0;
+    s->resp_len = 0;
+    s->cmdreq = 0;
+    memset(s->tx_fifo, 0, sizeof(s->tx_fifo));
+    memset(s->rx_fifo, 0, sizeof(s->rx_fifo));
+    memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
+}
+
 static void pxa2xx_mmci_instance_init(Object *obj)
 {
     PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
@@ -506,6 +535,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
     dc->realize = pxa2xx_mmci_realize;
     dc->props = pxa2xx_mmci_properties;
     dc->vmsd = &vmstate_pxa2xx_mmci;
+    dc->reset = pxa2xx_mmci_reset;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
-- 
1.9.1