Re: [Qemu-devel] [PATCH v3 2/2] ahci: add support for non-PCI based controllers

[Mark Langsdorf <mark.langsdorf@calxeda.com>]

On 01/05/2012 08:26 AM, Alexander Graf wrote:
> 
> On 05.01.2012, at 15:16, Andreas Färber wrote:
> 
>> Am 05.01.2012 14:52, schrieb Mark Langsdorf:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> Add support for ahci on sysbus.
>>>
>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
>>> ---
>>> Changes from v1, v2
>>> 	Corrected indentation of PlatAHCIState members
>>> 	Made plat_ahci_info into a single structure, not a list
>>>
>>> hw/ide/ahci.c |   31 +++++++++++++++++++++++++++++++
>>> 1 files changed, 31 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index 135d0ee..f052e55 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -25,6 +25,7 @@
>>> #include <hw/msi.h>
>>> #include <hw/pc.h>
>>> #include <hw/pci.h>
>>> +#include <hw/sysbus.h>
>>>
>>> #include "monitor.h"
>>> #include "dma.h"
>>> @@ -1214,3 +1215,33 @@ void ahci_reset(void *opaque)
>>>         ahci_reset_port(s, i);
>>>     }
>>> }
>>> +
>>> +typedef struct PlatAHCIState {
>>> +    SysBusDevice busdev;
>>> +    AHCIState ahci;
>>> +} PlatAHCIState;
>>> +
>>> +static int plat_ahci_init(SysBusDevice *dev)
>>> +{
>>> +    PlatAHCIState *s = FROM_SYSBUS(PlatAHCIState, dev);
>>> +    ahci_init(&s->ahci, &dev->qdev, 1);
>>> +
>>> +    sysbus_init_mmio(dev, &s->ahci.mem);
>>> +    sysbus_init_irq(dev, &s->ahci.irq);
> 
> It's still unclear to me how you connect an irq line on the command line. How do you instantiate this device?

I'm not sure how it's done on the command line. In the SoC model that
this is intended for, I call sysbus_create_simple().

>>> +    qemu_register_reset(ahci_reset, &s->ahci);
>>> +    return 0;
>>> +}
>>> +
>>> +static SysBusDeviceInfo plat_ahci_info = {
>>> +    .qdev.name    = "plat-ahci",
>>
>> The commit message does not given an indication where "plat" comes from
>> - is that an ARM device name?
> 
> "plat" here means "platform device". I'm not sure I like the naming though. Basically it's a sysbus version of AHCI, similar to how virtio-mmio is a sysbus version of virtio.
> 
> How about "sysbus-ahci"?

Sure. I'll make that change.

>> Does this patch actually make something work? If yes, please state so,
>> including usage instructions. If not, then I would suggest to hold this
>> one back and to send it together with any follow-on patches that wire it
>> up on some machine.
> 
> You can always just create the device manually with -device and hook it up in the guest device tree or machine description manually.
> 
> However the question still stands: Have you verified this code works?

It's used in the Highbank SoC model, which hasn't been released yet. I'm
waiting on some other patches to make it upstream, mostly Trustzone
support now.

I can resubmit it with the Highbank SoC model when that goes out if
you would prefer.

--Mark Langsdorf
Calxeda, Inc.