On Sat, Apr 20, 2013 at 11:34 AM, Blue Swirl <blauwir...@gmail.com> wrote: > On Sun, Apr 14, 2013 at 9:41 PM, Artyom Tarasenko <atar4q...@gmail.com> wrote: >> On Sun, Apr 14, 2013 at 10:05 AM, Hervé Poussineau <hpous...@reactos.org> >> wrote: >>> As m48t59 devices can only be created with m48t59_init() or >>> m48t59_init_isa(), >>> we know exactly which nvram types are required. Register only those three >>> types. >>> Remove .model and .size properties as they can be infered from nvram name. >>> Remove .io_base ISA address port as m48t59_init_isa() is always called with >>> ioport 0x74. >> >> While this it indeed how it's currently called, this is wrong for the >> sun4u emulation. >> The isa (ebus) variant of the sun4u m48t59_init_isa() should be called >> with a mem_base, not io_base. > > Why? I don't see much difference between EBUS and ISA and with the > memory API, the difference between PIO and MMIO is almost nonexistent > anyway.
Can you elaborate? Do you mean we just need to change the io_base? > But it should be possible to change the base to match real HW, whatever it is: > http://git.kernel.org/cgit/linux/kernel/git/davem/prtconfs.git/tree/ultra5#n273 Yes, I know where it is supposed to be, I'm just asking how to achieve this best with our current tooling. > So NACK for the original patch. > >> Do you think it should be implemented as another device type? >> >> >>> Signed-off-by: Hervé Poussineau <hpous...@reactos.org> >>> --- >>> hw/timer/m48t59.c | 187 >>> ++++++++++++++++++++++++++++++++++++----------------- >>> 1 file changed, 126 insertions(+), 61 deletions(-) >>> >>> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c >>> index 41022f2..29ec462 100644 >>> --- a/hw/timer/m48t59.c >>> +++ b/hw/timer/m48t59.c >>> @@ -43,6 +43,13 @@ >>> * PPC platform there is also a nvram lock function. >>> */ >>> >>> +typedef struct M48txxInfo { >>> + const char *isa_name; >>> + const char *sysbus_name; >>> + uint32_t model; /* 2 = m48t02, 8 = m48t08, 59 = m48t59 */ >>> + uint32_t size; >>> +} M48txxInfo; >>> + >>> /* >>> * Chipset docs: >>> * http://www.st.com/stonline/products/literature/ds/2410/m48t02.pdf >>> @@ -54,7 +61,6 @@ struct M48t59State { >>> /* Hardware parameters */ >>> qemu_irq IRQ; >>> MemoryRegion iomem; >>> - uint32_t io_base; >>> uint32_t size; >>> /* RTC management */ >>> time_t time_offset; >>> @@ -78,12 +84,39 @@ typedef struct M48t59ISAState { >>> MemoryRegion io; >>> } M48t59ISAState; >>> >>> +typedef struct M48txxISADeviceClass { >>> + ISADeviceClass parent_class; >>> + M48txxInfo info; >>> +} M48txxISADeviceClass; >>> + >>> typedef struct M48t59SysBusState { >>> SysBusDevice busdev; >>> M48t59State state; >>> MemoryRegion io; >>> } M48t59SysBusState; >>> >>> +typedef struct M48txxSysBusDeviceClass { >>> + SysBusDeviceClass parent_class; >>> + M48txxInfo info; >>> +} M48txxSysBusDeviceClass; >>> + >>> +static M48txxInfo m48txx_info[] = { >>> + { >>> + .sysbus_name = "m48t02", >>> + .model = 2, >>> + .size = 0x800, >>> + },{ >>> + .sysbus_name = "m48t08", >>> + .model = 8, >>> + .size = 0x2000, >>> + },{ >>> + .isa_name = "m48t59_isa", >>> + .model = 59, >>> + .size = 0x2000, >>> + } >>> +}; >>> + >>> + >>> /* Fake timer functions */ >>> >>> /* Alarm management */ >>> @@ -640,25 +673,34 @@ M48t59State *m48t59_init(qemu_irq IRQ, hwaddr >>> mem_base, >>> SysBusDevice *s; >>> M48t59SysBusState *d; >>> M48t59State *state; >>> + int i; >>> >>> - dev = qdev_create(NULL, "m48t59"); >>> - qdev_prop_set_uint32(dev, "model", model); >>> - qdev_prop_set_uint32(dev, "size", size); >>> - qdev_prop_set_uint32(dev, "io_base", io_base); >>> - qdev_init_nofail(dev); >>> - s = SYS_BUS_DEVICE(dev); >>> - d = FROM_SYSBUS(M48t59SysBusState, s); >>> - state = &d->state; >>> - sysbus_connect_irq(s, 0, IRQ); >>> - memory_region_init_io(&d->io, &m48t59_io_ops, state, "m48t59", 4); >>> - if (io_base != 0) { >>> - memory_region_add_subregion(get_system_io(), io_base, &d->io); >>> - } >>> - if (mem_base != 0) { >>> - sysbus_mmio_map(s, 0, mem_base); >>> + for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) { >>> + if (!m48txx_info[i].sysbus_name || >>> + m48txx_info[i].size != size || >>> + m48txx_info[i].model != model) { >>> + continue; >>> + } >>> + >>> + dev = qdev_create(NULL, m48txx_info[i].sysbus_name); >>> + qdev_init_nofail(dev); >>> + s = SYS_BUS_DEVICE(dev); >>> + d = FROM_SYSBUS(M48t59SysBusState, s); >>> + state = &d->state; >>> + sysbus_connect_irq(s, 0, IRQ); >>> + memory_region_init_io(&d->io, &m48t59_io_ops, state, "m48t59", 4); >>> + if (io_base != 0) { >>> + memory_region_add_subregion(get_system_io(), io_base, &d->io); >>> + } >>> + if (mem_base != 0) { >>> + sysbus_mmio_map(s, 0, mem_base); >>> + } >>> + >>> + return state; >>> } >>> >>> - return state; >>> + assert(false); >>> + return NULL; >>> } >>> >>> M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size, >>> @@ -667,16 +709,27 @@ M48t59State *m48t59_init_isa(ISABus *bus, uint32_t >>> io_base, uint16_t size, >>> M48t59ISAState *d; >>> ISADevice *dev; >>> M48t59State *s; >>> + int i; >>> + >>> + assert(io_base == 0x74); >>> + >>> + for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) { >>> + if (!m48txx_info[i].isa_name || >>> + m48txx_info[i].size != size || >>> + m48txx_info[i].model != model) { >>> + continue; >>> + } >>> >>> - dev = isa_create(bus, "m48t59_isa"); >>> - qdev_prop_set_uint32(&dev->qdev, "model", model); >>> - qdev_prop_set_uint32(&dev->qdev, "size", size); >>> - qdev_prop_set_uint32(&dev->qdev, "io_base", io_base); >>> - qdev_init_nofail(&dev->qdev); >>> - d = DO_UPCAST(M48t59ISAState, busdev, dev); >>> - s = &d->state; >>> + dev = isa_create(bus, m48txx_info[i].isa_name); >>> + qdev_init_nofail(&dev->qdev); >>> + d = DO_UPCAST(M48t59ISAState, busdev, dev); >>> + s = &d->state; >>> >>> - return s; >>> + return s; >>> + } >>> + >>> + assert(false); >>> + return NULL; >>> } >>> >>> static void m48t59_init_common(M48t59State *s) >>> @@ -693,24 +746,32 @@ static void m48t59_init_common(M48t59State *s) >>> >>> static int m48t59_init_isa1(ISADevice *dev) >>> { >>> + ISADeviceClass *ic = ISA_DEVICE_GET_CLASS(dev); >>> + M48txxISADeviceClass *u = container_of(ic, M48txxISADeviceClass, >>> + parent_class); >>> M48t59ISAState *d = DO_UPCAST(M48t59ISAState, busdev, dev); >>> M48t59State *s = &d->state; >>> >>> + s->model = u->info.model; >>> + s->size = u->info.size; >>> isa_init_irq(dev, &s->IRQ, 8); >>> m48t59_init_common(s); >>> memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); >>> - if (s->io_base != 0) { >>> - isa_register_ioport(dev, &d->io, s->io_base); >>> - } >>> + isa_register_ioport(dev, &d->io, 0x74); >>> >>> return 0; >>> } >>> >>> static int m48t59_init1(SysBusDevice *dev) >>> { >>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_GET_CLASS(dev); >>> + M48txxSysBusDeviceClass *u = container_of(k, M48txxSysBusDeviceClass, >>> + parent_class); >>> M48t59SysBusState *d = FROM_SYSBUS(M48t59SysBusState, dev); >>> M48t59State *s = &d->state; >>> >>> + s->model = u->info.model; >>> + s->size = u->info.size; >>> sysbus_init_irq(dev, &s->IRQ); >>> >>> memory_region_init_io(&s->iomem, &nvram_ops, s, "m48t59.nvram", >>> s->size); >>> @@ -720,58 +781,62 @@ static int m48t59_init1(SysBusDevice *dev) >>> return 0; >>> } >>> >>> -static Property m48t59_isa_properties[] = { >>> - DEFINE_PROP_UINT32("size", M48t59ISAState, state.size, -1), >>> - DEFINE_PROP_UINT32("model", M48t59ISAState, state.model, -1), >>> - DEFINE_PROP_HEX32( "io_base", M48t59ISAState, state.io_base, 0), >>> - DEFINE_PROP_END_OF_LIST(), >>> -}; >>> - >>> -static void m48t59_init_class_isa1(ObjectClass *klass, void *data) >>> +static void m48t59_isa_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); >>> + M48txxISADeviceClass *u = container_of(ic, M48txxISADeviceClass, >>> + parent_class); >>> + M48txxInfo *info = data; >>> + >>> ic->init = m48t59_init_isa1; >>> dc->no_user = 1; >>> dc->reset = m48t59_reset_isa; >>> - dc->props = m48t59_isa_properties; >>> + u->info = *info; >>> } >>> >>> -static const TypeInfo m48t59_isa_info = { >>> - .name = "m48t59_isa", >>> - .parent = TYPE_ISA_DEVICE, >>> - .instance_size = sizeof(M48t59ISAState), >>> - .class_init = m48t59_init_class_isa1, >>> -}; >>> - >>> -static Property m48t59_properties[] = { >>> - DEFINE_PROP_UINT32("size", M48t59SysBusState, state.size, -1), >>> - DEFINE_PROP_UINT32("model", M48t59SysBusState, state.model, -1), >>> - DEFINE_PROP_HEX32( "io_base", M48t59SysBusState, state.io_base, 0), >>> - DEFINE_PROP_END_OF_LIST(), >>> -}; >>> - >>> static void m48t59_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >>> + M48txxSysBusDeviceClass *u = container_of(k, M48txxSysBusDeviceClass, >>> + parent_class); >>> + M48txxInfo *info = data; >>> >>> k->init = m48t59_init1; >>> dc->reset = m48t59_reset_sysbus; >>> - dc->props = m48t59_properties; >>> + u->info = *info; >>> } >>> >>> -static const TypeInfo m48t59_info = { >>> - .name = "m48t59", >>> - .parent = TYPE_SYS_BUS_DEVICE, >>> - .instance_size = sizeof(M48t59SysBusState), >>> - .class_init = m48t59_class_init, >>> -}; >>> - >>> static void m48t59_register_types(void) >>> { >>> - type_register_static(&m48t59_info); >>> - type_register_static(&m48t59_isa_info); >>> + TypeInfo m48txx_type_info = { >>> + .parent = TYPE_SYS_BUS_DEVICE, >>> + .instance_size = sizeof(M48t59SysBusState), >>> + .class_size = sizeof(M48txxSysBusDeviceClass), >>> + .class_init = m48t59_class_init, >>> + }; >>> + TypeInfo m48txx_isa_type_info = { >>> + .parent = TYPE_ISA_DEVICE, >>> + .instance_size = sizeof(M48t59ISAState), >>> + .class_size = sizeof(M48txxISADeviceClass), >>> + .class_init = m48t59_isa_class_init, >>> + }; >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) { >>> + if (m48txx_info[i].sysbus_name) { >>> + m48txx_type_info.name = m48txx_info[i].sysbus_name; >>> + m48txx_type_info.class_data = &m48txx_info[i]; >>> + type_register(&m48txx_type_info); >>> + } >>> + >>> + if (m48txx_info[i].isa_name) { >>> + m48txx_isa_type_info.name = m48txx_info[i].isa_name; >>> + m48txx_isa_type_info.class_data = &m48txx_info[i]; >>> + type_register(&m48txx_isa_type_info); >>> + } >>> + } >>> } >>> >>> type_init(m48t59_register_types) >>> -- >>> 1.7.10.4 >>> >>> >> >> >> >> -- >> Regards, >> Artyom Tarasenko >> >> linux/sparc and solaris/sparc under qemu blog: >> http://tyom.blogspot.com/search/label/qemu -- Regards, Artyom Tarasenko linux/sparc and solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu
