[Qemu-devel] [PATCH v2 00/20] SDHCI: housekeeping, add a qtest and fix few issues

[Qemu-devel] [PATCH v2 00/20] SDHCI: housekeeping, add a qtest and fix few issues

[Philippe Mathieu-Daudé]

Since v1:
- addressed Alistair Francis review comments, added some R-b
- only move register defines to "sd-internal.h"
- fixed deposit64() arguments
- dropped unuseful s->fifo_buffer = NULL
- use a qemu_irq for the LED, restrict the logging to ON/OFF
- fixed a trace format string error
- included Andrey Smirnov ACMD12ERRSTS write patch
- dropped few unuseful patches, and separate the Python polemical ones for later

>From the "SDHCI housekeeping" series:
- 1: we restrict part of "sd/sd.h" into local "sd-internal.h",
- 2,3: we somehow beautiful the code, no logical changes,
- 4-7: we refactor the common sysbus/pci qdev code,
- 8-10: we add plenty of trace events which will result useful later,
- 11: we finally expose a "dma-memory" property.
>From the "SDHCI: add a qtest and fix few issues" series:
- 12,13: fix registers
- 14,15: boards can specify which SDHCI Spec to use (v2 and v3 so far)
- 15-20: HCI qtest

Regards,

Phil.

$ git backport-diff
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/20:[----] [-C] 'sdhci: clean up includes'
002/20:[0004] [FC] 'sdhci: use deposit64()'
003/20:[----] [--] 'sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"'
004/20:[----] [--] 'sdhci: refactor same sysbus/pci properties into a common one'
005/20:[----] [--] 'sdhci: refactor common sysbus/pci realize() into sdhci_realizefn()'
006/20:[----] [--] 'sdhci: refactor common sysbus/pci class_init() into sdhci_class_init()'
007/20:[0001] [FC] 'sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()'
008/20:[----] [--] 'sdhci: use qemu_log_mask(UNIMP) instead of fprintf()'
009/20:[0004] [FC] 'sdhci: convert the DPRINT() calls into trace events'
010/20:[down] 'sdhci: add a GPIO for the access control LED'
011/20:[0032] [FC] 'sdhci: add a "dma-memory" property'
012/20:[0006] [FC] 'sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only'
013/20:[down] 'sdhci: Implement write method of ACMD12ERRSTS register'
014/20:[----] [-C] 'sdhci: add a "sd-spec-version" property'
015/20:[----] [-C] 'sdhci: some ARM boards do support SD_HOST_SPECv3_VERS'
016/20:[0001] [FC] 'sdhci: add qtest to check the SD Spec version'
017/20:[----] [--] 'sdhci: add check_capab_readonly() qtest'
018/20:[----] [--] 'sdhci: add a check_capab_baseclock() qtest'
019/20:[----] [--] 'sdhci: add a check_capab_sdma() qtest'
020/20:[----] [--] 'sdhci: add a check_capab_v3() qtest'

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

Andrey Smirnov (1):
  sdhci: Implement write method of ACMD12ERRSTS register

Philippe Mathieu-Daudé (19):
  sdhci: clean up includes
  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 GPIO for the access control LED
  sdhci: add a "dma-memory" property
  sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only
  sdhci: add a "sd-spec-version" property
  sdhci: some ARM boards do support SD_HOST_SPECv3_VERS
  sdhci: add qtest to check the SD Spec version
  sdhci: add check_capab_readonly() qtest
  sdhci: add a check_capab_baseclock() qtest
  sdhci: add a check_capab_sdma() qtest
  sdhci: add a check_capab_v3() qtest

 include/hw/sd/sdhci.h        |  22 +++-
 hw/sd/sdhci-internal.h       |   9 +-
 hw/arm/bcm2835_peripherals.c |   7 ++
 hw/arm/fsl-imx6.c            |   6 +
 hw/arm/xilinx_zynq.c         |   2 +
 hw/sd/sdhci.c                | 267 +++++++++++++++++++++++++------------------
 hw/sd/trace-events           |  15 +++
 tests/sdhci-test.c           | 152 ++++++++++++++++++++++++
 tests/Makefile.include       |   2 +
 9 files changed, 359 insertions(+), 123 deletions(-)
 create mode 100644 tests/sdhci-test.c

-- 
2.15.1

[Qemu-devel] [PATCH v2 01/20] sdhci: clean up includes

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 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 b064a087c9..b7d2a20985 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -29,6 +29,7 @@
 #include "sysemu/dma.h"
 #include "qemu/timer.h"
 #include "qemu/bitops.h"
+#include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
 #include "qemu/log.h"
 
-- 
2.15.1

[Qemu-devel] [PATCH v2 02/20] 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 b7d2a20985..3c78033d49 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1132,12 +1132,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, 0, 32, value);
         break;
     case SDHC_ADMASYSADDR + 4:
-        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
-                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
+        s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
         break;
     case SDHC_FEAER:
         s->acmd12errsts |= value;
-- 
2.15.1

[Qemu-devel] [PATCH v2 03/20] 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>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 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 3c78033d49..2823da00da 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)
@@ -1052,7 +1051,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 v2 04/20] 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 2823da00da..dbdfd54350 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1265,13 +1265,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);
@@ -1304,7 +1308,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;
 }
 
@@ -1319,14 +1323,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)
 {
@@ -1359,7 +1356,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;
 }
@@ -1373,6 +1370,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

