All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] hw/i2c: Add designware i2c controller
@ 2026-05-07 12:05 Nicholas Piggin
  2026-05-07 12:05 ` [PATCH 1/4] " Nicholas Piggin
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-07 12:05 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Nicholas Piggin, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

Hi,

This series contains the DW I2C model written by Chris Rauer and
updated for the Tenstorrent Atlantis machine recently. There was
some more review comment on that submission and so we decided to
take the I2C device out of that series and work on it separately,
see here:

https://lore.kernel.org/qemu-devel/20260425131721.932250-1-joel@jms.id.au/T/#mb1ef2824c2f1f37bf4574dc1ef0fb95566c3a2f2

The big thing suggested was to move to the QEMU register API. That
is a big change and difficult to review, so I have split that and
a some smaller changes out into their own patches. I don't expect
detailed reviews on the register API patch -- it's quite mechanical
and I did attempt to verify it by diff'ing register traces. But it
would be good to make sure maintainers are happy to go that way.

Unfortunately the patch 1 was quite well reviewed and tested so
incremental changes would be preferable, but it is painful to maintain
migration compatibility across these changes.

Thanks,
Nick

Chris Rauer (1):
  hw/i2c: Add designware i2c controller

Nicholas Piggin (3):
  [RFC] hw/i2c/designware_i2c: Switch to Fifo8
  [RFC] hw/i2c/designware_i2c: Switch to QEMU register API
  [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK

 MAINTAINERS                     |   8 +
 hw/i2c/Kconfig                  |   5 +
 hw/i2c/designware_i2c.c         | 742 ++++++++++++++++++++++++++++++++
 hw/i2c/meson.build              |   1 +
 hw/i2c/trace-events             |   4 +
 include/hw/i2c/designware_i2c.h |  56 +++
 roms/seabios-hppa               |   2 +-
 7 files changed, 817 insertions(+), 1 deletion(-)
 create mode 100644 hw/i2c/designware_i2c.c
 create mode 100644 include/hw/i2c/designware_i2c.h

-- 
2.53.0



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] hw/i2c: Add designware i2c controller
  2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
@ 2026-05-07 12:05 ` Nicholas Piggin
  2026-05-11 10:20   ` Philippe Mathieu-Daudé
  2026-05-07 12:05 ` [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8 Nicholas Piggin
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-07 12:05 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Nicholas Piggin, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé, Joel Stanley

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>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes since posting in v4 of the TT Atlantis series, thanks to Phil
for comments:

- Fix setting of underflow interrupt bit
- Recalculate irq state after clearing RESTART_DET interrupt bit
- Add .impl attributes to register MMIO ops
- Set DEVICE_CATEGORY_BRIDGE
- Fix 'parent_obj' naming
- constify VMState fields

Thanks,
Nick
---
 MAINTAINERS                     |   8 +
 hw/i2c/Kconfig                  |   4 +
 hw/i2c/designware_i2c.c         | 824 ++++++++++++++++++++++++++++++++
 hw/i2c/meson.build              |   1 +
 hw/i2c/trace-events             |   4 +
 include/hw/i2c/designware_i2c.h | 101 ++++
 roms/seabios-hppa               |   2 +-
 7 files changed, 943 insertions(+), 1 deletion(-)
 create mode 100644 hw/i2c/designware_i2c.c
 create mode 100644 include/hw/i2c/designware_i2c.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e106cadb81..8df2464aa0 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>
+R: Alano Song <alanosong@163.com>
+R: Joel Stanley <joel@jms.id.au>
+S: Maintained
+F: hw/i2c/designware_i2c.c
+F: include/hw/i2c/designware_i2c.h
+
 Generic Loader
 M: Alistair Francis <alistair@alistair23.me>
 S: Maintained
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 596a7a3165..d3f394edeb 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -18,6 +18,10 @@ config ARM_SBCON_I2C
     bool
     select BITBANG_I2C
 
+config DESIGNWARE_I2C
+    bool
+    select I2C
+
 config ACPI_SMBUS
     bool
     select SMBUS
diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
new file mode 100644
index 0000000000..1e5505f571
--- /dev/null
+++ b/hw/i2c/designware_i2c.c
@@ -0,0 +1,824 @@
+/*
+ * DesignWare I2C Module.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/i2c/designware_i2c.h"
+#include "migration/vmstate.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+#include "trace.h"
+
+enum DesignWareI2CRegister {
+    DW_IC_CON                   = 0x00,
+    DW_IC_TAR                   = 0x04,
+    DW_IC_SAR                   = 0x08,
+    DW_IC_DATA_CMD              = 0x10,
+    DW_IC_SS_SCL_HCNT           = 0x14,
+    DW_IC_SS_SCL_LCNT           = 0x18,
+    DW_IC_FS_SCL_HCNT           = 0x1c,
+    DW_IC_FS_SCL_LCNT           = 0x20,
+    DW_IC_INTR_STAT             = 0x2c,
+    DW_IC_INTR_MASK             = 0x30,
+    DW_IC_RAW_INTR_STAT         = 0x34,
+    DW_IC_RX_TL                 = 0x38,
+    DW_IC_TX_TL                 = 0x3c,
+    DW_IC_CLR_INTR              = 0x40,
+    DW_IC_CLR_RX_UNDER          = 0x44,
+    DW_IC_CLR_RX_OVER           = 0x48,
+    DW_IC_CLR_TX_OVER           = 0x4c,
+    DW_IC_CLR_RD_REQ            = 0x50,
+    DW_IC_CLR_TX_ABRT           = 0x54,
+    DW_IC_CLR_RX_DONE           = 0x58,
+    DW_IC_CLR_ACTIVITY          = 0x5c,
+    DW_IC_CLR_STOP_DET          = 0x60,
+    DW_IC_CLR_START_DET         = 0x64,
+    DW_IC_CLR_GEN_CALL          = 0x68,
+    DW_IC_ENABLE                = 0x6c,
+    DW_IC_STATUS                = 0x70,
+    DW_IC_TXFLR                 = 0x74,
+    DW_IC_RXFLR                 = 0x78,
+    DW_IC_SDA_HOLD              = 0x7c,
+    DW_IC_TX_ABRT_SOURCE        = 0x80,
+    DW_IC_SLV_DATA_NACK_ONLY    = 0x84,
+    DW_IC_DMA_CR                = 0x88,
+    DW_IC_DMA_TDLR              = 0x8c,
+    DW_IC_DMA_RDLR              = 0x90,
+    DW_IC_SDA_SETUP             = 0x94,
+    DW_IC_ACK_GENERAL_CALL      = 0x98,
+    DW_IC_ENABLE_STATUS         = 0x9c,
+    DW_IC_FS_SPKLEN             = 0xa0,
+    DW_IC_CLR_RESTART_DET       = 0xa8,
+    DW_IC_COMP_PARAM_1          = 0xf4,
+    DW_IC_COMP_VERSION          = 0xf8,
+    DW_IC_COMP_TYPE             = 0xfc,
+};
+
+/* DW_IC_CON fields */
+#define DW_IC_CON_STOP_DET_IF_MASTER_ACTIV  BIT(10)
+#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL     BIT(9)
+#define DW_IC_CON_TX_EMPTY_CTRL             BIT(8)
+#define DW_IC_CON_STOP_IF_ADDRESSED         BIT(7)
+#define DW_IC_CON_SLAVE_DISABLE             BIT(6)
+#define DW_IC_CON_IC_RESTART_EN             BIT(5)
+#define DW_IC_CON_10BITADDR_MASTER          BIT(4)
+#define DW_IC_CON_10BITADDR_SLAVE           BIT(3)
+#define DW_IC_CON_SPEED(rv)                 extract32((rv), 1, 2)
+#define DW_IC_CON_MASTER_MODE               BIT(0)
+
+/* DW_IC_TAR fields */
+#define DW_IC_TAR_IC_10BITADDR_MASTER  BIT(12)
+#define DW_IC_TAR_SPECIAL              BIT(11)
+#define DW_IC_TAR_GC_OR_START          BIT(10)
+#define DW_IC_TAR_ADDRESS(rv)          extract32((rv), 0, 10)
+
+/* DW_IC_DATA_CMD fields */
+#define DW_IC_DATA_CMD_RESTART  BIT(10)
+#define DW_IC_DATA_CMD_STOP     BIT(9)
+#define DW_IC_DATA_CMD_CMD      BIT(8)
+#define DW_IC_DATA_CMD_DAT(rv)  extract32((rv), 0, 8)
+
+/* DW_IC_INTR_STAT/INTR_MASK/RAW_INTR_STAT fields */
+#define DW_IC_INTR_RESTART_DET  BIT(12)
+#define DW_IC_INTR_GEN_CALL     BIT(11)
+#define DW_IC_INTR_START_DET    BIT(10)
+#define DW_IC_INTR_STOP_DET     BIT(9)
+#define DW_IC_INTR_ACTIVITY     BIT(8)
+#define DW_IC_INTR_RX_DONE      BIT(7)
+#define DW_IC_INTR_TX_ABRT      BIT(6)
+#define DW_IC_INTR_RD_REQ       BIT(5)
+#define DW_IC_INTR_TX_EMPTY     BIT(4) /* Hardware clear only. */
+#define DW_IC_INTR_TX_OVER      BIT(3)
+#define DW_IC_INTR_RX_FULL      BIT(2) /* Hardware clear only. */
+#define DW_IC_INTR_RX_OVER      BIT(1)
+#define DW_IC_INTR_RX_UNDER     BIT(0)
+
+/* DW_IC_ENABLE fields */
+#define DW_IC_ENABLE_TX_CMD_BLOCK  BIT(2)
+#define DW_IC_ENABLE_ABORT         BIT(1)
+#define DW_IC_ENABLE_ENABLE        BIT(0)
+
+/* DW_IC_STATUS fields */
+#define DW_IC_STATUS_SLV_ACTIVITY  BIT(6)
+#define DW_IC_STATUS_MST_ACTIVITY  BIT(5)
+#define DW_IC_STATUS_RFF           BIT(4)
+#define DW_IC_STATUS_RFNE          BIT(3)
+#define DW_IC_STATUS_TFE           BIT(2)
+#define DW_IC_STATUS_TFNF          BIT(1)
+#define DW_IC_STATUS_ACTIVITY      BIT(0)
+
+/* DW_IC_TX_ABRT_SOURCE fields */
+#define DW_IC_TX_TX_FLUSH_CNT          extract32((rv), 23, 9)
+#define DW_IC_TX_ABRT_USER_ABRT        BIT(16)
+#define DW_IC_TX_ABRT_SLVRD_INTX       BIT(15)
+#define DW_IC_TX_ABRT_SLV_ARBLOST      BIT(14)
+#define DW_IC_TX_ABRT_SLVFLUSH_TXFIFO  BIT(13)
+#define DW_IC_TX_ARB_LOST              BIT(12)
+#define DW_IC_TX_ABRT_MASTER_DIS       BIT(11)
+#define DW_IC_TX_ABRT_10B_RD_NORSTRT   BIT(10)
+#define DW_IC_TX_ABRT_SBYTE_NORSTRT    BIT(9)
+#define DW_IC_TX_ABRT_HS_NORSTRT       BIT(8)
+#define DW_IC_TX_ABRT_SBYTE_ACKDET     BIT(7)
+#define DW_IC_TX_ABRT_HS_ACKDET        BIT(6)
+#define DW_IC_TX_ABRT_GCALL_READ       BIT(5)
+#define DW_IC_TX_ABRT_GCALL_NOACK      BIT(4)
+#define DW_IC_TX_ABRT_TXDATA_NOACK     BIT(3)
+#define DW_IC_TX_ABRT_10ADDR2_NOACK    BIT(2)
+#define DW_IC_TX_ABRT_10ADDR1_NOACK    BIT(1)
+#define DW_IC_TX_ABRT_7B_ADDR_NOACK    BIT(0)
+
+
+/* IC_ENABLE_STATUS fields */
+#define DW_IC_ENABLE_STATUS_SLV_RX_DATA_LOST         BIT(2)
+#define DW_IC_ENABLE_STATUS_SLV_DISABLED_WHILE_BUSY  BIT(1)
+#define DW_IC_ENABLE_STATUS_IC_EN                    BIT(0)
+
+/* Masks for writable registers. */
+#define DW_IC_CON_MASK          0x000003ff
+#define DW_IC_TAR_MASK          0x00000fff
+#define DW_IC_SAR_MASK          0x000003ff
+#define DW_IC_SS_SCL_HCNT_MASK  0x0000ffff
+#define DW_IC_SS_SCL_LCNT_MASK  0x0000ffff
+#define DW_IC_FS_SCL_HCNT_MASK  0x0000ffff
+#define DW_IC_FS_SCL_LCNT_MASK  0x0000ffff
+#define DW_IC_INTR_MASK_MASK    0x00001fff
+#define DW_IC_ENABLE_MASK       0x00000007
+#define DW_IC_SDA_HOLD_MASK     0x00ffffff
+#define DW_IC_SDA_SETUP_MASK    0x000000ff
+#define DW_IC_FS_SPKLEN_MASK    0x000000ff
+
+/* Reset values */
+#define DW_IC_CON_INIT_VAL          0x7d
+#define DW_IC_TAR_INIT_VAL          0x1055
+#define DW_IC_SAR_INIT_VAL          0x55
+#define DW_IC_SS_SCL_HCNT_INIT_VAL  0x190
+#define DW_IC_SS_SCL_LCNT_INIT_VAL  0x1d6
+#define DW_IC_FS_SCL_HCNT_INIT_VAL  0x3c
+#define DW_IC_FS_SCL_LCNT_INIT_VAL  0x82
+#define DW_IC_INTR_MASK_INIT_VAL    0x8ff
+#define DW_IC_STATUS_INIT_VAL       0x6
+#define DW_IC_SDA_HOLD_INIT_VAL     0x1
+#define DW_IC_SDA_SETUP_INIT_VAL    0x64
+#define DW_IC_FS_SPKLEN_INIT_VAL    0x2
+
+#define DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS   BIT(7)
+#define DW_IC_COMP_PARAM_1_HAS_DMA              0 /* bit 6 - DMA disabled. */
+#define DW_IC_COMP_PARAM_1_INTR_IO              BIT(5)
+#define DW_IC_COMP_PARAM_1_HC_COUNT_VAL         0 /* bit 4 - disabled */
+#define DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE      (BIT(2) | BIT(3))
+#define DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32    BIT(1) /* bits 0, 1 */
+#define DW_IC_COMP_PARAM_1_INIT_VAL             \
+    (DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32 | \
+    DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE | \
+    DW_IC_COMP_PARAM_1_HC_COUNT_VAL | \
+    DW_IC_COMP_PARAM_1_INTR_IO | \
+    DW_IC_COMP_PARAM_1_HAS_DMA | \
+    DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS | \
+    ((DESIGNWARE_I2C_RX_FIFO_SIZE - 1) << 8) | \
+    ((DESIGNWARE_I2C_TX_FIFO_SIZE - 1) << 16))
+#define DW_IC_COMP_VERSION_INIT_VAL             0x3132302a
+#define DW_IC_COMP_TYPE_INIT_VAL                0x44570140
+
+static void dw_i2c_update_irq(DesignWareI2CState *s)
+{
+    int level;
+    uint32_t intr = s->ic_raw_intr_stat & s->ic_intr_mask;
+
+    level = !!((intr & DW_IC_INTR_RX_UNDER) |
+        (intr & DW_IC_INTR_RX_OVER) |
+        (intr & DW_IC_INTR_RX_FULL) |
+        (intr & DW_IC_INTR_TX_OVER) |
+        (intr & DW_IC_INTR_TX_EMPTY) |
+        (intr & DW_IC_INTR_RD_REQ) |
+        (intr & DW_IC_INTR_TX_ABRT) |
+        (intr & DW_IC_INTR_RX_DONE) |
+        (intr & DW_IC_INTR_ACTIVITY) |
+        (intr & DW_IC_INTR_STOP_DET) |
+        (intr & DW_IC_INTR_START_DET) |
+        (intr & DW_IC_INTR_GEN_CALL) |
+        (intr & DW_IC_INTR_RESTART_DET)
+        );
+    qemu_set_irq(s->irq, level);
+}
+
+static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
+{
+    uint32_t value = s->rx_fifo[s->rx_cur];
+
+    if (s->status != DW_I2C_STATUS_RECEIVING) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%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;
+            dw_i2c_update_irq(s);
+        }
+        return 0;
+    }
+
+    s->rx_cur = (s->rx_cur + 1) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+
+    if (s->ic_rxflr > 0) {
+        s->ic_rxflr--;
+    } else {
+        s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
+        dw_i2c_update_irq(s);
+        return 0;
+    }
+
+    if (s->ic_rxflr <= s->ic_rx_tl) {
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
+        dw_i2c_update_irq(s);
+    }
+
+    return value;
+}
+
+static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
+{
+    uint64_t value = 0;
+
+    DesignWareI2CState *s = opaque;
+
+    switch (offset) {
+    case DW_IC_CON:
+        value = s->ic_con;
+        break;
+    case DW_IC_TAR:
+        value = s->ic_tar;
+        break;
+    case DW_IC_SAR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_sar\n",
+                      DEVICE(s)->canonical_path);
+        value = s->ic_sar;
+        break;
+    case DW_IC_DATA_CMD:
+        value = dw_i2c_read_ic_data_cmd(s);
+        break;
+    case DW_IC_SS_SCL_HCNT:
+        value = s->ic_ss_scl_hcnt;
+        break;
+    case DW_IC_SS_SCL_LCNT:
+        value = s->ic_ss_scl_lcnt;
+        break;
+    case DW_IC_FS_SCL_HCNT:
+        value = s->ic_fs_scl_hcnt;
+        break;
+    case DW_IC_FS_SCL_LCNT:
+        value = s->ic_fs_scl_lcnt;
+        break;
+    case DW_IC_INTR_STAT:
+        value = s->ic_raw_intr_stat & s->ic_intr_mask;
+        break;
+    case DW_IC_INTR_MASK:
+        value = s->ic_intr_mask;
+        break;
+    case DW_IC_RAW_INTR_STAT:
+        value = s->ic_raw_intr_stat;
+        break;
+    case DW_IC_RX_TL:
+        value = s->ic_rx_tl;
+        break;
+    case DW_IC_TX_TL:
+        value = s->ic_tx_tl;
+        break;
+    case DW_IC_CLR_INTR:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL |
+            DW_IC_INTR_RESTART_DET |
+            DW_IC_INTR_START_DET |
+            DW_IC_INTR_STOP_DET |
+            DW_IC_INTR_ACTIVITY |
+            DW_IC_INTR_RX_DONE |
+            DW_IC_INTR_TX_ABRT |
+            DW_IC_INTR_RD_REQ |
+            DW_IC_INTR_TX_OVER |
+            DW_IC_INTR_RX_OVER |
+            DW_IC_INTR_RX_UNDER);
+        s->ic_tx_abrt_source = 0;
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_RX_UNDER:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_UNDER);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_RX_OVER:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_OVER);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_TX_OVER:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_OVER);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_RD_REQ:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RD_REQ);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_TX_ABRT:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_ABRT);
+        s->ic_tx_abrt_source = 0;
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_RX_DONE:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_DONE);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_ACTIVITY:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_ACTIVITY);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_STOP_DET:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_STOP_DET);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_START_DET:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_START_DET);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_CLR_GEN_CALL:
+        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL);
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_ENABLE:
+        value = s->ic_enable;
+        break;
+    case DW_IC_STATUS:
+        value = s->ic_status;
+        break;
+    case DW_IC_TXFLR:
+        value = s->ic_txflr;
+        break;
+    case DW_IC_RXFLR:
+        value = s->ic_rxflr;
+        break;
+    case DW_IC_SDA_HOLD:
+        value = s->ic_sda_hold;
+        break;
+    case DW_IC_TX_ABRT_SOURCE:
+        value = s->ic_tx_abrt_source;
+        break;
+    case DW_IC_SLV_DATA_NACK_ONLY:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unsupported read - ic_slv_data_nack_only\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_CR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_cr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_TDLR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_tdlr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_RDLR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_rdlr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_SDA_SETUP:
+        value = s->ic_sda_setup;
+        break;
+    case DW_IC_ACK_GENERAL_CALL:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_ack_general_call\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_ENABLE_STATUS:
+        value = s->ic_enable_status;
+        break;
+    case DW_IC_FS_SPKLEN:
+        value = s->ic_fs_spklen;
+        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;
+        break;
+    case DW_IC_COMP_VERSION:
+        value = s->ic_comp_version;
+        break;
+    case DW_IC_COMP_TYPE:
+        value = s->ic_comp_type;
+        break;
+
+    /* This register is invalid at this point. */
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
+                      DEVICE(s)->canonical_path, offset);
+        break;
+    }
+
+    trace_dw_i2c_read(DEVICE(s)->canonical_path, offset, value);
+
+    return value;
+}
+
+static void dw_i2c_write_ic_con(DesignWareI2CState *s, uint32_t value)
+{
+    if (value & DW_IC_CON_RX_FIFO_FULL_HLD_CTRL) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unsupported ic_con flag - RX_FIFO_FULL_HLD_CTRL\n",
+                      DEVICE(s)->canonical_path);
+    }
+
+    if (!(s->ic_enable & DW_IC_ENABLE_ENABLE)) {
+        s->ic_con = value & DW_IC_CON_MASK;
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid setting to ic_con %d when ic_enable[0]==1\n",
+                      DEVICE(s)->canonical_path, value);
+    }
+}
+
+static void dw_i2c_reset_to_idle(DesignWareI2CState *s)
+{
+        s->ic_enable_status &= ~DW_IC_ENABLE_STATUS_IC_EN;
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_OVER;
+        s->ic_rxflr = 0;
+        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
+        s->status = DW_I2C_STATUS_IDLE;
+        dw_i2c_update_irq(s);
+}
+
+static void dw_ic_tx_abort(DesignWareI2CState *s, uint32_t src)
+{
+    s->ic_tx_abrt_source |= src;
+    s->ic_raw_intr_stat |= DW_IC_INTR_TX_ABRT;
+    dw_i2c_reset_to_idle(s);
+    dw_i2c_update_irq(s);
+}
+
+static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
+{
+    int recv = !!(value & DW_IC_DATA_CMD_CMD);
+
+    if (s->status == DW_I2C_STATUS_IDLE ||
+        s->ic_raw_intr_stat & DW_IC_INTR_TX_ABRT) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Attempted to write to TX fifo when it is held in "
+                      "reset.\n", DEVICE(s)->canonical_path);
+        return;
+    }
+
+    /* Send the address if it hasn't been sent yet. */
+    if (s->status == DW_I2C_STATUS_SENDING_ADDRESS) {
+        int rv = i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv);
+        if (rv) {
+            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
+            return;
+        }
+        s->status = recv ? DW_I2C_STATUS_RECEIVING : DW_I2C_STATUS_SENDING;
+    }
+
+    /* Send data */
+    if (!recv) {
+        int rv = i2c_send(s->bus, DW_IC_DATA_CMD_DAT(value));
+        if (rv) {
+            i2c_end_transfer(s->bus);
+            dw_ic_tx_abort(s, DW_IC_TX_ABRT_TXDATA_NOACK);
+            return;
+        }
+        dw_i2c_update_irq(s);
+    }
+
+    /* Restart command */
+    if (value & DW_IC_DATA_CMD_RESTART && s->ic_con & DW_IC_CON_IC_RESTART_EN) {
+        s->ic_raw_intr_stat |= DW_IC_INTR_RESTART_DET |
+                               DW_IC_INTR_START_DET |
+                               DW_IC_INTR_ACTIVITY;
+        s->ic_status |= DW_IC_STATUS_ACTIVITY;
+        dw_i2c_update_irq(s);
+
+        if (i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv)) {
+            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
+            return;
+        }
+
+        s->status = recv ? DW_I2C_STATUS_RECEIVING : DW_I2C_STATUS_SENDING;
+    }
+
+    /* Receive data */
+    if (recv) {
+        uint8_t pos = (s->rx_cur + s->ic_rxflr) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+
+        if (s->ic_rxflr < DESIGNWARE_I2C_RX_FIFO_SIZE) {
+            s->rx_fifo[pos] = i2c_recv(s->bus);
+            s->ic_rxflr++;
+        } else {
+            s->ic_raw_intr_stat |= DW_IC_INTR_RX_OVER;
+            dw_i2c_update_irq(s);
+        }
+
+        if (s->ic_rxflr > s->ic_rx_tl) {
+            s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
+            dw_i2c_update_irq(s);
+        }
+        if (value & DW_IC_DATA_CMD_STOP) {
+            i2c_nack(s->bus);
+        }
+    }
+
+    /* Stop command */
+    if (value & DW_IC_DATA_CMD_STOP) {
+        s->ic_raw_intr_stat |= DW_IC_INTR_STOP_DET;
+        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
+        i2c_end_transfer(s->bus);
+        dw_i2c_update_irq(s);
+    }
+}
+
+static void dw_i2c_write_ic_enable(DesignWareI2CState *s, uint32_t value)
+{
+    if (value & DW_IC_ENABLE_ENABLE && !(s->ic_con & DW_IC_CON_SLAVE_DISABLE)) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Designware I2C slave mode is not supported.\n",
+                      DEVICE(s)->canonical_path);
+        return;
+    }
+
+    s->ic_enable = value & DW_IC_ENABLE_MASK;
+
+    if (value & DW_IC_ENABLE_ABORT || value & DW_IC_ENABLE_TX_CMD_BLOCK) {
+        dw_ic_tx_abort(s, DW_IC_TX_ABRT_USER_ABRT);
+        return;
+    }
+
+    if (value & DW_IC_ENABLE_ENABLE) {
+        s->ic_enable_status |= DW_IC_ENABLE_STATUS_IC_EN;
+        s->ic_status |= DW_IC_STATUS_ACTIVITY;
+        s->ic_raw_intr_stat |= DW_IC_INTR_ACTIVITY |
+                               DW_IC_INTR_START_DET |
+                               DW_IC_INTR_TX_EMPTY;
+        s->status = DW_I2C_STATUS_SENDING_ADDRESS;
+        dw_i2c_update_irq(s);
+    } else if ((value & DW_IC_ENABLE_ENABLE) == 0) {
+        dw_i2c_reset_to_idle(s);
+    }
+
+}
+
+static void dw_i2c_write_ic_rx_tl(DesignWareI2CState *s, uint32_t value)
+{
+    /* Note that a value of 0 for ic_rx_tl indicates a threashold of 1. */
+    if (value > DESIGNWARE_I2C_RX_FIFO_SIZE - 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid setting to ic_rx_tl %d\n",
+                      DEVICE(s)->canonical_path, value);
+        s->ic_rx_tl = DESIGNWARE_I2C_RX_FIFO_SIZE - 1;
+    } else {
+        s->ic_rx_tl = value;
+    }
+
+    if (s->ic_rxflr > s->ic_rx_tl && s->ic_enable & DW_IC_ENABLE_ENABLE) {
+        s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
+    } else {
+        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
+    }
+    dw_i2c_update_irq(s);
+}
+
+static void dw_i2c_write_ic_tx_tl(DesignWareI2CState *s, uint32_t value)
+{
+    /*
+     * Note that a value of 0 for ic_tx_tl indicates a threashold of 1.
+     * However, the tx threshold is not used in the model because commands are
+     * always sent out as soon as they are written.
+     */
+    if (value > DESIGNWARE_I2C_TX_FIFO_SIZE - 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid setting to ic_tx_tl %d\n",
+                      DEVICE(s)->canonical_path, value);
+        s->ic_tx_tl = DESIGNWARE_I2C_TX_FIFO_SIZE - 1;
+    } else {
+        s->ic_tx_tl = value;
+    }
+}
+
+static void dw_i2c_write(void *opaque, hwaddr offset, uint64_t value,
+                              unsigned size)
+{
+    DesignWareI2CState *s = opaque;
+
+    trace_dw_i2c_write(DEVICE(s)->canonical_path, offset, value);
+
+    /* The order of the registers are their order in memory. */
+    switch (offset) {
+    case DW_IC_CON:
+        dw_i2c_write_ic_con(s, value);
+        break;
+    case DW_IC_TAR:
+        s->ic_tar = value & DW_IC_TAR_MASK;
+        break;
+    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;
+        break;
+    case DW_IC_DATA_CMD:
+        dw_i2c_write_ic_data_cmd(s, value);
+        break;
+    case DW_IC_SS_SCL_HCNT:
+        s->ic_ss_scl_hcnt = value & DW_IC_SS_SCL_HCNT_MASK;
+        break;
+    case DW_IC_SS_SCL_LCNT:
+        s->ic_ss_scl_lcnt = value & DW_IC_SS_SCL_LCNT_MASK;
+        break;
+    case DW_IC_FS_SCL_HCNT:
+        s->ic_fs_scl_hcnt = value & DW_IC_FS_SCL_HCNT_MASK;
+        break;
+    case DW_IC_FS_SCL_LCNT:
+        s->ic_fs_scl_lcnt = value & DW_IC_FS_SCL_LCNT_MASK;
+        break;
+    case DW_IC_INTR_MASK:
+        s->ic_intr_mask = value & DW_IC_INTR_MASK_MASK;
+        dw_i2c_update_irq(s);
+        break;
+    case DW_IC_RX_TL:
+        dw_i2c_write_ic_rx_tl(s, value);
+        break;
+    case DW_IC_TX_TL:
+        dw_i2c_write_ic_tx_tl(s, value);
+        break;
+    case DW_IC_ENABLE:
+        dw_i2c_write_ic_enable(s, value);
+        break;
+    case DW_IC_SDA_HOLD:
+        s->ic_sda_hold = value & DW_IC_SDA_HOLD_MASK;
+        break;
+    case DW_IC_SLV_DATA_NACK_ONLY:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unsupported write - ic_slv_data_nack_only\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_CR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_cr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_TDLR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_tdlr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_DMA_RDLR:
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_rdlr\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_SDA_SETUP:
+        s->ic_sda_setup = value & DW_IC_SDA_SETUP_MASK;
+        break;
+    case DW_IC_ACK_GENERAL_CALL:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unsupported write - ic_ack_general_call\n",
+                      DEVICE(s)->canonical_path);
+        break;
+    case DW_IC_FS_SPKLEN:
+        s->ic_fs_spklen = value & DW_IC_FS_SPKLEN_MASK;
+        break;
+
+    /* This register is invalid at this point. */
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: write to invalid offset or readonly register 0x%"
+                      HWADDR_PRIx "\n",
+                      DEVICE(s)->canonical_path, offset);
+        break;
+    }
+}
+
+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,
+        .unaligned = false,
+    },
+};
+
+static void designware_i2c_enter_reset(Object *obj, ResetType type)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(obj);
+
+    s->ic_con = DW_IC_CON_INIT_VAL;
+    s->ic_tar = DW_IC_TAR_INIT_VAL;
+    s->ic_sar = DW_IC_SAR_INIT_VAL;
+    s->ic_ss_scl_hcnt = DW_IC_SS_SCL_HCNT_INIT_VAL;
+    s->ic_ss_scl_lcnt = DW_IC_SS_SCL_LCNT_INIT_VAL;
+    s->ic_fs_scl_hcnt = DW_IC_FS_SCL_HCNT_INIT_VAL;
+    s->ic_fs_scl_lcnt = DW_IC_FS_SCL_LCNT_INIT_VAL;
+    s->ic_intr_mask = DW_IC_INTR_MASK_INIT_VAL;
+    s->ic_raw_intr_stat = 0;
+    s->ic_rx_tl = 0;
+    s->ic_tx_tl = 0;
+    s->ic_enable = 0;
+    s->ic_status = DW_IC_STATUS_INIT_VAL;
+    s->ic_txflr = 0;
+    s->ic_rxflr = 0;
+    s->ic_sda_hold = DW_IC_SDA_HOLD_INIT_VAL;
+    s->ic_tx_abrt_source = 0;
+    s->ic_sda_setup = DW_IC_SDA_SETUP_INIT_VAL;
+    s->ic_enable_status = 0;
+    s->ic_fs_spklen = DW_IC_FS_SPKLEN_INIT_VAL;
+    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;
+
+    s->rx_cur = 0;
+    s->status = DW_I2C_STATUS_IDLE;
+}
+
+static void designware_i2c_hold_reset(Object *obj, ResetType type)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(obj);
+
+    qemu_irq_lower(s->irq);
+}
+
+static const VMStateDescription vmstate_designware_i2c = {
+    .name = TYPE_DESIGNWARE_I2C,
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT32(ic_con, DesignWareI2CState),
+        VMSTATE_UINT32(ic_tar, DesignWareI2CState),
+        VMSTATE_UINT32(ic_sar, DesignWareI2CState),
+        VMSTATE_UINT32(ic_ss_scl_hcnt, DesignWareI2CState),
+        VMSTATE_UINT32(ic_ss_scl_lcnt, DesignWareI2CState),
+        VMSTATE_UINT32(ic_fs_scl_hcnt, DesignWareI2CState),
+        VMSTATE_UINT32(ic_fs_scl_lcnt, DesignWareI2CState),
+        VMSTATE_UINT32(ic_intr_mask, DesignWareI2CState),
+        VMSTATE_UINT32(ic_raw_intr_stat, DesignWareI2CState),
+        VMSTATE_UINT32(ic_rx_tl, DesignWareI2CState),
+        VMSTATE_UINT32(ic_tx_tl, DesignWareI2CState),
+        VMSTATE_UINT32(ic_enable, DesignWareI2CState),
+        VMSTATE_UINT32(ic_status, DesignWareI2CState),
+        VMSTATE_UINT32(ic_txflr, DesignWareI2CState),
+        VMSTATE_UINT32(ic_rxflr, DesignWareI2CState),
+        VMSTATE_UINT32(ic_sda_hold, DesignWareI2CState),
+        VMSTATE_UINT32(ic_tx_abrt_source, DesignWareI2CState),
+        VMSTATE_UINT32(ic_sda_setup, DesignWareI2CState),
+        VMSTATE_UINT32(ic_enable_status, DesignWareI2CState),
+        VMSTATE_UINT32(ic_fs_spklen, DesignWareI2CState),
+        VMSTATE_UINT32(ic_comp_param_1, DesignWareI2CState),
+        VMSTATE_UINT32(ic_comp_version, DesignWareI2CState),
+        VMSTATE_UINT32(ic_comp_type, DesignWareI2CState),
+        VMSTATE_UINT32(status, DesignWareI2CState),
+        VMSTATE_UINT8_ARRAY(rx_fifo, DesignWareI2CState,
+                        DESIGNWARE_I2C_RX_FIFO_SIZE),
+        VMSTATE_UINT8(rx_cur, DesignWareI2CState),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+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);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
+}
+
+static void designware_i2c_class_init(ObjectClass *klass, const void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "Designware I2C";
+    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[] = {
+    {
+        .name = TYPE_DESIGNWARE_I2C,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(DesignWareI2CState),
+        .class_init = designware_i2c_class_init,
+        .instance_init = designware_i2c_smbus_init,
+    },
+};
+DEFINE_TYPES(designware_i2c_types);
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index c459adcb59..88aea35662 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -11,6 +11,7 @@ i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
 i2c_ss.add(when: 'CONFIG_ALLWINNER_I2C', if_true: files('allwinner-i2c.c'))
 i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
 i2c_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_smbus.c'))
+i2c_ss.add(when: 'CONFIG_DESIGNWARE_I2C', if_true: files('designware_i2c.c'))
 i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
 i2c_ss.add(when: 'CONFIG_ARM_SBCON_I2C', if_true: files('arm_sbcon_i2c.c'))
 i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 1ad0e95c0e..8a78d2d3c8 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -61,3 +61,7 @@ pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
 
 imx_i2c_read(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] -> 0x%02" PRIx64
 imx_i2c_write(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] <- 0x%02" PRIx64
+
+# designware_i2c.c
+dw_i2c_read(const char *id, uint64_t ofs, uint64_t value) "%s: offset 0x%02" PRIx64 " -> value: 0x%02" PRIx64
+dw_i2c_write(const char *id, uint64_t ofs, uint64_t value) "%s: offset: 0x%02" PRIx64 " <- value: 0x%02" PRIx64
diff --git a/include/hw/i2c/designware_i2c.h b/include/hw/i2c/designware_i2c.h
new file mode 100644
index 0000000000..71d74c9141
--- /dev/null
+++ b/include/hw/i2c/designware_i2c.h
@@ -0,0 +1,101 @@
+/*
+ * DesignWare I2C Module.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef DESIGNWARE_I2C_H
+#define DESIGNWARE_I2C_H
+
+#include "hw/i2c/i2c.h"
+#include "hw/core/irq.h"
+#include "hw/core/sysbus.h"
+
+/* Size of the FIFO buffers. */
+#define DESIGNWARE_I2C_RX_FIFO_SIZE 16
+#define DESIGNWARE_I2C_TX_FIFO_SIZE 16
+
+typedef enum DesignWareI2CStatus {
+    DW_I2C_STATUS_IDLE,
+    DW_I2C_STATUS_SENDING_ADDRESS,
+    DW_I2C_STATUS_SENDING,
+    DW_I2C_STATUS_RECEIVING,
+} DesignWareI2CStatus;
+
+/*
+ * struct DesignWareI2CState - DesignWare I2C device state.
+ * @bus: The underlying I2C Bus
+ * @irq: GIC interrupt line to fire on events
+ * @ic_con: : I2C control register
+ * @ic_tar: I2C target address register
+ * @ic_sar: I2C slave address register
+ * @ic_ss_scl_hcnt: Standard speed i2c clock scl high count register
+ * @ic_ss_scl_lcnt: Standard speed i2c clock scl low count register
+ * @ic_fs_scl_hcnt: Fast mode or fast mode plus i2c clock scl high count
+ *                  register
+ * @ic_fs_scl_lcnt:Fast mode or fast mode plus i2c clock scl low count
+ *                  register
+ * @ic_intr_mask: I2C Interrupt Mask Register
+ * @ic_raw_intr_stat: I2C raw interrupt status register
+ * @ic_rx_tl: I2C receive FIFO threshold register
+ * @ic_tx_tl: I2C transmit FIFO threshold register
+ * @ic_enable: I2C enable register
+ * @ic_status: I2C status register
+ * @ic_txflr: I2C transmit fifo level register
+ * @ic_rxflr: I2C receive fifo level register
+ * @ic_sda_hold: I2C SDA hold time length register
+ * @ic_tx_abrt_source: The I2C transmit abort source register
+ * @ic_sda_setup: I2C SDA setup register
+ * @ic_enable_status: I2C enable status register
+ * @ic_fs_spklen: I2C SS, FS or FM+ spike suppression limit
+ * @ic_comp_param_1: Component parameter register
+ * @ic_comp_version: I2C component version register
+ * @ic_comp_type: I2C component type register
+ * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
+ * @rx_cur: The current position of rx_fifo.
+ * @status: The current status of the SMBus.
+ */
+typedef struct DesignWareI2CState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+
+    I2CBus      *bus;
+    qemu_irq     irq;
+
+    uint32_t ic_con;
+    uint32_t ic_tar;
+    uint32_t ic_sar;
+    uint32_t ic_ss_scl_hcnt;
+    uint32_t ic_ss_scl_lcnt;
+    uint32_t ic_fs_scl_hcnt;
+    uint32_t ic_fs_scl_lcnt;
+    uint32_t ic_intr_mask;
+    uint32_t ic_raw_intr_stat;
+    uint32_t ic_rx_tl;
+    uint32_t ic_tx_tl;
+    uint32_t ic_enable;
+    uint32_t ic_status;
+    uint32_t ic_txflr;
+    uint32_t ic_rxflr;
+    uint32_t ic_sda_hold;
+    uint32_t ic_tx_abrt_source;
+    uint32_t ic_sda_setup;
+    uint32_t ic_enable_status;
+    uint32_t ic_fs_spklen;
+    uint32_t ic_comp_param_1;
+    uint32_t ic_comp_version;
+    uint32_t ic_comp_type;
+
+    uint8_t      rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
+    uint8_t      rx_cur;
+
+    DesignWareI2CStatus status;
+} DesignWareI2CState;
+
+#define TYPE_DESIGNWARE_I2C "designware-i2c"
+#define DESIGNWARE_I2C(obj) OBJECT_CHECK(DesignWareI2CState, (obj), \
+                                        TYPE_DESIGNWARE_I2C)
+
+#endif /* DESIGNWARE_I2C_H */
diff --git a/roms/seabios-hppa b/roms/seabios-hppa
index d9560852a3..1a8ada1fb7 160000
--- a/roms/seabios-hppa
+++ b/roms/seabios-hppa
@@ -1 +1 @@
-Subproject commit d9560852a34f156155b3777745baa0d96d553f22
+Subproject commit 1a8ada1fb70643172e251aacbac673c9ecda99e9
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8
  2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
  2026-05-07 12:05 ` [PATCH 1/4] " Nicholas Piggin
@ 2026-05-07 12:05 ` Nicholas Piggin
  2026-05-11 10:18   ` Philippe Mathieu-Daudé
  2026-05-07 12:05 ` [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API Nicholas Piggin
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-07 12:05 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Nicholas Piggin, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

Alistair suggested moving to Fifo8, which I think is
a good improvement.

Broken out for individual review, but IMO we should
squash before merge since it changes VMState format.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/i2c/designware_i2c.c         | 37 ++++++++++++++++++++-------------
 include/hw/i2c/designware_i2c.h |  7 +++----
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
index 1e5505f571..83d0968580 100644
--- a/hw/i2c/designware_i2c.c
+++ b/hw/i2c/designware_i2c.c
@@ -208,10 +208,8 @@ static void dw_i2c_update_irq(DesignWareI2CState *s)
     qemu_set_irq(s->irq, level);
 }
 
