[Qemu-devel] [PATCH 00/14] SDHCI housekeeping

[Qemu-devel] [PATCH 00/14] SDHCI housekeeping

[Philippe Mathieu-Daudé]

Hi,

This series refactor the SDHCI codebase to ease further development/series.

- 1: we restrict part of "sd/sd.h" into local "sd-internal.h",
- 2-5,13: we somehow beautiful the code, no logical changes,
- 6-9: we refactor the common sysbus/pci qdev code,
- 10-12: we add plenty of trace events which will result useful later,
- 14: we finally expose a "dma-memory" property.

Regards,

Phil.

Based-on: 20171213051736.17755-5-f4bug@amsat.org
          (Trivial changes in "registerfields.h")

Philippe Mathieu-Daudé (14):
  sd: split "sd-internal.h" of "hw/sd/sd.h"
  sdhci: clean up includes
  sdhci: use the ldst_le_dma API
  sdhci: use deposit64()
  sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"
  sdhci: refactor same sysbus/pci properties into a common one
  sdhci: refactor common sysbus/pci realize() into sdhci_realizefn()
  sdhci: refactor common sysbus/pci class_init() into sdhci_class_init()
  sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()
  sdhci: use qemu_log_mask(UNIMP) instead of fprintf()
  sdhci: convert the DPRINT() calls into trace events
  sdhci: add a trace event for the LED control
  sdhci: add sdhci_init_capareg() to initialize the CAPAB register
  sdhci: add a "dma-memory" property

 include/hw/sd/sd.h        |  95 ++-----------------
 include/hw/sd/sdhci.h     |   6 +-
 hw/sd/sd-internal.h       | 119 +++++++++++++++++++++++
 hw/sd/sdhci-internal.h    |   5 +-
 hw/sd/core.c              |   3 +-
 hw/sd/milkymist-memcard.c |   2 +-
 hw/sd/omap_mmc.c          |   1 +
 hw/sd/pl181.c             |   2 +-
 hw/sd/pxa2xx_mmci.c       |   1 +
 hw/sd/sd.c                |   6 +-
 hw/sd/sdhci.c             | 235 ++++++++++++++++++++++++----------------------
 hw/sd/ssi-sd.c            |   2 +-
 hw/sd/trace-events        |  15 +++
 13 files changed, 276 insertions(+), 216 deletions(-)
 create mode 100644 hw/sd/sd-internal.h

-- 
2.15.1

[Qemu-devel] [PATCH 01/14] sd: split "sd-internal.h" of "hw/sd/sd.h"

[Philippe Mathieu-Daudé]

Now only SD 'producers' are able to use the "sd-internal.h" API,
while SD 'consumers' are restricted to the "hw/sd/sd.h" 'public' API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd-internal.h       | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/sd/sd.h        |  95 +++---------------------------------
 hw/sd/core.c              |   3 +-
 hw/sd/milkymist-memcard.c |   2 +-
 hw/sd/omap_mmc.c          |   1 +
 hw/sd/pl181.c             |   2 +-
 hw/sd/pxa2xx_mmci.c       |   1 +
 hw/sd/sd.c                |   6 +--
 hw/sd/sdhci.c             |   2 +-
 hw/sd/ssi-sd.c            |   2 +-
 10 files changed, 135 insertions(+), 98 deletions(-)
 create mode 100644 hw/sd/sd-internal.h

diff --git a/hw/sd/sd-internal.h b/hw/sd/sd-internal.h
new file mode 100644
index 0000000000..afd5dbf194
--- /dev/null
+++ b/hw/sd/sd-internal.h
@@ -0,0 +1,119 @@
+/*
+ * SD Memory Card emulation.
+ *
+ * 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 SD_INTERNAL_H
+#define SD_INTERNAL_H
+
+#include "hw/qdev.h"
+#include "sysemu/block-backend.h"
+
+#define OUT_OF_RANGE        (1 << 31)
+#define ADDRESS_ERROR       (1 << 30)
+#define BLOCK_LEN_ERROR     (1 << 29)
+#define ERASE_SEQ_ERROR     (1 << 28)
+#define ERASE_PARAM         (1 << 27)
+#define WP_VIOLATION        (1 << 26)
+#define CARD_IS_LOCKED      (1 << 25)
+#define LOCK_UNLOCK_FAILED  (1 << 24)
+#define COM_CRC_ERROR       (1 << 23)
+#define ILLEGAL_COMMAND     (1 << 22)
+#define CARD_ECC_FAILED     (1 << 21)
+#define CC_ERROR            (1 << 20)
+#define SD_ERROR            (1 << 19)
+#define CID_CSD_OVERWRITE   (1 << 16)
+#define WP_ERASE_SKIP       (1 << 15)
+#define CARD_ECC_DISABLED   (1 << 14)
+#define ERASE_RESET         (1 << 13)
+#define CURRENT_STATE       (7 << 9)
+#define READY_FOR_DATA      (1 << 8)
+#define APP_CMD             (1 << 5)
+#define AKE_SEQ_ERROR       (1 << 3)
+
+#define OCR_CCS_BITN        30
+
+typedef enum {
+    sd_none = -1,
+    sd_bc = 0,      /* broadcast -- no response */
+    sd_bcr,         /* broadcast with response */
+    sd_ac,          /* addressed -- no data transfer */
+    sd_adtc,        /* addressed with data transfer */
+} sd_cmd_type_t;
+
+typedef struct SDState SDState;
+
+#define SD_CARD_CLASS(klass) \
+    OBJECT_CLASS_CHECK(SDCardClass, (klass), TYPE_SD_CARD)
+#define SD_CARD_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(SDCardClass, (obj), TYPE_SD_CARD)
+
+typedef struct {
+    /*< private >*/
+    DeviceClass parent_class;
+    /*< public >*/
+
+    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
+    void (*write_data)(SDState *sd, uint8_t value);
+    uint8_t (*read_data)(SDState *sd);
+    bool (*data_ready)(SDState *sd);
+    void (*enable)(SDState *sd, bool enable);
+    bool (*get_inserted)(SDState *sd);
+    bool (*get_readonly)(SDState *sd);
+} SDCardClass;
+
+#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
+#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
+
+typedef struct {
+    /*< private >*/
+    BusClass parent_class;
+    /*< public >*/
+
+    /* These methods are called by the SD device to notify the controller
+     * when the card insertion or readonly status changes
+     */
+    void (*set_inserted)(DeviceState *dev, bool inserted);
+    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);
+
+#endif
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 96caefe373..f6994e61f2 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -32,108 +32,25 @@
 
 #include "hw/qdev.h"
 
-#define OUT_OF_RANGE		(1 << 31)
-#define ADDRESS_ERROR		(1 << 30)
-#define BLOCK_LEN_ERROR		(1 << 29)
-#define ERASE_SEQ_ERROR		(1 << 28)
-#define ERASE_PARAM		(1 << 27)
-#define WP_VIOLATION		(1 << 26)
-#define CARD_IS_LOCKED		(1 << 25)
-#define LOCK_UNLOCK_FAILED	(1 << 24)
-#define COM_CRC_ERROR		(1 << 23)
-#define ILLEGAL_COMMAND		(1 << 22)
-#define CARD_ECC_FAILED		(1 << 21)
-#define CC_ERROR		(1 << 20)
-#define SD_ERROR		(1 << 19)
-#define CID_CSD_OVERWRITE	(1 << 16)
-#define WP_ERASE_SKIP		(1 << 15)
-#define CARD_ECC_DISABLED	(1 << 14)
-#define ERASE_RESET		(1 << 13)
-#define CURRENT_STATE		(7 << 9)
-#define READY_FOR_DATA		(1 << 8)
-#define APP_CMD			(1 << 5)
-#define AKE_SEQ_ERROR		(1 << 3)
-#define OCR_CCS_BITN        30
-
-typedef enum {
-    sd_none = -1,
-    sd_bc = 0,	/* broadcast -- no response */
-    sd_bcr,	/* broadcast with response */
-    sd_ac,	/* addressed -- no data transfer */
-    sd_adtc,	/* addressed with data transfer */
-} sd_cmd_type_t;
-
 typedef struct {
-    uint8_t cmd;
-    uint32_t arg;
-    uint8_t crc;
-} SDRequest;
+    uint8_t cmd;        /*  6 bits */
+    uint32_t arg;       /* 32 bits */
+    uint8_t crc;        /*  7 bits */
+} SDRequest;     /* total: 48 bits shifted */
 
-typedef struct SDState SDState;
 typedef struct SDBus SDBus;
 
 #define TYPE_SD_CARD "sd-card"
 #define SD_CARD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
-#define SD_CARD_CLASS(klass) \
-    OBJECT_CLASS_CHECK(SDCardClass, (klass), TYPE_SD_CARD)
-#define SD_CARD_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(SDCardClass, (obj), TYPE_SD_CARD)
-
-typedef struct {
-    /*< private >*/
-    DeviceClass parent_class;
-    /*< public >*/
-
-    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
-    void (*write_data)(SDState *sd, uint8_t value);
-    uint8_t (*read_data)(SDState *sd);
-    bool (*data_ready)(SDState *sd);
-    void (*enable)(SDState *sd, bool enable);
-    bool (*get_inserted)(SDState *sd);
-    bool (*get_readonly)(SDState *sd);
-} SDCardClass;
 
 #define TYPE_SD_BUS "sd-bus"
 #define SD_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SD_BUS)
-#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
-#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
 
 struct SDBus {
     BusState qbus;
 };
 
-typedef struct {
-    /*< private >*/
-    BusClass parent_class;
-    /*< public >*/
-
-    /* These methods are called by the SD device to notify the controller
-     * when the card insertion or readonly status changes
-     */
-    void (*set_inserted)(DeviceState *dev, bool inserted);
-    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)
- */
+/* Functions to be used by qdevified callers */
 int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
 void sdbus_write_data(SDBus *sd, uint8_t value);
 uint8_t sdbus_read_data(SDBus *sd);
@@ -154,6 +71,6 @@ void sdbus_reparent_card(SDBus *from, SDBus *to);
 
 /* Functions to be used by SD devices to report back to qdevified controllers */
 void sdbus_set_inserted(SDBus *sd, bool inserted);
-void sdbus_set_readonly(SDBus *sd, bool inserted);
+void sdbus_set_readonly(SDBus *sd, bool readonly);
 
 #endif /* HW_SD_H */
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 295dc44ab7..bd9350d21c 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -20,9 +20,8 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/qdev-core.h"
-#include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
+#include "sd-internal.h"
 
 static SDState *get_card(SDBus *sdbus)
 {
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 4008c81002..8c9bb27229 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -27,9 +27,9 @@
 #include "sysemu/sysemu.h"
 #include "trace.h"
 #include "qemu/error-report.h"
-#include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "hw/sd/sd.h"
+#include "sd-internal.h"
 
 enum {
     ENABLE_CMD_TX   = (1<<0),
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index e934cd3656..f7e42b3525 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -20,6 +20,7 @@
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sd/sd.h"
+#include "sd-internal.h"
 
 struct omap_mmc_s {
     qemu_irq irq;
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 55c8098ecd..d568b12818 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -8,10 +8,10 @@
  */
 
 #include "qemu/osdep.h"
-#include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "hw/sysbus.h"
 #include "hw/sd/sd.h"
+#include "sd-internal.h"
 #include "qemu/log.h"
 #include "qapi/error.h"
 
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 3deccf02c9..f34951e1d1 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -16,6 +16,7 @@
 #include "hw/sysbus.h"
 #include "hw/arm/pxa.h"
 #include "hw/sd/sd.h"
+#include "sd-internal.h"
 #include "hw/qdev.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 35347a5bbc..9b7dee2ec4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -32,7 +32,6 @@
 #include "qemu/osdep.h"
 #include "hw/qdev.h"
 #include "hw/hw.h"
-#include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
 #include "qapi/error.h"
 #include "qemu/bitmap.h"
@@ -40,6 +39,7 @@
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "qemu/log.h"
+#include "sd-internal.h"
 
 //#define DEBUG_SD 1
 
@@ -1466,8 +1466,8 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
             || sd_cmd_class[req->cmd & 0x3F] == 7;
 }
 
-int sd_do_command(SDState *sd, SDRequest *req,
-                  uint8_t *response) {
+int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response)
+{
     int last_state;
     sd_rsp_type_t rtype;
     int rsplen;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b064a087c9..e7cd3258a3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -24,12 +24,12 @@
 
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
 #include "qemu/timer.h"
 #include "qemu/bitops.h"
 #include "sdhci-internal.h"
+#include "sd-internal.h"
 #include "qemu/log.h"
 
 /* host controller debug messages */
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 24001dc3e6..bd0b593b6e 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -11,11 +11,11 @@
  */
 
 #include "qemu/osdep.h"
-#include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "hw/ssi/ssi.h"
 #include "hw/sd/sd.h"
 #include "qapi/error.h"
+#include "sd-internal.h"
 
 //#define DEBUG_SSI_SD 1
 
-- 
2.15.1

Re: [Qemu-devel] [PATCH 01/14] sd: split "sd-internal.h" of "hw/sd/sd.h"

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Now only SD 'producers' are able to use the "sd-internal.h" API,
> while SD 'consumers' are restricted to the "hw/sd/sd.h" 'public' API.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I don't see what this gets us. Why bother moving this code into an
internal header?

Alistair

> ---
>  hw/sd/sd-internal.h       | 119 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/sd/sd.h        |  95 +++---------------------------------
>  hw/sd/core.c              |   3 +-
>  hw/sd/milkymist-memcard.c |   2 +-
>  hw/sd/omap_mmc.c          |   1 +
>  hw/sd/pl181.c             |   2 +-
>  hw/sd/pxa2xx_mmci.c       |   1 +
>  hw/sd/sd.c                |   6 +--
>  hw/sd/sdhci.c             |   2 +-
>  hw/sd/ssi-sd.c            |   2 +-
>  10 files changed, 135 insertions(+), 98 deletions(-)
>  create mode 100644 hw/sd/sd-internal.h
>
> diff --git a/hw/sd/sd-internal.h b/hw/sd/sd-internal.h
> new file mode 100644
> index 0000000000..afd5dbf194
> --- /dev/null
> +++ b/hw/sd/sd-internal.h
> @@ -0,0 +1,119 @@
> +/*
> + * SD Memory Card emulation.
> + *
> + * 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 SD_INTERNAL_H
> +#define SD_INTERNAL_H
> +
> +#include "hw/qdev.h"
> +#include "sysemu/block-backend.h"
> +
> +#define OUT_OF_RANGE        (1 << 31)
> +#define ADDRESS_ERROR       (1 << 30)
> +#define BLOCK_LEN_ERROR     (1 << 29)
> +#define ERASE_SEQ_ERROR     (1 << 28)
> +#define ERASE_PARAM         (1 << 27)
> +#define WP_VIOLATION        (1 << 26)
> +#define CARD_IS_LOCKED      (1 << 25)
> +#define LOCK_UNLOCK_FAILED  (1 << 24)
> +#define COM_CRC_ERROR       (1 << 23)
> +#define ILLEGAL_COMMAND     (1 << 22)
> +#define CARD_ECC_FAILED     (1 << 21)
> +#define CC_ERROR            (1 << 20)
> +#define SD_ERROR            (1 << 19)
> +#define CID_CSD_OVERWRITE   (1 << 16)
> +#define WP_ERASE_SKIP       (1 << 15)
> +#define CARD_ECC_DISABLED   (1 << 14)
> +#define ERASE_RESET         (1 << 13)
> +#define CURRENT_STATE       (7 << 9)
> +#define READY_FOR_DATA      (1 << 8)
> +#define APP_CMD             (1 << 5)
> +#define AKE_SEQ_ERROR       (1 << 3)
> +
> +#define OCR_CCS_BITN        30
> +
> +typedef enum {
> +    sd_none = -1,
> +    sd_bc = 0,      /* broadcast -- no response */
> +    sd_bcr,         /* broadcast with response */
> +    sd_ac,          /* addressed -- no data transfer */
> +    sd_adtc,        /* addressed with data transfer */
> +} sd_cmd_type_t;
> +
> +typedef struct SDState SDState;
> +
> +#define SD_CARD_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(SDCardClass, (klass), TYPE_SD_CARD)
> +#define SD_CARD_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(SDCardClass, (obj), TYPE_SD_CARD)
> +
> +typedef struct {
> +    /*< private >*/
> +    DeviceClass parent_class;
> +    /*< public >*/
> +
> +    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
> +    void (*write_data)(SDState *sd, uint8_t value);
> +    uint8_t (*read_data)(SDState *sd);
> +    bool (*data_ready)(SDState *sd);
> +    void (*enable)(SDState *sd, bool enable);
> +    bool (*get_inserted)(SDState *sd);
> +    bool (*get_readonly)(SDState *sd);
> +} SDCardClass;
> +
> +#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
> +#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
> +
> +typedef struct {
> +    /*< private >*/
> +    BusClass parent_class;
> +    /*< public >*/
> +
> +    /* These methods are called by the SD device to notify the controller
> +     * when the card insertion or readonly status changes
> +     */
> +    void (*set_inserted)(DeviceState *dev, bool inserted);
> +    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);
> +
> +#endif
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 96caefe373..f6994e61f2 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -32,108 +32,25 @@
>
>  #include "hw/qdev.h"
>
> -#define OUT_OF_RANGE           (1 << 31)
> -#define ADDRESS_ERROR          (1 << 30)
> -#define BLOCK_LEN_ERROR                (1 << 29)
> -#define ERASE_SEQ_ERROR                (1 << 28)
> -#define ERASE_PARAM            (1 << 27)
> -#define WP_VIOLATION           (1 << 26)
> -#define CARD_IS_LOCKED         (1 << 25)
> -#define LOCK_UNLOCK_FAILED     (1 << 24)
> -#define COM_CRC_ERROR          (1 << 23)
> -#define ILLEGAL_COMMAND                (1 << 22)
> -#define CARD_ECC_FAILED                (1 << 21)
> -#define CC_ERROR               (1 << 20)
> -#define SD_ERROR               (1 << 19)
> -#define CID_CSD_OVERWRITE      (1 << 16)
> -#define WP_ERASE_SKIP          (1 << 15)
> -#define CARD_ECC_DISABLED      (1 << 14)
> -#define ERASE_RESET            (1 << 13)
> -#define CURRENT_STATE          (7 << 9)
> -#define READY_FOR_DATA         (1 << 8)
> -#define APP_CMD                        (1 << 5)
> -#define AKE_SEQ_ERROR          (1 << 3)
> -#define OCR_CCS_BITN        30
> -
> -typedef enum {
> -    sd_none = -1,
> -    sd_bc = 0, /* broadcast -- no response */
> -    sd_bcr,    /* broadcast with response */
> -    sd_ac,     /* addressed -- no data transfer */
> -    sd_adtc,   /* addressed with data transfer */
> -} sd_cmd_type_t;
> -
>  typedef struct {
> -    uint8_t cmd;
> -    uint32_t arg;
> -    uint8_t crc;
> -} SDRequest;
> +    uint8_t cmd;        /*  6 bits */
> +    uint32_t arg;       /* 32 bits */
> +    uint8_t crc;        /*  7 bits */
> +} SDRequest;     /* total: 48 bits shifted */
>
> -typedef struct SDState SDState;
>  typedef struct SDBus SDBus;
>
>  #define TYPE_SD_CARD "sd-card"
>  #define SD_CARD(obj) OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
> -#define SD_CARD_CLASS(klass) \
> -    OBJECT_CLASS_CHECK(SDCardClass, (klass), TYPE_SD_CARD)
> -#define SD_CARD_GET_CLASS(obj) \
> -    OBJECT_GET_CLASS(SDCardClass, (obj), TYPE_SD_CARD)
> -
> -typedef struct {
> -    /*< private >*/
> -    DeviceClass parent_class;
> -    /*< public >*/
> -
> -    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
> -    void (*write_data)(SDState *sd, uint8_t value);
> -    uint8_t (*read_data)(SDState *sd);
> -    bool (*data_ready)(SDState *sd);
> -    void (*enable)(SDState *sd, bool enable);
> -    bool (*get_inserted)(SDState *sd);
> -    bool (*get_readonly)(SDState *sd);
> -} SDCardClass;
>
>  #define TYPE_SD_BUS "sd-bus"
>  #define SD_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SD_BUS)
> -#define SD_BUS_CLASS(klass) OBJECT_CLASS_CHECK(SDBusClass, (klass), TYPE_SD_BUS)
> -#define SD_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(SDBusClass, (obj), TYPE_SD_BUS)
>
>  struct SDBus {
>      BusState qbus;
>  };
>
> -typedef struct {
> -    /*< private >*/
> -    BusClass parent_class;
> -    /*< public >*/
> -
> -    /* These methods are called by the SD device to notify the controller
> -     * when the card insertion or readonly status changes
> -     */
> -    void (*set_inserted)(DeviceState *dev, bool inserted);
> -    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)
> - */
> +/* Functions to be used by qdevified callers */
>  int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
>  void sdbus_write_data(SDBus *sd, uint8_t value);
>  uint8_t sdbus_read_data(SDBus *sd);
> @@ -154,6 +71,6 @@ void sdbus_reparent_card(SDBus *from, SDBus *to);
>
>  /* Functions to be used by SD devices to report back to qdevified controllers */
>  void sdbus_set_inserted(SDBus *sd, bool inserted);
> -void sdbus_set_readonly(SDBus *sd, bool inserted);
> +void sdbus_set_readonly(SDBus *sd, bool readonly);
>
>  #endif /* HW_SD_H */
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index 295dc44ab7..bd9350d21c 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -20,9 +20,8 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "hw/qdev-core.h"
> -#include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
> +#include "sd-internal.h"
>
>  static SDState *get_card(SDBus *sdbus)
>  {
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index 4008c81002..8c9bb27229 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -27,9 +27,9 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> -#include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/sd/sd.h"
> +#include "sd-internal.h"
>
>  enum {
>      ENABLE_CMD_TX   = (1<<0),
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index e934cd3656..f7e42b3525 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -20,6 +20,7 @@
>  #include "hw/hw.h"
>  #include "hw/arm/omap.h"
>  #include "hw/sd/sd.h"
> +#include "sd-internal.h"
>
>  struct omap_mmc_s {
>      qemu_irq irq;
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 55c8098ecd..d568b12818 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -8,10 +8,10 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/sysbus.h"
>  #include "hw/sd/sd.h"
> +#include "sd-internal.h"
>  #include "qemu/log.h"
>  #include "qapi/error.h"
>
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index 3deccf02c9..f34951e1d1 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -16,6 +16,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sd/sd.h"
> +#include "sd-internal.h"
>  #include "hw/qdev.h"
>  #include "hw/qdev-properties.h"
>  #include "qemu/error-report.h"
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 35347a5bbc..9b7dee2ec4 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -32,7 +32,6 @@
>  #include "qemu/osdep.h"
>  #include "hw/qdev.h"
>  #include "hw/hw.h"
> -#include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qapi/error.h"
>  #include "qemu/bitmap.h"
> @@ -40,6 +39,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
> +#include "sd-internal.h"
>
>  //#define DEBUG_SD 1
>
> @@ -1466,8 +1466,8 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
>              || sd_cmd_class[req->cmd & 0x3F] == 7;
>  }
>
> -int sd_do_command(SDState *sd, SDRequest *req,
> -                  uint8_t *response) {
> +int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response)
> +{
>      int last_state;
>      sd_rsp_type_t rtype;
>      int rsplen;
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index b064a087c9..e7cd3258a3 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -24,12 +24,12 @@
>
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
> -#include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "sysemu/dma.h"
>  #include "qemu/timer.h"
>  #include "qemu/bitops.h"
>  #include "sdhci-internal.h"
> +#include "sd-internal.h"
>  #include "qemu/log.h"
>
>  /* host controller debug messages */
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 24001dc3e6..bd0b593b6e 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -11,11 +11,11 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/ssi/ssi.h"
>  #include "hw/sd/sd.h"
>  #include "qapi/error.h"
> +#include "sd-internal.h"
>
>  //#define DEBUG_SSI_SD 1
>
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 01/14] sd: split "sd-internal.h" of "hw/sd/sd.h"

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:50 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> Now only SD 'producers' are able to use the "sd-internal.h" API,
>> while SD 'consumers' are restricted to the "hw/sd/sd.h" 'public' API.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> I don't see what this gets us. Why bother moving this code into an
> internal header?

This helped me to sort out the QOM design, however it is indeed not necessary.

I later use the registerfields API to add a bunch of other registers
and bitfields, so I prefer to still move the register #defines in
another internal header, else this API header get pretty unreadable.
I'll respin this series without this split.

Re: [Qemu-devel] [PATCH 01/14] sd: split "sd-internal.h" of "hw/sd/sd.h"

[Alistair Francis]

On Thu, Dec 14, 2017 at 9:59 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On Thu, Dec 14, 2017 at 2:50 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>> Now only SD 'producers' are able to use the "sd-internal.h" API,
>>> while SD 'consumers' are restricted to the "hw/sd/sd.h" 'public' API.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I don't see what this gets us. Why bother moving this code into an
>> internal header?
>
> This helped me to sort out the QOM design, however it is indeed not necessary.

If it makes development easier that is a good reason. I just don't see
in this series how it helps.

>
> I later use the registerfields API to add a bunch of other registers
> and bitfields, so I prefer to still move the register #defines in
> another internal header, else this API header get pretty unreadable.

Ah, that makes some more sense.

Alistair

> I'll respin this series without this split.
>

[Qemu-devel] [PATCH 02/14] sdhci: clean up includes

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci-internal.h | 4 ----
 include/hw/sd/sdhci.h  | 4 +++-
 hw/sd/sdhci.c          | 1 +
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 161177cf39..248fd027f9 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -24,8 +24,6 @@
 #ifndef SDHCI_INTERNAL_H
 #define SDHCI_INTERNAL_H
 
-#include "hw/sd/sdhci.h"
-
 /* R/W SDMA System Address register 0x0 */
 #define SDHC_SYSAD                     0x00
 
@@ -227,6 +225,4 @@ enum {
     sdhc_gap_write  = 2   /* SDHC stopped at block gap during write operation */
 };
 
-extern const VMStateDescription sdhci_vmstate;
-
 #endif
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 0f0c3f1e64..1b6a98d578 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -26,17 +26,19 @@
 #define SDHCI_H
 
 #include "qemu-common.h"
-#include "hw/block/block.h"
 #include "hw/pci/pci.h"
 #include "hw/sysbus.h"
 #include "hw/sd/sd.h"
 
 /* SD/MMC host controller state */
 typedef struct SDHCIState {
+    /*< private >*/
     union {
         PCIDevice pcidev;
         SysBusDevice busdev;
     };
+
+    /*< public >*/
     SDBus sdbus;
     MemoryRegion iomem;
 
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e7cd3258a3..312b167bfa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -28,6 +28,7 @@
 #include "sysemu/dma.h"
 #include "qemu/timer.h"
 #include "qemu/bitops.h"
+#include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
 #include "sd-internal.h"
 #include "qemu/log.h"
-- 
2.15.1

Re: [Qemu-devel] [PATCH 02/14] sdhci: clean up includes

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  hw/sd/sdhci-internal.h | 4 ----
>  include/hw/sd/sdhci.h  | 4 +++-
>  hw/sd/sdhci.c          | 1 +
>  3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 161177cf39..248fd027f9 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -24,8 +24,6 @@
>  #ifndef SDHCI_INTERNAL_H
>  #define SDHCI_INTERNAL_H
>
> -#include "hw/sd/sdhci.h"
> -
>  /* R/W SDMA System Address register 0x0 */
>  #define SDHC_SYSAD                     0x00
>
> @@ -227,6 +225,4 @@ enum {
>      sdhc_gap_write  = 2   /* SDHC stopped at block gap during write operation */
>  };
>
> -extern const VMStateDescription sdhci_vmstate;
> -
>  #endif
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 0f0c3f1e64..1b6a98d578 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -26,17 +26,19 @@
>  #define SDHCI_H
>
>  #include "qemu-common.h"
> -#include "hw/block/block.h"
>  #include "hw/pci/pci.h"
>  #include "hw/sysbus.h"
>  #include "hw/sd/sd.h"
>
>  /* SD/MMC host controller state */
>  typedef struct SDHCIState {
> +    /*< private >*/
>      union {
>          PCIDevice pcidev;
>          SysBusDevice busdev;
>      };
> +
> +    /*< public >*/
>      SDBus sdbus;
>      MemoryRegion iomem;
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index e7cd3258a3..312b167bfa 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/dma.h"
>  #include "qemu/timer.h"
>  #include "qemu/bitops.h"
> +#include "hw/sd/sdhci.h"
>  #include "sdhci-internal.h"
>  #include "sd-internal.h"
>  #include "qemu/log.h"
> --
> 2.15.1
>
>

[Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API

[Philippe Mathieu-Daudé]

This makes the code slightly safer, also easier to review.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 312b167bfa..e39623baba 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -603,14 +603,12 @@ typedef struct ADMADescr {
 
 static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 {
-    uint32_t adma1 = 0;
-    uint64_t adma2 = 0;
+    uint32_t adma1;
+    uint64_t adma2;
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
     switch (SDHC_DMA_TYPE(s->hostctl)) {
     case SDHC_CTRL_ADMA2_32:
-        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
-                        sizeof(adma2));
-        adma2 = le64_to_cpu(adma2);
+        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
         /* The spec does not specify endianness of descriptor table.
          * We currently assume that it is LE.
          */
@@ -620,9 +618,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dscr->incr = 8;
         break;
     case SDHC_CTRL_ADMA1_32:
-        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma1,
-                        sizeof(adma1));
-        adma1 = le32_to_cpu(adma1);
+        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
         dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
         dscr->attr = (uint8_t)extract32(adma1, 0, 7);
         dscr->incr = 4;
@@ -633,14 +629,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         }
         break;
     case SDHC_CTRL_ADMA2_64:
-        dma_memory_read(&address_space_memory, entry_addr,
-                        (uint8_t *)(&dscr->attr), 1);
-        dma_memory_read(&address_space_memory, entry_addr + 2,
-                        (uint8_t *)(&dscr->length), 2);
-        dscr->length = le16_to_cpu(dscr->length);
-        dma_memory_read(&address_space_memory, entry_addr + 4,
-                        (uint8_t *)(&dscr->addr), 8);
-        dscr->attr = le64_to_cpu(dscr->attr);
+        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
+        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
+        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
         dscr->attr &= 0xfffffff8;
         dscr->incr = 12;
         break;
-- 
2.15.1

Re: [Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> This makes the code slightly safer, also easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 312b167bfa..e39623baba 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -603,14 +603,12 @@ typedef struct ADMADescr {
>
>  static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>  {
> -    uint32_t adma1 = 0;
> -    uint64_t adma2 = 0;
> +    uint32_t adma1;
> +    uint64_t adma2;
>      hwaddr entry_addr = (hwaddr)s->admasysaddr;
>      switch (SDHC_DMA_TYPE(s->hostctl)) {
>      case SDHC_CTRL_ADMA2_32:
> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
> -                        sizeof(adma2));
> -        adma2 = le64_to_cpu(adma2);
> +        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
>          /* The spec does not specify endianness of descriptor table.
>           * We currently assume that it is LE.
>           */
> @@ -620,9 +618,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>          dscr->incr = 8;
>          break;
>      case SDHC_CTRL_ADMA1_32:
> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma1,
> -                        sizeof(adma1));
> -        adma1 = le32_to_cpu(adma1);
> +        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
>          dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
>          dscr->attr = (uint8_t)extract32(adma1, 0, 7);
>          dscr->incr = 4;
> @@ -633,14 +629,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>          }
>          break;
>      case SDHC_CTRL_ADMA2_64:
> -        dma_memory_read(&address_space_memory, entry_addr,
> -                        (uint8_t *)(&dscr->attr), 1);
> -        dma_memory_read(&address_space_memory, entry_addr + 2,
> -                        (uint8_t *)(&dscr->length), 2);
> -        dscr->length = le16_to_cpu(dscr->length);
> -        dma_memory_read(&address_space_memory, entry_addr + 4,
> -                        (uint8_t *)(&dscr->addr), 8);
> -        dscr->attr = le64_to_cpu(dscr->attr);

> +        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
> +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
> +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);

Why this is being overwritten?

It looks like you have dropped an addr as well.

Alistair

>          dscr->attr &= 0xfffffff8;
>          dscr->incr = 12;
>          break;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:23 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> This makes the code slightly safer, also easier to review.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sdhci.c | 23 +++++++----------------
>>  1 file changed, 7 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 312b167bfa..e39623baba 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -603,14 +603,12 @@ typedef struct ADMADescr {
>>
>>  static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>  {
>> -    uint32_t adma1 = 0;
>> -    uint64_t adma2 = 0;
>> +    uint32_t adma1;
>> +    uint64_t adma2;
>>      hwaddr entry_addr = (hwaddr)s->admasysaddr;
>>      switch (SDHC_DMA_TYPE(s->hostctl)) {
>>      case SDHC_CTRL_ADMA2_32:
>> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
>> -                        sizeof(adma2));
>> -        adma2 = le64_to_cpu(adma2);
>> +        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
>>          /* The spec does not specify endianness of descriptor table.
>>           * We currently assume that it is LE.
>>           */
>> @@ -620,9 +618,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>          dscr->incr = 8;
>>          break;
>>      case SDHC_CTRL_ADMA1_32:
>> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma1,
>> -                        sizeof(adma1));
>> -        adma1 = le32_to_cpu(adma1);
>> +        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
>>          dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
>>          dscr->attr = (uint8_t)extract32(adma1, 0, 7);
>>          dscr->incr = 4;
>> @@ -633,14 +629,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>          }
>>          break;
>>      case SDHC_CTRL_ADMA2_64:
>> -        dma_memory_read(&address_space_memory, entry_addr,
>> -                        (uint8_t *)(&dscr->attr), 1);
>> -        dma_memory_read(&address_space_memory, entry_addr + 2,
>> -                        (uint8_t *)(&dscr->length), 2);
>> -        dscr->length = le16_to_cpu(dscr->length);
>> -        dma_memory_read(&address_space_memory, entry_addr + 4,
>> -                        (uint8_t *)(&dscr->addr), 8);
>> -        dscr->attr = le64_to_cpu(dscr->attr);
>
>> +        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
>> +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
>> +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
>
> Why this is being overwritten?

Do you mean 'rewritten'?

> It looks like you have dropped an addr as well.

Which addr? The diff is a bit compressed, if you separate it is more obvious:

 -        dma_memory_read(&address_space_memory, entry_addr,
 -                        (uint8_t *)(&dscr->attr), 1);
 +        dscr->attr = ldub_dma(&address_space_memory, entry_addr);


 -        dma_memory_read(&address_space_memory, entry_addr + 2,
 -                        (uint8_t *)(&dscr->length), 2);
 -        dscr->length = le16_to_cpu(dscr->length);
 +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);


 -        dma_memory_read(&address_space_memory, entry_addr + 4,
 -                        (uint8_t *)(&dscr->addr), 8);
 -        dscr->attr = le64_to_cpu(dscr->attr);
 +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);

The API is detailled in this file: docs/devel/loads-stores.rst

I decided to rewrite this code with this API because it looks easier
to understand (maybe once you read the API doc), so easier to review
:)

Re: [Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API

[Alistair Francis]

On Thu, Dec 14, 2017 at 3:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On Thu, Dec 14, 2017 at 2:23 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>> This makes the code slightly safer, also easier to review.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sdhci.c | 23 +++++++----------------
>>>  1 file changed, 7 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 312b167bfa..e39623baba 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -603,14 +603,12 @@ typedef struct ADMADescr {
>>>
>>>  static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>>  {
>>> -    uint32_t adma1 = 0;
>>> -    uint64_t adma2 = 0;
>>> +    uint32_t adma1;
>>> +    uint64_t adma2;
>>>      hwaddr entry_addr = (hwaddr)s->admasysaddr;
>>>      switch (SDHC_DMA_TYPE(s->hostctl)) {
>>>      case SDHC_CTRL_ADMA2_32:
>>> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
>>> -                        sizeof(adma2));
>>> -        adma2 = le64_to_cpu(adma2);
>>> +        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
>>>          /* The spec does not specify endianness of descriptor table.
>>>           * We currently assume that it is LE.
>>>           */
>>> @@ -620,9 +618,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>>          dscr->incr = 8;
>>>          break;
>>>      case SDHC_CTRL_ADMA1_32:
>>> -        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma1,
>>> -                        sizeof(adma1));
>>> -        adma1 = le32_to_cpu(adma1);
>>> +        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
>>>          dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
>>>          dscr->attr = (uint8_t)extract32(adma1, 0, 7);
>>>          dscr->incr = 4;
>>> @@ -633,14 +629,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>>>          }
>>>          break;
>>>      case SDHC_CTRL_ADMA2_64:
>>> -        dma_memory_read(&address_space_memory, entry_addr,
>>> -                        (uint8_t *)(&dscr->attr), 1);
>>> -        dma_memory_read(&address_space_memory, entry_addr + 2,
>>> -                        (uint8_t *)(&dscr->length), 2);
>>> -        dscr->length = le16_to_cpu(dscr->length);
>>> -        dma_memory_read(&address_space_memory, entry_addr + 4,
>>> -                        (uint8_t *)(&dscr->addr), 8);
>>> -        dscr->attr = le64_to_cpu(dscr->attr);
>>
>>> +        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
>>> +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
>>> +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
>>
>> Why this is being overwritten?
>
> Do you mean 'rewritten'?
>
>> It looks like you have dropped an addr as well.
>
> Which addr? The diff is a bit compressed, if you separate it is more obvious:
>
>  -        dma_memory_read(&address_space_memory, entry_addr,
>  -                        (uint8_t *)(&dscr->attr), 1);
>  +        dscr->attr = ldub_dma(&address_space_memory, entry_addr);

This is rewritten later, why bother setting this here?

>
>
>  -        dma_memory_read(&address_space_memory, entry_addr + 2,
>  -                        (uint8_t *)(&dscr->length), 2);
>  -        dscr->length = le16_to_cpu(dscr->length);
>  +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
>
>
>  -        dma_memory_read(&address_space_memory, entry_addr + 4,
>  -                        (uint8_t *)(&dscr->addr), 8);

What about dscr->addr which is set here?

Alistair

>  -        dscr->attr = le64_to_cpu(dscr->attr);
>  +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
>
> The API is detailled in this file: docs/devel/loads-stores.rst
>
> I decided to rewrite this code with this API because it looks easier
> to understand (maybe once you read the API doc), so easier to review
> :)
>

Re: [Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API

[Philippe Mathieu-Daudé]

> This is rewritten later, why bother setting this here?
>
>>
>>
>>  -        dma_memory_read(&address_space_memory, entry_addr + 2,
>>  -                        (uint8_t *)(&dscr->length), 2);
>>  -        dscr->length = le16_to_cpu(dscr->length);
>>  +        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
>>
>>
>>  -        dma_memory_read(&address_space_memory, entry_addr + 4,
>>  -                        (uint8_t *)(&dscr->addr), 8);
>
> What about dscr->addr which is set here?

Oops. I'll drop this patch.

>>  -        dscr->attr = le64_to_cpu(dscr->attr);
>>  +        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
>>
>> The API is detailled in this file: docs/devel/loads-stores.rst

[Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Philippe Mathieu-Daudé]

This makes the code slightly safer, also easier to review.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e39623baba..295a89e5d3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         MASKED_WRITE(s->admaerr, mask, value);
         break;
     case SDHC_ADMASYSADDR:
-        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
-                (uint64_t)mask)) | (uint64_t)value;
+        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
         break;
     case SDHC_ADMASYSADDR + 4:
-        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
-                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
+        s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
         break;
     case SDHC_FEAER:
         s->acmd12errsts |= value;
-- 
2.15.1

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> This makes the code slightly safer, also easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index e39623baba..295a89e5d3 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          MASKED_WRITE(s->admaerr, mask, value);
>          break;
>      case SDHC_ADMASYSADDR:
> -        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
> -                (uint64_t)mask)) | (uint64_t)value;
> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);

This doesn't look right.

Originally we were masking admasysaddr with (mask and
0xFFFFFFFF00000000). Then ORing in the value.

Now we are depositing value into a bit field that starts at bit 32 and
has 0 length. I don't see how value will be deposited at all with a 0
length.

Alistair

>          break;
>      case SDHC_ADMASYSADDR + 4:
> -        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
> -                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
> +        s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
>          break;
>      case SDHC_FEAER:
>          s->acmd12errsts |= value;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Philippe Mathieu-Daudé]

>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>          MASKED_WRITE(s->admaerr, mask, value);
>>          break;
>>      case SDHC_ADMASYSADDR:
>> -        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
>> -                (uint64_t)mask)) | (uint64_t)value;
>> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
>
> This doesn't look right.
>
> Originally we were masking admasysaddr with (mask and
> 0xFFFFFFFF00000000). Then ORing in the value.
>
> Now we are depositing value into a bit field that starts at bit 32 and
> has 0 length. I don't see how value will be deposited at all with a 0
> length.

good catch :) I'll respin with:

    case SDHC_ADMASYSADDR:
        s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
        break;
    case SDHC_ADMASYSADDR + 4:
        s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
        break;

>>          break;
>>      case SDHC_ADMASYSADDR + 4:
>> -        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
>> -                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
>> +        s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
>>          break;
>>      case SDHC_FEAER:
>>          s->acmd12errsts |= value;

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Alistair Francis]