[Qemu-devel] [PATCH v2 05/20] sdhci: refactor common sysbus/pci realize() into sdhci_realizefn()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 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 dbdfd54350..fc5bac5cb9 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1191,6 +1191,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);
@@ -1281,12 +1290,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);
 }
 
@@ -1343,11 +1351,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 v2 06/20] 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 fc5bac5cb9..f574495b19 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1283,6 +1283,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)
@@ -1306,7 +1316,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;
@@ -1314,10 +1323,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 = {
@@ -1361,10 +1368,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 v2 07/20] 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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f574495b19..53c1f11855 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -31,6 +31,7 @@
 #include "qemu/bitops.h"
 #include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
 
 /* host controller debug messages */
@@ -1200,17 +1201,20 @@ 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);
+}
+
 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)
@@ -1311,6 +1315,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);
 }
 
@@ -1364,11 +1370,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

[Qemu-devel] [PATCH v2 08/20] 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 53c1f11855..ca71a70148 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -945,7 +945,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;
     }
 
@@ -1149,8 +1150,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

[Qemu-devel] [PATCH v2 09/20] 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>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 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 ca71a70148..ba3afddcf2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -33,30 +33,7 @@
 #include "sdhci-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) {
@@ -653,15 +628,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 */
@@ -744,8 +718,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;
@@ -753,8 +726,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;
             }
@@ -765,15 +737,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;
                 }
@@ -809,7 +781,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;
             }
 
@@ -817,7 +789,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;
             }
 
@@ -826,14 +798,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 {
@@ -868,8 +840,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;
@@ -899,8 +871,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;
@@ -952,7 +923,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;
 }
 
@@ -1154,8 +1125,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..e4e26c6d73 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" PRIx64 ", 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" PRIx64 "] %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

[Qemu-devel] [PATCH v2 10/20] sdhci: add a GPIO for the access control LED

[Philippe Mathieu-Daudé]

It blinks to caution the user not to remove the card while the SD card is
being accessed.
So far it only emit a trace event.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 1b6a98d578..5391430c59 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -47,6 +47,8 @@ typedef struct SDHCIState {
     qemu_irq eject_cb;
     qemu_irq ro_cb;
     qemu_irq irq;
+    qemu_irq access_led;
+    int access_led_level;
 
     uint32_t sdmasysad;    /* SDMA System Address register */
     uint16_t blksize;      /* Host DMA Buff Boundary and Transfer BlkSize Reg */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ba3afddcf2..866be44db5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -202,6 +202,16 @@ static void sdhci_poweron_reset(DeviceState *dev)
     }
 }
 
+static void sdhci_led_handler(void *opaque, int line, int level)
+{
+    SDHCIState *s = (SDHCIState *)opaque;
+
+    if (s->access_led_level != level) {
+        trace_sdhci_led(level);
+        s->access_led_level = level;
+    }
+}
+
 static void sdhci_data_transfer(void *opaque);
 
 static void sdhci_send_command(SDHCIState *s)
@@ -1050,6 +1060,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;
         }
+        qemu_set_irq(s->access_led, s->hostctl & 1);
         break;
     case SDHC_CLKCON:
         if (!(mask & 0xFF000000)) {
@@ -1160,6 +1171,7 @@ static void sdhci_initfn(SDHCIState *s)
     qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
                         TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
 
+    s->access_led = qemu_allocate_irq(sdhci_led_handler, s, 0);
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
 }
@@ -1185,6 +1197,7 @@ static void sdhci_uninitfn(SDHCIState *s)
     timer_del(s->transfer_timer);
     timer_free(s->transfer_timer);
 
+    qemu_free_irq(s->access_led);
     qemu_free_irq(s->eject_cb);
     qemu_free_irq(s->ro_cb);
 }
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index e4e26c6d73..f821db2046 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" PRIx64 "] %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

[Qemu-devel] [PATCH v2 11/20] 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>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 include/hw/sd/sdhci.h |  2 ++
 hw/sd/sdhci.c         | 36 +++++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 5391430c59..715048d77b 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 866be44db5..0f3ff657cf 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -505,7 +505,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                     s->blkcnt--;
                 }
             }
-            dma_memory_write(&address_space_memory, s->sdmasysad,
+            dma_memory_write(&s->dma_as, s->sdmasysad,
                              &s->fifo_buffer[begin], s->data_count - begin);
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
@@ -527,7 +527,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                 s->data_count = block_size;
                 boundary_count -= block_size - begin;
             }
