hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region

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

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

On 8 March 2012 23:48, Andreas Färber <afaerber@suse.de> 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 <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.

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

On 9 March 2012 08:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> I'm happy to tweak the commit message if you have suggestions
> for wording to add.

How about this redrafting?

===begin===
hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region

The pxa2xx I2C controller can have its registers at an arbitrary offset
within the MemoryRegion it creates. We use this to create two controllers,
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 region.

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 mapped
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 value
to set the "offset" qdev property we included a rounding to a page
boundary.

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, resulting
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)) == base & region_size

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.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
===endit===

Am 14.03.2012 13:45, schrieb Peter Maydell:
> On 9 March 2012 08:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I'm happy to tweak the commit message if you have suggestions
>> for wording to add.
> 
> How about this redrafting?
> 
> ===begin===
> hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region
> 
> The pxa2xx I2C controller can have its registers at an arbitrary offset
> within the MemoryRegion it creates. We use this to create two controllers,
> 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 region.
> 
> 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 mapped
> 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 value
> to set the "offset" qdev property we included a rounding to a page
> boundary.
> 
> 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, resulting
> 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)) == base & region_size
> 
> 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.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ===endit===

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ärber <afaerber@suse.de>

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

On 14 March 2012 13:13, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.03.2012 13:45, schrieb Peter Maydell:
>> On 9 March 2012 08:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I'm happy to tweak the commit message if you have suggestions
>>> for wording to add.
>>
>> How about this redrafting?
>>
>> ===begin===
>> hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region
>>
>> The pxa2xx I2C controller can have its registers at an arbitrary offset
>> within the MemoryRegion it creates. We use this to create two controllers,
>> 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 region.
>>
>> 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 mapped
>> 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 value
>> to set the "offset" qdev property we included a rounding to a page
>> boundary.
>>
>> 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, resulting
>> 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)) == base & region_size
>>
>> 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.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ===endit===
>
> 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ärber <afaerber@suse.de>

Thanks. I've put the patch in arm-devs.next with the updated
commit message and your reviewed-by tag.

> Sorry for loosing track of this message, my inbox works more like a
> stack than a queue these days...

No problem, I have mechanisms for tracking patches I've sent so they
don't get lost ;-)

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

Yep, I'm fine with cleaning that up in a follow-up patch (though I
don't personally care about pxa2xx enough to write the patch, I
agree it would look better to have the function argument and the
qdev property both be the same thing.)

-- PMM