[Qemu-devel] [PATCH v6 00/13] SDHCI: housekeeping

[Qemu-devel] [PATCH v6 00/13] SDHCI: housekeeping

[Philippe Mathieu-Daudé]

All patches reviewed :)

Since v5:
- addressed Alistair reviews
- added Alistair R-b
- renamed the dma property "dma-memory" -> "dma"

Since v4:
- fixed incorrect use of &local_err in sdhci_sysbus/pci_realize()

Since v3:
- since the series was getting too big and first part reviewed, split in 2. 
- addressed Fam's review from "refactor the common sysbus/pci qdev"
  - improved commit descriptions
  - restored useful s->fifo_buffer = NULL
- added Alistair R-b

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
001/13:[----] [--] 'sdhci: clean up includes'
002/13:[----] [--] 'sdhci: remove dead code'
003/13:[0004] [FC] 'sdhci: refactor same sysbus/pci properties into a common one'
004/13:[----] [--] 'sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()'
005/13:[----] [--] 'sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()'
006/13:[----] [--] 'sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()'
007/13:[----] [--] 'sdhci: use qemu_log_mask(UNIMP) instead of fprintf()'
008/13:[0020] [FC] 'sdhci: convert the DPRINT() calls into trace events'
009/13:[----] [--] 'sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"'
010/13:[----] [-C] 'sdhci: rename the SDHC_CAPAB register'
011/13:[0002] [FC] 'sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only'
012/13:[----] [--] 'sdhci: Implement write method of ACMD12ERRSTS register'
013/13:[0004] [FC] 'sdhci: add a "dma" property'

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

Philippe Mathieu-Daudé (12):
  sdhci: clean up includes
  sdhci: remove dead code
  sdhci: refactor same sysbus/pci properties into a common one
  sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()
  sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()
  sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()
  sdhci: use qemu_log_mask(UNIMP) instead of fprintf()
  sdhci: convert the DPRINT() calls into trace events
  sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"
  sdhci: rename the SDHC_CAPAB register
  sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only
  sdhci: add a "dma" property

 include/hw/sd/sdhci.h  |  19 ++--
 hw/sd/sdhci-internal.h |   7 +-
 hw/sd/sdhci.c          | 260 ++++++++++++++++++++++++++++---------------------
 hw/sd/trace-events     |  14 +++
 4 files changed, 176 insertions(+), 124 deletions(-)

-- 
2.15.1

[Qemu-devel] [PATCH v6 01/13] sdhci: clean up includes

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 161177cf39..248fd027f9 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -24,8 +24,6 @@
 #ifndef SDHCI_INTERNAL_H
 #define SDHCI_INTERNAL_H
 
-#include "hw/sd/sdhci.h"
-
 /* R/W SDMA System Address register 0x0 */
 #define SDHC_SYSAD                     0x00
 
@@ -227,6 +225,4 @@ enum {
     sdhc_gap_write  = 2   /* SDHC stopped at block gap during write operation */
 };
 
-extern const VMStateDescription sdhci_vmstate;
-
 #endif
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 0f0c3f1e64..1335373d3c 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -26,17 +26,19 @@
 #define SDHCI_H
 
 #include "qemu-common.h"
-#include "hw/block/block.h"
 #include "hw/pci/pci.h"
 #include "hw/sysbus.h"
 #include "hw/sd/sd.h"
 
 /* SD/MMC host controller state */
 typedef struct SDHCIState {
+    /*< private >*/
     union {
         PCIDevice pcidev;
         SysBusDevice busdev;
     };
+
+    /*< public >*/
     SDBus sdbus;
     MemoryRegion iomem;
 
@@ -46,6 +48,7 @@ typedef struct SDHCIState {
     qemu_irq ro_cb;
     qemu_irq irq;
 
+    /* Registers cleared on reset */
     uint32_t sdmasysad;    /* SDMA System Address register */
     uint16_t blksize;      /* Host DMA Buff Boundary and Transfer BlkSize Reg */
     uint16_t blkcnt;       /* Blocks count for current transfer */
@@ -70,8 +73,10 @@ typedef struct SDHCIState {
     uint16_t acmd12errsts; /* Auto CMD12 error status register */
     uint64_t admasysaddr;  /* ADMA System Address Register */
 
+    /* Read-only registers */
     uint32_t capareg;      /* Capabilities Register */
     uint32_t maxcurr;      /* Maximum Current Capabilities Register */
+
     uint8_t  *fifo_buffer; /* SD host i/o FIFO buffer */
     uint32_t buf_maxsz;
     uint16_t data_count;   /* current element in FIFO buffer */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b064a087c9..b7d2a20985 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -29,6 +29,7 @@
 #include "sysemu/dma.h"
 #include "qemu/timer.h"
 #include "qemu/bitops.h"
+#include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
 #include "qemu/log.h"
 
-- 
2.15.1

[Qemu-devel] [PATCH v6 02/13] sdhci: remove dead code

[Philippe Mathieu-Daudé]

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         | 2 --
 2 files changed, 4 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 1335373d3c..dacd726537 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -44,8 +44,6 @@ typedef struct SDHCIState {
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
-    qemu_irq eject_cb;
-    qemu_irq ro_cb;
     qemu_irq irq;
 
     /* Registers cleared on reset */
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b7d2a20985..365bc80009 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1200,8 +1200,6 @@ static void sdhci_uninitfn(SDHCIState *s)
     timer_free(s->insert_timer);
     timer_del(s->transfer_timer);
     timer_free(s->transfer_timer);
-    qemu_free_irq(s->eject_cb);
-    qemu_free_irq(s->ro_cb);
 
     g_free(s->fifo_buffer);
     s->fifo_buffer = NULL;
-- 
2.15.1

[Qemu-devel] [PATCH v6 03/13] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

Now both sysbus/pci classes inherit of the 'pending-insert-quirk' property,
which is a HCI dependent property (regardless if accessed through a MMIO
sysbus or a PCI bus).
So far only the BCM implementation has to use it.

Add sysbus/pci/sdbus comments to have clearer code blocks separation.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 include/hw/sd/sdhci.h |  4 +++-
 hw/sd/sdhci.c         | 21 ++++++++++-----------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index dacd726537..8041c9629e 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -79,13 +79,15 @@ typedef struct SDHCIState {
     uint32_t buf_maxsz;
     uint16_t data_count;   /* current element in FIFO buffer */
     uint8_t  stopped_state;/* Current SDHC state */
-    bool     pending_insert_quirk;/* Quirk for Raspberry Pi card insert int */
     bool     pending_insert_state;
     /* Buffer Data Port Register - virtual access point to R and W buffers */
     /* Software Reset Register - always reads as 0 */
     /* Force Event Auto CMD12 Error Interrupt Reg - write only */
     /* Force Event Error Interrupt Register- write only */
     /* RO Host Controller Version Register always reads as 0x2401 */
+
+    /* Configurable properties */
+    bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
 } SDHCIState;
 
 #define TYPE_PCI_SDHCI "sdhci-pci"
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 365bc80009..a11469fbca 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1266,13 +1266,17 @@ const VMStateDescription sdhci_vmstate = {
 
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
-static Property sdhci_pci_properties[] = {
+static Property sdhci_properties[] = {
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* --- qdev PCI --- */
+
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
     SDHCIState *s = PCI_SDHCI(dev);
@@ -1305,7 +1309,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_pci_properties;
+    dc->props = sdhci_properties;
     dc->reset = sdhci_poweron_reset;
 }
 
@@ -1320,14 +1324,7 @@ static const TypeInfo sdhci_pci_info = {
     },
 };
 
-static Property sdhci_sysbus_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
-            SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
-                     false),
-    DEFINE_PROP_END_OF_LIST(),
-};
+/* --- qdev SysBus --- */
 
 static void sdhci_sysbus_init(Object *obj)
 {
@@ -1360,7 +1357,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_sysbus_properties;
+    dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
     dc->reset = sdhci_poweron_reset;
 }
@@ -1374,6 +1371,8 @@ static const TypeInfo sdhci_sysbus_info = {
     .class_init = sdhci_sysbus_class_init,
 };
 
+/* --- qdev bus master --- */
+
 static void sdhci_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);
-- 
2.15.1

Re: [Qemu-devel] [PATCH v6 03/13] sdhci: refactor same sysbus/pci properties into a common one

[Peter Maydell]

On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Now both sysbus/pci classes inherit of the 'pending-insert-quirk' property,
> which is a HCI dependent property (regardless if accessed through a MMIO
> sysbus or a PCI bus).
> So far only the BCM implementation has to use it.
>
> Add sysbus/pci/sdbus comments to have clearer code blocks separation.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

I'm not sure about this patch, because it means that we now have
no mechanism for having a property which exists on the sysbus
device but not on the PCI device (and as per my comments on the
last patch in this set, we do need to be able to do that).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 03/13] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

On Fri, Jan 12, 2018 at 2:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Now both sysbus/pci classes inherit of the 'pending-insert-quirk' property,
>> which is a HCI dependent property (regardless if accessed through a MMIO
>> sysbus or a PCI bus).
>> So far only the BCM implementation has to use it.
>>
>> Add sysbus/pci/sdbus comments to have clearer code blocks separation.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>
> I'm not sure about this patch, because it means that we now have
> no mechanism for having a property which exists on the sysbus
> device but not on the PCI device (and as per my comments on the
> last patch in this set, we do need to be able to do that).

This might be what Alistair tried to explain me in his previous reviews...

I'll respin, thanks!

Phil.

Re: [Qemu-devel] [PATCH v6 03/13] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

Hi Peter,

On 01/12/2018 02:20 PM, Philippe Mathieu-Daudé wrote:
> On Fri, Jan 12, 2018 at 2:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Now both sysbus/pci classes inherit of the 'pending-insert-quirk' property,
>>> which is a HCI dependent property (regardless if accessed through a MMIO
>>> sysbus or a PCI bus).
>>> So far only the BCM implementation has to use it.
>>>
>>> Add sysbus/pci/sdbus comments to have clearer code blocks separation.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>
>> I'm not sure about this patch, because it means that we now have
>> no mechanism for having a property which exists on the sysbus
>> device but not on the PCI device (and as per my comments on the
>> last patch in this set, we do need to be able to do that).

Does this look OK to you?

static Property sdhci_sysbus_pending_insert_quirk_property =
     DEFINE_PROP_BOOL("pending-insert-quirk",
           SDHCIState, pending_insert_quirk, false);

static Property sdhci_sysbus_dma_mr_property =
     DEFINE_PROP_LINK("dma",
         SDHCIState, dma_mr, TYPE_MEMORY_REGION, MemoryRegion *);

static void sdhci_sysbus_post_init(Object *obj)
{
    SDHCIState *s = SYSBUS_SDHCI(obj);

    sdhci_common_post_init(obj); /* add common properties */
    qdev_property_add_static(DEVICE(obj),
        &sdhci_sysbus_pending_insert_quirk_property, &error_abort);
    qdev_property_add_static(DEVICE(obj),
        &sdhci_sysbus_dma_mr_property, &error_abort);
}

static const TypeInfo sdhci_sysbus_info = {
    .name = TYPE_SYSBUS_SDHCI,
    .parent = TYPE_SYS_BUS_DEVICE,
    .instance_size = sizeof(SDHCIState),
    .instance_init = sdhci_sysbus_init,
    .instance_post_init = sdhci_sysbus_post_init,
    ...
};

This way we can have a property which exists on the sysbus
device but not on the PCI device, and keep the common one in the same
piece of code (I later add a few common properties).

Regards,

Phil.

Re: [Qemu-devel] [PATCH v6 03/13] sdhci: refactor same sysbus/pci properties into a common one

[Philippe Mathieu-Daudé]

> On 01/12/2018 02:20 PM, Philippe Mathieu-Daudé wrote:
>> On Fri, Jan 12, 2018 at 2:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Now both sysbus/pci classes inherit of the 'pending-insert-quirk' property,
>>>> which is a HCI dependent property (regardless if accessed through a MMIO
>>>> sysbus or a PCI bus).
>>>> So far only the BCM implementation has to use it.
>>>>
>>>> Add sysbus/pci/sdbus comments to have clearer code blocks separation.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>>
>>> I'm not sure about this patch, because it means that we now have
>>> no mechanism for having a property which exists on the sysbus
>>> device but not on the PCI device (and as per my comments on the
>>> last patch in this set, we do need to be able to do that).
>
> Does this look OK to you?
>
> static Property sdhci_sysbus_pending_insert_quirk_property =
>      DEFINE_PROP_BOOL("pending-insert-quirk",
>            SDHCIState, pending_insert_quirk, false);
>
> static Property sdhci_sysbus_dma_mr_property =
>      DEFINE_PROP_LINK("dma",
>          SDHCIState, dma_mr, TYPE_MEMORY_REGION, MemoryRegion *);
>
> static void sdhci_sysbus_post_init(Object *obj)

No need to use the post_init(), this can go in the sdhci_initfn().

> {
>     SDHCIState *s = SYSBUS_SDHCI(obj);
>
>     sdhci_common_post_init(obj); /* add common properties */
>     qdev_property_add_static(DEVICE(obj),
>         &sdhci_sysbus_pending_insert_quirk_property, &error_abort);
>     qdev_property_add_static(DEVICE(obj),
>         &sdhci_sysbus_dma_mr_property, &error_abort);
> }
>
> static const TypeInfo sdhci_sysbus_info = {
>     .name = TYPE_SYSBUS_SDHCI,
>     .parent = TYPE_SYS_BUS_DEVICE,
>     .instance_size = sizeof(SDHCIState),
>     .instance_init = sdhci_sysbus_init,
>     .instance_post_init = sdhci_sysbus_post_init,

Not needed.

>     ...
> };
>
> This way we can have a property which exists on the sysbus
> device but not on the PCI device, and keep the common one in the same
> piece of code (I later add a few common properties).

[Qemu-devel] [PATCH v6 04/13] sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()

[Philippe Mathieu-Daudé]

Now both inherited classes appear as DEVICE_CATEGORY_STORAGE.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index a11469fbca..68bcf75478 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1275,6 +1275,16 @@ static Property sdhci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void sdhci_common_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->vmsd = &sdhci_vmstate;
+    dc->props = sdhci_properties;
+    dc->reset = sdhci_poweron_reset;
+}
+
 /* --- qdev PCI --- */
 
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
@@ -1299,7 +1309,6 @@ static void sdhci_pci_exit(PCIDevice *dev)
 
 static void sdhci_pci_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = sdhci_pci_realize;
@@ -1307,10 +1316,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
-    dc->reset = sdhci_poweron_reset;
+
+    sdhci_common_class_init(klass, data);
 }
 
 static const TypeInfo sdhci_pci_info = {
@@ -1356,10 +1363,9 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
-    dc->reset = sdhci_poweron_reset;
+
+    sdhci_common_class_init(klass, data);
 }
 
 static const TypeInfo sdhci_sysbus_info = {
-- 
2.15.1

[Qemu-devel] [PATCH v6 05/13] sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 68bcf75478..b3dbd994fd 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1194,6 +1194,15 @@ static void sdhci_initfn(SDHCIState *s)
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
 }
 
+static void sdhci_common_realize(SDHCIState *s, Error **errp)
+{
+    s->buf_maxsz = sdhci_get_fifolen(s);
+    s->fifo_buffer = g_malloc0(s->buf_maxsz);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
+                          SDHC_REGISTERS_MAP_SIZE);
+}
+
 static void sdhci_uninitfn(SDHCIState *s)
 {
     timer_del(s->insert_timer);
@@ -1290,14 +1299,16 @@ static void sdhci_common_class_init(ObjectClass *klass, void *data)
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
     SDHCIState *s = PCI_SDHCI(dev);
+
+    sdhci_initfn(s);
+    sdhci_common_realize(s, errp);
+    if (errp && *errp) {
+        return;
+    }
+
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
-    sdhci_initfn(s);
-    s->buf_maxsz = sdhci_get_fifolen(s);
-    s->fifo_buffer = g_malloc0(s->buf_maxsz);
     s->irq = pci_allocate_irq(dev);
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
-            SDHC_REGISTERS_MAP_SIZE);
     pci_register_bar(dev, 0, 0, &s->iomem);
 }
 
