[Qemu-devel] [PATCH v13 00/30] SDHCI: clean v1/2 Specs, UHS-I cards tuning sequence

[Qemu-devel] [PATCH v13 00/30] SDHCI: clean v1/2 Specs, UHS-I cards tuning sequence

[Philippe Mathieu-Daudé]

This series includes the last versions of both series:
- SDHCI: clean v1/v2 Specs (part 2)
- SDHCI: add tuning sequence for UHS-I cards (part 3)

Since v12:
- rebased...
- bcc'ing patchew, spaming others

Since v11:
- rebased due to conflict (IMX_USDHC fd1e5c817964)
- QSDHCI uses union (Paolo)
- do not enable UNIMP logging when running qtests

Since v10:
- rebased
- add Paolo's R-b in patch 2
- rename patch 11 subject (Alistair)
- add Alistair's R-b in "UHS-I cards tuning sequence" patches

Thanks Paolo :)

Phil.

$ git backport-diff with v11
001/30:[----] [--] 'sdhci: use error_propagate(local_err) in realize()'
002/30:[0028] [FC] 'sdhci: add qtest to check the SD capabilities register'
003/30:[0016] [FC] 'sdhci: add check_capab_readonly() qtest'
004/30:[0010] [FC] 'sdhci: add a check_capab_baseclock() qtest'
005/30:[0006] [FC] 'sdhci: add a check_capab_sdma() qtest'
006/30:[0012] [FC] 'sdhci: add qtest to check the SD Spec version'
007/30:[----] [-C] 'sdhci: add a 'spec_version property' (default to v2)'
008/30:[----] [--] 'sdhci: use a numeric value for the default CAPAB register'
009/30:[----] [--] 'sdhci: simplify sdhci_get_fifolen()'
010/30:[0029] [FC] 'sdhci: check the Spec v1 capabilities correctness'
011/30:[----] [--] 'sdhci: replace DMA magic value by BLOCK_SIZE_MASK'
012/30:[----] [--] 'sdhci: Fix 64-bit ADMA2'
013/30:[0006] [FC] 'sdhci: check Spec v2 capabilities (DMA and 64-bit bus)'
014/30:[----] [--] 'hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()'
015/30:[----] [--] 'hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)'
016/30:[----] [--] 'hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet'
017/30:[----] [-C] 'sdhci: add support for v3 capabilities'
018/30:[0006] [FC] 'sdhci: rename the hostctl1 register'
019/30:[----] [--] 'sdhci: implement the Host Control 2 register (tuning sequence)'
020/30:[----] [--] 'sdbus: add trace events'
021/30:[----] [-C] 'sdhci: implement UHS-I voltage switch'
022/30:[----] [--] 'sdhci: implement CMD/DAT[] fields in the Present State register'
023/30:[----] [--] 'hw/arm/bcm2835_peripherals: implement SDHCI Spec v3'
024/30:[----] [--] 'hw/arm/bcm2835_peripherals: change maximum block size to 1kB'
025/30:[----] [--] 'hw/arm/fsl-imx6: implement SDHCI Spec. v3'
026/30:[----] [--] 'hw/arm/xilinx_zynqmp: fix the capabilities/spec version to match the datasheet'
027/30:[----] [--] 'hw/arm/xilinx_zynqmp: enable the UHS-I mode'
028/30:[----] [--] 'sdhci: check Spec v3 capabilities qtest'
029/30:[0006] [FC] 'sdhci: add a check_capab_v3() qtest'
030/30:[0008] [FC] 'sdhci: add Spec v4.2 register definitions'

Philippe Mathieu-Daudé (29):
  sdhci: use error_propagate(local_err) in realize()
  sdhci: add qtest to check the SD capabilities register
  sdhci: add check_capab_readonly() qtest
  sdhci: add a check_capab_baseclock() qtest
  sdhci: add a check_capab_sdma() qtest
  sdhci: add qtest to check the SD Spec version
  sdhci: add a 'spec_version property' (default to v2)
  sdhci: use a numeric value for the default CAPAB register
  sdhci: simplify sdhci_get_fifolen()
  sdhci: check the Spec v1 capabilities correctness
  sdhci: replace DMA magic value by BLOCK_SIZE_MASK
  sdhci: check Spec v2 capabilities (DMA and 64-bit bus)
  hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()
  hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)
  hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet
  sdhci: add support for v3 capabilities
  sdhci: rename the hostctl1 register
  sdhci: implement the Host Control 2 register (tuning sequence)
  sdbus: add trace events
  sdhci: implement UHS-I voltage switch
  sdhci: implement CMD/DAT[] fields in the Present State register
  hw/arm/bcm2835_peripherals: implement SDHCI Spec v3
  hw/arm/bcm2835_peripherals: change maximum block size to 1kB
  hw/arm/fsl-imx6: implement SDHCI Spec. v3
  hw/arm/xilinx_zynqmp: fix the capabilities/spec version to match the datasheet
  hw/arm/xilinx_zynqmp: enable the UHS-I mode
  sdhci: check Spec v3 capabilities qtest
  sdhci: add a check_capab_v3() qtest
  sdhci: add Spec v4.2 register definitions

Sai Pavan Boddu (1):
  sdhci: Fix 64-bit ADMA2

 include/hw/sd/sd.h           |  20 +++
 include/hw/sd/sdhci.h        |   6 +-
 hw/sd/sdhci-internal.h       |  78 +++++++--
 hw/arm/bcm2835_peripherals.c |  23 +--
 hw/arm/exynos4210.c          |  14 +-
 hw/arm/fsl-imx6.c            |   7 +
 hw/arm/xilinx_zynq.c         |  53 +++---
 hw/arm/xlnx-zynqmp.c         |  30 ++--
 hw/sd/core.c                 |  61 ++++++-
 hw/sd/sd.c                   |  29 ++++
 hw/sd/sdhci.c                | 375 +++++++++++++++++++++++++++++++------------
 hw/sd/trace-events           |   9 ++
 tests/sdhci-test.c           | 252 +++++++++++++++++++++++++++++
 tests/Makefile.include       |   4 +
 14 files changed, 800 insertions(+), 161 deletions(-)
 create mode 100644 tests/sdhci-test.c

-- 
2.16.1

[Qemu-devel] [PATCH v13 01/30] sdhci: use error_propagate(local_err) in realize()

[Philippe Mathieu-Daudé]

