All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device
@ 2021-01-26 19:32 wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 1/6] hw/arm: Remove GPIO from unimplemented NPCM7XX wuhaotsh--- via
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

This patch set implements the System manager bus (SMBus) module in NPCM7XX
SoC. Basically, it emulates the data transactions of the module, not the
SDA/SCL levels. We have also added a QTest which contains read and write
operations for both single-byte and FIFO mode, and added basic I2C device
trees for npcm750-evb and quanta-gsj boards.

We also cleaned up the unimplemented GPIO devices in npcm7xx.c since they
are already implemented.

Hao Wu (6):
  hw/arm: Remove GPIO from unimplemented NPCM7XX
  hw/i2c: Implement NPCM7XX SMBus Module Single Mode
  hw/arm: Add I2C device tree for NPCM750 eval board
  hw/arm: Add I2C device tree for Quanta GSJ
  hw/i2c: Add a QTest for NPCM7XX SMBus Device
  hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode

 docs/system/arm/nuvoton.rst      |    2 +-
 hw/arm/npcm7xx.c                 |   76 ++-
 hw/arm/npcm7xx_boards.c          |   32 +
 hw/i2c/meson.build               |    1 +
 hw/i2c/npcm7xx_smbus.c           | 1071 ++++++++++++++++++++++++++++++
 hw/i2c/trace-events              |   12 +
 include/hw/arm/npcm7xx.h         |    2 +
 include/hw/i2c/npcm7xx_smbus.h   |  113 ++++
 tests/qtest/meson.build          |    1 +
 tests/qtest/npcm7xx_smbus-test.c |  495 ++++++++++++++
 10 files changed, 1780 insertions(+), 25 deletions(-)
 create mode 100644 hw/i2c/npcm7xx_smbus.c
 create mode 100644 include/hw/i2c/npcm7xx_smbus.h
 create mode 100644 tests/qtest/npcm7xx_smbus-test.c

-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 1/6] hw/arm: Remove GPIO from unimplemented NPCM7XX
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode wuhaotsh--- via
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

NPCM7XX GPIO devices have been implemented in hw/gpio/npcm7xx-gpio.c. So
we removed them from the unimplemented devices list.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu<wuhaotsh@google.com>
---
 hw/arm/npcm7xx.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 72040d4079..d1fe9bd1df 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -576,14 +576,6 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("npcm7xx.pcierc",       0xe1000000,  64 * KiB);
     create_unimplemented_device("npcm7xx.kcs",          0xf0007000,   4 * KiB);
     create_unimplemented_device("npcm7xx.gfxi",         0xf000e000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[0]",      0xf0010000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[1]",      0xf0011000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[2]",      0xf0012000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[3]",      0xf0013000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[4]",      0xf0014000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[5]",      0xf0015000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[6]",      0xf0016000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.gpio[7]",      0xf0017000,   4 * KiB);
     create_unimplemented_device("npcm7xx.smbus[0]",     0xf0080000,   4 * KiB);
     create_unimplemented_device("npcm7xx.smbus[1]",     0xf0081000,   4 * KiB);
     create_unimplemented_device("npcm7xx.smbus[2]",     0xf0082000,   4 * KiB);
