On Sat, Apr 20, 2013 at 9:56 AM, Artyom Tarasenko <atar4q...@gmail.com> wrote: > 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?
Why wouldn't that work? >> 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