Re: [Qemu-arm] [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 <address@hidden> 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 <address@hidden>
> ---
>  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
>
>