[PATCH v2 1/3] hw: allwinner-i2c: Make the trace message more readable

[PATCH v2 1/3] hw: allwinner-i2c: Make the trace message more readable

[qianfanguijin]

From: qianfan Zhao <qianfanguijin@163.com>

Next is an example when read/write trace enabled:

allwinner_i2c_write write  XADDR(0x04): 0x00
allwinner_i2c_write write   CNTR(0x0c): 0x50 M_STP BUS_EN
allwinner_i2c_write write   CNTR(0x0c): 0xe4 A_ACK M_STA BUS_EN INT_EN
allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
allwinner_i2c_read  read    STAT(0x10): 0x08 STAT_M_STA_TX

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
 hw/i2c/allwinner-i2c.c | 110 ++++++++++++++++++++++++++++++++++++++++-
 hw/i2c/trace-events    |   5 +-
 2 files changed, 110 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/allwinner-i2c.c b/hw/i2c/allwinner-i2c.c
index a435965836..fa650e7e02 100644
--- a/hw/i2c/allwinner-i2c.c
+++ b/hw/i2c/allwinner-i2c.c
@@ -129,6 +129,39 @@ enum {
     STAT_IDLE = 0x1f
 } TWI_STAT_STA;
 
+#define TWI_STAT_STA_DESC(sta)  [sta] = #sta
+static const char *twi_stat_sta_descriptors[] = {
+    TWI_STAT_STA_DESC(STAT_BUS_ERROR),
+    TWI_STAT_STA_DESC(STAT_M_STA_TX),
+    TWI_STAT_STA_DESC(STAT_M_RSTA_TX),
+    TWI_STAT_STA_DESC(STAT_M_ADDR_WR_ACK),
+    TWI_STAT_STA_DESC(STAT_M_ADDR_WR_NACK),
+    TWI_STAT_STA_DESC(STAT_M_DATA_TX_ACK),
+    TWI_STAT_STA_DESC(STAT_M_DATA_TX_NACK),
+    TWI_STAT_STA_DESC(STAT_M_ARB_LOST),
+    TWI_STAT_STA_DESC(STAT_M_ADDR_RD_ACK),
+    TWI_STAT_STA_DESC(STAT_M_ADDR_RD_NACK),
+    TWI_STAT_STA_DESC(STAT_M_DATA_RX_ACK),
+    TWI_STAT_STA_DESC(STAT_M_DATA_RX_NACK),
+    TWI_STAT_STA_DESC(STAT_S_ADDR_WR_ACK),
+    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_AW_ACK),
+    TWI_STAT_STA_DESC(STAT_S_GCA_ACK),
+    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_GCA_ACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_RX_SA_ACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_RX_SA_NACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_RX_GCA_ACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_RX_GCA_NACK),
+    TWI_STAT_STA_DESC(STAT_S_STP_RSTA),
+    TWI_STAT_STA_DESC(STAT_S_ADDR_RD_ACK),
+    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_AR_ACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_TX_ACK),
+    TWI_STAT_STA_DESC(STAT_S_DATA_TX_NACK),
+    TWI_STAT_STA_DESC(STAT_S_LB_TX_ACK),
+    TWI_STAT_STA_DESC(STAT_M_2ND_ADDR_WR_ACK),
+    TWI_STAT_STA_DESC(STAT_M_2ND_ADDR_WR_NACK),
+    TWI_STAT_STA_DESC(STAT_IDLE),
+};
+
 static const char *allwinner_i2c_get_regname(unsigned offset)
 {
     switch (offset) {
@@ -155,6 +188,79 @@ static const char *allwinner_i2c_get_regname(unsigned offset)
     }
 }
 
