[Qemu-arm] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Qemu-arm] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Philippe Mathieu-Daudé]

using the sdbus_*() API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/pl181.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 3ba1f7dd23..ce696c5d7d 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -33,6 +33,7 @@ typedef struct PL181State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
+    SDBus sdbus;
     SDState *card;
     uint32_t clock;
     uint32_t power;
@@ -179,7 +180,7 @@ static void pl181_send_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) {
@@ -223,12 +224,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) {
@@ -249,7 +250,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;
             }
         }
@@ -480,10 +481,6 @@ static void pl181_reset(DeviceState *d)
 
     /* We can assume our GPIO outputs have been wired up now */
     sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
-    /* Since we're still using the legacy SD API the card is not plugged
-     * into any bus, and we must reset it manually.
-     */
-    device_reset(DEVICE(s->card));
 }
 
 static void pl181_init(Object *obj)
@@ -502,14 +499,26 @@ static void pl181_init(Object *obj)
 static void pl181_realize(DeviceState *dev, Error **errp)
 {
     PL181State *s = PL181(dev);
+    DeviceState *carddev;
     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);
-    s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
-    if (s->card == NULL) {
-        error_setg(errp, "sd_init failed");
+    carddev = qdev_create(&s->sdbus.qbus, TYPE_SD_CARD);
+    if (dinfo) {
+        qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo), &err);
+    }
+    object_property_set_bool(OBJECT(carddev), true, "realized", &err);
+    if (err) {
+        error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
+        return;
     }
+    s->card = SD_CARD(carddev);
 }
 
 static void pl181_class_init(ObjectClass *klass, void *data)
-- 
2.15.1

[Qemu-devel] [PATCH v2 6/6] hw/sd: move sdcard legacy API to "hw/sd/sdcard_legacy.h"

[Philippe Mathieu-Daudé]

roughly 2 users left.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h            | 17 ---------------
 include/hw/sd/sdcard_legacy.h | 51 +++++++++++++++++++++++++++++++++++++++++++
 hw/sd/omap_mmc.c              |  2 +-
 hw/sd/pl181.c                 |  1 +
 hw/sd/sd.c                    |  1 +
 5 files changed, 54 insertions(+), 18 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 9bdb3c9285..74bfab9386 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -130,23 +130,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);
-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
- * 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..882e13a8f1
--- /dev/null
+++ b/include/hw/sd/sdcard_legacy.h
@@ -0,0 +1,51 @@
+/*
+ * 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);
+bool sd_data_ready(SDState *card);
+
+/* 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 5b47cadf11..be14ac4f40 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -19,7 +19,7 @@
 #include "qemu/osdep.h"
 #include "hw/hw.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/pl181.c b/hw/sd/pl181.c
index ce696c5d7d..7591d016cd 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -12,6 +12,7 @@
 #include "sysemu/blockdev.h"
 #include "hw/sysbus.h"
 #include "hw/sd/sd.h"
+#include "hw/sd/sdcard_legacy.h"
 #include "qemu/log.h"
 #include "qapi/error.h"
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9880a5d090..6942aa4df3 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -35,6 +35,7 @@
 #include "hw/registerfields.h"
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
+#include "hw/sd/sdcard_legacy.h"
 #include "qapi/error.h"
 #include "qemu/bitmap.h"
 #include "qemu/cutils.h"
-- 
2.15.1

Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:58 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> using the sdbus_*() API.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/pl181.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 3ba1f7dd23..ce696c5d7d 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -33,6 +33,7 @@ typedef struct PL181State {
>      SysBusDevice parent_obj;
>
>      MemoryRegion iomem;
> +    SDBus sdbus;
>      SDState *card;

Shouludn't card be removed?

Alistair

>      uint32_t clock;
>      uint32_t power;
> @@ -179,7 +180,7 @@ static void pl181_send_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) {
> @@ -223,12 +224,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) {
> @@ -249,7 +250,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;
>              }
>          }
> @@ -480,10 +481,6 @@ static void pl181_reset(DeviceState *d)
>
>      /* We can assume our GPIO outputs have been wired up now */
>      sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
> -    /* Since we're still using the legacy SD API the card is not plugged
> -     * into any bus, and we must reset it manually.
> -     */
> -    device_reset(DEVICE(s->card));
>  }
>
>  static void pl181_init(Object *obj)
> @@ -502,14 +499,26 @@ static void pl181_init(Object *obj)
>  static void pl181_realize(DeviceState *dev, Error **errp)
>  {
>      PL181State *s = PL181(dev);
> +    DeviceState *carddev;
>      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);
> -    s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
> -    if (s->card == NULL) {
> -        error_setg(errp, "sd_init failed");
> +    carddev = qdev_create(&s->sdbus.qbus, TYPE_SD_CARD);
> +    if (dinfo) {
> +        qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo), &err);
> +    }
> +    object_property_set_bool(OBJECT(carddev), true, "realized", &err);
> +    if (err) {
> +        error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
> +        return;
>      }
> +    s->card = SD_CARD(carddev);
>  }
>
>  static void pl181_class_init(ObjectClass *klass, void *data)
> --
> 2.15.1
>
>

