[PULL 00/23] SD/MMC patches for 2020-08-21

[PULL 00/23] SD/MMC patches for 2020-08-21

[Philippe Mathieu-Daudé]

The following changes since commit d6f83a72a7db94a3ede9f5cc4fb39f9c8e89f954:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/acceptance-testing=
-20200812' into staging (2020-08-21 14:51:43 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/sd-next-20200821

for you to fetch changes up to 6d2d4069c47e23b9e3913f9c8204fd0edcb99fb3:

  hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card (2020=
-08-21 16:49:22 +0200)

----------------------------------------------------------------
SD/MMC patches

- Convert legacy SD host controller to the SDBus API
- Move legacy API to a separate "sdcard_legacy.h" header
- Introduce methods to access multiple bytes on SDBus data lines
- Fix 'switch function' group location
- Fix SDSC maximum card size (2GB)

CI jobs result:
  https://gitlab.com/philmd/qemu/-/pipelines/180605963
----------------------------------------------------------------

Alistair Francis (1):
  hw/sd/pl181: Replace fprintf(stderr, "*\n") with error_report()

Bin Meng (2):
  hw/sd: Fix incorrect populated function switch status data structure
  hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card

Philippe Mathieu-Daud=C3=A9 (20):
  hw/sd/pxa2xx_mmci: Do not create SD card within the SD host controller
  hw/sd/pxa2xx_mmci: Trivial simplification
  hw/lm32/milkymist: Un-inline milkymist_memcard_create()
  hw/sd/milkymist: Create the SDBus at init()
  hw/sd/milkymist: Do not create SD card within the SD host controller
  hw/sd/pl181: Rename pl181_send_command() as pl181_do_command()
  hw/sd/pl181: Add TODO to use Fifo32 API
  hw/sd/pl181: Use named GPIOs
  hw/sd/pl181: Expose a SDBus and connect the SDCard to it
  hw/sd/pl181: Do not create SD card within the SD host controller
  hw/sd/pl181: Replace disabled fprintf()s by trace events
  hw/sd/sdcard: Make sd_data_ready() static
  hw/sd: Move sdcard legacy API to 'hw/sd/sdcard_legacy.h'
  hw/sd: Rename read/write_data() as read/write_byte()
  hw/sd: Rename sdbus_write_data() as sdbus_write_byte()
  hw/sd: Rename sdbus_read_data() as sdbus_read_byte()
  hw/sd: Add sdbus_write_data() to write multiples bytes on the data
    line
  hw/sd: Use sdbus_write_data() instead of sdbus_write_byte when
    possible
  hw/sd: Add sdbus_read_data() to read multiples bytes on the data line
  hw/sd: Use sdbus_read_data() instead of sdbus_read_byte() when
    possible

 hw/lm32/milkymist-hw.h        |  11 ----
 include/hw/arm/pxa.h          |   3 +-
 include/hw/sd/sd.h            |  73 +++++++++++++++-------
 include/hw/sd/sdcard_legacy.h |  50 +++++++++++++++
 hw/arm/integratorcp.c         |  17 +++++-
 hw/arm/pxa2xx.c               |  39 +++++++++---
 hw/arm/realview.c             |  16 ++++-
 hw/arm/versatilepb.c          |  26 +++++++-
 hw/arm/vexpress.c             |  15 ++++-
 hw/lm32/milkymist.c           |  24 ++++++++
 hw/sd/allwinner-sdhost.c      |  24 +++-----
 hw/sd/bcm2835_sdhost.c        |   4 +-
 hw/sd/core.c                  |  38 ++++++++++--
 hw/sd/milkymist-memcard.c     |  71 ++++++++++++----------
 hw/sd/omap_mmc.c              |  10 +--
 hw/sd/pl181.c                 | 111 +++++++++++++++++++---------------
 hw/sd/pxa2xx_mmci.c           |  19 ++----
 hw/sd/sd.c                    |  28 +++++----
 hw/sd/sdhci.c                 |  46 ++++----------
 hw/sd/ssi-sd.c                |   2 +-
 hw/sd/trace-events            |  10 +++
 21 files changed, 415 insertions(+), 222 deletions(-)
 create mode 100644 include/hw/sd/sdcard_legacy.h

--=20
2.26.2

[PULL 01/23] hw/sd/pxa2xx_mmci: Do not create SD card within the SD host controller

[Philippe Mathieu-Daudé]

SD/MMC host controllers provide a SD Bus to plug SD cards,
but don't come with SD card plugged in :)

The machine/board object is where the SD cards are created.
Since the PXA2xx is not qdevified, for now create the cards
in pxa270_init() which is the SoC model.
In the future we will move this to the board model.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705213350.24725-2-f4bug@amsat.org>
---
 include/hw/arm/pxa.h |  3 +--
 hw/arm/pxa2xx.c      | 39 +++++++++++++++++++++++++++++----------
 hw/sd/pxa2xx_mmci.c  | 11 ++---------
 3 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
index 8843e5f9107..d99b6192daf 100644
--- a/include/hw/arm/pxa.h
+++ b/include/hw/arm/pxa.h
@@ -89,8 +89,7 @@ void pxa2xx_lcd_vsync_notifier(PXA2xxLCDState *s, qemu_irq handler);
 typedef struct PXA2xxMMCIState PXA2xxMMCIState;
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 hwaddr base,
-                BlockBackend *blk, qemu_irq irq,
-                qemu_irq rx_dma, qemu_irq tx_dma);
+                qemu_irq irq, qemu_irq rx_dma, qemu_irq tx_dma);
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
                 qemu_irq coverswitch);
 
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 6203c4cfe0b..20fa201dd57 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -22,6 +22,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ssi/ssi.h"
+#include "hw/sd/sd.h"
 #include "chardev/char-fe.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/qtest.h"
@@ -2136,15 +2137,24 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
 
     s->gpio = pxa2xx_gpio_init(0x40e00000, s->cpu, s->pic, 121);
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo && !qtest_enabled()) {
-        warn_report("missing SecureDigital device");
-    }
     s->mmc = pxa2xx_mmci_init(address_space, 0x41100000,
-                    dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                     qdev_get_gpio_in(s->pic, PXA2XX_PIC_MMC),
                     qdev_get_gpio_in(s->dma, PXA2XX_RX_RQ_MMCI),
                     qdev_get_gpio_in(s->dma, PXA2XX_TX_RQ_MMCI));
+    dinfo = drive_get(IF_SD, 0, 0);
+    if (dinfo) {
+        DeviceState *carddev;
+
+        /* Create and plug in the sd card */
+        carddev = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(carddev, "drive",
+                                blk_by_legacy_dinfo(dinfo), &error_fatal);
+        qdev_realize_and_unref(carddev, qdev_get_child_bus(DEVICE(s->mmc),
+                                                           "sd-bus"),
+                               &error_fatal);
+    } else if (!qtest_enabled()) {
+        warn_report("missing SecureDigital device");
+    }
 
     for (i = 0; pxa270_serial[i].io_base; i++) {
         if (serial_hd(i)) {
@@ -2260,15 +2270,24 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
 
     s->gpio = pxa2xx_gpio_init(0x40e00000, s->cpu, s->pic, 85);
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo && !qtest_enabled()) {
-        warn_report("missing SecureDigital device");
-    }
     s->mmc = pxa2xx_mmci_init(address_space, 0x41100000,
-                    dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                     qdev_get_gpio_in(s->pic, PXA2XX_PIC_MMC),
                     qdev_get_gpio_in(s->dma, PXA2XX_RX_RQ_MMCI),
                     qdev_get_gpio_in(s->dma, PXA2XX_TX_RQ_MMCI));
+    dinfo = drive_get(IF_SD, 0, 0);
+    if (dinfo) {
+        DeviceState *carddev;
+
+        /* Create and plug in the sd card */
+        carddev = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(carddev, "drive",
+                                blk_by_legacy_dinfo(dinfo), &error_fatal);
+        qdev_realize_and_unref(carddev, qdev_get_child_bus(DEVICE(s->mmc),
+                                                           "sd-bus"),
+                               &error_fatal);
+    } else if (!qtest_enabled()) {
+        warn_report("missing SecureDigital device");
+    }
 
     for (i = 0; pxa255_serial[i].io_base; i++) {
         if (serial_hd(i)) {
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 68bed24480e..9482b9212dd 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -476,10 +476,9 @@ static const MemoryRegionOps pxa2xx_mmci_ops = {
 
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 hwaddr base,
-                BlockBackend *blk, qemu_irq irq,
-                qemu_irq rx_dma, qemu_irq tx_dma)
+                qemu_irq irq, qemu_irq rx_dma, qemu_irq tx_dma)
 {
-    DeviceState *dev, *carddev;
+    DeviceState *dev;
     SysBusDevice *sbd;
     PXA2xxMMCIState *s;
 
@@ -492,12 +491,6 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     qdev_connect_gpio_out_named(dev, "tx-dma", 0, tx_dma);
     sysbus_realize_and_unref(sbd, &error_fatal);
 
-    /* Create and plug in the sd card */
-    carddev = qdev_new(TYPE_SD_CARD);
-    qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
-    qdev_realize_and_unref(carddev, qdev_get_child_bus(dev, "sd-bus"),
-                           &error_fatal);
-
     return s;
 }
 
-- 
2.26.2

[PULL 02/23] hw/sd/pxa2xx_mmci: Trivial simplification

[Philippe Mathieu-Daudé]

Avoid declaring PXA2xxMMCIState local variable, return it directly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705213350.24725-3-f4bug@amsat.org>
---
 hw/sd/pxa2xx_mmci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 9482b9212dd..2996a2ef177 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -480,10 +480,8 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 {
     DeviceState *dev;
     SysBusDevice *sbd;
-    PXA2xxMMCIState *s;
 
     dev = qdev_new(TYPE_PXA2XX_MMCI);
-    s = PXA2XX_MMCI(dev);
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, base);
     sysbus_connect_irq(sbd, 0, irq);
@@ -491,7 +489,7 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     qdev_connect_gpio_out_named(dev, "tx-dma", 0, tx_dma);
     sysbus_realize_and_unref(sbd, &error_fatal);
 
-    return s;
+    return PXA2XX_MMCI(dev);
 }
 
 static void pxa2xx_mmci_set_inserted(DeviceState *dev, bool inserted)
-- 
2.26.2

[PULL 03/23] hw/lm32/milkymist: Un-inline milkymist_memcard_create()

[Philippe Mathieu-Daudé]

As we will modify milkymist_memcard_create(), move it first
to the source file where it is used.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200705211016.15241-2-f4bug@amsat.org>
---
 hw/lm32/milkymist-hw.h | 11 -----------
 hw/lm32/milkymist.c    | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
index 05e2c2a5a75..5dca5d52f57 100644
--- a/hw/lm32/milkymist-hw.h
+++ b/hw/lm32/milkymist-hw.h
@@ -31,17 +31,6 @@ static inline DeviceState *milkymist_hpdmc_create(hwaddr base)
     return dev;
 }
 
-static inline DeviceState *milkymist_memcard_create(hwaddr base)
-{
-    DeviceState *dev;
-
-    dev = qdev_new("milkymist-memcard");
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-
-    return dev;
-}
-
 static inline DeviceState *milkymist_vgafb_create(hwaddr base,
         uint32_t fb_offset, uint32_t fb_mask)
 {
diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 85913bb68b6..469e3c43225 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -80,6 +80,17 @@ static void main_cpu_reset(void *opaque)
     env->deba = reset_info->flash_base;
 }
 
+static DeviceState *milkymist_memcard_create(hwaddr base)
+{
+    DeviceState *dev;
+
+    dev = qdev_new("milkymist-memcard");
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+
+    return dev;
+}
+
 static void
 milkymist_init(MachineState *machine)
 {
-- 
2.26.2

[PULL 04/23] hw/sd/milkymist: Create the SDBus at init()

[Philippe Mathieu-Daudé]

We don't need to wait until realize() to create the SDBus,
create it in init() directly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200705211016.15241-4-f4bug@amsat.org>
---
 hw/sd/milkymist-memcard.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 11f61294fcf..747c5c6136b 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -261,6 +261,9 @@ static void milkymist_memcard_init(Object *obj)
     memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
             "milkymist-memcard", R_MAX * 4);
     sysbus_init_mmio(dev, &s->regs_region);
+
+    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
+                        DEVICE(obj), "sd-bus");
 }
 
 static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
@@ -271,9 +274,6 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
     DriveInfo *dinfo;
     Error *err = NULL;
 
-    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
-                        dev, "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);
-- 
2.26.2

[PULL 05/23] hw/sd/milkymist: Do not create SD card within the SD host controller

[Philippe Mathieu-Daudé]

SD/MMC host controllers provide a SD Bus to plug SD cards,
but don't come with SD card plugged in :) Let the machine/board
model create and plug the SD cards when required.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200705211016.15241-5-f4bug@amsat.org>
---
 hw/lm32/milkymist.c       | 13 +++++++++
 hw/sd/milkymist-memcard.c | 55 +++++++++++++++++++++++----------------
 2 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 469e3c43225..9f8fe9fef15 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -34,6 +34,7 @@
 #include "elf.h"
 #include "milkymist-hw.h"
 #include "hw/display/milkymist_tmu2.h"
+#include "hw/sd/sd.h"
 #include "lm32.h"
 #include "exec/address-spaces.h"
 #include "qemu/cutils.h"
@@ -83,11 +84,23 @@ static void main_cpu_reset(void *opaque)
 static DeviceState *milkymist_memcard_create(hwaddr base)
 {
     DeviceState *dev;
+    DriveInfo *dinfo;
 
     dev = qdev_new("milkymist-memcard");
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
 
+    dinfo = drive_get_next(IF_SD);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
+
     return dev;
 }
 
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 747c5c6136b..e9f5db5e22d 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -66,6 +66,8 @@ enum {
 #define MILKYMIST_MEMCARD(obj) \
     OBJECT_CHECK(MilkymistMemcardState, (obj), TYPE_MILKYMIST_MEMCARD)
 
+#define TYPE_MILKYMIST_SDBUS "milkymist-sdbus"
+
 struct MilkymistMemcardState {
     SysBusDevice parent_obj;
 
@@ -253,6 +255,19 @@ static void milkymist_memcard_reset(DeviceState *d)
     }
 }
 
