From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7o1R-0000pZ-Ps for qemu-devel@nongnu.org; Wed, 14 Mar 2012 09:13:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7o13-0002G4-1R for qemu-devel@nongnu.org; Wed, 14 Mar 2012 09:13:45 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59792 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7o12-0002Fg-Oe for qemu-devel@nongnu.org; Wed, 14 Mar 2012 09:13:20 -0400 Message-ID: <4F60996E.4050503@suse.de> Date: Wed, 14 Mar 2012 14:13:18 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1331215088-28816-1-git-send-email-peter.maydell@linaro.org> <4F594562.3030001@suse.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Avi Kivity , qemu-devel@nongnu.org, patches@linaro.org Am 14.03.2012 13:45, schrieb Peter Maydell: > On 9 March 2012 08:26, Peter Maydell wrote: >> I'm happy to tweak the commit message if you have suggestions >> for wording to add. >=20 > How about this redrafting? >=20 > =3D=3D=3Dbegin=3D=3D=3D > hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region >=20 > The pxa2xx I2C controller can have its registers at an arbitrary offset > within the MemoryRegion it creates. We use this to create two controlle= rs, > one which covers a region of size 0x10000 with registers starting at an > offset 0x1600 into that region, and a second one which covers a region > of size just 0x100 with the registers starting at the base of the regio= n. >=20 > The implementation of this offsetting uses two qdev properties, "offset= " > (which sets the offset which must be subtracted from the address to > get the offset into the actual register bank) and "size", which is the > size of the MemoryRegion. We were actually using "offset" for two > purposes: firstly the required one of handling the registers not being > at the base of the MemoryRegion, and secondly as a workaround for a > deficiency of QEMU. Until commit 5312bd8b3, if a MemoryRegion was mappe= d > at a non-page boundary, the address passed into the read and write > functions would be the offset from the start of the page, not the > offset from the start of the MemoryRegion. So when calculating the valu= e > to set the "offset" qdev property we included a rounding to a page > boundary. >=20 > Following commit 5312bd8b3 MemoryRegion read/write functions are now > correctly passed the offset from the base of the region, and our > workaround now means we're subtracting too much from addresses, resulti= ng > in warnings like "pxa2xx_i2c_read: Bad register 0xffffff90". > The fix for this is simply to remove the rounding to a page boundary; > this allows us to slightly simplify the expression since > base - (base & (~region_size)) =3D=3D base & region_size >=20 > The qdev property "offset" itself must remain because it is still > performing its primary job of handling register banks not being at > the base of the MemoryRegion. >=20 > Signed-off-by: Peter Maydell > =3D=3D=3Dendit=3D=3D=3D That's more text than I expected but a perfect explanation. In short it translates to us dropping the page alignment as a bugfix and simplifying the resulting expression in one go. Reviewed-by: Andreas F=C3=A4rber Sorry for loosing track of this message, my inbox works more like a stack than a queue these days... I'd still like to change the "region_size" being off by one (0xff rather than the above 0x100) in a follow-up patch. That should not stop us applying this immediate fix. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg