qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/9] Add powernv10 I2C devices and tests
@ 2024-01-25 22:48 Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 1/9] misc/pca9552: Fix inverted input status Glenn Miles
                   ` (8 more replies)
  0 siblings, 9 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

This series of patches includes support, tests and fixes for
adding PCA9552 and PCA9554 I2C devices to the powernv10 chip.

The PCA9552 device is used for PCIe slot hotplug power control
and monitoring, while the PCA9554 device is used for presence
detection of IBM CableCard devices.  Both devices are required
by the Power Hypervisor Firmware on the Power10 Ranier platform.

Changes from previous version:
  - Made several changes to commit 9/9 to address comments from
    Cédric.
  - Added new PnvI2cCtlr struct to commit 9/9 for convenience in
    passing parameters.

Glenn Miles (9):
  misc/pca9552: Fix inverted input status
  misc/pca9552: Let external devices set pca9552 inputs
  ppc/pnv: New powernv10-rainier machine type
  ppc/pnv: Add pca9552 to powernv10-rainier for PCIe hotplug power
    control
  ppc/pnv: Wire up pca9552 GPIO pins for PCIe hotplug power control
  ppc/pnv: Use resettable interface to reset child I2C buses
  misc: Add a pca9554 GPIO device model
  ppc/pnv: Add a pca9554 I2C device to powernv10-rainier
  ppc/pnv: Test pnv i2c master and connected devices

 MAINTAINERS                     |  10 +-
 hw/misc/Kconfig                 |   4 +
 hw/misc/meson.build             |   1 +
 hw/misc/pca9552.c               |  58 +++-
 hw/misc/pca9554.c               | 328 +++++++++++++++++++++
 hw/ppc/Kconfig                  |   2 +
 hw/ppc/pnv.c                    |  67 ++++-
 hw/ppc/pnv_i2c.c                | 146 +---------
 include/hw/i2c/pnv_i2c_regs.h   | 143 ++++++++++
 include/hw/misc/pca9552.h       |   3 +-
 include/hw/misc/pca9554.h       |  36 +++
 include/hw/misc/pca9554_regs.h  |  19 ++
 include/hw/ppc/pnv.h            |   1 +
 tests/qtest/meson.build         |   1 +
 tests/qtest/pca9552-test.c      |   6 +-
 tests/qtest/pnv-host-i2c-test.c | 491 ++++++++++++++++++++++++++++++++
 tests/qtest/pnv-xscom-test.c    |  61 +---
 tests/qtest/pnv-xscom.h         |  80 ++++++
 18 files changed, 1251 insertions(+), 206 deletions(-)
 create mode 100644 hw/misc/pca9554.c
 create mode 100644 include/hw/i2c/pnv_i2c_regs.h
 create mode 100644 include/hw/misc/pca9554.h
 create mode 100644 include/hw/misc/pca9554_regs.h
 create mode 100644 tests/qtest/pnv-host-i2c-test.c
 create mode 100644 tests/qtest/pnv-xscom.h

-- 
2.31.1



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

* [PATCH v7 1/9] misc/pca9552: Fix inverted input status
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 2/9] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab, Andrew Jeffery

The pca9552 INPUT0 and INPUT1 registers are supposed to
hold the logical values of the LED pins.  A logical 0
should be seen in the INPUT0/1 registers for a pin when
its corresponding LSn bits are set to 0, which is also
the state needed for turning on an LED in a typical
usage scenario.  Existing code was doing the opposite
and setting INPUT0/1 bit to a 1 when the LSn bit was
set to 0, so this commit fixes that.

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/misc/pca9552.c          | 18 +++++++++++++-----
 tests/qtest/pca9552-test.c |  6 +++---
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 72b653463f..f00a149d61 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
 
 DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
                        TYPE_PCA955X)
-
+/*
+ * Note:  The LED_ON and LED_OFF configuration values for the PCA955X
+ *        chips are the reverse of the PCA953X family of chips.
+ */
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
@@ -112,13 +115,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
         switch (config) {
         case PCA9552_LED_ON:
-            qemu_set_irq(s->gpio[i], 1);
-            s->regs[input_reg] |= 1 << input_shift;
-            break;
-        case PCA9552_LED_OFF:
+            /* Pin is set to 0V to turn on LED */
             qemu_set_irq(s->gpio[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
+        case PCA9552_LED_OFF:
+            /*
+             * Pin is set to Hi-Z to turn off LED and
+             * pullup sets it to a logical 1.
+             */
+            qemu_set_irq(s->gpio[i], 1);
+            s->regs[input_reg] |= 1 << input_shift;
+            break;
         case PCA9552_LED_PWM0:
         case PCA9552_LED_PWM1:
             /* TODO */
diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
index d80ed93cd3..ccca2b3d91 100644
--- a/tests/qtest/pca9552-test.c
+++ b/tests/qtest/pca9552-test.c
@@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x55);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x0);
+    g_assert_cmphex(value, ==, 0xFF);
 
     pca9552_init(i2cdev);
 
@@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x01);
+    g_assert_cmphex(value, ==, 0xFE);
 
     value = i2c_get8(i2cdev, PCA9552_LS3);
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT1);
-    g_assert_cmphex(value, ==, 0x10);
+    g_assert_cmphex(value, ==, 0xEF);
 }
 
 static void pca9552_register_nodes(void)
-- 
2.31.1



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

* [PATCH v7 2/9] misc/pca9552: Let external devices set pca9552 inputs
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 1/9] misc/pca9552: Fix inverted input status Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 3/9] ppc/pnv: New powernv10-rainier machine type Glenn Miles
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab, Andrew Jeffery

Allow external devices to drive pca9552 input pins by adding
input GPIO's to the model.  This allows a device to connect
its output GPIO's to the pca9552 input GPIO's.

In order for an external device to set the state of a pca9552
pin, the pin must first be configured for high impedance (LED
is off).  If the pca9552 pin is configured to drive the pin low
(LED is on), then external input will be ignored.

Here is a table describing the logical state of a pca9552 pin
given the state being driven by the pca9552 and an external device:

                   PCA9552
                   Configured
                   State

                  | Hi-Z | Low |
            ------+------+-----+
  External   Hi-Z |  Hi  | Low |
  Device    ------+------+-----+
  State      Low  |  Low | Low |
            ------+------+-----+

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/misc/pca9552.c         | 50 +++++++++++++++++++++++++++++++++------
 include/hw/misc/pca9552.h |  3 ++-
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index f00a149d61..2ae13af35e 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
 #define PCA9552_LED_PWM1 0x3
+#define PCA9552_PIN_LOW  0x0
+#define PCA9552_PIN_HIZ  0x1
 
 static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
 
@@ -110,22 +112,27 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
     for (i = 0; i < k->pin_count; i++) {
         uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
-        uint8_t input_shift = (i % 8);
+        uint8_t bit_mask = 1 << (i % 8);
         uint8_t config = pca955x_pin_get_config(s, i);
+        uint8_t old_value = s->regs[input_reg] & bit_mask;
+        uint8_t new_value;
 
         switch (config) {
         case PCA9552_LED_ON:
             /* Pin is set to 0V to turn on LED */
-            qemu_set_irq(s->gpio[i], 0);
-            s->regs[input_reg] &= ~(1 << input_shift);
+            s->regs[input_reg] &= ~bit_mask;
             break;
         case PCA9552_LED_OFF:
             /*
              * Pin is set to Hi-Z to turn off LED and
-             * pullup sets it to a logical 1.
+             * pullup sets it to a logical 1 unless
+             * external device drives it low.
              */
-            qemu_set_irq(s->gpio[i], 1);
-            s->regs[input_reg] |= 1 << input_shift;
+            if (s->ext_state[i] == PCA9552_PIN_LOW) {
+                s->regs[input_reg] &= ~bit_mask;
+            } else {
+                s->regs[input_reg] |=  bit_mask;
+            }
             break;
         case PCA9552_LED_PWM0:
         case PCA9552_LED_PWM1:
@@ -133,6 +140,12 @@ static void pca955x_update_pin_input(PCA955xState *s)
         default:
             break;
         }
+
+        /* update irq state only if pin state changed */
+        new_value = s->regs[input_reg] & bit_mask;
+        if (new_value != old_value) {
+            qemu_set_irq(s->gpio_out[i], !!new_value);
+        }
     }
 }
 
@@ -340,6 +353,7 @@ static const VMStateDescription pca9552_vmstate = {
         VMSTATE_UINT8(len, PCA955xState),
         VMSTATE_UINT8(pointer, PCA955xState),
         VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
+        VMSTATE_UINT8_ARRAY(ext_state, PCA955xState, PCA955X_PIN_COUNT_MAX),
         VMSTATE_I2C_SLAVE(i2c, PCA955xState),
         VMSTATE_END_OF_LIST()
     }
@@ -358,6 +372,7 @@ static void pca9552_reset(DeviceState *dev)
     s->regs[PCA9552_LS2] = 0x55;
     s->regs[PCA9552_LS3] = 0x55;
 
+    memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
     pca955x_update_pin_input(s);
 
     s->pointer = 0xFF;
@@ -380,6 +395,26 @@ static void pca955x_initfn(Object *obj)
     }
 }
 
+static void pca955x_set_ext_state(PCA955xState *s, int pin, int level)
+{
+    if (s->ext_state[pin] != level) {
+        uint16_t pins_status = pca955x_pins_get_status(s);
+        s->ext_state[pin] = level;
+        pca955x_update_pin_input(s);
+        pca955x_display_pins_status(s, pins_status);
+    }
+}
+
+static void pca955x_gpio_in_handler(void *opaque, int pin, int level)
+{
+
+    PCA955xState *s = PCA955X(opaque);
+    PCA955xClass *k = PCA955X_GET_CLASS(s);
+
+    assert((pin >= 0) && (pin < k->pin_count));
+    pca955x_set_ext_state(s, pin, level);
+}
+
 static void pca955x_realize(DeviceState *dev, Error **errp)
 {
     PCA955xClass *k = PCA955X_GET_CLASS(dev);
@@ -389,7 +424,8 @@ static void pca955x_realize(DeviceState *dev, Error **errp)
         s->description = g_strdup("pca-unspecified");
     }
 
-    qdev_init_gpio_out(dev, s->gpio, k->pin_count);
+    qdev_init_gpio_out(dev, s->gpio_out, k->pin_count);
+    qdev_init_gpio_in(dev, pca955x_gpio_in_handler, k->pin_count);
 }
 
 static Property pca955x_properties[] = {
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index b6f4e264fe..c36525f0c3 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -30,7 +30,8 @@ struct PCA955xState {
     uint8_t pointer;
 
     uint8_t regs[PCA955X_NR_REGS];
-    qemu_irq gpio[PCA955X_PIN_COUNT_MAX];
+    qemu_irq gpio_out[PCA955X_PIN_COUNT_MAX];
+    uint8_t ext_state[PCA955X_PIN_COUNT_MAX];
     char *description; /* For debugging purpose only */
 };
 
-- 
2.31.1



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

* [PATCH v7 3/9] ppc/pnv: New powernv10-rainier machine type
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 1/9] misc/pca9552: Fix inverted input status Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 2/9] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 4/9] ppc/pnv: Add pca9552 to powernv10-rainier for PCIe hotplug power control Glenn Miles
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

Create a new powernv machine type, powernv10-rainier, that
will contain rainier-specific devices.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/ppc/pnv.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0297871bdd..08704ce695 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2251,7 +2251,7 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
 }
 
-static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
+static void pnv_machine_p10_common_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
@@ -2263,7 +2263,6 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
         { TYPE_PNV_PHB_ROOT_PORT, "version", "5" },
     };
 
-    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v2.0");
     compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat));
 
@@ -2276,6 +2275,22 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
 }
 
+static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    pnv_machine_p10_common_class_init(oc, data);
+    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
+}
+
+static void pnv_machine_p10_rainier_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    pnv_machine_p10_common_class_init(oc, data);
+    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10 Rainier";
+}
+
 static bool pnv_machine_get_hb(Object *obj, Error **errp)
 {
     PnvMachineState *pnv = PNV_MACHINE(obj);
@@ -2381,6 +2396,11 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
     }
 
 static const TypeInfo types[] = {
+    {
+        .name          = MACHINE_TYPE_NAME("powernv10-rainier"),
+        .parent        = MACHINE_TYPE_NAME("powernv10"),
+        .class_init    = pnv_machine_p10_rainier_class_init,
+    },
     {
         .name          = MACHINE_TYPE_NAME("powernv10"),
         .parent        = TYPE_PNV_MACHINE,
-- 
2.31.1



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

* [PATCH v7 4/9] ppc/pnv: Add pca9552 to powernv10-rainier for PCIe hotplug power control
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (2 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 3/9] ppc/pnv: New powernv10-rainier machine type Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 5/9] ppc/pnv: Wire up pca9552 GPIO pins " Glenn Miles
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

The Power Hypervisor code expects to see a pca9552 device connected
to the 3rd PNV I2C engine on port 1 at I2C address 0x63 (or left-
justified address of 0xC6).  This is used by hypervisor code to
control PCIe slot power during hotplug events.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/ppc/Kconfig       |  1 +
 hw/ppc/pnv.c         | 25 +++++++++++++++++++++++++
 include/hw/ppc/pnv.h |  1 +
 3 files changed, 27 insertions(+)

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 44263a58c4..8e592e4307 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -32,6 +32,7 @@ config POWERNV
     select XIVE
     select FDT_PPC
     select PCI_POWERNV
+    select PCA9552
 
 config PPC405
     bool
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 08704ce695..d8d19fb065 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -790,6 +790,7 @@ static void pnv_init(MachineState *machine)
     const char *bios_name = machine->firmware ?: FW_FILE_NAME;
     PnvMachineState *pnv = PNV_MACHINE(machine);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine);
     char *fw_filename;
     long fw_size;
     uint64_t chip_ram_start = 0;
@@ -979,6 +980,13 @@ static void pnv_init(MachineState *machine)
      */
     pnv->powerdown_notifier.notify = pnv_powerdown_notify;
     qemu_register_powerdown_notifier(&pnv->powerdown_notifier);
+
+    /*
+     * Create/Connect any machine-specific I2C devices
+     */
+    if (pmc->i2c_init) {
+        pmc->i2c_init(pnv);
+    }
 }
 
 /*
@@ -1879,6 +1887,21 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
                               qdev_get_gpio_in(DEVICE(&chip10->psi),
                                                PSIHB9_IRQ_SBE_I2C));
     }
+
+}
+
+static void pnv_rainier_i2c_init(PnvMachineState *pnv)
+{
+    int i;
+    for (i = 0; i < pnv->num_chips; i++) {
+        Pnv10Chip *chip10 = PNV10_CHIP(pnv->chips[i]);
+
+        /*
+         * Add a PCA9552 I2C device for PCIe hotplug control
+         * to engine 2, bus 1, address 0x63
+         */
+        i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9552", 0x63);
+    }
 }
 
 static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
@@ -2286,9 +2309,11 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
 static void pnv_machine_p10_rainier_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
 
     pnv_machine_p10_common_class_init(oc, data);
     mc->desc = "IBM PowerNV (Non-Virtualized) POWER10 Rainier";
+    pmc->i2c_init = pnv_rainier_i2c_init;
 }
 
 static bool pnv_machine_get_hb(Object *obj, Error **errp)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 7e5fef7c43..110ac9aace 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -76,6 +76,7 @@ struct PnvMachineClass {
     int compat_size;
 
     void (*dt_power_mgt)(PnvMachineState *pnv, void *fdt);
+    void (*i2c_init)(PnvMachineState *pnv);
 };
 
 struct PnvMachineState {
-- 
2.31.1



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

* [PATCH v7 5/9] ppc/pnv: Wire up pca9552 GPIO pins for PCIe hotplug power control
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (3 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 4/9] ppc/pnv: Add pca9552 to powernv10-rainier for PCIe hotplug power control Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 6/9] ppc/pnv: Use resettable interface to reset child I2C buses Glenn Miles
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

For power10-rainier, a pca9552 device is used for PCIe slot hotplug
power control by the Power Hypervisor code.  The code expects that
some time after it enables power to a PCIe slot by asserting one of
the pca9552 GPIO pins 0-4, it should see a "power good" signal asserted
on one of pca9552 GPIO pins 5-9.

To simulate this behavior, we simply connect the GPIO outputs for
pins 0-4 to the GPIO inputs for pins 5-9.

Each PCIe slot is assigned 3 GPIO pins on the pca9552 device, for
control of up to 5 PCIe slots.  The per-slot signal names are:

   SLOTx_EN.......PHYP uses this as an output to enable
                  slot power.  We connect this to the
                  SLOTx_PG pin to simulate a PGOOD signal.
   SLOTx_PG.......PHYP uses this as in input to detect
                  PGOOD for the slot.  For our purposes
                  we just connect this to the SLOTx_EN
                  output.
   SLOTx_Control..PHYP uses this as an output to prevent
                  a race condition in the real hotplug
                  circuitry, but we can ignore this output
                  for simulation.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/ppc/pnv.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d8d19fb065..42105211f5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1900,7 +1900,19 @@ static void pnv_rainier_i2c_init(PnvMachineState *pnv)
          * Add a PCA9552 I2C device for PCIe hotplug control
          * to engine 2, bus 1, address 0x63
          */
-        i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9552", 0x63);
+        I2CSlave *dev = i2c_slave_create_simple(chip10->i2c[2].busses[1],
+                                                "pca9552", 0x63);
+
+        /*
+         * Connect PCA9552 GPIO pins 0-4 (SLOTx_EN) outputs to GPIO pins 5-9
+         * (SLOTx_PG) inputs in order to fake the pgood state of PCIe slots
+         * after hypervisor code sets a SLOTx_EN pin high.
+         */
+        qdev_connect_gpio_out(DEVICE(dev), 0, qdev_get_gpio_in(DEVICE(dev), 5));
+        qdev_connect_gpio_out(DEVICE(dev), 1, qdev_get_gpio_in(DEVICE(dev), 6));
+        qdev_connect_gpio_out(DEVICE(dev), 2, qdev_get_gpio_in(DEVICE(dev), 7));
+        qdev_connect_gpio_out(DEVICE(dev), 3, qdev_get_gpio_in(DEVICE(dev), 8));
+        qdev_connect_gpio_out(DEVICE(dev), 4, qdev_get_gpio_in(DEVICE(dev), 9));
     }
 }
 
-- 
2.31.1



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

* [PATCH v7 6/9] ppc/pnv: Use resettable interface to reset child I2C buses
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (4 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 5/9] ppc/pnv: Wire up pca9552 GPIO pins " Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 7/9] misc: Add a pca9554 GPIO device model Glenn Miles
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

The QEMU I2C buses and devices use the resettable
interface for resetting while the PNV I2C controller
and parent buses and devices have not yet transitioned
to this new interface and use the old reset strategy.
This was preventing the I2C buses and devices wired
to the PNV I2C controller from being reset.

The short term fix for this is to have the PNV I2C
Controller's reset function explicitly call the resettable
interface function, bus_cold_reset(), on all child
I2C buses.

The long term fix should be to transition all PNV parent
devices and buses to use the resettable interface so that
all child buses and devices are automatically reset.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/ppc/pnv_i2c.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index 656a48eebe..774946d6b2 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -629,6 +629,19 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void *fdt,
     return 0;
 }
 
+static void pnv_i2c_sys_reset(void *dev)
+{
+    int port;
+    PnvI2C *i2c = PNV_I2C(dev);
+
+    pnv_i2c_reset(dev);
+
+    /* reset all buses connected to this i2c controller */
+    for (port = 0; port < i2c->num_busses; port++) {
+        bus_cold_reset(BUS(i2c->busses[port]));
+    }
+}
+
 static void pnv_i2c_realize(DeviceState *dev, Error **errp)
 {
     PnvI2C *i2c = PNV_I2C(dev);
@@ -654,7 +667,7 @@ static void pnv_i2c_realize(DeviceState *dev, Error **errp)
 
     fifo8_create(&i2c->fifo, PNV_I2C_FIFO_SIZE);
 
-    qemu_register_reset(pnv_i2c_reset, dev);
+    qemu_register_reset(pnv_i2c_sys_reset, dev);
 
     qdev_init_gpio_out(DEVICE(dev), &i2c->psi_irq, 1);
 }
-- 
2.31.1



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

* [PATCH v7 7/9] misc: Add a pca9554 GPIO device model
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (5 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 6/9] ppc/pnv: Use resettable interface to reset child I2C buses Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-03-04 22:32   ` Paolo Bonzini
  2024-01-25 22:48 ` [PATCH v7 8/9] ppc/pnv: Add a pca9554 I2C device to powernv10-rainier Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices Glenn Miles
  8 siblings, 1 reply; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

Specs are available here:

    https://www.nxp.com/docs/en/data-sheet/PCA9554_9554A.pdf

This is a simple model supporting the basic registers for GPIO
mode.  The device also supports an interrupt output line but the
model does not yet support this.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 MAINTAINERS                    |  10 +-
 hw/misc/pca9554.c              | 328 +++++++++++++++++++++++++++++++++
 include/hw/misc/pca9554.h      |  36 ++++
 include/hw/misc/pca9554_regs.h |  19 ++
 4 files changed, 391 insertions(+), 2 deletions(-)
 create mode 100644 hw/misc/pca9554.c
 create mode 100644 include/hw/misc/pca9554.h
 create mode 100644 include/hw/misc/pca9554_regs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index dfaca8323e..51861e3c7d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1169,9 +1169,7 @@ R: Joel Stanley <joel@jms.id.au>
 L: qemu-arm@nongnu.org
 S: Maintained
 F: hw/*/*aspeed*
-F: hw/misc/pca9552.c
 F: include/hw/*/*aspeed*
-F: include/hw/misc/pca9552*.h
 F: hw/net/ftgmac100.c
 F: include/hw/net/ftgmac100.h
 F: docs/system/arm/aspeed.rst
@@ -1540,6 +1538,14 @@ F: include/hw/pci-host/pnv*
 F: pc-bios/skiboot.lid
 F: tests/qtest/pnv*
 
+pca955x
+M: Glenn Miles <milesg@linux.vnet.ibm.com>
+L: qemu-ppc@nongnu.org
+L: qemu-arm@nongnu.org
+S: Odd Fixes
+F: hw/misc/pca955*.c
+F: include/hw/misc/pca955*.h
+
 virtex_ml507
 M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
 L: qemu-ppc@nongnu.org
diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
new file mode 100644
index 0000000000..778b32e443
--- /dev/null
+++ b/hw/misc/pca9554.c
@@ -0,0 +1,328 @@
+/*
+ * PCA9554 I/O port
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/bitops.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/pca9554.h"
+#include "hw/misc/pca9554_regs.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "trace.h"
+#include "qom/object.h"
+
+struct PCA9554Class {
+    /*< private >*/
+    I2CSlaveClass parent_class;
+    /*< public >*/
+};
+typedef struct PCA9554Class PCA9554Class;
+
+DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
+                       TYPE_PCA9554)
+
+#define PCA9554_PIN_LOW  0x0
+#define PCA9554_PIN_HIZ  0x1
+
+static const char *pin_state[] = {"low", "high"};
+
+static void pca9554_update_pin_input(PCA9554State *s)
+{
+    int i;
+    uint8_t config = s->regs[PCA9554_CONFIG];
+    uint8_t output = s->regs[PCA9554_OUTPUT];
+    uint8_t internal_state = config | output;
+
+    for (i = 0; i < PCA9554_PIN_COUNT; i++) {
+        uint8_t bit_mask = 1 << i;
+        uint8_t internal_pin_state = (internal_state >> i) & 0x1;
+        uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
+        uint8_t new_value;
+
+        switch (internal_pin_state) {
+        case PCA9554_PIN_LOW:
+            s->regs[PCA9554_INPUT] &= ~bit_mask;
+            break;
+        case PCA9554_PIN_HIZ:
+            /*
+             * pullup sets it to a logical 1 unless
+             * external device drives it low.
+             */
+            if (s->ext_state[i] == PCA9554_PIN_LOW) {
+                s->regs[PCA9554_INPUT] &= ~bit_mask;
+            } else {
+                s->regs[PCA9554_INPUT] |=  bit_mask;
+            }
+            break;
+        default:
+            break;
+        }
+
+        /* update irq state only if pin state changed */
+        new_value = s->regs[PCA9554_INPUT] & bit_mask;
+        if (new_value != old_value) {
+            if (new_value) {
+                /* changed from 0 to 1 */
+                qemu_set_irq(s->gpio_out[i], 1);
+            } else {
+                /* changed from 1 to 0 */
+                qemu_set_irq(s->gpio_out[i], 0);
+            }
+        }
+    }
+}
+
+static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
+{
+    switch (reg) {
+    case PCA9554_INPUT:
+        return s->regs[PCA9554_INPUT] ^ s->regs[PCA9554_POLARITY];
+    case PCA9554_OUTPUT:
+    case PCA9554_POLARITY:
+    case PCA9554_CONFIG:
+        return s->regs[reg];
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
+                      __func__, reg);
+        return 0xFF;
+    }
+}
+
+static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
+{
+    switch (reg) {
+    case PCA9554_OUTPUT:
+    case PCA9554_CONFIG:
+        s->regs[reg] = data;
+        pca9554_update_pin_input(s);
+        break;
+    case PCA9554_POLARITY:
+        s->regs[reg] = data;
+        break;
+    case PCA9554_INPUT:
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
+                      __func__, reg);
+    }
+}
+
+static uint8_t pca9554_recv(I2CSlave *i2c)
+{
+    PCA9554State *s = PCA9554(i2c);
+    uint8_t ret;
+
+    ret = pca9554_read(s, s->pointer & 0x3);
+
+    return ret;
+}
+
+static int pca9554_send(I2CSlave *i2c, uint8_t data)
+{
+    PCA9554State *s = PCA9554(i2c);
+
+    /* First byte sent by is the register address */
+    if (s->len == 0) {
+        s->pointer = data;
+        s->len++;
+    } else {
+        pca9554_write(s, s->pointer & 0x3, data);
+    }
+
+    return 0;
+}
+
+static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
+{
+    PCA9554State *s = PCA9554(i2c);
+
+    s->len = 0;
+    return 0;
+}
+
+static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    PCA9554State *s = PCA9554(obj);
+    int pin, rc;
+    uint8_t state;
+
+    rc = sscanf(name, "pin%2d", &pin);
+    if (rc != 1) {
+        error_setg(errp, "%s: error reading %s", __func__, name);
+        return;
+    }
+    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
+        error_setg(errp, "%s invalid pin %s", __func__, name);
+        return;
+    }
+
+    state = pca9554_read(s, PCA9554_CONFIG);
+    state |= pca9554_read(s, PCA9554_OUTPUT);
+    state = (state >> pin) & 0x1;
+    visit_type_str(v, name, (char **)&pin_state[state], errp);
+}
+
+static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    PCA9554State *s = PCA9554(obj);
+    int pin, rc, val;
+    uint8_t state, mask;
+    char *state_str;
+
+    if (!visit_type_str(v, name, &state_str, errp)) {
+        return;
+    }
+    rc = sscanf(name, "pin%2d", &pin);
+    if (rc != 1) {
+        error_setg(errp, "%s: error reading %s", __func__, name);
+        return;
+    }
+    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
+        error_setg(errp, "%s invalid pin %s", __func__, name);
+        return;
+    }
+
+    for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
+        if (!strcmp(state_str, pin_state[state])) {
+            break;
+        }
+    }
+    if (state >= ARRAY_SIZE(pin_state)) {
+        error_setg(errp, "%s invalid pin state %s", __func__, state_str);
+        return;
+    }
+
+    /* First, modify the output register bit */
+    val = pca9554_read(s, PCA9554_OUTPUT);
+    mask = 0x1 << pin;
+    if (state == PCA9554_PIN_LOW) {
+        val &= ~(mask);
+    } else {
+        val |= mask;
+    }
+    pca9554_write(s, PCA9554_OUTPUT, val);
+
+    /* Then, clear the config register bit for output mode */
+    val = pca9554_read(s, PCA9554_CONFIG);
+    val &= ~mask;
+    pca9554_write(s, PCA9554_CONFIG, val);
+}
+
+static const VMStateDescription pca9554_vmstate = {
+    .name = "PCA9554",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(len, PCA9554State),
+        VMSTATE_UINT8(pointer, PCA9554State),
+        VMSTATE_UINT8_ARRAY(regs, PCA9554State, PCA9554_NR_REGS),
+        VMSTATE_UINT8_ARRAY(ext_state, PCA9554State, PCA9554_PIN_COUNT),
+        VMSTATE_I2C_SLAVE(i2c, PCA9554State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void pca9554_reset(DeviceState *dev)
+{
+    PCA9554State *s = PCA9554(dev);
+
+    s->regs[PCA9554_INPUT] = 0xFF;
+    s->regs[PCA9554_OUTPUT] = 0xFF;
+    s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
+    s->regs[PCA9554_CONFIG] = 0xFF; /* All pins are inputs */
+
+    memset(s->ext_state, PCA9554_PIN_HIZ, PCA9554_PIN_COUNT);
+    pca9554_update_pin_input(s);
+
+    s->pointer = 0x0;
+    s->len = 0;
+}
+
+static void pca9554_initfn(Object *obj)
+{
+    int pin;
+
+    for (pin = 0; pin < PCA9554_PIN_COUNT; pin++) {
+        char *name;
+
+        name = g_strdup_printf("pin%d", pin);
+        object_property_add(obj, name, "bool", pca9554_get_pin, pca9554_set_pin,
+                            NULL, NULL);
+        g_free(name);
+    }
+}
+
+static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
+{
+    if (s->ext_state[pin] != level) {
+        s->ext_state[pin] = level;
+        pca9554_update_pin_input(s);
+    }
+}
+
+static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
+{
+
+    PCA9554State *s = PCA9554(opaque);
+
+    assert((pin >= 0) && (pin < PCA9554_PIN_COUNT));
+    pca9554_set_ext_state(s, pin, level);
+}
+
+static void pca9554_realize(DeviceState *dev, Error **errp)
+{
+    PCA9554State *s = PCA9554(dev);
+
+    if (!s->description) {
+        s->description = g_strdup("pca9554");
+    }
+
+    qdev_init_gpio_out(dev, s->gpio_out, PCA9554_PIN_COUNT);
+    qdev_init_gpio_in(dev, pca9554_gpio_in_handler, PCA9554_PIN_COUNT);
+}
+
+static Property pca9554_properties[] = {
+    DEFINE_PROP_STRING("description", PCA9554State, description),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pca9554_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    k->event = pca9554_event;
+    k->recv = pca9554_recv;
+    k->send = pca9554_send;
+    dc->realize = pca9554_realize;
+    dc->reset = pca9554_reset;
+    dc->vmsd = &pca9554_vmstate;
+    device_class_set_props(dc, pca9554_properties);
+}
+
+static const TypeInfo pca9554_info = {
+    .name          = TYPE_PCA9554,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_init = pca9554_initfn,
+    .instance_size = sizeof(PCA9554State),
+    .class_init    = pca9554_class_init,
+    .class_size    = sizeof(PCA9554Class),
+    .abstract      = false,
+};
+
+static void pca9554_register_types(void)
+{
+    type_register_static(&pca9554_info);
+}
+
+type_init(pca9554_register_types)
diff --git a/include/hw/misc/pca9554.h b/include/hw/misc/pca9554.h
new file mode 100644
index 0000000000..54bfc4c4c7
--- /dev/null
+++ b/include/hw/misc/pca9554.h
@@ -0,0 +1,36 @@
+/*
+ * PCA9554 I/O port
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef PCA9554_H
+#define PCA9554_H
+
+#include "hw/i2c/i2c.h"
+#include "qom/object.h"
+
+#define TYPE_PCA9554 "pca9554"
+typedef struct PCA9554State PCA9554State;
+DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
+                         TYPE_PCA9554)
+
+#define PCA9554_NR_REGS 4
+#define PCA9554_PIN_COUNT 8
+
+struct PCA9554State {
+    /*< private >*/
+    I2CSlave i2c;
+    /*< public >*/
+
+    uint8_t len;
+    uint8_t pointer;
+
+    uint8_t regs[PCA9554_NR_REGS];
+    qemu_irq gpio_out[PCA9554_PIN_COUNT];
+    uint8_t ext_state[PCA9554_PIN_COUNT];
+    char *description; /* For debugging purpose only */
+};
+
+#endif
diff --git a/include/hw/misc/pca9554_regs.h b/include/hw/misc/pca9554_regs.h
new file mode 100644
index 0000000000..602c4a90e0
--- /dev/null
+++ b/include/hw/misc/pca9554_regs.h
@@ -0,0 +1,19 @@
+/*
+ * PCA9554 I/O port registers
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef PCA9554_REGS_H
+#define PCA9554_REGS_H
+
+/*
+ * Bits [0:1] are used to address a specific register.
+ */
+#define PCA9554_INPUT       0 /* read only input register */
+#define PCA9554_OUTPUT      1 /* read/write pin output state */
+#define PCA9554_POLARITY    2 /* Set polarity of input register */
+#define PCA9554_CONFIG      3 /* Set pins as inputs our ouputs */
+
+#endif
-- 
2.31.1



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

* [PATCH v7 8/9] ppc/pnv: Add a pca9554 I2C device to powernv10-rainier
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (6 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 7/9] misc: Add a pca9554 GPIO device model Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-25 22:48 ` [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices Glenn Miles
  8 siblings, 0 replies; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

For powernv10-rainier, the Power Hypervisor code expects to see a
pca9554 device connected to the 3rd PNV I2C engine on port 1 at I2C
address 0x25 (or left-justified address of 0x4A).  This is used by
the hypervisor code to detect if a "Cable Card" is present.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

No changes from previous version

 hw/misc/Kconfig     | 4 ++++
 hw/misc/meson.build | 1 +
 hw/ppc/Kconfig      | 1 +
 hw/ppc/pnv.c        | 6 ++++++
 4 files changed, 12 insertions(+)

diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 4fc6b29b43..83ad849b62 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -34,6 +34,10 @@ config PCA9552
     bool
     depends on I2C
 
+config PCA9554
+    bool
+    depends on I2C
+
 config I2C_ECHO
     bool
     default y if TEST_DEVICES
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 2ca2ce4b62..8bf7a3b8da 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -4,6 +4,7 @@ system_ss.add(when: 'CONFIG_FW_CFG_DMA', if_true: files('vmcoreinfo.c'))
 system_ss.add(when: 'CONFIG_ISA_DEBUG', if_true: files('debugexit.c'))
 system_ss.add(when: 'CONFIG_ISA_TESTDEV', if_true: files('pc-testdev.c'))
 system_ss.add(when: 'CONFIG_PCA9552', if_true: files('pca9552.c'))
+system_ss.add(when: 'CONFIG_PCA9554', if_true: files('pca9554.c'))
 system_ss.add(when: 'CONFIG_PCI_TESTDEV', if_true: files('pci-testdev.c'))
 system_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
 system_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 8e592e4307..d97743d02f 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -33,6 +33,7 @@ config POWERNV
     select FDT_PPC
     select PCI_POWERNV
     select PCA9552
+    select PCA9554
 
 config PPC405
     bool
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 42105211f5..40f6b8fea0 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1913,6 +1913,12 @@ static void pnv_rainier_i2c_init(PnvMachineState *pnv)
         qdev_connect_gpio_out(DEVICE(dev), 2, qdev_get_gpio_in(DEVICE(dev), 7));
         qdev_connect_gpio_out(DEVICE(dev), 3, qdev_get_gpio_in(DEVICE(dev), 8));
         qdev_connect_gpio_out(DEVICE(dev), 4, qdev_get_gpio_in(DEVICE(dev), 9));
+
+        /*
+         * Add a PCA9554 I2C device for cable card presence detection
+         * to engine 2, bus 1, address 0x25
+         */
+        i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9554", 0x25);
     }
 }
 