avoid the "errp && *errp" pattern (not recommended in "qapi/error.h" comments).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ee95e78aeb..3602286f46 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1302,10 +1302,12 @@ static Property sdhci_pci_properties[] = {
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
     SDHCIState *s = PCI_SDHCI(dev);
+    Error *local_err = NULL;
 
     sdhci_initfn(s);
     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
@@ -1383,9 +1385,11 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
 {
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    Error *local_err = NULL;
 
     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
-- 
2.16.1

[Qemu-devel] [PATCH v13 02/30] sdhci: add qtest to check the SD capabilities register

[Philippe Mathieu-Daudé]

The PCI model is tested with the pc/x86_64 machine,
the SysBus model with the smdkc210/arm machine.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/sdhci-test.c     | 145 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   3 +
 2 files changed, 148 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..82f3785a72
--- /dev/null
+++ b/tests/sdhci-test.c
@@ -0,0 +1,145 @@
+/*
+ * 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.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "hw/registerfields.h"
+#include "libqtest.h"
+#include "libqos/pci-pc.h"
+#include "hw/pci/pci.h"
+
+#define SDHC_CAPAB                      0x40
+#define SDHC_HCVER                      0xFE
+
+static const struct sdhci_t {
+    const char *arch, *machine;
+    struct {
+        uintptr_t addr;
+        uint8_t version;
+        uint8_t baseclock;
+        struct {
+            bool sdma;
+            uint64_t reg;
+        } capab;
+    } sdhci;
+    struct {
+        uint16_t vendor_id, device_id;
+    } pci;
+} models[] = {
+    /* PC via PCI */
+    { "x86_64", "pc",
+        {-1,         2, 0,  {1, 0x057834b4} },
+        .pci = { PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_SDHCI } },
+
+    /* Exynos4210 */
+    { "arm",    "smdkc210",
+        {0x12510000, 2, 0,  {1, 0x5e80080} } },
+};
+
+typedef struct QSDHCI {
+    struct {
+        QPCIBus *bus;
+        QPCIDevice *dev;
+    } pci;
+    union {
+        QPCIBar mem_bar;
+        uint64_t addr;
+    };
+} QSDHCI;
+
+static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
+{
+    uint64_t val;
+
+    if (s->pci.dev) {
+        qpci_memread(s->pci.dev, s->mem_bar, reg, &val, sizeof(val));
+    } else {
+        val = qtest_readq(global_qtest, s->addr + reg);
+    }
+
+    return val;
+}
+
+/* tests */
+
+static void check_capab_capareg(QSDHCI *s, uint64_t expected_capab)
+{
+    uint64_t capab;
+
+    capab = sdhci_readq(s, SDHC_CAPAB);
+    g_assert_cmphex(capab, ==, expected_capab);
+}
+
+static QSDHCI *machine_start(const struct sdhci_t *test)
+{
+    QSDHCI *s = g_new0(QSDHCI, 1);
+
+    if (test->pci.vendor_id) {
+        /* PCI */
+        uint16_t vendor_id, device_id;
+        uint64_t barsize;
+
+        global_qtest = qtest_startf("-machine %s -device sdhci-pci",
+                                    test->machine);
+
+        s->pci.bus = qpci_init_pc(NULL);
+
+        /* Find PCI device and verify it's the right one */
+        s->pci.dev = qpci_device_find(s->pci.bus, QPCI_DEVFN(4, 0));
+        g_assert_nonnull(s->pci.dev);
+        vendor_id = qpci_config_readw(s->pci.dev, PCI_VENDOR_ID);
+        device_id = qpci_config_readw(s->pci.dev, PCI_DEVICE_ID);
+        g_assert(vendor_id == test->pci.vendor_id);
+        g_assert(device_id == test->pci.device_id);
+        s->mem_bar = qpci_iomap(s->pci.dev, 0, &barsize);
+        qpci_device_enable(s->pci.dev);
+    } else {
+        /* SysBus */
+        global_qtest = qtest_startf("-machine %s", test->machine);
+        s->addr = test->sdhci.addr;
+    }
+
+    return s;
+}
+
+static void machine_stop(QSDHCI *s)
+{
+    g_free(s->pci.dev);
+    qtest_quit(global_qtest);
+}
+
+static void test_machine(const void *data)
+{
+    const struct sdhci_t *test = data;
+    QSDHCI *s;
+
+    s = machine_start(test);
+
+    check_capab_capareg(s, test->sdhci.capab.reg);
+
+    machine_stop(s);
+}
+
+int main(int argc, char *argv[])
+{
+    const char *arch = qtest_get_arch();
+    char *name;
+    int i;
+
+    g_test_init(&argc, &argv, NULL);
+    for (i = 0; i < ARRAY_SIZE(models); i++) {
+        if (strcmp(arch, models[i].arch)) {
+            continue;
+        }
+        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 f41da235ae..52be9b3fa5 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -294,6 +294,7 @@ check-qtest-i386-y += tests/migration-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
+check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
 
@@ -367,6 +368,7 @@ 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/boot-serial-test$(EXESUF)
+check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 
@@ -822,6 +824,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
+tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
-- 
2.16.1

[Qemu-devel] [PATCH v13 03/30] sdhci: add check_capab_readonly() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/sdhci-test.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 82f3785a72..51af5eac67 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -65,6 +65,15 @@ static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
     return val;
 }
 
+static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
+{
+    if (s->pci.dev) {
+        qpci_memwrite(s->pci.dev, s->mem_bar, reg, &val, sizeof(val));
+    } else {
+        qtest_writeq(global_qtest, s->addr + reg, val);
+    }
+}
+
 /* tests */
 
 static void check_capab_capareg(QSDHCI *s, uint64_t expected_capab)
@@ -75,6 +84,20 @@ static void check_capab_capareg(QSDHCI *s, uint64_t expected_capab)
     g_assert_cmphex(capab, ==, expected_capab);
 }
 
+static void check_capab_readonly(QSDHCI *s)
+{
+    const uint64_t vrand = 0x123456789abcdef;
+    uint64_t capab0, capab1;
+
+    capab0 = sdhci_readq(s, SDHC_CAPAB);
+    g_assert_cmpuint(capab0, !=, vrand);
+
+    sdhci_writeq(s, SDHC_CAPAB, vrand);
+    capab1 = sdhci_readq(s, SDHC_CAPAB);
+    g_assert_cmpuint(capab1, !=, vrand);
+    g_assert_cmpuint(capab1, ==, capab0);
+}
+
 static QSDHCI *machine_start(const struct sdhci_t *test)
 {
     QSDHCI *s = g_new0(QSDHCI, 1);
@@ -121,6 +144,7 @@ static void test_machine(const void *data)
     s = machine_start(test);
 
     check_capab_capareg(s, test->sdhci.capab.reg);
+    check_capab_readonly(s);
 
     machine_stop(s);
 }
-- 
2.16.1

[Qemu-devel] [PATCH v13 04/30] sdhci: add a check_capab_baseclock() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/sdhci-test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 51af5eac67..d6eb3c3a48 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -14,6 +14,7 @@
 #include "hw/pci/pci.h"
 
 #define SDHC_CAPAB                      0x40
+FIELD(SDHC_CAPAB, BASECLKFREQ,               8, 8); /* since v2 */
 #define SDHC_HCVER                      0xFE
 
 static const struct sdhci_t {
@@ -98,6 +99,18 @@ static void check_capab_readonly(QSDHCI *s)
     g_assert_cmpuint(capab1, ==, capab0);
 }
 
+static void check_capab_baseclock(QSDHCI *s, uint8_t expected_freq)
+{
+    uint64_t capab, capab_freq;
+
+    if (!expected_freq) {
+        return;
+    }
+    capab = sdhci_readq(s, SDHC_CAPAB);
+    capab_freq = FIELD_EX64(capab, SDHC_CAPAB, BASECLKFREQ);
+    g_assert_cmpuint(capab_freq, ==, expected_freq);
+}
+
 static QSDHCI *machine_start(const struct sdhci_t *test)
 {
     QSDHCI *s = g_new0(QSDHCI, 1);
@@ -145,6 +158,7 @@ static void test_machine(const void *data)
 
     check_capab_capareg(s, test->sdhci.capab.reg);
     check_capab_readonly(s);
+    check_capab_baseclock(s, test->sdhci.baseclock);
 
     machine_stop(s);
 }
-- 
2.16.1

[Qemu-devel] [PATCH v13 05/30] sdhci: add a check_capab_sdma() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/sdhci-test.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index d6eb3c3a48..7c50c0482b 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -15,6 +15,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 {
@@ -111,6 +112,15 @@ static void check_capab_baseclock(QSDHCI *s, uint8_t expected_freq)
     g_assert_cmpuint(capab_freq, ==, expected_freq);
 }
 
+static void check_capab_sdma(QSDHCI *s, bool supported)
+{
+    uint64_t capab, capab_sdma;
+
+    capab = sdhci_readq(s, SDHC_CAPAB);
+    capab_sdma = FIELD_EX64(capab, SDHC_CAPAB, SDMA);
+    g_assert_cmpuint(capab_sdma, ==, supported);
+}
+
 static QSDHCI *machine_start(const struct sdhci_t *test)
 {
     QSDHCI *s = g_new0(QSDHCI, 1);
@@ -158,6 +168,7 @@ static void test_machine(const void *data)
 
     check_capab_capareg(s, test->sdhci.capab.reg);
     check_capab_readonly(s);
+    check_capab_sdma(s, test->sdhci.capab.sdma);
     check_capab_baseclock(s, test->sdhci.baseclock);
 
     machine_stop(s);
-- 
2.16.1

[Qemu-devel] [PATCH v13 06/30] sdhci: add qtest to check the SD Spec version

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/sdhci-test.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 7c50c0482b..24feea744a 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -54,6 +54,19 @@ typedef struct QSDHCI {
     };
 } QSDHCI;
 
+static uint32_t sdhci_readl(QSDHCI *s, uint32_t reg)
+{
+    uint32_t val;
+
+    if (s->pci.dev) {
+        qpci_memread(s->pci.dev, s->mem_bar, reg, &val, sizeof(val));
+    } else {
+        val = qtest_readl(global_qtest, s->addr + reg);
+    }
+
+    return val;
+}
+
 static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
 {
     uint64_t val;
@@ -78,6 +91,16 @@ static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
 
 /* tests */
 
+static void check_specs_version(QSDHCI *s, uint8_t version)
+{
+    uint32_t v;
+
+    v = sdhci_readl(s, SDHC_HCVER);
+    v &= 0xff;
+    v += 1;
+    g_assert_cmpuint(v, ==, version);
+}
+
 static void check_capab_capareg(QSDHCI *s, uint64_t expected_capab)
 {
     uint64_t capab;
@@ -166,6 +189,7 @@ static void test_machine(const void *data)
 
     s = machine_start(test);
 
+    check_specs_version(s, test->sdhci.version);
     check_capab_capareg(s, test->sdhci.capab.reg);
     check_capab_readonly(s);
     check_capab_sdma(s, test->sdhci.capab.sdma);
-- 
2.16.1

[Qemu-devel] [PATCH v13 07/30] sdhci: add a 'spec_version property' (default to v2)

[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  |  2 ++
 hw/sd/sdhci.c          | 27 +++++++++++++++++++++++----
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 0991acd724..64556480a9 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -216,9 +216,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 f8d1ba3538..2a26b46f05 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -78,6 +78,7 @@ typedef struct SDHCIState {
     /* Read-only registers */
     uint64_t capareg;      /* Capabilities Register */
     uint64_t maxcurr;      /* Maximum Current Capabilities Register */
+    uint16_t version;      /* Host Controller Version Register */
 
     uint8_t  *fifo_buffer; /* SD host i/o FIFO buffer */
     uint32_t buf_maxsz;
@@ -93,6 +94,7 @@ typedef struct SDHCIState {
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
     uint32_t quirks;
+    uint8_t sd_spec_version;
 } SDHCIState;
 
 /*
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3602286f46..17a1348f0f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -173,7 +173,8 @@ static void sdhci_reset(SDHCIState *s)
 
     timer_del(s->insert_timer);
     timer_del(s->transfer_timer);
-    /* Set all registers to 0. Capabilities registers are not cleared
+
+    /* Set all registers to 0. Capabilities/Version registers are not cleared
      * and assumed to always preserve their value, given to them during
      * initialization */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
@@ -918,7 +919,7 @@ 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 = (s->version << 16) | sdhci_slotint(s);
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "SDHC rd_%ub @0x%02" HWADDR_PRIx " "
@@ -1174,11 +1175,22 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
     }
 }
 
+static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
+{
+    if (s->sd_spec_version != 2) {
+        error_setg(errp, "Only Spec v2 is supported");
+        return;
+    }
+    s->version = (SDHC_HCVER_VENDOR << 8) | (s->sd_spec_version - 1);
+}
+
 /* --- qdev common --- */
 
 #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
-    /* Capabilities registers provide information on supported features
-     * of this specific host controller implementation */ \
+    DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
+    \
+    /* Capabilities registers provide information on supported
+     * features of this specific host controller implementation */ \
     DEFINE_PROP_UINT64("capareg", _state, capareg, SDHC_CAPAB_REG_DEFAULT), \
     DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
 
@@ -1206,6 +1218,13 @@ static void sdhci_uninitfn(SDHCIState *s)
 
 static void sdhci_common_realize(SDHCIState *s, Error **errp)
 {
+    Error *local_err = NULL;
+
+    sdhci_init_readonly_registers(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
-- 
2.16.1

[Qemu-devel] [PATCH v13 08/30] sdhci: use a numeric value for the default CAPAB register

[Philippe Mathieu-Daudé]

using many #defines is not portable when scaling to different HCI.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci.c | 74 +++++++++++++----------------------------------------------
 1 file changed, 16 insertions(+), 58 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 17a1348f0f..491e624262 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -38,67 +38,25 @@
 #define TYPE_SDHCI_BUS "sdhci-bus"
 #define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SDHCI_BUS)
 
+#define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
+
 /* Default SD/MMC host controller features information, which will be
  * presented in CAPABILITIES register of generic SD host controller at reset.
- * If not stated otherwise:
- * 0 - not supported, 1 - supported, other - prohibited.
+ *
+ * support:
+ * - 3.3v and 1.8v voltages
+ * - SDMA/ADMA1/ADMA2
+ * - high-speed
+ * max host controller R/W buffers size: 512B
+ * max clock frequency for SDclock: 52 MHz
+ * timeout clock frequency: 52 MHz
+ *
+ * does not support:
+ * - 3.0v voltage
+ * - 64-bit system bus
+ * - suspend/resume
  */
-#define SDHC_CAPAB_64BITBUS       0ul        /* 64-bit System Bus Support */
-#define SDHC_CAPAB_18V            1ul        /* Voltage support 1.8v */
-#define SDHC_CAPAB_30V            0ul        /* Voltage support 3.0v */
-#define SDHC_CAPAB_33V            1ul        /* Voltage support 3.3v */
-#define SDHC_CAPAB_SUSPRESUME     0ul        /* Suspend/resume support */
-#define SDHC_CAPAB_SDMA           1ul        /* SDMA support */
-#define SDHC_CAPAB_HIGHSPEED      1ul        /* High speed support */
-#define SDHC_CAPAB_ADMA1          1ul        /* ADMA1 support */
-#define SDHC_CAPAB_ADMA2          1ul        /* ADMA2 support */
-/* Maximum host controller R/W buffers size
- * Possible values: 512, 1024, 2048 bytes */
-#define SDHC_CAPAB_MAXBLOCKLENGTH 512ul
-/* Maximum clock frequency for SDclock in MHz
- * value in range 10-63 MHz, 0 - not defined */
-#define SDHC_CAPAB_BASECLKFREQ    52ul
-#define SDHC_CAPAB_TOUNIT         1ul  /* Timeout clock unit 0 - kHz, 1 - MHz */
-/* Timeout clock frequency 1-63, 0 - not defined */
-#define SDHC_CAPAB_TOCLKFREQ      52ul
-
-/* Now check all parameters and calculate CAPABILITIES REGISTER value */
-#if SDHC_CAPAB_64BITBUS > 1 || SDHC_CAPAB_18V > 1 || SDHC_CAPAB_30V > 1 ||     \
-    SDHC_CAPAB_33V > 1 || SDHC_CAPAB_SUSPRESUME > 1 || SDHC_CAPAB_SDMA > 1 ||  \
-    SDHC_CAPAB_HIGHSPEED > 1 || SDHC_CAPAB_ADMA2 > 1 || SDHC_CAPAB_ADMA1 > 1 ||\
-    SDHC_CAPAB_TOUNIT > 1
-#error Capabilities features can have value 0 or 1 only!
-#endif
-
-#if SDHC_CAPAB_MAXBLOCKLENGTH == 512
-#define MAX_BLOCK_LENGTH 0ul
-#elif SDHC_CAPAB_MAXBLOCKLENGTH == 1024
-#define MAX_BLOCK_LENGTH 1ul
-#elif SDHC_CAPAB_MAXBLOCKLENGTH == 2048
-#define MAX_BLOCK_LENGTH 2ul
-#else
-#error Max host controller block size can have value 512, 1024 or 2048 only!
-#endif
-
-#if (SDHC_CAPAB_BASECLKFREQ > 0 && SDHC_CAPAB_BASECLKFREQ < 10) || \
-    SDHC_CAPAB_BASECLKFREQ > 63
-#error SDclock frequency can have value in range 0, 10-63 only!
-#endif
-
-#if SDHC_CAPAB_TOCLKFREQ > 63
-#error Timeout clock frequency can have value in range 0-63 only!
-#endif
-
-#define SDHC_CAPAB_REG_DEFAULT                                 \
-   ((SDHC_CAPAB_64BITBUS << 28) | (SDHC_CAPAB_18V << 26) |     \
-    (SDHC_CAPAB_30V << 25) | (SDHC_CAPAB_33V << 24) |          \
-    (SDHC_CAPAB_SUSPRESUME << 23) | (SDHC_CAPAB_SDMA << 22) |  \
-    (SDHC_CAPAB_HIGHSPEED << 21) | (SDHC_CAPAB_ADMA1 << 20) |  \
-    (SDHC_CAPAB_ADMA2 << 19) | (MAX_BLOCK_LENGTH << 16) |      \
-    (SDHC_CAPAB_BASECLKFREQ << 8) | (SDHC_CAPAB_TOUNIT << 7) | \
-    (SDHC_CAPAB_TOCLKFREQ))
-
-#define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
+#define SDHC_CAPAB_REG_DEFAULT 0x057834b4
 
 static uint8_t sdhci_slotint(SDHCIState *s)
 {
-- 
2.16.1

[Qemu-devel] [PATCH v13 09/30] sdhci: simplify sdhci_get_fifolen()

[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 +++-
 hw/sd/sdhci.c          | 20 +++++---------------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 64556480a9..def1c7f7aa 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -24,6 +24,8 @@
 #ifndef SDHCI_INTERNAL_H
 #define SDHCI_INTERNAL_H
 
+#include "hw/registerfields.h"
+
 /* R/W SDMA System Address register 0x0 */
 #define SDHC_SYSAD                     0x00
 
@@ -185,7 +187,7 @@
 #define SDHC_CAN_DO_ADMA2              0x00080000
 #define SDHC_CAN_DO_ADMA1              0x00100000
 #define SDHC_64_BIT_BUS_SUPPORT        (1 << 28)
-#define SDHC_CAPAB_BLOCKSIZE(x)        (((x) >> 16) & 0x3)
+FIELD(SDHC_CAPAB, MAXBLOCKLENGTH,     16, 2);
 
 /* HWInit Maximum Current Capabilities Register 0x0 */
 #define SDHC_MAXCURR                   0x48
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 491e624262..f22ed9181c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -58,6 +58,11 @@
  */
 #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
 
+static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
+{
+    return 1 << (9 + FIELD_EX32(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH));
+}
+
 static uint8_t sdhci_slotint(SDHCIState *s)
 {
     return (s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
@@ -1118,21 +1123,6 @@ static const MemoryRegionOps sdhci_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
-{
-    switch (SDHC_CAPAB_BLOCKSIZE(s->capareg)) {
-    case 0:
-        return 512;
-    case 1:
-        return 1024;
-    case 2:
-        return 2048;
-    default:
-        hw_error("SDHC: unsupported value for maximum block size\n");
-        return 0;
-    }
-}
-
 static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
 {
     if (s->sd_spec_version != 2) {
-- 
2.16.1

[Qemu-devel] [PATCH v13 10/30] sdhci: check the Spec v1 capabilities correctness

[Philippe Mathieu-Daudé]

Incorrect value will throw an error.

Note than Spec v2 is supported by default.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci-internal.h | 22 ++++++++++-
 hw/sd/sdhci.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/sd/trace-events     |  1 +
 3 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index def1c7f7aa..96d7f4dde7 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -86,7 +86,9 @@
 
 /* R/W Host control Register 0x0 */
 #define SDHC_HOSTCTL                   0x28
-#define SDHC_CTRL_LED                  0x01
+FIELD(SDHC_HOSTCTL, LED_CTRL,          0, 1);
+FIELD(SDHC_HOSTCTL, DATATRANSFERWIDTH, 1, 1); /* SD mode only */
+FIELD(SDHC_HOSTCTL, HIGH_SPEED,        2, 1);
 #define SDHC_CTRL_DMA_CHECK_MASK       0x18
 #define SDHC_CTRL_SDMA                 0x00
 #define SDHC_CTRL_ADMA1_32             0x08
@@ -102,6 +104,7 @@
 /* R/W Power Control Register 0x0 */
 #define SDHC_PWRCON                    0x29
 #define SDHC_POWER_ON                  (1 << 0)
+FIELD(SDHC_PWRCON, BUS_VOLTAGE,        1, 3);
 
 /* R/W Block Gap Control Register 0x0 */
 #define SDHC_BLKGAP                    0x2A
@@ -124,6 +127,7 @@
 
 /* R/W Timeout Control Register 0x0 */
 #define SDHC_TIMEOUTCON                0x2E
+FIELD(SDHC_TIMEOUTCON, COUNTER,        0, 4);
 
 /* R/W Software Reset Register 0x0 */
 #define SDHC_SWRST                     0x2F
@@ -180,17 +184,31 @@
 
 /* ROC Auto CMD12 error status register 0x0 */
 #define SDHC_ACMD12ERRSTS              0x3C
+FIELD(SDHC_ACMD12ERRSTS, TIMEOUT_ERR,  1, 1);
+FIELD(SDHC_ACMD12ERRSTS, CRC_ERR,      2, 1);
+FIELD(SDHC_ACMD12ERRSTS, INDEX_ERR,    4, 1);
 
 /* HWInit Capabilities Register 0x05E80080 */
 #define SDHC_CAPAB                     0x40
-#define SDHC_CAN_DO_DMA                0x00400000
 #define SDHC_CAN_DO_ADMA2              0x00080000
 #define SDHC_CAN_DO_ADMA1              0x00100000
 #define SDHC_64_BIT_BUS_SUPPORT        (1 << 28)
+FIELD(SDHC_CAPAB, TOCLKFREQ,           0, 6);
+FIELD(SDHC_CAPAB, TOUNIT,              7, 1);
+FIELD(SDHC_CAPAB, BASECLKFREQ,         8, 8);
 FIELD(SDHC_CAPAB, MAXBLOCKLENGTH,     16, 2);
+FIELD(SDHC_CAPAB, HIGHSPEED,          21, 1);
+FIELD(SDHC_CAPAB, SDMA,               22, 1);
+FIELD(SDHC_CAPAB, SUSPRESUME,         23, 1);
+FIELD(SDHC_CAPAB, V33,                24, 1);
+FIELD(SDHC_CAPAB, V30,                25, 1);
+FIELD(SDHC_CAPAB, V18,                26, 1);
 
 /* HWInit Maximum Current Capabilities Register 0x0 */
 #define SDHC_MAXCURR                   0x48
+FIELD(SDHC_MAXCURR, V33_VDD1,          0, 8);
+FIELD(SDHC_MAXCURR, V30_VDD1,          8, 8);
+FIELD(SDHC_MAXCURR, V18_VDD1,         16, 8);
 
 /* W Force Event Auto CMD12 Error Interrupt Register 0x0000 */
 #define SDHC_FEAER                     0x50
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f22ed9181c..bd3581502a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "hw/hw.h"
 #include "sysemu/block-backend.h"
@@ -63,6 +64,92 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
     return 1 << (9 + FIELD_EX32(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH));
 }
 
+/* return true on error */
+static bool sdhci_check_capab_freq_range(SDHCIState *s, const char *desc,
+                                         uint8_t freq, Error **errp)
+{
+    switch (freq) {
+    case 0:
+    case 10 ... 63:
+        break;
+    default:
+        error_setg(errp, "SD %s clock frequency can have value"
+                   "in range 0-63 only", desc);
+        return true;
+    }
+    return false;
+}
+
+static void sdhci_check_capareg(SDHCIState *s, Error **errp)
+{
+    uint64_t msk = s->capareg;
+    uint32_t val;
+    bool unit_mhz;
+
+    switch (s->sd_spec_version) {
+    case 2: /* default version */
+
+    /* fallback */
+    case 1:
+        unit_mhz = FIELD_EX64(s->capareg, SDHC_CAPAB, TOUNIT);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, TOUNIT, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, TOCLKFREQ);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, TOCLKFREQ, 0);
+        trace_sdhci_capareg(unit_mhz ? "timeout (MHz)" : "timeout (KHz)", val);
+        if (sdhci_check_capab_freq_range(s, "timeout", val, errp)) {
+            return;
+        }
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, BASECLKFREQ);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, BASECLKFREQ, 0);
+        trace_sdhci_capareg(unit_mhz ? "base (MHz)" : "Base (KHz)", val);
+        if (sdhci_check_capab_freq_range(s, "base", val, errp)) {
+            return;
+        }
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH);
+        if (val >= 0b11) {
+            error_setg(errp, "block size can be 512, 1024 or 2048 only");
+            return;
+        }
+        msk = FIELD_DP64(msk, SDHC_CAPAB, MAXBLOCKLENGTH, 0);
+        trace_sdhci_capareg("max block length", sdhci_get_fifolen(s));
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, HIGHSPEED);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, HIGHSPEED, 0);
+        trace_sdhci_capareg("high speed", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, SDMA);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, SDMA, 0);
+        trace_sdhci_capareg("SDMA", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, SUSPRESUME);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, SUSPRESUME, 0);
+        trace_sdhci_capareg("suspend/resume", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V33);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V33, 0);
+        trace_sdhci_capareg("3.3v", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V30);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V30, 0);
+        trace_sdhci_capareg("3.0v", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V18);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V18, 0);
+        trace_sdhci_capareg("1.8v", val);
+        break;
+
+    default:
+        error_setg(errp, "Unsupported spec version: %u", s->sd_spec_version);
+    }
+    if (msk) {
+        qemu_log_mask(LOG_UNIMP,
+                      "SDHCI: unknown CAPAB mask: 0x%016" PRIx64 "\n", msk);
+    }
+}
+
 static uint8_t sdhci_slotint(SDHCIState *s)
 {
     return (s->norintsts & s->norintsigen) || (s->errintsts & s->errintsigen) ||
@@ -990,7 +1077,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
     case SDHC_TRNMOD:
         /* DMA can be enabled only if it is supported as indicated by
          * capabilities register */
-        if (!(s->capareg & SDHC_CAN_DO_DMA)) {
+        if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) {
             value &= ~SDHC_TRNS_DMA;
         }
         MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK);
@@ -1125,11 +1212,19 @@ static const MemoryRegionOps sdhci_mmio_ops = {
 
 static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (s->sd_spec_version != 2) {
         error_setg(errp, "Only Spec v2 is supported");
         return;
     }
     s->version = (SDHC_HCVER_VENDOR << 8) | (s->sd_spec_version - 1);
+
+    sdhci_check_capareg(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 /* --- qdev common --- */
@@ -1537,7 +1632,7 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         /*
          * First, save bits 7 6 and 0 since they are identical
          */
-        hostctl = value & (SDHC_CTRL_LED |
+        hostctl = value & (R_SDHC_HOSTCTL_LED_CTRL_MASK |
                            SDHC_CTRL_CDTEST_INS |
                            SDHC_CTRL_CDTEST_EN);
         /*
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 0a121156a3..78d8707669 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -13,6 +13,7 @@ sdhci_adma_transfer_completed(void) ""
 sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s%u: addr[0x%04" PRIx64 "] %s 0x%08" PRIx64 " (%" PRIu64 ")"
 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_capareg(const char *desc, uint16_t val) "%s: %u"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.16.1

[Qemu-devel] [PATCH v13 12/30] sdhci: Fix 64-bit ADMA2

[Philippe Mathieu-Daudé]

From: Sai Pavan Boddu <saipava@xilinx.com>

The 64-bit ADMA address is not converted to the cpu endianes correctly.
This patch fixes the issue and uses a valid mask for the attribute data.

Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
[AF: Re-write commit message]
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 650265a472..4bd35078a9 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -667,8 +667,8 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dscr->length = le16_to_cpu(dscr->length);
         dma_memory_read(s->dma_as, entry_addr + 4,
                         (uint8_t *)(&dscr->addr), 8);
-        dscr->attr = le64_to_cpu(dscr->attr);
-        dscr->attr &= 0xfffffff8;
+        dscr->addr = le64_to_cpu(dscr->addr);
+        dscr->attr &= (uint8_t) ~0xC0;
         dscr->incr = 12;
         break;
     }
-- 
2.16.1

[Qemu-devel] [PATCH v13 14/30] hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()

[Philippe Mathieu-Daudé]

We only set a 32-bit value, but this is a good practice in case this
code is used as reference.

(missed in 5efc9016e52)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/exynos4210.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index e8e1d81e62..d89322c7ea 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -378,7 +378,7 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
         DriveInfo *di;
 
         dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
-        qdev_prop_set_uint32(dev, "capareg", EXYNOS4210_SDHCI_CAPABILITIES);
+        qdev_prop_set_uint64(dev, "capareg", EXYNOS4210_SDHCI_CAPABILITIES);
         qdev_init_nofail(dev);
 
         busdev = SYS_BUS_DEVICE(dev);
-- 
2.16.1

[Qemu-devel] [PATCH v13 15/30] hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/exynos4210.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index d89322c7ea..06f9d1ffa4 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -377,6 +377,18 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
         BlockBackend *blk;
         DriveInfo *di;
 
+        /* Compatible with:
+         * - SD Host Controller Specification Version 2.0
+         * - SDIO Specification Version 2.0
+         * - MMC Specification Version 4.3
+         * - SDMA
+         * - ADMA2
+         *
+         * As this part of the Exynos4210 is not publically available,
+         * we used the "HS-MMC Controller S3C2416X RISC Microprocessor"
+         * public datasheet which is very similar (implementing
+         * MMC Specification Version 4.0 being the only difference noted)
+         */
         dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
         qdev_prop_set_uint64(dev, "capareg", EXYNOS4210_SDHCI_CAPABILITIES);
         qdev_init_nofail(dev);
-- 
2.16.1

[Qemu-devel] [PATCH v13 16/30] hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet

[Philippe Mathieu-Daudé]

checking Xilinx datasheet "UG585" (v1.12.1)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/xilinx_zynq.c | 53 ++++++++++++++++++++++++++++------------------------
 tests/sdhci-test.c   |  5 +++++
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..0f76333770 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -61,6 +61,8 @@ static const int dma_irqs[8] = {
 #define SLCR_XILINX_UNLOCK_KEY  0xdf0d
 #define SLCR_XILINX_LOCK_KEY    0x767b
 
+#define ZYNQ_SDHCI_CAPABILITIES 0x69ec0080  /* Datasheet: UG585 (v1.12.1) */
+
 #define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
                         extract32((x), 12,  4) << 16)
 
@@ -165,10 +167,8 @@ static void zynq_init(MachineState *machine)
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-    DeviceState *dev, *carddev;
+    DeviceState *dev;
     SysBusDevice *busdev;
-    DriveInfo *di;
-    BlockBackend *blk;
     qemu_irq pic[64];
     int n;
 
@@ -247,27 +247,32 @@ static void zynq_init(MachineState *machine)
     gem_init(&nd_table[0], 0xE000B000, pic[54-IRQ_OFFSET]);
     gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
 
-    dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
-    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]);
-
-    di = drive_get_next(IF_SD);
-    blk = di ? blk_by_legacy_dinfo(di) : NULL;
-    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
-    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
-    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
-
-    dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
-    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]);
-
-    di = drive_get_next(IF_SD);
-    blk = di ? blk_by_legacy_dinfo(di) : NULL;
-    carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
-    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
-    object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
+    for (n = 0; n < 2; n++) {
+        int hci_irq = n ? 79 : 56;
+        hwaddr hci_addr = n ? 0xE0101000 : 0xE0100000;
+        DriveInfo *di;
+        BlockBackend *blk;
+        DeviceState *carddev;
+
+        /* Compatible with:
+         * - SD Host Controller Specification Version 2.0 Part A2
+         * - SDIO Specification Version 2.0
+         * - MMC Specification Version 3.31
+         */
+        dev = qdev_create(NULL, TYPE_SYSBUS_SDHCI);
+        qdev_prop_set_uint8(dev, "sd-spec-version", 2);
+        qdev_prop_set_uint64(dev, "capareg", ZYNQ_SDHCI_CAPABILITIES);
+        qdev_init_nofail(dev);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - IRQ_OFFSET]);
+
+        di = drive_get_next(IF_SD);
+        blk = di ? blk_by_legacy_dinfo(di) : NULL;
+        carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
+        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+        object_property_set_bool(OBJECT(carddev), true, "realized",
+                                 &error_fatal);
+    }
 
     dev = qdev_create(NULL, TYPE_ZYNQ_XADC);
     qdev_init_nofail(dev);
diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 24feea744a..898c43ff4f 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -41,6 +41,11 @@ static const struct sdhci_t {
     /* Exynos4210 */
     { "arm",    "smdkc210",
         {0x12510000, 2, 0,  {1, 0x5e80080} } },
+
+    /* Zynq-7000 */
+    { "arm",    "xilinx-zynq-a9",   /* Datasheet: UG585 (v1.12.1) */
+        {0xe0100000, 2, 0,  {1, 0x69ec0080} } },
+
 };
 
 typedef struct QSDHCI {
-- 
2.16.1

[Qemu-devel] [PATCH v13 18/30] sdhci: rename the hostctl1 register

[Philippe Mathieu-Daudé]

As per the Spec v3.00

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         | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 2a26b46f05..54594845ce 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -59,7 +59,7 @@ typedef struct SDHCIState {
     uint16_t cmdreg;       /* Command Register */
     uint32_t rspreg[4];    /* Response Registers 0-3 */
     uint32_t prnsts;       /* Present State Register */
-    uint8_t  hostctl;      /* Host Control Register */
+    uint8_t  hostctl1;     /* Host Control Register */
     uint8_t  pwrcon;       /* Power control Register */
     uint8_t  blkgap;       /* Block Gap Control Register */
     uint8_t  wakcon;       /* WakeUp Control Register */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e7214d6f60..9a8cdd551c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -691,7 +691,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
     uint32_t adma1 = 0;
     uint64_t adma2 = 0;
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
-    switch (SDHC_DMA_TYPE(s->hostctl)) {
+    switch (SDHC_DMA_TYPE(s->hostctl1)) {
     case SDHC_CTRL_ADMA2_32:
         dma_memory_read(s->dma_as, entry_addr, (uint8_t *)&adma2,
                         sizeof(adma2));
@@ -880,7 +880,7 @@ static void sdhci_data_transfer(void *opaque)
     SDHCIState *s = (SDHCIState *)opaque;
 
     if (s->trnmod & SDHC_TRNS_DMA) {
-        switch (SDHC_DMA_TYPE(s->hostctl)) {
+        switch (SDHC_DMA_TYPE(s->hostctl1)) {
         case SDHC_CTRL_SDMA:
             if ((s->blkcnt == 1) || !(s->trnmod & SDHC_TRNS_MULTI)) {
                 sdhci_sdma_transfer_single_block(s);
@@ -989,7 +989,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = s->prnsts;
         break;
     case SDHC_HOSTCTL:
-        ret = s->hostctl | (s->pwrcon << 8) | (s->blkgap << 16) |
+        ret = s->hostctl1 | (s->pwrcon << 8) | (s->blkgap << 16) |
               (s->wakcon << 24);
         break;
     case SDHC_CLKCON:
@@ -1107,7 +1107,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         MASKED_WRITE(s->sdmasysad, mask, value);
         /* Writing to last byte of sdmasysad might trigger transfer */
         if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt &&
-                s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
+                s->blksize && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
             if (s->trnmod & SDHC_TRNS_MULTI) {
                 sdhci_sdma_transfer_multi_blocks(s);
             } else {
@@ -1159,7 +1159,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         if (!(mask & 0xFF0000)) {
             sdhci_blkgap_write(s, value >> 16);
         }
-        MASKED_WRITE(s->hostctl, mask, value);
+        MASKED_WRITE(s->hostctl1, mask, value);
         MASKED_WRITE(s->pwrcon, mask >> 8, value >> 8);
         MASKED_WRITE(s->wakcon, mask >> 24, value >> 24);
         if (!(s->prnsts & SDHC_CARD_PRESENT) || ((s->pwrcon >> 1) & 0x7) < 5 ||
@@ -1380,7 +1380,7 @@ const VMStateDescription sdhci_vmstate = {
         VMSTATE_UINT16(cmdreg, SDHCIState),
         VMSTATE_UINT32_ARRAY(rspreg, SDHCIState, 4),
         VMSTATE_UINT32(prnsts, SDHCIState),
-        VMSTATE_UINT8(hostctl, SDHCIState),
+        VMSTATE_UINT8(hostctl1, SDHCIState),
         VMSTATE_UINT8(pwrcon, SDHCIState),
         VMSTATE_UINT8(blkgap, SDHCIState),
         VMSTATE_UINT8(wakcon, SDHCIState),
@@ -1598,13 +1598,13 @@ static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
          * manipulation code see comments in a similar part of
          * usdhc_write()
          */
-        hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
+        hostctl = SDHC_DMA_TYPE(s->hostctl1) << (8 - 3);
 
-        if (s->hostctl & SDHC_CTRL_8BITBUS) {
+        if (s->hostctl1 & SDHC_CTRL_8BITBUS) {
             hostctl |= ESDHC_CTRL_8BITBUS;
         }
 
-        if (s->hostctl & SDHC_CTRL_4BITBUS) {
+        if (s->hostctl1 & SDHC_CTRL_4BITBUS) {
             hostctl |= ESDHC_CTRL_4BITBUS;
         }
 
-- 
2.16.1

[Qemu-devel] [PATCH v13 19/30] sdhci: implement the Host Control 2 register (tuning sequence)

[Philippe Mathieu-Daudé]

[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>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci-internal.h | 10 ++++++++++
 include/hw/sd/sdhci.h  |  1 +
 hw/sd/sdhci.c          | 22 +++++++++++++++++++---
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index bfb39d614b..5c69270988 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -189,6 +189,16 @@ FIELD(SDHC_ACMD12ERRSTS, TIMEOUT_ERR,  1, 1);
 FIELD(SDHC_ACMD12ERRSTS, CRC_ERR,      2, 1);
 FIELD(SDHC_ACMD12ERRSTS, INDEX_ERR,    4, 1);
 
+/* Host Control Register 2 (since v3) */
+#define SDHC_HOSTCTL2                  0x3E
+FIELD(SDHC_HOSTCTL2, UHS_MODE_SEL,     0, 3);
+FIELD(SDHC_HOSTCTL2, V18_ENA,          3, 1); /* UHS-I only */
+FIELD(SDHC_HOSTCTL2, DRIVER_STRENGTH,  4, 2); /* UHS-I only */
+FIELD(SDHC_HOSTCTL2, EXECUTE_TUNING,   6, 1); /* UHS-I only */
+FIELD(SDHC_HOSTCTL2, SAMPLING_CLKSEL,  7, 1); /* UHS-I only */
+FIELD(SDHC_HOSTCTL2, ASYNC_INT,       14, 1);
+FIELD(SDHC_HOSTCTL2, PRESET_ENA,      15, 1);
+
 /* HWInit Capabilities Register 0x05E80080 */
 #define SDHC_CAPAB                     0x40
 FIELD(SDHC_CAPAB, TOCLKFREQ,           0, 6);
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 54594845ce..fd606e9928 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -73,6 +73,7 @@ typedef struct SDHCIState {
     uint16_t norintsigen;  /* Normal Interrupt Signal Enable Register */
     uint16_t errintsigen;  /* Error Interrupt Signal Enable Register */
     uint16_t acmd12errsts; /* Auto CMD12 error status register */
+    uint16_t hostctl2;     /* Host Control 2 */
     uint64_t admasysaddr;  /* ADMA System Address Register */
 
     /* Read-only registers */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 9a8cdd551c..1dbcb99f52 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -408,14 +408,29 @@ static void sdhci_end_transfer(SDHCIState *s)
 static void sdhci_read_block_from_card(SDHCIState *s)
 {
     int index = 0;
+    uint8_t data;
+    const uint16_t blk_size = s->blksize & BLOCK_SIZE_MASK;
 
     if ((s->trnmod & SDHC_TRNS_MULTI) &&
             (s->trnmod & SDHC_TRNS_BLK_CNT_EN) && (s->blkcnt == 0)) {
         return;
     }
 
-    for (index = 0; index < (s->blksize & BLOCK_SIZE_MASK); index++) {
-        s->fifo_buffer[index] = sdbus_read_data(&s->sdbus);
+    for (index = 0; index < blk_size; index++) {
+        data = sdbus_read_data(&s->sdbus);
+        if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
+            /* Device is not in tunning */
+            s->fifo_buffer[index] = data;
+        }
+    }
+
+    if (FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
+        /* Device is in tunning */
+        s->hostctl2 &= ~R_SDHC_HOSTCTL2_EXECUTE_TUNING_MASK;
+        s->hostctl2 |= R_SDHC_HOSTCTL2_SAMPLING_CLKSEL_MASK;
+        s->prnsts &= ~(SDHC_DAT_LINE_ACTIVE | SDHC_DOING_READ |
+                       SDHC_DATA_INHIBIT);
+        goto read_done;
     }
 
     /* New data now available for READ through Buffer Port Register */
@@ -440,6 +455,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
         }
     }
 
+read_done:
     sdhci_update_irq(s);
 }
 
@@ -1005,7 +1021,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = s->norintsigen | (s->errintsigen << 16);
         break;
     case SDHC_ACMD12ERRSTS:
-        ret = s->acmd12errsts;
+        ret = s->acmd12errsts | (s->hostctl2 << 16);
         break;
     case SDHC_CAPAB:
         ret = (uint32_t)s->capareg;
-- 
2.16.1

[Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/core.c       | 14 ++++++++++++--
 hw/sd/trace-events |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/sd/core.c b/hw/sd/core.c
index 295dc44ab7..498284f109 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -23,6 +23,12 @@
 #include "hw/qdev-core.h"
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
+#include "trace.h"
+
+static inline const char *sdbus_name(SDBus *sdbus)
+{
+    return sdbus->qbus.name;
+}
 
 static SDState *get_card(SDBus *sdbus)
 {
@@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
 {
     SDState *card = get_card(sdbus);
 
+    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
@@ -52,6 +59,7 @@ void sdbus_write_data(SDBus *sdbus, uint8_t value)
 {
     SDState *card = get_card(sdbus);
 
+    trace_sdbus_write(sdbus_name(sdbus), value);
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
@@ -62,14 +70,16 @@ void sdbus_write_data(SDBus *sdbus, uint8_t value)
 uint8_t sdbus_read_data(SDBus *sdbus)
 {
     SDState *card = get_card(sdbus);
+    uint8_t value = 0;
 
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
-        return sc->read_data(card);
+        value = sc->read_data(card);
     }
+    trace_sdbus_read(sdbus_name(sdbus), value);
 
-    return 0;
+    return value;
 }
 
 bool sdbus_data_ready(SDBus *sdbus)
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 78d8707669..ea2746c8b7 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -1,5 +1,10 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/sd/core.c
+sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s CMD%02d arg 0x%08x crc 0x%02x"
+sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x"
+sdbus_write(const char *bus_name, uint8_t value) "@%s value 0x%02x"
+
 # hw/sd/sdhci.c
 sdhci_set_inserted(const char *level) "card state changed: %s"
 sdhci_send_command(uint8_t cmd, uint32_t arg) "CMD%02u ARG[0x%08x]"
-- 
2.16.1

[Qemu-devel] [PATCH v13 21/30] sdhci: implement UHS-I voltage switch

[Philippe Mathieu-Daudé]

[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>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 include/hw/sd/sd.h    | 16 ++++++++++++++++
 include/hw/sd/sdhci.h |  1 +
 hw/sd/core.c          | 13 +++++++++++++
 hw/sd/sd.c            | 13 +++++++++++++
 hw/sd/sdhci.c         | 12 +++++++++++-
 hw/sd/trace-events    |  1 +
 6 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 96caefe373..f086679493 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -55,6 +55,20 @@
 #define AKE_SEQ_ERROR		(1 << 3)
 #define OCR_CCS_BITN        30
 
+typedef enum {
+    SD_VOLTAGE_0_4V     = 400,  /* currently not supported */
+    SD_VOLTAGE_1_8V     = 1800,
+    SD_VOLTAGE_3_0V     = 3000,
+    SD_VOLTAGE_3_3V     = 3300,
+} sd_voltage_mv_t;
+
+typedef enum  {
+    UHS_NOT_SUPPORTED   = 0,
+    UHS_I               = 1,
+    UHS_II              = 2,    /* currently not supported */
+    UHS_III             = 3,    /* currently not supported */
+} sd_uhs_mode_t;
+
 typedef enum {
     sd_none = -1,
     sd_bc = 0,	/* broadcast -- no response */
@@ -88,6 +102,7 @@ typedef struct {
     void (*write_data)(SDState *sd, uint8_t value);
     uint8_t (*read_data)(SDState *sd);
     bool (*data_ready)(SDState *sd);
+    void (*set_voltage)(SDState *sd, uint16_t millivolts);
     void (*enable)(SDState *sd, bool enable);
     bool (*get_inserted)(SDState *sd);
     bool (*get_readonly)(SDState *sd);
@@ -134,6 +149,7 @@ void sd_enable(SDState *sd, bool enable);
 /* Functions to be used by qdevified callers (working via
  * an SDBus rather than directly with SDState)
  */
+void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts);
 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);
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index fd606e9928..f321767c56 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -96,6 +96,7 @@ typedef struct SDHCIState {
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
     uint32_t quirks;
     uint8_t sd_spec_version;
+    uint8_t uhs_mode;
 } SDHCIState;
 
 /*
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 498284f109..6d198ea775 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -41,6 +41,19 @@ static SDState *get_card(SDBus *sdbus)
     return SD_CARD(kid->child);
 }
 
+void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts)
+{
+    SDState *card = get_card(sdbus);
+
+    trace_sdbus_set_voltage(sdbus_name(sdbus), millivolts);
+    if (card) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+        assert(sc->set_voltage);
+        sc->set_voltage(card, millivolts);
+    }
+}
+
 int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
 {
     SDState *card = get_card(sdbus);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 73e405a04f..a8d7a522c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -128,6 +128,18 @@ struct SDState {
     bool enable;
 };
 
+static void sd_set_voltage(SDState *sd, uint16_t millivolts)
+{
+    switch (millivolts) {
+    case 3001 ... 3600: /* SD_VOLTAGE_3_3V */
+    case 2001 ... 3000: /* SD_VOLTAGE_3_0V */
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "SD card voltage not supported: %.3fV",
+                      millivolts / 1000.f);
+    }
+}
+
 static void sd_set_mode(SDState *sd)
 {
     switch (sd->state) {
@@ -1926,6 +1938,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
     dc->reset = sd_reset;
     dc->bus_type = TYPE_SD_BUS;
 
+    sc->set_voltage = sd_set_voltage;
     sc->do_command = sd_do_command;
     sc->write_data = sd_write_data;
     sc->read_data = sd_read_data;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1dbcb99f52..4e717117e1 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1255,7 +1255,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         sdhci_update_irq(s);
         break;
     case SDHC_ACMD12ERRSTS:
-        MASKED_WRITE(s->acmd12errsts, mask, value);
+        MASKED_WRITE(s->acmd12errsts, mask, value & UINT16_MAX);
+        if (s->uhs_mode >= UHS_I) {
+            MASKED_WRITE(s->hostctl2, mask >> 16, value >> 16);
+
+            if (FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, V18_ENA)) {
+                sdbus_set_voltage(&s->sdbus, SD_VOLTAGE_1_8V);
+            } else {
+                sdbus_set_voltage(&s->sdbus, SD_VOLTAGE_3_3V);
+            }
+        }
         break;
 
     case SDHC_CAPAB:
@@ -1310,6 +1319,7 @@ static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
 
 #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
+    DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
     \
     /* Capabilities registers provide information on supported
      * features of this specific host controller implementation */ \
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index ea2746c8b7..84d2f398b1 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -4,6 +4,7 @@
 sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s CMD%02d arg 0x%08x crc 0x%02x"
 sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x"
 sdbus_write(const char *bus_name, uint8_t value) "@%s value 0x%02x"
+sdbus_set_voltage(const char *bus_name, uint16_t millivolts) "@%s %u (mV)"
 
 # hw/sd/sdhci.c
 sdhci_set_inserted(const char *level) "card state changed: %s"
-- 
2.16.1

[Qemu-devel] [PATCH v13 22/30] sdhci: implement CMD/DAT[] fields in the Present State register

[Philippe Mathieu-Daudé]

[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>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci-internal.h |  2 ++
 include/hw/sd/sd.h     |  4 ++++
 hw/sd/core.c           | 34 ++++++++++++++++++++++++++++++++++
 hw/sd/sd.c             | 16 ++++++++++++++++
 hw/sd/sdhci.c          |  4 ++++
 hw/sd/trace-events     |  2 ++
 6 files changed, 62 insertions(+)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 5c69270988..0092627076 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -82,6 +82,8 @@
 #define SDHC_CARD_PRESENT              0x00010000
 #define SDHC_CARD_DETECT               0x00040000
 #define SDHC_WRITE_PROTECT             0x00080000
+FIELD(SDHC_PRNSTS, DAT_LVL,            20, 4);
+FIELD(SDHC_PRNSTS, CMD_LVL,            24, 1);
 #define TRANSFERRING_DATA(x)           \
     ((x) & (SDHC_DOING_READ | SDHC_DOING_WRITE))
 
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index f086679493..bf1eb0713c 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -103,6 +103,8 @@ typedef struct {
     uint8_t (*read_data)(SDState *sd);
     bool (*data_ready)(SDState *sd);
     void (*set_voltage)(SDState *sd, uint16_t millivolts);
+    uint8_t (*get_dat_lines)(SDState *sd);
+    bool (*get_cmd_line)(SDState *sd);
     void (*enable)(SDState *sd, bool enable);
     bool (*get_inserted)(SDState *sd);
     bool (*get_readonly)(SDState *sd);
@@ -150,6 +152,8 @@ void sd_enable(SDState *sd, bool enable);
  * an SDBus rather than directly with SDState)
  */
 void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts);
+uint8_t sdbus_get_dat_lines(SDBus *sdbus);
+bool sdbus_get_cmd_line(SDBus *sdbus);
 int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
 void sdbus_write_data(SDBus *sd, uint8_t value);
 uint8_t sdbus_read_data(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 6d198ea775..3c6eae6c88 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -41,6 +41,40 @@ static SDState *get_card(SDBus *sdbus)
     return SD_CARD(kid->child);
 }
 
+uint8_t sdbus_get_dat_lines(SDBus *sdbus)
+{
+    SDState *slave = get_card(sdbus);
+    uint8_t dat_lines = 0b1111; /* 4 bit bus width */
+
+    if (slave) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(slave);
+
+        if (sc->get_dat_lines) {
+            dat_lines = sc->get_dat_lines(slave);
+        }
+    }
+    trace_sdbus_get_dat_lines(sdbus_name(sdbus), dat_lines);
+
+    return dat_lines;
+}
+
+bool sdbus_get_cmd_line(SDBus *sdbus)
+{
+    SDState *slave = get_card(sdbus);
+    bool cmd_line = true;
+
+    if (slave) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(slave);
+
+        if (sc->get_cmd_line) {
+            cmd_line = sc->get_cmd_line(slave);
+        }
+    }
+    trace_sdbus_get_cmd_line(sdbus_name(sdbus), cmd_line);
+
+    return cmd_line;
+}
+
 void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts)
 {
     SDState *card = get_card(sdbus);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a8d7a522c0..9ac9b63ff8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -126,8 +126,20 @@ struct SDState {
     BlockBackend *blk;
 
     bool enable;
+    uint8_t dat_lines;
+    bool cmd_line;
 };
 
+static uint8_t sd_get_dat_lines(SDState *sd)
+{
+    return sd->enable ? sd->dat_lines : 0;
+}
+
+static bool sd_get_cmd_line(SDState *sd)
+{
+    return sd->enable ? sd->cmd_line : false;
+}
+
 static void sd_set_voltage(SDState *sd, uint16_t millivolts)
 {
     switch (millivolts) {
@@ -457,6 +469,8 @@ static void sd_reset(DeviceState *dev)
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
     sd->expecting_acmd = false;
+    sd->dat_lines = 0xf;
+    sd->cmd_line = true;
     sd->multi_blk_cnt = 0;
 }
 
@@ -1939,6 +1953,8 @@ static void sd_class_init(ObjectClass *klass, void *data)
     dc->bus_type = TYPE_SD_BUS;
 
     sc->set_voltage = sd_set_voltage;
+    sc->get_dat_lines = sd_get_dat_lines;
+    sc->get_cmd_line = sd_get_cmd_line;
     sc->do_command = sd_do_command;
     sc->write_data = sd_write_data;
     sc->read_data = sd_read_data;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 4e717117e1..0cd968fc6b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1003,6 +1003,10 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         break;
     case SDHC_PRNSTS:
         ret = s->prnsts;
+        ret = FIELD_DP32(ret, SDHC_PRNSTS, DAT_LVL,
+                         sdbus_get_dat_lines(&s->sdbus));
+        ret = FIELD_DP32(ret, SDHC_PRNSTS, CMD_LVL,
+                         sdbus_get_cmd_line(&s->sdbus));
         break;
     case SDHC_HOSTCTL:
         ret = s->hostctl1 | (s->pwrcon << 8) | (s->blkgap << 16) |
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 84d2f398b1..0f8536db32 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -5,6 +5,8 @@ sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s
 sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x"
 sdbus_write(const char *bus_name, uint8_t value) "@%s value 0x%02x"
 sdbus_set_voltage(const char *bus_name, uint16_t millivolts) "@%s %u (mV)"
+sdbus_get_dat_lines(const char *bus_name, uint8_t dat_lines) "@%s dat_lines: %u"
+sdbus_get_cmd_line(const char *bus_name, bool cmd_line) "@%s cmd_line: %u"
 
 # hw/sd/sdhci.c
 sdhci_set_inserted(const char *level) "card state changed: %s"
-- 
2.16.1

[Qemu-devel] [PATCH v13 23/30] hw/arm/bcm2835_peripherals: implement SDHCI Spec v3

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/bcm2835_peripherals.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 12e0dd11af..ca971e83e0 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -254,14 +254,19 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->peri_mr, RNG_OFFSET,
                 sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0));
 
-    /* Extended Mass Media Controller */
-    object_property_set_int(OBJECT(&s->sdhci), BCM2835_SDHC_CAPAREG, "capareg",
-                            &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-
+    /* Extended Mass Media Controller
+     *
+     * Compatible with:
+     * - SD Host Controller Specification Version 3.0 Draft 1.0
+     * - SDIO Specification Version 3.0
+     * - MMC Specification Version 4.4
+     *
+     * For the exact details please refer to the Arasan documentation:
+     *   SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf
+     */
+    object_property_set_uint(OBJECT(&s->sdhci), 3, "sd-spec-version", &err);
+    object_property_set_uint(OBJECT(&s->sdhci), BCM2835_SDHC_CAPAREG, "capareg",
+                             &err);
     object_property_set_bool(OBJECT(&s->sdhci), true, "pending-insert-quirk",
                              &err);
     if (err) {
-- 
2.16.1

[Qemu-devel] [PATCH v13 24/30] hw/arm/bcm2835_peripherals: change maximum block size to 1kB

[Philippe Mathieu-Daudé]

following the datasheet.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/bcm2835_peripherals.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index ca971e83e0..13b63970d7 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -19,7 +19,7 @@
 #define BCM2835_VC_PERI_BASE 0x7e000000
 
 /* Capabilities for SD controller: no DMA, high-speed, default clocks etc. */
-#define BCM2835_SDHC_CAPAREG 0x52034b4
+#define BCM2835_SDHC_CAPAREG 0x52134b4
 
 static void bcm2835_peripherals_init(Object *obj)
 {
-- 
2.16.1

[Qemu-devel] [PATCH v13 25/30] hw/arm/fsl-imx6: implement SDHCI Spec. v3

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/fsl-imx6.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index e6559a8b12..b6ac72de27 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -27,6 +27,8 @@
 #include "chardev/char.h"
 #include "qemu/error-report.h"
 
+#define IMX6_ESDHC_CAPABILITIES     0x057834b4
+
 #define NAME_SIZE 20
 
 static void fsl_imx6_init(Object *obj)
@@ -348,6 +350,11 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
             { FSL_IMX6_uSDHC4_ADDR, FSL_IMX6_uSDHC4_IRQ },
         };
 
+        /* UHS-I SDIO3.0 SDR104 1.8V ADMA */
+        object_property_set_uint(OBJECT(&s->esdhc[i]), 3, "sd-spec-version",
+                                 &err);
+        object_property_set_uint(OBJECT(&s->esdhc[i]), IMX6_ESDHC_CAPABILITIES,
+                                 "capareg", &err);
         object_property_set_bool(OBJECT(&s->esdhc[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
-- 
2.16.1

[Qemu-devel] [PATCH v13 26/30] hw/arm/xilinx_zynqmp: fix the capabilities/spec version to match the datasheet

[Philippe Mathieu-Daudé]

checking Xilinx datasheet "UG1085" (v1.7)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index ca398c4159..e39ad73bec 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -53,6 +53,8 @@
 #define IPI_ADDR            0xFF300000
 #define IPI_IRQ             64
 
+#define SDHCI_CAPABILITIES  0x280737ec6481 /* Datasheet: UG1085 (v1.7) */
+
 static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
     0xFF0B0000, 0xFF0C0000, 0xFF0D0000, 0xFF0E0000,
 };
@@ -387,22 +389,27 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
-        char *bus_name;
-
-        object_property_set_bool(OBJECT(&s->sdhci[i]), true,
-                                 "realized", &err);
+        char *bus_name = g_strdup_printf("sd-bus%d", i);
+        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->sdhci[i]);
+        Object *sdhci = OBJECT(&s->sdhci[i]);
+
+        /* Compatible with:
+         * - SD Host Controller Specification Version 3.00
+         * - SDIO Specification Version 3.0
+         * - eMMC Specification Version 4.51
+         */
+        object_property_set_uint(sdhci, 3, "sd-spec-version", &err);
+        object_property_set_uint(sdhci, SDHCI_CAPABILITIES, "capareg", &err);
+        object_property_set_bool(sdhci, true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
             return;
         }
-        sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci[i]), 0,
-                        sdhci_addr[i]);
-        sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci[i]), 0,
-                           gic_spi[sdhci_intr[i]]);
+        sysbus_mmio_map(sbd, 0, sdhci_addr[i]);
+        sysbus_connect_irq(sbd, 0, gic_spi[sdhci_intr[i]]);
+
         /* Alias controller SD bus to the SoC itself */
-        bus_name = g_strdup_printf("sd-bus%d", i);
-        object_property_add_alias(OBJECT(s), bus_name,
-                                  OBJECT(&s->sdhci[i]), "sd-bus",
+        object_property_add_alias(OBJECT(s), bus_name, sdhci, "sd-bus",
                                   &error_abort);
         g_free(bus_name);
     }
-- 
2.16.1

[Qemu-devel] [PATCH v13 27/30] hw/arm/xilinx_zynqmp: enable the UHS-I mode

[Philippe Mathieu-Daudé]

see the Xilinx datasheet "UG1085" (v1.7)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index e39ad73bec..4b93a3abd2 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -400,6 +400,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
          */
         object_property_set_uint(sdhci, 3, "sd-spec-version", &err);
         object_property_set_uint(sdhci, SDHCI_CAPABILITIES, "capareg", &err);
+        object_property_set_uint(sdhci, UHS_I, "uhs", &err);
         object_property_set_bool(sdhci, true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
-- 
2.16.1

[Qemu-devel] [PATCH v13 28/30] sdhci: check Spec v3 capabilities qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 tests/sdhci-test.c     | 12 ++++++++++++
 tests/Makefile.include |  1 +
 2 files changed, 13 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 898c43ff4f..39d0f87788 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -42,10 +42,22 @@ static const struct sdhci_t {
     { "arm",    "smdkc210",
         {0x12510000, 2, 0,  {1, 0x5e80080} } },
 
+    /* i.MX 6 */
+    { "arm",    "sabrelite",
+        {0x02190000, 3, 0,  {1, 0x057834b4} } },
+
+    /* BCM2835 */
+    { "arm",    "raspi2",
+        {0x3f300000, 3, 52, {0, 0x052134b4} } },
+
     /* Zynq-7000 */
     { "arm",    "xilinx-zynq-a9",   /* Datasheet: UG585 (v1.12.1) */
         {0xe0100000, 2, 0,  {1, 0x69ec0080} } },
 
+    /* ZynqMP */
+    { "aarch64", "xlnx-zcu102",     /* Datasheet: UG1085 (v1.7) */
+        {0xff160000, 3, 0,  {1, 0x280737ec6481} } },
+
 };
 
 typedef struct QSDHCI {
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 52be9b3fa5..278c13aa93 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -371,6 +371,7 @@ check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
+check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 
-- 
2.16.1

[Qemu-devel] [PATCH v13 29/30] sdhci: add a check_capab_v3() qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/sdhci-test.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 39d0f87788..c0b45da88a 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -16,6 +16,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 {
@@ -161,6 +163,20 @@ static void check_capab_sdma(QSDHCI *s, bool supported)
     g_assert_cmpuint(capab_sdma, ==, supported);
 }
 
+static void check_capab_v3(QSDHCI *s, uint8_t version)
+{
+    uint64_t capab, capab_v3;
+
+    if (version < 3) {
+        /* before v3 those fields are RESERVED */
+        capab = sdhci_readq(s, 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 QSDHCI *machine_start(const struct sdhci_t *test)
 {
     QSDHCI *s = g_new0(QSDHCI, 1);
@@ -209,6 +225,7 @@ static void test_machine(const void *data)
     check_specs_version(s, test->sdhci.version);
     check_capab_capareg(s, test->sdhci.capab.reg);
     check_capab_readonly(s);
+    check_capab_v3(s, test->sdhci.version);
     check_capab_sdma(s, test->sdhci.capab.sdma);
     check_capab_baseclock(s, test->sdhci.baseclock);
 
-- 
2.16.1

[Qemu-devel] [PATCH v13 30/30] sdhci: add Spec v4.2 register definitions

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 0092627076..e1bb733aed 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -198,6 +198,10 @@ FIELD(SDHC_HOSTCTL2, V18_ENA,          3, 1); /* UHS-I only */
 FIELD(SDHC_HOSTCTL2, DRIVER_STRENGTH,  4, 2); /* UHS-I only */
 FIELD(SDHC_HOSTCTL2, EXECUTE_TUNING,   6, 1); /* UHS-I only */
 FIELD(SDHC_HOSTCTL2, SAMPLING_CLKSEL,  7, 1); /* UHS-I only */
+FIELD(SDHC_HOSTCTL2, UHS_II_ENA,       8, 1); /* since v4 */
+FIELD(SDHC_HOSTCTL2, ADMA2_LENGTH,    10, 1); /* since v4 */
+FIELD(SDHC_HOSTCTL2, CMD23_ENA,       11, 1); /* since v4 */
+FIELD(SDHC_HOSTCTL2, VERSION4,        12, 1); /* since v4 */
 FIELD(SDHC_HOSTCTL2, ASYNC_INT,       14, 1);
 FIELD(SDHC_HOSTCTL2, PRESET_ENA,      15, 1);
 
@@ -216,10 +220,12 @@ FIELD(SDHC_CAPAB, SUSPRESUME,         23, 1);
 FIELD(SDHC_CAPAB, V33,                24, 1);
 FIELD(SDHC_CAPAB, V30,                25, 1);
 FIELD(SDHC_CAPAB, V18,                26, 1);
+FIELD(SDHC_CAPAB, BUS64BIT_V4,        27, 1); /* since v4.10 */
 FIELD(SDHC_CAPAB, BUS64BIT,           28, 1); /* since v2 */
 FIELD(SDHC_CAPAB, ASYNC_INT,          29, 1); /* since v3 */
 FIELD(SDHC_CAPAB, SLOT_TYPE,          30, 2); /* since v3 */
 FIELD(SDHC_CAPAB, BUS_SPEED,          32, 3); /* since v3 */
+FIELD(SDHC_CAPAB, UHS_II,             35, 8); /* since v4.20 */
 FIELD(SDHC_CAPAB, DRIVER_STRENGTH,    36, 3); /* since v3 */
 FIELD(SDHC_CAPAB, DRIVER_TYPE_A,      36, 1); /* since v3 */
 FIELD(SDHC_CAPAB, DRIVER_TYPE_C,      37, 1); /* since v3 */
@@ -228,12 +234,15 @@ FIELD(SDHC_CAPAB, TIMER_RETUNING,     40, 4); /* since v3 */
 FIELD(SDHC_CAPAB, SDR50_TUNING,       45, 1); /* since v3 */
 FIELD(SDHC_CAPAB, RETUNING_MODE,      46, 2); /* since v3 */
 FIELD(SDHC_CAPAB, CLOCK_MULT,         48, 8); /* since v3 */
+FIELD(SDHC_CAPAB, ADMA3,              59, 1); /* since v4.20 */
+FIELD(SDHC_CAPAB, V18_VDD2,           60, 1); /* since v4.20 */
 
 /* HWInit Maximum Current Capabilities Register 0x0 */
 #define SDHC_MAXCURR                   0x48
 FIELD(SDHC_MAXCURR, V33_VDD1,          0, 8);
 FIELD(SDHC_MAXCURR, V30_VDD1,          8, 8);
 FIELD(SDHC_MAXCURR, V18_VDD1,         16, 8);
+FIELD(SDHC_MAXCURR, V18_VDD2,         32, 8); /* since v4.20 */
 
 /* W Force Event Auto CMD12 Error Interrupt Register 0x0000 */
 #define SDHC_FEAER                     0x50
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0cd968fc6b..74b1802503 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -91,6 +91,20 @@ static void sdhci_check_capareg(SDHCIState *s, Error **errp)
     bool unit_mhz;
 
     switch (s->sd_spec_version) {
+    case 4:
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS64BIT_V4);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, BUS64BIT_V4, 0);
+        trace_sdhci_capareg("64-bit system bus (v4)", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, UHS_II);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, UHS_II, 0);
+        trace_sdhci_capareg("UHS-II", val);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA3);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA3, 0);
+        trace_sdhci_capareg("ADMA3", val);
+
+    /* fallback */
     case 3:
         val = FIELD_EX64(s->capareg, SDHC_CAPAB, ASYNC_INT);
         trace_sdhci_capareg("async interrupt", val);
-- 
2.16.1

Re: [Qemu-devel] [PATCH v13 30/30] sdhci: add Spec v4.2 register definitions

[Alistair Francis]

On Mon, Feb 12, 2018 at 8:08 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-internal.h |  9 +++++++++
>  hw/sd/sdhci.c          | 14 ++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 0092627076..e1bb733aed 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -198,6 +198,10 @@ FIELD(SDHC_HOSTCTL2, V18_ENA,          3, 1); /* UHS-I only */
>  FIELD(SDHC_HOSTCTL2, DRIVER_STRENGTH,  4, 2); /* UHS-I only */
>  FIELD(SDHC_HOSTCTL2, EXECUTE_TUNING,   6, 1); /* UHS-I only */
>  FIELD(SDHC_HOSTCTL2, SAMPLING_CLKSEL,  7, 1); /* UHS-I only */
> +FIELD(SDHC_HOSTCTL2, UHS_II_ENA,       8, 1); /* since v4 */
> +FIELD(SDHC_HOSTCTL2, ADMA2_LENGTH,    10, 1); /* since v4 */
> +FIELD(SDHC_HOSTCTL2, CMD23_ENA,       11, 1); /* since v4 */
> +FIELD(SDHC_HOSTCTL2, VERSION4,        12, 1); /* since v4 */
>  FIELD(SDHC_HOSTCTL2, ASYNC_INT,       14, 1);
>  FIELD(SDHC_HOSTCTL2, PRESET_ENA,      15, 1);
>
> @@ -216,10 +220,12 @@ FIELD(SDHC_CAPAB, SUSPRESUME,         23, 1);
>  FIELD(SDHC_CAPAB, V33,                24, 1);
>  FIELD(SDHC_CAPAB, V30,                25, 1);
>  FIELD(SDHC_CAPAB, V18,                26, 1);
> +FIELD(SDHC_CAPAB, BUS64BIT_V4,        27, 1); /* since v4.10 */
>  FIELD(SDHC_CAPAB, BUS64BIT,           28, 1); /* since v2 */
>  FIELD(SDHC_CAPAB, ASYNC_INT,          29, 1); /* since v3 */
>  FIELD(SDHC_CAPAB, SLOT_TYPE,          30, 2); /* since v3 */
>  FIELD(SDHC_CAPAB, BUS_SPEED,          32, 3); /* since v3 */
> +FIELD(SDHC_CAPAB, UHS_II,             35, 8); /* since v4.20 */
>  FIELD(SDHC_CAPAB, DRIVER_STRENGTH,    36, 3); /* since v3 */
>  FIELD(SDHC_CAPAB, DRIVER_TYPE_A,      36, 1); /* since v3 */
>  FIELD(SDHC_CAPAB, DRIVER_TYPE_C,      37, 1); /* since v3 */
> @@ -228,12 +234,15 @@ FIELD(SDHC_CAPAB, TIMER_RETUNING,     40, 4); /* since v3 */
>  FIELD(SDHC_CAPAB, SDR50_TUNING,       45, 1); /* since v3 */
>  FIELD(SDHC_CAPAB, RETUNING_MODE,      46, 2); /* since v3 */
>  FIELD(SDHC_CAPAB, CLOCK_MULT,         48, 8); /* since v3 */
> +FIELD(SDHC_CAPAB, ADMA3,              59, 1); /* since v4.20 */
> +FIELD(SDHC_CAPAB, V18_VDD2,           60, 1); /* since v4.20 */
>
>  /* HWInit Maximum Current Capabilities Register 0x0 */
>  #define SDHC_MAXCURR                   0x48
>  FIELD(SDHC_MAXCURR, V33_VDD1,          0, 8);
>  FIELD(SDHC_MAXCURR, V30_VDD1,          8, 8);
>  FIELD(SDHC_MAXCURR, V18_VDD1,         16, 8);
> +FIELD(SDHC_MAXCURR, V18_VDD2,         32, 8); /* since v4.20 */
>
>  /* W Force Event Auto CMD12 Error Interrupt Register 0x0000 */
>  #define SDHC_FEAER                     0x50
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 0cd968fc6b..74b1802503 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -91,6 +91,20 @@ static void sdhci_check_capareg(SDHCIState *s, Error **errp)
>      bool unit_mhz;
>
>      switch (s->sd_spec_version) {
> +    case 4:
> +        val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS64BIT_V4);
> +        msk = FIELD_DP64(msk, SDHC_CAPAB, BUS64BIT_V4, 0);
> +        trace_sdhci_capareg("64-bit system bus (v4)", val);
> +
> +        val = FIELD_EX64(s->capareg, SDHC_CAPAB, UHS_II);
> +        msk = FIELD_DP64(msk, SDHC_CAPAB, UHS_II, 0);
> +        trace_sdhci_capareg("UHS-II", val);
> +
> +        val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA3);
> +        msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA3, 0);
> +        trace_sdhci_capareg("ADMA3", val);
> +
> +    /* fallback */
>      case 3:
>          val = FIELD_EX64(s->capareg, SDHC_CAPAB, ASYNC_INT);
>          trace_sdhci_capareg("async interrupt", val);
> --
> 2.16.1
>
>

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Peter Maydell]

On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

> @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
>  {
>      SDState *card = get_card(sdbus);
>
> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
>      if (card) {
>          SDCardClass *sc = SD_CARD_GET_CLASS(card);

Hi -- as a result of this trace event being added, we now get
warnings from Coverity that it might use uninitialized data
(CID1386074, CID1390571, CID1386072, CID1386076). This is because not
all of our SD
controllers bother to initialize req->crc, because up til now
nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
sdhci.c and ssi-sd.c do this).

Could you have a look at this, please? I think there are
three plausible lines of approach:

(1) just don't bother to trace the CRC field
(2) make bcm2835_sdhost.c, sdhci.c, ssi-sd.c set crc to 0 like
omap and pxa2xx already do
(3) "proper" implementation of CRC, so that an sd controller
can either (a) mark the SDRequest as "no CRC" and have
sd_req_crc_validate() always pass, or (b) mark the SDRequest
as having a CRC and have sd_req_crc_validate() actually
do the check which it currently stubs out with "return 0"

(3 is because it doesn't seem very sensible to spend time
calculating a CRC just to test it against a CRC calculated
a little bit later on; but we don't really want to drop the
CRC stuff entirely because on some controllers like
milkymist-memcard.c the CRC byte comes from the guest and
we'd ideally like to check it.)

I don't have a strong preference for which of 1/2/3 we do.

PS: CID1005332 is the coverity issue for "sd_req_crc_validate
stubs out its check code with 'return 0' leaving a line of
unreachable code".

thanks
-- PMM

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Edgar E. Iglesias]

On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> 
> > @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
> >  {
> >      SDState *card = get_card(sdbus);
> >
> > +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
> >      if (card) {
> >          SDCardClass *sc = SD_CARD_GET_CLASS(card);
> 
> Hi -- as a result of this trace event being added, we now get
> warnings from Coverity that it might use uninitialized data
> (CID1386074, CID1390571, CID1386072, CID1386076). This is because not
> all of our SD
> controllers bother to initialize req->crc, because up til now
> nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
> sdhci.c and ssi-sd.c do this).
> 
> Could you have a look at this, please? I think there are
> three plausible lines of approach:
> 
> (1) just don't bother to trace the CRC field
> (2) make bcm2835_sdhost.c, sdhci.c, ssi-sd.c set crc to 0 like
> omap and pxa2xx already do

Hi,

Philippe, any opinion here?

Otherwise I suggest we do #2 right away to avoid the warnings
until someone cares enough to implement #3...

Cheers,
Edgar



> (3) "proper" implementation of CRC, so that an sd controller
> can either (a) mark the SDRequest as "no CRC" and have
> sd_req_crc_validate() always pass, or (b) mark the SDRequest
> as having a CRC and have sd_req_crc_validate() actually
> do the check which it currently stubs out with "return 0"
> 
> (3 is because it doesn't seem very sensible to spend time
> calculating a CRC just to test it against a CRC calculated
> a little bit later on; but we don't really want to drop the
> CRC stuff entirely because on some controllers like
> milkymist-memcard.c the CRC byte comes from the guest and
> we'd ideally like to check it.)
> 
> I don't have a strong preference for which of 1/2/3 we do.
> 
> PS: CID1005332 is the coverity issue for "sd_req_crc_validate
> stubs out its check code with 'return 0' leaving a line of
> unreachable code".
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Philippe Mathieu-Daudé]

On 04/30/2018 10:49 AM, Edgar E. Iglesias wrote:
> On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
>> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>
>>> @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
>>>  {
>>>      SDState *card = get_card(sdbus);
>>>
>>> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
>>>      if (card) {
>>>          SDCardClass *sc = SD_CARD_GET_CLASS(card);
>>
>> Hi -- as a result of this trace event being added, we now get
>> warnings from Coverity that it might use uninitialized data
>> (CID1386074, CID1390571, CID1386072, CID1386076). This is because not
>> all of our SD
>> controllers bother to initialize req->crc, because up til now
>> nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
>> sdhci.c and ssi-sd.c do this).

I was pretty sure we ran Coverity before the stable release :|
Maybe they added new 'uninitialized data' checks recently.

>>
>> Could you have a look at this, please? I think there are
>> three plausible lines of approach:
>>
>> (1) just don't bother to trace the CRC field
>> (2) make bcm2835_sdhost.c, sdhci.c, ssi-sd.c set crc to 0 like
>> omap and pxa2xx already do
> 
> Hi,
> 
> Philippe, any opinion here?
> 
> Otherwise I suggest we do #2 right away to avoid the warnings
> until someone cares enough to implement #3...

I prefer to start with #3 directly, else we'll keep this forever.

> 
> Cheers,
> Edgar
> 
> 
> 
>> (3) "proper" implementation of CRC, so that an sd controller
>> can either (a) mark the SDRequest as "no CRC" and have
>> sd_req_crc_validate() always pass, or (b) mark the SDRequest
>> as having a CRC and have sd_req_crc_validate() actually
>> do the check which it currently stubs out with "return 0"
>>
>> (3 is because it doesn't seem very sensible to spend time
>> calculating a CRC just to test it against a CRC calculated
>> a little bit later on; but we don't really want to drop the
>> CRC stuff entirely because on some controllers like
>> milkymist-memcard.c the CRC byte comes from the guest and
>> we'd ideally like to check it.)
>>
>> I don't have a strong preference for which of 1/2/3 we do.
>>
>> PS: CID1005332 is the coverity issue for "sd_req_crc_validate
>> stubs out its check code with 'return 0' leaving a line of
>> unreachable code".
>>
>> thanks
>> -- PMM
> 

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Peter Maydell]

On 1 May 2018 at 04:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 04/30/2018 10:49 AM, Edgar E. Iglesias wrote:
>> On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
>>> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>>
>>>> @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
>>>>  {
>>>>      SDState *card = get_card(sdbus);
>>>>
>>>> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
>>>>      if (card) {
>>>>          SDCardClass *sc = SD_CARD_GET_CLASS(card);
>>>
>>> Hi -- as a result of this trace event being added, we now get
>>> warnings from Coverity that it might use uninitialized data
>>> (CID1386074, CID1390571, CID1386072, CID1386076). This is because not
>>> all of our SD
>>> controllers bother to initialize req->crc, because up til now
>>> nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
>>> sdhci.c and ssi-sd.c do this).
>
> I was pretty sure we ran Coverity before the stable release :|
> Maybe they added new 'uninitialized data' checks recently.

No, we didn't get a coverity run for a long time (a combination
of Scan being down and then requiring a new version of the local
build tools, which we didn't realize because the website didn't
give a useful error message for that case). So this most recent
run has brought up a pile of accumulated issues since about
mid-february.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Philippe Mathieu-Daudé]

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

Hi Peter, Edgar,

On 05/01/2018 06:03 AM, Peter Maydell wrote:
> On 1 May 2018 at 04:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 04/30/2018 10:49 AM, Edgar E. Iglesias wrote:
>>> On Fri, Apr 27, 2018 at 12:55:21PM +0100, Peter Maydell wrote:
>>>> On 13 February 2018 at 04:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>
>>>>> @@ -39,6 +45,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
>>>>>  {
>>>>>      SDState *card = get_card(sdbus);
>>>>>
>>>>> +    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
>>>>>      if (card) {
>>>>>          SDCardClass *sc = SD_CARD_GET_CLASS(card);
>>>>
>>>> Hi -- as a result of this trace event being added, we now get
>>>> warnings from Coverity that it might use uninitialized data
>>>> (CID1386074, CID1390571, CID1386072, CID1386076). This is because not
>>>> all of our SD
>>>> controllers bother to initialize req->crc, because up til now
>>>> nothing in the SD code looks at it. (I think at least bcm2835_sdhost.c
>>>> sdhci.c and ssi-sd.c do this).
>>
>> I was pretty sure we ran Coverity before the stable release :|
>> Maybe they added new 'uninitialized data' checks recently.
> 
> No, we didn't get a coverity run for a long time (a combination
> of Scan being down and then requiring a new version of the local
> build tools, which we didn't realize because the website didn't
> give a useful error message for that case). So this most recent
> run has brought up a pile of accumulated issues since about
> mid-february.

I sent a fix for those here:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg01130.html

I went to the Coverity dashboard to update the Triage information but
the 'apply' box appears grey so I can not update those CIDs.

Regards,

Phil.


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

Re: [Qemu-devel] [PATCH v13 20/30] sdbus: add trace events

[Peter Maydell]

On 4 May 2018 at 17:11, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I went to the Coverity dashboard to update the Triage information but
> the 'apply' box appears grey so I can not update those CIDs.

You're only registered with Scan as a "Defect Viewer (read-only)".
The Scan UI appears to be busted and doesn't provide any way for
me as an admin to upgrade your account rights :-/ (except possibly
by removing you from the user list entirely and having you
reapply...)

thanks
-- PMM