-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 1/6] hw/arm: Remove GPIO from unimplemented NPCM7XX wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-26 23:00   ` Corey Minyard
  2021-01-28 17:28   ` Peter Maydell
  2021-01-26 19:32 ` [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board wuhaotsh--- via
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

This commit implements the single-byte mode of the SMBus.

Each Nuvoton SoC has 16 System Management Bus (SMBus). These buses
compliant with SMBus and I2C protocol.

This patch implements the single-byte mode of the SMBus. In this mode,
the user sends or receives a byte each time. The SMBus device transmits
it to the underlying i2c device and sends an interrupt back to the QEMU
guest.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 docs/system/arm/nuvoton.rst    |   2 +-
 hw/arm/npcm7xx.c               |  68 ++-
 hw/i2c/meson.build             |   1 +
 hw/i2c/npcm7xx_smbus.c         | 766 +++++++++++++++++++++++++++++++++
 hw/i2c/trace-events            |  11 +
 include/hw/arm/npcm7xx.h       |   2 +
 include/hw/i2c/npcm7xx_smbus.h |  88 ++++
 7 files changed, 921 insertions(+), 17 deletions(-)
 create mode 100644 hw/i2c/npcm7xx_smbus.c
 create mode 100644 include/hw/i2c/npcm7xx_smbus.h

diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index a1786342e2..34fc799b2d 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -43,6 +43,7 @@ Supported devices
  * GPIO controller
  * Analog to Digital Converter (ADC)
  * Pulse Width Modulation (PWM)
+ * SMBus controller (SMBF)
 
 Missing devices
 ---------------
@@ -58,7 +59,6 @@ Missing devices
 
  * Ethernet controllers (GMAC and EMC)
  * USB device (USBD)
- * SMBus controller (SMBF)
  * Peripheral SPI controller (PSPI)
  * SD/MMC host
  * PECI interface
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index d1fe9bd1df..8f596ffd69 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -104,6 +104,22 @@ enum NPCM7xxInterrupt {
     NPCM7XX_OHCI_IRQ            = 62,
     NPCM7XX_PWM0_IRQ            = 93,   /* PWM module 0 */
     NPCM7XX_PWM1_IRQ,                   /* PWM module 1 */
+    NPCM7XX_SMBUS0_IRQ          = 64,
+    NPCM7XX_SMBUS1_IRQ,
+    NPCM7XX_SMBUS2_IRQ,
+    NPCM7XX_SMBUS3_IRQ,
+    NPCM7XX_SMBUS4_IRQ,
+    NPCM7XX_SMBUS5_IRQ,
+    NPCM7XX_SMBUS6_IRQ,
+    NPCM7XX_SMBUS7_IRQ,
+    NPCM7XX_SMBUS8_IRQ,
+    NPCM7XX_SMBUS9_IRQ,
+    NPCM7XX_SMBUS10_IRQ,
+    NPCM7XX_SMBUS11_IRQ,
+    NPCM7XX_SMBUS12_IRQ,
+    NPCM7XX_SMBUS13_IRQ,
+    NPCM7XX_SMBUS14_IRQ,
+    NPCM7XX_SMBUS15_IRQ,
     NPCM7XX_GPIO0_IRQ           = 116,
     NPCM7XX_GPIO1_IRQ,
     NPCM7XX_GPIO2_IRQ,
@@ -152,6 +168,26 @@ static const hwaddr npcm7xx_pwm_addr[] = {
     0xf0104000,
 };
 
+/* Direct memory-mapped access to each SMBus Module. */
+static const hwaddr npcm7xx_smbus_addr[] = {
+    0xf0080000,
+    0xf0081000,
+    0xf0082000,
+    0xf0083000,
+    0xf0084000,
+    0xf0085000,
+    0xf0086000,
+    0xf0087000,
+    0xf0088000,
+    0xf0089000,
+    0xf008a000,
+    0xf008b000,
+    0xf008c000,
+    0xf008d000,
+    0xf008e000,
+    0xf008f000,
+};
+
 static const struct {
     hwaddr regs_addr;
     uint32_t unconnected_pins;
@@ -353,6 +389,11 @@ static void npcm7xx_init(Object *obj)
         object_initialize_child(obj, "gpio[*]", &s->gpio[i], TYPE_NPCM7XX_GPIO);
     }
 
+    for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
+        object_initialize_child(obj, "smbus[*]", &s->smbus[i],
+                                TYPE_NPCM7XX_SMBUS);
+    }
+
     object_initialize_child(obj, "ehci", &s->ehci, TYPE_NPCM7XX_EHCI);
     object_initialize_child(obj, "ohci", &s->ohci, TYPE_SYSBUS_OHCI);
 
@@ -509,6 +550,17 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
                            npcm7xx_irq(s, NPCM7XX_GPIO0_IRQ + i));
     }
 
+    /* SMBus modules. Cannot fail. */
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_smbus_addr) != ARRAY_SIZE(s->smbus));
+    for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
+        Object *obj = OBJECT(&s->smbus[i]);
+
+        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
+        sysbus_mmio_map(SYS_BUS_DEVICE(obj), 0, npcm7xx_smbus_addr[i]);
+        sysbus_connect_irq(SYS_BUS_DEVICE(obj), 0,
+                           npcm7xx_irq(s, NPCM7XX_SMBUS0_IRQ + i));
+    }
+
     /* USB Host */
     object_property_set_bool(OBJECT(&s->ehci), "companion-enable", true,
                              &error_abort);
@@ -576,22 +628,6 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("npcm7xx.pcierc",       0xe1000000,  64 * KiB);
     create_unimplemented_device("npcm7xx.kcs",          0xf0007000,   4 * KiB);
     create_unimplemented_device("npcm7xx.gfxi",         0xf000e000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[0]",     0xf0080000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[1]",     0xf0081000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[2]",     0xf0082000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[3]",     0xf0083000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[4]",     0xf0084000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[5]",     0xf0085000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[6]",     0xf0086000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[7]",     0xf0087000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[8]",     0xf0088000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[9]",     0xf0089000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[10]",    0xf008a000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[11]",    0xf008b000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[12]",    0xf008c000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[13]",    0xf008d000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[14]",    0xf008e000,   4 * KiB);
-    create_unimplemented_device("npcm7xx.smbus[15]",    0xf008f000,   4 * KiB);
     create_unimplemented_device("npcm7xx.espi",         0xf009f000,   4 * KiB);
     create_unimplemented_device("npcm7xx.peci",         0xf0100000,   4 * KiB);
     create_unimplemented_device("npcm7xx.siox[1]",      0xf0101000,   4 * KiB);
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index 3a511539ad..cdcd694a7f 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -9,6 +9,7 @@ i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
 i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c'))
 i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
 i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
+i2c_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_smbus.c'))
 i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
 i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
 i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
new file mode 100644
index 0000000000..e8a8fdbaff
--- /dev/null
+++ b/hw/i2c/npcm7xx_smbus.c
@@ -0,0 +1,766 @@
+/*
+ * Nuvoton NPCM7xx SMBus Module.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/i2c/npcm7xx_smbus.h"
+#include "migration/vmstate.h"
+#include "qemu/bitops.h"
+#include "qemu/guest-random.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+
+#include "trace.h"
+
+#define NPCM7XX_SMBUS_VERSION 1
+#define NPCM7XX_SMBUS_FIFO_EN 0
+
+enum NPCM7xxSMBusCommonRegister {
+    NPCM7XX_SMB_SDA     = 0x0,
+    NPCM7XX_SMB_ST      = 0x2,
+    NPCM7XX_SMB_CST     = 0x4,
+    NPCM7XX_SMB_CTL1    = 0x6,
+    NPCM7XX_SMB_ADDR1   = 0x8,
+    NPCM7XX_SMB_CTL2    = 0xa,
+    NPCM7XX_SMB_ADDR2   = 0xc,
+    NPCM7XX_SMB_CTL3    = 0xe,
+    NPCM7XX_SMB_CST2    = 0x18,
+    NPCM7XX_SMB_CST3    = 0x19,
+    NPCM7XX_SMB_VER     = 0x1f,
+};
+
+enum NPCM7xxSMBusBank0Register {
+    NPCM7XX_SMB_ADDR3   = 0x10,
+    NPCM7XX_SMB_ADDR7   = 0x11,
+    NPCM7XX_SMB_ADDR4   = 0x12,
+    NPCM7XX_SMB_ADDR8   = 0x13,
+    NPCM7XX_SMB_ADDR5   = 0x14,
+    NPCM7XX_SMB_ADDR9   = 0x15,
+    NPCM7XX_SMB_ADDR6   = 0x16,
+    NPCM7XX_SMB_ADDR10  = 0x17,
+    NPCM7XX_SMB_CTL4    = 0x1a,
+    NPCM7XX_SMB_CTL5    = 0x1b,
+    NPCM7XX_SMB_SCLLT   = 0x1c,
+    NPCM7XX_SMB_FIF_CTL = 0x1d,
+    NPCM7XX_SMB_SCLHT   = 0x1e,
+};
+
+enum NPCM7xxSMBusBank1Register {
+    NPCM7XX_SMB_FIF_CTS  = 0x10,
+    NPCM7XX_SMB_FAIR_PER = 0x11,
+    NPCM7XX_SMB_TXF_CTL  = 0x12,
+    NPCM7XX_SMB_T_OUT    = 0x14,
+    NPCM7XX_SMB_TXF_STS  = 0x1a,
+    NPCM7XX_SMB_RXF_STS  = 0x1c,
+    NPCM7XX_SMB_RXF_CTL  = 0x1e,
+};
+
+/* ST fields */
+#define NPCM7XX_SMBST_STP           BIT(7)
+#define NPCM7XX_SMBST_SDAST         BIT(6)
+#define NPCM7XX_SMBST_BER           BIT(5)
+#define NPCM7XX_SMBST_NEGACK        BIT(4)
+#define NPCM7XX_SMBST_STASTR        BIT(3)
+#define NPCM7XX_SMBST_NMATCH        BIT(2)
+#define NPCM7XX_SMBST_MODE          BIT(1)
+#define NPCM7XX_SMBST_XMIT          BIT(0)
+
+/* CST fields */
+#define NPCM7XX_SMBCST_ARPMATCH        BIT(7)
+#define NPCM7XX_SMBCST_MATCHAF         BIT(6)
+#define NPCM7XX_SMBCST_TGSCL           BIT(5)
+#define NPCM7XX_SMBCST_TSDA            BIT(4)
+#define NPCM7XX_SMBCST_GCMATCH         BIT(3)
+#define NPCM7XX_SMBCST_MATCH           BIT(2)
+#define NPCM7XX_SMBCST_BB              BIT(1)
+#define NPCM7XX_SMBCST_BUSY            BIT(0)
+
+/* CST2 fields */
+#define NPCM7XX_SMBCST2_INTSTS         BIT(7)
+#define NPCM7XX_SMBCST2_MATCH7F        BIT(6)
+#define NPCM7XX_SMBCST2_MATCH6F        BIT(5)
+#define NPCM7XX_SMBCST2_MATCH5F        BIT(4)
+#define NPCM7XX_SMBCST2_MATCH4F        BIT(3)
+#define NPCM7XX_SMBCST2_MATCH3F        BIT(2)
+#define NPCM7XX_SMBCST2_MATCH2F        BIT(1)
+#define NPCM7XX_SMBCST2_MATCH1F        BIT(0)
+
+/* CST3 fields */
+#define NPCM7XX_SMBCST3_EO_BUSY        BIT(7)
+#define NPCM7XX_SMBCST3_MATCH10F       BIT(2)
+#define NPCM7XX_SMBCST3_MATCH9F        BIT(1)
+#define NPCM7XX_SMBCST3_MATCH8F        BIT(0)
+
+/* CTL1 fields */
+#define NPCM7XX_SMBCTL1_STASTRE     BIT(7)
+#define NPCM7XX_SMBCTL1_NMINTE      BIT(6)
+#define NPCM7XX_SMBCTL1_GCMEN       BIT(5)
+#define NPCM7XX_SMBCTL1_ACK         BIT(4)
+#define NPCM7XX_SMBCTL1_EOBINTE     BIT(3)
+#define NPCM7XX_SMBCTL1_INTEN       BIT(2)
+#define NPCM7XX_SMBCTL1_STOP        BIT(1)
+#define NPCM7XX_SMBCTL1_START       BIT(0)
+
+/* CTL2 fields */
+#define NPCM7XX_SMBCTL2_SCLFRQ(rv)  extract8((rv), 1, 6)
+#define NPCM7XX_SMBCTL2_ENABLE      BIT(0)
+
+/* CTL3 fields */
+#define NPCM7XX_SMBCTL3_SCL_LVL     BIT(7)
+#define NPCM7XX_SMBCTL3_SDA_LVL     BIT(6)
+#define NPCM7XX_SMBCTL3_BNK_SEL     BIT(5)
+#define NPCM7XX_SMBCTL3_400K_MODE   BIT(4)
+#define NPCM7XX_SMBCTL3_IDL_START   BIT(3)
+#define NPCM7XX_SMBCTL3_ARPMEN      BIT(2)
+#define NPCM7XX_SMBCTL3_SCLFRQ(rv)  extract8((rv), 0, 2)
+
+/* ADDR fields */
+#define NPCM7XX_ADDR_EN             BIT(7)
+#define NPCM7XX_ADDR_A(rv)          extract8((rv), 0, 6)
+
+#define KEEP_OLD_BIT(o, n, b)       (((n) & (~(b))) | ((o) & (b)))
+#define WRITE_ONE_CLEAR(o, n, b)    ((n) & (b) ? (o) & (~(b)) : (o))
+
+#define NPCM7XX_SMBUS_ENABLED(s)    ((s)->ctl2 & NPCM7XX_SMBCTL2_ENABLE)
+
+/* Reset values */
+#define NPCM7XX_SMB_ST_INIT_VAL     0x00
+#define NPCM7XX_SMB_CST_INIT_VAL    0x10
+#define NPCM7XX_SMB_CST2_INIT_VAL   0x00
+#define NPCM7XX_SMB_CST3_INIT_VAL   0x00
+#define NPCM7XX_SMB_CTL1_INIT_VAL   0x00
+#define NPCM7XX_SMB_CTL2_INIT_VAL   0x00
+#define NPCM7XX_SMB_CTL3_INIT_VAL   0xc0
+#define NPCM7XX_SMB_CTL4_INIT_VAL   0x07
+#define NPCM7XX_SMB_CTL5_INIT_VAL   0x00
+#define NPCM7XX_SMB_ADDR_INIT_VAL   0x00
+#define NPCM7XX_SMB_SCLLT_INIT_VAL  0x00
+#define NPCM7XX_SMB_SCLHT_INIT_VAL  0x00
+
+static uint8_t npcm7xx_smbus_get_version(void)
+{
+    return NPCM7XX_SMBUS_FIFO_EN << 7 | NPCM7XX_SMBUS_VERSION;
+}
+
+static void npcm7xx_smbus_update_irq(NPCM7xxSMBusState *s)
+{
+    int level;
+
+    if (s->ctl1 & NPCM7XX_SMBCTL1_INTEN) {
+        level = !!((s->ctl1 & NPCM7XX_SMBCTL1_NMINTE &&
+                    s->st & NPCM7XX_SMBST_NMATCH) ||
+                   (s->st & NPCM7XX_SMBST_BER) ||
+                   (s->st & NPCM7XX_SMBST_NEGACK) ||
+                   (s->st & NPCM7XX_SMBST_SDAST) ||
+                   (s->ctl1 & NPCM7XX_SMBCTL1_STASTRE &&
+                    s->st & NPCM7XX_SMBST_SDAST) ||
+                   (s->ctl1 & NPCM7XX_SMBCTL1_EOBINTE &&
+                    s->cst3 & NPCM7XX_SMBCST3_EO_BUSY));
+
+        if (level) {
+            s->cst2 |= NPCM7XX_SMBCST2_INTSTS;
+        } else {
+            s->cst2 &= ~NPCM7XX_SMBCST2_INTSTS;
+        }
+        qemu_set_irq(s->irq, level);
+    }
+}
+
+static void npcm7xx_smbus_nack(NPCM7xxSMBusState *s)
+{
+    s->st &= ~NPCM7XX_SMBST_SDAST;
+    s->st |= NPCM7XX_SMBST_NEGACK;
+    s->status = NPCM7XX_SMBUS_STATUS_NEGACK;
+}
+
+static void npcm7xx_smbus_send_byte(NPCM7xxSMBusState *s, uint8_t value)
+{
+    int rv = i2c_send(s->bus, value);
+
+    if (rv) {
+        npcm7xx_smbus_nack(s);
+    } else {
+        s->st |= NPCM7XX_SMBST_SDAST;
+    }
+    trace_npcm7xx_smbus_send_byte((DEVICE(s)->canonical_path), value, !rv);
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_recv_byte(NPCM7xxSMBusState *s)
+{
+    s->sda = i2c_recv(s->bus);
+    s->st |= NPCM7XX_SMBST_SDAST;
+    if (s->st & NPCM7XX_SMBCTL1_ACK) {
+        trace_npcm7xx_smbus_nack(DEVICE(s)->canonical_path);
+        i2c_nack(s->bus);
+        s->st &= NPCM7XX_SMBCTL1_ACK;
+    }
+    trace_npcm7xx_smbus_recv_byte((DEVICE(s)->canonical_path), s->sda);
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_start(NPCM7xxSMBusState *s)
+{
+    /*
+     * We can start the bus if one of these is true:
+     * 1. The bus is idle (so we can request it)
+     * 2. We are the occupier (it's a repeated start condition.)
+     */
+    int available = !i2c_bus_busy(s->bus) ||
+                    s->status != NPCM7XX_SMBUS_STATUS_IDLE;
+
+    if (available) {
+        s->st |= NPCM7XX_SMBST_MODE | NPCM7XX_SMBST_XMIT | NPCM7XX_SMBST_SDAST;
+        s->cst |= NPCM7XX_SMBCST_BUSY;
+    } else {
+        s->st &= ~NPCM7XX_SMBST_MODE;
+        s->cst &= ~NPCM7XX_SMBCST_BUSY;
+        s->st |= NPCM7XX_SMBST_BER;
+    }
+
+    trace_npcm7xx_smbus_start(DEVICE(s)->canonical_path, available);
+    s->cst |= NPCM7XX_SMBCST_BB;
+    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_send_address(NPCM7xxSMBusState *s, uint8_t value)
+{
+    int recv;
+    int rv;
+
+    recv = value & BIT(0);
+    rv = i2c_start_transfer(s->bus, value >> 1, recv);
+    trace_npcm7xx_smbus_send_address(DEVICE(s)->canonical_path,
+                                     value >> 1, recv, !rv);
+    if (rv) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: requesting i2c bus for 0x%02x failed: %d\n",
+                      DEVICE(s)->canonical_path, value, rv);
+        /* Failed to start transfer. NACK to reject.*/
+        if (recv) {
+            s->st &= ~NPCM7XX_SMBST_XMIT;
+        } else {
+            s->st |= NPCM7XX_SMBST_XMIT;
+        }
+        npcm7xx_smbus_nack(s);
+        npcm7xx_smbus_update_irq(s);
+        return;
+    }
+
+    s->st &= ~NPCM7XX_SMBST_NEGACK;
+    if (recv) {
+        s->status = NPCM7XX_SMBUS_STATUS_RECEIVING;
+        s->st &= ~NPCM7XX_SMBST_XMIT;
+    } else {
+        s->status = NPCM7XX_SMBUS_STATUS_SENDING;
+        s->st |= NPCM7XX_SMBST_XMIT;
+    }
+
+    if (s->ctl1 & NPCM7XX_SMBCTL1_STASTRE) {
+        s->st |= NPCM7XX_SMBST_STASTR;
+        if (!recv) {
+            s->st |= NPCM7XX_SMBST_SDAST;
+        }
+    } else if (recv) {
+        npcm7xx_smbus_recv_byte(s);
+    }
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_execute_stop(NPCM7xxSMBusState *s)
+{
+    i2c_end_transfer(s->bus);
+    s->st = 0;
+    s->cst = 0;
+    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
+    s->cst3 |= NPCM7XX_SMBCST3_EO_BUSY;
+    trace_npcm7xx_smbus_stop(DEVICE(s)->canonical_path);
+    npcm7xx_smbus_update_irq(s);
+}
+
+
+static void npcm7xx_smbus_stop(NPCM7xxSMBusState *s)
+{
+    if (s->st & NPCM7XX_SMBST_MODE) {
+        switch (s->status) {
+        case NPCM7XX_SMBUS_STATUS_RECEIVING:
+        case NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE:
+            s->status = NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE;
+            break;
+
+        case NPCM7XX_SMBUS_STATUS_NEGACK:
+            s->status = NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK;
+            break;
+
+        default:
+            npcm7xx_smbus_execute_stop(s);
+            break;
+        }
+    }
+}
+
+static uint8_t npcm7xx_smbus_read_sda(NPCM7xxSMBusState *s)
+{
+    uint8_t value = s->sda;
+
+    switch (s->status) {
+    case NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE:
+        npcm7xx_smbus_execute_stop(s);
+        break;
+
+    case NPCM7XX_SMBUS_STATUS_RECEIVING:
+        npcm7xx_smbus_recv_byte(s);
+        break;
+
+    default:
+        /* Do nothing */
+        break;
+    }
+
+    return value;
+}
+
+static void npcm7xx_smbus_write_sda(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->sda = value;
+    if (s->st & NPCM7XX_SMBST_MODE) {
+        switch (s->status) {
+        case NPCM7XX_SMBUS_STATUS_IDLE:
+            npcm7xx_smbus_send_address(s, value);
+            break;
+        case NPCM7XX_SMBUS_STATUS_SENDING:
+            npcm7xx_smbus_send_byte(s, value);
+            break;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: write to SDA in invalid status %d: %u\n",
+                          DEVICE(s)->canonical_path, s->status, value);
+            break;
+        }
+    }
+}
+
+static void npcm7xx_smbus_write_st(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_STP);
+    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_BER);
+    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_STASTR);
+    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_NMATCH);
+
+    if (value & NPCM7XX_SMBST_NEGACK) {
+        s->st &= ~NPCM7XX_SMBST_NEGACK;
+        if (s->status == NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK) {
+            npcm7xx_smbus_execute_stop(s);
+        }
+    }
+
+    if (value & NPCM7XX_SMBST_STASTR &&
+            s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
+        npcm7xx_smbus_recv_byte(s);
+    }
+
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_write_cst(NPCM7xxSMBusState *s, uint8_t value)
+{
+    uint8_t new_value = s->cst;
+
+    s->cst = WRITE_ONE_CLEAR(new_value, value, NPCM7XX_SMBCST_BB);
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_write_cst3(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->cst3 = WRITE_ONE_CLEAR(s->cst3, value, NPCM7XX_SMBCST3_EO_BUSY);
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_write_ctl1(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->ctl1 = KEEP_OLD_BIT(s->ctl1, value,
+            NPCM7XX_SMBCTL1_START | NPCM7XX_SMBCTL1_STOP | NPCM7XX_SMBCTL1_ACK);
+
+    if (value & NPCM7XX_SMBCTL1_START) {
+        npcm7xx_smbus_start(s);
+    }
+
+    if (value & NPCM7XX_SMBCTL1_STOP) {
+        npcm7xx_smbus_stop(s);
+    }
+
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_write_ctl2(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->ctl2 = value;
+
+    if (!NPCM7XX_SMBUS_ENABLED(s)) {
+        /* Disable this SMBus module. */
+        s->ctl1 = 0;
+        s->st = 0;
+        s->cst3 = s->cst3 & (~NPCM7XX_SMBCST3_EO_BUSY);
+        s->cst = 0;
+    }
+}
+
+static void npcm7xx_smbus_write_ctl3(NPCM7xxSMBusState *s, uint8_t value)
+{
+    uint8_t old_ctl3 = s->ctl3;
+
+    /* Write to SDA and SCL bits are ignored. */
+    s->ctl3 =  KEEP_OLD_BIT(old_ctl3, value,
+                            NPCM7XX_SMBCTL3_SCL_LVL | NPCM7XX_SMBCTL3_SDA_LVL);
+}
+
+static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
+{
+    NPCM7xxSMBusState *s = opaque;
+    uint64_t value = 0;
+    uint8_t bank = s->ctl3 & NPCM7XX_SMBCTL3_BNK_SEL;
+
+    switch (offset) {
+    case NPCM7XX_SMB_SDA:
+        value = npcm7xx_smbus_read_sda(s);
+        break;
+
+    case NPCM7XX_SMB_ST:
+        value = s->st;
+        break;
+
+    case NPCM7XX_SMB_CST:
+        value = s->cst;
+        break;
+
+    case NPCM7XX_SMB_CTL1:
+        value = s->ctl1;
+        break;
+
+    case NPCM7XX_SMB_ADDR1:
+        value = s->addr[0];
+        break;
+
+    case NPCM7XX_SMB_CTL2:
+        value = s->ctl2;
+        break;
+
+    case NPCM7XX_SMB_ADDR2:
+        value = s->addr[1];
+        break;
+
+    case NPCM7XX_SMB_CTL3:
+        value = s->ctl3;
+        break;
+
+    case NPCM7XX_SMB_CST2:
+        value = s->cst2;
+        break;
+
+    case NPCM7XX_SMB_CST3:
+        value = s->cst3;
+        break;
+
+    case NPCM7XX_SMB_VER:
+        value = npcm7xx_smbus_get_version();
+        break;
+
+    /* This register is either invalid or banked at this point. */
+    default:
+        if (bank) {
+            /* Bank 1 */
+            qemu_log_mask(LOG_GUEST_ERROR,
+                    "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
+                    DEVICE(s)->canonical_path, offset);
+        } else {
+            /* Bank 0 */
+            switch (offset) {
+            case NPCM7XX_SMB_ADDR3:
+                value = s->addr[2];
+                break;
+
+            case NPCM7XX_SMB_ADDR7:
+                value = s->addr[6];
+                break;
+
+            case NPCM7XX_SMB_ADDR4:
+                value = s->addr[3];
+                break;
+
+            case NPCM7XX_SMB_ADDR8:
+                value = s->addr[7];
+                break;
+
+            case NPCM7XX_SMB_ADDR5:
+                value = s->addr[4];
+                break;
+
+            case NPCM7XX_SMB_ADDR9:
+                value = s->addr[8];
+                break;
+
+            case NPCM7XX_SMB_ADDR6:
+                value = s->addr[5];
+                break;
+
+            case NPCM7XX_SMB_ADDR10:
+                value = s->addr[9];
+                break;
+
+            case NPCM7XX_SMB_CTL4:
+                value = s->ctl4;
+                break;
+
+            case NPCM7XX_SMB_CTL5:
+                value = s->ctl5;
+                break;
+
+            case NPCM7XX_SMB_SCLLT:
+                value = s->scllt;
+                break;
+
+            case NPCM7XX_SMB_SCLHT:
+                value = s->sclht;
+                break;
+
+            default:
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
+                        DEVICE(s)->canonical_path, offset);
+                break;
+            }
+        }
+        break;
+    }
+
+    trace_npcm7xx_smbus_read(DEVICE(s)->canonical_path, offset, value, size);
+
+    return value;
+}
+
+static void npcm7xx_smbus_write(void *opaque, hwaddr offset, uint64_t value,
+                              unsigned size)
+{
+    NPCM7xxSMBusState *s = opaque;
+    uint8_t bank = s->ctl3 & NPCM7XX_SMBCTL3_BNK_SEL;
+
+    trace_npcm7xx_smbus_write(DEVICE(s)->canonical_path, offset, value, size);
+
+    switch (offset) {
+    case NPCM7XX_SMB_SDA:
+        npcm7xx_smbus_write_sda(s, value);
+        break;
+
+    case NPCM7XX_SMB_ST:
+        npcm7xx_smbus_write_st(s, value);
+        break;
+
+    case NPCM7XX_SMB_CST:
+        npcm7xx_smbus_write_cst(s, value);
+        break;
+
+    case NPCM7XX_SMB_CTL1:
+        npcm7xx_smbus_write_ctl1(s, value);
+        break;
+
+    case NPCM7XX_SMB_ADDR1:
+        s->addr[0] = value;
+        break;
+
+    case NPCM7XX_SMB_CTL2:
+        npcm7xx_smbus_write_ctl2(s, value);
+        break;
+
+    case NPCM7XX_SMB_ADDR2:
+        s->addr[1] = value;
+        break;
+
+    case NPCM7XX_SMB_CTL3:
+        npcm7xx_smbus_write_ctl3(s, value);
+        break;
+
+    case NPCM7XX_SMB_CST2:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: write to read-only reg: offset 0x%" HWADDR_PRIx "\n",
+                DEVICE(s)->canonical_path, offset);
+        break;
+
+    case NPCM7XX_SMB_CST3:
+        npcm7xx_smbus_write_cst3(s, value);
+        break;
+
+    case NPCM7XX_SMB_VER:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: write to read-only reg: offset 0x%" HWADDR_PRIx "\n",
+                DEVICE(s)->canonical_path, offset);
+        break;
+
+    /* This register is either invalid or banked at this point. */
+    default:
+        if (bank) {
+            /* Bank 1 */
+            qemu_log_mask(LOG_GUEST_ERROR,
+                    "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
+                    DEVICE(s)->canonical_path, offset);
+        } else {
+            /* Bank 0 */
+            switch (offset) {
+            case NPCM7XX_SMB_ADDR3:
+                s->addr[2] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR7:
+                s->addr[6] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR4:
+                s->addr[3] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR8:
+                s->addr[7] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR5:
+                s->addr[4] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR9:
+                s->addr[8] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR6:
+                s->addr[5] = value;
+                break;
+
+            case NPCM7XX_SMB_ADDR10:
+                s->addr[9] = value;
+                break;
+
+            case NPCM7XX_SMB_CTL4:
+                s->ctl4 = value;
+                break;
+
+            case NPCM7XX_SMB_CTL5:
+                s->ctl5 = value;
+                break;
+
+            case NPCM7XX_SMB_SCLLT:
+                s->scllt = value;
+                break;
+
+            case NPCM7XX_SMB_SCLHT:
+                s->sclht = value;
+                break;
+
+            default:
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
+                        DEVICE(s)->canonical_path, offset);
+                break;
+            }
+        }
+        break;
+    }
+}
+
+static const MemoryRegionOps npcm7xx_smbus_ops = {
+    .read = npcm7xx_smbus_read,
+    .write = npcm7xx_smbus_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+        .unaligned = false,
+    },
+};
+
+static void npcm7xx_smbus_enter_reset(Object *obj, ResetType type)
+{
+    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
+
+    s->st = NPCM7XX_SMB_ST_INIT_VAL;
+    s->cst = NPCM7XX_SMB_CST_INIT_VAL;
+    s->cst2 = NPCM7XX_SMB_CST2_INIT_VAL;
+    s->cst3 = NPCM7XX_SMB_CST3_INIT_VAL;
+    s->ctl1 = NPCM7XX_SMB_CTL1_INIT_VAL;
+    s->ctl2 = NPCM7XX_SMB_CTL2_INIT_VAL;
+    s->ctl3 = NPCM7XX_SMB_CTL3_INIT_VAL;
+    s->ctl4 = NPCM7XX_SMB_CTL4_INIT_VAL;
+    s->ctl5 = NPCM7XX_SMB_CTL5_INIT_VAL;
+
+    for (int i = 0; i < NPCM7XX_SMBUS_NR_ADDRS; ++i) {
+        s->addr[i] = NPCM7XX_SMB_ADDR_INIT_VAL;
+    }
+    s->scllt = NPCM7XX_SMB_SCLLT_INIT_VAL;
+    s->sclht = NPCM7XX_SMB_SCLHT_INIT_VAL;
+
+    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
+}
+
+static void npcm7xx_smbus_hold_reset(Object *obj)
+{
+    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
+
+    qemu_irq_lower(s->irq);
+}
+
+static void npcm7xx_smbus_init(Object *obj)
+{
+    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    sysbus_init_irq(sbd, &s->irq);
+    memory_region_init_io(&s->iomem, obj, &npcm7xx_smbus_ops, s,
+                          "regs", 4 * KiB);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
+    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
+}
+
+static const VMStateDescription vmstate_npcm7xx_smbus = {
+    .name = "npcm7xx-smbus",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void npcm7xx_smbus_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "NPCM7xx System Management Bus";
+    dc->vmsd = &vmstate_npcm7xx_smbus;
+    rc->phases.enter = npcm7xx_smbus_enter_reset;
+    rc->phases.hold = npcm7xx_smbus_hold_reset;
+}
+
+static const TypeInfo npcm7xx_smbus_types[] = {
+    {
+        .name = TYPE_NPCM7XX_SMBUS,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(NPCM7xxSMBusState),
+        .class_init = npcm7xx_smbus_class_init,
+        .instance_init = npcm7xx_smbus_init,
+    },
+};
+DEFINE_TYPES(npcm7xx_smbus_types);
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 08db8fa689..c3bb70ad04 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -14,3 +14,14 @@ aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t val
 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"
 aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv %d/%d 0x%02x"
+
+# npcm7xx_smbus.c
+
+npcm7xx_smbus_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
+npcm7xx_smbus_write(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
+npcm7xx_smbus_start(const char *id, int success) "%s starting, success: %d"
+npcm7xx_smbus_send_address(const char *id, uint8_t addr, int recv, int success) "%s sending address: 0x%02x, recv: %d, success: %d"
+npcm7xx_smbus_send_byte(const char *id, uint8_t value, int success) "%s send byte: 0x%02x, success: %d"
+npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
+npcm7xx_smbus_stop(const char *id) "%s stopping"
+npcm7xx_smbus_nack(const char *id) "%s nacking"
diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
index f6227aa8aa..cea1bd1f62 100644
--- a/include/hw/arm/npcm7xx.h
+++ b/include/hw/arm/npcm7xx.h
@@ -20,6 +20,7 @@
 #include "hw/adc/npcm7xx_adc.h"
 #include "hw/cpu/a9mpcore.h"
 #include "hw/gpio/npcm7xx_gpio.h"
+#include "hw/i2c/npcm7xx_smbus.h"
 #include "hw/mem/npcm7xx_mc.h"
 #include "hw/misc/npcm7xx_clk.h"
 #include "hw/misc/npcm7xx_gcr.h"
@@ -85,6 +86,7 @@ typedef struct NPCM7xxState {
     NPCM7xxMCState      mc;
     NPCM7xxRNGState     rng;
     NPCM7xxGPIOState    gpio[8];
+    NPCM7xxSMBusState   smbus[16];
     EHCISysBusState     ehci;
     OHCISysBusState     ohci;
     NPCM7xxFIUState     fiu[2];
diff --git a/include/hw/i2c/npcm7xx_smbus.h b/include/hw/i2c/npcm7xx_smbus.h
new file mode 100644
index 0000000000..b9761a6993
--- /dev/null
+++ b/include/hw/i2c/npcm7xx_smbus.h
@@ -0,0 +1,88 @@
+/*
+ * Nuvoton NPCM7xx SMBus Module.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+#ifndef NPCM7XX_SMBUS_H
+#define NPCM7XX_SMBUS_H
+
+#include "exec/memory.h"
+#include "hw/i2c/i2c.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+/*
+ * Number of addresses this module contains. Do not change this without
+ * incrementing the version_id in the vmstate.
+ */
+#define NPCM7XX_SMBUS_NR_ADDRS 10
+
+typedef enum NPCM7xxSMBusStatus {
+    NPCM7XX_SMBUS_STATUS_IDLE,
+    NPCM7XX_SMBUS_STATUS_SENDING,
+    NPCM7XX_SMBUS_STATUS_RECEIVING,
+    NPCM7XX_SMBUS_STATUS_NEGACK,
+    NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE,
+    NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK,
+} NPCM7xxSMBusStatus;
+
+/*
+ * struct NPCM7xxSMBusState - System Management Bus device state.
+ * @bus: The underlying I2C Bus.
+ * @irq: GIC interrupt line to fire on events (if enabled).
+ * @sda: The serial data register.
+ * @st: The status register.
+ * @cst: The control status register.
+ * @cst2: The control status register 2.
+ * @cst3: The control status register 3.
+ * @ctl1: The control register 1.
+ * @ctl2: The control register 2.
+ * @ctl3: The control register 3.
+ * @ctl4: The control register 4.
+ * @ctl5: The control register 5.
+ * @addr: The SMBus module's own addresses on the I2C bus.
+ * @scllt: The SCL low time register.
+ * @sclht: The SCL high time register.
+ * @status: The current status of the SMBus.
+ */
+typedef struct NPCM7xxSMBusState {
+    SysBusDevice parent;
+
+    MemoryRegion iomem;
+
+    I2CBus      *bus;
+    qemu_irq     irq;
+
+    uint8_t      sda;
+    uint8_t      st;
+    uint8_t      cst;
+    uint8_t      cst2;
+    uint8_t      cst3;
+    uint8_t      ctl1;
+    uint8_t      ctl2;
+    uint8_t      ctl3;
+    uint8_t      ctl4;
+    uint8_t      ctl5;
+    uint8_t      addr[NPCM7XX_SMBUS_NR_ADDRS];
+
+    uint8_t      scllt;
+    uint8_t      sclht;
+
+    NPCM7xxSMBusStatus status;
+} NPCM7xxSMBusState;
+
+#define TYPE_NPCM7XX_SMBUS "npcm7xx-smbus"
+#define NPCM7XX_SMBUS(obj) OBJECT_CHECK(NPCM7xxSMBusState, (obj), \
+                                        TYPE_NPCM7XX_SMBUS)
+
+#endif /* NPCM7XX_SMBUS_H */
-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 1/6] hw/arm: Remove GPIO from unimplemented NPCM7XX wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-28 17:32   ` Peter Maydell
  2021-01-26 19:32 ` [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ wuhaotsh--- via
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

Add an I2C device tree for NPCM750 evaluation board.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx_boards.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 3fdd5cab01..2d82f48848 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -98,6 +98,20 @@ static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
     return NPCM7XX(obj);
 }
 
+static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num)
+{
+    g_assert(num < ARRAY_SIZE(soc->smbus));
+    return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus"));
+}
+
+static void npcm750_evb_i2c_init(NPCM7xxState *soc)
+{
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 0), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 2), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 6), "tmp105", 0x48);
+}
+
 static void npcm750_evb_init(MachineState *machine)
 {
     NPCM7xxState *soc;
@@ -108,6 +122,7 @@ static void npcm750_evb_init(MachineState *machine)
 
     npcm7xx_load_bootrom(machine, soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, 0));
+    npcm750_evb_i2c_init(soc);
     npcm7xx_load_kernel(machine, soc);
 }
 
-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
                   ` (2 preceding siblings ...)
  2021-01-26 19:32 ` [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-26 23:05   ` Corey Minyard
  2021-01-28 17:33   ` Peter Maydell
  2021-01-26 19:32 ` [PATCH 5/6] hw/i2c: Add a QTest for NPCM7XX SMBus Device wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode wuhaotsh--- via
  5 siblings, 2 replies; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

