[Qemu-devel] [PATCH v10 00/16] SDHCI: clean v1/v2 Specs (part 2)

[Qemu-devel] [PATCH v10 00/16] SDHCI: clean v1/v2 Specs (part 2)

[Philippe Mathieu-Daudé]

Patches missing review: 2 (PCI qtests) and 11 (trivial)

Since v9:
- replace global by struct QSDHCI in qtests (suggested by Paolo)
  (this generates changes in patches 3-6 but since they are only code adapted
  and no logical changes I kept Stefan R-b)
- added Alistair R-b

Since v8:
- we keep the 'capareg' property, this simplify a lot the series
- the Zynq 7000 uses the datasheet CAPAREG
- reset R-b/A-b

Since v7:
- use error_propagate()

Since v6:
- rebased on upstream to use DEFINE_SDHCI_COMMON_PROPERTIES
- included qtests back
- add PCI qtests
- series was getting big, splitting again; next one is "implement Spec v3"
- following Alistair advice, new properties default to current
  SDHC_CAPAB_REG_DEFAULT (probably 'default' for historical reason, being
  the first capabilities set). This leads to weird properties imho, i.e.
  defaulting 'max-frequency' to 52 MHz, the Exynos SoC has to set this
  property to 0. 
- addressed Alistair reviews
- confirmed the Exynos4210 specs after chatting on IRC with Krzysztof
  Kozlowski who kindly verified in the datasheet.

Since v5:
- addressed Alistair reviews
- dropped "abstract generic-sdhci"
- dropped Linux Device Tree names
- split qtests in another series
- change the bcm2835 minimum blocksize to 1KB (Andrew Baumann)
- added Alistair R-b
- based on Alistair work:
  - add SD tunning sequence via Host Control 2 to use UHS-I cards
  - add CMD/DAT[] fields in the Present State (used in next series
    to switch card voltage)

based on Alistair work:
- add SD tunning sequence via Host Control 2 to use UHS-I cards
- add CMD/DAT[] fields in the Present State (used in next series
  to switch card voltage)

Since v4 ("SDHCI: add qtests and fix few issues"):
- spec_version default to v2 (current behaviour)
- addressed Alistair review (no v1, tell user about valid version)

Since v3:
- no change, but split back in 2 series, 1st part is "SDHCI: housekeeping v5",

Since v2:
- more detailed 'capabilities', all boards converted to use these properties
- since all qtests pass, removed the previous 'capareg' property
- added Stefan/Alistair R-b
- corrected 'access' LED behavior (Alistair's review)
- more uses of the registerfields API
- remove some dead code
- cosmetix:
  - added more comments
  - renamed a pair of registers
  - reordered few struct members

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

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

Regards,

Phil.

$ git backport-diff with v9
001/16:[----] [--] 'sdhci: use error_propagate(local_err) in realize()'
002/16:[0065] [FC] 'sdhci: add qtest to check the SD capabilities register'
003/16:[0020] [FC] 'sdhci: add check_capab_readonly() qtest'
004/16:[0010] [FC] 'sdhci: add a check_capab_baseclock() qtest'
005/16:[0006] [FC] 'sdhci: add a check_capab_sdma() qtest'
006/16:[0022] [FC] 'sdhci: add qtest to check the SD Spec version'
007/16:[----] [--] 'sdhci: add a 'spec_version property' (default to v2)'
008/16:[----] [--] 'sdhci: use a numeric value for the default CAPAB register'
009/16:[----] [--] 'sdhci: simplify sdhci_get_fifolen()'
010/16:[----] [--] 'sdhci: check the Spec v1 capabilities correctness'
011/16:[----] [--] 'sdhci: add BLOCK_SIZE_MASK for DMA'
012/16:[----] [--] 'sdhci: Fix 64-bit ADMA2'
013/16:[----] [--] 'sdhci: check Spec v2 capabilities (DMA and 64-bit bus)'
014/16:[----] [--] 'hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()'
015/16:[----] [--] 'hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)'
016/16:[----] [-C] 'hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet'

Philippe Mathieu-Daudé (15):
  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: add BLOCK_SIZE_MASK for DMA
  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

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

 include/hw/sd/sdhci.h  |   2 +
 hw/sd/sdhci-internal.h |  43 ++++++---
 hw/arm/exynos4210.c    |  14 ++-
 hw/arm/xilinx_zynq.c   |  53 +++++-----
 hw/sd/sdhci.c          | 257 ++++++++++++++++++++++++++++++++-----------------
 hw/sd/trace-events     |   1 +
 tests/sdhci-test.c     | 217 +++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   3 +
 8 files changed, 465 insertions(+), 125 deletions(-)
 create mode 100644 tests/sdhci-test.c

-- 
2.16.1

[Qemu-devel] [PATCH v10 01/16] 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 fac7fa5c72..817c2525e6 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1300,10 +1300,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;
     }
 
@@ -1381,9 +1383,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 v10 02/16] 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>
---
 tests/sdhci-test.c     | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   3 ++
 2 files changed, 142 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..1105e07093
--- /dev/null
+++ b/tests/sdhci-test.c
@@ -0,0 +1,139 @@
+/*
+ * 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;
+        QPCIBar mem_bar;
+    } pci;
+} QSDHCI;
+
+static uint64_t sdhci_readq(QSDHCI *s, uintptr_t base, uint32_t reg)
+{
+    uint64_t val;
+
+    if (s->pci.dev) {
+        qpci_memread(s->pci.dev, s->pci.mem_bar, reg, &val, sizeof(val));
+    } else {
+        val = qtest_readq(global_qtest, base + reg);
+    }
+
+    return val;
+}
+
+static void check_capab_capareg(QSDHCI *s, uintptr_t addr, uint64_t expec_capab)
+{
+    uint64_t capab;
+
+    capab = sdhci_readq(s, addr, SDHC_CAPAB);
+    g_assert_cmphex(capab, ==, expec_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 -d unimp -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->pci.mem_bar = qpci_iomap(s->pci.dev, 0, &barsize);
+        qpci_device_enable(s->pci.dev);
+    } else {
+        /* SysBus */
+        global_qtest = qtest_startf("-machine %s -d unimp", test->machine);
+    }
+
+    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.addr, 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 v10 03/16] 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 1105e07093..e961f2b997 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -62,6 +62,15 @@ static uint64_t sdhci_readq(QSDHCI *s, uintptr_t base, uint32_t reg)
     return val;
 }
 
+static void sdhci_writeq(QSDHCI *s, uintptr_t base, uint32_t reg, uint64_t val)
+{
+    if (s->pci.dev) {
+        qpci_memwrite(s->pci.dev, s->pci.mem_bar, reg, &val, sizeof(val));
+    } else {
+        qtest_writeq(global_qtest, base + reg, val);
+    }
+}
+
 static void check_capab_capareg(QSDHCI *s, uintptr_t addr, uint64_t expec_capab)
 {
     uint64_t capab;
@@ -70,6 +79,20 @@ static void check_capab_capareg(QSDHCI *s, uintptr_t addr, uint64_t expec_capab)
     g_assert_cmphex(capab, ==, expec_capab);
 }
 
+static void check_capab_readonly(QSDHCI *s, uintptr_t addr)
+{
+    const uint64_t vrand = 0x123456789abcdef;
+    uint64_t capab0, capab1;
+
+    capab0 = sdhci_readq(s, addr, SDHC_CAPAB);
+    g_assert_cmpuint(capab0, !=, vrand);
+
+    sdhci_writeq(s, addr, SDHC_CAPAB, vrand);
+    capab1 = sdhci_readq(s, addr, 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);
@@ -115,6 +138,7 @@ static void test_machine(const void *data)
     s = machine_start(test);
 
     check_capab_capareg(s, test->sdhci.addr, test->sdhci.capab.reg);
+    check_capab_readonly(s, test->sdhci.addr);
 
     machine_stop(s);
 }
-- 
2.16.1

[Qemu-devel] [PATCH v10 04/16] 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 e961f2b997..45caf66455 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 {
@@ -93,6 +94,18 @@ static void check_capab_readonly(QSDHCI *s, uintptr_t addr)
     g_assert_cmpuint(capab1, ==, capab0);
 }
 
+static void check_capab_baseclock(QSDHCI *s, uintptr_t addr, uint8_t expec_freq)
+{
+    uint64_t capab, capab_freq;
+
+    if (!expec_freq) {
+        return;
+    }
+    capab = sdhci_readq(s, addr, SDHC_CAPAB);
+    capab_freq = FIELD_EX64(capab, SDHC_CAPAB, BASECLKFREQ);
+    g_assert_cmpuint(capab_freq, ==, expec_freq);
+}
+
 static QSDHCI *machine_start(const struct sdhci_t *test)
 {
     QSDHCI *s = g_new0(QSDHCI, 1);
@@ -139,6 +152,7 @@ static void test_machine(const void *data)
 
     check_capab_capareg(s, test->sdhci.addr, test->sdhci.capab.reg);
     check_capab_readonly(s, test->sdhci.addr);
+    check_capab_baseclock(s, test->sdhci.addr, test->sdhci.baseclock);
 
     machine_stop(s);
 }
-- 
2.16.1

[Qemu-devel] [PATCH v10 05/16] 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 45caf66455..4b901b6cf1 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 {
@@ -106,6 +107,15 @@ static void check_capab_baseclock(QSDHCI *s, uintptr_t addr, uint8_t expec_freq)
     g_assert_cmpuint(capab_freq, ==, expec_freq);
 }
 
+static void check_capab_sdma(QSDHCI *s, uintptr_t addr, bool supported)
+{
+    uint64_t capab, capab_sdma;
+
+    capab = sdhci_readq(s, addr, 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);
@@ -152,6 +162,7 @@ static void test_machine(const void *data)
 
     check_capab_capareg(s, test->sdhci.addr, test->sdhci.capab.reg);
     check_capab_readonly(s, test->sdhci.addr);
+    check_capab_sdma(s, test->sdhci.addr, test->sdhci.capab.sdma);
     check_capab_baseclock(s, test->sdhci.addr, test->sdhci.baseclock);
 
     machine_stop(s);
-- 
2.16.1

[Qemu-devel] [PATCH v10 06/16] 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 4b901b6cf1..ee12c4be7b 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -51,6 +51,19 @@ typedef struct QSDHCI {
     } pci;
 } QSDHCI;
 
+static uint32_t sdhci_readl(QSDHCI *s, uintptr_t base, uint32_t reg)
+{
+    uint32_t val;
+
+    if (s->pci.dev) {
+        qpci_memread(s->pci.dev, s->pci.mem_bar, reg, &val, sizeof(val));
+    } else {
+        val = qtest_readl(global_qtest, base + reg);
+    }
+
+    return val;
+}
+
 static uint64_t sdhci_readq(QSDHCI *s, uintptr_t base, uint32_t reg)
 {
     uint64_t val;
@@ -73,6 +86,16 @@ static void sdhci_writeq(QSDHCI *s, uintptr_t base, uint32_t reg, uint64_t val)
     }
 }
 
+static void check_specs_version(QSDHCI *s, uintptr_t addr, uint8_t version)
+{
+    uint32_t v;
+
+    v = sdhci_readl(s, addr, SDHC_HCVER);
+    v &= 0xff;
+    v += 1;
+    g_assert_cmpuint(v, ==, version);
+}
+
 static void check_capab_capareg(QSDHCI *s, uintptr_t addr, uint64_t expec_capab)
 {
     uint64_t capab;
@@ -160,6 +183,7 @@ static void test_machine(const void *data)
 
     s = machine_start(test);
 
+    check_specs_version(s, test->sdhci.addr, test->sdhci.version);
     check_capab_capareg(s, test->sdhci.addr, test->sdhci.capab.reg);
     check_capab_readonly(s, test->sdhci.addr);
     check_capab_sdma(s, test->sdhci.addr, test->sdhci.capab.sdma);
-- 
2.16.1

[Qemu-devel] [PATCH v10 07/16] 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 fc807f08f3..b7751c815f 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -210,9 +210,9 @@
 /* Slot interrupt status */
 #define SDHC_SLOT_INT_STATUS            0xFC
 
-/* HWInit Host Controller Version Register 0x0401 */
+/* HWInit Host Controller Version Register */
 #define SDHC_HCVER                      0xFE
-#define SD_HOST_SPECv2_VERS             0x2401
+#define SDHC_HCVER_VENDOR               0x24
 
 #define SDHC_REGISTERS_MAP_SIZE         0x100
 #define SDHC_INSERTION_DELAY            (NANOSECONDS_PER_SECOND)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 1cf70f8c23..40798aed58 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -77,6 +77,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;
@@ -91,6 +92,7 @@ typedef struct SDHCIState {
 
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
+    uint8_t sd_spec_version;
 } SDHCIState;
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 817c2525e6..2bcc5ff58a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -174,7 +174,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)
 
@@ -1204,6 +1216,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 v10 08/16] 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 2bcc5ff58a..da30b58723 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -39,67 +39,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 v10 09/16] 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 b7751c815f..577ca9da54 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
 
@@ -179,7 +181,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 da30b58723..b1bcaa6153 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -59,6 +59,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 v10 10/16] 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 | 21 ++++++++++-
 hw/sd/sdhci.c          | 97 +++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/sd/trace-events     |  1 +
 3 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 577ca9da54..c5e26bf8f3 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -86,6 +86,9 @@
 
 /* R/W Host control Register 0x0 */
 #define SDHC_HOSTCTL                   0x28
+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
@@ -96,6 +99,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
@@ -118,6 +122,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
@@ -174,17 +179,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 b1bcaa6153..48f2f962f0 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"
@@ -64,6 +65,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 y;
+
+    switch (s->sd_spec_version) {
+    case 2: /* default version */
+
+    /* fallback */
+    case 1:
+        y = FIELD_EX64(s->capareg, SDHC_CAPAB, TOUNIT);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, TOUNIT, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, TOCLKFREQ);
+        trace_sdhci_capareg(y ? "timeout (MHz)" : "Timeout (KHz)", val);
+        if (sdhci_check_capab_freq_range(s, "timeout", val, errp)) {
+            return;
+        }
+        msk = FIELD_DP64(msk, SDHC_CAPAB, TOCLKFREQ, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, BASECLKFREQ);
+        trace_sdhci_capareg(y ? "base (MHz)" : "Base (KHz)", val);
+        if (sdhci_check_capab_freq_range(s, "base", val, errp)) {
+            return;
+        }
+        msk = FIELD_DP64(msk, SDHC_CAPAB, BASECLKFREQ, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH);
+        if (val >= 0b11) {
+            error_setg(errp, "block size can be 512, 1024 or 2048 only");
+            return;
+        }
+        trace_sdhci_capareg("max block length", sdhci_get_fifolen(s));
+        msk = FIELD_DP64(msk, SDHC_CAPAB, MAXBLOCKLENGTH, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, HIGHSPEED);
+        trace_sdhci_capareg("high speed", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, HIGHSPEED, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, SDMA);
+        trace_sdhci_capareg("SDMA", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, SDMA, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, SUSPRESUME);
+        trace_sdhci_capareg("suspend/resume", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, SUSPRESUME, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V33);
+        trace_sdhci_capareg("3.3v", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V33, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V30);
+        trace_sdhci_capareg("3.0v", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V30, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, V18);
+        trace_sdhci_capareg("1.8v", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, V18, 0);
+        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 --- */
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 v10 12/16] 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 8d7a1469f7..8962070f24 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 v10 13/16] sdhci: check Spec v2 capabilities (DMA and 64-bit bus)

[Philippe Mathieu-Daudé]

Incorrect value will throw an error.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci-internal.h | 14 +++++++-------
 hw/sd/sdhci.c          | 19 +++++++++++++++----
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index c5e26bf8f3..4ed9727ec3 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -89,12 +89,12 @@
 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
+FIELD(SDHC_HOSTCTL, DMA,               3, 2);
 #define SDHC_CTRL_SDMA                 0x00
-#define SDHC_CTRL_ADMA1_32             0x08
+#define SDHC_CTRL_ADMA1_32             0x08 /* NOT ALLOWED since v2 */
 #define SDHC_CTRL_ADMA2_32             0x10
-#define SDHC_CTRL_ADMA2_64             0x18
-#define SDHC_DMA_TYPE(x)               ((x) & SDHC_CTRL_DMA_CHECK_MASK)
+#define SDHC_CTRL_ADMA2_64             0x18 /* only v1 & v2 (v3 optional) */
+#define SDHC_DMA_TYPE(x)               ((x) & R_SDHC_HOSTCTL_DMA_MASK)
 
 /* R/W Power Control Register 0x0 */
 #define SDHC_PWRCON                    0x29
@@ -185,19 +185,19 @@ FIELD(SDHC_ACMD12ERRSTS, INDEX_ERR,    4, 1);
 
 /* HWInit Capabilities Register 0x05E80080 */
 #define SDHC_CAPAB                     0x40
-#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, ADMA2,              19, 1); /* since v2 */
+FIELD(SDHC_CAPAB, ADMA1,              20, 1); /* v1 only? */
 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);
+FIELD(SDHC_CAPAB, BUS64BIT,           28, 1); /* since v2 */
 
 /* HWInit Maximum Current Capabilities Register 0x0 */
 #define SDHC_MAXCURR                   0x48
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8962070f24..aa2cbcd981 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -90,6 +90,17 @@ static void sdhci_check_capareg(SDHCIState *s, Error **errp)
 
     switch (s->sd_spec_version) {
     case 2: /* default version */
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA2);
+        trace_sdhci_capareg("ADMA2", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA2, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, ADMA1);
+        trace_sdhci_capareg("ADMA1", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, ADMA1, 0);
+
+        val = FIELD_EX64(s->capareg, SDHC_CAPAB, BUS64BIT);
+        trace_sdhci_capareg("64-bit system bus", val);
+        msk = FIELD_DP64(msk, SDHC_CAPAB, BUS64BIT, 0);
 
     /* fallback */
     case 1:
@@ -832,7 +843,7 @@ static void sdhci_data_transfer(void *opaque)
 
             break;
         case SDHC_CTRL_ADMA1_32:
-            if (!(s->capareg & SDHC_CAN_DO_ADMA1)) {
+            if (!(s->capareg & R_SDHC_CAPAB_ADMA1_MASK)) {
                 trace_sdhci_error("ADMA1 not supported");
                 break;
             }
@@ -840,7 +851,7 @@ static void sdhci_data_transfer(void *opaque)
             sdhci_do_adma(s);
             break;
         case SDHC_CTRL_ADMA2_32:
-            if (!(s->capareg & SDHC_CAN_DO_ADMA2)) {
+            if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK)) {
                 trace_sdhci_error("ADMA2 not supported");
                 break;
             }
@@ -848,8 +859,8 @@ static void sdhci_data_transfer(void *opaque)
             sdhci_do_adma(s);
             break;
         case SDHC_CTRL_ADMA2_64:
-            if (!(s->capareg & SDHC_CAN_DO_ADMA2) ||
-                    !(s->capareg & SDHC_64_BIT_BUS_SUPPORT)) {
+            if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK) ||
+                    !(s->capareg & R_SDHC_CAPAB_BUS64BIT_MASK)) {
                 trace_sdhci_error("64 bit ADMA not supported");
                 break;
             }
-- 
2.16.1

[Qemu-devel] [PATCH v10 14/16] 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 v10 15/16] 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 v10 16/16] 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 ee12c4be7b..3f3370e224 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

Re: [Qemu-devel] [PATCH v10 00/16] SDHCI: clean v1/v2 Specs (part 2)

[Alistair Francis]

On Wed, Feb 7, 2018 at 10:19 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Patches missing review: 2 (PCI qtests) and 11 (trivial)

I don't see patch 11 on the list.

I can see it in my Xilinx email as it was sent directly to that. I
can't cleanly reply to it from my Xilinx Outlook account though.

For patch 11 the commit message should be improved so that it can be
standalone from the title. Although you could just remove the message
as well as it doesn't add anything.

Once you have done that you can add my reviewed by to patch 11.

Alistair

