qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC v2 3/7] m48t59: register a QOM type for each nvram


From: Blue Swirl
Subject: Re: [Qemu-devel] [RFC v2 3/7] m48t59: register a QOM type for each nvram type we support
Date: Sat, 20 Apr 2013 10:39:52 +0000

On Sat, Apr 20, 2013 at 9:56 AM, Artyom Tarasenko <address@hidden> wrote:
> On Sat, Apr 20, 2013 at 11:34 AM, Blue Swirl <address@hidden> wrote:
>> On Sun, Apr 14, 2013 at 9:41 PM, Artyom Tarasenko <address@hidden> wrote:
>>> On Sun, Apr 14, 2013 at 10:05 AM, Hervé Poussineau <address@hidden> 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 <address@hidden>
>>>> ---
>>>>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]