On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>          MASKED_WRITE(s->admaerr, mask, value);
>>>          break;
>>>      case SDHC_ADMASYSADDR:
>>> -        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
>>> -                (uint64_t)mask)) | (uint64_t)value;
>>> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
>>
>> This doesn't look right.
>>
>> Originally we were masking admasysaddr with (mask and
>> 0xFFFFFFFF00000000). Then ORing in the value.
>>
>> Now we are depositing value into a bit field that starts at bit 32 and
>> has 0 length. I don't see how value will be deposited at all with a 0
>> length.
>
> good catch :) I'll respin with:
>
>     case SDHC_ADMASYSADDR:
>         s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
>         break;
>     case SDHC_ADMASYSADDR + 4:
>         s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>         break;

This still doesn't take the mask value into account though.

Also, doesn't deposit() shift value up in this case? We want to mask
the low bits out. I don't have the code in front of me though, so I
could be wrong here.

Alistair

>
>>>          break;
>>>      case SDHC_ADMASYSADDR + 4:
>>> -        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
>>> -                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
>>> +        s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
>>>          break;
>>>      case SDHC_FEAER:
>>>          s->acmd12errsts |= value;
>

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>>          MASKED_WRITE(s->admaerr, mask, value);
>>>>          break;
>>>>      case SDHC_ADMASYSADDR:
>>>> -        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
>>>> -                (uint64_t)mask)) | (uint64_t)value;
>>>> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
>>>
>>> This doesn't look right.
>>>
>>> Originally we were masking admasysaddr with (mask and
>>> 0xFFFFFFFF00000000). Then ORing in the value.
>>>
>>> Now we are depositing value into a bit field that starts at bit 32 and
>>> has 0 length. I don't see how value will be deposited at all with a 0
>>> length.
>>
>> good catch :) I'll respin with:
>>
>>     case SDHC_ADMASYSADDR:
>>         s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
>>         break;
>>     case SDHC_ADMASYSADDR + 4:
>>         s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>>         break;
>
> This still doesn't take the mask value into account though.
>
> Also, doesn't deposit() shift value up in this case? We want to mask
> the low bits out. I don't have the code in front of me though, so I
> could be wrong here.

We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits.

Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit.

/**
 * Deposit @fieldval into the 64 bit @value at the bit field specified
 * by the @start and @length parameters, and return the modified
 * @value. Bits of @value outside the bit field are not modified.

uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval);

in both access we use length=32

at SDHC_ADMASYSADDR we use start=0,
while at SDHC_ADMASYSADDR + 4 we use start=32.

both deposit the 32b value (32b masked) into a 64b s->admasysaddr.

This is good to clarify this now, because the Spec v3 series (and
v4.20 if we want it) add a lot of them.

Regards,

Phil.

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Alistair Francis]

On Thu, Dec 14, 2017 at 4:07 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>>>          MASKED_WRITE(s->admaerr, mask, value);
>>>>>          break;
>>>>>      case SDHC_ADMASYSADDR:
>>>>> -        s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
>>>>> -                (uint64_t)mask)) | (uint64_t)value;
>>>>> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
>>>>
>>>> This doesn't look right.
>>>>
>>>> Originally we were masking admasysaddr with (mask and
>>>> 0xFFFFFFFF00000000). Then ORing in the value.
>>>>
>>>> Now we are depositing value into a bit field that starts at bit 32 and
>>>> has 0 length. I don't see how value will be deposited at all with a 0
>>>> length.
>>>
>>> good catch :) I'll respin with:
>>>
>>>     case SDHC_ADMASYSADDR:
>>>         s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
>>>         break;
>>>     case SDHC_ADMASYSADDR + 4:
>>>         s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>>>         break;
>>
>> This still doesn't take the mask value into account though.
>>
>> Also, doesn't deposit() shift value up in this case? We want to mask
>> the low bits out. I don't have the code in front of me though, so I
>> could be wrong here.
>
> We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits.

Ah! Good point.

>
> Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit.
>
> /**
>  * Deposit @fieldval into the 64 bit @value at the bit field specified
>  * by the @start and @length parameters, and return the modified
>  * @value. Bits of @value outside the bit field are not modified.
>
> uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval);
>
> in both access we use length=32
>
> at SDHC_ADMASYSADDR we use start=0,
> while at SDHC_ADMASYSADDR + 4 we use start=32.
>
> both deposit the 32b value (32b masked) into a 64b s->admasysaddr.
>
> This is good to clarify this now, because the Spec v3 series (and
> v4.20 if we want it) add a lot of them.

Ok, this sounds fine to me then.

The mask variable is still being ignored though. value should be anded
with mask.

Alistair

>
> Regards,
>
> Phil.
>

Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()

[Philippe Mathieu-Daudé]

>>>> good catch :) I'll respin with:
>>>>
>>>>     case SDHC_ADMASYSADDR:
>>>>         s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
>>>>         break;
>>>>     case SDHC_ADMASYSADDR + 4:
>>>>         s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>>>>         break;
>>>
>>> This still doesn't take the mask value into account though.
>>>
>>> Also, doesn't deposit() shift value up in this case? We want to mask
>>> the low bits out. I don't have the code in front of me though, so I
>>> could be wrong here.
>>
>> We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits.
>
> Ah! Good point.
>
>> Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit.
>>
>> /**
>>  * Deposit @fieldval into the 64 bit @value at the bit field specified
>>  * by the @start and @length parameters, and return the modified
>>  * @value. Bits of @value outside the bit field are not modified.
>>
>> uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval);
>>
>> in both access we use length=32
>>
>> at SDHC_ADMASYSADDR we use start=0,
>> while at SDHC_ADMASYSADDR + 4 we use start=32.
>>
>> both deposit the 32b value (32b masked) into a 64b s->admasysaddr.
>>
>> This is good to clarify this now, because the Spec v3 series (and
>> v4.20 if we want it) add a lot of them.
>
> Ok, this sounds fine to me then.
>
> The mask variable is still being ignored though. value should be anded
> with mask.

This is what deposit64() does:

uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval)
{
    uint64_t mask;
    assert(start >= 0 && length > 0 && length <= 64 - start);
    mask = (~0ULL >> (64 - length)) << start;
    return (value & ~mask) | ((fieldval << start) & mask);
}

[Qemu-devel] [PATCH 05/14] sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci-internal.h | 1 +
 hw/sd/sdhci.c          | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 248fd027f9..e941bc2386 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -43,6 +43,7 @@
 #define SDHC_TRNS_ACMD12               0x0004
 #define SDHC_TRNS_READ                 0x0010
 #define SDHC_TRNS_MULTI                0x0020
+#define SDHC_TRNMOD_MASK               0x0037
 
 /* R/W Command Register 0x0 */
 #define SDHC_CMDREG                    0x0E
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 295a89e5d3..a56c0c273e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -120,7 +120,6 @@
     (SDHC_CAPAB_BASECLKFREQ << 8) | (SDHC_CAPAB_TOUNIT << 7) | \
     (SDHC_CAPAB_TOCLKFREQ))
 
-#define MASK_TRNMOD     0x0037
 #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
 
 static uint8_t sdhci_slotint(SDHCIState *s)
@@ -1043,7 +1042,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         if (!(s->capareg & SDHC_CAN_DO_DMA)) {
             value &= ~SDHC_TRNS_DMA;
         }
-        MASKED_WRITE(s->trnmod, mask, value & MASK_TRNMOD);
+        MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK);
         MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
 
         /* Writing to the upper byte of CMDREG triggers SD command generation */
-- 
2.15.1

Re: [Qemu-devel] [PATCH 05/14] sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  hw/sd/sdhci-internal.h | 1 +
>  hw/sd/sdhci.c          | 3 +--
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 248fd027f9..e941bc2386 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -43,6 +43,7 @@
>  #define SDHC_TRNS_ACMD12               0x0004
>  #define SDHC_TRNS_READ                 0x0010
>  #define SDHC_TRNS_MULTI                0x0020
> +#define SDHC_TRNMOD_MASK               0x0037
>
>  /* R/W Command Register 0x0 */
>  #define SDHC_CMDREG                    0x0E
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 295a89e5d3..a56c0c273e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -120,7 +120,6 @@
>      (SDHC_CAPAB_BASECLKFREQ << 8) | (SDHC_CAPAB_TOUNIT << 7) | \
>      (SDHC_CAPAB_TOCLKFREQ))
>
> -#define MASK_TRNMOD     0x0037
>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>
>  static uint8_t sdhci_slotint(SDHCIState *s)
> @@ -1043,7 +1042,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          if (!(s->capareg & SDHC_CAN_DO_DMA)) {
>              value &= ~SDHC_TRNS_DMA;
>          }
> -        MASKED_WRITE(s->trnmod, mask, value & MASK_TRNMOD);
> +        MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK);
>          MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
>
>          /* Writing to the upper byte of CMDREG triggers SD command generation */
> --
> 2.15.1
>
>

[Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

add sysbus/pci/sdbus separator comments to keep it clearer

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index a56c0c273e..c8b7b1ca4c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1256,13 +1256,17 @@ const VMStateDescription sdhci_vmstate = {
 
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
-static Property sdhci_pci_properties[] = {
+static Property sdhci_properties[] = {
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* --- qdev PCI --- */
+
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
     SDHCIState *s = PCI_SDHCI(dev);
@@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_pci_properties;
+    dc->props = sdhci_properties;
     dc->reset = sdhci_poweron_reset;
 }
 
@@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info = {
     },
 };
 
-static Property sdhci_sysbus_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
-            SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
-                     false),
-    DEFINE_PROP_END_OF_LIST(),
-};
+/* --- qdev SysBus --- */
 
 static void sdhci_sysbus_init(Object *obj)
 {
@@ -1350,7 +1347,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_sysbus_properties;
+    dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
     dc->reset = sdhci_poweron_reset;
 }
@@ -1364,6 +1361,8 @@ static const TypeInfo sdhci_sysbus_info = {
     .class_init = sdhci_sysbus_class_init,
 };
 
+/* --- qdev bus master --- */
+
 static void sdhci_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);
-- 
2.15.1

Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> add sysbus/pci/sdbus separator comments to keep it clearer
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index a56c0c273e..c8b7b1ca4c 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1256,13 +1256,17 @@ const VMStateDescription sdhci_vmstate = {
>
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
> -static Property sdhci_pci_properties[] = {
> +static Property sdhci_properties[] = {
>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> +                     false),

I like the reduction of code in this patch, but aren't we now going to
have device properties that aren't actually connected to anything?

Alistair

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +/* --- qdev PCI --- */
> +
>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>  {
>      SDHCIState *s = PCI_SDHCI(dev);
> @@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_pci_properties;
> +    dc->props = sdhci_properties;
>      dc->reset = sdhci_poweron_reset;
>  }
>
> @@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info = {
>      },
>  };
>
> -static Property sdhci_sysbus_properties[] = {
> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
> -            SDHC_CAPAB_REG_DEFAULT),
> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> -                     false),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> +/* --- qdev SysBus --- */
>
>  static void sdhci_sysbus_init(Object *obj)
>  {
> @@ -1350,7 +1347,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_sysbus_properties;
> +    dc->props = sdhci_properties;
>      dc->realize = sdhci_sysbus_realize;
>      dc->reset = sdhci_poweron_reset;
>  }
> @@ -1364,6 +1361,8 @@ static const TypeInfo sdhci_sysbus_info = {
>      .class_init = sdhci_sysbus_class_init,
>  };
>
> +/* --- qdev bus master --- */
> +
>  static void sdhci_bus_class_init(ObjectClass *klass, void *data)
>  {
>      SDBusClass *sbc = SD_BUS_CLASS(klass);
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

>>  /* Capabilities registers provide information on supported features of this
>>   * specific host controller implementation */
>> -static Property sdhci_pci_properties[] = {
>> +static Property sdhci_properties[] = {
>>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>              SDHC_CAPAB_REG_DEFAULT),
>>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>> +                     false),
>
> I like the reduction of code in this patch, but aren't we now going to
> have device properties that aren't actually connected to anything?

I'm not sure I understand, ar you worried about the PCI_SDHCI will now
have this property but not use it?

I couldn't find any machine using SDHCI via PCI and was tempted to
just remove this code,
the only references to it is the REDHAT_SDHCI from commits ece5e5bfa13
and 224d10ff5ae.

Maybe an attempt to write SDHCI qtests via PCI?

>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> +/* --- qdev PCI --- */
>> +
>>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>  {
>>      SDHCIState *s = PCI_SDHCI(dev);
>> @@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>      dc->vmsd = &sdhci_vmstate;
>> -    dc->props = sdhci_pci_properties;
>> +    dc->props = sdhci_properties;
>>      dc->reset = sdhci_poweron_reset;
>>  }
>>
>> @@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info = {
>>      },
>>  };
>>
>> -static Property sdhci_sysbus_properties[] = {
>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>> -            SDHC_CAPAB_REG_DEFAULT),
>> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>> -                     false),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> +/* --- qdev SysBus --- */

Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Alistair Francis]

On Thu, Dec 14, 2017 at 10:40 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
>>>  /* Capabilities registers provide information on supported features of this
>>>   * specific host controller implementation */
>>> -static Property sdhci_pci_properties[] = {
>>> +static Property sdhci_properties[] = {
>>>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>>              SDHC_CAPAB_REG_DEFAULT),
>>>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>> +                     false),
>>
>> I like the reduction of code in this patch, but aren't we now going to
>> have device properties that aren't actually connected to anything?
>
> I'm not sure I understand, ar you worried about the PCI_SDHCI will now
> have this property but not use it?
>
> I couldn't find any machine using SDHCI via PCI and was tempted to
> just remove this code,
> the only references to it is the REDHAT_SDHCI from commits ece5e5bfa13
> and 224d10ff5ae.

So it is also possible to set these properties from the command line.

If I'm a user and I see that my device has a property "foo" I expect
that setting that property will have an affect on that device. So
there shouldn't be properties that aren't actually connected to the
device.

Alistair

>
> Maybe an attempt to write SDHCI qtests via PCI?
>
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> +/* --- qdev PCI --- */
>>> +
>>>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      SDHCIState *s = PCI_SDHCI(dev);
>>> @@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>>      dc->vmsd = &sdhci_vmstate;
>>> -    dc->props = sdhci_pci_properties;
>>> +    dc->props = sdhci_properties;
>>>      dc->reset = sdhci_poweron_reset;
>>>  }
>>>
>>> @@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info = {
>>>      },
>>>  };
>>>
>>> -static Property sdhci_sysbus_properties[] = {
>>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>> -            SDHC_CAPAB_REG_DEFAULT),
>>> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>> -                     false),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> +/* --- qdev SysBus --- */
>

Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Kevin O'Connor]

On Thu, Dec 14, 2017 at 03:40:17PM -0300, Philippe Mathieu-Daudé wrote:
> >>  /* Capabilities registers provide information on supported features of this
> >>   * specific host controller implementation */
> >> -static Property sdhci_pci_properties[] = {
> >> +static Property sdhci_properties[] = {
> >>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
> >>              SDHC_CAPAB_REG_DEFAULT),
> >>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> >> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> >> +                     false),
> >
> > I like the reduction of code in this patch, but aren't we now going to
> > have device properties that aren't actually connected to anything?
> 
> I'm not sure I understand, ar you worried about the PCI_SDHCI will now
> have this property but not use it?
> 
> I couldn't find any machine using SDHCI via PCI and was tempted to
> just remove this code,

I'm not sure if you are suggesting the removal of PCI SDHCI support or
removal of some of the properties.

I do find qemu's PCI SDHCI support useful for testing.  SeaBIOS can
launch an OS from PCI SDHCI (qemu-system-x86_64 -device sdhci-pci
-device sd-card,drive=drive0 -drive id=drive0,if=none,file=dos-drivec)
and linux has drivers for it as well.  A number of the Chromebooks
ship with PCI SDHCI devices on them, so it's not an unheard of
configuration.

I've never manually set any of the PCI properites, however.

-Kevin

Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Paolo Bonzini]

On 14/12/2017 19:40, Philippe Mathieu-Daudé wrote:
>>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>> +                     false),
>> I like the reduction of code in this patch, but aren't we now going to
>> have device properties that aren't actually connected to anything?
> I'm not sure I understand, ar you worried about the PCI_SDHCI will now
> have this property but not use it?
> 
> I couldn't find any machine using SDHCI via PCI and was tempted to
> just remove this code,

PCI devices can be created with -device, they don't have to be added by
boards.

Paolo

[Qemu-devel] [PATCH 07/14] sdhci: refactor common sysbus/pci realize() into sdhci_realizefn()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c8b7b1ca4c..a96cd8a2df 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1182,6 +1182,15 @@ static void sdhci_initfn(SDHCIState *s)
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
 }
 
+static void sdhci_realizefn(SDHCIState *s, Error **errp)
+{
+    s->buf_maxsz = sdhci_get_fifolen(s);
+    s->fifo_buffer = g_malloc0(s->buf_maxsz);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
+                          SDHC_REGISTERS_MAP_SIZE);
+}
+
 static void sdhci_uninitfn(SDHCIState *s)
 {
     timer_del(s->insert_timer);
@@ -1272,12 +1281,11 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     SDHCIState *s = PCI_SDHCI(dev);
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
+
     sdhci_initfn(s);
-    s->buf_maxsz = sdhci_get_fifolen(s);
-    s->fifo_buffer = g_malloc0(s->buf_maxsz);
+    sdhci_realizefn(s, errp);
+
     s->irq = pci_allocate_irq(dev);
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
-            SDHC_REGISTERS_MAP_SIZE);
     pci_register_bar(dev, 0, 0, &s->iomem);
 }
 
@@ -1334,11 +1342,9 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    s->buf_maxsz = sdhci_get_fifolen(s);
-    s->fifo_buffer = g_malloc0(s->buf_maxsz);
+    sdhci_realizefn(s, errp);
+
     sysbus_init_irq(sbd, &s->irq);
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
-            SDHC_REGISTERS_MAP_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
-- 
2.15.1

Re: [Qemu-devel] [PATCH 07/14] sdhci: refactor common sysbus/pci realize() into sdhci_realizefn()

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  hw/sd/sdhci.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index c8b7b1ca4c..a96cd8a2df 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1182,6 +1182,15 @@ static void sdhci_initfn(SDHCIState *s)
>      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>  }
>
> +static void sdhci_realizefn(SDHCIState *s, Error **errp)
> +{
> +    s->buf_maxsz = sdhci_get_fifolen(s);
> +    s->fifo_buffer = g_malloc0(s->buf_maxsz);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> +                          SDHC_REGISTERS_MAP_SIZE);
> +}
> +
>  static void sdhci_uninitfn(SDHCIState *s)
>  {
>      timer_del(s->insert_timer);
> @@ -1272,12 +1281,11 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>      SDHCIState *s = PCI_SDHCI(dev);
>      dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
>      dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
> +
>      sdhci_initfn(s);
> -    s->buf_maxsz = sdhci_get_fifolen(s);
> -    s->fifo_buffer = g_malloc0(s->buf_maxsz);
> +    sdhci_realizefn(s, errp);
> +
>      s->irq = pci_allocate_irq(dev);
> -    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> -            SDHC_REGISTERS_MAP_SIZE);
>      pci_register_bar(dev, 0, 0, &s->iomem);
>  }
>
> @@ -1334,11 +1342,9 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>      SDHCIState *s = SYSBUS_SDHCI(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>
> -    s->buf_maxsz = sdhci_get_fifolen(s);
> -    s->fifo_buffer = g_malloc0(s->buf_maxsz);
> +    sdhci_realizefn(s, errp);
> +
>      sysbus_init_irq(sbd, &s->irq);
> -    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> -            SDHC_REGISTERS_MAP_SIZE);
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
>
> --
> 2.15.1
>
>

[Qemu-devel] [PATCH 08/14] sdhci: refactor common sysbus/pci class_init() into sdhci_class_init()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index a96cd8a2df..893be0e606 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1274,6 +1274,16 @@ static Property sdhci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void sdhci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->vmsd = &sdhci_vmstate;
+    dc->props = sdhci_properties;
+    dc->reset = sdhci_poweron_reset;
+}
+
 /* --- qdev PCI --- */
 
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
@@ -1297,7 +1307,6 @@ static void sdhci_pci_exit(PCIDevice *dev)
 
 static void sdhci_pci_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = sdhci_pci_realize;
@@ -1305,10 +1314,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
-    dc->reset = sdhci_poweron_reset;
+
+    sdhci_class_init(klass, data);
 }
 
 static const TypeInfo sdhci_pci_info = {
@@ -1352,10 +1359,9 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
-    dc->reset = sdhci_poweron_reset;
+
+    sdhci_class_init(klass, data);
 }
 
 static const TypeInfo sdhci_sysbus_info = {
-- 
2.15.1

Re: [Qemu-devel] [PATCH 08/14] sdhci: refactor common sysbus/pci class_init() into sdhci_class_init()

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index a96cd8a2df..893be0e606 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1274,6 +1274,16 @@ static Property sdhci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static void sdhci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);

This changes the sysbus one, does that matter?

Alistair

> +    dc->vmsd = &sdhci_vmstate;
> +    dc->props = sdhci_properties;
> +    dc->reset = sdhci_poweron_reset;
> +}
> +
>  /* --- qdev PCI --- */
>
>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
> @@ -1297,7 +1307,6 @@ static void sdhci_pci_exit(PCIDevice *dev)
>
>  static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
>      k->realize = sdhci_pci_realize;
> @@ -1305,10 +1314,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>      k->vendor_id = PCI_VENDOR_ID_REDHAT;
>      k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
> -    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -    dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_properties;
> -    dc->reset = sdhci_poweron_reset;
> +
> +    sdhci_class_init(klass, data);
>  }
>
>  static const TypeInfo sdhci_pci_info = {
> @@ -1352,10 +1359,9 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
> -    dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_properties;
>      dc->realize = sdhci_sysbus_realize;
> -    dc->reset = sdhci_poweron_reset;
> +
> +    sdhci_class_init(klass, data);
>  }
>
>  static const TypeInfo sdhci_sysbus_info = {
> --
> 2.15.1
>
>

[Qemu-devel] [PATCH 09/14] sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 893be0e606..044e3d62f1 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -31,6 +31,7 @@
 #include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
 #include "sd-internal.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
 
 /* host controller debug messages */
@@ -1191,17 +1192,21 @@ static void sdhci_realizefn(SDHCIState *s, Error **errp)
                           SDHC_REGISTERS_MAP_SIZE);
 }
 
+static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
+{
+    g_free(s->fifo_buffer);
+    s->fifo_buffer = NULL;
+}
+
 static void sdhci_uninitfn(SDHCIState *s)
 {
     timer_del(s->insert_timer);
     timer_free(s->insert_timer);
     timer_del(s->transfer_timer);
     timer_free(s->transfer_timer);
+
     qemu_free_irq(s->eject_cb);
     qemu_free_irq(s->ro_cb);
-
-    g_free(s->fifo_buffer);
-    s->fifo_buffer = NULL;
 }
 
 static bool sdhci_pending_insert_vmstate_needed(void *opaque)
@@ -1302,6 +1307,8 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 static void sdhci_pci_exit(PCIDevice *dev)
 {
     SDHCIState *s = PCI_SDHCI(dev);
+
+    sdhci_unrealizefn(s, &error_abort);
     sdhci_uninitfn(s);
 }
 
@@ -1355,11 +1362,19 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
+static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
+{
+    SDHCIState *s = SYSBUS_SDHCI(dev);
+
+    sdhci_unrealizefn(s, errp);
+}
+
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = sdhci_sysbus_realize;
+    dc->unrealize = sdhci_sysbus_unrealize;
 
     sdhci_class_init(klass, data);
 }
-- 
2.15.1

Re: [Qemu-devel] [PATCH 09/14] sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 893be0e606..044e3d62f1 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -31,6 +31,7 @@
>  #include "hw/sd/sdhci.h"
>  #include "sdhci-internal.h"
>  #include "sd-internal.h"
> +#include "qapi/error.h"
>  #include "qemu/log.h"
>
>  /* host controller debug messages */
> @@ -1191,17 +1192,21 @@ static void sdhci_realizefn(SDHCIState *s, Error **errp)
>                            SDHC_REGISTERS_MAP_SIZE);
>  }
>
> +static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
> +{
> +    g_free(s->fifo_buffer);
> +    s->fifo_buffer = NULL;

I don't think setting this to NULL is required. Shouldn't g_free() do
that for you?

Alistair

> +}
> +
>  static void sdhci_uninitfn(SDHCIState *s)
>  {
>      timer_del(s->insert_timer);
>      timer_free(s->insert_timer);
>      timer_del(s->transfer_timer);
>      timer_free(s->transfer_timer);
> +
>      qemu_free_irq(s->eject_cb);
>      qemu_free_irq(s->ro_cb);
> -
> -    g_free(s->fifo_buffer);
> -    s->fifo_buffer = NULL;
>  }
>
>  static bool sdhci_pending_insert_vmstate_needed(void *opaque)
> @@ -1302,6 +1307,8 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>  static void sdhci_pci_exit(PCIDevice *dev)
>  {
>      SDHCIState *s = PCI_SDHCI(dev);
> +
> +    sdhci_unrealizefn(s, &error_abort);
>      sdhci_uninitfn(s);
>  }
>
> @@ -1355,11 +1362,19 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
>
> +static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
> +{
> +    SDHCIState *s = SYSBUS_SDHCI(dev);
> +
> +    sdhci_unrealizefn(s, errp);
> +}
> +
>  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->realize = sdhci_sysbus_realize;
> +    dc->unrealize = sdhci_sysbus_unrealize;
>
>      sdhci_class_init(klass, data);
>  }
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 09/14] sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:46 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sdhci.c | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 893be0e606..044e3d62f1 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/sd/sdhci.h"
>>  #include "sdhci-internal.h"
>>  #include "sd-internal.h"
>> +#include "qapi/error.h"
>>  #include "qemu/log.h"
>>
>>  /* host controller debug messages */
>> @@ -1191,17 +1192,21 @@ static void sdhci_realizefn(SDHCIState *s, Error **errp)
>>                            SDHC_REGISTERS_MAP_SIZE);
>>  }
>>
>> +static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
>> +{
>> +    g_free(s->fifo_buffer);
>> +    s->fifo_buffer = NULL;
>
> I don't think setting this to NULL is required. Shouldn't g_free() do
> that for you?

You are right, good eye :) I didn't notice, this was just code
movement from the current code.

Indeed g_free() doesn't free() if the pointer is NULL, however since
it doesn't have access to the pointer address it can't set it to NULL.
In some case it matters (RCU code maybe?) but I just checked and it
this case it doesn't, so I'll drop this line.

[Qemu-devel] [PATCH 10/14] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 044e3d62f1..8fcd48f849 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -936,7 +936,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = (SD_HOST_SPECv2_VERS << 16) | sdhci_slotint(s);
         break;
     default:
-        ERRPRINT("bad %ub read: addr[0x%04x]\n", size, (int)offset);
+        qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
+                      "not implemented\n", size, offset);
         break;
     }
 
@@ -1140,8 +1141,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         sdhci_update_irq(s);
         break;
     default:
-        ERRPRINT("bad %ub write offset: addr[0x%04x] <- %u(0x%x)\n",
-                 size, (int)offset, value >> shift, value >> shift);
+        qemu_log_mask(LOG_UNIMP, "SDHC wr_%ub @0x%02" HWADDR_PRIx " <- 0x%08x "
+                      "not implemented\n", size, offset, value >> shift);
         break;
     }
     DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n",
-- 
2.15.1

Re: [Qemu-devel] [PATCH 10/14] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 044e3d62f1..8fcd48f849 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -936,7 +936,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>          ret = (SD_HOST_SPECv2_VERS << 16) | sdhci_slotint(s);
>          break;
>      default:
> -        ERRPRINT("bad %ub read: addr[0x%04x]\n", size, (int)offset);
> +        qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
> +                      "not implemented\n", size, offset);

Is this actually unimplemented? Or is it just a guest error?

Alistair

>          break;
>      }
>
> @@ -1140,8 +1141,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          sdhci_update_irq(s);
>          break;
>      default:
> -        ERRPRINT("bad %ub write offset: addr[0x%04x] <- %u(0x%x)\n",
> -                 size, (int)offset, value >> shift, value >> shift);
> +        qemu_log_mask(LOG_UNIMP, "SDHC wr_%ub @0x%02" HWADDR_PRIx " <- 0x%08x "
> +                      "not implemented\n", size, offset, value >> shift);
>          break;
>      }
>      DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n",
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 10/14] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:47 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sdhci.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 044e3d62f1..8fcd48f849 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -936,7 +936,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>>          ret = (SD_HOST_SPECv2_VERS << 16) | sdhci_slotint(s);
>>          break;
>>      default:
>> -        ERRPRINT("bad %ub read: addr[0x%04x]\n", size, (int)offset);
>> +        qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
>> +                      "not implemented\n", size, offset);
>
> Is this actually unimplemented? Or is it just a guest error?

So far we don't have those registers implemented, that's why I decided
to use UNIMP to remove the fprintf().

This log become useful when we pretend to support the Spec v3, as the
guest start using register we don't have yet implemented.

However depending on the Spec version implemented, this might be a GUEST_ERROR.

I'll sort this out for the current version, v2 (we don't care about v1
since all QEMU models support at least v2).

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 10/14] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Alistair Francis]

On Thu, Dec 14, 2017 at 10:14 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> On Thu, Dec 14, 2017 at 2:47 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sdhci.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 044e3d62f1..8fcd48f849 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -936,7 +936,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>>>          ret = (SD_HOST_SPECv2_VERS << 16) | sdhci_slotint(s);
>>>          break;
>>>      default:
>>> -        ERRPRINT("bad %ub read: addr[0x%04x]\n", size, (int)offset);
>>> +        qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
>>> +                      "not implemented\n", size, offset);
>>
>> Is this actually unimplemented? Or is it just a guest error?
>
> So far we don't have those registers implemented, that's why I decided
> to use UNIMP to remove the fprintf().

Ah, very confusing. I'll let you decide here what to do.

Alistair

>
> This log become useful when we pretend to support the Spec v3, as the
> guest start using register we don't have yet implemented.
>
> However depending on the Spec version implemented, this might be a GUEST_ERROR.
>
> I'll sort this out for the current version, v2 (we don't care about v1
> since all QEMU models support at least v2).
>
> Thanks,
>
> Phil.
>

[Qemu-devel] [PATCH 11/14] sdhci: convert the DPRINT() calls into trace events

[Philippe Mathieu-Daudé]

zero-initialize ADMADescr 'dscr' in sdhci_do_adma() to avoid:

  hw/sd/sdhci.c: In function ‘sdhci_do_adma’:
  hw/sd/sdhci.c:714:29: error: ‘dscr.addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
             trace_sdhci_adma("link", s->admasysaddr);
                             ^

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c      | 89 ++++++++++++++++++------------------------------------
 hw/sd/trace-events | 14 +++++++++
 2 files changed, 44 insertions(+), 59 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8fcd48f849..aa2d2fa3d3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -33,30 +33,7 @@
 #include "sd-internal.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
-
-/* host controller debug messages */
-#ifndef SDHC_DEBUG
-#define SDHC_DEBUG                        0
-#endif
-
-#define DPRINT_L1(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG) { \
-            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
-        } \
-    } while (0)
-#define DPRINT_L2(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG > 1) { \
-            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
-        } \
-    } while (0)
-#define ERRPRINT(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG) { \
-            fprintf(stderr, "QEMU SDHC ERROR: " fmt, ## args); \
-        } \
-    } while (0)
+#include "trace.h"
 
 #define TYPE_SDHCI_BUS "sdhci-bus"
 #define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SDHCI_BUS)