-            dma_memory_read(&address_space_memory, s->sdmasysad,
+            dma_memory_read(&s->dma_as, s->sdmasysad,
                             &s->fifo_buffer[begin], s->data_count - begin);
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
@@ -565,11 +565,9 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
         for (n = 0; n < datacnt; n++) {
             s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
         }
-        dma_memory_write(&address_space_memory, s->sdmasysad, s->fifo_buffer,
-                         datacnt);
+        dma_memory_write(&s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
     } else {
-        dma_memory_read(&address_space_memory, s->sdmasysad, s->fifo_buffer,
-                        datacnt);
+        dma_memory_read(&s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
         for (n = 0; n < datacnt; n++) {
             sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
         }
@@ -593,7 +591,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:
-        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
+        dma_memory_read(&s->dma_as, entry_addr, (uint8_t *)&adma2,
                         sizeof(adma2));
         adma2 = le64_to_cpu(adma2);
         /* The spec does not specify endianness of descriptor table.
@@ -605,7 +603,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,
+        dma_memory_read(&s->dma_as, entry_addr, (uint8_t *)&adma1,
                         sizeof(adma1));
         adma1 = le32_to_cpu(adma1);
         dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
@@ -618,12 +616,12 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         }
         break;
     case SDHC_CTRL_ADMA2_64:
-        dma_memory_read(&address_space_memory, entry_addr,
+        dma_memory_read(&s->dma_as, entry_addr,
                         (uint8_t *)(&dscr->attr), 1);
-        dma_memory_read(&address_space_memory, entry_addr + 2,
+        dma_memory_read(&s->dma_as, entry_addr + 2,
                         (uint8_t *)(&dscr->length), 2);
         dscr->length = le16_to_cpu(dscr->length);
-        dma_memory_read(&address_space_memory, entry_addr + 4,
+        dma_memory_read(&s->dma_as, entry_addr + 4,
                         (uint8_t *)(&dscr->addr), 8);
         dscr->attr = le64_to_cpu(dscr->attr);
         dscr->attr &= 0xfffffff8;
@@ -682,7 +680,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;
@@ -706,7 +704,7 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_read(&address_space_memory, dscr.addr,
+                    dma_memory_read(&s->dma_as, dscr.addr,
                                     &s->fifo_buffer[begin],
                                     s->data_count - begin);
                     dscr.addr += s->data_count - begin;
@@ -1183,10 +1181,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);
 }
 
@@ -1269,6 +1277,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

[Qemu-devel] [PATCH v2 12/20] sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only

[Philippe Mathieu-Daudé]

running qtests:

  $ make check-qtest-arm
    GTESTER check-qtest-arm
  SDHC rd_4b @0x44 not implemented
  SDHC wr_4b @0x40 <- 0x89abcdef not implemented
  SDHC wr_4b @0x44 <- 0x01234567 not implemented

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 715048d77b..a8ffac9dba 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -76,8 +76,8 @@ typedef struct SDHCIState {
     uint16_t acmd12errsts; /* Auto CMD12 error status register */
     uint64_t admasysaddr;  /* ADMA System Address Register */
 
-    uint32_t capareg;      /* Capabilities Register */
-    uint32_t maxcurr;      /* Maximum Current Capabilities Register */
+    uint64_t capareg;      /* Capabilities Register */
+    uint64_t maxcurr;      /* Maximum Current Capabilities Register */
     uint8_t  *fifo_buffer; /* SD host i/o FIFO buffer */
     uint32_t buf_maxsz;
     uint16_t data_count;   /* current element in FIFO buffer */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0f3ff657cf..9c1b28d9dd 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -906,10 +906,16 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = s->acmd12errsts;
         break;
     case SDHC_CAPAREG:
-        ret = s->capareg;
+        ret = (uint32_t)s->capareg;
+        break;
+    case SDHC_CAPAREG + 4:
+        ret = (uint32_t)(s->capareg >> 32);
         break;
     case SDHC_MAXCURR:
-        ret = s->maxcurr;
+        ret = (uint32_t)s->maxcurr;
+        break;
+    case SDHC_MAXCURR + 4:
+        ret = (uint32_t)(s->maxcurr >> 32);
         break;
     case SDHC_ADMAERR:
         ret =  s->admaerr;
@@ -1129,6 +1135,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         }
         sdhci_update_irq(s);
         break;
+
+    case SDHC_CAPAREG:
+    case SDHC_CAPAREG + 4:
+    case SDHC_MAXCURR:
+    case SDHC_MAXCURR + 4:
+        qemu_log_mask(LOG_GUEST_ERROR, "SDHC wr_%ub @0x%02" HWADDR_PRIx
+                      " <- 0x%08x read-only\n", size, offset, value >> shift);
+        break;
+
     default:
         qemu_log_mask(LOG_UNIMP, "SDHC wr_%ub @0x%02" HWADDR_PRIx " <- 0x%08x "
                       "not implemented\n", size, offset, value >> shift);
@@ -1272,9 +1287,9 @@ 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,
+    DEFINE_PROP_UINT64("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
     DEFINE_PROP_LINK("dma-memory", SDHCIState, dma_mr,
-- 
2.15.1

[Qemu-devel] [PATCH v2 13/20] sdhci: Implement write method of ACMD12ERRSTS register

[Philippe Mathieu-Daudé]

From: Andrey Smirnov <andrew.smirnov@gmail.com>

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 9c1b28d9dd..64a9b26e0d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1124,6 +1124,9 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
     case SDHC_ADMASYSADDR + 4:
         s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
         break;
+    case SDHC_ACMD12ERRSTS:
+        MASKED_WRITE(s->acmd12errsts, mask, value);
+        break;
     case SDHC_FEAER:
         s->acmd12errsts |= value;
         s->errintsts |= (value >> 16) & s->errintstsen;
-- 
2.15.1

[Qemu-devel] [PATCH v2 14/20] sdhci: add a "sd-spec-version" property

[Philippe Mathieu-Daudé]

default to SDHCI v2

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index e941bc2386..7e4a9d79d1 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -210,9 +210,9 @@
 /* Slot interrupt status */
 #define SDHC_SLOT_INT_STATUS            0xFC
 
-/* HWInit Host Controller Version Register 0x0401 */
+/* HWInit Host Controller Version Register */
 #define SDHC_HCVER                      0xFE
-#define SD_HOST_SPECv2_VERS             0x2401
+#define SDHC_HCVER_VENDOR               0x24
 
 #define SDHC_REGISTERS_MAP_SIZE         0x100
 #define SDHC_INSERTION_DELAY            (NANOSECONDS_PER_SECOND)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index a8ffac9dba..68f5902cdf 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -89,6 +89,9 @@ typedef struct SDHCIState {
     /* Force Event Auto CMD12 Error Interrupt Reg - write only */
     /* Force Event Error Interrupt Register- write only */
     /* RO Host Controller Version Register always reads as 0x2401 */
+    struct {
+        uint8_t spec_version;
+    } capabilities;
 } SDHCIState;
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
@@ -98,4 +101,11 @@ typedef struct SDHCIState {
 #define SYSBUS_SDHCI(obj)                               \
      OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
 
+/* Host Controller Specification Version */
+enum sdhci_spec_version {
+    SD_HOST_SPECv1_VERS     = 0x00,
+    SD_HOST_SPECv2_VERS     = 0x01,
+    SD_HOST_SPECv3_VERS     = 0x02
+};
+
 #endif /* SDHCI_H */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 64a9b26e0d..926f6d1958 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -927,7 +927,8 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = (uint32_t)(s->admasysaddr >> 32);
         break;
     case SDHC_SLOT_INT_STATUS:
-        ret = (SD_HOST_SPECv2_VERS << 16) | sdhci_slotint(s);
+        ret = (SDHC_HCVER_VENDOR << 24) | (s->capabilities.spec_version << 16);
+        ret |= sdhci_slotint(s);
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
@@ -1290,6 +1291,8 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_properties[] = {
+    DEFINE_PROP_UINT8("sd-spec-version", SDHCIState,
+                      capabilities.spec_version, SD_HOST_SPECv2_VERS),
     DEFINE_PROP_UINT64("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
-- 
2.15.1

[Qemu-devel] [PATCH v2 15/20] sdhci: some ARM boards do support SD_HOST_SPECv3_VERS

[Philippe Mathieu-Daudé]

set the property with object_property_set_uint() or qdev_prop_set_uint8().

[Zynq part based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2015.2]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/bcm2835_peripherals.c | 7 +++++++
 hw/arm/fsl-imx6.c            | 6 ++++++
 hw/arm/xilinx_zynq.c         | 2 ++
 3 files changed, 15 insertions(+)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 12e0dd11af..ea57b3ecac 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -269,6 +269,13 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    object_property_set_uint(OBJECT(&s->sdhci), SD_HOST_SPECv3_VERS,
+                             "sd-spec-version", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
     object_property_set_bool(OBJECT(&s->sdhci), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 59ef33efa9..c474e707af 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -348,6 +348,12 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
             { FSL_IMX6_uSDHC4_ADDR, FSL_IMX6_uSDHC4_IRQ },
         };
 
+        object_property_set_uint(OBJECT(&s->esdhc[i]), SD_HOST_SPECv3_VERS,
+                                 "sd-spec-version", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_bool(OBJECT(&s->esdhc[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..22e909afc9 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -248,6 +248,7 @@ static void zynq_init(MachineState *machine)
     gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
 
     dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
+    qdev_prop_set_uint8(dev, "sd-spec-version", SD_HOST_SPECv3_VERS);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]);
@@ -259,6 +260,7 @@ static void zynq_init(MachineState *machine)
     object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
 
     dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
+    qdev_prop_set_uint8(dev, "sd-spec-version", SD_HOST_SPECv3_VERS);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
-- 
2.15.1

[Qemu-devel] [PATCH v2 16/20] sdhci: add qtest to check the SD Spec version

[Philippe Mathieu-Daudé]

with check_specs_version()

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdhci-test.c     | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |  2 ++
 2 files changed, 76 insertions(+)
 create mode 100644 tests/sdhci-test.c

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
new file mode 100644
index 0000000000..4ebe1e349b
--- /dev/null
+++ b/tests/sdhci-test.c
@@ -0,0 +1,74 @@
+/*
+ * QTest testcase for SDHCI controllers
+ *
+ * Written by Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+#define SDHC_HCVER                      0xFE
+
+static const struct sdhci_t {
+    const char *arch;
+    const char *machine;
+    struct {
+        uintptr_t addr;
+        uint8_t version;
+    } sdhci;
+} models[] = {
+    { "arm",    "smdkc210",
+        {0x12510000, 2} },
+    { "arm",    "sabrelite",
+        {0x02190000, 3} },
+    { "arm",    "raspi2",           /* bcm2835 */
+        {0x3f300000, 3} },
+    { "arm",    "xilinx-zynq-a9",   /* exynos4210 */
+        {0xe0100000, 3} },
+};
+
+static uint32_t sdhci_readl(uintptr_t base, uint32_t reg_addr)
+{
+    QTestState *qtest = global_qtest;
+
+    return qtest_readl(qtest, base + reg_addr);
+}
+
+static void check_specs_version(uintptr_t addr, uint8_t version)
+{
+    uint32_t v;
+
+    v = sdhci_readl(addr, SDHC_HCVER);
+    v &= 0xff;
+    v += 1;
+    g_assert_cmpuint(v, ==, version);
+}
+
+static void test_machine(const void *data)
+{
+    const struct sdhci_t *test = data;
+
+    global_qtest = qtest_startf("-machine %s -d unimp", test->machine);
+
+    check_specs_version(test->sdhci.addr, test->sdhci.version);
+
+    qtest_quit(global_qtest);
+}
+
+int main(int argc, char *argv[])
+{
+    char *name;
+    int i;
+
+    g_test_init(&argc, &argv, NULL);
+
+    for (i = 0; i < ARRAY_SIZE(models); i++) {
+        name = g_strdup_printf("sdhci/%s", models[i].machine);
+        qtest_add_data_func(name, &models[i], test_machine);
+        g_free(name);
+    }
+
+    return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index c002352134..af7f324e07 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -355,6 +355,7 @@ check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
 gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 gcov-files-arm-y += hw/timer/arm_mptimer.c
+check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 
@@ -609,6 +610,7 @@ tests/test-qht-par$(EXESUF): tests/test-qht-par.o tests/qht-bench$(EXESUF) $(tes
 tests/qht-bench$(EXESUF): tests/qht-bench.o $(test-util-obj-y)
 tests/test-bufferiszero$(EXESUF): tests/test-bufferiszero.o $(test-util-obj-y)
 tests/atomic_add-bench$(EXESUF): tests/atomic_add-bench.o $(test-util-obj-y)
+tests/sdhci-test$(EXESUF): tests/sdhci-test.o
 
 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
 	hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
-- 
2.15.1

[Qemu-devel] [PATCH v2 17/20] sdhci: add check_capab_readonly() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdhci-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 4ebe1e349b..01373a69df 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -9,6 +9,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 
+#define SDHC_CAPAB                      0x40
 #define SDHC_HCVER                      0xFE
 
 static const struct sdhci_t {
@@ -36,6 +37,20 @@ static uint32_t sdhci_readl(uintptr_t base, uint32_t reg_addr)
     return qtest_readl(qtest, base + reg_addr);
 }
 
+static uint64_t sdhci_readq(uintptr_t base, uint32_t reg_addr)
+{
+    QTestState *qtest = global_qtest;
+
+    return qtest_readq(qtest, base + reg_addr);
+}
+
+static void sdhci_writeq(uintptr_t base, uint32_t reg_addr, uint64_t value)
+{
+    QTestState *qtest = global_qtest;
+
+    qtest_writeq(qtest, base + reg_addr, value);
+}
+
 static void check_specs_version(uintptr_t addr, uint8_t version)
 {
     uint32_t v;
@@ -46,6 +61,20 @@ static void check_specs_version(uintptr_t addr, uint8_t version)
     g_assert_cmpuint(v, ==, version);
 }
 
+static void check_capab_readonly(uintptr_t addr)
+{
+    const uint64_t vrand = 0x123456789abcdef;
+    uint64_t capab0, capab1;
+
+    capab0 = sdhci_readq(addr, SDHC_CAPAB);
+    g_assert_cmpuint(capab0, !=, vrand);
+
+    sdhci_writeq(addr, SDHC_CAPAB, vrand);
+    capab1 = sdhci_readq(addr, SDHC_CAPAB);
+    g_assert_cmpuint(capab1, !=, vrand);
+    g_assert_cmpuint(capab1, ==, capab0);
+}
+
 static void test_machine(const void *data)
 {
     const struct sdhci_t *test = data;
@@ -53,6 +82,7 @@ static void test_machine(const void *data)
     global_qtest = qtest_startf("-machine %s -d unimp", test->machine);
 
     check_specs_version(test->sdhci.addr, test->sdhci.version);
+    check_capab_readonly(test->sdhci.addr);
 
     qtest_quit(global_qtest);
 }
-- 
2.15.1

[Qemu-devel] [PATCH v2 18/20] sdhci: add a check_capab_baseclock() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdhci-test.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 01373a69df..966bd00499 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -7,9 +7,11 @@
  * See the COPYING file in the top-level directory.
  */
 #include "qemu/osdep.h"
+#include "hw/registerfields.h"
 #include "libqtest.h"
 
 #define SDHC_CAPAB                      0x40
+FIELD(SDHC_CAPAB, BASECLKFREQ,               8, 8); /* since v2 */
 #define SDHC_HCVER                      0xFE
 
 static const struct sdhci_t {
@@ -18,16 +20,17 @@ static const struct sdhci_t {
     struct {
         uintptr_t addr;
         uint8_t version;
+        uint8_t baseclock;
     } sdhci;
 } models[] = {
     { "arm",    "smdkc210",
-        {0x12510000, 2} },
+        {0x12510000, 2, 0} },
     { "arm",    "sabrelite",
-        {0x02190000, 3} },
+        {0x02190000, 3, 0} },
     { "arm",    "raspi2",           /* bcm2835 */
-        {0x3f300000, 3} },
+        {0x3f300000, 3, 52} },
     { "arm",    "xilinx-zynq-a9",   /* exynos4210 */
-        {0xe0100000, 3} },
+        {0xe0100000, 3, 0} },
 };
 
 static uint32_t sdhci_readl(uintptr_t base, uint32_t reg_addr)
@@ -75,6 +78,18 @@ static void check_capab_readonly(uintptr_t addr)
     g_assert_cmpuint(capab1, ==, capab0);
 }
 
+static void check_capab_baseclock(uintptr_t addr, uint8_t expected_freq)
+{
+    uint64_t capab, capab_freq;
+
+    if (!expected_freq) {
+        return;
+    }
+    capab = sdhci_readq(addr, SDHC_CAPAB);
+    capab_freq = FIELD_EX64(capab, SDHC_CAPAB, BASECLKFREQ);
+    g_assert_cmpuint(capab_freq, ==, expected_freq);
+}
+
 static void test_machine(const void *data)
 {
     const struct sdhci_t *test = data;
@@ -83,6 +98,7 @@ static void test_machine(const void *data)
 
     check_specs_version(test->sdhci.addr, test->sdhci.version);
     check_capab_readonly(test->sdhci.addr);
+    check_capab_baseclock(test->sdhci.addr, test->sdhci.baseclock);
 
     qtest_quit(global_qtest);
 }
-- 
2.15.1

[Qemu-devel] [PATCH v2 19/20] sdhci: add a check_capab_sdma() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdhci-test.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 966bd00499..b7646bccc6 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -12,6 +12,7 @@
 
 #define SDHC_CAPAB                      0x40
 FIELD(SDHC_CAPAB, BASECLKFREQ,               8, 8); /* since v2 */
+FIELD(SDHC_CAPAB, SDMA,                     22, 1);
 #define SDHC_HCVER                      0xFE
 
 static const struct sdhci_t {
@@ -21,16 +22,19 @@ static const struct sdhci_t {
         uintptr_t addr;
         uint8_t version;
         uint8_t baseclock;
+        struct {
+            bool sdma;
+        } capab;
     } sdhci;
 } models[] = {
     { "arm",    "smdkc210",
-        {0x12510000, 2, 0} },
+        {0x12510000, 2, 0, {1} } },
     { "arm",    "sabrelite",
-        {0x02190000, 3, 0} },
+        {0x02190000, 3, 0, {1} } },
     { "arm",    "raspi2",           /* bcm2835 */
-        {0x3f300000, 3, 52} },
+        {0x3f300000, 3, 52, {0} } },
     { "arm",    "xilinx-zynq-a9",   /* exynos4210 */
-        {0xe0100000, 3, 0} },
+        {0xe0100000, 3, 0, {1} } },
 };
 
 static uint32_t sdhci_readl(uintptr_t base, uint32_t reg_addr)
@@ -90,6 +94,15 @@ static void check_capab_baseclock(uintptr_t addr, uint8_t expected_freq)
     g_assert_cmpuint(capab_freq, ==, expected_freq);
 }
 
