Re: [Qemu-devel] [PATCH v7 00/14] SDHCI: housekeeping (part 1)

[Philippe Mathieu-Daudé]

On Mon, Jan 15, 2018 at 11:47 AM, Peter Maydell
<address@hidden> wrote:
> On 15 January 2018 at 14:04, Philippe Mathieu-Daudé <address@hidden> wrote:
>> On 01/15/2018 10:51 AM, Peter Maydell wrote:
>>> On 13 January 2018 at 05:07, Philippe Mathieu-Daudé <address@hidden> 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.