-- 
2.31.1



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

* [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices
  2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
                   ` (7 preceding siblings ...)
  2024-01-25 22:48 ` [PATCH v7 8/9] ppc/pnv: Add a pca9554 I2C device to powernv10-rainier Glenn Miles
@ 2024-01-25 22:48 ` Glenn Miles
  2024-01-26  7:52   ` Cédric Le Goater
  8 siblings, 1 reply; 15+ messages in thread
From: Glenn Miles @ 2024-01-25 22:48 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc
  Cc: Glenn Miles, Cédric Le Goater, Nicholas Piggin,
	Frédéric Barrat, Reza Arbab

Tests the following for both P9 and P10:
  - I2C master POR status
  - I2C master status after immediate reset

Tests the following for powernv10-ranier only:
  - Config pca9552 hotplug device pins as inputs then
    Read the INPUT0/1 registers to verify all pins are high
  - Connected GPIO pin tests of P10 PCA9552 device.  Tests
    output of pins 0-4 affect input of pins 5-9 respectively.
  - PCA9554 GPIO pins test.  Tests input and ouput functionality.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---

Changes from previous version:
  - Moved PNV I2C register definitions from pnv-host-i2c-test.c and
    pnv_i2c.c into pnv_i2c_regs.h.
  - Moved PNV XSCOM definitions from pnv-host-i2c-test.c and
    pnv-xscom-test.c into pnv-xscom.h.
  - Renamed pnv_i2c_dev_t to PnvI2cDev.
  - Added PnvI2cCtlr structure for conveniece in passing parameters.

 hw/ppc/pnv_i2c.c                | 131 +--------
 include/hw/i2c/pnv_i2c_regs.h   | 143 ++++++++++
 tests/qtest/meson.build         |   1 +
 tests/qtest/pnv-host-i2c-test.c | 491 ++++++++++++++++++++++++++++++++
 tests/qtest/pnv-xscom-test.c    |  61 +---
 tests/qtest/pnv-xscom.h         |  80 ++++++
 6 files changed, 717 insertions(+), 190 deletions(-)
 create mode 100644 include/hw/i2c/pnv_i2c_regs.h
 create mode 100644 tests/qtest/pnv-host-i2c-test.c
 create mode 100644 tests/qtest/pnv-xscom.h

diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index 774946d6b2..4581cc5e5d 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -22,136 +22,7 @@
 
 #include <libfdt.h>
 
-/* I2C FIFO register */
-#define I2C_FIFO_REG                    0x4
-#define I2C_FIFO                        PPC_BITMASK(0, 7)
-
-/* I2C command register */
-#define I2C_CMD_REG                     0x5
-#define I2C_CMD_WITH_START              PPC_BIT(0)
-#define I2C_CMD_WITH_ADDR               PPC_BIT(1)
-#define I2C_CMD_READ_CONT               PPC_BIT(2)
-#define I2C_CMD_WITH_STOP               PPC_BIT(3)
-#define I2C_CMD_INTR_STEERING           PPC_BITMASK(6, 7) /* P9 */
-#define   I2C_CMD_INTR_STEER_HOST       1
-#define   I2C_CMD_INTR_STEER_OCC        2
-#define I2C_CMD_DEV_ADDR                PPC_BITMASK(8, 14)
-#define I2C_CMD_READ_NOT_WRITE          PPC_BIT(15)
-#define I2C_CMD_LEN_BYTES               PPC_BITMASK(16, 31)
-#define I2C_MAX_TFR_LEN                 0xfff0ull
-
-/* I2C mode register */
-#define I2C_MODE_REG                    0x6
-#define I2C_MODE_BIT_RATE_DIV           PPC_BITMASK(0, 15)
-#define I2C_MODE_PORT_NUM               PPC_BITMASK(16, 21)
-#define I2C_MODE_ENHANCED               PPC_BIT(28)
-#define I2C_MODE_DIAGNOSTIC             PPC_BIT(29)
-#define I2C_MODE_PACING_ALLOW           PPC_BIT(30)
-#define I2C_MODE_WRAP                   PPC_BIT(31)
-
-/* I2C watermark register */
-#define I2C_WATERMARK_REG               0x7
-#define I2C_WATERMARK_HIGH              PPC_BITMASK(16, 19)
-#define I2C_WATERMARK_LOW               PPC_BITMASK(24, 27)
-
-/*
- * I2C interrupt mask and condition registers
- *
- * NB: The function of 0x9 and 0xa changes depending on whether you're reading
- *     or writing to them. When read they return the interrupt condition bits
- *     and on writes they update the interrupt mask register.
- *
- *  The bit definitions are the same for all the interrupt registers.
- */
-#define I2C_INTR_MASK_REG               0x8
-
-#define I2C_INTR_RAW_COND_REG           0x9 /* read */
-#define I2C_INTR_MASK_OR_REG            0x9 /* write*/
-
-#define I2C_INTR_COND_REG               0xa /* read */
-#define I2C_INTR_MASK_AND_REG           0xa /* write */
-
-#define I2C_INTR_ALL                    PPC_BITMASK(16, 31)
-#define I2C_INTR_INVALID_CMD            PPC_BIT(16)
-#define I2C_INTR_LBUS_PARITY_ERR        PPC_BIT(17)
-#define I2C_INTR_BKEND_OVERRUN_ERR      PPC_BIT(18)
-#define I2C_INTR_BKEND_ACCESS_ERR       PPC_BIT(19)
-#define I2C_INTR_ARBT_LOST_ERR          PPC_BIT(20)
-#define I2C_INTR_NACK_RCVD_ERR          PPC_BIT(21)
-#define I2C_INTR_DATA_REQ               PPC_BIT(22)
-#define I2C_INTR_CMD_COMP               PPC_BIT(23)
-#define I2C_INTR_STOP_ERR               PPC_BIT(24)
-#define I2C_INTR_I2C_BUSY               PPC_BIT(25)
-#define I2C_INTR_NOT_I2C_BUSY           PPC_BIT(26)
-#define I2C_INTR_SCL_EQ_1               PPC_BIT(28)
-#define I2C_INTR_SCL_EQ_0               PPC_BIT(29)
-#define I2C_INTR_SDA_EQ_1               PPC_BIT(30)
-#define I2C_INTR_SDA_EQ_0               PPC_BIT(31)
-
-/* I2C status register */
-#define I2C_RESET_I2C_REG               0xb /* write */
-#define I2C_RESET_ERRORS                0xc
-#define I2C_STAT_REG                    0xb /* read */
-#define I2C_STAT_INVALID_CMD            PPC_BIT(0)
-#define I2C_STAT_LBUS_PARITY_ERR        PPC_BIT(1)
-#define I2C_STAT_BKEND_OVERRUN_ERR      PPC_BIT(2)
-#define I2C_STAT_BKEND_ACCESS_ERR       PPC_BIT(3)
-#define I2C_STAT_ARBT_LOST_ERR          PPC_BIT(4)
-#define I2C_STAT_NACK_RCVD_ERR          PPC_BIT(5)
-#define I2C_STAT_DATA_REQ               PPC_BIT(6)
-#define I2C_STAT_CMD_COMP               PPC_BIT(7)
-#define I2C_STAT_STOP_ERR               PPC_BIT(8)
-#define I2C_STAT_UPPER_THRS             PPC_BITMASK(9, 15)
-#define I2C_STAT_ANY_I2C_INTR           PPC_BIT(16)
-#define I2C_STAT_PORT_HISTORY_BUSY      PPC_BIT(19)
-#define I2C_STAT_SCL_INPUT_LEVEL        PPC_BIT(20)
-#define I2C_STAT_SDA_INPUT_LEVEL        PPC_BIT(21)
-#define I2C_STAT_PORT_BUSY              PPC_BIT(22)
-#define I2C_STAT_INTERFACE_BUSY         PPC_BIT(23)
-#define I2C_STAT_FIFO_ENTRY_COUNT       PPC_BITMASK(24, 31)
-
-#define I2C_STAT_ANY_ERR (I2C_STAT_INVALID_CMD | I2C_STAT_LBUS_PARITY_ERR | \
-                          I2C_STAT_BKEND_OVERRUN_ERR | \
-                          I2C_STAT_BKEND_ACCESS_ERR | I2C_STAT_ARBT_LOST_ERR | \
-                          I2C_STAT_NACK_RCVD_ERR | I2C_STAT_STOP_ERR)
-
-
-#define I2C_INTR_ACTIVE \
-        ((I2C_STAT_ANY_ERR >> 16) | I2C_INTR_CMD_COMP | I2C_INTR_DATA_REQ)
-
-/* Pseudo-status used for timeouts */
-#define I2C_STAT_PSEUDO_TIMEOUT         PPC_BIT(63)
-
-/* I2C extended status register */
-#define I2C_EXTD_STAT_REG               0xc
-#define I2C_EXTD_STAT_FIFO_SIZE         PPC_BITMASK(0, 7)
-#define I2C_EXTD_STAT_MSM_CURSTATE      PPC_BITMASK(11, 15)
-#define I2C_EXTD_STAT_SCL_IN_SYNC       PPC_BIT(16)
-#define I2C_EXTD_STAT_SDA_IN_SYNC       PPC_BIT(17)
-#define I2C_EXTD_STAT_S_SCL             PPC_BIT(18)
-#define I2C_EXTD_STAT_S_SDA             PPC_BIT(19)
-#define I2C_EXTD_STAT_M_SCL             PPC_BIT(20)
-#define I2C_EXTD_STAT_M_SDA             PPC_BIT(21)
-#define I2C_EXTD_STAT_HIGH_WATER        PPC_BIT(22)
-#define I2C_EXTD_STAT_LOW_WATER         PPC_BIT(23)
-#define I2C_EXTD_STAT_I2C_BUSY          PPC_BIT(24)
-#define I2C_EXTD_STAT_SELF_BUSY         PPC_BIT(25)
-#define I2C_EXTD_STAT_I2C_VERSION       PPC_BITMASK(27, 31)
-
-/* I2C residual front end/back end length */
-#define I2C_RESIDUAL_LEN_REG            0xd
-#define I2C_RESIDUAL_FRONT_END          PPC_BITMASK(0, 15)
-#define I2C_RESIDUAL_BACK_END           PPC_BITMASK(16, 31)
-
-/* Port busy register */
-#define I2C_PORT_BUSY_REG               0xe
-#define I2C_SET_S_SCL_REG               0xd
-#define I2C_RESET_S_SCL_REG             0xf
-#define I2C_SET_S_SDA_REG               0x10
-#define I2C_RESET_S_SDA_REG             0x11
-
-#define PNV_I2C_FIFO_SIZE 8
-#define PNV_I2C_MAX_BUSSES 64
+#include "hw/i2c/pnv_i2c_regs.h"
 
 static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c)
 {
diff --git a/include/hw/i2c/pnv_i2c_regs.h b/include/hw/i2c/pnv_i2c_regs.h
new file mode 100644
index 0000000000..85e96ff480
--- /dev/null
+++ b/include/hw/i2c/pnv_i2c_regs.h
@@ -0,0 +1,143 @@
+/*
+ * PowerNV I2C Controller Register Definitions
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef PNV_I2C_REGS_H
+#define PNV_I2C_REGS_H
+
+/* I2C FIFO register */
+#define I2C_FIFO_REG                    0x4
+#define I2C_FIFO                        PPC_BITMASK(0, 7)
+
+/* I2C command register */
+#define I2C_CMD_REG                     0x5
+#define I2C_CMD_WITH_START              PPC_BIT(0)
+#define I2C_CMD_WITH_ADDR               PPC_BIT(1)
+#define I2C_CMD_READ_CONT               PPC_BIT(2)
+#define I2C_CMD_WITH_STOP               PPC_BIT(3)
+#define I2C_CMD_INTR_STEERING           PPC_BITMASK(6, 7) /* P9 */
+#define   I2C_CMD_INTR_STEER_HOST       1
+#define   I2C_CMD_INTR_STEER_OCC        2
+#define I2C_CMD_DEV_ADDR                PPC_BITMASK(8, 14)
+#define I2C_CMD_READ_NOT_WRITE          PPC_BIT(15)
+#define I2C_CMD_LEN_BYTES               PPC_BITMASK(16, 31)
+#define I2C_MAX_TFR_LEN                 0xfff0ull
+
+/* I2C mode register */
+#define I2C_MODE_REG                    0x6
+#define I2C_MODE_BIT_RATE_DIV           PPC_BITMASK(0, 15)
+#define I2C_MODE_PORT_NUM               PPC_BITMASK(16, 21)
+#define I2C_MODE_ENHANCED               PPC_BIT(28)
+#define I2C_MODE_DIAGNOSTIC             PPC_BIT(29)
+#define I2C_MODE_PACING_ALLOW           PPC_BIT(30)
+#define I2C_MODE_WRAP                   PPC_BIT(31)
+
+/* I2C watermark register */
+#define I2C_WATERMARK_REG               0x7
+#define I2C_WATERMARK_HIGH              PPC_BITMASK(16, 19)
+#define I2C_WATERMARK_LOW               PPC_BITMASK(24, 27)
+
+/*
+ * I2C interrupt mask and condition registers
+ *
+ * NB: The function of 0x9 and 0xa changes depending on whether you're reading
+ *     or writing to them. When read they return the interrupt condition bits
+ *     and on writes they update the interrupt mask register.
+ *
+ *  The bit definitions are the same for all the interrupt registers.
+ */
+#define I2C_INTR_MASK_REG               0x8
+
+#define I2C_INTR_RAW_COND_REG           0x9 /* read */
+#define I2C_INTR_MASK_OR_REG            0x9 /* write*/
+
+#define I2C_INTR_COND_REG               0xa /* read */
+#define I2C_INTR_MASK_AND_REG           0xa /* write */
+
+#define I2C_INTR_ALL                    PPC_BITMASK(16, 31)
+#define I2C_INTR_INVALID_CMD            PPC_BIT(16)
+#define I2C_INTR_LBUS_PARITY_ERR        PPC_BIT(17)
+#define I2C_INTR_BKEND_OVERRUN_ERR      PPC_BIT(18)
+#define I2C_INTR_BKEND_ACCESS_ERR       PPC_BIT(19)
+#define I2C_INTR_ARBT_LOST_ERR          PPC_BIT(20)
+#define I2C_INTR_NACK_RCVD_ERR          PPC_BIT(21)
+#define I2C_INTR_DATA_REQ               PPC_BIT(22)
+#define I2C_INTR_CMD_COMP               PPC_BIT(23)
+#define I2C_INTR_STOP_ERR               PPC_BIT(24)
+#define I2C_INTR_I2C_BUSY               PPC_BIT(25)
+#define I2C_INTR_NOT_I2C_BUSY           PPC_BIT(26)
+#define I2C_INTR_SCL_EQ_1               PPC_BIT(28)
+#define I2C_INTR_SCL_EQ_0               PPC_BIT(29)
+#define I2C_INTR_SDA_EQ_1               PPC_BIT(30)
+#define I2C_INTR_SDA_EQ_0               PPC_BIT(31)
+
+/* I2C status register */
+#define I2C_RESET_I2C_REG               0xb /* write */
+#define I2C_RESET_ERRORS                0xc
+#define I2C_STAT_REG                    0xb /* read */
+#define I2C_STAT_INVALID_CMD            PPC_BIT(0)
+#define I2C_STAT_LBUS_PARITY_ERR        PPC_BIT(1)
+#define I2C_STAT_BKEND_OVERRUN_ERR      PPC_BIT(2)
+#define I2C_STAT_BKEND_ACCESS_ERR       PPC_BIT(3)
+#define I2C_STAT_ARBT_LOST_ERR          PPC_BIT(4)
+#define I2C_STAT_NACK_RCVD_ERR          PPC_BIT(5)
+#define I2C_STAT_DATA_REQ               PPC_BIT(6)
+#define I2C_STAT_CMD_COMP               PPC_BIT(7)
+#define I2C_STAT_STOP_ERR               PPC_BIT(8)
+#define I2C_STAT_UPPER_THRS             PPC_BITMASK(9, 15)
+#define I2C_STAT_ANY_I2C_INTR           PPC_BIT(16)
+#define I2C_STAT_PORT_HISTORY_BUSY      PPC_BIT(19)
+#define I2C_STAT_SCL_INPUT_LEVEL        PPC_BIT(20)
+#define I2C_STAT_SDA_INPUT_LEVEL        PPC_BIT(21)
+#define I2C_STAT_PORT_BUSY              PPC_BIT(22)
+#define I2C_STAT_INTERFACE_BUSY         PPC_BIT(23)
+#define I2C_STAT_FIFO_ENTRY_COUNT       PPC_BITMASK(24, 31)
+
+#define I2C_STAT_ANY_ERR (I2C_STAT_INVALID_CMD | I2C_STAT_LBUS_PARITY_ERR | \
+                          I2C_STAT_BKEND_OVERRUN_ERR | \
+                          I2C_STAT_BKEND_ACCESS_ERR | I2C_STAT_ARBT_LOST_ERR | \
+                          I2C_STAT_NACK_RCVD_ERR | I2C_STAT_STOP_ERR)
+
+
+#define I2C_INTR_ACTIVE \
+        ((I2C_STAT_ANY_ERR >> 16) | I2C_INTR_CMD_COMP | I2C_INTR_DATA_REQ)
+
+/* Pseudo-status used for timeouts */
+#define I2C_STAT_PSEUDO_TIMEOUT         PPC_BIT(63)
+
+/* I2C extended status register */
+#define I2C_EXTD_STAT_REG               0xc
+#define I2C_EXTD_STAT_FIFO_SIZE         PPC_BITMASK(0, 7)
+#define I2C_EXTD_STAT_MSM_CURSTATE      PPC_BITMASK(11, 15)
+#define I2C_EXTD_STAT_SCL_IN_SYNC       PPC_BIT(16)
+#define I2C_EXTD_STAT_SDA_IN_SYNC       PPC_BIT(17)
+#define I2C_EXTD_STAT_S_SCL             PPC_BIT(18)
+#define I2C_EXTD_STAT_S_SDA             PPC_BIT(19)
+#define I2C_EXTD_STAT_M_SCL             PPC_BIT(20)
+#define I2C_EXTD_STAT_M_SDA             PPC_BIT(21)
+#define I2C_EXTD_STAT_HIGH_WATER        PPC_BIT(22)
+#define I2C_EXTD_STAT_LOW_WATER         PPC_BIT(23)
+#define I2C_EXTD_STAT_I2C_BUSY          PPC_BIT(24)
+#define I2C_EXTD_STAT_SELF_BUSY         PPC_BIT(25)
+#define I2C_EXTD_STAT_I2C_VERSION       PPC_BITMASK(27, 31)
+
+/* I2C residual front end/back end length */
+#define I2C_RESIDUAL_LEN_REG            0xd
+#define I2C_RESIDUAL_FRONT_END          PPC_BITMASK(0, 15)
+#define I2C_RESIDUAL_BACK_END           PPC_BITMASK(16, 31)
+
+/* Port busy register */
+#define I2C_PORT_BUSY_REG               0xe
+#define I2C_SET_S_SCL_REG               0xd
+#define I2C_RESET_S_SCL_REG             0xf
+#define I2C_SET_S_SDA_REG               0x10
+#define I2C_RESET_S_SDA_REG             0x11
+
+#define PNV_I2C_FIFO_SIZE 8
+#define PNV_I2C_MAX_BUSSES 64
+
+#endif /* PNV_I2C_REGS_H */
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 84a055a7d9..bde1e04990 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -167,6 +167,7 @@ qtests_ppc64 = \
   qtests_ppc + \
   (config_all_devices.has_key('CONFIG_PSERIES') ? ['device-plug-test'] : []) +               \
   (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-xscom-test'] : []) +                 \
+  (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-host-i2c-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_PSERIES') ? ['rtas-test'] : []) +                      \
   (slirp.found() ? ['pxe-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) +             \
diff --git a/tests/qtest/pnv-host-i2c-test.c b/tests/qtest/pnv-host-i2c-test.c
new file mode 100644
index 0000000000..c635177252
--- /dev/null
+++ b/tests/qtest/pnv-host-i2c-test.c
@@ -0,0 +1,491 @@
+/*
+ * QTest testcase for PowerNV 10 Host I2C Communications
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "hw/misc/pca9554_regs.h"
+#include "hw/misc/pca9552_regs.h"
+#include "pnv-xscom.h"
+
+#define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
+#define PPC_BIT32(bit)          (0x80000000 >> (bit))
+#define PPC_BIT8(bit)           (0x80 >> (bit))
+#define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
+#define PPC_BITMASK32(bs, be)   ((PPC_BIT32(bs) - PPC_BIT32(be)) | \
+                                 PPC_BIT32(bs))
+
+#define MASK_TO_LSH(m)          (__builtin_ffsll(m) - 1)
+#define GETFIELD(m, v)          (((v) & (m)) >> MASK_TO_LSH(m))
+#define SETFIELD(m, v, val) \
+        (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
+
+#define PNV10_XSCOM_I2CM_BASE   0xa0000
+#define PNV10_XSCOM_I2CM_SIZE   0x1000
+
+#include "hw/i2c/pnv_i2c_regs.h"
+
+typedef struct {
+    QTestState    *qts;
+    const PnvChip *chip;
+    int           engine;
+} PnvI2cCtlr;
+
+typedef struct {
+    PnvI2cCtlr  *ctlr;
+    int         port;
+    uint8_t     addr;
+} PnvI2cDev;
+
+
+static uint64_t pnv_i2c_xscom_addr(PnvI2cCtlr *ctlr, uint32_t reg)
+{
+    return pnv_xscom_addr(ctlr->chip, PNV10_XSCOM_I2CM_BASE +
+                          (PNV10_XSCOM_I2CM_SIZE * ctlr->engine) + reg);
+}
+
+static uint64_t pnv_i2c_xscom_read(PnvI2cCtlr *ctlr, uint32_t reg)
+{
+    return qtest_readq(ctlr->qts, pnv_i2c_xscom_addr(ctlr, reg));
+}
+
+static void pnv_i2c_xscom_write(PnvI2cCtlr *ctlr, uint32_t reg, uint64_t val)
+{
+    qtest_writeq(ctlr->qts, pnv_i2c_xscom_addr(ctlr, reg), val);
+}
+
+/* Write len bytes from buf to i2c device with given addr and port */
+static void pnv_i2c_send(PnvI2cDev *dev, const uint8_t *buf, uint16_t len)
+{
+    int byte_num;
+    uint64_t reg64;
+
+    /* select requested port */
+    reg64 = SETFIELD(I2C_MODE_BIT_RATE_DIV, 0ull, 0x2be);
+    reg64 = SETFIELD(I2C_MODE_PORT_NUM, reg64, dev->port);
+    pnv_i2c_xscom_write(dev->ctlr, I2C_MODE_REG, reg64);
+
+    /* check status for cmd complete and bus idle */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
+                    I2C_STAT_CMD_COMP);
+
+    /* Send start, with stop, with address and len bytes of data */
+    reg64 = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR | I2C_CMD_WITH_STOP;
+    reg64 = SETFIELD(I2C_CMD_DEV_ADDR, reg64, dev->addr);
+    reg64 = SETFIELD(I2C_CMD_LEN_BYTES, reg64, len);
+    pnv_i2c_xscom_write(dev->ctlr, I2C_CMD_REG, reg64);
+
+    /* check status for errors */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_STAT_ANY_ERR, ==, 0);
+
+    /* write data bytes to fifo register */
+    for (byte_num = 0; byte_num < len; byte_num++) {
+        reg64 = SETFIELD(I2C_FIFO, 0ull, buf[byte_num]);
+        pnv_i2c_xscom_write(dev->ctlr, I2C_FIFO_REG, reg64);
+    }
+
+    /* check status for cmd complete and bus idle */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
+                    I2C_STAT_CMD_COMP);
+}
+
+/* Recieve len bytes into buf from i2c device with given addr and port */
+static void pnv_i2c_recv(PnvI2cDev *dev, uint8_t *buf, uint16_t len)
+{
+    int byte_num;
+    uint64_t reg64;
+
+    /* select requested port */
+    reg64 = SETFIELD(I2C_MODE_BIT_RATE_DIV, 0ull, 0x2be);
+    reg64 = SETFIELD(I2C_MODE_PORT_NUM, reg64, dev->port);
+    pnv_i2c_xscom_write(dev->ctlr, I2C_MODE_REG, reg64);
+
+    /* check status for cmd complete and bus idle */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
+                    I2C_STAT_CMD_COMP);
+
+    /* Send start, with stop, with address and len bytes of data */
+    reg64 = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR |
+            I2C_CMD_WITH_STOP | I2C_CMD_READ_NOT_WRITE;
+    reg64 = SETFIELD(I2C_CMD_DEV_ADDR, reg64, dev->addr);
+    reg64 = SETFIELD(I2C_CMD_LEN_BYTES, reg64, len);
+    pnv_i2c_xscom_write(dev->ctlr, I2C_CMD_REG, reg64);
+
+    /* check status for errors */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_STAT_ANY_ERR, ==, 0);
+
+    /* Read data bytes from fifo register */
+    for (byte_num = 0; byte_num < len; byte_num++) {
+        reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_FIFO_REG);
+        buf[byte_num] = GETFIELD(I2C_FIFO, reg64);
+    }
+
+    /* check status for cmd complete and bus idle */
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
+    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
+    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
+    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
+                    I2C_STAT_CMD_COMP);
+}
+
+static void pnv_i2c_pca9554_default_cfg(PnvI2cDev *dev)
+{
+    uint8_t buf[2];
+
+    /* input register bits are not inverted */
+    buf[0] = PCA9554_POLARITY;
+    buf[1] = 0;
+    pnv_i2c_send(dev, buf, 2);
+
+    /* All pins are inputs */
+    buf[0] = PCA9554_CONFIG;
+    buf[1] = 0xff;
+    pnv_i2c_send(dev, buf, 2);
+
+    /* Output value for when pins are outputs */
+    buf[0] = PCA9554_OUTPUT;
+    buf[1] = 0xff;
+    pnv_i2c_send(dev, buf, 2);
+}
+
+static void pnv_i2c_pca9554_set_pin(PnvI2cDev *dev, int pin, bool high)
+{
+    uint8_t send_buf[2];
+    uint8_t recv_buf[2];
+    uint8_t mask = 0x1 << pin;
+    uint8_t new_value = ((high) ? 1 : 0) << pin;
+
+    /* read current OUTPUT value */
+    send_buf[0] = PCA9554_OUTPUT;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+
+    /* write new OUTPUT value */
+    send_buf[1] = (recv_buf[0] & ~mask) | new_value;
+    pnv_i2c_send(dev, send_buf, 2);
+
+    /* Update config bit for output */
+    send_buf[0] = PCA9554_CONFIG;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+    send_buf[1] = recv_buf[0] & ~mask;
+    pnv_i2c_send(dev, send_buf, 2);
+}
+
+static uint8_t pnv_i2c_pca9554_read_pins(PnvI2cDev *dev)
+{
+    uint8_t send_buf[1];
+    uint8_t recv_buf[1];
+    uint8_t inputs;
+    send_buf[0] = PCA9554_INPUT;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+    inputs = recv_buf[0];
+    return inputs;
+}
+
+static void pnv_i2c_pca9554_flip_polarity(PnvI2cDev *dev)
+{
+    uint8_t recv_buf[1];
+    uint8_t send_buf[2];
+
+    send_buf[0] = PCA9554_POLARITY;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+    send_buf[1] = recv_buf[0] ^ 0xff;
+    pnv_i2c_send(dev, send_buf, 2);
+}
+
+static void pnv_i2c_pca9554_default_inputs(PnvI2cDev *dev)
+{
+    uint8_t pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0xff);
+}
+
+/* Check that setting pin values and polarity changes inputs as expected */
+static void pnv_i2c_pca554_set_pins(PnvI2cDev *dev)
+{
+    uint8_t pin_values;
+    pnv_i2c_pca9554_set_pin(dev, 0, 0);
+    pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0xfe);
+    pnv_i2c_pca9554_flip_polarity(dev);
+    pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0x01);
+    pnv_i2c_pca9554_set_pin(dev, 2, 0);
+    pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0x05);
+    pnv_i2c_pca9554_flip_polarity(dev);
+    pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0xfa);
+    pnv_i2c_pca9554_default_cfg(dev);
+    pin_values = pnv_i2c_pca9554_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0xff);
+}
+
+static void pnv_i2c_pca9552_default_cfg(PnvI2cDev *dev)
+{
+    uint8_t buf[2];
+    /* configure pwm/psc regs */
+    buf[0] = PCA9552_PSC0;
+    buf[1] = 0xff;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_PWM0;
+    buf[1] = 0x80;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_PSC1;
+    buf[1] = 0xff;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_PWM1;
+    buf[1] = 0x80;
+    pnv_i2c_send(dev, buf, 2);
+
+    /* configure all pins as inputs */
+    buf[0] = PCA9552_LS0;
+    buf[1] = 0x55;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_LS1;
+    buf[1] = 0x55;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_LS2;
+    buf[1] = 0x55;
+    pnv_i2c_send(dev, buf, 2);
+    buf[0] = PCA9552_LS3;
+    buf[1] = 0x55;
+    pnv_i2c_send(dev, buf, 2);
+}
+
+static void pnv_i2c_pca9552_set_pin(PnvI2cDev *dev, int pin, bool high)
+{
+    uint8_t send_buf[2];
+    uint8_t recv_buf[2];
+    uint8_t reg = PCA9552_LS0 + (pin / 4);
+    uint8_t shift = (pin % 4) * 2;
+    uint8_t mask = ~(0x3 << shift);
+    uint8_t new_value = ((high) ? 1 : 0) << shift;
+
+    /* read current LSx value */
+    send_buf[0] = reg;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+
+    /* write new value to LSx */
+    send_buf[1] = (recv_buf[0] & mask) | new_value;
+    pnv_i2c_send(dev, send_buf, 2);
+}
+
+static uint16_t pnv_i2c_pca9552_read_pins(PnvI2cDev *dev)
+{
+    uint8_t send_buf[2];
+    uint8_t recv_buf[2];
+    uint16_t inputs;
+    send_buf[0] = PCA9552_INPUT0;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+    inputs = recv_buf[0];
+    send_buf[0] = PCA9552_INPUT1;
+    pnv_i2c_send(dev, send_buf, 1);
+    pnv_i2c_recv(dev, recv_buf, 1);
+    inputs |= recv_buf[0] << 8;
+    return inputs;
+}
+
+static void pnv_i2c_pca9552_default_inputs(PnvI2cDev *dev)
+{
+    uint16_t pin_values = pnv_i2c_pca9552_read_pins(dev);
+    g_assert_cmphex(pin_values, ==, 0xffff);
+}
+
+/*
+ * Set pins 0-4 one at a time and verify that pins 5-9 are
+ * set to the same value
+ */
+static void pnv_i2c_pca552_set_pins(PnvI2cDev *dev)
+{
+    uint16_t pin_values;
+
+    /* set pin 0 low */
+    pnv_i2c_pca9552_set_pin(dev, 0, 0);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* pins 0 and 5 should be low */
+    g_assert_cmphex(pin_values, ==, 0xffde);
+
+    /* set pin 1 low */
+    pnv_i2c_pca9552_set_pin(dev, 1, 0);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* pins 0, 1, 5 and 6 should be low */
+    g_assert_cmphex(pin_values, ==, 0xff9c);
+
+    /* set pin 2 low */
+    pnv_i2c_pca9552_set_pin(dev, 2, 0);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* pins 0, 1, 2, 5, 6 and 7 should be low */
+    g_assert_cmphex(pin_values, ==, 0xff18);
+
+    /* set pin 3 low */
+    pnv_i2c_pca9552_set_pin(dev, 3, 0);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* pins 0, 1, 2, 3, 5, 6, 7 and 8 should be low */
+    g_assert_cmphex(pin_values, ==, 0xfe10);
+
+    /* set pin 4 low */
+    pnv_i2c_pca9552_set_pin(dev, 4, 0);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* pins 0, 1, 2, 3, 5, 6, 7, 8 and 9 should be low */
+    g_assert_cmphex(pin_values, ==, 0xfc00);
+
+    /* reset all pins to the high state */
+    pnv_i2c_pca9552_default_cfg(dev);
+    pin_values = pnv_i2c_pca9552_read_pins(dev);
+
+    /* verify all pins went back to the high state */
+    g_assert_cmphex(pin_values, ==, 0xffff);
+}
+
+static void reset_engine(PnvI2cCtlr *ctlr)
+{
+    pnv_i2c_xscom_write(ctlr, I2C_RESET_I2C_REG, 0);
+}
+
+static void check_i2cm_por_regs(QTestState *qts, const PnvChip *chip)
+{
+    int engine;
+    for (engine = 0; engine < chip->num_i2c; engine++) {
+        PnvI2cCtlr ctlr;
+        ctlr.qts = qts;
+        ctlr.chip = chip;
+        ctlr.engine = engine;
+
+        /* Check version in Extended Status Register */
+        uint64_t value = pnv_i2c_xscom_read(&ctlr, I2C_EXTD_STAT_REG);
+        g_assert_cmphex(value & I2C_EXTD_STAT_I2C_VERSION, ==, 0x1700000000);
+
+        /* Check for command complete and bus idle in Status Register */
+        value = pnv_i2c_xscom_read(&ctlr, I2C_STAT_REG);
+        g_assert_cmphex(value & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP),
+                        ==,
+                        I2C_STAT_CMD_COMP);
+    }
+}
+
+static void reset_all(QTestState *qts, const PnvChip *chip)
+{
+    int engine;
+    for (engine = 0; engine < chip->num_i2c; engine++) {
+        PnvI2cCtlr ctlr;
+        ctlr.qts = qts;
+        ctlr.chip = chip;
+        ctlr.engine = engine;
+        reset_engine(&ctlr);
+        pnv_i2c_xscom_write(&ctlr, I2C_MODE_REG, 0x02be040000000000);
+    }
+}
+
+static void test_host_i2c(const void *data)
+{
+    const PnvChip *chip = data;
+    QTestState *qts;
+    const char *machine = "powernv8";
+    PnvI2cCtlr ctlr;
+    PnvI2cDev pca9552;
+    PnvI2cDev pca9554;
+
+    if (chip->chip_type == PNV_CHIP_POWER9) {
+        machine = "powernv9";
+    } else if (chip->chip_type == PNV_CHIP_POWER10) {
+        machine = "powernv10-rainier";
+    }
+
+    qts = qtest_initf("-M %s -smp %d,cores=1,threads=%d -nographic "
+                      "-nodefaults -serial mon:stdio -S "
+                      "-d guest_errors",
+                      machine, SMT, SMT);
+
+    /* Check the I2C master status registers after POR */
+    check_i2cm_por_regs(qts, chip);
+
+    /* Now do a forced "immediate" reset on all engines */
+    reset_all(qts, chip);
+
+    /* Check that the status values are still good */
+    check_i2cm_por_regs(qts, chip);
+
+    /* P9 doesn't have any i2c devices attached at this time */
+    if (chip->chip_type != PNV_CHIP_POWER10) {
+        qtest_quit(qts);
+        return;
+    }
+
+    /* Initialize for a P10 pca9552 hotplug device */
+    ctlr.qts = qts;
+    ctlr.chip = chip;
+    ctlr.engine = 2;
+    pca9552.ctlr = &ctlr;
+    pca9552.port = 1;
+    pca9552.addr = 0x63;
+
+    /* Set all pca9552 pins as inputs */
+    pnv_i2c_pca9552_default_cfg(&pca9552);
+
+    /* Check that all pins of the pca9552 are high */
+    pnv_i2c_pca9552_default_inputs(&pca9552);
+
+    /* perform individual pin tests */
+    pnv_i2c_pca552_set_pins(&pca9552);
+
+    /* Initialize for a P10 pca9554 CableCard Presence detection device */
+    pca9554.ctlr = &ctlr;
+    pca9554.port = 1;
+    pca9554.addr = 0x25;
+
+    /* Set all pca9554 pins as inputs */
+    pnv_i2c_pca9554_default_cfg(&pca9554);
+
+    /* Check that all pins of the pca9554 are high */
+    pnv_i2c_pca9554_default_inputs(&pca9554);
+
+    /* perform individual pin tests */
+    pnv_i2c_pca554_set_pins(&pca9554);
+
+    qtest_quit(qts);
+}
+
+static void add_test(const char *name, void (*test)(const void *data))
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pnv_chips); i++) {
+        char *tname = g_strdup_printf("pnv-xscom/%s/%s", name,
+                                      pnv_chips[i].cpu_model);
+        qtest_add_data_func(tname, &pnv_chips[i], test);
+        g_free(tname);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    add_test("host-i2c", test_host_i2c);
+    return g_test_run();
+}
diff --git a/tests/qtest/pnv-xscom-test.c b/tests/qtest/pnv-xscom-test.c
index 8a5ac11037..c814c0f4f5 100644
--- a/tests/qtest/pnv-xscom-test.c
+++ b/tests/qtest/pnv-xscom-test.c
@@ -10,66 +10,7 @@
 
 #include "libqtest.h"
 
