[PATCH v3 0/4] hw/misc/pca9552: Trace LED On/Off events

[PATCH v3 0/4] hw/misc/pca9552: Trace LED On/Off events

[Philippe Mathieu-Daudé]

This series add trace events to better display LEDs changes.
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_leds_status
  19286@1592574170.202791:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [*...............]
  19286@1592574170.203609:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [**..............]
  19286@1592574170.204102:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [***.............]
  19286@1592574170.204415:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****............]
  19286@1592574170.204758:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*..]
  19286@1592574170.205070:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........**.]
  19286@1592574170.205380:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574235.384845:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574235.894049:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574236.404277:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574236.914644:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574237.424558:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574237.934580:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574238.444688:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]

- Only display LEDs which status changes:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_led_change
  23367@1592575218.896117:pca9552_led_change 0x557cb6896d80 LED id:0 status: 0 -> 1
  23367@1592575218.897072:pca9552_led_change 0x557cb6896d80 LED id:1 status: 0 -> 1
  23367@1592575218.897487:pca9552_led_change 0x557cb6896d80 LED id:2 status: 0 -> 1
  23367@1592575218.897855:pca9552_led_change 0x557cb6896d80 LED id:3 status: 0 -> 1
  23367@1592575218.898256:pca9552_led_change 0x557cb6896d80 LED id:13 status: 0 -> 1
  23367@1592575218.898663:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
  23367@1592575218.899138:pca9552_led_change 0x557cb6896d80 LED id:15 status: 0 -> 1
  23367@1592575281.593379:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
  23367@1592575282.102994:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
  23367@1592575282.613558:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
  23367@1592575283.122774:pca9552_led_change 0x557cb6896d80 LED 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: <20200617064734.26956-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (4):
  hw/misc/pca9552: Replace magic value by PCA9552_LED_COUNT definition
  hw/misc/pca9552: Add a PCA955X_LED_MAX definition
  hw/misc/pca9552: Trace LED On/Off events
  hw/misc/pca9552: Trace LED change events

 include/hw/misc/pca9552.h |  2 ++
 hw/misc/pca9552.c         | 60 ++++++++++++++++++++++++++++++++++++++-
 hw/misc/trace-events      |  4 +++
 3 files changed, 65 insertions(+), 1 deletion(-)

-- 
2.21.3

[PATCH v3 1/4] hw/misc/pca9552: Replace magic value by PCA9552_LED_COUNT definition

[Philippe Mathieu-Daudé]

Replace the '16' magic value by the PCA9552_LED_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..b2b9a5d9d4 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_LED_COUNT 16
 
 typedef struct PCA9552State {
     /*< private >*/
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cac729e35a..e015ef0e5d 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_LED_COUNT;
 
     for (led = 0; led < s->nr_leds; led++) {
         char *name;
-- 
2.21.3

[PATCH v3 2/4] hw/misc/pca9552: Add a PCA955X_LED_MAX 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 LEDs (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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index e015ef0e5d..00fa91b7f4 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -18,6 +18,8 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 
+#define PCA955X_LED_MAX 16
+
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
@@ -303,6 +305,17 @@ static void pca9552_initfn(Object *obj)
     }
 }
 
+static void pca9552_realize(DeviceState *dev, Error **errp)
+{
+    PCA9552State *s = PCA9552(dev);
+
+    if (s->nr_leds > PCA955X_LED_MAX) {
+        error_setg(errp, "%s invalid led count %u (max: %u)",
+                   __func__, s->nr_leds, PCA955X_LED_MAX);
+        return;
+    }
+}
+
 static void pca9552_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -311,6 +324,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 v3 3/4] hw/misc/pca9552: Trace LED On/Off events

[Philippe Mathieu-Daudé]

Add a trivial representation of the PCA9552 LEDs.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_leds_status
  19286@1592574170.202791:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [*...............]
  19286@1592574170.203609:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [**..............]
  19286@1592574170.204102:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [***.............]
  19286@1592574170.204415:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****............]
  19286@1592574170.204758:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*..]
  19286@1592574170.205070:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........**.]
  19286@1592574170.205380:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574235.384845:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574235.894049:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574236.404277:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574236.914644:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574237.424558:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
  19286@1592574237.934580:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
  19286@1592574238.444688:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]

We notice the LED #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 b2b9a5d9d4..4e171d88c6 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 leds_status; /* Holds latest INPUT0 & INPUT1 status */
     uint8_t max_reg;
     uint8_t nr_leds;
 } PCA9552State;
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 00fa91b7f4..50c149077d 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -12,11 +12,13 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.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 PCA955X_LED_MAX 16
 
@@ -60,6 +62,32 @@ static void pca9552_update_pin_input(PCA9552State *s)
     }
 }
 
+static void pca9552_display_leds(PCA9552State *s)
+{
+    uint16_t leds_status, led_changed;
+    int i;
+
+    leds_status = (s->regs[PCA9552_INPUT1] << 8) | s->regs[PCA9552_INPUT0];
+    led_changed = s->leds_status ^ leds_status;
+    if (!led_changed) {
+        return;
+    }
+    s->leds_status = leds_status;
+    if (trace_event_get_state_backends(TRACE_PCA9552_LEDS_STATUS)) {
+        char buf[PCA9552_LED_COUNT + 1];
+
+        for (i = 0; i < s->nr_leds; i++) {
+            if (extract32(leds_status, i, 1)) {
+                buf[i] = '*';
+            } else {
+                buf[i] = '.';
+            }
+        }
+        buf[i] = '\0';
+        trace_pca9552_leds_status(s, buf);
+    }
+}
+
 static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
 {
     switch (reg) {
@@ -97,6 +125,7 @@ static void pca9552_write(PCA9552State *s, uint8_t reg, uint8_t data)
     case PCA9552_LS3:
         s->regs[reg] = data;
         pca9552_update_pin_input(s);
+        pca9552_display_leds(s);
         break;
 
     case PCA9552_INPUT0:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..5d9505ad0f 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_leds_status(void *object, const char *buf) "%p LEDs 0-15 [%s]"
-- 
2.21.3

[PATCH v3 4/4] hw/misc/pca9552: Trace LED change events

[Philippe Mathieu-Daudé]

Emit a trace event when a LED change its value.

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace pca9552_led_change
  23367@1592575218.896117:pca9552_led_change 0x557cb6896d80 LED id:0 status: 0 -> 1
  23367@1592575218.897072:pca9552_led_change 0x557cb6896d80 LED id:1 status: 0 -> 1
  23367@1592575218.897487:pca9552_led_change 0x557cb6896d80 LED id:2 status: 0 -> 1
  23367@1592575218.897855:pca9552_led_change 0x557cb6896d80 LED id:3 status: 0 -> 1
  23367@1592575218.898256:pca9552_led_change 0x557cb6896d80 LED id:13 status: 0 -> 1
  23367@1592575218.898663:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
  23367@1592575218.899138:pca9552_led_change 0x557cb6896d80 LED id:15 status: 0 -> 1
  23367@1592575281.593379:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
  23367@1592575282.102994:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
  23367@1592575282.613558:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
  23367@1592575283.122774:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1

We notice the LED #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 50c149077d..096ec3112a 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -62,6 +62,16 @@ static void pca9552_update_pin_input(PCA9552State *s)
     }
 }
 
+static void pca9552_update_led(PCA9552State *s, unsigned id, unsigned new_state)
+{
+    /*
+     * 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_led_change(s, id, !new_state, new_state);
+}
+
 static void pca9552_display_leds(PCA9552State *s)
 {
     uint16_t leds_status, led_changed;
@@ -86,6 +96,11 @@ static void pca9552_display_leds(PCA9552State *s)
         buf[i] = '\0';
         trace_pca9552_leds_status(s, buf);
     }
+    for (i = 0; i < s->nr_leds; i++) {
+        if (extract32(led_changed, i, 1)) {
+            pca9552_update_led(s, i, extract32(leds_status, i, 1));
+        }
+    }
 }
 
 static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5d9505ad0f..29892c8b28 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_leds_status(void *object, const char *buf) "%p LEDs 0-15 [%s]"
+pca9552_led_change(void *object, unsigned id, unsigned prev_state, unsigned current_state) "%p LED id:%u status: %u -> %u"
-- 
2.21.3

Re: [PATCH v3 0/4] hw/misc/pca9552: Trace LED On/Off events

[Philippe Mathieu-Daudé]

On 6/19/20 4:50 PM, Philippe Mathieu-Daudé wrote:
> This series add trace events to better display LEDs changes.
> This helps me to work on a generic LED device, see:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg711917.html

FYI I'm going to send a v4, using the generic LED device.

> 
> 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_leds_status
>   19286@1592574170.202791:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [*...............]
>   19286@1592574170.203609:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [**..............]
>   19286@1592574170.204102:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [***.............]
>   19286@1592574170.204415:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****............]
>   19286@1592574170.204758:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*..]
>   19286@1592574170.205070:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........**.]
>   19286@1592574170.205380:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
>   19286@1592574235.384845:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
>   19286@1592574235.894049:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
>   19286@1592574236.404277:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
>   19286@1592574236.914644:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
>   19286@1592574237.424558:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
>   19286@1592574237.934580:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........***]
>   19286@1592574238.444688:pca9552_leds_status 0x55dde47807c0 LEDs 0-15 [****.........*.*]
> 
> - Only display LEDs which status changes:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace pca9552_led_change
>   23367@1592575218.896117:pca9552_led_change 0x557cb6896d80 LED id:0 status: 0 -> 1
>   23367@1592575218.897072:pca9552_led_change 0x557cb6896d80 LED id:1 status: 0 -> 1
>   23367@1592575218.897487:pca9552_led_change 0x557cb6896d80 LED id:2 status: 0 -> 1
>   23367@1592575218.897855:pca9552_led_change 0x557cb6896d80 LED id:3 status: 0 -> 1
>   23367@1592575218.898256:pca9552_led_change 0x557cb6896d80 LED id:13 status: 0 -> 1
>   23367@1592575218.898663:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
>   23367@1592575218.899138:pca9552_led_change 0x557cb6896d80 LED id:15 status: 0 -> 1
>   23367@1592575281.593379:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
>   23367@1592575282.102994:pca9552_led_change 0x557cb6896d80 LED id:14 status: 0 -> 1
>   23367@1592575282.613558:pca9552_led_change 0x557cb6896d80 LED id:14 status: 1 -> 0
>   23367@1592575283.122774:pca9552_led_change 0x557cb6896d80 LED 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: <20200617064734.26956-1-f4bug@amsat.org>
> 
> Philippe Mathieu-Daudé (4):
>   hw/misc/pca9552: Replace magic value by PCA9552_LED_COUNT definition
>   hw/misc/pca9552: Add a PCA955X_LED_MAX definition
>   hw/misc/pca9552: Trace LED On/Off events
>   hw/misc/pca9552: Trace LED change events
> 
>  include/hw/misc/pca9552.h |  2 ++
>  hw/misc/pca9552.c         | 60 ++++++++++++++++++++++++++++++++++++++-
>  hw/misc/trace-events      |  4 +++
>  3 files changed, 65 insertions(+), 1 deletion(-)
>