[Qemu-devel] [PATCH 00/10] hw/sd: QOMify sd.c (and pxa2xx_mmci)

[Qemu-devel] [PATCH 00/10] hw/sd: QOMify sd.c (and pxa2xx_mmci)

[Peter Maydell]

This series attempts to QOMify sd.c (the actual SD card model),
including a proper QOM bus between the controller and the card.

This series removes the experimental x-drive property on sdhci-pci;
the syntax for using that device changes:

instead of using the x-drive property:

  -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]

we create a specific sd device (which is autoplugged into the
sdhci-pci device's "sd-bus" bus if you don't manually connect them):

  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive


The basic structure of the patch series is:
 * QOMify sd.c itself
 * Add the QOM bus APIs
 * Convert sdhci to use the QOM bus APIs
 * QOMify pxa2xx_mmci
 * Convert pxa2xx_mmci to use the QOM bus APIs


Points worthy of review:
 * is the code in "sdhci_sysbus: Create SD card device in users"
   for xlnx-ep108.c to connect the SD cards up to the two controllers
   inside the SoC doing this in the right way?
 * I chose not to try to make the SD card type a subclass of
   SSI, because the interface we have at the moment didn't really
   seem to line up with what SSI has
 * generally do I have the QOM style right?


Some notes on compatibility/bisection:
 * the old-style non-QOMified controllers (which get their drive
   via blk_by_legacy_dinfo() continue to work through the whole series
 * the only QOMified controller which doesn't do that is sdhci-pci,
   which uses the experimental x-drive property to take a drive. This
   support and property is removed in patch 1; support for the new
   syntax is added in patch 5.
   (Since we're breaking the old syntax anyway I chose to not try to
   introduce the new syntax in the same commit as breaking the old one;
   I think it makes the patches easier to review.)


I don't have any Xilinx test images, so I haven't been able to test
the changes to those boards (beyond confirming that the board doesn't
crash on startup and that 'info qtree' seems to show SD cards
connected to the right things). I have tested sdhci-pci and
the pxa2xx.

Peter Maydell (10):
  hw/sd/sdhci.c: Remove x-drive property
  hw/sd/sd.c: QOMify
  hw/sd/sd.c: Convert sd_reset() function into Device reset method
  hw/sd: Add QOM bus which SD cards plug in to
  hw/sd/sdhci.c: Update to use SDBus APIs
  sdhci_sysbus: Create SD card device in users, not the device itself
  hw/sd/pxa2xx_mmci: convert to SysBusDevice object
  hw/sd/pxa2xx_mmci: Update to use new SDBus APIs
  hw/sd/pxa2xx_mmci: Convert to VMStateDescription
  hw/sd/pxa2xx_mmci: Add reset function

 hw/arm/xilinx_zynq.c  |  17 ++-
 hw/arm/xlnx-ep108.c   |  19 ++++
 hw/sd/Makefile.objs   |   2 +-
 hw/sd/core.c          | 146 ++++++++++++++++++++++++
 hw/sd/pxa2xx_mmci.c   | 304 ++++++++++++++++++++++++++++++++------------------
 hw/sd/sd.c            | 149 ++++++++++++++++++++-----
 hw/sd/sdhci.c         |  83 ++++++++------
 include/hw/sd/sd.h    |  63 +++++++++++
 include/hw/sd/sdhci.h |   3 +-
 9 files changed, 610 insertions(+), 176 deletions(-)
 create mode 100644 hw/sd/core.c

-- 
1.9.1

[Qemu-devel] [PATCH 01/10] hw/sd/sdhci.c: Remove x-drive property

[Peter Maydell]

The following commits will remove support for the old sdhci-pci
command line syntax using the x-drive property:
 -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]
and replace it with an explicit sd device:
 -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

(This is OK because x-drive is experimental.)

This commit removes the x-drive property so that old style
command lines will fail with a reasonable error message:
  -device sdhci-pci,x-drive=mydrive: Property '.x-drive' not found

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sdhci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8612760..991c9b5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1221,12 +1221,6 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_pci_properties[] = {
-    /*
-     * We currently fuse controller and card into a single device
-     * model, but we intend to separate them.  For that purpose, the
-     * properties that belong to the card are marked as experimental.
-     */
-    DEFINE_PROP_DRIVE("x-drive", SDHCIState, blk),
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-- 
1.9.1

[Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Maydell]

Turn the SD card into a QOM device.
This conversion only changes the device itself; the various
functions which are effectively methods on the device are not
touched at this point.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sd.c         | 99 ++++++++++++++++++++++++++++++++++++++++++------------
 include/hw/sd/sd.h |  3 ++
 2 files changed, 80 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1a9935c..7c79217 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -33,6 +33,8 @@
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
 #include "qemu/bitmap.h"
+#include "hw/qdev-properties.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_SD 1
 
@@ -77,6 +79,8 @@ enum SDCardStates {
 };
 
 struct SDState {
+    DeviceState parent_obj;
+
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t ocr;
@@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
     }
 };
 
-/* We do not model the chip select pin, so allow the board to select
-   whether card should be in SSI or MMC/SD mode.  It is also up to the
-   board to ensure that ssi transfers only occur when the chip select
-   is asserted.  */
+/* Legacy initialization function for use by non-qdevified callers */
 SDState *sd_init(BlockBackend *blk, bool is_spi)
 {
-    SDState *sd;
+    DeviceState *dev;
+    Error *err = NULL;
 
-    if (blk && blk_is_read_only(blk)) {
-        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
+    dev = qdev_create(NULL, TYPE_SD);
+    qdev_prop_set_drive(dev, "drive", blk, &err);
+    if (err) {
+        error_report("sd_init failed: %s", error_get_pretty(err));
         return NULL;
     }
-
-    sd = (SDState *) g_malloc0(sizeof(SDState));
-    sd->buf = blk_blockalign(blk, 512);
-    sd->spi = is_spi;
-    sd->enable = true;
-    sd->blk = blk;
-    sd_reset(sd);
-    if (sd->blk) {
-        /* Attach dev if not already attached.  (This call ignores an
-         * error return code if sd->blk is already attached.) */
-        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
-        blk_attach_dev(sd->blk, sd);
-        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
+    qdev_prop_set_bit(dev, "spi", is_spi);
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_report("sd_init failed: %s", error_get_pretty(err));
+        return NULL;
     }
-    vmstate_register(NULL, -1, &sd_vmstate, sd);
-    return sd;
+
+    return SD(dev);
 }
 
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
@@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
 {
     sd->enable = enable;
 }
+
+static void sd_instance_init(Object *obj)
+{
+    SDState *sd = SD(obj);
+
+    sd->enable = true;
+}
+
+static void sd_realize(DeviceState *dev, Error ** errp)
+{
+    SDState *sd = SD(dev);
+
+    if (sd->blk && blk_is_read_only(sd->blk)) {
+        error_setg(errp, "Cannot use read-only drive as SD card");
+        return;
+    }
+
+    sd->buf = blk_blockalign(sd->blk, 512);
+
+    if (sd->blk) {
+        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
+    }
+
+    sd_reset(sd);
+}
+
+static Property sd_properties[] = {
+    DEFINE_PROP_DRIVE("drive", SDState, blk),
+    /* We do not model the chip select pin, so allow the board to select
+     * whether card should be in SSI or MMC/SD mode.  It is also up to the
+     * board to ensure that ssi transfers only occur when the chip select
+     * is asserted.  */
+    DEFINE_PROP_BOOL("spi", SDState, spi, false),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void sd_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = sd_realize;
+    dc->props = sd_properties;
+    dc->vmsd = &sd_vmstate;
+}
+
+static const TypeInfo sd_info = {
+    .name = TYPE_SD,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(SDState),
+    .class_init = sd_class_init,
+    .instance_init = sd_instance_init,
+};
+
+static void sd_register_types(void)
+{
+    type_register_static(&sd_info);
+}
+
+type_init(sd_register_types)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 79adb5b..6997dfc 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -68,6 +68,9 @@ typedef struct {
 
 typedef struct SDState SDState;
 
+#define TYPE_SD "sd"
+#define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
+
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response);
-- 
1.9.1

[Qemu-devel] [PATCH 03/10] hw/sd/sd.c: Convert sd_reset() function into Device reset method

[Peter Maydell]

Convert the sd_reset() function into a proper Device reset method.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7c79217..b4a5a62 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -393,8 +393,9 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
     return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 }
 
-static void sd_reset(SDState *sd)
+static void sd_reset(DeviceState *dev)
 {
+    SDState *sd = SD(dev);
     uint64_t size;
     uint64_t sect;
 
@@ -435,7 +436,7 @@ static void sd_cardchange(void *opaque, bool load)
 
     qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
     if (blk_is_inserted(sd->blk)) {
-        sd_reset(sd);
+        sd_reset(DEVICE(sd));
         qemu_set_irq(sd->readonly_cb, sd->wp_switch);
     }
 }
@@ -677,7 +678,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 
         default:
             sd->state = sd_idle_state;
-            sd_reset(sd);
+            sd_reset(DEVICE(sd));
             return sd->spi ? sd_r1 : sd_r0;
         }
         break;
@@ -1783,8 +1784,6 @@ static void sd_realize(DeviceState *dev, Error ** errp)
     if (sd->blk) {
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
-
-    sd_reset(sd);
 }
 
 static Property sd_properties[] = {
@@ -1804,6 +1803,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
     dc->realize = sd_realize;
     dc->props = sd_properties;
     dc->vmsd = &sd_vmstate;
+    dc->reset = sd_reset;
 }
 
 static const TypeInfo sd_info = {
-- 
1.9.1

[Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Maydell]

Add a QOM bus for SD cards to plug in to.

Note that since sd_enable() is used only by one board and there
only as part of a broken implementation, we do not provide it in
the SDBus API (but instead add a warning comment about the old
function). Whoever converts OMAP and the nseries boards to QOM
will need to either implement the card switch properly or move
the enable hack into the OMAP MMC controller model.

In the SDBus API, the old-style use of sd_set_cb to register some
qemu_irqs for notification of card insertion and write-protect
toggling is replaced with methods in the SDBusClass which the
card calls on status changes and methods in the SDClass which
the controller can call to find out the current status. The
query methods will allow us to remove the abuse of the 'register
irqs' API by controllers in their reset methods to trigger
the card to tell them about the current status again.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/Makefile.objs |   2 +-
 hw/sd/core.c        | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/sd/sd.c          |  46 +++++++++++++++--
 include/hw/sd/sd.h  |  60 +++++++++++++++++++++
 4 files changed, 249 insertions(+), 5 deletions(-)
 create mode 100644 hw/sd/core.c

diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index f1aed83..31c8330 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
-common-obj-$(CONFIG_SD) += sd.o
+common-obj-$(CONFIG_SD) += sd.o core.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
diff --git a/hw/sd/core.c b/hw/sd/core.c
new file mode 100644
index 0000000..584eeb5
--- /dev/null
+++ b/hw/sd/core.c
@@ -0,0 +1,146 @@
+/*
+ * SD card bus interface code.
+ *
+ * Copyright (c) 2015 Linaro Limited
+ *
+ * Author:
+ *  Peter Maydell <peter.maydell@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "sysemu/block-backend.h"
+#include "hw/sd/sd.h"
+
+static SDState *get_card(SDBus *sdbus)
+{
+    /* We only ever have one child on the bus so just return it */
+    BusChild *kid = QTAILQ_FIRST(&sdbus->qbus.children);
+
+    if (!kid) {
+        return NULL;
+    }
+    return SD(kid->child);
+}
+
+int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        return sc->do_command(card, req, response);
+    }
+
+    return 0;
+}
+
+void sdbus_write_data(SDBus *sdbus, uint8_t value)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        sc->write_data(card, value);
+    }
+}
+
+uint8_t sdbus_read_data(SDBus *sdbus)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        return sc->read_data(card);
+    }
+
+    return 0;
+}
+
+bool sdbus_data_ready(SDBus *sdbus)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        return sc->data_ready(card);
+    }
+
+    return false;
+}
+
+bool sdbus_get_inserted(SDBus *sdbus)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        return sc->get_inserted(card);
+    }
+
+    return false;
+}
+
+bool sdbus_get_readonly(SDBus *sdbus)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDClass *sc = SD_GET_CLASS(card);
+
+        return sc->get_readonly(card);
+    }
+
+    return false;
+}
+
+void sdbus_set_inserted(SDBus *sdbus, bool inserted)
+{
+    SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+    BusState *qbus = BUS(sdbus);
+
+    if (sbc->set_inserted) {
+        sbc->set_inserted(qbus->parent, inserted);
+    }
+}
+
+void sdbus_set_readonly(SDBus *sdbus, bool readonly)
+{
+    SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+    BusState *qbus = BUS(sdbus);
+
+    if (sbc->set_readonly) {
+        sbc->set_readonly(qbus->parent, readonly);
+    }
+}
+
+static const TypeInfo sd_bus_info = {
+    .name = TYPE_SD_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_size = sizeof(SDBusClass),
+};
+
+static void sd_bus_register_types(void)
+{
+    type_register_static(&sd_bus_info);
+}
+
+type_init(sd_bus_register_types)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b4a5a62..4d17029 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -430,14 +430,41 @@ static void sd_reset(DeviceState *dev)
     sd->expecting_acmd = false;
 }
 
