Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite]

On Fri, Dec 4, 2015 at 8:42 AM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 04/12/2015 12:00, Peter Maydell wrote:
>> On 4 December 2015 at 07:30, Markus Armbruster <address@hidden> wrote:
>>> Peter Maydell <address@hidden> writes:
>>>
>>>> On 7 September 2015 at 17:57, Markus Armbruster <address@hidden> wrote:
>>>>> Peter Maydell <address@hidden> writes:
>>>>>
>>>>>> On 7 September 2015 at 17:40, Markus Armbruster <address@hidden> wrote:
>>>>>>> Peter Maydell <address@hidden> writes:
>>>>>>>
>>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>>>
>>>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>>>> +     * setting a 'drive' property results in a call to 
>>>>>>>> blk_attach_dev()
>>>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>>>> +     */
>>>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>>>> +};
>>>>>>>
>>>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>>>> pre-qdev world: it creates and completely initializes the common
>>>>>>> SDState.
>>>>>>>
>>>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>>>
>>>>>> Yes, but I don't really want to gate QOMification of mmc
>>>>>> controller devices on the more complicated problem of
>>>>>> QOMifying sd.c itself, especially since we already have several
>>>>>> QOMified mmc controllers...
>>>>>
>>>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>>>> QOM-friendly way: typedef SerialState, realize helper
>>>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>>>> SerialState had more properties, we'd also need a macro to define them.
>>>>
>>>> It looks like since we had this conversation the problem has been
>>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>>>> now I think...
>>>
>>> Ignoring the error is intentional according to the comment, but why is
>>> it appropriate?
>>
>> That seems like a question to ask the author and reviewer of that
>> commit :-) [cc'd].
>>
>> The intention seems to have been to allow sdhci to do the same thing
>> I want -- take a drive property (which attaches the BlockBackend to
>> the controller device) and then hand the BlockBackend to sd_init()
>> without having it blow up.
>>
>> Incidentally, in an ideal world wouldn't the block/drive properties
>> be on the SD card object rather than the controller object ? At least,
>> we seem to have that split for IDE and SCSI disks.
>
> [copying from the other thread]
>
> FWIW, I don't think the SD card will be qdevified because it doesn't
> need a bus.  It's similar indeed to SerialState, which was supposed to
> be the poster child of QOM embedding and never got QOMified.
>

SD is a bus in its own right and should be busified and QOMified IMO.
SDHCI can talk to non-sd cards (SDIO). There is also a range of
incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
anything that couples the controller to an SD card is a bug, the card
and device should be arranged as separate devices.

> A host controller controls exactly one SD card, the SSI bridge is also
> for exactly one SD card, etc.

I think you can RYO chip selects with a GPIO and control multiple SD
cards with one SDHCI.

Regards,
Peter

> So there's not much to gain from
> qdevification of the card itself.  There would be a gain from
> qdevification of the OMAP and StrongARM controllers, which is exactly
> what this series does.
>
> Paolo
>