-typedef enum PnvChipType {
-    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
-    PNV_CHIP_POWER8,      /* AKA Venice */
-    PNV_CHIP_POWER8NVL,   /* AKA Naples */
-    PNV_CHIP_POWER9,      /* AKA Nimbus */
-    PNV_CHIP_POWER10,
-} PnvChipType;
-
-typedef struct PnvChip {
-    PnvChipType chip_type;
-    const char *cpu_model;
-    uint64_t    xscom_base;
-    uint64_t    cfam_id;
-    uint32_t    first_core;
-} PnvChip;
-
-static const PnvChip pnv_chips[] = {
-    {
-        .chip_type  = PNV_CHIP_POWER8,
-        .cpu_model  = "POWER8",
-        .xscom_base = 0x0003fc0000000000ull,
-        .cfam_id    = 0x220ea04980000000ull,
-        .first_core = 0x1,
-    }, {
-        .chip_type  = PNV_CHIP_POWER8NVL,
-        .cpu_model  = "POWER8NVL",
-        .xscom_base = 0x0003fc0000000000ull,
-        .cfam_id    = 0x120d304980000000ull,
-        .first_core = 0x1,
-    },
-    {
-        .chip_type  = PNV_CHIP_POWER9,
-        .cpu_model  = "POWER9",
-        .xscom_base = 0x000603fc00000000ull,
-        .cfam_id    = 0x220d104900008000ull,
-        .first_core = 0x0,
-    },
-    {
-        .chip_type  = PNV_CHIP_POWER10,
-        .cpu_model  = "POWER10",
-        .xscom_base = 0x000603fc00000000ull,
-        .cfam_id    = 0x120da04900008000ull,
-        .first_core = 0x0,
-    },
-};
-
-static uint64_t pnv_xscom_addr(const PnvChip *chip, uint32_t pcba)
-{
-    uint64_t addr = chip->xscom_base;
-
-    if (chip->chip_type == PNV_CHIP_POWER10) {
-        addr |= ((uint64_t) pcba << 3);
-    } else if (chip->chip_type == PNV_CHIP_POWER9) {
-        addr |= ((uint64_t) pcba << 3);
-    } else {
-        addr |= (((uint64_t) pcba << 4) & ~0xffull) |
-            (((uint64_t) pcba << 3) & 0x78);
-    }
-    return addr;
-}
+#include "pnv-xscom.h"
 
 static uint64_t pnv_xscom_read(QTestState *qts, const PnvChip *chip,
                                uint32_t pcba)
