Re: [PATCH] fix BCM2835_I2C_CLKT

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);
  }
---