@@ -1351,11 +1362,12 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    s->buf_maxsz = sdhci_get_fifolen(s);
-    s->fifo_buffer = g_malloc0(s->buf_maxsz);
+    sdhci_common_realize(s, errp);
+    if (errp && *errp) {
+        return;
+    }
+
     sysbus_init_irq(sbd, &s->irq);
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
-            SDHC_REGISTERS_MAP_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
-- 
2.15.1

[Qemu-devel] [PATCH v6 06/13] sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b3dbd994fd..089961bd66 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -31,6 +31,7 @@
 #include "qemu/bitops.h"
 #include "hw/sd/sdhci.h"
 #include "sdhci-internal.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
 
 /* host controller debug messages */
@@ -1203,15 +1204,23 @@ static void sdhci_common_realize(SDHCIState *s, Error **errp)
                           SDHC_REGISTERS_MAP_SIZE);
 }
 
+static void sdhci_common_unrealize(SDHCIState *s, Error **errp)
+{
+    /* This function is expected to be called only once for each class:
+     * - SysBus:    via DeviceClass->unrealize(),
+     * - PCI:       via PCIDeviceClass->exit().
+     * However to avoid double-free and/or use-after-free we still nullify
+     * this variable (better safe than sorry!). */
+    g_free(s->fifo_buffer);
+    s->fifo_buffer = NULL;
+}
+
 static void sdhci_uninitfn(SDHCIState *s)
 {
     timer_del(s->insert_timer);
     timer_free(s->insert_timer);
     timer_del(s->transfer_timer);
     timer_free(s->transfer_timer);
-
-    g_free(s->fifo_buffer);
-    s->fifo_buffer = NULL;
 }
 
 static bool sdhci_pending_insert_vmstate_needed(void *opaque)
@@ -1315,6 +1324,8 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 static void sdhci_pci_exit(PCIDevice *dev)
 {
     SDHCIState *s = PCI_SDHCI(dev);
+
+    sdhci_common_unrealize(s, &error_abort);
     sdhci_uninitfn(s);
 }
 
@@ -1371,11 +1382,19 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
+static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
+{
+    SDHCIState *s = SYSBUS_SDHCI(dev);
+
+    sdhci_common_unrealize(s, &error_abort);
+}
+
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = sdhci_sysbus_realize;
+    dc->unrealize = sdhci_sysbus_unrealize;
 
     sdhci_common_class_init(klass, data);
 }
-- 
2.15.1

[Qemu-devel] [PATCH v6 07/13] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()

[Philippe Mathieu-Daudé]

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

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

[Qemu-devel] [PATCH v6 08/13] sdhci: convert the DPRINT() calls into trace events

[Philippe Mathieu-Daudé]

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

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

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0e27dc3042..ad60cd50d3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -33,30 +33,7 @@
 #include "sdhci-internal.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