Add an I2C device tree for Quanta GSJ. We only included devices with
existing QEMU implementation, including AT24 EEPROM and temperature
sensors.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx_boards.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 2d82f48848..1418629e06 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -19,6 +19,7 @@
 #include "exec/address-spaces.h"
 #include "hw/arm/npcm7xx.h"
 #include "hw/core/cpu.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
@@ -112,6 +113,21 @@ static void npcm750_evb_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 6), "tmp105", 0x48);
 }
 
+static void quanta_gsj_i2c_init(NPCM7xxState *soc)
+{
+    uint8_t *eeprom_buf0 = g_malloc0(32 * 1024);
+    uint8_t *eeprom_buf1 = g_malloc0(32 * 1024);
+
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 2), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x48);
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x48);
+    smbus_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 9), 0x55, eeprom_buf0);
+    smbus_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 10), 0x55, eeprom_buf1);
+
+    /* TODO: Add addtional i2c devices. */
+}
+
 static void npcm750_evb_init(MachineState *machine)
 {
     NPCM7xxState *soc;
@@ -137,6 +153,7 @@ static void quanta_gsj_init(MachineState *machine)
     npcm7xx_load_bootrom(machine, soc);
     npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e",
                           drive_get(IF_MTD, 0, 0));
+    quanta_gsj_i2c_init(soc);
     npcm7xx_load_kernel(machine, soc);
 }
 
