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

[Peter Maydell]

On 14 March 2012 13:13, Andreas Färber <address@hidden> wrote:
> Am 14.03.2012 13:45, schrieb Peter Maydell:
>> On 9 March 2012 08:26, Peter Maydell <address@hidden> 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 <address@hidden>
>> ===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 <address@hidden>

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