| Message ID | 20181001220942.2382-6-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | another SysBusDevice::init to Device::realize cleanup | expand |
On 1 October 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Comment says DeviceState::realize but the code is using
PCIDevice::realize ?
I didn't realize pci devices had their own realize method:
what's the difference between it and plain old DeviceState::realize,
which they also have, since PCIDeviceClass inherits from DeviceClass ?
thanks
-- PMM
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote: > Move from the legacy SysBusDevice::init method to using DeviceState::realize. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> one question below, Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/sh4/sh_pci.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c > index 4ec2e35500..84c52df067 100644 > --- a/hw/sh4/sh_pci.c > +++ b/hw/sh4/sh_pci.c > @@ -120,16 +120,15 @@ static void sh_pci_set_irq(void *opaque, int irq_num, int level) > qemu_set_irq(pic[irq_num], level); > } > > -static int sh_pci_device_init(SysBusDevice *dev) > +static void sh_pci_device_realize(PCIDevice *dev, Error **errp) > { > - PCIHostState *phb; > - SHPCIState *s; > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + SHPCIState *s = SH_PCI_HOST_BRIDGE(dev); > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > int i; > > - s = SH_PCI_HOST_BRIDGE(dev); > - phb = PCI_HOST_BRIDGE(s); > for (i = 0; i < 4; i++) { is that PCI_NUM_PINS ? > - sysbus_init_irq(dev, &s->irq[i]); > + sysbus_init_irq(sbd, &s->irq[i]); > } > phb->bus = pci_register_root_bus(DEVICE(dev), "pci", > sh_pci_set_irq, sh_pci_map_irq, > @@ -143,13 +142,12 @@ static int sh_pci_device_init(SysBusDevice *dev) > &s->memconfig_p4, 0, 0x224); > memory_region_init_alias(&s->isa, OBJECT(s), "sh_pci.isa", > get_system_io(), 0, 0x40000); > - sysbus_init_mmio(dev, &s->memconfig_p4); > - sysbus_init_mmio(dev, &s->memconfig_a7); > + sysbus_init_mmio(sbd, &s->memconfig_p4); > + sysbus_init_mmio(sbd, &s->memconfig_a7); > s->iobr = 0xfe240000; > memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa); > > s->dev = pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "sh_pci_host"); > - return 0; > } > > static void sh_pci_host_realize(PCIDevice *d, Error **errp) > @@ -187,9 +185,9 @@ static const TypeInfo sh_pci_host_info = { > > static void sh_pci_device_class_init(ObjectClass *klass, void *data) > { > - SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass); > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > > - sdc->init = sh_pci_device_init; > + k->realize = sh_pci_device_realize; > } > > static const TypeInfo sh_pci_device_info = { >
On 10/2/18 3:13 PM, Peter Maydell wrote: > On 1 October 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Move from the legacy SysBusDevice::init method to using DeviceState::realize. > > Comment says DeviceState::realize but the code is using > PCIDevice::realize ? > > I didn't realize pci devices had their own realize method: > what's the difference between it and plain old DeviceState::realize, > which they also have, since PCIDeviceClass inherits from DeviceClass ? Cc'ing Michael and Marcel.
Hi Philippe, Peter On 10/2/18 10:59 PM, Philippe Mathieu-Daudé wrote: > On 10/2/18 3:13 PM, Peter Maydell wrote: >> On 1 October 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> Move from the legacy SysBusDevice::init method to using DeviceState::realize. >> Comment says DeviceState::realize but the code is using >> PCIDevice::realize ? >> >> I didn't realize pci devices had their own realize method: >> what's the difference between it and plain old DeviceState::realize, PCIDevice:realize handles PCI specific "realization" tasks: * setup of the PCI configuration space * PCI BARs configuration * MSI initalization *... The PCIDevice's DeviceState::realize function named 'pci_qdev_realize' calls PCIDevice:realize after it runs some generic initialization code. It is possible we have this design so we would be able to move from "qdev" to QOM. Thanks, Marcel >> which they also have, since PCIDeviceClass inherits from DeviceClass ? > Cc'ing Michael and Marcel.
On 10/2/18 10:37 PM, Marcel Apfelbaum wrote: > Hi Philippe, Peter > > On 10/2/18 10:59 PM, Philippe Mathieu-Daudé wrote: >> On 10/2/18 3:13 PM, Peter Maydell wrote: >>> On 1 October 2018 at 23:09, Philippe Mathieu-Daudé <f4bug@amsat.org> >>> wrote: >>>> Move from the legacy SysBusDevice::init method to using >>>> DeviceState::realize. >>> Comment says DeviceState::realize but the code is using >>> PCIDevice::realize ? >>> >>> I didn't realize pci devices had their own realize method: >>> what's the difference between it and plain old DeviceState::realize, > > PCIDevice:realize handles PCI specific "realization" tasks: > * setup of the PCI configuration space > * PCI BARs configuration > * MSI initalization > *... > > The PCIDevice's DeviceState::realize function named 'pci_qdev_realize' > calls PCIDevice:realize after it runs some generic initialization code. > > It is possible we have this design so we would be able to move from > "qdev" to QOM. Thanks Marcel, so this patch is incorrect. TYPE_SH_PCI_HOST_BRIDGE inherits TYPE_PCI_HOST_BRIDGE which inherits TYPE_SYS_BUS_DEVICE which inherits TYPE_DEVICE, so can implement DeviceState::realize but not PCIDevice::realize. TYPE_"sh_pci_host" inherits TYPE_PCI_DEVICE, so can implement PCIDevice::realize. I'll respin. > Thanks, > Marcel > >>> which they also have, since PCIDeviceClass inherits from DeviceClass ? >> Cc'ing Michael and Marcel.
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c index 4ec2e35500..84c52df067 100644 --- a/hw/sh4/sh_pci.c +++ b/hw/sh4/sh_pci.c @@ -120,16 +120,15 @@ static void sh_pci_set_irq(void *opaque, int irq_num, int level) qemu_set_irq(pic[irq_num], level); } -static int sh_pci_device_init(SysBusDevice *dev) +static void sh_pci_device_realize(PCIDevice *dev, Error **errp) { - PCIHostState *phb; - SHPCIState *s; + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + SHPCIState *s = SH_PCI_HOST_BRIDGE(dev); + PCIHostState *phb = PCI_HOST_BRIDGE(s); int i; - s = SH_PCI_HOST_BRIDGE(dev); - phb = PCI_HOST_BRIDGE(s); for (i = 0; i < 4; i++) { - sysbus_init_irq(dev, &s->irq[i]); + sysbus_init_irq(sbd, &s->irq[i]); } phb->bus = pci_register_root_bus(DEVICE(dev), "pci", sh_pci_set_irq, sh_pci_map_irq, @@ -143,13 +142,12 @@ static int sh_pci_device_init(SysBusDevice *dev) &s->memconfig_p4, 0, 0x224); memory_region_init_alias(&s->isa, OBJECT(s), "sh_pci.isa", get_system_io(), 0, 0x40000); - sysbus_init_mmio(dev, &s->memconfig_p4); - sysbus_init_mmio(dev, &s->memconfig_a7); + sysbus_init_mmio(sbd, &s->memconfig_p4); + sysbus_init_mmio(sbd, &s->memconfig_a7); s->iobr = 0xfe240000; memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa); s->dev = pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "sh_pci_host"); - return 0; } static void sh_pci_host_realize(PCIDevice *d, Error **errp) @@ -187,9 +185,9 @@ static const TypeInfo sh_pci_host_info = { static void sh_pci_device_class_init(ObjectClass *klass, void *data) { - SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass); + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - sdc->init = sh_pci_device_init; + k->realize = sh_pci_device_realize; } static const TypeInfo sh_pci_device_info = {
Move from the legacy SysBusDevice::init method to using DeviceState::realize. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/sh4/sh_pci.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)