-static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
+static uint8_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
 {
-    uint32_t value = s->rx_fifo[s->rx_cur];
-
     if (s->status != DW_I2C_STATUS_RECEIVING) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Attempted to read from RX fifo when not in receive "
@@ -223,22 +221,21 @@ static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
         return 0;
     }
 
-    s->rx_cur = (s->rx_cur + 1) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+    g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
 
-    if (s->ic_rxflr > 0) {
-        s->ic_rxflr--;
-    } else {
+    if (fifo8_is_empty(&s->rx_fifo)) {
         s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
         dw_i2c_update_irq(s);
         return 0;
     }
 
+    s->ic_rxflr--;
     if (s->ic_rxflr <= s->ic_rx_tl) {
         s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
         dw_i2c_update_irq(s);
     }
 
-    return value;
+    return fifo8_pop(&s->rx_fifo);
 }
 
 static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
@@ -445,6 +442,7 @@ static void dw_i2c_reset_to_idle(DesignWareI2CState *s)
         s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
         s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_OVER;
         s->ic_rxflr = 0;
+        fifo8_reset(&s->rx_fifo);
         s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
         s->status = DW_I2C_STATUS_IDLE;
         dw_i2c_update_irq(s);
@@ -509,10 +507,10 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
 
     /* Receive data */
     if (recv) {
-        uint8_t pos = (s->rx_cur + s->ic_rxflr) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+        g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
 
-        if (s->ic_rxflr < DESIGNWARE_I2C_RX_FIFO_SIZE) {
-            s->rx_fifo[pos] = i2c_recv(s->bus);
+        if (!fifo8_is_full(&s->rx_fifo)) {
+            fifo8_push(&s->rx_fifo, i2c_recv(s->bus));
             s->ic_rxflr++;
         } else {
             s->ic_raw_intr_stat |= DW_IC_INTR_RX_OVER;
@@ -738,7 +736,8 @@ static void designware_i2c_enter_reset(Object *obj, ResetType type)
     s->ic_comp_version = DW_IC_COMP_VERSION_INIT_VAL;
     s->ic_comp_type = DW_IC_COMP_TYPE_INIT_VAL;
 
-    s->rx_cur = 0;
+    fifo8_reset(&s->rx_fifo);
+
     s->status = DW_I2C_STATUS_IDLE;
 }
 
@@ -777,10 +776,8 @@ static const VMStateDescription vmstate_designware_i2c = {
         VMSTATE_UINT32(ic_comp_param_1, DesignWareI2CState),
         VMSTATE_UINT32(ic_comp_version, DesignWareI2CState),
         VMSTATE_UINT32(ic_comp_type, DesignWareI2CState),
+        VMSTATE_FIFO8(rx_fifo, DesignWareI2CState),
         VMSTATE_UINT32(status, DesignWareI2CState),
-        VMSTATE_UINT8_ARRAY(rx_fifo, DesignWareI2CState,
-                        DESIGNWARE_I2C_RX_FIFO_SIZE),
-        VMSTATE_UINT8(rx_cur, DesignWareI2CState),
         VMSTATE_END_OF_LIST(),
     },
 };
@@ -790,6 +787,8 @@ static void designware_i2c_smbus_init(Object *obj)
     DesignWareI2CState *s = DESIGNWARE_I2C(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
+    fifo8_create(&s->rx_fifo, DESIGNWARE_I2C_RX_FIFO_SIZE);
+
     sysbus_init_irq(sbd, &s->irq);
 
     memory_region_init_io(&s->iomem, obj, &designware_i2c_ops, s,
@@ -799,6 +798,13 @@ static void designware_i2c_smbus_init(Object *obj)
     s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
 }
 
+static void designware_i2c_finalize(Object *obj)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(obj);
+
+    fifo8_destroy(&s->rx_fifo);
+}
+
 static void designware_i2c_class_init(ObjectClass *klass, const void *data)
 {
     ResettableClass *rc = RESETTABLE_CLASS(klass);
@@ -819,6 +825,7 @@ static const TypeInfo designware_i2c_types[] = {
         .instance_size = sizeof(DesignWareI2CState),
         .class_init = designware_i2c_class_init,
         .instance_init = designware_i2c_smbus_init,
+	.instance_finalize = designware_i2c_finalize,
     },
 };
 DEFINE_TYPES(designware_i2c_types);
diff --git a/include/hw/i2c/designware_i2c.h b/include/hw/i2c/designware_i2c.h
index 71d74c9141..affaf983a2 100644
--- a/include/hw/i2c/designware_i2c.h
+++ b/include/hw/i2c/designware_i2c.h
@@ -8,6 +8,7 @@
 #ifndef DESIGNWARE_I2C_H
 #define DESIGNWARE_I2C_H
 
+#include "qemu/fifo8.h"
 #include "hw/i2c/i2c.h"
 #include "hw/core/irq.h"
 #include "hw/core/sysbus.h"
@@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
  * @ic_comp_version: I2C component version register
  * @ic_comp_type: I2C component type register
  * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
- * @rx_cur: The current position of rx_fifo.
- * @status: The current status of the SMBus.
  */
 typedef struct DesignWareI2CState {
     SysBusDevice parent_obj;
@@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
     uint32_t ic_comp_version;
     uint32_t ic_comp_type;
 
-    uint8_t      rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
-    uint8_t      rx_cur;
+    /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
+    Fifo8    rx_fifo;
 
     DesignWareI2CStatus status;
 } DesignWareI2CState;
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API
  2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
  2026-05-07 12:05 ` [PATCH 1/4] " Nicholas Piggin
  2026-05-07 12:05 ` [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8 Nicholas Piggin
@ 2026-05-07 12:05 ` Nicholas Piggin
  2026-05-13  2:03   ` Alistair Francis
  2026-05-07 12:05 ` [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK Nicholas Piggin
  2026-05-10 13:03 ` [PATCH 0/4] hw/i2c: Add designware i2c controller Corey Minyard
  4 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-07 12:05 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Nicholas Piggin, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

Alistair suggested moving to QEMU register API. I haven't used it before
but IMO it is an improvement. It is a lot of churn that is not very
review-able but unfortunately also changes VMState making it a pain to
do incrementally. If we decide to go this way, we should squash the
conversion patch when we're happy with it.

I tried not to rearrange the code too much, to reduce patch size. I
prefer putting register implementation all together rather than grouped
by read and write, which this API promotes. So if others like that style
then I will move that.

I didn't change to use all the FIELD APIs to set/clear bits, since
it worked out a bit easier to script and only a smal win since there is
little/no multibit fields and shifting/masking required.

Register traces of loading the kernel drivers for the 2 devices added on
the Atlantis machine in the series Joel posted earlier look identical
modulo RTC data reads (which I guess is due to time differences).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/i2c/Kconfig                  |    1 +
 hw/i2c/designware_i2c.c         | 1022 ++++++++++++++-----------------
 include/hw/i2c/designware_i2c.h |   56 +-
 3 files changed, 471 insertions(+), 608 deletions(-)

diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index d3f394edeb..0766130b59 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -20,6 +20,7 @@ config ARM_SBCON_I2C
 
 config DESIGNWARE_I2C
     bool
+    select REGISTER
     select I2C
 
 config ACPI_SMBUS
diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
index 83d0968580..b7be4d68c4 100644
--- a/hw/i2c/designware_i2c.c
+++ b/hw/i2c/designware_i2c.c
@@ -16,452 +16,312 @@
 #include "qemu/units.h"
 #include "trace.h"
 
-enum DesignWareI2CRegister {
-    DW_IC_CON                   = 0x00,
-    DW_IC_TAR                   = 0x04,
-    DW_IC_SAR                   = 0x08,
-    DW_IC_DATA_CMD              = 0x10,
-    DW_IC_SS_SCL_HCNT           = 0x14,
-    DW_IC_SS_SCL_LCNT           = 0x18,
-    DW_IC_FS_SCL_HCNT           = 0x1c,
-    DW_IC_FS_SCL_LCNT           = 0x20,
-    DW_IC_INTR_STAT             = 0x2c,
-    DW_IC_INTR_MASK             = 0x30,
-    DW_IC_RAW_INTR_STAT         = 0x34,
-    DW_IC_RX_TL                 = 0x38,
-    DW_IC_TX_TL                 = 0x3c,
-    DW_IC_CLR_INTR              = 0x40,
-    DW_IC_CLR_RX_UNDER          = 0x44,
-    DW_IC_CLR_RX_OVER           = 0x48,
-    DW_IC_CLR_TX_OVER           = 0x4c,
-    DW_IC_CLR_RD_REQ            = 0x50,
-    DW_IC_CLR_TX_ABRT           = 0x54,
-    DW_IC_CLR_RX_DONE           = 0x58,
-    DW_IC_CLR_ACTIVITY          = 0x5c,
-    DW_IC_CLR_STOP_DET          = 0x60,
-    DW_IC_CLR_START_DET         = 0x64,
-    DW_IC_CLR_GEN_CALL          = 0x68,
-    DW_IC_ENABLE                = 0x6c,
-    DW_IC_STATUS                = 0x70,
-    DW_IC_TXFLR                 = 0x74,
-    DW_IC_RXFLR                 = 0x78,
-    DW_IC_SDA_HOLD              = 0x7c,
-    DW_IC_TX_ABRT_SOURCE        = 0x80,
-    DW_IC_SLV_DATA_NACK_ONLY    = 0x84,
-    DW_IC_DMA_CR                = 0x88,
-    DW_IC_DMA_TDLR              = 0x8c,
-    DW_IC_DMA_RDLR              = 0x90,
-    DW_IC_SDA_SETUP             = 0x94,
-    DW_IC_ACK_GENERAL_CALL      = 0x98,
-    DW_IC_ENABLE_STATUS         = 0x9c,
-    DW_IC_FS_SPKLEN             = 0xa0,
-    DW_IC_CLR_RESTART_DET       = 0xa8,
-    DW_IC_COMP_PARAM_1          = 0xf4,
-    DW_IC_COMP_VERSION          = 0xf8,
-    DW_IC_COMP_TYPE             = 0xfc,
-};
-
-/* DW_IC_CON fields */
-#define DW_IC_CON_STOP_DET_IF_MASTER_ACTIV  BIT(10)
-#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL     BIT(9)
-#define DW_IC_CON_TX_EMPTY_CTRL             BIT(8)
-#define DW_IC_CON_STOP_IF_ADDRESSED         BIT(7)
-#define DW_IC_CON_SLAVE_DISABLE             BIT(6)
-#define DW_IC_CON_IC_RESTART_EN             BIT(5)
-#define DW_IC_CON_10BITADDR_MASTER          BIT(4)
-#define DW_IC_CON_10BITADDR_SLAVE           BIT(3)
-#define DW_IC_CON_SPEED(rv)                 extract32((rv), 1, 2)
-#define DW_IC_CON_MASTER_MODE               BIT(0)
-
-/* DW_IC_TAR fields */
-#define DW_IC_TAR_IC_10BITADDR_MASTER  BIT(12)
-#define DW_IC_TAR_SPECIAL              BIT(11)
-#define DW_IC_TAR_GC_OR_START          BIT(10)
-#define DW_IC_TAR_ADDRESS(rv)          extract32((rv), 0, 10)
-
-/* DW_IC_DATA_CMD fields */
-#define DW_IC_DATA_CMD_RESTART  BIT(10)
-#define DW_IC_DATA_CMD_STOP     BIT(9)
-#define DW_IC_DATA_CMD_CMD      BIT(8)
-#define DW_IC_DATA_CMD_DAT(rv)  extract32((rv), 0, 8)
-
-/* DW_IC_INTR_STAT/INTR_MASK/RAW_INTR_STAT fields */
-#define DW_IC_INTR_RESTART_DET  BIT(12)
-#define DW_IC_INTR_GEN_CALL     BIT(11)
-#define DW_IC_INTR_START_DET    BIT(10)
-#define DW_IC_INTR_STOP_DET     BIT(9)
-#define DW_IC_INTR_ACTIVITY     BIT(8)
-#define DW_IC_INTR_RX_DONE      BIT(7)
-#define DW_IC_INTR_TX_ABRT      BIT(6)
-#define DW_IC_INTR_RD_REQ       BIT(5)
-#define DW_IC_INTR_TX_EMPTY     BIT(4) /* Hardware clear only. */
-#define DW_IC_INTR_TX_OVER      BIT(3)
-#define DW_IC_INTR_RX_FULL      BIT(2) /* Hardware clear only. */
-#define DW_IC_INTR_RX_OVER      BIT(1)
-#define DW_IC_INTR_RX_UNDER     BIT(0)
-
-/* DW_IC_ENABLE fields */
-#define DW_IC_ENABLE_TX_CMD_BLOCK  BIT(2)
-#define DW_IC_ENABLE_ABORT         BIT(1)
-#define DW_IC_ENABLE_ENABLE        BIT(0)
-
-/* DW_IC_STATUS fields */
-#define DW_IC_STATUS_SLV_ACTIVITY  BIT(6)
-#define DW_IC_STATUS_MST_ACTIVITY  BIT(5)
-#define DW_IC_STATUS_RFF           BIT(4)
-#define DW_IC_STATUS_RFNE          BIT(3)
-#define DW_IC_STATUS_TFE           BIT(2)
-#define DW_IC_STATUS_TFNF          BIT(1)
-#define DW_IC_STATUS_ACTIVITY      BIT(0)
-
-/* DW_IC_TX_ABRT_SOURCE fields */
-#define DW_IC_TX_TX_FLUSH_CNT          extract32((rv), 23, 9)
-#define DW_IC_TX_ABRT_USER_ABRT        BIT(16)
-#define DW_IC_TX_ABRT_SLVRD_INTX       BIT(15)
-#define DW_IC_TX_ABRT_SLV_ARBLOST      BIT(14)
-#define DW_IC_TX_ABRT_SLVFLUSH_TXFIFO  BIT(13)
-#define DW_IC_TX_ARB_LOST              BIT(12)
-#define DW_IC_TX_ABRT_MASTER_DIS       BIT(11)
-#define DW_IC_TX_ABRT_10B_RD_NORSTRT   BIT(10)
-#define DW_IC_TX_ABRT_SBYTE_NORSTRT    BIT(9)
-#define DW_IC_TX_ABRT_HS_NORSTRT       BIT(8)
-#define DW_IC_TX_ABRT_SBYTE_ACKDET     BIT(7)
-#define DW_IC_TX_ABRT_HS_ACKDET        BIT(6)
-#define DW_IC_TX_ABRT_GCALL_READ       BIT(5)
-#define DW_IC_TX_ABRT_GCALL_NOACK      BIT(4)
-#define DW_IC_TX_ABRT_TXDATA_NOACK     BIT(3)
-#define DW_IC_TX_ABRT_10ADDR2_NOACK    BIT(2)
-#define DW_IC_TX_ABRT_10ADDR1_NOACK    BIT(1)
-#define DW_IC_TX_ABRT_7B_ADDR_NOACK    BIT(0)
-
-
-/* IC_ENABLE_STATUS fields */
-#define DW_IC_ENABLE_STATUS_SLV_RX_DATA_LOST         BIT(2)
-#define DW_IC_ENABLE_STATUS_SLV_DISABLED_WHILE_BUSY  BIT(1)
-#define DW_IC_ENABLE_STATUS_IC_EN                    BIT(0)
-
-/* Masks for writable registers. */
-#define DW_IC_CON_MASK          0x000003ff
-#define DW_IC_TAR_MASK          0x00000fff
-#define DW_IC_SAR_MASK          0x000003ff
-#define DW_IC_SS_SCL_HCNT_MASK  0x0000ffff
-#define DW_IC_SS_SCL_LCNT_MASK  0x0000ffff
-#define DW_IC_FS_SCL_HCNT_MASK  0x0000ffff
-#define DW_IC_FS_SCL_LCNT_MASK  0x0000ffff
-#define DW_IC_INTR_MASK_MASK    0x00001fff
-#define DW_IC_ENABLE_MASK       0x00000007
-#define DW_IC_SDA_HOLD_MASK     0x00ffffff
-#define DW_IC_SDA_SETUP_MASK    0x000000ff
-#define DW_IC_FS_SPKLEN_MASK    0x000000ff
-
-/* Reset values */
-#define DW_IC_CON_INIT_VAL          0x7d
-#define DW_IC_TAR_INIT_VAL          0x1055
-#define DW_IC_SAR_INIT_VAL          0x55
-#define DW_IC_SS_SCL_HCNT_INIT_VAL  0x190
-#define DW_IC_SS_SCL_LCNT_INIT_VAL  0x1d6
-#define DW_IC_FS_SCL_HCNT_INIT_VAL  0x3c
-#define DW_IC_FS_SCL_LCNT_INIT_VAL  0x82
-#define DW_IC_INTR_MASK_INIT_VAL    0x8ff
-#define DW_IC_STATUS_INIT_VAL       0x6
-#define DW_IC_SDA_HOLD_INIT_VAL     0x1
-#define DW_IC_SDA_SETUP_INIT_VAL    0x64
-#define DW_IC_FS_SPKLEN_INIT_VAL    0x2
-
-#define DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS   BIT(7)
-#define DW_IC_COMP_PARAM_1_HAS_DMA              0 /* bit 6 - DMA disabled. */
-#define DW_IC_COMP_PARAM_1_INTR_IO              BIT(5)
-#define DW_IC_COMP_PARAM_1_HC_COUNT_VAL         0 /* bit 4 - disabled */
-#define DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE      (BIT(2) | BIT(3))
-#define DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32    BIT(1) /* bits 0, 1 */
-#define DW_IC_COMP_PARAM_1_INIT_VAL             \
-    (DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32 | \
-    DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE | \
-    DW_IC_COMP_PARAM_1_HC_COUNT_VAL | \
-    DW_IC_COMP_PARAM_1_INTR_IO | \
-    DW_IC_COMP_PARAM_1_HAS_DMA | \
-    DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS | \
-    ((DESIGNWARE_I2C_RX_FIFO_SIZE - 1) << 8) | \
-    ((DESIGNWARE_I2C_TX_FIFO_SIZE - 1) << 16))
-#define DW_IC_COMP_VERSION_INIT_VAL             0x3132302a
-#define DW_IC_COMP_TYPE_INIT_VAL                0x44570140
+#ifndef DESIGNWARE_I2C_ERR_DEBUG
+#define DESIGNWARE_I2C_ERR_DEBUG 0
+#endif
+
+REG32(DW_IC_CON,                0x00) /* I2C control */
+    FIELD(DW_IC_CON, STOP_DET_IF_MASTER_ACTIV, 10, 1)
+    FIELD(DW_IC_CON, RX_FIFO_FULL_HLD_CTRL,     9, 1)
+    FIELD(DW_IC_CON, TX_EMPTY_CTRL,             8, 1)
+    FIELD(DW_IC_CON, STOP_IF_ADDRESSED,         7, 1)
+    FIELD(DW_IC_CON, SLAVE_DISABLE,             6, 1)
+    FIELD(DW_IC_CON, IC_RESTART_EN,             5, 1)
+    FIELD(DW_IC_CON, 10BITADDR_MASTER,          4, 1)
+    FIELD(DW_IC_CON, 10BITADDR_SLAVE,           3, 1)
+    FIELD(DW_IC_CON, SPEED,                     1, 2)
+    FIELD(DW_IC_CON, MASTER_MODE,               0, 1)
+REG32(DW_IC_TAR,                0x04) /* I2C target address */
+    FIELD(DW_IC_TAR, IC_10BITADDR_MASTER, 12,  1)
+    FIELD(DW_IC_TAR, SPECIAL,             11,  1)
+    FIELD(DW_IC_TAR, GC_OR_START,         10,  1)
+    FIELD(DW_IC_TAR, ADDRESS,              0, 10)
+REG32(DW_IC_SAR,                0x08) /* I2C slave address */
+REG32(DW_IC_DATA_CMD,           0x10)
+    FIELD(DW_IC_DATA_CMD, RESTART, 10, 1)
+    FIELD(DW_IC_DATA_CMD, STOP,     9, 1)
+    FIELD(DW_IC_DATA_CMD, CMD,      8, 1)
+    FIELD(DW_IC_DATA_CMD, DAT,      0, 8)
+REG32(DW_IC_SS_SCL_HCNT,        0x14) /* Standard speed i2c clock scl high count */
+REG32(DW_IC_SS_SCL_LCNT,        0x18) /* Standard speed i2c clock scl low count */
+REG32(DW_IC_FS_SCL_HCNT,        0x1c) /* Fast or fast plus i2c clock scl high count */
+REG32(DW_IC_FS_SCL_LCNT,        0x20) /* Fast or fast plus i2c clock scl low count */
+REG32(DW_IC_INTR_STAT,          0x2c)
+REG32(DW_IC_INTR_MASK,          0x30) /* I2C Interrupt Mask */
+REG32(DW_IC_RAW_INTR_STAT,      0x34) /* I2C raw interrupt status */
+    /* DW_IC_INTR_STAT/INTR_MASK/RAW_INTR_STAT fields */
+    SHARED_FIELD(DW_IC_INTR_RESTART_DET, 12, 1)
+    SHARED_FIELD(DW_IC_INTR_GEN_CALL,    11, 1)
+    SHARED_FIELD(DW_IC_INTR_START_DET,   10, 1)
+    SHARED_FIELD(DW_IC_INTR_STOP_DET,    9, 1)
+    SHARED_FIELD(DW_IC_INTR_ACTIVITY,    8, 1)
+    SHARED_FIELD(DW_IC_INTR_RX_DONE,     7, 1)
+    SHARED_FIELD(DW_IC_INTR_TX_ABRT,     6, 1)
+    SHARED_FIELD(DW_IC_INTR_RD_REQ,      5, 1)
+    SHARED_FIELD(DW_IC_INTR_TX_EMPTY,    4, 1) /* Hardware clear only. */
+    SHARED_FIELD(DW_IC_INTR_TX_OVER,     3, 1)
+    SHARED_FIELD(DW_IC_INTR_RX_FULL,     2, 1) /* Hardware clear only. */
+    SHARED_FIELD(DW_IC_INTR_RX_OVER,     1, 1)
+    SHARED_FIELD(DW_IC_INTR_RX_UNDER,    0, 1)
+
+#define DW_IC_INTR_ANY_MASK                \
+            (DW_IC_INTR_RESTART_DET_MASK | \
+             DW_IC_INTR_GEN_CALL_MASK    | \
+             DW_IC_INTR_START_DET_MASK   | \
+             DW_IC_INTR_STOP_DET_MASK    | \
+             DW_IC_INTR_ACTIVITY_MASK    | \
+             DW_IC_INTR_RX_DONE_MASK     | \
+             DW_IC_INTR_TX_ABRT_MASK     | \
+             DW_IC_INTR_RD_REQ_MASK      | \
+             DW_IC_INTR_TX_EMPTY_MASK    | \
+             DW_IC_INTR_TX_OVER_MASK     | \
+             DW_IC_INTR_RX_FULL_MASK     | \
+             DW_IC_INTR_RX_OVER_MASK     | \
+             DW_IC_INTR_RX_UNDER_MASK)
+
+#define DW_IC_INTR_ANY_SW_CLEAR_MASK       \
+            (DW_IC_INTR_ANY_MASK         & \
+            ~(DW_IC_INTR_TX_EMPTY_MASK   | \
+              DW_IC_INTR_RX_FULL_MASK))
+
+REG32(DW_IC_RX_TL,              0x38) /* I2C receive FIFO threshold */
+REG32(DW_IC_TX_TL,              0x3c) /* I2C transmit FIFO threshold */
+REG32(DW_IC_CLR_INTR,           0x40)
+REG32(DW_IC_CLR_RX_UNDER,       0x44)
+REG32(DW_IC_CLR_RX_OVER,        0x48)
+REG32(DW_IC_CLR_TX_OVER,        0x4c)
+REG32(DW_IC_CLR_RD_REQ,         0x50)
+REG32(DW_IC_CLR_TX_ABRT,        0x54)
+REG32(DW_IC_CLR_RX_DONE,        0x58)
+REG32(DW_IC_CLR_ACTIVITY,       0x5c)
+REG32(DW_IC_CLR_STOP_DET,       0x60)
+REG32(DW_IC_CLR_START_DET,      0x64)
+REG32(DW_IC_CLR_GEN_CALL,       0x68)
+REG32(DW_IC_ENABLE,             0x6c) /* I2C enable */
+    FIELD(DW_IC_ENABLE, TX_CMD_BLOCK, 2, 1)
+    FIELD(DW_IC_ENABLE, ABORT,        1, 1)
+    FIELD(DW_IC_ENABLE, ENABLE,       0, 1)
+REG32(DW_IC_STATUS,             0x70) /* I2C status */
+    FIELD(DW_IC_STATUS, SLV_ACTIVITY, 6, 1)
+    FIELD(DW_IC_STATUS, MST_ACTIVITY, 5, 1)
+    FIELD(DW_IC_STATUS, RFF,          4, 1)
+    FIELD(DW_IC_STATUS, RFNE,         3, 1)
+    FIELD(DW_IC_STATUS, TFE,          2, 1)
+    FIELD(DW_IC_STATUS, TFNF,         1, 1)
+    FIELD(DW_IC_STATUS, ACTIVITY,     0, 1)
+REG32(DW_IC_TXFLR,              0x74) /* I2C transmit fifo level */
+REG32(DW_IC_RXFLR,              0x78) /* I2C receive fifo level */
+REG32(DW_IC_SDA_HOLD,           0x7c) /* I2C SDA hold time length */
+REG32(DW_IC_TX_ABRT_SOURCE,     0x80) /* The I2C transmit abort source */
+    FIELD(DW_IC_TX_ABRT_SOURCE, USER_ABRT,       16, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, SLVRD_INTX,      15, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, SLV_ARBLOST,     14, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, SLVFLUSH_TXFIFO, 13, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, ARB_LOST,        12, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, MASTER_DIS,      11, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, 10B_RD_NORSTRT,  10, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, SBYTE_NORSTRT,   9, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, HS_NORSTRT,      8, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, SBYTE_ACKDET,    7, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, HS_ACKDET,       6, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, GCALL_READ,      5, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, GCALL_NOACK,     4, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, TXDATA_NOACK,    3, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, 10ADDR2_NOACK,   2, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, 10ADDR1_NOACK,   1, 1)
+    FIELD(DW_IC_TX_ABRT_SOURCE, 7B_ADDR_NOACK,   0, 1)
+REG32(DW_IC_SLV_DATA_NACK_ONLY, 0x84)
+REG32(DW_IC_DMA_CR,             0x88)
+REG32(DW_IC_DMA_TDLR,           0x8c)
+REG32(DW_IC_DMA_RDLR,           0x90)
+REG32(DW_IC_SDA_SETUP,          0x94) /* I2C SDA setup */
+REG32(DW_IC_ACK_GENERAL_CALL,   0x98)
+REG32(DW_IC_ENABLE_STATUS,      0x9c) /* I2C enable status */
+    FIELD(DW_IC_ENABLE_STATUS, SLV_RX_DATA_LOST,        2, 1)
+    FIELD(DW_IC_ENABLE_STATUS, SLV_DISABLED_WHILE_BUSY, 1, 1)
+    FIELD(DW_IC_ENABLE_STATUS, IC_EN,                   0, 1)
+REG32(DW_IC_FS_SPKLEN,          0xa0) /* I2C SS, FS or FM+ spike suppression limit */
+REG32(DW_IC_CLR_RESTART_DET,    0xa8)
+REG32(DW_IC_COMP_PARAM_1,       0xf4) /* Component parameter */
+    FIELD(DW_IC_COMP_PARAM_1, TX_FIFO_SIZE,       16, 8)
+    FIELD(DW_IC_COMP_PARAM_1, RX_FIFO_SIZE,        8, 8)
+    FIELD(DW_IC_COMP_PARAM_1, HAS_ENCODED_PARAMS,  7, 1)
+    FIELD(DW_IC_COMP_PARAM_1, HAS_DMA,             6, 1)
+    FIELD(DW_IC_COMP_PARAM_1, INTR_IO,             5, 1)
+    FIELD(DW_IC_COMP_PARAM_1, HC_COUNT_VAL,        4, 1)
+    FIELD(DW_IC_COMP_PARAM_1, HIGH_SPEED_MODE,     2, 2)
+    FIELD(DW_IC_COMP_PARAM_1, APB_DATA_WIDTH_32,   0, 2)
+REG32(DW_IC_COMP_VERSION,       0xf8) /* I2C component version */
+REG32(DW_IC_COMP_TYPE,          0xfc) /* I2C component type */
 
 static void dw_i2c_update_irq(DesignWareI2CState *s)
 {
-    int level;
-    uint32_t intr = s->ic_raw_intr_stat & s->ic_intr_mask;
-
-    level = !!((intr & DW_IC_INTR_RX_UNDER) |
-        (intr & DW_IC_INTR_RX_OVER) |
-        (intr & DW_IC_INTR_RX_FULL) |
-        (intr & DW_IC_INTR_TX_OVER) |
-        (intr & DW_IC_INTR_TX_EMPTY) |
-        (intr & DW_IC_INTR_RD_REQ) |
-        (intr & DW_IC_INTR_TX_ABRT) |
-        (intr & DW_IC_INTR_RX_DONE) |
-        (intr & DW_IC_INTR_ACTIVITY) |
-        (intr & DW_IC_INTR_STOP_DET) |
-        (intr & DW_IC_INTR_START_DET) |
-        (intr & DW_IC_INTR_GEN_CALL) |
-        (intr & DW_IC_INTR_RESTART_DET)
-        );
-    qemu_set_irq(s->irq, level);
+    uint32_t intr = s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];
+
+    qemu_set_irq(s->irq, !!(intr & DW_IC_INTR_ANY_MASK));
 }
 
-static uint8_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
+static uint64_t dw_ic_data_cmd_reg_post_read(RegisterInfo *reg, uint64_t value)
 {
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    g_assert(value == 0);
+
     if (s->status != DW_I2C_STATUS_RECEIVING) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%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;
+            SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT,
+                                    DW_IC_INTR_RX_UNDER, 1);
             dw_i2c_update_irq(s);
         }
         return 0;
     }
 
