qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/7] aspeed: i2c: Add new mode support
@ 2022-03-31  4:32 Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 1/7] hw/registerfields: Add shared fields macros Joe Komlodi
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

Hi all,

This series migrates the Aspeed I2C controller to use the register API,
and then adds new mode support.

New mode has some behavior changes and a register layout change compared
to old mode.

The series starts by adding "shared field" macros to help simplify logic
when adding new mode.
Generally, the macros are to be used in cases where register locations
change depending on what mode a peripheral is operating in, but
the fields in those registers have the same offsets.
Specifically, this happens very frequently with new and old I2C modes on
Aspeed I2C.

After that we migrate over to the register API and then add new mode
support.

Thanks!
Joe

Joe Komlodi (7):
  hw/registerfields: Add shared fields macros
  aspeed: i2c: Add ctrl_global_rsvd property
  aspeed: i2c: Migrate to registerfields API
  aspeed: i2c: Use reg array instead of individual vars
  aspeed: i2c: Add new mode support
  aspeed: i2c: Add PKT_DONE IRQ to trace
  aspeed: i2c: Move regs and helpers to header file

 hw/arm/aspeed_ast2600.c     |   2 +
 hw/i2c/aspeed_i2c.c         | 792 ++++++++++++++++++++++--------------
 hw/i2c/trace-events         |   2 +-
 include/hw/i2c/aspeed_i2c.h | 282 ++++++++++++-
 include/hw/registerfields.h |  70 ++++
 5 files changed, 828 insertions(+), 320 deletions(-)

-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 1/7] hw/registerfields: Add shared fields macros
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-04-06 10:26   ` Cédric Le Goater
  2022-03-31  4:32 ` [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property Joe Komlodi
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

Occasionally a peripheral will have different operating modes, where the
MMIO layout changes, but some of the register fields have the same offsets
and behaviors.

To help support this, we add SHARED_FIELD_XX macros that create SHIFT,
LENGTH, and MASK macros for the fields that are shared across registers,
and accessors for these fields.

An example use may look as follows:
There is a peripheral with registers REG_MODE1 and REG_MODE2 at
different addreses, and both have a field FIELD1 initialized by
SHARED_FIELD().

Depending on what mode the peripheral is operating in, the user could
extract FIELD1 via
SHARED_ARRAY_FIELD_EX32(s->regs, R_REG_MODE1, FIELD1)
or
SHARED_ARRAY_FIELD_EX32(s->regs, R_REG_MODE2, FIELD1)

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: Id3dc53e7d2f8741c95697cbae69a81bb699fa3cb
---
 include/hw/registerfields.h | 70 +++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
index f2a3c9c41f..cf3bc3a6e3 100644
--- a/include/hw/registerfields.h
+++ b/include/hw/registerfields.h
@@ -108,4 +108,74 @@
 #define ARRAY_FIELD_DP64(regs, reg, field, val)                           \
     (regs)[R_ ## reg] = FIELD_DP64((regs)[R_ ## reg], reg, field, val);
 
+
+/*
+ * These macros can be used for defining and extracting fields that have the
+ * same bit position across multiple registers.
+ */
+
+/* Define shared SHIFT, LENGTH, and MASK constants */
+#define SHARED_FIELD(name, shift, length)   \
+    enum { name ## _ ## SHIFT = (shift)};   \
+    enum { name ## _ ## LENGTH = (length)}; \
+    enum { name ## _ ## MASK = MAKE_64BIT_MASK(shift, length)};
+
+/* Extract a shared field */
+#define SHARED_FIELD_EX8(storage, field) \
+    extract8((storage), field ## _SHIFT, field ## _LENGTH)
+
+#define SHARED_FIELD_EX16(storage, field) \
+    extract16((storage), field ## _SHIFT, field ## _LENGTH)
+
+#define SHARED_FIELD_EX32(storage, field) \
+    extract32((storage), field ## _SHIFT, field ## _LENGTH)
+
+#define SHARED_FIELD_EX64(storage, field) \
+    extract64((storage), field ## _SHIFT, field ## _LENGTH)
+
+/* Extract a shared field from a register array */
+#define SHARED_ARRAY_FIELD_EX32(regs, offset, field) \
+    SHARED_FIELD_EX32((regs)[(offset)], field)
+#define SHARED_ARRAY_FIELD_EX64(regs, offset, field) \
+    SHARED_FIELD_EX64((regs)[(offset)], field)
+
+/* Deposit a shared field */
+#define SHARED_FIELD_DP8(storage, field, val) ({                        \
+    struct {                                                            \
+        unsigned int v:field ## _LENGTH;                                \
+    } _v = { .v = val };                                                \
+    uint8_t _d;                                                         \
+    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
+    _d; })
+
+#define SHARED_FIELD_DP16(storage, field, val) ({                       \
+    struct {                                                            \
+        unsigned int v:field ## _LENGTH;                                \
+    } _v = { .v = val };                                                \
+    uint16_t _d;                                                        \
+    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
+    _d; })
+
+#define SHARED_FIELD_DP32(storage, field, val) ({                       \
+    struct {                                                            \
+        unsigned int v:field ## _LENGTH;                                \
+    } _v = { .v = val };                                                \
+    uint32_t _d;                                                        \
+    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
+    _d; })
+
+#define SHARED_FIELD_DP64(storage, field, val) ({                       \
+    struct {                                                            \
+        uint64_t v:field ## _LENGTH;                                    \
+    } _v = { .v = val };                                                \
+    uint64_t _d;                                                        \
+    _d = deposit64((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
+    _d; })
+
+/* Deposit a shared field to a register array */
+#define SHARED_ARRAY_FIELD_DP32(regs, offset, field, val) \
+    (regs)[(offset)] = SHARED_FIELD_DP32((regs)[(offset)], field, val);
+#define SHARED_ARRAY_FIELD_DP64(regs, offset, field, val) \
+    (regs)[(offset)] = SHARED_FIELD_DP64((regs)[(offset)], field, val);
+
 #endif
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 1/7] hw/registerfields: Add shared fields macros Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-04-06  9:57   ` Cédric Le Goater
  2022-03-31  4:32 ` [RFC PATCH 3/7] aspeed: i2c: Migrate to registerfields API Joe Komlodi
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

The Aspeed I2C controller is used across other SKUs that have different
reserved bits for the ctrl_global_rsvd register.

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: I606c5933c527274a9d2b0afe559b2e895767636c
---
 hw/arm/aspeed_ast2600.c     | 2 ++
 hw/i2c/aspeed_i2c.c         | 4 ++++
 include/hw/i2c/aspeed_i2c.h | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 43f8223139..9f14a35a75 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -344,6 +344,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                    serial_hd(0), DEVICE_LITTLE_ENDIAN);
 
     /* I2C */
+    object_property_set_int(OBJECT(&s->i2c), "ctrl-global-rsvd", 0xfffc3e00,
+                            &error_abort);
     object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
                              &error_abort);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 03a4f5a910..97eb9d5792 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -648,6 +648,7 @@ static void aspeed_i2c_ctrl_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case I2C_CTRL_GLOBAL:
+        value &= ~s->ctrl_global_rsvd;
         s->ctrl_global = value;
         break;
     case I2C_CTRL_STATUS:
@@ -730,6 +731,7 @@ static const VMStateDescription aspeed_i2c_vmstate = {
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(intr_status, AspeedI2CState),
+        VMSTATE_UINT32(ctrl_global_rsvd, AspeedI2CState),
         VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
                              ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
                              AspeedI2CBus),
@@ -828,6 +830,8 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
 static Property aspeed_i2c_properties[] = {
     DEFINE_PROP_LINK("dram", AspeedI2CState, dram_mr,
                      TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_UINT32("ctrl-global-rsvd", AspeedI2CState, ctrl_global_rsvd,
+                       0xfffffffe),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 4b9be09274..3912fcc3ff 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -71,6 +71,8 @@ struct AspeedI2CState {
     MemoryRegion pool_iomem;
     uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
 
+    uint32_t ctrl_global_rsvd;
+
     AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
     MemoryRegion *dram_mr;
     AddressSpace dram_as;
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 3/7] aspeed: i2c: Migrate to registerfields API
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 1/7] hw/registerfields: Add shared fields macros Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars Joe Komlodi
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

This cleans up some of the field accessing, setting, and clearing
bitwise operations, and wraps them in macros instead.

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: I33018d6325fa04376e7c29dc4a49ab389a8e333a
---
 hw/i2c/aspeed_i2c.c | 393 ++++++++++++++++++++++----------------------
 1 file changed, 196 insertions(+), 197 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 97eb9d5792..be81a798cc 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -28,70 +28,61 @@
 #include "hw/i2c/aspeed_i2c.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "hw/registerfields.h"
 #include "trace.h"
 
 /* I2C Global Register */
-
-#define I2C_CTRL_STATUS         0x00        /* Device Interrupt Status */
-#define I2C_CTRL_ASSIGN         0x08        /* Device Interrupt Target
-                                               Assignment */
-#define I2C_CTRL_GLOBAL         0x0C        /* Global Control Register */
-#define   I2C_CTRL_SRAM_EN                 BIT(0)
+REG32(I2C_CTRL_STATUS, 0x0) /* Device Interrupt Status */
+REG32(I2C_CTRL_ASSIGN, 0x8) /* Device Interrupt Target Assignment */
+REG32(I2C_CTRL_GLOBAL, 0xC) /* Global Control Register */
+    FIELD(I2C_CTRL_GLOBAL, SRAM_EN, 0, 1)
 
 /* I2C Device (Bus) Register */
-
-#define I2CD_FUN_CTRL_REG       0x00       /* I2CD Function Control  */
-#define   I2CD_POOL_PAGE_SEL(x)            (((x) >> 20) & 0x7)  /* AST2400 */
-#define   I2CD_M_SDA_LOCK_EN               (0x1 << 16)
-#define   I2CD_MULTI_MASTER_DIS            (0x1 << 15)
-#define   I2CD_M_SCL_DRIVE_EN              (0x1 << 14)
-#define   I2CD_MSB_STS                     (0x1 << 9)
-#define   I2CD_SDA_DRIVE_1T_EN             (0x1 << 8)
-#define   I2CD_M_SDA_DRIVE_1T_EN           (0x1 << 7)
-#define   I2CD_M_HIGH_SPEED_EN             (0x1 << 6)
-#define   I2CD_DEF_ADDR_EN                 (0x1 << 5)
-#define   I2CD_DEF_ALERT_EN                (0x1 << 4)
-#define   I2CD_DEF_ARP_EN                  (0x1 << 3)
-#define   I2CD_DEF_GCALL_EN                (0x1 << 2)
-#define   I2CD_SLAVE_EN                    (0x1 << 1)
-#define   I2CD_MASTER_EN                   (0x1)
-
-#define I2CD_AC_TIMING_REG1     0x04       /* Clock and AC Timing Control #1 */
-#define I2CD_AC_TIMING_REG2     0x08       /* Clock and AC Timing Control #1 */
-#define I2CD_INTR_CTRL_REG      0x0c       /* I2CD Interrupt Control */
-#define I2CD_INTR_STS_REG       0x10       /* I2CD Interrupt Status */
-
-#define   I2CD_INTR_SLAVE_ADDR_MATCH       (0x1 << 31) /* 0: addr1 1: addr2 */
-#define   I2CD_INTR_SLAVE_ADDR_RX_PENDING  (0x1 << 30)
-/* bits[19-16] Reserved */
-
-/* All bits below are cleared by writing 1 */
-#define   I2CD_INTR_SLAVE_INACTIVE_TIMEOUT (0x1 << 15)
-#define   I2CD_INTR_SDA_DL_TIMEOUT         (0x1 << 14)
-#define   I2CD_INTR_BUS_RECOVER_DONE       (0x1 << 13)
-#define   I2CD_INTR_SMBUS_ALERT            (0x1 << 12) /* Bus [0-3] only */
-#define   I2CD_INTR_SMBUS_ARP_ADDR         (0x1 << 11) /* Removed */
-#define   I2CD_INTR_SMBUS_DEV_ALERT_ADDR   (0x1 << 10) /* Removed */
-#define   I2CD_INTR_SMBUS_DEF_ADDR         (0x1 << 9)  /* Removed */
-#define   I2CD_INTR_GCALL_ADDR             (0x1 << 8)  /* Removed */
-#define   I2CD_INTR_SLAVE_ADDR_RX_MATCH    (0x1 << 7)  /* use RX_DONE */
-#define   I2CD_INTR_SCL_TIMEOUT            (0x1 << 6)
-#define   I2CD_INTR_ABNORMAL               (0x1 << 5)
-#define   I2CD_INTR_NORMAL_STOP            (0x1 << 4)
-#define   I2CD_INTR_ARBIT_LOSS             (0x1 << 3)
-#define   I2CD_INTR_RX_DONE                (0x1 << 2)
-#define   I2CD_INTR_TX_NAK                 (0x1 << 1)
-#define   I2CD_INTR_TX_ACK                 (0x1 << 0)
-
-#define I2CD_CMD_REG            0x14       /* I2CD Command/Status */
-#define   I2CD_SDA_OE                      (0x1 << 28)
-#define   I2CD_SDA_O                       (0x1 << 27)
-#define   I2CD_SCL_OE                      (0x1 << 26)
-#define   I2CD_SCL_O                       (0x1 << 25)
-#define   I2CD_TX_TIMING                   (0x1 << 24)
-#define   I2CD_TX_STATUS                   (0x1 << 23)
-
-#define   I2CD_TX_STATE_SHIFT              19 /* Tx State Machine */
+REG32(I2CD_FUN_CTRL, 0x0) /* I2CD Function Control  */
+    FIELD(I2CD_FUN_CTRL, POOL_PAGE_SEL, 20, 3) /* AST2400 */
+    FIELD(I2CD_FUN_CTRL, M_SDA_LOCK_EN, 16, 1)
+    FIELD(I2CD_FUN_CTRL, MULTI_MASTER_DIS, 15, 1)
+    FIELD(I2CD_FUN_CTRL, M_SCL_DRIVE_EN, 14, 1)
+    FIELD(I2CD_FUN_CTRL, MSB_STS, 9, 1)
+    FIELD(I2CD_FUN_CTRL, SDA_DRIVE_IT_EN, 8, 1)
+    FIELD(I2CD_FUN_CTRL, M_SDA_DRIVE_IT_EN, 7, 1)
+    FIELD(I2CD_FUN_CTRL, M_HIGH_SPEED_EN, 6, 1)
+    FIELD(I2CD_FUN_CTRL, DEF_ADDR_EN, 5, 1)
+    FIELD(I2CD_FUN_CTRL, DEF_ALERT_EN, 4, 1)
+    FIELD(I2CD_FUN_CTRL, DEF_ARP_EN, 3, 1)
+    FIELD(I2CD_FUN_CTRL, DEF_GCALL_EN, 2, 1)
+    FIELD(I2CD_FUN_CTRL, SLAVE_EN, 1, 1)
+    FIELD(I2CD_FUN_CTRL, MASTER_EN, 0, 1)
+REG32(I2CD_AC_TIMING1, 0x04) /* Clock and AC Timing Control #1 */
+REG32(I2CD_AC_TIMING2, 0x08) /* Clock and AC Timing Control #2 */
+REG32(I2CD_INTR_CTRL, 0x0C)  /* I2CD Interrupt Control */
+REG32(I2CD_INTR_STS, 0x10)   /* I2CD Interrupt Status */
+    FIELD(I2CD_INTR_STS, SLAVE_ADDR_MATCH, 31, 1)    /* 0: addr1 1: addr2 */
+    FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_PENDING, 29, 1)
+    FIELD(I2CD_INTR_STS, SLAVE_INACTIVE_TIMEOUT, 15, 1)
+    FIELD(I2CD_INTR_STS, SDA_DL_TIMEOUT, 14, 1)
+    FIELD(I2CD_INTR_STS, BUS_RECOVER_DONE, 13, 1)
+    FIELD(I2CD_INTR_STS, SMBUS_ALERT, 12, 1)            /* Bus [0-3] only */
+    FIELD(I2CD_INTR_STS, SMBUS_ARP_ADDR, 11, 1)         /* Removed */
+    FIELD(I2CD_INTR_STS, SMBUS_DEV_ALERT_ADDR, 10, 1)   /* Removed */
+    FIELD(I2CD_INTR_STS, SMBUS_DEF_ADDR, 9, 1)          /* Removed */
+    FIELD(I2CD_INTR_STS, GCALL_ADDR, 8, 1)              /* Removed */
+    FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)     /* use RX_DONE */
+    FIELD(I2CD_INTR_STS, SCL_TIMEOUT, 6, 1)
+    FIELD(I2CD_INTR_STS, ABNORMAL, 5, 1)
+    FIELD(I2CD_INTR_STS, NORMAL_STOP, 4, 1)
+    FIELD(I2CD_INTR_STS, ARBIT_LOSS, 3, 1)
+    FIELD(I2CD_INTR_STS, RX_DONE, 2, 1)
+    FIELD(I2CD_INTR_STS, TX_NAK, 1, 1)
+    FIELD(I2CD_INTR_STS, TX_ACK, 0, 1)
+REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
+    FIELD(I2CD_CMD, SDA_OE, 28, 1)
+    FIELD(I2CD_CMD, SDA_O, 27, 1)
+    FIELD(I2CD_CMD, SCL_OE, 26, 1)
+    FIELD(I2CD_CMD, SCL_O, 25, 1)
+    FIELD(I2CD_CMD, TX_TIMING, 23, 2)
+    FIELD(I2CD_CMD, TX_STATE, 19, 4)
+/* Tx State Machine */
 #define   I2CD_TX_STATE_MASK                  0xf
 #define     I2CD_IDLE                         0x0
 #define     I2CD_MACTIVE                      0x8
@@ -108,51 +99,47 @@
 #define     I2CD_STXD                         0x6
 #define     I2CD_SRXACK                       0x7
 #define     I2CD_RECOVER                      0x3
