Re: [Qemu-devel] [PATCH] imx.31 and KZM board support

From: Peter Chubb <peter.chubb@nicta.com.au>
To: "Andreas Färber" <andreas.faerber@web.de>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Peter Chubb <peter.chubb@nicta.com.au>,
	Peter Chubb <peterc@gelato.unsw.edu.au>,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] imx.31 and KZM board support
Date: Tue, 22 Nov 2011 10:04:56 +1100	[thread overview]
Message-ID: <w41ut1rs9z.wl%peter@chubb.wattle.id.au> (raw)
In-Reply-To: <4ECAD474.4050209@web.de>

>>>>> "Andreas" == Andreas Färber <andreas.faerber@web.de> writes:

Andreas> Hi Peter, Am 21.11.2011 22:58, schrieb Peter Chubb:
>> Hi Peter, Please find appended a patch containing initial support
>> for the FreeScale i.MX31 and the KZM Arm11 evaluation board.

Andreas> Your patch format is a bit unusual.

Andreas> Please don't include personal messages in the description,
Andreas> keep it in a format we can apply unchanged with git-am.

OK, I'll try to fix it -- I'm using Quilt so it's not too hard.

Andreas> git-am complains:

Andreas> /home/andreas/QEMU/qemu-arm/.git/rebase-apply/patch:43: new
Andreas> blank line at EOF.  + warning: 1 line adds whitespace errors.

Dunno where that comes from.

>> The implementation was originally written by Hans Jang and Adam
>> Clench of OK-Labs; I've updated it to the current qdev and memory
>> region paradigms and implemented enough extra that Linux will boot
>> on the patched QEMU using a ram disk.
>> 
>> The i.MX 31 Serial controller is found in most of the i.MX SoCs;
>> the AVIC and timer implementations can also be shared, albeit with
>> fewer chips.
>> 
>> Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>
>> Signed-off-by: Hans Jang <hsjang@ok-labs.com> Signed-off-by: Adam
>> Clench <adamc@ok-labs.com>

Andreas> If as you describe above, you polished up patches originally
Andreas> by OK-Labs then your SoB should be placed last.

Will fix.

>> Index: qemu-working/hw/imx_avic.c
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++
>> qemu-working/hw/imx_avic.c 2011-11-22 08:51:09.733239638 +1100 @@
>> -0,0 +1,294 @@ +/* + * IMX31 Vectored Interrupt Controller + * + *
>> Note this is NOT the PL192 provided by ARM, but + * a custom
>> implementation by FreeScale.  + * + * Copyright (c) 2008 OKL + *
>> Written by Hans + * + * This code is licenced under the GPL.

Andreas> If you can, it would be nice to clarify the "GPL" license:

Andreas>     ... the GNU General Public License as published by the
Andreas> Free Software Foundation; either version 1, or (at your
Andreas> option) any later version.

I think it's version 2.0 or later.  I'll check and fix.

>> + * + * TODO: implement vectors and priorities.  + */ + +#include
>> "hw.h" +#include "sysbus.h" +#include <string.h> /* ffsll */ +
>> +#define DEBUG_INT 1 +#undef DEBUG_INT /* comment out for debugging
>> */

Andreas> Usually we just do //#define DEBUG_...

Checkpatch.pl complains about that!

>> ??? Save/restore.  */

Andreas> What does this comment tell us? :)

Dunno.  It was in the codebase I got; I think it means that save and
restore aren't implemented yet.

Andreas> My sharp eye also spotted an | without spaces somewhere. If
Andreas> you haven't already, try running scripts/checkpatch.pl.

I couldn't find one that was obvious, except in the big list of
reset-by-write-one bits in the imx_serial.c code:

 value &= (1<<15)|(1<<13)|(1<<12)|(1<<11)|(1<<10)|
            (1<<8)|(1<<7)|(1<<6)|(1<<4)|(1<<2)|(1<<1);

checkpatch.pl didn't complain about this!

Thanks for reviewing.

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