On 27/7/26 12:04, Peter Maydell wrote:
On Fri, 24 Jul 2026 at 13:51, botszhuang <[email protected]> wrote:
hw/i2c/bcm2835_i2c: Correct CLKT register offset
The Clock Stretch Timeout (CLKT) register for the BCM2835 I2C controller is
actually located at offset 0x1c, not 0x20.
The previous incorrect value caused the guest OS to read from and write to the
wrong memory address when configuring the I2C timeout, which could lead to
unexpected controller behavior.
Update the BCM2835_I2C_CLKT macro to match the hardware specification.
Checking the datasheet, this is definitely the wrong value,
so the fix is correct. I'm a bit confused by your commit message,
though. The value here in QEMU can't cause the guest OS to write
to the wrong memory address, because the guest OS doesn't ever
see what we have defined here. It has the offset hardcoded into
its own source code.
It looks to me like the effect of getting this wrong is that when
the guest writes to CLKT (at its actual 0x1c offset) we will log
a guest error and give read-as-zero behaviour. That's not correct,
but I expect for most guest OSes it'll be pretty harmless because
the guest probably doesn't write the value it wants to configure
and then read it back to check. (QEMU ignores the value written
anyway.)
I've applied this to target-arm.next.
Should we also reduce the region size?
-- >8 --
diff --git a/hw/i2c/bcm2835_i2c.c b/hw/i2c/bcm2835_i2c.c
index 34de1f36e5b..5f6093087dd 100644
--- a/hw/i2c/bcm2835_i2c.c
+++ b/hw/i2c/bcm2835_i2c.c
@@ -222,7 +222,7 @@ static void bcm2835_i2c_realize(DeviceState *dev,
Error **errp)
s->bus = i2c_init_bus(dev, NULL);
memory_region_init_io(&s->iomem, OBJECT(dev), &bcm2835_i2c_ops, s,
- TYPE_BCM2835_I2C, 0x24);
+ TYPE_BCM2835_I2C, 0x20);
sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
}
---
