Re: [PATCH v4 01/13] hw/i2c: Add designware i2c controller

[Nicholas Piggin <npiggin@gmail.com>]

On Tue, May 05, 2026 at 09:57:20AM +0200, Philippe Mathieu-Daudé wrote:
> On 25/4/26 15:17, Joel Stanley wrote:
> > From: Chris Rauer <crauer@google.com>
> > 
> > In the past this model has been submitted for use with the arm virt
> > machine, however in this case it will be used by the upcoming
> > Tenstorrent Atlantis RISC-V machine.
> > 
> > This is a re-submission of the model with Chris' permission, with a
> > light touch of updates to make it build with qemu master.
> > 
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Signed-off-by: Chris Rauer <crauer@google.com>
> > Link: https://lore.kernel.org/qemu-devel/20220110214755.810343-2-venture@google.com
> > [jms: rebase and minor build fixes for class_init and reset callback]
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > v4: Remove unused random header
> > v2: Add trace event for read and write, document Alano and myself as
> > reviewers.
> > ---
> >   MAINTAINERS                     |   8 +
> >   include/hw/i2c/designware_i2c.h | 101 ++++
> >   hw/i2c/designware_i2c.c         | 817 ++++++++++++++++++++++++++++++++
> >   hw/i2c/Kconfig                  |   4 +
> >   hw/i2c/meson.build              |   1 +
> >   hw/i2c/trace-events             |   4 +
> >   6 files changed, 935 insertions(+)
> >   create mode 100644 include/hw/i2c/designware_i2c.h
> >   create mode 100644 hw/i2c/designware_i2c.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aa4267b15806..e1942a86eba5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2716,6 +2716,14 @@ S: Orphan
> >   F: hw/gpio/pcf8574.c
> >   F: include/gpio/pcf8574.h
> > +DesignWare I2C
> > +M: Chris Rauer <crauer@google.com>
> 
> This was 4 years ago. Is Chris still up to this?

Joel is away for a few weeks, but AFAIK he did ping Chris and got
ack to upstream his patch.

> > +    case DW_IC_SAR:
> > +        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_sar\n",
> > +                      DEVICE(s)->canonical_path);
> > +        s->ic_sar = value & DW_IC_SAR_MASK;
> 
> Unsupported but updated...

Yeah, not sure what the best course of action is, maybe device driver
was working better if the value got saved?

> > +    s->ic_comp_param_1 = DW_IC_COMP_PARAM_1_INIT_VAL;
> > +    s->ic_comp_version = DW_IC_COMP_VERSION_INIT_VAL;
> > +    s->ic_comp_type = DW_IC_COMP_TYPE_INIT_VAL;
> 
> Preferably initialize the IC_COMP registers once in the
> designware_i2c_realize() handler.

This I didn't change yet because I started register API converstion and
that all changes.

> > +static void designware_i2c_smbus_init(Object *obj)
> > +{
> > +    DesignWareI2CState *s = DESIGNWARE_I2C(obj);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +
> > +    sysbus_init_irq(sbd, &s->irq);
> > +
> > +    memory_region_init_io(&s->iomem, obj, &designware_i2c_ops, s,
> > +                          "regs", 4 * KiB);
> 
> I note the current registers only describe a 1KiB region.

256 bytes even. It might have 4K alignment in data sheet.

> > +}
> Since I couldn't look at the datasheet values, for overall modelling:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

I have an incremental patch here that incorporates review comments and
also a couple of possible issues I found - RX underflow bit was never
being set AFAIKS it should be set here, and one of the INTR bit clear
registers was not recalculating irqs.

I will fold this in before next repost if there are no objections.

Thanks,
Nick

---
diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
index 7d8b1c1353..1e5505f571 100644
--- a/hw/i2c/designware_i2c.c
+++ b/hw/i2c/designware_i2c.c
@@ -217,7 +217,7 @@ static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
                       "%s: Attempted to read from RX fifo when not in receive "
                       "state.\n", DEVICE(s)->canonical_path);
         if (s->status != DW_I2C_STATUS_IDLE) {
-            s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
+            s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
             dw_i2c_update_irq(s);
         }
         return 0;
@@ -228,7 +228,7 @@ static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
     if (s->ic_rxflr > 0) {
         s->ic_rxflr--;
     } else {
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
+        s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
         dw_i2c_update_irq(s);
         return 0;
     }
@@ -395,6 +395,7 @@ static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
         break;
     case DW_IC_CLR_RESTART_DET:
         s->ic_raw_intr_stat &= ~(DW_IC_INTR_RESTART_DET);
+        dw_i2c_update_irq(s);
         break;
     case DW_IC_COMP_PARAM_1:
         value = s->ic_comp_param_1;
@@ -698,6 +699,10 @@ static const MemoryRegionOps designware_i2c_ops = {
     .read = dw_i2c_read,
     .write = dw_i2c_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -748,7 +753,7 @@ static const VMStateDescription vmstate_designware_i2c = {
     .name = TYPE_DESIGNWARE_I2C,
     .version_id = 0,
     .minimum_version_id = 0,
-    .fields = (VMStateField[]) {
+    .fields = (const VMStateField[]) {
         VMSTATE_UINT32(ic_con, DesignWareI2CState),
         VMSTATE_UINT32(ic_tar, DesignWareI2CState),
         VMSTATE_UINT32(ic_sar, DesignWareI2CState),
@@ -803,6 +808,8 @@ static void designware_i2c_class_init(ObjectClass *klass, const void *data)
     dc->vmsd = &vmstate_designware_i2c;
     rc->phases.enter = designware_i2c_enter_reset;
     rc->phases.hold = designware_i2c_hold_reset;
+
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo designware_i2c_types[] = {
diff --git a/include/hw/i2c/designware_i2c.h b/include/hw/i2c/designware_i2c.h
index 0d8f904f51..71d74c9141 100644
--- a/include/hw/i2c/designware_i2c.h
+++ b/include/hw/i2c/designware_i2c.h
@@ -57,7 +57,7 @@ typedef enum DesignWareI2CStatus {
  * @status: The current status of the SMBus.
  */
 typedef struct DesignWareI2CState {
-    SysBusDevice parent;
+    SysBusDevice parent_obj;
 
     MemoryRegion iomem;