@@ -154,8 +131,8 @@ static void sdhci_raise_insertion_irq(void *opaque)
 static void sdhci_set_inserted(DeviceState *dev, bool level)
 {
     SDHCIState *s = (SDHCIState *)dev;
-    DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
 
+    trace_sdhci_set_inserted(level ? "insert" : "eject");
     if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
         /* Give target some time to notice card ejection */
         timer_mod(s->insert_timer,
@@ -237,7 +214,8 @@ static void sdhci_send_command(SDHCIState *s)
     s->acmd12errsts = 0;
     request.cmd = s->cmdreg >> 8;
     request.arg = s->argument;
-    DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
+
+    trace_sdhci_send_command(request.cmd, request.arg);
     rlen = sdbus_do_command(&s->sdbus, &request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
@@ -245,7 +223,7 @@ static void sdhci_send_command(SDHCIState *s)
             s->rspreg[0] = (response[0] << 24) | (response[1] << 16) |
                            (response[2] << 8)  |  response[3];
             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
-            DPRINT_L1("Response: RSPREG[31..0]=0x%08x\n", s->rspreg[0]);
+            trace_sdhci_response4(s->rspreg[0]);
         } else if (rlen == 16) {
             s->rspreg[0] = (response[11] << 24) | (response[12] << 16) |
                            (response[13] << 8) |  response[14];
@@ -255,11 +233,10 @@ static void sdhci_send_command(SDHCIState *s)
                            (response[5] << 8)  |  response[6];
             s->rspreg[3] = (response[0] << 16) | (response[1] << 8) |
                             response[2];
-            DPRINT_L1("Response received:\n RSPREG[127..96]=0x%08x, RSPREG[95.."
-                  "64]=0x%08x,\n RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x\n",
-                  s->rspreg[3], s->rspreg[2], s->rspreg[1], s->rspreg[0]);
+            trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
+                                   s->rspreg[1], s->rspreg[0]);
         } else {
-            ERRPRINT("Timeout waiting for command response\n");
+            trace_sdhci_error("timeout waiting for command response");
             if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
                 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
                 s->norintsts |= SDHC_NIS_ERR;
@@ -293,7 +270,7 @@ static void sdhci_end_transfer(SDHCIState *s)
 
         request.cmd = 0x0C;
         request.arg = 0;
-        DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, request.arg);
+        trace_sdhci_end_transfer(request.cmd, request.arg);
         sdbus_do_command(&s->sdbus, &request, response);
         /* Auto CMD12 response goes to the upper Response register */
         s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
@@ -362,7 +339,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
 
     /* first check that a valid data exists in host controller input buffer */
     if ((s->prnsts & SDHC_DATA_AVAILABLE) == 0) {
-        ERRPRINT("Trying to read from empty buffer\n");
+        trace_sdhci_error("read from empty buffer");
         return 0;
     }
 
@@ -371,8 +348,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
         s->data_count++;
         /* check if we've read all valid data (blksize bytes) from buffer */
         if ((s->data_count) >= (s->blksize & 0x0fff)) {
-            DPRINT_L2("All %u bytes of data have been read from input buffer\n",
-                    s->data_count);
+            trace_sdhci_read_dataport(s->data_count);
             s->prnsts &= ~SDHC_DATA_AVAILABLE; /* no more data in a buffer */
             s->data_count = 0;  /* next buff read must start at position [0] */
 
@@ -455,7 +431,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
 
     /* Check that there is free space left in a buffer */
     if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) {
-        ERRPRINT("Can't write to data buffer: buffer full\n");
+        trace_sdhci_error("Can't write to data buffer: buffer full");
         return;
     }
 
@@ -464,8 +440,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
         s->data_count++;
         value >>= 8;
         if (s->data_count >= (s->blksize & 0x0fff)) {
-            DPRINT_L2("write buffer filled with %u bytes of data\n",
-                    s->data_count);
+            trace_sdhci_write_dataport(s->data_count);
             s->data_count = 0;
             s->prnsts &= ~SDHC_SPACE_AVAILABLE;
             if (s->prnsts & SDHC_DOING_WRITE) {
@@ -644,15 +619,14 @@ static void sdhci_do_adma(SDHCIState *s)
 {
     unsigned int n, begin, length;
     const uint16_t block_size = s->blksize & 0x0fff;
-    ADMADescr dscr;
+    ADMADescr dscr = {};
     int i;
 
     for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
         get_adma_description(s, &dscr);
-        DPRINT_L2("ADMA loop: addr=" TARGET_FMT_plx ", len=%d, attr=%x\n",
-                dscr.addr, dscr.length, dscr.attr);
+        trace_sdhci_adma_loop(dscr.addr, dscr.length, dscr.attr);
 
         if ((dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
             /* Indicate that error occurred in ST_FDS state */
@@ -735,8 +709,7 @@ static void sdhci_do_adma(SDHCIState *s)
             break;
         case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
             s->admasysaddr = dscr.addr;
-            DPRINT_L1("ADMA link: admasysaddr=0x%" PRIx64 "\n",
-                      s->admasysaddr);
+            trace_sdhci_adma("link", s->admasysaddr);
             break;
         default:
             s->admasysaddr += dscr.incr;
@@ -744,8 +717,7 @@ static void sdhci_do_adma(SDHCIState *s)
         }
 
         if (dscr.attr & SDHC_ADMA_ATTR_INT) {
-            DPRINT_L1("ADMA interrupt: admasysaddr=0x%" PRIx64 "\n",
-                      s->admasysaddr);
+            trace_sdhci_adma("interrupt", s->admasysaddr);
             if (s->norintstsen & SDHC_NISEN_DMA) {
                 s->norintsts |= SDHC_NIS_DMA;
             }
@@ -756,15 +728,15 @@ static void sdhci_do_adma(SDHCIState *s)
         /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
         if (((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
                     (s->blkcnt == 0)) || (dscr.attr & SDHC_ADMA_ATTR_END)) {
-            DPRINT_L2("ADMA transfer completed\n");
+            trace_sdhci_adma_transfer_completed();
             if (length || ((dscr.attr & SDHC_ADMA_ATTR_END) &&
                 (s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
                 s->blkcnt != 0)) {
-                ERRPRINT("SD/MMC host ADMA length mismatch\n");
+                trace_sdhci_error("SD/MMC host ADMA length mismatch");
                 s->admaerr |= SDHC_ADMAERR_LENGTH_MISMATCH |
                         SDHC_ADMAERR_STATE_ST_TFR;
                 if (s->errintstsen & SDHC_EISEN_ADMAERR) {
-                    ERRPRINT("Set ADMA error flag\n");
+                    trace_sdhci_error("Set ADMA error flag");
                     s->errintsts |= SDHC_EIS_ADMAERR;
                     s->norintsts |= SDHC_NIS_ERR;
                 }
@@ -800,7 +772,7 @@ static void sdhci_data_transfer(void *opaque)
             break;
         case SDHC_CTRL_ADMA1_32:
             if (!(s->capareg & SDHC_CAN_DO_ADMA1)) {
-                ERRPRINT("ADMA1 not supported\n");
+                trace_sdhci_error("ADMA1 not supported");
                 break;
             }
 
@@ -808,7 +780,7 @@ static void sdhci_data_transfer(void *opaque)
             break;
         case SDHC_CTRL_ADMA2_32:
             if (!(s->capareg & SDHC_CAN_DO_ADMA2)) {
-                ERRPRINT("ADMA2 not supported\n");
+                trace_sdhci_error("ADMA2 not supported");
                 break;
             }
 
@@ -817,14 +789,14 @@ static void sdhci_data_transfer(void *opaque)
         case SDHC_CTRL_ADMA2_64:
             if (!(s->capareg & SDHC_CAN_DO_ADMA2) ||
                     !(s->capareg & SDHC_64_BIT_BUS_SUPPORT)) {
-                ERRPRINT("64 bit ADMA not supported\n");
+                trace_sdhci_error("64 bit ADMA not supported");
                 break;
             }
 
             sdhci_do_adma(s);
             break;
         default:
-            ERRPRINT("Unsupported DMA type\n");
+            trace_sdhci_error("Unsupported DMA type");
             break;
         }
     } else {
@@ -859,8 +831,8 @@ static inline bool
 sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
 {
     if ((s->data_count & 0x3) != byte_num) {
-        ERRPRINT("Non-sequential access to Buffer Data Port register"
-                "is prohibited\n");
+        trace_sdhci_error("Non-sequential access to Buffer Data Port register"
+                          "is prohibited\n");
         return false;
     }
     return true;
@@ -890,8 +862,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
     case  SDHC_BDATA:
         if (sdhci_buff_access_is_sequential(s, offset - SDHC_BDATA)) {
             ret = sdhci_read_dataport(s, size);
-            DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset,
-                      ret, ret);
+            trace_sdhci_access("read", size, offset, "->", ret, ret);
             return ret;
         }
         break;
@@ -943,7 +914,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
 
     ret >>= (offset & 0x3) * 8;
     ret &= (1ULL << (size * 8)) - 1;
-    DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset, ret, ret);
+    trace_sdhci_access("read", size, offset, "->", ret, ret);
     return ret;
 }
 
@@ -1145,8 +1116,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                       "not implemented\n", size, offset, value >> shift);
         break;
     }
-    DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n",
-              size, (int)offset, value >> shift, value >> shift);
+    trace_sdhci_access("write", size, offset, "<-",
+                       value >> shift, value >> shift);
 }
 
 static const MemoryRegionOps sdhci_mmio_ops = {
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 1fc0bcf44b..f7a85be53d 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -1,5 +1,19 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/sd/sdhci.c
+sdhci_set_inserted(const char *level) "card state changed: %s"
+sdhci_send_command(uint8_t cmd, uint32_t arg) "sending CMD%02u ARG[0x%08x]"
+sdhci_error(const char *msg) "%s"
+sdhci_response4(uint32_t r0) "Response: RSPREG[31..0]=0x%08x"
+sdhci_response16(uint32_t r3, uint32_t r2, uint32_t r1, uint32_t r0) "Response received: RSPREG[127..96]=0x%08x, RSPREG[95..64]=0x%08x, RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x"
+sdhci_end_transfer(uint8_t cmd, uint32_t arg) "Automatically issue CMD%02u 0x%08x"
+sdhci_adma(const char *desc, uint32_t sysad) "ADMA %s: admasysaddr=0x%" PRIx32
+sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr) "ADMA loop: addr=0x%08" HWADDR_PRIx ", len=%d, attr=0x%x"
+sdhci_adma_transfer_completed(void) "ADMA transfer completed"
+sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s %ub: addr[0x%04" HWADDR_PRIx "] %s %" PRIu64 "(0x%" PRIx64 ")"
+sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
+sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
+
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
 milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.15.1

Re: [Qemu-devel] [PATCH 11/14] sdhci: convert the DPRINT() calls into trace events

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> zero-initialize ADMADescr 'dscr' in sdhci_do_adma() to avoid:
>
>   hw/sd/sdhci.c: In function ‘sdhci_do_adma’:
>   hw/sd/sdhci.c:714:29: error: ‘dscr.addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>              trace_sdhci_adma("link", s->admasysaddr);
>                              ^
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  hw/sd/sdhci.c      | 89 ++++++++++++++++++------------------------------------
>  hw/sd/trace-events | 14 +++++++++
>  2 files changed, 44 insertions(+), 59 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8fcd48f849..aa2d2fa3d3 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -33,30 +33,7 @@
>  #include "sd-internal.h"
>  #include "qapi/error.h"
>  #include "qemu/log.h"
> -
> -/* host controller debug messages */
> -#ifndef SDHC_DEBUG
> -#define SDHC_DEBUG                        0
> -#endif
> -
> -#define DPRINT_L1(fmt, args...) \
> -    do { \
> -        if (SDHC_DEBUG) { \
> -            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
> -        } \
> -    } while (0)
> -#define DPRINT_L2(fmt, args...) \
> -    do { \
> -        if (SDHC_DEBUG > 1) { \
> -            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
> -        } \
> -    } while (0)
> -#define ERRPRINT(fmt, args...) \
> -    do { \
> -        if (SDHC_DEBUG) { \
> -            fprintf(stderr, "QEMU SDHC ERROR: " fmt, ## args); \
> -        } \
> -    } while (0)
> +#include "trace.h"
>
>  #define TYPE_SDHCI_BUS "sdhci-bus"
>  #define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SDHCI_BUS)
> @@ -154,8 +131,8 @@ static void sdhci_raise_insertion_irq(void *opaque)
>  static void sdhci_set_inserted(DeviceState *dev, bool level)
>  {
>      SDHCIState *s = (SDHCIState *)dev;
> -    DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
>
> +    trace_sdhci_set_inserted(level ? "insert" : "eject");
>      if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
>          /* Give target some time to notice card ejection */
>          timer_mod(s->insert_timer,
> @@ -237,7 +214,8 @@ static void sdhci_send_command(SDHCIState *s)
>      s->acmd12errsts = 0;
>      request.cmd = s->cmdreg >> 8;
>      request.arg = s->argument;
> -    DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
> +
> +    trace_sdhci_send_command(request.cmd, request.arg);
>      rlen = sdbus_do_command(&s->sdbus, &request, response);
>
>      if (s->cmdreg & SDHC_CMD_RESPONSE) {
> @@ -245,7 +223,7 @@ static void sdhci_send_command(SDHCIState *s)
>              s->rspreg[0] = (response[0] << 24) | (response[1] << 16) |
>                             (response[2] << 8)  |  response[3];
>              s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
> -            DPRINT_L1("Response: RSPREG[31..0]=0x%08x\n", s->rspreg[0]);
> +            trace_sdhci_response4(s->rspreg[0]);
>          } else if (rlen == 16) {
>              s->rspreg[0] = (response[11] << 24) | (response[12] << 16) |
>                             (response[13] << 8) |  response[14];
> @@ -255,11 +233,10 @@ static void sdhci_send_command(SDHCIState *s)
>                             (response[5] << 8)  |  response[6];
>              s->rspreg[3] = (response[0] << 16) | (response[1] << 8) |
>                              response[2];
> -            DPRINT_L1("Response received:\n RSPREG[127..96]=0x%08x, RSPREG[95.."
> -                  "64]=0x%08x,\n RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x\n",
> -                  s->rspreg[3], s->rspreg[2], s->rspreg[1], s->rspreg[0]);
> +            trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> +                                   s->rspreg[1], s->rspreg[0]);
>          } else {
> -            ERRPRINT("Timeout waiting for command response\n");
> +            trace_sdhci_error("timeout waiting for command response");
>              if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
>                  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
>                  s->norintsts |= SDHC_NIS_ERR;
> @@ -293,7 +270,7 @@ static void sdhci_end_transfer(SDHCIState *s)
>
>          request.cmd = 0x0C;
>          request.arg = 0;
> -        DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, request.arg);
> +        trace_sdhci_end_transfer(request.cmd, request.arg);
>          sdbus_do_command(&s->sdbus, &request, response);
>          /* Auto CMD12 response goes to the upper Response register */
>          s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
> @@ -362,7 +339,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
>
>      /* first check that a valid data exists in host controller input buffer */
>      if ((s->prnsts & SDHC_DATA_AVAILABLE) == 0) {
> -        ERRPRINT("Trying to read from empty buffer\n");
> +        trace_sdhci_error("read from empty buffer");
>          return 0;
>      }
>
> @@ -371,8 +348,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
>          s->data_count++;
>          /* check if we've read all valid data (blksize bytes) from buffer */
>          if ((s->data_count) >= (s->blksize & 0x0fff)) {
> -            DPRINT_L2("All %u bytes of data have been read from input buffer\n",
> -                    s->data_count);
> +            trace_sdhci_read_dataport(s->data_count);
>              s->prnsts &= ~SDHC_DATA_AVAILABLE; /* no more data in a buffer */
>              s->data_count = 0;  /* next buff read must start at position [0] */
>
> @@ -455,7 +431,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
>
>      /* Check that there is free space left in a buffer */
>      if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) {
> -        ERRPRINT("Can't write to data buffer: buffer full\n");
> +        trace_sdhci_error("Can't write to data buffer: buffer full");
>          return;
>      }
>
> @@ -464,8 +440,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
>          s->data_count++;
>          value >>= 8;
>          if (s->data_count >= (s->blksize & 0x0fff)) {
> -            DPRINT_L2("write buffer filled with %u bytes of data\n",
> -                    s->data_count);
> +            trace_sdhci_write_dataport(s->data_count);
>              s->data_count = 0;
>              s->prnsts &= ~SDHC_SPACE_AVAILABLE;
>              if (s->prnsts & SDHC_DOING_WRITE) {
> @@ -644,15 +619,14 @@ static void sdhci_do_adma(SDHCIState *s)
>  {
>      unsigned int n, begin, length;
>      const uint16_t block_size = s->blksize & 0x0fff;
> -    ADMADescr dscr;
> +    ADMADescr dscr = {};
>      int i;
>
>      for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
>          s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
>
>          get_adma_description(s, &dscr);
> -        DPRINT_L2("ADMA loop: addr=" TARGET_FMT_plx ", len=%d, attr=%x\n",
> -                dscr.addr, dscr.length, dscr.attr);
> +        trace_sdhci_adma_loop(dscr.addr, dscr.length, dscr.attr);
>
>          if ((dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
>              /* Indicate that error occurred in ST_FDS state */
> @@ -735,8 +709,7 @@ static void sdhci_do_adma(SDHCIState *s)
>              break;
>          case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
>              s->admasysaddr = dscr.addr;
> -            DPRINT_L1("ADMA link: admasysaddr=0x%" PRIx64 "\n",
> -                      s->admasysaddr);
> +            trace_sdhci_adma("link", s->admasysaddr);
>              break;
>          default:
>              s->admasysaddr += dscr.incr;
> @@ -744,8 +717,7 @@ static void sdhci_do_adma(SDHCIState *s)
>          }
>
>          if (dscr.attr & SDHC_ADMA_ATTR_INT) {
> -            DPRINT_L1("ADMA interrupt: admasysaddr=0x%" PRIx64 "\n",
> -                      s->admasysaddr);
> +            trace_sdhci_adma("interrupt", s->admasysaddr);
>              if (s->norintstsen & SDHC_NISEN_DMA) {
>                  s->norintsts |= SDHC_NIS_DMA;
>              }
> @@ -756,15 +728,15 @@ static void sdhci_do_adma(SDHCIState *s)
>          /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
>          if (((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
>                      (s->blkcnt == 0)) || (dscr.attr & SDHC_ADMA_ATTR_END)) {
> -            DPRINT_L2("ADMA transfer completed\n");
> +            trace_sdhci_adma_transfer_completed();
>              if (length || ((dscr.attr & SDHC_ADMA_ATTR_END) &&
>                  (s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
>                  s->blkcnt != 0)) {
> -                ERRPRINT("SD/MMC host ADMA length mismatch\n");
> +                trace_sdhci_error("SD/MMC host ADMA length mismatch");
>                  s->admaerr |= SDHC_ADMAERR_LENGTH_MISMATCH |
>                          SDHC_ADMAERR_STATE_ST_TFR;
>                  if (s->errintstsen & SDHC_EISEN_ADMAERR) {
> -                    ERRPRINT("Set ADMA error flag\n");
> +                    trace_sdhci_error("Set ADMA error flag");
>                      s->errintsts |= SDHC_EIS_ADMAERR;
>                      s->norintsts |= SDHC_NIS_ERR;
>                  }
> @@ -800,7 +772,7 @@ static void sdhci_data_transfer(void *opaque)
>              break;
>          case SDHC_CTRL_ADMA1_32:
>              if (!(s->capareg & SDHC_CAN_DO_ADMA1)) {
> -                ERRPRINT("ADMA1 not supported\n");
> +                trace_sdhci_error("ADMA1 not supported");
>                  break;
>              }
>
> @@ -808,7 +780,7 @@ static void sdhci_data_transfer(void *opaque)
>              break;
>          case SDHC_CTRL_ADMA2_32:
>              if (!(s->capareg & SDHC_CAN_DO_ADMA2)) {
> -                ERRPRINT("ADMA2 not supported\n");
> +                trace_sdhci_error("ADMA2 not supported");
>                  break;
>              }
>
> @@ -817,14 +789,14 @@ static void sdhci_data_transfer(void *opaque)
>          case SDHC_CTRL_ADMA2_64:
>              if (!(s->capareg & SDHC_CAN_DO_ADMA2) ||
>                      !(s->capareg & SDHC_64_BIT_BUS_SUPPORT)) {
> -                ERRPRINT("64 bit ADMA not supported\n");
> +                trace_sdhci_error("64 bit ADMA not supported");
>                  break;
>              }
>
>              sdhci_do_adma(s);
>              break;
>          default:
> -            ERRPRINT("Unsupported DMA type\n");
> +            trace_sdhci_error("Unsupported DMA type");
>              break;
>          }
>      } else {
> @@ -859,8 +831,8 @@ static inline bool
>  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>  {
>      if ((s->data_count & 0x3) != byte_num) {
> -        ERRPRINT("Non-sequential access to Buffer Data Port register"
> -                "is prohibited\n");
> +        trace_sdhci_error("Non-sequential access to Buffer Data Port register"
> +                          "is prohibited\n");
>          return false;
>      }
>      return true;
> @@ -890,8 +862,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>      case  SDHC_BDATA:
>          if (sdhci_buff_access_is_sequential(s, offset - SDHC_BDATA)) {
>              ret = sdhci_read_dataport(s, size);
> -            DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset,
> -                      ret, ret);
> +            trace_sdhci_access("read", size, offset, "->", ret, ret);
>              return ret;
>          }
>          break;
> @@ -943,7 +914,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>
>      ret >>= (offset & 0x3) * 8;
>      ret &= (1ULL << (size * 8)) - 1;
> -    DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset, ret, ret);
> +    trace_sdhci_access("read", size, offset, "->", ret, ret);
>      return ret;
>  }
>
> @@ -1145,8 +1116,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>                        "not implemented\n", size, offset, value >> shift);
>          break;
>      }
> -    DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n",
> -              size, (int)offset, value >> shift, value >> shift);
> +    trace_sdhci_access("write", size, offset, "<-",
> +                       value >> shift, value >> shift);
>  }
>
>  static const MemoryRegionOps sdhci_mmio_ops = {
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 1fc0bcf44b..f7a85be53d 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -1,5 +1,19 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>
> +# hw/sd/sdhci.c
> +sdhci_set_inserted(const char *level) "card state changed: %s"
> +sdhci_send_command(uint8_t cmd, uint32_t arg) "sending CMD%02u ARG[0x%08x]"
> +sdhci_error(const char *msg) "%s"
> +sdhci_response4(uint32_t r0) "Response: RSPREG[31..0]=0x%08x"
> +sdhci_response16(uint32_t r3, uint32_t r2, uint32_t r1, uint32_t r0) "Response received: RSPREG[127..96]=0x%08x, RSPREG[95..64]=0x%08x, RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x"
> +sdhci_end_transfer(uint8_t cmd, uint32_t arg) "Automatically issue CMD%02u 0x%08x"
> +sdhci_adma(const char *desc, uint32_t sysad) "ADMA %s: admasysaddr=0x%" PRIx32
> +sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr) "ADMA loop: addr=0x%08" HWADDR_PRIx ", len=%d, attr=0x%x"
> +sdhci_adma_transfer_completed(void) "ADMA transfer completed"
> +sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s %ub: addr[0x%04" HWADDR_PRIx "] %s %" PRIu64 "(0x%" PRIx64 ")"
> +sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
> +sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
> +
>  # hw/sd/milkymist-memcard.c
>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>  milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 11/14] sdhci: convert the DPRINT() calls into trace events

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:54 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> zero-initialize ADMADescr 'dscr' in sdhci_do_adma() to avoid:
>>
>>   hw/sd/sdhci.c: In function ‘sdhci_do_adma’:
>>   hw/sd/sdhci.c:714:29: error: ‘dscr.addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>              trace_sdhci_adma("link", s->admasysaddr);
>>                              ^
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks, note this patch has two incorrect uint64_t string format, I'll
add your Acked-by in the respin with "%x" replaced by PRIx64.

>> +# hw/sd/sdhci.c
>> +sdhci_set_inserted(const char *level) "card state changed: %s"
>> +sdhci_send_command(uint8_t cmd, uint32_t arg) "sending CMD%02u ARG[0x%08x]"
>> +sdhci_error(const char *msg) "%s"
>> +sdhci_response4(uint32_t r0) "Response: RSPREG[31..0]=0x%08x"
>> +sdhci_response16(uint32_t r3, uint32_t r2, uint32_t r1, uint32_t r0) "Response received: RSPREG[127..96]=0x%08x, RSPREG[95..64]=0x%08x, RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x"
>> +sdhci_end_transfer(uint8_t cmd, uint32_t arg) "Automatically issue CMD%02u 0x%08x"
>> +sdhci_adma(const char *desc, uint32_t sysad) "ADMA %s: admasysaddr=0x%" PRIx32
>> +sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr) "ADMA loop: addr=0x%08" HWADDR_PRIx ", len=%d, attr=0x%x"

^ incorrect uint64_t format

>> +sdhci_adma_transfer_completed(void) "ADMA transfer completed"
>> +sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s %ub: addr[0x%04" HWADDR_PRIx "] %s %" PRIu64 "(0x%" PRIx64 ")"
>> +sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
>> +sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"

[Qemu-devel] [PATCH 12/14] sdhci: add a trace event for the LED control

[Philippe Mathieu-Daudé]

It blinks to caution the user not to remove the card while the SD card is
being accessed.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c      | 1 +
 hw/sd/trace-events | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index aa2d2fa3d3..dadc4787b2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1041,6 +1041,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                 !(s->capareg & (1 << (31 - ((s->pwrcon >> 1) & 0x7))))) {
             s->pwrcon &= ~SDHC_POWER_ON;
         }
+        trace_sdhci_led(s->hostctl & 1);
         break;
     case SDHC_CLKCON:
         if (!(mask & 0xFF000000)) {
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index f7a85be53d..11f8fa4fc1 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -13,6 +13,7 @@ sdhci_adma_transfer_completed(void) "ADMA transfer completed"
 sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s %ub: addr[0x%04" HWADDR_PRIx "] %s %" PRIu64 "(0x%" PRIx64 ")"
 sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
 sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
+sdhci_led(bool state) "LED: %u"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.15.1

Re: [Qemu-devel] [PATCH 12/14] sdhci: add a trace event for the LED control

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> It blinks to caution the user not to remove the card while the SD card is
> being accessed.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

If you turn this on does it just fill the trace log?

It seems like this will generate too much output if it happens on
every LED blink.

Alistair

> ---
>  hw/sd/sdhci.c      | 1 +
>  hw/sd/trace-events | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index aa2d2fa3d3..dadc4787b2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1041,6 +1041,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>                  !(s->capareg & (1 << (31 - ((s->pwrcon >> 1) & 0x7))))) {
>              s->pwrcon &= ~SDHC_POWER_ON;
>          }
> +        trace_sdhci_led(s->hostctl & 1);
>          break;
>      case SDHC_CLKCON:
>          if (!(mask & 0xFF000000)) {
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index f7a85be53d..11f8fa4fc1 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -13,6 +13,7 @@ sdhci_adma_transfer_completed(void) "ADMA transfer completed"
>  sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s %ub: addr[0x%04" HWADDR_PRIx "] %s %" PRIu64 "(0x%" PRIx64 ")"
>  sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
>  sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
> +sdhci_led(bool state) "LED: %u"
>
>  # hw/sd/milkymist-memcard.c
>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 12/14] sdhci: add a trace event for the LED control

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:55 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> It blinks to caution the user not to remove the card while the SD card is
>> being accessed.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> If you turn this on does it just fill the trace log?
>
> It seems like this will generate too much output if it happens on
> every LED blink.

You don't need to enable this trace event if you don't want it :)
It is supposed to turn ON previous to start write access, and then turn OFF.

Anyway you are right, I currently log it on each access while I should
just log the ON/OFF state changes, I'll fix this.

[Qemu-devel] [PATCH 13/14] sdhci: add sdhci_init_capareg() to initialize the CAPAB register

[Philippe Mathieu-Daudé]

following patches modifying CAPAB will be easier to review.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dadc4787b2..78bb8e8e89 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -100,6 +100,13 @@
 
 #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
 
+static void sdhci_init_capareg(SDHCIState *s, Error **errp)
+{
+    if (s->capareg == UINT32_MAX) {
+        s->capareg = SDHC_CAPAB_REG_DEFAULT;
+    }
+}
+
 static uint8_t sdhci_slotint(SDHCIState *s)
 {
     return (s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
@@ -1158,6 +1165,8 @@ static void sdhci_initfn(SDHCIState *s)
 
 static void sdhci_realizefn(SDHCIState *s, Error **errp)
 {
+    sdhci_init_capareg(s, errp);
+
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
@@ -1244,8 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
-            SDHC_CAPAB_REG_DEFAULT),
+    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, UINT32_MAX),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
-- 
2.15.1

Re: [Qemu-devel] [PATCH 13/14] sdhci: add sdhci_init_capareg() to initialize the CAPAB register

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> following patches modifying CAPAB will be easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index dadc4787b2..78bb8e8e89 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -100,6 +100,13 @@
>
>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>
> +static void sdhci_init_capareg(SDHCIState *s, Error **errp)
> +{
> +    if (s->capareg == UINT32_MAX) {
> +        s->capareg = SDHC_CAPAB_REG_DEFAULT;
> +    }
> +}
> +
>  static uint8_t sdhci_slotint(SDHCIState *s)
>  {
>      return (s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
> @@ -1158,6 +1165,8 @@ static void sdhci_initfn(SDHCIState *s)
>
>  static void sdhci_realizefn(SDHCIState *s, Error **errp)
>  {
> +    sdhci_init_capareg(s, errp);
> +
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
> @@ -1244,8 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
>  static Property sdhci_properties[] = {
> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
> -            SDHC_CAPAB_REG_DEFAULT),
> +    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, UINT32_MAX),

This looks like a step in the wrong direction.

If this is required to do something smarter latter it should probably
be in that series.

Alistair

>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>      DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>                       false),
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 13/14] sdhci: add sdhci_init_capareg() to initialize the CAPAB register

[Philippe Mathieu-Daudé]

On Thu, Dec 14, 2017 at 2:51 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
> <f4bug@amsat.org> wrote:
>> following patches modifying CAPAB will be easier to review.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sdhci.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index dadc4787b2..78bb8e8e89 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -100,6 +100,13 @@
>>
>>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>>
>> +static void sdhci_init_capareg(SDHCIState *s, Error **errp)
>> +{
>> +    if (s->capareg == UINT32_MAX) {
>> +        s->capareg = SDHC_CAPAB_REG_DEFAULT;
>> +    }
>> +}
>> +
>>  static uint8_t sdhci_slotint(SDHCIState *s)
>>  {
>>      return (s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
>> @@ -1158,6 +1165,8 @@ static void sdhci_initfn(SDHCIState *s)
>>
>>  static void sdhci_realizefn(SDHCIState *s, Error **errp)
>>  {
>> +    sdhci_init_capareg(s, errp);
>> +
>>      s->buf_maxsz = sdhci_get_fifolen(s);
>>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>
>> @@ -1244,8 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
>>  /* Capabilities registers provide information on supported features of this
>>   * specific host controller implementation */
>>  static Property sdhci_properties[] = {
>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>> -            SDHC_CAPAB_REG_DEFAULT),
>> +    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, UINT32_MAX),
>
> This looks like a step in the wrong direction.
>
> If this is required to do something smarter latter it should probably
> be in that series.

You are right, I might have add this patch in the incorrect series.
I'll move it to the one where the CAPAB register is correctly set
regarding the SDHCI supports the v2 or v3 spec.

[Qemu-devel] [PATCH 14/14] sdhci: add a "dma-memory" property

[Philippe Mathieu-Daudé]

Add a dma property allowing machine creation to provide the address-space
sdhci dma operates on.

[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2016.1]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sdhci.h |  2 ++
 hw/sd/sdhci.c         | 24 ++++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 1b6a98d578..e6644e6e7d 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -41,6 +41,8 @@ typedef struct SDHCIState {
     /*< public >*/
     SDBus sdbus;
     MemoryRegion iomem;
+    MemoryRegion *dma_mr;
+    AddressSpace dma_as;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 78bb8e8e89..5f8064c59b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -590,7 +590,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
     switch (SDHC_DMA_TYPE(s->hostctl)) {
     case SDHC_CTRL_ADMA2_32:
-        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
+        adma2 = ldq_le_dma(&s->dma_as, entry_addr);
         /* The spec does not specify endianness of descriptor table.
          * We currently assume that it is LE.
          */
@@ -600,7 +600,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dscr->incr = 8;
         break;
     case SDHC_CTRL_ADMA1_32:
-        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
+        adma1 = ldl_le_dma(&s->dma_as, entry_addr);
         dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
         dscr->attr = (uint8_t)extract32(adma1, 0, 7);
         dscr->incr = 4;
@@ -611,9 +611,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         }
         break;
     case SDHC_CTRL_ADMA2_64:
-        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
-        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
-        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
+        dscr->attr = ldub_dma(&s->dma_as, entry_addr);
+        dscr->length = lduw_le_dma(&s->dma_as, entry_addr + 2);
+        dscr->attr = ldq_le_dma(&s->dma_as, entry_addr + 4);
         dscr->attr &= 0xfffffff8;
         dscr->incr = 12;
         break;
@@ -670,7 +670,7 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_write(&address_space_memory, dscr.addr,
+                    dma_memory_write(&s->dma_as, dscr.addr,
                                      &s->fifo_buffer[begin],
                                      s->data_count - begin);
                     dscr.addr += s->data_count - begin;
@@ -1172,10 +1172,20 @@ static void sdhci_realizefn(SDHCIState *s, Error **errp)
 
     memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
                           SDHC_REGISTERS_MAP_SIZE);
+
+    /* use system_memory() if property "dma-memory" not set */
+    address_space_init(&s->dma_as,
+                       s->dma_mr ? s->dma_mr : get_system_memory(),
+                       "sdhci-dma");
 }
 
 static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
 {
+    if (s->dma_mr) {
+        address_space_destroy(&s->dma_as);
+        object_unparent(OBJECT(&s->dma_mr));
+    }
+
     g_free(s->fifo_buffer);
     s->fifo_buffer = NULL;
 }
@@ -1257,6 +1267,8 @@ static Property sdhci_properties[] = {
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
+    DEFINE_PROP_LINK("dma-memory", SDHCIState, dma_mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.15.1

Re: [Qemu-devel] [PATCH 14/14] sdhci: add a "dma-memory" property

[Alistair Francis]

On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Add a dma property allowing machine creation to provide the address-space
> sdhci dma operates on.
>
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2016.1]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  include/hw/sd/sdhci.h |  2 ++
>  hw/sd/sdhci.c         | 24 ++++++++++++++++++------
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 1b6a98d578..e6644e6e7d 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -41,6 +41,8 @@ typedef struct SDHCIState {
>      /*< public >*/
>      SDBus sdbus;
>      MemoryRegion iomem;
> +    MemoryRegion *dma_mr;
> +    AddressSpace dma_as;
>
>      QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
>      QEMUTimer *transfer_timer;
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 78bb8e8e89..5f8064c59b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -590,7 +590,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>      hwaddr entry_addr = (hwaddr)s->admasysaddr;
>      switch (SDHC_DMA_TYPE(s->hostctl)) {
>      case SDHC_CTRL_ADMA2_32:
> -        adma2 = ldq_le_dma(&address_space_memory, entry_addr);
> +        adma2 = ldq_le_dma(&s->dma_as, entry_addr);
>          /* The spec does not specify endianness of descriptor table.
>           * We currently assume that it is LE.
>           */
> @@ -600,7 +600,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>          dscr->incr = 8;
>          break;
>      case SDHC_CTRL_ADMA1_32:
> -        adma1 = ldl_le_dma(&address_space_memory, entry_addr);
> +        adma1 = ldl_le_dma(&s->dma_as, entry_addr);
>          dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
>          dscr->attr = (uint8_t)extract32(adma1, 0, 7);
>          dscr->incr = 4;
> @@ -611,9 +611,9 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>          }
>          break;
>      case SDHC_CTRL_ADMA2_64:
> -        dscr->attr = ldub_dma(&address_space_memory, entry_addr);
> -        dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2);
> -        dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4);
> +        dscr->attr = ldub_dma(&s->dma_as, entry_addr);
> +        dscr->length = lduw_le_dma(&s->dma_as, entry_addr + 2);
> +        dscr->attr = ldq_le_dma(&s->dma_as, entry_addr + 4);
>          dscr->attr &= 0xfffffff8;
>          dscr->incr = 12;
>          break;
> @@ -670,7 +670,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                          s->data_count = block_size;
>                          length -= block_size - begin;
>                      }
> -                    dma_memory_write(&address_space_memory, dscr.addr,
> +                    dma_memory_write(&s->dma_as, dscr.addr,
>                                       &s->fifo_buffer[begin],
>                                       s->data_count - begin);
>                      dscr.addr += s->data_count - begin;
> @@ -1172,10 +1172,20 @@ static void sdhci_realizefn(SDHCIState *s, Error **errp)
>
>      memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>                            SDHC_REGISTERS_MAP_SIZE);
> +
> +    /* use system_memory() if property "dma-memory" not set */
> +    address_space_init(&s->dma_as,
> +                       s->dma_mr ? s->dma_mr : get_system_memory(),
> +                       "sdhci-dma");
>  }
>
>  static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
>  {
> +    if (s->dma_mr) {
> +        address_space_destroy(&s->dma_as);
> +        object_unparent(OBJECT(&s->dma_mr));
> +    }
> +
>      g_free(s->fifo_buffer);
>      s->fifo_buffer = NULL;
>  }
> @@ -1257,6 +1267,8 @@ static Property sdhci_properties[] = {
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>      DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>                       false),
> +    DEFINE_PROP_LINK("dma-memory", SDHCIState, dma_mr,
> +                     TYPE_MEMORY_REGION, MemoryRegion *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.15.1
>
>