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  
> 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 
> > ---
> >  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, 0xE010);
> >  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, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized", 
> > _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, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized", 
> > _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(>soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> >  object_property_add_child(OBJECT(machine), "soc", OBJECT(>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(>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, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized",
> > + _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, 

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
 wrote:
> On Fri, Dec 11, 2015 at 8:37 AM, Peter Maydell  
> 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(>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 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  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 
> ---
>  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, 0xE010);
>  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, _fatal);
> +object_property_set_bool(OBJECT(carddev), true, "realized", 
> _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, _fatal);
> +object_property_set_bool(OBJECT(carddev), true, "realized", 
> _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(>soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
>  object_property_add_child(OBJECT(machine), "soc", OBJECT(>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(>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, _fatal);
> +object_property_set_bool(OBJECT(carddev), true, "realized",
> + _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 ? 

[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 
---
 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, 0xE010);
 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, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized", _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, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized", _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(>soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
 object_property_add_child(OBJECT(machine), "soc", OBJECT(>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(>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, _fatal);
+object_property_set_bool(OBJECT(carddev), true, "realized",
+ _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 = _vmstate;
 dc->props = sdhci_sysbus_properties;
 dc->realize = sdhci_sysbus_realize;
-/* Reason: instance_init() method uses