+static bool sd_get_inserted(SDState *sd)
+{
+    return blk_is_inserted(sd->blk);
+}
+
+static bool sd_get_readonly(SDState *sd)
+{
+    return sd->wp_switch;
+}
+
 static void sd_cardchange(void *opaque, bool load)
 {
     SDState *sd = opaque;
+    DeviceState *dev = DEVICE(sd);
+    SDBus *sdbus = SD_BUS(qdev_get_parent_bus(dev));
+    bool inserted = sd_get_inserted(sd);
+    bool readonly = sd_get_readonly(sd);
 
-    qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
-    if (blk_is_inserted(sd->blk)) {
-        sd_reset(DEVICE(sd));
-        qemu_set_irq(sd->readonly_cb, sd->wp_switch);
+    if (inserted) {
+        sd_reset(dev);
+    }
+
+    /* The IRQ notification is for legacy non-QOM SD controller devices;
+     * QOMified controllers use the SDBus APIs.
+     */
+    if (sdbus) {
+        sdbus_set_inserted(sdbus, inserted);
+        if (inserted) {
+            sdbus_set_readonly(sdbus, readonly);
+        }
+    } else {
+        qemu_set_irq(sd->inserted_cb, inserted);
+        if (inserted) {
+            qemu_set_irq(sd->readonly_cb, readonly);
+        }
     }
 }
 
@@ -1799,17 +1826,28 @@ static Property sd_properties[] = {
 static void sd_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    SDClass *sc = SD_CLASS(klass);
 
     dc->realize = sd_realize;
     dc->props = sd_properties;
     dc->vmsd = &sd_vmstate;
     dc->reset = sd_reset;
+    dc->bus_type = TYPE_SD_BUS;
+
+    sc->do_command = sd_do_command;
+    sc->write_data = sd_write_data;
+    sc->read_data = sd_read_data;
+    sc->data_ready = sd_data_ready;
+    sc->enable = sd_enable;
+    sc->get_inserted = sd_get_inserted;
+    sc->get_readonly = sd_get_readonly;
 }
 
 static const TypeInfo sd_info = {
     .name = TYPE_SD,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(SDState),
+    .class_size = sizeof(SDClass),
     .class_init = sd_class_init,
     .instance_init = sd_instance_init,
 };
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 6997dfc..5e0321d 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -67,10 +67,49 @@ typedef struct {
 } SDRequest;
 
 typedef struct SDState SDState;
+typedef struct SDBus SDBus;
 
 #define TYPE_SD "sd"
 #define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
+#define SD_CLASS(klass) OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD)
+#define SD_GET_CLASS(obj) OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD)
 
+typedef struct {
+    /*< private >*/
+    DeviceClass parent_class;
+    /*< public >*/
+
+    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
+    void (*write_data)(SDState *sd, uint8_t value);
+    uint8_t (*read_data)(SDState *sd);
+    bool (*data_ready)(SDState *sd);
+    void (*enable)(SDState *sd, bool enable);
+    bool (*get_inserted)(SDState *sd);
+    bool (*get_readonly)(SDState *sd);
+} SDClass;
+
+#define TYPE_SD_BUS "sd-bus"
+#define SD_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SD_BUS)
+#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
+#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
+
+struct SDBus {
+    BusState qbus;
+};
+
+typedef struct {
+    /*< private >*/
+    BusClass parent_class;
+    /*< public >*/
+
+    /* These methods are called by the SD device to notify the controller
+     * when the card insertion or readonly status changes
+     */
+    void (*set_inserted)(DeviceState *dev, bool inserted);
+    void (*set_readonly)(DeviceState *dev, bool readonly);
+} SDBusClass;
+
+/* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response);
@@ -78,6 +117,27 @@ void sd_write_data(SDState *sd, uint8_t value);
 uint8_t sd_read_data(SDState *sd);
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
 bool sd_data_ready(SDState *sd);
+/* sd_enable should not be used -- it is only used on the nseries boards,
+ * where it is part of a broken implementation of the MMC card slot switch
+ * (there should be two card slots which are multiplexed to a single MMC
+ * controller, but instead we model it with one card and controller and
+ * disable the card when the second slot is selected, so it looks like the
+ * second slot is always empty).
+ */
 void sd_enable(SDState *sd, bool enable);
 
+/* Functions to be used by qdevified callers (working via
+ * an SDBus rather than directly with SDState)
+ */
+int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
+void sdbus_write_data(SDBus *sd, uint8_t value);
+uint8_t sdbus_read_data(SDBus *sd);
+bool sdbus_data_ready(SDBus *sd);
+bool sdbus_get_inserted(SDBus *sd);
+bool sdbus_get_readonly(SDBus *sd);
+
+/* Functions to be used by SD devices to report back to qdevified controllers */
+void sdbus_set_inserted(SDBus *sd, bool inserted);
+void sdbus_set_readonly(SDBus *sd, bool inserted);
+
 #endif	/* __hw_sd_h */
-- 
1.9.1

[Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

Update the SDHCI code to use the new SDBus APIs.

This commit introduces the new command line options required
to connect a disk to sdhci-pci:

 -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sdhci.c         | 95 +++++++++++++++++++++++++++++++++++----------------
 include/hw/sd/sdhci.h |  3 +-
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 991c9b5..17e08a2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -55,6 +55,9 @@
         } \
     } while (0)
 
+#define TYPE_SDHCI_BUS "sdhci-bus"
+#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
+
 /* Default SD/MMC host controller features information, which will be
  * presented in CAPABILITIES register of generic SD host controller at reset.
  * If not stated otherwise:
@@ -145,9 +148,9 @@ static void sdhci_raise_insertion_irq(void *opaque)
     }
 }
 
-static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
+static void sdhci_set_inserted(DeviceState *dev, bool level)
 {
-    SDHCIState *s = (SDHCIState *)opaque;
+    SDHCIState *s = (SDHCIState *)dev;
     DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
 
     if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
@@ -172,9 +175,9 @@ static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
     }
 }
 
-static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
+static void sdhci_set_readonly(DeviceState *dev, bool level)
 {
-    SDHCIState *s = (SDHCIState *)opaque;
+    SDHCIState *s = (SDHCIState *)dev;
 
     if (level) {
         s->prnsts &= ~SDHC_WRITE_PROTECT;
@@ -186,6 +189,8 @@ static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
 
 static void sdhci_reset(SDHCIState *s)
 {
+    DeviceState *dev = DEVICE(s);
+
     timer_del(s->insert_timer);
     timer_del(s->transfer_timer);
     /* Set all registers to 0. Capabilities registers are not cleared
@@ -193,7 +198,10 @@ static void sdhci_reset(SDHCIState *s)
      * initialization */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
 
-    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+    /* Reset other state based on current card insertion/readonly status */
+    sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
+    sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
+
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
 }
@@ -211,7 +219,7 @@ static void sdhci_send_command(SDHCIState *s)
     request.cmd = s->cmdreg >> 8;
     request.arg = s->argument;
     DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
-    rlen = sd_do_command(s->card, &request, response);
+    rlen = sdbus_do_command(&s->sdbus, &request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
         if (rlen == 4) {
@@ -270,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
         request.cmd = 0x0C;
         request.arg = 0;
         DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, request.arg);
-        sd_do_command(s->card, &request, response);
+        sdbus_do_command(&s->sdbus, &request, response);
         /* Auto CMD12 response goes to the upper Response register */
         s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
                 (response[2] << 8) | response[3];
@@ -302,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
     }
 
     for (index = 0; index < (s->blksize & 0x0fff); index++) {
-        s->fifo_buffer[index] = sd_read_data(s->card);
+        s->fifo_buffer[index] = sdbus_read_data(&s->sdbus);
     }
 
     /* New data now available for READ through Buffer Port Register */
@@ -395,7 +403,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
     }
 
     for (index = 0; index < (s->blksize & 0x0fff); index++) {
-        sd_write_data(s->card, s->fifo_buffer[index]);
+        sdbus_write_data(&s->sdbus, s->fifo_buffer[index]);
     }
 
     /* Next data can be written through BUFFER DATORT register */
@@ -477,7 +485,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         while (s->blkcnt) {
             if (s->data_count == 0) {
                 for (n = 0; n < block_size; n++) {
-                    s->fifo_buffer[n] = sd_read_data(s->card);
+                    s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
                 }
             }
             begin = s->data_count;
@@ -518,7 +526,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
                 for (n = 0; n < block_size; n++) {
-                    sd_write_data(s->card, s->fifo_buffer[n]);
+                    sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
                 }
                 s->data_count = 0;
                 if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
@@ -550,7 +558,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 
     if (s->trnmod & SDHC_TRNS_READ) {
         for (n = 0; n < datacnt; n++) {
-            s->fifo_buffer[n] = sd_read_data(s->card);
+            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
         }
         dma_memory_write(&address_space_memory, s->sdmasysad, s->fifo_buffer,
                          datacnt);
@@ -558,7 +566,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
         dma_memory_read(&address_space_memory, s->sdmasysad, s->fifo_buffer,
                         datacnt);
         for (n = 0; n < datacnt; n++) {
-            sd_write_data(s->card, s->fifo_buffer[n]);
+            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
         }
     }
 
@@ -662,7 +670,7 @@ static void sdhci_do_adma(SDHCIState *s)
                 while (length) {
                     if (s->data_count == 0) {
                         for (n = 0; n < block_size; n++) {
-                            s->fifo_buffer[n] = sd_read_data(s->card);
+                            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
                         }
                     }
                     begin = s->data_count;
@@ -703,7 +711,7 @@ static void sdhci_do_adma(SDHCIState *s)
                     dscr.addr += s->data_count - begin;
                     if (s->data_count == block_size) {
                         for (n = 0; n < block_size; n++) {
-                            sd_write_data(s->card, s->fifo_buffer[n]);
+                            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
                         }
                         s->data_count = 0;
                         if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
@@ -817,7 +825,7 @@ static void sdhci_data_transfer(void *opaque)
             break;
         }
     } else {
-        if ((s->trnmod & SDHC_TRNS_READ) && sd_data_ready(s->card)) {
+        if ((s->trnmod & SDHC_TRNS_READ) && sdbus_data_ready(&s->sdbus)) {
             s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
                     SDHC_DAT_LINE_ACTIVE;
             sdhci_read_block_from_card(s);
@@ -1154,15 +1162,10 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
     }
 }
 
-static void sdhci_initfn(SDHCIState *s, BlockBackend *blk)
+static void sdhci_initfn(SDHCIState *s)
 {
-    s->card = sd_init(blk, false);
-    if (s->card == NULL) {
-        exit(1);
-    }
-    s->eject_cb = qemu_allocate_irq(sdhci_insert_eject_cb, s, 0);
-    s->ro_cb = qemu_allocate_irq(sdhci_card_readonly_cb, s, 0);
-    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
+                        TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
 
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
@@ -1232,7 +1235,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     SDHCIState *s = PCI_SDHCI(dev);
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
-    sdhci_initfn(s, s->blk);
+    sdhci_initfn(s);
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
     s->irq = pci_allocate_irq(dev);
@@ -1279,11 +1282,8 @@ static Property sdhci_sysbus_properties[] = {
 static void sdhci_sysbus_init(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
-    DriveInfo *di;
 
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    di = drive_get_next(IF_SD);
-    sdhci_initfn(s, di ? blk_by_legacy_dinfo(di) : NULL);
+    sdhci_initfn(s);
 }
 
 static void sdhci_sysbus_finalize(Object *obj)
@@ -1296,6 +1296,26 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
 {
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    DriveInfo *di;
+    BlockBackend *blk;
+    DeviceState *carddev;
+
+    /* Create and plug in the sd card.
+     * FIXME: this should be done by the users of this device so we
+     * do not use drive_get_next() here.
+     */
+    di = drive_get_next(IF_SD);
+    blk = di ? blk_by_legacy_dinfo(di) : NULL;
+
+    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+    qdev_prop_set_drive(carddev, "drive", blk, errp);
+    if (*errp) {
+        return;
+    }
+    object_property_set_bool(OBJECT(carddev), true, "realized", errp);
+    if (*errp) {
+        return;
+    }
 
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
@@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
             SDHC_REGISTERS_MAP_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
+
 }
 
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
@@ -1325,10 +1346,26 @@ static const TypeInfo sdhci_sysbus_info = {
     .class_init = sdhci_sysbus_class_init,
 };
 
+static void sdhci_bus_class_init(ObjectClass *klass, void *data)
+{
+    SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+    sbc->set_inserted = sdhci_set_inserted;
+    sbc->set_readonly = sdhci_set_readonly;
+}
+
+static const TypeInfo sdhci_bus_info = {
+    .name = TYPE_SDHCI_BUS,
+    .parent = TYPE_SD_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_init = sdhci_bus_class_init,
+};
+
 static void sdhci_register_types(void)
 {
     type_register_static(&sdhci_pci_info);
     type_register_static(&sdhci_sysbus_info);
+    type_register_static(&sdhci_bus_info);
 }
 
 type_init(sdhci_register_types)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index e78d938..4816516 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -37,9 +37,8 @@ typedef struct SDHCIState {
         PCIDevice pcidev;
         SysBusDevice busdev;
     };
-    SDState *card;
+    SDBus sdbus;
     MemoryRegion iomem;
-    BlockBackend *blk;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
-- 
1.9.1

[Qemu-devel] [PATCH 06/10] sdhci_sysbus: Create SD card device in users, not the device itself

[Peter Maydell]

Move the creation of the SD card device from the sdhci_sysbus
device itself into the boards that create these devices.
This allows us to remove the cannot_instantiate_with_device_add
notation because we no longer call drive_get_next in the device
model.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/xilinx_zynq.c | 17 ++++++++++++++++-
 hw/arm/xlnx-ep108.c  | 19 +++++++++++++++++++
 hw/sd/sdhci.c        | 22 ----------------------
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1c1a445..3195055 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -27,6 +27,7 @@
 #include "hw/misc/zynq-xadc.h"
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
+#include "hw/sd/sd.h"
 
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
@@ -153,8 +154,10 @@ static void zynq_init(MachineState *machine)
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-    DeviceState *dev;
+    DeviceState *dev, *carddev;
     SysBusDevice *busdev;
+    DriveInfo *di;
+    BlockBackend *blk;
     qemu_irq pic[64];
     Error *err = NULL;
     int n;
@@ -260,11 +263,23 @@ static void zynq_init(MachineState *machine)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]);
 
+    di = drive_get_next(IF_SD);
+    blk = di ? blk_by_legacy_dinfo(di) : NULL;
+    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
+
     dev = qdev_create(NULL, "generic-sdhci");
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
 
+    di = drive_get_next(IF_SD);
+    blk = di ? blk_by_legacy_dinfo(di) : NULL;
+    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
+
     dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 85b978f..cb95d32 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -34,6 +34,7 @@ static void xlnx_ep108_init(MachineState *machine)
 {
     XlnxEP108 *s = g_new0(XlnxEP108, 1);
     Error *err = NULL;
+    int i;
 
     object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
@@ -45,6 +46,24 @@ static void xlnx_ep108_init(MachineState *machine)
         exit(1);
     }
 
+    /* Create and plug in the SD cards */
+    for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
+        BusState *bus;
+        DriveInfo *di = drive_get_next(IF_SD);
+        BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
+        DeviceState *carddev;
+
+        bus = qdev_get_child_bus(DEVICE(&s->soc.sdhci[i]), "sd-bus");
+        if (!bus) {
+            error_report("No SD bus found for SD card %d", i);
+            exit(1);
+        }
+        carddev = qdev_create(bus, TYPE_SD);
+        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+        object_property_set_bool(OBJECT(carddev), true, "realized",
+                                 &error_fatal);
+    }
+
     if (machine->ram_size > EP108_MAX_RAM_SIZE) {
         error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
                      "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 17e08a2..c8e3865 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1296,26 +1296,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
 {
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    DriveInfo *di;
-    BlockBackend *blk;
-    DeviceState *carddev;
-
-    /* Create and plug in the sd card.
-     * FIXME: this should be done by the users of this device so we
-     * do not use drive_get_next() here.
-     */
-    di = drive_get_next(IF_SD);
-    blk = di ? blk_by_legacy_dinfo(di) : NULL;
-
-    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
-    qdev_prop_set_drive(carddev, "drive", blk, errp);
-    if (*errp) {
-        return;
-    }
-    object_property_set_bool(OBJECT(carddev), true, "realized", errp);
-    if (*errp) {
-        return;
-    }
 
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
@@ -1333,8 +1313,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &sdhci_vmstate;
     dc->props = sdhci_sysbus_properties;
     dc->realize = sdhci_sysbus_realize;
-    /* Reason: instance_init() method uses drive_get_next() */
-    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sdhci_sysbus_info = {
-- 
1.9.1

[Qemu-devel] [PATCH 07/10] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

Convert the pxa2xx_mmci device to be a sysbus device.

In this commit we only change the device itself, and leave
the interface to the SD card using the old non-SDBus APIs.

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

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index b217080..b6bb390 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,60 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 BlockBackend *blk, qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma)
 {
+    DeviceState *dev;
+    SysBusDevice *sbd;
     PXA2xxMMCIState *s;
 
-    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
-    s->irq = irq;
-    s->rx_dma = rx_dma;
-    s->tx_dma = tx_dma;
-
-    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
-                          "pxa2xx-mmci", 0x00100000);
-    memory_region_add_subregion(sysmem, base, &s->iomem);
-
-    /* Instantiate the actual storage */
-    s->card = sd_init(blk, false);
+    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
+    s = PXA2XX_MMCI(dev);
+    /* Reach into the device and initialize the SD card. This is
+     * unclean but will vanish when we update to SDBus APIs.
+     */
+    s->card = sd_init(s->blk, false);
     if (s->card == NULL) {
         exit(1);
     }
-
-    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
-
+    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 s;
 }
 
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
-                qemu_irq coverswitch)
+                          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, obj, &pxa2xx_mmci_ops, s,
+                          "pxa2xx-mmci", 0x00100000);
+    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 const TypeInfo pxa2xx_mmci_info = {
+    .name = TYPE_PXA2XX_MMCI,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PXA2xxMMCIState),
+    .instance_init = pxa2xx_mmci_instance_init,
+};
+
+static void pxa2xx_mmci_register_types(void)
+{
+    type_register_static(&pxa2xx_mmci_info);
+}
+
+type_init(pxa2xx_mmci_register_types)
-- 
1.9.1

