[Qemu-devel] [PATCH v3 1/2] ahci: convert ahci_reset to use AHCIState

[Qemu-devel] [PATCH v3 1/2] ahci: convert ahci_reset to use AHCIState

[Mark Langsdorf]

From: Rob Herring <rob.herring@calxeda.com>

Use AHCIState instead of AHCIPCIState so the function can be used for
non-PCI based AHCI controllers.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v1, v2
	None

 hw/ide/ahci.c |   14 +++++++-------
 hw/ide/ich.c  |    4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0af201d..135d0ee 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -336,7 +336,7 @@ static void ahci_mem_write(void *opaque, target_phys_addr_t addr,
             case HOST_CTL: /* R/W */
                 if (val & HOST_CTL_RESET) {
                     DPRINTF(-1, "HBA Reset\n");
-                    ahci_reset(container_of(s, AHCIPCIState, ahci));
+                    ahci_reset(s);
                 } else {
                     s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
                     ahci_check_irq(s);
@@ -1199,18 +1199,18 @@ void ahci_uninit(AHCIState *s)
 
 void ahci_reset(void *opaque)
 {
-    struct AHCIPCIState *d = opaque;
+    struct AHCIState *s = opaque;
     AHCIPortRegs *pr;
     int i;
 
-    d->ahci.control_regs.irqstatus = 0;
-    d->ahci.control_regs.ghc = 0;
+    s->control_regs.irqstatus = 0;
+    s->control_regs.ghc = 0;
 
-    for (i = 0; i < d->ahci.ports; i++) {
-        pr = &d->ahci.dev[i].port_regs;
+    for (i = 0; i < s->ports; i++) {
+        pr = &s->dev[i].port_regs;
         pr->irq_stat = 0;
         pr->irq_mask = 0;
         pr->scr_ctl = 0;
-        ahci_reset_port(&d->ahci, i);
+        ahci_reset_port(s, i);
     }
 }
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 3f7510f..44363ec 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -102,7 +102,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     /* XXX Software should program this register */
     d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
 
-    qemu_register_reset(ahci_reset, d);
+    qemu_register_reset(ahci_reset, &d->ahci);
 
     msi_init(dev, 0x50, 1, true, false);
     d->ahci.irq = d->card.irq[0];
@@ -133,7 +133,7 @@ static int pci_ich9_uninit(PCIDevice *dev)
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
 
     msi_uninit(dev);
-    qemu_unregister_reset(ahci_reset, d);
+    qemu_unregister_reset(ahci_reset, &d->ahci);
     ahci_uninit(&d->ahci);
 
     return 0;
-- 
1.7.5.4

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

[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);
+
+    qemu_register_reset(ahci_reset, &s->ahci);
+    return 0;
+}
+
+static SysBusDeviceInfo plat_ahci_info = {
+    .qdev.name    = "plat-ahci",
+    .qdev.size    = sizeof(PlatAHCIState),
+    .init         = plat_ahci_init,
+};
+
+static void plat_ahci_register(void)
+{
+    sysbus_register_withprop(&plat_ahci_info);
+}
+device_init(plat_ahci_register);
+
-- 
1.7.5.4

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

[Andreas Färber]

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);
> +
> +    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?

> +    .qdev.size    = sizeof(PlatAHCIState),
> +    .init         = plat_ahci_init,
> +};
> +
> +static void plat_ahci_register(void)
> +{
> +    sysbus_register_withprop(&plat_ahci_info);
> +}
> +device_init(plat_ahci_register);
> +

Please move the empty line to above the device_init().

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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Alexander Graf]

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?

>> +
>> +    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"?

> 
>> +    .qdev.size    = sizeof(PlatAHCIState),
>> +    .init         = plat_ahci_init,
>> +};
>> +
>> +static void plat_ahci_register(void)
>> +{
>> +    sysbus_register_withprop(&plat_ahci_info);
>> +}
>> +device_init(plat_ahci_register);
>> +
> 
> Please move the empty line to above the device_init().
> 
> 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?


Alex

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

[Andreas Färber]

Am 05.01.2012 15:26, schrieb Alexander Graf:
> 
> 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 thought, with qdev that's not possible at all.

>>> +
>>> +    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"?

Yeah, sounds better to me.

> However the question still stands: Have you verified this code works?

I believe that one's addressed to Mark. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Mark Langsdorf]

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.

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

[Rob Herring]

On 01/05/2012 08:35 AM, Mark Langsdorf wrote:
> 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.
> 
Which has been extensively tested for some time with the Linux AHCI
platform driver. The only issue we've seen are guest "DMA timeouts" when
the disk image file is on an NFS share, but that should not be caused by
this change.

Mark, there is not a qemu code dependency on Trustzone support, so we
don't need to wait for that to add highbank support.

Rob

> I can resubmit it with the Highbank SoC model when that goes out if
> you would prefer.
> 
> --Mark Langsdorf
> Calxeda, Inc.

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

[Alexander Graf]

On 05.01.2012, at 16:11, Rob Herring wrote:

> On 01/05/2012 08:35 AM, Mark Langsdorf wrote:
>> 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.
>> 
> Which has been extensively tested for some time with the Linux AHCI
> platform driver. The only issue we've seen are guest "DMA timeouts" when
> the disk image file is on an NFS share, but that should not be caused by
> this change.
> 
> Mark, there is not a qemu code dependency on Trustzone support, so we
> don't need to wait for that to add highbank support.

I agree, the SoC model should still work without trustzone. Just post the whole thing including this patch, so we make sure we don't have dead code in the tree. If the other patches you're waiting on look like they're basically accepted but need a bit to actually work their way through to upstream, just post the SoC patches with a comment saying that they are based on the others.


Alex

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

[Peter Maydell]

On 5 January 2012 15:11, Rob Herring <rob.herring@calxeda.com> wrote:
> Mark, there is not a qemu code dependency on Trustzone support, so we
> don't need to wait for that to add highbank support.

That's good, because Trustzone support is not going to land imminently
I suspect, and it would be better not to have it as a dependency.
(I'll consider minimal workarounds for lack-of-trustzone if we have to
to get things booting.)

-- PMM

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

[Mark Langsdorf]

On 01/05/2012 09:32 AM, Peter Maydell wrote:
> On 5 January 2012 15:11, Rob Herring <rob.herring@calxeda.com> wrote:
>> Mark, there is not a qemu code dependency on Trustzone support, so we
>> don't need to wait for that to add highbank support.
> 
> That's good, because Trustzone support is not going to land imminently
> I suspect, and it would be better not to have it as a dependency.
> (I'll consider minimal workarounds for lack-of-trustzone if we have to
> to get things booting.)

I'll work with Rob today to resolve this. My experience is that
the Highbank model requires c1_scr, which Peter says is part of
Trustzone. I may have missed something, though.

--Mark Langsdorf
Calxeda, Inc.

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

[Rob Herring]

On 01/05/2012 09:32 AM, Peter Maydell wrote:
> On 5 January 2012 15:11, Rob Herring <rob.herring@calxeda.com> wrote:
>> Mark, there is not a qemu code dependency on Trustzone support, so we
>> don't need to wait for that to add highbank support.
> 
> That's good, because Trustzone support is not going to land imminently
> I suspect, and it would be better not to have it as a dependency.
> (I'll consider minimal workarounds for lack-of-trustzone if we have to
> to get things booting.)

We don't need the full Trustzone secure and non-secure states to work.
We just need writes to the various secure config registers to be ignored
and not error out.

Rob

Re: [Qemu-devel] [PATCH v3 1/2] ahci: convert ahci_reset to use AHCIState

[Andreas Färber]

Am 05.01.2012 14:52, schrieb Mark Langsdorf:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Use AHCIState instead of AHCIPCIState so the function can be used for
> non-PCI based AHCI controllers.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

> ---
> Changes from v1, v2
> 	None
> 
>  hw/ide/ahci.c |   14 +++++++-------
>  hw/ide/ich.c  |    4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 0af201d..135d0ee 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -336,7 +336,7 @@ static void ahci_mem_write(void *opaque, target_phys_addr_t addr,
>              case HOST_CTL: /* R/W */
>                  if (val & HOST_CTL_RESET) {
>                      DPRINTF(-1, "HBA Reset\n");
> -                    ahci_reset(container_of(s, AHCIPCIState, ahci));
> +                    ahci_reset(s);
>                  } else {
>                      s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
>                      ahci_check_irq(s);
> @@ -1199,18 +1199,18 @@ void ahci_uninit(AHCIState *s)
>  
>  void ahci_reset(void *opaque)
>  {
> -    struct AHCIPCIState *d = opaque;
> +    struct AHCIState *s = opaque;
>      AHCIPortRegs *pr;
>      int i;
>  
> -    d->ahci.control_regs.irqstatus = 0;
> -    d->ahci.control_regs.ghc = 0;
> +    s->control_regs.irqstatus = 0;
> +    s->control_regs.ghc = 0;
>  
> -    for (i = 0; i < d->ahci.ports; i++) {
> -        pr = &d->ahci.dev[i].port_regs;
> +    for (i = 0; i < s->ports; i++) {
> +        pr = &s->dev[i].port_regs;
>          pr->irq_stat = 0;
>          pr->irq_mask = 0;
>          pr->scr_ctl = 0;
> -        ahci_reset_port(&d->ahci, i);
> +        ahci_reset_port(s, i);
>      }
>  }
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 3f7510f..44363ec 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -102,7 +102,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>      /* XXX Software should program this register */
>      d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
>  
> -    qemu_register_reset(ahci_reset, d);
> +    qemu_register_reset(ahci_reset, &d->ahci);
>  
>      msi_init(dev, 0x50, 1, true, false);
>      d->ahci.irq = d->card.irq[0];
> @@ -133,7 +133,7 @@ static int pci_ich9_uninit(PCIDevice *dev)
>      d = DO_UPCAST(struct AHCIPCIState, card, dev);
>  
>      msi_uninit(dev);
> -    qemu_unregister_reset(ahci_reset, d);
> +    qemu_unregister_reset(ahci_reset, &d->ahci);
>      ahci_uninit(&d->ahci);
>  
>      return 0;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 1/2] ahci: convert ahci_reset to use AHCIState

[Alexander Graf]

On 05.01.2012, at 14:52, Mark Langsdorf wrote:

> From: Rob Herring <rob.herring@calxeda.com>
> 
> Use AHCIState instead of AHCIPCIState so the function can be used for
> non-PCI based AHCI controllers.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>

Acked-by: Alexander Graf <agraf@suse.de>


Alex