diff --git a/tests/qtest/pnv-xscom.h b/tests/qtest/pnv-xscom.h
new file mode 100644
index 0000000000..6f62941744
--- /dev/null
+++ b/tests/qtest/pnv-xscom.h
@@ -0,0 +1,80 @@
+/*
+ * PowerNV XSCOM Bus
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef PNV_XSCOM_H
+#define PNV_XSCOM_H
+
+#define SMT                     4 /* some tests will break if less than 4 */
+
+typedef enum PnvChipType {
+    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
+    PNV_CHIP_POWER8,      /* AKA Venice */
+    PNV_CHIP_POWER8NVL,   /* AKA Naples */
+    PNV_CHIP_POWER9,      /* AKA Nimbus */
+    PNV_CHIP_POWER10,
+} PnvChipType;
+
+typedef struct PnvChip {
+    PnvChipType chip_type;
+    const char *cpu_model;
+    uint64_t    xscom_base;
+    uint64_t    cfam_id;
+    uint32_t    first_core;
+    uint32_t    num_i2c;
+} PnvChip;
+
+static const PnvChip pnv_chips[] = {
+    {
+        .chip_type  = PNV_CHIP_POWER8,
+        .cpu_model  = "POWER8",
+        .xscom_base = 0x0003fc0000000000ull,
+        .cfam_id    = 0x220ea04980000000ull,
+        .first_core = 0x1,
+        .num_i2c    = 0,
+    }, {
+        .chip_type  = PNV_CHIP_POWER8NVL,
+        .cpu_model  = "POWER8NVL",
+        .xscom_base = 0x0003fc0000000000ull,
+        .cfam_id    = 0x120d304980000000ull,
+        .first_core = 0x1,
+        .num_i2c    = 0,
+    },
+    {
+        .chip_type  = PNV_CHIP_POWER9,
+        .cpu_model  = "POWER9",
+        .xscom_base = 0x000603fc00000000ull,
+        .cfam_id    = 0x220d104900008000ull,
+        .first_core = 0x0,
+        .num_i2c    = 4,
+    },
+    {
+        .chip_type  = PNV_CHIP_POWER10,
+        .cpu_model  = "POWER10",
+        .xscom_base = 0x000603fc00000000ull,
+        .cfam_id    = 0x120da04900008000ull,
+        .first_core = 0x0,
+        .num_i2c    = 4,
+    },
+};
+
+static inline uint64_t pnv_xscom_addr(const PnvChip *chip, uint32_t pcba)
+{
+    uint64_t addr = chip->xscom_base;
+
+    if (chip->chip_type == PNV_CHIP_POWER10) {
+        addr |= ((uint64_t) pcba << 3);
+    } else if (chip->chip_type == PNV_CHIP_POWER9) {
+        addr |= ((uint64_t) pcba << 3);
+    } else {
+        addr |= (((uint64_t) pcba << 4) & ~0xffull) |
+            (((uint64_t) pcba << 3) & 0x78);
+    }
+    return addr;
+}
+
+#endif /* PNV_XSCOM_H */
-- 
2.31.1



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

