| Message ID | 1325771559-15570-2-git-send-email-mark.langsdorf@calxeda.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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.
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
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
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.
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
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); +