Peter Maydell <peter.mayd...@linaro.org> writes:
> Convert the pxa2xx_mmci device to be a sysbus device.
>
> Signed-off-by: Peter Maydell <peter.mayd...@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.
