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

["Andreas Färber" <afaerber@suse.de>]

Am 19.06.2012 08:40, schrieb Peter Crosthwaite:
> On Tue, Jun 19, 2012 at 12:33 AM, Igor Mitsyanko
> <i.mitsyanko@samsung.com> 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<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
> 
> 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

Yes, the lack of const in uses such as these has historic reasons and
keeps propagating. If you touch it anyway, ..._type_info would be more
self-describing but not a hard requirement.
Thanks for keeping eyes open, Igor. :)

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

Not my requirement but Peter's (cc'ing). Usually it's really trivial
adding a handful of fields to the VMSD, so I can understand though.

Regards,
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 <i.mitsyanko@samsung.com>
>>
> 
> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg