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

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

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.

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

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.

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

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.