+static void check_capab_sdma(uintptr_t addr, bool supported)
+{
+    uint64_t capab, capab_sdma;
+
+    capab = sdhci_readq(addr, SDHC_CAPAB);
+    capab_sdma = FIELD_EX64(capab, SDHC_CAPAB, SDMA);
+    g_assert_cmpuint(capab_sdma, ==, supported);
+}
+
 static void test_machine(const void *data)
 {
     const struct sdhci_t *test = data;
@@ -98,6 +111,7 @@ static void test_machine(const void *data)
 
     check_specs_version(test->sdhci.addr, test->sdhci.version);
     check_capab_readonly(test->sdhci.addr);
+    check_capab_sdma(test->sdhci.addr, test->sdhci.capab.sdma);
     check_capab_baseclock(test->sdhci.addr, test->sdhci.baseclock);
 
     qtest_quit(global_qtest);
-- 
2.15.1

[Qemu-devel] [PATCH v2 20/20] sdhci: add a check_capab_v3() qtest

[Philippe Mathieu-Daudé]

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

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index b7646bccc6..60b28f27b3 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -13,6 +13,8 @@
 #define SDHC_CAPAB                      0x40
 FIELD(SDHC_CAPAB, BASECLKFREQ,               8, 8); /* since v2 */
 FIELD(SDHC_CAPAB, SDMA,                     22, 1);
+FIELD(SDHC_CAPAB, SDR,                      32, 3); /* since v3 */
+FIELD(SDHC_CAPAB, DRIVER,                   36, 3); /* since v3 */
 #define SDHC_HCVER                      0xFE
 
 static const struct sdhci_t {
@@ -103,6 +105,21 @@ static void check_capab_sdma(uintptr_t addr, bool supported)
     g_assert_cmpuint(capab_sdma, ==, supported);
 }
 
+static void check_capab_v3(uintptr_t addr, uint8_t version)
+{
+    uint64_t capab, capab_v3;
+
+    if (version >= 3) {
+        return;
+    }
+    /* before v3 those fields are RESERVED */
+    capab = sdhci_readq(addr, SDHC_CAPAB);
+    capab_v3 = FIELD_EX64(capab, SDHC_CAPAB, SDR);
+    g_assert_cmpuint(capab_v3, ==, 0);
+    capab_v3 = FIELD_EX64(capab, SDHC_CAPAB, DRIVER);
+    g_assert_cmpuint(capab_v3, ==, 0);
+}
+
 static void test_machine(const void *data)
 {
     const struct sdhci_t *test = data;
@@ -111,6 +128,7 @@ static void test_machine(const void *data)
 
     check_specs_version(test->sdhci.addr, test->sdhci.version);
     check_capab_readonly(test->sdhci.addr);
+    check_capab_v3(test->sdhci.addr, test->sdhci.version);
     check_capab_sdma(test->sdhci.addr, test->sdhci.capab.sdma);
     check_capab_baseclock(test->sdhci.addr, test->sdhci.baseclock);
 
-- 
2.15.1

Re: [Qemu-devel] [PATCH v2 16/20] sdhci: add qtest to check the SD Spec version

[Stefan Hajnoczi]

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

On Fri, Dec 15, 2017 at 12:15:43AM -0300, Philippe Mathieu-Daudé wrote:
> with check_specs_version()
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/sdhci-test.c     | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |  2 ++
>  2 files changed, 76 insertions(+)
>  create mode 100644 tests/sdhci-test.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 17/20] sdhci: add check_capab_readonly() qtest

[Stefan Hajnoczi]

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

On Fri, Dec 15, 2017 at 12:15:44AM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/sdhci-test.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 18/20] sdhci: add a check_capab_baseclock() qtest

[Stefan Hajnoczi]

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

On Fri, Dec 15, 2017 at 12:15:45AM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/sdhci-test.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 19/20] sdhci: add a check_capab_sdma() qtest

[Stefan Hajnoczi]

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

On Fri, Dec 15, 2017 at 12:15:46AM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/sdhci-test.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
> index 966bd00499..b7646bccc6 100644
> --- a/tests/sdhci-test.c
> +++ b/tests/sdhci-test.c
> @@ -12,6 +12,7 @@
>  
>  #define SDHC_CAPAB                      0x40
>  FIELD(SDHC_CAPAB, BASECLKFREQ,               8, 8); /* since v2 */
> +FIELD(SDHC_CAPAB, SDMA,                     22, 1);
>  #define SDHC_HCVER                      0xFE
>  
>  static const struct sdhci_t {
> @@ -21,16 +22,19 @@ static const struct sdhci_t {
>          uintptr_t addr;
>          uint8_t version;
>          uint8_t baseclock;
> +        struct {
> +            bool sdma;
> +        } capab;
>      } sdhci;
>  } models[] = {
>      { "arm",    "smdkc210",
> -        {0x12510000, 2, 0} },
> +        {0x12510000, 2, 0, {1} } },
>      { "arm",    "sabrelite",
> -        {0x02190000, 3, 0} },
> +        {0x02190000, 3, 0, {1} } },
>      { "arm",    "raspi2",           /* bcm2835 */
> -        {0x3f300000, 3, 52} },
> +        {0x3f300000, 3, 52, {0} } },
>      { "arm",    "xilinx-zynq-a9",   /* exynos4210 */
> -        {0xe0100000, 3, 0} },
> +        {0xe0100000, 3, 0, {1} } },
>  };
>  
>  static uint32_t sdhci_readl(uintptr_t base, uint32_t reg_addr)
> @@ -90,6 +94,15 @@ static void check_capab_baseclock(uintptr_t addr, uint8_t expected_freq)
>      g_assert_cmpuint(capab_freq, ==, expected_freq);
>  }
>  
> +static void check_capab_sdma(uintptr_t addr, bool supported)
> +{
> +    uint64_t capab, capab_sdma;
> +
> +    capab = sdhci_readq(addr, SDHC_CAPAB);
> +    capab_sdma = FIELD_EX64(capab, SDHC_CAPAB, SDMA);
> +    g_assert_cmpuint(capab_sdma, ==, supported);
> +}
> +
>  static void test_machine(const void *data)
>  {
>      const struct sdhci_t *test = data;
> @@ -98,6 +111,7 @@ static void test_machine(const void *data)
>  
>      check_specs_version(test->sdhci.addr, test->sdhci.version);
>      check_capab_readonly(test->sdhci.addr);
> +    check_capab_sdma(test->sdhci.addr, test->sdhci.capab.sdma);
>      check_capab_baseclock(test->sdhci.addr, test->sdhci.baseclock);
>  
>      qtest_quit(global_qtest);
> -- 
> 2.15.1
> 

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

Re: [Qemu-devel] [PATCH v2 20/20] sdhci: add a check_capab_v3() qtest

[Stefan Hajnoczi]

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

On Fri, Dec 15, 2017 at 12:15:47AM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/sdhci-test.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 16/20] sdhci: add qtest to check the SD Spec version

[Philippe Mathieu-Daudé]

On 12/15/2017 12:15 AM, Philippe Mathieu-Daudé wrote:
> +static const struct sdhci_t {
> +    const char *arch;
> +    const char *machine;
> +    struct {
> +        uintptr_t addr;
> +        uint8_t version;
> +    } sdhci;
> +} models[] = {
> +    { "arm",    "smdkc210",
> +        {0x12510000, 2} },
> +    { "arm",    "sabrelite",
> +        {0x02190000, 3} },
> +    { "arm",    "raspi2",           /* bcm2835 */
> +        {0x3f300000, 3} },
> +    { "arm",    "xilinx-zynq-a9",   /* exynos4210 */

Just noticed this comment is for the smdkc210 above,
I'll fix in respin.

> +        {0xe0100000, 3} },
> +};

Re: [Qemu-devel] [PATCH v2 04/20] sdhci: refactor same sysbus/pci properties into a common one

[Alistair Francis]

On Thu, Dec 14, 2017 at 7:15 PM, 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>

I'm still unsure about this. Won't this leave us with properties that
have no impact on the device? That seems very confusing to me.

Alistair

> ---
>  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 2823da00da..dbdfd54350 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1265,13 +1265,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);
> @@ -1304,7 +1308,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;
>  }
>
> @@ -1319,14 +1323,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)
>  {
> @@ -1359,7 +1356,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;
>  }
> @@ -1373,6 +1370,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 v2 02/20] sdhci: use deposit64()

