On 14 March 2012 13:13, Andreas Färber <afaer...@suse.de> wrote: > Am 14.03.2012 13:45, schrieb Peter Maydell: >> On 9 March 2012 08:26, Peter Maydell <peter.mayd...@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.mayd...@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 <afaer...@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
