[PATCH v4 00/10] hw/sd: convert legacy SDHCI devices to the SDBus API

[PATCH v4 00/10] hw/sd: convert legacy SDHCI devices to the SDBus API

[Philippe Mathieu-Daudé]

Hi,

Since v3:
- rebased (was from Feb 2018)
- use named GPIOs
- addressed Peter review comment (adding TYPE_PL181_BUS object)
- convert DPRINF to trace events

Since v2:
- pl181: remove legacy sd_set_cb() (Peter)

Since v1:
- rebased on /master (Peter sdcard reset() patches)
- fix milkymist-mmc from previous seris using instance_init (Michael Walle)

This series convert 3 devices using the legacy SDCard API to the SDBus API:
- milkymist-mmc
- pl181
- ssi-sd

Then move the legacy API to a separate header "sdcard_legacy.h".

Now the OMAP MMC is the last device using the legacy API, but need to get
QOM'ified first.

Regards,

Phil.

v3: https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html

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

Philippe Mathieu-Daudé (9):
  hw/sd/pl181: Rename pl181_send_command() as pl181_do_command()
  hw/sd/pl181: Add TODO to use Fifo32 API
  hw/arm/versatilepb: Comment to remember some IRQs lines are left
    unwired
  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 SDHCI 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'

 include/hw/sd/sd.h            |  17 ------
 include/hw/sd/sdcard_legacy.h |  50 +++++++++++++++
 hw/arm/integratorcp.c         |  17 +++++-
 hw/arm/realview.c             |  16 ++++-
 hw/arm/versatilepb.c          |  28 ++++++++-
 hw/arm/vexpress.c             |  15 ++++-
 hw/sd/omap_mmc.c              |   2 +-
 hw/sd/pl181.c                 | 111 +++++++++++++++++++---------------
 hw/sd/sd.c                    |   3 +-
 hw/sd/trace-events            |  10 +++
 10 files changed, 193 insertions(+), 76 deletions(-)
 create mode 100644 include/hw/sd/sdcard_legacy.h

-- 
2.21.3

[PATCH v4 01/10] 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>
---
 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 2b3776a6a0..649386ec3d 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.21.3

[PATCH v4 02/10] 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>
---
 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 649386ec3d..3fc2cdd71a 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.21.3

[PATCH v4 03/10] 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>
---
 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 3fc2cdd71a..86219c851d 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.21.3

[PATCH v4 04/10] hw/arm/versatilepb: Comment to remember some IRQs lines are left unwired

[Philippe Mathieu-Daudé]

The 'card is readonly' and 'card inserted' IRQs are not wired.
Add a comment in case someone know where to wire them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/versatilepb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index e596b8170f..45a13ae2b9 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -310,7 +310,9 @@ static void versatile_init(MachineState *machine, int board_id)
     qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
 
     sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
+    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
     sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
+    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
 
     /* Add PL031 Real Time Clock. */
     sysbus_create_simple("pl031", 0x101e8000, pic[10]);
-- 
2.21.3

[PATCH v4 05/10] 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>
---
 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 b11a846355..2595e4d052 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 b6c0a1adb9..8dc5f77139 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 5bf9cff8a8..16629d6599 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 86219c851d..ab4cd733a4 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.21.3

[PATCH v4 06/10] 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>
---
Since v3:
Addressed Peter comment, adding TYPE_PL181_BUS:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
---
 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 ab4cd733a4..f6de06ece8 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.21.3

[PATCH v4 07/10] hw/sd/pl181: Do not create SD card within the SDHCI controller

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <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 2595e4d052..9bf25945d2 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 8dc5f77139..4df8ea4fce 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 45a13ae2b9..7fb9da5a47 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,10 +310,31 @@ 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);
+    dev = sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
     /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
-    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);
+    }
+
+    dev = sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
     /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
+    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 16629d6599..a2f70588bd 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 f6de06ece8..f69488ebac 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.21.3

[PATCH v4 08/10] 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>
---
 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 f69488ebac..574500ce60 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 5f09d32eb2..a87d7355fb 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.21.3

[PATCH v4 09/10] 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>
---
 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 a84b8e274a..ace350e0e8 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 97a9d32964..54b1414131 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2059,7 +2059,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.21.3

[PATCH v4 10/10] 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>
---
 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 ace350e0e8..8767ab817c 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 0000000000..8681f8089b
--- /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 4088a8a80b..7d33c59226 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 54b1414131..6bd7ed59e7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -36,6 +36,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.21.3

Re: [PATCH v4 02/10] hw/sd/pl181: Rename pl181_send_command() as pl181_do_command()

[Alistair Francis]

On Sun, Jul 5, 2020 at 1:46 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

Alistair

> ---
>  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 649386ec3d..3fc2cdd71a 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.21.3
>
>

Re: [PATCH v4 05/10] hw/sd/pl181: Use named GPIOs

[Alistair Francis]

On Sun, Jul 5, 2020 at 1:50 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

Alistair

> ---
>  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 b11a846355..2595e4d052 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 b6c0a1adb9..8dc5f77139 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 5bf9cff8a8..16629d6599 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 86219c851d..ab4cd733a4 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.21.3
>
>

Re: [PATCH v4 07/10] hw/sd/pl181: Do not create SD card within the SDHCI controller

[Alistair Francis]

On Sun, Jul 5, 2020 at 1:52 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> SDHCI controllers provide a SD Bus to plug SD cards, but don't
> come with SD card plugged in :) Let the machine/board object
> 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>

Alistair

> ---
>  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 2595e4d052..9bf25945d2 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 8dc5f77139..4df8ea4fce 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 45a13ae2b9..7fb9da5a47 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,10 +310,31 @@ 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);
> +    dev = sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
>      /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
> -    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);
> +    }
> +
> +    dev = sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
>      /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
> +    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 16629d6599..a2f70588bd 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 f6de06ece8..f69488ebac 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.21.3
>
>

Re: [PATCH v4 06/10] hw/sd/pl181: Expose a SDBus and connect the SDCard to it

[Alistair Francis]

On Sun, Jul 5, 2020 at 1:47 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

Alistair

> ---
> Since v3:
> Addressed Peter comment, adding TYPE_PL181_BUS:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
> ---
>  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 ab4cd733a4..f6de06ece8 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.21.3
>
>

Re: [PATCH v4 08/10] hw/sd/pl181: Replace disabled fprintf()s by trace events

[Alistair Francis]

On Sun, Jul 5, 2020 at 1:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

Alistair

> ---
>  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 f69488ebac..574500ce60 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 5f09d32eb2..a87d7355fb 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.21.3
>
>

Re: [PATCH v4 04/10] hw/arm/versatilepb: Comment to remember some IRQs lines are left unwired

[Peter Maydell]

On Sun, 5 Jul 2020 at 21:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The 'card is readonly' and 'card inserted' IRQs are not wired.
> Add a comment in case someone know where to wire them.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/versatilepb.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index e596b8170f..45a13ae2b9 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -310,7 +310,9 @@ static void versatile_init(MachineState *machine, int board_id)
>      qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
>
>      sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>      sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */

These should be wired up to the SYS_MCI register in the
"system and configuration registers" block:
https://developer.arm.com/documentation/dui0225/d/programmer-s-reference/status-and-system-control-registers/mci-register--sys-mci

Our "realview_sysctl" device implements this (we use it on the
realviewpb). However it only has support for having one MMC
device -- we'd need to extend it to have the GPIO inputs for
the 2nd MMC controller, and make the arm_sysctl_gpio_set()
function handle them to set the right bits. Then we could
wire it up at the board level fairly easily (more simply
than for realview, which also has to wire the same lines up
to a GPIO controller).

If you believe the documentation (which includes a little
circuit diagram in both the versatilepb and realviewpb manuals)
then the versatilepb doesn't have the inverter on the CARDIN
line that the realview does.

thanks
-- PMM

Re: [PATCH v4 07/10] hw/sd/pl181: Do not create SD card within the SDHCI controller

[Peter Maydell]

On Sun, 5 Jul 2020 at 21:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> SDHCI controllers provide a SD Bus to plug SD cards, but don't
> come with SD card plugged in :) Let the machine/board object
> create and plug the SD cards when required.

Nit on the commit message: the PL181 isn't an SDHCI controller
(which is a theoretically standard controller as regards
register interface etc, and which in QEMU we model in hw/sd/sdhci.c);
it's just a plain old MMC/SD card controller.

> Signed-off-by: Philippe Mathieu-Daudé <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(-)

> @@ -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);
> +    }

So instead of the pl181 creating the card object, the board
now does it, and only if there actually is an IF_SD drive to
use with it. I guess that makes sense. (If it turns out that
something depends on the existence of the card-without-backing
we could do what xlnx-zcu102.c for instance does, and create
the card in the board wtih a NULL backing drive, but let's not
do that unless we're obliged to.)

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

thanks
-- PMM

Re: [PATCH v4 00/10] hw/sd: convert legacy SDHCI devices to the SDBus API

[Peter Maydell]

On Sun, 5 Jul 2020 at 21:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi,
>
> Since v3:
> - rebased (was from Feb 2018)
> - use named GPIOs
> - addressed Peter review comment (adding TYPE_PL181_BUS object)
> - convert DPRINF to trace events
>
> Since v2:
> - pl181: remove legacy sd_set_cb() (Peter)
>
> Since v1:
> - rebased on /master (Peter sdcard reset() patches)
> - fix milkymist-mmc from previous seris using instance_init (Michael Walle)
>
> This series convert 3 devices using the legacy SDCard API to the SDBus API:
> - milkymist-mmc
> - pl181
> - ssi-sd
>
> Then move the legacy API to a separate header "sdcard_legacy.h".

Hi; I had a couple of minor comments on a few patches. Alistair
has reviewed the rest. This is all for-5.2 material now, but did
you want to take it via your sd queue or should I take it via
target-arm.next ? I don't mind either way.

If you prefer the former you can have an
Acked-by: Peter Maydell <peter.maydell@linaro.org>
for the series.

thanks
-- PMM