Re: [Qemu-devel] [PATCH] hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region

["Andreas Färber" <afaerber@suse.de>]

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 this
> 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".
> 
> Retain the offset handling but remove the adjustment to the page
> boundary.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/pxa2xx.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> 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,
>  
>      i2c_dev = 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);
>  
>      qdev_init_nofail(&i2c_dev->qdev);
>  

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg