[PATCH v4 0/8] hw/misc/pca9552: Trace GPIO change events

[PATCH v4 0/8] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

This series add trace events to better display GPIO changes.
We'll continue in the following series by connecting LEDs to
these GPIOs.

This helps me to work on a generic LED device, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html

Example when booting an obmc-phosphor-image, we can see the LED #14
(front-power LED) starting to blink.

- ASCII LED bar view:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_status
  1592689902.327837:pca9552_gpio_status pca-unspecified GPIOs 0-15 [*...............]
  1592689902.329934:pca9552_gpio_status pca-unspecified GPIOs 0-15 [**..............]
  1592689902.330717:pca9552_gpio_status pca-unspecified GPIOs 0-15 [***.............]
  1592689902.331431:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****............]
  1592689902.332163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
  1592689902.332888:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
  1592689902.333629:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690032.793289:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690033.303163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690033.812962:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690034.323234:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690034.832922:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]

- Only display GPIOs which status changes:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_change
  1592690552.687372:pca9552_gpio_change pca1 GPIO id:0 status: 0 -> 1
  1592690552.690169:pca9552_gpio_change pca1 GPIO id:1 status: 0 -> 1
  1592690552.691673:pca9552_gpio_change pca1 GPIO id:2 status: 0 -> 1
  1592690552.696886:pca9552_gpio_change pca1 GPIO id:3 status: 0 -> 1
  1592690552.698614:pca9552_gpio_change pca1 GPIO id:13 status: 0 -> 1
  1592690552.699833:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690552.700842:pca9552_gpio_change pca1 GPIO id:15 status: 0 -> 1
  1592690683.841921:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690683.861660:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690684.371460:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690684.882115:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690685.391411:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690685.901391:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1

For information about how to test the obmc-phosphor-image, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg712911.html

Supersedes: <20200619145101.1637-1-f4bug@amsat.org>
Based-on: <20200620162818.22340-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (8):
  hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()
  hw/misc/pca9552: Replace magic value by PCA9552_PIN_COUNT definition
  hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition
  hw/misc/pca9552: Add a 'description' property for debugging purpose
  hw/misc/pca9552: Trace GPIO High/Low events
  hw/arm/aspeed: Describe each PCA9552 device
  hw/misc/pca9552: Trace GPIO change events
  hw/misc/pca9552: Model qdev output GPIOs

 include/hw/i2c/i2c.h      |  2 ++
 include/hw/misc/pca9552.h |  4 +++
 hw/arm/aspeed.c           | 13 ++++---
 hw/i2c/core.c             | 18 ++++++++--
 hw/misc/pca9552.c         | 72 ++++++++++++++++++++++++++++++++++++++-
 hw/misc/trace-events      |  4 +++
 6 files changed, 106 insertions(+), 7 deletions(-)

-- 
2.21.3

[PATCH v4 1/8] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Philippe Mathieu-Daudé]

Extract i2c_try_create_slave() and i2c_realize_and_unref()
from i2c_create_slave().
We can now set properties on a I2CSlave before it is realized.

This is in line with the recent qdev/QOM changes merged
in commit 6675a653d2e.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i2c/i2c.h |  2 ++
 hw/i2c/core.c        | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 4117211565..d6e3d85faf 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 
 DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
+DeviceState *i2c_try_create_slave(const char *name, uint8_t addr);
+bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
 
 /* lm832x.c */
 void lm832x_key_event(DeviceState *dev, int key, int state);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 1aac457a2a..acf34a12d6 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = {
     }
 };
 
-DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
+DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
 {
     DeviceState *dev;
 
     dev = qdev_new(name);
     qdev_prop_set_uint8(dev, "address", addr);
-    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
+    return dev;
+}
+
+bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
+{
+    return qdev_realize_and_unref(dev, &bus->qbus, errp);
+}
+
+DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
+{
+    DeviceState *dev;
+
+    dev = i2c_try_create_slave(name, addr);
+    i2c_realize_and_unref(dev, bus, &error_fatal);
+
     return dev;
 }
 
-- 
2.21.3

[PATCH v4 2/8] hw/misc/pca9552: Replace magic value by PCA9552_PIN_COUNT definition

[Philippe Mathieu-Daudé]

Replace the '16' magic value by the PCA9552_PIN_COUNT definition.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h | 1 +
 hw/misc/pca9552.c         | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index ebb43c63fe..ef6da4988f 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -15,6 +15,7 @@
 #define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
 
 #define PCA9552_NR_REGS 10
+#define PCA9552_PIN_COUNT 16
 
 typedef struct PCA9552State {
     /*< private >*/
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cac729e35a..cfefb8fce8 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -291,7 +291,7 @@ static void pca9552_initfn(Object *obj)
      * PCA955X device
      */
     s->max_reg = PCA9552_LS3;
-    s->nr_leds = 16;
+    s->nr_leds = PCA9552_PIN_COUNT;
 
     for (led = 0; led < s->nr_leds; led++) {
         char *name;
-- 
2.21.3

[PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition

[Philippe Mathieu-Daudé]

The current code models the PCA9552, but there are comments
saying the code could be easily adapted for the rest of the
PCA955x family.
Since we assume we have at most 16 pins (for the PCA9552),
add a definition and check the instance doesn't use more than
this number. This makes the code a bit safer in case other
PCA955x devices are added.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/pca9552.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cfefb8fce8..b97fc2893c 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -303,6 +303,17 @@ static void pca9552_initfn(Object *obj)
     }
 }
 
+static void pca9552_realize(DeviceState *dev, Error **errp)
+{
+    PCA9552State *s = PCA9552(dev);
+
+    if (s->nr_leds > PCA9552_PIN_COUNT) {
+        error_setg(errp, "%s invalid led count %u (max: %u)",
+                   __func__, s->nr_leds, PCA9552_PIN_COUNT);
+        return;
+    }
+}
+
 static void pca9552_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -311,6 +322,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
     k->event = pca9552_event;
     k->recv = pca9552_recv;
     k->send = pca9552_send;
+    dc->realize = pca9552_realize;
     dc->reset = pca9552_reset;
     dc->vmsd = &pca9552_vmstate;
 }
-- 
2.21.3

[PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

Add a description field to distinguish between multiple devices.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h |  1 +
 hw/misc/pca9552.c         | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index ef6da4988f..c5be7f1c5e 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -27,6 +27,7 @@ typedef struct PCA9552State {
 
     uint8_t regs[PCA9552_NR_REGS];
     uint8_t max_reg;
+    char *description; /* For debugging purpose only */
     uint8_t nr_leds;
 } PCA9552State;
 
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index b97fc2893c..54ccdcf6d4 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
@@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
                    __func__, s->nr_leds, PCA9552_PIN_COUNT);
         return;
     }
+    if (!s->description) {
+        s->description = g_strdup("pca-unspecified");
+    }
 }
 
+static Property pca9552_properties[] = {
+    DEFINE_PROP_STRING("description", PCA9552State, description),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pca9552_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
     dc->realize = pca9552_realize;
     dc->reset = pca9552_reset;
     dc->vmsd = &pca9552_vmstate;
+    device_class_set_props(dc, pca9552_properties);
 }
 
 static const TypeInfo pca9552_info = {
-- 
2.21.3

[PATCH v4 5/8] hw/misc/pca9552: Trace GPIO High/Low events

[Philippe Mathieu-Daudé]

Add a trivial representation of the PCA9552 GPIOs.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_status
  1592689902.327837:pca9552_gpio_status pca-unspecified GPIOs 0-15 [*...............]
  1592689902.329934:pca9552_gpio_status pca-unspecified GPIOs 0-15 [**..............]
  1592689902.330717:pca9552_gpio_status pca-unspecified GPIOs 0-15 [***.............]
  1592689902.331431:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****............]
  1592689902.332163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
  1592689902.332888:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
  1592689902.333629:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690032.793289:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690033.303163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690033.812962:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
  1592690034.323234:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
  1592690034.832922:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]

We notice the GPIO #14 (front-power LED) starts to blink.

This LED is described in the witherspoon device-tree [*]:

  front-power {
      retain-state-shutdown;
      default-state = "keep";
      gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
  };

[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h |  1 +
 hw/misc/pca9552.c         | 29 +++++++++++++++++++++++++++++
 hw/misc/trace-events      |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index c5be7f1c5e..755be2e8e5 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -26,6 +26,7 @@ typedef struct PCA9552State {
     uint8_t pointer;
 
     uint8_t regs[PCA9552_NR_REGS];
+    uint16_t pins_status; /* Holds latest INPUT0 & INPUT1 status */
     uint8_t max_reg;
     char *description; /* For debugging purpose only */
     uint8_t nr_leds;
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 54ccdcf6d4..f0d435e310 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -12,12 +12,14 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "trace.h"
 
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
@@ -34,6 +36,32 @@ static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
     return extract32(s->regs[reg], shift, 2);
 }
 
+static void pca9552_display_pins_status(PCA9552State *s)
+{
+    uint16_t pins_status, pins_changed;
+    int i;
+
+    pins_status = (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
+    pins_changed = s->pins_status ^ pins_status;
+    if (!pins_changed) {
+        return;
+    }
+    s->pins_status = pins_status;
+    if (trace_event_get_state_backends(TRACE_PCA9552_GPIO_STATUS)) {
+        char buf[PCA9552_PIN_COUNT + 1];
+
+        for (i = 0; i < s->nr_leds; i++) {
+            if (extract32(pins_status, i, 1)) {
+                buf[i] = '*';
+            } else {
+                buf[i] = '.';
+            }
+        }
+        buf[i] = '\0';
+        trace_pca9552_gpio_status(s->description, buf);
+    }
+}
+
 static void pca9552_update_pin_input(PCA9552State *s)
 {
     int i;
@@ -57,6 +85,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
             break;
         }
     }
+    pca9552_display_pins_status(s);
 }
 
 static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..7630e59652 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
 # grlib_ahb_apb_pnp.c
 grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
 grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
+
+# pca9552.c
+pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
-- 
2.21.3

[PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device

[Philippe Mathieu-Daudé]

We have 2 distinct PCA9552 devices. Set their description
to distinguish them when looking at the trace events.

Description name taken from:
https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6b7533aeee..3d5dec4692 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
+    DeviceState *dev;
 
     /* Bus 3: TODO bmp280@77 */
     /* Bus 3: TODO max31785@52 */
     /* Bus 3: TODO dps310@76 */
-    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
-                     0x60);
+    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+    qdev_prop_set_string(dev, "description", "pca1");
+    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
+                          &error_fatal);
 
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
@@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 
     smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
                           eeprom_buf);
-    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
-                     0x60);
+    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
+    qdev_prop_set_string(dev, "description", "pca0");
+    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
+                          &error_fatal);
     /* Bus 11: TODO ucd90160@64 */
 }
 
-- 
2.21.3

[PATCH v4 7/8] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

Emit a trace event when a GPIO change its state.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_change
  1592690552.687372:pca9552_gpio_change pca1 GPIO id:0 status: 0 -> 1
  1592690552.690169:pca9552_gpio_change pca1 GPIO id:1 status: 0 -> 1
  1592690552.691673:pca9552_gpio_change pca1 GPIO id:2 status: 0 -> 1
  1592690552.696886:pca9552_gpio_change pca1 GPIO id:3 status: 0 -> 1
  1592690552.698614:pca9552_gpio_change pca1 GPIO id:13 status: 0 -> 1
  1592690552.699833:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690552.700842:pca9552_gpio_change pca1 GPIO id:15 status: 0 -> 1
  1592690683.841921:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690683.861660:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690684.371460:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690684.882115:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690685.391411:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690685.901391:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
  1592690686.411678:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
  1592690686.921279:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1

We notice the GPIO #14 (front-power LED) starts to blink.

This LED is described in the witherspoon device-tree [*]:

  front-power {
      retain-state-shutdown;
      default-state = "keep";
      gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
  };

[*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/pca9552.c    | 15 +++++++++++++++
 hw/misc/trace-events |  1 +
 2 files changed, 16 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index f0d435e310..85557b78d9 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -60,6 +60,21 @@ static void pca9552_display_pins_status(PCA9552State *s)
         buf[i] = '\0';
         trace_pca9552_gpio_status(s->description, buf);
     }
+    if (trace_event_get_state_backends(TRACE_PCA9552_GPIO_CHANGE)) {
+        for (i = 0; i < s->nr_leds; i++) {
+            if (extract32(pins_changed, i, 1)) {
+                unsigned new_state = extract32(pins_status, i, 1);
+
+                /*
+                 * We display the state using the PCA logic ("active-high").
+                 * This is not the state of the LED, which signal might be
+                 * wired "active-low" on the board.
+                 */
+                trace_pca9552_gpio_change(s->description, i,
+                                          !new_state, new_state);
+            }
+        }
+    }
 }
 
 static void pca9552_update_pin_input(PCA9552State *s)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 7630e59652..805d2110e0 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
 
 # pca9552.c
 pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
+pca9552_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
-- 
2.21.3

[PATCH v4 8/8] hw/misc/pca9552: Model qdev output GPIOs

[Philippe Mathieu-Daudé]

The PCA9552 has 16 GPIOs which can be used as input,
output or PWM mode. QEMU models the output GPIO with
the qemu_irq type. Let the device expose the 16 GPIOs
to allow us to later connect LEDs to these outputs.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/pca9552.h | 1 +
 hw/misc/pca9552.c         | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index 755be2e8e5..2545087da3 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -30,6 +30,7 @@ typedef struct PCA9552State {
     uint8_t max_reg;
     char *description; /* For debugging purpose only */
     uint8_t nr_leds;
+    qemu_irq gpio[PCA9552_PIN_COUNT];
 } PCA9552State;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 85557b78d9..8f0ce19b74 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -16,6 +16,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
+#include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
@@ -88,9 +89,11 @@ static void pca9552_update_pin_input(PCA9552State *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:
+            qemu_set_irq(s->gpio[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
         case PCA9552_LED_PWM0:
@@ -360,6 +363,7 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
     if (!s->description) {
         s->description = g_strdup("pca-unspecified");
     }
+    qdev_init_gpio_out(dev, s->gpio, s->nr_leds);
 }
 
 static Property pca9552_properties[] = {
-- 
2.21.3

Re: [PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> The current code models the PCA9552, but there are comments
> saying the code could be easily adapted for the rest of the
> PCA955x family.
> Since we assume we have at most 16 pins (for the PCA9552),
> add a definition and check the instance doesn't use more than
> this number. This makes the code a bit safer in case other
> PCA955x devices are added.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I would introduce a PCA9552Class and move nr_leds under the class. 

C. 


> ---
>  hw/misc/pca9552.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index cfefb8fce8..b97fc2893c 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -303,6 +303,17 @@ static void pca9552_initfn(Object *obj)
>      }
>  }
>  
> +static void pca9552_realize(DeviceState *dev, Error **errp)
> +{
> +    PCA9552State *s = PCA9552(dev);
> +
> +    if (s->nr_leds > PCA9552_PIN_COUNT) {
> +        error_setg(errp, "%s invalid led count %u (max: %u)",
> +                   __func__, s->nr_leds, PCA9552_PIN_COUNT);
> +        return;
> +    }
> +}
> +
>  static void pca9552_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -311,6 +322,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>      k->event = pca9552_event;
>      k->recv = pca9552_recv;
>      k->send = pca9552_send;
> +    dc->realize = pca9552_realize;
>      dc->reset = pca9552_reset;
>      dc->vmsd = &pca9552_vmstate;
>  }
> 

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> Add a description field to distinguish between multiple devices.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Could it be a QOM attribute ? 

C.

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/pca9552.h |  1 +
>  hw/misc/pca9552.c         | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index ef6da4988f..c5be7f1c5e 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -27,6 +27,7 @@ typedef struct PCA9552State {
>  
>      uint8_t regs[PCA9552_NR_REGS];
>      uint8_t max_reg;
> +    char *description; /* For debugging purpose only */
>      uint8_t nr_leds;
>  } PCA9552State;
>  
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index b97fc2893c..54ccdcf6d4 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -12,6 +12,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/misc/pca9552.h"
>  #include "hw/misc/pca9552_regs.h"
>  #include "migration/vmstate.h"
> @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
>                     __func__, s->nr_leds, PCA9552_PIN_COUNT);
>          return;
>      }
> +    if (!s->description) {
> +        s->description = g_strdup("pca-unspecified");
> +    }
>  }
>  
> +static Property pca9552_properties[] = {
> +    DEFINE_PROP_STRING("description", PCA9552State, description),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void pca9552_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>      dc->realize = pca9552_realize;
>      dc->reset = pca9552_reset;
>      dc->vmsd = &pca9552_vmstate;
> +    device_class_set_props(dc, pca9552_properties);
>  }
>  
>  static const TypeInfo pca9552_info = {
> 

Re: [PATCH v4 2/8] hw/misc/pca9552: Replace magic value by PCA9552_PIN_COUNT definition

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> Replace the '16' magic value by the PCA9552_PIN_COUNT definition.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/pca9552.h | 1 +
>  hw/misc/pca9552.c         | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index ebb43c63fe..ef6da4988f 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -15,6 +15,7 @@
>  #define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
>  
>  #define PCA9552_NR_REGS 10
> +#define PCA9552_PIN_COUNT 16
>  
>  typedef struct PCA9552State {
>      /*< private >*/
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index cac729e35a..cfefb8fce8 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -291,7 +291,7 @@ static void pca9552_initfn(Object *obj)
>       * PCA955X device
>       */
>      s->max_reg = PCA9552_LS3;
> -    s->nr_leds = 16;
> +    s->nr_leds = PCA9552_PIN_COUNT;
>  
>      for (led = 0; led < s->nr_leds; led++) {
>          char *name;
> 

Re: [PATCH v4 5/8] hw/misc/pca9552: Trace GPIO High/Low events

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> Add a trivial representation of the PCA9552 GPIOs.
> 
> Example booting obmc-phosphor-image:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_status
>   1592689902.327837:pca9552_gpio_status pca-unspecified GPIOs 0-15 [*...............]
>   1592689902.329934:pca9552_gpio_status pca-unspecified GPIOs 0-15 [**..............]
>   1592689902.330717:pca9552_gpio_status pca-unspecified GPIOs 0-15 [***.............]
>   1592689902.331431:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****............]
>   1592689902.332163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*..]
>   1592689902.332888:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........**.]
>   1592689902.333629:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690032.793289:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
>   1592690033.303163:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690033.812962:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
>   1592690034.323234:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........***]
>   1592690034.832922:pca9552_gpio_status pca-unspecified GPIOs 0-15 [****.........*.*]
> 
> We notice the GPIO #14 (front-power LED) starts to blink.
> 
> This LED is described in the witherspoon device-tree [*]:
> 
>   front-power {
>       retain-state-shutdown;
>       default-state = "keep";
>       gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
>   };
> 
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/pca9552.h |  1 +
>  hw/misc/pca9552.c         | 29 +++++++++++++++++++++++++++++
>  hw/misc/trace-events      |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index c5be7f1c5e..755be2e8e5 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -26,6 +26,7 @@ typedef struct PCA9552State {
>      uint8_t pointer;
>  
>      uint8_t regs[PCA9552_NR_REGS];
> +    uint16_t pins_status; /* Holds latest INPUT0 & INPUT1 status */

Hmm, that is an extra state redundant with regs.

>      uint8_t max_reg;
>      char *description; /* For debugging purpose only */
>      uint8_t nr_leds;
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 54ccdcf6d4..f0d435e310 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -12,12 +12,14 @@
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "qemu/bitops.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/misc/pca9552.h"
>  #include "hw/misc/pca9552_regs.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> +#include "trace.h"
>  
>  #define PCA9552_LED_ON   0x0
>  #define PCA9552_LED_OFF  0x1
> @@ -34,6 +36,32 @@ static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
>      return extract32(s->regs[reg], shift, 2);
>  }
>  
> +static void pca9552_display_pins_status(PCA9552State *s)
> +{
> +    uint16_t pins_status, pins_changed;
> +    int i;
> +
> +    pins_status = (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
> +    pins_changed = s->pins_status ^ pins_status;
> +    if (!pins_changed) {
> +        return;
> +    }
> +    s->pins_status = pins_status;

It's nice to have but it won't kill puppies if we recompute the 
state each time when traces are on. 

> +    if (trace_event_get_state_backends(TRACE_PCA9552_GPIO_STATUS)) {
> +        char buf[PCA9552_PIN_COUNT + 1];
> +
> +        for (i = 0; i < s->nr_leds; i++) {
> +            if (extract32(pins_status, i, 1)) {
> +                buf[i] = '*';
> +            } else {
> +                buf[i] = '.';
> +            }
> +        }
> +        buf[i] = '\0';
> +        trace_pca9552_gpio_status(s->description, buf);
> +    }
> +}
> +
>  static void pca9552_update_pin_input(PCA9552State *s)
>  {
>      int i;
> @@ -57,6 +85,7 @@ static void pca9552_update_pin_input(PCA9552State *s)
>              break;
>          }

and we could test TRACE_PCA9552_GPIO_STATUS here : 
>      }
> +    pca9552_display_pins_status(s);
>  }
>  
>  static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 5561746866..7630e59652 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>  # grlib_ahb_apb_pnp.c
>  grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>  grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
> +
> +# pca9552.c
> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
> 

Re: [PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> We have 2 distinct PCA9552 devices. Set their description
> to distinguish them when looking at the trace events.

It's nice and usefull but couldn't we do the same with a QOM object name ? 

C. 

> 
> Description name taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/aspeed.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 6b7533aeee..3d5dec4692 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>  {
>      AspeedSoCState *soc = &bmc->soc;
>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> +    DeviceState *dev;
>  
>      /* Bus 3: TODO bmp280@77 */
>      /* Bus 3: TODO max31785@52 */
>      /* Bus 3: TODO dps310@76 */
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca1");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
> +                          &error_fatal);
>  
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
> @@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>  
>      smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
>                            eeprom_buf);
> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
> -                     0x60);
> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
> +    qdev_prop_set_string(dev, "description", "pca0");
> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
> +                          &error_fatal);
>      /* Bus 11: TODO ucd90160@64 */
>  }
>  
> 

Re: [PATCH v4 7/8] hw/misc/pca9552: Trace GPIO change events

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> Emit a trace event when a GPIO change its state.

I understand now why you need 'pin_status'. 

We could compute 'pin_status' and 'pin_changed' in routine 
pca9552_update_pin_input() when updating the PCA9552_INPUT0/1 
register values and pass them to pca9552_display_pins_status()
or something like that.

Maybe add two different display routines also. 
 
