On Tue, Jun 19, 2012 at 12:33 AM, Igor Mitsyanko <i.mitsya...@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.crosthwa...@petalogix.com> >> Signed-off-by: Kirill Batuzov<batuz...@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 >> +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 <i.mitsya...@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