Am 14.03.2012 13:45, schrieb Peter Maydell: > On 9 March 2012 08:26, Peter Maydell <[email protected]> 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 <[email protected]> > ===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 <[email protected]> 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