[Qemu-devel] [PATCH 08/10] hw/sd/pxa2xx_mmci: Update to use new SDBus APIs

[Peter Maydell]

Now the PXA2xx MMCI device is QOMified itself, we can
update it to use the SDBus APIs to talk to the SD card.

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

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index b6bb390..a30be2b 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -16,10 +16,14 @@
 #include "hw/sd/sd.h"
 #include "hw/qdev.h"
 #include "hw/qdev-properties.h"
+#include "qemu/error-report.h"
 
 #define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
 #define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
 
+#define TYPE_PXA2XX_MMCI_BUS "pxa2xx-mmci-bus"
+#define PXA2XX_MMCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
+
 typedef struct PXA2xxMMCIState {
     SysBusDevice parent_obj;
 
@@ -27,9 +31,11 @@ typedef struct PXA2xxMMCIState {
     qemu_irq irq;
     qemu_irq rx_dma;
     qemu_irq tx_dma;
+    qemu_irq inserted;
+    qemu_irq readonly;
 
     BlockBackend *blk;
-    SDState *card;
+    SDBus sdbus;
 
     uint32_t status;
     uint32_t clkrt;
@@ -129,7 +135,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 
     if (s->cmdat & CMDAT_WR_RD) {
         while (s->bytesleft && s->tx_len) {
-            sd_write_data(s->card, s->tx_fifo[s->tx_start ++]);
+            sdbus_write_data(&s->sdbus, s->tx_fifo[s->tx_start++]);
             s->tx_start &= 0x1f;
             s->tx_len --;
             s->bytesleft --;
@@ -139,7 +145,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
     } else
         while (s->bytesleft && s->rx_len < 32) {
             s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
-                sd_read_data(s->card);
+                sdbus_read_data(&s->sdbus);
             s->bytesleft --;
             s->intreq |= INT_RXFIFO_REQ;
         }
@@ -173,7 +179,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
     request.arg = s->arg;
     request.crc = 0;	/* FIXME */
 
-    rsplen = sd_do_command(s->card, &request, response);
+    rsplen = sdbus_do_command(&s->sdbus, &request, response);
     s->intreq |= INT_END_CMD;
 
     memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
@@ -482,32 +488,59 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 BlockBackend *blk, qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma)
 {
-    DeviceState *dev;
+    DeviceState *dev, *carddev;
     SysBusDevice *sbd;
     PXA2xxMMCIState *s;
+    Error *err = NULL;
 
     dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
     s = PXA2XX_MMCI(dev);
-    /* Reach into the device and initialize the SD card. This is
-     * unclean but will vanish when we update to SDBus APIs.
-     */
-    s->card = sd_init(s->blk, false);
-    if (s->card == NULL) {
-        exit(1);
-    }
-    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);
+
+    /* Create and plug in the sd card */
+    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
+    qdev_prop_set_drive(carddev, "drive", blk, &err);
+    if (err) {
+        error_report("failed to init SD card: %s", error_get_pretty(err));
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(carddev), true, "realized", &err);
+    if (err) {
+        error_report("failed to init SD card: %s", error_get_pretty(err));
+        return NULL;
+    }
+
     return s;
 }
 
+static void pxa2xx_mmci_set_inserted(DeviceState *dev, bool inserted)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
+
+    qemu_set_irq(s->inserted, inserted);
+}
+
+static void pxa2xx_mmci_set_readonly(DeviceState *dev, bool readonly)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
+
+    qemu_set_irq(s->readonly, readonly);
+}
+
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
                           qemu_irq coverswitch)
 {
-    sd_set_cb(s->card, readonly, coverswitch);
+    DeviceState *dev = DEVICE(s);
+
+    s->readonly = readonly;
+    s->inserted = coverswitch;
+
+    pxa2xx_mmci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
+    pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
 }
 
 static void pxa2xx_mmci_instance_init(Object *obj)
@@ -524,6 +557,17 @@ static void pxa2xx_mmci_instance_init(Object *obj)
 
     register_savevm(NULL, "pxa2xx_mmci", 0, 0,
                     pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+
+    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
+                        TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
+}
+
+static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
+{
+    SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+    sbc->set_inserted = pxa2xx_mmci_set_inserted;
+    sbc->set_readonly = pxa2xx_mmci_set_readonly;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
@@ -533,9 +577,17 @@ static const TypeInfo pxa2xx_mmci_info = {
     .instance_init = pxa2xx_mmci_instance_init,
 };
 
+static const TypeInfo pxa2xx_mmci_bus_info = {
+    .name = TYPE_PXA2XX_MMCI_BUS,
+    .parent = TYPE_SD_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_init = pxa2xx_mmci_bus_class_init,
+};
+
 static void pxa2xx_mmci_register_types(void)
 {
     type_register_static(&pxa2xx_mmci_info);
+    type_register_static(&pxa2xx_mmci_bus_info);
 }
 
 type_init(pxa2xx_mmci_register_types)
-- 
1.9.1

[Qemu-devel] [PATCH 09/10] 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 | 148 ++++++++++++++++++++--------------------------------
 1 file changed, 56 insertions(+), 92 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index a30be2b..81fec4d 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -43,27 +43,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 */
@@ -405,84 +450,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,
@@ -555,9 +522,6 @@ static void pxa2xx_mmci_instance_init(Object *obj)
     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);
-
     qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
                         TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
 }
-- 
1.9.1

[Qemu-devel] [PATCH 10/10] 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 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 81fec4d..3df927e 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -510,6 +510,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
     pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
 }
 
+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);
@@ -526,6 +555,13 @@ static void pxa2xx_mmci_instance_init(Object *obj)
                         TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
 }
 
+static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = pxa2xx_mmci_reset;
+}
+
 static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);
@@ -539,6 +575,7 @@ static const TypeInfo pxa2xx_mmci_info = {
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PXA2xxMMCIState),
     .instance_init = pxa2xx_mmci_instance_init,
+    .class_init = pxa2xx_mmci_class_init,
 };
 
 static const TypeInfo pxa2xx_mmci_bus_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Kevin O'Connor]

On Fri, Dec 11, 2015 at 04:37:06PM +0000, Peter Maydell wrote:
> Update the SDHCI code to use the new SDBus APIs.
> 
> This commit introduces the new command line options required
> to connect a disk to sdhci-pci:
> 
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

I can't review in depth right now, but I did notice

[...]
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -55,6 +55,9 @@
>          } \
>      } while (0)
>  
> +#define TYPE_SDHCI_BUS "sdhci-bus"
> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)

the above PXA2XX typo

[...]
> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>      memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>              SDHC_REGISTERS_MAP_SIZE);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
>  }

and the above white space damage.

-Kevin

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

On 11 December 2015 at 19:01, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Fri, Dec 11, 2015 at 04:37:06PM +0000, Peter Maydell wrote:
>> Update the SDHCI code to use the new SDBus APIs.
>>
>> This commit introduces the new command line options required
>> to connect a disk to sdhci-pci:
>>
>>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
>
> I can't review in depth right now, but I did notice
>
> [...]
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -55,6 +55,9 @@
>>          } \
>>      } while (0)
>>
>> +#define TYPE_SDHCI_BUS "sdhci-bus"
>> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
>
> the above PXA2XX typo

Oops. We never actually use this macro, so I didn't notice the
cut-n-paste error.

> [...]
>> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>>      memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>>              SDHC_REGISTERS_MAP_SIZE);
>>      sysbus_init_mmio(sbd, &s->iomem);
>> +
>>  }
>
> and the above white space damage.

Will fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/10] hw/sd: QOMify sd.c (and pxa2xx_mmci)

[Alistair Francis]

+Sai Pavan

On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> This series attempts to QOMify sd.c (the actual SD card model),
> including a proper QOM bus between the controller and the card.
>
> This series removes the experimental x-drive property on sdhci-pci;
> the syntax for using that device changes:
>
> instead of using the x-drive property:
>
>   -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]
>
> we create a specific sd device (which is autoplugged into the
> sdhci-pci device's "sd-bus" bus if you don't manually connect them):
>
>   -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
>
>
> The basic structure of the patch series is:
>  * QOMify sd.c itself
>  * Add the QOM bus APIs
>  * Convert sdhci to use the QOM bus APIs
>  * QOMify pxa2xx_mmci
>  * Convert pxa2xx_mmci to use the QOM bus APIs
>
>
> Points worthy of review:
>  * is the code in "sdhci_sysbus: Create SD card device in users"
>    for xlnx-ep108.c to connect the SD cards up to the two controllers
>    inside the SoC doing this in the right way?
>  * I chose not to try to make the SD card type a subclass of
>    SSI, because the interface we have at the moment didn't really
>    seem to line up with what SSI has
>  * generally do I have the QOM style right?
>
>
> Some notes on compatibility/bisection:
>  * the old-style non-QOMified controllers (which get their drive
>    via blk_by_legacy_dinfo() continue to work through the whole series
>  * the only QOMified controller which doesn't do that is sdhci-pci,
>    which uses the experimental x-drive property to take a drive. This
>    support and property is removed in patch 1; support for the new
>    syntax is added in patch 5.
>    (Since we're breaking the old syntax anyway I chose to not try to
>    introduce the new syntax in the same commit as breaking the old one;
>    I think it makes the patches easier to review.)
>
>
> I don't have any Xilinx test images, so I haven't been able to test
> the changes to those boards (beyond confirming that the board doesn't
> crash on startup and that 'info qtree' seems to show SD cards
> connected to the right things). I have tested sdhci-pci and
> the pxa2xx.
>
> Peter Maydell (10):
>   hw/sd/sdhci.c: Remove x-drive property
>   hw/sd/sd.c: QOMify
>   hw/sd/sd.c: Convert sd_reset() function into Device reset method
>   hw/sd: Add QOM bus which SD cards plug in to
>   hw/sd/sdhci.c: Update to use SDBus APIs
>   sdhci_sysbus: Create SD card device in users, not the device itself
>   hw/sd/pxa2xx_mmci: convert to SysBusDevice object
>   hw/sd/pxa2xx_mmci: Update to use new SDBus APIs
>   hw/sd/pxa2xx_mmci: Convert to VMStateDescription
>   hw/sd/pxa2xx_mmci: Add reset function
>
>  hw/arm/xilinx_zynq.c  |  17 ++-
>  hw/arm/xlnx-ep108.c   |  19 ++++
>  hw/sd/Makefile.objs   |   2 +-
>  hw/sd/core.c          | 146 ++++++++++++++++++++++++
>  hw/sd/pxa2xx_mmci.c   | 304 ++++++++++++++++++++++++++++++++------------------
>  hw/sd/sd.c            | 149 ++++++++++++++++++++-----
>  hw/sd/sdhci.c         |  83 ++++++++------
>  include/hw/sd/sd.h    |  63 +++++++++++
>  include/hw/sd/sdhci.h |   3 +-
>  9 files changed, 610 insertions(+), 176 deletions(-)
>  create mode 100644 hw/sd/core.c
>
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 01/10] hw/sd/sdhci.c: Remove x-drive property

[Alistair Francis]

On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The following commits will remove support for the old sdhci-pci
> command line syntax using the x-drive property:
>  -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]
> and replace it with an explicit sd device:
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
>
> (This is OK because x-drive is experimental.)
>
> This commit removes the x-drive property so that old style
> command lines will fail with a reasonable error message:
>   -device sdhci-pci,x-drive=mydrive: Property '.x-drive' not found
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I agree with the way you have split up the patches. I don't think it
is worth maintaining compatibility over the series.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/sd/sdhci.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8612760..991c9b5 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1221,12 +1221,6 @@ const VMStateDescription sdhci_vmstate = {
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
>  static Property sdhci_pci_properties[] = {
> -    /*
> -     * We currently fuse controller and card into a single device
> -     * model, but we intend to separate them.  For that purpose, the
> -     * properties that belong to the card are marked as experimental.
> -     */
> -    DEFINE_PROP_DRIVE("x-drive", SDHCIState, blk),
>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Alistair Francis]

On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Turn the SD card into a QOM device.
> This conversion only changes the device itself; the various
> functions which are effectively methods on the device are not
> touched at this point.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/sd.c         | 99 ++++++++++++++++++++++++++++++++++++++++++------------
>  include/hw/sd/sd.h |  3 ++
>  2 files changed, 80 insertions(+), 22 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a9935c..7c79217 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -33,6 +33,8 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/bitmap.h"
> +#include "hw/qdev-properties.h"
> +#include "qemu/error-report.h"
>
>  //#define DEBUG_SD 1
>
> @@ -77,6 +79,8 @@ enum SDCardStates {
>  };
>
>  struct SDState {
> +    DeviceState parent_obj;
> +
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
>      uint32_t ocr;
> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
>      }
>  };
>
> -/* We do not model the chip select pin, so allow the board to select
> -   whether card should be in SSI or MMC/SD mode.  It is also up to the
> -   board to ensure that ssi transfers only occur when the chip select
> -   is asserted.  */
> +/* Legacy initialization function for use by non-qdevified callers */
>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>  {
> -    SDState *sd;
> +    DeviceState *dev;
> +    Error *err = NULL;
>
> -    if (blk && blk_is_read_only(blk)) {
> -        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
> +    dev = qdev_create(NULL, TYPE_SD);
> +    qdev_prop_set_drive(dev, "drive", blk, &err);
> +    if (err) {
> +        error_report("sd_init failed: %s", error_get_pretty(err));
>          return NULL;
>      }
> -
> -    sd = (SDState *) g_malloc0(sizeof(SDState));
> -    sd->buf = blk_blockalign(blk, 512);
> -    sd->spi = is_spi;
> -    sd->enable = true;
> -    sd->blk = blk;
> -    sd_reset(sd);
> -    if (sd->blk) {
> -        /* Attach dev if not already attached.  (This call ignores an
> -         * error return code if sd->blk is already attached.) */
> -        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
> -        blk_attach_dev(sd->blk, sd);

This functionality is removed with this patch. If the block is not
already attached won't this cause errors?

> -        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
> +    qdev_prop_set_bit(dev, "spi", is_spi);
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_report("sd_init failed: %s", error_get_pretty(err));
> +        return NULL;
>      }
> -    vmstate_register(NULL, -1, &sd_vmstate, sd);
> -    return sd;
> +
> +    return SD(dev);
>  }
>
>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
>  {
>      sd->enable = enable;
>  }
> +
> +static void sd_instance_init(Object *obj)
> +{
> +    SDState *sd = SD(obj);
> +
> +    sd->enable = true;
> +}
> +
> +static void sd_realize(DeviceState *dev, Error ** errp)

You have one too many spaces here.

Thanks,

Alistair

> +{
> +    SDState *sd = SD(dev);
> +
> +    if (sd->blk && blk_is_read_only(sd->blk)) {
> +        error_setg(errp, "Cannot use read-only drive as SD card");
> +        return;
> +    }
> +
> +    sd->buf = blk_blockalign(sd->blk, 512);
> +
> +    if (sd->blk) {
> +        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
> +    }
> +
> +    sd_reset(sd);
> +}
> +
> +static Property sd_properties[] = {
> +    DEFINE_PROP_DRIVE("drive", SDState, blk),
> +    /* We do not model the chip select pin, so allow the board to select
> +     * whether card should be in SSI or MMC/SD mode.  It is also up to the
> +     * board to ensure that ssi transfers only occur when the chip select
> +     * is asserted.  */
> +    DEFINE_PROP_BOOL("spi", SDState, spi, false),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void sd_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = sd_realize;
> +    dc->props = sd_properties;
> +    dc->vmsd = &sd_vmstate;
> +}
> +
> +static const TypeInfo sd_info = {
> +    .name = TYPE_SD,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SDState),
> +    .class_init = sd_class_init,
> +    .instance_init = sd_instance_init,
> +};
> +
> +static void sd_register_types(void)
> +{
> +    type_register_static(&sd_info);
> +}
> +
> +type_init(sd_register_types)
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 79adb5b..6997dfc 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -68,6 +68,9 @@ typedef struct {
>
>  typedef struct SDState SDState;
>
> +#define TYPE_SD "sd"
> +#define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
> +
>  SDState *sd_init(BlockBackend *bs, bool is_spi);
>  int sd_do_command(SDState *sd, SDRequest *req,
>                    uint8_t *response);
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Maydell]

On 17 December 2015 at 21:45, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Turn the SD card into a QOM device.
>> This conversion only changes the device itself; the various
>> functions which are effectively methods on the device are not
>> touched at this point.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
>>      }
>>  };
>>
>> -/* We do not model the chip select pin, so allow the board to select
>> -   whether card should be in SSI or MMC/SD mode.  It is also up to the
>> -   board to ensure that ssi transfers only occur when the chip select
>> -   is asserted.  */
>> +/* Legacy initialization function for use by non-qdevified callers */
>>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>>  {
>> -    SDState *sd;
>> +    DeviceState *dev;
>> +    Error *err = NULL;
>>
>> -    if (blk && blk_is_read_only(blk)) {
>> -        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
>> +    dev = qdev_create(NULL, TYPE_SD);
>> +    qdev_prop_set_drive(dev, "drive", blk, &err);
>> +    if (err) {
>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>>          return NULL;
>>      }
>> -
>> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>> -    sd->buf = blk_blockalign(blk, 512);
>> -    sd->spi = is_spi;
>> -    sd->enable = true;
>> -    sd->blk = blk;
>> -    sd_reset(sd);
>> -    if (sd->blk) {
>> -        /* Attach dev if not already attached.  (This call ignores an
>> -         * error return code if sd->blk is already attached.) */
>> -        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
>> -        blk_attach_dev(sd->blk, sd);
>
> This functionality is removed with this patch. If the block is not
> already attached won't this cause errors?

The block backend is attached when we set the "drive" property
(which we do in this function in the new code just above).
[the actual call to blk_attach_dev() is in parse_drive() in
hw/core/qdev-properties-system.c.]

The blk_set_dev_ops() moves in this patch to sd_realize().

There is incidentally no longer any case where the block backend
could be already attached when we call sd_init(), because the
only case there was when it had been attached to the sdhci
controller device because of the x-drive property on that device,
and we removed the property in the previous patch. It's exactly
because setting a drive property does an implicit blk_attach_dev
that this code previously had to have special casing for
"attach of an already attached blk".

>> -        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>> +    qdev_prop_set_bit(dev, "spi", is_spi);
>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> +    if (err) {
>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>> +        return NULL;
>>      }
>> -    vmstate_register(NULL, -1, &sd_vmstate, sd);
>> -    return sd;
>> +
>> +    return SD(dev);
>>  }
>>
>>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
>>  {
>>      sd->enable = enable;
>>  }
>> +
>> +static void sd_instance_init(Object *obj)
>> +{
>> +    SDState *sd = SD(obj);
>> +
>> +    sd->enable = true;
>> +}
>> +
>> +static void sd_realize(DeviceState *dev, Error ** errp)
>
> You have one too many spaces here.

Yep, will fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] hw/sd/sd.c: Convert sd_reset() function into Device reset method

[Alistair Francis]

On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Convert the sd_reset() function into a proper Device reset method.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/sd/sd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 7c79217..b4a5a62 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -393,8 +393,9 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
>      return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
>  }
>
> -static void sd_reset(SDState *sd)
> +static void sd_reset(DeviceState *dev)
>  {
> +    SDState *sd = SD(dev);
>      uint64_t size;
>      uint64_t sect;
>
> @@ -435,7 +436,7 @@ static void sd_cardchange(void *opaque, bool load)
>
>      qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
>      if (blk_is_inserted(sd->blk)) {
> -        sd_reset(sd);
> +        sd_reset(DEVICE(sd));
>          qemu_set_irq(sd->readonly_cb, sd->wp_switch);
>      }
>  }
> @@ -677,7 +678,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>
>          default:
>              sd->state = sd_idle_state;
> -            sd_reset(sd);
> +            sd_reset(DEVICE(sd));
>              return sd->spi ? sd_r1 : sd_r0;
>          }
>          break;
> @@ -1783,8 +1784,6 @@ static void sd_realize(DeviceState *dev, Error ** errp)
>      if (sd->blk) {
>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>      }
> -
> -    sd_reset(sd);
>  }
>
>  static Property sd_properties[] = {
> @@ -1804,6 +1803,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
>      dc->realize = sd_realize;
>      dc->props = sd_properties;
>      dc->vmsd = &sd_vmstate;
> +    dc->reset = sd_reset;
>  }
>
>  static const TypeInfo sd_info = {
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 06/10] sdhci_sysbus: Create SD card device in users, not the device itself

[Alistair Francis]

On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Move the creation of the SD card device from the sdhci_sysbus
> device itself into the boards that create these devices.
> This allows us to remove the cannot_instantiate_with_device_add
> notation because we no longer call drive_get_next in the device
> model.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/xilinx_zynq.c | 17 ++++++++++++++++-
>  hw/arm/xlnx-ep108.c  | 19 +++++++++++++++++++
>  hw/sd/sdhci.c        | 22 ----------------------
>  3 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 1c1a445..3195055 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -27,6 +27,7 @@
>  #include "hw/misc/zynq-xadc.h"
>  #include "hw/ssi.h"
>  #include "qemu/error-report.h"
> +#include "hw/sd/sd.h"
>
>  #define NUM_SPI_FLASHES 4
>  #define NUM_QSPI_FLASHES 2
> @@ -153,8 +154,10 @@ static void zynq_init(MachineState *machine)
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
> -    DeviceState *dev;
> +    DeviceState *dev, *carddev;
>      SysBusDevice *busdev;
> +    DriveInfo *di;
> +    BlockBackend *blk;
>      qemu_irq pic[64];
>      Error *err = NULL;
>      int n;
> @@ -260,11 +263,23 @@ static void zynq_init(MachineState *machine)
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000);
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]);
>
> +    di = drive_get_next(IF_SD);
> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> +
>      dev = qdev_create(NULL, "generic-sdhci");
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
>
> +    di = drive_get_next(IF_SD);
> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> +
>      dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 85b978f..cb95d32 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -34,6 +34,7 @@ static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>      Error *err = NULL;
> +    int i;
>
>      object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> @@ -45,6 +46,24 @@ static void xlnx_ep108_init(MachineState *machine)
>          exit(1);
>      }
>
> +    /* Create and plug in the SD cards */
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
> +        BusState *bus;
> +        DriveInfo *di = drive_get_next(IF_SD);
> +        BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +        DeviceState *carddev;
> +
> +        bus = qdev_get_child_bus(DEVICE(&s->soc.sdhci[i]), "sd-bus");

This looks like the same thing I was trying to avoid with my SPI
patches. We were trying to avoid the machine reaching into the SoC
when getting the child busses. Instead expose the bus to the SoC so
the board can just get it straight from there.

> +        if (!bus) {
> +            error_report("No SD bus found for SD card %d", i);
> +            exit(1);
> +        }
> +        carddev = qdev_create(bus, TYPE_SD);
> +        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +        object_property_set_bool(OBJECT(carddev), true, "realized",
> +                                 &error_fatal);
> +    }
> +

I also think this should go after the other memory creation, not before.

Thanks,

Alistair

