Re: [Qemu-devel] [PATCH v4 1/2] pl330: initial version

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

Hi Peter, sorry for not properly reviewing your patch for such a long 
time, I'll try to do this as soon as possible. Right now I have a few 
small coments


On 06/18/2012 04:42 AM, Peter A. G. Crosthwaite wrote:
> Device model for Primecell PL330 dma controller.
>
> Signed-off-by: Peter A. G. Crosthwaite<peter.crosthwaite@petalogix.com>
> Signed-off-by: Kirill Batuzov<batuzovk@ispras.ru>
> ---
> [..snip..]
> +static void pl330_dmago(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
> +{
> +    uint8_t chan_id;
> +    uint8_t ns;
> +    uint32_t pc;
> +    PL330Chan *s;
> +
> +    DB_PRINT("\n");
> +
> +    if (!ch->is_manager) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
According to description its more likely to cause UNDEF_INSTR here, not 
OPERAND_INVALID
> +        return;
> +    }
> +    ns = !!(opcode&  2);
> [..snip..]
> +
> +static Property pl330_properties[] = {
> +    DEFINE_PROP_UINT32("cfg0", PL330, cfg[0], 0),
> +    DEFINE_PROP_UINT32("cfg1", PL330, cfg[1], 0),
> +    DEFINE_PROP_UINT32("cfg2", PL330, cfg[2], 0),
> +    DEFINE_PROP_UINT32("cfg3", PL330, cfg[3], 0),
> +    DEFINE_PROP_UINT32("cfg4", PL330, cfg[4], 0),
> +    DEFINE_PROP_UINT32("cfg5", PL330, cfg[5], 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pl330_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pl330_init;
> +    dc->reset = pl330_reset;
> +    dc->props = pl330_properties;
> +}
> +
> +static TypeInfo pl330_info = {
> +    .name           = "pl330",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(PL330),
> +    .class_init      = pl330_class_init,
> +};
> +
I think Andreas requires all static TypeInfos to have const qualifier 
and their names to comply with "<device>_type_info" naming convention. 
I'm not sure about this though.
> +static void pl330_register_types(void)
> +{
> +    type_register_static(&pl330_info);
> +}
> +
> +type_init(pl330_register_types)

And it still has no save/load support, it is really mandatory for all 
new devices. I can recall that one of the maintainers wrote a while ago 
that every device at least needs to mark itself as non-migratable, if it 
doesn't implement a proper vmstate.

We used this PL330 implementation to transfer sound data in our emulated 
exynos-based system. It works, but very slow, because the way real 
hardware performs data transfers is not optimal for emulation.

Tested by: Igor Mitsyanko <i.mitsyanko@samsung.com>