* Re: [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices
  2024-01-25 22:48 ` [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices Glenn Miles
@ 2024-01-26  7:52   ` Cédric Le Goater
  0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2024-01-26  7:52 UTC (permalink / raw)
  To: Glenn Miles, qemu-devel, qemu-ppc
  Cc: Nicholas Piggin, Frédéric Barrat, Reza Arbab

On 1/25/24 23:48, Glenn Miles wrote:
> Tests the following for both P9 and P10:
>    - I2C master POR status
>    - I2C master status after immediate reset
> 
> Tests the following for powernv10-ranier only:
>    - Config pca9552 hotplug device pins as inputs then
>      Read the INPUT0/1 registers to verify all pins are high
>    - Connected GPIO pin tests of P10 PCA9552 device.  Tests
>      output of pins 0-4 affect input of pins 5-9 respectively.
>    - PCA9554 GPIO pins test.  Tests input and ouput functionality.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> 
> Changes from previous version:
>    - Moved PNV I2C register definitions from pnv-host-i2c-test.c and
>      pnv_i2c.c into pnv_i2c_regs.h.
>    - Moved PNV XSCOM definitions from pnv-host-i2c-test.c and
>      pnv-xscom-test.c into pnv-xscom.h.
>    - Renamed pnv_i2c_dev_t to PnvI2cDev.
>    - Added PnvI2cCtlr structure for conveniece in passing parameters.

LGTM. Thanks for doing the proposed changes.

If you add more tests in the future, you could consider splitting "host-i2c"
in subtests. No need to resend for that.

C.


> 
>   hw/ppc/pnv_i2c.c                | 131 +--------
>   include/hw/i2c/pnv_i2c_regs.h   | 143 ++++++++++
>   tests/qtest/meson.build         |   1 +
>   tests/qtest/pnv-host-i2c-test.c | 491 ++++++++++++++++++++++++++++++++
>   tests/qtest/pnv-xscom-test.c    |  61 +---
>   tests/qtest/pnv-xscom.h         |  80 ++++++
>   6 files changed, 717 insertions(+), 190 deletions(-)
>   create mode 100644 include/hw/i2c/pnv_i2c_regs.h
>   create mode 100644 tests/qtest/pnv-host-i2c-test.c
>   create mode 100644 tests/qtest/pnv-xscom.h
> 
> diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
> index 774946d6b2..4581cc5e5d 100644
> --- a/hw/ppc/pnv_i2c.c
> +++ b/hw/ppc/pnv_i2c.c
> @@ -22,136 +22,7 @@
>   
>   #include <libfdt.h>
>   
> -/* I2C FIFO register */
> -#define I2C_FIFO_REG                    0x4
> -#define I2C_FIFO                        PPC_BITMASK(0, 7)
> -
> -/* I2C command register */
> -#define I2C_CMD_REG                     0x5
> -#define I2C_CMD_WITH_START              PPC_BIT(0)
> -#define I2C_CMD_WITH_ADDR               PPC_BIT(1)
> -#define I2C_CMD_READ_CONT               PPC_BIT(2)
> -#define I2C_CMD_WITH_STOP               PPC_BIT(3)
> -#define I2C_CMD_INTR_STEERING           PPC_BITMASK(6, 7) /* P9 */
> -#define   I2C_CMD_INTR_STEER_HOST       1
> -#define   I2C_CMD_INTR_STEER_OCC        2
> -#define I2C_CMD_DEV_ADDR                PPC_BITMASK(8, 14)
> -#define I2C_CMD_READ_NOT_WRITE          PPC_BIT(15)
> -#define I2C_CMD_LEN_BYTES               PPC_BITMASK(16, 31)
> -#define I2C_MAX_TFR_LEN                 0xfff0ull
> -
> -/* I2C mode register */
> -#define I2C_MODE_REG                    0x6
> -#define I2C_MODE_BIT_RATE_DIV           PPC_BITMASK(0, 15)
> -#define I2C_MODE_PORT_NUM               PPC_BITMASK(16, 21)
> -#define I2C_MODE_ENHANCED               PPC_BIT(28)
> -#define I2C_MODE_DIAGNOSTIC             PPC_BIT(29)
> -#define I2C_MODE_PACING_ALLOW           PPC_BIT(30)
> -#define I2C_MODE_WRAP                   PPC_BIT(31)
> -
> -/* I2C watermark register */
> -#define I2C_WATERMARK_REG               0x7
> -#define I2C_WATERMARK_HIGH              PPC_BITMASK(16, 19)
> -#define I2C_WATERMARK_LOW               PPC_BITMASK(24, 27)
> -
> -/*
> - * I2C interrupt mask and condition registers
> - *
> - * NB: The function of 0x9 and 0xa changes depending on whether you're reading
> - *     or writing to them. When read they return the interrupt condition bits
> - *     and on writes they update the interrupt mask register.
> - *
> - *  The bit definitions are the same for all the interrupt registers.
> - */
> -#define I2C_INTR_MASK_REG               0x8
> -
> -#define I2C_INTR_RAW_COND_REG           0x9 /* read */
> -#define I2C_INTR_MASK_OR_REG            0x9 /* write*/
> -
> -#define I2C_INTR_COND_REG               0xa /* read */
> -#define I2C_INTR_MASK_AND_REG           0xa /* write */
> -
> -#define I2C_INTR_ALL                    PPC_BITMASK(16, 31)
> -#define I2C_INTR_INVALID_CMD            PPC_BIT(16)
> -#define I2C_INTR_LBUS_PARITY_ERR        PPC_BIT(17)
> -#define I2C_INTR_BKEND_OVERRUN_ERR      PPC_BIT(18)
> -#define I2C_INTR_BKEND_ACCESS_ERR       PPC_BIT(19)
> -#define I2C_INTR_ARBT_LOST_ERR          PPC_BIT(20)
> -#define I2C_INTR_NACK_RCVD_ERR          PPC_BIT(21)
> -#define I2C_INTR_DATA_REQ               PPC_BIT(22)
> -#define I2C_INTR_CMD_COMP               PPC_BIT(23)
> -#define I2C_INTR_STOP_ERR               PPC_BIT(24)
> -#define I2C_INTR_I2C_BUSY               PPC_BIT(25)
> -#define I2C_INTR_NOT_I2C_BUSY           PPC_BIT(26)
> -#define I2C_INTR_SCL_EQ_1               PPC_BIT(28)
> -#define I2C_INTR_SCL_EQ_0               PPC_BIT(29)
> -#define I2C_INTR_SDA_EQ_1               PPC_BIT(30)
> -#define I2C_INTR_SDA_EQ_0               PPC_BIT(31)
> -
> -/* I2C status register */
> -#define I2C_RESET_I2C_REG               0xb /* write */
> -#define I2C_RESET_ERRORS                0xc
> -#define I2C_STAT_REG                    0xb /* read */
> -#define I2C_STAT_INVALID_CMD            PPC_BIT(0)
> -#define I2C_STAT_LBUS_PARITY_ERR        PPC_BIT(1)
> -#define I2C_STAT_BKEND_OVERRUN_ERR      PPC_BIT(2)
> -#define I2C_STAT_BKEND_ACCESS_ERR       PPC_BIT(3)
> -#define I2C_STAT_ARBT_LOST_ERR          PPC_BIT(4)
> -#define I2C_STAT_NACK_RCVD_ERR          PPC_BIT(5)
> -#define I2C_STAT_DATA_REQ               PPC_BIT(6)
> -#define I2C_STAT_CMD_COMP               PPC_BIT(7)
> -#define I2C_STAT_STOP_ERR               PPC_BIT(8)
> -#define I2C_STAT_UPPER_THRS             PPC_BITMASK(9, 15)
> -#define I2C_STAT_ANY_I2C_INTR           PPC_BIT(16)
> -#define I2C_STAT_PORT_HISTORY_BUSY      PPC_BIT(19)
> -#define I2C_STAT_SCL_INPUT_LEVEL        PPC_BIT(20)
> -#define I2C_STAT_SDA_INPUT_LEVEL        PPC_BIT(21)
> -#define I2C_STAT_PORT_BUSY              PPC_BIT(22)
> -#define I2C_STAT_INTERFACE_BUSY         PPC_BIT(23)
> -#define I2C_STAT_FIFO_ENTRY_COUNT       PPC_BITMASK(24, 31)
> -
> -#define I2C_STAT_ANY_ERR (I2C_STAT_INVALID_CMD | I2C_STAT_LBUS_PARITY_ERR | \
> -                          I2C_STAT_BKEND_OVERRUN_ERR | \
> -                          I2C_STAT_BKEND_ACCESS_ERR | I2C_STAT_ARBT_LOST_ERR | \
> -                          I2C_STAT_NACK_RCVD_ERR | I2C_STAT_STOP_ERR)
> -
> -
> -#define I2C_INTR_ACTIVE \
> -        ((I2C_STAT_ANY_ERR >> 16) | I2C_INTR_CMD_COMP | I2C_INTR_DATA_REQ)
> -
> -/* Pseudo-status used for timeouts */
> -#define I2C_STAT_PSEUDO_TIMEOUT         PPC_BIT(63)
> -
> -/* I2C extended status register */
> -#define I2C_EXTD_STAT_REG               0xc
> -#define I2C_EXTD_STAT_FIFO_SIZE         PPC_BITMASK(0, 7)
> -#define I2C_EXTD_STAT_MSM_CURSTATE      PPC_BITMASK(11, 15)
> -#define I2C_EXTD_STAT_SCL_IN_SYNC       PPC_BIT(16)
> -#define I2C_EXTD_STAT_SDA_IN_SYNC       PPC_BIT(17)
> -#define I2C_EXTD_STAT_S_SCL             PPC_BIT(18)
> -#define I2C_EXTD_STAT_S_SDA             PPC_BIT(19)
> -#define I2C_EXTD_STAT_M_SCL             PPC_BIT(20)
> -#define I2C_EXTD_STAT_M_SDA             PPC_BIT(21)
> -#define I2C_EXTD_STAT_HIGH_WATER        PPC_BIT(22)
> -#define I2C_EXTD_STAT_LOW_WATER         PPC_BIT(23)
> -#define I2C_EXTD_STAT_I2C_BUSY          PPC_BIT(24)
> -#define I2C_EXTD_STAT_SELF_BUSY         PPC_BIT(25)
> -#define I2C_EXTD_STAT_I2C_VERSION       PPC_BITMASK(27, 31)
> -
> -/* I2C residual front end/back end length */
> -#define I2C_RESIDUAL_LEN_REG            0xd
> -#define I2C_RESIDUAL_FRONT_END          PPC_BITMASK(0, 15)
> -#define I2C_RESIDUAL_BACK_END           PPC_BITMASK(16, 31)
> -
> -/* Port busy register */
> -#define I2C_PORT_BUSY_REG               0xe
> -#define I2C_SET_S_SCL_REG               0xd
> -#define I2C_RESET_S_SCL_REG             0xf
> -#define I2C_SET_S_SDA_REG               0x10
> -#define I2C_RESET_S_SDA_REG             0x11
> -
> -#define PNV_I2C_FIFO_SIZE 8
> -#define PNV_I2C_MAX_BUSSES 64
> +#include "hw/i2c/pnv_i2c_regs.h"
>   
>   static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c)
>   {
> diff --git a/include/hw/i2c/pnv_i2c_regs.h b/include/hw/i2c/pnv_i2c_regs.h
> new file mode 100644
> index 0000000000..85e96ff480
> --- /dev/null
> +++ b/include/hw/i2c/pnv_i2c_regs.h
> @@ -0,0 +1,143 @@
> +/*
> + * PowerNV I2C Controller Register Definitions
> + *
> + * Copyright (c) 2024, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef PNV_I2C_REGS_H
> +#define PNV_I2C_REGS_H
> +
> +/* I2C FIFO register */
> +#define I2C_FIFO_REG                    0x4
> +#define I2C_FIFO                        PPC_BITMASK(0, 7)
> +
> +/* I2C command register */
> +#define I2C_CMD_REG                     0x5
> +#define I2C_CMD_WITH_START              PPC_BIT(0)
> +#define I2C_CMD_WITH_ADDR               PPC_BIT(1)
> +#define I2C_CMD_READ_CONT               PPC_BIT(2)
> +#define I2C_CMD_WITH_STOP               PPC_BIT(3)
> +#define I2C_CMD_INTR_STEERING           PPC_BITMASK(6, 7) /* P9 */
> +#define   I2C_CMD_INTR_STEER_HOST       1
> +#define   I2C_CMD_INTR_STEER_OCC        2
> +#define I2C_CMD_DEV_ADDR                PPC_BITMASK(8, 14)
> +#define I2C_CMD_READ_NOT_WRITE          PPC_BIT(15)
> +#define I2C_CMD_LEN_BYTES               PPC_BITMASK(16, 31)
> +#define I2C_MAX_TFR_LEN                 0xfff0ull
> +
> +/* I2C mode register */
> +#define I2C_MODE_REG                    0x6
> +#define I2C_MODE_BIT_RATE_DIV           PPC_BITMASK(0, 15)
> +#define I2C_MODE_PORT_NUM               PPC_BITMASK(16, 21)
> +#define I2C_MODE_ENHANCED               PPC_BIT(28)
> +#define I2C_MODE_DIAGNOSTIC             PPC_BIT(29)
> +#define I2C_MODE_PACING_ALLOW           PPC_BIT(30)
> +#define I2C_MODE_WRAP                   PPC_BIT(31)
> +
> +/* I2C watermark register */
> +#define I2C_WATERMARK_REG               0x7
> +#define I2C_WATERMARK_HIGH              PPC_BITMASK(16, 19)
> +#define I2C_WATERMARK_LOW               PPC_BITMASK(24, 27)
> +
> +/*
> + * I2C interrupt mask and condition registers
> + *
> + * NB: The function of 0x9 and 0xa changes depending on whether you're reading
> + *     or writing to them. When read they return the interrupt condition bits
> + *     and on writes they update the interrupt mask register.
> + *
> + *  The bit definitions are the same for all the interrupt registers.
> + */
> +#define I2C_INTR_MASK_REG               0x8
> +
> +#define I2C_INTR_RAW_COND_REG           0x9 /* read */
> +#define I2C_INTR_MASK_OR_REG            0x9 /* write*/
> +
> +#define I2C_INTR_COND_REG               0xa /* read */
> +#define I2C_INTR_MASK_AND_REG           0xa /* write */
> +
> +#define I2C_INTR_ALL                    PPC_BITMASK(16, 31)
> +#define I2C_INTR_INVALID_CMD            PPC_BIT(16)
> +#define I2C_INTR_LBUS_PARITY_ERR        PPC_BIT(17)
> +#define I2C_INTR_BKEND_OVERRUN_ERR      PPC_BIT(18)
> +#define I2C_INTR_BKEND_ACCESS_ERR       PPC_BIT(19)
> +#define I2C_INTR_ARBT_LOST_ERR          PPC_BIT(20)
> +#define I2C_INTR_NACK_RCVD_ERR          PPC_BIT(21)
> +#define I2C_INTR_DATA_REQ               PPC_BIT(22)
> +#define I2C_INTR_CMD_COMP               PPC_BIT(23)
> +#define I2C_INTR_STOP_ERR               PPC_BIT(24)
> +#define I2C_INTR_I2C_BUSY               PPC_BIT(25)
> +#define I2C_INTR_NOT_I2C_BUSY           PPC_BIT(26)
> +#define I2C_INTR_SCL_EQ_1               PPC_BIT(28)
> +#define I2C_INTR_SCL_EQ_0               PPC_BIT(29)
> +#define I2C_INTR_SDA_EQ_1               PPC_BIT(30)
> +#define I2C_INTR_SDA_EQ_0               PPC_BIT(31)
> +
> +/* I2C status register */
> +#define I2C_RESET_I2C_REG               0xb /* write */
> +#define I2C_RESET_ERRORS                0xc
> +#define I2C_STAT_REG                    0xb /* read */
> +#define I2C_STAT_INVALID_CMD            PPC_BIT(0)
> +#define I2C_STAT_LBUS_PARITY_ERR        PPC_BIT(1)
> +#define I2C_STAT_BKEND_OVERRUN_ERR      PPC_BIT(2)
> +#define I2C_STAT_BKEND_ACCESS_ERR       PPC_BIT(3)
> +#define I2C_STAT_ARBT_LOST_ERR          PPC_BIT(4)
> +#define I2C_STAT_NACK_RCVD_ERR          PPC_BIT(5)
> +#define I2C_STAT_DATA_REQ               PPC_BIT(6)
> +#define I2C_STAT_CMD_COMP               PPC_BIT(7)
> +#define I2C_STAT_STOP_ERR               PPC_BIT(8)
> +#define I2C_STAT_UPPER_THRS             PPC_BITMASK(9, 15)
> +#define I2C_STAT_ANY_I2C_INTR           PPC_BIT(16)
> +#define I2C_STAT_PORT_HISTORY_BUSY      PPC_BIT(19)
> +#define I2C_STAT_SCL_INPUT_LEVEL        PPC_BIT(20)
> +#define I2C_STAT_SDA_INPUT_LEVEL        PPC_BIT(21)
> +#define I2C_STAT_PORT_BUSY              PPC_BIT(22)
> +#define I2C_STAT_INTERFACE_BUSY         PPC_BIT(23)
> +#define I2C_STAT_FIFO_ENTRY_COUNT       PPC_BITMASK(24, 31)
> +
> +#define I2C_STAT_ANY_ERR (I2C_STAT_INVALID_CMD | I2C_STAT_LBUS_PARITY_ERR | \
> +                          I2C_STAT_BKEND_OVERRUN_ERR | \
> +                          I2C_STAT_BKEND_ACCESS_ERR | I2C_STAT_ARBT_LOST_ERR | \
> +                          I2C_STAT_NACK_RCVD_ERR | I2C_STAT_STOP_ERR)
> +
> +
> +#define I2C_INTR_ACTIVE \
> +        ((I2C_STAT_ANY_ERR >> 16) | I2C_INTR_CMD_COMP | I2C_INTR_DATA_REQ)
> +
> +/* Pseudo-status used for timeouts */
> +#define I2C_STAT_PSEUDO_TIMEOUT         PPC_BIT(63)
> +
> +/* I2C extended status register */
> +#define I2C_EXTD_STAT_REG               0xc
> +#define I2C_EXTD_STAT_FIFO_SIZE         PPC_BITMASK(0, 7)
> +#define I2C_EXTD_STAT_MSM_CURSTATE      PPC_BITMASK(11, 15)
> +#define I2C_EXTD_STAT_SCL_IN_SYNC       PPC_BIT(16)
> +#define I2C_EXTD_STAT_SDA_IN_SYNC       PPC_BIT(17)
> +#define I2C_EXTD_STAT_S_SCL             PPC_BIT(18)
> +#define I2C_EXTD_STAT_S_SDA             PPC_BIT(19)
> +#define I2C_EXTD_STAT_M_SCL             PPC_BIT(20)
> +#define I2C_EXTD_STAT_M_SDA             PPC_BIT(21)
> +#define I2C_EXTD_STAT_HIGH_WATER        PPC_BIT(22)
> +#define I2C_EXTD_STAT_LOW_WATER         PPC_BIT(23)
> +#define I2C_EXTD_STAT_I2C_BUSY          PPC_BIT(24)
> +#define I2C_EXTD_STAT_SELF_BUSY         PPC_BIT(25)
> +#define I2C_EXTD_STAT_I2C_VERSION       PPC_BITMASK(27, 31)
> +
> +/* I2C residual front end/back end length */
> +#define I2C_RESIDUAL_LEN_REG            0xd
> +#define I2C_RESIDUAL_FRONT_END          PPC_BITMASK(0, 15)
> +#define I2C_RESIDUAL_BACK_END           PPC_BITMASK(16, 31)
> +
> +/* Port busy register */
> +#define I2C_PORT_BUSY_REG               0xe
> +#define I2C_SET_S_SCL_REG               0xd
> +#define I2C_RESET_S_SCL_REG             0xf
> +#define I2C_SET_S_SDA_REG               0x10
> +#define I2C_RESET_S_SDA_REG             0x11
> +
> +#define PNV_I2C_FIFO_SIZE 8
> +#define PNV_I2C_MAX_BUSSES 64
> +
> +#endif /* PNV_I2C_REGS_H */
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 84a055a7d9..bde1e04990 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -167,6 +167,7 @@ qtests_ppc64 = \
>     qtests_ppc + \
>     (config_all_devices.has_key('CONFIG_PSERIES') ? ['device-plug-test'] : []) +               \
>     (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-xscom-test'] : []) +                 \
> +  (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-host-i2c-test'] : []) +              \
>     (config_all_devices.has_key('CONFIG_PSERIES') ? ['rtas-test'] : []) +                      \
>     (slirp.found() ? ['pxe-test'] : []) +              \
>     (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) +             \
> diff --git a/tests/qtest/pnv-host-i2c-test.c b/tests/qtest/pnv-host-i2c-test.c
> new file mode 100644
> index 0000000000..c635177252
> --- /dev/null
> +++ b/tests/qtest/pnv-host-i2c-test.c
> @@ -0,0 +1,491 @@
> +/*
> + * QTest testcase for PowerNV 10 Host I2C Communications
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later. See the COPYING file in the top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "hw/misc/pca9554_regs.h"
> +#include "hw/misc/pca9552_regs.h"
> +#include "pnv-xscom.h"
> +
> +#define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
> +#define PPC_BIT32(bit)          (0x80000000 >> (bit))
> +#define PPC_BIT8(bit)           (0x80 >> (bit))
> +#define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> +#define PPC_BITMASK32(bs, be)   ((PPC_BIT32(bs) - PPC_BIT32(be)) | \
> +                                 PPC_BIT32(bs))
> +
> +#define MASK_TO_LSH(m)          (__builtin_ffsll(m) - 1)
> +#define GETFIELD(m, v)          (((v) & (m)) >> MASK_TO_LSH(m))
> +#define SETFIELD(m, v, val) \
> +        (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
> +
> +#define PNV10_XSCOM_I2CM_BASE   0xa0000
> +#define PNV10_XSCOM_I2CM_SIZE   0x1000
> +
> +#include "hw/i2c/pnv_i2c_regs.h"
> +
> +typedef struct {
> +    QTestState    *qts;
> +    const PnvChip *chip;
> +    int           engine;
> +} PnvI2cCtlr;
> +
> +typedef struct {
> +    PnvI2cCtlr  *ctlr;
> +    int         port;
> +    uint8_t     addr;
> +} PnvI2cDev;
> +
> +
> +static uint64_t pnv_i2c_xscom_addr(PnvI2cCtlr *ctlr, uint32_t reg)
> +{
> +    return pnv_xscom_addr(ctlr->chip, PNV10_XSCOM_I2CM_BASE +
> +                          (PNV10_XSCOM_I2CM_SIZE * ctlr->engine) + reg);
> +}
> +
> +static uint64_t pnv_i2c_xscom_read(PnvI2cCtlr *ctlr, uint32_t reg)
> +{
> +    return qtest_readq(ctlr->qts, pnv_i2c_xscom_addr(ctlr, reg));
> +}
> +
> +static void pnv_i2c_xscom_write(PnvI2cCtlr *ctlr, uint32_t reg, uint64_t val)
> +{
> +    qtest_writeq(ctlr->qts, pnv_i2c_xscom_addr(ctlr, reg), val);
> +}
> +
> +/* Write len bytes from buf to i2c device with given addr and port */
> +static void pnv_i2c_send(PnvI2cDev *dev, const uint8_t *buf, uint16_t len)
> +{
> +    int byte_num;
> +    uint64_t reg64;
> +
> +    /* select requested port */
> +    reg64 = SETFIELD(I2C_MODE_BIT_RATE_DIV, 0ull, 0x2be);
> +    reg64 = SETFIELD(I2C_MODE_PORT_NUM, reg64, dev->port);
> +    pnv_i2c_xscom_write(dev->ctlr, I2C_MODE_REG, reg64);
> +
> +    /* check status for cmd complete and bus idle */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
> +                    I2C_STAT_CMD_COMP);
> +
> +    /* Send start, with stop, with address and len bytes of data */
> +    reg64 = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR | I2C_CMD_WITH_STOP;
> +    reg64 = SETFIELD(I2C_CMD_DEV_ADDR, reg64, dev->addr);
> +    reg64 = SETFIELD(I2C_CMD_LEN_BYTES, reg64, len);
> +    pnv_i2c_xscom_write(dev->ctlr, I2C_CMD_REG, reg64);
> +
> +    /* check status for errors */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_STAT_ANY_ERR, ==, 0);
> +
> +    /* write data bytes to fifo register */
> +    for (byte_num = 0; byte_num < len; byte_num++) {
> +        reg64 = SETFIELD(I2C_FIFO, 0ull, buf[byte_num]);
> +        pnv_i2c_xscom_write(dev->ctlr, I2C_FIFO_REG, reg64);
> +    }
> +
> +    /* check status for cmd complete and bus idle */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
> +                    I2C_STAT_CMD_COMP);
> +}
> +
> +/* Recieve len bytes into buf from i2c device with given addr and port */
> +static void pnv_i2c_recv(PnvI2cDev *dev, uint8_t *buf, uint16_t len)
> +{
> +    int byte_num;
> +    uint64_t reg64;
> +
> +    /* select requested port */
> +    reg64 = SETFIELD(I2C_MODE_BIT_RATE_DIV, 0ull, 0x2be);
> +    reg64 = SETFIELD(I2C_MODE_PORT_NUM, reg64, dev->port);
> +    pnv_i2c_xscom_write(dev->ctlr, I2C_MODE_REG, reg64);
> +
> +    /* check status for cmd complete and bus idle */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
> +                    I2C_STAT_CMD_COMP);
> +
> +    /* Send start, with stop, with address and len bytes of data */
> +    reg64 = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR |
> +            I2C_CMD_WITH_STOP | I2C_CMD_READ_NOT_WRITE;
> +    reg64 = SETFIELD(I2C_CMD_DEV_ADDR, reg64, dev->addr);
> +    reg64 = SETFIELD(I2C_CMD_LEN_BYTES, reg64, len);
> +    pnv_i2c_xscom_write(dev->ctlr, I2C_CMD_REG, reg64);
> +
> +    /* check status for errors */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_STAT_ANY_ERR, ==, 0);
> +
> +    /* Read data bytes from fifo register */
> +    for (byte_num = 0; byte_num < len; byte_num++) {
> +        reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_FIFO_REG);
> +        buf[byte_num] = GETFIELD(I2C_FIFO, reg64);
> +    }
> +
> +    /* check status for cmd complete and bus idle */
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_EXTD_STAT_REG);
> +    g_assert_cmphex(reg64 & I2C_EXTD_STAT_I2C_BUSY, ==, 0);
> +    reg64 = pnv_i2c_xscom_read(dev->ctlr, I2C_STAT_REG);
> +    g_assert_cmphex(reg64 & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP), ==,
> +                    I2C_STAT_CMD_COMP);
> +}
> +
> +static void pnv_i2c_pca9554_default_cfg(PnvI2cDev *dev)
> +{
> +    uint8_t buf[2];
> +
> +    /* input register bits are not inverted */
> +    buf[0] = PCA9554_POLARITY;
> +    buf[1] = 0;
> +    pnv_i2c_send(dev, buf, 2);
> +
> +    /* All pins are inputs */
> +    buf[0] = PCA9554_CONFIG;
> +    buf[1] = 0xff;
> +    pnv_i2c_send(dev, buf, 2);
> +
> +    /* Output value for when pins are outputs */
> +    buf[0] = PCA9554_OUTPUT;
> +    buf[1] = 0xff;
> +    pnv_i2c_send(dev, buf, 2);
> +}
> +
> +static void pnv_i2c_pca9554_set_pin(PnvI2cDev *dev, int pin, bool high)
> +{
> +    uint8_t send_buf[2];
> +    uint8_t recv_buf[2];
> +    uint8_t mask = 0x1 << pin;
> +    uint8_t new_value = ((high) ? 1 : 0) << pin;
> +
> +    /* read current OUTPUT value */
> +    send_buf[0] = PCA9554_OUTPUT;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +
> +    /* write new OUTPUT value */
> +    send_buf[1] = (recv_buf[0] & ~mask) | new_value;
> +    pnv_i2c_send(dev, send_buf, 2);
> +
> +    /* Update config bit for output */
> +    send_buf[0] = PCA9554_CONFIG;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +    send_buf[1] = recv_buf[0] & ~mask;
> +    pnv_i2c_send(dev, send_buf, 2);
> +}
> +
> +static uint8_t pnv_i2c_pca9554_read_pins(PnvI2cDev *dev)
> +{
> +    uint8_t send_buf[1];
> +    uint8_t recv_buf[1];
> +    uint8_t inputs;
> +    send_buf[0] = PCA9554_INPUT;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +    inputs = recv_buf[0];
> +    return inputs;
> +}
> +
> +static void pnv_i2c_pca9554_flip_polarity(PnvI2cDev *dev)
> +{
> +    uint8_t recv_buf[1];
> +    uint8_t send_buf[2];
> +
> +    send_buf[0] = PCA9554_POLARITY;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +    send_buf[1] = recv_buf[0] ^ 0xff;
> +    pnv_i2c_send(dev, send_buf, 2);
> +}
> +
> +static void pnv_i2c_pca9554_default_inputs(PnvI2cDev *dev)
> +{
> +    uint8_t pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0xff);
> +}
> +
> +/* Check that setting pin values and polarity changes inputs as expected */
> +static void pnv_i2c_pca554_set_pins(PnvI2cDev *dev)
> +{
> +    uint8_t pin_values;
> +    pnv_i2c_pca9554_set_pin(dev, 0, 0);
> +    pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0xfe);
> +    pnv_i2c_pca9554_flip_polarity(dev);
> +    pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0x01);
> +    pnv_i2c_pca9554_set_pin(dev, 2, 0);
> +    pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0x05);
> +    pnv_i2c_pca9554_flip_polarity(dev);
> +    pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0xfa);
> +    pnv_i2c_pca9554_default_cfg(dev);
> +    pin_values = pnv_i2c_pca9554_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0xff);
> +}
> +
> +static void pnv_i2c_pca9552_default_cfg(PnvI2cDev *dev)
> +{
> +    uint8_t buf[2];
> +    /* configure pwm/psc regs */
> +    buf[0] = PCA9552_PSC0;
> +    buf[1] = 0xff;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_PWM0;
> +    buf[1] = 0x80;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_PSC1;
> +    buf[1] = 0xff;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_PWM1;
> +    buf[1] = 0x80;
> +    pnv_i2c_send(dev, buf, 2);
> +
> +    /* configure all pins as inputs */
> +    buf[0] = PCA9552_LS0;
> +    buf[1] = 0x55;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_LS1;
> +    buf[1] = 0x55;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_LS2;
> +    buf[1] = 0x55;
> +    pnv_i2c_send(dev, buf, 2);
> +    buf[0] = PCA9552_LS3;
> +    buf[1] = 0x55;
> +    pnv_i2c_send(dev, buf, 2);
> +}
> +
> +static void pnv_i2c_pca9552_set_pin(PnvI2cDev *dev, int pin, bool high)
> +{
> +    uint8_t send_buf[2];
> +    uint8_t recv_buf[2];
> +    uint8_t reg = PCA9552_LS0 + (pin / 4);
> +    uint8_t shift = (pin % 4) * 2;
> +    uint8_t mask = ~(0x3 << shift);
> +    uint8_t new_value = ((high) ? 1 : 0) << shift;
> +
> +    /* read current LSx value */
> +    send_buf[0] = reg;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +
> +    /* write new value to LSx */
> +    send_buf[1] = (recv_buf[0] & mask) | new_value;
> +    pnv_i2c_send(dev, send_buf, 2);
> +}
> +
> +static uint16_t pnv_i2c_pca9552_read_pins(PnvI2cDev *dev)
> +{
> +    uint8_t send_buf[2];
> +    uint8_t recv_buf[2];
> +    uint16_t inputs;
> +    send_buf[0] = PCA9552_INPUT0;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +    inputs = recv_buf[0];
> +    send_buf[0] = PCA9552_INPUT1;
> +    pnv_i2c_send(dev, send_buf, 1);
> +    pnv_i2c_recv(dev, recv_buf, 1);
> +    inputs |= recv_buf[0] << 8;
> +    return inputs;
> +}
> +
> +static void pnv_i2c_pca9552_default_inputs(PnvI2cDev *dev)
> +{
> +    uint16_t pin_values = pnv_i2c_pca9552_read_pins(dev);
> +    g_assert_cmphex(pin_values, ==, 0xffff);
> +}
> +
> +/*
> + * Set pins 0-4 one at a time and verify that pins 5-9 are
> + * set to the same value
> + */
> +static void pnv_i2c_pca552_set_pins(PnvI2cDev *dev)
> +{
> +    uint16_t pin_values;
> +
> +    /* set pin 0 low */
> +    pnv_i2c_pca9552_set_pin(dev, 0, 0);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* pins 0 and 5 should be low */
> +    g_assert_cmphex(pin_values, ==, 0xffde);
> +
> +    /* set pin 1 low */
> +    pnv_i2c_pca9552_set_pin(dev, 1, 0);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* pins 0, 1, 5 and 6 should be low */
> +    g_assert_cmphex(pin_values, ==, 0xff9c);
> +
> +    /* set pin 2 low */
> +    pnv_i2c_pca9552_set_pin(dev, 2, 0);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* pins 0, 1, 2, 5, 6 and 7 should be low */
> +    g_assert_cmphex(pin_values, ==, 0xff18);
> +
> +    /* set pin 3 low */
> +    pnv_i2c_pca9552_set_pin(dev, 3, 0);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* pins 0, 1, 2, 3, 5, 6, 7 and 8 should be low */
> +    g_assert_cmphex(pin_values, ==, 0xfe10);
> +
> +    /* set pin 4 low */
> +    pnv_i2c_pca9552_set_pin(dev, 4, 0);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* pins 0, 1, 2, 3, 5, 6, 7, 8 and 9 should be low */
> +    g_assert_cmphex(pin_values, ==, 0xfc00);
> +
> +    /* reset all pins to the high state */
> +    pnv_i2c_pca9552_default_cfg(dev);
> +    pin_values = pnv_i2c_pca9552_read_pins(dev);
> +
> +    /* verify all pins went back to the high state */
> +    g_assert_cmphex(pin_values, ==, 0xffff);
> +}
> +
> +static void reset_engine(PnvI2cCtlr *ctlr)
> +{
> +    pnv_i2c_xscom_write(ctlr, I2C_RESET_I2C_REG, 0);
> +}
> +
> +static void check_i2cm_por_regs(QTestState *qts, const PnvChip *chip)
> +{
> +    int engine;
> +    for (engine = 0; engine < chip->num_i2c; engine++) {
> +        PnvI2cCtlr ctlr;
> +        ctlr.qts = qts;
> +        ctlr.chip = chip;
> +        ctlr.engine = engine;
> +
> +        /* Check version in Extended Status Register */
> +        uint64_t value = pnv_i2c_xscom_read(&ctlr, I2C_EXTD_STAT_REG);
> +        g_assert_cmphex(value & I2C_EXTD_STAT_I2C_VERSION, ==, 0x1700000000);
> +
> +        /* Check for command complete and bus idle in Status Register */
> +        value = pnv_i2c_xscom_read(&ctlr, I2C_STAT_REG);
> +        g_assert_cmphex(value & (I2C_STAT_ANY_ERR | I2C_STAT_CMD_COMP),
> +                        ==,
> +                        I2C_STAT_CMD_COMP);
> +    }
> +}
> +
> +static void reset_all(QTestState *qts, const PnvChip *chip)
> +{
> +    int engine;
> +    for (engine = 0; engine < chip->num_i2c; engine++) {
> +        PnvI2cCtlr ctlr;
> +        ctlr.qts = qts;
> +        ctlr.chip = chip;
> +        ctlr.engine = engine;
> +        reset_engine(&ctlr);
> +        pnv_i2c_xscom_write(&ctlr, I2C_MODE_REG, 0x02be040000000000);
> +    }
> +}
> +
> +static void test_host_i2c(const void *data)
> +{
> +    const PnvChip *chip = data;
> +    QTestState *qts;
> +    const char *machine = "powernv8";
> +    PnvI2cCtlr ctlr;
> +    PnvI2cDev pca9552;
> +    PnvI2cDev pca9554;
> +
> +    if (chip->chip_type == PNV_CHIP_POWER9) {
> +        machine = "powernv9";
> +    } else if (chip->chip_type == PNV_CHIP_POWER10) {
> +        machine = "powernv10-rainier";
> +    }
> +
> +    qts = qtest_initf("-M %s -smp %d,cores=1,threads=%d -nographic "
> +                      "-nodefaults -serial mon:stdio -S "
> +                      "-d guest_errors",
> +                      machine, SMT, SMT);
> +
> +    /* Check the I2C master status registers after POR */
> +    check_i2cm_por_regs(qts, chip);
> +
> +    /* Now do a forced "immediate" reset on all engines */
> +    reset_all(qts, chip);
> +
> +    /* Check that the status values are still good */
> +    check_i2cm_por_regs(qts, chip);
> +
> +    /* P9 doesn't have any i2c devices attached at this time */
> +    if (chip->chip_type != PNV_CHIP_POWER10) {
> +        qtest_quit(qts);
> +        return;
> +    }
> +
> +    /* Initialize for a P10 pca9552 hotplug device */
> +    ctlr.qts = qts;
> +    ctlr.chip = chip;
> +    ctlr.engine = 2;
> +    pca9552.ctlr = &ctlr;
> +    pca9552.port = 1;
> +    pca9552.addr = 0x63;
> +
> +    /* Set all pca9552 pins as inputs */
> +    pnv_i2c_pca9552_default_cfg(&pca9552);
> +
> +    /* Check that all pins of the pca9552 are high */
> +    pnv_i2c_pca9552_default_inputs(&pca9552);
> +
> +    /* perform individual pin tests */
> +    pnv_i2c_pca552_set_pins(&pca9552);
> +
> +    /* Initialize for a P10 pca9554 CableCard Presence detection device */
> +    pca9554.ctlr = &ctlr;
> +    pca9554.port = 1;
> +    pca9554.addr = 0x25;
> +
> +    /* Set all pca9554 pins as inputs */
> +    pnv_i2c_pca9554_default_cfg(&pca9554);
> +
> +    /* Check that all pins of the pca9554 are high */
> +    pnv_i2c_pca9554_default_inputs(&pca9554);
> +
> +    /* perform individual pin tests */
> +    pnv_i2c_pca554_set_pins(&pca9554);
> +
> +    qtest_quit(qts);
> +}
> +
> +static void add_test(const char *name, void (*test)(const void *data))
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(pnv_chips); i++) {
> +        char *tname = g_strdup_printf("pnv-xscom/%s/%s", name,
> +                                      pnv_chips[i].cpu_model);
> +        qtest_add_data_func(tname, &pnv_chips[i], test);
> +        g_free(tname);
> +    }
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    add_test("host-i2c", test_host_i2c);
> +    return g_test_run();
> +}
> diff --git a/tests/qtest/pnv-xscom-test.c b/tests/qtest/pnv-xscom-test.c
> index 8a5ac11037..c814c0f4f5 100644
> --- a/tests/qtest/pnv-xscom-test.c
> +++ b/tests/qtest/pnv-xscom-test.c
> @@ -10,66 +10,7 @@
>   
>   #include "libqtest.h"
>   
> -typedef enum PnvChipType {
> -    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
> -    PNV_CHIP_POWER8,      /* AKA Venice */
> -    PNV_CHIP_POWER8NVL,   /* AKA Naples */
> -    PNV_CHIP_POWER9,      /* AKA Nimbus */
> -    PNV_CHIP_POWER10,
> -} PnvChipType;
> -
> -typedef struct PnvChip {
> -    PnvChipType chip_type;
> -    const char *cpu_model;
> -    uint64_t    xscom_base;
> -    uint64_t    cfam_id;
> -    uint32_t    first_core;
> -} PnvChip;
> -
> -static const PnvChip pnv_chips[] = {
> -    {
> -        .chip_type  = PNV_CHIP_POWER8,
> -        .cpu_model  = "POWER8",
> -        .xscom_base = 0x0003fc0000000000ull,
> -        .cfam_id    = 0x220ea04980000000ull,
> -        .first_core = 0x1,
> -    }, {
> -        .chip_type  = PNV_CHIP_POWER8NVL,
> -        .cpu_model  = "POWER8NVL",
> -        .xscom_base = 0x0003fc0000000000ull,
> -        .cfam_id    = 0x120d304980000000ull,
> -        .first_core = 0x1,
> -    },
> -    {
> -        .chip_type  = PNV_CHIP_POWER9,
> -        .cpu_model  = "POWER9",
> -        .xscom_base = 0x000603fc00000000ull,
> -        .cfam_id    = 0x220d104900008000ull,
> -        .first_core = 0x0,
> -    },
> -    {
> -        .chip_type  = PNV_CHIP_POWER10,
> -        .cpu_model  = "POWER10",
> -        .xscom_base = 0x000603fc00000000ull,
> -        .cfam_id    = 0x120da04900008000ull,
> -        .first_core = 0x0,
> -    },
> -};
> -
> -static uint64_t pnv_xscom_addr(const PnvChip *chip, uint32_t pcba)
> -{
> -    uint64_t addr = chip->xscom_base;
> -
> -    if (chip->chip_type == PNV_CHIP_POWER10) {
> -        addr |= ((uint64_t) pcba << 3);
> -    } else if (chip->chip_type == PNV_CHIP_POWER9) {
> -        addr |= ((uint64_t) pcba << 3);
> -    } else {
> -        addr |= (((uint64_t) pcba << 4) & ~0xffull) |
> -            (((uint64_t) pcba << 3) & 0x78);
> -    }
> -    return addr;
> -}
> +#include "pnv-xscom.h"
>   
>   static uint64_t pnv_xscom_read(QTestState *qts, const PnvChip *chip,
>                                  uint32_t pcba)
> diff --git a/tests/qtest/pnv-xscom.h b/tests/qtest/pnv-xscom.h
> new file mode 100644
> index 0000000000..6f62941744
> --- /dev/null
> +++ b/tests/qtest/pnv-xscom.h
> @@ -0,0 +1,80 @@
> +/*
> + * PowerNV XSCOM Bus
> + *
> + * Copyright (c) 2024, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef PNV_XSCOM_H
> +#define PNV_XSCOM_H
> +
> +#define SMT                     4 /* some tests will break if less than 4 */
> +
> +typedef enum PnvChipType {
> +    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
> +    PNV_CHIP_POWER8,      /* AKA Venice */
> +    PNV_CHIP_POWER8NVL,   /* AKA Naples */
> +    PNV_CHIP_POWER9,      /* AKA Nimbus */
> +    PNV_CHIP_POWER10,
> +} PnvChipType;
> +
> +typedef struct PnvChip {
> +    PnvChipType chip_type;
> +    const char *cpu_model;
> +    uint64_t    xscom_base;
> +    uint64_t    cfam_id;
> +    uint32_t    first_core;
> +    uint32_t    num_i2c;
> +} PnvChip;
> +
> +static const PnvChip pnv_chips[] = {
> +    {
> +        .chip_type  = PNV_CHIP_POWER8,
> +        .cpu_model  = "POWER8",
> +        .xscom_base = 0x0003fc0000000000ull,
> +        .cfam_id    = 0x220ea04980000000ull,
> +        .first_core = 0x1,
> +        .num_i2c    = 0,
> +    }, {
> +        .chip_type  = PNV_CHIP_POWER8NVL,
> +        .cpu_model  = "POWER8NVL",
> +        .xscom_base = 0x0003fc0000000000ull,
> +        .cfam_id    = 0x120d304980000000ull,
> +        .first_core = 0x1,
> +        .num_i2c    = 0,
> +    },
> +    {
> +        .chip_type  = PNV_CHIP_POWER9,
> +        .cpu_model  = "POWER9",
> +        .xscom_base = 0x000603fc00000000ull,
> +        .cfam_id    = 0x220d104900008000ull,
> +        .first_core = 0x0,
> +        .num_i2c    = 4,
> +    },
> +    {
> +        .chip_type  = PNV_CHIP_POWER10,
> +        .cpu_model  = "POWER10",
> +        .xscom_base = 0x000603fc00000000ull,
> +        .cfam_id    = 0x120da04900008000ull,
> +        .first_core = 0x0,
> +        .num_i2c    = 4,
> +    },
> +};
> +
> +static inline uint64_t pnv_xscom_addr(const PnvChip *chip, uint32_t pcba)
> +{
> +    uint64_t addr = chip->xscom_base;
> +
> +    if (chip->chip_type == PNV_CHIP_POWER10) {
> +        addr |= ((uint64_t) pcba << 3);
> +    } else if (chip->chip_type == PNV_CHIP_POWER9) {
> +        addr |= ((uint64_t) pcba << 3);
> +    } else {
> +        addr |= (((uint64_t) pcba << 4) & ~0xffull) |
> +            (((uint64_t) pcba << 3) & 0x78);
> +    }
> +    return addr;
> +}
> +
> +#endif /* PNV_XSCOM_H */



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