> 
> Example booting obmc-phosphor-image:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_change
>   1592690552.687372:pca9552_gpio_change pca1 GPIO id:0 status: 0 -> 1
>   1592690552.690169:pca9552_gpio_change pca1 GPIO id:1 status: 0 -> 1
>   1592690552.691673:pca9552_gpio_change pca1 GPIO id:2 status: 0 -> 1
>   1592690552.696886:pca9552_gpio_change pca1 GPIO id:3 status: 0 -> 1
>   1592690552.698614:pca9552_gpio_change pca1 GPIO id:13 status: 0 -> 1
>   1592690552.699833:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690552.700842:pca9552_gpio_change pca1 GPIO id:15 status: 0 -> 1
>   1592690683.841921:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690683.861660:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690684.371460:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690684.882115:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690685.391411:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690685.901391:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>   1592690686.411678:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>   1592690686.921279:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
> 
> We notice the GPIO #14 (front-power LED) starts to blink.
> 
> This LED is described in the witherspoon device-tree [*]:
> 
>   front-power {
>       retain-state-shutdown;
>       default-state = "keep";
>       gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
>   };
> 
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/misc/pca9552.c    | 15 +++++++++++++++
>  hw/misc/trace-events |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index f0d435e310..85557b78d9 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -60,6 +60,21 @@ static void pca9552_display_pins_status(PCA9552State *s)
>          buf[i] = '\0';
>          trace_pca9552_gpio_status(s->description, buf);
>      }
> +    if (trace_event_get_state_backends(TRACE_PCA9552_GPIO_CHANGE)) {
> +        for (i = 0; i < s->nr_leds; i++) {
> +            if (extract32(pins_changed, i, 1)) {
> +                unsigned new_state = extract32(pins_status, i, 1);
> +
> +                /*
> +                 * We display the state using the PCA logic ("active-high").
> +                 * This is not the state of the LED, which signal might be
> +                 * wired "active-low" on the board.
> +                 */
> +                trace_pca9552_gpio_change(s->description, i,
> +                                          !new_state, new_state);
> +            }
> +        }
> +    }
>  }
>  
>  static void pca9552_update_pin_input(PCA9552State *s)
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 7630e59652..805d2110e0 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
>  
>  # pca9552.c
>  pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
> +pca9552_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
> 

Re: [PATCH v4 8/8] hw/misc/pca9552: Model qdev output GPIOs

[Cédric Le Goater]

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> The PCA9552 has 16 GPIOs which can be used as input,
> output or PWM mode. QEMU models the output GPIO with
> the qemu_irq type. Let the device expose the 16 GPIOs
> to allow us to later connect LEDs to these outputs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  include/hw/misc/pca9552.h | 1 +
>  hw/misc/pca9552.c         | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index 755be2e8e5..2545087da3 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -30,6 +30,7 @@ typedef struct PCA9552State {
>      uint8_t max_reg;
>      char *description; /* For debugging purpose only */
>      uint8_t nr_leds;
> +    qemu_irq gpio[PCA9552_PIN_COUNT];
>  } PCA9552State;
>  
>  #endif
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 85557b78d9..8f0ce19b74 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -16,6 +16,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/misc/pca9552.h"
>  #include "hw/misc/pca9552_regs.h"
> +#include "hw/irq.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> @@ -88,9 +89,11 @@ static void pca9552_update_pin_input(PCA9552State *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:
> +            qemu_set_irq(s->gpio[i], 0);
>              s->regs[input_reg] &= ~(1 << input_shift);
>              break;
>          case PCA9552_LED_PWM0:
> @@ -360,6 +363,7 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
>      if (!s->description) {
>          s->description = g_strdup("pca-unspecified");
>      }
> +    qdev_init_gpio_out(dev, s->gpio, s->nr_leds);
>  }
>  
>  static Property pca9552_properties[] = {
> 

Re: [PATCH v4 1/8] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Philippe Mathieu-Daudé]

+Markus

On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> Extract i2c_try_create_slave() and i2c_realize_and_unref()
> from i2c_create_slave().
> We can now set properties on a I2CSlave before it is realized.
> 
> This is in line with the recent qdev/QOM changes merged
> in commit 6675a653d2e.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/i2c/i2c.h |  2 ++
>  hw/i2c/core.c        | 18 ++++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 4117211565..d6e3d85faf 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data);
>  uint8_t i2c_recv(I2CBus *bus);
>  
>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr);
> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
>  
>  /* lm832x.c */
>  void lm832x_key_event(DeviceState *dev, int key, int state);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 1aac457a2a..acf34a12d6 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = {
>      }
>  };
>  
> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_new(name);
>      qdev_prop_set_uint8(dev, "address", addr);
> -    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> +    return dev;
> +}
> +
> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
> +{
> +    return qdev_realize_and_unref(dev, &bus->qbus, errp);
> +}
> +
> +DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> +{
> +    DeviceState *dev;
> +
> +    dev = i2c_try_create_slave(name, addr);
> +    i2c_realize_and_unref(dev, bus, &error_fatal);
> +
>      return dev;
>  }
>  
> 

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

On 6/22/20 8:27 AM, Cédric Le Goater wrote:
> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>> Add a description field to distinguish between multiple devices.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Could it be a QOM attribute ? 

What do you call a 'QOM attribute'?
Is it what qdev properties implement?
(in this case via DEFINE_PROP_STRING).

> 
> C.
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/pca9552.h |  1 +
>>  hw/misc/pca9552.c         | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
>> index ef6da4988f..c5be7f1c5e 100644
>> --- a/include/hw/misc/pca9552.h
>> +++ b/include/hw/misc/pca9552.h
>> @@ -27,6 +27,7 @@ typedef struct PCA9552State {
>>  
>>      uint8_t regs[PCA9552_NR_REGS];
>>      uint8_t max_reg;
>> +    char *description; /* For debugging purpose only */
>>      uint8_t nr_leds;
>>  } PCA9552State;
>>  
>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>> index b97fc2893c..54ccdcf6d4 100644
>> --- a/hw/misc/pca9552.c
>> +++ b/hw/misc/pca9552.c
>> @@ -12,6 +12,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu/log.h"
>>  #include "qemu/module.h"
>> +#include "hw/qdev-properties.h"
>>  #include "hw/misc/pca9552.h"
>>  #include "hw/misc/pca9552_regs.h"
>>  #include "migration/vmstate.h"
>> @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
>>                     __func__, s->nr_leds, PCA9552_PIN_COUNT);
>>          return;
>>      }
>> +    if (!s->description) {
>> +        s->description = g_strdup("pca-unspecified");
>> +    }
>>  }
>>  
>> +static Property pca9552_properties[] = {
>> +    DEFINE_PROP_STRING("description", PCA9552State, description),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void pca9552_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>>      dc->realize = pca9552_realize;
>>      dc->reset = pca9552_reset;
>>      dc->vmsd = &pca9552_vmstate;
>> +    device_class_set_props(dc, pca9552_properties);
>>  }
>>  
>>  static const TypeInfo pca9552_info = {
>>
> 
> 

Re: [PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device

[Philippe Mathieu-Daudé]

+Markus

On 6/22/20 8:49 AM, Cédric Le Goater wrote:
> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>> We have 2 distinct PCA9552 devices. Set their description
>> to distinguish them when looking at the trace events.
> 
> It's nice and usefull but couldn't we do the same with a QOM object name ?

qdev inherits QOM and overloads it with the qdev_ API.
Since we have a qdev object, isn't it better to use the qdev_ API?

I'd keep the QOM API for bare QOM objects.(I find confusing to use
different APIs).

> 
> C. 
> 
>>
>> Description name taken from:
>> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/aspeed.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 6b7533aeee..3d5dec4692 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -508,12 +508,15 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>>  {
>>      AspeedSoCState *soc = &bmc->soc;
>>      uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>> +    DeviceState *dev;
>>  
>>      /* Bus 3: TODO bmp280@77 */
>>      /* Bus 3: TODO max31785@52 */
>>      /* Bus 3: TODO dps310@76 */
>> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), TYPE_PCA9552,
>> -                     0x60);
>> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> +    qdev_prop_set_string(dev, "description", "pca1");
>> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
>> +                          &error_fatal);
>>  
>>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
>>      i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
>> @@ -528,8 +531,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>>  
>>      smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
>>                            eeprom_buf);
>> -    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), TYPE_PCA9552,
>> -                     0x60);
>> +    dev = i2c_try_create_slave(TYPE_PCA9552, 0x60);
>> +    qdev_prop_set_string(dev, "description", "pca0");
>> +    i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11),
>> +                          &error_fatal);
>>      /* Bus 11: TODO ucd90160@64 */
>>  }
>>  
>>
> 
> 

Re: [PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition

[Philippe Mathieu-Daudé]

On 6/22/20 8:25 AM, Cédric Le Goater wrote:
> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>> The current code models the PCA9552, but there are comments
>> saying the code could be easily adapted for the rest of the
>> PCA955x family.
>> Since we assume we have at most 16 pins (for the PCA9552),
>> add a definition and check the instance doesn't use more than
>> this number. This makes the code a bit safer in case other
>> PCA955x devices are added.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I would introduce a PCA9552Class and move nr_leds under the class.

A bit excessive (for now) for the hobbyist time I can dedicate
to this, but I'll try (also renaming nr_leds -> nr_pins).

> 
> C. 
> 
> 
>> ---
>>  hw/misc/pca9552.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>> index cfefb8fce8..b97fc2893c 100644
>> --- a/hw/misc/pca9552.c
>> +++ b/hw/misc/pca9552.c
>> @@ -303,6 +303,17 @@ static void pca9552_initfn(Object *obj)
>>      }
>>  }
>>  
>> +static void pca9552_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PCA9552State *s = PCA9552(dev);
>> +
>> +    if (s->nr_leds > PCA9552_PIN_COUNT) {
>> +        error_setg(errp, "%s invalid led count %u (max: %u)",
>> +                   __func__, s->nr_leds, PCA9552_PIN_COUNT);
>> +        return;
>> +    }
>> +}
>> +
>>  static void pca9552_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -311,6 +322,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>>      k->event = pca9552_event;
>>      k->recv = pca9552_recv;
>>      k->send = pca9552_send;
>> +    dc->realize = pca9552_realize;
>>      dc->reset = pca9552_reset;
>>      dc->vmsd = &pca9552_vmstate;
>>  }
>>
> 
> 

Re: [PATCH v4 7/8] hw/misc/pca9552: Trace GPIO change events

[Philippe Mathieu-Daudé]

On 6/22/20 9:01 AM, Cédric Le Goater wrote:
> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>> Emit a trace event when a GPIO change its state.
> 
> I understand now why you need 'pin_status'. 
> 
> We could compute 'pin_status' and 'pin_changed' in routine 
> pca9552_update_pin_input() when updating the PCA9552_INPUT0/1 
> register values and pass them to pca9552_display_pins_status()
> or something like that.

Good idea, thanks!

> 
> Maybe add two different display routines also. 
>  
>>
>> Example booting obmc-phosphor-image:
>>
>>   $ qemu-system-arm -M witherspoon-bmc -trace pca9552_gpio_change
>>   1592690552.687372:pca9552_gpio_change pca1 GPIO id:0 status: 0 -> 1
>>   1592690552.690169:pca9552_gpio_change pca1 GPIO id:1 status: 0 -> 1
>>   1592690552.691673:pca9552_gpio_change pca1 GPIO id:2 status: 0 -> 1
>>   1592690552.696886:pca9552_gpio_change pca1 GPIO id:3 status: 0 -> 1
>>   1592690552.698614:pca9552_gpio_change pca1 GPIO id:13 status: 0 -> 1
>>   1592690552.699833:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>>   1592690552.700842:pca9552_gpio_change pca1 GPIO id:15 status: 0 -> 1
>>   1592690683.841921:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>>   1592690683.861660:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>>   1592690684.371460:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>>   1592690684.882115:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>>   1592690685.391411:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>>   1592690685.901391:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>>   1592690686.411678:pca9552_gpio_change pca1 GPIO id:14 status: 1 -> 0
>>   1592690686.921279:pca9552_gpio_change pca1 GPIO id:14 status: 0 -> 1
>>
>> We notice the GPIO #14 (front-power LED) starts to blink.
>>
>> This LED is described in the witherspoon device-tree [*]:
>>
>>   front-power {
>>       retain-state-shutdown;
>>       default-state = "keep";
>>       gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
>>   };
>>
>> [*] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts?id=b1f9be9392f0#n140
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/misc/pca9552.c    | 15 +++++++++++++++
>>  hw/misc/trace-events |  1 +
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>> index f0d435e310..85557b78d9 100644
>> --- a/hw/misc/pca9552.c
>> +++ b/hw/misc/pca9552.c
>> @@ -60,6 +60,21 @@ static void pca9552_display_pins_status(PCA9552State *s)
>>          buf[i] = '\0';
>>          trace_pca9552_gpio_status(s->description, buf);
>>      }
>> +    if (trace_event_get_state_backends(TRACE_PCA9552_GPIO_CHANGE)) {
>> +        for (i = 0; i < s->nr_leds; i++) {
>> +            if (extract32(pins_changed, i, 1)) {
>> +                unsigned new_state = extract32(pins_status, i, 1);
>> +
>> +                /*
>> +                 * We display the state using the PCA logic ("active-high").
>> +                 * This is not the state of the LED, which signal might be
>> +                 * wired "active-low" on the board.
>> +                 */
>> +                trace_pca9552_gpio_change(s->description, i,
>> +                                          !new_state, new_state);
>> +            }
>> +        }
>> +    }
>>  }
>>  
>>  static void pca9552_update_pin_input(PCA9552State *s)
>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>> index 7630e59652..805d2110e0 100644
>> --- a/hw/misc/trace-events
>> +++ b/hw/misc/trace-events
>> @@ -209,3 +209,4 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
>>  
>>  # pca9552.c
>>  pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
>> +pca9552_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
>>
> 
> 

Re: [PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition

[Cédric Le Goater]

On 6/22/20 10:37 AM, Philippe Mathieu-Daudé wrote:
> On 6/22/20 8:25 AM, Cédric Le Goater wrote:
>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>> The current code models the PCA9552, but there are comments
>>> saying the code could be easily adapted for the rest of the
>>> PCA955x family.
>>> Since we assume we have at most 16 pins (for the PCA9552),
>>> add a definition and check the instance doesn't use more than
>>> this number. This makes the code a bit safer in case other
>>> PCA955x devices are added.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I would introduce a PCA9552Class and move nr_leds under the class.
> 
> A bit excessive (for now) for the hobbyist time I can dedicate
> to this, but I'll try (also renaming nr_leds -> nr_pins).


It's fine with me if you don't have time.  

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> 
>>
>> C. 
>>
>>
>>> ---
>>>  hw/misc/pca9552.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>>> index cfefb8fce8..b97fc2893c 100644
>>> --- a/hw/misc/pca9552.c
>>> +++ b/hw/misc/pca9552.c
>>> @@ -303,6 +303,17 @@ static void pca9552_initfn(Object *obj)
>>>      }
>>>  }
>>>  
>>> +static void pca9552_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    PCA9552State *s = PCA9552(dev);
>>> +
>>> +    if (s->nr_leds > PCA9552_PIN_COUNT) {
>>> +        error_setg(errp, "%s invalid led count %u (max: %u)",
>>> +                   __func__, s->nr_leds, PCA9552_PIN_COUNT);
>>> +        return;
>>> +    }
>>> +}
>>> +
>>>  static void pca9552_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -311,6 +322,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>>>      k->event = pca9552_event;
>>>      k->recv = pca9552_recv;
>>>      k->send = pca9552_send;
>>> +    dc->realize = pca9552_realize;
>>>      dc->reset = pca9552_reset;
>>>      dc->vmsd = &pca9552_vmstate;
>>>  }
>>>
>>
>>

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Cédric Le Goater]

On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>> Add a description field to distinguish between multiple devices.
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Could it be a QOM attribute ? 
> 
> What do you call a 'QOM attribute'?
> Is it what qdev properties implement?
> (in this case via DEFINE_PROP_STRING).

I meant a default Object property, which would apply to all Objects. 

What you did is fine, so :

Reviewed-by: Cédric Le Goater <clg@kaod.org>

but, may be, a well defined child name is enough for the purpose.

C.


> 
>>
>> C.
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  include/hw/misc/pca9552.h |  1 +
>>>  hw/misc/pca9552.c         | 10 ++++++++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
>>> index ef6da4988f..c5be7f1c5e 100644
>>> --- a/include/hw/misc/pca9552.h
>>> +++ b/include/hw/misc/pca9552.h
>>> @@ -27,6 +27,7 @@ typedef struct PCA9552State {
>>>  
>>>      uint8_t regs[PCA9552_NR_REGS];
>>>      uint8_t max_reg;
>>> +    char *description; /* For debugging purpose only */
>>>      uint8_t nr_leds;
>>>  } PCA9552State;
>>>  
>>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>>> index b97fc2893c..54ccdcf6d4 100644
>>> --- a/hw/misc/pca9552.c
>>> +++ b/hw/misc/pca9552.c
>>> @@ -12,6 +12,7 @@
>>>  #include "qemu/osdep.h"
>>>  #include "qemu/log.h"
>>>  #include "qemu/module.h"
>>> +#include "hw/qdev-properties.h"
>>>  #include "hw/misc/pca9552.h"
>>>  #include "hw/misc/pca9552_regs.h"
>>>  #include "migration/vmstate.h"
>>> @@ -312,8 +313,16 @@ static void pca9552_realize(DeviceState *dev, Error **errp)
>>>                     __func__, s->nr_leds, PCA9552_PIN_COUNT);
>>>          return;
>>>      }
>>> +    if (!s->description) {
>>> +        s->description = g_strdup("pca-unspecified");
>>> +    }
>>>  }
>>>  
>>> +static Property pca9552_properties[] = {
>>> +    DEFINE_PROP_STRING("description", PCA9552State, description),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>>  static void pca9552_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -325,6 +334,7 @@ static void pca9552_class_init(ObjectClass *klass, void *data)
>>>      dc->realize = pca9552_realize;
>>>      dc->reset = pca9552_reset;
>>>      dc->vmsd = &pca9552_vmstate;
>>> +    device_class_set_props(dc, pca9552_properties);
>>>  }
>>>  
>>>  static const TypeInfo pca9552_info = {
>>>
>>
>>

Re: [PATCH v4 1/8] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Extract i2c_try_create_slave() and i2c_realize_and_unref()
> from i2c_create_slave().
> We can now set properties on a I2CSlave before it is realized.
>
> This is in line with the recent qdev/QOM changes merged
> in commit 6675a653d2e.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/i2c/i2c.h |  2 ++
>  hw/i2c/core.c        | 18 ++++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 4117211565..d6e3d85faf 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data);
>  uint8_t i2c_recv(I2CBus *bus);
>  
>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr);
> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
>  
>  /* lm832x.c */
>  void lm832x_key_event(DeviceState *dev, int key, int state);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 1aac457a2a..acf34a12d6 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = {
>      }
>  };
>  
> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_new(name);
>      qdev_prop_set_uint8(dev, "address", addr);
> -    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> +    return dev;
> +}
> +
> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
> +{
> +    return qdev_realize_and_unref(dev, &bus->qbus, errp);
> +}
> +
> +DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> +{
> +    DeviceState *dev;
> +
> +    dev = i2c_try_create_slave(name, addr);
> +    i2c_realize_and_unref(dev, bus, &error_fatal);
> +
>      return dev;
>  }

No objections, except I want to see actual users.

Re: [PATCH v4 1/8] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Philippe Mathieu-Daudé]

On 6/22/20 5:17 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Extract i2c_try_create_slave() and i2c_realize_and_unref()
>> from i2c_create_slave().
>> We can now set properties on a I2CSlave before it is realized.
>>
>> This is in line with the recent qdev/QOM changes merged
>> in commit 6675a653d2e.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/i2c/i2c.h |  2 ++
>>  hw/i2c/core.c        | 18 ++++++++++++++++--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index 4117211565..d6e3d85faf 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -80,6 +80,8 @@ int i2c_send(I2CBus *bus, uint8_t data);
>>  uint8_t i2c_recv(I2CBus *bus);
>>  
>>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
>> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr);
>> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp);
>>  
>>  /* lm832x.c */
>>  void lm832x_key_event(DeviceState *dev, int key, int state);
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 1aac457a2a..acf34a12d6 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -267,13 +267,27 @@ const VMStateDescription vmstate_i2c_slave = {
>>      }
>>  };
>>  
>> -DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> +DeviceState *i2c_try_create_slave(const char *name, uint8_t addr)
>>  {
>>      DeviceState *dev;
>>  
>>      dev = qdev_new(name);
>>      qdev_prop_set_uint8(dev, "address", addr);
>> -    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
>> +    return dev;
>> +}
>> +
>> +bool i2c_realize_and_unref(DeviceState *dev, I2CBus *bus, Error **errp)
>> +{
>> +    return qdev_realize_and_unref(dev, &bus->qbus, errp);
>> +}
>> +
>> +DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = i2c_try_create_slave(name, addr);
>> +    i2c_realize_and_unref(dev, bus, &error_fatal);
>> +
>>      return dev;
>>  }
> 
> No objections, except I want to see actual users.

You weren't Cc'ed on the whole series.

User is patch #6/8 "hw/arm/aspeed: Describe each PCA9552 device":
https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html

Re: [PATCH v4 1/8] hw/i2c/core: Add i2c_try_create_slave() and i2c_realize_and_unref()

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 6/22/20 5:17 PM, Markus Armbruster wrote:
>> 
>> No objections, except I want to see actual users.
>
> You weren't Cc'ed on the whole series.
>
> User is patch #6/8 "hw/arm/aspeed: Describe each PCA9552 device":
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html

Fat-fingered my grep, sorry for the noise.

Re: [PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device

[Philippe Mathieu-Daudé]

Hi Cédric,

On Mon, Jun 22, 2020 at 10:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 6/22/20 8:49 AM, Cédric Le Goater wrote:
> > On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
> >> We have 2 distinct PCA9552 devices. Set their description
> >> to distinguish them when looking at the trace events.
> >
> > It's nice and usefull but couldn't we do the same with a QOM object name ?
>
> qdev inherits QOM and overloads it with the qdev_ API.
> Since we have a qdev object, isn't it better to use the qdev_ API?
>
> I'd keep the QOM API for bare QOM objects.(I find confusing to use
> different APIs).

FYI you can get an idea of the QOM -> qdev -> sysbus -> ... tree here:
https://observablehq.com/@philmd/qemu-aarch64-softmmu-qom-tree

Re: [PATCH v4 6/8] hw/arm/aspeed: Describe each PCA9552 device

[Cédric Le Goater]

On 6/24/20 6:54 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On Mon, Jun 22, 2020 at 10:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 6/22/20 8:49 AM, Cédric Le Goater wrote:
>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>> We have 2 distinct PCA9552 devices. Set their description
>>>> to distinguish them when looking at the trace events.
>>>
>>> It's nice and usefull but couldn't we do the same with a QOM object name ?
>>
>> qdev inherits QOM and overloads it with the qdev_ API.
>> Since we have a qdev object, isn't it better to use the qdev_ API?
>>
>> I'd keep the QOM API for bare QOM objects.(I find confusing to use
>> different APIs).
> 
> FYI you can get an idea of the QOM -> qdev -> sysbus -> ... tree here:
> https://observablehq.com/@philmd/qemu-aarch64-softmmu-qom-tree
> 

nice ! 

Thanks,

C.

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>> Add a description field to distinguish between multiple devices.

Pardon my lack of imagination: how does this help you with debugging?

>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Could it be a QOM attribute ? 
>> 
>> What do you call a 'QOM attribute'?
>> Is it what qdev properties implement?
>> (in this case via DEFINE_PROP_STRING).
>
> I meant a default Object property, which would apply to all Objects. 

Good point.  Many devices have multiple component objects of the same
type.

> What you did is fine, so :
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> but, may be, a well defined child name is enough for the purpose.

object_get_canonical_path() returns a distinct path for each (component)
object.  The path components are the child property names.

Properties can have descriptions: object_property_set_description().

Sufficient?

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

On 6/25/20 8:37 AM, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>> Add a description field to distinguish between multiple devices.
> 
> Pardon my lack of imagination: how does this help you with debugging?

Ah, the patch subject is indeed incorrect, this should be:
"... for *tracing* purpose" (I use tracing when debugging).

In the next patch, we use the 'description' property:

+# pca9552.c
+pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
0-15 [%s]"

So when the machine has multiple PCA9552 and guest accesses both,
we can distinct which one is used. For me having "pca1" / "pca0"
is easier to follow than the address of the QOM object.

> 
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> Could it be a QOM attribute ? 
>>>
>>> What do you call a 'QOM attribute'?
>>> Is it what qdev properties implement?
>>> (in this case via DEFINE_PROP_STRING).
>>
>> I meant a default Object property, which would apply to all Objects. 
> 
> Good point.  Many devices have multiple component objects of the same
> type.
> 
>> What you did is fine, so :
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> but, may be, a well defined child name is enough for the purpose.
> 
> object_get_canonical_path() returns a distinct path for each (component)
> object.  The path components are the child property names.
> 
> Properties can have descriptions: object_property_set_description().

TIL object_property_set_description :>

Ah, there is no equivalent object_property_get_description(),
we have to use object_get_canonical_path(). Hmm, not obvious.

> 
> Sufficient?

I don't know... This seems a complex way to do something simple...
This is already a QDEV. Having to use QOM API seems going
backward, since we have the DEFINE_PROP_STRING() macros available
in "hw/qdev-properties.h".

Maybe I'm not seeing the advantages clearly. I'll try later.

Thanks for your review,

Phil.

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote:
> On 6/25/20 8:37 AM, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>>
>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>>> Add a description field to distinguish between multiple devices.
>>
>> Pardon my lack of imagination: how does this help you with debugging?
> 
> Ah, the patch subject is indeed incorrect, this should be:
> "... for *tracing* purpose" (I use tracing when debugging).
> 
> In the next patch, we use the 'description' property:
> 
> +# pca9552.c
> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
> 0-15 [%s]"
> 
> So when the machine has multiple PCA9552 and guest accesses both,
> we can distinct which one is used. For me having "pca1" / "pca0"
> is easier to follow than the address of the QOM object.
> 
>>
>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>
>>>>> Could it be a QOM attribute ? 
>>>>
>>>> What do you call a 'QOM attribute'?
>>>> Is it what qdev properties implement?
>>>> (in this case via DEFINE_PROP_STRING).
>>>
>>> I meant a default Object property, which would apply to all Objects. 
>>
>> Good point.  Many devices have multiple component objects of the same
>> type.
>>
>>> What you did is fine, so :
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> but, may be, a well defined child name is enough for the purpose.
>>
>> object_get_canonical_path() returns a distinct path for each (component)
>> object.  The path components are the child property names.
>>
>> Properties can have descriptions: object_property_set_description().
> 
> TIL object_property_set_description :>
> 
> Ah, there is no equivalent object_property_get_description(),
> we have to use object_get_canonical_path(). Hmm, not obvious.
> 
>>
>> Sufficient?
> 
> I don't know... This seems a complex way to do something simple...
> This is already a QDEV. Having to use QOM API seems going
> backward, since we have the DEFINE_PROP_STRING() macros available
> in "hw/qdev-properties.h".
> 
> Maybe I'm not seeing the advantages clearly. I'll try later.

The canonical path is not very helpful in trace log...

The description I set matches the hardware definitions
and is easier to follow, see patch #6 (where it is set)
where the description comes from:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html

  Description name taken from:
  https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

So in this particular case I don't find the canonical pathname
practical (from an hardware debugging perspective).

> 
> Thanks for your review,
> 
> Phil.
> 

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote:
>> On 6/25/20 8:37 AM, Markus Armbruster wrote:
>>> Cédric Le Goater <clg@kaod.org> writes:
>>>
>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>>>> Add a description field to distinguish between multiple devices.
>>>
>>> Pardon my lack of imagination: how does this help you with debugging?
>> 
>> Ah, the patch subject is indeed incorrect, this should be:
>> "... for *tracing* purpose" (I use tracing when debugging).
>> 
>> In the next patch, we use the 'description' property:
>> 
>> +# pca9552.c
>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
>> 0-15 [%s]"
>> 
>> So when the machine has multiple PCA9552 and guest accesses both,
>> we can distinct which one is used. For me having "pca1" / "pca0"
>> is easier to follow than the address of the QOM object.
>> 
>>>
>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>>
>>>>>> Could it be a QOM attribute ? 
>>>>>
>>>>> What do you call a 'QOM attribute'?
>>>>> Is it what qdev properties implement?
>>>>> (in this case via DEFINE_PROP_STRING).
>>>>
>>>> I meant a default Object property, which would apply to all Objects. 
>>>
>>> Good point.  Many devices have multiple component objects of the same
>>> type.
>>>
>>>> What you did is fine, so :
>>>>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> but, may be, a well defined child name is enough for the purpose.
>>>
>>> object_get_canonical_path() returns a distinct path for each (component)
>>> object.  The path components are the child property names.
>>>
>>> Properties can have descriptions: object_property_set_description().
>> 
>> TIL object_property_set_description :>
>> 
>> Ah, there is no equivalent object_property_get_description(),
>> we have to use object_get_canonical_path(). Hmm, not obvious.
>> 
>>>
>>> Sufficient?
>> 
>> I don't know... This seems a complex way to do something simple...
>> This is already a QDEV. Having to use QOM API seems going
>> backward, since we have the DEFINE_PROP_STRING() macros available
>> in "hw/qdev-properties.h".
>> 
>> Maybe I'm not seeing the advantages clearly. I'll try later.
>
> The canonical path is not very helpful in trace log...

Why?

Okay, I checked the code.  Since the devices in question don't get a
composition tree parent assigned, realize puts them in the
/machine/unattached orphanage.  The canonical path is something like
"/machine/unattached/device[6]", which is less than clear.

The components of the canonical path are the names of the QOM child
properties.  object_get_canonical_path_component() returns the last one,
in this case "device[6]".

If we made the devices QOM children of some other device, we could name
the child properties "pca0" and "pca1".
object_get_canonical_path_component() would then return the strings you
want to see.

We make a device a QOM child of some QOM parent device only if the child
is a component device of the parent (hence the name "composition
tree").

Are these devices integral components of something else, or are they
separate chips?

> The description I set matches the hardware definitions
> and is easier to follow, see patch #6 (where it is set)
> where the description comes from:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html
>
>   Description name taken from:
>   https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>
> So in this particular case I don't find the canonical pathname
> practical (from an hardware debugging perspective).

Personally, I'd be content with i2c bus and address for debugging
purposes.

The i2c buses *are* components: canonical paths look like
"/machine/soc/i2c/aspeed.i2c.3".  The combination of
object_get_canonical_path_component(dev) and
object_property_get_uint(dev, "address", &error_abort) identifies any
i2c device on this machine, not just the two you decorate with a
description string.

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Philippe Mathieu-Daudé]