-
-#define   I2CD_SCL_LINE_STS                (0x1 << 18)
-#define   I2CD_SDA_LINE_STS                (0x1 << 17)
-#define   I2CD_BUS_BUSY_STS                (0x1 << 16)
-#define   I2CD_SDA_OE_OUT_DIR              (0x1 << 15)
-#define   I2CD_SDA_O_OUT_DIR               (0x1 << 14)
-#define   I2CD_SCL_OE_OUT_DIR              (0x1 << 13)
-#define   I2CD_SCL_O_OUT_DIR               (0x1 << 12)
-#define   I2CD_BUS_RECOVER_CMD_EN          (0x1 << 11)
-#define   I2CD_S_ALT_EN                    (0x1 << 10)
-
-/* Command Bit */
-#define   I2CD_RX_DMA_ENABLE               (0x1 << 9)
-#define   I2CD_TX_DMA_ENABLE               (0x1 << 8)
-#define   I2CD_RX_BUFF_ENABLE              (0x1 << 7)
-#define   I2CD_TX_BUFF_ENABLE              (0x1 << 6)
-#define   I2CD_M_STOP_CMD                  (0x1 << 5)
-#define   I2CD_M_S_RX_CMD_LAST             (0x1 << 4)
-#define   I2CD_M_RX_CMD                    (0x1 << 3)
-#define   I2CD_S_TX_CMD                    (0x1 << 2)
-#define   I2CD_M_TX_CMD                    (0x1 << 1)
-#define   I2CD_M_START_CMD                 (0x1)
-
-#define I2CD_DEV_ADDR_REG       0x18       /* Slave Device Address */
-#define I2CD_POOL_CTRL_REG      0x1c       /* Pool Buffer Control */
-#define   I2CD_POOL_RX_COUNT(x)            (((x) >> 24) & 0xff)
-#define   I2CD_POOL_RX_SIZE(x)             ((((x) >> 16) & 0xff) + 1)
-#define   I2CD_POOL_TX_COUNT(x)            ((((x) >> 8) & 0xff) + 1)
-#define   I2CD_POOL_OFFSET(x)              (((x) & 0x3f) << 2)  /* AST2400 */
-#define I2CD_BYTE_BUF_REG       0x20       /* Transmit/Receive Byte Buffer */
-#define   I2CD_BYTE_BUF_TX_SHIFT           0
-#define   I2CD_BYTE_BUF_TX_MASK            0xff
-#define   I2CD_BYTE_BUF_RX_SHIFT           8
-#define   I2CD_BYTE_BUF_RX_MASK            0xff
-#define I2CD_DMA_ADDR           0x24       /* DMA Buffer Address */
-#define I2CD_DMA_LEN            0x28       /* DMA Transfer Length < 4KB */
+    FIELD(I2CD_CMD, SCL_LINE_STS, 18, 1)
+    FIELD(I2CD_CMD, SDA_LINE_STS, 17, 1)
+    FIELD(I2CD_CMD, BUS_BUSY_STS, 16, 1)
+    FIELD(I2CD_CMD, SDA_OE_OUT_DIR, 15, 1)
+    FIELD(I2CD_CMD, SDA_O_OUT_DIR, 14, 1)
+    FIELD(I2CD_CMD, SCL_OE_OUT_DIR, 13, 1)
+    FIELD(I2CD_CMD, SCL_O_OUT_DIR, 12, 1)
+    FIELD(I2CD_CMD, BUS_RECOVER_CMD_EN, 11, 1)
+    FIELD(I2CD_CMD, S_ALT_EN, 10, 1)
+    /* Command Bits */
+    FIELD(I2CD_CMD, RX_DMA_EN, 9, 1)
+    FIELD(I2CD_CMD, TX_DMA_EN, 8, 1)
+    FIELD(I2CD_CMD, RX_BUFF_EN, 7, 1)
+    FIELD(I2CD_CMD, TX_BUFF_EN, 6, 1)
+    FIELD(I2CD_CMD, M_STOP_CMD, 5, 1)
+    FIELD(I2CD_CMD, M_S_RX_CMD_LAST, 4, 1)
+    FIELD(I2CD_CMD, M_RX_CMD, 3, 1)
+    FIELD(I2CD_CMD, S_TX_CMD, 2, 1)
+    FIELD(I2CD_CMD, M_TX_CMD, 1, 1)
+    FIELD(I2CD_CMD, M_START_CMD, 0, 1)
+REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
+REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
+    FIELD(I2CD_POOL_CTRL, RX_COUNT, 24, 5)
+    FIELD(I2CD_POOL_CTRL, RX_SIZE, 16, 5)
+    FIELD(I2CD_POOL_CTRL, TX_COUNT, 9, 5)
+    FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
+REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
+    FIELD(I2CD_BYTE_BUF, RX_BUF, 8, 8)
+    FIELD(I2CD_BYTE_BUF, TX_BUF, 0, 8)
+REG32(I2CD_DMA_ADDR, 0x24) /* DMA Buffer Address */
+REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
 
 static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
 {
-    return bus->ctrl & I2CD_MASTER_EN;
+    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN);
 }
 
 static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
 {
-    return bus->ctrl & (I2CD_MASTER_EN | I2CD_SLAVE_EN);
+    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN) ||
+           FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, SLAVE_EN);
 }
 
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
@@ -160,11 +147,13 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
 
     trace_aspeed_i2c_bus_raise_interrupt(bus->intr_status,
-          bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
-          bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
-          bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
-          bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
-          bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
+        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
+        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
+        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
+        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
+                                                                 : "",
+        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
+                                                              : "");
 
     bus->intr_status &= bus->intr_ctrl;
     if (bus->intr_status) {
@@ -181,38 +170,38 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
     uint64_t value = -1;
 
     switch (offset) {
-    case I2CD_FUN_CTRL_REG:
+    case A_I2CD_FUN_CTRL:
         value = bus->ctrl;
         break;
-    case I2CD_AC_TIMING_REG1:
+    case A_I2CD_AC_TIMING1:
         value = bus->timing[0];
         break;
-    case I2CD_AC_TIMING_REG2:
+    case A_I2CD_AC_TIMING2:
         value = bus->timing[1];
         break;
-    case I2CD_INTR_CTRL_REG:
+    case A_I2CD_INTR_CTRL:
         value = bus->intr_ctrl;
         break;
-    case I2CD_INTR_STS_REG:
+    case A_I2CD_INTR_STS:
         value = bus->intr_status;
         break;
-    case I2CD_POOL_CTRL_REG:
+    case A_I2CD_POOL_CTRL:
         value = bus->pool_ctrl;
         break;
-    case I2CD_BYTE_BUF_REG:
+    case A_I2CD_BYTE_BUF:
         value = bus->buf;
         break;
-    case I2CD_CMD_REG:
+    case A_I2CD_CMD:
         value = bus->cmd | (i2c_bus_busy(bus->bus) << 16);
         break;
-    case I2CD_DMA_ADDR:
+    case A_I2CD_DMA_ADDR:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
         }
         value = bus->dma_addr;
         break;
-    case I2CD_DMA_LEN:
+    case A_I2CD_DMA_LEN:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
@@ -233,13 +222,12 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
 
 static void aspeed_i2c_set_state(AspeedI2CBus *bus, uint8_t state)
 {
-    bus->cmd &= ~(I2CD_TX_STATE_MASK << I2CD_TX_STATE_SHIFT);
-    bus->cmd |= (state & I2CD_TX_STATE_MASK) << I2CD_TX_STATE_SHIFT;
+    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_STATE, state);
 }
 
 static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
 {
-    return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
+    return FIELD_EX32(bus->cmd, I2CD_CMD, TX_STATE);
 }
 
 static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
@@ -265,21 +253,21 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     int ret = -1;
     int i;
+    int pool_tx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
 
-    if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
-        for (i = pool_start; i < I2CD_POOL_TX_COUNT(bus->pool_ctrl); i++) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
+        for (i = pool_start; i < pool_tx_count; i++) {
             uint8_t *pool_base = aic->bus_pool_base(bus);
 
-            trace_aspeed_i2c_bus_send("BUF", i + 1,
-                                      I2CD_POOL_TX_COUNT(bus->pool_ctrl),
+            trace_aspeed_i2c_bus_send("BUF", i + 1, pool_tx_count,
                                       pool_base[i]);
             ret = i2c_send(bus->bus, pool_base[i]);
             if (ret) {
                 break;
             }
         }
-        bus->cmd &= ~I2CD_TX_BUFF_ENABLE;
-    } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_BUFF_EN, 0);
+    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
         while (bus->dma_len) {
             uint8_t data;
             aspeed_i2c_dma_read(bus, &data);
@@ -289,7 +277,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
                 break;
             }
         }
-        bus->cmd &= ~I2CD_TX_DMA_ENABLE;
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_DMA_EN, 0);
     } else {
         trace_aspeed_i2c_bus_send("BYTE", pool_start, 1, bus->buf);
         ret = i2c_send(bus->bus, bus->buf);
@@ -304,22 +292,22 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
     uint8_t data;
     int i;
+    int pool_rx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT);
 
-    if (bus->cmd & I2CD_RX_BUFF_ENABLE) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
-        for (i = 0; i < I2CD_POOL_RX_SIZE(bus->pool_ctrl); i++) {
+        for (i = 0; i < pool_rx_count; i++) {
             pool_base[i] = i2c_recv(bus->bus);
-            trace_aspeed_i2c_bus_recv("BUF", i + 1,
-                                      I2CD_POOL_RX_SIZE(bus->pool_ctrl),
+            trace_aspeed_i2c_bus_recv("BUF", i + 1, pool_rx_count,
                                       pool_base[i]);
         }
 
         /* Update RX count */
-        bus->pool_ctrl &= ~(0xff << 24);
-        bus->pool_ctrl |= (i & 0xff) << 24;
-        bus->cmd &= ~I2CD_RX_BUFF_ENABLE;
-    } else if (bus->cmd & I2CD_RX_DMA_ENABLE) {
+        bus->pool_ctrl = FIELD_DP32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT,
+                                    i & 0xff);
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_BUFF_EN, 0);
+    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
         uint8_t data;
 
         while (bus->dma_len) {
@@ -337,11 +325,11 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
             bus->dma_addr++;
             bus->dma_len--;
         }
-        bus->cmd &= ~I2CD_RX_DMA_ENABLE;
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_DMA_EN, 0);
     } else {
         data = i2c_recv(bus->bus);
         trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->buf);
-        bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, RX_BUF, data);
     }
 }
 
@@ -349,11 +337,12 @@ static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
 {
     aspeed_i2c_set_state(bus, I2CD_MRXD);
     aspeed_i2c_bus_recv(bus);
-    bus->intr_status |= I2CD_INTR_RX_DONE;
-    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+    bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS, RX_DONE, 1);
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) {
         i2c_nack(bus->bus);
     }
-    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_RX_CMD, 0);
+    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST, 0);
     aspeed_i2c_set_state(bus, I2CD_MACTIVE);
 }
 
@@ -361,11 +350,11 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
 
-    if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
         return pool_base[0];
-    } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
         uint8_t data;
 
         aspeed_i2c_dma_read(bus, &data);
@@ -379,7 +368,10 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
 {
     AspeedI2CState *s = bus->controller;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
-
+    bool dma_en = FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)  ||
+                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)  ||
+                  FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ||
+                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN);
     if (!aic->check_sram) {
         return true;
     }
@@ -388,9 +380,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
      * AST2500: SRAM must be enabled before using the Buffer Pool or
      * DMA mode.
      */
-    if (!(s->ctrl_global & I2C_CTRL_SRAM_EN) &&
-        (bus->cmd & (I2CD_RX_DMA_ENABLE | I2CD_TX_DMA_ENABLE |
-                     I2CD_RX_BUFF_ENABLE | I2CD_TX_BUFF_ENABLE))) {
+    if (!FIELD_EX32(s->ctrl_global, I2C_CTRL_GLOBAL, SRAM_EN) && dma_en) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: SRAM is not enabled\n", __func__);
         return false;
     }
@@ -402,25 +392,24 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
 {
     g_autofree char *cmd_flags = NULL;
     uint32_t count;
-
-    if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
-        count = I2CD_POOL_TX_COUNT(bus->pool_ctrl);
-    } else if (bus->cmd & (I2CD_RX_DMA_ENABLE | I2CD_RX_DMA_ENABLE)) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
+        count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
+    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
         count = bus->dma_len;
     } else { /* BYTE mode */
         count = 1;
     }
 
     cmd_flags = g_strdup_printf("%s%s%s%s%s%s%s%s%s",
-                                bus->cmd & I2CD_M_START_CMD ? "start|" : "",
-                                bus->cmd & I2CD_RX_DMA_ENABLE ? "rxdma|" : "",
-                                bus->cmd & I2CD_TX_DMA_ENABLE ? "txdma|" : "",
-                                bus->cmd & I2CD_RX_BUFF_ENABLE ? "rxbuf|" : "",
-                                bus->cmd & I2CD_TX_BUFF_ENABLE ? "txbuf|" : "",
-                                bus->cmd & I2CD_M_TX_CMD ? "tx|" : "",
-                                bus->cmd & I2CD_M_RX_CMD ? "rx|" : "",
-                                bus->cmd & I2CD_M_S_RX_CMD_LAST ? "last|" : "",
-                                bus->cmd & I2CD_M_STOP_CMD ? "stop" : "");
+             FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD) ? "start|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
+             FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
 
     trace_aspeed_i2c_bus_cmd(bus->cmd, cmd_flags, count, bus->intr_status);
 }
@@ -444,7 +433,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_bus_cmd_dump(bus);
     }
 
-    if (bus->cmd & I2CD_M_START_CMD) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD)) {
         uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
             I2CD_MSTARTR : I2CD_MSTART;
         uint8_t addr;
@@ -455,21 +444,23 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 
         if (i2c_start_transfer(bus->bus, extract32(addr, 1, 7),
                                extract32(addr, 0, 1))) {
-            bus->intr_status |= I2CD_INTR_TX_NAK;
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          TX_NAK, 1);
         } else {
-            bus->intr_status |= I2CD_INTR_TX_ACK;
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          TX_ACK, 1);
         }
 
-        bus->cmd &= ~I2CD_M_START_CMD;
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_START_CMD, 0);
 
         /*
          * The START command is also a TX command, as the slave
          * address is sent on the bus. Drop the TX flag if nothing
          * else needs to be sent in this sequence.
          */
-        if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
-            if (I2CD_POOL_TX_COUNT(bus->pool_ctrl) == 1) {
-                bus->cmd &= ~I2CD_M_TX_CMD;
+        if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
+            if (FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT) == 1) {
+                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
             } else {
                 /*
                  * Increase the start index in the TX pool buffer to
@@ -477,12 +468,12 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
                  */
                 pool_start++;
             }
-        } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+        } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
             if (bus->dma_len == 0) {
-                bus->cmd &= ~I2CD_M_TX_CMD;
+                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
             }
         } else {
-            bus->cmd &= ~I2CD_M_TX_CMD;
+            bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
         }
 
         /* No slave found */
@@ -492,33 +483,38 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if (bus->cmd & I2CD_M_TX_CMD) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD)) {
         aspeed_i2c_set_state(bus, I2CD_MTXD);
         if (aspeed_i2c_bus_send(bus, pool_start)) {
-            bus->intr_status |= (I2CD_INTR_TX_NAK);
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          TX_NAK, 1);
             i2c_end_transfer(bus->bus);
         } else {
-            bus->intr_status |= I2CD_INTR_TX_ACK;
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          TX_ACK, 1);
         }
-        bus->cmd &= ~I2CD_M_TX_CMD;
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if ((bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) &&
-        !(bus->intr_status & I2CD_INTR_RX_DONE)) {
+    if ((FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
+         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) &&
+        !FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE)) {
         aspeed_i2c_handle_rx_cmd(bus);
     }
 
-    if (bus->cmd & I2CD_M_STOP_CMD) {
+    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD)) {
         if (!(aspeed_i2c_get_state(bus) & I2CD_MACTIVE)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: abnormal stop\n", __func__);
-            bus->intr_status |= I2CD_INTR_ABNORMAL;
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          ABNORMAL, 1);
         } else {
             aspeed_i2c_set_state(bus, I2CD_MSTOP);
             i2c_end_transfer(bus->bus);
-            bus->intr_status |= I2CD_INTR_NORMAL_STOP;
+            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
+                                          NORMAL_STOP, 1);
         }
-        bus->cmd &= ~I2CD_M_STOP_CMD;
+        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_STOP_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_IDLE);
     }
 }
@@ -533,49 +529,50 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
     trace_aspeed_i2c_bus_write(bus->id, offset, size, value);
 
     switch (offset) {
-    case I2CD_FUN_CTRL_REG:
-        if (value & I2CD_SLAVE_EN) {
+    case A_I2CD_FUN_CTRL:
+        if (FIELD_EX32(value, I2CD_FUN_CTRL, SLAVE_EN)) {
             qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
                           __func__);
             break;
         }
         bus->ctrl = value & 0x0071C3FF;
         break;
-    case I2CD_AC_TIMING_REG1:
+    case A_I2CD_AC_TIMING1:
         bus->timing[0] = value & 0xFFFFF0F;
         break;
-    case I2CD_AC_TIMING_REG2:
+    case A_I2CD_AC_TIMING2:
         bus->timing[1] = value & 0x7;
         break;
-    case I2CD_INTR_CTRL_REG:
+    case A_I2CD_INTR_CTRL:
         bus->intr_ctrl = value & 0x7FFF;
         break;
-    case I2CD_INTR_STS_REG:
-        handle_rx = (bus->intr_status & I2CD_INTR_RX_DONE) &&
-                (value & I2CD_INTR_RX_DONE);
+    case A_I2CD_INTR_STS:
+        handle_rx = FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) &&
+                    FIELD_EX32(value, I2CD_INTR_STS, RX_DONE);
         bus->intr_status &= ~(value & 0x7FFF);
         if (!bus->intr_status) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
         }
-        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+        if (handle_rx && (FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
+                         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST))) {
             aspeed_i2c_handle_rx_cmd(bus);
             aspeed_i2c_bus_raise_interrupt(bus);
         }
         break;
-    case I2CD_DEV_ADDR_REG:
+    case A_I2CD_DEV_ADDR:
         qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
                       __func__);
         break;
-    case I2CD_POOL_CTRL_REG:
+    case A_I2CD_POOL_CTRL:
         bus->pool_ctrl &= ~0xffffff;
         bus->pool_ctrl |= (value & 0xffffff);
         break;
 
-    case I2CD_BYTE_BUF_REG:
-        bus->buf = (value & I2CD_BYTE_BUF_TX_MASK) << I2CD_BYTE_BUF_TX_SHIFT;
+    case A_I2CD_BYTE_BUF:
+        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, TX_BUF, value);
         break;
-    case I2CD_CMD_REG:
+    case A_I2CD_CMD:
         if (!aspeed_i2c_bus_is_enabled(bus)) {
             break;
         }
@@ -587,7 +584,8 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         }
 
         if (!aic->has_dma &&
-            value & (I2CD_RX_DMA_ENABLE | I2CD_TX_DMA_ENABLE)) {
+            (FIELD_EX32(value, I2CD_CMD, RX_DMA_EN) ||
+             FIELD_EX32(value, I2CD_CMD, TX_DMA_EN))) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
         }
@@ -595,7 +593,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         aspeed_i2c_bus_handle_cmd(bus, value);
         aspeed_i2c_bus_raise_interrupt(bus);
         break;
-    case I2CD_DMA_ADDR:
+    case A_I2CD_DMA_ADDR:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
@@ -604,7 +602,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         bus->dma_addr = value & 0x3ffffffc;
         break;
 
