[PATCH v2 00/13] Eliminate drive_get_next()

[PATCH v2 00/13] Eliminate drive_get_next()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If
the order changes, or new calls appear "in the middle", unit numbers
change.  ABI break.  Hard to spot in review.  Replace its uses by
drive_get(), then delete it.

Markus Armbruster (13):
  hw/sd/ssi-sd: Do not create SD card within controller's realize
  hw: Replace trivial drive_get_next() by drive_get()
  hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get()
  hw/arm/versatilepb hw/arm/vexpress: Replace drive_get_next() by
    drive_get()
  hw/arm/imx25_pdk: Replace drive_get_next() by drive_get()
  hw/arm/mcimx6ul-evk: Replace drive_get_next() by drive_get()
  hw/arm/mcimx7d-sabre: Replace drive_get_next() by drive_get()
  hw/arm/xlnx-versal-virt: Replace drive_get_next() by drive_get()
  hw/microblaze: Replace drive_get_next() by drive_get()
  hw/arm/xlnx-zcu102: Replace drive_get_next() by drive_get()
  hw/arm/xilinx_zynq: Replace drive_get_next() by drive_get()
  hw/arm/aspeed: Replace drive_get_next() by drive_get()
  blockdev: Drop unused drive_get_next()

 include/sysemu/blockdev.h           |  1 -
 blockdev.c                          | 10 ----------
 hw/arm/aspeed.c                     | 21 +++++++++++++--------
 hw/arm/cubieboard.c                 |  2 +-
 hw/arm/imx25_pdk.c                  |  2 +-
 hw/arm/integratorcp.c               |  2 +-
 hw/arm/mcimx6ul-evk.c               |  2 +-
 hw/arm/mcimx7d-sabre.c              |  2 +-
 hw/arm/msf2-som.c                   |  2 +-
 hw/arm/npcm7xx_boards.c             |  6 +++---
 hw/arm/orangepi.c                   |  2 +-
 hw/arm/raspi.c                      |  2 +-
 hw/arm/realview.c                   |  2 +-
 hw/arm/sabrelite.c                  |  2 +-
 hw/arm/stellaris.c                  | 15 ++++++++++++++-
 hw/arm/versatilepb.c                |  4 ++--
 hw/arm/vexpress.c                   |  6 +++---
 hw/arm/xilinx_zynq.c                | 16 +++++++++-------
 hw/arm/xlnx-versal-virt.c           |  3 ++-
 hw/arm/xlnx-zcu102.c                |  6 +++---
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/misc/sifive_u_otp.c              |  2 +-
 hw/riscv/microchip_pfsoc.c          |  2 +-
 hw/riscv/sifive_u.c                 | 15 +++++++++++++--
 hw/sd/ssi-sd.c                      | 29 +----------------------------
 hw/sparc64/niagara.c                |  2 +-
 26 files changed, 77 insertions(+), 83 deletions(-)

-- 
2.31.1

[PATCH v2 01/13] hw/sd/ssi-sd: Do not create SD card within controller's realize

[Markus Armbruster]

ssi_sd_realize() creates an "sd-card" device.  This is inappropriate,
and marked FIXME.

Move it to the boards that create these devices.  Prior art: commit
eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for
device "pl181".

The device remains not user-creatable, because its users should (and
do) wire up its GPIO chip-select line.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: qemu-arm@nongnu.org
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/stellaris.c  | 15 ++++++++++++++-
 hw/riscv/sifive_u.c | 13 ++++++++++++-
 hw/sd/ssi-sd.c      | 29 +----------------------------
 3 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 78827ace6b..b6c8a5d609 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/sysbus.h"
+#include "hw/sd/sd.h"
 #include "hw/ssi/ssi.h"
 #include "hw/arm/boot.h"
 #include "qemu/timer.h"
@@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
             void *bus;
             DeviceState *sddev;
             DeviceState *ssddev;
+            DriveInfo *dinfo;
+            DeviceState *carddev;
+            BlockBackend *blk;
 
             /*
              * Some boards have both an OLED controller and SD card connected to
@@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
              *  - Make the ssd0323 OLED controller chipselect active-low
              */
             bus = qdev_get_child_bus(dev, "ssi");
-
             sddev = ssi_create_peripheral(bus, "ssi-sd");
+
+            dinfo = drive_get(IF_SD, 0, 0);
+            blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
+            carddev = qdev_new(TYPE_SD_CARD);
+            qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
+            qdev_prop_set_bit(carddev, "spi", true);
+            qdev_realize_and_unref(carddev,
+                                   qdev_get_child_bus(sddev, "sd-bus"),
+                                   &error_fatal);
+
             ssddev = ssi_create_peripheral(bus, "ssd0323");
             gpio_out[GPIO_D][0] = qemu_irq_split(
                     qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0),
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 589ae72a59..a4ecadaf12 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -46,6 +46,7 @@
 #include "hw/char/serial.h"
 #include "hw/cpu/cluster.h"
 #include "hw/misc/unimp.h"
+#include "hw/sd/sd.h"
 #include "hw/ssi/ssi.h"
 #include "target/riscv/cpu.h"
 #include "hw/riscv/riscv_hart.h"
@@ -536,7 +537,8 @@ static void sifive_u_machine_init(MachineState *machine)
     uint32_t fdt_load_addr;
     uint64_t kernel_entry;
     DriveInfo *dinfo;
-    DeviceState *flash_dev, *sd_dev;
+    BlockBackend *blk;
+    DeviceState *flash_dev, *sd_dev, *card_dev;
     qemu_irq flash_cs, sd_cs;
 
     /* Initialize SoC */
@@ -686,6 +688,15 @@ static void sifive_u_machine_init(MachineState *machine)
 
     sd_cs = qdev_get_gpio_in_named(sd_dev, SSI_GPIO_CS, 0);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi2), 1, sd_cs);
+
+    dinfo = drive_get(IF_SD, 0, 0);
+    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
+    card_dev = qdev_new(TYPE_SD_CARD);
+    qdev_prop_set_drive_err(card_dev, "drive", blk, &error_fatal);
+    qdev_prop_set_bit(card_dev, "spi", true);
+    qdev_realize_and_unref(card_dev,
+                           qdev_get_child_bus(sd_dev, "sd-bus"),
+                           &error_fatal);
 }
 
 static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index e60854eeef..167c03b780 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -368,36 +368,9 @@ static const VMStateDescription vmstate_ssi_sd = {
 
 static void ssi_sd_realize(SSIPeripheral *d, Error **errp)
 {
-    ERRP_GUARD();
     ssi_sd_state *s = SSI_SD(d);
-    DeviceState *carddev;
-    DriveInfo *dinfo;
 
     qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, DEVICE(d), "sd-bus");
-
-    /* Create and plug in the sd card */
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    dinfo = drive_get_next(IF_SD);
-    carddev = qdev_new(TYPE_SD_CARD);
-    if (dinfo) {
-        if (!qdev_prop_set_drive_err(carddev, "drive",
-                                     blk_by_legacy_dinfo(dinfo), errp)) {
-            goto fail;
-        }
-    }
-
-    if (!object_property_set_bool(OBJECT(carddev), "spi", true, errp)) {
-        goto fail;
-    }
-
-    if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), errp)) {
-        goto fail;
-    }
-
-    return;
-
-fail:
-    error_prepend(errp, "failed to init SD card: ");
 }
 
 static void ssi_sd_reset(DeviceState *dev)
@@ -426,7 +399,7 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
     k->cs_polarity = SSI_CS_LOW;
     dc->vmsd = &vmstate_ssi_sd;
     dc->reset = ssi_sd_reset;
-    /* Reason: init() method uses drive_get_next() */
+    /* Reason: GPIO chip-select line should be wired up */
     dc->user_creatable = false;
 }
 
-- 
2.31.1

Re: [PATCH v2 01/13] hw/sd/ssi-sd: Do not create SD card within controller's realize

[Philippe Mathieu-Daudé]

Hi Markus, Peter,

On 11/17/21 17:33, Markus Armbruster wrote:
> ssi_sd_realize() creates an "sd-card" device.  This is inappropriate,
> and marked FIXME.
> 
> Move it to the boards that create these devices.  Prior art: commit
> eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for
> device "pl181".
> 
> The device remains not user-creatable, because its users should (and
> do) wire up its GPIO chip-select line.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/stellaris.c  | 15 ++++++++++++++-
>  hw/riscv/sifive_u.c | 13 ++++++++++++-
>  hw/sd/ssi-sd.c      | 29 +----------------------------
>  3 files changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 78827ace6b..b6c8a5d609 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -10,6 +10,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/sysbus.h"
> +#include "hw/sd/sd.h"
>  #include "hw/ssi/ssi.h"
>  #include "hw/arm/boot.h"
>  #include "qemu/timer.h"
> @@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>              void *bus;
>              DeviceState *sddev;
>              DeviceState *ssddev;
> +            DriveInfo *dinfo;
> +            DeviceState *carddev;
> +            BlockBackend *blk;
>  
>              /*
>               * Some boards have both an OLED controller and SD card connected to
> @@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>               *  - Make the ssd0323 OLED controller chipselect active-low
>               */
>              bus = qdev_get_child_bus(dev, "ssi");
> -
>              sddev = ssi_create_peripheral(bus, "ssi-sd");
> +
> +            dinfo = drive_get(IF_SD, 0, 0);
> +            blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> +            carddev = qdev_new(TYPE_SD_CARD);
> +            qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);

I guess a already asked this once but don't remember now... What is the
point of creating a SD card without drive? Is this due to the old design
issue we hotplug the drive to the SD card and not the SD card on the SD
bus?

> +            qdev_prop_set_bit(carddev, "spi", true);
> +            qdev_realize_and_unref(carddev,
> +                                   qdev_get_child_bus(sddev, "sd-bus"),
> +                                   &error_fatal);
> +

Re: [PATCH v2 01/13] hw/sd/ssi-sd: Do not create SD card within controller's realize

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Markus, Peter,
>
> On 11/17/21 17:33, Markus Armbruster wrote:
>> ssi_sd_realize() creates an "sd-card" device.  This is inappropriate,
>> and marked FIXME.
>> 
>> Move it to the boards that create these devices.  Prior art: commit
>> eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for
>> device "pl181".
>> 
>> The device remains not user-creatable, because its users should (and
>> do) wire up its GPIO chip-select line.
>> 
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Alistair Francis <Alistair.Francis@wdc.com>
>> Cc: Bin Meng <bin.meng@windriver.com>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
>> Cc: qemu-arm@nongnu.org
>> Cc: qemu-riscv@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> ---
>>  hw/arm/stellaris.c  | 15 ++++++++++++++-
>>  hw/riscv/sifive_u.c | 13 ++++++++++++-
>>  hw/sd/ssi-sd.c      | 29 +----------------------------
>>  3 files changed, 27 insertions(+), 30 deletions(-)
>> 
>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>> index 78827ace6b..b6c8a5d609 100644
>> --- a/hw/arm/stellaris.c
>> +++ b/hw/arm/stellaris.c
>> @@ -10,6 +10,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>>  #include "hw/sysbus.h"
>> +#include "hw/sd/sd.h"
>>  #include "hw/ssi/ssi.h"
>>  #include "hw/arm/boot.h"
>>  #include "qemu/timer.h"
>> @@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>              void *bus;
>>              DeviceState *sddev;
>>              DeviceState *ssddev;
>> +            DriveInfo *dinfo;
>> +            DeviceState *carddev;
>> +            BlockBackend *blk;
>>  
>>              /*
>>               * Some boards have both an OLED controller and SD card connected to
>> @@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>               *  - Make the ssd0323 OLED controller chipselect active-low
>>               */
>>              bus = qdev_get_child_bus(dev, "ssi");
>> -
>>              sddev = ssi_create_peripheral(bus, "ssi-sd");
>> +
>> +            dinfo = drive_get(IF_SD, 0, 0);
>> +            blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
>> +            carddev = qdev_new(TYPE_SD_CARD);
>> +            qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
>
> I guess a already asked this once but don't remember now... What is the
> point of creating a SD card without drive? Is this due to the old design
> issue we hotplug the drive to the SD card and not the SD card on the SD
> bus?

Device model "sd-card" implements BlockdevOps method change_media_cb().
This menas it models a device with a removable medium.  No drive behaves
like no medium.  I guess it's an SD card reader.

>
>> +            qdev_prop_set_bit(carddev, "spi", true);
>> +            qdev_realize_and_unref(carddev,
>> +                                   qdev_get_child_bus(sddev, "sd-bus"),
>> +                                   &error_fatal);
>> +

Re: [PATCH v2 01/13] hw/sd/ssi-sd: Do not create SD card within controller's realize

[Peter Maydell]

On Mon, 6 Dec 2021 at 12:35, Markus Armbruster <armbru@redhat.com> wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > I guess a already asked this once but don't remember now... What is the
> > point of creating a SD card without drive? Is this due to the old design
> > issue we hotplug the drive to the SD card and not the SD card on the SD
> > bus?
>
> Device model "sd-card" implements BlockdevOps method change_media_cb().
> This menas it models a device with a removable medium.  No drive behaves
> like no medium.  I guess it's an SD card reader.

No, device sd-card really is the SD card -- the protocol between
the SD controller device and the sd-card device is (a slightly
abstracted version of) the protocol that goes between the card
and the controller in hardware. I think the reason it's implemented
as a "device with a removable medium" is historical -- back in
2007 that was the only way to support runtime ejecting and
insertion. We'd implement it differently today, but we've wanted
to preserve the user-facing compatibility of how the monitor
commands for ejecting and inserting an sd card work.

(Side note, the initial sd implementation actually tells the block
layer that it's BDRV_TYPE_FLOPPY as a "reasonable approximation"...)

-- PMM

Re: [PATCH v2 01/13] hw/sd/ssi-sd: Do not create SD card within controller's realize

[Alistair Francis]

On Thu, Nov 18, 2021 at 2:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> ssi_sd_realize() creates an "sd-card" device.  This is inappropriate,
> and marked FIXME.
>
> Move it to the boards that create these devices.  Prior art: commit
> eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for
> device "pl181".
>
> The device remains not user-creatable, because its users should (and
> do) wire up its GPIO chip-select line.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

Alistair

> ---
>  hw/arm/stellaris.c  | 15 ++++++++++++++-
>  hw/riscv/sifive_u.c | 13 ++++++++++++-
>  hw/sd/ssi-sd.c      | 29 +----------------------------
>  3 files changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 78827ace6b..b6c8a5d609 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -10,6 +10,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/sysbus.h"
> +#include "hw/sd/sd.h"
>  #include "hw/ssi/ssi.h"
>  #include "hw/arm/boot.h"
>  #include "qemu/timer.h"
> @@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>              void *bus;
>              DeviceState *sddev;
>              DeviceState *ssddev;
> +            DriveInfo *dinfo;
> +            DeviceState *carddev;
> +            BlockBackend *blk;
>
>              /*
>               * Some boards have both an OLED controller and SD card connected to
> @@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>               *  - Make the ssd0323 OLED controller chipselect active-low
>               */
>              bus = qdev_get_child_bus(dev, "ssi");
> -
>              sddev = ssi_create_peripheral(bus, "ssi-sd");
> +
> +            dinfo = drive_get(IF_SD, 0, 0);
> +            blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> +            carddev = qdev_new(TYPE_SD_CARD);
> +            qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
> +            qdev_prop_set_bit(carddev, "spi", true);
> +            qdev_realize_and_unref(carddev,
> +                                   qdev_get_child_bus(sddev, "sd-bus"),
> +                                   &error_fatal);
> +
>              ssddev = ssi_create_peripheral(bus, "ssd0323");
>              gpio_out[GPIO_D][0] = qemu_irq_split(
>                      qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0),
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 589ae72a59..a4ecadaf12 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -46,6 +46,7 @@
>  #include "hw/char/serial.h"
>  #include "hw/cpu/cluster.h"
>  #include "hw/misc/unimp.h"
> +#include "hw/sd/sd.h"
>  #include "hw/ssi/ssi.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/riscv_hart.h"
> @@ -536,7 +537,8 @@ static void sifive_u_machine_init(MachineState *machine)
>      uint32_t fdt_load_addr;
>      uint64_t kernel_entry;
>      DriveInfo *dinfo;
> -    DeviceState *flash_dev, *sd_dev;
> +    BlockBackend *blk;
> +    DeviceState *flash_dev, *sd_dev, *card_dev;
>      qemu_irq flash_cs, sd_cs;
>
>      /* Initialize SoC */
> @@ -686,6 +688,15 @@ static void sifive_u_machine_init(MachineState *machine)
>
>      sd_cs = qdev_get_gpio_in_named(sd_dev, SSI_GPIO_CS, 0);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi2), 1, sd_cs);
> +
> +    dinfo = drive_get(IF_SD, 0, 0);
> +    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> +    card_dev = qdev_new(TYPE_SD_CARD);
> +    qdev_prop_set_drive_err(card_dev, "drive", blk, &error_fatal);
> +    qdev_prop_set_bit(card_dev, "spi", true);
> +    qdev_realize_and_unref(card_dev,
> +                           qdev_get_child_bus(sd_dev, "sd-bus"),
> +                           &error_fatal);
>  }
>
>  static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp)
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index e60854eeef..167c03b780 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -368,36 +368,9 @@ static const VMStateDescription vmstate_ssi_sd = {
>
>  static void ssi_sd_realize(SSIPeripheral *d, Error **errp)
>  {
> -    ERRP_GUARD();
>      ssi_sd_state *s = SSI_SD(d);
> -    DeviceState *carddev;
> -    DriveInfo *dinfo;
>
>      qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, DEVICE(d), "sd-bus");
> -
> -    /* Create and plug in the sd card */
> -    /* FIXME use a qdev drive property instead of drive_get_next() */
> -    dinfo = drive_get_next(IF_SD);
> -    carddev = qdev_new(TYPE_SD_CARD);
> -    if (dinfo) {
> -        if (!qdev_prop_set_drive_err(carddev, "drive",
> -                                     blk_by_legacy_dinfo(dinfo), errp)) {
> -            goto fail;
> -        }
> -    }
> -
> -    if (!object_property_set_bool(OBJECT(carddev), "spi", true, errp)) {
> -        goto fail;
> -    }
> -
> -    if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), errp)) {
> -        goto fail;
> -    }
> -
> -    return;
> -
> -fail:
> -    error_prepend(errp, "failed to init SD card: ");
>  }
>
>  static void ssi_sd_reset(DeviceState *dev)
> @@ -426,7 +399,7 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
>      k->cs_polarity = SSI_CS_LOW;
>      dc->vmsd = &vmstate_ssi_sd;
>      dc->reset = ssi_sd_reset;
> -    /* Reason: init() method uses drive_get_next() */
> +    /* Reason: GPIO chip-select line should be wired up */
>      dc->user_creatable = false;
>  }
>
> --
> 2.31.1
>
>

[PATCH v2 02/13] hw: Replace trivial drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

A number of machines connect just one backend with drive_get_next().
Change them to use drive_get() directly.  This makes the (zero) unit
number explicit in the code.

Cc: Beniamino Galvani <b.galvani@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Subbaraya Sundeep <sundeep.lkml@gmail.com>
Cc: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: qemu-arm@nongnu.org
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/cubieboard.c        | 2 +-
 hw/arm/integratorcp.c      | 2 +-
 hw/arm/msf2-som.c          | 2 +-
 hw/arm/orangepi.c          | 2 +-
 hw/arm/raspi.c             | 2 +-
 hw/arm/realview.c          | 2 +-
 hw/arm/sabrelite.c         | 2 +-
 hw/misc/sifive_u_otp.c     | 2 +-
 hw/riscv/microchip_pfsoc.c | 2 +-
 hw/riscv/sifive_u.c        | 2 +-
 hw/sparc64/niagara.c       | 2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 294ba5de6e..5e3372a3c7 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -81,7 +81,7 @@ static void cubieboard_init(MachineState *machine)
     }
 
     /* Retrieve SD bus */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(a10), "sd-bus");
 
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 16e8985953..b109ece3ae 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -649,7 +649,7 @@ static void integratorcp_init(MachineState *machine)
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_WPROT, 0));
     qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 0));
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
index 396e8b9913..d9f881690e 100644
--- a/hw/arm/msf2-som.c
+++ b/hw/arm/msf2-som.c
@@ -45,7 +45,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
     DeviceState *spi_flash;
     MSF2State *soc;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    DriveInfo *dinfo = drive_get_next(IF_MTD);
+    DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
     qemu_irq cs_line;
     BusState *spi_bus;
     MemoryRegion *sysmem = get_system_memory();
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 0cf9895ce7..e796382236 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -85,7 +85,7 @@ static void orangepi_init(MachineState *machine)
     qdev_realize(DEVICE(h3), NULL, &error_abort);
 
     /* Retrieve SD bus */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(h3), "sd-bus");
 
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 146d35382b..b4dd6c1e99 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -284,7 +284,7 @@ static void raspi_machine_init(MachineState *machine)
     qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* Create and plug in the SD cards */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
     if (bus == NULL) {
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 1c54316ba3..ddc70b54a5 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -237,7 +237,7 @@ static void realview_init(MachineState *machine,
         qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
     qdev_connect_gpio_out_named(dev, "card-read-only", 0, mmc_irq[0]);
     qdev_connect_gpio_out_named(dev, "card-inserted", 0, mmc_irq[1]);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index 553608e583..cce49aa25c 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -76,7 +76,7 @@ static void sabrelite_init(MachineState *machine)
             if (spi_bus) {
                 DeviceState *flash_dev;
                 qemu_irq cs_line;
-                DriveInfo *dinfo = drive_get_next(IF_MTD);
+                DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
 
                 flash_dev = qdev_new("sst25vf016b");
                 if (dinfo) {
diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 18aa0bd55d..5d5a8c8a90 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -209,7 +209,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
                           TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
 
-    dinfo = drive_get_next(IF_NONE);
+    dinfo = drive_get(IF_NONE, 0, 0);
     if (dinfo) {
         int ret;
         uint64_t perm;
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 57d779fb55..d1d065efbc 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -458,7 +458,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
     target_ulong firmware_end_addr, kernel_start_addr;
     uint64_t kernel_entry;
     uint32_t fdt_load_addr;
-    DriveInfo *dinfo = drive_get_next(IF_SD);
+    DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
 
     /* Sanity check on RAM size */
     if (machine->ram_size < mc->default_ram_size) {
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index a4ecadaf12..aa74e67889 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -672,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine)
 
     /* Connect an SPI flash to SPI0 */
     flash_dev = qdev_new("is25wp256");
-    dinfo = drive_get_next(IF_MTD);
+    dinfo = drive_get(IF_MTD, 0, 0);
     if (dinfo) {
         qdev_prop_set_drive_err(flash_dev, "drive",
                                 blk_by_legacy_dinfo(dinfo),
diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index f3e42d0326..ccad2c43a3 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -98,7 +98,7 @@ static void add_rom_or_fail(const char *file, const hwaddr addr)
 static void niagara_init(MachineState *machine)
 {
     NiagaraBoardState *s = g_new(NiagaraBoardState, 1);
-    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
+    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
     MemoryRegion *sysmem = get_system_memory();
 
     /* init CPUs */
-- 
2.31.1

Re: [PATCH v2 02/13] hw: Replace trivial drive_get_next() by drive_get()

[Alistair Francis]

On Thu, Nov 18, 2021 at 2:42 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> A number of machines connect just one backend with drive_get_next().
> Change them to use drive_get() directly.  This makes the (zero) unit
> number explicit in the code.
>
> Cc: Beniamino Galvani <b.galvani@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Subbaraya Sundeep <sundeep.lkml@gmail.com>
> Cc: Niek Linnenbank <nieklinnenbank@gmail.com>
> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
> Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

Alistair

> ---
>  hw/arm/cubieboard.c        | 2 +-
>  hw/arm/integratorcp.c      | 2 +-
>  hw/arm/msf2-som.c          | 2 +-
>  hw/arm/orangepi.c          | 2 +-
>  hw/arm/raspi.c             | 2 +-
>  hw/arm/realview.c          | 2 +-
>  hw/arm/sabrelite.c         | 2 +-
>  hw/misc/sifive_u_otp.c     | 2 +-
>  hw/riscv/microchip_pfsoc.c | 2 +-
>  hw/riscv/sifive_u.c        | 2 +-
>  hw/sparc64/niagara.c       | 2 +-
>  11 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> index 294ba5de6e..5e3372a3c7 100644
> --- a/hw/arm/cubieboard.c
> +++ b/hw/arm/cubieboard.c
> @@ -81,7 +81,7 @@ static void cubieboard_init(MachineState *machine)
>      }
>
>      /* Retrieve SD bus */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(a10), "sd-bus");
>
> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index 16e8985953..b109ece3ae 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -649,7 +649,7 @@ static void integratorcp_init(MachineState *machine)
>                            qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_WPROT, 0));
>      qdev_connect_gpio_out_named(dev, "card-inserted", 0,
>                            qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 0));
> -    dinfo = drive_get_next(IF_SD);
> +    dinfo = drive_get(IF_SD, 0, 0);
>      if (dinfo) {
>          DeviceState *card;
>
> diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
> index 396e8b9913..d9f881690e 100644
> --- a/hw/arm/msf2-som.c
> +++ b/hw/arm/msf2-som.c
> @@ -45,7 +45,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
>      DeviceState *spi_flash;
>      MSF2State *soc;
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> -    DriveInfo *dinfo = drive_get_next(IF_MTD);
> +    DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
>      qemu_irq cs_line;
>      BusState *spi_bus;
>      MemoryRegion *sysmem = get_system_memory();
> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> index 0cf9895ce7..e796382236 100644
> --- a/hw/arm/orangepi.c
> +++ b/hw/arm/orangepi.c
> @@ -85,7 +85,7 @@ static void orangepi_init(MachineState *machine)
>      qdev_realize(DEVICE(h3), NULL, &error_abort);
>
>      /* Retrieve SD bus */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(h3), "sd-bus");
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 146d35382b..b4dd6c1e99 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -284,7 +284,7 @@ static void raspi_machine_init(MachineState *machine)
>      qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
>
>      /* Create and plug in the SD cards */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
>      if (bus == NULL) {
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 1c54316ba3..ddc70b54a5 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -237,7 +237,7 @@ static void realview_init(MachineState *machine,
>          qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
>      qdev_connect_gpio_out_named(dev, "card-read-only", 0, mmc_irq[0]);
>      qdev_connect_gpio_out_named(dev, "card-inserted", 0, mmc_irq[1]);
> -    dinfo = drive_get_next(IF_SD);
> +    dinfo = drive_get(IF_SD, 0, 0);
>      if (dinfo) {
>          DeviceState *card;
>
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index 553608e583..cce49aa25c 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -76,7 +76,7 @@ static void sabrelite_init(MachineState *machine)
>              if (spi_bus) {
>                  DeviceState *flash_dev;
>                  qemu_irq cs_line;
> -                DriveInfo *dinfo = drive_get_next(IF_MTD);
> +                DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
>
>                  flash_dev = qdev_new("sst25vf016b");
>                  if (dinfo) {
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 18aa0bd55d..5d5a8c8a90 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -209,7 +209,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
>                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>
> -    dinfo = drive_get_next(IF_NONE);
> +    dinfo = drive_get(IF_NONE, 0, 0);
>      if (dinfo) {
>          int ret;
>          uint64_t perm;
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 57d779fb55..d1d065efbc 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -458,7 +458,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>      target_ulong firmware_end_addr, kernel_start_addr;
>      uint64_t kernel_entry;
>      uint32_t fdt_load_addr;
> -    DriveInfo *dinfo = drive_get_next(IF_SD);
> +    DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
>
>      /* Sanity check on RAM size */
>      if (machine->ram_size < mc->default_ram_size) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index a4ecadaf12..aa74e67889 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -672,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine)
>
>      /* Connect an SPI flash to SPI0 */
>      flash_dev = qdev_new("is25wp256");
> -    dinfo = drive_get_next(IF_MTD);
> +    dinfo = drive_get(IF_MTD, 0, 0);
>      if (dinfo) {
>          qdev_prop_set_drive_err(flash_dev, "drive",
>                                  blk_by_legacy_dinfo(dinfo),
> diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> index f3e42d0326..ccad2c43a3 100644
> --- a/hw/sparc64/niagara.c
> +++ b/hw/sparc64/niagara.c
> @@ -98,7 +98,7 @@ static void add_rom_or_fail(const char *file, const hwaddr addr)
>  static void niagara_init(MachineState *machine)
>  {
>      NiagaraBoardState *s = g_new(NiagaraBoardState, 1);
> -    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
> +    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>      MemoryRegion *sysmem = get_system_memory();
>
>      /* init CPUs */
> --
> 2.31.1
>
>

[PATCH v2 03/13] hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "quanta-gbs-bmc" connects just one backend with
drive_get_next(), but with a helper function.  Change it to use
drive_get() directly.  This makes the unit numbers explicit in the
code.

Cc: Havard Skinnemoen <hskinnemoen@google.com>
Cc: Tyrone Ting <kfting@nuvoton.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/npcm7xx_boards.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index dec7d16ae5..d8a49e4e85 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
                              &error_abort);
 }
 
-static void sdhci_attach_drive(SDHCIState *sdhci)
+static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
 {
-        DriveInfo *di = drive_get_next(IF_SD);
+        DriveInfo *di = drive_get(IF_SD, 0, unit);
         BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
 
         BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus");
@@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)
                           drive_get(IF_MTD, 0, 0));
 
     quanta_gbs_i2c_init(soc);
-    sdhci_attach_drive(&soc->mmc.sdhci);
+    sdhci_attach_drive(&soc->mmc.sdhci, 0);
     npcm7xx_load_kernel(machine, soc);
 }
 
-- 
2.31.1

Re: [PATCH v2 03/13] hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get()

[Havard Skinnemoen]

On Wed, Nov 17, 2021 at 8:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> Machine "quanta-gbs-bmc" connects just one backend with
> drive_get_next(), but with a helper function.  Change it to use
> drive_get() directly.  This makes the unit numbers explicit in the
> code.
>
> Cc: Havard Skinnemoen <hskinnemoen@google.com>
> Cc: Tyrone Ting <kfting@nuvoton.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/npcm7xx_boards.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index dec7d16ae5..d8a49e4e85 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
>                               &error_abort);
>  }
>
> -static void sdhci_attach_drive(SDHCIState *sdhci)
> +static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
>  {
> -        DriveInfo *di = drive_get_next(IF_SD);
> +        DriveInfo *di = drive_get(IF_SD, 0, unit);

+Hao Wu IIRC the chip has separate SD and eMMC buses. Would it make
sense to take the bus number as a parameter as well? Is bus 0 the
right one to use in this case?

The existing code always uses bus 0, so this is an improvement either way.

Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>

>          BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
>
>          BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus");
> @@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)
>                            drive_get(IF_MTD, 0, 0));
>
>      quanta_gbs_i2c_init(soc);
> -    sdhci_attach_drive(&soc->mmc.sdhci);
> +    sdhci_attach_drive(&soc->mmc.sdhci, 0);
>      npcm7xx_load_kernel(machine, soc);
>  }
>
> --
> 2.31.1
>

Re: [PATCH v2 03/13] hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get()

[Hao Wu]

[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]

Yes, there's SD and MMC buses. It looks like the current code only supports
mmc ("soc->mmc.sdhci") but not the sd ("soc->sd.sdhci").

It's probably good to make the bus number a parameter as well and use them
to distinguish. We might need a separate patch to do that.

On Wed, Nov 17, 2021 at 8:54 AM Havard Skinnemoen <hskinnemoen@google.com>
wrote:

> On Wed, Nov 17, 2021 at 8:34 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> > drive_get_next() is basically a bad idea.  It returns the "next" block
> > backend of a certain interface type.  "Next" means bus=0,unit=N, where
> > subsequent calls count N up from zero, per interface type.
> >
> > This lets you define unit numbers implicitly by execution order.  If the
> > order changes, or new calls appear "in the middle", unit numbers change.
> > ABI break.  Hard to spot in review.
> >
> > Machine "quanta-gbs-bmc" connects just one backend with
> > drive_get_next(), but with a helper function.  Change it to use
> > drive_get() directly.  This makes the unit numbers explicit in the
> > code.
> >
> > Cc: Havard Skinnemoen <hskinnemoen@google.com>
> > Cc: Tyrone Ting <kfting@nuvoton.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: qemu-arm@nongnu.org
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/arm/npcm7xx_boards.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> > index dec7d16ae5..d8a49e4e85 100644
> > --- a/hw/arm/npcm7xx_boards.c
> > +++ b/hw/arm/npcm7xx_boards.c
> > @@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc,
> MemoryRegion *dram)
> >                               &error_abort);
> >  }
> >
> > -static void sdhci_attach_drive(SDHCIState *sdhci)
> > +static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
> >  {
> > -        DriveInfo *di = drive_get_next(IF_SD);
> > +        DriveInfo *di = drive_get(IF_SD, 0, unit);
>
> +Hao Wu IIRC the chip has separate SD and eMMC buses. Would it make
> sense to take the bus number as a parameter as well? Is bus 0 the
> right one to use in this case?
>
> The existing code always uses bus 0, so this is an improvement either way.
>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
>
> >          BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> >
> >          BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus");
> > @@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)
> >                            drive_get(IF_MTD, 0, 0));
> >
> >      quanta_gbs_i2c_init(soc);
> > -    sdhci_attach_drive(&soc->mmc.sdhci);
> > +    sdhci_attach_drive(&soc->mmc.sdhci, 0);
> >      npcm7xx_load_kernel(machine, soc);
> >  }
> >
> > --
> > 2.31.1
> >
>