-    g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
+    g_assert(s->regs[R_DW_IC_RXFLR] == fifo8_num_used(&s->rx_fifo));
 
     if (fifo8_is_empty(&s->rx_fifo)) {
-        s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
+        SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT, DW_IC_INTR_RX_UNDER, 1);
         dw_i2c_update_irq(s);
         return 0;
     }
 
-    s->ic_rxflr--;
-    if (s->ic_rxflr <= s->ic_rx_tl) {
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
+    s->regs[R_DW_IC_RXFLR]--;
+    if (s->regs[R_DW_IC_RXFLR] <= s->regs[R_DW_IC_RX_TL]) {
+        SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT, DW_IC_INTR_RX_FULL, 0);
         dw_i2c_update_irq(s);
     }
 
     return fifo8_pop(&s->rx_fifo);
 }
 
-static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
+static uint64_t dw_ic_clr_intr_reg_post_read(RegisterInfo *reg, uint64_t value)
 {
-    uint64_t value = 0;
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
 
-    DesignWareI2CState *s = opaque;
+    g_assert(value == 0);
 
-    switch (offset) {
-    case DW_IC_CON:
-        value = s->ic_con;
+    switch (reg->access->addr) {
+    case A_DW_IC_CLR_INTR:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_ANY_SW_CLEAR_MASK;
         break;
-    case DW_IC_TAR:
-        value = s->ic_tar;
+    case A_DW_IC_CLR_RX_UNDER:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_UNDER_MASK;
         break;
-    case DW_IC_SAR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_sar\n",
-                      DEVICE(s)->canonical_path);
-        value = s->ic_sar;
-        break;
-    case DW_IC_DATA_CMD:
-        value = dw_i2c_read_ic_data_cmd(s);
-        break;
-    case DW_IC_SS_SCL_HCNT:
-        value = s->ic_ss_scl_hcnt;
+    case A_DW_IC_CLR_RX_OVER:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_OVER_MASK;
         break;
-    case DW_IC_SS_SCL_LCNT:
-        value = s->ic_ss_scl_lcnt;
+    case A_DW_IC_CLR_TX_OVER:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_OVER_MASK;
         break;
-    case DW_IC_FS_SCL_HCNT:
-        value = s->ic_fs_scl_hcnt;
+    case A_DW_IC_CLR_RD_REQ:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RD_REQ_MASK;
         break;
-    case DW_IC_FS_SCL_LCNT:
-        value = s->ic_fs_scl_lcnt;
+    case A_DW_IC_CLR_TX_ABRT:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_ABRT_MASK;
         break;
-    case DW_IC_INTR_STAT:
-        value = s->ic_raw_intr_stat & s->ic_intr_mask;
+    case A_DW_IC_CLR_RX_DONE:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_DONE_MASK;
         break;
-    case DW_IC_INTR_MASK:
-        value = s->ic_intr_mask;
+    case A_DW_IC_CLR_ACTIVITY:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_ACTIVITY_MASK;
         break;
-    case DW_IC_RAW_INTR_STAT:
-        value = s->ic_raw_intr_stat;
+    case A_DW_IC_CLR_STOP_DET:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_STOP_DET_MASK;
         break;
-    case DW_IC_RX_TL:
-        value = s->ic_rx_tl;
+    case A_DW_IC_CLR_START_DET:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_START_DET_MASK;
         break;
-    case DW_IC_TX_TL:
-        value = s->ic_tx_tl;
-        break;
-    case DW_IC_CLR_INTR:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL |
-            DW_IC_INTR_RESTART_DET |
-            DW_IC_INTR_START_DET |
-            DW_IC_INTR_STOP_DET |
-            DW_IC_INTR_ACTIVITY |
-            DW_IC_INTR_RX_DONE |
-            DW_IC_INTR_TX_ABRT |
-            DW_IC_INTR_RD_REQ |
-            DW_IC_INTR_TX_OVER |
-            DW_IC_INTR_RX_OVER |
-            DW_IC_INTR_RX_UNDER);
-        s->ic_tx_abrt_source = 0;
-        dw_i2c_update_irq(s);
+    case A_DW_IC_CLR_GEN_CALL:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_GEN_CALL_MASK;
         break;
-    case DW_IC_CLR_RX_UNDER:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_UNDER);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_RX_OVER:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_OVER);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_TX_OVER:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_OVER);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_RD_REQ:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RD_REQ);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_TX_ABRT:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_ABRT);
-        s->ic_tx_abrt_source = 0;
-        dw_i2c_update_irq(s);
+    case A_DW_IC_CLR_RESTART_DET:
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RESTART_DET_MASK;
         break;
