From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9ZKJ-0004Mh-OT for qemu-devel@nongnu.org; Fri, 30 Sep 2011 05:24:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R9ZKD-0001IV-Gk for qemu-devel@nongnu.org; Fri, 30 Sep 2011 05:24:15 -0400 Received: from lemon.ertos.nicta.com.au ([203.143.174.143]:52856 helo=lemon.ken.nicta.com.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9ZKD-0001Gs-05 for qemu-devel@nongnu.org; Fri, 30 Sep 2011 05:24:09 -0400 Date: Fri, 30 Sep 2011 19:23:45 +1000 Message-ID: From: Peter Chubb In-Reply-To: References: 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] [ARM] Fix sp804 dual-timer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: davidm@ok-labs.com, Peter Chubb , qemu-devel@nongnu.org, philipo@ok-labs.com, Paul Brook >>>>> "Peter" =3D=3D Peter Maydell writes: Peter> On 30 September 2011 01:20, Peter Chubb Peter> wrote: >> Thanks Peter! >>=20 >> Here's a reworked patch. Peter> NB: when you resend patches it's better to send them as Peter> completely fresh emails (via git-send-email or equivalent) Peter> because otherwise they're more of a pain to apply (you end up Peter> with random email chatter in the commit message, usually). Thanks for the heads-up. I'll edit the patch in Quilt. >> +/* + * sp804_id should be: + * union { + * struct { + * =C2=A0 uint32_t >> PartNumber:12; =3D=3D 0x804 + * =C2=A0 uint32_t DesignerID:8; =3D=3D 'A'= + * =C2=A0 >> uint32_t Revision:4; =3D=3D 1 + * =C2=A0 uint32_t Configurations:6; =3D= =3D 0 + * >> }; + * uint8_t bytes[4]; + * }; + * but that gets into too many >> byte-ordering and packing issues. + */ +static const uint8_t >> sp804_id[] =3D {0x04, 0x18, 0x14, 0}; +static const uint8_t >> sp804_PrimeCellID[] =3D {0xB1, 0x05, 0xF0, 0x0D}; Peter> I disagree with "should be" -- yes, semantically the ID Peter> registers have a number of subfields but for practical purposes Peter> they're just bytes; so I don't think that comment is necessary. Peter> There's no need to split the two sets of ID registers into Peter> different arrays, either -- it just complicates the code. Also Peter> you have the primecell ID values in the wrong order (check the Peter> 'register summary' table in the docs). You want: OK, and thanks. Peter> static const uint8_t sp804_id[] =3D { 0x04, 0x18, 0x14, 0, 0x0d, Peter> 0xf0, 0x05, 0xb1 }; >> + =C2=A0 =C2=A0/* + =C2=A0 =C2=A0 * Ids are packed into a word, then acc= essed one byte >> per word. + =C2=A0 =C2=A0 */ Peter> No, they're just a set of byte registers. At word offsets. >> + =C2=A0 =C2=A0/* TimerPeriphID */ + =C2=A0 =C2=A0if (offset >=3D 0xfe0 = && offset <=3D >> 0xfec) + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return sp804_id[(offset - 0x= fe0) >> 2]; Peter> Coding style requires braces on if statements (plus your Peter> indentation is wrong). If you run your patch through Peter> scripts/checkpatch.pl it will catch this sort of thing. Thanks. >> + =C2=A0 =C2=A0hw_error("sp804_read: Bad offset %x\n", (int)offset); + = =C2=A0 >> =C2=A0return 0; Peter> hw_error() is a fatal error -- don't use it for conditions that Peter> can be triggered by a malicious guest. (And since it's noreturn Peter> there's not much point putting any code after it...) Is there a better `tell the programmer s/he's done something stupid' error function? The plxxx.c files all used hw_error() for bad offsets. I shan't be able to get to this again until Tuesday my time. Expect another patch then. 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