Changeset
include/hw/sd/sdhci.h  |  19 +++-
hw/sd/sdhci-internal.h |   7 +-
hw/sd/sdhci.c          | 291 +++++++++++++++++++++++++++++--------------------
hw/sd/trace-events     |  14 +++
4 files changed, 204 insertions(+), 127 deletions(-)
Git apply log
Switched to a new branch '20180113050717.22969-1-f4bug@amsat.org'
Applying: sdhci: clean up includes
Applying: sdhci: remove dead code
Applying: sdhci: refactor same sysbus/pci properties into a common one
Applying: sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()
Applying: sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()
Applying: sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()
Applying: sdhci: use qemu_log_mask(UNIMP) instead of fprintf()
Applying: sdhci: convert the DPRINT() calls into trace events
Applying: sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"
Applying: sdhci: rename the SDHC_CAPAB register
Applying: sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only
Applying: sdhci: Implement write method of ACMD12ERRSTS register
Applying: sdhci: fix the PCI device, using the PCI address space for DMA
Applying: sdhci: add a 'dma' property to the sysbus devices
To https://github.com/patchew-project/qemu
 + 02c0935c72...4743dfb03e patchew/20180113050717.22969-1-f4bug@amsat.org -> patchew/20180113050717.22969-1-f4bug@amsat.org (forced update)
Test passed: docker

loading

Test passed: checkpatch

loading

Test passed: s390x

loading

Test passed: ppc

loading

[Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Philippe Mathieu-Daudé, 23 weeks ago
Since v6:
- addressed Peter reviews
  - do not use an unique Property[] for both sysbus/pci
    Peter didn't recommend me to use the qdev_property_add_static() API since
    it is only used by the ARM cpus and may be due for removal, however I found
    it cleaner.
    At the end of those series, the only differences is the sysbus having 2
    more properties: "pending-insert-quirk" and "dma", all other are generic
    to SDHCI and shared.
  - use the PCI AS for DMA.
    Should we check for the PCI_COMMAND_MASTER bit?

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

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

Philippe Mathieu-Daudé (13):
  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: fix the PCI device, using the PCI address space for DMA
  sdhci: add a 'dma' property to the sysbus devices

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

-- 
2.15.1


Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Peter Maydell, 22 weeks ago
On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Since v6:
> - addressed Peter reviews
>   - do not use an unique Property[] for both sysbus/pci
>     Peter didn't recommend me to use the qdev_property_add_static() API since
>     it is only used by the ARM cpus and may be due for removal, however I found
>     it cleaner.

Your cover letter says this but patches 3 and 14 in the v7 you sent
to the list use qdev_property_add_static(). Did you send the wrong
version of the code?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Philippe Mathieu-Daudé, 22 weeks ago
On 01/15/2018 10:51 AM, Peter Maydell wrote:
> On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Since v6:
>> - addressed Peter reviews
>>   - do not use an unique Property[] for both sysbus/pci
>>     Peter didn't recommend me to use the qdev_property_add_static() API since
>>     it is only used by the ARM cpus and may be due for removal, however I found
>>     it cleaner.
> 
> Your cover letter says this but patches 3 and 14 in the v7 you sent
> to the list use qdev_property_add_static(). Did you send the wrong
> version of the code?

The cover is not clear, I'll try to reword it:

'''
Peter recommended me to NON use the qdev_property_add_static() API since
it is only used by the ARM cpus and may be due for removal.

However I found using qdev_property_add_static() cleaner, and sent this
series with using the not recommended qdev_property_add_static().
'''

Is it possible to share properties without using qdev_property_add_static()?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Peter Maydell, 22 weeks ago
On 15 January 2018 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 01/15/2018 10:51 AM, Peter Maydell wrote:
>> On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Since v6:
>>> - addressed Peter reviews
>>>   - do not use an unique Property[] for both sysbus/pci
>>>     Peter didn't recommend me to use the qdev_property_add_static() API since
>>>     it is only used by the ARM cpus and may be due for removal, however I found
>>>     it cleaner.
>>
>> Your cover letter says this but patches 3 and 14 in the v7 you sent
>> to the list use qdev_property_add_static(). Did you send the wrong
>> version of the code?
>
> The cover is not clear, I'll try to reword it:
>
> '''
> Peter recommended me to NON use the qdev_property_add_static() API since
> it is only used by the ARM cpus and may be due for removal.
>
> However I found using qdev_property_add_static() cleaner, and sent this
> series with using the not recommended qdev_property_add_static().
> '''

> Is it possible to share properties without using qdev_property_add_static()?

Not very neatly. There's the DEFINE_BLOCK_PROPERTIES approach that
include/hw/block/block.h has.

In this case there aren't very many properties involved so I would
just leave them as two separate Property arrays.

(The Arm cpu models are odd because they dynamically decide which
properties they have based on the CPU model. That isn't the case here:
these devices always have the same set of properties.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Philippe Mathieu-Daudé, 22 weeks ago
On Mon, Jan 15, 2018 at 11:47 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 15 January 2018 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 01/15/2018 10:51 AM, Peter Maydell wrote:
>>> On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Since v6:
>>>> - addressed Peter reviews
>>>>   - do not use an unique Property[] for both sysbus/pci
>>>>     Peter didn't recommend me to use the qdev_property_add_static() API since
>>>>     it is only used by the ARM cpus and may be due for removal, however I found
>>>>     it cleaner.
>>>
>>> Your cover letter says this but patches 3 and 14 in the v7 you sent
>>> to the list use qdev_property_add_static(). Did you send the wrong
>>> version of the code?
>>
>> The cover is not clear, I'll try to reword it:
>>
>> '''
>> Peter recommended me to NON use the qdev_property_add_static() API since
>> it is only used by the ARM cpus and may be due for removal.
>>
>> However I found using qdev_property_add_static() cleaner, and sent this
>> series with using the not recommended qdev_property_add_static().
>> '''
>
>> Is it possible to share properties without using qdev_property_add_static()?
>
> Not very neatly. There's the DEFINE_BLOCK_PROPERTIES approach that
> include/hw/block/block.h has.
>
> In this case there aren't very many properties involved so I would
> just leave them as two separate Property arrays.
>
> (The Arm cpu models are odd because they dynamically decide which
> properties they have based on the CPU model. That isn't the case here:
> these devices always have the same set of properties.)

Yes, both sysbus/pci devices share:

static Property sdhci_common_properties[] = {
    DEFINE_PROP_UINT8("sd-spec-version", SDHCIState, spec_version, 2),

    /* Timeout clock frequency 1-63, 0 - not defined */
    DEFINE_PROP_UINT8("timeout-freq", SDHCIState, cap.timeout_clk_freq, 0),
    /* Timeout clock unit 0 - kHz, 1 - MHz */
    DEFINE_PROP_BOOL("freq-in-mhz", SDHCIState, cap.timeout_clk_in_mhz, true),
    /* Maximum base clock frequency for SD clock in MHz (range 10-63 MHz, 0) */
    DEFINE_PROP_UINT8("max-frequency", SDHCIState, cap.base_clk_freq_mhz, 0),

    /* Maximum host controller R/W buffers size
     * Possible values: 512, 1024, 2048 bytes */
    DEFINE_PROP_UINT16("max-block-length", SDHCIState, cap.max_blk_len, 512),
    /* DMA */
    DEFINE_PROP_BOOL("sdma", SDHCIState, cap.sdma, true),
    DEFINE_PROP_BOOL("adma1", SDHCIState, cap.adma1, false),
    DEFINE_PROP_BOOL("adma2", SDHCIState, cap.adma2, true),
    /* Suspend/resume support */
    DEFINE_PROP_BOOL("suspend", SDHCIState, cap.suspend, false),
    /* High speed support */
    DEFINE_PROP_BOOL("high-speed", SDHCIState, cap.high_speed, true),
    /* Voltage support 3.3/3.0/1.8V */
    DEFINE_PROP_BOOL("3v3", SDHCIState, cap.v33, true),
    DEFINE_PROP_BOOL("3v0", SDHCIState, cap.v30, false),
    DEFINE_PROP_BOOL("1v8", SDHCIState, cap.v18, false),
    DEFINE_PROP_UINT8("uhs", SDHCIState, cap.uhs_mode, UHS_NOT_SUPPORTED),

    DEFINE_PROP_BOOL("64bit", SDHCIState, cap.bus64, false),
    DEFINE_PROP_UINT8("slot-type", SDHCIState, cap.slot_type, 0),
    DEFINE_PROP_UINT8("bus-speed", SDHCIState, cap.sdr, 0),
    DEFINE_PROP_UINT8("driver-strength", SDHCIState, cap.strength, 0),

    DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0),
    DEFINE_PROP_END_OF_LIST(),
};

The only 2 properties specific to sysbus are:

static Property sdhci_sysbus_properties[] = {
    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                     false),
    DEFINE_PROP_LINK("dma", SDHCIState, dma_mr,
                     TYPE_MEMORY_REGION, MemoryRegion *),
}

I guess I assumed the Property to be const (being compilation-time
allocated, ended with DEFINE_PROP_END_OF_LIST),
so I couldn't add more properties to it, but it seems each device has
his properties allocated at runtime, so I can use the same
sdhci_common_properties[] array and only use
qdev_property_add_static() for the 2 sysbus specific properties.

Does this sound alright to you?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Paolo Bonzini, 22 weeks ago
On 15/01/2018 16:17, Philippe Mathieu-Daudé wrote:
> 
> The only 2 properties specific to sysbus are:
> 
> static Property sdhci_sysbus_properties[] = {
>     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>                      false),
>     DEFINE_PROP_LINK("dma", SDHCIState, dma_mr,
>                      TYPE_MEMORY_REGION, MemoryRegion *),
> }
> 
> I guess I assumed the Property to be const (being compilation-time
> allocated, ended with DEFINE_PROP_END_OF_LIST),
> so I couldn't add more properties to it, but it seems each device has
> his properties allocated at runtime, so I can use the same
> sdhci_common_properties[] array and only use
> qdev_property_add_static() for the 2 sysbus specific properties.

You can define a macro DEFINE_SDHCI_PROPERTIES for the "common" properties.

Paolo

Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)
Posted by Philippe Mathieu-Daudé, 22 weeks ago
On Mon, Jan 15, 2018 at 1:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 15/01/2018 16:17, Philippe Mathieu-Daudé wrote:
>>
>> The only 2 properties specific to sysbus are:
>>
>> static Property sdhci_sysbus_properties[] = {
>>     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>                      false),
>>     DEFINE_PROP_LINK("dma", SDHCIState, dma_mr,
>>                      TYPE_MEMORY_REGION, MemoryRegion *),
>> }
>>
>> I guess I assumed the Property to be const (being compilation-time
>> allocated, ended with DEFINE_PROP_END_OF_LIST),
>> so I couldn't add more properties to it, but it seems each device has
>> his properties allocated at runtime, so I can use the same
>> sdhci_common_properties[] array and only use
>> qdev_property_add_static() for the 2 sysbus specific properties.
>
> You can define a macro DEFINE_SDHCI_PROPERTIES for the "common" properties.

Clever :)

Thanks Paolo!

Phil.

[Qemu-devel] [PATCH v7 01/14] sdhci: clean up includes
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 v7 02/14] sdhci: remove dead code
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 v7 03/14] sdhci: refactor same sysbus/pci properties into a common one
Posted by Philippe Mathieu-Daudé, 23 weeks ago
Add common/sysbus/pci/sdbus comments to have clearer code blocks separation.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sdhci.h |  4 +++-
 hw/sd/sdhci.c         | 46 ++++++++++++++++++++++++++++------------------
 2 files changed, 31 insertions(+), 19 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..6c3389cfdc 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/hw.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
@@ -1185,8 +1186,23 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
     }
 }
 
+/* --- qdev common --- */
+
+/* Capabilities registers provide information on supported features of this
+ * specific host controller implementation */
+static Property sdhci_capareg_property =
+    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT);
+
+static Property sdhci_maxcurr_property =
+    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0);
+
 static void sdhci_initfn(SDHCIState *s)
 {
+    DeviceState *dev = DEVICE(s);
+
+    qdev_property_add_static(dev, &sdhci_capareg_property, &error_abort);
+    qdev_property_add_static(dev, &sdhci_maxcurr_property, &error_abort);
+
     qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
                         TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
 
@@ -1264,14 +1280,7 @@ const VMStateDescription sdhci_vmstate = {
     },
 };
 
-/* Capabilities registers provide information on supported features of this
- * specific host controller implementation */
-static Property sdhci_pci_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
-            SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-    DEFINE_PROP_END_OF_LIST(),
-};
+/* --- qdev PCI --- */
 
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
@@ -1305,7 +1314,6 @@ 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->reset = sdhci_poweron_reset;
 }
 
@@ -1320,20 +1328,21 @@ 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 Property sdhci_sysbus_pending_insert_quirk_property =
+     DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState,
+                      pending_insert_quirk, false);
 
 static void sdhci_sysbus_init(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
 
     sdhci_initfn(s);
+
+    qdev_property_add_static(DEVICE(obj),
+                             &sdhci_sysbus_pending_insert_quirk_property,
+                             &error_abort);
 }
 
 static void sdhci_sysbus_finalize(Object *obj)
@@ -1360,7 +1369,6 @@ 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->realize = sdhci_sysbus_realize;
     dc->reset = sdhci_poweron_reset;
 }
@@ -1374,6 +1382,8 @@ static const TypeInfo sdhci_sysbus_info = {
     .class_init = sdhci_sysbus_class_init,
 };
 
+/* --- qdev bus master --- */
+
 static void sdhci_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);
-- 
2.15.1


[Qemu-devel] [PATCH v7 04/14] sdhci: refactor common sysbus/pci class_init() into sdhci_common_class_init()
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6c3389cfdc..191f0d26ee 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1280,6 +1280,15 @@ const VMStateDescription sdhci_vmstate = {
     },
 };
 
+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->reset = sdhci_poweron_reset;
+}
+
 /* --- qdev PCI --- */
 
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
@@ -1304,7 +1313,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;
@@ -1312,9 +1320,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->reset = sdhci_poweron_reset;
+
+    sdhci_common_class_init(klass, data);
 }
 
 static const TypeInfo sdhci_pci_info = {
@@ -1368,9 +1375,9 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &sdhci_vmstate;
     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 v7 05/14] sdhci: refactor common sysbus/pci realize() into sdhci_common_realize()
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 191f0d26ee..b464143959 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1221,6 +1221,15 @@ static void sdhci_uninitfn(SDHCIState *s)
     s->fifo_buffer = NULL;
 }
 
+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 bool sdhci_pending_insert_vmstate_needed(void *opaque)
 {
     SDHCIState *s = opaque;
@@ -1294,14 +1303,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);
 }
 
@@ -1363,11 +1374,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 v7 06/14] sdhci: refactor common sysbus/pci unrealize() into sdhci_common_unrealize()
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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, 22 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b464143959..ded4fa4885 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -32,6 +32,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 */
@@ -1230,6 +1231,17 @@ 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 bool sdhci_pending_insert_vmstate_needed(void *opaque)
 {
     SDHCIState *s = opaque;
@@ -1319,6 +1331,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);
 }
 
@@ -1383,11 +1397,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 v7 07/14] sdhci: use qemu_log_mask(UNIMP) instead of fprintf()
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 ded4fa4885..d0a91b60d5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -947,7 +947,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;
     }
 
@@ -1153,8 +1154,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 v7 08/14] sdhci: convert the DPRINT() calls into trace events
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 d0a91b60d5..7b075f6cb8 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -34,30 +34,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)
@@ -156,8 +133,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,
@@ -239,7 +216,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) {
@@ -247,7 +225,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];
@@ -257,11 +235,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;
@@ -295,7 +272,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) |
@@ -364,7 +341,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;
     }
 
@@ -373,8 +350,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] */
 
@@ -457,7 +433,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;
     }
 
@@ -466,8 +442,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) {
@@ -655,15 +630,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 */
@@ -746,8 +720,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;
@@ -755,8 +728,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;
             }
@@ -767,15 +739,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;
                 }
@@ -811,7 +783,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;
             }
 
@@ -819,7 +791,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;
             }
 
@@ -828,14 +800,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 {
@@ -870,8 +842,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;
@@ -901,8 +873,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;
@@ -954,7 +925,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;
 }
 
@@ -1158,8 +1129,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 v7 09/14] sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h"
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 7b075f6cb8..e8f4a3478a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -99,7 +99,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)
@@ -1026,7 +1025,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 v7 10/14] sdhci: rename the SDHC_CAPAB register
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 e8f4a3478a..3df33f5e93 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -898,7 +898,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 v7 11/14] sdhci: fix CAPAB/MAXCURR registers, both are 64bit and read-only
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 3df33f5e93..983569ba8b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -899,10 +899,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;
@@ -1123,6 +1129,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);
@@ -1163,10 +1178,10 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_capareg_property =
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT);
+    DEFINE_PROP_UINT64("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT);
 
 static Property sdhci_maxcurr_property =
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0);
+    DEFINE_PROP_UINT64("maxcurr", SDHCIState, maxcurr, 0);
 
 static void sdhci_initfn(SDHCIState *s)
 {
-- 
2.15.1


[Qemu-devel] [PATCH v7 12/14] sdhci: Implement write method of ACMD12ERRSTS register
Posted by Philippe Mathieu-Daudé, 23 weeks ago
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 983569ba8b..226a22c5fb 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1129,6 +1129,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 v7 13/14] sdhci: fix the PCI device, using the PCI address space for DMA
Posted by Philippe Mathieu-Daudé, 23 weeks ago
While SysBus devices can use the get_system_memory() address space,
PCI devices should use the bus master address space for DMA.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Should we check for the PCI_COMMAND_MASTER bit before
using pci_get_address_space()?

 include/hw/sd/sdhci.h |  1 +
 hw/sd/sdhci.c         | 33 +++++++++++++++++++--------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 442e30aff2..4a102b86ce 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -41,6 +41,7 @@ typedef struct SDHCIState {
     /*< public >*/
     SDBus sdbus;
     MemoryRegion iomem;
+    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 226a22c5fb..024b559f14 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -496,7 +496,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) {
@@ -518,7 +518,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) {
@@ -556,11 +556,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]);
         }
@@ -584,7 +582,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.
@@ -596,7 +594,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);
@@ -609,12 +607,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;
@@ -673,7 +671,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;
@@ -697,7 +695,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;
@@ -1314,7 +1312,8 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
     s->irq = pci_allocate_irq(dev);
-    pci_register_bar(dev, 0, 0, &s->iomem);
+    s->dma_as = pci_get_address_space(dev);
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->iomem);
 }
 
 static void sdhci_pci_exit(PCIDevice *dev)
@@ -1382,6 +1381,9 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
         return;
     }
 
+    s->dma_as = g_new0(AddressSpace, 1);
+    address_space_init(s->dma_as, get_system_memory(), "sdhci-dma");
+
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_mmio(sbd, &s->iomem);
 }
@@ -1391,6 +1393,9 @@ static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
     SDHCIState *s = SYSBUS_SDHCI(dev);
 
     sdhci_common_unrealize(s, &error_abort);
+
+    address_space_destroy(s->dma_as);
+    g_free(s->dma_as);
 }
 
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
-- 
2.15.1


Re: [Qemu-devel] [PATCH v7 13/14] sdhci: fix the PCI device, using the PCI address space for DMA
Posted by Peter Maydell, 22 weeks ago
On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> While SysBus devices can use the get_system_memory() address space,
> PCI devices should use the bus master address space for DMA.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Should we check for the PCI_COMMAND_MASTER bit before
> using pci_get_address_space()?

That isn't necessary -- the PCI core code handles the master
bit by mapping or unmapping a memory region from the container
which is what the pci_get_address_space() does its writes to.

> @@ -1382,6 +1381,9 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>          return;
>      }
>
> +    s->dma_as = g_new0(AddressSpace, 1);
> +    address_space_init(s->dma_as, get_system_memory(), "sdhci-dma");

You can just use address_space_memory rather than creating
a new address space here.

> +
>      sysbus_init_irq(sbd, &s->irq);
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
> @@ -1391,6 +1393,9 @@ static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
>      SDHCIState *s = SYSBUS_SDHCI(dev);
>
>      sdhci_common_unrealize(s, &error_abort);
> +
> +    address_space_destroy(s->dma_as);
> +    g_free(s->dma_as);
>  }
>
>  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
> --
> 2.15.1

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v7 14/14] sdhci: add a 'dma' property to the sysbus devices
Posted by Philippe Mathieu-Daudé, 23 weeks ago
Add a 'dma' property allowing machine creation to provide the address-space
SDHCI DMA operates on.

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

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 4a102b86ce..cb37182536 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -42,6 +42,7 @@ typedef struct SDHCIState {
     SDBus sdbus;
     MemoryRegion iomem;
     AddressSpace *dma_as;
+    MemoryRegion *dma_mr;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 024b559f14..cfb4320509 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1354,6 +1354,10 @@ 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_init(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
@@ -1363,11 +1367,19 @@ static void sdhci_sysbus_init(Object *obj)
     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 void sdhci_sysbus_finalize(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
+
+    if (s->dma_mr) {
+        object_unparent(OBJECT(s->dma_mr));
+    }
+
     sdhci_uninitfn(s);
 }
 
@@ -1382,7 +1394,10 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     }
 
     s->dma_as = g_new0(AddressSpace, 1);
-    address_space_init(s->dma_as, get_system_memory(), "sdhci-dma");
+    /* 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");
 
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_mmio(sbd, &s->iomem);
-- 
2.15.1