-    case DW_IC_CLR_RX_DONE:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_DONE);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_ACTIVITY:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_ACTIVITY);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_STOP_DET:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_STOP_DET);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_START_DET:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_START_DET);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_CLR_GEN_CALL:
-        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL);
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_ENABLE:
-        value = s->ic_enable;
-        break;
-    case DW_IC_STATUS:
-        value = s->ic_status;
-        break;
-    case DW_IC_TXFLR:
-        value = s->ic_txflr;
-        break;
-    case DW_IC_RXFLR:
-        value = s->ic_rxflr;
-        break;
-    case DW_IC_SDA_HOLD:
-        value = s->ic_sda_hold;
-        break;
-    case DW_IC_TX_ABRT_SOURCE:
-        value = s->ic_tx_abrt_source;
-        break;
-    case DW_IC_SLV_DATA_NACK_ONLY:
-        qemu_log_mask(LOG_UNIMP,
-                      "%s: unsupported read - ic_slv_data_nack_only\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_CR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_cr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_TDLR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_tdlr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_RDLR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_rdlr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_SDA_SETUP:
-        value = s->ic_sda_setup;
-        break;
-    case DW_IC_ACK_GENERAL_CALL:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_ack_general_call\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_ENABLE_STATUS:
-        value = s->ic_enable_status;
-        break;
-    case DW_IC_FS_SPKLEN:
-        value = s->ic_fs_spklen;
-        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;
-        break;
-    case DW_IC_COMP_VERSION:
-        value = s->ic_comp_version;
-        break;
-    case DW_IC_COMP_TYPE:
-        value = s->ic_comp_type;
-        break;
-
-    /* This register is invalid at this point. */
     default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
-                      DEVICE(s)->canonical_path, offset);
-        break;
+        g_assert_not_reached();
     }
 
-    trace_dw_i2c_read(DEVICE(s)->canonical_path, offset, value);
+    dw_i2c_update_irq(s);
 
-    return value;
+    return 0;
 }
 
-static void dw_i2c_write_ic_con(DesignWareI2CState *s, uint32_t value)
+static uint64_t dw_ic_intr_stat_reg_post_read(RegisterInfo *reg, uint64_t value)
 {
-    if (value & DW_IC_CON_RX_FIFO_FULL_HLD_CTRL) {
-        qemu_log_mask(LOG_UNIMP,
-                      "%s: unsupported ic_con flag - RX_FIFO_FULL_HLD_CTRL\n",
-                      DEVICE(s)->canonical_path);
-    }
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
 
-    if (!(s->ic_enable & DW_IC_ENABLE_ENABLE)) {
-        s->ic_con = value & DW_IC_CON_MASK;
-    } else {
+    g_assert(value == 0);
+
+    return s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];
+}
+
+static uint64_t dw_ic_unsupported_reg_post_read(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    qemu_log_mask(LOG_UNIMP, "%s: unsupported read - %s\n",
+                  DEVICE(s)->canonical_path, reg->access->name);
+
+    return 0;
+}
+
+static uint64_t dw_ic_unsupported_reg_pre_write(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    qemu_log_mask(LOG_UNIMP, "%s: unsupported write - %s\n",
+                  DEVICE(s)->canonical_path, reg->access->name);
+
+    return 0;
+}
+
+static uint64_t dw_ic_con_reg_pre_write(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    if (s->regs[R_DW_IC_ENABLE] & R_DW_IC_ENABLE_ENABLE_MASK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid setting to ic_con %d when ic_enable[0]==1\n",
-                      DEVICE(s)->canonical_path, value);
+                      DEVICE(s)->canonical_path, (int)value);
+        return s->regs[R_DW_IC_CON]; /* keep old value */
     }
+
+    return value;
 }
 
 static void dw_i2c_reset_to_idle(DesignWareI2CState *s)
 {
-        s->ic_enable_status &= ~DW_IC_ENABLE_STATUS_IC_EN;
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_OVER;
-        s->ic_rxflr = 0;
+        s->regs[R_DW_IC_ENABLE_STATUS] &= ~R_DW_IC_ENABLE_STATUS_IC_EN_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_EMPTY_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_FULL_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_UNDER_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_OVER_MASK;
+        s->regs[R_DW_IC_RXFLR] = 0;
         fifo8_reset(&s->rx_fifo);
-        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
+        s->regs[R_DW_IC_STATUS] &= ~R_DW_IC_STATUS_ACTIVITY_MASK;
         s->status = DW_I2C_STATUS_IDLE;
         dw_i2c_update_irq(s);
 }
 
 static void dw_ic_tx_abort(DesignWareI2CState *s, uint32_t src)
 {
-    s->ic_tx_abrt_source |= src;
-    s->ic_raw_intr_stat |= DW_IC_INTR_TX_ABRT;
+    s->regs[R_DW_IC_TX_ABRT_SOURCE] |= src;
+    s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_TX_ABRT_MASK;
     dw_i2c_reset_to_idle(s);
     dw_i2c_update_irq(s);
 }
 
-static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
+static void dw_ic_data_cmd_reg_post_write(RegisterInfo *reg, uint64_t value)
 {
-    int recv = !!(value & DW_IC_DATA_CMD_CMD);
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+    int recv = !!(value & R_DW_IC_DATA_CMD_CMD_MASK);
+
+    s->regs[R_DW_IC_DATA_CMD] = 0; /* Register has no storage */
 
     if (s->status == DW_I2C_STATUS_IDLE ||
-        s->ic_raw_intr_stat & DW_IC_INTR_TX_ABRT) {
+        s->regs[R_DW_IC_RAW_INTR_STAT] & DW_IC_INTR_TX_ABRT_MASK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Attempted to write to TX fifo when it is held in "
                       "reset.\n", DEVICE(s)->canonical_path);
@@ -470,9 +330,10 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
 
     /* Send the address if it hasn't been sent yet. */
     if (s->status == DW_I2C_STATUS_SENDING_ADDRESS) {
-        int rv = i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv);
+        int rv = i2c_start_transfer(s->bus,
+                     ARRAY_FIELD_EX32(s->regs, DW_IC_TAR, ADDRESS), recv);
         if (rv) {
-            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
+            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_7B_ADDR_NOACK_MASK);
             return;
         }
         s->status = recv ? DW_I2C_STATUS_RECEIVING : DW_I2C_STATUS_SENDING;
@@ -480,25 +341,27 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
 
     /* Send data */
     if (!recv) {
-        int rv = i2c_send(s->bus, DW_IC_DATA_CMD_DAT(value));
+        int rv = i2c_send(s->bus, FIELD_EX32(value, DW_IC_DATA_CMD, DAT));
         if (rv) {
             i2c_end_transfer(s->bus);
-            dw_ic_tx_abort(s, DW_IC_TX_ABRT_TXDATA_NOACK);
+            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_TXDATA_NOACK_MASK);
             return;
         }
         dw_i2c_update_irq(s);
     }
 
     /* Restart command */