-    case I2CD_DMA_LEN:
+    case A_I2CD_DMA_LEN:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
@@ -628,9 +626,9 @@ static uint64_t aspeed_i2c_ctrl_read(void *opaque, hwaddr offset,
     AspeedI2CState *s = opaque;
 
     switch (offset) {
-    case I2C_CTRL_STATUS:
+    case A_I2C_CTRL_STATUS:
         return s->intr_status;
-    case I2C_CTRL_GLOBAL:
+    case A_I2C_CTRL_GLOBAL:
         return s->ctrl_global;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
@@ -647,11 +645,11 @@ static void aspeed_i2c_ctrl_write(void *opaque, hwaddr offset,
     AspeedI2CState *s = opaque;
 
     switch (offset) {
-    case I2C_CTRL_GLOBAL:
+    case A_I2C_CTRL_GLOBAL:
         value &= ~s->ctrl_global_rsvd;
         s->ctrl_global = value;
         break;
-    case I2C_CTRL_STATUS:
+    case A_I2C_CTRL_STATUS:
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
                       __func__, offset);
@@ -923,9 +921,10 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
 static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
     uint8_t *pool_page =
-        &bus->controller->pool[I2CD_POOL_PAGE_SEL(bus->ctrl) * 0x100];
+        &bus->controller->pool[FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL,
+                                          POOL_PAGE_SEL) * 0x100];
 
-    return &pool_page[I2CD_POOL_OFFSET(bus->pool_ctrl)];
+    return &pool_page[FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, OFFSET)];
 }
 
 static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (2 preceding siblings ...)
  2022-03-31  4:32 ` [RFC PATCH 3/7] aspeed: i2c: Migrate to registerfields API Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-04-06 10:34   ` Cédric Le Goater
  2022-03-31  4:32 ` [RFC PATCH 5/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

Using a register array will allow us to represent old-mode and new-mode
I2C registers by using the same underlying register array, instead of
adding an entire new set of variables to represent new mode.

As part of this, we also do additional cleanup to use ARRAY_FIELD_
macros instead of FIELD_ macros on registers.

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: Ib94996b17c361b8490c042b43c99d8abc69332e3
---
 hw/i2c/aspeed_i2c.c         | 286 +++++++++++++++++-------------------
 include/hw/i2c/aspeed_i2c.h |  11 +-
 2 files changed, 133 insertions(+), 164 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index be81a798cc..e938055150 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -133,30 +133,30 @@ REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
 
 static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
 {
-    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN);
+    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN);
 }
 
 static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
 {
-    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN) ||
-           FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, SLAVE_EN);
+    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN) ||
+           ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, SLAVE_EN);
 }
 
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
 
-    trace_aspeed_i2c_bus_raise_interrupt(bus->intr_status,
-        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
-        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
-        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
-        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
-                                                                 : "",
-        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
-                                                              : "");
-
-    bus->intr_status &= bus->intr_ctrl;
-    if (bus->intr_status) {
+    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[R_I2CD_INTR_STS],
+          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
+                                                                  : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
+                                                               : "");
+
+    bus->regs[R_I2CD_INTR_STS] &= bus->regs[R_I2CD_INTR_CTRL];
+    if (bus->regs[R_I2CD_INTR_STS]) {
         bus->controller->intr_status |= 1 << bus->id;
         qemu_irq_raise(aic->bus_get_irq(bus));
     }
@@ -167,46 +167,33 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
 {
     AspeedI2CBus *bus = opaque;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
-    uint64_t value = -1;
+    uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
 
     switch (offset) {
     case A_I2CD_FUN_CTRL:
-        value = bus->ctrl;
-        break;
     case A_I2CD_AC_TIMING1:
-        value = bus->timing[0];
-        break;
     case A_I2CD_AC_TIMING2:
-        value = bus->timing[1];
-        break;
     case A_I2CD_INTR_CTRL:
-        value = bus->intr_ctrl;
-        break;
     case A_I2CD_INTR_STS:
-        value = bus->intr_status;
-        break;
     case A_I2CD_POOL_CTRL:
-        value = bus->pool_ctrl;
-        break;
     case A_I2CD_BYTE_BUF:
-        value = bus->buf;
+        /* Value is already set, don't do anything. */
         break;
     case A_I2CD_CMD:
-        value = bus->cmd | (i2c_bus_busy(bus->bus) << 16);
+        value = FIELD_DP32(value, I2CD_CMD, BUS_BUSY_STS,
+                           i2c_bus_busy(bus->bus));
         break;
     case A_I2CD_DMA_ADDR:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
-            break;
+            value = -1;
         }
-        value = bus->dma_addr;
         break;
     case A_I2CD_DMA_LEN:
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
-            break;
+            value = -1;
         }
-        value = bus->dma_len;
         break;
 
     default:
@@ -222,12 +209,12 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
 
 static void aspeed_i2c_set_state(AspeedI2CBus *bus, uint8_t state)
 {
-    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_STATE, state);
+    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_STATE, state);
 }
 
 static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
 {
-    return FIELD_EX32(bus->cmd, I2CD_CMD, TX_STATE);
+    return ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_STATE);
 }
 
 static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
@@ -235,16 +222,16 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
     MemTxResult result;
     AspeedI2CState *s = bus->controller;
 
-    result = address_space_read(&s->dram_as, bus->dma_addr,
+    result = address_space_read(&s->dram_as, bus->regs[R_I2CD_DMA_ADDR],
                                 MEMTXATTRS_UNSPECIFIED, data, 1);
     if (result != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
-                      __func__, bus->dma_addr);
+                      __func__, bus->regs[R_I2CD_DMA_ADDR]);
         return -1;
     }
 
-    bus->dma_addr++;
-    bus->dma_len--;
+    bus->regs[R_I2CD_DMA_ADDR]++;
+    bus->regs[R_I2CD_DMA_LEN]--;
     return 0;
 }
 
@@ -253,9 +240,9 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     int ret = -1;
     int i;
-    int pool_tx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
+    int pool_tx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
         for (i = pool_start; i < pool_tx_count; i++) {
             uint8_t *pool_base = aic->bus_pool_base(bus);
 
@@ -266,21 +253,23 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
                 break;
             }
         }
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_BUFF_EN, 0);
-    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
-        while (bus->dma_len) {
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_BUFF_EN, 0);
+    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
+        while (bus->regs[R_I2CD_DMA_LEN]) {
             uint8_t data;
             aspeed_i2c_dma_read(bus, &data);
-            trace_aspeed_i2c_bus_send("DMA", bus->dma_len, bus->dma_len, data);
+            trace_aspeed_i2c_bus_send("DMA", bus->regs[R_I2CD_DMA_LEN],
+                                      bus->regs[R_I2CD_DMA_LEN], data);
             ret = i2c_send(bus->bus, data);
             if (ret) {
                 break;
             }
         }
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_DMA_EN, 0);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_DMA_EN, 0);
     } else {
-        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1, bus->buf);
-        ret = i2c_send(bus->bus, bus->buf);
+        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
+                                  bus->regs[R_I2CD_BYTE_BUF]);
+        ret = i2c_send(bus->bus, bus->regs[R_I2CD_BYTE_BUF]);
     }
 
     return ret;
@@ -292,9 +281,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
     uint8_t data;
     int i;
-    int pool_rx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT);
+    int pool_rx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, RX_COUNT);
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
         for (i = 0; i < pool_rx_count; i++) {
@@ -304,32 +293,33 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
         }
 
         /* Update RX count */
-        bus->pool_ctrl = FIELD_DP32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT,
-                                    i & 0xff);
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_BUFF_EN, 0);
-    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
+        ARRAY_FIELD_DP32(bus->regs, I2CD_POOL_CTRL, RX_COUNT, i & 0xff);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_BUFF_EN, 0);
+    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
         uint8_t data;
 
-        while (bus->dma_len) {
+        while (bus->regs[R_I2CD_DMA_LEN]) {
             MemTxResult result;
 
             data = i2c_recv(bus->bus);
-            trace_aspeed_i2c_bus_recv("DMA", bus->dma_len, bus->dma_len, data);
-            result = address_space_write(&s->dram_as, bus->dma_addr,
+            trace_aspeed_i2c_bus_recv("DMA", bus->regs[R_I2CD_DMA_LEN],
+                                      bus->regs[R_I2CD_DMA_LEN], data);
+            result = address_space_write(&s->dram_as,
+                                         bus->regs[R_I2CD_DMA_ADDR],
                                          MEMTXATTRS_UNSPECIFIED, &data, 1);
             if (result != MEMTX_OK) {
                 qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
-                              __func__, bus->dma_addr);
+                              __func__, bus->regs[R_I2CD_DMA_ADDR]);
                 return;
             }
-            bus->dma_addr++;
-            bus->dma_len--;
+            bus->regs[R_I2CD_DMA_ADDR]++;
+            bus->regs[R_I2CD_DMA_LEN]--;
         }
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_DMA_EN, 0);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_DMA_EN, 0);
     } else {
         data = i2c_recv(bus->bus);
-        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->buf);
-        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, RX_BUF, data);
+        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->regs[R_I2CD_BYTE_BUF]);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, RX_BUF, data);
     }
 }
 
@@ -337,12 +327,12 @@ static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
 {
     aspeed_i2c_set_state(bus, I2CD_MRXD);
     aspeed_i2c_bus_recv(bus);
-    bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS, RX_DONE, 1);
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) {
+    ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, RX_DONE, 1);
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) {
         i2c_nack(bus->bus);
     }
-    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_RX_CMD, 0);
-    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST, 0);
+    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_RX_CMD, 0);
+    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST, 0);
     aspeed_i2c_set_state(bus, I2CD_MACTIVE);
 }
 
@@ -350,17 +340,17 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
         return pool_base[0];
-    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
+    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
         uint8_t data;
 
         aspeed_i2c_dma_read(bus, &data);
         return data;
     } else {
-        return bus->buf;
+        return bus->regs[R_I2CD_BYTE_BUF];
     }
 }
 
@@ -368,10 +358,10 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
 {
     AspeedI2CState *s = bus->controller;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
-    bool dma_en = FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)  ||
-                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)  ||
-                  FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ||
-                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN);
+    bool dma_en = ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)  ||
+                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)  ||
+                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ||
+                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN);
     if (!aic->check_sram) {
         return true;
     }
@@ -392,26 +382,27 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
 {
     g_autofree char *cmd_flags = NULL;
     uint32_t count;
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
-        count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
-    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
-        count = bus->dma_len;
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
+        count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
+    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
+        count = bus->regs[R_I2CD_DMA_LEN];
     } else { /* BYTE mode */
         count = 1;
     }
 
     cmd_flags = g_strdup_printf("%s%s%s%s%s%s%s%s%s",
-             FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD) ? "start|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
-             FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
-
-    trace_aspeed_i2c_bus_cmd(bus->cmd, cmd_flags, count, bus->intr_status);
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD) ? "start|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
+          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
+
+    trace_aspeed_i2c_bus_cmd(bus->regs[R_I2CD_CMD], cmd_flags, count,
+                             bus->regs[R_I2CD_INTR_STS]);
 }
 
 /*
@@ -422,8 +413,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 {
     uint8_t pool_start = 0;
 
-    bus->cmd &= ~0xFFFF;
-    bus->cmd |= value & 0xFFFF;
+    bus->regs[R_I2CD_CMD] &= ~0xFFFF;
+    bus->regs[R_I2CD_CMD] |= value & 0xFFFF;
 
     if (!aspeed_i2c_check_sram(bus)) {
         return;
@@ -433,7 +424,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_bus_cmd_dump(bus);
     }
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD)) {
         uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
             I2CD_MSTARTR : I2CD_MSTART;
         uint8_t addr;
@@ -444,23 +435,21 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 
         if (i2c_start_transfer(bus->bus, extract32(addr, 1, 7),
                                extract32(addr, 0, 1))) {
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          TX_NAK, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
         } else {
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          TX_ACK, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
         }
 
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_START_CMD, 0);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_START_CMD, 0);
 
         /*
          * The START command is also a TX command, as the slave
          * address is sent on the bus. Drop the TX flag if nothing
          * else needs to be sent in this sequence.
          */
-        if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
-            if (FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT) == 1) {
-                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
+        if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
+            if (ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT) == 1) {
+                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
             } else {
                 /*
                  * Increase the start index in the TX pool buffer to
@@ -468,12 +457,12 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
                  */
                 pool_start++;
             }
-        } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
-            if (bus->dma_len == 0) {
-                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
+        } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
+            if (bus->regs[R_I2CD_DMA_LEN] == 0) {
+                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
             }
         } else {
-            bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
         }
 
         /* No slave found */
@@ -483,38 +472,34 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD)) {
         aspeed_i2c_set_state(bus, I2CD_MTXD);
         if (aspeed_i2c_bus_send(bus, pool_start)) {
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          TX_NAK, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
             i2c_end_transfer(bus->bus);
         } else {
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          TX_ACK, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
         }
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if ((FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
-         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) &&
-        !FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE)) {
+    if ((ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
+         ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) &&
+        !ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE)) {
         aspeed_i2c_handle_rx_cmd(bus);
     }
 
-    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD)) {
+    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD)) {
         if (!(aspeed_i2c_get_state(bus) & I2CD_MACTIVE)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: abnormal stop\n", __func__);
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          ABNORMAL, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, ABNORMAL, 1);
         } else {
             aspeed_i2c_set_state(bus, I2CD_MSTOP);
             i2c_end_transfer(bus->bus);
-            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
-                                          NORMAL_STOP, 1);
+            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, NORMAL_STOP, 1);
         }
-        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_STOP_CMD, 0);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_STOP_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_IDLE);
     }
 }
@@ -535,27 +520,27 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
                           __func__);
             break;
         }
-        bus->ctrl = value & 0x0071C3FF;
+        bus->regs[R_I2CD_FUN_CTRL] = value & 0x0071C3FF;
         break;
     case A_I2CD_AC_TIMING1:
-        bus->timing[0] = value & 0xFFFFF0F;
+        bus->regs[R_I2CD_AC_TIMING1] = value & 0xFFFFF0F;
         break;
     case A_I2CD_AC_TIMING2:
-        bus->timing[1] = value & 0x7;
+        bus->regs[R_I2CD_AC_TIMING2] = value & 0x7;
         break;
     case A_I2CD_INTR_CTRL:
-        bus->intr_ctrl = value & 0x7FFF;
+        bus->regs[R_I2CD_INTR_CTRL] = value & 0x7FFF;
         break;
     case A_I2CD_INTR_STS:
-        handle_rx = FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) &&
+        handle_rx = ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) &&
                     FIELD_EX32(value, I2CD_INTR_STS, RX_DONE);
-        bus->intr_status &= ~(value & 0x7FFF);
-        if (!bus->intr_status) {
+        bus->regs[R_I2CD_INTR_STS] &= ~(value & 0x7FFF);
+        if (!bus->regs[R_I2CD_INTR_STS]) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
         }
-        if (handle_rx && (FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
-                         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST))) {
+        if (handle_rx && (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
+                      ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST))) {
             aspeed_i2c_handle_rx_cmd(bus);
             aspeed_i2c_bus_raise_interrupt(bus);
         }
@@ -565,12 +550,12 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
                       __func__);
         break;
     case A_I2CD_POOL_CTRL:
-        bus->pool_ctrl &= ~0xffffff;
-        bus->pool_ctrl |= (value & 0xffffff);
+        bus->regs[R_I2CD_POOL_CTRL] &= ~0xffffff;
+        bus->regs[R_I2CD_POOL_CTRL] |= (value & 0xffffff);
         break;
 
     case A_I2CD_BYTE_BUF:
-        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, TX_BUF, value);
+        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, TX_BUF, value);
         break;
     case A_I2CD_CMD:
         if (!aspeed_i2c_bus_is_enabled(bus)) {
@@ -599,7 +584,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
             break;
         }
 
-        bus->dma_addr = value & 0x3ffffffc;
+        bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
         break;
 
     case A_I2CD_DMA_LEN:
@@ -608,8 +593,8 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
             break;
         }
 
-        bus->dma_len = value & 0xfff;
-        if (!bus->dma_len) {
+        bus->regs[R_I2CD_DMA_LEN] = value & 0xfff;
+        if (!bus->regs[R_I2CD_DMA_LEN]) {
             qemu_log_mask(LOG_UNIMP, "%s: invalid DMA length\n",  __func__);
         }
         break;
@@ -706,19 +691,10 @@ static const MemoryRegionOps aspeed_i2c_pool_ops = {
 
 static const VMStateDescription aspeed_i2c_bus_vmstate = {
     .name = TYPE_ASPEED_I2C,
-    .version_id = 3,
-    .minimum_version_id = 3,
+    .version_id = 4,
+    .minimum_version_id = 4,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT8(id, AspeedI2CBus),
-        VMSTATE_UINT32(ctrl, AspeedI2CBus),
-        VMSTATE_UINT32_ARRAY(timing, AspeedI2CBus, 2),
-        VMSTATE_UINT32(intr_ctrl, AspeedI2CBus),
-        VMSTATE_UINT32(intr_status, AspeedI2CBus),
-        VMSTATE_UINT32(cmd, AspeedI2CBus),
-        VMSTATE_UINT32(buf, AspeedI2CBus),
-        VMSTATE_UINT32(pool_ctrl, AspeedI2CBus),
-        VMSTATE_UINT32(dma_addr, AspeedI2CBus),
-        VMSTATE_UINT32(dma_len, AspeedI2CBus),
+        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_OLD_NUM_REG),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -858,12 +834,12 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
 {
     AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
 
-    s->intr_ctrl = 0;
-    s->intr_status = 0;
-    s->cmd = 0;
-    s->buf = 0;
-    s->dma_addr = 0;
-    s->dma_len = 0;
+    s->regs[R_I2CD_INTR_CTRL] = 0;
+    s->regs[R_I2CD_INTR_STS] = 0;
+    s->regs[R_I2CD_CMD] = 0;
+    s->regs[R_I2CD_BYTE_BUF] = 0;
+    s->regs[R_I2CD_DMA_ADDR] = 0;
+    s->regs[R_I2CD_DMA_LEN] = 0;
     i2c_end_transfer(s->bus);
 }
 
@@ -921,10 +897,10 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
 static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
     uint8_t *pool_page =
-        &bus->controller->pool[FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL,
-                                          POOL_PAGE_SEL) * 0x100];
+        &bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
+                                                POOL_PAGE_SEL) * 0x100];
 
-    return &pool_page[FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, OFFSET)];
+    return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
 }
 
 static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 3912fcc3ff..e9f83a9540 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -33,6 +33,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 
 #define ASPEED_I2C_NR_BUSSES 16
 #define ASPEED_I2C_MAX_POOL_SIZE 0x800
+#define ASPEED_I2C_OLD_NUM_REG 11
 
 struct AspeedI2CState;
 
@@ -49,15 +50,7 @@ struct AspeedI2CBus {
     uint8_t id;
     qemu_irq irq;
 
-    uint32_t ctrl;
-    uint32_t timing[2];
-    uint32_t intr_ctrl;
-    uint32_t intr_status;
-    uint32_t cmd;
-    uint32_t buf;
-    uint32_t pool_ctrl;
-    uint32_t dma_addr;
-    uint32_t dma_len;
+    uint32_t regs[ASPEED_I2C_OLD_NUM_REG];
 };
 
 struct AspeedI2CState {
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 5/7] aspeed: i2c: Add new mode support
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (3 preceding siblings ...)
  2022-03-31  4:32 ` [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-03-31  4:32 ` [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace Joe Komlodi
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

On AST2600, I2C has a secondary mode, called "new mode", which changes
the layout of registers, adds some minor behavior changes, and
introduces a new way to transfer data called "packet mode".

Most of the bit positions of the fields are the same between old and new
mode, so we use SHARED_FIELD_XX macros to reuse most of the code between
the different modes.

For packet mode, most of the command behavior is the same compared to
other modes, but there are some minor changes to how interrupts are
handled compared to other modes.

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: I072f8301964f623afc74af1fe50c12e5caef199e
---
 hw/i2c/aspeed_i2c.c         | 844 +++++++++++++++++++++++++++---------
 include/hw/i2c/aspeed_i2c.h |   4 +-
 2 files changed, 653 insertions(+), 195 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index e938055150..948d8dc2bb 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -31,141 +31,302 @@
 #include "hw/registerfields.h"
 #include "trace.h"
 
+/* Tx State Machine */
+#define   I2CD_TX_STATE_MASK                  0xf
+#define     I2CD_IDLE                         0x0
+#define     I2CD_MACTIVE                      0x8
+#define     I2CD_MSTART                       0x9
+#define     I2CD_MSTARTR                      0xa
+#define     I2CD_MSTOP                        0xb
+#define     I2CD_MTXD                         0xc
+#define     I2CD_MRXACK                       0xd
+#define     I2CD_MRXD                         0xe
+#define     I2CD_MTXACK                       0xf
+#define     I2CD_SWAIT                        0x1
+#define     I2CD_SRXD                         0x4
+#define     I2CD_STXACK                       0x5
+#define     I2CD_STXD                         0x6
+#define     I2CD_SRXACK                       0x7
+#define     I2CD_RECOVER                      0x3
+
 /* I2C Global Register */
 REG32(I2C_CTRL_STATUS, 0x0) /* Device Interrupt Status */
 REG32(I2C_CTRL_ASSIGN, 0x8) /* Device Interrupt Target Assignment */
 REG32(I2C_CTRL_GLOBAL, 0xC) /* Global Control Register */
+    FIELD(I2C_CTRL_GLOBAL, REG_MODE, 2, 1)
     FIELD(I2C_CTRL_GLOBAL, SRAM_EN, 0, 1)
+REG32(I2C_CTRL_NEW_CLK_DIVIDER, 0x10) /* New mode clock divider */
 
-/* I2C Device (Bus) Register */
+/* I2C Old Mode Device (Bus) Register */
 REG32(I2CD_FUN_CTRL, 0x0) /* I2CD Function Control  */
     FIELD(I2CD_FUN_CTRL, POOL_PAGE_SEL, 20, 3) /* AST2400 */
-    FIELD(I2CD_FUN_CTRL, M_SDA_LOCK_EN, 16, 1)
-    FIELD(I2CD_FUN_CTRL, MULTI_MASTER_DIS, 15, 1)
-    FIELD(I2CD_FUN_CTRL, M_SCL_DRIVE_EN, 14, 1)
-    FIELD(I2CD_FUN_CTRL, MSB_STS, 9, 1)
-    FIELD(I2CD_FUN_CTRL, SDA_DRIVE_IT_EN, 8, 1)
-    FIELD(I2CD_FUN_CTRL, M_SDA_DRIVE_IT_EN, 7, 1)
-    FIELD(I2CD_FUN_CTRL, M_HIGH_SPEED_EN, 6, 1)
-    FIELD(I2CD_FUN_CTRL, DEF_ADDR_EN, 5, 1)
-    FIELD(I2CD_FUN_CTRL, DEF_ALERT_EN, 4, 1)
-    FIELD(I2CD_FUN_CTRL, DEF_ARP_EN, 3, 1)
-    FIELD(I2CD_FUN_CTRL, DEF_GCALL_EN, 2, 1)
-    FIELD(I2CD_FUN_CTRL, SLAVE_EN, 1, 1)
-    FIELD(I2CD_FUN_CTRL, MASTER_EN, 0, 1)
+    SHARED_FIELD(M_SDA_LOCK_EN, 16, 1)
+    SHARED_FIELD(MULTI_MASTER_DIS, 15, 1)
+    SHARED_FIELD(M_SCL_DRIVE_EN, 14, 1)
+    SHARED_FIELD(MSB_STS, 9, 1)
+    SHARED_FIELD(SDA_DRIVE_IT_EN, 8, 1)
+    SHARED_FIELD(M_SDA_DRIVE_IT_EN, 7, 1)
+    SHARED_FIELD(M_HIGH_SPEED_EN, 6, 1)
+    SHARED_FIELD(DEF_ADDR_EN, 5, 1)
+    SHARED_FIELD(DEF_ALERT_EN, 4, 1)
+    SHARED_FIELD(DEF_ARP_EN, 3, 1)
+    SHARED_FIELD(DEF_GCALL_EN, 2, 1)
+    SHARED_FIELD(SLAVE_EN, 1, 1)
+    SHARED_FIELD(MASTER_EN, 0, 1)
 REG32(I2CD_AC_TIMING1, 0x04) /* Clock and AC Timing Control #1 */
 REG32(I2CD_AC_TIMING2, 0x08) /* Clock and AC Timing Control #2 */
 REG32(I2CD_INTR_CTRL, 0x0C)  /* I2CD Interrupt Control */
 REG32(I2CD_INTR_STS, 0x10)   /* I2CD Interrupt Status */
-    FIELD(I2CD_INTR_STS, SLAVE_ADDR_MATCH, 31, 1)    /* 0: addr1 1: addr2 */
-    FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_PENDING, 29, 1)
-    FIELD(I2CD_INTR_STS, SLAVE_INACTIVE_TIMEOUT, 15, 1)
-    FIELD(I2CD_INTR_STS, SDA_DL_TIMEOUT, 14, 1)
-    FIELD(I2CD_INTR_STS, BUS_RECOVER_DONE, 13, 1)
-    FIELD(I2CD_INTR_STS, SMBUS_ALERT, 12, 1)            /* Bus [0-3] only */
+    SHARED_FIELD(SLAVE_ADDR_MATCH, 31, 1)    /* 0: addr1 1: addr2 */
+    SHARED_FIELD(SLAVE_ADDR_RX_PENDING, 29, 1)
+    SHARED_FIELD(SLAVE_INACTIVE_TIMEOUT, 15, 1)
+    SHARED_FIELD(SDA_DL_TIMEOUT, 14, 1)
+    SHARED_FIELD(BUS_RECOVER_DONE, 13, 1)
+    SHARED_FIELD(SMBUS_ALERT, 12, 1)                    /* Bus [0-3] only */
     FIELD(I2CD_INTR_STS, SMBUS_ARP_ADDR, 11, 1)         /* Removed */
     FIELD(I2CD_INTR_STS, SMBUS_DEV_ALERT_ADDR, 10, 1)   /* Removed */
     FIELD(I2CD_INTR_STS, SMBUS_DEF_ADDR, 9, 1)          /* Removed */
     FIELD(I2CD_INTR_STS, GCALL_ADDR, 8, 1)              /* Removed */
     FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)     /* use RX_DONE */
-    FIELD(I2CD_INTR_STS, SCL_TIMEOUT, 6, 1)
-    FIELD(I2CD_INTR_STS, ABNORMAL, 5, 1)
-    FIELD(I2CD_INTR_STS, NORMAL_STOP, 4, 1)
-    FIELD(I2CD_INTR_STS, ARBIT_LOSS, 3, 1)
-    FIELD(I2CD_INTR_STS, RX_DONE, 2, 1)
-    FIELD(I2CD_INTR_STS, TX_NAK, 1, 1)
-    FIELD(I2CD_INTR_STS, TX_ACK, 0, 1)
+    SHARED_FIELD(SCL_TIMEOUT, 6, 1)
+    SHARED_FIELD(ABNORMAL, 5, 1)
+    SHARED_FIELD(NORMAL_STOP, 4, 1)
+    SHARED_FIELD(ARBIT_LOSS, 3, 1)
+    SHARED_FIELD(RX_DONE, 2, 1)
+    SHARED_FIELD(TX_NAK, 1, 1)
+    SHARED_FIELD(TX_ACK, 0, 1)
 REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
-    FIELD(I2CD_CMD, SDA_OE, 28, 1)
-    FIELD(I2CD_CMD, SDA_O, 27, 1)
-    FIELD(I2CD_CMD, SCL_OE, 26, 1)
-    FIELD(I2CD_CMD, SCL_O, 25, 1)
-    FIELD(I2CD_CMD, TX_TIMING, 23, 2)
-    FIELD(I2CD_CMD, TX_STATE, 19, 4)
-/* Tx State Machine */
-#define   I2CD_TX_STATE_MASK                  0xf
-#define     I2CD_IDLE                         0x0
-#define     I2CD_MACTIVE                      0x8
-#define     I2CD_MSTART                       0x9
-#define     I2CD_MSTARTR                      0xa
-#define     I2CD_MSTOP                        0xb
-#define     I2CD_MTXD                         0xc
-#define     I2CD_MRXACK                       0xd
-#define     I2CD_MRXD                         0xe
-#define     I2CD_MTXACK                       0xf
-#define     I2CD_SWAIT                        0x1
-#define     I2CD_SRXD                         0x4
-#define     I2CD_STXACK                       0x5
-#define     I2CD_STXD                         0x6
-#define     I2CD_SRXACK                       0x7
-#define     I2CD_RECOVER                      0x3
-    FIELD(I2CD_CMD, SCL_LINE_STS, 18, 1)
-    FIELD(I2CD_CMD, SDA_LINE_STS, 17, 1)
-    FIELD(I2CD_CMD, BUS_BUSY_STS, 16, 1)
-    FIELD(I2CD_CMD, SDA_OE_OUT_DIR, 15, 1)
-    FIELD(I2CD_CMD, SDA_O_OUT_DIR, 14, 1)
-    FIELD(I2CD_CMD, SCL_OE_OUT_DIR, 13, 1)
-    FIELD(I2CD_CMD, SCL_O_OUT_DIR, 12, 1)
-    FIELD(I2CD_CMD, BUS_RECOVER_CMD_EN, 11, 1)
-    FIELD(I2CD_CMD, S_ALT_EN, 10, 1)
+    SHARED_FIELD(SDA_OE, 28, 1)
+    SHARED_FIELD(SDA_O, 27, 1)
+    SHARED_FIELD(SCL_OE, 26, 1)
+    SHARED_FIELD(SCL_O, 25, 1)
+    SHARED_FIELD(TX_TIMING, 23, 2)
+    SHARED_FIELD(TX_STATE, 19, 4)
+    SHARED_FIELD(SCL_LINE_STS, 18, 1)
+    SHARED_FIELD(SDA_LINE_STS, 17, 1)
+    SHARED_FIELD(BUS_BUSY_STS, 16, 1)
+    SHARED_FIELD(SDA_OE_OUT_DIR, 15, 1)
+    SHARED_FIELD(SDA_O_OUT_DIR, 14, 1)
+    SHARED_FIELD(SCL_OE_OUT_DIR, 13, 1)
+    SHARED_FIELD(SCL_O_OUT_DIR, 12, 1)
+    SHARED_FIELD(BUS_RECOVER_CMD_EN, 11, 1)
+    SHARED_FIELD(S_ALT_EN, 10, 1)
     /* Command Bits */
-    FIELD(I2CD_CMD, RX_DMA_EN, 9, 1)
-    FIELD(I2CD_CMD, TX_DMA_EN, 8, 1)
-    FIELD(I2CD_CMD, RX_BUFF_EN, 7, 1)
-    FIELD(I2CD_CMD, TX_BUFF_EN, 6, 1)
-    FIELD(I2CD_CMD, M_STOP_CMD, 5, 1)
-    FIELD(I2CD_CMD, M_S_RX_CMD_LAST, 4, 1)
-    FIELD(I2CD_CMD, M_RX_CMD, 3, 1)
-    FIELD(I2CD_CMD, S_TX_CMD, 2, 1)
-    FIELD(I2CD_CMD, M_TX_CMD, 1, 1)
-    FIELD(I2CD_CMD, M_START_CMD, 0, 1)
+    SHARED_FIELD(RX_DMA_EN, 9, 1)
+    SHARED_FIELD(TX_DMA_EN, 8, 1)
+    SHARED_FIELD(RX_BUFF_EN, 7, 1)
+    SHARED_FIELD(TX_BUFF_EN, 6, 1)
+    SHARED_FIELD(M_STOP_CMD, 5, 1)
+    SHARED_FIELD(M_S_RX_CMD_LAST, 4, 1)
+    SHARED_FIELD(M_RX_CMD, 3, 1)
+    SHARED_FIELD(S_TX_CMD, 2, 1)
+    SHARED_FIELD(M_TX_CMD, 1, 1)
+    SHARED_FIELD(M_START_CMD, 0, 1)
 REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
 REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
-    FIELD(I2CD_POOL_CTRL, RX_COUNT, 24, 5)
-    FIELD(I2CD_POOL_CTRL, RX_SIZE, 16, 5)
-    FIELD(I2CD_POOL_CTRL, TX_COUNT, 9, 5)
+    SHARED_FIELD(RX_COUNT, 24, 5)
+    SHARED_FIELD(RX_SIZE, 16, 5)
+    SHARED_FIELD(TX_COUNT, 9, 5)
     FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
 REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
-    FIELD(I2CD_BYTE_BUF, RX_BUF, 8, 8)
-    FIELD(I2CD_BYTE_BUF, TX_BUF, 0, 8)
+    SHARED_FIELD(RX_BUF, 8, 8)
+    SHARED_FIELD(TX_BUF, 0, 8)
 REG32(I2CD_DMA_ADDR, 0x24) /* DMA Buffer Address */
 REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
 
+/* I2C New Mode Device (Bus) Register */
+REG32(I2CC_FUN_CTRL, 0x0)
+    FIELD(I2CC_FUN_CTRL, RB_EARLY_DONE_EN, 22, 1)
+    FIELD(I2CC_FUN_CTRL, DMA_DIS_AUTO_RECOVER, 21, 1)
+    FIELD(I2CC_FUN_CTRL, S_SAVE_ADDR, 20, 1)
+    FIELD(I2CC_FUN_CTRL, M_PKT_RETRY_CNT, 18, 2)
+    /* 17:0 shared with I2CD_FUN_CTRL[17:0] */
+REG32(I2CC_AC_TIMING, 0x04)
+REG32(I2CC_MS_TXRX_BYTE_BUF, 0x08)
+    /* 31:16 shared with I2CD_CMD[31:16] */
+    /* 15:0  shared with I2CD_BYTE_BUF[15:0] */
+REG32(I2CC_POOL_CTRL, 0x0c)
+    /* 31:0 shared with I2CD_POOL_CTRL[31:0] */
+REG32(I2CM_INTR_CTRL, 0x10)
+REG32(I2CM_INTR_STS, 0x14)
+    FIELD(I2CM_INTR_STS, PKT_STATE, 28, 4)
+    FIELD(I2CM_INTR_STS, PKT_CMD_TIMEOUT, 18, 1)
+    FIELD(I2CM_INTR_STS, PKT_CMD_FAIL, 17, 1)
+    FIELD(I2CM_INTR_STS, PKT_CMD_DONE, 16, 1)
+    FIELD(I2CM_INTR_STS, BUS_RECOVER_FAIL, 15, 1)
+    /* 14:0 shared with I2CD_INTR_STS[14:0] */
+REG32(I2CM_CMD, 0x18)
+    FIELD(I2CM_CMD, W1_CTRL, 31, 1)
+    FIELD(I2CM_CMD, PKT_DEV_ADDR, 24, 7)
+    FIELD(I2CM_CMD, HS_MASTER_MODE_LSB, 17, 3)
+    FIELD(I2CM_CMD, PKT_OP_EN, 16, 1)
+    /* 15:0 shared with I2CD_CMD[15:0] */
+REG32(I2CM_DMA_LEN, 0x1c)
+    FIELD(I2CM_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
+    FIELD(I2CM_DMA_LEN, RX_BUF_LEN, 16, 11)
+    FIELD(I2CM_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
+    FIELD(I2CM_DMA_LEN, TX_BUF_LEN, 0, 11)
+REG32(I2CS_INTR_CTRL, 0x20)
+REG32(I2CS_INTR_STS, 0x24)
+    /* 31:29 shared with I2CD_INTR_STS[31:29] */
+    FIELD(I2CS_INTR_STS, SLAVE_PARKING_STS, 24, 2)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR3_NAK, 22, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR2_NAK, 21, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR1_NAK, 20, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR_INDICATOR, 18, 2)
+    FIELD(I2CS_INTR_STS, PKT_CMD_FAIL, 17, 1)
+    FIELD(I2CS_INTR_STS, PKT_CMD_DONE, 16, 1)
+    /* 14:0 shared with I2CD_INTR_STS[14:0] */
+REG32(I2CS_CMD, 0x28)
+    FIELD(I2CS_CMD, W1_CTRL, 31, 1)
+    FIELD(I2CS_CMD, PKT_MODE_ACTIVE_ADDR, 17, 2)
+    FIELD(I2CS_CMD, PKT_MODE_EN, 16, 1)
+    FIELD(I2CS_CMD, AUTO_NAK_INACTIVE_ADDR, 15, 1)
+    FIELD(I2CS_CMD, AUTO_NAK_ACTIVE_ADDR, 14, 1)
+    /* 13:0 shared with I2CD_CMD[13:0] */
+REG32(I2CS_DMA_LEN, 0x2c)
+    FIELD(I2CS_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
+    FIELD(I2CS_DMA_LEN, RX_BUF_LEN, 16, 11)
+    FIELD(I2CS_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
+    FIELD(I2CS_DMA_LEN, TX_BUF_LEN, 0, 11)
+REG32(I2CM_DMA_TX_ADDR, 0x30)
+    FIELD(I2CM_DMA_TX_ADDR, ADDR, 0, 31)
+REG32(I2CM_DMA_RX_ADDR, 0x34)
+    FIELD(I2CM_DMA_RX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DMA_TX_ADDR, 0x38)
+    FIELD(I2CS_DMA_TX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DMA_RX_ADDR, 0x3c)
+    FIELD(I2CS_DMA_RX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DEV_ADDR, 0x40)
+REG32(I2CM_DMA_LEN_STS, 0x48)
+    FIELD(I2CM_DMA_LEN_STS, RX_LEN, 16, 13)
+    FIELD(I2CM_DMA_LEN_STS, TX_LEN, 0, 13)
+REG32(I2CS_DMA_LEN_STS, 0x4c)
+    FIELD(I2CS_DMA_LEN_STS, RX_LEN, 16, 13)
+    FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
+REG32(I2CC_DMA_ADDR, 0x50)
+REG32(I2CC_DMA_LEN, 0x54)
+
+static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
+{
+    return FIELD_EX32(s->ctrl_global, I2C_CTRL_GLOBAL, REG_MODE);
+}
+
+static inline bool aspeed_i2c_bus_pkt_mode_en(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return ARRAY_FIELD_EX32(bus->regs, I2CM_CMD, PKT_OP_EN);
+    }
+    return false;
+}
+
+static inline uint32_t aspeed_i2c_bus_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_FUN_CTRL;
+    }
+    return R_I2CD_FUN_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_CMD;
+    }
+    return R_I2CD_CMD;
+}
+
+static inline uint32_t aspeed_i2c_bus_intr_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_INTR_CTRL;
+    }
+    return R_I2CD_INTR_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_intr_sts_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_INTR_STS;
+    }
+    return R_I2CD_INTR_STS;
+}
+
+static inline uint32_t aspeed_i2c_bus_pool_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_POOL_CTRL;
+    }
+    return R_I2CD_POOL_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_byte_buf_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_MS_TXRX_BYTE_BUF;
+    }
+    return R_I2CD_BYTE_BUF;
+}
+
+static inline uint32_t aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_DMA_LEN;
+    }
+    return R_I2CD_DMA_LEN;
+}
+
+static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_DMA_ADDR;
+    }
+    return R_I2CD_DMA_ADDR;
+}
+
 static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
 {
-    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN);
+    return SHARED_ARRAY_FIELD_EX32(bus->regs, aspeed_i2c_bus_ctrl_offset(bus),
+                                   MASTER_EN);
 }
 
 static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
 {
-    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN) ||
-           ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, SLAVE_EN);
+    uint32_t ctrl_reg = aspeed_i2c_bus_ctrl_offset(bus);
+    return SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, MASTER_EN) ||
+           SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, SLAVE_EN);
 }
 
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
-
-    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[R_I2CD_INTR_STS],
-          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+    uint32_t intr_ctrl_reg = aspeed_i2c_bus_intr_ctrl_offset(bus);
+    bool raise_irq;
+
+    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
+        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
+        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
+        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
                                                                   : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
-                                                               : "");
-
-    bus->regs[R_I2CD_INTR_STS] &= bus->regs[R_I2CD_INTR_CTRL];
-    if (bus->regs[R_I2CD_INTR_STS]) {
+        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
+                                                                "normal|" : "",
+        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"
+                                                                   : "");
+    raise_irq = bus->regs[reg_intr_sts] & bus->regs[intr_ctrl_reg];
+    /* In packet mode we don't mask off INTR_STS */
+    if (!aspeed_i2c_bus_pkt_mode_en(bus)) {
+        bus->regs[reg_intr_sts] &= bus->regs[intr_ctrl_reg];
+    }
+    if (raise_irq) {
         bus->controller->intr_status |= 1 << bus->id;
         qemu_irq_raise(aic->bus_get_irq(bus));
     }
 }
 
-static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
-                                    unsigned size)
+static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
+                                        unsigned size)
 {
-    AspeedI2CBus *bus = opaque;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
 
@@ -180,8 +341,7 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
         /* Value is already set, don't do anything. */
         break;
     case A_I2CD_CMD:
-        value = FIELD_DP32(value, I2CD_CMD, BUS_BUSY_STS,
-                           i2c_bus_busy(bus->bus));
+        value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, i2c_bus_busy(bus->bus));
         break;
     case A_I2CD_DMA_ADDR:
         if (!aic->has_dma) {
@@ -207,31 +367,86 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
     return value;
 }
 
+static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
+                                        unsigned size)
+{
+    uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
+
+    switch (offset) {
+    case A_I2CC_FUN_CTRL:
+    case A_I2CC_AC_TIMING:
+    case A_I2CC_POOL_CTRL:
+    case A_I2CM_INTR_CTRL:
+    case A_I2CM_INTR_STS:
+    case A_I2CC_MS_TXRX_BYTE_BUF:
+    case A_I2CM_DMA_LEN:
+    case A_I2CM_DMA_TX_ADDR:
+    case A_I2CM_DMA_RX_ADDR:
+    case A_I2CM_DMA_LEN_STS:
+    case A_I2CC_DMA_ADDR:
+    case A_I2CC_DMA_LEN:
+        /* Value is already set, don't do anything. */
+        break;
+    case A_I2CM_CMD:
+        value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, i2c_bus_busy(bus->bus));
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+        value = -1;
+        break;
+    }
+
+    trace_aspeed_i2c_bus_read(bus->id, offset, size, value);
+    return value;
+}
+
+static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
+                                    unsigned size)
+{
+    AspeedI2CBus *bus = opaque;
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return aspeed_i2c_bus_new_read(bus, offset, size);
+    }
+    return aspeed_i2c_bus_old_read(bus, offset, size);
+}
+
 static void aspeed_i2c_set_state(AspeedI2CBus *bus, uint8_t state)
 {
-    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_STATE, state);
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CC_MS_TXRX_BYTE_BUF, TX_STATE,
+                                state);
+    } else {
+        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CD_CMD, TX_STATE, state);
+    }
 }
 
 static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
 {
-    return ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_STATE);
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CC_MS_TXRX_BYTE_BUF,
+                                       TX_STATE);
+    }
+    return SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD, TX_STATE);
 }
 
 static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
 {
     MemTxResult result;
     AspeedI2CState *s = bus->controller;
+    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
+    uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
 
-    result = address_space_read(&s->dram_as, bus->regs[R_I2CD_DMA_ADDR],
+    result = address_space_read(&s->dram_as, bus->regs[reg_dma_addr],
                                 MEMTXATTRS_UNSPECIFIED, data, 1);
     if (result != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
-                      __func__, bus->regs[R_I2CD_DMA_ADDR]);
+                      __func__, bus->regs[reg_dma_addr]);
         return -1;
     }
 
-    bus->regs[R_I2CD_DMA_ADDR]++;
-    bus->regs[R_I2CD_DMA_LEN]--;
+    bus->regs[reg_dma_addr]++;
+    bus->regs[reg_dma_len]--;
     return 0;
 }
 
@@ -240,9 +455,14 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     int ret = -1;
     int i;
-    int pool_tx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
-
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
+    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+    uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
+    int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
+                                                TX_COUNT);
+
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
         for (i = pool_start; i < pool_tx_count; i++) {
             uint8_t *pool_base = aic->bus_pool_base(bus);
 
@@ -253,23 +473,33 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
                 break;
             }
         }
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_BUFF_EN, 0);
-    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
-        while (bus->regs[R_I2CD_DMA_LEN]) {
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_BUFF_EN, 0);
+    } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
+        /* In new mode, clear how many bytes we TXed */
+        if (aspeed_i2c_is_new_mode(bus->controller)) {
+            ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, TX_LEN, 0);
+        }
+        while (bus->regs[reg_dma_len]) {
             uint8_t data;
             aspeed_i2c_dma_read(bus, &data);
-            trace_aspeed_i2c_bus_send("DMA", bus->regs[R_I2CD_DMA_LEN],
-                                      bus->regs[R_I2CD_DMA_LEN], data);
+            trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
+                                      bus->regs[reg_dma_len], data);
             ret = i2c_send(bus->bus, data);
             if (ret) {
                 break;
             }
+            /* In new mode, keep track of how many bytes we TXed */
+            if (aspeed_i2c_is_new_mode(bus->controller)) {
+                ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, TX_LEN,
+                                 ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN_STS,
+                                                  TX_LEN) + 1);
+            }
         }
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_DMA_EN, 0);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_DMA_EN, 0);
     } else {
         trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
-                                  bus->regs[R_I2CD_BYTE_BUF]);
-        ret = i2c_send(bus->bus, bus->regs[R_I2CD_BYTE_BUF]);
+                                  bus->regs[reg_byte_buf]);
+        ret = i2c_send(bus->bus, bus->regs[reg_byte_buf]);
     }
 
     return ret;
@@ -281,9 +511,15 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
     uint8_t data;
     int i;
-    int pool_rx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, RX_COUNT);
-
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
+    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+    uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
+    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
+    int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
+                                                RX_COUNT);
+
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
         for (i = 0; i < pool_rx_count; i++) {
@@ -293,64 +529,82 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
         }
 
         /* Update RX count */
-        ARRAY_FIELD_DP32(bus->regs, I2CD_POOL_CTRL, RX_COUNT, i & 0xff);
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_BUFF_EN, 0);
-    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_pool_ctrl, RX_COUNT, i & 0xff);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_BUFF_EN, 0);
+    } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
         uint8_t data;
+        /* In new mode, clear how many bytes we RXed */
+        if (aspeed_i2c_is_new_mode(bus->controller)) {
+            ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN, 0);
+        }
 
-        while (bus->regs[R_I2CD_DMA_LEN]) {
+        while (bus->regs[reg_dma_len]) {
             MemTxResult result;
 
             data = i2c_recv(bus->bus);
-            trace_aspeed_i2c_bus_recv("DMA", bus->regs[R_I2CD_DMA_LEN],
-                                      bus->regs[R_I2CD_DMA_LEN], data);
-            result = address_space_write(&s->dram_as,
-                                         bus->regs[R_I2CD_DMA_ADDR],
+            trace_aspeed_i2c_bus_recv("DMA", bus->regs[reg_dma_len],
+                                      bus->regs[reg_dma_len], data);
+            result = address_space_write(&s->dram_as, bus->regs[reg_dma_addr],
                                          MEMTXATTRS_UNSPECIFIED, &data, 1);
             if (result != MEMTX_OK) {
                 qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
-                              __func__, bus->regs[R_I2CD_DMA_ADDR]);
+                              __func__, bus->regs[reg_dma_addr]);
                 return;
             }
-            bus->regs[R_I2CD_DMA_ADDR]++;
-            bus->regs[R_I2CD_DMA_LEN]--;
+            bus->regs[reg_dma_addr]++;
+            bus->regs[reg_dma_len]--;
+            /* In new mode, keep track of how many bytes we RXed */
+            if (aspeed_i2c_is_new_mode(bus->controller)) {
+                ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN,
+                                 ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN_STS,
+                                                  RX_LEN) + 1);
+            }
         }
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_DMA_EN, 0);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_DMA_EN, 0);
     } else {
         data = i2c_recv(bus->bus);
-        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->regs[R_I2CD_BYTE_BUF]);
-        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, RX_BUF, data);
+        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->regs[reg_byte_buf]);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, data);
     }
 }
 
 static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
 {
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+
     aspeed_i2c_set_state(bus, I2CD_MRXD);
     aspeed_i2c_bus_recv(bus);
-    ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, RX_DONE, 1);
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) {
+    SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_S_RX_CMD_LAST)) {
         i2c_nack(bus->bus);
     }
-    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_RX_CMD, 0);
-    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST, 0);
+    SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_RX_CMD, 0);
+    SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_S_RX_CMD_LAST, 0);
     aspeed_i2c_set_state(bus, I2CD_MACTIVE);
 }
 
 static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+    uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
 
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
+    if (aspeed_i2c_bus_pkt_mode_en(bus)) {
+        return (ARRAY_FIELD_EX32(bus->regs, I2CM_CMD, PKT_DEV_ADDR) << 1) |
+                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_RX_CMD);
+    }
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
 
         return pool_base[0];
-    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
+    } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
         uint8_t data;
 
         aspeed_i2c_dma_read(bus, &data);
         return data;
     } else {
-        return bus->regs[R_I2CD_BYTE_BUF];
+        return bus->regs[reg_byte_buf];
     }
 }
 
@@ -358,10 +612,11 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
 {
     AspeedI2CState *s = bus->controller;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
-    bool dma_en = ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)  ||
-                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)  ||
-                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ||
-                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN);
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    bool dma_en = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)  ||
+                  SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)  ||
+                  SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN) ||
+                  SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN);
     if (!aic->check_sram) {
         return true;
     }
@@ -382,27 +637,31 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
 {
     g_autofree char *cmd_flags = NULL;
     uint32_t count;
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
-        count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
-    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
-        count = bus->regs[R_I2CD_DMA_LEN];
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+    uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
+        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
+    } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
+        count = bus->regs[reg_dma_len];
     } else { /* BYTE mode */
         count = 1;
     }
 
     cmd_flags = g_strdup_printf("%s%s%s%s%s%s%s%s%s",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD) ? "start|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
-          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
-
-    trace_aspeed_i2c_bus_cmd(bus->regs[R_I2CD_CMD], cmd_flags, count,
-                             bus->regs[R_I2CD_INTR_STS]);
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_START_CMD) ? "start|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN) ? "rxdma|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN) ? "txdma|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN) ? "rxbuf|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN) ? "txbuf|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_TX_CMD) ? "tx|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_RX_CMD) ? "rx|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_S_RX_CMD_LAST) ? "last|" : "",
+    SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_STOP_CMD) ? "stop|" : "");
+
+    trace_aspeed_i2c_bus_cmd(bus->regs[reg_cmd], cmd_flags, count,
+                             bus->regs[reg_intr_sts]);
 }
 
 /*
@@ -412,9 +671,10 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
 static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 {
     uint8_t pool_start = 0;
-
-    bus->regs[R_I2CD_CMD] &= ~0xFFFF;
-    bus->regs[R_I2CD_CMD] |= value & 0xFFFF;
+    uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+    uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
+    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
+    uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
 
     if (!aspeed_i2c_check_sram(bus)) {
         return;
@@ -424,7 +684,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_bus_cmd_dump(bus);
     }
 
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD)) {
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_START_CMD)) {
         uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
             I2CD_MSTARTR : I2CD_MSTART;
         uint8_t addr;
@@ -432,24 +692,30 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_set_state(bus, state);
 
         addr = aspeed_i2c_get_addr(bus);
-
         if (i2c_start_transfer(bus->bus, extract32(addr, 1, 7),
                                extract32(addr, 0, 1))) {
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_NAK, 1);
+            if (aspeed_i2c_bus_pkt_mode_en(bus)) {
+                ARRAY_FIELD_DP32(bus->regs, I2CM_INTR_STS, PKT_CMD_FAIL, 1);
+            }
         } else {
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
+            /* START doesn't set TX_ACK in packet mode */
+            if (!aspeed_i2c_bus_pkt_mode_en(bus)) {
+                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_ACK, 1);
+            }
         }
 
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_START_CMD, 0);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_START_CMD, 0);
 
         /*
          * The START command is also a TX command, as the slave
          * address is sent on the bus. Drop the TX flag if nothing
          * else needs to be sent in this sequence.
          */
-        if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
-            if (ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT) == 1) {
-                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
+            if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
+                == 1) {
+                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
             } else {
                 /*
                  * Increase the start index in the TX pool buffer to
@@ -457,57 +723,216 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
                  */
                 pool_start++;
             }
-        } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
-            if (bus->regs[R_I2CD_DMA_LEN] == 0) {
-                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
+        } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
+            if (bus->regs[reg_dma_len] == 0) {
+                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
             }
         } else {
-            ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
         }
 
         /* No slave found */
         if (!i2c_bus_busy(bus->bus)) {
+            if (aspeed_i2c_bus_pkt_mode_en(bus)) {
+                ARRAY_FIELD_DP32(bus->regs, I2CM_INTR_STS, PKT_CMD_FAIL, 1);
+                ARRAY_FIELD_DP32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE, 1);
+            }
             return;
         }
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD)) {
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_TX_CMD)) {
         aspeed_i2c_set_state(bus, I2CD_MTXD);
         if (aspeed_i2c_bus_send(bus, pool_start)) {
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_NAK, 1);
             i2c_end_transfer(bus->bus);
         } else {
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_ACK, 1);
         }
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if ((ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
-         ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) &&
-        !ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE)) {
+    if ((SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_RX_CMD) ||
+         SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_S_RX_CMD_LAST)) &&
+        !SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE)) {
         aspeed_i2c_handle_rx_cmd(bus);
     }
 
-    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD)) {
+    if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_STOP_CMD)) {
         if (!(aspeed_i2c_get_state(bus) & I2CD_MACTIVE)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: abnormal stop\n", __func__);
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, ABNORMAL, 1);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, ABNORMAL, 1);
+            if (aspeed_i2c_bus_pkt_mode_en(bus)) {
+                ARRAY_FIELD_DP32(bus->regs, I2CM_INTR_STS, PKT_CMD_FAIL, 1);
+            }
         } else {
             aspeed_i2c_set_state(bus, I2CD_MSTOP);
             i2c_end_transfer(bus->bus);
-            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, NORMAL_STOP, 1);
+            SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, NORMAL_STOP, 1);
         }
-        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_STOP_CMD, 0);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_IDLE);
     }
+
+    if (aspeed_i2c_bus_pkt_mode_en(bus)) {
+        ARRAY_FIELD_DP32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE, 1);
+    }
 }
 
-static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
-                                 uint64_t value, unsigned size)
+static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
+                                     uint64_t value, unsigned size)
+{
+    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+    bool handle_rx;
+    bool w1t;
+
+    trace_aspeed_i2c_bus_write(bus->id, offset, size, value);
+
+    switch (offset) {
+    case A_I2CC_FUN_CTRL:
+        if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
+            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+                          __func__);
+            break;
+        }
+        bus->regs[R_I2CD_FUN_CTRL] = value & 0x007dc3ff;
+        break;
+    case A_I2CC_AC_TIMING:
+        bus->regs[R_I2CC_AC_TIMING] = value & 0x1ffff0ff;
+        break;
+    case A_I2CC_MS_TXRX_BYTE_BUF:
+        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CC_MS_TXRX_BYTE_BUF, TX_BUF,
+                                value);
+        break;
+    case A_I2CC_POOL_CTRL:
+        bus->regs[R_I2CC_POOL_CTRL] &= ~0xffffff;
+        bus->regs[R_I2CC_POOL_CTRL] |= (value & 0xffffff);
+        break;
+    case A_I2CM_INTR_CTRL:
+        bus->regs[R_I2CM_INTR_CTRL] = value & 0x0007f07f;
+        break;
+    case A_I2CM_INTR_STS:
+        handle_rx = SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CM_INTR_STS, RX_DONE)
+                    && SHARED_FIELD_EX32(value, RX_DONE);
+
+        /* In packet mode, clearing PKT_CMD_DONE clears other interrupts. */
+        if (aspeed_i2c_bus_pkt_mode_en(bus) &&
+           FIELD_EX32(value, I2CM_INTR_STS, PKT_CMD_DONE)) {
+            bus->regs[R_I2CM_INTR_STS] &= 0xf0001000;
+            if (!bus->regs[R_I2CM_INTR_STS]) {
+                bus->controller->intr_status &= ~(1 << bus->id);
+                qemu_irq_lower(aic->bus_get_irq(bus));
+            }
+            break;
+        }
+        bus->regs[R_I2CM_INTR_STS] &= ~(value & 0xf007f07f);
+        if (!bus->regs[R_I2CM_INTR_STS]) {
+            bus->controller->intr_status &= ~(1 << bus->id);
+            qemu_irq_lower(aic->bus_get_irq(bus));
+        }
+        if (handle_rx && (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CM_CMD,
+                                                  M_RX_CMD) ||
+                          SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CM_CMD,
+                                                  M_S_RX_CMD_LAST))) {
+            aspeed_i2c_handle_rx_cmd(bus);
+            aspeed_i2c_bus_raise_interrupt(bus);
+        }
+        break;
+    case A_I2CM_CMD:
+        if (!aspeed_i2c_bus_is_enabled(bus)) {
+            break;
+        }
+
+        if (!aspeed_i2c_bus_is_master(bus)) {
+            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+                          __func__);
+            break;
+        }
+
+        if (!aic->has_dma &&
+            (SHARED_FIELD_EX32(value, RX_DMA_EN) ||
+             SHARED_FIELD_EX32(value, TX_DMA_EN))) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
+            break;
+        }
+
+        if (bus->regs[R_I2CM_INTR_STS] & 0xffff0000) {
+            qemu_log_mask(LOG_UNIMP, "%s: Packet mode is not implemented\n",
+                          __func__);
+            break;
+        }
+
+        value &= 0xff0ffbfb;
+        if (ARRAY_FIELD_EX32(bus->regs, I2CM_CMD, W1_CTRL)) {
+            bus->regs[R_I2CM_CMD] |= value;
+        } else {
+            bus->regs[R_I2CM_CMD] = value;
+        }
+
+        aspeed_i2c_bus_handle_cmd(bus, value);
+        aspeed_i2c_bus_raise_interrupt(bus);
+        break;
+    case A_I2CM_DMA_TX_ADDR:
+        bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR,
+                                                   ADDR);
+        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR);
+        bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
+                                                     TX_BUF_LEN) + 1;
+        break;
+    case A_I2CM_DMA_RX_ADDR:
+        bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR,
+                                                   ADDR);
+        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR);
+        bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
+                                                     RX_BUF_LEN) + 1;
+        break;
+    case A_I2CM_DMA_LEN:
+        w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
+                   ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
+        /* If none of the w1t bits are set, just write to the reg as normal. */
+        if (!w1t) {
+            bus->regs[R_I2CM_DMA_LEN] = value;
+            break;
+        }
+        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
+            ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN,
+                             FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN));
+        }
+        if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
+            ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN,
+                             FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN));
+        }
+        break;
+    case A_I2CM_DMA_LEN_STS:
+        /* Writes clear to 0 */
+        bus->regs[R_I2CM_DMA_LEN_STS] = 0;
+        break;
+    case A_I2CC_DMA_ADDR:
+    case A_I2CC_DMA_LEN:
+        /* RO */
+        break;
+    case A_I2CS_DMA_LEN_STS:
+    case A_I2CS_DMA_TX_ADDR:
+    case A_I2CS_DMA_RX_ADDR:
+    case A_I2CS_DEV_ADDR:
+    case A_I2CS_INTR_CTRL:
+    case A_I2CS_INTR_STS:
+    case A_I2CS_CMD:
+    case A_I2CS_DMA_LEN:
+        qemu_log_mask(LOG_UNIMP, "%s: Slave mode is not implemented\n",
+                      __func__);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+    }
+}
+
+static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
+                                     uint64_t value, unsigned size)
 {
-    AspeedI2CBus *bus = opaque;
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     bool handle_rx;
 
@@ -515,7 +940,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case A_I2CD_FUN_CTRL:
-        if (FIELD_EX32(value, I2CD_FUN_CTRL, SLAVE_EN)) {
+        if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
             qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
                           __func__);
             break;
@@ -532,15 +957,17 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         bus->regs[R_I2CD_INTR_CTRL] = value & 0x7FFF;
         break;
     case A_I2CD_INTR_STS:
-        handle_rx = ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) &&
-                    FIELD_EX32(value, I2CD_INTR_STS, RX_DONE);
+        handle_rx = SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_INTR_STS, RX_DONE)
+                    && SHARED_FIELD_EX32(value, RX_DONE);
         bus->regs[R_I2CD_INTR_STS] &= ~(value & 0x7FFF);
         if (!bus->regs[R_I2CD_INTR_STS]) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
         }
-        if (handle_rx && (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
-                      ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST))) {
+        if (handle_rx && (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
+                                                  M_RX_CMD) ||
+                      SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
+                                              M_S_RX_CMD_LAST))) {
             aspeed_i2c_handle_rx_cmd(bus);
             aspeed_i2c_bus_raise_interrupt(bus);
         }
@@ -555,7 +982,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         break;
 
     case A_I2CD_BYTE_BUF:
-        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, TX_BUF, value);
+        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CD_BYTE_BUF, TX_BUF, value);
         break;
     case A_I2CD_CMD:
         if (!aspeed_i2c_bus_is_enabled(bus)) {
@@ -569,12 +996,15 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         }
 
         if (!aic->has_dma &&
-            (FIELD_EX32(value, I2CD_CMD, RX_DMA_EN) ||
-             FIELD_EX32(value, I2CD_CMD, TX_DMA_EN))) {
+            (SHARED_FIELD_EX32(value, RX_DMA_EN) ||
+             SHARED_FIELD_EX32(value, TX_DMA_EN))) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             break;
         }
 
+        bus->regs[R_I2CD_CMD] &= ~0xFFFF;
+        bus->regs[R_I2CD_CMD] |= value & 0xFFFF;
+
         aspeed_i2c_bus_handle_cmd(bus, value);
         aspeed_i2c_bus_raise_interrupt(bus);
         break;
@@ -605,6 +1035,17 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
     }
 }
 
+static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
+                                     uint64_t value, unsigned size)
+{
+    AspeedI2CBus *bus = opaque;
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        aspeed_i2c_bus_new_write(bus, offset, value, size);
+    } else {
+        aspeed_i2c_bus_old_write(bus, offset, value, size);
+    }
+}
+
 static uint64_t aspeed_i2c_ctrl_read(void *opaque, hwaddr offset,
                                    unsigned size)
 {
@@ -615,6 +1056,13 @@ static uint64_t aspeed_i2c_ctrl_read(void *opaque, hwaddr offset,
         return s->intr_status;
     case A_I2C_CTRL_GLOBAL:
         return s->ctrl_global;
+    case A_I2C_CTRL_NEW_CLK_DIVIDER:
+        if (aspeed_i2c_is_new_mode(s)) {
+            return s->new_clk_divider;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
                       __func__, offset);
@@ -634,6 +1082,14 @@ static void aspeed_i2c_ctrl_write(void *opaque, hwaddr offset,
         value &= ~s->ctrl_global_rsvd;
         s->ctrl_global = value;
         break;
+    case A_I2C_CTRL_NEW_CLK_DIVIDER:
+        if (aspeed_i2c_is_new_mode(s)) {
+            s->new_clk_divider = value;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx
+                          "\n", __func__, offset);
+        }
+        break;
     case A_I2C_CTRL_STATUS:
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
@@ -691,10 +1147,10 @@ static const MemoryRegionOps aspeed_i2c_pool_ops = {
 
 static const VMStateDescription aspeed_i2c_bus_vmstate = {
     .name = TYPE_ASPEED_I2C,
-    .version_id = 4,
-    .minimum_version_id = 4,
+    .version_id = 5,
+    .minimum_version_id = 5,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_OLD_NUM_REG),
+        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_NEW_NUM_REG),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index e9f83a9540..86b18d6751 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -34,6 +34,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 #define ASPEED_I2C_NR_BUSSES 16
 #define ASPEED_I2C_MAX_POOL_SIZE 0x800
 #define ASPEED_I2C_OLD_NUM_REG 11
+#define ASPEED_I2C_NEW_NUM_REG 22
 
 struct AspeedI2CState;
 
@@ -50,7 +51,7 @@ struct AspeedI2CBus {
     uint8_t id;
     qemu_irq irq;
 
-    uint32_t regs[ASPEED_I2C_OLD_NUM_REG];
+    uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
 };
 
 struct AspeedI2CState {
@@ -61,6 +62,7 @@ struct AspeedI2CState {
 
     uint32_t intr_status;
     uint32_t ctrl_global;
+    uint32_t new_clk_divider;
     MemoryRegion pool_iomem;
     uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
 
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (4 preceding siblings ...)
  2022-03-31  4:32 ` [RFC PATCH 5/7] aspeed: i2c: Add new mode support Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-04-06 10:05   ` Cédric Le Goater
  2022-03-31  4:32 ` [RFC PATCH 7/7] aspeed: i2c: Move regs and helpers to header file Joe Komlodi
  2022-04-06 10:36 ` [RFC PATCH 0/7] aspeed: i2c: Add new mode support Cédric Le Goater
  7 siblings, 1 reply; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: I566eb09f4b9016e24570572f367627f6594039f5
---
 hw/i2c/aspeed_i2c.c | 3 +++
 hw/i2c/trace-events | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 948d8dc2bb..f179f78ee9 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -305,6 +305,9 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
     bool raise_irq;
 
     trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
+        aspeed_i2c_bus_pkt_mode_en(bus) &&
+        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
+                                                               "pktdone|" : "",
         SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
         SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
         SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 7d8907c1ee..85e4bddff9 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
 # aspeed_i2c.c
 
 aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
-aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
+aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
 aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
 aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
 aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
-- 
2.35.1.1021.g381101b075-goog



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

* [RFC PATCH 7/7] aspeed: i2c: Move regs and helpers to header file
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (5 preceding siblings ...)
  2022-03-31  4:32 ` [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace Joe Komlodi
@ 2022-03-31  4:32 ` Joe Komlodi
  2022-04-06 10:36 ` [RFC PATCH 0/7] aspeed: i2c: Add new mode support Cédric Le Goater
  7 siblings, 0 replies; 13+ messages in thread
From: Joe Komlodi @ 2022-03-31  4:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: clg, peter.maydell, andrew, joel, venture

Moves register definitions and short commonly used inlined functiosn to
the header file to help tidy up the implementation file.

Signed-off-by: Joe Komlodi <komlodi@google.com>
Change-Id: I34dff7485b6bbe3c9482715ccd94dbd65dc5f324
---
 hw/i2c/aspeed_i2c.c         | 266 -----------------------------------
 include/hw/i2c/aspeed_i2c.h | 267 ++++++++++++++++++++++++++++++++++++
 2 files changed, 267 insertions(+), 266 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index f179f78ee9..8425e92889 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -31,272 +31,6 @@
 #include "hw/registerfields.h"
 #include "trace.h"
 
-/* Tx State Machine */
-#define   I2CD_TX_STATE_MASK                  0xf
-#define     I2CD_IDLE                         0x0
-#define     I2CD_MACTIVE                      0x8
-#define     I2CD_MSTART                       0x9
-#define     I2CD_MSTARTR                      0xa
-#define     I2CD_MSTOP                        0xb
-#define     I2CD_MTXD                         0xc
-#define     I2CD_MRXACK                       0xd
-#define     I2CD_MRXD                         0xe
-#define     I2CD_MTXACK                       0xf
-#define     I2CD_SWAIT                        0x1
-#define     I2CD_SRXD                         0x4
-#define     I2CD_STXACK                       0x5
-#define     I2CD_STXD                         0x6
-#define     I2CD_SRXACK                       0x7
-#define     I2CD_RECOVER                      0x3
-
-/* I2C Global Register */
-REG32(I2C_CTRL_STATUS, 0x0) /* Device Interrupt Status */
-REG32(I2C_CTRL_ASSIGN, 0x8) /* Device Interrupt Target Assignment */
-REG32(I2C_CTRL_GLOBAL, 0xC) /* Global Control Register */
-    FIELD(I2C_CTRL_GLOBAL, REG_MODE, 2, 1)
-    FIELD(I2C_CTRL_GLOBAL, SRAM_EN, 0, 1)
-REG32(I2C_CTRL_NEW_CLK_DIVIDER, 0x10) /* New mode clock divider */
-
-/* I2C Old Mode Device (Bus) Register */
-REG32(I2CD_FUN_CTRL, 0x0) /* I2CD Function Control  */
-    FIELD(I2CD_FUN_CTRL, POOL_PAGE_SEL, 20, 3) /* AST2400 */
-    SHARED_FIELD(M_SDA_LOCK_EN, 16, 1)
-    SHARED_FIELD(MULTI_MASTER_DIS, 15, 1)
-    SHARED_FIELD(M_SCL_DRIVE_EN, 14, 1)
-    SHARED_FIELD(MSB_STS, 9, 1)
-    SHARED_FIELD(SDA_DRIVE_IT_EN, 8, 1)
-    SHARED_FIELD(M_SDA_DRIVE_IT_EN, 7, 1)
-    SHARED_FIELD(M_HIGH_SPEED_EN, 6, 1)
-    SHARED_FIELD(DEF_ADDR_EN, 5, 1)
-    SHARED_FIELD(DEF_ALERT_EN, 4, 1)
-    SHARED_FIELD(DEF_ARP_EN, 3, 1)
-    SHARED_FIELD(DEF_GCALL_EN, 2, 1)
-    SHARED_FIELD(SLAVE_EN, 1, 1)
-    SHARED_FIELD(MASTER_EN, 0, 1)
-REG32(I2CD_AC_TIMING1, 0x04) /* Clock and AC Timing Control #1 */
-REG32(I2CD_AC_TIMING2, 0x08) /* Clock and AC Timing Control #2 */
-REG32(I2CD_INTR_CTRL, 0x0C)  /* I2CD Interrupt Control */
-REG32(I2CD_INTR_STS, 0x10)   /* I2CD Interrupt Status */
-    SHARED_FIELD(SLAVE_ADDR_MATCH, 31, 1)    /* 0: addr1 1: addr2 */
-    SHARED_FIELD(SLAVE_ADDR_RX_PENDING, 29, 1)
-    SHARED_FIELD(SLAVE_INACTIVE_TIMEOUT, 15, 1)
-    SHARED_FIELD(SDA_DL_TIMEOUT, 14, 1)
-    SHARED_FIELD(BUS_RECOVER_DONE, 13, 1)
-    SHARED_FIELD(SMBUS_ALERT, 12, 1)                    /* Bus [0-3] only */
-    FIELD(I2CD_INTR_STS, SMBUS_ARP_ADDR, 11, 1)         /* Removed */
-    FIELD(I2CD_INTR_STS, SMBUS_DEV_ALERT_ADDR, 10, 1)   /* Removed */
-    FIELD(I2CD_INTR_STS, SMBUS_DEF_ADDR, 9, 1)          /* Removed */
-    FIELD(I2CD_INTR_STS, GCALL_ADDR, 8, 1)              /* Removed */
-    FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)     /* use RX_DONE */
-    SHARED_FIELD(SCL_TIMEOUT, 6, 1)
-    SHARED_FIELD(ABNORMAL, 5, 1)
-    SHARED_FIELD(NORMAL_STOP, 4, 1)
-    SHARED_FIELD(ARBIT_LOSS, 3, 1)
-    SHARED_FIELD(RX_DONE, 2, 1)
-    SHARED_FIELD(TX_NAK, 1, 1)
-    SHARED_FIELD(TX_ACK, 0, 1)
-REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
-    SHARED_FIELD(SDA_OE, 28, 1)
-    SHARED_FIELD(SDA_O, 27, 1)
-    SHARED_FIELD(SCL_OE, 26, 1)
-    SHARED_FIELD(SCL_O, 25, 1)
-    SHARED_FIELD(TX_TIMING, 23, 2)
-    SHARED_FIELD(TX_STATE, 19, 4)
-    SHARED_FIELD(SCL_LINE_STS, 18, 1)
-    SHARED_FIELD(SDA_LINE_STS, 17, 1)
-    SHARED_FIELD(BUS_BUSY_STS, 16, 1)
-    SHARED_FIELD(SDA_OE_OUT_DIR, 15, 1)
-    SHARED_FIELD(SDA_O_OUT_DIR, 14, 1)
-    SHARED_FIELD(SCL_OE_OUT_DIR, 13, 1)
-    SHARED_FIELD(SCL_O_OUT_DIR, 12, 1)
-    SHARED_FIELD(BUS_RECOVER_CMD_EN, 11, 1)
-    SHARED_FIELD(S_ALT_EN, 10, 1)
-    /* Command Bits */
-    SHARED_FIELD(RX_DMA_EN, 9, 1)
-    SHARED_FIELD(TX_DMA_EN, 8, 1)
-    SHARED_FIELD(RX_BUFF_EN, 7, 1)
-    SHARED_FIELD(TX_BUFF_EN, 6, 1)
-    SHARED_FIELD(M_STOP_CMD, 5, 1)
-    SHARED_FIELD(M_S_RX_CMD_LAST, 4, 1)
-    SHARED_FIELD(M_RX_CMD, 3, 1)
-    SHARED_FIELD(S_TX_CMD, 2, 1)
-    SHARED_FIELD(M_TX_CMD, 1, 1)
-    SHARED_FIELD(M_START_CMD, 0, 1)
-REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
-REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
-    SHARED_FIELD(RX_COUNT, 24, 5)
-    SHARED_FIELD(RX_SIZE, 16, 5)
-    SHARED_FIELD(TX_COUNT, 9, 5)
-    FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
-REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
-    SHARED_FIELD(RX_BUF, 8, 8)
-    SHARED_FIELD(TX_BUF, 0, 8)
-REG32(I2CD_DMA_ADDR, 0x24) /* DMA Buffer Address */
-REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
-
-/* I2C New Mode Device (Bus) Register */
-REG32(I2CC_FUN_CTRL, 0x0)
-    FIELD(I2CC_FUN_CTRL, RB_EARLY_DONE_EN, 22, 1)
-    FIELD(I2CC_FUN_CTRL, DMA_DIS_AUTO_RECOVER, 21, 1)
-    FIELD(I2CC_FUN_CTRL, S_SAVE_ADDR, 20, 1)
-    FIELD(I2CC_FUN_CTRL, M_PKT_RETRY_CNT, 18, 2)
-    /* 17:0 shared with I2CD_FUN_CTRL[17:0] */
-REG32(I2CC_AC_TIMING, 0x04)
-REG32(I2CC_MS_TXRX_BYTE_BUF, 0x08)
-    /* 31:16 shared with I2CD_CMD[31:16] */
-    /* 15:0  shared with I2CD_BYTE_BUF[15:0] */
-REG32(I2CC_POOL_CTRL, 0x0c)
-    /* 31:0 shared with I2CD_POOL_CTRL[31:0] */
-REG32(I2CM_INTR_CTRL, 0x10)
-REG32(I2CM_INTR_STS, 0x14)
-    FIELD(I2CM_INTR_STS, PKT_STATE, 28, 4)
-    FIELD(I2CM_INTR_STS, PKT_CMD_TIMEOUT, 18, 1)
-    FIELD(I2CM_INTR_STS, PKT_CMD_FAIL, 17, 1)
-    FIELD(I2CM_INTR_STS, PKT_CMD_DONE, 16, 1)
-    FIELD(I2CM_INTR_STS, BUS_RECOVER_FAIL, 15, 1)
-    /* 14:0 shared with I2CD_INTR_STS[14:0] */
-REG32(I2CM_CMD, 0x18)
-    FIELD(I2CM_CMD, W1_CTRL, 31, 1)
-    FIELD(I2CM_CMD, PKT_DEV_ADDR, 24, 7)
-    FIELD(I2CM_CMD, HS_MASTER_MODE_LSB, 17, 3)
-    FIELD(I2CM_CMD, PKT_OP_EN, 16, 1)
-    /* 15:0 shared with I2CD_CMD[15:0] */
-REG32(I2CM_DMA_LEN, 0x1c)
-    FIELD(I2CM_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
-    FIELD(I2CM_DMA_LEN, RX_BUF_LEN, 16, 11)
-    FIELD(I2CM_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
-    FIELD(I2CM_DMA_LEN, TX_BUF_LEN, 0, 11)
-REG32(I2CS_INTR_CTRL, 0x20)
-REG32(I2CS_INTR_STS, 0x24)
-    /* 31:29 shared with I2CD_INTR_STS[31:29] */
-    FIELD(I2CS_INTR_STS, SLAVE_PARKING_STS, 24, 2)
-    FIELD(I2CS_INTR_STS, SLAVE_ADDR3_NAK, 22, 1)
-    FIELD(I2CS_INTR_STS, SLAVE_ADDR2_NAK, 21, 1)
-    FIELD(I2CS_INTR_STS, SLAVE_ADDR1_NAK, 20, 1)
-    FIELD(I2CS_INTR_STS, SLAVE_ADDR_INDICATOR, 18, 2)
-    FIELD(I2CS_INTR_STS, PKT_CMD_FAIL, 17, 1)
-    FIELD(I2CS_INTR_STS, PKT_CMD_DONE, 16, 1)
-    /* 14:0 shared with I2CD_INTR_STS[14:0] */
-REG32(I2CS_CMD, 0x28)
-    FIELD(I2CS_CMD, W1_CTRL, 31, 1)
-    FIELD(I2CS_CMD, PKT_MODE_ACTIVE_ADDR, 17, 2)
-    FIELD(I2CS_CMD, PKT_MODE_EN, 16, 1)
-    FIELD(I2CS_CMD, AUTO_NAK_INACTIVE_ADDR, 15, 1)
-    FIELD(I2CS_CMD, AUTO_NAK_ACTIVE_ADDR, 14, 1)
-    /* 13:0 shared with I2CD_CMD[13:0] */
-REG32(I2CS_DMA_LEN, 0x2c)
-    FIELD(I2CS_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
-    FIELD(I2CS_DMA_LEN, RX_BUF_LEN, 16, 11)
-    FIELD(I2CS_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
-    FIELD(I2CS_DMA_LEN, TX_BUF_LEN, 0, 11)
-REG32(I2CM_DMA_TX_ADDR, 0x30)
-    FIELD(I2CM_DMA_TX_ADDR, ADDR, 0, 31)
-REG32(I2CM_DMA_RX_ADDR, 0x34)
-    FIELD(I2CM_DMA_RX_ADDR, ADDR, 0, 31)
-REG32(I2CS_DMA_TX_ADDR, 0x38)
-    FIELD(I2CS_DMA_TX_ADDR, ADDR, 0, 31)
-REG32(I2CS_DMA_RX_ADDR, 0x3c)
-    FIELD(I2CS_DMA_RX_ADDR, ADDR, 0, 31)
-REG32(I2CS_DEV_ADDR, 0x40)
-REG32(I2CM_DMA_LEN_STS, 0x48)
-    FIELD(I2CM_DMA_LEN_STS, RX_LEN, 16, 13)
-    FIELD(I2CM_DMA_LEN_STS, TX_LEN, 0, 13)
-REG32(I2CS_DMA_LEN_STS, 0x4c)
-    FIELD(I2CS_DMA_LEN_STS, RX_LEN, 16, 13)
-    FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
-REG32(I2CC_DMA_ADDR, 0x50)
-REG32(I2CC_DMA_LEN, 0x54)
-
-static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
-{
-    return FIELD_EX32(s->ctrl_global, I2C_CTRL_GLOBAL, REG_MODE);
-}
-
-static inline bool aspeed_i2c_bus_pkt_mode_en(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return ARRAY_FIELD_EX32(bus->regs, I2CM_CMD, PKT_OP_EN);
-    }
-    return false;
-}
-
-static inline uint32_t aspeed_i2c_bus_ctrl_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_FUN_CTRL;
-    }
-    return R_I2CD_FUN_CTRL;
-}
-
-static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CM_CMD;
-    }
-    return R_I2CD_CMD;
-}
-
-static inline uint32_t aspeed_i2c_bus_intr_ctrl_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CM_INTR_CTRL;
-    }
-    return R_I2CD_INTR_CTRL;
-}
-
-static inline uint32_t aspeed_i2c_bus_intr_sts_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CM_INTR_STS;
-    }
-    return R_I2CD_INTR_STS;
-}
-
-static inline uint32_t aspeed_i2c_bus_pool_ctrl_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_POOL_CTRL;
-    }
-    return R_I2CD_POOL_CTRL;
-}
-
-static inline uint32_t aspeed_i2c_bus_byte_buf_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_MS_TXRX_BYTE_BUF;
-    }
-    return R_I2CD_BYTE_BUF;
-}
-
-static inline uint32_t aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_DMA_LEN;
-    }
-    return R_I2CD_DMA_LEN;
-}
-
-static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_DMA_ADDR;
-    }
-    return R_I2CD_DMA_ADDR;
-}
-
-static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
-{
-    return SHARED_ARRAY_FIELD_EX32(bus->regs, aspeed_i2c_bus_ctrl_offset(bus),
-                                   MASTER_EN);
-}
-
-static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
-{
-    uint32_t ctrl_reg = aspeed_i2c_bus_ctrl_offset(bus);
-    return SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, MASTER_EN) ||
-           SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, SLAVE_EN);
-}
-
 static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 86b18d6751..506bcf1a9d 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -23,6 +23,7 @@
 
 #include "hw/i2c/i2c.h"
 #include "hw/sysbus.h"