>      if (machine->ram_size > EP108_MAX_RAM_SIZE) {
>          error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
>                       "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 17e08a2..c8e3865 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1296,26 +1296,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>  {
>      SDHCIState *s = SYSBUS_SDHCI(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    DriveInfo *di;
> -    BlockBackend *blk;
> -    DeviceState *carddev;
> -
> -    /* Create and plug in the sd card.
> -     * FIXME: this should be done by the users of this device so we
> -     * do not use drive_get_next() here.
> -     */
> -    di = drive_get_next(IF_SD);
> -    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> -
> -    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> -    qdev_prop_set_drive(carddev, "drive", blk, errp);
> -    if (*errp) {
> -        return;
> -    }
> -    object_property_set_bool(OBJECT(carddev), true, "realized", errp);
> -    if (*errp) {
> -        return;
> -    }
>
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
> @@ -1333,8 +1313,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &sdhci_vmstate;
>      dc->props = sdhci_sysbus_properties;
>      dc->realize = sdhci_sysbus_realize;
> -    /* Reason: instance_init() method uses drive_get_next() */
> -    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>
>  static const TypeInfo sdhci_sysbus_info = {
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Alistair Francis]

On Thu, Dec 17, 2015 at 2:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 December 2015 at 21:45, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Turn the SD card into a QOM device.
>>> This conversion only changes the device itself; the various
>>> functions which are effectively methods on the device are not
>>> touched at this point.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>>> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
>>>      }
>>>  };
>>>
>>> -/* We do not model the chip select pin, so allow the board to select
>>> -   whether card should be in SSI or MMC/SD mode.  It is also up to the
>>> -   board to ensure that ssi transfers only occur when the chip select
>>> -   is asserted.  */
>>> +/* Legacy initialization function for use by non-qdevified callers */
>>>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>>>  {
>>> -    SDState *sd;
>>> +    DeviceState *dev;
>>> +    Error *err = NULL;
>>>
>>> -    if (blk && blk_is_read_only(blk)) {
>>> -        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
>>> +    dev = qdev_create(NULL, TYPE_SD);
>>> +    qdev_prop_set_drive(dev, "drive", blk, &err);
>>> +    if (err) {
>>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>>>          return NULL;
>>>      }
>>> -
>>> -    sd = (SDState *) g_malloc0(sizeof(SDState));
>>> -    sd->buf = blk_blockalign(blk, 512);
>>> -    sd->spi = is_spi;
>>> -    sd->enable = true;
>>> -    sd->blk = blk;
>>> -    sd_reset(sd);
>>> -    if (sd->blk) {
>>> -        /* Attach dev if not already attached.  (This call ignores an
>>> -         * error return code if sd->blk is already attached.) */
>>> -        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
>>> -        blk_attach_dev(sd->blk, sd);
>>
>> This functionality is removed with this patch. If the block is not
>> already attached won't this cause errors?
>
> The block backend is attached when we set the "drive" property
> (which we do in this function in the new code just above).
> [the actual call to blk_attach_dev() is in parse_drive() in
> hw/core/qdev-properties-system.c.]
>
> The blk_set_dev_ops() moves in this patch to sd_realize().
>
> There is incidentally no longer any case where the block backend
> could be already attached when we call sd_init(), because the
> only case there was when it had been attached to the sdhci
> controller device because of the x-drive property on that device,
> and we removed the property in the previous patch. It's exactly
> because setting a drive property does an implicit blk_attach_dev
> that this code previously had to have special casing for
> "attach of an already attached blk".

Ok, fair enough, just thought I would check. The rest of the logic
looks good then.

Thanks,

Alistair

>
>>> -        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>>> +    qdev_prop_set_bit(dev, "spi", is_spi);
>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>> +    if (err) {
>>> +        error_report("sd_init failed: %s", error_get_pretty(err));
>>> +        return NULL;
>>>      }
>>> -    vmstate_register(NULL, -1, &sd_vmstate, sd);
>>> -    return sd;
>>> +
>>> +    return SD(dev);
>>>  }
>>>
>>>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
>>>  {
>>>      sd->enable = enable;
>>>  }
>>> +
>>> +static void sd_instance_init(Object *obj)
>>> +{
>>> +    SDState *sd = SD(obj);
>>> +
>>> +    sd->enable = true;
>>> +}
>>> +
>>> +static void sd_realize(DeviceState *dev, Error ** errp)
>>
>> You have one too many spaces here.
>
> Yep, will fix.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 06/10] sdhci_sysbus: Create SD card device in users, not the device itself

[Peter Maydell]

On 18 December 2015 at 00:18, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> +    /* Create and plug in the SD cards */
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
>> +        BusState *bus;
>> +        DriveInfo *di = drive_get_next(IF_SD);
>> +        BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> +        DeviceState *carddev;
>> +
>> +        bus = qdev_get_child_bus(DEVICE(&s->soc.sdhci[i]), "sd-bus");
>
> This looks like the same thing I was trying to avoid with my SPI
> patches. We were trying to avoid the machine reaching into the SoC
> when getting the child busses. Instead expose the bus to the SoC so
> the board can just get it straight from there.

Yes, I wrote this code first and then saw your patches second.
Whatever we do, we should deal with the problem the same way.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/10] hw/sd/sdhci.c: Remove x-drive property

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:02PM +0000, Peter Maydell wrote:
> The following commits will remove support for the old sdhci-pci
> command line syntax using the x-drive property:
>  -device sdhci-pci,x-drive=mydrive -drive id=mydrive,[...]
> and replace it with an explicit sd device:
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
> 
> (This is OK because x-drive is experimental.)
> 
> This commit removes the x-drive property so that old style
> command lines will fail with a reasonable error message:
>   -device sdhci-pci,x-drive=mydrive: Property '.x-drive' not found
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/sd/sdhci.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8612760..991c9b5 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1221,12 +1221,6 @@ const VMStateDescription sdhci_vmstate = {
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
>  static Property sdhci_pci_properties[] = {
> -    /*
> -     * We currently fuse controller and card into a single device
> -     * model, but we intend to separate them.  For that purpose, the
> -     * properties that belong to the card are marked as experimental.
> -     */
> -    DEFINE_PROP_DRIVE("x-drive", SDHCIState, blk),
>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:03PM +0000, Peter Maydell wrote:
> Turn the SD card into a QOM device.
> This conversion only changes the device itself; the various
> functions which are effectively methods on the device are not
> touched at this point.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/sd.c         | 99 ++++++++++++++++++++++++++++++++++++++++++------------
>  include/hw/sd/sd.h |  3 ++
>  2 files changed, 80 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a9935c..7c79217 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -33,6 +33,8 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/bitmap.h"
> +#include "hw/qdev-properties.h"
> +#include "qemu/error-report.h"
>  
>  //#define DEBUG_SD 1
>  
> @@ -77,6 +79,8 @@ enum SDCardStates {
>  };
>  
>  struct SDState {
> +    DeviceState parent_obj;
> +
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
>      uint32_t ocr;
> @@ -472,34 +476,26 @@ static const VMStateDescription sd_vmstate = {
>      }
>  };
>  
> -/* We do not model the chip select pin, so allow the board to select
> -   whether card should be in SSI or MMC/SD mode.  It is also up to the
> -   board to ensure that ssi transfers only occur when the chip select
> -   is asserted.  */
> +/* Legacy initialization function for use by non-qdevified callers */
>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>  {
> -    SDState *sd;
> +    DeviceState *dev;
> +    Error *err = NULL;
>  
> -    if (blk && blk_is_read_only(blk)) {
> -        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
> +    dev = qdev_create(NULL, TYPE_SD);
> +    qdev_prop_set_drive(dev, "drive", blk, &err);
> +    if (err) {
> +        error_report("sd_init failed: %s", error_get_pretty(err));
>          return NULL;
>      }
> -
> -    sd = (SDState *) g_malloc0(sizeof(SDState));
> -    sd->buf = blk_blockalign(blk, 512);
> -    sd->spi = is_spi;
> -    sd->enable = true;
> -    sd->blk = blk;
> -    sd_reset(sd);
> -    if (sd->blk) {
> -        /* Attach dev if not already attached.  (This call ignores an
> -         * error return code if sd->blk is already attached.) */
> -        /* FIXME ignoring blk_attach_dev() failure is dangerously brittle */
> -        blk_attach_dev(sd->blk, sd);
> -        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
> +    qdev_prop_set_bit(dev, "spi", is_spi);
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_report("sd_init failed: %s", error_get_pretty(err));
> +        return NULL;
>      }
> -    vmstate_register(NULL, -1, &sd_vmstate, sd);
> -    return sd;
> +
> +    return SD(dev);
>  }
>  
>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> @@ -1765,3 +1761,62 @@ void sd_enable(SDState *sd, bool enable)
>  {
>      sd->enable = enable;
>  }
> +
> +static void sd_instance_init(Object *obj)
> +{
> +    SDState *sd = SD(obj);
> +
> +    sd->enable = true;
> +}
> +
> +static void sd_realize(DeviceState *dev, Error ** errp)
> +{
> +    SDState *sd = SD(dev);
> +
> +    if (sd->blk && blk_is_read_only(sd->blk)) {
> +        error_setg(errp, "Cannot use read-only drive as SD card");
> +        return;
> +    }
> +
> +    sd->buf = blk_blockalign(sd->blk, 512);
> +
> +    if (sd->blk) {
> +        blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
> +    }
> +
> +    sd_reset(sd);
> +}
> +
> +static Property sd_properties[] = {
> +    DEFINE_PROP_DRIVE("drive", SDState, blk),
> +    /* We do not model the chip select pin, so allow the board to select
> +     * whether card should be in SSI or MMC/SD mode.  It is also up to the
> +     * board to ensure that ssi transfers only occur when the chip select
> +     * is asserted.  */
> +    DEFINE_PROP_BOOL("spi", SDState, spi, false),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void sd_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = sd_realize;
> +    dc->props = sd_properties;
> +    dc->vmsd = &sd_vmstate;
> +}
> +
> +static const TypeInfo sd_info = {
> +    .name = TYPE_SD,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SDState),
> +    .class_init = sd_class_init,
> +    .instance_init = sd_instance_init,
> +};
> +
> +static void sd_register_types(void)
> +{
> +    type_register_static(&sd_info);
> +}
> +
> +type_init(sd_register_types)
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 79adb5b..6997dfc 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -68,6 +68,9 @@ typedef struct {
>  
>  typedef struct SDState SDState;
>  
> +#define TYPE_SD "sd"

Can we get "card" in there? I think unqualified SD should be usable to refer
to the bus standard.

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> +#define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
> +
>  SDState *sd_init(BlockBackend *bs, bool is_spi);
>  int sd_do_command(SDState *sd, SDRequest *req,
>                    uint8_t *response);
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 03/10] hw/sd/sd.c: Convert sd_reset() function into Device reset method

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:04PM +0000, Peter Maydell wrote:
> Convert the sd_reset() function into a proper Device reset method.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/sd/sd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 7c79217..b4a5a62 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -393,8 +393,9 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
>      return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
>  }
>  
> -static void sd_reset(SDState *sd)
> +static void sd_reset(DeviceState *dev)
>  {
> +    SDState *sd = SD(dev);
>      uint64_t size;
>      uint64_t sect;
>  
> @@ -435,7 +436,7 @@ static void sd_cardchange(void *opaque, bool load)
>  
>      qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
>      if (blk_is_inserted(sd->blk)) {
> -        sd_reset(sd);
> +        sd_reset(DEVICE(sd));
>          qemu_set_irq(sd->readonly_cb, sd->wp_switch);
>      }
>  }
> @@ -677,7 +678,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>  
>          default:
>              sd->state = sd_idle_state;
> -            sd_reset(sd);
> +            sd_reset(DEVICE(sd));
>              return sd->spi ? sd_r1 : sd_r0;
>          }
>          break;
> @@ -1783,8 +1784,6 @@ static void sd_realize(DeviceState *dev, Error ** errp)
>      if (sd->blk) {
>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>      }
> -
> -    sd_reset(sd);
>  }
>  
>  static Property sd_properties[] = {
> @@ -1804,6 +1803,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
>      dc->realize = sd_realize;
>      dc->props = sd_properties;
>      dc->vmsd = &sd_vmstate;
> +    dc->reset = sd_reset;
>  }
>  
>  static const TypeInfo sd_info = {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:05PM +0000, Peter Maydell wrote:
> Add a QOM bus for SD cards to plug in to.
> 
> Note that since sd_enable() is used only by one board and there
> only as part of a broken implementation, we do not provide it in
> the SDBus API (but instead add a warning comment about the old
> function). Whoever converts OMAP and the nseries boards to QOM
> will need to either implement the card switch properly or move
> the enable hack into the OMAP MMC controller model.
> 
> In the SDBus API, the old-style use of sd_set_cb to register some
> qemu_irqs for notification of card insertion and write-protect
> toggling is replaced with methods in the SDBusClass which the
> card calls on status changes and methods in the SDClass which
> the controller can call to find out the current status. The
> query methods will allow us to remove the abuse of the 'register
> irqs' API by controllers in their reset methods to trigger
> the card to tell them about the current status again.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/Makefile.objs |   2 +-
>  hw/sd/core.c        | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/sd/sd.c          |  46 +++++++++++++++--
>  include/hw/sd/sd.h  |  60 +++++++++++++++++++++
>  4 files changed, 249 insertions(+), 5 deletions(-)
>  create mode 100644 hw/sd/core.c
> 
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index f1aed83..31c8330 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -1,6 +1,6 @@
>  common-obj-$(CONFIG_PL181) += pl181.o
>  common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
> -common-obj-$(CONFIG_SD) += sd.o
> +common-obj-$(CONFIG_SD) += sd.o core.o
>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>  
>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> new file mode 100644
> index 0000000..584eeb5
> --- /dev/null
> +++ b/hw/sd/core.c
> @@ -0,0 +1,146 @@
> +/*
> + * SD card bus interface code.
> + *
> + * Copyright (c) 2015 Linaro Limited
> + *
> + * Author:
> + *  Peter Maydell <peter.maydell@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev-core.h"
> +#include "sysemu/block-backend.h"
> +#include "hw/sd/sd.h"
> +
> +static SDState *get_card(SDBus *sdbus)
> +{
> +    /* We only ever have one child on the bus so just return it */
> +    BusChild *kid = QTAILQ_FIRST(&sdbus->qbus.children);
> +
> +    if (!kid) {
> +        return NULL;
> +    }
> +    return SD(kid->child);
> +}
> +
> +int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        return sc->do_command(card, req, response);
> +    }
> +
> +    return 0;
> +}
> +
> +void sdbus_write_data(SDBus *sdbus, uint8_t value)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        sc->write_data(card, value);
> +    }
> +}
> +
> +uint8_t sdbus_read_data(SDBus *sdbus)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        return sc->read_data(card);
> +    }
> +
> +    return 0;
> +}
> +
> +bool sdbus_data_ready(SDBus *sdbus)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        return sc->data_ready(card);
> +    }
> +
> +    return false;
> +}
> +
> +bool sdbus_get_inserted(SDBus *sdbus)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        return sc->get_inserted(card);
> +    }
> +
> +    return false;
> +}

This I am not sure about. Realistically, the card has no self
awareness of its ejection state so it can't be polled for "are
you there". The card insertion switch is implemented as a
physical switch on the slot itself and a property of the bus.

The absence on presence of a device should determine this, making me
think this should return !!card.

Unfortunately, we have the case of the block UI being able to trigger a
card ejection from underneath the bus level. But since the SD card is already
using qdev_get_parent_bus() the removal from the bus can be managed at the
card level.

> +
> +bool sdbus_get_readonly(SDBus *sdbus)
> +{
> +    SDState *card = get_card(sdbus);
> +
> +    if (card) {
> +        SDClass *sc = SD_GET_CLASS(card);
> +
> +        return sc->get_readonly(card);
> +    }
> +
> +    return false;
> +}
> +
> +void sdbus_set_inserted(SDBus *sdbus, bool inserted)
> +{
> +    SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
> +    BusState *qbus = BUS(sdbus);
> +
> +    if (sbc->set_inserted) {
> +        sbc->set_inserted(qbus->parent, inserted);
> +    }
> +}
> +

And following on from the bus removals, would this instead be a hotplug
handler? qdev_unplug allows for callbacks to be registered for bus unplug
events which seems to fit. The card would trigger a hot unplug while the
controller implements a hotplug handler.

Regards,
Peter

> +void sdbus_set_readonly(SDBus *sdbus, bool readonly)
> +{
> +    SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
> +    BusState *qbus = BUS(sdbus);
> +
> +    if (sbc->set_readonly) {
> +        sbc->set_readonly(qbus->parent, readonly);
> +    }
> +}
> +
> +    .name = TYPE_SD_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(SDBus),
> +    .class_size = sizeof(SDBusClass),
> +};
> +
> +static void sd_bus_register_types(void)
> +{
> +    type_register_static(&sd_bus_info);
> +}
> +
> +type_init(sd_bus_register_types)
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b4a5a62..4d17029 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -430,14 +430,41 @@ static void sd_reset(DeviceState *dev)
>      sd->expecting_acmd = false;
>  }
>  
> +static bool sd_get_inserted(SDState *sd)
> +{
> +    return blk_is_inserted(sd->blk);
> +}
> +
> +static bool sd_get_readonly(SDState *sd)
> +{
> +    return sd->wp_switch;
> +}
> +
>  static void sd_cardchange(void *opaque, bool load)
>  {
>      SDState *sd = opaque;
> +    DeviceState *dev = DEVICE(sd);
> +    SDBus *sdbus = SD_BUS(qdev_get_parent_bus(dev));
> +    bool inserted = sd_get_inserted(sd);
> +    bool readonly = sd_get_readonly(sd);
>  
> -    qemu_set_irq(sd->inserted_cb, blk_is_inserted(sd->blk));
> -    if (blk_is_inserted(sd->blk)) {
> -        sd_reset(DEVICE(sd));
> -        qemu_set_irq(sd->readonly_cb, sd->wp_switch);
> +    if (inserted) {
> +        sd_reset(dev);
> +    }
> +
> +    /* The IRQ notification is for legacy non-QOM SD controller devices;
> +     * QOMified controllers use the SDBus APIs.
> +     */
> +    if (sdbus) {
> +        sdbus_set_inserted(sdbus, inserted);
> +        if (inserted) {
> +            sdbus_set_readonly(sdbus, readonly);
> +        }
> +    } else {
> +        qemu_set_irq(sd->inserted_cb, inserted);
> +        if (inserted) {
> +            qemu_set_irq(sd->readonly_cb, readonly);
> +        }
>      }
>  }
>  
> @@ -1799,17 +1826,28 @@ static Property sd_properties[] = {
>  static void sd_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    SDClass *sc = SD_CLASS(klass);
>  
>      dc->realize = sd_realize;
>      dc->props = sd_properties;
>      dc->vmsd = &sd_vmstate;
>      dc->reset = sd_reset;
> +    dc->bus_type = TYPE_SD_BUS;
> +
> +    sc->do_command = sd_do_command;
> +    sc->write_data = sd_write_data;
> +    sc->read_data = sd_read_data;
> +    sc->data_ready = sd_data_ready;
> +    sc->enable = sd_enable;
> +    sc->get_inserted = sd_get_inserted;
> +    sc->get_readonly = sd_get_readonly;
>  }
>  
>  static const TypeInfo sd_info = {
>      .name = TYPE_SD,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(SDState),
> +    .class_size = sizeof(SDClass),
>      .class_init = sd_class_init,
>      .instance_init = sd_instance_init,
>  };
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 6997dfc..5e0321d 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -67,10 +67,49 @@ typedef struct {
>  } SDRequest;
>  
>  typedef struct SDState SDState;
> +typedef struct SDBus SDBus;
>  
>  #define TYPE_SD "sd"
>  #define SD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD)
> +#define SD_CLASS(klass) OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD)
> +#define SD_GET_CLASS(obj) OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD)
>  
> +typedef struct {
> +    /*< private >*/
> +    DeviceClass parent_class;
> +    /*< public >*/
> +
> +    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
> +    void (*write_data)(SDState *sd, uint8_t value);
> +    uint8_t (*read_data)(SDState *sd);
> +    bool (*data_ready)(SDState *sd);
> +    void (*enable)(SDState *sd, bool enable);
> +    bool (*get_inserted)(SDState *sd);
> +    bool (*get_readonly)(SDState *sd);
> +} SDClass;
> +
> +#define TYPE_SD_BUS "sd-bus"
> +#define SD_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SD_BUS)
> +#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
> +#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
> +
> +struct SDBus {
> +    BusState qbus;
> +};
> +
> +typedef struct {
> +    /*< private >*/
> +    BusClass parent_class;
> +    /*< public >*/
> +
> +    /* These methods are called by the SD device to notify the controller
> +     * when the card insertion or readonly status changes
> +     */
> +    void (*set_inserted)(DeviceState *dev, bool inserted);
> +    void (*set_readonly)(DeviceState *dev, bool readonly);
> +} SDBusClass;
> +
> +/* Legacy functions to be used only by non-qdevified callers */
>  SDState *sd_init(BlockBackend *bs, bool is_spi);
>  int sd_do_command(SDState *sd, SDRequest *req,
>                    uint8_t *response);
> @@ -78,6 +117,27 @@ void sd_write_data(SDState *sd, uint8_t value);
>  uint8_t sd_read_data(SDState *sd);
>  void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
>  bool sd_data_ready(SDState *sd);
> +/* sd_enable should not be used -- it is only used on the nseries boards,
> + * where it is part of a broken implementation of the MMC card slot switch
> + * (there should be two card slots which are multiplexed to a single MMC
> + * controller, but instead we model it with one card and controller and
> + * disable the card when the second slot is selected, so it looks like the
> + * second slot is always empty).
> + */
>  void sd_enable(SDState *sd, bool enable);
>  
> +/* Functions to be used by qdevified callers (working via
> + * an SDBus rather than directly with SDState)
> + */
> +int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
> +void sdbus_write_data(SDBus *sd, uint8_t value);
> +uint8_t sdbus_read_data(SDBus *sd);
> +bool sdbus_data_ready(SDBus *sd);
> +bool sdbus_get_inserted(SDBus *sd);
> +bool sdbus_get_readonly(SDBus *sd);
> +
> +/* Functions to be used by SD devices to report back to qdevified controllers */
> +void sdbus_set_inserted(SDBus *sd, bool inserted);
> +void sdbus_set_readonly(SDBus *sd, bool inserted);
> +
>  #endif	/* __hw_sd_h */
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:06PM +0000, Peter Maydell wrote:
> Update the SDHCI code to use the new SDBus APIs.
> 
> This commit introduces the new command line options required
> to connect a disk to sdhci-pci:
> 
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/sdhci.c         | 95 +++++++++++++++++++++++++++++++++++----------------
>  include/hw/sd/sdhci.h |  3 +-
>  2 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 991c9b5..17e08a2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -55,6 +55,9 @@
>          } \
>      } while (0)
>  
> +#define TYPE_SDHCI_BUS "sdhci-bus"
> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
> +
>  /* Default SD/MMC host controller features information, which will be
>   * presented in CAPABILITIES register of generic SD host controller at reset.
>   * If not stated otherwise:
> @@ -145,9 +148,9 @@ static void sdhci_raise_insertion_irq(void *opaque)
>      }
>  }
>  
> -static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
> +static void sdhci_set_inserted(DeviceState *dev, bool level)
>  {
> -    SDHCIState *s = (SDHCIState *)opaque;
> +    SDHCIState *s = (SDHCIState *)dev;
>      DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
>  
>      if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
> @@ -172,9 +175,9 @@ static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
>      }
>  }
>  
> -static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
> +static void sdhci_set_readonly(DeviceState *dev, bool level)
>  {
> -    SDHCIState *s = (SDHCIState *)opaque;
> +    SDHCIState *s = (SDHCIState *)dev;
>  
>      if (level) {
>          s->prnsts &= ~SDHC_WRITE_PROTECT;
> @@ -186,6 +189,8 @@ static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
>  
>  static void sdhci_reset(SDHCIState *s)
>  {
> +    DeviceState *dev = DEVICE(s);
> +
>      timer_del(s->insert_timer);
>      timer_del(s->transfer_timer);
>      /* Set all registers to 0. Capabilities registers are not cleared
> @@ -193,7 +198,10 @@ static void sdhci_reset(SDHCIState *s)
>       * initialization */
>      memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
>  
> -    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +    /* Reset other state based on current card insertion/readonly status */
> +    sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
> +    sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
> +
>      s->data_count = 0;
>      s->stopped_state = sdhc_not_stopped;
>  }
> @@ -211,7 +219,7 @@ static void sdhci_send_command(SDHCIState *s)
>      request.cmd = s->cmdreg >> 8;
>      request.arg = s->argument;
>      DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
> -    rlen = sd_do_command(s->card, &request, response);
> +    rlen = sdbus_do_command(&s->sdbus, &request, response);
>  
>      if (s->cmdreg & SDHC_CMD_RESPONSE) {
>          if (rlen == 4) {
> @@ -270,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
>          request.cmd = 0x0C;
>          request.arg = 0;
>          DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, request.arg);
> -        sd_do_command(s->card, &request, response);
> +        sdbus_do_command(&s->sdbus, &request, response);
>          /* Auto CMD12 response goes to the upper Response register */
>          s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
>                  (response[2] << 8) | response[3];
> @@ -302,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
>      }
>  
>      for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -        s->fifo_buffer[index] = sd_read_data(s->card);
> +        s->fifo_buffer[index] = sdbus_read_data(&s->sdbus);
>      }
>  
>      /* New data now available for READ through Buffer Port Register */
> @@ -395,7 +403,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
>      }
>  
>      for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -        sd_write_data(s->card, s->fifo_buffer[index]);
> +        sdbus_write_data(&s->sdbus, s->fifo_buffer[index]);
>      }
>  
>      /* Next data can be written through BUFFER DATORT register */
> @@ -477,7 +485,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>          while (s->blkcnt) {
>              if (s->data_count == 0) {
>                  for (n = 0; n < block_size; n++) {
> -                    s->fifo_buffer[n] = sd_read_data(s->card);
> +                    s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
>                  }
>              }
>              begin = s->data_count;
> @@ -518,7 +526,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>              s->sdmasysad += s->data_count - begin;
>              if (s->data_count == block_size) {
>                  for (n = 0; n < block_size; n++) {
> -                    sd_write_data(s->card, s->fifo_buffer[n]);
> +                    sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
>                  }
>                  s->data_count = 0;
>                  if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> @@ -550,7 +558,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
>  
>      if (s->trnmod & SDHC_TRNS_READ) {
>          for (n = 0; n < datacnt; n++) {
> -            s->fifo_buffer[n] = sd_read_data(s->card);
> +            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
>          }
>          dma_memory_write(&address_space_memory, s->sdmasysad, s->fifo_buffer,
>                           datacnt);
> @@ -558,7 +566,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
>          dma_memory_read(&address_space_memory, s->sdmasysad, s->fifo_buffer,
>                          datacnt);
>          for (n = 0; n < datacnt; n++) {
> -            sd_write_data(s->card, s->fifo_buffer[n]);
> +            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
>          }
>      }
>  
> @@ -662,7 +670,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                  while (length) {
>                      if (s->data_count == 0) {
>                          for (n = 0; n < block_size; n++) {
> -                            s->fifo_buffer[n] = sd_read_data(s->card);
> +                            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
>                          }
>                      }
>                      begin = s->data_count;
> @@ -703,7 +711,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                      dscr.addr += s->data_count - begin;
>                      if (s->data_count == block_size) {
>                          for (n = 0; n < block_size; n++) {
> -                            sd_write_data(s->card, s->fifo_buffer[n]);
> +                            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
>                          }
>                          s->data_count = 0;
>                          if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> @@ -817,7 +825,7 @@ static void sdhci_data_transfer(void *opaque)
>              break;
>          }
>      } else {
> -        if ((s->trnmod & SDHC_TRNS_READ) && sd_data_ready(s->card)) {
> +        if ((s->trnmod & SDHC_TRNS_READ) && sdbus_data_ready(&s->sdbus)) {
>              s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
>                      SDHC_DAT_LINE_ACTIVE;
>              sdhci_read_block_from_card(s);
> @@ -1154,15 +1162,10 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
>      }
>  }
>  
> -static void sdhci_initfn(SDHCIState *s, BlockBackend *blk)
> +static void sdhci_initfn(SDHCIState *s)
>  {
> -    s->card = sd_init(blk, false);
> -    if (s->card == NULL) {
> -        exit(1);
> -    }
> -    s->eject_cb = qemu_allocate_irq(sdhci_insert_eject_cb, s, 0);
> -    s->ro_cb = qemu_allocate_irq(sdhci_card_readonly_cb, s, 0);
> -    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
> +                        TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
>  
>      s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
>      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
> @@ -1232,7 +1235,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>      SDHCIState *s = PCI_SDHCI(dev);
>      dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
>      dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
> -    sdhci_initfn(s, s->blk);
> +    sdhci_initfn(s);
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>      s->irq = pci_allocate_irq(dev);
> @@ -1279,11 +1282,8 @@ static Property sdhci_sysbus_properties[] = {
>  static void sdhci_sysbus_init(Object *obj)
>  {
>      SDHCIState *s = SYSBUS_SDHCI(obj);
> -    DriveInfo *di;
>  
> -    /* FIXME use a qdev drive property instead of drive_get_next() */
> -    di = drive_get_next(IF_SD);
> -    sdhci_initfn(s, di ? blk_by_legacy_dinfo(di) : NULL);
> +    sdhci_initfn(s);
>  }
>  
>  static void sdhci_sysbus_finalize(Object *obj)
> @@ -1296,6 +1296,26 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>  {
>      SDHCIState *s = SYSBUS_SDHCI(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    DriveInfo *di;
> +    BlockBackend *blk;
> +    DeviceState *carddev;
> +
> +    /* Create and plug in the sd card.
> +     * FIXME: this should be done by the users of this device so we
> +     * do not use drive_get_next() here.
> +     */
> +    di = drive_get_next(IF_SD);
> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +
> +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> +    qdev_prop_set_drive(carddev, "drive", blk, errp);
> +    if (*errp) {

It is generally valid for an errp to be NULL. I think we should use a
local even if we know the caller will not pass NULL.

Regards,
Peter

> +        return;
> +    }
> +    object_property_set_bool(OBJECT(carddev), true, "realized", errp);
> +    if (*errp) {
> +        return;
> +    }
>  
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>      memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>              SDHC_REGISTERS_MAP_SIZE);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
>  }
>  
>  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
> @@ -1325,10 +1346,26 @@ static const TypeInfo sdhci_sysbus_info = {
>      .class_init = sdhci_sysbus_class_init,
>  };
>  
> +static void sdhci_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    SDBusClass *sbc = SD_BUS_CLASS(klass);
> +
> +    sbc->set_inserted = sdhci_set_inserted;
> +    sbc->set_readonly = sdhci_set_readonly;
> +}
> +
> +static const TypeInfo sdhci_bus_info = {
> +    .name = TYPE_SDHCI_BUS,
> +    .parent = TYPE_SD_BUS,
> +    .instance_size = sizeof(SDBus),
> +    .class_init = sdhci_bus_class_init,
> +};
> +
>  static void sdhci_register_types(void)
>  {
>      type_register_static(&sdhci_pci_info);
>      type_register_static(&sdhci_sysbus_info);
> +    type_register_static(&sdhci_bus_info);
>  }
>  
>  type_init(sdhci_register_types)
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index e78d938..4816516 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -37,9 +37,8 @@ typedef struct SDHCIState {
>          PCIDevice pcidev;
>          SysBusDevice busdev;
>      };
> -    SDState *card;
> +    SDBus sdbus;
>      MemoryRegion iomem;
> -    BlockBackend *blk;
>  
>      QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>      QEMUTimer *transfer_timer;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 06/10] sdhci_sysbus: Create SD card device in users, not the device itself

[Peter Crosthwaite]

On Thu, Dec 17, 2015 at 04:18:19PM -0800, Alistair Francis wrote:
> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Move the creation of the SD card device from the sdhci_sysbus
> > device itself into the boards that create these devices.
> > This allows us to remove the cannot_instantiate_with_device_add
> > notation because we no longer call drive_get_next in the device
> > model.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/xilinx_zynq.c | 17 ++++++++++++++++-
> >  hw/arm/xlnx-ep108.c  | 19 +++++++++++++++++++
> >  hw/sd/sdhci.c        | 22 ----------------------
> >  3 files changed, 35 insertions(+), 23 deletions(-)
> >
> > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> > index 1c1a445..3195055 100644
> > --- a/hw/arm/xilinx_zynq.c
> > +++ b/hw/arm/xilinx_zynq.c
> > @@ -27,6 +27,7 @@
> >  #include "hw/misc/zynq-xadc.h"
> >  #include "hw/ssi.h"
> >  #include "qemu/error-report.h"
> > +#include "hw/sd/sd.h"
> >
> >  #define NUM_SPI_FLASHES 4
> >  #define NUM_QSPI_FLASHES 2
> > @@ -153,8 +154,10 @@ static void zynq_init(MachineState *machine)
> >      MemoryRegion *address_space_mem = get_system_memory();
> >      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
> >      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
> > -    DeviceState *dev;
> > +    DeviceState *dev, *carddev;
> >      SysBusDevice *busdev;
> > +    DriveInfo *di;
> > +    BlockBackend *blk;
> >      qemu_irq pic[64];
> >      Error *err = NULL;
> >      int n;
> > @@ -260,11 +263,23 @@ static void zynq_init(MachineState *machine)
> >      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000);
> >      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]);
> >
> > +    di = drive_get_next(IF_SD);
> > +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> > +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> > +    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> > +
> >      dev = qdev_create(NULL, "generic-sdhci");
> >      qdev_init_nofail(dev);
> >      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
> >      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
> >
> > +    di = drive_get_next(IF_SD);
> > +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> > +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> > +    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> > +
> >      dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
> >      qdev_init_nofail(dev);
> >      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8007100);
> > diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> > index 85b978f..cb95d32 100644
> > --- a/hw/arm/xlnx-ep108.c
> > +++ b/hw/arm/xlnx-ep108.c
> > @@ -34,6 +34,7 @@ static void xlnx_ep108_init(MachineState *machine)
> >  {
> >      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> >      Error *err = NULL;
> > +    int i;
> >
> >      object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> >      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > @@ -45,6 +46,24 @@ static void xlnx_ep108_init(MachineState *machine)
> >          exit(1);
> >      }
> >
> > +    /* Create and plug in the SD cards */
> > +    for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
> > +        BusState *bus;
> > +        DriveInfo *di = drive_get_next(IF_SD);
> > +        BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +        DeviceState *carddev;
> > +
> > +        bus = qdev_get_child_bus(DEVICE(&s->soc.sdhci[i]), "sd-bus");
> 
> This looks like the same thing I was trying to avoid with my SPI
> patches. We were trying to avoid the machine reaching into the SoC
> when getting the child busses. Instead expose the bus to the SoC so
> the board can just get it straight from there.
> 