>
> Since v9:
> - replace global by struct QSDHCI in qtests (suggested by Paolo)
>   (this generates changes in patches 3-6 but since they are only code adapted
>   and no logical changes I kept Stefan R-b)
> - added Alistair R-b
>
> Since v8:
> - we keep the 'capareg' property, this simplify a lot the series
> - the Zynq 7000 uses the datasheet CAPAREG
> - reset R-b/A-b
>
> Since v7:
> - use error_propagate()
>
> Since v6:
> - rebased on upstream to use DEFINE_SDHCI_COMMON_PROPERTIES
> - included qtests back
> - add PCI qtests
> - series was getting big, splitting again; next one is "implement Spec v3"
> - following Alistair advice, new properties default to current
>   SDHC_CAPAB_REG_DEFAULT (probably 'default' for historical reason, being
>   the first capabilities set). This leads to weird properties imho, i.e.
>   defaulting 'max-frequency' to 52 MHz, the Exynos SoC has to set this
>   property to 0.
> - addressed Alistair reviews
> - confirmed the Exynos4210 specs after chatting on IRC with Krzysztof
>   Kozlowski who kindly verified in the datasheet.
>
> Since v5:
> - addressed Alistair reviews
> - dropped "abstract generic-sdhci"
> - dropped Linux Device Tree names
> - split qtests in another series
> - change the bcm2835 minimum blocksize to 1KB (Andrew Baumann)
> - added Alistair R-b
> - based on Alistair work:
>   - add SD tunning sequence via Host Control 2 to use UHS-I cards
>   - add CMD/DAT[] fields in the Present State (used in next series
>     to switch card voltage)
>
> based on Alistair work:
> - add SD tunning sequence via Host Control 2 to use UHS-I cards
> - add CMD/DAT[] fields in the Present State (used in next series
>   to switch card voltage)
>
> Since v4 ("SDHCI: add qtests and fix few issues"):
> - spec_version default to v2 (current behaviour)
> - addressed Alistair review (no v1, tell user about valid version)
>
> Since v3:
> - no change, but split back in 2 series, 1st part is "SDHCI: housekeeping v5",
>
> Since v2:
> - more detailed 'capabilities', all boards converted to use these properties
> - since all qtests pass, removed the previous 'capareg' property
> - added Stefan/Alistair R-b
> - corrected 'access' LED behavior (Alistair's review)
> - more uses of the registerfields API
> - remove some dead code
> - cosmetix:
>   - added more comments
>   - renamed a pair of registers
>   - reordered few struct members
>
> Since v1:
> - addressed Alistair Francis review comments, added some R-b
> - only move register defines to "sd-internal.h"
> - fixed deposit64() arguments
> - dropped unuseful s->fifo_buffer = NULL
> - use a qemu_irq for the LED, restrict the logging to ON/OFF
> - fixed a trace format string error
> - included Andrey Smirnov ACMD12ERRSTS write patch
> - dropped few unuseful patches, and separate the Python polemical ones for later
>
> From the "SDHCI housekeeping" series:
> - 1: we restrict part of "sd/sd.h" into local "sd-internal.h",
> - 2,3: we somehow beautiful the code, no logical changes,
> - 4-7: we refactor the common sysbus/pci qdev code,
> - 8-10: we add plenty of trace events which will result useful later,
> - 11: we finally expose a "dma-memory" property.
> From the "SDHCI: add a qtest and fix few issues" series:
> - 12,13: fix registers
> - 14,15: boards can specify which SDHCI Spec to use (v2 and v3 so far)
> - 15-20: HCI qtest
>
> Regards,
>
> Phil.
>
> $ git backport-diff with v9
> 001/16:[----] [--] 'sdhci: use error_propagate(local_err) in realize()'
> 002/16:[0065] [FC] 'sdhci: add qtest to check the SD capabilities register'
> 003/16:[0020] [FC] 'sdhci: add check_capab_readonly() qtest'
> 004/16:[0010] [FC] 'sdhci: add a check_capab_baseclock() qtest'
> 005/16:[0006] [FC] 'sdhci: add a check_capab_sdma() qtest'
> 006/16:[0022] [FC] 'sdhci: add qtest to check the SD Spec version'
> 007/16:[----] [--] 'sdhci: add a 'spec_version property' (default to v2)'
> 008/16:[----] [--] 'sdhci: use a numeric value for the default CAPAB register'
> 009/16:[----] [--] 'sdhci: simplify sdhci_get_fifolen()'
> 010/16:[----] [--] 'sdhci: check the Spec v1 capabilities correctness'
> 011/16:[----] [--] 'sdhci: add BLOCK_SIZE_MASK for DMA'
> 012/16:[----] [--] 'sdhci: Fix 64-bit ADMA2'
> 013/16:[----] [--] 'sdhci: check Spec v2 capabilities (DMA and 64-bit bus)'
> 014/16:[----] [--] 'hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()'
> 015/16:[----] [--] 'hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)'
> 016/16:[----] [-C] 'hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet'
>
> Philippe Mathieu-Daudé (15):
>   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: add BLOCK_SIZE_MASK for DMA
>   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
>
> Sai Pavan Boddu (1):
>   sdhci: Fix 64-bit ADMA2
>
>  include/hw/sd/sdhci.h  |   2 +
>  hw/sd/sdhci-internal.h |  43 ++++++---
>  hw/arm/exynos4210.c    |  14 ++-
>  hw/arm/xilinx_zynq.c   |  53 +++++-----
>  hw/sd/sdhci.c          | 257 ++++++++++++++++++++++++++++++++-----------------
>  hw/sd/trace-events     |   1 +
>  tests/sdhci-test.c     | 217 +++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |   3 +
>  8 files changed, 465 insertions(+), 125 deletions(-)
>  create mode 100644 tests/sdhci-test.c
>
> --
> 2.16.1
>
>