From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49084) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RXjRn-0002Nw-Oa for qemu-devel@nongnu.org; Mon, 05 Dec 2011 20:03:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RXjRm-0004gP-6S for qemu-devel@nongnu.org; Mon, 05 Dec 2011 20:03:51 -0500 Received: from lemon.ertos.nicta.com.au ([203.143.174.143]:47573 helo=lemon.ken.nicta.com.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RXjRl-0004fw-I1 for qemu-devel@nongnu.org; Mon, 05 Dec 2011 20:03:50 -0500 Date: Tue, 06 Dec 2011 12:03:36 +1100 Message-ID: From: Peter Chubb In-Reply-To: References: <20111130033628.097950901@nicta.com.au> <20111130034234.403045191@nicta.com.au> MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V3 1/4] Implement the FreeScale i.MX UART. This uart is used in a variety of SoCs, including some by Motorola, as well as in the FreeScale i.MX series. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Hans Jang , Adam Clench , Peter Chubb , qemu-devel@nongnu.org, Andreas =?UTF-8?B?RsOkcmJlcg==?= >>>>> "Peter" =3D=3D Peter Maydell writes: Peter> On 30 November 2011 03:36, Peter Chubb Peter> wrote: Commit messages should be Peter> formatted with a short summary line, then a blank line, then a Peter> more detailed description. You've put everything into one Peter> extremely long summary line here, which looks odd in git Peter> log. Try: Thanks I've fixed that now. >> top-level directory. Peter> Still GPL v2 only. Fixed. >> + =C2=A0 =C2=A0uint32_t ubrm; Peter> The MCIMX31RM calls this UBMR, not UBRM... Fixed. Peter> My copy of the MCIMX31RM says we also set RXDS on reset. Fixed. >> + =C2=A0 =C2=A0s->usr2 =3D USR2_TXFE | USR2_TXDC; Peter> Presumably we don't set DCDIN here because we haven't Peter> implemented the modem control signals yet? Exactly. But there's no harm in setting it, so I've added it in. >> + =C2=A0 =C2=A0s->uts1 =3D UTS1_RXEMPTY | UTS1_TXEMPTY; + =C2=A0 =C2=A0s= ->ubrm =3D 0; + =C2=A0 >> =C2=A0s->ubrc =3D 0; Peter> RM says the reset value for UBRC is 0x4. OK. Peter> You also need to reset s->ucr1. (If you want to retain that Peter> hack in the init function then you still need to reset the Peter> other bits even if you don't allow reset to clear UARTEN.) OK. Fixed. >> + =C2=A0 =C2=A0s->readbuff =3D 0;=20 >> + =C2=A0 =C2=A0imx_update(s); Peter> This will call qemu_set_irq() which is a bad idea in a reset Peter> function. Don't call imx_update() here, instead call it after Peter> calling imx_serial_reset() from your register write function. Does the infrastructure guarantee that any interrupt will be cleared when the reset function is called? >> +=C2=A0 =C2=A0case 0x21: /* UCR2 */=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1; /* reset complete */ Peter> UCR2_SRST. Fixed. >> =C2=A0 =C2=A0case 0x20: /* UCR1 */=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->ucr1 =3D value; Peter> RM says the top 16 bits of UCR1 are read-only. Fixed. Peter> This doesn't implement writing to any of the other bits of Peter> UCR2. No, apart from the TXEN and RXEN bits they're all to do with=20 >> + =C2=A0 =C2=A0case 0x26: /* USR2 */=20 >> + =C2=A0 =C2=A0 =C2=A0 /*=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* Writing 1 to some bits clears them; all o= ther=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* values are ignored=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0value &=3D (1<<15) | (1<<13) | (1<<12) | (= 1<<11) | (1<<10)|=20 >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(1<<8) | (1<<7) | (1<<6) | (1= <<4) | (1<<2) | (1<<1); Peter> define and use USR2_FOO named constants. Done. >> UCR3 */ + =C2=A0 =C2=A0case 0x23: /* UCR4 */ + =C2=A0 =C2=A0case 0x24: /= * UFCR */ + =C2=A0 >> =C2=A0case 0x25: /* USR1 */ + =C2=A0 =C2=A0case 0x2c: /* BIPR1 */ + =C2= =A0 =C2=A0 =C2=A0 =C2=A0/* TODO >> */ Peter> No IPRINTF() ? This one gets hit too often. >> + =C2=A0 =C2=A0.qdev.size =3D sizeof (imx_state), Peter> Unnecessary space (and checkpatch will complain). I disagree -- there should be a space between the sizeof operator and the cast that is its operand. sizeof is not a function; and the parentheses in this case are part of its operand. I couldn't find any mention of sizeof in the Coding Style document. However, I'll go with whatever makes you happier.... (but note that the current qemu source mixes sizeof (type) and sizeof(type) all over the place). Look out for a new patch series real soon now... Peter C -- Dr Peter Chubb http://www.gelato.unsw.edu.au peterc AT gelato.unsw.edu.au http://www.ertos.nicta.com.au ERTOS within National ICT Australia