I have an alternate QOM fix that should enable this. Will comment the SPI
discussion.

Regards,
Peter

> > +        if (!bus) {
> > +            error_report("No SD bus found for SD card %d", i);
> > +            exit(1);
> > +        }
> > +        carddev = qdev_create(bus, TYPE_SD);
> > +        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> > +        object_property_set_bool(OBJECT(carddev), true, "realized",
> > +                                 &error_fatal);
> > +    }
> > +
> 
> I also think this should go after the other memory creation, not before.
> 
> Thanks,
> 
> Alistair
> 
> >      if (machine->ram_size > EP108_MAX_RAM_SIZE) {
> >          error_report("WARNING: RAM size " RAM_ADDR_FMT " above max supported, "
> >                       "reduced to %llx", machine->ram_size, EP108_MAX_RAM_SIZE);
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 17e08a2..c8e3865 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1296,26 +1296,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
> >  {
> >      SDHCIState *s = SYSBUS_SDHCI(dev);
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > -    DriveInfo *di;
> > -    BlockBackend *blk;
> > -    DeviceState *carddev;
> > -
> > -    /* Create and plug in the sd card.
> > -     * FIXME: this should be done by the users of this device so we
> > -     * do not use drive_get_next() here.
> > -     */
> > -    di = drive_get_next(IF_SD);
> > -    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > -
> > -    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> > -    qdev_prop_set_drive(carddev, "drive", blk, errp);
> > -    if (*errp) {
> > -        return;
> > -    }
> > -    object_property_set_bool(OBJECT(carddev), true, "realized", errp);
> > -    if (*errp) {
> > -        return;
> > -    }
> >
> >      s->buf_maxsz = sdhci_get_fifolen(s);
> >      s->fifo_buffer = g_malloc0(s->buf_maxsz);
> > @@ -1333,8 +1313,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
> >      dc->vmsd = &sdhci_vmstate;
> >      dc->props = sdhci_sysbus_properties;
> >      dc->realize = sdhci_sysbus_realize;
> > -    /* Reason: instance_init() method uses drive_get_next() */
> > -    dc->cannot_instantiate_with_device_add_yet = true;
> >  }
> >
> >  static const TypeInfo sdhci_sysbus_info = {
> > --
> > 1.9.1
> >
> >

Re: [Qemu-devel] [PATCH 07/10] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:08PM +0000, Peter Maydell wrote:
> Convert the pxa2xx_mmci device to be a sysbus device.
> 
> In this commit we only change the device itself, and leave
> the interface to the SD card using the old non-SDBus APIs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 73 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index b217080..b6bb390 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,60 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  BlockBackend *blk, qemu_irq irq,
>                  qemu_irq rx_dma, qemu_irq tx_dma)
>  {
> +    DeviceState *dev;
> +    SysBusDevice *sbd;
>      PXA2xxMMCIState *s;
>  
> -    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
> -    s->irq = irq;
> -    s->rx_dma = rx_dma;
> -    s->tx_dma = tx_dma;
> -
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
> -                          "pxa2xx-mmci", 0x00100000);
> -    memory_region_add_subregion(sysmem, base, &s->iomem);
> -
> -    /* Instantiate the actual storage */
> -    s->card = sd_init(blk, false);
> +    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
> +    s = PXA2XX_MMCI(dev);
> +    /* Reach into the device and initialize the SD card. This is
> +     * unclean but will vanish when we update to SDBus APIs.
> +     */
> +    s->card = sd_init(s->blk, false);
>      if (s->card == NULL) {
>          exit(1);
>      }
> -
> -    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> -                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> -
> +    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);

This looks like heterogenous GPIOs so should be separate named GPIO
sets. Looking at the machine model, the irq goes to the "pic" but the
DMA signals go to a different IP. I would leave the IRQ as the one and
only sysbus IRQ, but implement the two DMAs as named GPIOs.

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>      return s;
>  }
>  
>  void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> -                qemu_irq coverswitch)
> +                          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, obj, &pxa2xx_mmci_ops, s,
> +                          "pxa2xx-mmci", 0x00100000);
> +    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 const TypeInfo pxa2xx_mmci_info = {
> +    .name = TYPE_PXA2XX_MMCI,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PXA2xxMMCIState),
> +    .instance_init = pxa2xx_mmci_instance_init,
> +};
> +
> +static void pxa2xx_mmci_register_types(void)
> +{
> +    type_register_static(&pxa2xx_mmci_info);
> +}
> +
> +type_init(pxa2xx_mmci_register_types)
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 08/10] hw/sd/pxa2xx_mmci: Update to use new SDBus APIs

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:09PM +0000, Peter Maydell wrote:
> Now the PXA2xx MMCI device is QOMified itself, we can
> update it to use the SDBus APIs to talk to the SD card.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 80 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index b6bb390..a30be2b 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -16,10 +16,14 @@
>  #include "hw/sd/sd.h"
>  #include "hw/qdev.h"
>  #include "hw/qdev-properties.h"
> +#include "qemu/error-report.h"
>  
>  #define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
>  #define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
>  
> +#define TYPE_PXA2XX_MMCI_BUS "pxa2xx-mmci-bus"
> +#define PXA2XX_MMCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
> +
>  typedef struct PXA2xxMMCIState {
>      SysBusDevice parent_obj;
>  
> @@ -27,9 +31,11 @@ typedef struct PXA2xxMMCIState {
>      qemu_irq irq;
>      qemu_irq rx_dma;
>      qemu_irq tx_dma;
> +    qemu_irq inserted;
> +    qemu_irq readonly;
>  
>      BlockBackend *blk;
> -    SDState *card;
> +    SDBus sdbus;
>  
>      uint32_t status;
>      uint32_t clkrt;
> @@ -129,7 +135,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
>  
>      if (s->cmdat & CMDAT_WR_RD) {
>          while (s->bytesleft && s->tx_len) {
> -            sd_write_data(s->card, s->tx_fifo[s->tx_start ++]);
> +            sdbus_write_data(&s->sdbus, s->tx_fifo[s->tx_start++]);
>              s->tx_start &= 0x1f;
>              s->tx_len --;
>              s->bytesleft --;
> @@ -139,7 +145,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
>      } else
>          while (s->bytesleft && s->rx_len < 32) {
>              s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
> -                sd_read_data(s->card);
> +                sdbus_read_data(&s->sdbus);
>              s->bytesleft --;
>              s->intreq |= INT_RXFIFO_REQ;
>          }
> @@ -173,7 +179,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
>      request.arg = s->arg;
>      request.crc = 0;	/* FIXME */
>  
> -    rsplen = sd_do_command(s->card, &request, response);
> +    rsplen = sdbus_do_command(&s->sdbus, &request, response);
>      s->intreq |= INT_END_CMD;
>  
>      memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
> @@ -482,32 +488,59 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  BlockBackend *blk, qemu_irq irq,
>                  qemu_irq rx_dma, qemu_irq tx_dma)
>  {
> -    DeviceState *dev;
> +    DeviceState *dev, *carddev;
>      SysBusDevice *sbd;
>      PXA2xxMMCIState *s;
> +    Error *err = NULL;
>  
>      dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
>      s = PXA2XX_MMCI(dev);
> -    /* Reach into the device and initialize the SD card. This is
> -     * unclean but will vanish when we update to SDBus APIs.
> -     */
> -    s->card = sd_init(s->blk, false);
> -    if (s->card == NULL) {
> -        exit(1);
> -    }
> -    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);
> +
> +    /* Create and plug in the sd card */
> +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
> +    qdev_prop_set_drive(carddev, "drive", blk, &err);
> +    if (err) {
> +        error_report("failed to init SD card: %s", error_get_pretty(err));
> +        return NULL;
> +    }
> +    object_property_set_bool(OBJECT(carddev), true, "realized", &err);
> +    if (err) {
> +        error_report("failed to init SD card: %s", error_get_pretty(err));
> +        return NULL;
> +    }
> +
>      return s;
>  }
>  
> +static void pxa2xx_mmci_set_inserted(DeviceState *dev, bool inserted)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
> +
> +    qemu_set_irq(s->inserted, inserted);
> +}
> +
> +static void pxa2xx_mmci_set_readonly(DeviceState *dev, bool readonly)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
> +
> +    qemu_set_irq(s->readonly, readonly);
> +}
> +
>  void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
>                            qemu_irq coverswitch)
>  {
> -    sd_set_cb(s->card, readonly, coverswitch);
> +    DeviceState *dev = DEVICE(s);
> +
> +    s->readonly = readonly;
> +    s->inserted = coverswitch;
> +
> +    pxa2xx_mmci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
> +    pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));

Looking at the machine models, _mmci_handlers is only called at
machine init time. So this should not be needed. devices should be
triggering an initial call of _set_readonly that reliably propagates
back up.

If we do need these two lines they should be in the reset handler.

Regards,
Peter

>  }
>  
>  static void pxa2xx_mmci_instance_init(Object *obj)
> @@ -524,6 +557,17 @@ static void pxa2xx_mmci_instance_init(Object *obj)
>  
>      register_savevm(NULL, "pxa2xx_mmci", 0, 0,
>                      pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> +
> +    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
> +                        TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
> +}
> +
> +static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    SDBusClass *sbc = SD_BUS_CLASS(klass);
> +
> +    sbc->set_inserted = pxa2xx_mmci_set_inserted;
> +    sbc->set_readonly = pxa2xx_mmci_set_readonly;
>  }
>  
>  static const TypeInfo pxa2xx_mmci_info = {
> @@ -533,9 +577,17 @@ static const TypeInfo pxa2xx_mmci_info = {
>      .instance_init = pxa2xx_mmci_instance_init,
>  };
>  
> +static const TypeInfo pxa2xx_mmci_bus_info = {
> +    .name = TYPE_PXA2XX_MMCI_BUS,
> +    .parent = TYPE_SD_BUS,
> +    .instance_size = sizeof(SDBus),
> +    .class_init = pxa2xx_mmci_bus_class_init,
> +};
> +
>  static void pxa2xx_mmci_register_types(void)
>  {
>      type_register_static(&pxa2xx_mmci_info);
> +    type_register_static(&pxa2xx_mmci_bus_info);
>  }
>  
>  type_init(pxa2xx_mmci_register_types)
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 09/10] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:10PM +0000, Peter Maydell wrote:
> 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 | 148 ++++++++++++++++++++--------------------------------
>  1 file changed, 56 insertions(+), 92 deletions(-)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index a30be2b..81fec4d 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -43,27 +43,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);


A nit, but I wonder if ARRAY_SIZE should be generally used in this case, as
it is a little more self documenting and would make code identical to
non-byte FIFOs.

> +}
> +

Extra blank.

