From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51117) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5n4p-0006hX-3V for qemu-devel@nongnu.org; Thu, 08 Mar 2012 18:49:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S5n4m-0005t6-SX for qemu-devel@nongnu.org; Thu, 08 Mar 2012 18:48:54 -0500 Received: from cantor2.suse.de ([195.135.220.15]:58128 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5n4m-0005qs-J5 for qemu-devel@nongnu.org; Thu, 08 Mar 2012 18:48:52 -0500 Message-ID: <4F594562.3030001@suse.de> Date: Fri, 09 Mar 2012 00:48:50 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1331215088-28816-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1331215088-28816-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=ISO-8859-15 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: patches@linaro.org, qemu-devel@nongnu.org, Avi Kivity Am 08.03.2012 14:58, schrieb Peter Maydell: > The pxa2xx I2C controller can be at an arbitrary offset within its > region (this is used because one of the controllers starts at offset > 0x1600 into an 0x10000 sized region). The previous implementation of th= is > included an adjustment which worked around the fact that memory region > read/write functions were passed an offset from the start of a page > rather than from the start of the region. Since commit 5312bd8b3 > offsets are now from the start of the region and so we were applying > an incorrect adjustment, resulting in warnings like > "pxa2xx_i2c_read: Bad register 0xffffff90". >=20 > Retain the offset handling but remove the adjustment to the page > boundary. >=20 > Signed-off-by: Peter Maydell > --- > hw/pxa2xx.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) >=20 > diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c > index 1ab2701..d1efef4 100644 > --- a/hw/pxa2xx.c > +++ b/hw/pxa2xx.c > @@ -1507,8 +1507,7 @@ PXA2xxI2CState *pxa2xx_i2c_init(target_phys_addr_= t base, > =20 > i2c_dev =3D sysbus_from_qdev(qdev_create(NULL, "pxa2xx_i2c")); > qdev_prop_set_uint32(&i2c_dev->qdev, "size", region_size + 1); > - qdev_prop_set_uint32(&i2c_dev->qdev, "offset", > - base - (base & (~region_size) & TARGET_PAGE_MASK)); > + qdev_prop_set_uint32(&i2c_dev->qdev, "offset", base & region_size)= ; > =20 > qdev_init_nofail(&i2c_dev->qdev); > =20 Sorry, but I don't immediately follow why this is correct. Thus someone else will, too. Are we talking about an offset of a to be added subregion or an offset used within read/write ops? My understanding from previous threads was the latter. Judging by the "size" property it seems region_size is a misnomer and is rather the size minus one, i.e. a mask to offset within an enclosing region of size region_size + 1? An offset of "base & region_size" surely reads wrong, especially when mentioning region size 0x10000. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg