Re: [Qemu-devel] [PATCH] imx.31 and KZM board support

[Peter Maydell <peter.maydell@linaro.org>]

On 22 November 2011 11:06, Juan Quintela <quintela@redhat.com> wrote:
> Peter Chubb <peterc@gelato.unsw.edu.au> wrote:
>> Hi Peter,
>>    Please find appended a patch containing initial support for the
>>    FreeScale i.MX31 and the KZM Arm11 evaluation board.
>>
>>    The implementation was originally written by Hans Jang and Adam
>>    Clench of OK-Labs; I've updated it to the current qdev and memory
>>    region paradigms and implemented enough extra that Linux will boot
>>    on the patched QEMU using a ram disk.
>>
>>    The i.MX 31 Serial controller is found in most of the i.MX SoCs;
>>    the AVIC and timer implementations can also be shared, albeit with
>>    fewer chips.
>>
>> Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>
>> Signed-off-by: Hans Jang <hsjang@ok-labs.com>
>> Signed-off-by: Adam Clench <adamc@ok-labs.com>
>
>> +static const VMStateDescription vmstate_imxg_timer = {
>> +    .name = "imxg-timer",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_UINT32(cr, imxg_timer_state),
>> +        VMSTATE_UINT32(sr, imxg_timer_state),
>> +        VMSTATE_UINT32(ir, imxg_timer_state),
>> +        VMSTATE_UINT32(cnt, imxg_timer_state),
>> +        VMSTATE_UINT32(ocr1, imxg_timer_state),
>> +        VMSTATE_TIMER(timer, imxg_timer_state),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>
> This is not used.  Shouldn't a call like this be needed?
>
>> +static int imxg_timer_init(SysBusDevice *dev)
>> +{
>> +    imxg_timer_state *s = FROM_SYSBUS(imxg_timer_state, dev);
>> +
>> +    sysbus_init_irq(dev, &s->irq);
>> +    memory_region_init_io(&s->iomem, &imxg_timer_ops,
>> +                          s, "imxg-timer",
>> +                          0x00001000);
>> +    sysbus_init_mmio_region(dev, &s->iomem);
>> +
>> +    s->timer = qemu_new_timer_ns(vm_clock, imxg_timer_timeout, s);
>> +    s->cr = 0;
>> +    s->ir = 0;
>> +    s->pr = 0;
>> +    s->ocr1 = 0;
>> +    imxg_timer_update_count(s);
>
>       vmstate_register(&dev->qdev, -1, &vmstate_imxg_timer, s);
> ???

Any particular reason to prefer a vmstate_register() call
over just having a SysBusDeviceInfo struct with the .qdev.vmsd
field set? I tend to prefer the latter myself.

-- PMM