[Alistair Francis]

On Thu, Dec 14, 2017 at 7:15 PM, 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>

I just looked at the surrounding code and realised what mask actually
is set to. Now I understand what you mean.

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

Alistair


> ---
>  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 b7d2a20985..3c78033d49 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1132,12 +1132,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, 0, 32, value);
>          break;
>      case SDHC_ADMASYSADDR + 4:
> -        s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
> -                ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
> +        s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>          break;
>      case SDHC_FEAER:
>          s->acmd12errsts |= value;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v2 08/20] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Alistair Francis]

On Thu, Dec 14, 2017 at 7:15 PM, 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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 53c1f11855..ca71a70148 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -945,7 +945,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;
>      }
>
> @@ -1149,8 +1150,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 v2 13/20] sdhci: Implement write method of ACMD12ERRSTS register

[Alistair Francis]

On Thu, Dec 14, 2017 at 7:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> From: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  hw/sd/sdhci.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 9c1b28d9dd..64a9b26e0d 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1124,6 +1124,9 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>      case SDHC_ADMASYSADDR + 4:
>          s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>          break;
> +    case SDHC_ACMD12ERRSTS:
> +        MASKED_WRITE(s->acmd12errsts, mask, value);
> +        break;
>      case SDHC_FEAER:
>          s->acmd12errsts |= value;
>          s->errintsts |= (value >> 16) & s->errintstsen;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v2 04/20] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

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

Hi Alistair,

On 12/18/2017 10:13 PM, Alistair Francis wrote:
> On Thu, Dec 14, 2017 at 7:15 PM, 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>
> 
> I'm still unsure about this. Won't this leave us with properties that
> have no impact on the device? That seems very confusing to me.

(from previous series)
> [...] but aren't we now going to
> have device properties that aren't actually connected to anything?

I'm also confused :)

My understanding is the "pending-insert-quirk" worries you.
This property is dependent of the HCI IP, so regardless the HCI is
accessed through a MMIO sysbus or a PCI bus the quirk might exists (for
this property, only the BCM implementation).

With v3 series applied, the monitor 'qtree' output is:

$ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S
(qemu) info qtree
  dev: generic-sdhci, id ""
    gpio-out "sysbus-irq" 1
    sd-spec-version = 3 (0x3)
    timeout-freq = 0 (0x0)
    freq-in-mhz = true
    clock-freq = 0 (0x0)
    max-block-length = 512 (0x200)
    dma = true
    sdma = true
    adma1 = false
    adma2 = true
    suspend = true
    high-speed = true
    3v3 = true
    3v0 = false
    1v8 = true
    64bit = true
    slot-type = 0 (0x0)
    bus-speed = 7 (0x7)
    driver-strength = 7 (0x7)
    maxcurr = 0 (0x0)
    pending-insert-quirk = false
    dma-memory = ""
    mmio 00000000ff170000/0000000000000100
    bus: sd-bus
      type sdhci-bus
      dev: sd-card, id ""
        drive = ""
        spi = false

Are you worried about this output?

>> ---
>>  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 2823da00da..dbdfd54350 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1265,13 +1265,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);
>> @@ -1304,7 +1308,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;
>>  }
>>
>> @@ -1319,14 +1323,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)
>>  {
>> @@ -1359,7 +1356,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;
>>  }
>> @@ -1373,6 +1370,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
>>
>>


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

Re: [Qemu-devel] [PATCH v2 04/20] sdhci: refactor same sysbus/pci properties into a common one

[Alistair Francis]

On Fri, Dec 29, 2017 at 9:21 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Alistair,
>
> On 12/18/2017 10:13 PM, Alistair Francis wrote:
>> On Thu, Dec 14, 2017 at 7:15 PM, 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>
>>
>> I'm still unsure about this. Won't this leave us with properties that
>> have no impact on the device? That seems very confusing to me.
>
> (from previous series)
>> [...] but aren't we now going to
>> have device properties that aren't actually connected to anything?
>
> I'm also confused :)
>
> My understanding is the "pending-insert-quirk" worries you.

Exactly, my worry is having properties that aren't connected. As a
user I expect that if I set a property that will have some effect on
how QEMU runs. A un-connected property is just confusing.

> This property is dependent of the HCI IP, so regardless the HCI is
> accessed through a MMIO sysbus or a PCI bus the quirk might exists (for
> this property, only the BCM implementation).

So it has always been configuration dependent?

Alistair

>
> With v3 series applied, the monitor 'qtree' output is:
>
> $ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S
> (qemu) info qtree
>   dev: generic-sdhci, id ""
>     gpio-out "sysbus-irq" 1
>     sd-spec-version = 3 (0x3)
>     timeout-freq = 0 (0x0)
>     freq-in-mhz = true
>     clock-freq = 0 (0x0)
>     max-block-length = 512 (0x200)
>     dma = true
>     sdma = true
>     adma1 = false
>     adma2 = true
>     suspend = true
>     high-speed = true
>     3v3 = true
>     3v0 = false
>     1v8 = true
>     64bit = true
>     slot-type = 0 (0x0)
>     bus-speed = 7 (0x7)
>     driver-strength = 7 (0x7)
>     maxcurr = 0 (0x0)
>     pending-insert-quirk = false
>     dma-memory = ""
>     mmio 00000000ff170000/0000000000000100
>     bus: sd-bus
>       type sdhci-bus
>       dev: sd-card, id ""
>         drive = ""
>         spi = false
>
> Are you worried about this output?
>
>>> ---
>>>  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 2823da00da..dbdfd54350 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1265,13 +1265,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);
>>> @@ -1304,7 +1308,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;
>>>  }
>>>
>>> @@ -1319,14 +1323,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)
>>>  {
>>> @@ -1359,7 +1356,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;
>>>  }
>>> @@ -1373,6 +1370,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
>>>
>>>
>