| Message ID | 1372626065-6043-8-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Hi Andreas, On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote: > From: Andreas Färber <andreas.faerber@web.de> > > Split the SysBusDevice initfn into instance_init and realizefn. > > Signed-off-by: Andreas Färber <andreas.faerber@web.de> > --- > hw/timer/arm_mptimer.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c > index 588e34b..a19ffa3 100644 > --- a/hw/timer/arm_mptimer.c > +++ b/hw/timer/arm_mptimer.c > @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev) > } > } > > -static int arm_mptimer_init(SysBusDevice *dev) > +static void arm_mptimer_init(Object *obj) > { > + ARMMPTimerState *s = ARM_MP_TIMER(obj); > + > + memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, > + "arm_mptimer_timer", 0x20); > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem); > +} Splitting off only one of the memory region setups to init seems a bit awkward. Is it really worth it given that we need to wait for realization for the rest to occur anyway? Regards, Peter > + > +static void arm_mptimer_realize(DeviceState *dev, Error **errp) > +{ > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > ARMMPTimerState *s = ARM_MP_TIMER(dev); > int i; > > @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev) > * * timer for core 1 > * and so on. > */ > - memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, > - "arm_mptimer_timer", 0x20); > - sysbus_init_mmio(dev, &s->iomem); > for (i = 0; i < s->num_cpu; i++) { > TimerBlock *tb = &s->timerblock[i]; > tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb); > - sysbus_init_irq(dev, &tb->irq); > + sysbus_init_irq(sbd, &tb->irq); > memory_region_init_io(&tb->iomem, &timerblock_ops, tb, > "arm_mptimer_timerblock", 0x20); > - sysbus_init_mmio(dev, &tb->iomem); > + sysbus_init_mmio(sbd, &tb->iomem); > } > - > - return 0; > } > > static const VMStateDescription vmstate_timerblock = { > @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = { > static void arm_mptimer_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass); > > - sbc->init = arm_mptimer_init; > + dc->realize = arm_mptimer_realize; > dc->vmsd = &vmstate_arm_mptimer; > dc->reset = arm_mptimer_reset; > dc->no_user = 1; > @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = { > .name = TYPE_ARM_MP_TIMER, > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(ARMMPTimerState), > + .instance_init = arm_mptimer_init, > .class_init = arm_mptimer_class_init, > }; > > -- > 1.8.1.4 > >
Am 01.07.2013 11:33, schrieb Peter Crosthwaite: > Hi Andreas, > > On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote: >> From: Andreas Färber <andreas.faerber@web.de> >> >> Split the SysBusDevice initfn into instance_init and realizefn. >> >> Signed-off-by: Andreas Färber <andreas.faerber@web.de> >> --- >> hw/timer/arm_mptimer.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c >> index 588e34b..a19ffa3 100644 >> --- a/hw/timer/arm_mptimer.c >> +++ b/hw/timer/arm_mptimer.c >> @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev) >> } >> } >> >> -static int arm_mptimer_init(SysBusDevice *dev) >> +static void arm_mptimer_init(Object *obj) >> { >> + ARMMPTimerState *s = ARM_MP_TIMER(obj); >> + >> + memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, >> + "arm_mptimer_timer", 0x20); >> + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem); >> +} > > Splitting off only one of the memory region setups to init seems a bit > awkward. Is it really worth it given that we need to wait for > realization for the rest to occur anyway? Well, my main interest wrt MemoryRegions here is to have the *mpcore_priv container MemoryRegion mappable before realize and to avoid having to implement unnecessary cleanups in unrealize. An alternative would be for PMM to use his array properties to dynamically allocate the MemoryRegions before realize, but so far he has failed to come up with a solution... Another solution, since this is a fixed-length array, would be to always initialize all MemoryRegions and just keep adding all 3(?) in realize. FWIU adding subregions is desired in instance_init as long as it affects only containing devices and not a global address space. Regards, Andreas >> + >> +static void arm_mptimer_realize(DeviceState *dev, Error **errp) >> +{ >> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> ARMMPTimerState *s = ARM_MP_TIMER(dev); >> int i; >> >> @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev) >> * * timer for core 1 >> * and so on. >> */ >> - memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, >> - "arm_mptimer_timer", 0x20); >> - sysbus_init_mmio(dev, &s->iomem); >> for (i = 0; i < s->num_cpu; i++) { >> TimerBlock *tb = &s->timerblock[i]; >> tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb); >> - sysbus_init_irq(dev, &tb->irq); >> + sysbus_init_irq(sbd, &tb->irq); >> memory_region_init_io(&tb->iomem, &timerblock_ops, tb, >> "arm_mptimer_timerblock", 0x20); >> - sysbus_init_mmio(dev, &tb->iomem); >> + sysbus_init_mmio(sbd, &tb->iomem); >> } >> - >> - return 0; >> } >> >> static const VMStateDescription vmstate_timerblock = { >> @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = { >> static void arm_mptimer_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass); >> >> - sbc->init = arm_mptimer_init; >> + dc->realize = arm_mptimer_realize; >> dc->vmsd = &vmstate_arm_mptimer; >> dc->reset = arm_mptimer_reset; >> dc->no_user = 1; >> @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = { >> .name = TYPE_ARM_MP_TIMER, >> .parent = TYPE_SYS_BUS_DEVICE, >> .instance_size = sizeof(ARMMPTimerState), >> + .instance_init = arm_mptimer_init, >> .class_init = arm_mptimer_class_init, >> }; >> >> -- >> 1.8.1.4 >> >>
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c index 588e34b..a19ffa3 100644 --- a/hw/timer/arm_mptimer.c +++ b/hw/timer/arm_mptimer.c @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev) } } -static int arm_mptimer_init(SysBusDevice *dev) +static void arm_mptimer_init(Object *obj) { + ARMMPTimerState *s = ARM_MP_TIMER(obj); + + memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, + "arm_mptimer_timer", 0x20); + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem); +} + +static void arm_mptimer_realize(DeviceState *dev, Error **errp) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); ARMMPTimerState *s = ARM_MP_TIMER(dev); int i; @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev) * * timer for core 1 * and so on. */ - memory_region_init_io(&s->iomem, &arm_thistimer_ops, s, - "arm_mptimer_timer", 0x20); - sysbus_init_mmio(dev, &s->iomem); for (i = 0; i < s->num_cpu; i++) { TimerBlock *tb = &s->timerblock[i]; tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb); - sysbus_init_irq(dev, &tb->irq); + sysbus_init_irq(sbd, &tb->irq); memory_region_init_io(&tb->iomem, &timerblock_ops, tb, "arm_mptimer_timerblock", 0x20); - sysbus_init_mmio(dev, &tb->iomem); + sysbus_init_mmio(sbd, &tb->iomem); } - - return 0; } static const VMStateDescription vmstate_timerblock = { @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = { static void arm_mptimer_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); - SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass); - sbc->init = arm_mptimer_init; + dc->realize = arm_mptimer_realize; dc->vmsd = &vmstate_arm_mptimer; dc->reset = arm_mptimer_reset; dc->no_user = 1; @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = { .name = TYPE_ARM_MP_TIMER, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(ARMMPTimerState), + .instance_init = arm_mptimer_init, .class_init = arm_mptimer_class_init, };