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

[Peter Crosthwaite]

On Tue, Jun 19, 2012 at 12:33 AM, Igor Mitsyanko
<address@hidden> wrote:
>
> 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<address@hidden>
>> Signed-off-by: Kirill Batuzov<address@hidden>
>> ---
>> [..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

Ok

>>
>> +        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.
>

Ok

>> +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.
>
Ok, ccing  Andreas

> 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.
>

Thats another battle for another day,

> Tested by: Igor Mitsyanko <address@hidden>
>

Sweet,

Ill roll a V5 soon, but im guessing PMM will do a review cycle here as
well, so ill give it a few days.

Regards,
Peter