+#include "hw/registerfields.h"
 #include "qom/object.h"
 
 #define TYPE_ASPEED_I2C "aspeed.i2c"
@@ -36,6 +37,182 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 #define ASPEED_I2C_OLD_NUM_REG 11
 #define ASPEED_I2C_NEW_NUM_REG 22
 
+/* Tx State Machine */
+#define   I2CD_TX_STATE_MASK                  0xf
+#define     I2CD_IDLE                         0x0
+#define     I2CD_MACTIVE                      0x8
+#define     I2CD_MSTART                       0x9
+#define     I2CD_MSTARTR                      0xa
+#define     I2CD_MSTOP                        0xb
+#define     I2CD_MTXD                         0xc
+#define     I2CD_MRXACK                       0xd
+#define     I2CD_MRXD                         0xe
+#define     I2CD_MTXACK                       0xf
+#define     I2CD_SWAIT                        0x1
+#define     I2CD_SRXD                         0x4
+#define     I2CD_STXACK                       0x5
+#define     I2CD_STXD                         0x6
+#define     I2CD_SRXACK                       0x7
+#define     I2CD_RECOVER                      0x3
+
+/* I2C Global Register */
+REG32(I2C_CTRL_STATUS, 0x0) /* Device Interrupt Status */
+REG32(I2C_CTRL_ASSIGN, 0x8) /* Device Interrupt Target Assignment */
+REG32(I2C_CTRL_GLOBAL, 0xC) /* Global Control Register */
+    FIELD(I2C_CTRL_GLOBAL, REG_MODE, 2, 1)
+    FIELD(I2C_CTRL_GLOBAL, SRAM_EN, 0, 1)
+REG32(I2C_CTRL_NEW_CLK_DIVIDER, 0x10) /* New mode clock divider */
+
+/* I2C Old Mode Device (Bus) Register */
+REG32(I2CD_FUN_CTRL, 0x0) /* I2CD Function Control  */
+    FIELD(I2CD_FUN_CTRL, POOL_PAGE_SEL, 20, 3) /* AST2400 */
+    SHARED_FIELD(M_SDA_LOCK_EN, 16, 1)
+    SHARED_FIELD(MULTI_MASTER_DIS, 15, 1)
+    SHARED_FIELD(M_SCL_DRIVE_EN, 14, 1)
+    SHARED_FIELD(MSB_STS, 9, 1)
+    SHARED_FIELD(SDA_DRIVE_IT_EN, 8, 1)
+    SHARED_FIELD(M_SDA_DRIVE_IT_EN, 7, 1)
+    SHARED_FIELD(M_HIGH_SPEED_EN, 6, 1)
+    SHARED_FIELD(DEF_ADDR_EN, 5, 1)
+    SHARED_FIELD(DEF_ALERT_EN, 4, 1)
+    SHARED_FIELD(DEF_ARP_EN, 3, 1)
+    SHARED_FIELD(DEF_GCALL_EN, 2, 1)
+    SHARED_FIELD(SLAVE_EN, 1, 1)
+    SHARED_FIELD(MASTER_EN, 0, 1)
+REG32(I2CD_AC_TIMING1, 0x04) /* Clock and AC Timing Control #1 */
+REG32(I2CD_AC_TIMING2, 0x08) /* Clock and AC Timing Control #2 */
+REG32(I2CD_INTR_CTRL, 0x0C)  /* I2CD Interrupt Control */
+REG32(I2CD_INTR_STS, 0x10)   /* I2CD Interrupt Status */
+    SHARED_FIELD(SLAVE_ADDR_MATCH, 31, 1)    /* 0: addr1 1: addr2 */
+    SHARED_FIELD(SLAVE_ADDR_RX_PENDING, 29, 1)
+    SHARED_FIELD(SLAVE_INACTIVE_TIMEOUT, 15, 1)
+    SHARED_FIELD(SDA_DL_TIMEOUT, 14, 1)
+    SHARED_FIELD(BUS_RECOVER_DONE, 13, 1)
+    SHARED_FIELD(SMBUS_ALERT, 12, 1)                    /* Bus [0-3] only */
+    FIELD(I2CD_INTR_STS, SMBUS_ARP_ADDR, 11, 1)         /* Removed */
+    FIELD(I2CD_INTR_STS, SMBUS_DEV_ALERT_ADDR, 10, 1)   /* Removed */
+    FIELD(I2CD_INTR_STS, SMBUS_DEF_ADDR, 9, 1)          /* Removed */
+    FIELD(I2CD_INTR_STS, GCALL_ADDR, 8, 1)              /* Removed */
+    FIELD(I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)     /* use RX_DONE */
+    SHARED_FIELD(SCL_TIMEOUT, 6, 1)
+    SHARED_FIELD(ABNORMAL, 5, 1)
+    SHARED_FIELD(NORMAL_STOP, 4, 1)
+    SHARED_FIELD(ARBIT_LOSS, 3, 1)
+    SHARED_FIELD(RX_DONE, 2, 1)
+    SHARED_FIELD(TX_NAK, 1, 1)
+    SHARED_FIELD(TX_ACK, 0, 1)
+REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
+    SHARED_FIELD(SDA_OE, 28, 1)
+    SHARED_FIELD(SDA_O, 27, 1)
+    SHARED_FIELD(SCL_OE, 26, 1)
+    SHARED_FIELD(SCL_O, 25, 1)
+    SHARED_FIELD(TX_TIMING, 23, 2)
+    SHARED_FIELD(TX_STATE, 19, 4)
+    SHARED_FIELD(SCL_LINE_STS, 18, 1)
+    SHARED_FIELD(SDA_LINE_STS, 17, 1)
+    SHARED_FIELD(BUS_BUSY_STS, 16, 1)
+    SHARED_FIELD(SDA_OE_OUT_DIR, 15, 1)
+    SHARED_FIELD(SDA_O_OUT_DIR, 14, 1)
+    SHARED_FIELD(SCL_OE_OUT_DIR, 13, 1)
+    SHARED_FIELD(SCL_O_OUT_DIR, 12, 1)
+    SHARED_FIELD(BUS_RECOVER_CMD_EN, 11, 1)
+    SHARED_FIELD(S_ALT_EN, 10, 1)
+    /* Command Bits */
+    SHARED_FIELD(RX_DMA_EN, 9, 1)
+    SHARED_FIELD(TX_DMA_EN, 8, 1)
+    SHARED_FIELD(RX_BUFF_EN, 7, 1)
+    SHARED_FIELD(TX_BUFF_EN, 6, 1)
+    SHARED_FIELD(M_STOP_CMD, 5, 1)
+    SHARED_FIELD(M_S_RX_CMD_LAST, 4, 1)
+    SHARED_FIELD(M_RX_CMD, 3, 1)
+    SHARED_FIELD(S_TX_CMD, 2, 1)
+    SHARED_FIELD(M_TX_CMD, 1, 1)
+    SHARED_FIELD(M_START_CMD, 0, 1)
+REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
+REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
+    SHARED_FIELD(RX_COUNT, 24, 5)
+    SHARED_FIELD(RX_SIZE, 16, 5)
+    SHARED_FIELD(TX_COUNT, 9, 5)
+    FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
+REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
+    SHARED_FIELD(RX_BUF, 8, 8)
+    SHARED_FIELD(TX_BUF, 0, 8)
+REG32(I2CD_DMA_ADDR, 0x24) /* DMA Buffer Address */
+REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
+
+/* I2C New Mode Device (Bus) Register */
+REG32(I2CC_FUN_CTRL, 0x0)
+    FIELD(I2CC_FUN_CTRL, RB_EARLY_DONE_EN, 22, 1)
+    FIELD(I2CC_FUN_CTRL, DMA_DIS_AUTO_RECOVER, 21, 1)
+    FIELD(I2CC_FUN_CTRL, S_SAVE_ADDR, 20, 1)
+    FIELD(I2CC_FUN_CTRL, M_PKT_RETRY_CNT, 18, 2)
+    /* 17:0 shared with I2CD_FUN_CTRL[17:0] */
+REG32(I2CC_AC_TIMING, 0x04)
+REG32(I2CC_MS_TXRX_BYTE_BUF, 0x08)
+    /* 31:16 shared with I2CD_CMD[31:16] */
+    /* 15:0  shared with I2CD_BYTE_BUF[15:0] */
+REG32(I2CC_POOL_CTRL, 0x0c)
+    /* 31:0 shared with I2CD_POOL_CTRL[31:0] */
+REG32(I2CM_INTR_CTRL, 0x10)
+REG32(I2CM_INTR_STS, 0x14)
+    FIELD(I2CM_INTR_STS, PKT_STATE, 28, 4)
+    FIELD(I2CM_INTR_STS, PKT_CMD_TIMEOUT, 18, 1)
+    FIELD(I2CM_INTR_STS, PKT_CMD_FAIL, 17, 1)
+    FIELD(I2CM_INTR_STS, PKT_CMD_DONE, 16, 1)
+    FIELD(I2CM_INTR_STS, BUS_RECOVER_FAIL, 15, 1)
+    /* 14:0 shared with I2CD_INTR_STS[14:0] */
+REG32(I2CM_CMD, 0x18)
+    FIELD(I2CM_CMD, W1_CTRL, 31, 1)
+    FIELD(I2CM_CMD, PKT_DEV_ADDR, 24, 7)
+    FIELD(I2CM_CMD, HS_MASTER_MODE_LSB, 17, 3)
+    FIELD(I2CM_CMD, PKT_OP_EN, 16, 1)
+    /* 15:0 shared with I2CD_CMD[15:0] */
+REG32(I2CM_DMA_LEN, 0x1c)
+    FIELD(I2CM_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
+    FIELD(I2CM_DMA_LEN, RX_BUF_LEN, 16, 11)
+    FIELD(I2CM_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
+    FIELD(I2CM_DMA_LEN, TX_BUF_LEN, 0, 11)
+REG32(I2CS_INTR_CTRL, 0x20)
+REG32(I2CS_INTR_STS, 0x24)
+    /* 31:29 shared with I2CD_INTR_STS[31:29] */
+    FIELD(I2CS_INTR_STS, SLAVE_PARKING_STS, 24, 2)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR3_NAK, 22, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR2_NAK, 21, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR1_NAK, 20, 1)
+    FIELD(I2CS_INTR_STS, SLAVE_ADDR_INDICATOR, 18, 2)
+    FIELD(I2CS_INTR_STS, PKT_CMD_FAIL, 17, 1)
+    FIELD(I2CS_INTR_STS, PKT_CMD_DONE, 16, 1)
+    /* 14:0 shared with I2CD_INTR_STS[14:0] */
+REG32(I2CS_CMD, 0x28)
+    FIELD(I2CS_CMD, W1_CTRL, 31, 1)
+    FIELD(I2CS_CMD, PKT_MODE_ACTIVE_ADDR, 17, 2)
+    FIELD(I2CS_CMD, PKT_MODE_EN, 16, 1)
+    FIELD(I2CS_CMD, AUTO_NAK_INACTIVE_ADDR, 15, 1)
+    FIELD(I2CS_CMD, AUTO_NAK_ACTIVE_ADDR, 14, 1)
+    /* 13:0 shared with I2CD_CMD[13:0] */
+REG32(I2CS_DMA_LEN, 0x2c)
+    FIELD(I2CS_DMA_LEN, RX_BUF_LEN_W1T, 31, 1)
+    FIELD(I2CS_DMA_LEN, RX_BUF_LEN, 16, 11)
+    FIELD(I2CS_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
+    FIELD(I2CS_DMA_LEN, TX_BUF_LEN, 0, 11)
+REG32(I2CM_DMA_TX_ADDR, 0x30)
+    FIELD(I2CM_DMA_TX_ADDR, ADDR, 0, 31)
+REG32(I2CM_DMA_RX_ADDR, 0x34)
+    FIELD(I2CM_DMA_RX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DMA_TX_ADDR, 0x38)
+    FIELD(I2CS_DMA_TX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DMA_RX_ADDR, 0x3c)
+    FIELD(I2CS_DMA_RX_ADDR, ADDR, 0, 31)
+REG32(I2CS_DEV_ADDR, 0x40)
+REG32(I2CM_DMA_LEN_STS, 0x48)
+    FIELD(I2CM_DMA_LEN_STS, RX_LEN, 16, 13)
+    FIELD(I2CM_DMA_LEN_STS, TX_LEN, 0, 13)
+REG32(I2CS_DMA_LEN_STS, 0x4c)
+    FIELD(I2CS_DMA_LEN_STS, RX_LEN, 16, 13)
+    FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
+REG32(I2CC_DMA_ADDR, 0x50)
+REG32(I2CC_DMA_LEN, 0x54)
+
 struct AspeedI2CState;
 
 #define TYPE_ASPEED_I2C_BUS "aspeed.i2c.bus"
@@ -90,6 +267,96 @@ struct AspeedI2CClass {
 
 };
 
+static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
+{
+    return FIELD_EX32(s->ctrl_global, I2C_CTRL_GLOBAL, REG_MODE);
+}
+
+static inline bool aspeed_i2c_bus_pkt_mode_en(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return ARRAY_FIELD_EX32(bus->regs, I2CM_CMD, PKT_OP_EN);
+    }
+    return false;
+}
+
+static inline uint32_t aspeed_i2c_bus_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_FUN_CTRL;
+    }
+    return R_I2CD_FUN_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_cmd_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_CMD;
+    }
+    return R_I2CD_CMD;
+}
+
+static inline uint32_t aspeed_i2c_bus_intr_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_INTR_CTRL;
+    }
+    return R_I2CD_INTR_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_intr_sts_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CM_INTR_STS;
+    }
+    return R_I2CD_INTR_STS;
+}
+
+static inline uint32_t aspeed_i2c_bus_pool_ctrl_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_POOL_CTRL;
+    }
+    return R_I2CD_POOL_CTRL;
+}
+
+static inline uint32_t aspeed_i2c_bus_byte_buf_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_MS_TXRX_BYTE_BUF;
+    }
+    return R_I2CD_BYTE_BUF;
+}
+
+static inline uint32_t aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_DMA_LEN;
+    }
+    return R_I2CD_DMA_LEN;
+}
+
+static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus *bus)
+{
+    if (aspeed_i2c_is_new_mode(bus->controller)) {
+        return R_I2CC_DMA_ADDR;
+    }
+    return R_I2CD_DMA_ADDR;
+}
+
+static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
+{
+    return SHARED_ARRAY_FIELD_EX32(bus->regs, aspeed_i2c_bus_ctrl_offset(bus),
+                                   MASTER_EN);
+}
+
+static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
+{
+    uint32_t ctrl_reg = aspeed_i2c_bus_ctrl_offset(bus);
+    return SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, MASTER_EN) ||
+           SHARED_ARRAY_FIELD_EX32(bus->regs, ctrl_reg, SLAVE_EN);
+}
+
 I2CBus *aspeed_i2c_get_bus(AspeedI2CState *s, int busnr);
 
 #endif /* ASPEED_I2C_H */
-- 
2.35.1.1021.g381101b075-goog



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

* Re: [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property
  2022-03-31  4:32 ` [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property Joe Komlodi
@ 2022-04-06  9:57   ` Cédric Le Goater
  0 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-04-06  9:57 UTC (permalink / raw)
  To: Joe Komlodi, qemu-devel; +Cc: andrew, peter.maydell, joel, venture

Hello Joe,

On 3/31/22 06:32, Joe Komlodi wrote:
> The Aspeed I2C controller is used across other SKUs that have different
> reserved bits for the ctrl_global_rsvd register.

So some AST2600 SoCs can not set the new/old modes and the clock divider ?
  

> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Change-Id: I606c5933c527274a9d2b0afe559b2e895767636c
> ---
>   hw/arm/aspeed_ast2600.c     | 2 ++
>   hw/i2c/aspeed_i2c.c         | 4 ++++
>   include/hw/i2c/aspeed_i2c.h | 2 ++
>   3 files changed, 8 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 43f8223139..9f14a35a75 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -344,6 +344,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                      serial_hd(0), DEVICE_LITTLE_ENDIAN);
>   
>       /* I2C */
> +    object_property_set_int(OBJECT(&s->i2c), "ctrl-global-rsvd", 0xfffc3e00,
> +                            &error_abort);