+static const char *twi_cntr_reg_bits[] = {
+    [2] = "A_ACK",
+    [3] = "INT_FLAG",
+    [4] = "M_STP",
+    [5] = "M_STA",
+    [6] = "BUS_EN",
+    [7] = "INT_EN",
+};
+
+static const char *twi_line_ctrl_reg_bits[] = {
+    [5] = "SCL_STATE",
+    [4] = "SDA_STATE",
+    [3] = "SCL_CTL",
+    [2] = "SCL_CTL_EN",
+    [1] = "SDA_CTL",
+    [0] = "SDA_CTL_EN",
+};
+
+static void make_reg_value_bit_descriptors(char *s, size_t sz, uint8_t value,
+                                           const char **desc_arrays,
+                                           size_t array_size)
+{
+    unsigned i = 0;
+
+    for (; i < array_size; i++) {
+        if ((value & (1 << i)) && desc_arrays[i]) {
+            strncat(s, desc_arrays[i], sz - 1);
+            strncat(s, " ", sz - 1);
+        }
+    }
+}
+
+static void make_reg_value_descriptors(char *s, size_t sz, uint8_t addr,
+                                       uint8_t value)
+{
+    switch (addr) {
+    case TWI_CNTR_REG:
+        make_reg_value_bit_descriptors(s, sz, value, twi_cntr_reg_bits,
+                                       ARRAY_SIZE(twi_cntr_reg_bits));
+        break;
+    case TWI_LCR_REG:
+        make_reg_value_bit_descriptors(s, sz, value, twi_line_ctrl_reg_bits,
+                                       ARRAY_SIZE(twi_line_ctrl_reg_bits));
+        break;
+    case TWI_STAT_REG:
+        if (STAT_TO_STA(value) <= STAT_IDLE)
+            strncat(s, twi_stat_sta_descriptors[STAT_TO_STA(value)], sz - 1);
+        break;
+    }
+}
+
+static void allwinner_i2c_trace_read(uint8_t addr, uint8_t value)
+{
+    char desc[256] = { 0 };
+
+    if (trace_event_get_state_backends(TRACE_ALLWINNER_I2C_READ)) {
+       make_reg_value_descriptors(desc, sizeof(desc), addr, value);
+       trace_allwinner_i2c_read(allwinner_i2c_get_regname(addr),
+                                addr, value, desc);
+    }
+}
+
+static void allwinner_i2c_trace_write(uint8_t addr, uint8_t value)
+{
+    char desc[256] = { 0 };
+
+    if (trace_event_get_state_backends(TRACE_ALLWINNER_I2C_WRITE)) {
+        make_reg_value_descriptors(desc, sizeof(desc), addr, value);
+        trace_allwinner_i2c_write(allwinner_i2c_get_regname(addr),
+                                  addr, value, desc);
+    }
+}
+
 static inline bool allwinner_i2c_is_reset(AWI2CState *s)
 {
     return s->srst & TWI_SRST_MASK;
@@ -271,7 +377,7 @@ static uint64_t allwinner_i2c_read(void *opaque, hwaddr offset,
         break;
     }
 
-    trace_allwinner_i2c_read(allwinner_i2c_get_regname(offset), offset, value);
+    allwinner_i2c_trace_read((uint8_t)offset, (uint8_t)value);
 
     return (uint64_t)value;
 }