> +
> +static const VMStateDescription vmstate_pxa2xx_mmci = {

Is the registration in class_init missing?

Otherwise,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> +    .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 */
> @@ -405,84 +450,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,
> @@ -555,9 +522,6 @@ static void pxa2xx_mmci_instance_init(Object *obj)
>      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);
> -
>      qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
>                          TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
>  }
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 10/10] hw/sd/pxa2xx_mmci: Add reset function

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:11PM +0000, Peter Maydell wrote:
> 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>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/sd/pxa2xx_mmci.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index 81fec4d..3df927e 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -510,6 +510,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
>      pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
>  }
>  
> +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);
> @@ -526,6 +555,13 @@ static void pxa2xx_mmci_instance_init(Object *obj)
>                          TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
>  }
>  
> +static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = pxa2xx_mmci_reset;
> +}
> +
>  static void pxa2xx_mmci_bus_class_init(ObjectClass *klass, void *data)
>  {
>      SDBusClass *sbc = SD_BUS_CLASS(klass);
> @@ -539,6 +575,7 @@ static const TypeInfo pxa2xx_mmci_info = {
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(PXA2xxMMCIState),
>      .instance_init = pxa2xx_mmci_instance_init,
> +    .class_init = pxa2xx_mmci_class_init,
>  };
>  
>  static const TypeInfo pxa2xx_mmci_bus_info = {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Maydell]

On 19 December 2015 at 21:36, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 04:37:03PM +0000, Peter Maydell wrote:
>> Turn the SD card into a QOM device.
>> This conversion only changes the device itself; the various
>> functions which are effectively methods on the device are not
>> touched at this point.

>> +#define TYPE_SD "sd"
>
> Can we get "card" in there? I think unqualified SD should be usable
> to refer to the bus standard.

Sure. I don't have strong opinions on any of the type names
in this series, and I was wondering about sticking 'card' in.
"sdcard" or "sd-card" ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Maydell]

On 19 December 2015 at 21:38, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 04:37:05PM +0000, Peter Maydell wrote:
>> +bool sdbus_get_inserted(SDBus *sdbus)
>> +{
>> +    SDState *card = get_card(sdbus);
>> +
>> +    if (card) {
>> +        SDClass *sc = SD_GET_CLASS(card);
>> +
>> +        return sc->get_inserted(card);
>> +    }
>> +
>> +    return false;
>> +}
>
> This I am not sure about. Realistically, the card has no self
> awareness of its ejection state so it can't be polled for "are
> you there". The card insertion switch is implemented as a
> physical switch on the slot itself and a property of the bus.
>
> The absence on presence of a device should determine this, making me
> think this should return !!card.
>
> Unfortunately, we have the case of the block UI being able to trigger a
> card ejection from underneath the bus level. But since the SD card is already
> using qdev_get_parent_bus() the removal from the bus can be managed at the
> card level.

For user-level back compat I think we need to retain "might have
an sdcard object with no block backend, and that means
'no-card-present'". This is both for the user facing
monitor commands to manipulate the sd card, and also
for the not-yet-QOMified controllers, which will always
create an sdcard object even if there's no block backend
(which should continue to mean "no card").

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

On 19 December 2015 at 21:39, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 04:37:06PM +0000, Peter Maydell wrote:
>> +    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
>> +    qdev_prop_set_drive(carddev, "drive", blk, errp);
>> +    if (*errp) {
>
> It is generally valid for an errp to be NULL. I think we should use a
> local even if we know the caller will not pass NULL.

Agreed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/10] hw/sd/pxa2xx_mmci: Update to use new SDBus APIs

[Peter Maydell]

On 19 December 2015 at 21:42, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 04:37:09PM +0000, Peter Maydell wrote:
>> Now the PXA2xx MMCI device is QOMified itself, we can
>> update it to use the SDBus APIs to talk to the SD card.

>>  void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
>>                            qemu_irq coverswitch)
>>  {
>> -    sd_set_cb(s->card, readonly, coverswitch);
>> +    DeviceState *dev = DEVICE(s);
>> +
>> +    s->readonly = readonly;
>> +    s->inserted = coverswitch;
>> +
>> +    pxa2xx_mmci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
>> +    pxa2xx_mmci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
>
> Looking at the machine models, _mmci_handlers is only called at
> machine init time. So this should not be needed. devices should be
> triggering an initial call of _set_readonly that reliably propagates
> back up.
>
> If we do need these two lines they should be in the reset handler.

This change is just retaining current behaviour over the QOMification
(we would assert the lines in this function before this patch,
so we should continue to do so afterwards).

We have no good mechanism for dealing with lines that ought to
be asserted on device reset (of which "sd card inserted" is
typically one). If we assert these lines on reset then we're
just introducing a different wrong behaviour where there's
a dependency on reset order. I preferred not to touch this
because we don't actually care much about the pxa2xx boards
themselves except that they're getting a bit elderly and
need updating to QOM.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/10] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Maydell]

On 19 December 2015 at 21:45, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Dec 11, 2015 at 04:37:10PM +0000, Peter Maydell wrote:
>> Convert the pxa2xx_mmci device from manual save/load
>> functions to a VMStateDescription structure.
>>
>> This is a migration compatibility break.

>> +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);
>
>
> A nit, but I wonder if ARRAY_SIZE should be generally used in this case, as
> it is a little more self documenting and would make code identical to
> non-byte FIFOs.

Yes. In particular ARRAY_SIZE(s->resp_fifo) is correct
and sizeof(s->resp_fifo) is wrong because that array is uint16_t...

>> +}
>> +
>
> Extra blank.
>
>> +
>> +static const VMStateDescription vmstate_pxa2xx_mmci = {
>
> Is the registration in class_init missing?

Does seem to be...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/10] hw/sd/sd.c: QOMify

[Peter Crosthwaite]

On Sun, Dec 20, 2015 at 9:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 December 2015 at 21:36, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Fri, Dec 11, 2015 at 04:37:03PM +0000, Peter Maydell wrote:
>>> Turn the SD card into a QOM device.
>>> This conversion only changes the device itself; the various
>>> functions which are effectively methods on the device are not
>>> touched at this point.
>
>>> +#define TYPE_SD "sd"
>>
>> Can we get "card" in there? I think unqualified SD should be usable
>> to refer to the bus standard.
>
> Sure. I don't have strong opinions on any of the type names
> in this series, and I was wondering about sticking 'card' in.
> "sdcard" or "sd-card" ?
>

I think "sd-card"

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Crosthwaite]

On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 December 2015 at 21:38, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Fri, Dec 11, 2015 at 04:37:05PM +0000, Peter Maydell wrote:
>>> +bool sdbus_get_inserted(SDBus *sdbus)
>>> +{
>>> +    SDState *card = get_card(sdbus);
>>> +
>>> +    if (card) {
>>> +        SDClass *sc = SD_GET_CLASS(card);
>>> +
>>> +        return sc->get_inserted(card);
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>
>> This I am not sure about. Realistically, the card has no self
>> awareness of its ejection state so it can't be polled for "are
>> you there". The card insertion switch is implemented as a
>> physical switch on the slot itself and a property of the bus.
>>
>> The absence on presence of a device should determine this, making me
>> think this should return !!card.
>>
>> Unfortunately, we have the case of the block UI being able to trigger a
>> card ejection from underneath the bus level. But since the SD card is already
>> using qdev_get_parent_bus() the removal from the bus can be managed at the
>> card level.
>
> For user-level back compat I think we need to retain "might have
> an sdcard object with no block backend, and that means
> 'no-card-present'". This is both for the user facing
> monitor commands to manipulate the sd card, and also

What are the user-facing monitor commands? I tried using "change" and
"eject", but they don't seem to work for SD, due to the tray being
closed. Has this ever worked in a way that is user manipulatable for
SD or is it just to handle the case of unconditional SD card creation
(with the card never hotplugging over the system lifetime)?

Test case below. Using ep108 with a backendless drive (-drive if=sd).

$ dd if=/dev/zero of=./sd0file bs=16M count=1
1+0 records in
1+0 records out
16777216 bytes (17 MB) copied, 0.170586 s, 98.4 MB/s
$ dd if=/dev/zero of=./sd1file bs=16M count=1
1+0 records in
1+0 records out
16777216 bytes (17 MB) copied, 0.183836 s, 91.3 MB/s
$ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -S -kernel
/dev/null -sd sd0file -drive if=sd -nographic
WARNING: Image format was not specified for 'sd0file' and probing guessed raw.
         Automatically detecting the format is dangerous for raw
images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
QEMU 2.5.50 monitor - type 'help' for more information
(qemu) info block
sd0 (#block197): sd0file (raw)
    Removable device: not locked, tray closed
    Cache mode:       writeback

sd1: [not inserted]
    Removable device: not locked, tray closed

ide1-cd0: [not inserted]
    Removable device: not locked, tray closed

floppy0: [not inserted]
    Removable device: not locked, tray closed
(qemu) change sd1 sd1file raw
Tray of device 'sd1' is not open
(qemu) eject -f sd1
Tray of device 'sd1' is not open
(qemu) info block
sd0 (#block197): sd0file (raw)
    Removable device: not locked, tray closed
    Cache mode:       writeback

sd1: [not inserted]
    Removable device: not locked, tray closed

ide1-cd0: [not inserted]
    Removable device: not locked, tray closed

floppy0: [not inserted]
    Removable device: not locked, tray closed
(qemu)

Regards,
Peter

> for the not-yet-QOMified controllers, which will always
> create an sdcard object even if there's no block backend
> (which should continue to mean "no card").
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Maydell]

On 20 December 2015 at 20:51, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 19 December 2015 at 21:38, Peter Crosthwaite
>> <crosthwaitepeter@gmail.com> wrote:
>>> On Fri, Dec 11, 2015 at 04:37:05PM +0000, Peter Maydell wrote:
>>>> +bool sdbus_get_inserted(SDBus *sdbus)
>>>> +{
>>>> +    SDState *card = get_card(sdbus);
>>>> +
>>>> +    if (card) {
>>>> +        SDClass *sc = SD_GET_CLASS(card);
>>>> +
>>>> +        return sc->get_inserted(card);
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>
>>> This I am not sure about. Realistically, the card has no self
>>> awareness of its ejection state so it can't be polled for "are
>>> you there". The card insertion switch is implemented as a
>>> physical switch on the slot itself and a property of the bus.
>>>
>>> The absence on presence of a device should determine this, making me
>>> think this should return !!card.
>>>
>>> Unfortunately, we have the case of the block UI being able to trigger a
>>> card ejection from underneath the bus level. But since the SD card is already
>>> using qdev_get_parent_bus() the removal from the bus can be managed at the
>>> card level.
>>
>> For user-level back compat I think we need to retain "might have
>> an sdcard object with no block backend, and that means
>> 'no-card-present'". This is both for the user facing
>> monitor commands to manipulate the sd card, and also
>
> What are the user-facing monitor commands? I tried using "change" and
> "eject", but they don't seem to work for SD, due to the tray being
> closed. Has this ever worked in a way that is user manipulatable for
> SD or is it just to handle the case of unconditional SD card creation
> (with the card never hotplugging over the system lifetime)?

I admit to just assuming that this stuff worked rather than
testing it :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Crosthwaite]

On Sun, Dec 20, 2015 at 3:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 December 2015 at 20:51, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 19 December 2015 at 21:38, Peter Crosthwaite
>>> <crosthwaitepeter@gmail.com> wrote:
>>>> On Fri, Dec 11, 2015 at 04:37:05PM +0000, Peter Maydell wrote:
>>>>> +bool sdbus_get_inserted(SDBus *sdbus)
>>>>> +{
>>>>> +    SDState *card = get_card(sdbus);
>>>>> +
>>>>> +    if (card) {
>>>>> +        SDClass *sc = SD_GET_CLASS(card);
>>>>> +
>>>>> +        return sc->get_inserted(card);
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>
>>>> This I am not sure about. Realistically, the card has no self
>>>> awareness of its ejection state so it can't be polled for "are
>>>> you there". The card insertion switch is implemented as a
>>>> physical switch on the slot itself and a property of the bus.
>>>>
>>>> The absence on presence of a device should determine this, making me
>>>> think this should return !!card.
>>>>
>>>> Unfortunately, we have the case of the block UI being able to trigger a
>>>> card ejection from underneath the bus level. But since the SD card is already
>>>> using qdev_get_parent_bus() the removal from the bus can be managed at the
>>>> card level.
>>>
>>> For user-level back compat I think we need to retain "might have
>>> an sdcard object with no block backend, and that means
>>> 'no-card-present'". This is both for the user facing
>>> monitor commands to manipulate the sd card, and also
>>
>> What are the user-facing monitor commands? I tried using "change" and
>> "eject", but they don't seem to work for SD, due to the tray being
>> closed. Has this ever worked in a way that is user manipulatable for
>> SD or is it just to handle the case of unconditional SD card creation
>> (with the card never hotplugging over the system lifetime)?
>
> I admit to just assuming that this stuff worked rather than
> testing it :-)
>

Can we capitilise on the opportunity here to break the backwards
compat for the non-functional feature and defeature eject and change
for SD? File substitution for SD cards doesn't match the real world
nicely, as we use the file size to modify the SD card implementation
(SDSC vs SDHC as well as the # of blocks). It really should be a
destroy+recreate rather than a substitution.We can then deal with SD
card ejection and insertion as proper hotplug.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Maydell]

On 20 December 2015 at 20:51, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> For user-level back compat I think we need to retain "might have
>> an sdcard object with no block backend, and that means
>> 'no-card-present'". This is both for the user facing
>> monitor commands to manipulate the sd card, and also
>
> What are the user-facing monitor commands? I tried using "change" and
> "eject", but they don't seem to work for SD, due to the tray being
> closed. Has this ever worked in a way that is user manipulatable for
> SD or is it just to handle the case of unconditional SD card creation
> (with the card never hotplugging over the system lifetime)?

I investigated this, and it looks like we accidentally broke
'change' for SD cards in 2.5 (specifically in commit de2c6c05).
I think we should fix that regression, which in turn implies that
we still want to support the "sd card object with no block backend" case.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

[Peter Crosthwaite]

On Thu, Jan 7, 2016 at 10:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 December 2015 at 20:51, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> For user-level back compat I think we need to retain "might have
>>> an sdcard object with no block backend, and that means
>>> 'no-card-present'". This is both for the user facing
>>> monitor commands to manipulate the sd card, and also
>>
>> What are the user-facing monitor commands? I tried using "change" and
>> "eject", but they don't seem to work for SD, due to the tray being
>> closed. Has this ever worked in a way that is user manipulatable for
>> SD or is it just to handle the case of unconditional SD card creation
>> (with the card never hotplugging over the system lifetime)?
>
> I investigated this, and it looks like we accidentally broke
> 'change' for SD cards in 2.5 (specifically in commit de2c6c05).
> I think we should fix that regression, which in turn implies that
> we still want to support the "sd card object with no block backend" case.
>

Yes, saw the patches on list. I guess we are stuck with it. It would
be good to do this in a way that supports use of the hotplug API
alongside though, so SDIO device could all be ejected and inserted
with the same way.

Regards,
Peter

> thanks
> -- PMM