From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35103) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SgdAk-000599-Ea for qemu-devel@nongnu.org; Mon, 18 Jun 2012 10:43:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SgdAd-0006MF-SW for qemu-devel@nongnu.org; Mon, 18 Jun 2012 10:43:18 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:16365) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SgdAd-0006Lk-Mp for qemu-devel@nongnu.org; Mon, 18 Jun 2012 10:43:11 -0400 Received: from eusync4.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M5T00BACHRVDK00@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 18 Jun 2012 15:33:31 +0100 (BST) Received: from [106.109.9.58] by eusync4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0M5T00ISTHR07650@eusync4.samsung.com> for qemu-devel@nongnu.org; Mon, 18 Jun 2012 15:33:01 +0100 (BST) Message-id: <4FDF3C1C.1020008@samsung.com> Date: Mon, 18 Jun 2012 18:33:00 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: <15126b43916ee3b1db4eafe3b0fabfcbfc8c0584.1339979922.git.peter.crosthwaite@petalogix.com> In-reply-to: <15126b43916ee3b1db4eafe3b0fabfcbfc8c0584.1339979922.git.peter.crosthwaite@petalogix.com> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/2] pl330: initial version List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Peter A. G. Crosthwaite" Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, zhur@ispras.ru, batuzovk@ispras.ru, kyungmin.park@samsung.com, paul@codesourcery.com, duyl@xilinx.com, linnj@xilinx.com, edgar.iglesias@gmail.com, john.williams@petalogix.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 > Signed-off-by: Kirill Batuzov > --- > [..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 "_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