+static void milkymist_memcard_set_readonly(DeviceState *dev, bool level)
+{
+    qemu_log_mask(LOG_UNIMP,
+                  "milkymist_memcard: read-only mode not supported\n");
+}
+
+static void milkymist_memcard_set_inserted(DeviceState *dev, bool level)
+{
+    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
+
+    s->enabled = !!level;
+}
+
 static void milkymist_memcard_init(Object *obj)
 {
     MilkymistMemcardState *s = MILKYMIST_MEMCARD(obj);
@@ -266,27 +281,6 @@ static void milkymist_memcard_init(Object *obj)
                         DEVICE(obj), "sd-bus");
 }
 
-static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
-{
-    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
-    DeviceState *carddev;
-    BlockBackend *blk;
-    DriveInfo *dinfo;
-    Error *err = NULL;
-
-    /* Create and plug in the sd card */
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    dinfo = drive_get_next(IF_SD);
-    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
-    carddev = qdev_new(TYPE_SD_CARD);
-    qdev_prop_set_drive(carddev, "drive", blk);
-    if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
-        error_propagate_prepend(errp, err, "failed to init SD card");
-        return;
-    }
-    s->enabled = blk && blk_is_inserted(blk);
-}
-
 static const VMStateDescription vmstate_milkymist_memcard = {
     .name = "milkymist-memcard",
     .version_id = 1,
@@ -308,10 +302,9 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->realize = milkymist_memcard_realize;
     dc->reset = milkymist_memcard_reset;
     dc->vmsd = &vmstate_milkymist_memcard;
-    /* Reason: init() method uses drive_get_next() */
+    /* Reason: output IRQs should be wired up */
     dc->user_creatable = false;
 }
 
@@ -323,9 +316,25 @@ static const TypeInfo milkymist_memcard_info = {
     .class_init    = milkymist_memcard_class_init,
 };
 
+static void milkymist_sdbus_class_init(ObjectClass *klass, void *data)
+{
+    SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+    sbc->set_inserted = milkymist_memcard_set_inserted;
+    sbc->set_readonly = milkymist_memcard_set_readonly;
+}
+
+static const TypeInfo milkymist_sdbus_info = {
+    .name = TYPE_MILKYMIST_SDBUS,
+    .parent = TYPE_SD_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_init = milkymist_sdbus_class_init,
+};
+
 static void milkymist_memcard_register_types(void)
 {
     type_register_static(&milkymist_memcard_info);
+    type_register_static(&milkymist_sdbus_info);
 }
 
 type_init(milkymist_memcard_register_types)
-- 
2.26.2

[PULL 06/23] hw/sd/pl181: Replace fprintf(stderr, "*\n") with error_report()

[Philippe Mathieu-Daudé]

From: Alistair Francis <alistair.francis@xilinx.com>

Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.

find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +

Some lines where then manually tweaked to pass checkpatch.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Message-Id: <488ba8d4c562ea44119de8ea0f385a898bd8fa1e.1513790495.git.alistair.francis@xilinx.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pl181.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 2b3776a6a0f..649386ec3d1 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -15,6 +15,7 @@
 #include "hw/sd/sd.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 
 //#define DEBUG_PL181 1
@@ -148,7 +149,7 @@ static void pl181_fifo_push(PL181State *s, uint32_t value)
     int n;
 
     if (s->fifo_len == PL181_FIFO_LEN) {
-        fprintf(stderr, "pl181: FIFO overflow\n");
+        error_report("%s: FIFO overflow", __func__);
         return;
     }
     n = (s->fifo_pos + s->fifo_len) & (PL181_FIFO_LEN - 1);
@@ -162,7 +163,7 @@ static uint32_t pl181_fifo_pop(PL181State *s)
     uint32_t value;
 
     if (s->fifo_len == 0) {
-        fprintf(stderr, "pl181: FIFO underflow\n");
+        error_report("%s: FIFO underflow", __func__);
         return 0;
     }
     value = s->fifo[s->fifo_pos];
-- 
2.26.2

[PULL 07/23] hw/sd/pl181: Rename pl181_send_command() as pl181_do_command()

[Philippe Mathieu-Daudé]

pl181_send_command() do a bus transaction (send or receive),
rename it as pl181_do_command().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-3-f4bug@amsat.org>
---
 hw/sd/pl181.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 649386ec3d1..3fc2cdd71a1 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -173,7 +173,7 @@ static uint32_t pl181_fifo_pop(PL181State *s)
     return value;
 }
 
-static void pl181_send_command(PL181State *s)
+static void pl181_do_command(PL181State *s)
 {
     SDRequest request;
     uint8_t response[16];
@@ -402,7 +402,7 @@ static void pl181_write(void *opaque, hwaddr offset,
                 qemu_log_mask(LOG_UNIMP,
                               "pl181: Pending commands not implemented\n");
             } else {
-                pl181_send_command(s);
+                pl181_do_command(s);
                 pl181_fifo_run(s);
             }
             /* The command has completed one way or the other.  */
-- 
2.26.2

[PULL 08/23] hw/sd/pl181: Add TODO to use Fifo32 API

[Philippe Mathieu-Daudé]

Add TODO to use Fifo32 API from "qemu/fifo32.h".

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-4-f4bug@amsat.org>
---
 hw/sd/pl181.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 3fc2cdd71a1..86219c851d3 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -57,7 +57,7 @@ typedef struct PL181State {
        http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4446/1
      */
     int32_t linux_hack;
-    uint32_t fifo[PL181_FIFO_LEN];
+    uint32_t fifo[PL181_FIFO_LEN]; /* TODO use Fifo32 */
     qemu_irq irq[2];
     /* GPIO outputs for 'card is readonly' and 'card inserted' */
     qemu_irq cardstatus[2];
-- 
2.26.2

[PULL 09/23] hw/sd/pl181: Use named GPIOs

[Philippe Mathieu-Daudé]

To make the code easier to manage/review/use, rename the
cardstatus[0] variable as 'card_readonly' and name the GPIO
"card-read-only".
Similarly with cardstatus[1], renamed as 'card_inserted' and
name its GPIO "card-inserted".

Adapt the users accordingly by using the qdev_init_gpio_out_named()
function.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-6-f4bug@amsat.org>
---
 hw/arm/integratorcp.c | 4 ++--
 hw/arm/realview.c     | 4 ++--
 hw/arm/vexpress.c     | 4 ++--
 hw/sd/pl181.c         | 8 +++++---
 4 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index f304c2b4f03..16c4d750a4f 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -645,9 +645,9 @@ static void integratorcp_init(MachineState *machine)
     sysbus_create_simple(TYPE_INTEGRATOR_DEBUG, 0x1a000000, 0);
 
     dev = sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
-    qdev_connect_gpio_out(dev, 0,
+    qdev_connect_gpio_out_named(dev, "card-read-only", 0,
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_WPROT, 0));
-    qdev_connect_gpio_out(dev, 1,
+    qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 0));
     sysbus_create_varargs("pl041", 0x1d000000, pic[25], NULL);
 
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index c1ff172b136..3e2360c261f 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -234,8 +234,8 @@ static void realview_init(MachineState *machine,
     mmc_irq[1] = qemu_irq_split(
         qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN),
         qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
-    qdev_connect_gpio_out(dev, 0, mmc_irq[0]);
-    qdev_connect_gpio_out(dev, 1, mmc_irq[1]);
+    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]);
 
     sysbus_create_simple("pl031", 0x10017000, pic[10]);
 
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 1dc971c34f2..049a0ec2c73 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -624,9 +624,9 @@ static void vexpress_common_init(MachineState *machine)
 
     dev = sysbus_create_varargs("pl181", map[VE_MMCI], pic[9], pic[10], NULL);
     /* Wire up MMC card detect and read-only signals */
-    qdev_connect_gpio_out(dev, 0,
+    qdev_connect_gpio_out_named(dev, "card-read-only", 0,
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
-    qdev_connect_gpio_out(dev, 1,
+    qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
 
     sysbus_create_simple("pl050_keyboard", map[VE_KMI0], pic[12]);
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 86219c851d3..ab4cd733a4d 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -60,7 +60,8 @@ typedef struct PL181State {
     uint32_t fifo[PL181_FIFO_LEN]; /* TODO use Fifo32 */
     qemu_irq irq[2];
     /* GPIO outputs for 'card is readonly' and 'card inserted' */
-    qemu_irq cardstatus[2];
+    qemu_irq card_readonly;
+    qemu_irq card_inserted;
 } PL181State;
 
 static const VMStateDescription vmstate_pl181 = {
@@ -479,7 +480,7 @@ static void pl181_reset(DeviceState *d)
     s->mask[1] = 0;
 
     /* We can assume our GPIO outputs have been wired up now */
-    sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
+    sd_set_cb(s->card, s->card_readonly, s->card_inserted);
     /* Since we're still using the legacy SD API the card is not plugged
      * into any bus, and we must reset it manually.
      */
@@ -496,7 +497,8 @@ static void pl181_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq[0]);
     sysbus_init_irq(sbd, &s->irq[1]);
-    qdev_init_gpio_out(dev, s->cardstatus, 2);
+    qdev_init_gpio_out_named(dev, &s->card_readonly, "card-read-only", 1);
+    qdev_init_gpio_out_named(dev, &s->card_inserted, "card-inserted", 1);
 }
 
 static void pl181_realize(DeviceState *dev, Error **errp)
-- 
2.26.2

[PULL 10/23] hw/sd/pl181: Expose a SDBus and connect the SDCard to it

[Philippe Mathieu-Daudé]

Convert the controller to the SDBus API:
- add the a TYPE_PL181_BUS object of type TYPE_SD_BUS,
- adapt the SDBusClass set_inserted/set_readonly handlers
- create the bus in the PL181 controller
- switch legacy sd_*() API to the sdbus_*() API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-7-f4bug@amsat.org>
---
 hw/sd/pl181.c | 67 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 16 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index ab4cd733a4d..f6de06ece82 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -17,6 +17,7 @@
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "hw/qdev-properties.h"
 
 //#define DEBUG_PL181 1
 
@@ -32,11 +33,13 @@ do { printf("pl181: " fmt , ## __VA_ARGS__); } while (0)
 #define TYPE_PL181 "pl181"
 #define PL181(obj) OBJECT_CHECK(PL181State, (obj), TYPE_PL181)
 
+#define TYPE_PL181_BUS "pl181-bus"
+
 typedef struct PL181State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    SDState *card;
+    SDBus sdbus;
     uint32_t clock;
     uint32_t power;
     uint32_t cmdarg;
@@ -183,7 +186,7 @@ static void pl181_do_command(PL181State *s)
     request.cmd = s->cmd & PL181_CMD_INDEX;
     request.arg = s->cmdarg;
     DPRINTF("Command %d %08x\n", request.cmd, request.arg);
-    rlen = sd_do_command(s->card, &request, response);
+    rlen = sdbus_do_command(&s->sdbus, &request, response);
     if (rlen < 0)
         goto error;
     if (s->cmd & PL181_CMD_RESPONSE) {
@@ -224,12 +227,12 @@ static void pl181_fifo_run(PL181State *s)
     int is_read;
 
     is_read = (s->datactrl & PL181_DATA_DIRECTION) != 0;
-    if (s->datacnt != 0 && (!is_read || sd_data_ready(s->card))
+    if (s->datacnt != 0 && (!is_read || sdbus_data_ready(&s->sdbus))
             && !s->linux_hack) {
         if (is_read) {
             n = 0;
             while (s->datacnt && s->fifo_len < PL181_FIFO_LEN) {
-                value |= (uint32_t)sd_read_data(s->card) << (n * 8);
+                value |= (uint32_t)sdbus_read_data(&s->sdbus) << (n * 8);
                 s->datacnt--;
                 n++;
                 if (n == 4) {
@@ -250,7 +253,7 @@ static void pl181_fifo_run(PL181State *s)
                 }
                 n--;
                 s->datacnt--;
-                sd_write_data(s->card, value & 0xff);
+                sdbus_write_data(&s->sdbus, value & 0xff);
                 value >>= 8;
             }
         }
@@ -456,6 +459,20 @@ static const MemoryRegionOps pl181_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void pl181_set_readonly(DeviceState *dev, bool level)
+{
+    PL181State *s = (PL181State *)dev;
+
+    qemu_set_irq(s->card_readonly, level);
+}
+
+static void pl181_set_inserted(DeviceState *dev, bool level)
+{
+    PL181State *s = (PL181State *)dev;
+
+    qemu_set_irq(s->card_inserted, level);
+}
+
 static void pl181_reset(DeviceState *d)
 {
     PL181State *s = PL181(d);
@@ -479,12 +496,9 @@ static void pl181_reset(DeviceState *d)
     s->mask[0] = 0;
     s->mask[1] = 0;
 
-    /* We can assume our GPIO outputs have been wired up now */
-    sd_set_cb(s->card, s->card_readonly, s->card_inserted);
-    /* Since we're still using the legacy SD API the card is not plugged
-     * into any bus, and we must reset it manually.
-     */
-    device_legacy_reset(DEVICE(s->card));
+    /* Reset other state based on current card insertion/readonly status */
+    pl181_set_inserted(DEVICE(s), sdbus_get_inserted(&s->sdbus));
+    pl181_set_readonly(DEVICE(s), sdbus_get_readonly(&s->sdbus));
 }
 
 static void pl181_init(Object *obj)
@@ -499,19 +513,24 @@ static void pl181_init(Object *obj)
     sysbus_init_irq(sbd, &s->irq[1]);
     qdev_init_gpio_out_named(dev, &s->card_readonly, "card-read-only", 1);
     qdev_init_gpio_out_named(dev, &s->card_inserted, "card-inserted", 1);
+
+    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
+                        TYPE_PL181_BUS, dev, "sd-bus");
 }
 
 static void pl181_realize(DeviceState *dev, Error **errp)
 {
-    PL181State *s = PL181(dev);
+    DeviceState *card;
     DriveInfo *dinfo;
 
     /* FIXME use a qdev drive property instead of drive_get_next() */
+    card = qdev_new(TYPE_SD_CARD);
     dinfo = drive_get_next(IF_SD);
-    s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
-    if (s->card == NULL) {
-        error_setg(errp, "sd_init failed");
-    }
+    qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                            &error_fatal);
+    qdev_realize_and_unref(card,
+                           qdev_get_child_bus(dev, "sd-bus"),
+                           &error_fatal);
 }
 
 static void pl181_class_init(ObjectClass *klass, void *data)
@@ -533,9 +552,25 @@ static const TypeInfo pl181_info = {
     .class_init    = pl181_class_init,
 };
 
+static void pl181_bus_class_init(ObjectClass *klass, void *data)
+{
+    SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+    sbc->set_inserted = pl181_set_inserted;
+    sbc->set_readonly = pl181_set_readonly;
+}
+
+static const TypeInfo pl181_bus_info = {
+    .name = TYPE_PL181_BUS,
+    .parent = TYPE_SD_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_init = pl181_bus_class_init,
+};
+
 static void pl181_register_types(void)
 {
     type_register_static(&pl181_info);
+    type_register_static(&pl181_bus_info);
 }
 
 type_init(pl181_register_types)
-- 
2.26.2

[PULL 11/23] hw/sd/pl181: Do not create SD card within the SD host controller

[Philippe Mathieu-Daudé]

SD/MMC host controllers provide a SD Bus to plug SD cards,
but don't come with SD card plugged in :) Let the machine/board
model create and plug the SD cards when required.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-8-f4bug@amsat.org>
---
 hw/arm/integratorcp.c | 13 +++++++++++++
 hw/arm/realview.c     | 12 ++++++++++++
 hw/arm/versatilepb.c  | 26 ++++++++++++++++++++++++--
 hw/arm/vexpress.c     | 11 +++++++++++
 hw/sd/pl181.c         | 19 +------------------
 5 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 16c4d750a4f..fe7c2b9d4b1 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -25,6 +25,7 @@
 #include "hw/char/pl011.h"
 #include "hw/hw.h"
 #include "hw/irq.h"
+#include "hw/sd/sd.h"
 
 #define TYPE_INTEGRATOR_CM "integrator_core"
 #define INTEGRATOR_CM(obj) \
@@ -595,6 +596,7 @@ static void integratorcp_init(MachineState *machine)
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     DeviceState *dev, *sic, *icp;
+    DriveInfo *dinfo;
     int i;
 
     cpuobj = object_new(machine->cpu_type);
@@ -649,6 +651,17 @@ 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);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
+
     sysbus_create_varargs("pl041", 0x1d000000, pic[25], NULL);
 
     if (nd_table[0].used)
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 3e2360c261f..5f1f36b15cd 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -27,6 +27,7 @@
 #include "hw/intc/realview_gic.h"
 #include "hw/irq.h"
 #include "hw/i2c/arm_sbcon_i2c.h"
+#include "hw/sd/sd.h"
 
 #define SMP_BOOT_ADDR 0xe0000000
 #define SMP_BOOTREG_ADDR 0x10000030
@@ -69,6 +70,7 @@ static void realview_init(MachineState *machine,
     qemu_irq mmc_irq[2];
     PCIBus *pci_bus = NULL;
     NICInfo *nd;
+    DriveInfo *dinfo;
     I2CBus *i2c;
     int n;
     unsigned int smp_cpus = machine->smp.cpus;
@@ -236,6 +238,16 @@ 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);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
 
     sysbus_create_simple("pl031", 0x10017000, pic[10]);
 
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 9dc93182b6b..9127579984f 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -25,6 +25,7 @@
 #include "hw/block/flash.h"
 #include "qemu/error-report.h"
 #include "hw/char/pl011.h"
+#include "hw/sd/sd.h"
 
 #define VERSATILE_FLASH_ADDR 0x34000000
 #define VERSATILE_FLASH_SIZE (64 * 1024 * 1024)
@@ -309,8 +310,29 @@ static void versatile_init(MachineState *machine, int board_id)
     /* Wire up the mux control signals from the SYS_CLCD register */
     qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
 
-    sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
-    sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
+    dev = sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
+    dinfo = drive_get_next(IF_SD);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
+
+    dev = sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
+    dinfo = drive_get_next(IF_SD);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
 
     /* Add PL031 Real Time Clock. */
     sysbus_create_simple("pl031", 0x101e8000, pic[10]);
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 049a0ec2c73..95405f59408 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -43,6 +43,7 @@
 #include "hw/cpu/a9mpcore.h"
 #include "hw/cpu/a15mpcore.h"
 #include "hw/i2c/arm_sbcon_i2c.h"
+#include "hw/sd/sd.h"
 
 #define VEXPRESS_BOARD_ID 0x8e0
 #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
@@ -628,6 +629,16 @@ 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);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
 
     sysbus_create_simple("pl050_keyboard", map[VE_KMI0], pic[12]);
     sysbus_create_simple("pl050_mouse", map[VE_KMI1], pic[13]);
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index f6de06ece82..f69488ebac3 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -17,7 +17,6 @@
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
-#include "hw/qdev-properties.h"
 
 //#define DEBUG_PL181 1
 
@@ -518,30 +517,14 @@ static void pl181_init(Object *obj)
                         TYPE_PL181_BUS, dev, "sd-bus");
 }
 