-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 5/6] hw/i2c: Add a QTest for NPCM7XX SMBus Device
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
                   ` (3 preceding siblings ...)
  2021-01-26 19:32 ` [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-26 19:32 ` [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode wuhaotsh--- via
  5 siblings, 0 replies; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

This patch adds a QTest for NPCM7XX SMBus's single byte mode. It sends a
byte to a device in the evaluation board, and verify the retrieved value
is equivalent to the sent value.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 tests/qtest/meson.build          |   1 +
 tests/qtest/npcm7xx_smbus-test.c | 352 +++++++++++++++++++++++++++++++
 2 files changed, 353 insertions(+)
 create mode 100644 tests/qtest/npcm7xx_smbus-test.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 16d04625b8..aa62d59817 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -138,6 +138,7 @@ qtests_npcm7xx = \
    'npcm7xx_gpio-test',
    'npcm7xx_pwm-test',
    'npcm7xx_rng-test',
+   'npcm7xx_smbus-test',
    'npcm7xx_timer-test',
    'npcm7xx_watchdog_timer-test']
 qtests_arm = \
diff --git a/tests/qtest/npcm7xx_smbus-test.c b/tests/qtest/npcm7xx_smbus-test.c
new file mode 100644
index 0000000000..4594b107df
--- /dev/null
+++ b/tests/qtest/npcm7xx_smbus-test.c
@@ -0,0 +1,352 @@
+/*
+ * QTests for Nuvoton NPCM7xx SMBus Modules.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bitops.h"
+#include "libqos/i2c.h"
+#include "libqos/libqtest.h"
+#include "hw/misc/tmp105_regs.h"
+
+#define NR_SMBUS_DEVICES    16
+#define SMBUS_ADDR(x)       (0xf0080000 + 0x1000 * (x))
+#define SMBUS_IRQ(x)        (64 + (x))
+
+#define EVB_DEVICE_ADDR     0x48
+#define INVALID_DEVICE_ADDR 0x01
+
+const int evb_bus_list[] = {0, 1, 2, 6};
+
+/* Offsets */
+enum CommonRegister {
+    OFFSET_SDA     = 0x0,
+    OFFSET_ST      = 0x2,
+    OFFSET_CST     = 0x4,
+    OFFSET_CTL1    = 0x6,
+    OFFSET_ADDR1   = 0x8,
+    OFFSET_CTL2    = 0xa,
+    OFFSET_ADDR2   = 0xc,
+    OFFSET_CTL3    = 0xe,
+    OFFSET_CST2    = 0x18,
+    OFFSET_CST3    = 0x19,
+};
+
+enum NPCM7xxSMBusBank0Register {
+    OFFSET_ADDR3   = 0x10,
+    OFFSET_ADDR7   = 0x11,
+    OFFSET_ADDR4   = 0x12,
+    OFFSET_ADDR8   = 0x13,
+    OFFSET_ADDR5   = 0x14,
+    OFFSET_ADDR9   = 0x15,
+    OFFSET_ADDR6   = 0x16,
+    OFFSET_ADDR10  = 0x17,
+    OFFSET_CTL4    = 0x1a,
+    OFFSET_CTL5    = 0x1b,
+    OFFSET_SCLLT   = 0x1c,
+    OFFSET_FIF_CTL = 0x1d,
+    OFFSET_SCLHT   = 0x1e,
+};
+
+enum NPCM7xxSMBusBank1Register {
+    OFFSET_FIF_CTS  = 0x10,
+    OFFSET_FAIR_PER = 0x11,
+    OFFSET_TXF_CTL  = 0x12,
+    OFFSET_T_OUT    = 0x14,
+    OFFSET_TXF_STS  = 0x1a,
+    OFFSET_RXF_STS  = 0x1c,
+    OFFSET_RXF_CTL  = 0x1e,
+};
+
+/* ST fields */
+#define ST_STP              BIT(7)
+#define ST_SDAST            BIT(6)
+#define ST_BER              BIT(5)
+#define ST_NEGACK           BIT(4)
+#define ST_STASTR           BIT(3)
+#define ST_NMATCH           BIT(2)
+#define ST_MODE             BIT(1)
+#define ST_XMIT             BIT(0)
+
+/* CST fields */
+#define CST_ARPMATCH        BIT(7)
+#define CST_MATCHAF         BIT(6)
+#define CST_TGSCL           BIT(5)
+#define CST_TSDA            BIT(4)
+#define CST_GCMATCH         BIT(3)
+#define CST_MATCH           BIT(2)
+#define CST_BB              BIT(1)
+#define CST_BUSY            BIT(0)
+
+/* CST2 fields */
+#define CST2_INSTTS         BIT(7)
+#define CST2_MATCH7F        BIT(6)
+#define CST2_MATCH6F        BIT(5)
+#define CST2_MATCH5F        BIT(4)
+#define CST2_MATCH4F        BIT(3)
+#define CST2_MATCH3F        BIT(2)
+#define CST2_MATCH2F        BIT(1)
+#define CST2_MATCH1F        BIT(0)
+
+/* CST3 fields */
+#define CST3_EO_BUSY        BIT(7)
+#define CST3_MATCH10F       BIT(2)
+#define CST3_MATCH9F        BIT(1)
+#define CST3_MATCH8F        BIT(0)
+
+/* CTL1 fields */
+#define CTL1_STASTRE        BIT(7)
+#define CTL1_NMINTE         BIT(6)
+#define CTL1_GCMEN          BIT(5)
+#define CTL1_ACK            BIT(4)
+#define CTL1_EOBINTE        BIT(3)
+#define CTL1_INTEN          BIT(2)
+#define CTL1_STOP           BIT(1)
+#define CTL1_START          BIT(0)
+
+/* CTL2 fields */
+#define CTL2_SCLFRQ(rv)     extract8((rv), 1, 6)
+#define CTL2_ENABLE         BIT(0)
+
+/* CTL3 fields */
+#define CTL3_SCL_LVL        BIT(7)
+#define CTL3_SDA_LVL        BIT(6)
+#define CTL3_BNK_SEL        BIT(5)
+#define CTL3_400K_MODE      BIT(4)
+#define CTL3_IDL_START      BIT(3)
+#define CTL3_ARPMEN         BIT(2)
+#define CTL3_SCLFRQ(rv)     extract8((rv), 0, 2)
+
+/* ADDR fields */
+#define ADDR_EN             BIT(7)
+#define ADDR_A(rv)          extract8((rv), 0, 6)
+
+
+static void check_running(QTestState *qts, uint64_t base_addr)
+{
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_CST) & CST_BUSY);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_CST) & CST_BB);
+}
+
+static void check_stopped(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t cst3;
+
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==, 0);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_CST) & CST_BUSY);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_CST) & CST_BB);
+
+    cst3 = qtest_readb(qts, base_addr + OFFSET_CST3);
+    g_assert_true(cst3 & CST3_EO_BUSY);
+    qtest_writeb(qts, base_addr + OFFSET_CST3, cst3);
+    cst3 = qtest_readb(qts, base_addr + OFFSET_CST3);
+    g_assert_false(cst3 & CST3_EO_BUSY);
+}
+
+static void enable_bus(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t ctl2 = qtest_readb(qts, base_addr + OFFSET_CTL2);
+
+    ctl2 |= CTL2_ENABLE;
+    qtest_writeb(qts, base_addr + OFFSET_CTL2, ctl2);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_CTL2) & CTL2_ENABLE);
+}
+
+static void disable_bus(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t ctl2 = qtest_readb(qts, base_addr + OFFSET_CTL2);
+
+    ctl2 &= ~CTL2_ENABLE;
+    qtest_writeb(qts, base_addr + OFFSET_CTL2, ctl2);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_CTL2) & CTL2_ENABLE);
+}
+
+static void start_transfer(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t ctl1;
+
+    ctl1 = CTL1_START | CTL1_INTEN | CTL1_STASTRE;
+    qtest_writeb(qts, base_addr + OFFSET_CTL1, ctl1);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_CTL1), ==,
+                    CTL1_INTEN | CTL1_STASTRE | CTL1_INTEN);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==,
+                    ST_MODE | ST_XMIT | ST_SDAST);
+    check_running(qts, base_addr);
+}
+
+static void stop_transfer(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t ctl1 = qtest_readb(qts, base_addr + OFFSET_CTL1);
+
+    ctl1 &= ~(CTL1_START | CTL1_ACK);
+    ctl1 |= CTL1_STOP | CTL1_INTEN | CTL1_EOBINTE;
+    qtest_writeb(qts, base_addr + OFFSET_CTL1, ctl1);
+    ctl1 = qtest_readb(qts, base_addr + OFFSET_CTL1);
+    g_assert_false(ctl1 & CTL1_STOP);
+}
+
+static void send_byte(QTestState *qts, uint64_t base_addr, uint8_t byte)
+{
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==,
+                    ST_MODE | ST_XMIT | ST_SDAST);
+    qtest_writeb(qts, base_addr + OFFSET_SDA, byte);
+}
+
+static uint8_t recv_byte(QTestState *qts, uint64_t base_addr)
+{
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==,
+                    ST_MODE | ST_SDAST);
+    return qtest_readb(qts, base_addr + OFFSET_SDA);
+}
+
+static void send_address(QTestState *qts, uint64_t base_addr, uint8_t addr,
+                         bool recv, bool valid)
+{
+    uint8_t encoded_addr = (addr << 1) | (recv ? 1 : 0);
+    uint8_t st;
+
+    qtest_writeb(qts, base_addr + OFFSET_SDA, encoded_addr);
+    st = qtest_readb(qts, base_addr + OFFSET_ST);
+
+    if (valid) {
+        if (recv) {
+            g_assert_cmphex(st, ==, ST_MODE | ST_SDAST | ST_STASTR);
+        } else {
+            g_assert_cmphex(st, ==, ST_MODE | ST_XMIT | ST_SDAST | ST_STASTR);
+        }
+
+        qtest_writeb(qts, base_addr + OFFSET_ST, ST_STASTR);
+        st = qtest_readb(qts, base_addr + OFFSET_ST);
+        if (recv) {
+            g_assert_cmphex(st, ==, ST_MODE | ST_SDAST);
+        } else {
+            g_assert_cmphex(st, ==, ST_MODE | ST_XMIT | ST_SDAST);
+        }
+    } else {
+        if (recv) {
+            g_assert_cmphex(st, ==, ST_MODE | ST_NEGACK);
+        } else {
+            g_assert_cmphex(st, ==, ST_MODE | ST_XMIT | ST_NEGACK);
+        }
+    }
+}
+
+static void send_nack(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t ctl1 = qtest_readb(qts, base_addr + OFFSET_CTL1);
+
+    ctl1 &= ~(CTL1_START | CTL1_STOP);
+    ctl1 |= CTL1_ACK | CTL1_INTEN;
+    qtest_writeb(qts, base_addr + OFFSET_CTL1, ctl1);
+}
+
+/* Check the SMBus's status is set correctly when disabled. */
+static void test_disable_bus(gconstpointer data)
+{
+    intptr_t index = (intptr_t)data;
+    uint64_t base_addr = SMBUS_ADDR(index);
+    QTestState *qts = qtest_init("-machine npcm750-evb");
+
+    disable_bus(qts, base_addr);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_CTL1), ==, 0);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==, 0);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_CST3) & CST3_EO_BUSY);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_CST), ==, 0);
+    qtest_quit(qts);
+}
+
+/* Check the SMBus returns a NACK for an invalid address. */
+static void test_invalid_addr(gconstpointer data)
+{
+    intptr_t index = (intptr_t)data;
+    uint64_t base_addr = SMBUS_ADDR(index);
+    int irq = SMBUS_IRQ(index);
+    QTestState *qts = qtest_init("-machine npcm750-evb");
+
+    qtest_irq_intercept_in(qts, "/machine/soc/a9mpcore/gic");
+    enable_bus(qts, base_addr);
+    g_assert_false(qtest_get_irq(qts, irq));
+    start_transfer(qts, base_addr);
+    send_address(qts, base_addr, INVALID_DEVICE_ADDR, false, false);
+    g_assert_true(qtest_get_irq(qts, irq));
+    stop_transfer(qts, base_addr);
+    check_running(qts, base_addr);
+    qtest_writeb(qts, base_addr + OFFSET_ST, ST_NEGACK);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_ST) & ST_NEGACK);
+    check_stopped(qts, base_addr);
+    qtest_quit(qts);
+}
+
+/* Check the SMBus can send and receive bytes to a device in single mode. */
+static void test_single_mode(gconstpointer data)
+{
+    intptr_t index = (intptr_t)data;
+    uint64_t base_addr = SMBUS_ADDR(index);
+    int irq = SMBUS_IRQ(index);
+    uint8_t value = 0x60;
+    QTestState *qts = qtest_init("-machine npcm750-evb");
+
+    qtest_irq_intercept_in(qts, "/machine/soc/a9mpcore/gic");
+    enable_bus(qts, base_addr);
+
+    /* Sending */
+    g_assert_false(qtest_get_irq(qts, irq));
+    start_transfer(qts, base_addr);
+    g_assert_true(qtest_get_irq(qts, irq));
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, false, true);
+    send_byte(qts, base_addr, TMP105_REG_CONFIG);
+    send_byte(qts, base_addr, value);
+    stop_transfer(qts, base_addr);
+    check_stopped(qts, base_addr);
+
+    /* Receiving */
+    start_transfer(qts, base_addr);
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, false, true);
+    send_byte(qts, base_addr, TMP105_REG_CONFIG);
+    start_transfer(qts, base_addr);
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, true, true);
+    send_nack(qts, base_addr);
+    stop_transfer(qts, base_addr);
+    check_running(qts, base_addr);
+    g_assert_cmphex(recv_byte(qts, base_addr), ==, value);
+    check_stopped(qts, base_addr);
+    qtest_quit(qts);
+}
+
+static void smbus_add_test(const char *name, int index, GTestDataFunc fn)
+{
+    g_autofree char *full_name = g_strdup_printf(
+            "npcm7xx_smbus[%d]/%s", index, name);
+    qtest_add_data_func(full_name, (void *)(intptr_t)index, fn);
+}
+#define add_test(name, td) smbus_add_test(#name, td, test_##name)
+
+int main(int argc, char **argv)
+{
+    int i;
+
+    g_test_init(&argc, &argv, NULL);
+    g_test_set_nonfatal_assertions();
+
+    for (i = 0; i < NR_SMBUS_DEVICES; ++i) {
+        add_test(disable_bus, i);
+        add_test(invalid_addr, i);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(evb_bus_list); ++i) {
+        add_test(single_mode, evb_bus_list[i]);
+    }
+
+    return g_test_run();
+}
-- 
2.30.0.365.g02bc693789-goog



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

* [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
                   ` (4 preceding siblings ...)
  2021-01-26 19:32 ` [PATCH 5/6] hw/i2c: Add a QTest for NPCM7XX SMBus Device wuhaotsh--- via
@ 2021-01-26 19:32 ` wuhaotsh--- via
  2021-01-26 23:47   ` Corey Minyard
  5 siblings, 1 reply; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-26 19:32 UTC (permalink / raw)
  To: peter.maydell
  Cc: venture, hskinnemoen, qemu-devel, wuhaotsh, kfting, qemu-arm,
	Avi.Fishman, dje

This patch implements the FIFO mode of the SMBus module. In FIFO, the
user transmits or receives at most 16 bytes at a time. The FIFO mode
allows the module to transmit large amount of data faster than single
byte mode.

Reviewed-by: Doug Evans<dje@google.com>
Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/i2c/npcm7xx_smbus.c           | 331 +++++++++++++++++++++++++++++--
 hw/i2c/trace-events              |   1 +
 include/hw/i2c/npcm7xx_smbus.h   |  25 +++
 tests/qtest/npcm7xx_smbus-test.c | 149 +++++++++++++-
 4 files changed, 490 insertions(+), 16 deletions(-)

diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
index e8a8fdbaff..19a9cdb179 100644
--- a/hw/i2c/npcm7xx_smbus.c
+++ b/hw/i2c/npcm7xx_smbus.c
@@ -27,7 +27,7 @@
 #include "trace.h"
 
 #define NPCM7XX_SMBUS_VERSION 1
-#define NPCM7XX_SMBUS_FIFO_EN 0
+#define NPCM7XX_SMBUS_FIFO_EN 1
 
 enum NPCM7xxSMBusCommonRegister {
     NPCM7XX_SMB_SDA     = 0x0,
@@ -132,10 +132,41 @@ enum NPCM7xxSMBusBank1Register {
 #define NPCM7XX_ADDR_EN             BIT(7)
 #define NPCM7XX_ADDR_A(rv)          extract8((rv), 0, 6)
 
+/* FIFO Mode Register Fields */
+/* FIF_CTL fields */
+#define NPCM7XX_SMBFIF_CTL_FIFO_EN          BIT(4)
+#define NPCM7XX_SMBFIF_CTL_FAIR_RDY_IE      BIT(2)
+#define NPCM7XX_SMBFIF_CTL_FAIR_RDY         BIT(1)
+#define NPCM7XX_SMBFIF_CTL_FAIR_BUSY        BIT(0)
+/* FIF_CTS fields */
+#define NPCM7XX_SMBFIF_CTS_STR              BIT(7)
+#define NPCM7XX_SMBFIF_CTS_CLR_FIFO         BIT(6)
+#define NPCM7XX_SMBFIF_CTS_RFTE_IE          BIT(3)
+#define NPCM7XX_SMBFIF_CTS_RXF_TXE          BIT(1)
+/* TXF_CTL fields */
+#define NPCM7XX_SMBTXF_CTL_THR_TXIE         BIT(6)
+#define NPCM7XX_SMBTXF_CTL_TX_THR(rv)       extract8((rv), 0, 5)
+/* T_OUT fields */
+#define NPCM7XX_SMBT_OUT_ST                 BIT(7)
+#define NPCM7XX_SMBT_OUT_IE                 BIT(6)
+#define NPCM7XX_SMBT_OUT_CLKDIV(rv)         extract8((rv), 0, 6)
+/* TXF_STS fields */
+#define NPCM7XX_SMBTXF_STS_TX_THST          BIT(6)
+#define NPCM7XX_SMBTXF_STS_TX_BYTES(rv)     extract8((rv), 0, 5)
+/* RXF_STS fields */
+#define NPCM7XX_SMBRXF_STS_RX_THST          BIT(6)
+#define NPCM7XX_SMBRXF_STS_RX_BYTES(rv)     extract8((rv), 0, 5)
+/* RXF_CTL fields */
+#define NPCM7XX_SMBRXF_CTL_THR_RXIE         BIT(6)
+#define NPCM7XX_SMBRXF_CTL_LAST             BIT(5)
+#define NPCM7XX_SMBRXF_CTL_RX_THR(rv)       extract8((rv), 0, 5)
+
 #define KEEP_OLD_BIT(o, n, b)       (((n) & (~(b))) | ((o) & (b)))
 #define WRITE_ONE_CLEAR(o, n, b)    ((n) & (b) ? (o) & (~(b)) : (o))
 
 #define NPCM7XX_SMBUS_ENABLED(s)    ((s)->ctl2 & NPCM7XX_SMBCTL2_ENABLE)
+#define NPCM7XX_SMBUS_FIFO_ENABLED(s) (NPCM7XX_SMBUS_FIFO_EN && \
+        (s)->fif_ctl & NPCM7XX_SMBFIF_CTL_FIFO_EN)
 
 /* Reset values */
 #define NPCM7XX_SMB_ST_INIT_VAL     0x00
@@ -150,6 +181,14 @@ enum NPCM7xxSMBusBank1Register {
 #define NPCM7XX_SMB_ADDR_INIT_VAL   0x00
 #define NPCM7XX_SMB_SCLLT_INIT_VAL  0x00
 #define NPCM7XX_SMB_SCLHT_INIT_VAL  0x00
+#define NPCM7XX_SMB_FIF_CTL_INIT_VAL 0x00
+#define NPCM7XX_SMB_FIF_CTS_INIT_VAL 0x00
+#define NPCM7XX_SMB_FAIR_PER_INIT_VAL 0x00
+#define NPCM7XX_SMB_TXF_CTL_INIT_VAL 0x00
+#define NPCM7XX_SMB_T_OUT_INIT_VAL 0x3f
+#define NPCM7XX_SMB_TXF_STS_INIT_VAL 0x00
+#define NPCM7XX_SMB_RXF_STS_INIT_VAL 0x00
+#define NPCM7XX_SMB_RXF_CTL_INIT_VAL 0x01
 
 static uint8_t npcm7xx_smbus_get_version(void)
 {
@@ -169,7 +208,13 @@ static void npcm7xx_smbus_update_irq(NPCM7xxSMBusState *s)
                    (s->ctl1 & NPCM7XX_SMBCTL1_STASTRE &&
                     s->st & NPCM7XX_SMBST_SDAST) ||
                    (s->ctl1 & NPCM7XX_SMBCTL1_EOBINTE &&
-                    s->cst3 & NPCM7XX_SMBCST3_EO_BUSY));
+                    s->cst3 & NPCM7XX_SMBCST3_EO_BUSY) ||
+                   (s->rxf_ctl & NPCM7XX_SMBRXF_CTL_THR_RXIE &&
+                    s->rxf_sts & NPCM7XX_SMBRXF_STS_RX_THST) ||
+                   (s->txf_ctl & NPCM7XX_SMBTXF_CTL_THR_TXIE &&
+                    s->txf_sts & NPCM7XX_SMBTXF_STS_TX_THST) ||
+                   (s->fif_cts & NPCM7XX_SMBFIF_CTS_RFTE_IE &&
+                    s->fif_cts & NPCM7XX_SMBFIF_CTS_RXF_TXE));
 
         if (level) {
             s->cst2 |= NPCM7XX_SMBCST2_INTSTS;
@@ -187,6 +232,13 @@ static void npcm7xx_smbus_nack(NPCM7xxSMBusState *s)
     s->status = NPCM7XX_SMBUS_STATUS_NEGACK;
 }
 
+static void npcm7xx_smbus_clear_buffer(NPCM7xxSMBusState *s)
+{
+    s->fif_cts &= ~NPCM7XX_SMBFIF_CTS_RXF_TXE;
+    s->txf_sts = 0;
+    s->rxf_sts = 0;
+}
+
 static void npcm7xx_smbus_send_byte(NPCM7xxSMBusState *s, uint8_t value)
 {
     int rv = i2c_send(s->bus, value);
@@ -195,6 +247,15 @@ static void npcm7xx_smbus_send_byte(NPCM7xxSMBusState *s, uint8_t value)
         npcm7xx_smbus_nack(s);
     } else {
         s->st |= NPCM7XX_SMBST_SDAST;
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            s->fif_cts |= NPCM7XX_SMBFIF_CTS_RXF_TXE;
+            if (NPCM7XX_SMBTXF_STS_TX_BYTES(s->txf_sts) ==
+                NPCM7XX_SMBTXF_CTL_TX_THR(s->txf_ctl)) {
+                s->txf_sts = NPCM7XX_SMBTXF_STS_TX_THST;
+            } else {
+                s->txf_sts = 0;
+            }
+        }
     }
     trace_npcm7xx_smbus_send_byte((DEVICE(s)->canonical_path), value, !rv);
     npcm7xx_smbus_update_irq(s);
@@ -213,6 +274,67 @@ static void npcm7xx_smbus_recv_byte(NPCM7xxSMBusState *s)
     npcm7xx_smbus_update_irq(s);
 }
 
+static void npcm7xx_smbus_recv_fifo(NPCM7xxSMBusState *s)
+{
+    uint8_t expected_bytes = NPCM7XX_SMBRXF_CTL_RX_THR(s->rxf_ctl);
+    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
+    uint8_t pos;
+
+    if (received_bytes == expected_bytes) {
+        return;
+    }
+
+    while (received_bytes < expected_bytes &&
+           received_bytes < NPCM7XX_SMBUS_FIFO_SIZE) {
+        pos = (s->rx_cur + received_bytes) % NPCM7XX_SMBUS_FIFO_SIZE;
+        s->rx_fifo[pos] = i2c_recv(s->bus);
+        trace_npcm7xx_smbus_recv_byte((DEVICE(s)->canonical_path),
+                                      s->rx_fifo[pos]);
+        ++received_bytes;
+    }
+
+    trace_npcm7xx_smbus_recv_fifo((DEVICE(s)->canonical_path),
+                                  received_bytes, expected_bytes);
+    s->rxf_sts = received_bytes;
+    if (unlikely(received_bytes < expected_bytes)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid rx_thr value: 0x%02x\n",
+                      DEVICE(s)->canonical_path, expected_bytes);
+        return;
+    }
+
+    s->rxf_sts |= NPCM7XX_SMBRXF_STS_RX_THST;
+    if (s->rxf_ctl & NPCM7XX_SMBRXF_CTL_LAST) {
+        trace_npcm7xx_smbus_nack(DEVICE(s)->canonical_path);
+        i2c_nack(s->bus);
+        s->rxf_ctl &= ~NPCM7XX_SMBRXF_CTL_LAST;
+    }
+    if (received_bytes == NPCM7XX_SMBUS_FIFO_SIZE) {
+        s->st |= NPCM7XX_SMBST_SDAST;
+        s->fif_cts |= NPCM7XX_SMBFIF_CTS_RXF_TXE;
+    } else if (!(s->rxf_ctl & NPCM7XX_SMBRXF_CTL_THR_RXIE)) {
+        s->st |= NPCM7XX_SMBST_SDAST;
+    } else {
+        s->st &= ~NPCM7XX_SMBST_SDAST;
+    }
+    npcm7xx_smbus_update_irq(s);
+}
+
+static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
+{
+    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
+
+    if (received_bytes == 0) {
+        npcm7xx_smbus_recv_fifo(s);
+        return;
+    }
+
+    s->sda = s->rx_fifo[s->rx_cur];
+    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
+    --s->rxf_sts;
+    npcm7xx_smbus_update_irq(s);
+}
+
 static void npcm7xx_smbus_start(NPCM7xxSMBusState *s)
 {
     /*
@@ -226,6 +348,9 @@ static void npcm7xx_smbus_start(NPCM7xxSMBusState *s)
     if (available) {
         s->st |= NPCM7XX_SMBST_MODE | NPCM7XX_SMBST_XMIT | NPCM7XX_SMBST_SDAST;
         s->cst |= NPCM7XX_SMBCST_BUSY;
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            s->fif_cts |= NPCM7XX_SMBFIF_CTS_RXF_TXE;
+        }
     } else {
         s->st &= ~NPCM7XX_SMBST_MODE;
         s->cst &= ~NPCM7XX_SMBCST_BUSY;
@@ -277,7 +402,15 @@ static void npcm7xx_smbus_send_address(NPCM7xxSMBusState *s, uint8_t value)
             s->st |= NPCM7XX_SMBST_SDAST;
         }
     } else if (recv) {
-        npcm7xx_smbus_recv_byte(s);
+        s->st |= NPCM7XX_SMBST_SDAST;
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            npcm7xx_smbus_recv_fifo(s);
+        } else {
+            npcm7xx_smbus_recv_byte(s);
+        }
+    } else if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+        s->st |= NPCM7XX_SMBST_SDAST;
+        s->fif_cts |= NPCM7XX_SMBFIF_CTS_RXF_TXE;
     }
     npcm7xx_smbus_update_irq(s);
 }
@@ -320,11 +453,31 @@ static uint8_t npcm7xx_smbus_read_sda(NPCM7xxSMBusState *s)
 
     switch (s->status) {
     case NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE:
-        npcm7xx_smbus_execute_stop(s);
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            if (NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) <= 1) {
+                npcm7xx_smbus_execute_stop(s);
+            }
+            if (NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) == 0) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "%s: read to SDA with an empty rx-fifo buffer, "
+                              "result undefined: %u\n",
+                              DEVICE(s)->canonical_path, s->sda);
+                break;
+            }
+            npcm7xx_smbus_read_byte_fifo(s);
+            value = s->sda;
+        } else {
+            npcm7xx_smbus_execute_stop(s);
+        }
         break;
 
     case NPCM7XX_SMBUS_STATUS_RECEIVING:
-        npcm7xx_smbus_recv_byte(s);
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            npcm7xx_smbus_read_byte_fifo(s);
+            value = s->sda;
+        } else {
+            npcm7xx_smbus_recv_byte(s);
+        }
         break;
 
     default:
@@ -370,8 +523,12 @@ static void npcm7xx_smbus_write_st(NPCM7xxSMBusState *s, uint8_t value)
     }
 
     if (value & NPCM7XX_SMBST_STASTR &&
-            s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
-        npcm7xx_smbus_recv_byte(s);
+        s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
+        if (NPCM7XX_SMBUS_FIFO_ENABLED(s)) {
+            npcm7xx_smbus_recv_fifo(s);
+        } else {
+            npcm7xx_smbus_recv_byte(s);
+        }
     }
 
     npcm7xx_smbus_update_irq(s);
@@ -417,6 +574,7 @@ static void npcm7xx_smbus_write_ctl2(NPCM7xxSMBusState *s, uint8_t value)
         s->st = 0;
         s->cst3 = s->cst3 & (~NPCM7XX_SMBCST3_EO_BUSY);
         s->cst = 0;
+        npcm7xx_smbus_clear_buffer(s);
     }
 }
 
@@ -429,6 +587,70 @@ static void npcm7xx_smbus_write_ctl3(NPCM7xxSMBusState *s, uint8_t value)
                             NPCM7XX_SMBCTL3_SCL_LVL | NPCM7XX_SMBCTL3_SDA_LVL);
 }
 
+static void npcm7xx_smbus_write_fif_ctl(NPCM7xxSMBusState *s, uint8_t value)
+{
+    uint8_t new_ctl = value;
+
+    new_ctl = KEEP_OLD_BIT(s->fif_ctl, new_ctl, NPCM7XX_SMBFIF_CTL_FAIR_RDY);
+    new_ctl = WRITE_ONE_CLEAR(new_ctl, value, NPCM7XX_SMBFIF_CTL_FAIR_RDY);
+    new_ctl = KEEP_OLD_BIT(s->fif_ctl, new_ctl, NPCM7XX_SMBFIF_CTL_FAIR_BUSY);
+    s->fif_ctl = new_ctl;
+}
+
+static void npcm7xx_smbus_write_fif_cts(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->fif_cts = WRITE_ONE_CLEAR(s->fif_cts, value, NPCM7XX_SMBFIF_CTS_STR);
+    s->fif_cts = WRITE_ONE_CLEAR(s->fif_cts, value, NPCM7XX_SMBFIF_CTS_RXF_TXE);
+    s->fif_cts = KEEP_OLD_BIT(value, s->fif_cts, NPCM7XX_SMBFIF_CTS_RFTE_IE);
+
+    if (value & NPCM7XX_SMBFIF_CTS_CLR_FIFO) {
+        npcm7xx_smbus_clear_buffer(s);
+    }
+}
+
+static void npcm7xx_smbus_write_txf_ctl(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->txf_ctl = value;
+}
+
+static void npcm7xx_smbus_write_t_out(NPCM7xxSMBusState *s, uint8_t value)
+{
+    uint8_t new_t_out = value;
+
+    if ((value & NPCM7XX_SMBT_OUT_ST) || (!(s->t_out & NPCM7XX_SMBT_OUT_ST))) {
+        new_t_out &= ~NPCM7XX_SMBT_OUT_ST;
+    } else {
+        new_t_out |= NPCM7XX_SMBT_OUT_ST;
+    }
+
+    s->t_out = new_t_out;
+}
+
+static void npcm7xx_smbus_write_txf_sts(NPCM7xxSMBusState *s, uint8_t value)
+{
+    s->txf_sts = WRITE_ONE_CLEAR(s->txf_sts, value, NPCM7XX_SMBTXF_STS_TX_THST);
+}
+
+static void npcm7xx_smbus_write_rxf_sts(NPCM7xxSMBusState *s, uint8_t value)
+{
+    if (value & NPCM7XX_SMBRXF_STS_RX_THST) {
+        s->rxf_sts &= ~NPCM7XX_SMBRXF_STS_RX_THST;
+        if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
+            npcm7xx_smbus_recv_fifo(s);
+        }
+    }
+}
+
+static void npcm7xx_smbus_write_rxf_ctl(NPCM7xxSMBusState *s, uint8_t value)
+{
+    uint8_t new_ctl = value;
+
+    if (!(value & NPCM7XX_SMBRXF_CTL_LAST)) {
+        new_ctl = KEEP_OLD_BIT(s->rxf_ctl, new_ctl, NPCM7XX_SMBRXF_CTL_LAST);
+    }
+    s->rxf_ctl = new_ctl;
+}
+
 static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
 {
     NPCM7xxSMBusState *s = opaque;
@@ -484,9 +706,41 @@ static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
     default:
         if (bank) {
             /* Bank 1 */
-            qemu_log_mask(LOG_GUEST_ERROR,
-                    "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
-                    DEVICE(s)->canonical_path, offset);
+            switch (offset) {
+            case NPCM7XX_SMB_FIF_CTS:
+                value = s->fif_cts;
+                break;
+
+            case NPCM7XX_SMB_FAIR_PER:
+                value = s->fair_per;
+                break;
+
+            case NPCM7XX_SMB_TXF_CTL:
+                value = s->txf_ctl;
+                break;
+
+            case NPCM7XX_SMB_T_OUT:
+                value = s->t_out;
+                break;
+
+            case NPCM7XX_SMB_TXF_STS:
+                value = s->txf_sts;
+                break;
+
+            case NPCM7XX_SMB_RXF_STS:
+                value = s->rxf_sts;
+                break;
+
+            case NPCM7XX_SMB_RXF_CTL:
+                value = s->rxf_ctl;
+                break;
+
+            default:
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
+                        DEVICE(s)->canonical_path, offset);
+                break;
+            }
         } else {
             /* Bank 0 */
             switch (offset) {
@@ -534,6 +788,10 @@ static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
                 value = s->scllt;
                 break;
 
+            case NPCM7XX_SMB_FIF_CTL:
+                value = s->fif_ctl;
+                break;
+
             case NPCM7XX_SMB_SCLHT:
                 value = s->sclht;
                 break;
@@ -614,9 +872,41 @@ static void npcm7xx_smbus_write(void *opaque, hwaddr offset, uint64_t value,
     default:
         if (bank) {
             /* Bank 1 */
-            qemu_log_mask(LOG_GUEST_ERROR,
-                    "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
-                    DEVICE(s)->canonical_path, offset);
+            switch (offset) {
+            case NPCM7XX_SMB_FIF_CTS:
+                npcm7xx_smbus_write_fif_cts(s, value);
+                break;
+
+            case NPCM7XX_SMB_FAIR_PER:
+                s->fair_per = value;
+                break;
+
+            case NPCM7XX_SMB_TXF_CTL:
+                npcm7xx_smbus_write_txf_ctl(s, value);
+                break;
+
+            case NPCM7XX_SMB_T_OUT:
+                npcm7xx_smbus_write_t_out(s, value);
+                break;
+
+            case NPCM7XX_SMB_TXF_STS:
+                npcm7xx_smbus_write_txf_sts(s, value);
+                break;
+
+            case NPCM7XX_SMB_RXF_STS:
+                npcm7xx_smbus_write_rxf_sts(s, value);
+                break;
+
+            case NPCM7XX_SMB_RXF_CTL:
+                npcm7xx_smbus_write_rxf_ctl(s, value);
+                break;
+
+            default:
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
+                        DEVICE(s)->canonical_path, offset);
+                break;
+            }
         } else {
             /* Bank 0 */
             switch (offset) {
@@ -664,6 +954,10 @@ static void npcm7xx_smbus_write(void *opaque, hwaddr offset, uint64_t value,
                 s->scllt = value;
                 break;
 
+            case NPCM7XX_SMB_FIF_CTL:
+                npcm7xx_smbus_write_fif_ctl(s, value);
+                break;
+
             case NPCM7XX_SMB_SCLHT:
                 s->sclht = value;
                 break;
@@ -710,7 +1004,18 @@ static void npcm7xx_smbus_enter_reset(Object *obj, ResetType type)
     s->scllt = NPCM7XX_SMB_SCLLT_INIT_VAL;
     s->sclht = NPCM7XX_SMB_SCLHT_INIT_VAL;
 
+    s->fif_ctl = NPCM7XX_SMB_FIF_CTL_INIT_VAL;
+    s->fif_cts = NPCM7XX_SMB_FIF_CTS_INIT_VAL;
+    s->fair_per = NPCM7XX_SMB_FAIR_PER_INIT_VAL;
+    s->txf_ctl = NPCM7XX_SMB_TXF_CTL_INIT_VAL;
+    s->t_out = NPCM7XX_SMB_T_OUT_INIT_VAL;
+    s->txf_sts = NPCM7XX_SMB_TXF_STS_INIT_VAL;
+    s->rxf_sts = NPCM7XX_SMB_RXF_STS_INIT_VAL;
+    s->rxf_ctl = NPCM7XX_SMB_RXF_CTL_INIT_VAL;
+
+    npcm7xx_smbus_clear_buffer(s);
     s->status = NPCM7XX_SMBUS_STATUS_IDLE;
+    s->rx_cur = 0;
 }
 
 static void npcm7xx_smbus_hold_reset(Object *obj)
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index c3bb70ad04..82fe6f965f 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -25,3 +25,4 @@ npcm7xx_smbus_send_byte(const char *id, uint8_t value, int success) "%s send byt
 npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
 npcm7xx_smbus_stop(const char *id) "%s stopping"
 npcm7xx_smbus_nack(const char *id) "%s nacking"
+npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s recv fifo: received %u, expected %u"
diff --git a/include/hw/i2c/npcm7xx_smbus.h b/include/hw/i2c/npcm7xx_smbus.h
index b9761a6993..7d59ee917e 100644
--- a/include/hw/i2c/npcm7xx_smbus.h
+++ b/include/hw/i2c/npcm7xx_smbus.h
@@ -27,6 +27,9 @@
  */
 #define NPCM7XX_SMBUS_NR_ADDRS 10
 
+/* Size of the FIFO buffer. */
+#define NPCM7XX_SMBUS_FIFO_SIZE 16
+
 typedef enum NPCM7xxSMBusStatus {
     NPCM7XX_SMBUS_STATUS_IDLE,
     NPCM7XX_SMBUS_STATUS_SENDING,
@@ -53,6 +56,16 @@ typedef enum NPCM7xxSMBusStatus {
  * @addr: The SMBus module's own addresses on the I2C bus.
  * @scllt: The SCL low time register.
  * @sclht: The SCL high time register.
+ * @fif_ctl: The FIFO control register.
+ * @fif_cts: The FIFO control status register.
+ * @fair_per: The fair preriod register.
+ * @txf_ctl: The transmit FIFO control register.
+ * @t_out: The SMBus timeout register.
+ * @txf_sts: The transmit FIFO status register.
+ * @rxf_sts: The receive FIFO status register.
+ * @rxf_ctl: The receive FIFO control register.
+ * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
+ * @rx_cur: The current position of rx_fifo.
  * @status: The current status of the SMBus.
  */
 typedef struct NPCM7xxSMBusState {
@@ -78,6 +91,18 @@ typedef struct NPCM7xxSMBusState {
     uint8_t      scllt;
     uint8_t      sclht;
 
+    uint8_t      fif_ctl;
+    uint8_t      fif_cts;
+    uint8_t      fair_per;
+    uint8_t      txf_ctl;
+    uint8_t      t_out;
+    uint8_t      txf_sts;
+    uint8_t      rxf_sts;
+    uint8_t      rxf_ctl;
+
+    uint8_t      rx_fifo[NPCM7XX_SMBUS_FIFO_SIZE];
+    uint8_t      rx_cur;
+
     NPCM7xxSMBusStatus status;
 } NPCM7xxSMBusState;
 
diff --git a/tests/qtest/npcm7xx_smbus-test.c b/tests/qtest/npcm7xx_smbus-test.c
index 4594b107df..4f9f493872 100644
--- a/tests/qtest/npcm7xx_smbus-test.c
+++ b/tests/qtest/npcm7xx_smbus-test.c
@@ -132,6 +132,44 @@ enum NPCM7xxSMBusBank1Register {
 #define ADDR_EN             BIT(7)
 #define ADDR_A(rv)          extract8((rv), 0, 6)
 
+/* FIF_CTL fields */
+#define FIF_CTL_FIFO_EN         BIT(4)
+
+/* FIF_CTS fields */
+#define FIF_CTS_CLR_FIFO        BIT(6)
+#define FIF_CTS_RFTE_IE         BIT(3)
+#define FIF_CTS_RXF_TXE         BIT(1)
+
+/* TXF_CTL fields */
+#define TXF_CTL_THR_TXIE        BIT(6)
+#define TXF_CTL_TX_THR(rv)      extract8((rv), 0, 5)
+
+/* TXF_STS fields */
+#define TXF_STS_TX_THST         BIT(6)
+#define TXF_STS_TX_BYTES(rv)    extract8((rv), 0, 5)
+
+/* RXF_CTL fields */
+#define RXF_CTL_THR_RXIE        BIT(6)
+#define RXF_CTL_LAST            BIT(5)
+#define RXF_CTL_RX_THR(rv)      extract8((rv), 0, 5)
+
+/* RXF_STS fields */
+#define RXF_STS_RX_THST         BIT(6)
+#define RXF_STS_RX_BYTES(rv)    extract8((rv), 0, 5)
+
+
+static void choose_bank(QTestState *qts, uint64_t base_addr, uint8_t bank)
+{
+    uint8_t ctl3 = qtest_readb(qts, base_addr + OFFSET_CTL3);
+
+    if (bank) {
+        ctl3 |= CTL3_BNK_SEL;
+    } else {
+        ctl3 &= ~CTL3_BNK_SEL;
+    }
+
+    qtest_writeb(qts, base_addr + OFFSET_CTL3, ctl3);
+}
 
 static void check_running(QTestState *qts, uint64_t base_addr)
 {
@@ -203,10 +241,33 @@ static void send_byte(QTestState *qts, uint64_t base_addr, uint8_t byte)
     qtest_writeb(qts, base_addr + OFFSET_SDA, byte);
 }
 
+static bool check_recv(QTestState *qts, uint64_t base_addr)
+{
+    uint8_t st, fif_ctl, rxf_ctl, rxf_sts;
+    bool fifo;
+
+    st = qtest_readb(qts, base_addr + OFFSET_ST);
+    choose_bank(qts, base_addr, 0);
+    fif_ctl = qtest_readb(qts, base_addr + OFFSET_FIF_CTL);
+    fifo = fif_ctl & FIF_CTL_FIFO_EN;
+    if (!fifo) {
+        return st == (ST_MODE | ST_SDAST);
+    }
+
+    choose_bank(qts, base_addr, 1);
+    rxf_ctl = qtest_readb(qts, base_addr + OFFSET_RXF_CTL);
+    rxf_sts = qtest_readb(qts, base_addr + OFFSET_RXF_STS);
+
+    if ((rxf_ctl & RXF_CTL_THR_RXIE) && RXF_STS_RX_BYTES(rxf_sts) < 16) {
+        return st == ST_MODE;
+    } else {
+        return st == (ST_MODE | ST_SDAST);
+    }
+}
+
 static uint8_t recv_byte(QTestState *qts, uint64_t base_addr)
 {
-    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_ST), ==,
-                    ST_MODE | ST_SDAST);
+    g_assert_true(check_recv(qts, base_addr));
     return qtest_readb(qts, base_addr + OFFSET_SDA);
 }
 
@@ -229,7 +290,7 @@ static void send_address(QTestState *qts, uint64_t base_addr, uint8_t addr,
         qtest_writeb(qts, base_addr + OFFSET_ST, ST_STASTR);
         st = qtest_readb(qts, base_addr + OFFSET_ST);
         if (recv) {
-            g_assert_cmphex(st, ==, ST_MODE | ST_SDAST);
+            g_assert_true(check_recv(qts, base_addr));
         } else {
             g_assert_cmphex(st, ==, ST_MODE | ST_XMIT | ST_SDAST);
         }
@@ -251,6 +312,29 @@ static void send_nack(QTestState *qts, uint64_t base_addr)
     qtest_writeb(qts, base_addr + OFFSET_CTL1, ctl1);
 }
 
+static void start_fifo_mode(QTestState *qts, uint64_t base_addr)
+{
+    choose_bank(qts, base_addr, 0);
+    qtest_writeb(qts, base_addr + OFFSET_FIF_CTL, FIF_CTL_FIFO_EN);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_FIF_CTL) &
+                  FIF_CTL_FIFO_EN);
+    choose_bank(qts, base_addr, 1);
+    qtest_writeb(qts, base_addr + OFFSET_FIF_CTS,
+                 FIF_CTS_CLR_FIFO | FIF_CTS_RFTE_IE);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_FIF_CTS), ==,
+                    FIF_CTS_RFTE_IE);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_TXF_STS), ==, 0);
+    g_assert_cmphex(qtest_readb(qts, base_addr + OFFSET_RXF_STS), ==, 0);
+}
+
+static void start_recv_fifo(QTestState *qts, uint64_t base_addr, uint8_t bytes)
+{
+    choose_bank(qts, base_addr, 1);
+    qtest_writeb(qts, base_addr + OFFSET_TXF_CTL, 0);
+    qtest_writeb(qts, base_addr + OFFSET_RXF_CTL,
+                 RXF_CTL_THR_RXIE | RXF_CTL_LAST | bytes);
+}
+
 /* Check the SMBus's status is set correctly when disabled. */
 static void test_disable_bus(gconstpointer data)
 {
@@ -324,6 +408,64 @@ static void test_single_mode(gconstpointer data)
     qtest_quit(qts);
 }
 
+/* Check the SMBus can send and receive bytes in FIFO mode. */
+static void test_fifo_mode(gconstpointer data)
+{
+    intptr_t index = (intptr_t)data;
+    uint64_t base_addr = SMBUS_ADDR(index);
+    int irq = SMBUS_IRQ(index);
+    uint8_t value = 0x60;
+    QTestState *qts = qtest_init("-machine npcm750-evb");
+
+    qtest_irq_intercept_in(qts, "/machine/soc/a9mpcore/gic");
+    enable_bus(qts, base_addr);
+    start_fifo_mode(qts, base_addr);
+    g_assert_false(qtest_get_irq(qts, irq));
+
+    /* Sending */
+    start_transfer(qts, base_addr);
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, false, true);
+    choose_bank(qts, base_addr, 1);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_FIF_CTS) &
+                  FIF_CTS_RXF_TXE);
+    qtest_writeb(qts, base_addr + OFFSET_TXF_CTL, TXF_CTL_THR_TXIE);
+    send_byte(qts, base_addr, TMP105_REG_CONFIG);
+    send_byte(qts, base_addr, value);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_FIF_CTS) &
+                  FIF_CTS_RXF_TXE);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_TXF_STS) &
+                  TXF_STS_TX_THST);
+    g_assert_cmpuint(TXF_STS_TX_BYTES(
+                        qtest_readb(qts, base_addr + OFFSET_TXF_STS)), ==, 0);
+    g_assert_true(qtest_get_irq(qts, irq));
+    stop_transfer(qts, base_addr);
+    check_stopped(qts, base_addr);
+
+    /* Receiving */
+    start_fifo_mode(qts, base_addr);
+    start_transfer(qts, base_addr);
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, false, true);
+    send_byte(qts, base_addr, TMP105_REG_CONFIG);
+    start_transfer(qts, base_addr);
+    qtest_writeb(qts, base_addr + OFFSET_FIF_CTS, FIF_CTS_RXF_TXE);
+    start_recv_fifo(qts, base_addr, 1);
+    send_address(qts, base_addr, EVB_DEVICE_ADDR, true, true);
+    g_assert_false(qtest_readb(qts, base_addr + OFFSET_FIF_CTS) &
+                   FIF_CTS_RXF_TXE);
+    g_assert_true(qtest_readb(qts, base_addr + OFFSET_RXF_STS) &
+                  RXF_STS_RX_THST);
+    g_assert_cmpuint(RXF_STS_RX_BYTES(
+                        qtest_readb(qts, base_addr + OFFSET_RXF_STS)), ==, 1);
+    send_nack(qts, base_addr);
+    stop_transfer(qts, base_addr);
+    check_running(qts, base_addr);
+    g_assert_cmphex(recv_byte(qts, base_addr), ==, value);
+    g_assert_cmpuint(RXF_STS_RX_BYTES(
+                        qtest_readb(qts, base_addr + OFFSET_RXF_STS)), ==, 0);
+    check_stopped(qts, base_addr);
+    qtest_quit(qts);
+}
+
 static void smbus_add_test(const char *name, int index, GTestDataFunc fn)
 {
     g_autofree char *full_name = g_strdup_printf(
@@ -346,6 +488,7 @@ int main(int argc, char **argv)
 
     for (i = 0; i < ARRAY_SIZE(evb_bus_list); ++i) {
         add_test(single_mode, evb_bus_list[i]);
+        add_test(fifo_mode, evb_bus_list[i]);
     }
 
     return g_test_run();
-- 
2.30.0.365.g02bc693789-goog



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

* Re: [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode
  2021-01-26 19:32 ` [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode wuhaotsh--- via
@ 2021-01-26 23:00   ` Corey Minyard
  2021-01-28 17:28   ` Peter Maydell
  1 sibling, 0 replies; 18+ messages in thread
From: Corey Minyard @ 2021-01-26 23:00 UTC (permalink / raw)
  To: Hao Wu
  Cc: peter.maydell, venture, qemu-devel, hskinnemoen, kfting,
	qemu-arm, Avi.Fishman, dje

On Tue, Jan 26, 2021 at 11:32:33AM -0800, wuhaotsh--- via wrote:
> This commit implements the single-byte mode of the SMBus.
> 
> Each Nuvoton SoC has 16 System Management Bus (SMBus). These buses
> compliant with SMBus and I2C protocol.
> 
> This patch implements the single-byte mode of the SMBus. In this mode,
> the user sends or receives a byte each time. The SMBus device transmits
> it to the underlying i2c device and sends an interrupt back to the QEMU
> guest.

I don't see any functional issues with this.

The register order in the switch statements is rather confusing.  I see
that it's the order of the registers in memory, but it kind of threw me
at first.  That's ok, I think, though a comment might be welcome.

The VMStateDescription structure is not necessary, and is misleading,
as there's no support for VMState transfer on this system, and you
haven't put anything into it, anyway.  So you should probably remove
that.

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

-corey

> 
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  docs/system/arm/nuvoton.rst    |   2 +-
>  hw/arm/npcm7xx.c               |  68 ++-
>  hw/i2c/meson.build             |   1 +
>  hw/i2c/npcm7xx_smbus.c         | 766 +++++++++++++++++++++++++++++++++
>  hw/i2c/trace-events            |  11 +
>  include/hw/arm/npcm7xx.h       |   2 +
>  include/hw/i2c/npcm7xx_smbus.h |  88 ++++
>  7 files changed, 921 insertions(+), 17 deletions(-)
>  create mode 100644 hw/i2c/npcm7xx_smbus.c
>  create mode 100644 include/hw/i2c/npcm7xx_smbus.h
> 
> diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
> index a1786342e2..34fc799b2d 100644
> --- a/docs/system/arm/nuvoton.rst
> +++ b/docs/system/arm/nuvoton.rst
> @@ -43,6 +43,7 @@ Supported devices
>   * GPIO controller
>   * Analog to Digital Converter (ADC)
>   * Pulse Width Modulation (PWM)
> + * SMBus controller (SMBF)
>  
>  Missing devices
>  ---------------
> @@ -58,7 +59,6 @@ Missing devices
>  
>   * Ethernet controllers (GMAC and EMC)
>   * USB device (USBD)
> - * SMBus controller (SMBF)
>   * Peripheral SPI controller (PSPI)
>   * SD/MMC host
>   * PECI interface
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index d1fe9bd1df..8f596ffd69 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -104,6 +104,22 @@ enum NPCM7xxInterrupt {
>      NPCM7XX_OHCI_IRQ            = 62,
>      NPCM7XX_PWM0_IRQ            = 93,   /* PWM module 0 */
>      NPCM7XX_PWM1_IRQ,                   /* PWM module 1 */
> +    NPCM7XX_SMBUS0_IRQ          = 64,
> +    NPCM7XX_SMBUS1_IRQ,
> +    NPCM7XX_SMBUS2_IRQ,
> +    NPCM7XX_SMBUS3_IRQ,
> +    NPCM7XX_SMBUS4_IRQ,
> +    NPCM7XX_SMBUS5_IRQ,
> +    NPCM7XX_SMBUS6_IRQ,
> +    NPCM7XX_SMBUS7_IRQ,
> +    NPCM7XX_SMBUS8_IRQ,
> +    NPCM7XX_SMBUS9_IRQ,
> +    NPCM7XX_SMBUS10_IRQ,
> +    NPCM7XX_SMBUS11_IRQ,
> +    NPCM7XX_SMBUS12_IRQ,
> +    NPCM7XX_SMBUS13_IRQ,
> +    NPCM7XX_SMBUS14_IRQ,
> +    NPCM7XX_SMBUS15_IRQ,
>      NPCM7XX_GPIO0_IRQ           = 116,
>      NPCM7XX_GPIO1_IRQ,
>      NPCM7XX_GPIO2_IRQ,
> @@ -152,6 +168,26 @@ static const hwaddr npcm7xx_pwm_addr[] = {
>      0xf0104000,
>  };
>  
> +/* Direct memory-mapped access to each SMBus Module. */
> +static const hwaddr npcm7xx_smbus_addr[] = {
> +    0xf0080000,
> +    0xf0081000,
> +    0xf0082000,
> +    0xf0083000,
> +    0xf0084000,
> +    0xf0085000,
> +    0xf0086000,
> +    0xf0087000,
> +    0xf0088000,
> +    0xf0089000,
> +    0xf008a000,
> +    0xf008b000,
> +    0xf008c000,
> +    0xf008d000,
> +    0xf008e000,
> +    0xf008f000,
> +};
> +
>  static const struct {
>      hwaddr regs_addr;
>      uint32_t unconnected_pins;
> @@ -353,6 +389,11 @@ static void npcm7xx_init(Object *obj)
>          object_initialize_child(obj, "gpio[*]", &s->gpio[i], TYPE_NPCM7XX_GPIO);
>      }
>  
> +    for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
> +        object_initialize_child(obj, "smbus[*]", &s->smbus[i],
> +                                TYPE_NPCM7XX_SMBUS);
> +    }
> +
>      object_initialize_child(obj, "ehci", &s->ehci, TYPE_NPCM7XX_EHCI);
>      object_initialize_child(obj, "ohci", &s->ohci, TYPE_SYSBUS_OHCI);
>  
> @@ -509,6 +550,17 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>                             npcm7xx_irq(s, NPCM7XX_GPIO0_IRQ + i));
>      }
>  
> +    /* SMBus modules. Cannot fail. */
> +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_smbus_addr) != ARRAY_SIZE(s->smbus));
> +    for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
> +        Object *obj = OBJECT(&s->smbus[i]);
> +
> +        sysbus_realize(SYS_BUS_DEVICE(obj), &error_abort);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(obj), 0, npcm7xx_smbus_addr[i]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(obj), 0,
> +                           npcm7xx_irq(s, NPCM7XX_SMBUS0_IRQ + i));
> +    }
> +
>      /* USB Host */
>      object_property_set_bool(OBJECT(&s->ehci), "companion-enable", true,
>                               &error_abort);
> @@ -576,22 +628,6 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>      create_unimplemented_device("npcm7xx.pcierc",       0xe1000000,  64 * KiB);
>      create_unimplemented_device("npcm7xx.kcs",          0xf0007000,   4 * KiB);
>      create_unimplemented_device("npcm7xx.gfxi",         0xf000e000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[0]",     0xf0080000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[1]",     0xf0081000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[2]",     0xf0082000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[3]",     0xf0083000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[4]",     0xf0084000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[5]",     0xf0085000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[6]",     0xf0086000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[7]",     0xf0087000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[8]",     0xf0088000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[9]",     0xf0089000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[10]",    0xf008a000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[11]",    0xf008b000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[12]",    0xf008c000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[13]",    0xf008d000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[14]",    0xf008e000,   4 * KiB);
> -    create_unimplemented_device("npcm7xx.smbus[15]",    0xf008f000,   4 * KiB);
>      create_unimplemented_device("npcm7xx.espi",         0xf009f000,   4 * KiB);
>      create_unimplemented_device("npcm7xx.peci",         0xf0100000,   4 * KiB);
>      create_unimplemented_device("npcm7xx.siox[1]",      0xf0101000,   4 * KiB);
> diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
> index 3a511539ad..cdcd694a7f 100644
> --- a/hw/i2c/meson.build
> +++ b/hw/i2c/meson.build
> @@ -9,6 +9,7 @@ i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
> +i2c_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_smbus.c'))
>  i2c_ss.add(when: 'CONFIG_SMBUS_EEPROM', if_true: files('smbus_eeprom.c'))
>  i2c_ss.add(when: 'CONFIG_VERSATILE_I2C', if_true: files('versatile_i2c.c'))
>  i2c_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_i2c.c'))
> diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
> new file mode 100644
> index 0000000000..e8a8fdbaff
> --- /dev/null
> +++ b/hw/i2c/npcm7xx_smbus.c
> @@ -0,0 +1,766 @@
> +/*
> + * Nuvoton NPCM7xx SMBus Module.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/i2c/npcm7xx_smbus.h"
> +#include "migration/vmstate.h"
> +#include "qemu/bitops.h"
> +#include "qemu/guest-random.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +
> +#include "trace.h"
> +
> +#define NPCM7XX_SMBUS_VERSION 1
> +#define NPCM7XX_SMBUS_FIFO_EN 0
> +
> +enum NPCM7xxSMBusCommonRegister {
> +    NPCM7XX_SMB_SDA     = 0x0,
> +    NPCM7XX_SMB_ST      = 0x2,
> +    NPCM7XX_SMB_CST     = 0x4,
> +    NPCM7XX_SMB_CTL1    = 0x6,
> +    NPCM7XX_SMB_ADDR1   = 0x8,
> +    NPCM7XX_SMB_CTL2    = 0xa,
> +    NPCM7XX_SMB_ADDR2   = 0xc,
> +    NPCM7XX_SMB_CTL3    = 0xe,
> +    NPCM7XX_SMB_CST2    = 0x18,
> +    NPCM7XX_SMB_CST3    = 0x19,
> +    NPCM7XX_SMB_VER     = 0x1f,
> +};
> +
> +enum NPCM7xxSMBusBank0Register {
> +    NPCM7XX_SMB_ADDR3   = 0x10,
> +    NPCM7XX_SMB_ADDR7   = 0x11,
> +    NPCM7XX_SMB_ADDR4   = 0x12,
> +    NPCM7XX_SMB_ADDR8   = 0x13,
> +    NPCM7XX_SMB_ADDR5   = 0x14,
> +    NPCM7XX_SMB_ADDR9   = 0x15,
> +    NPCM7XX_SMB_ADDR6   = 0x16,
> +    NPCM7XX_SMB_ADDR10  = 0x17,
> +    NPCM7XX_SMB_CTL4    = 0x1a,
> +    NPCM7XX_SMB_CTL5    = 0x1b,
> +    NPCM7XX_SMB_SCLLT   = 0x1c,
> +    NPCM7XX_SMB_FIF_CTL = 0x1d,
> +    NPCM7XX_SMB_SCLHT   = 0x1e,
> +};
> +
> +enum NPCM7xxSMBusBank1Register {
> +    NPCM7XX_SMB_FIF_CTS  = 0x10,
> +    NPCM7XX_SMB_FAIR_PER = 0x11,
> +    NPCM7XX_SMB_TXF_CTL  = 0x12,
> +    NPCM7XX_SMB_T_OUT    = 0x14,
> +    NPCM7XX_SMB_TXF_STS  = 0x1a,
> +    NPCM7XX_SMB_RXF_STS  = 0x1c,
> +    NPCM7XX_SMB_RXF_CTL  = 0x1e,
> +};
> +
> +/* ST fields */
> +#define NPCM7XX_SMBST_STP           BIT(7)
> +#define NPCM7XX_SMBST_SDAST         BIT(6)
> +#define NPCM7XX_SMBST_BER           BIT(5)
> +#define NPCM7XX_SMBST_NEGACK        BIT(4)
> +#define NPCM7XX_SMBST_STASTR        BIT(3)
> +#define NPCM7XX_SMBST_NMATCH        BIT(2)
> +#define NPCM7XX_SMBST_MODE          BIT(1)
> +#define NPCM7XX_SMBST_XMIT          BIT(0)
> +
> +/* CST fields */
> +#define NPCM7XX_SMBCST_ARPMATCH        BIT(7)
> +#define NPCM7XX_SMBCST_MATCHAF         BIT(6)
> +#define NPCM7XX_SMBCST_TGSCL           BIT(5)
> +#define NPCM7XX_SMBCST_TSDA            BIT(4)
> +#define NPCM7XX_SMBCST_GCMATCH         BIT(3)
> +#define NPCM7XX_SMBCST_MATCH           BIT(2)
> +#define NPCM7XX_SMBCST_BB              BIT(1)
> +#define NPCM7XX_SMBCST_BUSY            BIT(0)
> +
> +/* CST2 fields */
> +#define NPCM7XX_SMBCST2_INTSTS         BIT(7)
> +#define NPCM7XX_SMBCST2_MATCH7F        BIT(6)
> +#define NPCM7XX_SMBCST2_MATCH6F        BIT(5)
> +#define NPCM7XX_SMBCST2_MATCH5F        BIT(4)
> +#define NPCM7XX_SMBCST2_MATCH4F        BIT(3)
> +#define NPCM7XX_SMBCST2_MATCH3F        BIT(2)
> +#define NPCM7XX_SMBCST2_MATCH2F        BIT(1)
> +#define NPCM7XX_SMBCST2_MATCH1F        BIT(0)
> +
> +/* CST3 fields */
> +#define NPCM7XX_SMBCST3_EO_BUSY        BIT(7)
> +#define NPCM7XX_SMBCST3_MATCH10F       BIT(2)
> +#define NPCM7XX_SMBCST3_MATCH9F        BIT(1)
> +#define NPCM7XX_SMBCST3_MATCH8F        BIT(0)
> +
> +/* CTL1 fields */
> +#define NPCM7XX_SMBCTL1_STASTRE     BIT(7)
> +#define NPCM7XX_SMBCTL1_NMINTE      BIT(6)
> +#define NPCM7XX_SMBCTL1_GCMEN       BIT(5)
> +#define NPCM7XX_SMBCTL1_ACK         BIT(4)
> +#define NPCM7XX_SMBCTL1_EOBINTE     BIT(3)
> +#define NPCM7XX_SMBCTL1_INTEN       BIT(2)
> +#define NPCM7XX_SMBCTL1_STOP        BIT(1)
> +#define NPCM7XX_SMBCTL1_START       BIT(0)
> +
> +/* CTL2 fields */
> +#define NPCM7XX_SMBCTL2_SCLFRQ(rv)  extract8((rv), 1, 6)
> +#define NPCM7XX_SMBCTL2_ENABLE      BIT(0)
> +
> +/* CTL3 fields */
> +#define NPCM7XX_SMBCTL3_SCL_LVL     BIT(7)
> +#define NPCM7XX_SMBCTL3_SDA_LVL     BIT(6)
> +#define NPCM7XX_SMBCTL3_BNK_SEL     BIT(5)
> +#define NPCM7XX_SMBCTL3_400K_MODE   BIT(4)
> +#define NPCM7XX_SMBCTL3_IDL_START   BIT(3)
> +#define NPCM7XX_SMBCTL3_ARPMEN      BIT(2)
> +#define NPCM7XX_SMBCTL3_SCLFRQ(rv)  extract8((rv), 0, 2)
> +
> +/* ADDR fields */
> +#define NPCM7XX_ADDR_EN             BIT(7)
> +#define NPCM7XX_ADDR_A(rv)          extract8((rv), 0, 6)
> +
> +#define KEEP_OLD_BIT(o, n, b)       (((n) & (~(b))) | ((o) & (b)))
> +#define WRITE_ONE_CLEAR(o, n, b)    ((n) & (b) ? (o) & (~(b)) : (o))
> +
> +#define NPCM7XX_SMBUS_ENABLED(s)    ((s)->ctl2 & NPCM7XX_SMBCTL2_ENABLE)
> +
> +/* Reset values */
> +#define NPCM7XX_SMB_ST_INIT_VAL     0x00
> +#define NPCM7XX_SMB_CST_INIT_VAL    0x10
> +#define NPCM7XX_SMB_CST2_INIT_VAL   0x00
> +#define NPCM7XX_SMB_CST3_INIT_VAL   0x00
> +#define NPCM7XX_SMB_CTL1_INIT_VAL   0x00
> +#define NPCM7XX_SMB_CTL2_INIT_VAL   0x00
> +#define NPCM7XX_SMB_CTL3_INIT_VAL   0xc0
> +#define NPCM7XX_SMB_CTL4_INIT_VAL   0x07
> +#define NPCM7XX_SMB_CTL5_INIT_VAL   0x00
> +#define NPCM7XX_SMB_ADDR_INIT_VAL   0x00
> +#define NPCM7XX_SMB_SCLLT_INIT_VAL  0x00
> +#define NPCM7XX_SMB_SCLHT_INIT_VAL  0x00
> +
> +static uint8_t npcm7xx_smbus_get_version(void)
> +{
> +    return NPCM7XX_SMBUS_FIFO_EN << 7 | NPCM7XX_SMBUS_VERSION;
> +}
> +
> +static void npcm7xx_smbus_update_irq(NPCM7xxSMBusState *s)
> +{
> +    int level;
> +
> +    if (s->ctl1 & NPCM7XX_SMBCTL1_INTEN) {
> +        level = !!((s->ctl1 & NPCM7XX_SMBCTL1_NMINTE &&
> +                    s->st & NPCM7XX_SMBST_NMATCH) ||
> +                   (s->st & NPCM7XX_SMBST_BER) ||
> +                   (s->st & NPCM7XX_SMBST_NEGACK) ||
> +                   (s->st & NPCM7XX_SMBST_SDAST) ||
> +                   (s->ctl1 & NPCM7XX_SMBCTL1_STASTRE &&
> +                    s->st & NPCM7XX_SMBST_SDAST) ||
> +                   (s->ctl1 & NPCM7XX_SMBCTL1_EOBINTE &&
> +                    s->cst3 & NPCM7XX_SMBCST3_EO_BUSY));
> +
> +        if (level) {
> +            s->cst2 |= NPCM7XX_SMBCST2_INTSTS;
> +        } else {
> +            s->cst2 &= ~NPCM7XX_SMBCST2_INTSTS;
> +        }
> +        qemu_set_irq(s->irq, level);
> +    }
> +}
> +
> +static void npcm7xx_smbus_nack(NPCM7xxSMBusState *s)
> +{
> +    s->st &= ~NPCM7XX_SMBST_SDAST;
> +    s->st |= NPCM7XX_SMBST_NEGACK;
> +    s->status = NPCM7XX_SMBUS_STATUS_NEGACK;
> +}
> +
> +static void npcm7xx_smbus_send_byte(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    int rv = i2c_send(s->bus, value);
> +
> +    if (rv) {
> +        npcm7xx_smbus_nack(s);
> +    } else {
> +        s->st |= NPCM7XX_SMBST_SDAST;
> +    }
> +    trace_npcm7xx_smbus_send_byte((DEVICE(s)->canonical_path), value, !rv);
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_recv_byte(NPCM7xxSMBusState *s)
> +{
> +    s->sda = i2c_recv(s->bus);
> +    s->st |= NPCM7XX_SMBST_SDAST;
> +    if (s->st & NPCM7XX_SMBCTL1_ACK) {
> +        trace_npcm7xx_smbus_nack(DEVICE(s)->canonical_path);
> +        i2c_nack(s->bus);
> +        s->st &= NPCM7XX_SMBCTL1_ACK;
> +    }
> +    trace_npcm7xx_smbus_recv_byte((DEVICE(s)->canonical_path), s->sda);
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_start(NPCM7xxSMBusState *s)
> +{
> +    /*
> +     * We can start the bus if one of these is true:
> +     * 1. The bus is idle (so we can request it)
> +     * 2. We are the occupier (it's a repeated start condition.)
> +     */
> +    int available = !i2c_bus_busy(s->bus) ||
> +                    s->status != NPCM7XX_SMBUS_STATUS_IDLE;
> +
> +    if (available) {
> +        s->st |= NPCM7XX_SMBST_MODE | NPCM7XX_SMBST_XMIT | NPCM7XX_SMBST_SDAST;
> +        s->cst |= NPCM7XX_SMBCST_BUSY;
> +    } else {
> +        s->st &= ~NPCM7XX_SMBST_MODE;
> +        s->cst &= ~NPCM7XX_SMBCST_BUSY;
> +        s->st |= NPCM7XX_SMBST_BER;
> +    }
> +
> +    trace_npcm7xx_smbus_start(DEVICE(s)->canonical_path, available);
> +    s->cst |= NPCM7XX_SMBCST_BB;
> +    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_send_address(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    int recv;
> +    int rv;
> +
> +    recv = value & BIT(0);
> +    rv = i2c_start_transfer(s->bus, value >> 1, recv);
> +    trace_npcm7xx_smbus_send_address(DEVICE(s)->canonical_path,
> +                                     value >> 1, recv, !rv);
> +    if (rv) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: requesting i2c bus for 0x%02x failed: %d\n",
> +                      DEVICE(s)->canonical_path, value, rv);
> +        /* Failed to start transfer. NACK to reject.*/
> +        if (recv) {
> +            s->st &= ~NPCM7XX_SMBST_XMIT;
> +        } else {
> +            s->st |= NPCM7XX_SMBST_XMIT;
> +        }
> +        npcm7xx_smbus_nack(s);
> +        npcm7xx_smbus_update_irq(s);
> +        return;
> +    }
> +
> +    s->st &= ~NPCM7XX_SMBST_NEGACK;
> +    if (recv) {
> +        s->status = NPCM7XX_SMBUS_STATUS_RECEIVING;
> +        s->st &= ~NPCM7XX_SMBST_XMIT;
> +    } else {
> +        s->status = NPCM7XX_SMBUS_STATUS_SENDING;
> +        s->st |= NPCM7XX_SMBST_XMIT;
> +    }
> +
> +    if (s->ctl1 & NPCM7XX_SMBCTL1_STASTRE) {
> +        s->st |= NPCM7XX_SMBST_STASTR;
> +        if (!recv) {
> +            s->st |= NPCM7XX_SMBST_SDAST;
> +        }
> +    } else if (recv) {
> +        npcm7xx_smbus_recv_byte(s);
> +    }
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_execute_stop(NPCM7xxSMBusState *s)
> +{
> +    i2c_end_transfer(s->bus);
> +    s->st = 0;
> +    s->cst = 0;
> +    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
> +    s->cst3 |= NPCM7XX_SMBCST3_EO_BUSY;
> +    trace_npcm7xx_smbus_stop(DEVICE(s)->canonical_path);
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +
> +static void npcm7xx_smbus_stop(NPCM7xxSMBusState *s)
> +{
> +    if (s->st & NPCM7XX_SMBST_MODE) {
> +        switch (s->status) {
> +        case NPCM7XX_SMBUS_STATUS_RECEIVING:
> +        case NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE:
> +            s->status = NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE;
> +            break;
> +
> +        case NPCM7XX_SMBUS_STATUS_NEGACK:
> +            s->status = NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK;
> +            break;
> +
> +        default:
> +            npcm7xx_smbus_execute_stop(s);
> +            break;
> +        }
> +    }
> +}
> +
> +static uint8_t npcm7xx_smbus_read_sda(NPCM7xxSMBusState *s)
> +{
> +    uint8_t value = s->sda;
> +
> +    switch (s->status) {
> +    case NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE:
> +        npcm7xx_smbus_execute_stop(s);
> +        break;
> +
> +    case NPCM7XX_SMBUS_STATUS_RECEIVING:
> +        npcm7xx_smbus_recv_byte(s);
> +        break;
> +
> +    default:
> +        /* Do nothing */
> +        break;
> +    }
> +
> +    return value;
> +}
> +
> +static void npcm7xx_smbus_write_sda(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    s->sda = value;
> +    if (s->st & NPCM7XX_SMBST_MODE) {
> +        switch (s->status) {
> +        case NPCM7XX_SMBUS_STATUS_IDLE:
> +            npcm7xx_smbus_send_address(s, value);
> +            break;
> +        case NPCM7XX_SMBUS_STATUS_SENDING:
> +            npcm7xx_smbus_send_byte(s, value);
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: write to SDA in invalid status %d: %u\n",
> +                          DEVICE(s)->canonical_path, s->status, value);
> +            break;
> +        }
> +    }
> +}
> +
> +static void npcm7xx_smbus_write_st(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_STP);
> +    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_BER);
> +    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_STASTR);
> +    s->st = WRITE_ONE_CLEAR(s->st, value, NPCM7XX_SMBST_NMATCH);
> +
> +    if (value & NPCM7XX_SMBST_NEGACK) {
> +        s->st &= ~NPCM7XX_SMBST_NEGACK;
> +        if (s->status == NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK) {
> +            npcm7xx_smbus_execute_stop(s);
> +        }
> +    }
> +
> +    if (value & NPCM7XX_SMBST_STASTR &&
> +            s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
> +        npcm7xx_smbus_recv_byte(s);
> +    }
> +
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_write_cst(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    uint8_t new_value = s->cst;
> +
> +    s->cst = WRITE_ONE_CLEAR(new_value, value, NPCM7XX_SMBCST_BB);
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_write_cst3(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    s->cst3 = WRITE_ONE_CLEAR(s->cst3, value, NPCM7XX_SMBCST3_EO_BUSY);
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_write_ctl1(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    s->ctl1 = KEEP_OLD_BIT(s->ctl1, value,
> +            NPCM7XX_SMBCTL1_START | NPCM7XX_SMBCTL1_STOP | NPCM7XX_SMBCTL1_ACK);
> +
> +    if (value & NPCM7XX_SMBCTL1_START) {
> +        npcm7xx_smbus_start(s);
> +    }
> +
> +    if (value & NPCM7XX_SMBCTL1_STOP) {
> +        npcm7xx_smbus_stop(s);
> +    }
> +
> +    npcm7xx_smbus_update_irq(s);
> +}
> +
> +static void npcm7xx_smbus_write_ctl2(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    s->ctl2 = value;
> +
> +    if (!NPCM7XX_SMBUS_ENABLED(s)) {
> +        /* Disable this SMBus module. */
> +        s->ctl1 = 0;
> +        s->st = 0;
> +        s->cst3 = s->cst3 & (~NPCM7XX_SMBCST3_EO_BUSY);
> +        s->cst = 0;
> +    }
> +}
> +
> +static void npcm7xx_smbus_write_ctl3(NPCM7xxSMBusState *s, uint8_t value)
> +{
> +    uint8_t old_ctl3 = s->ctl3;
> +
> +    /* Write to SDA and SCL bits are ignored. */
> +    s->ctl3 =  KEEP_OLD_BIT(old_ctl3, value,
> +                            NPCM7XX_SMBCTL3_SCL_LVL | NPCM7XX_SMBCTL3_SDA_LVL);
> +}
> +
> +static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    NPCM7xxSMBusState *s = opaque;
> +    uint64_t value = 0;
> +    uint8_t bank = s->ctl3 & NPCM7XX_SMBCTL3_BNK_SEL;
> +
> +    switch (offset) {
> +    case NPCM7XX_SMB_SDA:
> +        value = npcm7xx_smbus_read_sda(s);
> +        break;
> +
> +    case NPCM7XX_SMB_ST:
> +        value = s->st;
> +        break;
> +
> +    case NPCM7XX_SMB_CST:
> +        value = s->cst;
> +        break;
> +
> +    case NPCM7XX_SMB_CTL1:
> +        value = s->ctl1;
> +        break;
> +
> +    case NPCM7XX_SMB_ADDR1:
> +        value = s->addr[0];
> +        break;
> +
> +    case NPCM7XX_SMB_CTL2:
> +        value = s->ctl2;
> +        break;
> +
> +    case NPCM7XX_SMB_ADDR2:
> +        value = s->addr[1];
> +        break;
> +
> +    case NPCM7XX_SMB_CTL3:
> +        value = s->ctl3;
> +        break;
> +
> +    case NPCM7XX_SMB_CST2:
> +        value = s->cst2;
> +        break;
> +
> +    case NPCM7XX_SMB_CST3:
> +        value = s->cst3;
> +        break;
> +
> +    case NPCM7XX_SMB_VER:
> +        value = npcm7xx_smbus_get_version();
> +        break;
> +
> +    /* This register is either invalid or banked at this point. */
> +    default:
> +        if (bank) {
> +            /* Bank 1 */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
> +                    DEVICE(s)->canonical_path, offset);
> +        } else {
> +            /* Bank 0 */
> +            switch (offset) {
> +            case NPCM7XX_SMB_ADDR3:
> +                value = s->addr[2];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR7:
> +                value = s->addr[6];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR4:
> +                value = s->addr[3];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR8:
> +                value = s->addr[7];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR5:
> +                value = s->addr[4];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR9:
> +                value = s->addr[8];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR6:
> +                value = s->addr[5];
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR10:
> +                value = s->addr[9];
> +                break;
> +
> +            case NPCM7XX_SMB_CTL4:
> +                value = s->ctl4;
> +                break;
> +
> +            case NPCM7XX_SMB_CTL5:
> +                value = s->ctl5;
> +                break;
> +
> +            case NPCM7XX_SMB_SCLLT:
> +                value = s->scllt;
> +                break;
> +
> +            case NPCM7XX_SMB_SCLHT:
> +                value = s->sclht;
> +                break;
> +
> +            default:
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "%s: read from invalid offset 0x%" HWADDR_PRIx "\n",
> +                        DEVICE(s)->canonical_path, offset);
> +                break;
> +            }
> +        }
> +        break;
> +    }
> +
> +    trace_npcm7xx_smbus_read(DEVICE(s)->canonical_path, offset, value, size);
> +
> +    return value;
> +}
> +
> +static void npcm7xx_smbus_write(void *opaque, hwaddr offset, uint64_t value,
> +                              unsigned size)
> +{
> +    NPCM7xxSMBusState *s = opaque;
> +    uint8_t bank = s->ctl3 & NPCM7XX_SMBCTL3_BNK_SEL;
> +
> +    trace_npcm7xx_smbus_write(DEVICE(s)->canonical_path, offset, value, size);
> +
> +    switch (offset) {
> +    case NPCM7XX_SMB_SDA:
> +        npcm7xx_smbus_write_sda(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_ST:
> +        npcm7xx_smbus_write_st(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_CST:
> +        npcm7xx_smbus_write_cst(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_CTL1:
> +        npcm7xx_smbus_write_ctl1(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_ADDR1:
> +        s->addr[0] = value;
> +        break;
> +
> +    case NPCM7XX_SMB_CTL2:
> +        npcm7xx_smbus_write_ctl2(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_ADDR2:
> +        s->addr[1] = value;
> +        break;
> +
> +    case NPCM7XX_SMB_CTL3:
> +        npcm7xx_smbus_write_ctl3(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_CST2:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: write to read-only reg: offset 0x%" HWADDR_PRIx "\n",
> +                DEVICE(s)->canonical_path, offset);
> +        break;
> +
> +    case NPCM7XX_SMB_CST3:
> +        npcm7xx_smbus_write_cst3(s, value);
> +        break;
> +
> +    case NPCM7XX_SMB_VER:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: write to read-only reg: offset 0x%" HWADDR_PRIx "\n",
> +                DEVICE(s)->canonical_path, offset);
> +        break;
> +
> +    /* This register is either invalid or banked at this point. */
> +    default:
> +        if (bank) {
> +            /* Bank 1 */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
> +                    DEVICE(s)->canonical_path, offset);
> +        } else {
> +            /* Bank 0 */
> +            switch (offset) {
> +            case NPCM7XX_SMB_ADDR3:
> +                s->addr[2] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR7:
> +                s->addr[6] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR4:
> +                s->addr[3] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR8:
> +                s->addr[7] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR5:
> +                s->addr[4] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR9:
> +                s->addr[8] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR6:
> +                s->addr[5] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_ADDR10:
> +                s->addr[9] = value;
> +                break;
> +
> +            case NPCM7XX_SMB_CTL4:
> +                s->ctl4 = value;
> +                break;
> +
> +            case NPCM7XX_SMB_CTL5:
> +                s->ctl5 = value;
> +                break;
> +
> +            case NPCM7XX_SMB_SCLLT:
> +                s->scllt = value;
> +                break;
> +
> +            case NPCM7XX_SMB_SCLHT:
> +                s->sclht = value;
> +                break;
> +
> +            default:
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "%s: write to invalid offset 0x%" HWADDR_PRIx "\n",
> +                        DEVICE(s)->canonical_path, offset);
> +                break;
> +            }
> +        }
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps npcm7xx_smbus_ops = {
> +    .read = npcm7xx_smbus_read,
> +    .write = npcm7xx_smbus_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +        .unaligned = false,
> +    },
> +};
> +
> +static void npcm7xx_smbus_enter_reset(Object *obj, ResetType type)
> +{
> +    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
> +
> +    s->st = NPCM7XX_SMB_ST_INIT_VAL;
> +    s->cst = NPCM7XX_SMB_CST_INIT_VAL;
> +    s->cst2 = NPCM7XX_SMB_CST2_INIT_VAL;
> +    s->cst3 = NPCM7XX_SMB_CST3_INIT_VAL;
> +    s->ctl1 = NPCM7XX_SMB_CTL1_INIT_VAL;
> +    s->ctl2 = NPCM7XX_SMB_CTL2_INIT_VAL;
> +    s->ctl3 = NPCM7XX_SMB_CTL3_INIT_VAL;
> +    s->ctl4 = NPCM7XX_SMB_CTL4_INIT_VAL;
> +    s->ctl5 = NPCM7XX_SMB_CTL5_INIT_VAL;
> +
> +    for (int i = 0; i < NPCM7XX_SMBUS_NR_ADDRS; ++i) {
> +        s->addr[i] = NPCM7XX_SMB_ADDR_INIT_VAL;
> +    }
> +    s->scllt = NPCM7XX_SMB_SCLLT_INIT_VAL;
> +    s->sclht = NPCM7XX_SMB_SCLHT_INIT_VAL;
> +
> +    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
> +}
> +
> +static void npcm7xx_smbus_hold_reset(Object *obj)
> +{
> +    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
> +
> +    qemu_irq_lower(s->irq);
> +}
> +
> +static void npcm7xx_smbus_init(Object *obj)
> +{
> +    NPCM7xxSMBusState *s = NPCM7XX_SMBUS(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +    memory_region_init_io(&s->iomem, obj, &npcm7xx_smbus_ops, s,
> +                          "regs", 4 * KiB);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
> +    s->status = NPCM7XX_SMBUS_STATUS_IDLE;
> +}
> +
> +static const VMStateDescription vmstate_npcm7xx_smbus = {
> +    .name = "npcm7xx-smbus",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static void npcm7xx_smbus_class_init(ObjectClass *klass, void *data)
> +{
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "NPCM7xx System Management Bus";
> +    dc->vmsd = &vmstate_npcm7xx_smbus;
> +    rc->phases.enter = npcm7xx_smbus_enter_reset;
> +    rc->phases.hold = npcm7xx_smbus_hold_reset;
> +}
> +
> +static const TypeInfo npcm7xx_smbus_types[] = {
> +    {
> +        .name = TYPE_NPCM7XX_SMBUS,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(NPCM7xxSMBusState),
> +        .class_init = npcm7xx_smbus_class_init,
> +        .instance_init = npcm7xx_smbus_init,
> +    },
> +};
> +DEFINE_TYPES(npcm7xx_smbus_types);
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 08db8fa689..c3bb70ad04 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -14,3 +14,14 @@ aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t val
>  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"
>  aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv %d/%d 0x%02x"
> +
> +# npcm7xx_smbus.c
> +
> +npcm7xx_smbus_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
> +npcm7xx_smbus_write(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
> +npcm7xx_smbus_start(const char *id, int success) "%s starting, success: %d"
> +npcm7xx_smbus_send_address(const char *id, uint8_t addr, int recv, int success) "%s sending address: 0x%02x, recv: %d, success: %d"
> +npcm7xx_smbus_send_byte(const char *id, uint8_t value, int success) "%s send byte: 0x%02x, success: %d"
> +npcm7xx_smbus_recv_byte(const char *id, uint8_t value) "%s recv byte: 0x%02x"
> +npcm7xx_smbus_stop(const char *id) "%s stopping"
> +npcm7xx_smbus_nack(const char *id) "%s nacking"
> diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
> index f6227aa8aa..cea1bd1f62 100644
> --- a/include/hw/arm/npcm7xx.h
> +++ b/include/hw/arm/npcm7xx.h
> @@ -20,6 +20,7 @@
>  #include "hw/adc/npcm7xx_adc.h"
>  #include "hw/cpu/a9mpcore.h"
>  #include "hw/gpio/npcm7xx_gpio.h"
> +#include "hw/i2c/npcm7xx_smbus.h"
>  #include "hw/mem/npcm7xx_mc.h"
>  #include "hw/misc/npcm7xx_clk.h"
>  #include "hw/misc/npcm7xx_gcr.h"
> @@ -85,6 +86,7 @@ typedef struct NPCM7xxState {
>      NPCM7xxMCState      mc;
>      NPCM7xxRNGState     rng;
>      NPCM7xxGPIOState    gpio[8];
> +    NPCM7xxSMBusState   smbus[16];
>      EHCISysBusState     ehci;
>      OHCISysBusState     ohci;
>      NPCM7xxFIUState     fiu[2];
> diff --git a/include/hw/i2c/npcm7xx_smbus.h b/include/hw/i2c/npcm7xx_smbus.h
> new file mode 100644
> index 0000000000..b9761a6993
> --- /dev/null
> +++ b/include/hw/i2c/npcm7xx_smbus.h
> @@ -0,0 +1,88 @@
> +/*
> + * Nuvoton NPCM7xx SMBus Module.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +#ifndef NPCM7XX_SMBUS_H
> +#define NPCM7XX_SMBUS_H
> +
> +#include "exec/memory.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/irq.h"
> +#include "hw/sysbus.h"
> +
> +/*
> + * Number of addresses this module contains. Do not change this without
> + * incrementing the version_id in the vmstate.
> + */
> +#define NPCM7XX_SMBUS_NR_ADDRS 10
> +
> +typedef enum NPCM7xxSMBusStatus {
> +    NPCM7XX_SMBUS_STATUS_IDLE,
> +    NPCM7XX_SMBUS_STATUS_SENDING,
> +    NPCM7XX_SMBUS_STATUS_RECEIVING,
> +    NPCM7XX_SMBUS_STATUS_NEGACK,
> +    NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE,
> +    NPCM7XX_SMBUS_STATUS_STOPPING_NEGACK,
> +} NPCM7xxSMBusStatus;
> +
> +/*
> + * struct NPCM7xxSMBusState - System Management Bus device state.
> + * @bus: The underlying I2C Bus.
> + * @irq: GIC interrupt line to fire on events (if enabled).
> + * @sda: The serial data register.
> + * @st: The status register.
> + * @cst: The control status register.
> + * @cst2: The control status register 2.
> + * @cst3: The control status register 3.
> + * @ctl1: The control register 1.
> + * @ctl2: The control register 2.
> + * @ctl3: The control register 3.
> + * @ctl4: The control register 4.
> + * @ctl5: The control register 5.
> + * @addr: The SMBus module's own addresses on the I2C bus.
> + * @scllt: The SCL low time register.
> + * @sclht: The SCL high time register.
> + * @status: The current status of the SMBus.
> + */
> +typedef struct NPCM7xxSMBusState {
> +    SysBusDevice parent;
> +
> +    MemoryRegion iomem;
> +
> +    I2CBus      *bus;
> +    qemu_irq     irq;
> +
> +    uint8_t      sda;
> +    uint8_t      st;
> +    uint8_t      cst;
> +    uint8_t      cst2;
> +    uint8_t      cst3;
> +    uint8_t      ctl1;
> +    uint8_t      ctl2;
> +    uint8_t      ctl3;
> +    uint8_t      ctl4;
> +    uint8_t      ctl5;
> +    uint8_t      addr[NPCM7XX_SMBUS_NR_ADDRS];
> +
> +    uint8_t      scllt;
> +    uint8_t      sclht;
> +
> +    NPCM7xxSMBusStatus status;
> +} NPCM7xxSMBusState;
> +
> +#define TYPE_NPCM7XX_SMBUS "npcm7xx-smbus"
> +#define NPCM7XX_SMBUS(obj) OBJECT_CHECK(NPCM7xxSMBusState, (obj), \
> +                                        TYPE_NPCM7XX_SMBUS)
> +
> +#endif /* NPCM7XX_SMBUS_H */
> -- 
> 2.30.0.365.g02bc693789-goog
> 
> 


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

* Re: [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ
  2021-01-26 19:32 ` [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ wuhaotsh--- via
@ 2021-01-26 23:05   ` Corey Minyard
  2021-01-28 17:33   ` Peter Maydell
  1 sibling, 0 replies; 18+ messages in thread
From: Corey Minyard @ 2021-01-26 23:05 UTC (permalink / raw)
  To: Hao Wu
  Cc: peter.maydell, venture, qemu-devel, hskinnemoen, kfting,
	qemu-arm, Avi.Fishman, dje

On Tue, Jan 26, 2021 at 11:32:35AM -0800, wuhaotsh--- via wrote:
> Add an I2C device tree for Quanta GSJ. We only included devices with
> existing QEMU implementation, including AT24 EEPROM and temperature
> sensors.
> 
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  hw/arm/npcm7xx_boards.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index 2d82f48848..1418629e06 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -19,6 +19,7 @@
>  #include "exec/address-spaces.h"
>  #include "hw/arm/npcm7xx.h"
>  #include "hw/core/cpu.h"
> +#include "hw/i2c/smbus_eeprom.h"
>  #include "hw/loader.h"
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
> @@ -112,6 +113,21 @@ static void npcm750_evb_i2c_init(NPCM7xxState *soc)
>      i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 6), "tmp105", 0x48);
>  }
>  
> +static void quanta_gsj_i2c_init(NPCM7xxState *soc)
> +{
> +    uint8_t *eeprom_buf0 = g_malloc0(32 * 1024);
> +    uint8_t *eeprom_buf1 = g_malloc0(32 * 1024);

This is kind of pointless because the smbus eeprom is 256 bytes.

It would be nice to modify the smbus eeprom code to take different
sizes, if you want to submit a patch.

-corey

> +
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 2), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x48);
> +    smbus_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 9), 0x55, eeprom_buf0);
> +    smbus_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 10), 0x55, eeprom_buf1);
> +
> +    /* TODO: Add addtional i2c devices. */
> +}
> +
>  static void npcm750_evb_init(MachineState *machine)
>  {
>      NPCM7xxState *soc;
> @@ -137,6 +153,7 @@ static void quanta_gsj_init(MachineState *machine)
>      npcm7xx_load_bootrom(machine, soc);
>      npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e",
>                            drive_get(IF_MTD, 0, 0));
> +    quanta_gsj_i2c_init(soc);
>      npcm7xx_load_kernel(machine, soc);
>  }
>  
> -- 
> 2.30.0.365.g02bc693789-goog
> 
> 


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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-26 19:32 ` [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode wuhaotsh--- via
@ 2021-01-26 23:47   ` Corey Minyard
  2021-01-27 20:37     ` wuhaotsh--- via
  0 siblings, 1 reply; 18+ messages in thread
From: Corey Minyard @ 2021-01-26 23:47 UTC (permalink / raw)
  To: Hao Wu
  Cc: peter.maydell, venture, qemu-devel, hskinnemoen, kfting,
	qemu-arm, Avi.Fishman, dje

On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> +
> +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> +{
> +    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> +
> +    if (received_bytes == 0) {
> +        npcm7xx_smbus_recv_fifo(s);
> +        return;
> +    }
> +
> +    s->sda = s->rx_fifo[s->rx_cur];
> +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> +    --s->rxf_sts;

This open-coded decrement seems a little risky.  Are you sure in every
case that s->rxf_sts > 0?  There's no way what's running in the VM can
game this and cause a buffer overrun?  One caller to this function seems
to protect against this, and another does not.

Other than this, I didn't see any issues with this patch.

-corey


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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-26 23:47   ` Corey Minyard
@ 2021-01-27 20:37     ` wuhaotsh--- via
  2021-01-27 21:42       ` Corey Minyard
  0 siblings, 1 reply; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-27 20:37 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Peter Maydell, Patrick Venture, QEMU Developers,
	Havard Skinnemoen, CS20 KFTing, qemu-arm, IS20 Avi Fishman,
	Doug Evans

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

On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:

> On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > +
> > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > +{
> > +    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > +
> > +    if (received_bytes == 0) {
> > +        npcm7xx_smbus_recv_fifo(s);
> > +        return;
> > +    }
> > +
> > +    s->sda = s->rx_fifo[s->rx_cur];
> > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > +    --s->rxf_sts;
>
> This open-coded decrement seems a little risky.  Are you sure in every
> case that s->rxf_sts > 0?  There's no way what's running in the VM can
> game this and cause a buffer overrun?  One caller to this function seems
> to protect against this, and another does not.
>
s->rxf_sts is uint8_t so it's guaranteed to be >=0.
In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
also 0, so it'll take the if-branch and return without running --s->rxf_sts.
I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.

>
> Other than this, I didn't see any issues with this patch.
>
> -corey
>

[-- Attachment #2: Type: text/html, Size: 1874 bytes --]

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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-27 20:37     ` wuhaotsh--- via
@ 2021-01-27 21:42       ` Corey Minyard
  2021-01-27 21:59         ` wuhaotsh--- via
  0 siblings, 1 reply; 18+ messages in thread
From: Corey Minyard @ 2021-01-27 21:42 UTC (permalink / raw)
  To: Hao Wu
  Cc: Peter Maydell, Patrick Venture, Havard Skinnemoen,
	QEMU Developers, CS20 KFTing, qemu-arm, IS20 Avi Fishman,
	Doug Evans

On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:
> 
> > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > +
> > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > +{
> > > +    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > +
> > > +    if (received_bytes == 0) {
> > > +        npcm7xx_smbus_recv_fifo(s);
> > > +        return;
> > > +    }
> > > +
> > > +    s->sda = s->rx_fifo[s->rx_cur];
> > > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > +    --s->rxf_sts;
> >
> > This open-coded decrement seems a little risky.  Are you sure in every
> > case that s->rxf_sts > 0?  There's no way what's running in the VM can
> > game this and cause a buffer overrun?  One caller to this function seems
> > to protect against this, and another does not.
> >
> s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> also 0, so it'll take the if-branch and return without running --s->rxf_sts.

That is true if called from the
NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case.  There is no such check
in the NPCM7XX_SMBUS_STATUS_RECEIVING case.

> I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.

You never want to do an assert if the hosted system can do something to
cause it.  If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
case, it would be ok, but really unnecessary.

If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
but you want to add a comment to that effect if so.  These sorts of
things look dangerous.

There is also the question about who takes these patches in.  I'm the
I2C maintainer, but there's other code in this series.  Once everything
is ready, I can ack them if we take it through the ARM tree.  Or I can
take it through my tree with the proper acks.

-corey

> 
> >
> > Other than this, I didn't see any issues with this patch.
> >
> > -corey
> >


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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-27 21:42       ` Corey Minyard
@ 2021-01-27 21:59         ` wuhaotsh--- via
  2021-01-27 23:37           ` Corey Minyard
  0 siblings, 1 reply; 18+ messages in thread
From: wuhaotsh--- via @ 2021-01-27 21:59 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Peter Maydell, Patrick Venture, Havard Skinnemoen,
	QEMU Developers, CS20 KFTing, qemu-arm, IS20 Avi Fishman,
	Doug Evans

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

On Wed, Jan 27, 2021 at 1:42 PM Corey Minyard <minyard@acm.org> wrote:

> On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> > On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:
> >
> > > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > > +
> > > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > > +{
> > > > +    uint8_t received_bytes =
> NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > > +
> > > > +    if (received_bytes == 0) {
> > > > +        npcm7xx_smbus_recv_fifo(s);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s->sda = s->rx_fifo[s->rx_cur];
> > > > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > > +    --s->rxf_sts;
> > >
> > > This open-coded decrement seems a little risky.  Are you sure in every
> > > case that s->rxf_sts > 0?  There's no way what's running in the VM can
> > > game this and cause a buffer overrun?  One caller to this function
> seems
> > > to protect against this, and another does not.
> > >
> > s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> > In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> > also 0, so it'll take the if-branch and return without running
> --s->rxf_sts.
>
> That is true if called from the
> NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case.  There is no such check
> in the NPCM7XX_SMBUS_STATUS_RECEIVING case.
>
I don't understand the reasoning here. The caller doesn't matter.
Previous code has:
 #define NPCM7XX_SMBRXF_STS_RX_BYTES(rv)     extract8((rv), 0, 5)
So
 uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
is guaranteed to be 0 if s->rxf_sts == 0.
As a result the code will take the following branch and returns:
 if (received_bytes == 0) {
    npcm7xx_smbus_recv_fifo(s);
    return;
 }
And will not execute the --s->rxf_sts sentence.
Please let me know if I missed anything here.

>
> > I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.
>
> You never want to do an assert if the hosted system can do something to
> cause it.  If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
> case, it would be ok, but really unnecessary.
>
> If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
> but you want to add a comment to that effect if so.  These sorts of
> things look dangerous.
>
> There is also the question about who takes these patches in.  I'm the
> I2C maintainer, but there's other code in this series.  Once everything
> is ready, I can ack them if we take it through the ARM tree.  Or I can
> take it through my tree with the proper acks.
>
I think either  way is fine. Previous NPCM7XX patch series were taken in
the ARM tree. But as i2c code taking into your tree is also fine.

>
> -corey
>
> >
> > >
> > > Other than this, I didn't see any issues with this patch.
> > >
> > > -corey
> > >
>

[-- Attachment #2: Type: text/html, Size: 4175 bytes --]

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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-27 21:59         ` wuhaotsh--- via
@ 2021-01-27 23:37           ` Corey Minyard
  2021-01-28  5:36             ` Corey Minyard
  0 siblings, 1 reply; 18+ messages in thread
From: Corey Minyard @ 2021-01-27 23:37 UTC (permalink / raw)
  To: Hao Wu
  Cc: Peter Maydell, Patrick Venture, Havard Skinnemoen,
	QEMU Developers, CS20 KFTing, qemu-arm, IS20 Avi Fishman,
	Doug Evans

On Wed, Jan 27, 2021 at 01:59:07PM -0800, Hao Wu wrote:
> On Wed, Jan 27, 2021 at 1:42 PM Corey Minyard <minyard@acm.org> wrote:
> 
> > On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> > > On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:
> > >
> > > > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > > > +
> > > > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > > > +{
> > > > > +    uint8_t received_bytes =
> > NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > > > +
> > > > > +    if (received_bytes == 0) {
> > > > > +        npcm7xx_smbus_recv_fifo(s);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    s->sda = s->rx_fifo[s->rx_cur];
> > > > > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > > > +    --s->rxf_sts;
> > > >
> > > > This open-coded decrement seems a little risky.  Are you sure in every
> > > > case that s->rxf_sts > 0?  There's no way what's running in the VM can
> > > > game this and cause a buffer overrun?  One caller to this function
> > seems
> > > > to protect against this, and another does not.
> > > >
> > > s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> > > In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> > > also 0, so it'll take the if-branch and return without running
> > --s->rxf_sts.
> >
> > That is true if called from the
> > NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case.  There is no such check
> > in the NPCM7XX_SMBUS_STATUS_RECEIVING case.
> >
> I don't understand the reasoning here. The caller doesn't matter.
> Previous code has:
>  #define NPCM7XX_SMBRXF_STS_RX_BYTES(rv)     extract8((rv), 0, 5)
> So
>  uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> is guaranteed to be 0 if s->rxf_sts == 0.
> As a result the code will take the following branch and returns:
>  if (received_bytes == 0) {
>     npcm7xx_smbus_recv_fifo(s);
>     return;
>  }
> And will not execute the --s->rxf_sts sentence.
> Please let me know if I missed anything here.

Ah, sorry, I missed that.  Yes, this is ok.  So...

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

> 
> >
> > > I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.
> >
> > You never want to do an assert if the hosted system can do something to
> > cause it.  If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
> > case, it would be ok, but really unnecessary.
> >
> > If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
> > but you want to add a comment to that effect if so.  These sorts of
> > things look dangerous.
> >
> > There is also the question about who takes these patches in.  I'm the
> > I2C maintainer, but there's other code in this series.  Once everything
> > is ready, I can ack them if we take it through the ARM tree.  Or I can
> > take it through my tree with the proper acks.
> >
> I think either  way is fine. Previous NPCM7XX patch series were taken in
> the ARM tree. But as i2c code taking into your tree is also fine.
> 
> >
> > -corey
> >
> > >
> > > >
> > > > Other than this, I didn't see any issues with this patch.
> > > >
> > > > -corey
> > > >
> >


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

* Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
  2021-01-27 23:37           ` Corey Minyard
@ 2021-01-28  5:36             ` Corey Minyard
  0 siblings, 0 replies; 18+ messages in thread
From: Corey Minyard @ 2021-01-28  5:36 UTC (permalink / raw)
  To: Hao Wu
  Cc: Peter Maydell, Patrick Venture, Havard Skinnemoen,
	QEMU Developers, CS20 KFTing, qemu-arm, IS20 Avi Fishman,
	Doug Evans

On Wed, Jan 27, 2021 at 05:37:25PM -0600, Corey Minyard wrote:
> On Wed, Jan 27, 2021 at 01:59:07PM -0800, Hao Wu wrote:
> > >
> > > There is also the question about who takes these patches in.  I'm the
> > > I2C maintainer, but there's other code in this series.  Once everything
> > > is ready, I can ack them if we take it through the ARM tree.  Or I can
> > > take it through my tree with the proper acks.
> > >
> > I think either  way is fine. Previous NPCM7XX patch series were taken in
> > the ARM tree. But as i2c code taking into your tree is also fine.
> > 

Let's go through the ARM tree, then.  So you have an:

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

For patches 2 and 6.

Patch 4 still has the issue with the eeprom size.  If you are expecting
a 32K eeprom to work, it's not going to, you are only going to get 256
bytes.

-corey


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

* Re: [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode
  2021-01-26 19:32 ` [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode wuhaotsh--- via
  2021-01-26 23:00   ` Corey Minyard
@ 2021-01-28 17:28   ` Peter Maydell
  1 sibling, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2021-01-28 17:28 UTC (permalink / raw)
  To: Hao Wu
  Cc: Patrick Venture, Havard Skinnemoen, QEMU Developers, CS20 KFTing,
	qemu-arm, IS20 Avi Fishman, Doug Evans

On Tue, 26 Jan 2021 at 19:32, Hao Wu <wuhaotsh@google.com> wrote:
>
> This commit implements the single-byte mode of the SMBus.
>
> Each Nuvoton SoC has 16 System Management Bus (SMBus). These buses
> compliant with SMBus and I2C protocol.
>
> This patch implements the single-byte mode of the SMBus. In this mode,
> the user sends or receives a byte each time. The SMBus device transmits
> it to the underlying i2c device and sends an interrupt back to the QEMU
> guest.
>
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  docs/system/arm/nuvoton.rst    |   2 +-
>  hw/arm/npcm7xx.c               |  68 ++-
>  hw/i2c/meson.build             |   1 +
>  hw/i2c/npcm7xx_smbus.c         | 766 +++++++++++++++++++++++++++++++++
>  hw/i2c/trace-events            |  11 +
>  include/hw/arm/npcm7xx.h       |   2 +
>  include/hw/i2c/npcm7xx_smbus.h |  88 ++++
>  7 files changed, 921 insertions(+), 17 deletions(-)
>  create mode 100644 hw/i2c/npcm7xx_smbus.c
>  create mode 100644 include/hw/i2c/npcm7xx_smbus.h
>
> diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
> index a1786342e2..34fc799b2d 100644
> --- a/docs/system/arm/nuvoton.rst
> +++ b/docs/system/arm/nuvoton.rst
> @@ -43,6 +43,7 @@ Supported devices
>   * GPIO controller
>   * Analog to Digital Converter (ADC)
>   * Pulse Width Modulation (PWM)
> + * SMBus controller (SMBF)
>
>  Missing devices
>  ---------------
> @@ -58,7 +59,6 @@ Missing devices
>
>   * Ethernet controllers (GMAC and EMC)
>   * USB device (USBD)
> - * SMBus controller (SMBF)
>   * Peripheral SPI controller (PSPI)
>   * SD/MMC host
>   * PECI interface
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index d1fe9bd1df..8f596ffd69 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -104,6 +104,22 @@ enum NPCM7xxInterrupt {
>      NPCM7XX_OHCI_IRQ            = 62,
>      NPCM7XX_PWM0_IRQ            = 93,   /* PWM module 0 */
>      NPCM7XX_PWM1_IRQ,                   /* PWM module 1 */
> +    NPCM7XX_SMBUS0_IRQ          = 64,
> +    NPCM7XX_SMBUS1_IRQ,
> +    NPCM7XX_SMBUS2_IRQ,
> +    NPCM7XX_SMBUS3_IRQ,
> +    NPCM7XX_SMBUS4_IRQ,
> +    NPCM7XX_SMBUS5_IRQ,
> +    NPCM7XX_SMBUS6_IRQ,
> +    NPCM7XX_SMBUS7_IRQ,
> +    NPCM7XX_SMBUS8_IRQ,
> +    NPCM7XX_SMBUS9_IRQ,
> +    NPCM7XX_SMBUS10_IRQ,
> +    NPCM7XX_SMBUS11_IRQ,
> +    NPCM7XX_SMBUS12_IRQ,
> +    NPCM7XX_SMBUS13_IRQ,
> +    NPCM7XX_SMBUS14_IRQ,
> +    NPCM7XX_SMBUS15_IRQ,

Would be nicer to put these in their correct place in numerical
order, ie above the PWM IRQs rather than below them. (The list
is otherwise already in numerical order.)

>      NPCM7XX_GPIO0_IRQ           = 116,
>      NPCM7XX_GPIO1_IRQ,
>      NPCM7XX_GPIO2_IRQ,
> @@ -152,6 +168,26 @@ static const hwaddr npcm7xx_pwm_addr[] = {
>      0xf0104000,

> +static const VMStateDescription vmstate_npcm7xx_smbus = {
> +    .name = "npcm7xx-smbus",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST(),
> +    },

Looks like you forgot to fill in the fields in the vmstate :-)

> +};
> +

thanks
-- PMM


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

* Re: [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board
  2021-01-26 19:32 ` [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board wuhaotsh--- via
@ 2021-01-28 17:32   ` Peter Maydell
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2021-01-28 17:32 UTC (permalink / raw)
  To: Hao Wu
  Cc: Patrick Venture, Havard Skinnemoen, QEMU Developers, CS20 KFTing,
	qemu-arm, IS20 Avi Fishman, Doug Evans

On Tue, 26 Jan 2021 at 19:32, Hao Wu <wuhaotsh@google.com> wrote:
>
> Add an I2C device tree for NPCM750 evaluation board.
>
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>

Slightly confusing commit message, because "device tree"
usually means the data structure describing hardware for Linux
(https://www.kernel.org/doc/html/latest/devicetree/usage-model.html).

Something like "Create the I2C tmp105 temperature sensor devices"
would be clearer, I think.

> ---
>  hw/arm/npcm7xx_boards.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index 3fdd5cab01..2d82f48848 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -98,6 +98,20 @@ static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
>      return NPCM7XX(obj);
>  }
>
> +static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num)
> +{
> +    g_assert(num < ARRAY_SIZE(soc->smbus));
> +    return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus"));
> +}
> +
> +static void npcm750_evb_i2c_init(NPCM7xxState *soc)
> +{
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 0), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 1), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 2), "tmp105", 0x48);
> +    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 6), "tmp105", 0x48);

I assume these correspond to temperature sensors on the real board.
Might be worth having a comment saying what their function is
(I'm guessing they're measuring temperature of different bits of
the board somehow?)

> +}
> +
>  static void npcm750_evb_init(MachineState *machine)
>  {
>      NPCM7xxState *soc;
> @@ -108,6 +122,7 @@ static void npcm750_evb_init(MachineState *machine)
>
>      npcm7xx_load_bootrom(machine, soc);
>      npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, 0));
> +    npcm750_evb_i2c_init(soc);
>      npcm7xx_load_kernel(machine, soc);
>  }
>
> --
> 2.30.0.365.g02bc693789-goog

thanks
-- PMM


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

* Re: [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ
  2021-01-26 19:32 ` [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ wuhaotsh--- via
  2021-01-26 23:05   ` Corey Minyard
@ 2021-01-28 17:33   ` Peter Maydell
  1 sibling, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2021-01-28 17:33 UTC (permalink / raw)
  To: Hao Wu
  Cc: Patrick Venture, Havard Skinnemoen, QEMU Developers, CS20 KFTing,
	qemu-arm, IS20 Avi Fishman, Doug Evans

On Tue, 26 Jan 2021 at 19:32, Hao Wu <wuhaotsh@google.com> wrote:
>
> Add an I2C device tree for Quanta GSJ. We only included devices with
> existing QEMU implementation, including AT24 EEPROM and temperature
> sensors.
>
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>

Same remarks as for patch 3 apply here too.

thanks
-- PMM


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

end of thread, other threads:[~2021-01-28 17:40 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-26 19:32 [PATCH 0/6] hw/i2c: Add NPCM7XX SMBus Device wuhaotsh--- via
2021-01-26 19:32 ` [PATCH 1/6] hw/arm: Remove GPIO from unimplemented NPCM7XX wuhaotsh--- via
2021-01-26 19:32 ` [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode wuhaotsh--- via
2021-01-26 23:00   ` Corey Minyard
2021-01-28 17:28   ` Peter Maydell
2021-01-26 19:32 ` [PATCH 3/6] hw/arm: Add I2C device tree for NPCM750 eval board wuhaotsh--- via
2021-01-28 17:32   ` Peter Maydell
2021-01-26 19:32 ` [PATCH 4/6] hw/arm: Add I2C device tree for Quanta GSJ wuhaotsh--- via
2021-01-26 23:05   ` Corey Minyard
2021-01-28 17:33   ` Peter Maydell
2021-01-26 19:32 ` [PATCH 5/6] hw/i2c: Add a QTest for NPCM7XX SMBus Device wuhaotsh--- via
2021-01-26 19:32 ` [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode wuhaotsh--- via
2021-01-26 23:47   ` Corey Minyard
2021-01-27 20:37     ` wuhaotsh--- via
2021-01-27 21:42       ` Corey Minyard
2021-01-27 21:59         ` wuhaotsh--- via
2021-01-27 23:37           ` Corey Minyard
2021-01-28  5:36             ` Corey Minyard

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