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 09:34:01 +0000

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.

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

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



reply via email to

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