-static void pl181_realize(DeviceState *dev, Error **errp)
-{
-    DeviceState *card;
-    DriveInfo *dinfo;
-
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    card = qdev_new(TYPE_SD_CARD);
-    dinfo = drive_get_next(IF_SD);
-    qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
-                            &error_fatal);
-    qdev_realize_and_unref(card,
-                           qdev_get_child_bus(dev, "sd-bus"),
-                           &error_fatal);
-}
-
 static void pl181_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
 
     k->vmsd = &vmstate_pl181;
     k->reset = pl181_reset;
-    /* Reason: init() method uses drive_get_next() */
+    /* Reason: output IRQs should be wired up */
     k->user_creatable = false;
-    k->realize = pl181_realize;
 }
 
 static const TypeInfo pl181_info = {
-- 
2.26.2

[PULL 12/23] hw/sd/pl181: Replace disabled fprintf()s by trace events

[Philippe Mathieu-Daudé]

Convert disabled DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200705204630.4133-9-f4bug@amsat.org>
---
 hw/sd/pl181.c      | 26 +++++++++-----------------
 hw/sd/trace-events | 10 ++++++++++
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index f69488ebac3..574500ce600 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -17,15 +17,7 @@
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
-
-//#define DEBUG_PL181 1
-
-#ifdef DEBUG_PL181
-#define DPRINTF(fmt, ...) \
-do { printf("pl181: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while(0)
-#endif
+#include "trace.h"
 
 #define PL181_FIFO_LEN 16
 
@@ -158,7 +150,7 @@ static void pl181_fifo_push(PL181State *s, uint32_t value)
     n = (s->fifo_pos + s->fifo_len) & (PL181_FIFO_LEN - 1);
     s->fifo_len++;
     s->fifo[n] = value;
-    DPRINTF("FIFO push %08x\n", (int)value);
+    trace_pl181_fifo_push(value);
 }
 
 static uint32_t pl181_fifo_pop(PL181State *s)
@@ -172,7 +164,7 @@ static uint32_t pl181_fifo_pop(PL181State *s)
     value = s->fifo[s->fifo_pos];
     s->fifo_len--;
     s->fifo_pos = (s->fifo_pos + 1) & (PL181_FIFO_LEN - 1);
-    DPRINTF("FIFO pop %08x\n", (int)value);
+    trace_pl181_fifo_pop(value);
     return value;
 }
 
@@ -184,7 +176,7 @@ static void pl181_do_command(PL181State *s)
 
     request.cmd = s->cmd & PL181_CMD_INDEX;
     request.arg = s->cmdarg;
-    DPRINTF("Command %d %08x\n", request.cmd, request.arg);
+    trace_pl181_command_send(request.cmd, request.arg);
     rlen = sdbus_do_command(&s->sdbus, &request, response);
     if (rlen < 0)
         goto error;
@@ -201,16 +193,16 @@ static void pl181_do_command(PL181State *s)
             s->response[2] = ldl_be_p(&response[8]);
             s->response[3] = ldl_be_p(&response[12]) & ~1;
         }
-        DPRINTF("Response received\n");
+        trace_pl181_command_response_pending();
         s->status |= PL181_STATUS_CMDRESPEND;
     } else {
-        DPRINTF("Command sent\n");
+        trace_pl181_command_sent();
         s->status |= PL181_STATUS_CMDSENT;
     }
     return;
 
 error:
-    DPRINTF("Timeout\n");
+    trace_pl181_command_timeout();
     s->status |= PL181_STATUS_CMDTIMEOUT;
 }
 
@@ -262,11 +254,11 @@ static void pl181_fifo_run(PL181State *s)
         s->status |= PL181_STATUS_DATAEND;
         /* HACK: */
         s->status |= PL181_STATUS_DATABLOCKEND;
