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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the pxa2xx_mmci device to be a sysbus device.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 96 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 17 deletions(-)
>
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index d1fe6d5..5b676c7 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -11,16 +11,23 @@
>   */
>  
>  #include "hw/hw.h"
> +#include "hw/sysbus.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sd.h"
>  #include "hw/qdev.h"
>  
> -struct PXA2xxMMCIState {
> +#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
> +#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
> +
> +typedef struct PXA2xxMMCIState {
> +    SysBusDevice parent_obj;
> +
>      MemoryRegion iomem;
>      qemu_irq irq;
>      qemu_irq rx_dma;
>      qemu_irq tx_dma;
>  
> +    void *blk; /* Should be a BlockBackend* */
>      SDState *card;
>  
>      uint32_t status;
> @@ -48,7 +55,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 +481,86 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  BlockBackend *blk, qemu_irq irq,
>                  qemu_irq rx_dma, qemu_irq tx_dma)
>  {
> -    PXA2xxMMCIState *s;
> +    DeviceState *dev;
> +    SysBusDevice *sbd;
> +
> +    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
> +    qdev_prop_set_ptr(dev, "drive", blk);
> +    qdev_init_nofail(dev);
> +    sbd = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(sbd, 0, base);
> +    sysbus_connect_irq(sbd, 0, irq);
> +    sysbus_connect_irq(sbd, 1, rx_dma);
> +    sysbus_connect_irq(sbd, 2, tx_dma);
> +    return PXA2XX_MMCI(dev);
> +}
>  
> -    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
> -    s->irq = irq;
> -    s->rx_dma = rx_dma;
> -    s->tx_dma = tx_dma;
> +void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> +                qemu_irq coverswitch)
> +{
> +    sd_set_cb(s->card, readonly, coverswitch);
> +}
> +
> +static void pxa2xx_mmci_instance_init(Object *obj)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
> +    memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s,
>                            "pxa2xx-mmci", 0x00100000);
> -    memory_region_add_subregion(sysmem, base, &s->iomem);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +    sysbus_init_irq(sbd, &s->rx_dma);
> +    sysbus_init_irq(sbd, &s->tx_dma);
> +
> +    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> +                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> +}
> +
> +static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
>  
>      /* Instantiate the actual storage */
> -    s->card = sd_init(blk, false);
> +    s->card = sd_init(s->blk, false);
>      if (s->card == NULL) {
> -        exit(1);
> +        error_setg(errp, "Could not initialize SD card with specified drive");
> +        return;
>      }
> +}
>  
> -    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> -                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> +static Property pxa2xx_mmci_properties[] = {
> +    /* Note: pointer property 'drive' may remain NULL, thus no need
> +     * for dc->cannot_instantiate_with_device_add_yet = true;
> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
> +     * setting a 'drive' property results in a call to blk_attach_dev()
> +     * attaching the BlockBackend to this device; that then means that
> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
> +     * attach the BlockBackend to the SD card object aborts.
> +     */
> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

As far as I can tell, this problem is an artifact of our interface to
the common sd-card code, namely sd_init().  sd_init() was made for the
pre-qdev world: it creates and completely initializes the common
SDState.

In qdev, we do this in three separate steps: create, set properties,
realize.  Split up sd_init(), and the problem should go away.

How?  Well, code common to several related qdevs exists elsewhere.  A
simple example is serial.c.  The serial qdevs embed a SerialState in
their device state, have properties pointing into that sub-state, and
call serial_realize_core() to realize that sub-state.

We also use composition, i.e. do the common part as full-blown qdev,
then wrap specialization qdevs around it.  I doubt you need to be that
fancy here.

>  
> -    return s;
> +static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pxa2xx_mmci_realize;
> +    dc->props = pxa2xx_mmci_properties;
>  }
>  
> -void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> -                qemu_irq coverswitch)
> +static const TypeInfo pxa2xx_mmci_info = {
> +    .name = TYPE_PXA2XX_MMCI,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PXA2xxMMCIState),
> +    .class_init = pxa2xx_mmci_class_init,
> +    .instance_init = pxa2xx_mmci_instance_init,
> +};
> +
> +static void pxa2xx_mmci_register_types(void)
>  {
> -    sd_set_cb(s->card, readonly, coverswitch);
> +    type_register_static(&pxa2xx_mmci_info);
>  }
> +
> +type_init(pxa2xx_mmci_register_types)

Sorry for taking so long to review.