On 6/26/20 7:49 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote:
>>> On 6/25/20 8:37 AM, Markus Armbruster wrote:
>>>> Cédric Le Goater <clg@kaod.org> writes:
>>>>
>>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>>>>> Add a description field to distinguish between multiple devices.
>>>>
>>>> Pardon my lack of imagination: how does this help you with debugging?
>>>
>>> Ah, the patch subject is indeed incorrect, this should be:
>>> "... for *tracing* purpose" (I use tracing when debugging).
>>>
>>> In the next patch, we use the 'description' property:
>>>
>>> +# pca9552.c
>>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
>>> 0-15 [%s]"
>>>
>>> So when the machine has multiple PCA9552 and guest accesses both,
>>> we can distinct which one is used. For me having "pca1" / "pca0"
>>> is easier to follow than the address of the QOM object.
>>>
>>>>
>>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>>>
>>>>>>> Could it be a QOM attribute ? 
>>>>>>
>>>>>> What do you call a 'QOM attribute'?
>>>>>> Is it what qdev properties implement?
>>>>>> (in this case via DEFINE_PROP_STRING).
>>>>>
>>>>> I meant a default Object property, which would apply to all Objects. 
>>>>
>>>> Good point.  Many devices have multiple component objects of the same
>>>> type.
>>>>
>>>>> What you did is fine, so :
>>>>>
>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>
>>>>> but, may be, a well defined child name is enough for the purpose.
>>>>
>>>> object_get_canonical_path() returns a distinct path for each (component)
>>>> object.  The path components are the child property names.
>>>>
>>>> Properties can have descriptions: object_property_set_description().
>>>
>>> TIL object_property_set_description :>
>>>
>>> Ah, there is no equivalent object_property_get_description(),
>>> we have to use object_get_canonical_path(). Hmm, not obvious.
>>>
>>>>
>>>> Sufficient?
>>>
>>> I don't know... This seems a complex way to do something simple...
>>> This is already a QDEV. Having to use QOM API seems going
>>> backward, since we have the DEFINE_PROP_STRING() macros available
>>> in "hw/qdev-properties.h".
>>>
>>> Maybe I'm not seeing the advantages clearly. I'll try later.
>>
>> The canonical path is not very helpful in trace log...
> 
> Why?
> 
> Okay, I checked the code.  Since the devices in question don't get a
> composition tree parent assigned, realize puts them in the
> /machine/unattached orphanage.  The canonical path is something like
> "/machine/unattached/device[6]", which is less than clear.
> 
> The components of the canonical path are the names of the QOM child
> properties.  object_get_canonical_path_component() returns the last one,
> in this case "device[6]".
> 
> If we made the devices QOM children of some other device, we could name
> the child properties "pca0" and "pca1".
> object_get_canonical_path_component() would then return the strings you
> want to see.
> 
> We make a device a QOM child of some QOM parent device only if the child
> is a component device of the parent (hence the name "composition
> tree").
> 
> Are these devices integral components of something else, or are they
> separate chips?

Separate chips in the machine (actually not even on the machine mother
board where is the CPU, but on a daughter board card).

So in the composition tree I expect to see them as

  /machine/pca0
  /machine/pca1

>> The description I set matches the hardware definitions
>> and is easier to follow, see patch #6 (where it is set)
>> where the description comes from:
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html
>>
>>   Description name taken from:
>>   https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>>
>> So in this particular case I don't find the canonical pathname
>> practical (from an hardware debugging perspective).
> 
> Personally, I'd be content with i2c bus and address for debugging
> purposes.
> 
> The i2c buses *are* components: canonical paths look like
> "/machine/soc/i2c/aspeed.i2c.3".  The combination of
> object_get_canonical_path_component(dev) and
> object_property_get_uint(dev, "address", &error_abort) identifies any
> i2c device on this machine, not just the two you decorate with a
> description string.

The I2C busses is provided by Aspeed peripherals. I counted 19 different
I2C busses on this machine.

"pca0" is connected to i2c bus #11, "pca1" to bus #3.

I still don't think this will be practical, but I'll try your
suggestion.

Regards,

Phil.

Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 6/26/20 7:49 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote:
>>>> On 6/25/20 8:37 AM, Markus Armbruster wrote:
>>>>> Cédric Le Goater <clg@kaod.org> writes:
>>>>>
>>>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>>>>>> Add a description field to distinguish between multiple devices.
>>>>>
>>>>> Pardon my lack of imagination: how does this help you with debugging?
>>>>
>>>> Ah, the patch subject is indeed incorrect, this should be:
>>>> "... for *tracing* purpose" (I use tracing when debugging).
>>>>
>>>> In the next patch, we use the 'description' property:
>>>>
>>>> +# pca9552.c
>>>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
>>>> 0-15 [%s]"
>>>>
>>>> So when the machine has multiple PCA9552 and guest accesses both,
>>>> we can distinct which one is used. For me having "pca1" / "pca0"
>>>> is easier to follow than the address of the QOM object.
>>>>
>>>>>
>>>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>>>>
>>>>>>>> Could it be a QOM attribute ? 
>>>>>>>
>>>>>>> What do you call a 'QOM attribute'?
>>>>>>> Is it what qdev properties implement?
>>>>>>> (in this case via DEFINE_PROP_STRING).
>>>>>>
>>>>>> I meant a default Object property, which would apply to all Objects. 
>>>>>
>>>>> Good point.  Many devices have multiple component objects of the same
>>>>> type.
>>>>>
>>>>>> What you did is fine, so :
>>>>>>
>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>>
>>>>>> but, may be, a well defined child name is enough for the purpose.
>>>>>
>>>>> object_get_canonical_path() returns a distinct path for each (component)
>>>>> object.  The path components are the child property names.
>>>>>
>>>>> Properties can have descriptions: object_property_set_description().
>>>>
>>>> TIL object_property_set_description :>
>>>>
>>>> Ah, there is no equivalent object_property_get_description(),
>>>> we have to use object_get_canonical_path(). Hmm, not obvious.
>>>>
>>>>>
>>>>> Sufficient?
>>>>
>>>> I don't know... This seems a complex way to do something simple...
>>>> This is already a QDEV. Having to use QOM API seems going
>>>> backward, since we have the DEFINE_PROP_STRING() macros available
>>>> in "hw/qdev-properties.h".
>>>>
>>>> Maybe I'm not seeing the advantages clearly. I'll try later.
>>>
>>> The canonical path is not very helpful in trace log...
>> 
>> Why?
>> 
>> Okay, I checked the code.  Since the devices in question don't get a
>> composition tree parent assigned, realize puts them in the
>> /machine/unattached orphanage.  The canonical path is something like
>> "/machine/unattached/device[6]", which is less than clear.

This is common for onboard devices.  I hate it.

Some boards do better than others.  For instance, ast2600-evb has 33
sensibly named entries under /machine/soc/, and only 9 entries in the
/machine/unattached/ orphanage.  i440fx has two sensible named ones
under /machine/, and 26 in the orphanage.

>> The components of the canonical path are the names of the QOM child
>> properties.  object_get_canonical_path_component() returns the last one,
>> in this case "device[6]".
>> 
>> If we made the devices QOM children of some other device, we could name
>> the child properties "pca0" and "pca1".
>> object_get_canonical_path_component() would then return the strings you
>> want to see.
>> 
>> We make a device a QOM child of some QOM parent device only if the child
>> is a component device of the parent (hence the name "composition
>> tree").
>> 
>> Are these devices integral components of something else, or are they
>> separate chips?
>
> Separate chips in the machine (actually not even on the machine mother
> board where is the CPU, but on a daughter board card).
>
> So in the composition tree I expect to see them as
>
>   /machine/pca0
>   /machine/pca1

As long as the final canonical path component is something readable
rather than auto-generated crap like "device[6]",
object_get_canonical_path_component() is usable for tracing.

>>> The description I set matches the hardware definitions
>>> and is easier to follow, see patch #6 (where it is set)
>>> where the description comes from:
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html
>>>
>>>   Description name taken from:
>>>   https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>>>
>>> So in this particular case I don't find the canonical pathname
>>> practical (from an hardware debugging perspective).
>> 
>> Personally, I'd be content with i2c bus and address for debugging
>> purposes.
>> 
>> The i2c buses *are* components: canonical paths look like
>> "/machine/soc/i2c/aspeed.i2c.3".  The combination of
>> object_get_canonical_path_component(dev) and
>> object_property_get_uint(dev, "address", &error_abort) identifies any
>> i2c device on this machine, not just the two you decorate with a
>> description string.
>
> The I2C busses is provided by Aspeed peripherals. I counted 19 different
> I2C busses on this machine.
>
> "pca0" is connected to i2c bus #11, "pca1" to bus #3.
>
> I still don't think this will be practical, but I'll try your
> suggestion.

"bus=11 addr=0x60" isn't exactly wonderful, but it seems practical
enough for me.  Even "device[6]" seems workable, just bothersome,
because it's needlessly hard to remember, and prone to change.

Mind, I'm not recommending any particular solution for "want a more
useful device ID in traces", I'm just throwing out ideas on how to solve
the problem in a more general way.

Working towards a a neater QOM composition tree might help with more
than tracing.