-        DPRINTF("Transfer Complete\n");
+        trace_pl181_fifo_transfer_complete();
     }
     if (s->datacnt == 0 && s->fifo_len == 0) {
         s->datactrl &= ~PL181_DATA_ENABLE;
-        DPRINTF("Data engine idle\n");
+        trace_pl181_data_engine_idle();
     } else {
         /* Update FIFO bits.  */
         bits = PL181_STATUS_TXACTIVE | PL181_STATUS_RXACTIVE;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 5f09d32eb2c..a87d7355fb8 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -62,3 +62,13 @@ milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value
 # pxa2xx_mmci.c
 pxa2xx_mmci_read(uint8_t size, uint32_t addr, uint32_t value) "size %d addr 0x%02x value 0x%08x"
 pxa2xx_mmci_write(uint8_t size, uint32_t addr, uint32_t value) "size %d addr 0x%02x value 0x%08x"
+
+# pl181.c
+pl181_command_send(uint8_t cmd, uint32_t arg) "sending CMD%02d arg 0x%08" PRIx32
+pl181_command_sent(void) "command sent"
+pl181_command_response_pending(void) "response received"
+pl181_command_timeout(void) "command timeouted"
+pl181_fifo_push(uint32_t data) "FIFO push 0x%08" PRIx32
+pl181_fifo_pop(uint32_t data) "FIFO pop 0x%08" PRIx32
+pl181_fifo_transfer_complete(void) "FIFO transfer complete"
+pl181_data_engine_idle(void) "data engine idle"
-- 
2.26.2

[PULL 13/23] hw/sd/sdcard: Make sd_data_ready() static

[Philippe Mathieu-Daudé]

sd_data_ready() belongs to the legacy API. As its last user has
been converted to the SDBus API, make it static.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Message-Id: <20180216022933.10945-7-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sd/sd.h | 1 -
 hw/sd/sd.c         | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index a84b8e274a3..ace350e0e83 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -143,7 +143,6 @@ int sd_do_command(SDState *sd, SDRequest *req,
 void sd_write_data(SDState *sd, uint8_t value);
 uint8_t sd_read_data(SDState *sd);
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
-bool sd_data_ready(SDState *sd);
 /* sd_enable should not be used -- it is only used on the nseries boards,
  * where it is part of a broken implementation of the MMC card slot switch
  * (there should be two card slots which are multiplexed to a single MMC
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index fad9cf1ee7a..a5ae5dccbe5 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2082,7 +2082,7 @@ uint8_t sd_read_data(SDState *sd)
     return ret;
 }
 
-bool sd_data_ready(SDState *sd)
+static bool sd_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
 }
-- 
2.26.2

[PULL 14/23] hw/sd: Move sdcard legacy API to 'hw/sd/sdcard_legacy.h'

[Philippe Mathieu-Daudé]

omap_mmc.c is the last device left using the legacy sdcard API.
Move the prototype declarations into a separate header, to
make it clear this is a legacy API.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Message-Id: <20180216022933.10945-8-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sd/sd.h            | 16 -----------
 include/hw/sd/sdcard_legacy.h | 50 +++++++++++++++++++++++++++++++++++
 hw/sd/omap_mmc.c              |  2 +-
 hw/sd/sd.c                    |  1 +
 4 files changed, 52 insertions(+), 17 deletions(-)
 create mode 100644 include/hw/sd/sdcard_legacy.h

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index ace350e0e83..8767ab817c1 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -136,22 +136,6 @@ typedef struct {
     void (*set_readonly)(DeviceState *dev, bool readonly);
 } SDBusClass;
 
-/* Legacy functions to be used only by non-qdevified callers */
-SDState *sd_init(BlockBackend *bs, bool is_spi);
-int sd_do_command(SDState *sd, SDRequest *req,
-                  uint8_t *response);
-void sd_write_data(SDState *sd, uint8_t value);
-uint8_t sd_read_data(SDState *sd);
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
-/* sd_enable should not be used -- it is only used on the nseries boards,
- * where it is part of a broken implementation of the MMC card slot switch
- * (there should be two card slots which are multiplexed to a single MMC
- * controller, but instead we model it with one card and controller and
- * disable the card when the second slot is selected, so it looks like the
- * second slot is always empty).
- */
-void sd_enable(SDState *sd, bool enable);
-
 /* Functions to be used by qdevified callers (working via
  * an SDBus rather than directly with SDState)
  */
diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
new file mode 100644
index 00000000000..8681f8089ba
--- /dev/null
+++ b/include/hw/sd/sdcard_legacy.h
@@ -0,0 +1,50 @@
+/*
+ * SD Memory Card emulation (deprecated legacy API)
+ *
+ * Copyright (c) 2006 Andrzej Zaborowski  <balrog@zabor.org>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#ifndef HW_SDCARD_LEGACY_H
+#define HW_SDCARD_LEGACY_H
+
+#include "hw/sd/sd.h"
+
+/* Legacy functions to be used only by non-qdevified callers */
+SDState *sd_init(BlockBackend *blk, bool is_spi);
+int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
+void sd_write_data(SDState *card, uint8_t value);
+uint8_t sd_read_data(SDState *card);
+void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
+
+/* sd_enable should not be used -- it is only used on the nseries boards,
+ * where it is part of a broken implementation of the MMC card slot switch
+ * (there should be two card slots which are multiplexed to a single MMC
+ * controller, but instead we model it with one card and controller and
+ * disable the card when the second slot is selected, so it looks like the
+ * second slot is always empty).
+ */
+void sd_enable(SDState *card, bool enable);
+
+#endif /* HW_SDCARD_LEGACY_H */
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 4088a8a80bc..7d33c59226a 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -23,7 +23,7 @@
 #include "qemu/log.h"
 #include "hw/irq.h"
 #include "hw/arm/omap.h"
-#include "hw/sd/sd.h"
+#include "hw/sd/sdcard_legacy.h"
 
 struct omap_mmc_s {
     qemu_irq irq;
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a5ae5dccbe5..5c6f5c94f3d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -37,6 +37,7 @@
 #include "hw/registerfields.h"
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
+#include "hw/sd/sdcard_legacy.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/bitmap.h"
-- 
2.26.2

[PULL 15/23] hw/sd: Rename read/write_data() as read/write_byte()

[Philippe Mathieu-Daudé]

The read/write_data() methods write do a single byte access
on the data line of a SD card. Rename them as read/write_byte().
Add some documentation (not in "hw/sd/sdcard_legacy.h" which we
are going to remove soon).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-2-f4bug@amsat.org>
---
 include/hw/sd/sd.h            | 19 +++++++++++++++++--
 include/hw/sd/sdcard_legacy.h |  4 ++--
 hw/sd/core.c                  |  4 ++--
 hw/sd/omap_mmc.c              |  8 ++++----
 hw/sd/sd.c                    | 16 ++++++++--------
 5 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 8767ab817c1..b58b5a19afe 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -104,8 +104,23 @@ typedef struct {
     /*< public >*/
 
     int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
-    void (*write_data)(SDState *sd, uint8_t value);
-    uint8_t (*read_data)(SDState *sd);
+    /**
+     * Write a byte to a SD card.
+     * @sd: card
+     * @value: byte to write
+     *
+     * Write a byte on the data lines of a SD card.
+     */
+    void (*write_byte)(SDState *sd, uint8_t value);
+    /**
+     * Read a byte from a SD card.
+     * @sd: card
+     *
+     * Read a byte from the data lines of a SD card.
+     *
+     * Return: byte value read
+     */
+    uint8_t (*read_byte)(SDState *sd);
     bool (*data_ready)(SDState *sd);
     void (*set_voltage)(SDState *sd, uint16_t millivolts);
     uint8_t (*get_dat_lines)(SDState *sd);
diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
index 8681f8089ba..0dc38895551 100644
--- a/include/hw/sd/sdcard_legacy.h
+++ b/include/hw/sd/sdcard_legacy.h
@@ -34,8 +34,8 @@
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *blk, bool is_spi);
 int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
-void sd_write_data(SDState *card, uint8_t value);
-uint8_t sd_read_data(SDState *card);
+void sd_write_byte(SDState *card, uint8_t value);
+uint8_t sd_read_byte(SDState *card);
 void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
 
 /* sd_enable should not be used -- it is only used on the nseries boards,
diff --git a/hw/sd/core.c b/hw/sd/core.c
index abec48bccb8..79d96576ead 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -110,7 +110,7 @@ void sdbus_write_data(SDBus *sdbus, uint8_t value)
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
-        sc->write_data(card, value);
+        sc->write_byte(card, value);
     }
 }
 
@@ -122,7 +122,7 @@ uint8_t sdbus_read_data(SDBus *sdbus)
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
-        value = sc->read_data(card);
+        value = sc->read_byte(card);
     }
     trace_sdbus_read(sdbus_name(sdbus), value);
 
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 7d33c59226a..1f946908fe1 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -232,10 +232,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
             if (host->fifo_len > host->af_level)
                 break;
 
-            value = sd_read_data(host->card);
+            value = sd_read_byte(host->card);
             host->fifo[(host->fifo_start + host->fifo_len) & 31] = value;
             if (-- host->blen_counter) {
-                value = sd_read_data(host->card);
+                value = sd_read_byte(host->card);
                 host->fifo[(host->fifo_start + host->fifo_len) & 31] |=
                         value << 8;
                 host->blen_counter --;
@@ -247,10 +247,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
                 break;
 
             value = host->fifo[host->fifo_start] & 0xff;
-            sd_write_data(host->card, value);
+            sd_write_byte(host->card, value);
             if (-- host->blen_counter) {
                 value = host->fifo[host->fifo_start] >> 8;
-                sd_write_data(host->card, value);
+                sd_write_byte(host->card, value);
                 host->blen_counter --;
             }
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5c6f5c94f3d..7c9d956f113 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1809,7 +1809,7 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
 #define APP_WRITE_BLOCK(a, len)
 
-void sd_write_data(SDState *sd, uint8_t value)
+void sd_write_byte(SDState *sd, uint8_t value)
 {
     int i;
 
@@ -1818,7 +1818,7 @@ void sd_write_data(SDState *sd, uint8_t value)
 
     if (sd->state != sd_receivingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "sd_write_data: not in Receiving-Data state\n");
+                      "%s: not in Receiving-Data state\n", __func__);
         return;
     }
 
@@ -1940,7 +1940,7 @@ void sd_write_data(SDState *sd, uint8_t value)
         break;
 
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "sd_write_data: unknown command\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unknown command\n", __func__);
         break;
     }
 }
@@ -1959,7 +1959,7 @@ static const uint8_t sd_tuning_block_pattern[SD_TUNING_BLOCK_SIZE] = {
     0xbb, 0xff, 0xf7, 0xff,         0xf7, 0x7f, 0x7b, 0xde,
 };
 
-uint8_t sd_read_data(SDState *sd)
+uint8_t sd_read_byte(SDState *sd)
 {
     /* TODO: Append CRCs */
     uint8_t ret;
@@ -1970,7 +1970,7 @@ uint8_t sd_read_data(SDState *sd)
 
     if (sd->state != sd_sendingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "sd_read_data: not in Sending-Data state\n");
+                      "%s: not in Sending-Data state\n", __func__);
         return 0x00;
     }
 
@@ -2076,7 +2076,7 @@ uint8_t sd_read_data(SDState *sd)
         break;
 
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "sd_read_data: unknown command\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unknown command\n", __func__);
         return 0x00;
     }
 
@@ -2192,8 +2192,8 @@ static void sd_class_init(ObjectClass *klass, void *data)
     sc->get_dat_lines = sd_get_dat_lines;
     sc->get_cmd_line = sd_get_cmd_line;
     sc->do_command = sd_do_command;
-    sc->write_data = sd_write_data;
-    sc->read_data = sd_read_data;
+    sc->write_byte = sd_write_byte;
+    sc->read_byte = sd_read_byte;
     sc->data_ready = sd_data_ready;
     sc->enable = sd_enable;
     sc->get_inserted = sd_get_inserted;
-- 
2.26.2

[PULL 16/23] hw/sd: Rename sdbus_write_data() as sdbus_write_byte()

[Philippe Mathieu-Daudé]

The sdbus_write_data() method do a single byte access on the data
line of a SD bus. Rename it as sdbus_write_byte() and document it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-3-f4bug@amsat.org>
---
 include/hw/sd/sd.h        |  9 ++++++++-
 hw/sd/allwinner-sdhost.c  | 10 +++++-----
 hw/sd/bcm2835_sdhost.c    |  2 +-
 hw/sd/core.c              |  2 +-
 hw/sd/milkymist-memcard.c |  8 ++++----
 hw/sd/pl181.c             |  2 +-
 hw/sd/pxa2xx_mmci.c       |  2 +-
 hw/sd/sdhci.c             |  8 ++++----
 8 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index b58b5a19afe..1e5ac955d05 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -158,7 +158,14 @@ void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts);
 uint8_t sdbus_get_dat_lines(SDBus *sdbus);
 bool sdbus_get_cmd_line(SDBus *sdbus);
 int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
-void sdbus_write_data(SDBus *sd, uint8_t value);
+/**
+ * Write a byte to a SD bus.
+ * @sd: bus
+ * @value: byte to write
+ *
+ * Write a byte on the data lines of a SD bus.
+ */
+void sdbus_write_byte(SDBus *sd, uint8_t value);
 uint8_t sdbus_read_data(SDBus *sd);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index f404e1fdb45..e05e8a3864c 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -335,7 +335,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
                                       buf, buf_bytes);
 
             for (uint32_t i = 0; i < buf_bytes; i++) {
-                sdbus_write_data(&s->sdbus, buf[i]);
+                sdbus_write_byte(&s->sdbus, buf[i]);
             }
 
         /* Read from SD bus */
@@ -654,10 +654,10 @@ static void allwinner_sdhost_write(void *opaque, hwaddr offset,
         s->startbit_detect = value;
         break;
     case REG_SD_FIFO:      /* Read/Write FIFO */
-        sdbus_write_data(&s->sdbus, value & 0xff);
-        sdbus_write_data(&s->sdbus, (value >> 8) & 0xff);
-        sdbus_write_data(&s->sdbus, (value >> 16) & 0xff);
-        sdbus_write_data(&s->sdbus, (value >> 24) & 0xff);
+        sdbus_write_byte(&s->sdbus, value & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 8) & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 16) & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 24) & 0xff);
         allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
         allwinner_sdhost_auto_stop(s);
         allwinner_sdhost_update_irq(s);
diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index 4a80fbcc861..16aba7cc92b 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -223,7 +223,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
                 }
                 n--;
                 s->datacnt--;
-                sdbus_write_data(&s->sdbus, value & 0xff);
+                sdbus_write_byte(&s->sdbus, value & 0xff);
                 value >>= 8;
             }
         }
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 79d96576ead..13b5ca03169 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -102,7 +102,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
     return 0;
 }
 
-void sdbus_write_data(SDBus *sdbus, uint8_t value)
+void sdbus_write_byte(SDBus *sdbus, uint8_t value)
 {
     SDState *card = get_card(sdbus);
 
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index e9f5db5e22d..4128109c047 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -209,10 +209,10 @@ static void memcard_write(void *opaque, hwaddr addr, uint64_t value,
         if (!s->enabled) {
             break;
         }
-        sdbus_write_data(&s->sdbus, (value >> 24) & 0xff);
-        sdbus_write_data(&s->sdbus, (value >> 16) & 0xff);
-        sdbus_write_data(&s->sdbus, (value >> 8) & 0xff);
-        sdbus_write_data(&s->sdbus, value & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 24) & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 16) & 0xff);
+        sdbus_write_byte(&s->sdbus, (value >> 8) & 0xff);
+        sdbus_write_byte(&s->sdbus, value & 0xff);
         break;
     case R_ENABLE:
         s->regs[addr] = value;
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 574500ce600..771bae193f5 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -244,7 +244,7 @@ static void pl181_fifo_run(PL181State *s)
                 }
                 n--;
                 s->datacnt--;
