Dear all,
Thanks for applying the patch and for the detailed explanation!
You are completely right—my commit message reasoning was flawed. QEMU's
macro doesn't change the guest's behavior, it only affects how QEMU handles
the guest's access (logging a guest error and returning read-as-zero, as
you noted). I appreciate you clarifying that.
Regarding the register layout, since the CLKT register is at offset 0x1c
(and assuming it's the last 32-bit register), the total memory region size
should technically be 0x20 instead of 0x24.
Do you think we should also reduce the region size, as shown in the diff
below? If it looks good to you, I can send this as a separate follow-up
patch.
-- >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);
}
---
Best regards,
Botsz HUANG
Signed-off-by: botszhuang <[email protected]>
Philippe Mathieu-Daudé <[email protected]> 於 2026年7月27日週一 下午7:54寫道：
> 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);
> }
> ---
>