[-- Attachment #2: Type: text/html, Size: 4021 bytes --]

Re: [PATCH v2 03/13] hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get()

[Markus Armbruster]

Havard Skinnemoen <hskinnemoen@google.com> writes:

> On Wed, Nov 17, 2021 at 8:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> drive_get_next() is basically a bad idea.  It returns the "next" block
>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>> subsequent calls count N up from zero, per interface type.
>>
>> This lets you define unit numbers implicitly by execution order.  If the
>> order changes, or new calls appear "in the middle", unit numbers change.
>> ABI break.  Hard to spot in review.
>>
>> Machine "quanta-gbs-bmc" connects just one backend with
>> drive_get_next(), but with a helper function.  Change it to use
>> drive_get() directly.  This makes the unit numbers explicit in the
>> code.
>>
>> Cc: Havard Skinnemoen <hskinnemoen@google.com>
>> Cc: Tyrone Ting <kfting@nuvoton.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/arm/npcm7xx_boards.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
>> index dec7d16ae5..d8a49e4e85 100644
>> --- a/hw/arm/npcm7xx_boards.c
>> +++ b/hw/arm/npcm7xx_boards.c
>> @@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
>>                               &error_abort);
>>  }
>>
>> -static void sdhci_attach_drive(SDHCIState *sdhci)
>> +static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
>>  {
>> -        DriveInfo *di = drive_get_next(IF_SD);
>> +        DriveInfo *di = drive_get(IF_SD, 0, unit);
>
> +Hao Wu IIRC the chip has separate SD and eMMC buses. Would it make
> sense to take the bus number as a parameter as well? Is bus 0 the
> right one to use in this case?
>
> The existing code always uses bus 0, so this is an improvement either way.

Using separate buses for different kinds of devices would be neater, but
it also would be an incompatible change.  I can't judge whether
incompatible change is okay here.

This patch is just refactoring code.  An interface change, if desired,
should be a separate patch.

> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>

Thanks!

>
>>          BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
>>
>>          BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus");
>> @@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)
>>                            drive_get(IF_MTD, 0, 0));
>>
>>      quanta_gbs_i2c_init(soc);
>> -    sdhci_attach_drive(&soc->mmc.sdhci);
>> +    sdhci_attach_drive(&soc->mmc.sdhci, 0);
>>      npcm7xx_load_kernel(machine, soc);
>>  }
>>
>> --
>> 2.31.1
>>

[PATCH v2 04/13] hw/arm/versatilepb hw/arm/vexpress: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

The versatile and vexpress machines ("versatileab", "versatilepb",
"vexpress-a9", "vexpress-a15") connect just one or two backends of a
type with drive_get_next().  Change them to use drive_get() directly.
This makes the unit numbers explicit in the code.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/versatilepb.c | 4 ++--
 hw/arm/vexpress.c    | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 575399c4fc..ecc1f6cf74 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -310,7 +310,7 @@ static void versatile_init(MachineState *machine, int board_id)
     qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
 
     dev = sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
@@ -322,7 +322,7 @@ static void versatile_init(MachineState *machine, int board_id)
     }
 
     dev = sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 1);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 58481c0762..966758cf82 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -625,7 +625,7 @@ static void vexpress_common_init(MachineState *machine)
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
     qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
@@ -657,7 +657,7 @@ static void vexpress_common_init(MachineState *machine)
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
 
-    dinfo = drive_get_next(IF_PFLASH);
+    dinfo = drive_get(IF_PFLASH, 0, 0);
     pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
                                        dinfo);
     if (!pflash0) {
@@ -673,7 +673,7 @@ static void vexpress_common_init(MachineState *machine)
         memory_region_add_subregion(sysmem, map[VE_NORFLASHALIAS], flashalias);
     }
 
-    dinfo = drive_get_next(IF_PFLASH);
+    dinfo = drive_get(IF_PFLASH, 0, 1);
     if (!ve_pflash_cfi01_register(map[VE_NORFLASH1], "vexpress.flash1",
                                   dinfo)) {
         error_report("vexpress: error registering flash 1");
-- 
2.31.1

Re: [PATCH v2 04/13] hw/arm/versatilepb hw/arm/vexpress: Replace drive_get_next() by drive_get()

[Peter Maydell]

On Wed, 17 Nov 2021 at 16:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> The versatile and vexpress machines ("versatileab", "versatilepb",
> "vexpress-a9", "vexpress-a15") connect just one or two backends of a
> type with drive_get_next().  Change them to use drive_get() directly.
> This makes the unit numbers explicit in the code.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 05/13] hw/arm/imx25_pdk: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "imx25-pdk" connects backends with drive_get_next() in a
counting loop.  Change it to use drive_get() directly.  This makes the
unit numbers explicit in the code.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/imx25_pdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index bd16acd4d9..6dff000163 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -123,7 +123,7 @@ static void imx25_pdk_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->soc.esdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
-- 
2.31.1

Re: [PATCH v2 05/13] hw/arm/imx25_pdk: Replace drive_get_next() by drive_get()

[Peter Maydell]

On Wed, 17 Nov 2021 at 16:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> Machine "imx25-pdk" connects backends with drive_get_next() in a
> counting loop.  Change it to use drive_get() directly.  This makes the
> unit numbers explicit in the code.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 06/13] hw/arm/mcimx6ul-evk: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "mcimx6ul-evk" connects backends with drive_get_next() in a
counting loop.  Change it to use drive_get() directly.  This makes the
unit numbers explicit in the code.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/mcimx6ul-evk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
index 77fae874b1..28b4886f48 100644
--- a/hw/arm/mcimx6ul-evk.c
+++ b/hw/arm/mcimx6ul-evk.c
@@ -52,7 +52,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
-- 
2.31.1

Re: [PATCH v2 06/13] hw/arm/mcimx6ul-evk: Replace drive_get_next() by drive_get()

[Peter Maydell]

On Wed, 17 Nov 2021 at 16:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> Machine "mcimx6ul-evk" connects backends with drive_get_next() in a
> counting loop.  Change it to use drive_get() directly.  This makes the
> unit numbers explicit in the code.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 07/13] hw/arm/mcimx7d-sabre: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "mcimx7d-sabre" connects backends with drive_get_next() in a
counting loop.  Change it to use drive_get() directly.  This makes the
unit numbers explicit in the code.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/mcimx7d-sabre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index 935d4b0f1c..50a5ecde31 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -52,7 +52,7 @@ static void mcimx7d_sabre_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
-- 
2.31.1

Re: [PATCH v2 07/13] hw/arm/mcimx7d-sabre: Replace drive_get_next() by drive_get()

[Peter Maydell]

On Wed, 17 Nov 2021 at 16:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> Machine "mcimx7d-sabre" connects backends with drive_get_next() in a
> counting loop.  Change it to use drive_get() directly.  This makes the
> unit numbers explicit in the code.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 08/13] hw/arm/xlnx-versal-virt: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "xlnx-versal-virt" connects backends with drive_get_next() in
a counting loop.  Change it to use drive_get() directly.  This makes
the unit numbers explicit in the code.

Cc: Alistair Francis <alistair@alistair23.me>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/xlnx-versal-virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index d2f55e29b6..0c5edc898e 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -669,7 +669,8 @@ static void versal_virt_init(MachineState *machine)
 
     /* Plugin SD cards.  */
     for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
-        sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
+        sd_plugin_card(&s->soc.pmc.iou.sd[i],
+                       drive_get(IF_SD, 0, i));
     }
 
     s->binfo.ram_size = machine->ram_size;
-- 
2.31.1

Re: [PATCH v2 08/13] hw/arm/xlnx-versal-virt: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:04PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-versal-virt" connects backends with drive_get_next() in
> a counting loop.  Change it to use drive_get() directly.  This makes
> the unit numbers explicit in the code.

Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/xlnx-versal-virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index d2f55e29b6..0c5edc898e 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -669,7 +669,8 @@ static void versal_virt_init(MachineState *machine)
>  
>      /* Plugin SD cards.  */
>      for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
> -        sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
> +        sd_plugin_card(&s->soc.pmc.iou.sd[i],
> +                       drive_get(IF_SD, 0, i));
>      }
>  
>      s->binfo.ram_size = machine->ram_size;
> -- 
> 2.31.1
> 

[PATCH v2 09/13] hw/microblaze: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "petalogix-ml605" connects backends with drive_get_next() in a
counting loop.  Change it to use drive_get() directly.  This makes the
unit numbers explicit in the code.

Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/microblaze/petalogix_ml605_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 159db6cbe2..a24fadddca 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
         spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
 
         for (i = 0; i < NUM_SPI_FLASHES; i++) {
-            DriveInfo *dinfo = drive_get_next(IF_MTD);
+            DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
             qemu_irq cs_line;
 
             dev = qdev_new("n25q128");
-- 
2.31.1

Re: [PATCH v2 09/13] hw/microblaze: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:05PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "petalogix-ml605" connects backends with drive_get_next() in a
> counting loop.  Change it to use drive_get() directly.  This makes the
> unit numbers explicit in the code.


Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/microblaze/petalogix_ml605_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
> index 159db6cbe2..a24fadddca 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
>          spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
>  
>          for (i = 0; i < NUM_SPI_FLASHES; i++) {
> -            DriveInfo *dinfo = drive_get_next(IF_MTD);
> +            DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>              qemu_irq cs_line;
>  
>              dev = qdev_new("n25q128");
> -- 
> 2.31.1
> 

[PATCH v2 10/13] hw/arm/xlnx-zcu102: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "xlnx-zcu102" connects backends with drive_get_next() in
several counting loops.  Change it to use drive_get() directly.  This
makes the unit numbers explicit in the code.

Cc: Alistair Francis <alistair@alistair23.me>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/xlnx-zcu102.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 3dc2b5e8ca..45eb19ab3b 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -169,7 +169,7 @@ static void xlnx_zcu102_init(MachineState *machine)
     /* 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);
+        DriveInfo *di = drive_get(IF_SD, 0, i);
         BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
         DeviceState *carddev;
         char *bus_name;
@@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
         BusState *spi_bus;
         DeviceState *flash_dev;
         qemu_irq cs_line;
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
         gchar *bus_name = g_strdup_printf("spi%d", i);
 
         spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
@@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
         BusState *spi_bus;
         DeviceState *flash_dev;
         qemu_irq cs_line;
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
         int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
         gchar *bus_name = g_strdup_printf("qspi%d", bus);
 
-- 
2.31.1

Re: [PATCH v2 10/13] hw/arm/xlnx-zcu102: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:06PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-zcu102" connects backends with drive_get_next() in
> several counting loops.  Change it to use drive_get() directly.  This
> makes the unit numbers explicit in the code.


Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/xlnx-zcu102.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 3dc2b5e8ca..45eb19ab3b 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -169,7 +169,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>      /* 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);
> +        DriveInfo *di = drive_get(IF_SD, 0, i);
>          BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
>          DeviceState *carddev;
>          char *bus_name;
> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>          BusState *spi_bus;
>          DeviceState *flash_dev;
>          qemu_irq cs_line;
> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>          gchar *bus_name = g_strdup_printf("spi%d", i);
>  
>          spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>          BusState *spi_bus;
>          DeviceState *flash_dev;
>          qemu_irq cs_line;
> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
>          int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
>          gchar *bus_name = g_strdup_printf("qspi%d", bus);
>  
> -- 
> 2.31.1
> 

[PATCH v2 11/13] hw/arm/xilinx_zynq: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Machine "xlnx-zcu102" connects backends with drive_get_next() in two
counting loops, one of them in a helper function.  Change it to use
drive_get() directly.  This makes the unit numbers explicit in the
code.

Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/xilinx_zynq.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 69c333e91b..50e7268396 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -125,9 +125,10 @@ static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     sysbus_connect_irq(s, 0, irq);
 }
 
-static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
-                                         bool is_qspi)
+static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
+                                        bool is_qspi, int unit0)
 {
+    int unit = unit0;
     DeviceState *dev;
     SysBusDevice *busdev;
     SSIBus *spi;
@@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
         spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
 
         for (j = 0; j < num_ss; ++j) {
-            DriveInfo *dinfo = drive_get_next(IF_MTD);
+            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
             flash_dev = qdev_new("n25q128");
             if (dinfo) {
                 qdev_prop_set_drive_err(flash_dev, "drive",
@@ -170,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
         }
     }
 
+    return unit;
 }
 
 static void zynq_init(MachineState *machine)
@@ -247,9 +249,9 @@ static void zynq_init(MachineState *machine)
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-    zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
-    zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET], false);
-    zynq_init_spi_flashes(0xE000D000, pic[51-IRQ_OFFSET], true);
+    n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
+    n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
+    n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
 
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
@@ -298,7 +300,7 @@ static void zynq_init(MachineState *machine)
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - IRQ_OFFSET]);
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, n);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         carddev = qdev_new(TYPE_SD_CARD);
         qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
-- 
2.31.1

Re: [PATCH v2 11/13] hw/arm/xilinx_zynq: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:07PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-zcu102" connects backends with drive_get_next() in two
> counting loops, one of them in a helper function.  Change it to use
> drive_get() directly.  This makes the unit numbers explicit in the
> code.


Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/xilinx_zynq.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 69c333e91b..50e7268396 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -125,9 +125,10 @@ static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>      sysbus_connect_irq(s, 0, irq);
>  }
>  
> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> -                                         bool is_qspi)
> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> +                                        bool is_qspi, int unit0)
>  {
> +    int unit = unit0;
>      DeviceState *dev;
>      SysBusDevice *busdev;
>      SSIBus *spi;
> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>          spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>  
>          for (j = 0; j < num_ss; ++j) {
> -            DriveInfo *dinfo = drive_get_next(IF_MTD);
> +            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
>              flash_dev = qdev_new("n25q128");
>              if (dinfo) {
>                  qdev_prop_set_drive_err(flash_dev, "drive",
> @@ -170,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>          }
>      }
>  
> +    return unit;
>  }
>  
>  static void zynq_init(MachineState *machine)
> @@ -247,9 +249,9 @@ static void zynq_init(MachineState *machine)
>          pic[n] = qdev_get_gpio_in(dev, n);
>      }
>  
> -    zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
> -    zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET], false);
> -    zynq_init_spi_flashes(0xE000D000, pic[51-IRQ_OFFSET], true);
> +    n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
> +    n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
> +    n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
>  
>      sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
>      sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
> @@ -298,7 +300,7 @@ static void zynq_init(MachineState *machine)
>          sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
>          sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - IRQ_OFFSET]);
>  
> -        di = drive_get_next(IF_SD);
> +        di = drive_get(IF_SD, 0, n);
>          blk = di ? blk_by_legacy_dinfo(di) : NULL;
>          carddev = qdev_new(TYPE_SD_CARD);
>          qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
> -- 
> 2.31.1
> 

[PATCH v2 12/13] hw/arm/aspeed: Replace drive_get_next() by drive_get()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

The aspeed machines connects backends with drive_get_next() in several
counting loops, one of them in a helper function, and a conditional.
Change it to use drive_get() directly.  This makes the unit numbers
explicit in the code.

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/aspeed.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a77f46b3ad..cf20ae0db5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -284,12 +284,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
 }
 
 static void aspeed_board_init_flashes(AspeedSMCState *s,
-                                      const char *flashtype)
+                                      const char *flashtype,
+                                      int unit0)
 {
     int i ;
 
     for (i = 0; i < s->num_cs; ++i) {
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
         qemu_irq cs_line;
         DeviceState *dev;
 
@@ -382,10 +383,12 @@ static void aspeed_machine_init(MachineState *machine)
                           "max_ram", max_ram_size  - machine->ram_size);
     memory_region_add_subregion(&bmc->ram_container, machine->ram_size, &bmc->max_ram);
 
-    aspeed_board_init_flashes(&bmc->soc.fmc, bmc->fmc_model ?
-                              bmc->fmc_model : amc->fmc_model);
-    aspeed_board_init_flashes(&bmc->soc.spi[0], bmc->spi_model ?
-                              bmc->spi_model : amc->spi_model);
+    aspeed_board_init_flashes(&bmc->soc.fmc,
+                              bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
+                              0);
+    aspeed_board_init_flashes(&bmc->soc.spi[0],
+                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
+                              bmc->soc.fmc.num_cs);
 
     /* Install first FMC flash content as a boot rom. */
     if (drive0) {
@@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
-        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
+        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
+                           drive_get(IF_SD, 0, i));
     }
 
     if (bmc->soc.emmc.num_slots) {
-        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
+        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
+                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
     }
 
     arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
-- 
2.31.1

Re: [PATCH v2 12/13] hw/arm/aspeed: Replace drive_get_next() by drive_get()

[Cédric Le Goater]

On 11/17/21 17:34, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> The aspeed machines connects backends with drive_get_next() in several
> counting loops, one of them in a helper function, and a conditional.
> Change it to use drive_get() directly.  This makes the unit numbers
> explicit in the code.

I hope we can introduce some bus id. At least for the SPI controllers.
  
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

>   hw/arm/aspeed.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index a77f46b3ad..cf20ae0db5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -284,12 +284,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
>   }
>   
>   static void aspeed_board_init_flashes(AspeedSMCState *s,
> -                                      const char *flashtype)
> +                                      const char *flashtype,
> +                                      int unit0)
>   {
>       int i ;
>   
>       for (i = 0; i < s->num_cs; ++i) {
> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
>           qemu_irq cs_line;
>           DeviceState *dev;
>   
> @@ -382,10 +383,12 @@ static void aspeed_machine_init(MachineState *machine)
>                             "max_ram", max_ram_size  - machine->ram_size);
>       memory_region_add_subregion(&bmc->ram_container, machine->ram_size, &bmc->max_ram);
>   
> -    aspeed_board_init_flashes(&bmc->soc.fmc, bmc->fmc_model ?
> -                              bmc->fmc_model : amc->fmc_model);
> -    aspeed_board_init_flashes(&bmc->soc.spi[0], bmc->spi_model ?
> -                              bmc->spi_model : amc->spi_model);
> +    aspeed_board_init_flashes(&bmc->soc.fmc,
> +                              bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
> +                              0);
> +    aspeed_board_init_flashes(&bmc->soc.spi[0],
> +                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
> +                              bmc->soc.fmc.num_cs);
>   
>       /* Install first FMC flash content as a boot rom. */
>       if (drive0) {
> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>       }
>   
>       for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
> +                           drive_get(IF_SD, 0, i));
>       }
>   
>       if (bmc->soc.emmc.num_slots) {
> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>       }
>   
>       arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
> 

[PATCH v2 13/13] blockdev: Drop unused drive_get_next()

[Markus Armbruster]

drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

The previous commits eliminated all uses.  Drop the function.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/sysemu/blockdev.h |  1 -
 blockdev.c                | 10 ----------
 2 files changed, 11 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 32c2d6023c..a750f99b79 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -50,7 +50,6 @@ void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 int drive_get_max_devs(BlockInterfaceType type);
-DriveInfo *drive_get_next(BlockInterfaceType type);
 
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
diff --git a/blockdev.c b/blockdev.c
index b35072644e..0eb2823b1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -303,16 +303,6 @@ int drive_get_max_bus(BlockInterfaceType type)
     return max_bus;
 }
 
-/* Get a block device.  This should only be used for single-drive devices
-   (e.g. SD/Floppy/MTD).  Multi-disk devices (scsi/ide) should use the
-   appropriate bus.  */
-DriveInfo *drive_get_next(BlockInterfaceType type)
-{
-    static int next_block_unit[IF_COUNT];
-
-    return drive_get(type, 0, next_block_unit[type]++);
-}
-
 static void bdrv_format_print(void *opaque, const char *name)
 {
     qemu_printf(" %s", name);
-- 
2.31.1

Re: [PATCH v2 13/13] blockdev: Drop unused drive_get_next()

[Hanna Reitz]

On 17.11.21 17:34, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> The previous commits eliminated all uses.  Drop the function.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/sysemu/blockdev.h |  1 -
>   blockdev.c                | 10 ----------
>   2 files changed, 11 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v2 00/13] Eliminate drive_get_next()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If
> the order changes, or new calls appear "in the middle", unit numbers
> change.  ABI break.  Hard to spot in review.  Replace its uses by
> drive_get(), then delete it.

Queued for 7.0.