Re: [Qemu-arm] [Qemu-devel] [PATCH v2 6/6] hw/sd: move sdcard legacy API to "hw/sd/sdcard_legacy.h"

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:58 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> roughly 2 users left.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  include/hw/sd/sd.h            | 17 ---------------
>  include/hw/sd/sdcard_legacy.h | 51 +++++++++++++++++++++++++++++++++++++++++++
>  hw/sd/omap_mmc.c              |  2 +-
>  hw/sd/pl181.c                 |  1 +
>  hw/sd/sd.c                    |  1 +
>  5 files changed, 54 insertions(+), 18 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 9bdb3c9285..74bfab9386 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -130,23 +130,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);
> -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
> - * 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..882e13a8f1
> --- /dev/null
> +++ b/include/hw/sd/sdcard_legacy.h
> @@ -0,0 +1,51 @@
> +/*
> + * 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);
> +bool sd_data_ready(SDState *card);
> +
> +/* 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 5b47cadf11..be14ac4f40 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -19,7 +19,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/hw.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/pl181.c b/hw/sd/pl181.c
> index ce696c5d7d..7591d016cd 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -12,6 +12,7 @@
>  #include "sysemu/blockdev.h"
>  #include "hw/sysbus.h"
>  #include "hw/sd/sd.h"
> +#include "hw/sd/sdcard_legacy.h"
>  #include "qemu/log.h"
>  #include "qapi/error.h"
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9880a5d090..6942aa4df3 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -35,6 +35,7 @@
>  #include "hw/registerfields.h"
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
> +#include "hw/sd/sdcard_legacy.h"
>  #include "qapi/error.h"
>  #include "qemu/bitmap.h"
>  #include "qemu/cutils.h"
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Philippe Mathieu-Daudé]

Hi Alistair,

On 01/31/2018 01:41 PM, Alistair Francis wrote:
> On Mon, Jan 22, 2018 at 7:58 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> using the sdbus_*() API.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/pl181.c | 31 ++++++++++++++++++++-----------
>>  1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
>> index 3ba1f7dd23..ce696c5d7d 100644
>> --- a/hw/sd/pl181.c
>> +++ b/hw/sd/pl181.c
>> @@ -33,6 +33,7 @@ typedef struct PL181State {
>>      SysBusDevice parent_obj;
>>
>>      MemoryRegion iomem;
>> +    SDBus sdbus;
>>      SDState *card;
> 
> Shouludn't card be removed?

Not yet :( It is still used by sd_set_cb() in pl181_reset().

In my first approach [1] I added the SDBus SLAVE/MASTER interfaces and
the cards inserted/readonly signals were only accessible by the bus, not
the HCI, leaving the SDCard objects only pluggable to SDBus (removing
the sdbus_reparent_card() need). But since it was out of scope for the
UHS cards goal, I kept it for later.

[1]
http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02318.html

> 
> Alistair
> 
>>      uint32_t clock;
>>      uint32_t power;
>> @@ -179,7 +180,7 @@ static void pl181_send_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) {
>> @@ -223,12 +224,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) {
>> @@ -249,7 +250,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;
>>              }
>>          }
>> @@ -480,10 +481,6 @@ static void pl181_reset(DeviceState *d)
>>
>>      /* We can assume our GPIO outputs have been wired up now */
>>      sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
>> -    /* Since we're still using the legacy SD API the card is not plugged
>> -     * into any bus, and we must reset it manually.
>> -     */
>> -    device_reset(DEVICE(s->card));
>>  }
>>
>>  static void pl181_init(Object *obj)
>> @@ -502,14 +499,26 @@ static void pl181_init(Object *obj)
>>  static void pl181_realize(DeviceState *dev, Error **errp)
>>  {
>>      PL181State *s = PL181(dev);
>> +    DeviceState *carddev;
>>      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);
>> -    s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>> -    if (s->card == NULL) {
>> -        error_setg(errp, "sd_init failed");
>> +    carddev = qdev_create(&s->sdbus.qbus, TYPE_SD_CARD);
>> +    if (dinfo) {
>> +        qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo), &err);
>> +    }
>> +    object_property_set_bool(OBJECT(carddev), true, "realized", &err);
>> +    if (err) {
>> +        error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
>> +        return;
>>      }
>> +    s->card = SD_CARD(carddev);
>>  }
>>
>>  static void pl181_class_init(ObjectClass *klass, void *data)
>> --
>> 2.15.1
>>
>>

Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Peter Maydell]