-    if (value & DW_IC_DATA_CMD_RESTART && s->ic_con & DW_IC_CON_IC_RESTART_EN) {
-        s->ic_raw_intr_stat |= DW_IC_INTR_RESTART_DET |
-                               DW_IC_INTR_START_DET |
-                               DW_IC_INTR_ACTIVITY;
-        s->ic_status |= DW_IC_STATUS_ACTIVITY;
+    if (value & R_DW_IC_DATA_CMD_RESTART_MASK &&
+            s->regs[R_DW_IC_CON] & R_DW_IC_CON_IC_RESTART_EN_MASK) {
+        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RESTART_DET_MASK |
+                                          DW_IC_INTR_START_DET_MASK |
+                                          DW_IC_INTR_ACTIVITY_MASK;
+        s->regs[R_DW_IC_STATUS] |= R_DW_IC_STATUS_ACTIVITY_MASK;
         dw_i2c_update_irq(s);
 
-        if (i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv)) {
-            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
+        if (i2c_start_transfer(s->bus,
+                    ARRAY_FIELD_EX32(s->regs, DW_IC_TAR, ADDRESS), recv)) {
+            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_7B_ADDR_NOACK_MASK);
             return;
         }
 
@@ -507,87 +370,113 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
 
     /* Receive data */
     if (recv) {
-        g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
+        g_assert(s->regs[R_DW_IC_RXFLR] == fifo8_num_used(&s->rx_fifo));
 
         if (!fifo8_is_full(&s->rx_fifo)) {
             fifo8_push(&s->rx_fifo, i2c_recv(s->bus));
-            s->ic_rxflr++;
+            s->regs[R_DW_IC_RXFLR]++;
         } else {
-            s->ic_raw_intr_stat |= DW_IC_INTR_RX_OVER;
+            s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_OVER_MASK;
             dw_i2c_update_irq(s);
         }
 
-        if (s->ic_rxflr > s->ic_rx_tl) {
-            s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
+        if (s->regs[R_DW_IC_RXFLR] > s->regs[R_DW_IC_RX_TL]) {
+            s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_FULL_MASK;
             dw_i2c_update_irq(s);
         }
-        if (value & DW_IC_DATA_CMD_STOP) {
+        if (value & R_DW_IC_DATA_CMD_STOP_MASK) {
             i2c_nack(s->bus);
         }
     }
 
     /* Stop command */
-    if (value & DW_IC_DATA_CMD_STOP) {
-        s->ic_raw_intr_stat |= DW_IC_INTR_STOP_DET;
-        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
+    if (value & R_DW_IC_DATA_CMD_STOP_MASK) {
+        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_STOP_DET_MASK;
+        s->regs[R_DW_IC_STATUS] &= ~R_DW_IC_STATUS_ACTIVITY_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_EMPTY_MASK;
         i2c_end_transfer(s->bus);
         dw_i2c_update_irq(s);
     }
 }
 
-static void dw_i2c_write_ic_enable(DesignWareI2CState *s, uint32_t value)
+static void dw_ic_intr_mask_reg_post_write(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    dw_i2c_update_irq(s);
+}
+
+static uint64_t dw_ic_enable_reg_pre_write(RegisterInfo *reg, uint64_t value)
 {
-    if (value & DW_IC_ENABLE_ENABLE && !(s->ic_con & DW_IC_CON_SLAVE_DISABLE)) {
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    if (value & R_DW_IC_ENABLE_ENABLE_MASK &&
+            !(s->regs[R_DW_IC_CON] & R_DW_IC_CON_SLAVE_DISABLE_MASK)) {
         qemu_log_mask(LOG_UNIMP,
                       "%s: Designware I2C slave mode is not supported.\n",
                       DEVICE(s)->canonical_path);
-        return;
+        return s->regs[R_DW_IC_ENABLE]; /* keep old value */
     }
 
-    s->ic_enable = value & DW_IC_ENABLE_MASK;
+    return value;
+}
+
+static void dw_ic_enable_reg_post_write(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    s->regs[R_DW_IC_ENABLE] = value & R_DW_IC_ENABLE_ENABLE_MASK;
 
-    if (value & DW_IC_ENABLE_ABORT || value & DW_IC_ENABLE_TX_CMD_BLOCK) {
-        dw_ic_tx_abort(s, DW_IC_TX_ABRT_USER_ABRT);
+    if (value & R_DW_IC_ENABLE_ABORT_MASK || value & R_DW_IC_ENABLE_TX_CMD_BLOCK_MASK) {
+        dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_USER_ABRT_MASK);
         return;
     }
 
-    if (value & DW_IC_ENABLE_ENABLE) {
-        s->ic_enable_status |= DW_IC_ENABLE_STATUS_IC_EN;
-        s->ic_status |= DW_IC_STATUS_ACTIVITY;
-        s->ic_raw_intr_stat |= DW_IC_INTR_ACTIVITY |
-                               DW_IC_INTR_START_DET |
-                               DW_IC_INTR_TX_EMPTY;
+    if (value & R_DW_IC_ENABLE_ENABLE_MASK) {
+        s->regs[R_DW_IC_ENABLE_STATUS] |= R_DW_IC_ENABLE_STATUS_IC_EN_MASK;
+        s->regs[R_DW_IC_STATUS] |= R_DW_IC_STATUS_ACTIVITY_MASK;
+        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_ACTIVITY_MASK |
+                                          DW_IC_INTR_START_DET_MASK |
+                                          DW_IC_INTR_TX_EMPTY_MASK;
         s->status = DW_I2C_STATUS_SENDING_ADDRESS;
         dw_i2c_update_irq(s);
-    } else if ((value & DW_IC_ENABLE_ENABLE) == 0) {
+    } else if ((value & R_DW_IC_ENABLE_ENABLE_MASK) == 0) {
         dw_i2c_reset_to_idle(s);
     }
-
 }
 
-static void dw_i2c_write_ic_rx_tl(DesignWareI2CState *s, uint32_t value)
+static uint64_t dw_ic_rx_tl_reg_pre_write(RegisterInfo *reg, uint64_t value)
 {
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
     /* Note that a value of 0 for ic_rx_tl indicates a threashold of 1. */
     if (value > DESIGNWARE_I2C_RX_FIFO_SIZE - 1) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid setting to ic_rx_tl %d\n",
-                      DEVICE(s)->canonical_path, value);
-        s->ic_rx_tl = DESIGNWARE_I2C_RX_FIFO_SIZE - 1;
-    } else {
-        s->ic_rx_tl = value;
+                      DEVICE(s)->canonical_path, (int)value);
+        return DESIGNWARE_I2C_RX_FIFO_SIZE - 1;
     }
 
-    if (s->ic_rxflr > s->ic_rx_tl && s->ic_enable & DW_IC_ENABLE_ENABLE) {
-        s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
+    return value;
+}
+
+static void dw_ic_rx_tl_reg_post_write(RegisterInfo *reg, uint64_t value)
+{
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
+    if (s->regs[R_DW_IC_RXFLR] > s->regs[R_DW_IC_RX_TL] &&
+            s->regs[R_DW_IC_ENABLE] & R_DW_IC_ENABLE_ENABLE_MASK) {
+        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_FULL_MASK;
     } else {
-        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
+        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_FULL_MASK;
     }
     dw_i2c_update_irq(s);
 }
 
-static void dw_i2c_write_ic_tx_tl(DesignWareI2CState *s, uint32_t value)
+static uint64_t dw_ic_tx_tl_reg_pre_write(RegisterInfo *reg, uint64_t value)
 {
+    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
+
     /*
      * Note that a value of 0 for ic_tx_tl indicates a threashold of 1.
      * However, the tx threshold is not used in the model because commands are
@@ -596,106 +485,154 @@ static void dw_i2c_write_ic_tx_tl(DesignWareI2CState *s, uint32_t value)
     if (value > DESIGNWARE_I2C_TX_FIFO_SIZE - 1) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid setting to ic_tx_tl %d\n",
-                      DEVICE(s)->canonical_path, value);
-        s->ic_tx_tl = DESIGNWARE_I2C_TX_FIFO_SIZE - 1;
-    } else {
-        s->ic_tx_tl = value;
+                      DEVICE(s)->canonical_path, (int)value);
+        return DESIGNWARE_I2C_TX_FIFO_SIZE - 1;
     }
-}
 
-static void dw_i2c_write(void *opaque, hwaddr offset, uint64_t value,
-                              unsigned size)
-{
-    DesignWareI2CState *s = opaque;
-
-    trace_dw_i2c_write(DEVICE(s)->canonical_path, offset, value);
-
-    /* The order of the registers are their order in memory. */
-    switch (offset) {
-    case DW_IC_CON:
-        dw_i2c_write_ic_con(s, value);
-        break;
-    case DW_IC_TAR:
-        s->ic_tar = value & DW_IC_TAR_MASK;
-        break;
-    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;
-        break;
-    case DW_IC_DATA_CMD:
-        dw_i2c_write_ic_data_cmd(s, value);
-        break;
-    case DW_IC_SS_SCL_HCNT:
-        s->ic_ss_scl_hcnt = value & DW_IC_SS_SCL_HCNT_MASK;
-        break;
-    case DW_IC_SS_SCL_LCNT:
-        s->ic_ss_scl_lcnt = value & DW_IC_SS_SCL_LCNT_MASK;
-        break;
-    case DW_IC_FS_SCL_HCNT:
-        s->ic_fs_scl_hcnt = value & DW_IC_FS_SCL_HCNT_MASK;
-        break;
-    case DW_IC_FS_SCL_LCNT:
-        s->ic_fs_scl_lcnt = value & DW_IC_FS_SCL_LCNT_MASK;
-        break;
-    case DW_IC_INTR_MASK:
-        s->ic_intr_mask = value & DW_IC_INTR_MASK_MASK;
-        dw_i2c_update_irq(s);
-        break;
-    case DW_IC_RX_TL:
-        dw_i2c_write_ic_rx_tl(s, value);
-        break;
-    case DW_IC_TX_TL:
-        dw_i2c_write_ic_tx_tl(s, value);
-        break;
-    case DW_IC_ENABLE:
-        dw_i2c_write_ic_enable(s, value);
-        break;
-    case DW_IC_SDA_HOLD:
-        s->ic_sda_hold = value & DW_IC_SDA_HOLD_MASK;
-        break;
-    case DW_IC_SLV_DATA_NACK_ONLY:
-        qemu_log_mask(LOG_UNIMP,
-                      "%s: unsupported write - ic_slv_data_nack_only\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_CR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_cr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_TDLR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_tdlr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_DMA_RDLR:
-        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_rdlr\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_SDA_SETUP:
-        s->ic_sda_setup = value & DW_IC_SDA_SETUP_MASK;
-        break;
-    case DW_IC_ACK_GENERAL_CALL:
-        qemu_log_mask(LOG_UNIMP,
-                      "%s: unsupported write - ic_ack_general_call\n",
-                      DEVICE(s)->canonical_path);
-        break;
-    case DW_IC_FS_SPKLEN:
-        s->ic_fs_spklen = value & DW_IC_FS_SPKLEN_MASK;
-        break;
+    return value;
+}
 
-    /* This register is invalid at this point. */
-    default:
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: write to invalid offset or readonly register 0x%"
-                      HWADDR_PRIx "\n",
-                      DEVICE(s)->canonical_path, offset);
-        break;
+static const RegisterAccessInfo designware_i2c_regs_info[] = {
+    {   .name  = "DW_IC_CON", .addr = A_DW_IC_CON,
+        .reset =       0x7d,
+        .unimp = 0xfffffc00,
+        .unimp = R_DW_IC_CON_RX_FIFO_FULL_HLD_CTRL_MASK,
+        .pre_write = dw_ic_con_reg_pre_write,
+    },{ .name  = "DW_IC_TAR", .addr = A_DW_IC_TAR,
+        .reset =     0x1055,
+        .unimp = 0xfffff000,
+    },{ .name  = "DW_IC_SAR", .addr = A_DW_IC_SAR,
+        .reset =       0x55,
+        .unimp = 0xfffffc00,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_DATA_CMD", .addr = A_DW_IC_DATA_CMD,
+        .post_read = dw_ic_data_cmd_reg_post_read,
+        .post_write = dw_ic_data_cmd_reg_post_write,
+    },{ .name  = "DW_IC_SS_SCL_HCNT", .addr = A_DW_IC_SS_SCL_HCNT,
+        .reset =      0x190,
+        .unimp = 0xffff0000,
+    },{ .name  = "DW_IC_SS_SCL_LCNT", .addr = A_DW_IC_SS_SCL_LCNT,
+        .reset =      0x1d6,
+        .unimp = 0xffff0000,
+    },{ .name  = "DW_IC_FS_SCL_HCNT", .addr = A_DW_IC_FS_SCL_HCNT,
+        .reset =       0x3c,
+        .unimp = 0xffff0000,
+    },{ .name  = "DW_IC_FS_SCL_LCNT", .addr = A_DW_IC_FS_SCL_LCNT,
+        .reset =       0x82,
+        .unimp = 0xffff0000,
+    },{ .name  = "DW_IC_INTR_STAT", .addr = A_DW_IC_INTR_STAT,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_intr_stat_reg_post_read,
+    },{ .name  = "DW_IC_INTR_MASK", .addr = A_DW_IC_INTR_MASK,
+        .reset =      0x8ff,
+        .unimp = 0xffff8000,
+        .post_write = dw_ic_intr_mask_reg_post_write,
+    },{ .name  = "DW_IC_RAW_INTR_STAT", .addr = A_DW_IC_RAW_INTR_STAT,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_RX_TL", .addr = A_DW_IC_RX_TL,
+        .pre_write = dw_ic_rx_tl_reg_pre_write,
+        .post_write = dw_ic_rx_tl_reg_post_write,
+    },{ .name  = "DW_IC_TX_TL", .addr = A_DW_IC_TX_TL,
+        .pre_write = dw_ic_tx_tl_reg_pre_write,
+    },{ .name  = "DW_IC_CLR_INTR", .addr = A_DW_IC_CLR_INTR,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_RX_UNDER", .addr = A_DW_IC_CLR_RX_UNDER,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_RX_OVER", .addr = A_DW_IC_CLR_RX_OVER,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_TX_OVER", .addr = A_DW_IC_CLR_TX_OVER,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_RD_REQ", .addr = A_DW_IC_CLR_RD_REQ,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_TX_ABRT", .addr = A_DW_IC_CLR_TX_ABRT,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_RX_DONE", .addr = A_DW_IC_CLR_RX_DONE,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_ACTIVITY", .addr = A_DW_IC_CLR_ACTIVITY,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_STOP_DET", .addr = A_DW_IC_CLR_STOP_DET,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_START_DET", .addr = A_DW_IC_CLR_START_DET,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_CLR_GEN_CALL", .addr = A_DW_IC_CLR_GEN_CALL,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_ENABLE", .addr = A_DW_IC_ENABLE,
+        .unimp = 0xfffffff8,
+        .pre_write = dw_ic_enable_reg_pre_write,
+        .post_write = dw_ic_enable_reg_post_write,
+    },{ .name  = "DW_IC_STATUS", .addr = A_DW_IC_STATUS,
+        .reset =        0x6,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_TXFLR", .addr = A_DW_IC_TXFLR,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_RXFLR", .addr = A_DW_IC_RXFLR,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_SDA_HOLD", .addr = A_DW_IC_SDA_HOLD,
+        .reset =        0x1,
+        .unimp = 0xff000000,
+    },{ .name  = "DW_IC_TX_ABRT_SOURCE", .addr = A_DW_IC_TX_ABRT_SOURCE,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_SLV_DATA_NACK_ONLY", .addr = A_DW_IC_SLV_DATA_NACK_ONLY,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_DMA_CR", .addr = A_DW_IC_DMA_CR,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_DMA_TDLR", .addr = A_DW_IC_DMA_TDLR,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_DMA_RDLR", .addr = A_DW_IC_DMA_RDLR,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_SDA_SETUP", .addr = A_DW_IC_SDA_SETUP,
+        .reset =       0x64,
+        .unimp = 0xffffff00,
+    },{ .name  = "DW_IC_ACK_GENERAL_CALL", .addr = A_DW_IC_ACK_GENERAL_CALL,
+        .post_read = dw_ic_unsupported_reg_post_read,
+        .pre_write = dw_ic_unsupported_reg_pre_write,
+    },{ .name  = "DW_IC_ENABLE_STATUS", .addr = A_DW_IC_ENABLE_STATUS,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_FS_SPKLEN", .addr = A_DW_IC_FS_SPKLEN,
+        .reset =        0x2,
+        .ro    = 0xffffff00,
+    },{ .name  = "DW_IC_CLR_RESTART_DET", .addr = A_DW_IC_CLR_RESTART_DET,
+        .ro    = 0xffffffff,
+        .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_COMP_PARAM_1", .addr = A_DW_IC_COMP_PARAM_1,
+        .reset = /* HAS_DMA and HC_COUNT_VAL are disabled */
+            ((2 << R_DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32_SHIFT) |
+             R_DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE_MASK           |
+             R_DW_IC_COMP_PARAM_1_INTR_IO_MASK                   |
+             R_DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS_MASK        |
+             ((DESIGNWARE_I2C_RX_FIFO_SIZE - 1)
+                  << R_DW_IC_COMP_PARAM_1_RX_FIFO_SIZE_SHIFT)    |
+             ((DESIGNWARE_I2C_TX_FIFO_SIZE - 1)
+                  << R_DW_IC_COMP_PARAM_1_TX_FIFO_SIZE_SHIFT)),
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_COMP_VERSION", .addr = A_DW_IC_COMP_VERSION,
+        .reset = 0x3132302a,
+        .ro    = 0xffffffff,
+    },{ .name  = "DW_IC_COMP_TYPE", .addr = A_DW_IC_COMP_TYPE,
+        .reset = 0x44570140,
+        .ro    = 0xffffffff,
     }
-}
+};
 
 static const MemoryRegionOps designware_i2c_ops = {
-    .read = dw_i2c_read,
-    .write = dw_i2c_write,
+    .read = register_read_memory,
+    .write = register_write_memory,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 4,
@@ -711,30 +648,11 @@ static const MemoryRegionOps designware_i2c_ops = {
 static void designware_i2c_enter_reset(Object *obj, ResetType type)
 {
     DesignWareI2CState *s = DESIGNWARE_I2C(obj);
+    unsigned int i;
 
-    s->ic_con = DW_IC_CON_INIT_VAL;
-    s->ic_tar = DW_IC_TAR_INIT_VAL;
-    s->ic_sar = DW_IC_SAR_INIT_VAL;
-    s->ic_ss_scl_hcnt = DW_IC_SS_SCL_HCNT_INIT_VAL;
-    s->ic_ss_scl_lcnt = DW_IC_SS_SCL_LCNT_INIT_VAL;
-    s->ic_fs_scl_hcnt = DW_IC_FS_SCL_HCNT_INIT_VAL;
-    s->ic_fs_scl_lcnt = DW_IC_FS_SCL_LCNT_INIT_VAL;
-    s->ic_intr_mask = DW_IC_INTR_MASK_INIT_VAL;
-    s->ic_raw_intr_stat = 0;
-    s->ic_rx_tl = 0;
-    s->ic_tx_tl = 0;
-    s->ic_enable = 0;
-    s->ic_status = DW_IC_STATUS_INIT_VAL;
-    s->ic_txflr = 0;
-    s->ic_rxflr = 0;
-    s->ic_sda_hold = DW_IC_SDA_HOLD_INIT_VAL;
-    s->ic_tx_abrt_source = 0;
-    s->ic_sda_setup = DW_IC_SDA_SETUP_INIT_VAL;
-    s->ic_enable_status = 0;
-    s->ic_fs_spklen = DW_IC_FS_SPKLEN_INIT_VAL;
-    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;
+    for (i = 0; i < ARRAY_SIZE(s->regs); ++i) {
+        register_reset(&s->regs_info[i]);
+    }
 
     fifo8_reset(&s->rx_fifo);
 
@@ -753,29 +671,7 @@ static const VMStateDescription vmstate_designware_i2c = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINT32(ic_con, DesignWareI2CState),
-        VMSTATE_UINT32(ic_tar, DesignWareI2CState),
-        VMSTATE_UINT32(ic_sar, DesignWareI2CState),
-        VMSTATE_UINT32(ic_ss_scl_hcnt, DesignWareI2CState),
-        VMSTATE_UINT32(ic_ss_scl_lcnt, DesignWareI2CState),
-        VMSTATE_UINT32(ic_fs_scl_hcnt, DesignWareI2CState),
-        VMSTATE_UINT32(ic_fs_scl_lcnt, DesignWareI2CState),
-        VMSTATE_UINT32(ic_intr_mask, DesignWareI2CState),
-        VMSTATE_UINT32(ic_raw_intr_stat, DesignWareI2CState),
-        VMSTATE_UINT32(ic_rx_tl, DesignWareI2CState),
-        VMSTATE_UINT32(ic_tx_tl, DesignWareI2CState),
-        VMSTATE_UINT32(ic_enable, DesignWareI2CState),
-        VMSTATE_UINT32(ic_status, DesignWareI2CState),
-        VMSTATE_UINT32(ic_txflr, DesignWareI2CState),
-        VMSTATE_UINT32(ic_rxflr, DesignWareI2CState),
-        VMSTATE_UINT32(ic_sda_hold, DesignWareI2CState),
-        VMSTATE_UINT32(ic_tx_abrt_source, DesignWareI2CState),
-        VMSTATE_UINT32(ic_sda_setup, DesignWareI2CState),
-        VMSTATE_UINT32(ic_enable_status, DesignWareI2CState),
-        VMSTATE_UINT32(ic_fs_spklen, DesignWareI2CState),
-        VMSTATE_UINT32(ic_comp_param_1, DesignWareI2CState),
-        VMSTATE_UINT32(ic_comp_version, DesignWareI2CState),
-        VMSTATE_UINT32(ic_comp_type, DesignWareI2CState),
+        VMSTATE_UINT32_ARRAY(regs, DesignWareI2CState, DESIGNWARE_I2C_R_MAX),
         VMSTATE_FIFO8(rx_fifo, DesignWareI2CState),
         VMSTATE_UINT32(status, DesignWareI2CState),
         VMSTATE_END_OF_LIST(),
@@ -786,6 +682,7 @@ static void designware_i2c_smbus_init(Object *obj)
 {
     DesignWareI2CState *s = DESIGNWARE_I2C(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    RegisterInfoArray *reg_array;
 
     fifo8_create(&s->rx_fifo, DESIGNWARE_I2C_RX_FIFO_SIZE);
 
@@ -796,6 +693,15 @@ static void designware_i2c_smbus_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
 
     s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
+
+    memory_region_init(&s->iomem, obj, TYPE_DESIGNWARE_I2C, 4 * KiB);
+    reg_array = register_init_block32(DEVICE(obj), designware_i2c_regs_info,
+                                      ARRAY_SIZE(designware_i2c_regs_info),
+                                      s->regs_info, s->regs,
+                                      &designware_i2c_ops,
+                                      DESIGNWARE_I2C_ERR_DEBUG,
+                                      DESIGNWARE_I2C_R_MAX * 4);
+    memory_region_add_subregion(&s->iomem, 0, &reg_array->mem);
 }
 
 static void designware_i2c_finalize(Object *obj)
diff --git a/include/hw/i2c/designware_i2c.h b/include/hw/i2c/designware_i2c.h
index affaf983a2..54112c38e7 100644
--- a/include/hw/i2c/designware_i2c.h
+++ b/include/hw/i2c/designware_i2c.h
@@ -11,9 +11,11 @@
 #include "qemu/fifo8.h"
 #include "hw/i2c/i2c.h"
 #include "hw/core/irq.h"
+#include "hw/core/register.h"
 #include "hw/core/sysbus.h"
 
-/* Size of the FIFO buffers. */
+#define DESIGNWARE_I2C_R_MAX (0x100 / 4)
+
 #define DESIGNWARE_I2C_RX_FIFO_SIZE 16
 #define DESIGNWARE_I2C_TX_FIFO_SIZE 16
 
@@ -28,31 +30,6 @@ typedef enum DesignWareI2CStatus {
  * struct DesignWareI2CState - DesignWare I2C device state.
  * @bus: The underlying I2C Bus
  * @irq: GIC interrupt line to fire on events
- * @ic_con: : I2C control register
- * @ic_tar: I2C target address register
- * @ic_sar: I2C slave address register
- * @ic_ss_scl_hcnt: Standard speed i2c clock scl high count register
- * @ic_ss_scl_lcnt: Standard speed i2c clock scl low count register
- * @ic_fs_scl_hcnt: Fast mode or fast mode plus i2c clock scl high count
- *                  register
- * @ic_fs_scl_lcnt:Fast mode or fast mode plus i2c clock scl low count
- *                  register
- * @ic_intr_mask: I2C Interrupt Mask Register
- * @ic_raw_intr_stat: I2C raw interrupt status register
- * @ic_rx_tl: I2C receive FIFO threshold register
- * @ic_tx_tl: I2C transmit FIFO threshold register
- * @ic_enable: I2C enable register
- * @ic_status: I2C status register
- * @ic_txflr: I2C transmit fifo level register
- * @ic_rxflr: I2C receive fifo level register
- * @ic_sda_hold: I2C SDA hold time length register
- * @ic_tx_abrt_source: The I2C transmit abort source register
- * @ic_sda_setup: I2C SDA setup register
- * @ic_enable_status: I2C enable status register
- * @ic_fs_spklen: I2C SS, FS or FM+ spike suppression limit
- * @ic_comp_param_1: Component parameter register
- * @ic_comp_version: I2C component version register
- * @ic_comp_type: I2C component type register
  * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
  */
 typedef struct DesignWareI2CState {
@@ -63,31 +40,10 @@ typedef struct DesignWareI2CState {
     I2CBus      *bus;
     qemu_irq     irq;
 
-    uint32_t ic_con;
-    uint32_t ic_tar;
-    uint32_t ic_sar;
-    uint32_t ic_ss_scl_hcnt;
-    uint32_t ic_ss_scl_lcnt;
-    uint32_t ic_fs_scl_hcnt;
-    uint32_t ic_fs_scl_lcnt;
-    uint32_t ic_intr_mask;
-    uint32_t ic_raw_intr_stat;
-    uint32_t ic_rx_tl;
-    uint32_t ic_tx_tl;
-    uint32_t ic_enable;
-    uint32_t ic_status;
-    uint32_t ic_txflr;
-    uint32_t ic_rxflr;
-    uint32_t ic_sda_hold;
-    uint32_t ic_tx_abrt_source;
-    uint32_t ic_sda_setup;
-    uint32_t ic_enable_status;
-    uint32_t ic_fs_spklen;
-    uint32_t ic_comp_param_1;
-    uint32_t ic_comp_version;
-    uint32_t ic_comp_type;
+    uint32_t regs[DESIGNWARE_I2C_R_MAX];
+    RegisterInfo regs_info[DESIGNWARE_I2C_R_MAX];
 
-    /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
+    /* fifo8_num_used(rx_fifo) should always equal DW_IC_RXFLR */
     Fifo8    rx_fifo;
 
     DesignWareI2CStatus status;
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK
  2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
                   ` (2 preceding siblings ...)
  2026-05-07 12:05 ` [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API Nicholas Piggin
@ 2026-05-07 12:05 ` Nicholas Piggin
  2026-05-11 10:19   ` Philippe Mathieu-Daudé
  2026-05-10 13:03 ` [PATCH 0/4] hw/i2c: Add designware i2c controller Corey Minyard
  4 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-07 12:05 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Nicholas Piggin, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

QEMU complains about access to unimplemented register when Linux
inits the controller. It is the SMBUS interrupt mask which the
driver unmasks. Since the model implements no SMBUS interrupts,
the mask can be implemented trivially.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/i2c/designware_i2c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
index b7be4d68c4..cc37bde2ae 100644
--- a/hw/i2c/designware_i2c.c
+++ b/hw/i2c/designware_i2c.c
@@ -142,6 +142,7 @@ REG32(DW_IC_ENABLE_STATUS,      0x9c) /* I2C enable status */
     FIELD(DW_IC_ENABLE_STATUS, IC_EN,                   0, 1)
 REG32(DW_IC_FS_SPKLEN,          0xa0) /* I2C SS, FS or FM+ spike suppression limit */
 REG32(DW_IC_CLR_RESTART_DET,    0xa8)
+REG32(DW_IC_SMBUS_INTR_MASK,    0xcc) /* SMBus Interrupt Mask */
 REG32(DW_IC_COMP_PARAM_1,       0xf4) /* Component parameter */
     FIELD(DW_IC_COMP_PARAM_1, TX_FIFO_SIZE,       16, 8)
     FIELD(DW_IC_COMP_PARAM_1, RX_FIFO_SIZE,        8, 8)
@@ -610,6 +611,10 @@ static const RegisterAccessInfo designware_i2c_regs_info[] = {
     },{ .name  = "DW_IC_CLR_RESTART_DET", .addr = A_DW_IC_CLR_RESTART_DET,
         .ro    = 0xffffffff,
         .post_read = dw_ic_clr_intr_reg_post_read,
+    },{ .name  = "DW_IC_SMBUS_INTR_MASK", .addr = A_DW_IC_SMBUS_INTR_MASK,
+        /* No SMBus interrupts are implemented, Linux updates the mask */
+        .reset =      0x7ff,
+        .unimp = 0xfffff800,
     },{ .name  = "DW_IC_COMP_PARAM_1", .addr = A_DW_IC_COMP_PARAM_1,
         .reset = /* HAS_DMA and HC_COUNT_VAL are disabled */
             ((2 << R_DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32_SHIFT) |
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] hw/i2c: Add designware i2c controller
  2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
                   ` (3 preceding siblings ...)
  2026-05-07 12:05 ` [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK Nicholas Piggin
@ 2026-05-10 13:03 ` Corey Minyard
  2026-05-15 23:00   ` Nicholas Piggin
  4 siblings, 1 reply; 13+ messages in thread
From: Corey Minyard @ 2026-05-10 13:03 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Alistair Francis, Daniel Henrique Barboza, Chao Liu, Chris Rauer,
	Michael Ellerman, Joel Stanley, Anirudh Srinivasan,
	Portia Stephens, qemu-riscv, qemu-devel, Hao Wu,
	Philippe Mathieu-Daudé

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On Thu, May 07, 2026 at 10:05:18PM +1000, Nicholas Piggin wrote:
> Hi,
> 
> This series contains the DW I2C model written by Chris Rauer and
> updated for the Tenstorrent Atlantis machine recently. There was
> some more review comment on that submission and so we decided to
> take the I2C device out of that series and work on it separately,
> see here:
> 
> https://lore.kernel.org/qemu-devel/20260425131721.932250-1-joel@jms.id.au/T/#mb1ef2824c2f1f37bf4574dc1ef0fb95566c3a2f2
> 
> The big thing suggested was to move to the QEMU register API. That
> is a big change and difficult to review, so I have split that and
> a some smaller changes out into their own patches. I don't expect
> detailed reviews on the register API patch -- it's quite mechanical
> and I did attempt to verify it by diff'ing register traces. But it
> would be good to make sure maintainers are happy to go that way.
> 
> Unfortunately the patch 1 was quite well reviewed and tested so
> incremental changes would be preferable, but it is painful to maintain
> migration compatibility across these changes.

I had a few comments on the first patch, but they were all fixed in
later patches.  From my review this all looks good.

Yes, please squash these as you suggested in the second patch.

Acked-by: Corey Minyard <cminyard@mvista.com>

-corey

> 
> Thanks,
> Nick
> 
> Chris Rauer (1):
>   hw/i2c: Add designware i2c controller
> 
> Nicholas Piggin (3):
>   [RFC] hw/i2c/designware_i2c: Switch to Fifo8
>   [RFC] hw/i2c/designware_i2c: Switch to QEMU register API
>   [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK
> 
>  MAINTAINERS                     |   8 +
>  hw/i2c/Kconfig                  |   5 +
>  hw/i2c/designware_i2c.c         | 742 ++++++++++++++++++++++++++++++++
>  hw/i2c/meson.build              |   1 +
>  hw/i2c/trace-events             |   4 +
>  include/hw/i2c/designware_i2c.h |  56 +++
>  roms/seabios-hppa               |   2 +-
>  7 files changed, 817 insertions(+), 1 deletion(-)
>  create mode 100644 hw/i2c/designware_i2c.c
>  create mode 100644 include/hw/i2c/designware_i2c.h
> 
> -- 
> 2.53.0
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3365 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8
  2026-05-07 12:05 ` [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8 Nicholas Piggin
@ 2026-05-11 10:18   ` Philippe Mathieu-Daudé
  2026-05-15 18:23     ` Nicholas Piggin
  0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-05-11 10:18 UTC (permalink / raw)
  To: Nicholas Piggin, Corey Minyard
  Cc: Alistair Francis, Daniel Henrique Barboza, Chao Liu, Chris Rauer,
	Michael Ellerman, Joel Stanley, Anirudh Srinivasan,
	Portia Stephens, qemu-riscv, qemu-devel, Hao Wu

On 7/5/26 14:05, Nicholas Piggin wrote:
> Alistair suggested moving to Fifo8, which I think is
> a good improvement.
> 
> Broken out for individual review, but IMO we should
> squash before merge since it changes VMState format.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/i2c/designware_i2c.c         | 37 ++++++++++++++++++++-------------
>   include/hw/i2c/designware_i2c.h |  7 +++----
>   2 files changed, 25 insertions(+), 19 deletions(-)


> @@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
>    * @ic_comp_version: I2C component version register
>    * @ic_comp_type: I2C component type register
>    * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
> - * @rx_cur: The current position of rx_fifo.
> - * @status: The current status of the SMBus.
>    */
>   typedef struct DesignWareI2CState {
>       SysBusDevice parent_obj;
> @@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
>       uint32_t ic_comp_version;
>       uint32_t ic_comp_type;
>   
> -    uint8_t      rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
> -    uint8_t      rx_cur;
> +    /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */

Why not remove ic_rxflr then?

> +    Fifo8    rx_fifo;
>   
>       DesignWareI2CStatus status;
>   } DesignWareI2CState;



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK
  2026-05-07 12:05 ` [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK Nicholas Piggin
@ 2026-05-11 10:19   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-05-11 10:19 UTC (permalink / raw)
  To: Nicholas Piggin, Corey Minyard
  Cc: Alistair Francis, Daniel Henrique Barboza, Chao Liu, Chris Rauer,
	Michael Ellerman, Joel Stanley, Anirudh Srinivasan,
	Portia Stephens, qemu-riscv, qemu-devel, Hao Wu

On 7/5/26 14:05, Nicholas Piggin wrote:
> QEMU complains about access to unimplemented register when Linux
> inits the controller. It is the SMBUS interrupt mask which the
> driver unmasks. Since the model implements no SMBUS interrupts,
> the mask can be implemented trivially.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/i2c/designware_i2c.c | 5 +++++
>   1 file changed, 5 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] hw/i2c: Add designware i2c controller
  2026-05-07 12:05 ` [PATCH 1/4] " Nicholas Piggin
@ 2026-05-11 10:20   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-05-11 10:20 UTC (permalink / raw)
  To: Nicholas Piggin, Corey Minyard
  Cc: Alistair Francis, Daniel Henrique Barboza, Chao Liu, Chris Rauer,
	Michael Ellerman, Joel Stanley, Anirudh Srinivasan,
	Portia Stephens, qemu-riscv, qemu-devel, Hao Wu, Joel Stanley

On 7/5/26 14:05, Nicholas Piggin 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>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---


> diff --git a/roms/seabios-hppa b/roms/seabios-hppa
> index d9560852a3..1a8ada1fb7 160000
> --- a/roms/seabios-hppa
> +++ b/roms/seabios-hppa
> @@ -1 +1 @@
> -Subproject commit d9560852a34f156155b3777745baa0d96d553f22
> +Subproject commit 1a8ada1fb70643172e251aacbac673c9ecda99e9

Unrelated change ;) Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API
  2026-05-07 12:05 ` [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API Nicholas Piggin
@ 2026-05-13  2:03   ` Alistair Francis
  2026-05-15 18:34     ` Nicholas Piggin
  0 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2026-05-13  2:03 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Corey Minyard, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

On Thu, May 7, 2026 at 10:07 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Alistair suggested moving to QEMU register API. I haven't used it before
> but IMO it is an improvement. It is a lot of churn that is not very
> review-able but unfortunately also changes VMState making it a pain to
> do incrementally. If we decide to go this way, we should squash the
> conversion patch when we're happy with it.
>
> I tried not to rearrange the code too much, to reduce patch size. I
> prefer putting register implementation all together rather than grouped
> by read and write, which this API promotes. So if others like that style
> then I will move that.
>
> I didn't change to use all the FIELD APIs to set/clear bits, since
> it worked out a bit easier to script and only a smal win since there is
> little/no multibit fields and shifting/masking required.
>
> Register traces of loading the kernel drivers for the 2 devices added on
> the Atlantis machine in the series Joel posted earlier look identical
> modulo RTC data reads (which I guess is due to time differences).
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/i2c/Kconfig                  |    1 +
>  hw/i2c/designware_i2c.c         | 1022 ++++++++++++++-----------------
>  include/hw/i2c/designware_i2c.h |   56 +-
>  3 files changed, 471 insertions(+), 608 deletions(-)
>
> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index d3f394edeb..0766130b59 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -20,6 +20,7 @@ config ARM_SBCON_I2C
>
>  config DESIGNWARE_I2C
>      bool
> +    select REGISTER
>      select I2C
>
>  config ACPI_SMBUS
> diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
> index 83d0968580..b7be4d68c4 100644
> --- a/hw/i2c/designware_i2c.c
> +++ b/hw/i2c/designware_i2c.c
> @@ -16,452 +16,312 @@
>  #include "qemu/units.h"
>  #include "trace.h"
>
> -enum DesignWareI2CRegister {
> -    DW_IC_CON                   = 0x00,
> -    DW_IC_TAR                   = 0x04,
> -    DW_IC_SAR                   = 0x08,
> -    DW_IC_DATA_CMD              = 0x10,
> -    DW_IC_SS_SCL_HCNT           = 0x14,
> -    DW_IC_SS_SCL_LCNT           = 0x18,
> -    DW_IC_FS_SCL_HCNT           = 0x1c,
> -    DW_IC_FS_SCL_LCNT           = 0x20,
> -    DW_IC_INTR_STAT             = 0x2c,
> -    DW_IC_INTR_MASK             = 0x30,
> -    DW_IC_RAW_INTR_STAT         = 0x34,
> -    DW_IC_RX_TL                 = 0x38,
> -    DW_IC_TX_TL                 = 0x3c,
> -    DW_IC_CLR_INTR              = 0x40,
> -    DW_IC_CLR_RX_UNDER          = 0x44,
> -    DW_IC_CLR_RX_OVER           = 0x48,
> -    DW_IC_CLR_TX_OVER           = 0x4c,
> -    DW_IC_CLR_RD_REQ            = 0x50,
> -    DW_IC_CLR_TX_ABRT           = 0x54,
> -    DW_IC_CLR_RX_DONE           = 0x58,
> -    DW_IC_CLR_ACTIVITY          = 0x5c,
> -    DW_IC_CLR_STOP_DET          = 0x60,
> -    DW_IC_CLR_START_DET         = 0x64,
> -    DW_IC_CLR_GEN_CALL          = 0x68,
> -    DW_IC_ENABLE                = 0x6c,
> -    DW_IC_STATUS                = 0x70,
> -    DW_IC_TXFLR                 = 0x74,
> -    DW_IC_RXFLR                 = 0x78,
> -    DW_IC_SDA_HOLD              = 0x7c,
> -    DW_IC_TX_ABRT_SOURCE        = 0x80,
> -    DW_IC_SLV_DATA_NACK_ONLY    = 0x84,
> -    DW_IC_DMA_CR                = 0x88,
> -    DW_IC_DMA_TDLR              = 0x8c,
> -    DW_IC_DMA_RDLR              = 0x90,
> -    DW_IC_SDA_SETUP             = 0x94,
> -    DW_IC_ACK_GENERAL_CALL      = 0x98,
> -    DW_IC_ENABLE_STATUS         = 0x9c,
> -    DW_IC_FS_SPKLEN             = 0xa0,
> -    DW_IC_CLR_RESTART_DET       = 0xa8,
> -    DW_IC_COMP_PARAM_1          = 0xf4,
> -    DW_IC_COMP_VERSION          = 0xf8,
> -    DW_IC_COMP_TYPE             = 0xfc,
> -};
> -
> -/* DW_IC_CON fields */
> -#define DW_IC_CON_STOP_DET_IF_MASTER_ACTIV  BIT(10)
> -#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL     BIT(9)
> -#define DW_IC_CON_TX_EMPTY_CTRL             BIT(8)
> -#define DW_IC_CON_STOP_IF_ADDRESSED         BIT(7)
> -#define DW_IC_CON_SLAVE_DISABLE             BIT(6)
> -#define DW_IC_CON_IC_RESTART_EN             BIT(5)
> -#define DW_IC_CON_10BITADDR_MASTER          BIT(4)
> -#define DW_IC_CON_10BITADDR_SLAVE           BIT(3)
> -#define DW_IC_CON_SPEED(rv)                 extract32((rv), 1, 2)
> -#define DW_IC_CON_MASTER_MODE               BIT(0)
> -
> -/* DW_IC_TAR fields */
> -#define DW_IC_TAR_IC_10BITADDR_MASTER  BIT(12)
> -#define DW_IC_TAR_SPECIAL              BIT(11)
> -#define DW_IC_TAR_GC_OR_START          BIT(10)
> -#define DW_IC_TAR_ADDRESS(rv)          extract32((rv), 0, 10)
> -
> -/* DW_IC_DATA_CMD fields */
> -#define DW_IC_DATA_CMD_RESTART  BIT(10)
> -#define DW_IC_DATA_CMD_STOP     BIT(9)
> -#define DW_IC_DATA_CMD_CMD      BIT(8)
> -#define DW_IC_DATA_CMD_DAT(rv)  extract32((rv), 0, 8)
> -
> -/* DW_IC_INTR_STAT/INTR_MASK/RAW_INTR_STAT fields */
> -#define DW_IC_INTR_RESTART_DET  BIT(12)
> -#define DW_IC_INTR_GEN_CALL     BIT(11)
> -#define DW_IC_INTR_START_DET    BIT(10)
> -#define DW_IC_INTR_STOP_DET     BIT(9)
> -#define DW_IC_INTR_ACTIVITY     BIT(8)
> -#define DW_IC_INTR_RX_DONE      BIT(7)
> -#define DW_IC_INTR_TX_ABRT      BIT(6)
> -#define DW_IC_INTR_RD_REQ       BIT(5)
> -#define DW_IC_INTR_TX_EMPTY     BIT(4) /* Hardware clear only. */
> -#define DW_IC_INTR_TX_OVER      BIT(3)
> -#define DW_IC_INTR_RX_FULL      BIT(2) /* Hardware clear only. */
> -#define DW_IC_INTR_RX_OVER      BIT(1)
> -#define DW_IC_INTR_RX_UNDER     BIT(0)
> -
> -/* DW_IC_ENABLE fields */
> -#define DW_IC_ENABLE_TX_CMD_BLOCK  BIT(2)
> -#define DW_IC_ENABLE_ABORT         BIT(1)
> -#define DW_IC_ENABLE_ENABLE        BIT(0)
> -
> -/* DW_IC_STATUS fields */
> -#define DW_IC_STATUS_SLV_ACTIVITY  BIT(6)
> -#define DW_IC_STATUS_MST_ACTIVITY  BIT(5)
> -#define DW_IC_STATUS_RFF           BIT(4)
> -#define DW_IC_STATUS_RFNE          BIT(3)
> -#define DW_IC_STATUS_TFE           BIT(2)
> -#define DW_IC_STATUS_TFNF          BIT(1)
> -#define DW_IC_STATUS_ACTIVITY      BIT(0)
> -
> -/* DW_IC_TX_ABRT_SOURCE fields */
> -#define DW_IC_TX_TX_FLUSH_CNT          extract32((rv), 23, 9)
> -#define DW_IC_TX_ABRT_USER_ABRT        BIT(16)
> -#define DW_IC_TX_ABRT_SLVRD_INTX       BIT(15)
> -#define DW_IC_TX_ABRT_SLV_ARBLOST      BIT(14)
> -#define DW_IC_TX_ABRT_SLVFLUSH_TXFIFO  BIT(13)
> -#define DW_IC_TX_ARB_LOST              BIT(12)
> -#define DW_IC_TX_ABRT_MASTER_DIS       BIT(11)
> -#define DW_IC_TX_ABRT_10B_RD_NORSTRT   BIT(10)
> -#define DW_IC_TX_ABRT_SBYTE_NORSTRT    BIT(9)
> -#define DW_IC_TX_ABRT_HS_NORSTRT       BIT(8)
> -#define DW_IC_TX_ABRT_SBYTE_ACKDET     BIT(7)
> -#define DW_IC_TX_ABRT_HS_ACKDET        BIT(6)
> -#define DW_IC_TX_ABRT_GCALL_READ       BIT(5)
> -#define DW_IC_TX_ABRT_GCALL_NOACK      BIT(4)
> -#define DW_IC_TX_ABRT_TXDATA_NOACK     BIT(3)
> -#define DW_IC_TX_ABRT_10ADDR2_NOACK    BIT(2)
> -#define DW_IC_TX_ABRT_10ADDR1_NOACK    BIT(1)
> -#define DW_IC_TX_ABRT_7B_ADDR_NOACK    BIT(0)
> -
> -
> -/* IC_ENABLE_STATUS fields */
> -#define DW_IC_ENABLE_STATUS_SLV_RX_DATA_LOST         BIT(2)
> -#define DW_IC_ENABLE_STATUS_SLV_DISABLED_WHILE_BUSY  BIT(1)
> -#define DW_IC_ENABLE_STATUS_IC_EN                    BIT(0)
> -
> -/* Masks for writable registers. */
> -#define DW_IC_CON_MASK          0x000003ff
> -#define DW_IC_TAR_MASK          0x00000fff
> -#define DW_IC_SAR_MASK          0x000003ff
> -#define DW_IC_SS_SCL_HCNT_MASK  0x0000ffff
> -#define DW_IC_SS_SCL_LCNT_MASK  0x0000ffff
> -#define DW_IC_FS_SCL_HCNT_MASK  0x0000ffff
> -#define DW_IC_FS_SCL_LCNT_MASK  0x0000ffff
> -#define DW_IC_INTR_MASK_MASK    0x00001fff
> -#define DW_IC_ENABLE_MASK       0x00000007
> -#define DW_IC_SDA_HOLD_MASK     0x00ffffff
> -#define DW_IC_SDA_SETUP_MASK    0x000000ff
> -#define DW_IC_FS_SPKLEN_MASK    0x000000ff
> -
> -/* Reset values */
> -#define DW_IC_CON_INIT_VAL          0x7d
> -#define DW_IC_TAR_INIT_VAL          0x1055
> -#define DW_IC_SAR_INIT_VAL          0x55
> -#define DW_IC_SS_SCL_HCNT_INIT_VAL  0x190
> -#define DW_IC_SS_SCL_LCNT_INIT_VAL  0x1d6
> -#define DW_IC_FS_SCL_HCNT_INIT_VAL  0x3c
> -#define DW_IC_FS_SCL_LCNT_INIT_VAL  0x82
> -#define DW_IC_INTR_MASK_INIT_VAL    0x8ff
> -#define DW_IC_STATUS_INIT_VAL       0x6
> -#define DW_IC_SDA_HOLD_INIT_VAL     0x1
> -#define DW_IC_SDA_SETUP_INIT_VAL    0x64
> -#define DW_IC_FS_SPKLEN_INIT_VAL    0x2
> -
> -#define DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS   BIT(7)
> -#define DW_IC_COMP_PARAM_1_HAS_DMA              0 /* bit 6 - DMA disabled. */
> -#define DW_IC_COMP_PARAM_1_INTR_IO              BIT(5)
> -#define DW_IC_COMP_PARAM_1_HC_COUNT_VAL         0 /* bit 4 - disabled */
> -#define DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE      (BIT(2) | BIT(3))
> -#define DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32    BIT(1) /* bits 0, 1 */
> -#define DW_IC_COMP_PARAM_1_INIT_VAL             \
> -    (DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32 | \
> -    DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE | \
> -    DW_IC_COMP_PARAM_1_HC_COUNT_VAL | \
> -    DW_IC_COMP_PARAM_1_INTR_IO | \
> -    DW_IC_COMP_PARAM_1_HAS_DMA | \
> -    DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS | \
> -    ((DESIGNWARE_I2C_RX_FIFO_SIZE - 1) << 8) | \
> -    ((DESIGNWARE_I2C_TX_FIFO_SIZE - 1) << 16))
> -#define DW_IC_COMP_VERSION_INIT_VAL             0x3132302a
> -#define DW_IC_COMP_TYPE_INIT_VAL                0x44570140
> +#ifndef DESIGNWARE_I2C_ERR_DEBUG
> +#define DESIGNWARE_I2C_ERR_DEBUG 0
> +#endif
> +
> +REG32(DW_IC_CON,                0x00) /* I2C control */
> +    FIELD(DW_IC_CON, STOP_DET_IF_MASTER_ACTIV, 10, 1)
> +    FIELD(DW_IC_CON, RX_FIFO_FULL_HLD_CTRL,     9, 1)
> +    FIELD(DW_IC_CON, TX_EMPTY_CTRL,             8, 1)
> +    FIELD(DW_IC_CON, STOP_IF_ADDRESSED,         7, 1)
> +    FIELD(DW_IC_CON, SLAVE_DISABLE,             6, 1)
> +    FIELD(DW_IC_CON, IC_RESTART_EN,             5, 1)
> +    FIELD(DW_IC_CON, 10BITADDR_MASTER,          4, 1)
> +    FIELD(DW_IC_CON, 10BITADDR_SLAVE,           3, 1)
> +    FIELD(DW_IC_CON, SPEED,                     1, 2)
> +    FIELD(DW_IC_CON, MASTER_MODE,               0, 1)
> +REG32(DW_IC_TAR,                0x04) /* I2C target address */
> +    FIELD(DW_IC_TAR, IC_10BITADDR_MASTER, 12,  1)
> +    FIELD(DW_IC_TAR, SPECIAL,             11,  1)
> +    FIELD(DW_IC_TAR, GC_OR_START,         10,  1)
> +    FIELD(DW_IC_TAR, ADDRESS,              0, 10)
> +REG32(DW_IC_SAR,                0x08) /* I2C slave address */
> +REG32(DW_IC_DATA_CMD,           0x10)
> +    FIELD(DW_IC_DATA_CMD, RESTART, 10, 1)
> +    FIELD(DW_IC_DATA_CMD, STOP,     9, 1)
> +    FIELD(DW_IC_DATA_CMD, CMD,      8, 1)
> +    FIELD(DW_IC_DATA_CMD, DAT,      0, 8)
> +REG32(DW_IC_SS_SCL_HCNT,        0x14) /* Standard speed i2c clock scl high count */
> +REG32(DW_IC_SS_SCL_LCNT,        0x18) /* Standard speed i2c clock scl low count */
> +REG32(DW_IC_FS_SCL_HCNT,        0x1c) /* Fast or fast plus i2c clock scl high count */
> +REG32(DW_IC_FS_SCL_LCNT,        0x20) /* Fast or fast plus i2c clock scl low count */
> +REG32(DW_IC_INTR_STAT,          0x2c)
> +REG32(DW_IC_INTR_MASK,          0x30) /* I2C Interrupt Mask */
> +REG32(DW_IC_RAW_INTR_STAT,      0x34) /* I2C raw interrupt status */
> +    /* DW_IC_INTR_STAT/INTR_MASK/RAW_INTR_STAT fields */
> +    SHARED_FIELD(DW_IC_INTR_RESTART_DET, 12, 1)
> +    SHARED_FIELD(DW_IC_INTR_GEN_CALL,    11, 1)
> +    SHARED_FIELD(DW_IC_INTR_START_DET,   10, 1)
> +    SHARED_FIELD(DW_IC_INTR_STOP_DET,    9, 1)
> +    SHARED_FIELD(DW_IC_INTR_ACTIVITY,    8, 1)
> +    SHARED_FIELD(DW_IC_INTR_RX_DONE,     7, 1)
> +    SHARED_FIELD(DW_IC_INTR_TX_ABRT,     6, 1)
> +    SHARED_FIELD(DW_IC_INTR_RD_REQ,      5, 1)
> +    SHARED_FIELD(DW_IC_INTR_TX_EMPTY,    4, 1) /* Hardware clear only. */
> +    SHARED_FIELD(DW_IC_INTR_TX_OVER,     3, 1)
> +    SHARED_FIELD(DW_IC_INTR_RX_FULL,     2, 1) /* Hardware clear only. */
> +    SHARED_FIELD(DW_IC_INTR_RX_OVER,     1, 1)
> +    SHARED_FIELD(DW_IC_INTR_RX_UNDER,    0, 1)
> +
> +#define DW_IC_INTR_ANY_MASK                \
> +            (DW_IC_INTR_RESTART_DET_MASK | \
> +             DW_IC_INTR_GEN_CALL_MASK    | \
> +             DW_IC_INTR_START_DET_MASK   | \
> +             DW_IC_INTR_STOP_DET_MASK    | \
> +             DW_IC_INTR_ACTIVITY_MASK    | \
> +             DW_IC_INTR_RX_DONE_MASK     | \
> +             DW_IC_INTR_TX_ABRT_MASK     | \
> +             DW_IC_INTR_RD_REQ_MASK      | \
> +             DW_IC_INTR_TX_EMPTY_MASK    | \
> +             DW_IC_INTR_TX_OVER_MASK     | \
> +             DW_IC_INTR_RX_FULL_MASK     | \
> +             DW_IC_INTR_RX_OVER_MASK     | \
> +             DW_IC_INTR_RX_UNDER_MASK)
> +
> +#define DW_IC_INTR_ANY_SW_CLEAR_MASK       \
> +            (DW_IC_INTR_ANY_MASK         & \
> +            ~(DW_IC_INTR_TX_EMPTY_MASK   | \
> +              DW_IC_INTR_RX_FULL_MASK))
> +
> +REG32(DW_IC_RX_TL,              0x38) /* I2C receive FIFO threshold */
> +REG32(DW_IC_TX_TL,              0x3c) /* I2C transmit FIFO threshold */
> +REG32(DW_IC_CLR_INTR,           0x40)
> +REG32(DW_IC_CLR_RX_UNDER,       0x44)
> +REG32(DW_IC_CLR_RX_OVER,        0x48)
> +REG32(DW_IC_CLR_TX_OVER,        0x4c)
> +REG32(DW_IC_CLR_RD_REQ,         0x50)
> +REG32(DW_IC_CLR_TX_ABRT,        0x54)
> +REG32(DW_IC_CLR_RX_DONE,        0x58)
> +REG32(DW_IC_CLR_ACTIVITY,       0x5c)
> +REG32(DW_IC_CLR_STOP_DET,       0x60)
> +REG32(DW_IC_CLR_START_DET,      0x64)
> +REG32(DW_IC_CLR_GEN_CALL,       0x68)
> +REG32(DW_IC_ENABLE,             0x6c) /* I2C enable */
> +    FIELD(DW_IC_ENABLE, TX_CMD_BLOCK, 2, 1)
> +    FIELD(DW_IC_ENABLE, ABORT,        1, 1)
> +    FIELD(DW_IC_ENABLE, ENABLE,       0, 1)
> +REG32(DW_IC_STATUS,             0x70) /* I2C status */
> +    FIELD(DW_IC_STATUS, SLV_ACTIVITY, 6, 1)
> +    FIELD(DW_IC_STATUS, MST_ACTIVITY, 5, 1)
> +    FIELD(DW_IC_STATUS, RFF,          4, 1)
> +    FIELD(DW_IC_STATUS, RFNE,         3, 1)
> +    FIELD(DW_IC_STATUS, TFE,          2, 1)
> +    FIELD(DW_IC_STATUS, TFNF,         1, 1)
> +    FIELD(DW_IC_STATUS, ACTIVITY,     0, 1)
> +REG32(DW_IC_TXFLR,              0x74) /* I2C transmit fifo level */
> +REG32(DW_IC_RXFLR,              0x78) /* I2C receive fifo level */
> +REG32(DW_IC_SDA_HOLD,           0x7c) /* I2C SDA hold time length */
> +REG32(DW_IC_TX_ABRT_SOURCE,     0x80) /* The I2C transmit abort source */
> +    FIELD(DW_IC_TX_ABRT_SOURCE, USER_ABRT,       16, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, SLVRD_INTX,      15, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, SLV_ARBLOST,     14, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, SLVFLUSH_TXFIFO, 13, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, ARB_LOST,        12, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, MASTER_DIS,      11, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, 10B_RD_NORSTRT,  10, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, SBYTE_NORSTRT,   9, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, HS_NORSTRT,      8, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, SBYTE_ACKDET,    7, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, HS_ACKDET,       6, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, GCALL_READ,      5, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, GCALL_NOACK,     4, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, TXDATA_NOACK,    3, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, 10ADDR2_NOACK,   2, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, 10ADDR1_NOACK,   1, 1)
> +    FIELD(DW_IC_TX_ABRT_SOURCE, 7B_ADDR_NOACK,   0, 1)
> +REG32(DW_IC_SLV_DATA_NACK_ONLY, 0x84)
> +REG32(DW_IC_DMA_CR,             0x88)
> +REG32(DW_IC_DMA_TDLR,           0x8c)
> +REG32(DW_IC_DMA_RDLR,           0x90)
> +REG32(DW_IC_SDA_SETUP,          0x94) /* I2C SDA setup */
> +REG32(DW_IC_ACK_GENERAL_CALL,   0x98)
> +REG32(DW_IC_ENABLE_STATUS,      0x9c) /* I2C enable status */
> +    FIELD(DW_IC_ENABLE_STATUS, SLV_RX_DATA_LOST,        2, 1)
> +    FIELD(DW_IC_ENABLE_STATUS, SLV_DISABLED_WHILE_BUSY, 1, 1)
> +    FIELD(DW_IC_ENABLE_STATUS, IC_EN,                   0, 1)
> +REG32(DW_IC_FS_SPKLEN,          0xa0) /* I2C SS, FS or FM+ spike suppression limit */
> +REG32(DW_IC_CLR_RESTART_DET,    0xa8)
> +REG32(DW_IC_COMP_PARAM_1,       0xf4) /* Component parameter */
> +    FIELD(DW_IC_COMP_PARAM_1, TX_FIFO_SIZE,       16, 8)
> +    FIELD(DW_IC_COMP_PARAM_1, RX_FIFO_SIZE,        8, 8)
> +    FIELD(DW_IC_COMP_PARAM_1, HAS_ENCODED_PARAMS,  7, 1)
> +    FIELD(DW_IC_COMP_PARAM_1, HAS_DMA,             6, 1)
> +    FIELD(DW_IC_COMP_PARAM_1, INTR_IO,             5, 1)
> +    FIELD(DW_IC_COMP_PARAM_1, HC_COUNT_VAL,        4, 1)
> +    FIELD(DW_IC_COMP_PARAM_1, HIGH_SPEED_MODE,     2, 2)
> +    FIELD(DW_IC_COMP_PARAM_1, APB_DATA_WIDTH_32,   0, 2)
> +REG32(DW_IC_COMP_VERSION,       0xf8) /* I2C component version */
> +REG32(DW_IC_COMP_TYPE,          0xfc) /* I2C component type */

This looks fine to me, again no datasheet so no idea if it's right though :)

For future reference, generally in QEMU when people say the Register
API they just mean this maco part above

>
>  static void dw_i2c_update_irq(DesignWareI2CState *s)
>  {
> -    int level;
> -    uint32_t intr = s->ic_raw_intr_stat & s->ic_intr_mask;
> -
> -    level = !!((intr & DW_IC_INTR_RX_UNDER) |
> -        (intr & DW_IC_INTR_RX_OVER) |
> -        (intr & DW_IC_INTR_RX_FULL) |
> -        (intr & DW_IC_INTR_TX_OVER) |
> -        (intr & DW_IC_INTR_TX_EMPTY) |
> -        (intr & DW_IC_INTR_RD_REQ) |
> -        (intr & DW_IC_INTR_TX_ABRT) |
> -        (intr & DW_IC_INTR_RX_DONE) |
> -        (intr & DW_IC_INTR_ACTIVITY) |
> -        (intr & DW_IC_INTR_STOP_DET) |
> -        (intr & DW_IC_INTR_START_DET) |
> -        (intr & DW_IC_INTR_GEN_CALL) |
> -        (intr & DW_IC_INTR_RESTART_DET)
> -        );
> -    qemu_set_irq(s->irq, level);
> +    uint32_t intr = s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];

Which gives you advantages like this and the FIELD set/clear which you
aren't using anyway.

> +
> +    qemu_set_irq(s->irq, !!(intr & DW_IC_INTR_ANY_MASK));
>  }
>
> -static uint8_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
> +static uint64_t dw_ic_data_cmd_reg_post_read(RegisterInfo *reg, uint64_t value)
>  {
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    g_assert(value == 0);
> +
>      if (s->status != DW_I2C_STATUS_RECEIVING) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%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;
> +            SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT,
> +                                    DW_IC_INTR_RX_UNDER, 1);
>              dw_i2c_update_irq(s);
>          }
>          return 0;
>      }
>
> -    g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
> +    g_assert(s->regs[R_DW_IC_RXFLR] == fifo8_num_used(&s->rx_fifo));
>
>      if (fifo8_is_empty(&s->rx_fifo)) {
> -        s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
> +        SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT, DW_IC_INTR_RX_UNDER, 1);
>          dw_i2c_update_irq(s);
>          return 0;
>      }
>
> -    s->ic_rxflr--;
> -    if (s->ic_rxflr <= s->ic_rx_tl) {
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
> +    s->regs[R_DW_IC_RXFLR]--;
> +    if (s->regs[R_DW_IC_RXFLR] <= s->regs[R_DW_IC_RX_TL]) {
> +        SHARED_ARRAY_FIELD_DP32(s->regs, R_DW_IC_RAW_INTR_STAT, DW_IC_INTR_RX_FULL, 0);
>          dw_i2c_update_irq(s);
>      }
>
>      return fifo8_pop(&s->rx_fifo);
>  }
>
> -static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t dw_ic_clr_intr_reg_post_read(RegisterInfo *reg, uint64_t value)
>  {
> -    uint64_t value = 0;
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
>
> -    DesignWareI2CState *s = opaque;
> +    g_assert(value == 0);
>
> -    switch (offset) {
> -    case DW_IC_CON:
> -        value = s->ic_con;
> +    switch (reg->access->addr) {
> +    case A_DW_IC_CLR_INTR:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_ANY_SW_CLEAR_MASK;
>          break;
> -    case DW_IC_TAR:
> -        value = s->ic_tar;
> +    case A_DW_IC_CLR_RX_UNDER:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_UNDER_MASK;
>          break;
> -    case DW_IC_SAR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_sar\n",
> -                      DEVICE(s)->canonical_path);
> -        value = s->ic_sar;
> -        break;
> -    case DW_IC_DATA_CMD:
> -        value = dw_i2c_read_ic_data_cmd(s);
> -        break;
> -    case DW_IC_SS_SCL_HCNT:
> -        value = s->ic_ss_scl_hcnt;
> +    case A_DW_IC_CLR_RX_OVER:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_OVER_MASK;
>          break;
> -    case DW_IC_SS_SCL_LCNT:
> -        value = s->ic_ss_scl_lcnt;
> +    case A_DW_IC_CLR_TX_OVER:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_OVER_MASK;
>          break;
> -    case DW_IC_FS_SCL_HCNT:
> -        value = s->ic_fs_scl_hcnt;
> +    case A_DW_IC_CLR_RD_REQ:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RD_REQ_MASK;
>          break;
> -    case DW_IC_FS_SCL_LCNT:
> -        value = s->ic_fs_scl_lcnt;
> +    case A_DW_IC_CLR_TX_ABRT:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_ABRT_MASK;
>          break;
> -    case DW_IC_INTR_STAT:
> -        value = s->ic_raw_intr_stat & s->ic_intr_mask;
> +    case A_DW_IC_CLR_RX_DONE:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_DONE_MASK;
>          break;
> -    case DW_IC_INTR_MASK:
> -        value = s->ic_intr_mask;
> +    case A_DW_IC_CLR_ACTIVITY:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_ACTIVITY_MASK;
>          break;
> -    case DW_IC_RAW_INTR_STAT:
> -        value = s->ic_raw_intr_stat;
> +    case A_DW_IC_CLR_STOP_DET:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_STOP_DET_MASK;
>          break;
> -    case DW_IC_RX_TL:
> -        value = s->ic_rx_tl;
> +    case A_DW_IC_CLR_START_DET:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_START_DET_MASK;
>          break;
> -    case DW_IC_TX_TL:
> -        value = s->ic_tx_tl;
> -        break;
> -    case DW_IC_CLR_INTR:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL |
> -            DW_IC_INTR_RESTART_DET |
> -            DW_IC_INTR_START_DET |
> -            DW_IC_INTR_STOP_DET |
> -            DW_IC_INTR_ACTIVITY |
> -            DW_IC_INTR_RX_DONE |
> -            DW_IC_INTR_TX_ABRT |
> -            DW_IC_INTR_RD_REQ |
> -            DW_IC_INTR_TX_OVER |
> -            DW_IC_INTR_RX_OVER |
> -            DW_IC_INTR_RX_UNDER);
> -        s->ic_tx_abrt_source = 0;
> -        dw_i2c_update_irq(s);
> +    case A_DW_IC_CLR_GEN_CALL:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_GEN_CALL_MASK;
>          break;
> -    case DW_IC_CLR_RX_UNDER:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_UNDER);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_RX_OVER:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_OVER);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_TX_OVER:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_OVER);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_RD_REQ:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RD_REQ);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_TX_ABRT:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_TX_ABRT);
> -        s->ic_tx_abrt_source = 0;
> -        dw_i2c_update_irq(s);
> +    case A_DW_IC_CLR_RESTART_DET:
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RESTART_DET_MASK;
>          break;
> -    case DW_IC_CLR_RX_DONE:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_RX_DONE);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_ACTIVITY:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_ACTIVITY);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_STOP_DET:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_STOP_DET);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_START_DET:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_START_DET);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_CLR_GEN_CALL:
> -        s->ic_raw_intr_stat &= ~(DW_IC_INTR_GEN_CALL);
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_ENABLE:
> -        value = s->ic_enable;
> -        break;
> -    case DW_IC_STATUS:
> -        value = s->ic_status;
> -        break;
> -    case DW_IC_TXFLR:
> -        value = s->ic_txflr;
> -        break;
> -    case DW_IC_RXFLR:
> -        value = s->ic_rxflr;
> -        break;
> -    case DW_IC_SDA_HOLD:
> -        value = s->ic_sda_hold;
> -        break;
> -    case DW_IC_TX_ABRT_SOURCE:
> -        value = s->ic_tx_abrt_source;
> -        break;
> -    case DW_IC_SLV_DATA_NACK_ONLY:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "%s: unsupported read - ic_slv_data_nack_only\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_CR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_cr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_TDLR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_tdlr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_RDLR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_dma_rdlr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_SDA_SETUP:
> -        value = s->ic_sda_setup;
> -        break;
> -    case DW_IC_ACK_GENERAL_CALL:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported read - ic_ack_general_call\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_ENABLE_STATUS:
> -        value = s->ic_enable_status;
> -        break;
> -    case DW_IC_FS_SPKLEN:
> -        value = s->ic_fs_spklen;
> -        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;
> -        break;
> -    case DW_IC_COMP_VERSION:
> -        value = s->ic_comp_version;
> -        break;
> -    case DW_IC_COMP_TYPE:
> -        value = s->ic_comp_type;
> -        break;
> -
> -    /* This register is invalid at this point. */
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
> -                      DEVICE(s)->canonical_path, offset);
> -        break;
> +        g_assert_not_reached();
>      }
>
> -    trace_dw_i2c_read(DEVICE(s)->canonical_path, offset, value);
> +    dw_i2c_update_irq(s);
>
> -    return value;
> +    return 0;
>  }
>
> -static void dw_i2c_write_ic_con(DesignWareI2CState *s, uint32_t value)
> +static uint64_t dw_ic_intr_stat_reg_post_read(RegisterInfo *reg, uint64_t value)
>  {
> -    if (value & DW_IC_CON_RX_FIFO_FULL_HLD_CTRL) {
> -        qemu_log_mask(LOG_UNIMP,
> -                      "%s: unsupported ic_con flag - RX_FIFO_FULL_HLD_CTRL\n",
> -                      DEVICE(s)->canonical_path);
> -    }
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
>
> -    if (!(s->ic_enable & DW_IC_ENABLE_ENABLE)) {
> -        s->ic_con = value & DW_IC_CON_MASK;
> -    } else {
> +    g_assert(value == 0);
> +
> +    return s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];
> +}
> +
> +static uint64_t dw_ic_unsupported_reg_post_read(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: unsupported read - %s\n",
> +                  DEVICE(s)->canonical_path, reg->access->name);
> +
> +    return 0;
> +}
> +
> +static uint64_t dw_ic_unsupported_reg_pre_write(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: unsupported write - %s\n",
> +                  DEVICE(s)->canonical_path, reg->access->name);
> +
> +    return 0;
> +}
> +
> +static uint64_t dw_ic_con_reg_pre_write(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    if (s->regs[R_DW_IC_ENABLE] & R_DW_IC_ENABLE_ENABLE_MASK) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: invalid setting to ic_con %d when ic_enable[0]==1\n",
> -                      DEVICE(s)->canonical_path, value);
> +                      DEVICE(s)->canonical_path, (int)value);
> +        return s->regs[R_DW_IC_CON]; /* keep old value */
>      }
> +
> +    return value;
>  }
>
>  static void dw_i2c_reset_to_idle(DesignWareI2CState *s)
>  {
> -        s->ic_enable_status &= ~DW_IC_ENABLE_STATUS_IC_EN;
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_OVER;
> -        s->ic_rxflr = 0;
> +        s->regs[R_DW_IC_ENABLE_STATUS] &= ~R_DW_IC_ENABLE_STATUS_IC_EN_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_EMPTY_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_FULL_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_UNDER_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_OVER_MASK;
> +        s->regs[R_DW_IC_RXFLR] = 0;
>          fifo8_reset(&s->rx_fifo);
> -        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
> +        s->regs[R_DW_IC_STATUS] &= ~R_DW_IC_STATUS_ACTIVITY_MASK;
>          s->status = DW_I2C_STATUS_IDLE;
>          dw_i2c_update_irq(s);
>  }
>
>  static void dw_ic_tx_abort(DesignWareI2CState *s, uint32_t src)
>  {
> -    s->ic_tx_abrt_source |= src;
> -    s->ic_raw_intr_stat |= DW_IC_INTR_TX_ABRT;
> +    s->regs[R_DW_IC_TX_ABRT_SOURCE] |= src;
> +    s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_TX_ABRT_MASK;
>      dw_i2c_reset_to_idle(s);
>      dw_i2c_update_irq(s);
>  }
>
> -static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
> +static void dw_ic_data_cmd_reg_post_write(RegisterInfo *reg, uint64_t value)
>  {
> -    int recv = !!(value & DW_IC_DATA_CMD_CMD);
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +    int recv = !!(value & R_DW_IC_DATA_CMD_CMD_MASK);
> +
> +    s->regs[R_DW_IC_DATA_CMD] = 0; /* Register has no storage */
>
>      if (s->status == DW_I2C_STATUS_IDLE ||
> -        s->ic_raw_intr_stat & DW_IC_INTR_TX_ABRT) {
> +        s->regs[R_DW_IC_RAW_INTR_STAT] & DW_IC_INTR_TX_ABRT_MASK) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Attempted to write to TX fifo when it is held in "
>                        "reset.\n", DEVICE(s)->canonical_path);
> @@ -470,9 +330,10 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
>
>      /* Send the address if it hasn't been sent yet. */
>      if (s->status == DW_I2C_STATUS_SENDING_ADDRESS) {
> -        int rv = i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv);
> +        int rv = i2c_start_transfer(s->bus,
> +                     ARRAY_FIELD_EX32(s->regs, DW_IC_TAR, ADDRESS), recv);
>          if (rv) {
> -            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
> +            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_7B_ADDR_NOACK_MASK);
>              return;
>          }
>          s->status = recv ? DW_I2C_STATUS_RECEIVING : DW_I2C_STATUS_SENDING;
> @@ -480,25 +341,27 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
>
>      /* Send data */
>      if (!recv) {
> -        int rv = i2c_send(s->bus, DW_IC_DATA_CMD_DAT(value));
> +        int rv = i2c_send(s->bus, FIELD_EX32(value, DW_IC_DATA_CMD, DAT));
>          if (rv) {
>              i2c_end_transfer(s->bus);
> -            dw_ic_tx_abort(s, DW_IC_TX_ABRT_TXDATA_NOACK);
> +            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_TXDATA_NOACK_MASK);
>              return;
>          }
>          dw_i2c_update_irq(s);
>      }
>
>      /* Restart command */
> -    if (value & DW_IC_DATA_CMD_RESTART && s->ic_con & DW_IC_CON_IC_RESTART_EN) {
> -        s->ic_raw_intr_stat |= DW_IC_INTR_RESTART_DET |
> -                               DW_IC_INTR_START_DET |
> -                               DW_IC_INTR_ACTIVITY;
> -        s->ic_status |= DW_IC_STATUS_ACTIVITY;
> +    if (value & R_DW_IC_DATA_CMD_RESTART_MASK &&
> +            s->regs[R_DW_IC_CON] & R_DW_IC_CON_IC_RESTART_EN_MASK) {
> +        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RESTART_DET_MASK |
> +                                          DW_IC_INTR_START_DET_MASK |
> +                                          DW_IC_INTR_ACTIVITY_MASK;
> +        s->regs[R_DW_IC_STATUS] |= R_DW_IC_STATUS_ACTIVITY_MASK;
>          dw_i2c_update_irq(s);
>
> -        if (i2c_start_transfer(s->bus, DW_IC_TAR_ADDRESS(s->ic_tar), recv)) {
> -            dw_ic_tx_abort(s, DW_IC_TX_ABRT_7B_ADDR_NOACK);
> +        if (i2c_start_transfer(s->bus,
> +                    ARRAY_FIELD_EX32(s->regs, DW_IC_TAR, ADDRESS), recv)) {
> +            dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_7B_ADDR_NOACK_MASK);
>              return;
>          }
>
> @@ -507,87 +370,113 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
>
>      /* Receive data */
>      if (recv) {
> -        g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
> +        g_assert(s->regs[R_DW_IC_RXFLR] == fifo8_num_used(&s->rx_fifo));
>
>          if (!fifo8_is_full(&s->rx_fifo)) {
>              fifo8_push(&s->rx_fifo, i2c_recv(s->bus));
> -            s->ic_rxflr++;
> +            s->regs[R_DW_IC_RXFLR]++;
>          } else {
> -            s->ic_raw_intr_stat |= DW_IC_INTR_RX_OVER;
> +            s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_OVER_MASK;
>              dw_i2c_update_irq(s);
>          }
>
> -        if (s->ic_rxflr > s->ic_rx_tl) {
> -            s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
> +        if (s->regs[R_DW_IC_RXFLR] > s->regs[R_DW_IC_RX_TL]) {
> +            s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_FULL_MASK;
>              dw_i2c_update_irq(s);
>          }
> -        if (value & DW_IC_DATA_CMD_STOP) {
> +        if (value & R_DW_IC_DATA_CMD_STOP_MASK) {
>              i2c_nack(s->bus);
>          }
>      }
>
>      /* Stop command */
> -    if (value & DW_IC_DATA_CMD_STOP) {
> -        s->ic_raw_intr_stat |= DW_IC_INTR_STOP_DET;
> -        s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_TX_EMPTY;
> +    if (value & R_DW_IC_DATA_CMD_STOP_MASK) {
> +        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_STOP_DET_MASK;
> +        s->regs[R_DW_IC_STATUS] &= ~R_DW_IC_STATUS_ACTIVITY_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_TX_EMPTY_MASK;
>          i2c_end_transfer(s->bus);
>          dw_i2c_update_irq(s);
>      }
>  }
>
> -static void dw_i2c_write_ic_enable(DesignWareI2CState *s, uint32_t value)
> +static void dw_ic_intr_mask_reg_post_write(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    dw_i2c_update_irq(s);
> +}
> +
> +static uint64_t dw_ic_enable_reg_pre_write(RegisterInfo *reg, uint64_t value)
>  {
> -    if (value & DW_IC_ENABLE_ENABLE && !(s->ic_con & DW_IC_CON_SLAVE_DISABLE)) {
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    if (value & R_DW_IC_ENABLE_ENABLE_MASK &&
> +            !(s->regs[R_DW_IC_CON] & R_DW_IC_CON_SLAVE_DISABLE_MASK)) {
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: Designware I2C slave mode is not supported.\n",
>                        DEVICE(s)->canonical_path);
> -        return;
> +        return s->regs[R_DW_IC_ENABLE]; /* keep old value */
>      }
>
> -    s->ic_enable = value & DW_IC_ENABLE_MASK;
> +    return value;
> +}
> +
> +static void dw_ic_enable_reg_post_write(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    s->regs[R_DW_IC_ENABLE] = value & R_DW_IC_ENABLE_ENABLE_MASK;
>
> -    if (value & DW_IC_ENABLE_ABORT || value & DW_IC_ENABLE_TX_CMD_BLOCK) {
> -        dw_ic_tx_abort(s, DW_IC_TX_ABRT_USER_ABRT);
> +    if (value & R_DW_IC_ENABLE_ABORT_MASK || value & R_DW_IC_ENABLE_TX_CMD_BLOCK_MASK) {
> +        dw_ic_tx_abort(s, R_DW_IC_TX_ABRT_SOURCE_USER_ABRT_MASK);
>          return;
>      }
>
> -    if (value & DW_IC_ENABLE_ENABLE) {
> -        s->ic_enable_status |= DW_IC_ENABLE_STATUS_IC_EN;
> -        s->ic_status |= DW_IC_STATUS_ACTIVITY;
> -        s->ic_raw_intr_stat |= DW_IC_INTR_ACTIVITY |
> -                               DW_IC_INTR_START_DET |
> -                               DW_IC_INTR_TX_EMPTY;
> +    if (value & R_DW_IC_ENABLE_ENABLE_MASK) {
> +        s->regs[R_DW_IC_ENABLE_STATUS] |= R_DW_IC_ENABLE_STATUS_IC_EN_MASK;
> +        s->regs[R_DW_IC_STATUS] |= R_DW_IC_STATUS_ACTIVITY_MASK;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_ACTIVITY_MASK |
> +                                          DW_IC_INTR_START_DET_MASK |
> +                                          DW_IC_INTR_TX_EMPTY_MASK;
>          s->status = DW_I2C_STATUS_SENDING_ADDRESS;
>          dw_i2c_update_irq(s);
> -    } else if ((value & DW_IC_ENABLE_ENABLE) == 0) {
> +    } else if ((value & R_DW_IC_ENABLE_ENABLE_MASK) == 0) {
>          dw_i2c_reset_to_idle(s);
>      }
> -
>  }
>
> -static void dw_i2c_write_ic_rx_tl(DesignWareI2CState *s, uint32_t value)
> +static uint64_t dw_ic_rx_tl_reg_pre_write(RegisterInfo *reg, uint64_t value)
>  {
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
>      /* Note that a value of 0 for ic_rx_tl indicates a threashold of 1. */
>      if (value > DESIGNWARE_I2C_RX_FIFO_SIZE - 1) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: invalid setting to ic_rx_tl %d\n",
> -                      DEVICE(s)->canonical_path, value);
> -        s->ic_rx_tl = DESIGNWARE_I2C_RX_FIFO_SIZE - 1;
> -    } else {
> -        s->ic_rx_tl = value;
> +                      DEVICE(s)->canonical_path, (int)value);
> +        return DESIGNWARE_I2C_RX_FIFO_SIZE - 1;
>      }
>
> -    if (s->ic_rxflr > s->ic_rx_tl && s->ic_enable & DW_IC_ENABLE_ENABLE) {
> -        s->ic_raw_intr_stat |= DW_IC_INTR_RX_FULL;
> +    return value;
> +}
> +
> +static void dw_ic_rx_tl_reg_post_write(RegisterInfo *reg, uint64_t value)
> +{
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
> +    if (s->regs[R_DW_IC_RXFLR] > s->regs[R_DW_IC_RX_TL] &&
> +            s->regs[R_DW_IC_ENABLE] & R_DW_IC_ENABLE_ENABLE_MASK) {
> +        s->regs[R_DW_IC_RAW_INTR_STAT] |= DW_IC_INTR_RX_FULL_MASK;
>      } else {
> -        s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
> +        s->regs[R_DW_IC_RAW_INTR_STAT] &= ~DW_IC_INTR_RX_FULL_MASK;
>      }
>      dw_i2c_update_irq(s);
>  }
>
> -static void dw_i2c_write_ic_tx_tl(DesignWareI2CState *s, uint32_t value)
> +static uint64_t dw_ic_tx_tl_reg_pre_write(RegisterInfo *reg, uint64_t value)
>  {
> +    DesignWareI2CState *s = DESIGNWARE_I2C(reg->opaque);
> +
>      /*
>       * Note that a value of 0 for ic_tx_tl indicates a threashold of 1.
>       * However, the tx threshold is not used in the model because commands are
> @@ -596,106 +485,154 @@ static void dw_i2c_write_ic_tx_tl(DesignWareI2CState *s, uint32_t value)
>      if (value > DESIGNWARE_I2C_TX_FIFO_SIZE - 1) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: invalid setting to ic_tx_tl %d\n",
> -                      DEVICE(s)->canonical_path, value);
> -        s->ic_tx_tl = DESIGNWARE_I2C_TX_FIFO_SIZE - 1;
> -    } else {
> -        s->ic_tx_tl = value;
> +                      DEVICE(s)->canonical_path, (int)value);
> +        return DESIGNWARE_I2C_TX_FIFO_SIZE - 1;
>      }
> -}
>
> -static void dw_i2c_write(void *opaque, hwaddr offset, uint64_t value,
> -                              unsigned size)
> -{
> -    DesignWareI2CState *s = opaque;
> -
> -    trace_dw_i2c_write(DEVICE(s)->canonical_path, offset, value);
> -
> -    /* The order of the registers are their order in memory. */
> -    switch (offset) {
> -    case DW_IC_CON:
> -        dw_i2c_write_ic_con(s, value);
> -        break;
> -    case DW_IC_TAR:
> -        s->ic_tar = value & DW_IC_TAR_MASK;
> -        break;
> -    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;
> -        break;
> -    case DW_IC_DATA_CMD:
> -        dw_i2c_write_ic_data_cmd(s, value);
> -        break;
> -    case DW_IC_SS_SCL_HCNT:
> -        s->ic_ss_scl_hcnt = value & DW_IC_SS_SCL_HCNT_MASK;
> -        break;
> -    case DW_IC_SS_SCL_LCNT:
> -        s->ic_ss_scl_lcnt = value & DW_IC_SS_SCL_LCNT_MASK;
> -        break;
> -    case DW_IC_FS_SCL_HCNT:
> -        s->ic_fs_scl_hcnt = value & DW_IC_FS_SCL_HCNT_MASK;
> -        break;
> -    case DW_IC_FS_SCL_LCNT:
> -        s->ic_fs_scl_lcnt = value & DW_IC_FS_SCL_LCNT_MASK;
> -        break;
> -    case DW_IC_INTR_MASK:
> -        s->ic_intr_mask = value & DW_IC_INTR_MASK_MASK;
> -        dw_i2c_update_irq(s);
> -        break;
> -    case DW_IC_RX_TL:
> -        dw_i2c_write_ic_rx_tl(s, value);
> -        break;
> -    case DW_IC_TX_TL:
> -        dw_i2c_write_ic_tx_tl(s, value);
> -        break;
> -    case DW_IC_ENABLE:
> -        dw_i2c_write_ic_enable(s, value);
> -        break;
> -    case DW_IC_SDA_HOLD:
> -        s->ic_sda_hold = value & DW_IC_SDA_HOLD_MASK;
> -        break;
> -    case DW_IC_SLV_DATA_NACK_ONLY:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "%s: unsupported write - ic_slv_data_nack_only\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_CR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_cr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_TDLR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_tdlr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_DMA_RDLR:
> -        qemu_log_mask(LOG_UNIMP, "%s: unsupported write - ic_dma_rdlr\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_SDA_SETUP:
> -        s->ic_sda_setup = value & DW_IC_SDA_SETUP_MASK;
> -        break;
> -    case DW_IC_ACK_GENERAL_CALL:
> -        qemu_log_mask(LOG_UNIMP,
> -                      "%s: unsupported write - ic_ack_general_call\n",
> -                      DEVICE(s)->canonical_path);
> -        break;
> -    case DW_IC_FS_SPKLEN:
> -        s->ic_fs_spklen = value & DW_IC_FS_SPKLEN_MASK;
> -        break;
> +    return value;
> +}
>
> -    /* This register is invalid at this point. */
> -    default:
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: write to invalid offset or readonly register 0x%"
> -                      HWADDR_PRIx "\n",
> -                      DEVICE(s)->canonical_path, offset);
> -        break;
> +static const RegisterAccessInfo designware_i2c_regs_info[] = {

QEMU doesn't expect you to implement the RegisterAccessInfo struct.
This struct exists as it's easy to autogenerate from RTL (or some
other source), but you don't have to write it out yourself. You can
use the REG/FIELD macros above and not RegisterAccessInfo, which is
what most devices do (Xilinx devices excluded).

Sorry for the extra hassle and work or converting it to
RegisterAccessInfo. Keeping the RegisterAccessInfo is fine, just
letting you know you don't have to do it next time.

I think squash these 4 together and that'll be it for the I2C patch in
your Atlantis series.

Alistair


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8
  2026-05-11 10:18   ` Philippe Mathieu-Daudé
@ 2026-05-15 18:23     ` Nicholas Piggin
  0 siblings, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-15 18:23 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Corey Minyard, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu

On Mon, May 11, 2026 at 12:18:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/5/26 14:05, Nicholas Piggin wrote:
> > Alistair suggested moving to Fifo8, which I think is
> > a good improvement.
> > 
> > Broken out for individual review, but IMO we should
> > squash before merge since it changes VMState format.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/i2c/designware_i2c.c         | 37 ++++++++++++++++++++-------------
> >   include/hw/i2c/designware_i2c.h |  7 +++----
> >   2 files changed, 25 insertions(+), 19 deletions(-)
> 
> 
> > @@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
> >    * @ic_comp_version: I2C component version register
> >    * @ic_comp_type: I2C component type register
> >    * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
> > - * @rx_cur: The current position of rx_fifo.
> > - * @status: The current status of the SMBus.
> >    */
> >   typedef struct DesignWareI2CState {
> >       SysBusDevice parent_obj;
> > @@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
> >       uint32_t ic_comp_version;
> >       uint32_t ic_comp_type;
> > -    uint8_t      rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
> > -    uint8_t      rx_cur;
> > +    /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
> 
> Why not remove ic_rxflr then?

I did consider that... but the next patch to rework the registers
adds storage for them all as a block. We could still just ignore
the storage and use fifo8_num_used() instead but I was vaguely
worried about complexity of vmstate compat if we change things, but
not sure if that is a rational concern. In the end the code is
quite simple and only a couple of places where this is adjusted so
I kept it this way.

Thanks,
Nick


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API
  2026-05-13  2:03   ` Alistair Francis
@ 2026-05-15 18:34     ` Nicholas Piggin
  0 siblings, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-15 18:34 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Corey Minyard, Alistair Francis, Daniel Henrique Barboza,
	Chao Liu, Chris Rauer, Michael Ellerman, Joel Stanley,
	Anirudh Srinivasan, Portia Stephens, qemu-riscv, qemu-devel,
	Hao Wu, Philippe Mathieu-Daudé

On Wed, May 13, 2026 at 12:03:17PM +1000, Alistair Francis wrote:
> On Thu, May 7, 2026 at 10:07 PM Nicholas Piggin <npiggin@gmail.com> wrote:

[...]

> > +REG32(DW_IC_COMP_PARAM_1,       0xf4) /* Component parameter */
> > +    FIELD(DW_IC_COMP_PARAM_1, TX_FIFO_SIZE,       16, 8)
> > +    FIELD(DW_IC_COMP_PARAM_1, RX_FIFO_SIZE,        8, 8)
> > +    FIELD(DW_IC_COMP_PARAM_1, HAS_ENCODED_PARAMS,  7, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HAS_DMA,             6, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, INTR_IO,             5, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HC_COUNT_VAL,        4, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HIGH_SPEED_MODE,     2, 2)
> > +    FIELD(DW_IC_COMP_PARAM_1, APB_DATA_WIDTH_32,   0, 2)
> > +REG32(DW_IC_COMP_VERSION,       0xf8) /* I2C component version */
> > +REG32(DW_IC_COMP_TYPE,          0xfc) /* I2C component type */
> 
> This looks fine to me, again no datasheet so no idea if it's right though :)
> 
> For future reference, generally in QEMU when people say the Register
> API they just mean this maco part above

Ah, okay!

> >  static void dw_i2c_update_irq(DesignWareI2CState *s)
> >  {
> > -    int level;
> > -    uint32_t intr = s->ic_raw_intr_stat & s->ic_intr_mask;
> > -
> > -    level = !!((intr & DW_IC_INTR_RX_UNDER) |
> > -        (intr & DW_IC_INTR_RX_OVER) |
> > -        (intr & DW_IC_INTR_RX_FULL) |
> > -        (intr & DW_IC_INTR_TX_OVER) |
> > -        (intr & DW_IC_INTR_TX_EMPTY) |
> > -        (intr & DW_IC_INTR_RD_REQ) |
> > -        (intr & DW_IC_INTR_TX_ABRT) |
> > -        (intr & DW_IC_INTR_RX_DONE) |
> > -        (intr & DW_IC_INTR_ACTIVITY) |
> > -        (intr & DW_IC_INTR_STOP_DET) |
> > -        (intr & DW_IC_INTR_START_DET) |
> > -        (intr & DW_IC_INTR_GEN_CALL) |
> > -        (intr & DW_IC_INTR_RESTART_DET)
> > -        );
> > -    qemu_set_irq(s->irq, level);
> > +    uint32_t intr = s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];
> 
> Which gives you advantages like this and the FIELD set/clear which you
> aren't using anyway.

Yeah it's quite nice. I started using FIELD set/clear etc but it looked
to make the patch harder to read and a bit harder to script it. Things
might be cleaned up a bit, although there is not much multi-bit field
manipulation in the driver where those kind fo macros help most.

> > +    return value;
> > +}
> >
> > -    /* This register is invalid at this point. */
> > -    default:
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "%s: write to invalid offset or readonly register 0x%"
> > -                      HWADDR_PRIx "\n",
> > -                      DEVICE(s)->canonical_path, offset);
> > -        break;
> > +static const RegisterAccessInfo designware_i2c_regs_info[] = {
> 
> QEMU doesn't expect you to implement the RegisterAccessInfo struct.
> This struct exists as it's easy to autogenerate from RTL (or some
> other source), but you don't have to write it out yourself. You can
> use the REG/FIELD macros above and not RegisterAccessInfo, which is
> what most devices do (Xilinx devices excluded).
> 
> Sorry for the extra hassle and work or converting it to
> RegisterAccessInfo. Keeping the RegisterAccessInfo is fine, just
> letting you know you don't have to do it next time.

Hah! That's fine, it was interesting to try using it anyway. It makes
things more consistent and hopefully less error prone, and ended up with
fewer lines of code.

> I think squash these 4 together and that'll be it for the I2C patch in
> your Atlantis series.

Thanks,
Nick


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] hw/i2c: Add designware i2c controller
  2026-05-10 13:03 ` [PATCH 0/4] hw/i2c: Add designware i2c controller Corey Minyard
@ 2026-05-15 23:00   ` Nicholas Piggin
  0 siblings, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2026-05-15 23:00 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Alistair Francis, Daniel Henrique Barboza, Chao Liu, Chris Rauer,
	Michael Ellerman, Joel Stanley, Anirudh Srinivasan,
	Portia Stephens, qemu-riscv, qemu-devel, Hao Wu,
	Philippe Mathieu-Daudé

On Sun, May 10, 2026 at 08:03:53AM -0500, Corey Minyard wrote:
> On Thu, May 07, 2026 at 10:05:18PM +1000, Nicholas Piggin wrote:
> > Hi,
> > 
> > This series contains the DW I2C model written by Chris Rauer and
> > updated for the Tenstorrent Atlantis machine recently. There was
> > some more review comment on that submission and so we decided to
> > take the I2C device out of that series and work on it separately,
> > see here:
> > 
> > https://lore.kernel.org/qemu-devel/20260425131721.932250-1-joel@jms.id.au/T/#mb1ef2824c2f1f37bf4574dc1ef0fb95566c3a2f2
> > 
> > The big thing suggested was to move to the QEMU register API. That
> > is a big change and difficult to review, so I have split that and
> > a some smaller changes out into their own patches. I don't expect
> > detailed reviews on the register API patch -- it's quite mechanical
> > and I did attempt to verify it by diff'ing register traces. But it
> > would be good to make sure maintainers are happy to go that way.
> > 
> > Unfortunately the patch 1 was quite well reviewed and tested so
> > incremental changes would be preferable, but it is painful to maintain
> > migration compatibility across these changes.
> 
> I had a few comments on the first patch, but they were all fixed in
> later patches.  From my review this all looks good.
> 
> Yes, please squash these as you suggested in the second patch.
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>

Thank you for the Ack, Corey. Since everybody is happier with it now
I will squash and submit it with the next revision of the tt-atlantis
series.

Thanks,
Nick


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-05-15 23:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-07 12:05 [PATCH 0/4] hw/i2c: Add designware i2c controller Nicholas Piggin
2026-05-07 12:05 ` [PATCH 1/4] " Nicholas Piggin
2026-05-11 10:20   ` Philippe Mathieu-Daudé
2026-05-07 12:05 ` [PATCH 2/4] [RFC] hw/i2c/designware_i2c: Switch to Fifo8 Nicholas Piggin
2026-05-11 10:18   ` Philippe Mathieu-Daudé
2026-05-15 18:23     ` Nicholas Piggin
2026-05-07 12:05 ` [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API Nicholas Piggin
2026-05-13  2:03   ` Alistair Francis
2026-05-15 18:34     ` Nicholas Piggin
2026-05-07 12:05 ` [PATCH 4/4] [RFC] hw/i2c/designware_i2c: add SMBUS_INTR_MASK Nicholas Piggin
2026-05-11 10:19   ` Philippe Mathieu-Daudé
2026-05-10 13:03 ` [PATCH 0/4] hw/i2c: Add designware i2c controller Corey Minyard
2026-05-15 23:00   ` Nicholas Piggin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.