-                sdbus_write_data(&s->sdbus, value & 0xff);
+                sdbus_write_byte(&s->sdbus, value & 0xff);
                 value >>= 8;
             }
         }
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 2996a2ef177..07ddc2eba3e 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -184,7 +184,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 
     if (s->cmdat & CMDAT_WR_RD) {
         while (s->bytesleft && s->tx_len) {
-            sdbus_write_data(&s->sdbus, s->tx_fifo[s->tx_start++]);
+            sdbus_write_byte(&s->sdbus, s->tx_fifo[s->tx_start++]);
             s->tx_start &= 0x1f;
             s->tx_len --;
             s->bytesleft --;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index deac1818650..4bf1ee88b2a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -515,7 +515,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
     }
 
     for (index = 0; index < (s->blksize & BLOCK_SIZE_MASK); index++) {
-        sdbus_write_data(&s->sdbus, s->fifo_buffer[index]);
+        sdbus_write_byte(&s->sdbus, s->fifo_buffer[index]);
     }
 
     /* Next data can be written through BUFFER DATORT register */
@@ -642,7 +642,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
                 for (n = 0; n < block_size; n++) {
-                    sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
+                    sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
                 }
                 s->data_count = 0;
                 if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
@@ -679,7 +679,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
     } else {
         dma_memory_read(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
         for (n = 0; n < datacnt; n++) {
-            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
+            sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
         }
     }
     s->blkcnt--;
@@ -815,7 +815,7 @@ static void sdhci_do_adma(SDHCIState *s)
                     dscr.addr += s->data_count - begin;
                     if (s->data_count == block_size) {
                         for (n = 0; n < block_size; n++) {
-                            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
+                            sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
                         }
                         s->data_count = 0;
                         if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
-- 
2.26.2

[PULL 17/23] hw/sd: Rename sdbus_read_data() as sdbus_read_byte()

[Philippe Mathieu-Daudé]

The sdbus_read_data() method do a single byte access on the data
line of a SD bus. Rename it as sdbus_read_byte() and document it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-4-f4bug@amsat.org>
---
 include/hw/sd/sd.h        | 10 +++++++++-
 hw/sd/allwinner-sdhost.c  | 10 +++++-----
 hw/sd/bcm2835_sdhost.c    |  2 +-
 hw/sd/core.c              |  2 +-
 hw/sd/milkymist-memcard.c |  8 ++++----
 hw/sd/pl181.c             |  2 +-
 hw/sd/pxa2xx_mmci.c       |  2 +-
 hw/sd/sdhci.c             |  8 ++++----
 hw/sd/ssi-sd.c            |  2 +-
 9 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 1e5ac955d05..14ffc7f4758 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -166,7 +166,15 @@ int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
  * Write a byte on the data lines of a SD bus.
  */
 void sdbus_write_byte(SDBus *sd, uint8_t value);
-uint8_t sdbus_read_data(SDBus *sd);
+/**
+ * Read a byte from a SD bus.
+ * @sd: bus
+ *
+ * Read a byte from the data lines of a SD bus.
+ *
+ * Return: byte value read
+ */
+uint8_t sdbus_read_byte(SDBus *sd);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
 bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index e05e8a3864c..c004aa39da6 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -341,7 +341,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
         /* Read from SD bus */
         } else {
             for (uint32_t i = 0; i < buf_bytes; i++) {
-                buf[i] = sdbus_read_data(&s->sdbus);
+                buf[i] = sdbus_read_byte(&s->sdbus);
             }
             cpu_physical_memory_write((desc->addr & DESC_SIZE_MASK) + num_done,
                                        buf, buf_bytes);
@@ -521,10 +521,10 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
         break;
     case REG_SD_FIFO:      /* Read/Write FIFO */
         if (sdbus_data_ready(&s->sdbus)) {
-            res = sdbus_read_data(&s->sdbus);
-            res |= sdbus_read_data(&s->sdbus) << 8;
-            res |= sdbus_read_data(&s->sdbus) << 16;
-            res |= sdbus_read_data(&s->sdbus) << 24;
+            res = sdbus_read_byte(&s->sdbus);
+            res |= sdbus_read_byte(&s->sdbus) << 8;
+            res |= sdbus_read_byte(&s->sdbus) << 16;
+            res |= sdbus_read_byte(&s->sdbus) << 24;
             allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
             allwinner_sdhost_auto_stop(s);
             allwinner_sdhost_update_irq(s);
diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index 16aba7cc92b..2c7a675a2d8 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -190,7 +190,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
         if (is_read) {
             n = 0;
             while (s->datacnt && s->fifo_len < BCM2835_SDHOST_FIFO_LEN) {
-                value |= (uint32_t)sdbus_read_data(&s->sdbus) << (n * 8);
+                value |= (uint32_t)sdbus_read_byte(&s->sdbus) << (n * 8);
                 s->datacnt--;
                 n++;
                 if (n == 4) {
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 13b5ca03169..a3b620b802b 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -114,7 +114,7 @@ void sdbus_write_byte(SDBus *sdbus, uint8_t value)
     }
 }
 
-uint8_t sdbus_read_data(SDBus *sdbus)
+uint8_t sdbus_read_byte(SDBus *sdbus)
 {
     SDState *card = get_card(sdbus);
     uint8_t value = 0;
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 4128109c047..e8d055bb895 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -152,10 +152,10 @@ static uint64_t memcard_read(void *opaque, hwaddr addr,
             r = 0xffffffff;
         } else {
             r = 0;
-            r |= sdbus_read_data(&s->sdbus) << 24;
-            r |= sdbus_read_data(&s->sdbus) << 16;
-            r |= sdbus_read_data(&s->sdbus) << 8;
-            r |= sdbus_read_data(&s->sdbus);
+            r |= sdbus_read_byte(&s->sdbus) << 24;
+            r |= sdbus_read_byte(&s->sdbus) << 16;
+            r |= sdbus_read_byte(&s->sdbus) << 8;
+            r |= sdbus_read_byte(&s->sdbus);
         }
         break;
     case R_CLK2XDIV:
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 771bae193f5..579d68ad83e 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -223,7 +223,7 @@ static void pl181_fifo_run(PL181State *s)
         if (is_read) {
             n = 0;
             while (s->datacnt && s->fifo_len < PL181_FIFO_LEN) {
-                value |= (uint32_t)sdbus_read_data(&s->sdbus) << (n * 8);
+                value |= (uint32_t)sdbus_read_byte(&s->sdbus) << (n * 8);
                 s->datacnt--;
                 n++;
                 if (n == 4) {
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 07ddc2eba3e..04f0a98f813 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -194,7 +194,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
     } else
         while (s->bytesleft && s->rx_len < 32) {
             s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
-                sdbus_read_data(&s->sdbus);
+                sdbus_read_byte(&s->sdbus);
             s->bytesleft --;
             s->intreq |= INT_RXFIFO_REQ;
         }
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 4bf1ee88b2a..b897b1121b8 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -409,7 +409,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
     }
 
     for (index = 0; index < blk_size; index++) {
-        data = sdbus_read_data(&s->sdbus);
+        data = sdbus_read_byte(&s->sdbus);
         if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
             /* Device is not in tuning */
             s->fifo_buffer[index] = data;
@@ -601,7 +601,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         while (s->blkcnt) {
             if (s->data_count == 0) {
                 for (n = 0; n < block_size; n++) {
-                    s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
+                    s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
                 }
             }
             begin = s->data_count;
@@ -673,7 +673,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 
     if (s->trnmod & SDHC_TRNS_READ) {
         for (n = 0; n < datacnt; n++) {
-            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
+            s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
         }
         dma_memory_write(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
     } else {
@@ -774,7 +774,7 @@ static void sdhci_do_adma(SDHCIState *s)
                 while (length) {
                     if (s->data_count == 0) {
                         for (n = 0; n < block_size; n++) {
-                            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
+                            s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
                         }
                     }
                     begin = s->data_count;
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 9210ef567f1..a7ef9cb9225 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -190,7 +190,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
         s->mode = SSI_SD_DATA_READ;
         return 0xfe;
     case SSI_SD_DATA_READ:
-        val = sdbus_read_data(&s->sdbus);
+        val = sdbus_read_byte(&s->sdbus);
         if (!sdbus_data_ready(&s->sdbus)) {
             DPRINTF("Data read end\n");
             s->mode = SSI_SD_CMD;
-- 
2.26.2

[PULL 18/23] hw/sd: Add sdbus_write_data() to write multiples bytes on the data line

[Philippe Mathieu-Daudé]

Add a sdbus_write_data() method to write multiple bytes on the
data line of a SD bus.
We might improve the tracing later, for now keep logging each
byte individually.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-5-f4bug@amsat.org>
---
 include/hw/sd/sd.h |  9 +++++++++
 hw/sd/core.c       | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 14ffc7f4758..3ae3e8939b3 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -175,6 +175,15 @@ void sdbus_write_byte(SDBus *sd, uint8_t value);
  * Return: byte value read
  */
 uint8_t sdbus_read_byte(SDBus *sd);
+/**
+ * Write data to a SD bus.
+ * @sdbus: bus
+ * @buf: data to write
+ * @length: number of bytes to write
+ *
+ * Write multiple bytes of data on the data lines of a SD bus.
+ */
+void sdbus_write_data(SDBus *sdbus, const void *buf, size_t length);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
 bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index a3b620b802b..9c2781ebf96 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -114,6 +114,21 @@ void sdbus_write_byte(SDBus *sdbus, uint8_t value)
     }
 }
 
+void sdbus_write_data(SDBus *sdbus, const void *buf, size_t length)
+{
+    SDState *card = get_card(sdbus);
+    const uint8_t *data = buf;
+
+    if (card) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+        for (size_t i = 0; i < length; i++) {
+            trace_sdbus_write(sdbus_name(sdbus), data[i]);
+            sc->write_byte(card, data[i]);
+        }
+    }
+}
+
 uint8_t sdbus_read_byte(SDBus *sdbus)
 {
     SDState *card = get_card(sdbus);
-- 
2.26.2

[PULL 19/23] hw/sd: Use sdbus_write_data() instead of sdbus_write_byte when possible

[Philippe Mathieu-Daudé]

Use the recently added sdbus_write_data() to write multiple
bytes at once, instead of looping calling sdbus_write_byte().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-6-f4bug@amsat.org>
---
 hw/sd/allwinner-sdhost.c  | 14 +++++---------
 hw/sd/milkymist-memcard.c |  7 +++----
 hw/sd/sdhci.c             | 18 ++++--------------
 3 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index c004aa39da6..eea5659c5f1 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -333,10 +333,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
         if (is_write) {
             cpu_physical_memory_read((desc->addr & DESC_SIZE_MASK) + num_done,
                                       buf, buf_bytes);
-
-            for (uint32_t i = 0; i < buf_bytes; i++) {
-                sdbus_write_byte(&s->sdbus, buf[i]);
-            }
+            sdbus_write_data(&s->sdbus, buf, buf_bytes);
 
         /* Read from SD bus */
         } else {
@@ -548,6 +545,7 @@ static void allwinner_sdhost_write(void *opaque, hwaddr offset,
                                    uint64_t value, unsigned size)
 {
     AwSdHostState *s = AW_SDHOST(opaque);
+    uint32_t u32;
 
     trace_allwinner_sdhost_write(offset, value, size);
 
@@ -654,11 +652,9 @@ static void allwinner_sdhost_write(void *opaque, hwaddr offset,
         s->startbit_detect = value;
         break;
     case REG_SD_FIFO:      /* Read/Write FIFO */
-        sdbus_write_byte(&s->sdbus, value & 0xff);
-        sdbus_write_byte(&s->sdbus, (value >> 8) & 0xff);
-        sdbus_write_byte(&s->sdbus, (value >> 16) & 0xff);
-        sdbus_write_byte(&s->sdbus, (value >> 24) & 0xff);
-        allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
+        u32 = cpu_to_le32(value);
+        sdbus_write_data(&s->sdbus, &u32, sizeof(u32));
+        allwinner_sdhost_update_transfer_cnt(s, sizeof(u32));
         allwinner_sdhost_auto_stop(s);
         allwinner_sdhost_update_irq(s);
         break;
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index e8d055bb895..12e091a46e7 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -181,6 +181,7 @@ static void memcard_write(void *opaque, hwaddr addr, uint64_t value,
                           unsigned size)
 {
     MilkymistMemcardState *s = opaque;
+    uint32_t val32;
 
     trace_milkymist_memcard_memory_write(addr, value);
 
@@ -209,10 +210,8 @@ static void memcard_write(void *opaque, hwaddr addr, uint64_t value,
         if (!s->enabled) {
             break;
         }
-        sdbus_write_byte(&s->sdbus, (value >> 24) & 0xff);
-        sdbus_write_byte(&s->sdbus, (value >> 16) & 0xff);
-        sdbus_write_byte(&s->sdbus, (value >> 8) & 0xff);
-        sdbus_write_byte(&s->sdbus, value & 0xff);
+        val32 = cpu_to_be32(value);
+        sdbus_write_data(&s->sdbus, &val32, sizeof(val32));
         break;
     case R_ENABLE:
         s->regs[addr] = value;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b897b1121b8..ddf36915619 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -496,8 +496,6 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
 /* Write data from host controller FIFO to card */
 static void sdhci_write_block_to_card(SDHCIState *s)
 {
-    int index = 0;
-
     if (s->prnsts & SDHC_SPACE_AVAILABLE) {
         if (s->norintstsen & SDHC_NISEN_WBUFRDY) {
             s->norintsts |= SDHC_NIS_WBUFRDY;
@@ -514,9 +512,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
         }
     }
 
-    for (index = 0; index < (s->blksize & BLOCK_SIZE_MASK); index++) {
-        sdbus_write_byte(&s->sdbus, s->fifo_buffer[index]);
-    }
+    sdbus_write_data(&s->sdbus, s->fifo_buffer, s->blksize & BLOCK_SIZE_MASK);
 
     /* Next data can be written through BUFFER DATORT register */
     s->prnsts |= SDHC_SPACE_AVAILABLE;
@@ -641,9 +637,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                             &s->fifo_buffer[begin], s->data_count - begin);
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
-                for (n = 0; n < block_size; n++) {
-                    sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
-                }
+                sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size);
                 s->data_count = 0;
                 if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
                     s->blkcnt--;
@@ -678,9 +672,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
         dma_memory_write(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
     } else {
         dma_memory_read(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
-        for (n = 0; n < datacnt; n++) {
-            sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
-        }
+        sdbus_write_data(&s->sdbus, s->fifo_buffer, datacnt);
     }
     s->blkcnt--;
 
@@ -814,9 +806,7 @@ static void sdhci_do_adma(SDHCIState *s)
                                     s->data_count - begin);
                     dscr.addr += s->data_count - begin;
                     if (s->data_count == block_size) {
-                        for (n = 0; n < block_size; n++) {
-                            sdbus_write_byte(&s->sdbus, s->fifo_buffer[n]);
-                        }
+                        sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size);
                         s->data_count = 0;
                         if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
                             s->blkcnt--;
-- 
2.26.2

[PULL 20/23] hw/sd: Add sdbus_read_data() to read multiples bytes on the data line

[Philippe Mathieu-Daudé]

Add a sdbus_read_data() method to read multiple bytes on the
data line of a SD bus.
We might improve the tracing later, for now keep logging each
byte individually.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-7-f4bug@amsat.org>
---
 include/hw/sd/sd.h |  9 +++++++++
 hw/sd/core.c       | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 3ae3e8939b3..ac02d61a7a0 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -184,6 +184,15 @@ uint8_t sdbus_read_byte(SDBus *sd);
  * Write multiple bytes of data on the data lines of a SD bus.
  */
 void sdbus_write_data(SDBus *sdbus, const void *buf, size_t length);
+/**
+ * Read data from a SD bus.
+ * @sdbus: bus
+ * @buf: buffer to read data into
+ * @length: number of bytes to read
+ *
+ * Read multiple bytes of data on the data lines of a SD bus.
+ */
+void sdbus_read_data(SDBus *sdbus, void *buf, size_t length);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
 bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 9c2781ebf96..957d116f1a7 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -144,6 +144,21 @@ uint8_t sdbus_read_byte(SDBus *sdbus)
     return value;
 }
 
+void sdbus_read_data(SDBus *sdbus, void *buf, size_t length)
+{
+    SDState *card = get_card(sdbus);
+    uint8_t *data = buf;
+
+    if (card) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+        for (size_t i = 0; i < length; i++) {
+            data[i] = sc->read_byte(card);
+            trace_sdbus_read(sdbus_name(sdbus), data[i]);
+        }
+    }
+}
+
 bool sdbus_data_ready(SDBus *sdbus)
 {
     SDState *card = get_card(sdbus);
-- 
2.26.2

[PULL 21/23] hw/sd: Use sdbus_read_data() instead of sdbus_read_byte() when possible

[Philippe Mathieu-Daudé]

Use the recently added sdbus_read_data() to read multiple
bytes at once, instead of looping calling sdbus_read_byte().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200814092346.21825-8-f4bug@amsat.org>
---
 hw/sd/allwinner-sdhost.c  | 10 +++-------
 hw/sd/milkymist-memcard.c |  7 ++-----
 hw/sd/sdhci.c             | 28 ++++++++--------------------
 3 files changed, 13 insertions(+), 32 deletions(-)

diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index eea5659c5f1..f9eb92c09ed 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -337,9 +337,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
 
         /* Read from SD bus */
         } else {
-            for (uint32_t i = 0; i < buf_bytes; i++) {
-                buf[i] = sdbus_read_byte(&s->sdbus);
-            }
+            sdbus_read_data(&s->sdbus, buf, buf_bytes);
             cpu_physical_memory_write((desc->addr & DESC_SIZE_MASK) + num_done,
                                        buf, buf_bytes);
         }
@@ -518,10 +516,8 @@ static uint64_t allwinner_sdhost_read(void *opaque, hwaddr offset,
         break;
     case REG_SD_FIFO:      /* Read/Write FIFO */
         if (sdbus_data_ready(&s->sdbus)) {
-            res = sdbus_read_byte(&s->sdbus);
-            res |= sdbus_read_byte(&s->sdbus) << 8;
-            res |= sdbus_read_byte(&s->sdbus) << 16;
-            res |= sdbus_read_byte(&s->sdbus) << 24;
+            sdbus_read_data(&s->sdbus, &res, sizeof(uint32_t));
+            le32_to_cpus(&res);
             allwinner_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
             allwinner_sdhost_auto_stop(s);
             allwinner_sdhost_update_irq(s);
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 12e091a46e7..be89a938763 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -151,11 +151,8 @@ static uint64_t memcard_read(void *opaque, hwaddr addr,
         if (!s->enabled) {
             r = 0xffffffff;
         } else {
-            r = 0;
-            r |= sdbus_read_byte(&s->sdbus) << 24;
-            r |= sdbus_read_byte(&s->sdbus) << 16;
-            r |= sdbus_read_byte(&s->sdbus) << 8;
-            r |= sdbus_read_byte(&s->sdbus);
+            sdbus_read_data(&s->sdbus, &r, sizeof(r));
+            be32_to_cpus(&r);
         }
         break;
     case R_CLK2XDIV:
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ddf36915619..1785d7e1f79 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -399,8 +399,6 @@ static void sdhci_end_transfer(SDHCIState *s)
 /* Fill host controller's read buffer with BLKSIZE bytes of data from card */
 static void sdhci_read_block_from_card(SDHCIState *s)
 {
-    int index = 0;
-    uint8_t data;
     const uint16_t blk_size = s->blksize & BLOCK_SIZE_MASK;
 
     if ((s->trnmod & SDHC_TRNS_MULTI) &&
@@ -408,12 +406,9 @@ static void sdhci_read_block_from_card(SDHCIState *s)
         return;
     }
 
-    for (index = 0; index < blk_size; index++) {
-        data = sdbus_read_byte(&s->sdbus);
-        if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
-            /* Device is not in tuning */
-            s->fifo_buffer[index] = data;
-        }
+    if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
+        /* Device is not in tuning */
+        sdbus_read_data(&s->sdbus, s->fifo_buffer, blk_size);
     }
 
     if (FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
@@ -574,7 +569,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
 static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 {
     bool page_aligned = false;
-    unsigned int n, begin;
+    unsigned int begin;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >> 12) + 12);
     uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
@@ -596,9 +591,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                 SDHC_DAT_LINE_ACTIVE;
         while (s->blkcnt) {
             if (s->data_count == 0) {
-                for (n = 0; n < block_size; n++) {
-                    s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
-                }
+                sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
             }
             begin = s->data_count;
             if (((boundary_count + begin) < block_size) && page_aligned) {
@@ -662,13 +655,10 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 /* single block SDMA transfer */
 static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 {
-    int n;
     uint32_t datacnt = s->blksize & BLOCK_SIZE_MASK;
 
     if (s->trnmod & SDHC_TRNS_READ) {
-        for (n = 0; n < datacnt; n++) {
-            s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
-        }
+        sdbus_read_data(&s->sdbus, s->fifo_buffer, datacnt);
         dma_memory_write(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
     } else {
         dma_memory_read(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
@@ -731,7 +721,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 
 static void sdhci_do_adma(SDHCIState *s)
 {
-    unsigned int n, begin, length;
+    unsigned int begin, length;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     ADMADescr dscr = {};
     int i;
@@ -765,9 +755,7 @@ static void sdhci_do_adma(SDHCIState *s)
             if (s->trnmod & SDHC_TRNS_READ) {
                 while (length) {
                     if (s->data_count == 0) {
-                        for (n = 0; n < block_size; n++) {
-                            s->fifo_buffer[n] = sdbus_read_byte(&s->sdbus);
-                        }
+                        sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
                     }
                     begin = s->data_count;
                     if ((length + begin) < block_size) {
-- 
2.26.2

[PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Philippe Mathieu-Daudé]

From: Bin Meng <bin.meng@windriver.com>

At present the function switch status data structure bit [399:376]
are wrongly pupulated. These 3 bytes encode function switch status
for the 6 function groups, with 4 bits per group, starting from
function group 6 at bit 399, then followed by function group 5 at
bit 395, and so on.

However the codes mistakenly fills in the function group 1 status
at bit 399. This fixes the code logic.

Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7c9d956f113..805e21fc883 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
     sd->data[11] = 0x43;
     sd->data[12] = 0x80;	/* Supported group 1 functions */
     sd->data[13] = 0x03;
+
     for (i = 0; i < 6; i ++) {
         new_func = (arg >> (i * 4)) & 0x0f;
         if (mode && new_func != 0x0f)
             sd->function_group[i] = new_func;
-        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
+        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
     }
     memset(&sd->data[17], 0, 47);
     stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
-- 
2.26.2

[PULL 23/23] hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card

[Philippe Mathieu-Daudé]

From: Bin Meng <bin.meng@windriver.com>

Per the SD spec, Standard Capacity SD Memory Card (SDSC) supports
capacity up to and including 2 GiB.

Fixes: 2d7adea4fe ("hw/sd: Support SDHC size cards")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Message-Id: <1598021136-49525-2-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 805e21fc883..483c4f17204 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -51,6 +51,8 @@
 
 //#define DEBUG_SD 1
 
+#define SDSC_MAX_CAPACITY   (2 * GiB)
+
 typedef enum {
     sd_r0 = 0,    /* no response */
     sd_r1,        /* normal response command */
@@ -314,7 +316,7 @@ static void sd_ocr_powerup(void *opaque)
     /* card power-up OK */
     sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
 
-    if (sd->size > 1 * GiB) {
+    if (sd->size > SDSC_MAX_CAPACITY) {
         sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_CAPACITY, 1);
     }
 }
@@ -386,7 +388,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
     uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1;
 
-    if (size <= 1 * GiB) { /* Standard Capacity SD */
+    if (size <= SDSC_MAX_CAPACITY) { /* Standard Capacity SD */
         sd->csd[0] = 0x00;	/* CSD structure */
         sd->csd[1] = 0x26;	/* Data read access-time-1 */
         sd->csd[2] = 0x00;	/* Data read access-time-2 */
-- 
2.26.2

Re: [PULL 00/23] SD/MMC patches for 2020-08-21

[Peter Maydell]

On Fri, 21 Aug 2020 at 18:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The following changes since commit d6f83a72a7db94a3ede9f5cc4fb39f9c8e89f954:
>
>   Merge remote-tracking branch 'remotes/philmd-gitlab/tags/acceptance-testing=
> -20200812' into staging (2020-08-21 14:51:43 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/sd-next-20200821
>
> for you to fetch changes up to 6d2d4069c47e23b9e3913f9c8204fd0edcb99fb3:
>
>   hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card (2020=
> -08-21 16:49:22 +0200)
>
> ----------------------------------------------------------------
> SD/MMC patches
>
> - Convert legacy SD host controller to the SDBus API
> - Move legacy API to a separate "sdcard_legacy.h" header
> - Introduce methods to access multiple bytes on SDBus data lines
> - Fix 'switch function' group location
> - Fix SDSC maximum card size (2GB)
>
> CI jobs result:
>   https://gitlab.com/philmd/qemu/-/pipelines/180605963


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Philippe Mathieu-Daudé]

Hi Bin,

On 8/21/20 7:29 PM, Philippe Mathieu-Daudé wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present the function switch status data structure bit [399:376]
> are wrongly pupulated. These 3 bytes encode function switch status
> for the 6 function groups, with 4 bits per group, starting from
> function group 6 at bit 399, then followed by function group 5 at
> bit 395, and so on.
> 
> However the codes mistakenly fills in the function group 1 status
> at bit 399. This fixes the code logic.
> 
> Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/sd/sd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 7c9d956f113..805e21fc883 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>       sd->data[11] = 0x43;
>       sd->data[12] = 0x80;	/* Supported group 1 functions */
>       sd->data[13] = 0x03;
> +
>       for (i = 0; i < 6; i ++) {
>           new_func = (arg >> (i * 4)) & 0x0f;
>           if (mode && new_func != 0x0f)
>               sd->function_group[i] = new_func;
> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);

This patch broke the orangepi machine, reproducible running
test_arm_orangepi_bionic:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg739449.html

Can you have a look?

>       }
>       memset(&sd->data[17], 0, 47);
>       stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
> 

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Bin Meng]

Hi Philippe,

On Tue, Oct 20, 2020 at 11:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Bin,
>
> On 8/21/20 7:29 PM, Philippe Mathieu-Daudé wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present the function switch status data structure bit [399:376]
> > are wrongly pupulated. These 3 bytes encode function switch status
> > for the 6 function groups, with 4 bits per group, starting from
> > function group 6 at bit 399, then followed by function group 5 at
> > bit 395, and so on.
> >
> > However the codes mistakenly fills in the function group 1 status
> > at bit 399. This fixes the code logic.
> >
> > Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   hw/sd/sd.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 7c9d956f113..805e21fc883 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
> >       sd->data[11] = 0x43;
> >       sd->data[12] = 0x80;    /* Supported group 1 functions */
> >       sd->data[13] = 0x03;
> > +
> >       for (i = 0; i < 6; i ++) {
> >           new_func = (arg >> (i * 4)) & 0x0f;
> >           if (mode && new_func != 0x0f)
> >               sd->function_group[i] = new_func;
> > -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> > +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
>
> This patch broke the orangepi machine, reproducible running
> test_arm_orangepi_bionic:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg739449.html
>
> Can you have a look?

Yes, I can take a look. Could you please send more details on how to
run "test_arm_orangepi_bionic"?

Regards,
Bin

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Philippe Mathieu-Daudé]

On 10/21/20 11:57 AM, Bin Meng wrote:
> Hi Philippe,
> 
> On Tue, Oct 20, 2020 at 11:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Bin,
>>
>> On 8/21/20 7:29 PM, Philippe Mathieu-Daudé wrote:
>>> From: Bin Meng <bin.meng@windriver.com>
>>>
>>> At present the function switch status data structure bit [399:376]
>>> are wrongly pupulated. These 3 bytes encode function switch status
>>> for the 6 function groups, with 4 bits per group, starting from
>>> function group 6 at bit 399, then followed by function group 5 at
>>> bit 395, and so on.
>>>
>>> However the codes mistakenly fills in the function group 1 status
>>> at bit 399. This fixes the code logic.
>>>
>>> Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
>>> Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>    hw/sd/sd.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 7c9d956f113..805e21fc883 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>>>        sd->data[11] = 0x43;
>>>        sd->data[12] = 0x80;    /* Supported group 1 functions */
>>>        sd->data[13] = 0x03;
>>> +
>>>        for (i = 0; i < 6; i ++) {
>>>            new_func = (arg >> (i * 4)) & 0x0f;
>>>            if (mode && new_func != 0x0f)
>>>                sd->function_group[i] = new_func;
>>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
>>> +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
>>
>> This patch broke the orangepi machine, reproducible running
>> test_arm_orangepi_bionic:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg739449.html
>>
>> Can you have a look?
> 
> Yes, I can take a look. Could you please send more details on how to
> run "test_arm_orangepi_bionic"?

Looking at the previous link, I think this should work:

$ make check-venv qemu-system-arm
$ AVOCADO_ALLOW_LARGE_STORAGE=1 \
   tests/venv/bin/python -m \
     avocado --show=app,console run \
       run -t machine:orangepi-pc tests/acceptance

> 
> Regards,
> Bin
> 

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Bin Meng]

Hi Philippe,

On Wed, Oct 21, 2020 at 6:07 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 10/21/20 11:57 AM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Tue, Oct 20, 2020 at 11:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi Bin,
> >>
> >> On 8/21/20 7:29 PM, Philippe Mathieu-Daudé wrote:
> >>> From: Bin Meng <bin.meng@windriver.com>
> >>>
> >>> At present the function switch status data structure bit [399:376]
> >>> are wrongly pupulated. These 3 bytes encode function switch status
> >>> for the 6 function groups, with 4 bits per group, starting from
> >>> function group 6 at bit 399, then followed by function group 5 at
> >>> bit 395, and so on.
> >>>
> >>> However the codes mistakenly fills in the function group 1 status
> >>> at bit 399. This fixes the code logic.
> >>>
> >>> Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> >>> Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>>    hw/sd/sd.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >>> index 7c9d956f113..805e21fc883 100644
> >>> --- a/hw/sd/sd.c
> >>> +++ b/hw/sd/sd.c
> >>> @@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
> >>>        sd->data[11] = 0x43;
> >>>        sd->data[12] = 0x80;    /* Supported group 1 functions */
> >>>        sd->data[13] = 0x03;
> >>> +
> >>>        for (i = 0; i < 6; i ++) {
> >>>            new_func = (arg >> (i * 4)) & 0x0f;
> >>>            if (mode && new_func != 0x0f)
> >>>                sd->function_group[i] = new_func;
> >>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> >>> +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
> >>
> >> This patch broke the orangepi machine, reproducible running
> >> test_arm_orangepi_bionic:
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg739449.html
> >>
> >> Can you have a look?
> >
> > Yes, I can take a look. Could you please send more details on how to
> > run "test_arm_orangepi_bionic"?
>
> Looking at the previous link, I think this should work:
>
> $ make check-venv qemu-system-arm
> $ AVOCADO_ALLOW_LARGE_STORAGE=1 \
>    tests/venv/bin/python -m \
>      avocado --show=app,console run \
>        run -t machine:orangepi-pc tests/acceptance
>

I tried the above command in my build tree, and got:

avocado run: error: unrecognized arguments: tests/acceptance
Perhaps a plugin is missing; run 'avocado plugins' to list the installed ones

I switched to `make check-acceptance` which seems to work, however not
for orangepi-pc related tests?

 (09/32) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi:
SKIP: Test artifacts fetched from unreliable apt.armbian.com
 (10/32) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd:
SKIP: Test artifacts fetched from unreliable apt.armbian.com
 (11/32) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd:
SKIP: Test artifacts fetched from unreliable apt.armbian.com
 (12/32) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic:
SKIP: storage limited
 (13/32) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
SKIP: storage limited

Any ideas?

Regards,
Bin

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Niek Linnenbank]

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

Hi Bin, Philippe,

If im correct the acceptance tests for orange pi need to be run with a flag
ARMBIAN_ARTIFACTS_CACHED set that explicitly allows them to be run using
the armbian mirror. So if you pass that flag on the same command that
Philippe gave, the rests should run.

I have a follow up question and Im interested to hear your opinion on that
Philippe. Should we perhaps update the orange pi tests (and maybe others)
so they use a reliable mirror that we can control, for example a github
repo? I would be happy to create a repo for that, at least for the orange
pi tests. But maybe there is already something planned as a more general
solution for artifacts of other machines as well?

regards,
Niek

Op do 22 okt. 2020 16:47 schreef Bin Meng <bmeng.cn@gmail.com>:

> Hi Philippe,
>
> On Wed, Oct 21, 2020 at 6:07 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> >
> > On 10/21/20 11:57 AM, Bin Meng wrote:
> > > Hi Philippe,
> > >
> > > On Tue, Oct 20, 2020 at 11:18 PM Philippe Mathieu-Daudé <
> f4bug@amsat.org> wrote:
> > >>
> > >> Hi Bin,
> > >>
> > >> On 8/21/20 7:29 PM, Philippe Mathieu-Daudé wrote:
> > >>> From: Bin Meng <bin.meng@windriver.com>
> > >>>
> > >>> At present the function switch status data structure bit [399:376]
> > >>> are wrongly pupulated. These 3 bytes encode function switch status
> > >>> for the 6 function groups, with 4 bits per group, starting from
> > >>> function group 6 at bit 399, then followed by function group 5 at
> > >>> bit 395, and so on.
> > >>>
> > >>> However the codes mistakenly fills in the function group 1 status
> > >>> at bit 399. This fixes the code logic.
> > >>>
> > >>> Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
> > >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >>> Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > >>> Message-Id: <1598021136-49525-1-git-send-email-bmeng.cn@gmail.com>
> > >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >>> ---
> > >>>    hw/sd/sd.c | 3 ++-
> > >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > >>> index 7c9d956f113..805e21fc883 100644
> > >>> --- a/hw/sd/sd.c
> > >>> +++ b/hw/sd/sd.c
> > >>> @@ -807,11 +807,12 @@ static void sd_function_switch(SDState *sd,
> uint32_t arg)
> > >>>        sd->data[11] = 0x43;
> > >>>        sd->data[12] = 0x80;    /* Supported group 1 functions */
> > >>>        sd->data[13] = 0x03;
> > >>> +
> > >>>        for (i = 0; i < 6; i ++) {
> > >>>            new_func = (arg >> (i * 4)) & 0x0f;
> > >>>            if (mode && new_func != 0x0f)
> > >>>                sd->function_group[i] = new_func;
> > >>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> > >>> +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
> > >>
> > >> This patch broke the orangepi machine, reproducible running
> > >> test_arm_orangepi_bionic:
> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg739449.html
> > >>
> > >> Can you have a look?
> > >
> > > Yes, I can take a look. Could you please send more details on how to
> > > run "test_arm_orangepi_bionic"?
> >
> > Looking at the previous link, I think this should work:
> >
> > $ make check-venv qemu-system-arm
> > $ AVOCADO_ALLOW_LARGE_STORAGE=1 \
> >    tests/venv/bin/python -m \
> >      avocado --show=app,console run \
> >        run -t machine:orangepi-pc tests/acceptance
> >
>
> I tried the above command in my build tree, and got:
>
> avocado run: error: unrecognized arguments: tests/acceptance
> Perhaps a plugin is missing; run 'avocado plugins' to list the installed
> ones
>
> I switched to `make check-acceptance` which seems to work, however not
> for orangepi-pc related tests?
>
>  (09/32)
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi:
> SKIP: Test artifacts fetched from unreliable apt.armbian.com
>  (10/32)
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd:
> SKIP: Test artifacts fetched from unreliable apt.armbian.com
>  (11/32)
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd:
> SKIP: Test artifacts fetched from unreliable apt.armbian.com
>  (12/32)
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic:
> SKIP: storage limited
>  (13/32)
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
> SKIP: storage limited
>
> Any ideas?
>
> Regards,
> Bin
>

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

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Bin Meng]

Hi Niek,

On Thu, Oct 22, 2020 at 11:20 PM Niek Linnenbank
<nieklinnenbank@gmail.com> wrote:
>
> Hi Bin, Philippe,
>
> If im correct the acceptance tests for orange pi need to be run with a flag ARMBIAN_ARTIFACTS_CACHED set that explicitly allows them to be run using the armbian mirror. So if you pass that flag on the same command that Philippe gave, the rests should run.

Thank you for the hints. Actually I noticed the environment variable
ARMBIAN_ARTIFACTS_CACHED when looking at the test codes, but after I
turned on the flag it still could not download the test asset from the
apt.armbian.com website.

> I have a follow up question and Im interested to hear your opinion on that Philippe. Should we perhaps update the orange pi tests (and maybe others) so they use a reliable mirror that we can control, for example a github repo? I would be happy to create a repo for that, at least for the orange pi tests. But maybe there is already something planned as a more general solution for artifacts of other machines as well?
>

Regards,
Bin

Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure

[Philippe Mathieu-Daudé]

On 10/23/20 4:02 AM, Bin Meng wrote:
> Hi Niek,
> 
> On Thu, Oct 22, 2020 at 11:20 PM Niek Linnenbank
> <nieklinnenbank@gmail.com> wrote:
>>
>> Hi Bin, Philippe,
>>
>> If im correct the acceptance tests for orange pi need to be run with a flag ARMBIAN_ARTIFACTS_CACHED set that explicitly allows them to be run using the armbian mirror. So if you pass that flag on the same command that Philippe gave, the rests should run.
> 
> Thank you for the hints. Actually I noticed the environment variable
> ARMBIAN_ARTIFACTS_CACHED when looking at the test codes, but after I
> turned on the flag it still could not download the test asset from the
> apt.armbian.com website.

This patch could help you, but it use a different image [*]:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg737760.html
(I haven't tested the image yet but I assume the same bug is present).

> 
>> I have a follow up question and Im interested to hear your opinion on that Philippe. Should we perhaps update the orange pi tests (and maybe others) so they use a reliable mirror that we can control, for example a github repo? I would be happy to create a repo for that, at least for the orange pi tests. But maybe there is already something planned as a more general solution for artifacts of other machines as well?

This is a technical debt between the Avocado and QEMU communities
(or a misunderstanding?).

QEMU community understood the Avocado caching would be an helpful
feature, but it ended being more of a problem.


I.e. here Niek, Michael Roth and myself still have the image cached,
so we can keep running the tests committed to the repository. You
Bin can not, as the armbian mirror is not stable and remove the old
image.

The old image (Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img)
has been manually tested by Niek and myself, I enabled tracing and
look at the SD packets for some time, was happy how the model worked
and decided we should keep running a test with this particular image.

Armbian released new images, in particular the one Cleber sent in [*]:
Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz. While it might
work similarly, I haven't tested it, and don't have time to do again
the same audit.
 From my experience with the Raspberry Pi, these images never work the
same way, as the Linux kernel evolves quickly with these kinda recent
embedded boards. The QEMU raspi models have to emulate new devices
(or complete current ones) every years, because Linux started to use
a device differently, or started to use a part of the SoC that was not
used before. Either the kernel doesn't boot, or there are side-effect
later when userspace program is executed. PITA to debug TBH. For this
reason I disagree with updating test images to the latest releases.

It would be nice to know QEMU works with the latest Armbian, but
nobody popped up on the mailing list asking for it. I am personally
happy enough using the 19.11 release for now.

Should we keep using Avocado for functional testing? (was: Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure)

[Philippe Mathieu-Daudé]

On 10/23/20 11:23 AM, Philippe Mathieu-Daudé wrote:
> On 10/23/20 4:02 AM, Bin Meng wrote:
>> Hi Niek,
>>
>> On Thu, Oct 22, 2020 at 11:20 PM Niek Linnenbank
>> <nieklinnenbank@gmail.com> wrote:
>>>
>>> Hi Bin, Philippe,
>>>
>>> If im correct the acceptance tests for orange pi need to be run with 
>>> a flag ARMBIAN_ARTIFACTS_CACHED set that explicitly allows them to be 
>>> run using the armbian mirror. So if you pass that flag on the same 
>>> command that Philippe gave, the rests should run.
>>
>> Thank you for the hints. Actually I noticed the environment variable
>> ARMBIAN_ARTIFACTS_CACHED when looking at the test codes, but after I
>> turned on the flag it still could not download the test asset from the
>> apt.armbian.com website.
> 
> This patch could help you, but it use a different image [*]:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg737760.html
> (I haven't tested the image yet but I assume the same bug is present).
> 
>>
>>> I have a follow up question and Im interested to hear your opinion on 
>>> that Philippe. Should we perhaps update the orange pi tests (and 
>>> maybe others) so they use a reliable mirror that we can control, for 
>>> example a github repo? I would be happy to create a repo for that, at 
>>> least for the orange pi tests. But maybe there is already something 
>>> planned as a more general solution for artifacts of other machines as 
>>> well?
> 
> This is a technical debt between the Avocado and QEMU communities
> (or a misunderstanding?).
> 
> QEMU community understood the Avocado caching would be an helpful
> feature, but it ended being more of a problem.
> 
> 
> I.e. here Niek, Michael Roth and myself still have the image cached,
> so we can keep running the tests committed to the repository. You
> Bin can not, as the armbian mirror is not stable and remove the old
> image.
> 
> The old image (Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img)
> has been manually tested by Niek and myself, I enabled tracing and
> look at the SD packets for some time, was happy how the model worked
> and decided we should keep running a test with this particular image.
> 
> Armbian released new images, in particular the one Cleber sent in [*]:
> Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz. While it might
> work similarly, I haven't tested it, and don't have time to do again
> the same audit.
>  From my experience with the Raspberry Pi, these images never work the
> same way, as the Linux kernel evolves quickly with these kinda recent
> embedded boards. The QEMU raspi models have to emulate new devices
> (or complete current ones) every years, because Linux started to use
> a device differently, or started to use a part of the SoC that was not
> used before. Either the kernel doesn't boot, or there are side-effect
> later when userspace program is executed. PITA to debug TBH. For this
> reason I disagree with updating test images to the latest releases.
> 
> It would be nice to know QEMU works with the latest Armbian, but
> nobody popped up on the mailing list asking for it. I am personally
> happy enough using the 19.11 release for now.

Back to the cache problem, I started to ask 2 years ago,
https://www.mail-archive.com/avocado-devel@redhat.com/msg00860.html
note this is the exact situation we are having:

 >> - What will happens if I add tests downloading running on their
 >> compiled u-boot
 >> 
(https://downloads.gumstix.com/images/angstrom/developer/2012-01-22-1750/u-boot.bin)
 >> and the company decides to remove this old directory?
 >> Since sometimes old open-source software are hard to rebuild with
 >> recent compilers, should we consider to use a public storage to keep
 >> open-source (signed) blobs we can use for integration testing?
 >
 > For Avocado-VT, there are the JeOS images[1], which we keep on a test
 > "assets" directory.  We have a lot of storage/bandwidth availability,
 > so it can be used for other assets proven to be necessary for tests.
 >
 > As long as distribution rights and licensing are not issues, we can
 > definitely use the same server for kernels, u-boot images and what
 > not.
 >
 > [1] - https://avocado-project.org/data/assets/


We discussed about this again last year at the KVM forum. Various
RFE have been filled:
https://www.mail-archive.com/avocado-devel@redhat.com/msg01183.html


What we need is a QEMU community file server similar to the asset
one used by the Avocado community. The problem is who is going to
pay and sysadmin it. IIRC Gerd suggested to use GitLab Page, but
the artifact file size is limited to a maximum of 1GiB:
https://docs.gitlab.com/ee/user/gitlab_com/#gitlab-pages
Alternative is to use a git-lfs server:
https://docs.gitlab.com/ee/topics/git/lfs/#hosting-lfs-objects-externally
This is now a project management problem.

Next week is the KVM forum 2020 and there might be a BoF session
to talk about that:
https://kvmforum2020.sched.com/overview/type/Birds+of+a+Feather+Sessions+(BoF)


Now that the QEMU community started to use gitlab-ci more and more
I received complains that Avocado is not practical (hard to
understand how to reproduce command line to debug, options to use
not clear, timeouts not clear, why download all artifacts to run
a single test, various issues regarding caching, cache filled /home
filesystem) so I have been asked to look at Avocado alternatives,
because we want contributors run more tests and CI, not them be
scared by it.
I haven't looked at alternatives yet, from the various features
we could have use, the biggest one is the ability to interact with
the serial console. And that feature is duplicated with the VM
testing class: tests/vm/basevm.py.
The SSH session used in linux_ssh_mips_malta.py is similar to the
one from basevm.py too (see 'make vm-boot-ssh-%').
Some tests are not 'acceptance' but simple qtest written in Python,
such cpu_queries.py / pc_cpu_hotplug_props.py / migration.py /
pc_cpu_hotplug_props.py / x86_cpu_model_versions.py.

The classes I find practical are downloading artifact, uncompressing
or extracting archive, and eventually the cloudinit one. But we can 
probably use them directly.


Lot of material to discuss :)

Regards,

Phil.

Re: Should we keep using Avocado for functional testing? (was: Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure)

[Niek Linnenbank]

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

Hi Philippe,

On Fri, Oct 23, 2020 at 11:34 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 10/23/20 11:23 AM, Philippe Mathieu-Daudé wrote:
> > On 10/23/20 4:02 AM, Bin Meng wrote:
> >> Hi Niek,
> >>
> >> On Thu, Oct 22, 2020 at 11:20 PM Niek Linnenbank
> >> <nieklinnenbank@gmail.com> wrote:
> >>>
> >>> Hi Bin, Philippe,
> >>>
> >>> If im correct the acceptance tests for orange pi need to be run with
> >>> a flag ARMBIAN_ARTIFACTS_CACHED set that explicitly allows them to be
> >>> run using the armbian mirror. So if you pass that flag on the same
> >>> command that Philippe gave, the rests should run.
> >>
> >> Thank you for the hints. Actually I noticed the environment variable
> >> ARMBIAN_ARTIFACTS_CACHED when looking at the test codes, but after I
> >> turned on the flag it still could not download the test asset from the
> >> apt.armbian.com website.
> >
> > This patch could help you, but it use a different image [*]:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg737760.html
> > (I haven't tested the image yet but I assume the same bug is present).
> >
> >>
> >>> I have a follow up question and Im interested to hear your opinion on
> >>> that Philippe. Should we perhaps update the orange pi tests (and
> >>> maybe others) so they use a reliable mirror that we can control, for
> >>> example a github repo? I would be happy to create a repo for that, at
> >>> least for the orange pi tests. But maybe there is already something
> >>> planned as a more general solution for artifacts of other machines as
> >>> well?
> >
> > This is a technical debt between the Avocado and QEMU communities
> > (or a misunderstanding?).
> >
> > QEMU community understood the Avocado caching would be an helpful
> > feature, but it ended being more of a problem.
> >
> >
> > I.e. here Niek, Michael Roth and myself still have the image cached,
> > so we can keep running the tests committed to the repository. You
> > Bin can not, as the armbian mirror is not stable and remove the old
> > image.
> >
> > The old image (Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img)
> > has been manually tested by Niek and myself, I enabled tracing and
> > look at the SD packets for some time, was happy how the model worked
> > and decided we should keep running a test with this particular image.
> >
> > Armbian released new images, in particular the one Cleber sent in [*]:
> > Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz. While it might
> > work similarly, I haven't tested it, and don't have time to do again
> > the same audit.
> >  From my experience with the Raspberry Pi, these images never work the
> > same way, as the Linux kernel evolves quickly with these kinda recent
> > embedded boards. The QEMU raspi models have to emulate new devices
> > (or complete current ones) every years, because Linux started to use
> > a device differently, or started to use a part of the SoC that was not
> > used before. Either the kernel doesn't boot, or there are side-effect
> > later when userspace program is executed. PITA to debug TBH. For this
> > reason I disagree with updating test images to the latest releases.
>

Yes, I agree that we should try to use stable artifacts for the acceptance
tests.


> >
> > It would be nice to know QEMU works with the latest Armbian, but
> > nobody popped up on the mailing list asking for it. I am personally
> > happy enough using the 19.11 release for now.
>
> Back to the cache problem, I started to ask 2 years ago,
> https://www.mail-archive.com/avocado-devel@redhat.com/msg00860.html
> note this is the exact situation we are having:
>
>  >> - What will happens if I add tests downloading running on their
>  >> compiled u-boot
>  >>
> (
> https://downloads.gumstix.com/images/angstrom/developer/2012-01-22-1750/u-boot.bin
> )
>  >> and the company decides to remove this old directory?
>  >> Since sometimes old open-source software are hard to rebuild with
>  >> recent compilers, should we consider to use a public storage to keep
>  >> open-source (signed) blobs we can use for integration testing?
>  >
>  > For Avocado-VT, there are the JeOS images[1], which we keep on a test
>  > "assets" directory.  We have a lot of storage/bandwidth availability,
>  > so it can be used for other assets proven to be necessary for tests.
>  >
>  > As long as distribution rights and licensing are not issues, we can
>  > definitely use the same server for kernels, u-boot images and what
>  > not.
>  >
>  > [1] - https://avocado-project.org/data/assets/
>
>
> We discussed about this again last year at the KVM forum. Various
> RFE have been filled:
> https://www.mail-archive.com/avocado-devel@redhat.com/msg01183.html
>
>
> What we need is a QEMU community file server similar to the asset
> one used by the Avocado community. The problem is who is going to
> pay and sysadmin it. IIRC Gerd suggested to use GitLab Page, but
> the artifact file size is limited to a maximum of 1GiB:
> https://docs.gitlab.com/ee/user/gitlab_com/#gitlab-pages
> Alternative is to use a git-lfs server:
> https://docs.gitlab.com/ee/topics/git/lfs/#hosting-lfs-objects-externally
> This is now a project management problem.
>
> What about the host / server where qemu.org is located? If we could
store them there in a sub-directory, that should work. Perhaps people from
the community would be willing to provide a mirror server to balance to
traffic load.


> Next week is the KVM forum 2020 and there might be a BoF session
> to talk about that:
>
> https://kvmforum2020.sched.com/overview/type/Birds+of+a+Feather+Sessions+(BoF)
>
>
> Now that the QEMU community started to use gitlab-ci more and more
> I received complains that Avocado is not practical (hard to
> understand how to reproduce command line to debug, options to use
> not clear, timeouts not clear, why download all artifacts to run
> a single test, various issues regarding caching, cache filled /home
> filesystem) so I have been asked to look at Avocado alternatives,
> because we want contributors run more tests and CI, not them be
> scared by it.
>
I haven't looked at alternatives yet, from the various features
> we could have use, the biggest one is the ability to interact with
> the serial console. And that feature is duplicated with the VM
> testing class: tests/vm/basevm.py.
> The SSH session used in linux_ssh_mips_malta.py is similar to the
> one from basevm.py too (see 'make vm-boot-ssh-%').
> Some tests are not 'acceptance' but simple qtest written in Python,
> such cpu_queries.py / pc_cpu_hotplug_props.py / migration.py /
> pc_cpu_hotplug_props.py / x86_cpu_model_versions.py.
>
> The classes I find practical are downloading artifact, uncompressing
> or extracting archive, and eventually the cloudinit one. But we can
> probably use them directly.
>
> Interesting to hear that. Yeah that is probably a big task that takes time
to find the best solution. There are quite some files indeed in the
tests/acceptance directory,
almost a full framework on its own. In fact it could even be something to
consider, to make
./tests/acceptance a stand-alone framework when avocado isn't used anymore
in the future?


>
> Lot of material to discuss :)
>

Thanks for your extensive reply Philippe. Looking forward to hear what the
outcome is on that with the discussions at the KVM forum.


>
> Regards,
>
> Phil.
>
>

-- 
Niek Linnenbank

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

Re: Should we keep using Avocado for functional testing? (was: Re: [PULL 22/23] hw/sd: Fix incorrect populated function switch status data structure)

[Cleber Rosa]

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

On Fri, Oct 23, 2020 at 11:34:29AM +0200, Philippe Mathieu-Daudé wrote:
> On 10/23/20 11:23 AM, Philippe Mathieu-Daudé wrote:
> 
> Back to the cache problem, I started to ask 2 years ago,
> https://www.mail-archive.com/avocado-devel@redhat.com/msg00860.html
> note this is the exact situation we are having:
>

Hi Phil,

I think the issue on this thread was more on the server mirror side,
than on the *cache* side of things, right?  Still, it's an open issue
that needs to be discussed indeed.

> >> - What will happens if I add tests downloading running on their
> >> compiled u-boot
> >> (https://downloads.gumstix.com/images/angstrom/developer/2012-01-22-1750/u-boot.bin)
> >> and the company decides to remove this old directory?
> >> Since sometimes old open-source software are hard to rebuild with
> >> recent compilers, should we consider to use a public storage to keep
> >> open-source (signed) blobs we can use for integration testing?
> >
> > For Avocado-VT, there are the JeOS images[1], which we keep on a test
> > "assets" directory.  We have a lot of storage/bandwidth availability,
> > so it can be used for other assets proven to be necessary for tests.
> >
> > As long as distribution rights and licensing are not issues, we can
> > definitely use the same server for kernels, u-boot images and what
> > not.
> >
> > [1] - https://avocado-project.org/data/assets/
> 
> 
> We discussed about this again last year at the KVM forum. Various
> RFE have been filled:
> https://www.mail-archive.com/avocado-devel@redhat.com/msg01183.html
>

I understand it has been a year, which is a lot of time specially when
it comes to an urgent need such as more and better testing and CI.
But some things can be patched quickly, while others need further
structural work.  More on that later...

> 
> What we need is a QEMU community file server similar to the asset
> one used by the Avocado community. The problem is who is going to
> pay and sysadmin it. IIRC Gerd suggested to use GitLab Page, but
> the artifact file size is limited to a maximum of 1GiB:
> https://docs.gitlab.com/ee/user/gitlab_com/#gitlab-pages
> Alternative is to use a git-lfs server:
> https://docs.gitlab.com/ee/topics/git/lfs/#hosting-lfs-objects-externally
> This is now a project management problem.
> 
> Next week is the KVM forum 2020 and there might be a BoF session
> to talk about that:
> https://kvmforum2020.sched.com/overview/type/Birds+of+a+Feather+Sessions+(BoF)
> 
> 
> Now that the QEMU community started to use gitlab-ci more and more
> I received complains that Avocado is not practical (hard to
> understand how to reproduce command line to debug, options to use
> not clear, timeouts not clear, why download all artifacts to run
> a single test, various issues regarding caching, cache filled /home
> filesystem) so I have been asked to look at Avocado alternatives,
> because we want contributors run more tests and CI, not them be
> scared by it.

What I can say is that the Avocado developers have been working
on a major overhaul to address those issues.  I'm not sure this
is the place/time to go over each of those items, but there may be
some confusion going on some of those issues.  For instance, your
point about "why download all artifacts to run a single test".
It shouldn't, and it doesn't:

  $ du -hcs ~/avocado/data/cache/
  4.0K    /home/cleber/avocado/data/cache/
  4.0K    total

  $ ./tests/venv/bin/avocado run tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc 
  Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc
  JOB ID     : a3c08d7a0acc613c207b27e4b291a0010965af2d
  JOB LOG    : /home/cleber/avocado/job-results/job-2020-10-26T11.50-a3c08d7/job.log
   (1/1) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc: PASS (2.04 s)
  RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
  JOB TIME   : 3.22 s

  $ du -hcs ~/avocado/data/cache/
  8.3M    /home/cleber/avocado/data/cache/
  8.3M    total

About Avocado being hard to understand, I'm a bit saddened by that,
because the team spent a lot of effort in:

 * Restructuring / rewriting / reviewing the documention
 * Normalizing the command line options
 * Deprecating and removing a lot of non-relevant functionality

I've probably said the same thing in a few different channels and
contexts, but for the last ~9 months, the Avocado project had been
working on a new architecture that was highly influenced by the
limitations we found while using Avocado in QEMU.  Version 82.0 was
the result of that work, and it'd be very bad timing if, instead of
integrating this "new Avocado", QEMU would start "from scratch" again.

I've been looking for an opportunity to present what this "new
Avocado" is.  Maybe this or another BoF would be a good place?

> I haven't looked at alternatives yet, from the various features
> we could have use, the biggest one is the ability to interact with
> the serial console. And that feature is duplicated with the VM
> testing class: tests/vm/basevm.py.
> The SSH session used in linux_ssh_mips_malta.py is similar to the
> one from basevm.py too (see 'make vm-boot-ssh-%').
> Some tests are not 'acceptance' but simple qtest written in Python,
> such cpu_queries.py / pc_cpu_hotplug_props.py / migration.py /
> pc_cpu_hotplug_props.py / x86_cpu_model_versions.py.
> 
> The classes I find practical are downloading artifact, uncompressing
> or extracting archive, and eventually the cloudinit one. But we can probably
> use them directly.
>

I understand this is your PoV with regards to how much value you
believe exists in those particular utility modules.  The challenge is
how to combine other people's PoV with regards to the same matter, and
manage them accordingly as a group.  One example is how Pavel Dovgaluk
may find the "gdb" utility module important.

Now consider that Red Hat is vested to have some of the functional /
integration tests that run downstream (and which are based on
Avocado+Avocado-VT) migrated upstreamed.  Then, all of a sudden, a
much larger group of "practical classes" arise.

Finally, these are, as you mentioned in another thread, utility
libraries (such as python/qemu) are commonly needed in other projects
(specially those related to QEMU).  I understand that suggesting to
have more code infrastructure code living in the QEMU would actually
make it harder to solve similar problems once.

> 
> Lot of material to discuss :)
>

Absolutely!

Best,
- Cleber.

> Regards,
> 
> Phil.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]