|
| From: | Cédric Le Goater |
| Subject: | Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() |
| Date: | Tue, 16 Nov 2021 09:52:35 +0100 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 11/15/21 16:57, Markus Armbruster wrote:
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:On 11/15/21 13:55, 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. Explicit is better than implicit: use drive_get() directly. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- 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/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/sparc64/niagara.c | 2 +- 23 files changed, 49 insertions(+), 52 deletions(-)@@ -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 we put SD on bus #0, ...}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));... we'd want to put eMMC on bus #1Using separate buses for different kinds of devices would be neater, but it also would be an incompatible change. This patch keeps existing bus/unit numbers working. drive_get_next() can only use bus 0.
All Aspeed SoCs have 3 SPI busses, each with multiple CS, and also multiple sdhci controllers with multiple slots. How drives are defined for the aspeed machines can/should be improved. The machine init iterates on the command line drives, attaches the DriveInfo, in the order found, to a m25p80 device model first and then follows on with the SD devices. This is fragile clearly and a bus+id would be most welcome to identify the drive backend. May be this is a prereq for this patchset ? Thanks, C.
but I see having eMMC cards on a IF_SD bus as a bug, since these cards are soldered on the board.IF_SD is not a bus, it's an "block interface type", which is really just a user interface thing.--- 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);Can we have one interface refactor per patch (IF_SD, IF_PFLASH, IF_MTD...)?Peter asked for one patch per "board/SoC model". I'll do whatever helps reviewers.@@ -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);-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++);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 @@ -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);If this is bus #0, ...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);... I'd expect we use bus #1 here (different connector on the board).See above.int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS; gchar *bus_name = g_strdup_printf("qspi%d", bus);
| [Prev in Thread] | Current Thread | [Next in Thread] |