* Re: [PATCH v7 7/9] misc: Add a pca9554 GPIO device model
  2024-01-25 22:48 ` [PATCH v7 7/9] misc: Add a pca9554 GPIO device model Glenn Miles
@ 2024-03-04 22:32   ` Paolo Bonzini
  2024-03-05  6:20     ` Cédric Le Goater
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2024-03-04 22:32 UTC (permalink / raw)
  To: Glenn Miles, qemu-devel, qemu-ppc
  Cc: Cédric Le Goater, Nicholas Piggin, Frédéric Barrat,
	Reza Arbab

On 1/25/24 23:48, Glenn Miles wrote:
> Specs are available here:
> 
>      https://www.nxp.com/docs/en/data-sheet/PCA9554_9554A.pdf
> 
> This is a simple model supporting the basic registers for GPIO
> mode.  The device also supports an interrupt output line but the
> model does not yet support this.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> 
> No changes from previous version
> 
>   MAINTAINERS                    |  10 +-
>   hw/misc/pca9554.c              | 328 +++++++++++++++++++++++++++++++++

Not a huge deal, but this should have been in hw/gpio.

Paolo

>   include/hw/misc/pca9554.h      |  36 ++++
>   include/hw/misc/pca9554_regs.h |  19 ++
>   4 files changed, 391 insertions(+), 2 deletions(-)
>   create mode 100644 hw/misc/pca9554.c
>   create mode 100644 include/hw/misc/pca9554.h
>   create mode 100644 include/hw/misc/pca9554_regs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dfaca8323e..51861e3c7d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1169,9 +1169,7 @@ R: Joel Stanley <joel@jms.id.au>
>   L: qemu-arm@nongnu.org
>   S: Maintained
>   F: hw/*/*aspeed*
> -F: hw/misc/pca9552.c
>   F: include/hw/*/*aspeed*
> -F: include/hw/misc/pca9552*.h
>   F: hw/net/ftgmac100.c
>   F: include/hw/net/ftgmac100.h
>   F: docs/system/arm/aspeed.rst
> @@ -1540,6 +1538,14 @@ F: include/hw/pci-host/pnv*
>   F: pc-bios/skiboot.lid
>   F: tests/qtest/pnv*
>   
> +pca955x
> +M: Glenn Miles <milesg@linux.vnet.ibm.com>
> +L: qemu-ppc@nongnu.org
> +L: qemu-arm@nongnu.org
> +S: Odd Fixes
> +F: hw/misc/pca955*.c
> +F: include/hw/misc/pca955*.h
> +
>   virtex_ml507
>   M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>   L: qemu-ppc@nongnu.org
> diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
> new file mode 100644
> index 0000000000..778b32e443
> --- /dev/null
> +++ b/hw/misc/pca9554.c
> @@ -0,0 +1,328 @@
> +/*
> + * PCA9554 I/O port
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/bitops.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/misc/pca9554.h"
> +#include "hw/misc/pca9554_regs.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "trace.h"
> +#include "qom/object.h"
> +
> +struct PCA9554Class {
> +    /*< private >*/
> +    I2CSlaveClass parent_class;
> +    /*< public >*/
> +};
> +typedef struct PCA9554Class PCA9554Class;
> +
> +DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> +                       TYPE_PCA9554)
> +
> +#define PCA9554_PIN_LOW  0x0
> +#define PCA9554_PIN_HIZ  0x1
> +
> +static const char *pin_state[] = {"low", "high"};
> +
> +static void pca9554_update_pin_input(PCA9554State *s)
> +{
> +    int i;
> +    uint8_t config = s->regs[PCA9554_CONFIG];
> +    uint8_t output = s->regs[PCA9554_OUTPUT];
> +    uint8_t internal_state = config | output;
> +
> +    for (i = 0; i < PCA9554_PIN_COUNT; i++) {
> +        uint8_t bit_mask = 1 << i;
> +        uint8_t internal_pin_state = (internal_state >> i) & 0x1;
> +        uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
> +        uint8_t new_value;
> +
> +        switch (internal_pin_state) {
> +        case PCA9554_PIN_LOW:
> +            s->regs[PCA9554_INPUT] &= ~bit_mask;
> +            break;
> +        case PCA9554_PIN_HIZ:
> +            /*
> +             * pullup sets it to a logical 1 unless
> +             * external device drives it low.
> +             */
> +            if (s->ext_state[i] == PCA9554_PIN_LOW) {
> +                s->regs[PCA9554_INPUT] &= ~bit_mask;
> +            } else {
> +                s->regs[PCA9554_INPUT] |=  bit_mask;
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +
> +        /* update irq state only if pin state changed */
> +        new_value = s->regs[PCA9554_INPUT] & bit_mask;
> +        if (new_value != old_value) {
> +            if (new_value) {
> +                /* changed from 0 to 1 */
> +                qemu_set_irq(s->gpio_out[i], 1);
> +            } else {
> +                /* changed from 1 to 0 */
> +                qemu_set_irq(s->gpio_out[i], 0);
> +            }
> +        }
> +    }
> +}
> +
> +static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
> +{
> +    switch (reg) {
> +    case PCA9554_INPUT:
> +        return s->regs[PCA9554_INPUT] ^ s->regs[PCA9554_POLARITY];
> +    case PCA9554_OUTPUT:
> +    case PCA9554_POLARITY:
> +    case PCA9554_CONFIG:
> +        return s->regs[reg];
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
> +                      __func__, reg);
> +        return 0xFF;
> +    }
> +}
> +
> +static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
> +{
> +    switch (reg) {
> +    case PCA9554_OUTPUT:
> +    case PCA9554_CONFIG:
> +        s->regs[reg] = data;
> +        pca9554_update_pin_input(s);
> +        break;
> +    case PCA9554_POLARITY:
> +        s->regs[reg] = data;
> +        break;
> +    case PCA9554_INPUT:
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static uint8_t pca9554_recv(I2CSlave *i2c)
> +{
> +    PCA9554State *s = PCA9554(i2c);
> +    uint8_t ret;
> +
> +    ret = pca9554_read(s, s->pointer & 0x3);
> +
> +    return ret;
> +}
> +
> +static int pca9554_send(I2CSlave *i2c, uint8_t data)
> +{
> +    PCA9554State *s = PCA9554(i2c);
> +
> +    /* First byte sent by is the register address */
> +    if (s->len == 0) {
> +        s->pointer = data;
> +        s->len++;
> +    } else {
> +        pca9554_write(s, s->pointer & 0x3, data);
> +    }
> +
> +    return 0;
> +}
> +
> +static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    PCA9554State *s = PCA9554(i2c);
> +
> +    s->len = 0;
> +    return 0;
> +}
> +
> +static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    PCA9554State *s = PCA9554(obj);
> +    int pin, rc;
> +    uint8_t state;
> +
> +    rc = sscanf(name, "pin%2d", &pin);
> +    if (rc != 1) {
> +        error_setg(errp, "%s: error reading %s", __func__, name);
> +        return;
> +    }
> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> +        error_setg(errp, "%s invalid pin %s", __func__, name);
> +        return;
> +    }
> +
> +    state = pca9554_read(s, PCA9554_CONFIG);
> +    state |= pca9554_read(s, PCA9554_OUTPUT);
> +    state = (state >> pin) & 0x1;
> +    visit_type_str(v, name, (char **)&pin_state[state], errp);
> +}
> +
> +static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    PCA9554State *s = PCA9554(obj);
> +    int pin, rc, val;
> +    uint8_t state, mask;
> +    char *state_str;
> +
> +    if (!visit_type_str(v, name, &state_str, errp)) {
> +        return;
> +    }
> +    rc = sscanf(name, "pin%2d", &pin);
> +    if (rc != 1) {
> +        error_setg(errp, "%s: error reading %s", __func__, name);
> +        return;
> +    }
> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> +        error_setg(errp, "%s invalid pin %s", __func__, name);
> +        return;
> +    }
> +
> +    for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> +        if (!strcmp(state_str, pin_state[state])) {
> +            break;
> +        }
> +    }
> +    if (state >= ARRAY_SIZE(pin_state)) {
> +        error_setg(errp, "%s invalid pin state %s", __func__, state_str);
> +        return;
> +    }
> +
> +    /* First, modify the output register bit */
> +    val = pca9554_read(s, PCA9554_OUTPUT);
> +    mask = 0x1 << pin;
> +    if (state == PCA9554_PIN_LOW) {
> +        val &= ~(mask);
> +    } else {
> +        val |= mask;
> +    }
> +    pca9554_write(s, PCA9554_OUTPUT, val);
> +
> +    /* Then, clear the config register bit for output mode */
> +    val = pca9554_read(s, PCA9554_CONFIG);
> +    val &= ~mask;
> +    pca9554_write(s, PCA9554_CONFIG, val);
> +}
> +
> +static const VMStateDescription pca9554_vmstate = {
> +    .name = "PCA9554",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(len, PCA9554State),
> +        VMSTATE_UINT8(pointer, PCA9554State),
> +        VMSTATE_UINT8_ARRAY(regs, PCA9554State, PCA9554_NR_REGS),
> +        VMSTATE_UINT8_ARRAY(ext_state, PCA9554State, PCA9554_PIN_COUNT),
> +        VMSTATE_I2C_SLAVE(i2c, PCA9554State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pca9554_reset(DeviceState *dev)
> +{
> +    PCA9554State *s = PCA9554(dev);
> +
> +    s->regs[PCA9554_INPUT] = 0xFF;
> +    s->regs[PCA9554_OUTPUT] = 0xFF;
> +    s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
> +    s->regs[PCA9554_CONFIG] = 0xFF; /* All pins are inputs */
> +
> +    memset(s->ext_state, PCA9554_PIN_HIZ, PCA9554_PIN_COUNT);
> +    pca9554_update_pin_input(s);
> +
> +    s->pointer = 0x0;
> +    s->len = 0;
> +}
> +
> +static void pca9554_initfn(Object *obj)
> +{
> +    int pin;
> +
> +    for (pin = 0; pin < PCA9554_PIN_COUNT; pin++) {
> +        char *name;
> +
> +        name = g_strdup_printf("pin%d", pin);
> +        object_property_add(obj, name, "bool", pca9554_get_pin, pca9554_set_pin,
> +                            NULL, NULL);
> +        g_free(name);
> +    }
> +}
> +
> +static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
> +{
> +    if (s->ext_state[pin] != level) {
> +        s->ext_state[pin] = level;
> +        pca9554_update_pin_input(s);
> +    }
> +}
> +
> +static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
> +{
> +
> +    PCA9554State *s = PCA9554(opaque);
> +
> +    assert((pin >= 0) && (pin < PCA9554_PIN_COUNT));
> +    pca9554_set_ext_state(s, pin, level);
> +}
> +
> +static void pca9554_realize(DeviceState *dev, Error **errp)
> +{
> +    PCA9554State *s = PCA9554(dev);
> +
> +    if (!s->description) {
> +        s->description = g_strdup("pca9554");
> +    }
> +
> +    qdev_init_gpio_out(dev, s->gpio_out, PCA9554_PIN_COUNT);
> +    qdev_init_gpio_in(dev, pca9554_gpio_in_handler, PCA9554_PIN_COUNT);
> +}
> +
> +static Property pca9554_properties[] = {
> +    DEFINE_PROP_STRING("description", PCA9554State, description),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pca9554_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> +    k->event = pca9554_event;
> +    k->recv = pca9554_recv;
> +    k->send = pca9554_send;
> +    dc->realize = pca9554_realize;
> +    dc->reset = pca9554_reset;
> +    dc->vmsd = &pca9554_vmstate;
> +    device_class_set_props(dc, pca9554_properties);
> +}
> +
> +static const TypeInfo pca9554_info = {
> +    .name          = TYPE_PCA9554,
> +    .parent        = TYPE_I2C_SLAVE,
> +    .instance_init = pca9554_initfn,
> +    .instance_size = sizeof(PCA9554State),
> +    .class_init    = pca9554_class_init,
> +    .class_size    = sizeof(PCA9554Class),
> +    .abstract      = false,
> +};
> +
> +static void pca9554_register_types(void)
> +{
> +    type_register_static(&pca9554_info);
> +}
> +
> +type_init(pca9554_register_types)
> diff --git a/include/hw/misc/pca9554.h b/include/hw/misc/pca9554.h
> new file mode 100644
> index 0000000000..54bfc4c4c7
> --- /dev/null
> +++ b/include/hw/misc/pca9554.h
> @@ -0,0 +1,36 @@
> +/*
> + * PCA9554 I/O port
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef PCA9554_H
> +#define PCA9554_H
> +
> +#include "hw/i2c/i2c.h"
> +#include "qom/object.h"
> +
> +#define TYPE_PCA9554 "pca9554"
> +typedef struct PCA9554State PCA9554State;
> +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> +                         TYPE_PCA9554)
> +
> +#define PCA9554_NR_REGS 4
> +#define PCA9554_PIN_COUNT 8
> +
> +struct PCA9554State {
> +    /*< private >*/
> +    I2CSlave i2c;
> +    /*< public >*/
> +
> +    uint8_t len;
> +    uint8_t pointer;
> +
> +    uint8_t regs[PCA9554_NR_REGS];
> +    qemu_irq gpio_out[PCA9554_PIN_COUNT];
> +    uint8_t ext_state[PCA9554_PIN_COUNT];
> +    char *description; /* For debugging purpose only */
> +};
> +
> +#endif
> diff --git a/include/hw/misc/pca9554_regs.h b/include/hw/misc/pca9554_regs.h
> new file mode 100644
> index 0000000000..602c4a90e0
> --- /dev/null
> +++ b/include/hw/misc/pca9554_regs.h
> @@ -0,0 +1,19 @@
> +/*
> + * PCA9554 I/O port registers
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef PCA9554_REGS_H
> +#define PCA9554_REGS_H
> +
> +/*
> + * Bits [0:1] are used to address a specific register.
> + */
> +#define PCA9554_INPUT       0 /* read only input register */
> +#define PCA9554_OUTPUT      1 /* read/write pin output state */
> +#define PCA9554_POLARITY    2 /* Set polarity of input register */
> +#define PCA9554_CONFIG      3 /* Set pins as inputs our ouputs */
> +
> +#endif



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

* Re: [PATCH v7 7/9] misc: Add a pca9554 GPIO device model
  2024-03-04 22:32   ` Paolo Bonzini