On 6 February 2018 at 12:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Alistair,
>
> On 01/31/2018 01:41 PM, Alistair Francis wrote:
>> On Mon, Jan 22, 2018 at 7:58 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> using the sdbus_*() API.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/pl181.c | 31 ++++++++++++++++++++-----------
>>>  1 file changed, 20 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
>>> index 3ba1f7dd23..ce696c5d7d 100644
>>> --- a/hw/sd/pl181.c
>>> +++ b/hw/sd/pl181.c
>>> @@ -33,6 +33,7 @@ typedef struct PL181State {
>>>      SysBusDevice parent_obj;
>>>
>>>      MemoryRegion iomem;
>>> +    SDBus sdbus;
>>>      SDState *card;
>>
>> Shouludn't card be removed?
>
> Not yet :( It is still used by sd_set_cb() in pl181_reset().

I think you have to change that sd_set_cb() code now.
If you look at sd_cardchange() it uses "is this SD card
object on an SDBus" to determine whether to notify the
controller via the old-API IRQ lines, or using the
set_inserted() and set_readonly() callbacks on the SDBusClass.

> In my first approach [1] I added the SDBus SLAVE/MASTER interfaces and
> the cards inserted/readonly signals were only accessible by the bus, not
> the HCI, leaving the SDCard objects only pluggable to SDBus (removing
> the sdbus_reparent_card() need). But since it was out of scope for the
> UHS cards goal, I kept it for later.

How do you manage to get rid of sdbus_reparent_card()? Raspi
needs it for its weirdo multiplexed SD controller setup, and
AFAIK we don't have a way to say "this thing is hotpluggable
but not by the user" yet...

PS: have you checked that these sd card refactorings don't
accidentally break the monitor "change" and "eject" commands
operating on SD cards ? (They are a bit weird because they
affect which backing file is attached to the SD card object,
rather than actually deleting and recreating the SD card
object.)

thanks
-- PMM

Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/06/2018 10:06 AM, Peter Maydell wrote:
> On 6 February 2018 at 12:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Alistair,
>>
>> On 01/31/2018 01:41 PM, Alistair Francis wrote:
>>> On Mon, Jan 22, 2018 at 7:58 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> using the sdbus_*() API.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/sd/pl181.c | 31 ++++++++++++++++++++-----------
>>>>  1 file changed, 20 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
>>>> index 3ba1f7dd23..ce696c5d7d 100644
>>>> --- a/hw/sd/pl181.c
>>>> +++ b/hw/sd/pl181.c
>>>> @@ -33,6 +33,7 @@ typedef struct PL181State {
>>>>      SysBusDevice parent_obj;
>>>>
>>>>      MemoryRegion iomem;
>>>> +    SDBus sdbus;
>>>>      SDState *card;
>>>
>>> Shouludn't card be removed?
>>
>> Not yet :( It is still used by sd_set_cb() in pl181_reset().
> 
> I think you have to change that sd_set_cb() code now.
> If you look at sd_cardchange() it uses "is this SD card
> object on an SDBus" to determine whether to notify the
> controller via the old-API IRQ lines, or using the
> set_inserted() and set_readonly() callbacks on the SDBusClass.

Oh, this can get simplified, cool!

>> In my first approach [1] I added the SDBus SLAVE/MASTER interfaces and
>> the cards inserted/readonly signals were only accessible by the bus, not
>> the HCI, leaving the SDCard objects only pluggable to SDBus (removing
>> the sdbus_reparent_card() need). But since it was out of scope for the
>> UHS cards goal, I kept it for later.
> 
> How do you manage to get rid of sdbus_reparent_card()? Raspi
> needs it for its weirdo multiplexed SD controller setup, and
> AFAIK we don't have a way to say "this thing is hotpluggable
> but not by the user" yet...

The card is hotpluggable, the bus isn't.

An unique SDBus is created in the bcm2835_peripherals object (model
closer than hardware), sdhci/sdhost/gpio controllers use it via property
links.
What I don't do is checking the gpio selector (sd_fsel), hoping the
guest isn't nasty enough to use both SD controllers at once...

> PS: have you checked that these sd card refactorings don't
> accidentally break the monitor "change" and "eject" commands
> operating on SD cards ? (They are a bit weird because they
> affect which backing file is attached to the SD card object,
> rather than actually deleting and recreating the SD card
> object.)

Nop, but I can now add a qtest for this :)

Regards,

Phil.

Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/6] hw/sd/pl181: expose a SDBus and connect the SDCard to it

[Peter Maydell]

On 6 February 2018 at 13:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 02/06/2018 10:06 AM, Peter Maydell wrote:
>> How do you manage to get rid of sdbus_reparent_card()? Raspi
>> needs it for its weirdo multiplexed SD controller setup, and
>> AFAIK we don't have a way to say "this thing is hotpluggable
>> but not by the user" yet...
>
> The card is hotpluggable, the bus isn't.
>
> An unique SDBus is created in the bcm2835_peripherals object (model
> closer than hardware), sdhci/sdhost/gpio controllers use it via property
> links.
> What I don't do is checking the gpio selector (sd_fsel), hoping the
> guest isn't nasty enough to use both SD controllers at once...

We do need to honour the sd_fsel -- otherwise newer Linux rpi
kernels don't boot, because they want to use the non-default
SD controller and they select it accordingly. At the moment
this works using sdbus_reparent_card(). (I do vaguely recall
looking at whether this could be done using hotplug but I
forget why I decided it was a bad idea.)

thanks
-- PMM