-
-/* host controller debug messages */
-#ifndef SDHC_DEBUG
-#define SDHC_DEBUG                        0
-#endif
-
-#define DPRINT_L1(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG) { \
-            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
-        } \
-    } while (0)
-#define DPRINT_L2(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG > 1) { \
-            fprintf(stderr, "QEMU SDHC: " fmt, ## args); \
-        } \
-    } while (0)
-#define ERRPRINT(fmt, args...) \
-    do { \
-        if (SDHC_DEBUG) { \
-            fprintf(stderr, "QEMU SDHC ERROR: " fmt, ## args); \
-        } \
-    } while (0)
+#include "trace.h"
 
 #define TYPE_SDHCI_BUS "sdhci-bus"
 #define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_SDHCI_BUS)
@@ -155,8 +132,8 @@ static void sdhci_raise_insertion_irq(void *opaque)
 static void sdhci_set_inserted(DeviceState *dev, bool level)
 {
     SDHCIState *s = (SDHCIState *)dev;
-    DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
 
+    trace_sdhci_set_inserted(level ? "insert" : "eject");
     if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
         /* Give target some time to notice card ejection */
         timer_mod(s->insert_timer,
@@ -238,7 +215,8 @@ static void sdhci_send_command(SDHCIState *s)
     s->acmd12errsts = 0;
     request.cmd = s->cmdreg >> 8;
     request.arg = s->argument;
-    DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
+
+    trace_sdhci_send_command(request.cmd, request.arg);
     rlen = sdbus_do_command(&s->sdbus, &request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
@@ -246,7 +224,7 @@ static void sdhci_send_command(SDHCIState *s)
             s->rspreg[0] = (response[0] << 24) | (response[1] << 16) |
                            (response[2] << 8)  |  response[3];
             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
-            DPRINT_L1("Response: RSPREG[31..0]=0x%08x\n", s->rspreg[0]);
+            trace_sdhci_response4(s->rspreg[0]);
         } else if (rlen == 16) {
             s->rspreg[0] = (response[11] << 24) | (response[12] << 16) |
                            (response[13] << 8) |  response[14];
@@ -256,11 +234,10 @@ static void sdhci_send_command(SDHCIState *s)
                            (response[5] << 8)  |  response[6];
             s->rspreg[3] = (response[0] << 16) | (response[1] << 8) |
                             response[2];
-            DPRINT_L1("Response received:\n RSPREG[127..96]=0x%08x, RSPREG[95.."
-                  "64]=0x%08x,\n RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x\n",
-                  s->rspreg[3], s->rspreg[2], s->rspreg[1], s->rspreg[0]);
+            trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
+                                   s->rspreg[1], s->rspreg[0]);
         } else {
-            ERRPRINT("Timeout waiting for command response\n");
+            trace_sdhci_error("timeout waiting for command response");
             if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
                 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
                 s->norintsts |= SDHC_NIS_ERR;
@@ -294,7 +271,7 @@ static void sdhci_end_transfer(SDHCIState *s)
 
         request.cmd = 0x0C;
         request.arg = 0;
-        DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, request.arg);
+        trace_sdhci_end_transfer(request.cmd, request.arg);
         sdbus_do_command(&s->sdbus, &request, response);
         /* Auto CMD12 response goes to the upper Response register */
         s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
@@ -363,7 +340,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
 
     /* first check that a valid data exists in host controller input buffer */
     if ((s->prnsts & SDHC_DATA_AVAILABLE) == 0) {
-        ERRPRINT("Trying to read from empty buffer\n");
+        trace_sdhci_error("read from empty buffer");
         return 0;
     }
 
@@ -372,8 +349,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size)
         s->data_count++;
         /* check if we've read all valid data (blksize bytes) from buffer */
         if ((s->data_count) >= (s->blksize & 0x0fff)) {
-            DPRINT_L2("All %u bytes of data have been read from input buffer\n",
-                    s->data_count);
+            trace_sdhci_read_dataport(s->data_count);
             s->prnsts &= ~SDHC_DATA_AVAILABLE; /* no more data in a buffer */
             s->data_count = 0;  /* next buff read must start at position [0] */
 
@@ -456,7 +432,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
 
     /* Check that there is free space left in a buffer */
     if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) {
-        ERRPRINT("Can't write to data buffer: buffer full\n");
+        trace_sdhci_error("Can't write to data buffer: buffer full");
         return;
     }
 
@@ -465,8 +441,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
         s->data_count++;
         value >>= 8;
         if (s->data_count >= (s->blksize & 0x0fff)) {
-            DPRINT_L2("write buffer filled with %u bytes of data\n",
-                    s->data_count);
+            trace_sdhci_write_dataport(s->data_count);
             s->data_count = 0;
             s->prnsts &= ~SDHC_SPACE_AVAILABLE;
             if (s->prnsts & SDHC_DOING_WRITE) {
@@ -654,15 +629,14 @@ static void sdhci_do_adma(SDHCIState *s)
 {
     unsigned int n, begin, length;
     const uint16_t block_size = s->blksize & 0x0fff;
-    ADMADescr dscr;
+    ADMADescr dscr = {};
     int i;
 
     for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
         get_adma_description(s, &dscr);
-        DPRINT_L2("ADMA loop: addr=" TARGET_FMT_plx ", len=%d, attr=%x\n",
-                dscr.addr, dscr.length, dscr.attr);
+        trace_sdhci_adma_loop(dscr.addr, dscr.length, dscr.attr);
 
         if ((dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
             /* Indicate that error occurred in ST_FDS state */
@@ -745,8 +719,7 @@ static void sdhci_do_adma(SDHCIState *s)
             break;
         case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
             s->admasysaddr = dscr.addr;
-            DPRINT_L1("ADMA link: admasysaddr=0x%" PRIx64 "\n",
-                      s->admasysaddr);
+            trace_sdhci_adma("link", s->admasysaddr);
             break;
         default:
             s->admasysaddr += dscr.incr;
@@ -754,8 +727,7 @@ static void sdhci_do_adma(SDHCIState *s)
         }
 
         if (dscr.attr & SDHC_ADMA_ATTR_INT) {
-            DPRINT_L1("ADMA interrupt: admasysaddr=0x%" PRIx64 "\n",
-                      s->admasysaddr);
+            trace_sdhci_adma("interrupt", s->admasysaddr);
             if (s->norintstsen & SDHC_NISEN_DMA) {
                 s->norintsts |= SDHC_NIS_DMA;
             }
@@ -766,15 +738,15 @@ static void sdhci_do_adma(SDHCIState *s)
         /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
         if (((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
                     (s->blkcnt == 0)) || (dscr.attr & SDHC_ADMA_ATTR_END)) {
-            DPRINT_L2("ADMA transfer completed\n");
+            trace_sdhci_adma_transfer_completed();
             if (length || ((dscr.attr & SDHC_ADMA_ATTR_END) &&
                 (s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
                 s->blkcnt != 0)) {
-                ERRPRINT("SD/MMC host ADMA length mismatch\n");
+                trace_sdhci_error("SD/MMC host ADMA length mismatch");
                 s->admaerr |= SDHC_ADMAERR_LENGTH_MISMATCH |
                         SDHC_ADMAERR_STATE_ST_TFR;
                 if (s->errintstsen & SDHC_EISEN_ADMAERR) {
-                    ERRPRINT("Set ADMA error flag\n");
+                    trace_sdhci_error("Set ADMA error flag");
                     s->errintsts |= SDHC_EIS_ADMAERR;
                     s->norintsts |= SDHC_NIS_ERR;
                 }
@@ -810,7 +782,7 @@ static void sdhci_data_transfer(void *opaque)
             break;
         case SDHC_CTRL_ADMA1_32:
             if (!(s->capareg & SDHC_CAN_DO_ADMA1)) {
-                ERRPRINT("ADMA1 not supported\n");
+                trace_sdhci_error("ADMA1 not supported");
                 break;
             }
 
@@ -818,7 +790,7 @@ static void sdhci_data_transfer(void *opaque)
             break;
         case SDHC_CTRL_ADMA2_32:
             if (!(s->capareg & SDHC_CAN_DO_ADMA2)) {
-                ERRPRINT("ADMA2 not supported\n");
+                trace_sdhci_error("ADMA2 not supported");
                 break;
             }
 
@@ -827,14 +799,14 @@ static void sdhci_data_transfer(void *opaque)
         case SDHC_CTRL_ADMA2_64:
             if (!(s->capareg & SDHC_CAN_DO_ADMA2) ||
                     !(s->capareg & SDHC_64_BIT_BUS_SUPPORT)) {
-                ERRPRINT("64 bit ADMA not supported\n");
+                trace_sdhci_error("64 bit ADMA not supported");
                 break;
             }
 
             sdhci_do_adma(s);
             break;
         default:
-            ERRPRINT("Unsupported DMA type\n");
+            trace_sdhci_error("Unsupported DMA type");
             break;
         }
     } else {
@@ -869,8 +841,8 @@ static inline bool
 sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
 {
     if ((s->data_count & 0x3) != byte_num) {
-        ERRPRINT("Non-sequential access to Buffer Data Port register"
-                "is prohibited\n");
+        trace_sdhci_error("Non-sequential access to Buffer Data Port register"
+                          "is prohibited\n");
         return false;
     }
     return true;
@@ -900,8 +872,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
     case  SDHC_BDATA:
         if (sdhci_buff_access_is_sequential(s, offset - SDHC_BDATA)) {
             ret = sdhci_read_dataport(s, size);
-            DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset,
-                      ret, ret);
+            trace_sdhci_access("rd", size << 3, offset, "->", ret, ret);
             return ret;
         }
         break;
@@ -953,7 +924,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
 
     ret >>= (offset & 0x3) * 8;
     ret &= (1ULL << (size * 8)) - 1;
-    DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, (int)offset, ret, ret);
+    trace_sdhci_access("rd", size << 3, offset, "->", ret, ret);
     return ret;
 }
 
@@ -1157,8 +1128,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                       "not implemented\n", size, offset, value >> shift);
         break;
     }
-    DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n",
-              size, (int)offset, value >> shift, value >> shift);
+    trace_sdhci_access("wr", size << 3, offset, "<-",
+                       value >> shift, value >> shift);
 }
 
 static const MemoryRegionOps sdhci_mmio_ops = {
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 6eca3470e2..0a121156a3 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -1,5 +1,19 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/sd/sdhci.c
+sdhci_set_inserted(const char *level) "card state changed: %s"
+sdhci_send_command(uint8_t cmd, uint32_t arg) "CMD%02u ARG[0x%08x]"
+sdhci_error(const char *msg) "%s"
+sdhci_response4(uint32_t r0) "RSPREG[31..0]=0x%08x"
+sdhci_response16(uint32_t r3, uint32_t r2, uint32_t r1, uint32_t r0) "RSPREG[127..96]=0x%08x, RSPREG[95..64]=0x%08x, RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x"
+sdhci_end_transfer(uint8_t cmd, uint32_t arg) "Automatically issue CMD%02u 0x%08x"
+sdhci_adma(const char *desc, uint32_t sysad) "%s: admasysaddr=0x%" PRIx32
+sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr) "addr=0x%08" PRIx64 ", len=%d, attr=0x%x"
+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"
+
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
 milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.15.1

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

[Philippe Mathieu-Daudé]

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

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

[Qemu-devel] [PATCH v6 10/13] sdhci: rename the SDHC_CAPAB register

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

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index e941bc2386..fc807f08f3 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -174,7 +174,7 @@
 #define SDHC_ACMD12ERRSTS              0x3C
 
 /* HWInit Capabilities Register 0x05E80080 */
-#define SDHC_CAPAREG                   0x40
+#define SDHC_CAPAB                     0x40
 #define SDHC_CAN_DO_DMA                0x00400000
 #define SDHC_CAN_DO_ADMA2              0x00080000
 #define SDHC_CAN_DO_ADMA1              0x00100000
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 73c1efbef3..f615adca97 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -897,7 +897,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
     case SDHC_ACMD12ERRSTS:
         ret = s->acmd12errsts;
         break;
-    case SDHC_CAPAREG:
+    case SDHC_CAPAB:
         ret = s->capareg;
         break;
     case SDHC_MAXCURR:
-- 
2.15.1

[Qemu-devel] [PATCH v6 11/13] sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only

[Philippe Mathieu-Daudé]

running qtests:

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

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 8041c9629e..442e30aff2 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -72,8 +72,8 @@ typedef struct SDHCIState {
     uint64_t admasysaddr;  /* ADMA System Address Register */
 
     /* Read-only registers */
-    uint32_t capareg;      /* Capabilities Register */
-    uint32_t maxcurr;      /* Maximum Current Capabilities Register */
+    uint64_t capareg;      /* Capabilities Register */
+    uint64_t maxcurr;      /* Maximum Current Capabilities Register */
 
     uint8_t  *fifo_buffer; /* SD host i/o FIFO buffer */
     uint32_t buf_maxsz;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f615adca97..18db67776f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -898,10 +898,16 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = s->acmd12errsts;
         break;
     case SDHC_CAPAB:
-        ret = s->capareg;
+        ret = (uint32_t)s->capareg;
+        break;
+    case SDHC_CAPAB + 4:
+        ret = (uint32_t)(s->capareg >> 32);
         break;
     case SDHC_MAXCURR:
-        ret = s->maxcurr;
+        ret = (uint32_t)s->maxcurr;
+        break;
+    case SDHC_MAXCURR + 4:
+        ret = (uint32_t)(s->maxcurr >> 32);
         break;
     case SDHC_ADMAERR:
         ret =  s->admaerr;
@@ -1122,6 +1128,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         }
         sdhci_update_irq(s);
         break;
+
+    case SDHC_CAPAB:
+    case SDHC_CAPAB + 4:
+    case SDHC_MAXCURR:
+    case SDHC_MAXCURR + 4:
+        qemu_log_mask(LOG_GUEST_ERROR, "SDHC wr_%ub @0x%02" HWADDR_PRIx
+                      " <- 0x%08x read-only\n", size, offset, value >> shift);
+        break;
+
     default:
         qemu_log_mask(LOG_UNIMP, "SDHC wr_%ub @0x%02" HWADDR_PRIx " <- 0x%08x "
                       "not implemented\n", size, offset, value >> shift);
@@ -1256,9 +1271,9 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
+    DEFINE_PROP_UINT64("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.15.1

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

[Philippe Mathieu-Daudé]

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

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 18db67776f..c86c2571b7 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1128,6 +1128,9 @@ 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);
+        break;
 
     case SDHC_CAPAB:
     case SDHC_CAPAB + 4:
-- 
2.15.1

[Qemu-devel] [PATCH v6 13/13] sdhci: add a "dma" property

[Philippe Mathieu-Daudé]

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

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 442e30aff2..6117004949 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -41,6 +41,8 @@ typedef struct SDHCIState {
     /*< public >*/
     SDBus sdbus;
     MemoryRegion iomem;
+    MemoryRegion *dma_mr;
+    AddressSpace dma_as;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c86c2571b7..7a2901ae4a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -495,7 +495,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                     s->blkcnt--;
                 }
             }
-            dma_memory_write(&address_space_memory, s->sdmasysad,
+            dma_memory_write(&s->dma_as, s->sdmasysad,
                              &s->fifo_buffer[begin], s->data_count - begin);
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
@@ -517,7 +517,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                 s->data_count = block_size;
                 boundary_count -= block_size - begin;
             }
-            dma_memory_read(&address_space_memory, s->sdmasysad,
+            dma_memory_read(&s->dma_as, s->sdmasysad,
                             &s->fifo_buffer[begin], s->data_count - begin);
             s->sdmasysad += s->data_count - begin;
             if (s->data_count == block_size) {
@@ -555,11 +555,9 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
         for (n = 0; n < datacnt; n++) {
             s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
         }
-        dma_memory_write(&address_space_memory, s->sdmasysad, s->fifo_buffer,
-                         datacnt);
+        dma_memory_write(&s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
     } else {
-        dma_memory_read(&address_space_memory, s->sdmasysad, s->fifo_buffer,
-                        datacnt);
+        dma_memory_read(&s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt);
         for (n = 0; n < datacnt; n++) {
             sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
         }
@@ -583,7 +581,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
     switch (SDHC_DMA_TYPE(s->hostctl)) {
     case SDHC_CTRL_ADMA2_32:
-        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma2,
+        dma_memory_read(&s->dma_as, entry_addr, (uint8_t *)&adma2,
                         sizeof(adma2));
         adma2 = le64_to_cpu(adma2);
         /* The spec does not specify endianness of descriptor table.
@@ -595,7 +593,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dscr->incr = 8;
         break;
     case SDHC_CTRL_ADMA1_32:
-        dma_memory_read(&address_space_memory, entry_addr, (uint8_t *)&adma1,
+        dma_memory_read(&s->dma_as, entry_addr, (uint8_t *)&adma1,
                         sizeof(adma1));
         adma1 = le32_to_cpu(adma1);
         dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
@@ -608,12 +606,12 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         }
         break;
     case SDHC_CTRL_ADMA2_64:
-        dma_memory_read(&address_space_memory, entry_addr,
+        dma_memory_read(&s->dma_as, entry_addr,
                         (uint8_t *)(&dscr->attr), 1);
-        dma_memory_read(&address_space_memory, entry_addr + 2,
+        dma_memory_read(&s->dma_as, entry_addr + 2,
                         (uint8_t *)(&dscr->length), 2);
         dscr->length = le16_to_cpu(dscr->length);
-        dma_memory_read(&address_space_memory, entry_addr + 4,
+        dma_memory_read(&s->dma_as, entry_addr + 4,
                         (uint8_t *)(&dscr->addr), 8);
         dscr->attr = le64_to_cpu(dscr->attr);
         dscr->attr &= 0xfffffff8;
@@ -672,7 +670,7 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_write(&address_space_memory, dscr.addr,
+                    dma_memory_write(&s->dma_as, dscr.addr,
                                      &s->fifo_buffer[begin],
                                      s->data_count - begin);
                     dscr.addr += s->data_count - begin;
@@ -696,7 +694,7 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_read(&address_space_memory, dscr.addr,
+                    dma_memory_read(&s->dma_as, dscr.addr,
                                     &s->fifo_buffer[begin],
                                     s->data_count - begin);
                     dscr.addr += s->data_count - begin;
@@ -1191,10 +1189,20 @@ static void sdhci_common_realize(SDHCIState *s, Error **errp)
 
     memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
                           SDHC_REGISTERS_MAP_SIZE);
+
+    /* use system_memory() if property "dma" not set */
+    address_space_init(&s->dma_as,
+                       s->dma_mr ? s->dma_mr : get_system_memory(),
+                       "sdhci-dma");
 }
 
 static void sdhci_common_unrealize(SDHCIState *s, Error **errp)
 {
+    if (s->dma_mr) {
+        address_space_destroy(&s->dma_as);
+        object_unparent(OBJECT(&s->dma_mr));
+    }
+
     /* This function is expected to be called only once for each class:
      * - SysBus:    via DeviceClass->unrealize(),
      * - PCI:       via PCIDeviceClass->exit().
@@ -1279,6 +1287,8 @@ static Property sdhci_properties[] = {
     DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                      false),
+    DEFINE_PROP_LINK("dma", SDHCIState, dma_mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.15.1

Re: [Qemu-devel] [PATCH v6 13/13] sdhci: add a "dma" property

[Peter Maydell]

On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Add a dma property allowing machine creation to provide the address-space
> sdhci dma operates on.
>
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2016.1]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  include/hw/sd/sdhci.h |  2 ++
>  hw/sd/sdhci.c         | 36 +++++++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 13 deletions(-)

This looks OK for the sysbus sdhci controller, but not for the
PCI one:
 (1) the link property is meaningless for PCI, and we shouldn't
     expose a meaningless thing to the end user
 (2) the PCI device should surely be doing its DMA via the AS
     returned by pci_get_address_space() ?  (compare pci_dma_read &c
     in include/hw/pci/pci.h)

2 is a bugfix to the existing code, of course, but if we're
going to tidy this up then the right answer to 2 may affect
how you want to handle 1.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 13/13] sdhci: add a "dma" property

[Philippe Mathieu-Daudé]

On Fri, Jan 12, 2018 at 2:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 January 2018 at 19:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Add a dma property allowing machine creation to provide the address-space
>> sdhci dma operates on.
>>
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2016.1]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>  include/hw/sd/sdhci.h |  2 ++
>>  hw/sd/sdhci.c         | 36 +++++++++++++++++++++++-------------
>>  2 files changed, 25 insertions(+), 13 deletions(-)
>
> This looks OK for the sysbus sdhci controller, but not for the
> PCI one:
>  (1) the link property is meaningless for PCI, and we shouldn't
>      expose a meaningless thing to the end user
>  (2) the PCI device should surely be doing its DMA via the AS
>      returned by pci_get_address_space() ?  (compare pci_dma_read &c
>      in include/hw/pci/pci.h)
>
> 2 is a bugfix to the existing code, of course, but if we're
> going to tidy this up then the right answer to 2 may affect
> how you want to handle 1.

Ok. I'll respin, thanks for your review :)

Re: [Qemu-devel] [PATCH v6 00/13] SDHCI: housekeeping

[Alistair Francis]

On Thu, Jan 11, 2018 at 11:30 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> All patches reviewed :)

Do you think this can go through you Peter?

Alistair

>
> Since v5:
> - addressed Alistair reviews
> - added Alistair R-b
> - renamed the dma property "dma-memory" -> "dma"
>
> Since v4:
> - fixed incorrect use of &local_err in sdhci_sysbus/pci_realize()
>
> Since v3:
> - since the series was getting too big and first part reviewed, split in 2.
> - addressed Fam's review from "refactor the common sysbus/pci qdev"
>   - improved commit descriptions
>   - restored useful s->fifo_buffer = NULL
> - added Alistair R-b
>
> 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
> 001/13:[----] [--] 'sdhci: clean up includes'
> 002/13:[----] [--] 'sdhci: remove dead code'
> 003/13:[0004] [FC] 'sdhci: refactor same sysbus/pci properties into a common one'
> 004/13:[----] [--] 'sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()'
> 005/13:[----] [--] 'sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()'
> 006/13:[----] [--] 'sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()'
> 007/13:[----] [--] 'sdhci: use qemu_log_mask(UNIMP) instead of fprintf()'
> 008/13:[0020] [FC] 'sdhci: convert the DPRINT() calls into trace events'
> 009/13:[----] [--] 'sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"'
> 010/13:[----] [-C] 'sdhci: rename the SDHC_CAPAB register'
> 011/13:[0002] [FC] 'sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only'
> 012/13:[----] [--] 'sdhci: Implement write method of ACMD12ERRSTS register'
> 013/13:[0004] [FC] 'sdhci: add a "dma" property'
>
> Andrey Smirnov (1):
>   sdhci: Implement write method of ACMD12ERRSTS register
>
> Philippe Mathieu-Daudé (12):
>   sdhci: clean up includes
>   sdhci: remove dead code
>   sdhci: refactor same sysbus/pci properties into a common one
>   sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()
>   sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()
>   sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()
>   sdhci: use qemu_log_mask(UNIMP) instead of fprintf()
>   sdhci: convert the DPRINT() calls into trace events
>   sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"
>   sdhci: rename the SDHC_CAPAB register
>   sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only
>   sdhci: add a "dma" property
>
>  include/hw/sd/sdhci.h  |  19 ++--
>  hw/sd/sdhci-internal.h |   7 +-
>  hw/sd/sdhci.c          | 260 ++++++++++++++++++++++++++++---------------------
>  hw/sd/trace-events     |  14 +++
>  4 files changed, 176 insertions(+), 124 deletions(-)
>
> --
> 2.15.1
>
>