@ 2024-03-05  6:20     ` Cédric Le Goater
  2024-03-05 17:03       ` Miles Glenn
  2024-03-05 23:38       ` Paolo Bonzini
  0 siblings, 2 replies; 15+ messages in thread
From: Cédric Le Goater @ 2024-03-05  6:20 UTC (permalink / raw)
  To: Paolo Bonzini, Glenn Miles, qemu-devel, qemu-ppc
  Cc: Nicholas Piggin, Frédéric Barrat, Reza Arbab

On 3/4/24 23:32, Paolo Bonzini wrote:
> On 1/25/24 23:48, Glenn Miles wrote:
>> Specs are available here:
>>
>>      https://www.nxp.com/docs/en/data-sheet/PCA9554_9554A.pdf
>>
>> This is a simple model supporting the basic registers for GPIO
>> mode.  The device also supports an interrupt output line but the
>> model does not yet support this.
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
>> ---
>>
>> No changes from previous version
>>
>>   MAINTAINERS                    |  10 +-
>>   hw/misc/pca9554.c              | 328 +++++++++++++++++++++++++++++++++
> 
> Not a huge deal, but this should have been in hw/gpio.

I started by putting the pca9552 model under misc :/

Glenn,

FYI, there are 2 issues reported by Coverity. See below. May be we could
move both files under gpio while at it ?

Thanks,

C.



** CID 1534919:  Error handling issues  (CHECKED_RETURN)
/builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()


________________________________________________________________________________________________________
*** CID 1534919:  Error handling issues  (CHECKED_RETURN)
/builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()
165             return;
166         }
167
168         state = pca9554_read(s, PCA9554_CONFIG);
169         state |= pca9554_read(s, PCA9554_OUTPUT);
170         state = (state >> pin) & 0x1;
>>>     CID 1534919:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "visit_type_str" without checking return value (as is done elsewhere 689 out of 740 times).
171         visit_type_str(v, name, (char **)&pin_state[state], errp);
172     }
173
174     static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
175                                 void *opaque, Error **errp)
176     {




** CID 1534917:  Integer handling issues  (BAD_SHIFT)
/builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()


________________________________________________________________________________________________________
*** CID 1534917:  Integer handling issues  (BAD_SHIFT)
/builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()
164             error_setg(errp, "%s invalid pin %s", __func__, name);
165             return;
166         }
167
168         state = pca9554_read(s, PCA9554_CONFIG);
169         state |= pca9554_read(s, PCA9554_OUTPUT);
>>>     CID 1534917:  Integer handling issues  (BAD_SHIFT)
>>>     In expression "state >> pin", right shifting "state" by more than 7 bits always yields zero.  The shift amount, "pin", is as much as 8.
170         state = (state >> pin) & 0x1;
171         visit_type_str(v, name, (char **)&pin_state[state], errp);
172     }
173
174     static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
175                                 void *opaque, Error **errp)



> 
> Paolo
> 
>>   include/hw/misc/pca9554.h      |  36 ++++
>>   include/hw/misc/pca9554_regs.h |  19 ++
>>   4 files changed, 391 insertions(+), 2 deletions(-)
>>   create mode 100644 hw/misc/pca9554.c
>>   create mode 100644 include/hw/misc/pca9554.h
>>   create mode 100644 include/hw/misc/pca9554_regs.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dfaca8323e..51861e3c7d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1169,9 +1169,7 @@ R: Joel Stanley <joel@jms.id.au>
>>   L: qemu-arm@nongnu.org
>>   S: Maintained
>>   F: hw/*/*aspeed*
>> -F: hw/misc/pca9552.c
>>   F: include/hw/*/*aspeed*
>> -F: include/hw/misc/pca9552*.h
>>   F: hw/net/ftgmac100.c
>>   F: include/hw/net/ftgmac100.h
>>   F: docs/system/arm/aspeed.rst
>> @@ -1540,6 +1538,14 @@ F: include/hw/pci-host/pnv*
>>   F: pc-bios/skiboot.lid
>>   F: tests/qtest/pnv*
>> +pca955x
>> +M: Glenn Miles <milesg@linux.vnet.ibm.com>
>> +L: qemu-ppc@nongnu.org
>> +L: qemu-arm@nongnu.org
>> +S: Odd Fixes
>> +F: hw/misc/pca955*.c
>> +F: include/hw/misc/pca955*.h
>> +
>>   virtex_ml507
>>   M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>>   L: qemu-ppc@nongnu.org
>> diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
>> new file mode 100644
>> index 0000000000..778b32e443
>> --- /dev/null
>> +++ b/hw/misc/pca9554.c
>> @@ -0,0 +1,328 @@
>> +/*
>> + * PCA9554 I/O port
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/module.h"
>> +#include "qemu/bitops.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/misc/pca9554.h"
>> +#include "hw/misc/pca9554_regs.h"
>> +#include "hw/irq.h"
>> +#include "migration/vmstate.h"
>> +#include "qapi/error.h"
>> +#include "qapi/visitor.h"
>> +#include "trace.h"
>> +#include "qom/object.h"
>> +
>> +struct PCA9554Class {
>> +    /*< private >*/
>> +    I2CSlaveClass parent_class;
>> +    /*< public >*/
>> +};
>> +typedef struct PCA9554Class PCA9554Class;
>> +
>> +DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
>> +                       TYPE_PCA9554)
>> +
>> +#define PCA9554_PIN_LOW  0x0
>> +#define PCA9554_PIN_HIZ  0x1
>> +
>> +static const char *pin_state[] = {"low", "high"};
>> +
>> +static void pca9554_update_pin_input(PCA9554State *s)
>> +{
>> +    int i;
>> +    uint8_t config = s->regs[PCA9554_CONFIG];
>> +    uint8_t output = s->regs[PCA9554_OUTPUT];
>> +    uint8_t internal_state = config | output;
>> +
>> +    for (i = 0; i < PCA9554_PIN_COUNT; i++) {
>> +        uint8_t bit_mask = 1 << i;
>> +        uint8_t internal_pin_state = (internal_state >> i) & 0x1;
>> +        uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
>> +        uint8_t new_value;
>> +
>> +        switch (internal_pin_state) {
>> +        case PCA9554_PIN_LOW:
>> +            s->regs[PCA9554_INPUT] &= ~bit_mask;
>> +            break;
>> +        case PCA9554_PIN_HIZ:
>> +            /*
>> +             * pullup sets it to a logical 1 unless
>> +             * external device drives it low.
>> +             */
>> +            if (s->ext_state[i] == PCA9554_PIN_LOW) {
>> +                s->regs[PCA9554_INPUT] &= ~bit_mask;
>> +            } else {
>> +                s->regs[PCA9554_INPUT] |=  bit_mask;
>> +            }
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +
>> +        /* update irq state only if pin state changed */
>> +        new_value = s->regs[PCA9554_INPUT] & bit_mask;
>> +        if (new_value != old_value) {
>> +            if (new_value) {
>> +                /* changed from 0 to 1 */
>> +                qemu_set_irq(s->gpio_out[i], 1);
>> +            } else {
>> +                /* changed from 1 to 0 */
>> +                qemu_set_irq(s->gpio_out[i], 0);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
>> +{
>> +    switch (reg) {
>> +    case PCA9554_INPUT:
>> +        return s->regs[PCA9554_INPUT] ^ s->regs[PCA9554_POLARITY];
>> +    case PCA9554_OUTPUT:
>> +    case PCA9554_POLARITY:
>> +    case PCA9554_CONFIG:
>> +        return s->regs[reg];
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
>> +                      __func__, reg);
>> +        return 0xFF;
>> +    }
>> +}
>> +
>> +static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
>> +{
>> +    switch (reg) {
>> +    case PCA9554_OUTPUT:
>> +    case PCA9554_CONFIG:
>> +        s->regs[reg] = data;
>> +        pca9554_update_pin_input(s);
>> +        break;
>> +    case PCA9554_POLARITY:
>> +        s->regs[reg] = data;
>> +        break;
>> +    case PCA9554_INPUT:
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
>> +                      __func__, reg);
>> +    }
>> +}
>> +
>> +static uint8_t pca9554_recv(I2CSlave *i2c)
>> +{
>> +    PCA9554State *s = PCA9554(i2c);
>> +    uint8_t ret;
>> +
>> +    ret = pca9554_read(s, s->pointer & 0x3);
>> +
>> +    return ret;
>> +}
>> +
>> +static int pca9554_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    PCA9554State *s = PCA9554(i2c);
>> +
>> +    /* First byte sent by is the register address */
>> +    if (s->len == 0) {
>> +        s->pointer = data;
>> +        s->len++;
>> +    } else {
>> +        pca9554_write(s, s->pointer & 0x3, data);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    PCA9554State *s = PCA9554(i2c);
>> +
>> +    s->len = 0;
>> +    return 0;
>> +}
>> +
>> +static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    PCA9554State *s = PCA9554(obj);
>> +    int pin, rc;
>> +    uint8_t state;
>> +
>> +    rc = sscanf(name, "pin%2d", &pin);
>> +    if (rc != 1) {
>> +        error_setg(errp, "%s: error reading %s", __func__, name);
>> +        return;
>> +    }
>> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
>> +        error_setg(errp, "%s invalid pin %s", __func__, name);
>> +        return;
>> +    }
>> +
>> +    state = pca9554_read(s, PCA9554_CONFIG);
>> +    state |= pca9554_read(s, PCA9554_OUTPUT);
>> +    state = (state >> pin) & 0x1;
>> +    visit_type_str(v, name, (char **)&pin_state[state], errp);
>> +}
>> +
>> +static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    PCA9554State *s = PCA9554(obj);
>> +    int pin, rc, val;
>> +    uint8_t state, mask;
>> +    char *state_str;
>> +
>> +    if (!visit_type_str(v, name, &state_str, errp)) {
>> +        return;
>> +    }
>> +    rc = sscanf(name, "pin%2d", &pin);
>> +    if (rc != 1) {
>> +        error_setg(errp, "%s: error reading %s", __func__, name);
>> +        return;
>> +    }
>> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
>> +        error_setg(errp, "%s invalid pin %s", __func__, name);
>> +        return;
>> +    }
>> +
>> +    for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
>> +        if (!strcmp(state_str, pin_state[state])) {
>> +            break;
>> +        }
>> +    }
>> +    if (state >= ARRAY_SIZE(pin_state)) {
>> +        error_setg(errp, "%s invalid pin state %s", __func__, state_str);
>> +        return;
>> +    }
>> +
>> +    /* First, modify the output register bit */
>> +    val = pca9554_read(s, PCA9554_OUTPUT);
>> +    mask = 0x1 << pin;
>> +    if (state == PCA9554_PIN_LOW) {
>> +        val &= ~(mask);
>> +    } else {
>> +        val |= mask;
>> +    }
>> +    pca9554_write(s, PCA9554_OUTPUT, val);
>> +
>> +    /* Then, clear the config register bit for output mode */
>> +    val = pca9554_read(s, PCA9554_CONFIG);
>> +    val &= ~mask;
>> +    pca9554_write(s, PCA9554_CONFIG, val);
>> +}
>> +
>> +static const VMStateDescription pca9554_vmstate = {
>> +    .name = "PCA9554",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(len, PCA9554State),
>> +        VMSTATE_UINT8(pointer, PCA9554State),
>> +        VMSTATE_UINT8_ARRAY(regs, PCA9554State, PCA9554_NR_REGS),
>> +        VMSTATE_UINT8_ARRAY(ext_state, PCA9554State, PCA9554_PIN_COUNT),
>> +        VMSTATE_I2C_SLAVE(i2c, PCA9554State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void pca9554_reset(DeviceState *dev)
>> +{
>> +    PCA9554State *s = PCA9554(dev);
>> +
>> +    s->regs[PCA9554_INPUT] = 0xFF;
>> +    s->regs[PCA9554_OUTPUT] = 0xFF;
>> +    s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
>> +    s->regs[PCA9554_CONFIG] = 0xFF; /* All pins are inputs */
>> +
>> +    memset(s->ext_state, PCA9554_PIN_HIZ, PCA9554_PIN_COUNT);
>> +    pca9554_update_pin_input(s);
>> +
>> +    s->pointer = 0x0;
>> +    s->len = 0;
>> +}
>> +
>> +static void pca9554_initfn(Object *obj)
>> +{
>> +    int pin;
>> +
>> +    for (pin = 0; pin < PCA9554_PIN_COUNT; pin++) {
>> +        char *name;
>> +
>> +        name = g_strdup_printf("pin%d", pin);
>> +        object_property_add(obj, name, "bool", pca9554_get_pin, pca9554_set_pin,
>> +                            NULL, NULL);
>> +        g_free(name);
>> +    }
>> +}
>> +
>> +static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
>> +{
>> +    if (s->ext_state[pin] != level) {
>> +        s->ext_state[pin] = level;
>> +        pca9554_update_pin_input(s);
>> +    }
>> +}
>> +
>> +static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
>> +{
>> +
>> +    PCA9554State *s = PCA9554(opaque);
>> +
>> +    assert((pin >= 0) && (pin < PCA9554_PIN_COUNT));
>> +    pca9554_set_ext_state(s, pin, level);
>> +}
>> +
>> +static void pca9554_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PCA9554State *s = PCA9554(dev);
>> +
>> +    if (!s->description) {
>> +        s->description = g_strdup("pca9554");
>> +    }
>> +
>> +    qdev_init_gpio_out(dev, s->gpio_out, PCA9554_PIN_COUNT);
>> +    qdev_init_gpio_in(dev, pca9554_gpio_in_handler, PCA9554_PIN_COUNT);
>> +}
>> +
>> +static Property pca9554_properties[] = {
>> +    DEFINE_PROP_STRING("description", PCA9554State, description),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pca9554_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> +
>> +    k->event = pca9554_event;
>> +    k->recv = pca9554_recv;
>> +    k->send = pca9554_send;
>> +    dc->realize = pca9554_realize;
>> +    dc->reset = pca9554_reset;
>> +    dc->vmsd = &pca9554_vmstate;
>> +    device_class_set_props(dc, pca9554_properties);
>> +}
>> +
>> +static const TypeInfo pca9554_info = {
>> +    .name          = TYPE_PCA9554,
>> +    .parent        = TYPE_I2C_SLAVE,
>> +    .instance_init = pca9554_initfn,
>> +    .instance_size = sizeof(PCA9554State),
>> +    .class_init    = pca9554_class_init,
>> +    .class_size    = sizeof(PCA9554Class),
>> +    .abstract      = false,
>> +};
>> +
>> +static void pca9554_register_types(void)
>> +{
>> +    type_register_static(&pca9554_info);
>> +}
>> +
>> +type_init(pca9554_register_types)
>> diff --git a/include/hw/misc/pca9554.h b/include/hw/misc/pca9554.h
>> new file mode 100644
>> index 0000000000..54bfc4c4c7
>> --- /dev/null
>> +++ b/include/hw/misc/pca9554.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * PCA9554 I/O port
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +#ifndef PCA9554_H
>> +#define PCA9554_H
>> +
>> +#include "hw/i2c/i2c.h"
>> +#include "qom/object.h"
>> +
>> +#define TYPE_PCA9554 "pca9554"
>> +typedef struct PCA9554State PCA9554State;
>> +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
>> +                         TYPE_PCA9554)
>> +
>> +#define PCA9554_NR_REGS 4
>> +#define PCA9554_PIN_COUNT 8
>> +
>> +struct PCA9554State {
>> +    /*< private >*/
>> +    I2CSlave i2c;
>> +    /*< public >*/
>> +
>> +    uint8_t len;
>> +    uint8_t pointer;
>> +
>> +    uint8_t regs[PCA9554_NR_REGS];
>> +    qemu_irq gpio_out[PCA9554_PIN_COUNT];
>> +    uint8_t ext_state[PCA9554_PIN_COUNT];
>> +    char *description; /* For debugging purpose only */
>> +};
>> +
>> +#endif
>> diff --git a/include/hw/misc/pca9554_regs.h b/include/hw/misc/pca9554_regs.h
>> new file mode 100644
>> index 0000000000..602c4a90e0
>> --- /dev/null
>> +++ b/include/hw/misc/pca9554_regs.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * PCA9554 I/O port registers
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +#ifndef PCA9554_REGS_H
>> +#define PCA9554_REGS_H
>> +
>> +/*
>> + * Bits [0:1] are used to address a specific register.
>> + */
>> +#define PCA9554_INPUT       0 /* read only input register */
>> +#define PCA9554_OUTPUT      1 /* read/write pin output state */
>> +#define PCA9554_POLARITY    2 /* Set polarity of input register */
>> +#define PCA9554_CONFIG      3 /* Set pins as inputs our ouputs */
>> +
>> +#endif
> 


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

* Re: [PATCH v7 7/9] misc: Add a pca9554 GPIO device model
  2024-03-05  6:20     ` Cédric Le Goater
@ 2024-03-05 17:03       ` Miles Glenn
  2024-03-05 23:38       ` Paolo Bonzini
  1 sibling, 0 replies; 15+ messages in thread
From: Miles Glenn @ 2024-03-05 17:03 UTC (permalink / raw)
  To: Cédric Le Goater, Paolo Bonzini, qemu-devel, qemu-ppc
  Cc: Nicholas Piggin, Frédéric Barrat, Reza Arbab

On Tue, 2024-03-05 at 07:20 +0100, Cédric Le Goater wrote:
> On 3/4/24 23:32, Paolo Bonzini wrote:
> > On 1/25/24 23:48, Glenn Miles wrote:
> > > Specs are available here:
> > > 
> > >      https://www.nxp.com/docs/en/data-sheet/PCA9554_9554A.pdf
> > > 
> > > This is a simple model supporting the basic registers for GPIO
> > > mode.  The device also supports an interrupt output line but the
> > > model does not yet support this.
> > > 
> > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > > ---
> > > 
> > > No changes from previous version
> > > 
> > >   MAINTAINERS                    |  10 +-
> > >   hw/misc/pca9554.c              | 328
> > > +++++++++++++++++++++++++++++++++
> > 
> > Not a huge deal, but this should have been in hw/gpio.
> 
> I started by putting the pca9552 model under misc :/
> 
> Glenn,
> 
> FYI, there are 2 issues reported by Coverity. See below. May be we
> could
> move both files under gpio while at it ?
> 
> Thanks,
> 
> C.
> 
> 

Sure, I'll get to that as soon as I can.

Glenn

> 
> ** CID 1534919:  Error handling issues  (CHECKED_RETURN)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()
> 
> 
> _____________________________________________________________________
> ___________________________________
> *** CID 1534919:  Error handling issues  (CHECKED_RETURN)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()
> 165             return;
> 166         }
> 167
> 168         state = pca9554_read(s, PCA9554_CONFIG);
> 169         state |= pca9554_read(s, PCA9554_OUTPUT);
> 170         state = (state >> pin) & 0x1;
> > > >     CID 1534919:  Error handling issues  (CHECKED_RETURN)
> > > >     Calling "visit_type_str" without checking return value (as
> > > > is done elsewhere 689 out of 740 times).
> 171         visit_type_str(v, name, (char **)&pin_state[state],
> errp);
> 172     }
> 173
> 174     static void pca9554_set_pin(Object *obj, Visitor *v, const
> char *name,
> 175                                 void *opaque, Error **errp)
> 176     {
> 
> 
> 
> 
> ** CID 1534917:  Integer handling issues  (BAD_SHIFT)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()
> 
> 
> _____________________________________________________________________
> ___________________________________
> *** CID 1534917:  Integer handling issues  (BAD_SHIFT)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()
> 164             error_setg(errp, "%s invalid pin %s", __func__,
> name);
> 165             return;
> 166         }
> 167
> 168         state = pca9554_read(s, PCA9554_CONFIG);
> 169         state |= pca9554_read(s, PCA9554_OUTPUT);
> > > >     CID 1534917:  Integer handling issues  (BAD_SHIFT)
> > > >     In expression "state >> pin", right shifting "state" by
> > > > more than 7 bits always yields zero.  The shift amount, "pin",
> > > > is as much as 8.
> 170         state = (state >> pin) & 0x1;
> 171         visit_type_str(v, name, (char **)&pin_state[state],
> errp);
> 172     }
> 173
> 174     static void pca9554_set_pin(Object *obj, Visitor *v, const
> char *name,
> 175                                 void *opaque, Error **errp)
> 
> 
> 
> > Paolo
> > 
> > >   include/hw/misc/pca9554.h      |  36 ++++
> > >   include/hw/misc/pca9554_regs.h |  19 ++
> > >   4 files changed, 391 insertions(+), 2 deletions(-)
> > >   create mode 100644 hw/misc/pca9554.c
> > >   create mode 100644 include/hw/misc/pca9554.h
> > >   create mode 100644 include/hw/misc/pca9554_regs.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index dfaca8323e..51861e3c7d 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -1169,9 +1169,7 @@ R: Joel Stanley <joel@jms.id.au>
> > >   L: qemu-arm@nongnu.org
> > >   S: Maintained
> > >   F: hw/*/*aspeed*
> > > -F: hw/misc/pca9552.c
> > >   F: include/hw/*/*aspeed*
> > > -F: include/hw/misc/pca9552*.h
> > >   F: hw/net/ftgmac100.c
> > >   F: include/hw/net/ftgmac100.h
> > >   F: docs/system/arm/aspeed.rst
> > > @@ -1540,6 +1538,14 @@ F: include/hw/pci-host/pnv*
> > >   F: pc-bios/skiboot.lid
> > >   F: tests/qtest/pnv*
> > > +pca955x
> > > +M: Glenn Miles <milesg@linux.vnet.ibm.com>
> > > +L: qemu-ppc@nongnu.org
> > > +L: qemu-arm@nongnu.org
> > > +S: Odd Fixes
> > > +F: hw/misc/pca955*.c
> > > +F: include/hw/misc/pca955*.h
> > > +
> > >   virtex_ml507
> > >   M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > >   L: qemu-ppc@nongnu.org
> > > diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
> > > new file mode 100644
> > > index 0000000000..778b32e443
> > > --- /dev/null
> > > +++ b/hw/misc/pca9554.c
> > > @@ -0,0 +1,328 @@
> > > +/*
> > > + * PCA9554 I/O port
> > > + *
> > > + * Copyright (c) 2023, IBM Corporation.
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/module.h"
> > > +#include "qemu/bitops.h"
> > > +#include "hw/qdev-properties.h"
> > > +#include "hw/misc/pca9554.h"
> > > +#include "hw/misc/pca9554_regs.h"
> > > +#include "hw/irq.h"
> > > +#include "migration/vmstate.h"
> > > +#include "qapi/error.h"
> > > +#include "qapi/visitor.h"
> > > +#include "trace.h"
> > > +#include "qom/object.h"
> > > +
> > > +struct PCA9554Class {
> > > +    /*< private >*/
> > > +    I2CSlaveClass parent_class;
> > > +    /*< public >*/
> > > +};
> > > +typedef struct PCA9554Class PCA9554Class;
> > > +
> > > +DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> > > +                       TYPE_PCA9554)
> > > +
> > > +#define PCA9554_PIN_LOW  0x0
> > > +#define PCA9554_PIN_HIZ  0x1
> > > +
> > > +static const char *pin_state[] = {"low", "high"};
> > > +
> > > +static void pca9554_update_pin_input(PCA9554State *s)
> > > +{
> > > +    int i;
> > > +    uint8_t config = s->regs[PCA9554_CONFIG];
> > > +    uint8_t output = s->regs[PCA9554_OUTPUT];
> > > +    uint8_t internal_state = config | output;
> > > +
> > > +    for (i = 0; i < PCA9554_PIN_COUNT; i++) {
> > > +        uint8_t bit_mask = 1 << i;
> > > +        uint8_t internal_pin_state = (internal_state >> i) &
> > > 0x1;
> > > +        uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
> > > +        uint8_t new_value;
> > > +
> > > +        switch (internal_pin_state) {
> > > +        case PCA9554_PIN_LOW:
> > > +            s->regs[PCA9554_INPUT] &= ~bit_mask;
> > > +            break;
> > > +        case PCA9554_PIN_HIZ:
> > > +            /*
> > > +             * pullup sets it to a logical 1 unless
> > > +             * external device drives it low.
> > > +             */
> > > +            if (s->ext_state[i] == PCA9554_PIN_LOW) {
> > > +                s->regs[PCA9554_INPUT] &= ~bit_mask;
> > > +            } else {
> > > +                s->regs[PCA9554_INPUT] |=  bit_mask;
> > > +            }
> > > +            break;
> > > +        default:
> > > +            break;
> > > +        }
> > > +
> > > +        /* update irq state only if pin state changed */
> > > +        new_value = s->regs[PCA9554_INPUT] & bit_mask;
> > > +        if (new_value != old_value) {
> > > +            if (new_value) {
> > > +                /* changed from 0 to 1 */
> > > +                qemu_set_irq(s->gpio_out[i], 1);
> > > +            } else {
> > > +                /* changed from 1 to 0 */
> > > +                qemu_set_irq(s->gpio_out[i], 0);
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
> > > +{
> > > +    switch (reg) {
> > > +    case PCA9554_INPUT:
> > > +        return s->regs[PCA9554_INPUT] ^ s-
> > > >regs[PCA9554_POLARITY];
> > > +    case PCA9554_OUTPUT:
> > > +    case PCA9554_POLARITY:
> > > +    case PCA9554_CONFIG:
> > > +        return s->regs[reg];
> > > +    default:
> > > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to
> > > register %d\n",
> > > +                      __func__, reg);
> > > +        return 0xFF;
> > > +    }
> > > +}
> > > +
> > > +static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t
> > > data)
> > > +{
> > > +    switch (reg) {
> > > +    case PCA9554_OUTPUT:
> > > +    case PCA9554_CONFIG:
> > > +        s->regs[reg] = data;
> > > +        pca9554_update_pin_input(s);
> > > +        break;
> > > +    case PCA9554_POLARITY:
> > > +        s->regs[reg] = data;
> > > +        break;
> > > +    case PCA9554_INPUT:
> > > +    default:
> > > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to
> > > register %d\n",
> > > +                      __func__, reg);
> > > +    }
> > > +}
> > > +
> > > +static uint8_t pca9554_recv(I2CSlave *i2c)
> > > +{
> > > +    PCA9554State *s = PCA9554(i2c);
> > > +    uint8_t ret;
> > > +
> > > +    ret = pca9554_read(s, s->pointer & 0x3);
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +static int pca9554_send(I2CSlave *i2c, uint8_t data)
> > > +{
> > > +    PCA9554State *s = PCA9554(i2c);
> > > +
> > > +    /* First byte sent by is the register address */
> > > +    if (s->len == 0) {
> > > +        s->pointer = data;
> > > +        s->len++;
> > > +    } else {
> > > +        pca9554_write(s, s->pointer & 0x3, data);
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
> > > +{
> > > +    PCA9554State *s = PCA9554(i2c);
> > > +
> > > +    s->len = 0;
> > > +    return 0;
> > > +}
> > > +
> > > +static void pca9554_get_pin(Object *obj, Visitor *v, const char
> > > *name,
> > > +                            void *opaque, Error **errp)
> > > +{
> > > +    PCA9554State *s = PCA9554(obj);
> > > +    int pin, rc;
> > > +    uint8_t state;
> > > +
> > > +    rc = sscanf(name, "pin%2d", &pin);
> > > +    if (rc != 1) {
> > > +        error_setg(errp, "%s: error reading %s", __func__,
> > > name);
> > > +        return;
> > > +    }
> > > +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> > > +        error_setg(errp, "%s invalid pin %s", __func__, name);
> > > +        return;
> > > +    }
> > > +
> > > +    state = pca9554_read(s, PCA9554_CONFIG);
> > > +    state |= pca9554_read(s, PCA9554_OUTPUT);
> > > +    state = (state >> pin) & 0x1;
> > > +    visit_type_str(v, name, (char **)&pin_state[state], errp);
> > > +}
> > > +
> > > +static void pca9554_set_pin(Object *obj, Visitor *v, const char
> > > *name,
> > > +                            void *opaque, Error **errp)
> > > +{
> > > +    PCA9554State *s = PCA9554(obj);
> > > +    int pin, rc, val;
> > > +    uint8_t state, mask;
> > > +    char *state_str;
> > > +
> > > +    if (!visit_type_str(v, name, &state_str, errp)) {
> > > +        return;
> > > +    }
> > > +    rc = sscanf(name, "pin%2d", &pin);
> > > +    if (rc != 1) {
> > > +        error_setg(errp, "%s: error reading %s", __func__,
> > > name);
> > > +        return;
> > > +    }
> > > +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> > > +        error_setg(errp, "%s invalid pin %s", __func__, name);
> > > +        return;
> > > +    }
> > > +
> > > +    for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> > > +        if (!strcmp(state_str, pin_state[state])) {
> > > +            break;
> > > +        }
> > > +    }
> > > +    if (state >= ARRAY_SIZE(pin_state)) {
> > > +        error_setg(errp, "%s invalid pin state %s", __func__,
> > > state_str);
> > > +        return;
> > > +    }
> > > +
> > > +    /* First, modify the output register bit */
> > > +    val = pca9554_read(s, PCA9554_OUTPUT);
> > > +    mask = 0x1 << pin;
> > > +    if (state == PCA9554_PIN_LOW) {
> > > +        val &= ~(mask);
> > > +    } else {
> > > +        val |= mask;
> > > +    }
> > > +    pca9554_write(s, PCA9554_OUTPUT, val);
> > > +
> > > +    /* Then, clear the config register bit for output mode */
> > > +    val = pca9554_read(s, PCA9554_CONFIG);
> > > +    val &= ~mask;
> > > +    pca9554_write(s, PCA9554_CONFIG, val);
> > > +}
> > > +
> > > +static const VMStateDescription pca9554_vmstate = {
> > > +    .name = "PCA9554",
> > > +    .version_id = 0,
> > > +    .minimum_version_id = 0,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT8(len, PCA9554State),
> > > +        VMSTATE_UINT8(pointer, PCA9554State),
> > > +        VMSTATE_UINT8_ARRAY(regs, PCA9554State,
> > > PCA9554_NR_REGS),
> > > +        VMSTATE_UINT8_ARRAY(ext_state, PCA9554State,
> > > PCA9554_PIN_COUNT),
> > > +        VMSTATE_I2C_SLAVE(i2c, PCA9554State),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static void pca9554_reset(DeviceState *dev)
> > > +{
> > > +    PCA9554State *s = PCA9554(dev);
> > > +
> > > +    s->regs[PCA9554_INPUT] = 0xFF;
> > > +    s->regs[PCA9554_OUTPUT] = 0xFF;
> > > +    s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
> > > +    s->regs[PCA9554_CONFIG] = 0xFF; /* All pins are inputs */
> > > +
> > > +    memset(s->ext_state, PCA9554_PIN_HIZ, PCA9554_PIN_COUNT);
> > > +    pca9554_update_pin_input(s);
> > > +
> > > +    s->pointer = 0x0;
> > > +    s->len = 0;
> > > +}
> > > +
> > > +static void pca9554_initfn(Object *obj)
> > > +{
> > > +    int pin;
> > > +
> > > +    for (pin = 0; pin < PCA9554_PIN_COUNT; pin++) {
> > > +        char *name;
> > > +
> > > +        name = g_strdup_printf("pin%d", pin);
> > > +        object_property_add(obj, name, "bool", pca9554_get_pin,
> > > pca9554_set_pin,
> > > +                            NULL, NULL);
> > > +        g_free(name);
> > > +    }
> > > +}
> > > +
> > > +static void pca9554_set_ext_state(PCA9554State *s, int pin, int
> > > level)
> > > +{
> > > +    if (s->ext_state[pin] != level) {
> > > +        s->ext_state[pin] = level;
> > > +        pca9554_update_pin_input(s);
> > > +    }
> > > +}
> > > +
> > > +static void pca9554_gpio_in_handler(void *opaque, int pin, int
> > > level)
> > > +{
> > > +
> > > +    PCA9554State *s = PCA9554(opaque);
> > > +
> > > +    assert((pin >= 0) && (pin < PCA9554_PIN_COUNT));
> > > +    pca9554_set_ext_state(s, pin, level);
> > > +}
> > > +
> > > +static void pca9554_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    PCA9554State *s = PCA9554(dev);
> > > +
> > > +    if (!s->description) {
> > > +        s->description = g_strdup("pca9554");
> > > +    }
> > > +
> > > +    qdev_init_gpio_out(dev, s->gpio_out, PCA9554_PIN_COUNT);
> > > +    qdev_init_gpio_in(dev, pca9554_gpio_in_handler,
> > > PCA9554_PIN_COUNT);
> > > +}
> > > +
> > > +static Property pca9554_properties[] = {
> > > +    DEFINE_PROP_STRING("description", PCA9554State,
> > > description),
> > > +    DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > +static void pca9554_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> > > +
> > > +    k->event = pca9554_event;
> > > +    k->recv = pca9554_recv;
> > > +    k->send = pca9554_send;
> > > +    dc->realize = pca9554_realize;
> > > +    dc->reset = pca9554_reset;
> > > +    dc->vmsd = &pca9554_vmstate;
> > > +    device_class_set_props(dc, pca9554_properties);
> > > +}
> > > +
> > > +static const TypeInfo pca9554_info = {
> > > +    .name          = TYPE_PCA9554,
> > > +    .parent        = TYPE_I2C_SLAVE,
> > > +    .instance_init = pca9554_initfn,
> > > +    .instance_size = sizeof(PCA9554State),
> > > +    .class_init    = pca9554_class_init,
> > > +    .class_size    = sizeof(PCA9554Class),
> > > +    .abstract      = false,
> > > +};
> > > +
> > > +static void pca9554_register_types(void)
> > > +{
> > > +    type_register_static(&pca9554_info);
> > > +}
> > > +
> > > +type_init(pca9554_register_types)
> > > diff --git a/include/hw/misc/pca9554.h
> > > b/include/hw/misc/pca9554.h
> > > new file mode 100644
> > > index 0000000000..54bfc4c4c7
> > > --- /dev/null
> > > +++ b/include/hw/misc/pca9554.h
> > > @@ -0,0 +1,36 @@
> > > +/*
> > > + * PCA9554 I/O port
> > > + *
> > > + * Copyright (c) 2023, IBM Corporation.
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + */
> > > +#ifndef PCA9554_H
> > > +#define PCA9554_H
> > > +
> > > +#include "hw/i2c/i2c.h"
> > > +#include "qom/object.h"
> > > +
> > > +#define TYPE_PCA9554 "pca9554"
> > > +typedef struct PCA9554State PCA9554State;
> > > +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> > > +                         TYPE_PCA9554)
> > > +
> > > +#define PCA9554_NR_REGS 4
> > > +#define PCA9554_PIN_COUNT 8
> > > +
> > > +struct PCA9554State {
> > > +    /*< private >*/
> > > +    I2CSlave i2c;
> > > +    /*< public >*/
> > > +
> > > +    uint8_t len;
> > > +    uint8_t pointer;
> > > +
> > > +    uint8_t regs[PCA9554_NR_REGS];
> > > +    qemu_irq gpio_out[PCA9554_PIN_COUNT];
> > > +    uint8_t ext_state[PCA9554_PIN_COUNT];
> > > +    char *description; /* For debugging purpose only */
> > > +};
> > > +
> > > +#endif
> > > diff --git a/include/hw/misc/pca9554_regs.h
> > > b/include/hw/misc/pca9554_regs.h
> > > new file mode 100644
> > > index 0000000000..602c4a90e0
> > > --- /dev/null
> > > +++ b/include/hw/misc/pca9554_regs.h
> > > @@ -0,0 +1,19 @@
> > > +/*
> > > + * PCA9554 I/O port registers
> > > + *
> > > + * Copyright (c) 2023, IBM Corporation.
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + */
> > > +#ifndef PCA9554_REGS_H
> > > +#define PCA9554_REGS_H
> > > +
> > > +/*
> > > + * Bits [0:1] are used to address a specific register.
> > > + */
> > > +#define PCA9554_INPUT       0 /* read only input register */
> > > +#define PCA9554_OUTPUT      1 /* read/write pin output state */
> > > +#define PCA9554_POLARITY    2 /* Set polarity of input register
> > > */
> > > +#define PCA9554_CONFIG      3 /* Set pins as inputs our ouputs
> > > */
> > > +
> > > +#endif



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

* Re: [PATCH v7 7/9] misc: Add a pca9554 GPIO device model
  2024-03-05  6:20     ` Cédric Le Goater
  2024-03-05 17:03       ` Miles Glenn
@ 2024-03-05 23:38       ` Paolo Bonzini
  1 sibling, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2024-03-05 23:38 UTC (permalink / raw)
  To: Cédric Le Goater
  Cc: Glenn Miles, qemu-devel,
	zmta06.collab.prod.int.phx2.redhat.com, list@suse.de,
	Nicholas Piggin, Frédéric Barrat, Reza Arbab

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

Il mar 5 mar 2024, 07:26 Cédric Le Goater <clg@kaod.org> ha scritto:

> ** CID 1534919:  Error handling issues  (CHECKED_RETURN)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()
>
>
>
> ________________________________________________________________________________________________________
> *** CID 1534919:  Error handling issues  (CHECKED_RETURN)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 171 in pca9554_get_pin()
> 165             return;
> 166         }
> 167
> 168         state = pca9554_read(s, PCA9554_CONFIG);
> 169         state |= pca9554_read(s, PCA9554_OUTPUT);
> 170         state = (state >> pin) & 0x1;
> >>>     CID 1534919:  Error handling issues  (CHECKED_RETURN)
> >>>     Calling "visit_type_str" without checking return value (as is done
> elsewhere 689 out of 740 times).
> 171         visit_type_str(v, name, (char **)&pin_state[state], errp);
> 172     }
>

This one is innocuous and need not be fixed (there are over 50 false
positive because generated code always checks the result and have written
code almost never needs to).

Paolo

174     static void pca9554_set_pin(Object *obj, Visitor *v, const char
> *name,
> 175                                 void *opaque, Error **errp)
> 176     {
>
>
>
>
> ** CID 1534917:  Integer handling issues  (BAD_SHIFT)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()
>
>
>
> ________________________________________________________________________________________________________
> *** CID 1534917:  Integer handling issues  (BAD_SHIFT)
> /builds/bonzini/qemu/hw/misc/pca9554.c: 170 in pca9554_get_pin()
> 164             error_setg(errp, "%s invalid pin %s", __func__, name);
> 165             return;
> 166         }
> 167
> 168         state = pca9554_read(s, PCA9554_CONFIG);
> 169         state |= pca9554_read(s, PCA9554_OUTPUT);
> >>>     CID 1534917:  Integer handling issues  (BAD_SHIFT)
> >>>     In expression "state >> pin", right shifting "state" by more than
> 7 bits always yields zero.  The shift amount, "pin", is as much as 8.
> 170         state = (state >> pin) & 0x1;
> 171         visit_type_str(v, name, (char **)&pin_state[state], errp);
> 172     }
> 173
> 174     static void pca9554_set_pin(Object *obj, Visitor *v, const char
> *name,
> 175                                 void *opaque, Error **errp)
>
>
>
> >
> > Paolo
> >
> >>   include/hw/misc/pca9554.h      |  36 ++++
> >>   include/hw/misc/pca9554_regs.h |  19 ++
> >>   4 files changed, 391 insertions(+), 2 deletions(-)
> >>   create mode 100644 hw/misc/pca9554.c
> >>   create mode 100644 include/hw/misc/pca9554.h
> >>   create mode 100644 include/hw/misc/pca9554_regs.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index dfaca8323e..51861e3c7d 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -1169,9 +1169,7 @@ R: Joel Stanley <joel@jms.id.au>
> >>   L: qemu-arm@nongnu.org
> >>   S: Maintained
> >>   F: hw/*/*aspeed*
> >> -F: hw/misc/pca9552.c
> >>   F: include/hw/*/*aspeed*
> >> -F: include/hw/misc/pca9552*.h
> >>   F: hw/net/ftgmac100.c
> >>   F: include/hw/net/ftgmac100.h
> >>   F: docs/system/arm/aspeed.rst
> >> @@ -1540,6 +1538,14 @@ F: include/hw/pci-host/pnv*
> >>   F: pc-bios/skiboot.lid
> >>   F: tests/qtest/pnv*
> >> +pca955x
> >> +M: Glenn Miles <milesg@linux.vnet.ibm.com>
> >> +L: qemu-ppc@nongnu.org
> >> +L: qemu-arm@nongnu.org
> >> +S: Odd Fixes
> >> +F: hw/misc/pca955*.c
> >> +F: include/hw/misc/pca955*.h
> >> +
> >>   virtex_ml507
> >>   M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >>   L: qemu-ppc@nongnu.org
> >> diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
> >> new file mode 100644
> >> index 0000000000..778b32e443
> >> --- /dev/null
> >> +++ b/hw/misc/pca9554.c
> >> @@ -0,0 +1,328 @@
> >> +/*
> >> + * PCA9554 I/O port
> >> + *
> >> + * Copyright (c) 2023, IBM Corporation.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "qemu/module.h"
> >> +#include "qemu/bitops.h"
> >> +#include "hw/qdev-properties.h"
> >> +#include "hw/misc/pca9554.h"
> >> +#include "hw/misc/pca9554_regs.h"
> >> +#include "hw/irq.h"
> >> +#include "migration/vmstate.h"
> >> +#include "qapi/error.h"
> >> +#include "qapi/visitor.h"
> >> +#include "trace.h"
> >> +#include "qom/object.h"
> >> +
> >> +struct PCA9554Class {
> >> +    /*< private >*/
> >> +    I2CSlaveClass parent_class;
> >> +    /*< public >*/
> >> +};
> >> +typedef struct PCA9554Class PCA9554Class;
> >> +
> >> +DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
> >> +                       TYPE_PCA9554)
> >> +
> >> +#define PCA9554_PIN_LOW  0x0
> >> +#define PCA9554_PIN_HIZ  0x1
> >> +
> >> +static const char *pin_state[] = {"low", "high"};
> >> +
> >> +static void pca9554_update_pin_input(PCA9554State *s)
> >> +{
> >> +    int i;
> >> +    uint8_t config = s->regs[PCA9554_CONFIG];
> >> +    uint8_t output = s->regs[PCA9554_OUTPUT];
> >> +    uint8_t internal_state = config | output;
> >> +
> >> +    for (i = 0; i < PCA9554_PIN_COUNT; i++) {
> >> +        uint8_t bit_mask = 1 << i;
> >> +        uint8_t internal_pin_state = (internal_state >> i) & 0x1;
> >> +        uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
> >> +        uint8_t new_value;
> >> +
> >> +        switch (internal_pin_state) {
> >> +        case PCA9554_PIN_LOW:
> >> +            s->regs[PCA9554_INPUT] &= ~bit_mask;
> >> +            break;
> >> +        case PCA9554_PIN_HIZ:
> >> +            /*
> >> +             * pullup sets it to a logical 1 unless
> >> +             * external device drives it low.
> >> +             */
> >> +            if (s->ext_state[i] == PCA9554_PIN_LOW) {
> >> +                s->regs[PCA9554_INPUT] &= ~bit_mask;
> >> +            } else {
> >> +                s->regs[PCA9554_INPUT] |=  bit_mask;
> >> +            }
> >> +            break;
> >> +        default:
> >> +            break;
> >> +        }
> >> +
> >> +        /* update irq state only if pin state changed */
> >> +        new_value = s->regs[PCA9554_INPUT] & bit_mask;
> >> +        if (new_value != old_value) {
> >> +            if (new_value) {
> >> +                /* changed from 0 to 1 */
> >> +                qemu_set_irq(s->gpio_out[i], 1);
> >> +            } else {
> >> +                /* changed from 1 to 0 */
> >> +                qemu_set_irq(s->gpio_out[i], 0);
> >> +            }
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
> >> +{
> >> +    switch (reg) {
> >> +    case PCA9554_INPUT:
> >> +        return s->regs[PCA9554_INPUT] ^ s->regs[PCA9554_POLARITY];
> >> +    case PCA9554_OUTPUT:
> >> +    case PCA9554_POLARITY:
> >> +    case PCA9554_CONFIG:
> >> +        return s->regs[reg];
> >> +    default:
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to
> register %d\n",
> >> +                      __func__, reg);
> >> +        return 0xFF;
> >> +    }
> >> +}
> >> +
> >> +static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
> >> +{
> >> +    switch (reg) {
> >> +    case PCA9554_OUTPUT:
> >> +    case PCA9554_CONFIG:
> >> +        s->regs[reg] = data;
> >> +        pca9554_update_pin_input(s);
> >> +        break;
> >> +    case PCA9554_POLARITY:
> >> +        s->regs[reg] = data;
> >> +        break;
> >> +    case PCA9554_INPUT:
> >> +    default:
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to
> register %d\n",
> >> +                      __func__, reg);
> >> +    }
> >> +}
> >> +
> >> +static uint8_t pca9554_recv(I2CSlave *i2c)
> >> +{
> >> +    PCA9554State *s = PCA9554(i2c);
> >> +    uint8_t ret;
> >> +
> >> +    ret = pca9554_read(s, s->pointer & 0x3);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static int pca9554_send(I2CSlave *i2c, uint8_t data)
> >> +{
> >> +    PCA9554State *s = PCA9554(i2c);
> >> +
> >> +    /* First byte sent by is the register address */
> >> +    if (s->len == 0) {
> >> +        s->pointer = data;
> >> +        s->len++;
> >> +    } else {
> >> +        pca9554_write(s, s->pointer & 0x3, data);
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
> >> +{
> >> +    PCA9554State *s = PCA9554(i2c);
> >> +
> >> +    s->len = 0;
> >> +    return 0;
> >> +}
> >> +
> >> +static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
> >> +                            void *opaque, Error **errp)
> >> +{
> >> +    PCA9554State *s = PCA9554(obj);
> >> +    int pin, rc;
> >> +    uint8_t state;
> >> +
> >> +    rc = sscanf(name, "pin%2d", &pin);
> >> +    if (rc != 1) {
> >> +        error_setg(errp, "%s: error reading %s", __func__, name);
> >> +        return;
> >> +    }
> >> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> >> +        error_setg(errp, "%s invalid pin %s", __func__, name);
> >> +        return;
> >> +    }
> >> +
> >> +    state = pca9554_read(s, PCA9554_CONFIG);
> >> +    state |= pca9554_read(s, PCA9554_OUTPUT);
> >> +    state = (state >> pin) & 0x1;
> >> +    visit_type_str(v, name, (char **)&pin_state[state], errp);
> >> +}
> >> +
> >> +static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
> >> +                            void *opaque, Error **errp)
> >> +{
> >> +    PCA9554State *s = PCA9554(obj);
> >> +    int pin, rc, val;
> >> +    uint8_t state, mask;
> >> +    char *state_str;
> >> +
> >> +    if (!visit_type_str(v, name, &state_str, errp)) {
> >> +        return;
> >> +    }
> >> +    rc = sscanf(name, "pin%2d", &pin);
> >> +    if (rc != 1) {
> >> +        error_setg(errp, "%s: error reading %s", __func__, name);
> >> +        return;
> >> +    }
> >> +    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> >> +        error_setg(errp, "%s invalid pin %s", __func__, name);
> >> +        return;
> >> +    }
> >> +
> >> +    for (state = 0; state < ARRAY_SIZE(pin_state); state++) {
> >> +        if (!strcmp(state_str, pin_state[state])) {
> >> +            break;
> >> +        }
> >> +    }
> >> +    if (state >= ARRAY_SIZE(pin_state)) {
> >> +        error_setg(errp, "%s invalid pin state %s", __func__,
> state_str);
> >> +        return;
> >> +    }
> >> +
> >> +    /* First, modify the output register bit */
> >> +    val = pca9554_read(s, PCA9554_OUTPUT);
> >> +    mask = 0x1 << pin;
> >> +    if (state == PCA9554_PIN_LOW) {
> >> +        val &= ~(mask);
> >> +    } else {
> >> +        val |= mask;
> >> +    }
> >> +    pca9554_write(s, PCA9554_OUTPUT, val);
> >> +
> >> +    /* Then, clear the config register bit for output mode */
> >> +    val = pca9554_read(s, PCA9554_CONFIG);
> >> +    val &= ~mask;
> >> +    pca9554_write(s, PCA9554_CONFIG, val);
> >> +}
> >> +
> >> +static const VMStateDescription pca9554_vmstate = {
> >> +    .name = "PCA9554",
> >> +    .version_id = 0,
> >> +    .minimum_version_id = 0,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT8(len, PCA9554State),
> >> +        VMSTATE_UINT8(pointer, PCA9554State),
> >> +        VMSTATE_UINT8_ARRAY(regs, PCA9554State, PCA9554_NR_REGS),
> >> +        VMSTATE_UINT8_ARRAY(ext_state, PCA9554State,
> PCA9554_PIN_COUNT),
> >> +        VMSTATE_I2C_SLAVE(i2c, PCA9554State),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >> +static void pca9554_reset(DeviceState *dev)
> >> +{
> >> +    PCA9554State *s = PCA9554(dev);
> >> +
> >> +    s->regs[PCA9554_INPUT] = 0xFF;
> >> +    s->regs[PCA9554_OUTPUT] = 0xFF;
> >> +    s->regs[PCA9554_POLARITY] = 0x0; /* No pins are inverted */
> >> +    s->regs[PCA9554_CONFIG] = 0xFF; /* All pins are inputs */
> >> +
> >> +    memset(s->ext_state, PCA9554_PIN_HIZ, PCA9554_PIN_COUNT);
> >> +    pca9554_update_pin_input(s);
> >> +
> >> +    s->pointer = 0x0;
> >> +    s->len = 0;
> >> +}
> >> +
> >> +static void pca9554_initfn(Object *obj)
> >> +{
> >> +    int pin;
> >> +
> >> +    for (pin = 0; pin < PCA9554_PIN_COUNT; pin++) {
> >> +        char *name;
> >> +
> >> +        name = g_strdup_printf("pin%d", pin);
> >> +        object_property_add(obj, name, "bool", pca9554_get_pin,
> pca9554_set_pin,
> >> +                            NULL, NULL);
> >> +        g_free(name);
> >> +    }
> >> +}
> >> +
> >> +static void pca9554_set_ext_state(PCA9554State *s, int pin, int level)
> >> +{
> >> +    if (s->ext_state[pin] != level) {
> >> +        s->ext_state[pin] = level;
> >> +        pca9554_update_pin_input(s);
> >> +    }
> >> +}
> >> +
> >> +static void pca9554_gpio_in_handler(void *opaque, int pin, int level)
> >> +{
> >> +
> >> +    PCA9554State *s = PCA9554(opaque);
> >> +
> >> +    assert((pin >= 0) && (pin < PCA9554_PIN_COUNT));
> >> +    pca9554_set_ext_state(s, pin, level);
> >> +}
> >> +
> >> +static void pca9554_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    PCA9554State *s = PCA9554(dev);
> >> +
> >> +    if (!s->description) {
> >> +        s->description = g_strdup("pca9554");
> >> +    }
> >> +
> >> +    qdev_init_gpio_out(dev, s->gpio_out, PCA9554_PIN_COUNT);
> >> +    qdev_init_gpio_in(dev, pca9554_gpio_in_handler, PCA9554_PIN_COUNT);
> >> +}
> >> +
> >> +static Property pca9554_properties[] = {
> >> +    DEFINE_PROP_STRING("description", PCA9554State, description),
> >> +    DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void pca9554_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> >> +
> >> +    k->event = pca9554_event;
> >> +    k->recv = pca9554_recv;
> >> +    k->send = pca9554_send;
> >> +    dc->realize = pca9554_realize;
> >> +    dc->reset = pca9554_reset;
> >> +    dc->vmsd = &pca9554_vmstate;
> >> +    device_class_set_props(dc, pca9554_properties);
> >> +}
> >> +
> >> +static const TypeInfo pca9554_info = {
> >> +    .name          = TYPE_PCA9554,
> >> +    .parent        = TYPE_I2C_SLAVE,
> >> +    .instance_init = pca9554_initfn,
> >> +    .instance_size = sizeof(PCA9554State),
> >> +    .class_init    = pca9554_class_init,
> >> +    .class_size    = sizeof(PCA9554Class),
> >> +    .abstract      = false,
> >> +};
> >> +
> >> +static void pca9554_register_types(void)
> >> +{
> >> +    type_register_static(&pca9554_info);
> >> +}
> >> +
> >> +type_init(pca9554_register_types)
> >> diff --git a/include/hw/misc/pca9554.h b/include/hw/misc/pca9554.h
> >> new file mode 100644
> >> index 0000000000..54bfc4c4c7
> >> --- /dev/null
> >> +++ b/include/hw/misc/pca9554.h
> >> @@ -0,0 +1,36 @@
> >> +/*
> >> + * PCA9554 I/O port
> >> + *
> >> + * Copyright (c) 2023, IBM Corporation.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + */
> >> +#ifndef PCA9554_H
> >> +#define PCA9554_H
> >> +
> >> +#include "hw/i2c/i2c.h"
> >> +#include "qom/object.h"
> >> +
> >> +#define TYPE_PCA9554 "pca9554"
> >> +typedef struct PCA9554State PCA9554State;
> >> +DECLARE_INSTANCE_CHECKER(PCA9554State, PCA9554,
> >> +                         TYPE_PCA9554)
> >> +
> >> +#define PCA9554_NR_REGS 4
> >> +#define PCA9554_PIN_COUNT 8
> >> +
> >> +struct PCA9554State {
> >> +    /*< private >*/
> >> +    I2CSlave i2c;
> >> +    /*< public >*/
> >> +
> >> +    uint8_t len;
> >> +    uint8_t pointer;
> >> +
> >> +    uint8_t regs[PCA9554_NR_REGS];
> >> +    qemu_irq gpio_out[PCA9554_PIN_COUNT];
> >> +    uint8_t ext_state[PCA9554_PIN_COUNT];
> >> +    char *description; /* For debugging purpose only */
> >> +};
> >> +
> >> +#endif
> >> diff --git a/include/hw/misc/pca9554_regs.h
> b/include/hw/misc/pca9554_regs.h
> >> new file mode 100644
> >> index 0000000000..602c4a90e0
> >> --- /dev/null
> >> +++ b/include/hw/misc/pca9554_regs.h
> >> @@ -0,0 +1,19 @@
> >> +/*
> >> + * PCA9554 I/O port registers
> >> + *
> >> + * Copyright (c) 2023, IBM Corporation.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + */
> >> +#ifndef PCA9554_REGS_H
> >> +#define PCA9554_REGS_H
> >> +
> >> +/*
> >> + * Bits [0:1] are used to address a specific register.
> >> + */
> >> +#define PCA9554_INPUT       0 /* read only input register */
> >> +#define PCA9554_OUTPUT      1 /* read/write pin output state */
> >> +#define PCA9554_POLARITY    2 /* Set polarity of input register */
> >> +#define PCA9554_CONFIG      3 /* Set pins as inputs our ouputs */
> >> +
> >> +#endif
> >
>
>

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

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

end of thread, other threads:[~2024-03-05 23:39 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-25 22:48 [PATCH v7 0/9] Add powernv10 I2C devices and tests Glenn Miles
2024-01-25 22:48 ` [PATCH v7 1/9] misc/pca9552: Fix inverted input status Glenn Miles
2024-01-25 22:48 ` [PATCH v7 2/9] misc/pca9552: Let external devices set pca9552 inputs Glenn Miles
2024-01-25 22:48 ` [PATCH v7 3/9] ppc/pnv: New powernv10-rainier machine type Glenn Miles
2024-01-25 22:48 ` [PATCH v7 4/9] ppc/pnv: Add pca9552 to powernv10-rainier for PCIe hotplug power control Glenn Miles
2024-01-25 22:48 ` [PATCH v7 5/9] ppc/pnv: Wire up pca9552 GPIO pins " Glenn Miles
2024-01-25 22:48 ` [PATCH v7 6/9] ppc/pnv: Use resettable interface to reset child I2C buses Glenn Miles
2024-01-25 22:48 ` [PATCH v7 7/9] misc: Add a pca9554 GPIO device model Glenn Miles
2024-03-04 22:32   ` Paolo Bonzini
2024-03-05  6:20     ` Cédric Le Goater
2024-03-05 17:03       ` Miles Glenn
2024-03-05 23:38       ` Paolo Bonzini
2024-01-25 22:48 ` [PATCH v7 8/9] ppc/pnv: Add a pca9554 I2C device to powernv10-rainier Glenn Miles
2024-01-25 22:48 ` [PATCH v7 9/9] ppc/pnv: Test pnv i2c master and connected devices Glenn Miles
2024-01-26  7:52   ` Cédric Le Goater

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