@@ -283,7 +389,7 @@ static void allwinner_i2c_write(void *opaque, hwaddr offset,
 
     value &= 0xff;
 
-    trace_allwinner_i2c_write(allwinner_i2c_get_regname(offset), offset, value);
+    allwinner_i2c_trace_write((uint8_t)offset, (uint8_t)value);
 
     switch (offset) {
     case TWI_ADDR_REG:
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 8e88aa24c1..963946bfdb 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -16,9 +16,8 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
 i2c_ack(void) ""
 
 # allwinner_i2c.c
-
-allwinner_i2c_read(const char* reg_name, uint64_t offset, uint64_t value) "read %s [0x%" PRIx64 "]: -> 0x%" PRIx64
-allwinner_i2c_write(const char* reg_name, uint64_t offset, uint64_t value) "write %s [0x%" PRIx64 "]: <- 0x%" PRIx64
+allwinner_i2c_read(const char *regname, uint8_t addr, uint8_t value, const char *desc)  " read  %6s(0x%02x): 0x%02x %s"
+allwinner_i2c_write(const char *regname, uint8_t addr, uint8_t value, const char *desc) "write %6s(0x%02x): 0x%02x %s"
 
 # aspeed_i2c.c
 
-- 
2.25.1

[PATCH v2 2/3] hw: allwinner-i2c: Fix TWI_CNTR_INT_FLAG on SUN6i SoCs

[qianfanguijin]

From: qianfan Zhao <qianfanguijin@163.com>

TWI_CNTR_INT_FLAG is W1C(write 1 to clear and write 0 has non-effect)
register on SUN6i based SoCs, we should lower interrupt when the guest
set this bit.

The linux kernel will hang in irq handler(mv64xxx_i2c_intr) if no
device connected on the i2c bus, next is the trace log:

allwinner_i2c_write write   CNTR(0x0c): 0xc4 A_ACK BUS_EN INT_EN
allwinner_i2c_write write   CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
allwinner_i2c_read  read    STAT(0x10): 0x20 STAT_M_ADDR_WR_NACK
allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
...

Fix it.

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
 hw/i2c/allwinner-i2c.c         | 26 ++++++++++++++++++++++++--
 include/hw/i2c/allwinner-i2c.h |  6 ++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/allwinner-i2c.c b/hw/i2c/allwinner-i2c.c
index fa650e7e02..819638d740 100644
--- a/hw/i2c/allwinner-i2c.c
+++ b/hw/i2c/allwinner-i2c.c
@@ -463,10 +463,16 @@ static void allwinner_i2c_write(void *opaque, hwaddr offset,
                 s->stat = STAT_FROM_STA(STAT_IDLE);
                 s->cntr &= ~TWI_CNTR_M_STP;
             }
-            if ((s->cntr & TWI_CNTR_INT_FLAG) == 0) {
-                /* Interrupt flag cleared */
+
+            if (!s->irq_clear_inverted && !(s->cntr & TWI_CNTR_INT_FLAG)) {
+                /* Write 0 to clear this flag */
+                qemu_irq_lower(s->irq);
+            } else if (s->irq_clear_inverted && (s->cntr & TWI_CNTR_INT_FLAG)) {
+                /* Write 1 to clear this flag */
+                s->cntr &= ~TWI_CNTR_INT_FLAG;
                 qemu_irq_lower(s->irq);
             }
+
             if ((s->cntr & TWI_CNTR_A_ACK) == 0) {
                 if (STAT_TO_STA(s->stat) == STAT_M_DATA_RX_ACK) {
                     s->stat = STAT_FROM_STA(STAT_M_DATA_RX_NACK);
@@ -557,9 +563,25 @@ static const TypeInfo allwinner_i2c_type_info = {
     .class_init = allwinner_i2c_class_init,
 };
 
+static void allwinner_i2c_sun6i_init(Object *obj)
+{
+    AWI2CState *s = AW_I2C(obj);
+
+    s->irq_clear_inverted = true;
+}
+
+static const TypeInfo allwinner_i2c_sun6i_type_info = {
+    .name = TYPE_AW_I2C_SUN6I,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AWI2CState),
+    .instance_init = allwinner_i2c_sun6i_init,
+    .class_init = allwinner_i2c_class_init,
+};
+
 static void allwinner_i2c_register_types(void)
 {
     type_register_static(&allwinner_i2c_type_info);
+    type_register_static(&allwinner_i2c_sun6i_type_info);
 }
 
 type_init(allwinner_i2c_register_types)
diff --git a/include/hw/i2c/allwinner-i2c.h b/include/hw/i2c/allwinner-i2c.h
index 4f378b86ba..0e325d265e 100644
--- a/include/hw/i2c/allwinner-i2c.h
+++ b/include/hw/i2c/allwinner-i2c.h
@@ -28,6 +28,10 @@
 #include "qom/object.h"
 
 #define TYPE_AW_I2C "allwinner.i2c"
+
+/** Allwinner I2C sun6i family and newer (A31, H2+, H3, etc) */
+#define TYPE_AW_I2C_SUN6I    TYPE_AW_I2C "-sun6i"
+
 OBJECT_DECLARE_SIMPLE_TYPE(AWI2CState, AW_I2C)
 
 #define AW_I2C_MEM_SIZE         0x24
@@ -50,6 +54,8 @@ struct AWI2CState {
     uint8_t srst;
     uint8_t efr;
     uint8_t lcr;
+
+    bool irq_clear_inverted;
 };
 
 #endif /* ALLWINNER_I2C_H */
-- 
2.25.1

[PATCH v2 3/3] hw: arm: allwinner-h3: Fix and complete H3 i2c devices

[qianfanguijin]

From: qianfan Zhao <qianfanguijin@163.com>

Allwinner h3 has 4 twi(i2c) devices named twi0, twi1, twi2 and r_twi.
The registers are compatible with TYPE_AW_I2C_SUN6I, write 1 to clear
control register's INT_FLAG bit.

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
 hw/arm/allwinner-h3.c         | 29 +++++++++++++++++++++++++----
 include/hw/arm/allwinner-h3.h |  6 ++++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index bfce3c8d92..69d0ad6f50 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -54,6 +54,8 @@ const hwaddr allwinner_h3_memmap[] = {
     [AW_H3_DEV_UART2]      = 0x01c28800,
     [AW_H3_DEV_UART3]      = 0x01c28c00,
     [AW_H3_DEV_TWI0]       = 0x01c2ac00,
+    [AW_H3_DEV_TWI1]       = 0x01c2b000,
+    [AW_H3_DEV_TWI2]       = 0x01c2b400,
     [AW_H3_DEV_EMAC]       = 0x01c30000,
     [AW_H3_DEV_DRAMCOM]    = 0x01c62000,
     [AW_H3_DEV_DRAMCTL]    = 0x01c63000,
@@ -64,6 +66,7 @@ const hwaddr allwinner_h3_memmap[] = {
     [AW_H3_DEV_GIC_VCPU]   = 0x01c86000,
     [AW_H3_DEV_RTC]        = 0x01f00000,
     [AW_H3_DEV_CPUCFG]     = 0x01f01c00,
+    [AW_H3_DEV_R_TWI]      = 0x01f02400,
     [AW_H3_DEV_SDRAM]      = 0x40000000
 };
 
@@ -107,8 +110,6 @@ struct AwH3Unimplemented {
     { "uart1",     0x01c28400, 1 * KiB },
     { "uart2",     0x01c28800, 1 * KiB },
     { "uart3",     0x01c28c00, 1 * KiB },
-    { "twi1",      0x01c2b000, 1 * KiB },
-    { "twi2",      0x01c2b400, 1 * KiB },
     { "scr",       0x01c2c400, 1 * KiB },
     { "gpu",       0x01c40000, 64 * KiB },
     { "hstmr",     0x01c60000, 4 * KiB },
@@ -123,7 +124,6 @@ struct AwH3Unimplemented {
     { "r_prcm",    0x01f01400, 1 * KiB },
     { "r_twd",     0x01f01800, 1 * KiB },
     { "r_cir-rx",  0x01f02000, 1 * KiB },
-    { "r_twi",     0x01f02400, 1 * KiB },
     { "r_uart",    0x01f02800, 1 * KiB },
     { "r_pio",     0x01f02c00, 1 * KiB },
     { "r_pwm",     0x01f03800, 1 * KiB },
@@ -151,8 +151,11 @@ enum {
     AW_H3_GIC_SPI_UART2     =  2,
     AW_H3_GIC_SPI_UART3     =  3,
     AW_H3_GIC_SPI_TWI0      =  6,
+    AW_H3_GIC_SPI_TWI1      =  7,
+    AW_H3_GIC_SPI_TWI2      =  8,
     AW_H3_GIC_SPI_TIMER0    = 18,
     AW_H3_GIC_SPI_TIMER1    = 19,
+    AW_H3_GIC_SPI_R_TWI     = 44,
     AW_H3_GIC_SPI_MMC0      = 60,
     AW_H3_GIC_SPI_EHCI0     = 72,
     AW_H3_GIC_SPI_OHCI0     = 73,
@@ -227,7 +230,10 @@ static void allwinner_h3_init(Object *obj)
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_AW_RTC_SUN6I);
 
-    object_initialize_child(obj, "twi0", &s->i2c0, TYPE_AW_I2C);
+    object_initialize_child(obj, "twi0",  &s->i2c0,  TYPE_AW_I2C_SUN6I);
+    object_initialize_child(obj, "twi1",  &s->i2c1,  TYPE_AW_I2C_SUN6I);
+    object_initialize_child(obj, "twi2",  &s->i2c2,  TYPE_AW_I2C_SUN6I);
+    object_initialize_child(obj, "r_twi", &s->r_twi, TYPE_AW_I2C_SUN6I);
 }
 
 static void allwinner_h3_realize(DeviceState *dev, Error **errp)
@@ -432,6 +438,21 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c0), 0,
                        qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI0));
 
+    sysbus_realize(SYS_BUS_DEVICE(&s->i2c1), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c1), 0, s->memmap[AW_H3_DEV_TWI1]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c1), 0,
+                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI1));
+
+    sysbus_realize(SYS_BUS_DEVICE(&s->i2c2), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c2), 0, s->memmap[AW_H3_DEV_TWI2]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c2), 0,
+                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI2));
+
+    sysbus_realize(SYS_BUS_DEVICE(&s->r_twi), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->r_twi), 0, s->memmap[AW_H3_DEV_R_TWI]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->r_twi), 0,
+                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_R_TWI));
+
     /* Unimplemented devices */
     for (i = 0; i < ARRAY_SIZE(unimplemented); i++) {
         create_unimplemented_device(unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index 1d7ce20589..59e0f822d2 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -84,6 +84,8 @@ enum {
     AW_H3_DEV_UART3,
     AW_H3_DEV_EMAC,
     AW_H3_DEV_TWI0,
+    AW_H3_DEV_TWI1,
+    AW_H3_DEV_TWI2,
     AW_H3_DEV_DRAMCOM,
     AW_H3_DEV_DRAMCTL,
     AW_H3_DEV_DRAMPHY,
@@ -93,6 +95,7 @@ enum {
     AW_H3_DEV_GIC_VCPU,
     AW_H3_DEV_RTC,
     AW_H3_DEV_CPUCFG,
+    AW_H3_DEV_R_TWI,
     AW_H3_DEV_SDRAM
 };
 
@@ -133,6 +136,9 @@ struct AwH3State {
     AwSidState sid;
     AwSdHostState mmc0;
     AWI2CState i2c0;
+    AWI2CState i2c1;
+    AWI2CState i2c2;
+    AWI2CState r_twi;
     AwSun8iEmacState emac;
     AwRtcState rtc;
     GICState gic;
-- 
2.25.1

Re: [PATCH v2 2/3] hw: allwinner-i2c: Fix TWI_CNTR_INT_FLAG on SUN6i SoCs

[Strahinja Jankovic]

On Mon, Feb 20, 2023 at 9:13 AM <qianfanguijin@163.com> wrote:
>
> From: qianfan Zhao <qianfanguijin@163.com>
>
> TWI_CNTR_INT_FLAG is W1C(write 1 to clear and write 0 has non-effect)
> register on SUN6i based SoCs, we should lower interrupt when the guest
> set this bit.
>
> The linux kernel will hang in irq handler(mv64xxx_i2c_intr) if no
> device connected on the i2c bus, next is the trace log:
>
> allwinner_i2c_write write   CNTR(0x0c): 0xc4 A_ACK BUS_EN INT_EN
> allwinner_i2c_write write   CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> allwinner_i2c_read  read    STAT(0x10): 0x20 STAT_M_ADDR_WR_NACK
> allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
> allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
> allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
> allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
> ...
>
> Fix it.
>
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  hw/i2c/allwinner-i2c.c         | 26 ++++++++++++++++++++++++--
>  include/hw/i2c/allwinner-i2c.h |  6 ++++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i2c/allwinner-i2c.c b/hw/i2c/allwinner-i2c.c
> index fa650e7e02..819638d740 100644
> --- a/hw/i2c/allwinner-i2c.c
> +++ b/hw/i2c/allwinner-i2c.c
> @@ -463,10 +463,16 @@ static void allwinner_i2c_write(void *opaque, hwaddr offset,
>                  s->stat = STAT_FROM_STA(STAT_IDLE);
>                  s->cntr &= ~TWI_CNTR_M_STP;
>              }
> -            if ((s->cntr & TWI_CNTR_INT_FLAG) == 0) {
> -                /* Interrupt flag cleared */
> +
> +            if (!s->irq_clear_inverted && !(s->cntr & TWI_CNTR_INT_FLAG)) {
> +                /* Write 0 to clear this flag */
> +                qemu_irq_lower(s->irq);
> +            } else if (s->irq_clear_inverted && (s->cntr & TWI_CNTR_INT_FLAG)) {
> +                /* Write 1 to clear this flag */
> +                s->cntr &= ~TWI_CNTR_INT_FLAG;
>                  qemu_irq_lower(s->irq);
>              }
> +
>              if ((s->cntr & TWI_CNTR_A_ACK) == 0) {
>                  if (STAT_TO_STA(s->stat) == STAT_M_DATA_RX_ACK) {
>                      s->stat = STAT_FROM_STA(STAT_M_DATA_RX_NACK);
> @@ -557,9 +563,25 @@ static const TypeInfo allwinner_i2c_type_info = {
>      .class_init = allwinner_i2c_class_init,
>  };
>
> +static void allwinner_i2c_sun6i_init(Object *obj)
> +{
> +    AWI2CState *s = AW_I2C(obj);
> +
> +    s->irq_clear_inverted = true;
> +}
> +
> +static const TypeInfo allwinner_i2c_sun6i_type_info = {
> +    .name = TYPE_AW_I2C_SUN6I,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AWI2CState),
> +    .instance_init = allwinner_i2c_sun6i_init,
> +    .class_init = allwinner_i2c_class_init,
> +};
> +
>  static void allwinner_i2c_register_types(void)
>  {
>      type_register_static(&allwinner_i2c_type_info);
> +    type_register_static(&allwinner_i2c_sun6i_type_info);
>  }
>
>  type_init(allwinner_i2c_register_types)
> diff --git a/include/hw/i2c/allwinner-i2c.h b/include/hw/i2c/allwinner-i2c.h
> index 4f378b86ba..0e325d265e 100644
> --- a/include/hw/i2c/allwinner-i2c.h
> +++ b/include/hw/i2c/allwinner-i2c.h
> @@ -28,6 +28,10 @@
>  #include "qom/object.h"
>
>  #define TYPE_AW_I2C "allwinner.i2c"
> +
> +/** Allwinner I2C sun6i family and newer (A31, H2+, H3, etc) */
> +#define TYPE_AW_I2C_SUN6I    TYPE_AW_I2C "-sun6i"
> +
>  OBJECT_DECLARE_SIMPLE_TYPE(AWI2CState, AW_I2C)
>
>  #define AW_I2C_MEM_SIZE         0x24
> @@ -50,6 +54,8 @@ struct AWI2CState {
>      uint8_t srst;
>      uint8_t efr;
>      uint8_t lcr;
> +
> +    bool irq_clear_inverted;
>  };
>
>  #endif /* ALLWINNER_I2C_H */
> --
> 2.25.1
>

Tried this patch with avocado for cubieboard and orangepi-pc, all
tests are passing.

So for me:

Reviewed-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
Tested-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>

Re: [PATCH v2 3/3] hw: arm: allwinner-h3: Fix and complete H3 i2c devices

[Strahinja Jankovic]

(adding Niek to the thread as well)

On Mon, Feb 20, 2023 at 9:13 AM <qianfanguijin@163.com> wrote:
>
> From: qianfan Zhao <qianfanguijin@163.com>
>
> Allwinner h3 has 4 twi(i2c) devices named twi0, twi1, twi2 and r_twi.
> The registers are compatible with TYPE_AW_I2C_SUN6I, write 1 to clear
> control register's INT_FLAG bit.
>
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  hw/arm/allwinner-h3.c         | 29 +++++++++++++++++++++++++----
>  include/hw/arm/allwinner-h3.h |  6 ++++++
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index bfce3c8d92..69d0ad6f50 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -54,6 +54,8 @@ const hwaddr allwinner_h3_memmap[] = {
>      [AW_H3_DEV_UART2]      = 0x01c28800,
>      [AW_H3_DEV_UART3]      = 0x01c28c00,
>      [AW_H3_DEV_TWI0]       = 0x01c2ac00,
> +    [AW_H3_DEV_TWI1]       = 0x01c2b000,
> +    [AW_H3_DEV_TWI2]       = 0x01c2b400,
>      [AW_H3_DEV_EMAC]       = 0x01c30000,
>      [AW_H3_DEV_DRAMCOM]    = 0x01c62000,
>      [AW_H3_DEV_DRAMCTL]    = 0x01c63000,
> @@ -64,6 +66,7 @@ const hwaddr allwinner_h3_memmap[] = {
>      [AW_H3_DEV_GIC_VCPU]   = 0x01c86000,
>      [AW_H3_DEV_RTC]        = 0x01f00000,
>      [AW_H3_DEV_CPUCFG]     = 0x01f01c00,
> +    [AW_H3_DEV_R_TWI]      = 0x01f02400,
>      [AW_H3_DEV_SDRAM]      = 0x40000000
>  };
>
> @@ -107,8 +110,6 @@ struct AwH3Unimplemented {
>      { "uart1",     0x01c28400, 1 * KiB },
>      { "uart2",     0x01c28800, 1 * KiB },
>      { "uart3",     0x01c28c00, 1 * KiB },
> -    { "twi1",      0x01c2b000, 1 * KiB },
> -    { "twi2",      0x01c2b400, 1 * KiB },
>      { "scr",       0x01c2c400, 1 * KiB },
>      { "gpu",       0x01c40000, 64 * KiB },
>      { "hstmr",     0x01c60000, 4 * KiB },
> @@ -123,7 +124,6 @@ struct AwH3Unimplemented {
>      { "r_prcm",    0x01f01400, 1 * KiB },
>      { "r_twd",     0x01f01800, 1 * KiB },
>      { "r_cir-rx",  0x01f02000, 1 * KiB },
> -    { "r_twi",     0x01f02400, 1 * KiB },
>      { "r_uart",    0x01f02800, 1 * KiB },
>      { "r_pio",     0x01f02c00, 1 * KiB },
>      { "r_pwm",     0x01f03800, 1 * KiB },
> @@ -151,8 +151,11 @@ enum {
>      AW_H3_GIC_SPI_UART2     =  2,
>      AW_H3_GIC_SPI_UART3     =  3,
>      AW_H3_GIC_SPI_TWI0      =  6,
> +    AW_H3_GIC_SPI_TWI1      =  7,
> +    AW_H3_GIC_SPI_TWI2      =  8,
>      AW_H3_GIC_SPI_TIMER0    = 18,
>      AW_H3_GIC_SPI_TIMER1    = 19,
> +    AW_H3_GIC_SPI_R_TWI     = 44,
>      AW_H3_GIC_SPI_MMC0      = 60,
>      AW_H3_GIC_SPI_EHCI0     = 72,
>      AW_H3_GIC_SPI_OHCI0     = 73,
> @@ -227,7 +230,10 @@ static void allwinner_h3_init(Object *obj)
>
>      object_initialize_child(obj, "rtc", &s->rtc, TYPE_AW_RTC_SUN6I);
>
> -    object_initialize_child(obj, "twi0", &s->i2c0, TYPE_AW_I2C);
> +    object_initialize_child(obj, "twi0",  &s->i2c0,  TYPE_AW_I2C_SUN6I);
> +    object_initialize_child(obj, "twi1",  &s->i2c1,  TYPE_AW_I2C_SUN6I);
> +    object_initialize_child(obj, "twi2",  &s->i2c2,  TYPE_AW_I2C_SUN6I);
> +    object_initialize_child(obj, "r_twi", &s->r_twi, TYPE_AW_I2C_SUN6I);
>  }
>
>  static void allwinner_h3_realize(DeviceState *dev, Error **errp)
> @@ -432,6 +438,21 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c0), 0,
>                         qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI0));
>
> +    sysbus_realize(SYS_BUS_DEVICE(&s->i2c1), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c1), 0, s->memmap[AW_H3_DEV_TWI1]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c1), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI1));
> +
> +    sysbus_realize(SYS_BUS_DEVICE(&s->i2c2), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c2), 0, s->memmap[AW_H3_DEV_TWI2]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c2), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_TWI2));
> +
> +    sysbus_realize(SYS_BUS_DEVICE(&s->r_twi), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->r_twi), 0, s->memmap[AW_H3_DEV_R_TWI]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->r_twi), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_R_TWI));
> +
>      /* Unimplemented devices */
>      for (i = 0; i < ARRAY_SIZE(unimplemented); i++) {
>          create_unimplemented_device(unimplemented[i].device_name,
> diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
> index 1d7ce20589..59e0f822d2 100644
> --- a/include/hw/arm/allwinner-h3.h
> +++ b/include/hw/arm/allwinner-h3.h
> @@ -84,6 +84,8 @@ enum {
>      AW_H3_DEV_UART3,
>      AW_H3_DEV_EMAC,
>      AW_H3_DEV_TWI0,
> +    AW_H3_DEV_TWI1,
> +    AW_H3_DEV_TWI2,
>      AW_H3_DEV_DRAMCOM,
>      AW_H3_DEV_DRAMCTL,
>      AW_H3_DEV_DRAMPHY,
> @@ -93,6 +95,7 @@ enum {
>      AW_H3_DEV_GIC_VCPU,
>      AW_H3_DEV_RTC,
>      AW_H3_DEV_CPUCFG,
> +    AW_H3_DEV_R_TWI,
>      AW_H3_DEV_SDRAM
>  };
>
> @@ -133,6 +136,9 @@ struct AwH3State {
>      AwSidState sid;
>      AwSdHostState mmc0;
>      AWI2CState i2c0;
> +    AWI2CState i2c1;
> +    AWI2CState i2c2;
> +    AWI2CState r_twi;
>      AwSun8iEmacState emac;
>      AwRtcState rtc;
>      GICState gic;
> --
> 2.25.1
>

As far as I can see, the TWI for H3 is indeed treated as
"allwinner,sun6i-a31-i2c" in Linux, so it should have W1C
functionality and use TYPE_AW_I2C_SUN6I.

Reviewed-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>

Best regards,
Strahinja

Re: [PATCH v2 1/3] hw: allwinner-i2c: Make the trace message more readable

[Peter Maydell]

On Mon, 20 Feb 2023 at 08:13, <qianfanguijin@163.com> wrote:
>
> From: qianfan Zhao <qianfanguijin@163.com>
>
> Next is an example when read/write trace enabled:
>
> allwinner_i2c_write write  XADDR(0x04): 0x00
> allwinner_i2c_write write   CNTR(0x0c): 0x50 M_STP BUS_EN
> allwinner_i2c_write write   CNTR(0x0c): 0xe4 A_ACK M_STA BUS_EN INT_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> allwinner_i2c_read  read    STAT(0x10): 0x08 STAT_M_STA_TX

This seems like overkill to me. We don't do it for
any other devices. If we did want to do it we should
do it by providing a generic mechanism somewhere that
it's easy for devices to use, not by implementing a lot
of string operations inside the code for this one device.

thanks
-- PMM

Re: [PATCH v2 2/3] hw: allwinner-i2c: Fix TWI_CNTR_INT_FLAG on SUN6i SoCs

[Peter Maydell]

On Mon, 20 Feb 2023 at 08:13, <qianfanguijin@163.com> wrote:
>
> From: qianfan Zhao <qianfanguijin@163.com>
>
> TWI_CNTR_INT_FLAG is W1C(write 1 to clear and write 0 has non-effect)
> register on SUN6i based SoCs, we should lower interrupt when the guest
> set this bit.
>
> The linux kernel will hang in irq handler(mv64xxx_i2c_intr) if no
> device connected on the i2c bus, next is the trace log:
>
> allwinner_i2c_write write   CNTR(0x0c): 0xc4 A_ACK BUS_EN INT_EN
> allwinner_i2c_write write   CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> allwinner_i2c_read  read    STAT(0x10): 0x20 STAT_M_ADDR_WR_NACK
> allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
> allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
> allwinner_i2c_write write   CNTR(0x0c): 0x54 A_ACK M_STP BUS_EN
> allwinner_i2c_write write   CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    CNTR(0x0c): 0x4c A_ACK INT_FLAG BUS_EN
> allwinner_i2c_read  read    STAT(0x10): 0xf8 STAT_IDLE
> ...
>
> Fix it.
>
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 3/3] hw: arm: allwinner-h3: Fix and complete H3 i2c devices

[Peter Maydell]

On Mon, 20 Feb 2023 at 08:13, <qianfanguijin@163.com> wrote:
>
> From: qianfan Zhao <qianfanguijin@163.com>
>
> Allwinner h3 has 4 twi(i2c) devices named twi0, twi1, twi2 and r_twi.
> The registers are compatible with TYPE_AW_I2C_SUN6I, write 1 to clear
> control register's INT_FLAG bit.
>
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 1/3] hw: allwinner-i2c: Make the trace message more readable

[Peter Maydell]

On Mon, 6 Mar 2023 at 13:23, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 20 Feb 2023 at 08:13, <qianfanguijin@163.com> wrote:
> >
> > From: qianfan Zhao <qianfanguijin@163.com>
> >
> > Next is an example when read/write trace enabled:
> >
> > allwinner_i2c_write write  XADDR(0x04): 0x00
> > allwinner_i2c_write write   CNTR(0x0c): 0x50 M_STP BUS_EN
> > allwinner_i2c_write write   CNTR(0x0c): 0xe4 A_ACK M_STA BUS_EN INT_EN
> > allwinner_i2c_read  read    CNTR(0x0c): 0xcc A_ACK INT_FLAG BUS_EN INT_EN
> > allwinner_i2c_read  read    STAT(0x10): 0x08 STAT_M_STA_TX
>
> This seems like overkill to me. We don't do it for
> any other devices. If we did want to do it we should
> do it by providing a generic mechanism somewhere that
> it's easy for devices to use, not by implementing a lot
> of string operations inside the code for this one device.

I disagree with this patch, but I've taken patches 2 and 3
into target-arm.next for 8.0.

thanks
-- PMM