Ideally, this should be set from a specific machine and not from the SoC
as it impacts all machines. One way to do this is to use an alias. See the
hw-strap1/2 properties.

Thanks,

C.

>       object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                                &error_abort);
>       if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 03a4f5a910..97eb9d5792 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -648,6 +648,7 @@ static void aspeed_i2c_ctrl_write(void *opaque, hwaddr offset,
>   
>       switch (offset) {
>       case I2C_CTRL_GLOBAL:
> +        value &= ~s->ctrl_global_rsvd;
>           s->ctrl_global = value;
>           break;
>       case I2C_CTRL_STATUS:
> @@ -730,6 +731,7 @@ static const VMStateDescription aspeed_i2c_vmstate = {
>       .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT32(intr_status, AspeedI2CState),
> +        VMSTATE_UINT32(ctrl_global_rsvd, AspeedI2CState),
>           VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
>                                ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
>                                AspeedI2CBus),
> @@ -828,6 +830,8 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>   static Property aspeed_i2c_properties[] = {
>       DEFINE_PROP_LINK("dram", AspeedI2CState, dram_mr,
>                        TYPE_MEMORY_REGION, MemoryRegion *),
> +    DEFINE_PROP_UINT32("ctrl-global-rsvd", AspeedI2CState, ctrl_global_rsvd,
> +                       0xfffffffe),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 4b9be09274..3912fcc3ff 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -71,6 +71,8 @@ struct AspeedI2CState {
>       MemoryRegion pool_iomem;
>       uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
>   
> +    uint32_t ctrl_global_rsvd;



> +
>       AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
>       MemoryRegion *dram_mr;
>       AddressSpace dram_as;



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

* Re: [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace
  2022-03-31  4:32 ` [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace Joe Komlodi
@ 2022-04-06 10:05   ` Cédric Le Goater
  0 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-04-06 10:05 UTC (permalink / raw)
  To: Joe Komlodi, qemu-devel
  Cc: peter.maydell, andrew, venture, Troy Lee, joel, Klaus Jensen

Klaus,

On 3/31/22 06:32, Joe Komlodi wrote:
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Change-Id: I566eb09f4b9016e24570572f367627f6594039f5
> ---
>   hw/i2c/aspeed_i2c.c | 3 +++
>   hw/i2c/trace-events | 2 +-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 948d8dc2bb..f179f78ee9 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -305,6 +305,9 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>       bool raise_irq;
>   
>       trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
> +        aspeed_i2c_bus_pkt_mode_en(bus) &&
> +        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
> +                                                               "pktdone|" : "",
>           SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
>           SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
>           SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"


If you resend the I2C slave proposal, can you please add the above flag
in the patch modifying the trace event. Troy and Joe would benefit from it.

The change to register fields can come later.

Thanks,

C.

> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 7d8907c1ee..85e4bddff9 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>   # aspeed_i2c.c
>   
>   aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
> -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
> +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
>   aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"



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

* Re: [RFC PATCH 1/7] hw/registerfields: Add shared fields macros
  2022-03-31  4:32 ` [RFC PATCH 1/7] hw/registerfields: Add shared fields macros Joe Komlodi
@ 2022-04-06 10:26   ` Cédric Le Goater
  0 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-04-06 10:26 UTC (permalink / raw)
  To: Joe Komlodi, qemu-devel; +Cc: andrew, peter.maydell, joel, venture

On 3/31/22 06:32, Joe Komlodi wrote:
> Occasionally a peripheral will have different operating modes, where the
> MMIO layout changes, but some of the register fields have the same offsets
> and behaviors.
> 
> To help support this, we add SHARED_FIELD_XX macros that create SHIFT,
> LENGTH, and MASK macros for the fields that are shared across registers,
> and accessors for these fields.
> 
> An example use may look as follows:
> There is a peripheral with registers REG_MODE1 and REG_MODE2 at
> different addreses, and both have a field FIELD1 initialized by
> SHARED_FIELD().
> 
> Depending on what mode the peripheral is operating in, the user could
> extract FIELD1 via
> SHARED_ARRAY_FIELD_EX32(s->regs, R_REG_MODE1, FIELD1)
> or
> SHARED_ARRAY_FIELD_EX32(s->regs, R_REG_MODE2, FIELD1)
> 
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Change-Id: Id3dc53e7d2f8741c95697cbae69a81bb699fa3cb

I would merge patch 1,3,7 but without the new mode definitions first
and in another header file because we don't need to expose the register
definitions outside the I2C model.

See hw/intc/pnv_xive*regs.h for an example.

Thanks,

C.

> ---
>   include/hw/registerfields.h | 70 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 70 insertions(+)
> 
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> index f2a3c9c41f..cf3bc3a6e3 100644
> --- a/include/hw/registerfields.h
> +++ b/include/hw/registerfields.h
> @@ -108,4 +108,74 @@
>   #define ARRAY_FIELD_DP64(regs, reg, field, val)                           \
>       (regs)[R_ ## reg] = FIELD_DP64((regs)[R_ ## reg], reg, field, val);
>   
> +
> +/*
> + * These macros can be used for defining and extracting fields that have the
> + * same bit position across multiple registers.
> + */
> +
> +/* Define shared SHIFT, LENGTH, and MASK constants */
> +#define SHARED_FIELD(name, shift, length)   \
> +    enum { name ## _ ## SHIFT = (shift)};   \
> +    enum { name ## _ ## LENGTH = (length)}; \
> +    enum { name ## _ ## MASK = MAKE_64BIT_MASK(shift, length)};
> +
> +/* Extract a shared field */
> +#define SHARED_FIELD_EX8(storage, field) \
> +    extract8((storage), field ## _SHIFT, field ## _LENGTH)
> +
> +#define SHARED_FIELD_EX16(storage, field) \
> +    extract16((storage), field ## _SHIFT, field ## _LENGTH)
> +
> +#define SHARED_FIELD_EX32(storage, field) \
> +    extract32((storage), field ## _SHIFT, field ## _LENGTH)
> +
> +#define SHARED_FIELD_EX64(storage, field) \
> +    extract64((storage), field ## _SHIFT, field ## _LENGTH)
> +
> +/* Extract a shared field from a register array */
> +#define SHARED_ARRAY_FIELD_EX32(regs, offset, field) \
> +    SHARED_FIELD_EX32((regs)[(offset)], field)
> +#define SHARED_ARRAY_FIELD_EX64(regs, offset, field) \
> +    SHARED_FIELD_EX64((regs)[(offset)], field)
> +
> +/* Deposit a shared field */
> +#define SHARED_FIELD_DP8(storage, field, val) ({                        \
> +    struct {                                                            \
> +        unsigned int v:field ## _LENGTH;                                \
> +    } _v = { .v = val };                                                \
> +    uint8_t _d;                                                         \
> +    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
> +    _d; })
> +
> +#define SHARED_FIELD_DP16(storage, field, val) ({                       \
> +    struct {                                                            \
> +        unsigned int v:field ## _LENGTH;                                \
> +    } _v = { .v = val };                                                \
> +    uint16_t _d;                                                        \
> +    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
> +    _d; })
> +
> +#define SHARED_FIELD_DP32(storage, field, val) ({                       \
> +    struct {                                                            \
> +        unsigned int v:field ## _LENGTH;                                \
> +    } _v = { .v = val };                                                \
> +    uint32_t _d;                                                        \
> +    _d = deposit32((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
> +    _d; })
> +
> +#define SHARED_FIELD_DP64(storage, field, val) ({                       \
> +    struct {                                                            \
> +        uint64_t v:field ## _LENGTH;                                    \
> +    } _v = { .v = val };                                                \
> +    uint64_t _d;                                                        \
> +    _d = deposit64((storage), field ## _SHIFT, field ## _LENGTH, _v.v); \
> +    _d; })
> +
> +/* Deposit a shared field to a register array */
> +#define SHARED_ARRAY_FIELD_DP32(regs, offset, field, val) \
> +    (regs)[(offset)] = SHARED_FIELD_DP32((regs)[(offset)], field, val);
> +#define SHARED_ARRAY_FIELD_DP64(regs, offset, field, val) \
> +    (regs)[(offset)] = SHARED_FIELD_DP64((regs)[(offset)], field, val);
> +
>   #endif



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

* Re: [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars
  2022-03-31  4:32 ` [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars Joe Komlodi
@ 2022-04-06 10:34   ` Cédric Le Goater
  0 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-04-06 10:34 UTC (permalink / raw)
  To: Joe Komlodi, qemu-devel; +Cc: andrew, peter.maydell, joel, venture

On 3/31/22 06:32, Joe Komlodi wrote:
> Using a register array will allow us to represent old-mode and new-mode
> I2C registers by using the same underlying register array, instead of
> adding an entire new set of variables to represent new mode.

I agree with this change. Please do it first before introducing the
the register field definitions.

Some comments below,

> 
> As part of this, we also do additional cleanup to use ARRAY_FIELD_
> macros instead of FIELD_ macros on registers.
> 
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Change-Id: Ib94996b17c361b8490c042b43c99d8abc69332e3
> ---
>   hw/i2c/aspeed_i2c.c         | 286 +++++++++++++++++-------------------
>   include/hw/i2c/aspeed_i2c.h |  11 +-
>   2 files changed, 133 insertions(+), 164 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index be81a798cc..e938055150 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -133,30 +133,30 @@ REG32(I2CD_DMA_LEN, 0x28) /* DMA Transfer Length < 4KB */
>   
>   static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
>   {
> -    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN);
> +    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN);
>   }
>   
>   static inline bool aspeed_i2c_bus_is_enabled(AspeedI2CBus *bus)
>   {
> -    return FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, MASTER_EN) ||
> -           FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL, SLAVE_EN);
> +    return ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, MASTER_EN) ||
> +           ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL, SLAVE_EN);
>   }
>   
>   static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>   {
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>   
> -    trace_aspeed_i2c_bus_raise_interrupt(bus->intr_status,
> -        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
> -        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
> -        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
> -        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
> -                                                                 : "",
> -        FIELD_EX32(bus->intr_status, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
> -                                                              : "");
> -
> -    bus->intr_status &= bus->intr_ctrl;
> -    if (bus->intr_status) {
> +    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[R_I2CD_INTR_STS],
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_NAK) ? "nak|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, TX_ACK) ? "ack|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) ? "done|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, NORMAL_STOP) ? "normal|"
> +                                                                  : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, ABNORMAL) ? "abnormal"
> +                                                               : "");
> +
> +    bus->regs[R_I2CD_INTR_STS] &= bus->regs[R_I2CD_INTR_CTRL];
> +    if (bus->regs[R_I2CD_INTR_STS]) {
>           bus->controller->intr_status |= 1 << bus->id;
>           qemu_irq_raise(aic->bus_get_irq(bus));
>       }
> @@ -167,46 +167,33 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
>   {
>       AspeedI2CBus *bus = opaque;
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> -    uint64_t value = -1;
> +    uint64_t value = bus->regs[offset / sizeof(*bus->regs)];

please introduce a reg variable.

>   
>       switch (offset) {
>       case A_I2CD_FUN_CTRL:
> -        value = bus->ctrl;
> -        break;
>       case A_I2CD_AC_TIMING1:
> -        value = bus->timing[0];
> -        break;
>       case A_I2CD_AC_TIMING2:
> -        value = bus->timing[1];
> -        break;
>       case A_I2CD_INTR_CTRL:
> -        value = bus->intr_ctrl;
> -        break;
>       case A_I2CD_INTR_STS:
> -        value = bus->intr_status;
> -        break;
>       case A_I2CD_POOL_CTRL:
> -        value = bus->pool_ctrl;
> -        break;
>       case A_I2CD_BYTE_BUF:
> -        value = bus->buf;
> +        /* Value is already set, don't do anything. */
>           break;
>       case A_I2CD_CMD:
> -        value = bus->cmd | (i2c_bus_busy(bus->bus) << 16);
> +        value = FIELD_DP32(value, I2CD_CMD, BUS_BUSY_STS,
> +                           i2c_bus_busy(bus->bus));
>           break;
>       case A_I2CD_DMA_ADDR:
>           if (!aic->has_dma) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
> -            break;
> +            value = -1;
>           }
> -        value = bus->dma_addr;
>           break;
>       case A_I2CD_DMA_LEN:
>           if (!aic->has_dma) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
> -            break;
> +            value = -1;
>           }
> -        value = bus->dma_len;
>           break;
>   
>       default:
> @@ -222,12 +209,12 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
>   
>   static void aspeed_i2c_set_state(AspeedI2CBus *bus, uint8_t state)
>   {
> -    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_STATE, state);
> +    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_STATE, state);
>   }
>   
>   static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>   {
> -    return FIELD_EX32(bus->cmd, I2CD_CMD, TX_STATE);
> +    return ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_STATE);
>   }
>   
>   static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
> @@ -235,16 +222,16 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
>       MemTxResult result;
>       AspeedI2CState *s = bus->controller;
>   
> -    result = address_space_read(&s->dram_as, bus->dma_addr,
> +    result = address_space_read(&s->dram_as, bus->regs[R_I2CD_DMA_ADDR],
>                                   MEMTXATTRS_UNSPECIFIED, data, 1);
>       if (result != MEMTX_OK) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
> -                      __func__, bus->dma_addr);
> +                      __func__, bus->regs[R_I2CD_DMA_ADDR]);
>           return -1;
>       }
>   
> -    bus->dma_addr++;
> -    bus->dma_len--;
> +    bus->regs[R_I2CD_DMA_ADDR]++;
> +    bus->regs[R_I2CD_DMA_LEN]--;
>       return 0;
>   }
>   
> @@ -253,9 +240,9 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>       int ret = -1;
>       int i;
> -    int pool_tx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
> +    int pool_tx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
>           for (i = pool_start; i < pool_tx_count; i++) {
>               uint8_t *pool_base = aic->bus_pool_base(bus);
>   
> @@ -266,21 +253,23 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>                   break;
>               }
>           }
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_BUFF_EN, 0);
> -    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
> -        while (bus->dma_len) {
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_BUFF_EN, 0);
> +    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
> +        while (bus->regs[R_I2CD_DMA_LEN]) {
>               uint8_t data;
>               aspeed_i2c_dma_read(bus, &data);
> -            trace_aspeed_i2c_bus_send("DMA", bus->dma_len, bus->dma_len, data);
> +            trace_aspeed_i2c_bus_send("DMA", bus->regs[R_I2CD_DMA_LEN],
> +                                      bus->regs[R_I2CD_DMA_LEN], data);
>               ret = i2c_send(bus->bus, data);
>               if (ret) {
>                   break;
>               }
>           }
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, TX_DMA_EN, 0);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, TX_DMA_EN, 0);
>       } else {
> -        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1, bus->buf);
> -        ret = i2c_send(bus->bus, bus->buf);
> +        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
> +                                  bus->regs[R_I2CD_BYTE_BUF]);
> +        ret = i2c_send(bus->bus, bus->regs[R_I2CD_BYTE_BUF]);
>       }
>   
>       return ret;
> @@ -292,9 +281,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
>       uint8_t data;
>       int i;
> -    int pool_rx_count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT);
> +    int pool_rx_count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, RX_COUNT);
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
>           uint8_t *pool_base = aic->bus_pool_base(bus);
>   
>           for (i = 0; i < pool_rx_count; i++) {
> @@ -304,32 +293,33 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>           }
>   
>           /* Update RX count */
> -        bus->pool_ctrl = FIELD_DP32(bus->pool_ctrl, I2CD_POOL_CTRL, RX_COUNT,
> -                                    i & 0xff);
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_BUFF_EN, 0);
> -    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_POOL_CTRL, RX_COUNT, i & 0xff);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_BUFF_EN, 0);
> +    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
>           uint8_t data;
>   
> -        while (bus->dma_len) {
> +        while (bus->regs[R_I2CD_DMA_LEN]) {
>               MemTxResult result;
>   
>               data = i2c_recv(bus->bus);
> -            trace_aspeed_i2c_bus_recv("DMA", bus->dma_len, bus->dma_len, data);
> -            result = address_space_write(&s->dram_as, bus->dma_addr,
> +            trace_aspeed_i2c_bus_recv("DMA", bus->regs[R_I2CD_DMA_LEN],
> +                                      bus->regs[R_I2CD_DMA_LEN], data);
> +            result = address_space_write(&s->dram_as,
> +                                         bus->regs[R_I2CD_DMA_ADDR],
>                                            MEMTXATTRS_UNSPECIFIED, &data, 1);
>               if (result != MEMTX_OK) {
>                   qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
> -                              __func__, bus->dma_addr);
> +                              __func__, bus->regs[R_I2CD_DMA_ADDR]);
>                   return;
>               }
> -            bus->dma_addr++;
> -            bus->dma_len--;
> +            bus->regs[R_I2CD_DMA_ADDR]++;
> +            bus->regs[R_I2CD_DMA_LEN]--;
>           }
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, RX_DMA_EN, 0);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, RX_DMA_EN, 0);
>       } else {
>           data = i2c_recv(bus->bus);
> -        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->buf);
> -        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, RX_BUF, data);
> +        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->regs[R_I2CD_BYTE_BUF]);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, RX_BUF, data);
>       }
>   }
>   
> @@ -337,12 +327,12 @@ static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>   {
>       aspeed_i2c_set_state(bus, I2CD_MRXD);
>       aspeed_i2c_bus_recv(bus);
> -    bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS, RX_DONE, 1);
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) {
> +    ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, RX_DONE, 1);
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) {
>           i2c_nack(bus->bus);
>       }
> -    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_RX_CMD, 0);
> -    bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST, 0);
> +    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_RX_CMD, 0);
> +    ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST, 0);
>       aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>   }
>   
> @@ -350,17 +340,17 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
>   {
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
>           uint8_t *pool_base = aic->bus_pool_base(bus);
>   
>           return pool_base[0];
> -    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
> +    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
>           uint8_t data;
>   
>           aspeed_i2c_dma_read(bus, &data);
>           return data;
>       } else {
> -        return bus->buf;
> +        return bus->regs[R_I2CD_BYTE_BUF];
>       }
>   }
>   
> @@ -368,10 +358,10 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
>   {
>       AspeedI2CState *s = bus->controller;
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
> -    bool dma_en = FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)  ||
> -                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)  ||
> -                  FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ||
> -                  FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN);
> +    bool dma_en = ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)  ||
> +                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)  ||
> +                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ||
> +                  ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN);
>       if (!aic->check_sram) {
>           return true;
>       }
> @@ -392,26 +382,27 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>   {
>       g_autofree char *cmd_flags = NULL;
>       uint32_t count;
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN)) {
> -        count = FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT);
> -    } else if (FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN)) {
> -        count = bus->dma_len;
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN)) {
> +        count = ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT);
> +    } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN)) {
> +        count = bus->regs[R_I2CD_DMA_LEN];
>       } else { /* BYTE mode */
>           count = 1;
>       }
>   
>       cmd_flags = g_strdup_printf("%s%s%s%s%s%s%s%s%s",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD) ? "start|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
> -             FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
> -
> -    trace_aspeed_i2c_bus_cmd(bus->cmd, cmd_flags, count, bus->intr_status);
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD) ? "start|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_DMA_EN) ? "rxdma|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN) ? "txdma|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, RX_BUFF_EN) ? "rxbuf|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN) ? "txbuf|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD) ? "tx|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ? "rx|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST) ? "last|" : "",
> +          ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD) ? "stop" : "");
> +
> +    trace_aspeed_i2c_bus_cmd(bus->regs[R_I2CD_CMD], cmd_flags, count,
> +                             bus->regs[R_I2CD_INTR_STS]);
>   }
>   
>   /*
> @@ -422,8 +413,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   {
>       uint8_t pool_start = 0;
>   
> -    bus->cmd &= ~0xFFFF;
> -    bus->cmd |= value & 0xFFFF;
> +    bus->regs[R_I2CD_CMD] &= ~0xFFFF;
> +    bus->regs[R_I2CD_CMD] |= value & 0xFFFF;
>   
>       if (!aspeed_i2c_check_sram(bus)) {
>           return;
> @@ -433,7 +424,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>           aspeed_i2c_bus_cmd_dump(bus);
>       }
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_START_CMD)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_START_CMD)) {
>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>               I2CD_MSTARTR : I2CD_MSTART;
>           uint8_t addr;
> @@ -444,23 +435,21 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   
>           if (i2c_start_transfer(bus->bus, extract32(addr, 1, 7),
>                                  extract32(addr, 0, 1))) {
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          TX_NAK, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
>           } else {
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          TX_ACK, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
>           }
>   
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_START_CMD, 0);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_START_CMD, 0);
>   
>           /*
>            * The START command is also a TX command, as the slave
>            * address is sent on the bus. Drop the TX flag if nothing
>            * else needs to be sent in this sequence.
>            */
> -        if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_BUFF_EN)) {
> -            if (FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, TX_COUNT) == 1) {
> -                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
> +        if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_BUFF_EN)) {
> +            if (ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, TX_COUNT) == 1) {
> +                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
>               } else {
>                   /*
>                    * Increase the start index in the TX pool buffer to
> @@ -468,12 +457,12 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>                    */
>                   pool_start++;
>               }
> -        } else if (FIELD_EX32(bus->cmd, I2CD_CMD, TX_DMA_EN)) {
> -            if (bus->dma_len == 0) {
> -                bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
> +        } else if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, TX_DMA_EN)) {
> +            if (bus->regs[R_I2CD_DMA_LEN] == 0) {
> +                ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
>               }
>           } else {
> -            bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
>           }
>   
>           /* No slave found */
> @@ -483,38 +472,34 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>           aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>       }
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_TX_CMD)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_TX_CMD)) {
>           aspeed_i2c_set_state(bus, I2CD_MTXD);
>           if (aspeed_i2c_bus_send(bus, pool_start)) {
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          TX_NAK, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_NAK, 1);
>               i2c_end_transfer(bus->bus);
>           } else {
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          TX_ACK, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, TX_ACK, 1);
>           }
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_TX_CMD, 0);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_TX_CMD, 0);
>           aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>       }
>   
> -    if ((FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
> -         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST)) &&
> -        !FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE)) {
> +    if ((ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
> +         ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST)) &&
> +        !ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE)) {
>           aspeed_i2c_handle_rx_cmd(bus);
>       }
>   
> -    if (FIELD_EX32(bus->cmd, I2CD_CMD, M_STOP_CMD)) {
> +    if (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_STOP_CMD)) {
>           if (!(aspeed_i2c_get_state(bus) & I2CD_MACTIVE)) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: abnormal stop\n", __func__);
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          ABNORMAL, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, ABNORMAL, 1);
>           } else {
>               aspeed_i2c_set_state(bus, I2CD_MSTOP);
>               i2c_end_transfer(bus->bus);
> -            bus->intr_status = FIELD_DP32(bus->intr_status, I2CD_INTR_STS,
> -                                          NORMAL_STOP, 1);
> +            ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, NORMAL_STOP, 1);
>           }
> -        bus->cmd = FIELD_DP32(bus->cmd, I2CD_CMD, M_STOP_CMD, 0);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_CMD, M_STOP_CMD, 0);
>           aspeed_i2c_set_state(bus, I2CD_IDLE);
>       }
>   }
> @@ -535,27 +520,27 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>                             __func__);
>               break;
>           }
> -        bus->ctrl = value & 0x0071C3FF;
> +        bus->regs[R_I2CD_FUN_CTRL] = value & 0x0071C3FF;
>           break;
>       case A_I2CD_AC_TIMING1:
> -        bus->timing[0] = value & 0xFFFFF0F;
> +        bus->regs[R_I2CD_AC_TIMING1] = value & 0xFFFFF0F;
>           break;
>       case A_I2CD_AC_TIMING2:
> -        bus->timing[1] = value & 0x7;
> +        bus->regs[R_I2CD_AC_TIMING2] = value & 0x7;
>           break;
>       case A_I2CD_INTR_CTRL:
> -        bus->intr_ctrl = value & 0x7FFF;
> +        bus->regs[R_I2CD_INTR_CTRL] = value & 0x7FFF;
>           break;
>       case A_I2CD_INTR_STS:
> -        handle_rx = FIELD_EX32(bus->intr_status, I2CD_INTR_STS, RX_DONE) &&
> +        handle_rx = ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, RX_DONE) &&
>                       FIELD_EX32(value, I2CD_INTR_STS, RX_DONE);
> -        bus->intr_status &= ~(value & 0x7FFF);
> -        if (!bus->intr_status) {
> +        bus->regs[R_I2CD_INTR_STS] &= ~(value & 0x7FFF);
> +        if (!bus->regs[R_I2CD_INTR_STS]) {
>               bus->controller->intr_status &= ~(1 << bus->id);
>               qemu_irq_lower(aic->bus_get_irq(bus));
>           }
> -        if (handle_rx && (FIELD_EX32(bus->cmd, I2CD_CMD, M_RX_CMD) ||
> -                         FIELD_EX32(bus->cmd, I2CD_CMD, M_S_RX_CMD_LAST))) {
> +        if (handle_rx && (ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_RX_CMD) ||
> +                      ARRAY_FIELD_EX32(bus->regs, I2CD_CMD, M_S_RX_CMD_LAST))) {
>               aspeed_i2c_handle_rx_cmd(bus);
>               aspeed_i2c_bus_raise_interrupt(bus);
>           }
> @@ -565,12 +550,12 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>                         __func__);
>           break;
>       case A_I2CD_POOL_CTRL:
> -        bus->pool_ctrl &= ~0xffffff;
> -        bus->pool_ctrl |= (value & 0xffffff);
> +        bus->regs[R_I2CD_POOL_CTRL] &= ~0xffffff;
> +        bus->regs[R_I2CD_POOL_CTRL] |= (value & 0xffffff);
>           break;
>   
>       case A_I2CD_BYTE_BUF:
> -        bus->buf = FIELD_DP32(bus->buf, I2CD_BYTE_BUF, TX_BUF, value);
> +        ARRAY_FIELD_DP32(bus->regs, I2CD_BYTE_BUF, TX_BUF, value);
>           break;
>       case A_I2CD_CMD:
>           if (!aspeed_i2c_bus_is_enabled(bus)) {
> @@ -599,7 +584,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>               break;
>           }
>   
> -        bus->dma_addr = value & 0x3ffffffc;
> +        bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
>           break;
>   
>       case A_I2CD_DMA_LEN:
> @@ -608,8 +593,8 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>               break;
>           }
>   
> -        bus->dma_len = value & 0xfff;
> -        if (!bus->dma_len) {
> +        bus->regs[R_I2CD_DMA_LEN] = value & 0xfff;
> +        if (!bus->regs[R_I2CD_DMA_LEN]) {
>               qemu_log_mask(LOG_UNIMP, "%s: invalid DMA length\n",  __func__);
>           }
>           break;
> @@ -706,19 +691,10 @@ static const MemoryRegionOps aspeed_i2c_pool_ops = {
>   
>   static const VMStateDescription aspeed_i2c_bus_vmstate = {
>       .name = TYPE_ASPEED_I2C,
> -    .version_id = 3,
> -    .minimum_version_id = 3,
> +    .version_id = 4,
> +    .minimum_version_id = 4,
>       .fields = (VMStateField[]) {
> -        VMSTATE_UINT8(id, AspeedI2CBus),
> -        VMSTATE_UINT32(ctrl, AspeedI2CBus),
> -        VMSTATE_UINT32_ARRAY(timing, AspeedI2CBus, 2),
> -        VMSTATE_UINT32(intr_ctrl, AspeedI2CBus),
> -        VMSTATE_UINT32(intr_status, AspeedI2CBus),
> -        VMSTATE_UINT32(cmd, AspeedI2CBus),
> -        VMSTATE_UINT32(buf, AspeedI2CBus),
> -        VMSTATE_UINT32(pool_ctrl, AspeedI2CBus),
> -        VMSTATE_UINT32(dma_addr, AspeedI2CBus),
> -        VMSTATE_UINT32(dma_len, AspeedI2CBus),
> +        VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_OLD_NUM_REG),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> @@ -858,12 +834,12 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
>   {
>       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
>   
> -    s->intr_ctrl = 0;
> -    s->intr_status = 0;
> -    s->cmd = 0;
> -    s->buf = 0;
> -    s->dma_addr = 0;
> -    s->dma_len = 0;
> +    s->regs[R_I2CD_INTR_CTRL] = 0;
> +    s->regs[R_I2CD_INTR_STS] = 0;
> +    s->regs[R_I2CD_CMD] = 0;
> +    s->regs[R_I2CD_BYTE_BUF] = 0;
> +    s->regs[R_I2CD_DMA_ADDR] = 0;
> +    s->regs[R_I2CD_DMA_LEN] = 0;

memset() ?

>       i2c_end_transfer(s->bus);
>   }
>   
> @@ -921,10 +897,10 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
>   static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
>   {
>       uint8_t *pool_page =
> -        &bus->controller->pool[FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL,
> -                                          POOL_PAGE_SEL) * 0x100];
> +        &bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
> +                                                POOL_PAGE_SEL) * 0x100];
>   
> -    return &pool_page[FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, OFFSET)];
> +    return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
>   }
>   
>   static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 3912fcc3ff..e9f83a9540 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -33,6 +33,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   
>   #define ASPEED_I2C_NR_BUSSES 16
>   #define ASPEED_I2C_MAX_POOL_SIZE 0x800
> +#define ASPEED_I2C_OLD_NUM_REG 11

Just use a number big enough for both modes.

Thanks,

C.

>   
>   struct AspeedI2CState;
>   
> @@ -49,15 +50,7 @@ struct AspeedI2CBus {
>       uint8_t id;
>       qemu_irq irq;
>   
> -    uint32_t ctrl;
> -    uint32_t timing[2];
> -    uint32_t intr_ctrl;
> -    uint32_t intr_status;
> -    uint32_t cmd;
> -    uint32_t buf;
> -    uint32_t pool_ctrl;
> -    uint32_t dma_addr;
> -    uint32_t dma_len;
> +    uint32_t regs[ASPEED_I2C_OLD_NUM_REG];
>   };
>   
>   struct AspeedI2CState {



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

* Re: [RFC PATCH 0/7] aspeed: i2c: Add new mode support
  2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
                   ` (6 preceding siblings ...)
  2022-03-31  4:32 ` [RFC PATCH 7/7] aspeed: i2c: Move regs and helpers to header file Joe Komlodi
@ 2022-04-06 10:36 ` Cédric Le Goater
  7 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-04-06 10:36 UTC (permalink / raw)
  To: Joe Komlodi, qemu-devel; +Cc: andrew, peter.maydell, joel, venture

Hello Joe,

On 3/31/22 06:32, Joe Komlodi wrote:
> Hi all,
> 
> This series migrates the Aspeed I2C controller to use the register API,

I like the idea but it's an earthquake in the I2C model and they are other
patchsets on the way. Let's wait a bit before changing it all.

> and then adds new mode support.
>
> New mode has some behavior changes and a register layout change compared
> to old mode.

Troy sent a similar proposal. Did you have a look ?

   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=291804

Thanks,

C.


> The series starts by adding "shared field" macros to help simplify logic
> when adding new mode.
> Generally, the macros are to be used in cases where register locations
> change depending on what mode a peripheral is operating in, but
> the fields in those registers have the same offsets.
> Specifically, this happens very frequently with new and old I2C modes on
> Aspeed I2C.
> 
> After that we migrate over to the register API and then add new mode
> support.
> 
> Thanks!
> Joe
> 
> Joe Komlodi (7):
>    hw/registerfields: Add shared fields macros
>    aspeed: i2c: Add ctrl_global_rsvd property
>    aspeed: i2c: Migrate to registerfields API
>    aspeed: i2c: Use reg array instead of individual vars
>    aspeed: i2c: Add new mode support
>    aspeed: i2c: Add PKT_DONE IRQ to trace
>    aspeed: i2c: Move regs and helpers to header file
> 
>   hw/arm/aspeed_ast2600.c     |   2 +
>   hw/i2c/aspeed_i2c.c         | 792 ++++++++++++++++++++++--------------
>   hw/i2c/trace-events         |   2 +-
>   include/hw/i2c/aspeed_i2c.h | 282 ++++++++++++-
>   include/hw/registerfields.h |  70 ++++
>   5 files changed, 828 insertions(+), 320 deletions(-)
> 



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

end of thread, other threads:[~2022-04-06 10:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-31  4:32 [RFC PATCH 0/7] aspeed: i2c: Add new mode support Joe Komlodi
2022-03-31  4:32 ` [RFC PATCH 1/7] hw/registerfields: Add shared fields macros Joe Komlodi
2022-04-06 10:26   ` Cédric Le Goater
2022-03-31  4:32 ` [RFC PATCH 2/7] aspeed: i2c: Add ctrl_global_rsvd property Joe Komlodi
2022-04-06  9:57   ` Cédric Le Goater
2022-03-31  4:32 ` [RFC PATCH 3/7] aspeed: i2c: Migrate to registerfields API Joe Komlodi
2022-03-31  4:32 ` [RFC PATCH 4/7] aspeed: i2c: Use reg array instead of individual vars Joe Komlodi
2022-04-06 10:34   ` Cédric Le Goater
2022-03-31  4:32 ` [RFC PATCH 5/7] aspeed: i2c: Add new mode support Joe Komlodi
2022-03-31  4:32 ` [RFC PATCH 6/7] aspeed: i2c: Add PKT_DONE IRQ to trace Joe Komlodi
2022-04-06 10:05   ` Cédric Le Goater
2022-03-31  4:32 ` [RFC PATCH 7/7] aspeed: i2c: Move regs and helpers to header file Joe Komlodi
2022-04-06 10:36 ` [RFC PATCH 0/7] aspeed: i2c: Add new mode support Cédric Le Goater

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).