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

[Peter Maydell]

On 8 March 2012 23:48, Andreas Färber <address@hidden> wrote:
> 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 <address@hidden>
>> ---
>>  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.

Yeah, it took me a couple of times round the existing code to figure out
what it was doing. Basically pxa2xx_i2c_init(base, irq, region_size)
means "create a memory region of region_size+1 at base-rounded-down-to-
region_size, such that the I2C registers are at address base".

So this:
    s->i2c[0] = pxa2xx_i2c_init(0x40301600,
                    qdev_get_gpio_in(s->pic, PXA2XX_PIC_I2C), 0xffff);
creates a region 0x40300000..0x4030ffff with the registers starting
at 0x40301600, and this:
    s->i2c[1] = pxa2xx_i2c_init(0x40f00100,
                    qdev_get_gpio_in(s->pic, PXA2XX_PIC_PWRI2C), 0xff);
creates a region 0x40f00100..0x40f001ff with the registers starting
at 0x40f00100.

So the qdev offset parameter is (now) the offset from the base of
the MemoryRegion of the i2c register area. It used to be the offset
from the base of the memory region rounded down to a page boundary.

(The PXA2xx TRMs say that addresses inside the 0x10000-sized region
decode but return undefined values, rather than causing a bus error,
so this weirdness is following the hardware behaviour.)

You're right that calling the qdev property and the function
parameter region_size when they're actually the region size - 1
is confusing, but this patch is trying to fix a bug, not clean
up the existing code.

I'm happy to tweak the commit message if you have suggestions
for wording to add.

-- PMM