[PATCH v3 0/7] hw/misc: Add LED device

[PATCH v3 0/7] hw/misc: Add LED device

[Philippe Mathieu-Daudé]

Hello,

These patches are part of the GSoC unselected 'QEMU visualizer'
project.  As the AVR port is not merged, I switched to microbit
to keep working on it.

This series presents a proof of concept of LED device that can
be easily connected to a GPIO.

Since v2:
- Rebased on PCA9552
- Model intensity to be ready for PWM use (Dave)
- Remove QMP events until we get a UI visualizer (Peter)
- Remove microbit patch (Peter)

Since v1: addressed Eric Blake review comments
- Added QMP rate limit

Next steps planned:

- PoC visualizer...
- look at using a dbus backend (elmarco)
- look at LED array/matrix such 7segments.

Regards,

Phil.

Based-on: <20200620225854.31160-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (7):
  hw/misc: Add a LED device
  hw/misc/led: Add helper to connect LED to GPIO output
  hw/misc/led: Emit a trace event when LED intensity has changed
  hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1
  hw/misc/mps2-fpgaio: Use the LED device
  hw/misc/mps2-scc: Use the LED device
  hw/arm/tosa: Replace fprintf() calls by LED devices

 include/hw/misc/led.h         | 100 ++++++++++++++++++++++
 include/hw/misc/mps2-fpgaio.h |   1 +
 include/hw/misc/mps2-scc.h    |   1 +
 hw/arm/aspeed.c               |  17 ++++
 hw/arm/tosa.c                 |  40 +++------
 hw/misc/led.c                 | 151 ++++++++++++++++++++++++++++++++++
 hw/misc/mps2-fpgaio.c         |  13 +--
 hw/misc/mps2-scc.c            |  23 +++---
 MAINTAINERS                   |   6 ++
 hw/arm/Kconfig                |   2 +
 hw/misc/Kconfig               |   5 ++
 hw/misc/Makefile.objs         |   1 +
 hw/misc/trace-events          |   6 +-
 13 files changed, 321 insertions(+), 45 deletions(-)
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

-- 
2.21.3

[PATCH v3 1/7] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

Add a LED device which can be connected to a GPIO output.
LEDs are limited to a set of colors.
They can also be dimmed with PWM devices. For now we do
not implement the dimmed mode, but in preparation of a
future implementation, we start using the LED intensity.
When used with GPIOs, the intensity can only be either
minium or maximum. This depends of the polarity of the
GPIO.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h |  79 +++++++++++++++++++++++++++
 hw/misc/led.c         | 121 ++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS           |   6 +++
 hw/misc/Kconfig       |   3 ++
 hw/misc/Makefile.objs |   1 +
 hw/misc/trace-events  |   3 ++
 6 files changed, 213 insertions(+)
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
new file mode 100644
index 0000000000..821ee1247d
--- /dev/null
+++ b/include/hw/misc/led.h
@@ -0,0 +1,79 @@
+/*
+ * QEMU single LED device
+ *
+ * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_MISC_LED_H
+#define HW_MISC_LED_H
+
+#include "hw/qdev-core.h"
+
+#define TYPE_LED "led"
+#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
+
+typedef enum {
+    LED_COLOR_UNKNOWN,
+    LED_COLOR_RED,
+    LED_COLOR_ORANGE,
+    LED_COLOR_AMBER,
+    LED_COLOR_YELLOW,
+    LED_COLOR_GREEN,
+    LED_COLOR_BLUE,
+    LED_COLOR_VIOLET, /* PURPLE */
+    LED_COLOR_WHITE,
+    LED_COLOR_COUNT
+} LEDColor;
+
+/* Definitions useful when a LED is connected to a GPIO */
+#define LED_RESET_INTENSITY_ACTIVE_LOW  UINT16_MAX
+#define LED_RESET_INTENSITY_ACTIVE_HIGH 0U
+
+typedef struct LEDState {
+    /* Private */
+    DeviceState parent_obj;
+    /* Public */
+
+    /* Properties */
+    char *description;
+    char *color;
+    /*
+     * When used with GPIO, the intensity at reset is related to GPIO polarity
+     */
+    uint16_t reset_intensity;
+} LEDState;
+
+/**
+ * led_set_intensity: set the intensity of a LED device
+ * @s: the LED object
+ * @intensity: new intensity
+ *
+ * This utility is meant for LED connected to PWM.
+ */
+void led_set_intensity(LEDState *s, uint16_t intensity);
+
+/**
+ * led_set_intensity: set the state of a LED device
+ * @s: the LED object
+ * @is_on: boolean indicating whether the LED is emitting
+ *
+ * This utility is meant for LED connected to GPIO.
+ */
+void led_set_state(LEDState *s, bool is_on);
+
+/**
+ * create_led: create and LED device
+ * @parent: the parent object
+ * @color: color of the LED
+ * @description: description of the LED (optional)
+ * @reset_intensity: LED intensity at reset
+ *
+ * This utility function creates a LED object.
+ */
+DeviceState *create_led(Object *parentobj,
+                        LEDColor color,
+                        const char *description,
+                        uint16_t reset_intensity);
+
+#endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
new file mode 100644
index 0000000000..e55ed7dbc4
--- /dev/null
+++ b/hw/misc/led.c
@@ -0,0 +1,121 @@
+/*
+ * QEMU single LED device
+ *
+ * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/led.h"
+#include "trace.h"
+
+static const char *led_color(LEDColor color)
+{
+    static const char *color_name[LED_COLOR_COUNT] = {
+        [LED_COLOR_RED] = "red",
+        [LED_COLOR_ORANGE] = "orange",
+        [LED_COLOR_AMBER] = "amber",
+        [LED_COLOR_YELLOW] = "yellow",
+        [LED_COLOR_GREEN] = "green",
+        [LED_COLOR_BLUE] = "blue",
+        [LED_COLOR_VIOLET] = "violet", /* PURPLE */
+        [LED_COLOR_WHITE] = "white",
+    };
+    return color_name[color] ? color_name[color] : "unknown";
+}
+
+void led_set_intensity(LEDState *s, uint16_t new_intensity)
+{
+    trace_led_set_intensity(s->description ? s->description : "n/a",
+                            s->color, new_intensity);
+    s->current_intensity = new_intensity;
+}
+
+void led_set_state(LEDState *s, bool is_on)
+{
+    led_set_intensity(s, is_on ? UINT16_MAX : 0);
+}
+
+static void led_reset(DeviceState *dev)
+{
+    LEDState *s = LED(dev);
+
+    led_set_intensity(s, s->reset_intensity);
+}
+
+static const VMStateDescription vmstate_led = {
+    .name = TYPE_LED,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void led_realize(DeviceState *dev, Error **errp)
+{
+    LEDState *s = LED(dev);
+
+    if (s->color == NULL) {
+        error_setg(errp, "property 'color' not specified");
+        return;
+    }
+}
+
+static Property led_properties[] = {
+    DEFINE_PROP_STRING("color", LEDState, color),
+    DEFINE_PROP_STRING("description", LEDState, description),
+    DEFINE_PROP_UINT16("reset_intensity", LEDState, reset_intensity, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void led_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "LED";
+    dc->vmsd = &vmstate_led;
+    dc->reset = led_reset;
+    dc->realize = led_realize;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    device_class_set_props(dc, led_properties);
+}
+
+static const TypeInfo led_info = {
+    .name = TYPE_LED,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(LEDState),
+    .class_init = led_class_init
+};
+
+static void led_register_types(void)
+{
+    type_register_static(&led_info);
+}
+
+type_init(led_register_types)
+
+DeviceState *create_led(Object *parentobj,
+                        LEDColor color,
+                        const char *description,
+                        uint16_t reset_intensity)
+{
+    DeviceState *dev;
+    char *name;
+
+    assert(description);
+    dev = qdev_new(TYPE_LED);
+    qdev_prop_set_uint16(dev, "reset_intensity", reset_intensity);
+    qdev_prop_set_string(dev, "color", led_color(color));
+    qdev_prop_set_string(dev, "description", description);
+    name = g_ascii_strdown(description, -1);
+    name = g_strdelimit(name, " #", '-');
+    object_property_add_child(parentobj, name, OBJECT(dev));
+    g_free(name);
+    qdev_realize_and_unref(dev, NULL, &error_fatal);
+
+    return dev;
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 955cc8dd5c..0fb8896b43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1863,6 +1863,12 @@ F: docs/specs/vmgenid.txt
 F: tests/qtest/vmgenid-test.c
 F: stubs/vmgenid.c
 
+LED
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Maintained
+F: include/hw/misc/led.h
+F: hw/misc/led.c
+
 Unimplemented device
 M: Peter Maydell <peter.maydell@linaro.org>
 R: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index bdd77d8020..f60dce694d 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -126,6 +126,9 @@ config AUX
 config UNIMP
     bool
 
+config LED
+    bool
+
 config MAC_VIA
     bool
     select MOS6522
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 5aaca8a039..9efa3c941c 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -91,3 +91,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
 obj-$(CONFIG_MAC_VIA) += mac_via.o
 
 common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
+common-obj-$(CONFIG_LED) += led.o
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 805d2110e0..f58853d367 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -207,6 +207,9 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
 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"
 
+# led.c
+led_set_intensity(const char *color, const char *desc, uint16_t intensity) "LED desc:'%s' color:%s intensity: 0x%04"PRIx16
+
 # 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 v3 2/7] hw/misc/led: Add helper to connect LED to GPIO output

[Philippe Mathieu-Daudé]

Some devices expose GPIO lines. Add the create_led_by_gpio_id()
helper to connect a LED to such GPIO.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Adding support for named GPIO is trivial. We don't need it yet.
---
 include/hw/misc/led.h | 20 ++++++++++++++++++++
 hw/misc/led.c         | 25 +++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 821ee1247d..883006bb8f 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -35,6 +35,8 @@ typedef struct LEDState {
     DeviceState parent_obj;
     /* Public */
 
+    qemu_irq irq;
+
     /* Properties */
     char *description;
     char *color;
@@ -76,4 +78,22 @@ DeviceState *create_led(Object *parentobj,
                         const char *description,
                         uint16_t reset_intensity);
 
+/**
+ * create_led_by_gpio_id: create and LED device and connect it to a GPIO output
+ * @parent: the parent object
+ * @gpio_dev: device exporting GPIOs
+ * @gpio_id: GPIO ID of this LED
+ * @color: color of the LED
+ * @description: description of the LED (optional)
+ * @reset_intensity: LED intensity at reset
+ *
+ * This utility function creates a LED and connects it to a
+ * GPIO exported by another device.
+ */
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   LEDColor color,
+                                   const char *description,
+                                   uint16_t reset_intensity);
+
 #endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
index e55ed7dbc4..8503dde777 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -10,6 +10,7 @@
 #include "migration/vmstate.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/led.h"
+#include "hw/irq.h"
 #include "trace.h"
 
 static const char *led_color(LEDColor color)
@@ -39,6 +40,14 @@ void led_set_state(LEDState *s, bool is_on)
     led_set_intensity(s, is_on ? UINT16_MAX : 0);
 }
 
+static void gpio_led_set(void *opaque, int line, int new_state)
+{
+    LEDState *s = LED(opaque);
+
+    assert(line == 0);
+    led_set_state(s, !!new_state);
+}
+
 static void led_reset(DeviceState *dev)
 {
     LEDState *s = LED(dev);
@@ -63,6 +72,8 @@ static void led_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "property 'color' not specified");
         return;
     }
+
+    qdev_init_gpio_in(DEVICE(s), gpio_led_set, 1);
 }
 
 static Property led_properties[] = {
@@ -119,3 +130,17 @@ DeviceState *create_led(Object *parentobj,
 
     return dev;
 }
+
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   LEDColor color,
+                                   const char *description,
+                                   uint16_t reset_intensity)
+{
+    DeviceState *dev;
+
+    dev = create_led(parentobj, color, description, reset_intensity);
+    qdev_connect_gpio_out(gpio_dev, gpio_id, qdev_get_gpio_in(dev, 0));
+
+    return dev;
+}
-- 
2.21.3

[PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Philippe Mathieu-Daudé]

Track the LED intensity, and emit a trace event when it changes.

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

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 883006bb8f..df5b32a2db 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -35,6 +35,7 @@ typedef struct LEDState {
     DeviceState parent_obj;
     /* Public */
 
+    uint16_t current_intensity;
     qemu_irq irq;
 
     /* Properties */
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 8503dde777..37d9f1f3d2 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -32,6 +32,11 @@ void led_set_intensity(LEDState *s, uint16_t new_intensity)
 {
     trace_led_set_intensity(s->description ? s->description : "n/a",
                             s->color, new_intensity);
+    if (new_intensity != s->current_intensity) {
+        trace_led_change_intensity(s->description ? s->description : "n/a",
+                                   s->color,
+                                   s->current_intensity, new_intensity);
+    }
     s->current_intensity = new_intensity;
 }
 
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index f58853d367..57d39bf9b9 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,6 +209,7 @@ grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx6
 
 # led.c
 led_set_intensity(const char *color, const char *desc, uint16_t intensity) "LED desc:'%s' color:%s intensity: 0x%04"PRIx16
+led_change_intensity(const char *color, const char *desc, uint16_t old_intensity, uint16_t new_intensity) "LED desc:'%s' color:%s intensity 0x%04"PRIx16" -> 0x%04"PRIx16""
 
 # pca9552.c
 pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
-- 
2.21.3

[PATCH v3 4/7] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1

[Philippe Mathieu-Daudé]

The Witherspoon has 3 LEDs connected to a PCA9552. Add them.
The names and reset values are taken from:
https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml

Example booting obmc-phosphor-image:

  $ qemu-system-arm -M witherspoon-bmc -trace led_change_intensity
  1592693373.997015:led_change_intensity LED desc:'front-fault-4' color:green intensity 0x0000 -> 0xffff
  1592693373.997632:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
  1592693373.998239:led_change_intensity LED desc:'front-id-5' color:green intensity 0x0000 -> 0xffff
  1592693500.291805:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
  1592693500.312041:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
  1592693500.821254:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
  1592693501.331517:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
  1592693501.841367:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
  1592693502.350839:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
  1592693502.861134:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
  1592693503.371090:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff

We notice the front-power LED starts to blink.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed.c | 17 +++++++++++++++++
 hw/arm/Kconfig  |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 3d5dec4692..217f8ad7d5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -20,6 +20,7 @@
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/tmp105.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
@@ -506,6 +507,16 @@ static void sonorapass_bmc_i2c_init(AspeedBoardState *bmc)
 
 static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 {
+    static const struct {
+        unsigned gpio_id;
+        LEDColor color;
+        const char *description;
+        uint16_t reset_intensity;
+    } pca1_leds[] = {
+        {13, LED_COLOR_GREEN, "front-fault-4",  LED_RESET_INTENSITY_ACTIVE_LOW},
+        {14, LED_COLOR_GREEN, "front-power-3",  LED_RESET_INTENSITY_ACTIVE_LOW},
+        {15, LED_COLOR_GREEN, "front-id-5",     LED_RESET_INTENSITY_ACTIVE_LOW},
+    };
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
     DeviceState *dev;
@@ -518,6 +529,12 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
     i2c_realize_and_unref(dev, aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
                           &error_fatal);
 
+    for (size_t i = 0; i < ARRAY_SIZE(pca1_leds); i++) {
+        create_led_by_gpio_id(OBJECT(bmc), dev,
+                              pca1_leds[i].gpio_id, pca1_leds[i].color,
+                              pca1_leds[i].description,
+                              pca1_leds[i].reset_intensity);
+    }
     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);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 9afa6eee79..1a57a861ac 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -393,6 +393,7 @@ config ASPEED_SOC
     select TMP105
     select TMP421
     select UNIMP
+    select LED
 
 config MPS2
     bool
-- 
2.21.3

[PATCH v3 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Philippe Mathieu-Daudé]

Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
Reference Manual' (100112_0200_07_en):

  2.1  Overview of the MPS2 and MPS2+ hardware

       The MPS2 and MPS2+ FPGA Prototyping Boards contain the
       following components and interfaces:

       * User switches and user LEDs:

         - Two green LEDs and two push buttons that connect to
           the FPGA.
         - Eight green LEDs and one 8-way dip switch that connect
           to the MCC.

Add the 2 LEDs connected to the FPGA.

This remplaces the 'mps2_fpgaio_leds' trace events by the generic
'led_set_intensity' event.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/mps2-fpgaio.h |  1 +
 hw/misc/mps2-fpgaio.c         | 13 ++++++++-----
 hw/misc/Kconfig               |  1 +
 hw/misc/trace-events          |  1 -
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h
index 69e265cd4b..228b813fd3 100644
--- a/include/hw/misc/mps2-fpgaio.h
+++ b/include/hw/misc/mps2-fpgaio.h
@@ -32,6 +32,7 @@ typedef struct {
 
     /*< public >*/
     MemoryRegion iomem;
+    DeviceState *led[2];
 
     uint32_t led0;
     uint32_t prescale;
diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
index 2f3fbeef34..65488f8634 100644
--- a/hw/misc/mps2-fpgaio.c
+++ b/hw/misc/mps2-fpgaio.c
@@ -24,6 +24,7 @@
 #include "migration/vmstate.h"
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-fpgaio.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 #include "qemu/timer.h"
 
@@ -176,12 +177,9 @@ static void mps2_fpgaio_write(void *opaque, hwaddr offset, uint64_t value,
 
     switch (offset) {
     case A_LED0:
-        /* LED bits [1:0] control board LEDs. We don't currently have
-         * a mechanism for displaying this graphically, so use a trace event.
-         */
-        trace_mps2_fpgaio_leds(value & 0x02 ? '*' : '.',
-                               value & 0x01 ? '*' : '.');
         s->led0 = value & 0x3;
+        led_set_state(LED(s->led[0]), value & 0x01);
+        led_set_state(LED(s->led[1]), value & 0x02);
         break;
     case A_PRESCALE:
         resync_counter(s);
@@ -249,6 +247,11 @@ static void mps2_fpgaio_init(Object *obj)
     memory_region_init_io(&s->iomem, obj, &mps2_fpgaio_ops, s,
                           "mps2-fpgaio", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    s->led[0] = create_led(obj, LED_COLOR_GREEN,
+                           "USERLED0", LED_RESET_INTENSITY_ACTIVE_HIGH);
+    s->led[1] = create_led(obj, LED_COLOR_GREEN,
+                           "USERLED1", LED_RESET_INTENSITY_ACTIVE_HIGH);
 }
 
 static bool mps2_fpgaio_counters_needed(void *opaque)
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index f60dce694d..889757731b 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -93,6 +93,7 @@ config MIPS_ITU
 
 config MPS2_FPGAIO
     bool
+    select LED
 
 config MPS2_SCC
     bool
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 57d39bf9b9..8bc7a675e8 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -89,7 +89,6 @@ mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC
 mps2_fpgaio_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_fpgaio_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 FPGAIO write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_fpgaio_reset(void) "MPS2 FPGAIO: reset"
-mps2_fpgaio_leds(char led1, char led0) "MPS2 FPGAIO LEDs: %c%c"
 
 # msf2-sysreg.c
 msf2_sysreg_write(uint64_t offset, uint32_t val, uint32_t prev) "msf2-sysreg write: addr 0x%08" PRIx64 " data 0x%" PRIx32 " prev 0x%" PRIx32
-- 
2.21.3

[PATCH v3 6/7] hw/misc/mps2-scc: Use the LED device

[Philippe Mathieu-Daudé]

Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
Reference Manual' (100112_0200_07_en):

  2.1  Overview of the MPS2 and MPS2+ hardware

       The MPS2 and MPS2+ FPGA Prototyping Boards contain the
       following components and interfaces:

       * User switches and user LEDs:

         - Two green LEDs and two push buttons that connect to
           the FPGA.
         - Eight green LEDs and one 8-way dip switch that connect
           to the MCC.

Add the 8 LEDs connected to the MCC.

This remplaces the 'mps2_scc_leds' trace events by the generic
'led_set_intensity' event.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
https://youtu.be/l9kD70uPchk?t=288
---
 include/hw/misc/mps2-scc.h |  1 +
 hw/misc/mps2-scc.c         | 23 ++++++++++++-----------
 hw/misc/Kconfig            |  1 +
 hw/misc/trace-events       |  1 -
 4 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
index 7045473788..29e12b832b 100644
--- a/include/hw/misc/mps2-scc.h
+++ b/include/hw/misc/mps2-scc.h
@@ -25,6 +25,7 @@ typedef struct {
 
     /*< public >*/
     MemoryRegion iomem;
+    DeviceState *led[8];
 
     uint32_t cfg0;
     uint32_t cfg1;
diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index 9d0909e7b3..2e59a382ec 100644
--- a/hw/misc/mps2-scc.c
+++ b/hw/misc/mps2-scc.c
@@ -20,11 +20,13 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "trace.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-scc.h"
+#include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 
 REG32(CFG0, 0)
@@ -152,18 +154,10 @@ static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
         s->cfg0 = value;
         break;
     case A_CFG1:
-        /* CFG1 bits [7:0] control the board LEDs. We don't currently have
-         * a mechanism for displaying this graphically, so use a trace event.
-         */
-        trace_mps2_scc_leds(value & 0x80 ? '*' : '.',
-                            value & 0x40 ? '*' : '.',
-                            value & 0x20 ? '*' : '.',
-                            value & 0x10 ? '*' : '.',
-                            value & 0x08 ? '*' : '.',
-                            value & 0x04 ? '*' : '.',
-                            value & 0x02 ? '*' : '.',
-                            value & 0x01 ? '*' : '.');
         s->cfg1 = value;
+        for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
+            led_set_state(LED(s->led[i]), !!extract32(value, i, 1));
+        }
         break;
     case A_CFGDATA_OUT:
         s->cfgdata_out = value;
@@ -245,6 +239,13 @@ static void mps2_scc_init(Object *obj)
 
     memory_region_init_io(&s->iomem, obj, &mps2_scc_ops, s, "mps2-scc", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
+        char *name = g_strdup_printf("SCC LED%zu", i);
+        s->led[i] = create_led(obj, LED_COLOR_GREEN,
+                               name, LED_RESET_INTENSITY_ACTIVE_HIGH);
+        g_free(name);
+    }
 }
 
 static void mps2_scc_realize(DeviceState *dev, Error **errp)
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 889757731b..f278f7f354 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -97,6 +97,7 @@ config MPS2_FPGAIO
 
 config MPS2_SCC
     bool
+    select LED
 
 config TZ_MPC
     bool
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 8bc7a675e8..490a0f341a 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -81,7 +81,6 @@ aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
 mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_scc_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 mps2_scc_reset(void) "MPS2 SCC: reset"
-mps2_scc_leds(char led7, char led6, char led5, char led4, char led3, char led2, char led1, char led0) "MPS2 SCC LEDs: %c%c%c%c%c%c%c%c"
 mps2_scc_cfg_write(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config write: function %d device %d data 0x%" PRIx32
 mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config read: function %d device %d data 0x%" PRIx32
 
-- 
2.21.3

[PATCH v3 7/7] hw/arm/tosa: Replace fprintf() calls by LED devices

[Philippe Mathieu-Daudé]

The recently added LED device reports LED status changes with
the 'led_set_intensity' trace event. It is less invasive than
the fprintf() calls. We need however to have a binary built
with tracing support.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/tosa.c  | 40 +++++++++++++---------------------------
 hw/arm/Kconfig |  1 +
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 5dee2d76c6..74b5fd1049 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -24,6 +24,7 @@
 #include "hw/irq.h"
 #include "hw/ssi/ssi.h"
 #include "hw/sysbus.h"
+#include "hw/misc/led.h"
 #include "exec/address-spaces.h"
 
 #define TOSA_RAM    0x04000000
@@ -65,27 +66,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
     pxa2xx_pcmcia_attach(cpu->pcmcia[0], md);
 }
 
-static void tosa_out_switch(void *opaque, int line, int level)
-{
-    switch (line) {
-        case 0:
-            fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
-            break;
-        case 1:
-            fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
-            break;
-        case 2:
-            fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
-            break;
-        case 3:
-            fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
-            break;
-        default:
-            fprintf(stderr, "Uhandled out event: %d = %d\n", line, level);
-            break;
-    }
-}
-
 static void tosa_reset(void *opaque, int line, int level)
 {
     if (level) {
@@ -98,7 +78,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                 DeviceState *scp1,
                 TC6393xbState *tmio)
 {
-    qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4);
     qemu_irq reset;
 
     /* MMC/SD host */
@@ -119,11 +98,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                         qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
                         NULL);
 
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);
-
     qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio));
 
     /* UDC Vbus */
@@ -234,6 +208,18 @@ static void tosa_init(MachineState *machine)
 
     scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
     scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_BT_LED,
+                          LED_COLOR_BLUE, "bluetooth", 0);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_NOTE_LED,
+                          LED_COLOR_GREEN, "note", 1);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_CHRG_ERR_LED,
+                          LED_COLOR_AMBER, "charger-error", 2);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_WLAN_LED,
+                          LED_COLOR_GREEN, "wlan", 3);
 
     tosa_gpio_setup(mpu, scp0, scp1, tmio);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 1a57a861ac..057c869d65 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -150,6 +150,7 @@ config TOSA
     select ZAURUS  # scoop
     select MICRODRIVE
     select PXA2XX
+    select LED
 
 config SPITZ
     bool
-- 
2.21.3

Re: [PATCH v3 0/7] hw/misc: Add LED device

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200620230719.32139-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    iotest-qcow2: 143
  TEST    iotest-qcow2: 150
**
ERROR:/tmp/qemu-test/src/hw/core/qdev.c:1074:device_finalize: assertion failed: (dev->canonical_path)
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 6, got 5)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 154
  TEST    iotest-qcow2: 156
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=33590eb6142241b28b0d323737d99da2', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0uwqudqi/src/docker-src.2020-06-20-19.46.06.26129:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=33590eb6142241b28b0d323737d99da2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0uwqudqi/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    12m38.919s
user    0m8.142s


The full log is available at
http://patchew.org/logs/20200620230719.32139-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 1/7] hw/misc: Add a LED device

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> Add a LED device which can be connected to a GPIO output.
> LEDs are limited to a set of colors.
> They can also be dimmed with PWM devices. For now we do
> not implement the dimmed mode, but in preparation of a
> future implementation, we start using the LED intensity.
> When used with GPIOs, the intensity can only be either
> minium or maximum. This depends of the polarity of the
> GPIO.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/led.h |  79 +++++++++++++++++++++++++++
>  hw/misc/led.c         | 121 ++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS           |   6 +++
>  hw/misc/Kconfig       |   3 ++
>  hw/misc/Makefile.objs |   1 +
>  hw/misc/trace-events  |   3 ++
>  6 files changed, 213 insertions(+)
>  create mode 100644 include/hw/misc/led.h
>  create mode 100644 hw/misc/led.c
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> new file mode 100644
> index 0000000000..821ee1247d
> --- /dev/null
> +++ b/include/hw/misc/led.h
> @@ -0,0 +1,79 @@
> +/*
> + * QEMU single LED device
> + *
> + * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef HW_MISC_LED_H
> +#define HW_MISC_LED_H
> +
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_LED "led"
> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
> +
> +typedef enum {
> +    LED_COLOR_UNKNOWN,
> +    LED_COLOR_RED,
> +    LED_COLOR_ORANGE,
> +    LED_COLOR_AMBER,
> +    LED_COLOR_YELLOW,
> +    LED_COLOR_GREEN,
> +    LED_COLOR_BLUE,
> +    LED_COLOR_VIOLET, /* PURPLE */
> +    LED_COLOR_WHITE,
> +    LED_COLOR_COUNT
> +} LEDColor;

Is color especially interesting, given that we only actually "display" the
color via tracing?

> +/* Definitions useful when a LED is connected to a GPIO */
> +#define LED_RESET_INTENSITY_ACTIVE_LOW  UINT16_MAX
> +#define LED_RESET_INTENSITY_ACTIVE_HIGH 0U
> +
> +typedef struct LEDState {
> +    /* Private */
> +    DeviceState parent_obj;
> +    /* Public */
> +
> +    /* Properties */
> +    char *description;
> +    char *color;

The enumeration is unused by the actual device, it would seem?

> +/**
> + * led_set_intensity: set the state of a LED device
> + * @s: the LED object
> + * @is_on: boolean indicating whether the LED is emitting
> + *
> + * This utility is meant for LED connected to GPIO.
> + */
> +void led_set_state(LEDState *s, bool is_on);

Comment mismatch.


> +void led_set_intensity(LEDState *s, uint16_t new_intensity)
> +{
> +    trace_led_set_intensity(s->description ? s->description : "n/a",
> +                            s->color, new_intensity);

Why not default description upon reset/realize?

> +static void led_realize(DeviceState *dev, Error **errp)
> +{
> +    LEDState *s = LED(dev);
> +
> +    if (s->color == NULL) {
> +        error_setg(errp, "property 'color' not specified");
> +        return;
> +    }
> +}

Indeed, why not insist that description is set?  If a board is forced to say
that the led is red, should it not also be forced to label it?

> +static Property led_properties[] = {
> +    DEFINE_PROP_STRING("color", LEDState, color),

It would appear that one can set any color via properties, including "plaid".
So if you do want the char *color field, what's the point in the enum?

> +# led.c
> +led_set_intensity(const char *color, const char *desc, uint16_t intensity) "LED desc:'%s' color:%s intensity: 0x%04"PRIx16

Is 0...65535 the best set of intensities?
Is that more valuable than e.g. a percentage?


r~

Re: [PATCH v3 7/7] hw/arm/tosa: Replace fprintf() calls by LED devices

[Philippe Mathieu-Daudé]

On 6/21/20 1:07 AM, Philippe Mathieu-Daudé wrote:
> The recently added LED device reports LED status changes with
> the 'led_set_intensity' trace event. It is less invasive than
> the fprintf() calls. We need however to have a binary built
> with tracing support.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/tosa.c  | 40 +++++++++++++---------------------------
>  hw/arm/Kconfig |  1 +
>  2 files changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 5dee2d76c6..74b5fd1049 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -24,6 +24,7 @@
>  #include "hw/irq.h"
>  #include "hw/ssi/ssi.h"
>  #include "hw/sysbus.h"
> +#include "hw/misc/led.h"
>  #include "exec/address-spaces.h"
>  
>  #define TOSA_RAM    0x04000000
> @@ -65,27 +66,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
>      pxa2xx_pcmcia_attach(cpu->pcmcia[0], md);
>  }
>  
> -static void tosa_out_switch(void *opaque, int line, int level)
> -{
> -    switch (line) {
> -        case 0:
> -            fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 1:
> -            fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 2:
> -            fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> -            break;
> -        case 3:
> -            fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> -            break;
> -        default:
> -            fprintf(stderr, "Uhandled out event: %d = %d\n", line, level);
> -            break;
> -    }
> -}
> -
>  static void tosa_reset(void *opaque, int line, int level)
>  {
>      if (level) {
> @@ -98,7 +78,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                  DeviceState *scp1,
>                  TC6393xbState *tmio)
>  {
> -    qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4);
>      qemu_irq reset;
>  
>      /* MMC/SD host */
> @@ -119,11 +98,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>                          qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
>                          NULL);
>  
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
> -    qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);
> -
>      qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio));
>  
>      /* UDC Vbus */
> @@ -234,6 +208,18 @@ static void tosa_init(MachineState *machine)
>  
>      scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
>      scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
> +    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
> +                          TOSA_GPIO_BT_LED,
> +                          LED_COLOR_BLUE, "bluetooth", 0);
> +    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
> +                          TOSA_GPIO_NOTE_LED,
> +                          LED_COLOR_GREEN, "note", 1);
> +    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
> +                          TOSA_GPIO_CHRG_ERR_LED,
> +                          LED_COLOR_AMBER, "charger-error", 2);
> +    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
> +                          TOSA_GPIO_WLAN_LED,
> +                          LED_COLOR_GREEN, "wlan", 3);

Oops, wrong last argument...

>  
>      tosa_gpio_setup(mpu, scp0, scp1, tmio);
>  
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 1a57a861ac..057c869d65 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -150,6 +150,7 @@ config TOSA
>      select ZAURUS  # scoop
>      select MICRODRIVE
>      select PXA2XX
> +    select LED
>  
>  config SPITZ
>      bool
> 

Re: [PATCH v3 6/7] hw/misc/mps2-scc: Use the LED device

[Philippe Mathieu-Daudé]

On 6/21/20 1:07 AM, Philippe Mathieu-Daudé wrote:
> Per the 'ARM MPS2 and MPS2+ FPGA Prototyping Boards Technical
> Reference Manual' (100112_0200_07_en):
> 
>   2.1  Overview of the MPS2 and MPS2+ hardware
> 
>        The MPS2 and MPS2+ FPGA Prototyping Boards contain the
>        following components and interfaces:
> 
>        * User switches and user LEDs:
> 
>          - Two green LEDs and two push buttons that connect to
>            the FPGA.
>          - Eight green LEDs and one 8-way dip switch that connect
>            to the MCC.
> 
> Add the 8 LEDs connected to the MCC.
> 
> This remplaces the 'mps2_scc_leds' trace events by the generic
> 'led_set_intensity' event.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> https://youtu.be/l9kD70uPchk?t=288
> ---
>  include/hw/misc/mps2-scc.h |  1 +
>  hw/misc/mps2-scc.c         | 23 ++++++++++++-----------
>  hw/misc/Kconfig            |  1 +
>  hw/misc/trace-events       |  1 -
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
> index 7045473788..29e12b832b 100644
> --- a/include/hw/misc/mps2-scc.h
> +++ b/include/hw/misc/mps2-scc.h
> @@ -25,6 +25,7 @@ typedef struct {
>  
>      /*< public >*/
>      MemoryRegion iomem;
> +    DeviceState *led[8];
>  
>      uint32_t cfg0;
>      uint32_t cfg1;
> diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
> index 9d0909e7b3..2e59a382ec 100644
> --- a/hw/misc/mps2-scc.c
> +++ b/hw/misc/mps2-scc.c
> @@ -20,11 +20,13 @@
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "qemu/bitops.h"
>  #include "trace.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "hw/registerfields.h"
>  #include "hw/misc/mps2-scc.h"
> +#include "hw/misc/led.h"
>  #include "hw/qdev-properties.h"
>  
>  REG32(CFG0, 0)
> @@ -152,18 +154,10 @@ static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
>          s->cfg0 = value;
>          break;
>      case A_CFG1:
> -        /* CFG1 bits [7:0] control the board LEDs. We don't currently have
> -         * a mechanism for displaying this graphically, so use a trace event.
> -         */
> -        trace_mps2_scc_leds(value & 0x80 ? '*' : '.',
> -                            value & 0x40 ? '*' : '.',
> -                            value & 0x20 ? '*' : '.',
> -                            value & 0x10 ? '*' : '.',
> -                            value & 0x08 ? '*' : '.',
> -                            value & 0x04 ? '*' : '.',
> -                            value & 0x02 ? '*' : '.',
> -                            value & 0x01 ? '*' : '.');
>          s->cfg1 = value;
> +        for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
> +            led_set_state(LED(s->led[i]), !!extract32(value, i, 1));
> +        }
>          break;
>      case A_CFGDATA_OUT:
>          s->cfgdata_out = value;
> @@ -245,6 +239,13 @@ static void mps2_scc_init(Object *obj)
>  
>      memory_region_init_io(&s->iomem, obj, &mps2_scc_ops, s, "mps2-scc", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
> +        char *name = g_strdup_printf("SCC LED%zu", i);
> +        s->led[i] = create_led(obj, LED_COLOR_GREEN,
> +                               name, LED_RESET_INTENSITY_ACTIVE_HIGH);
> +        g_free(name);
> +    }

This has to go ...

>  }
>  
>  static void mps2_scc_realize(DeviceState *dev, Error **errp)

... here to avoid:

**
ERROR:/tmp/qemu-test/src/hw/core/qdev.c:1074:device_finalize: assertion
failed: (dev->canonical_path)
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU
death from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 6, got 5)

> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 889757731b..f278f7f354 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -97,6 +97,7 @@ config MPS2_FPGAIO
>  
>  config MPS2_SCC
>      bool
> +    select LED
>  
>  config TZ_MPC
>      bool
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 8bc7a675e8..490a0f341a 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -81,7 +81,6 @@ aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
>  mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>  mps2_scc_write(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>  mps2_scc_reset(void) "MPS2 SCC: reset"
> -mps2_scc_leds(char led7, char led6, char led5, char led4, char led3, char led2, char led1, char led0) "MPS2 SCC LEDs: %c%c%c%c%c%c%c%c"
>  mps2_scc_cfg_write(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config write: function %d device %d data 0x%" PRIx32
>  mps2_scc_cfg_read(unsigned function, unsigned device, uint32_t value) "MPS2 SCC config read: function %d device %d data 0x%" PRIx32
>  
> 

Re: [PATCH v3 2/7] hw/misc/led: Add helper to connect LED to GPIO output

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> Some devices expose GPIO lines. Add the create_led_by_gpio_id()
> helper to connect a LED to such GPIO.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Adding support for named GPIO is trivial. We don't need it yet.
> ---
>  include/hw/misc/led.h | 20 ++++++++++++++++++++
>  hw/misc/led.c         | 25 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)

Modulo my question re colors, looks good.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> Track the LED intensity, and emit a trace event when it changes.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/led.h | 1 +
>  hw/misc/led.c         | 5 +++++
>  hw/misc/trace-events  | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
> index 883006bb8f..df5b32a2db 100644
> --- a/include/hw/misc/led.h
> +++ b/include/hw/misc/led.h
> @@ -35,6 +35,7 @@ typedef struct LEDState {
>      DeviceState parent_obj;
>      /* Public */
>  
> +    uint16_t current_intensity;
>      qemu_irq irq;

Why not sort this new field next to the other uint16_t and (partially) fill the
hole?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 1/7] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

Hi Richard,

On 6/21/20 4:00 AM, Richard Henderson wrote:
> On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
>> Add a LED device which can be connected to a GPIO output.
>> LEDs are limited to a set of colors.
>> They can also be dimmed with PWM devices. For now we do
>> not implement the dimmed mode, but in preparation of a
>> future implementation, we start using the LED intensity.
>> When used with GPIOs, the intensity can only be either
>> minium or maximum. This depends of the polarity of the
>> GPIO.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h |  79 +++++++++++++++++++++++++++
>>  hw/misc/led.c         | 121 ++++++++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS           |   6 +++
>>  hw/misc/Kconfig       |   3 ++
>>  hw/misc/Makefile.objs |   1 +
>>  hw/misc/trace-events  |   3 ++
>>  6 files changed, 213 insertions(+)
>>  create mode 100644 include/hw/misc/led.h
>>  create mode 100644 hw/misc/led.c
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> new file mode 100644
>> index 0000000000..821ee1247d
>> --- /dev/null
>> +++ b/include/hw/misc/led.h
>> @@ -0,0 +1,79 @@
>> +/*
>> + * QEMU single LED device
>> + *
>> + * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +#ifndef HW_MISC_LED_H
>> +#define HW_MISC_LED_H
>> +
>> +#include "hw/qdev-core.h"
>> +
>> +#define TYPE_LED "led"
>> +#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
>> +
>> +typedef enum {
>> +    LED_COLOR_UNKNOWN,
>> +    LED_COLOR_RED,
>> +    LED_COLOR_ORANGE,
>> +    LED_COLOR_AMBER,
>> +    LED_COLOR_YELLOW,
>> +    LED_COLOR_GREEN,
>> +    LED_COLOR_BLUE,
>> +    LED_COLOR_VIOLET, /* PURPLE */
>> +    LED_COLOR_WHITE,
>> +    LED_COLOR_COUNT
>> +} LEDColor;
> 
> Is color especially interesting, given that we only actually "display" the
> color via tracing?

The idea of this device is to easily visualize events. Currently
via tracing, but eventually an external UI could introspect the
board for devices able to represent visual changes such LEDs, and
automatically display them.
To limit the list of representable object the visualizer has to
support, I prefer to restrict this device to the existing LED
physical colors.

> 
>> +/* Definitions useful when a LED is connected to a GPIO */
>> +#define LED_RESET_INTENSITY_ACTIVE_LOW  UINT16_MAX
>> +#define LED_RESET_INTENSITY_ACTIVE_HIGH 0U
>> +
>> +typedef struct LEDState {
>> +    /* Private */
>> +    DeviceState parent_obj;
>> +    /* Public */
>> +
>> +    /* Properties */
>> +    char *description;
>> +    char *color;
> 
> The enumeration is unused by the actual device, it would seem?

I want to, but it seems having a enum qdev property involves
a lot of code.

> 
>> +/**
>> + * led_set_intensity: set the state of a LED device
>> + * @s: the LED object
>> + * @is_on: boolean indicating whether the LED is emitting
>> + *
>> + * This utility is meant for LED connected to GPIO.
>> + */
>> +void led_set_state(LEDState *s, bool is_on);
> 
> Comment mismatch.
> 
> 
>> +void led_set_intensity(LEDState *s, uint16_t new_intensity)
>> +{
>> +    trace_led_set_intensity(s->description ? s->description : "n/a",
>> +                            s->color, new_intensity);
> 
> Why not default description upon reset/realize?

Yes.

> 
>> +static void led_realize(DeviceState *dev, Error **errp)
>> +{
>> +    LEDState *s = LED(dev);
>> +
>> +    if (s->color == NULL) {
>> +        error_setg(errp, "property 'color' not specified");
>> +        return;
>> +    }
>> +}
> 
> Indeed, why not insist that description is set?  If a board is forced to say
> that the led is red, should it not also be forced to label it?

Because when we don't have access to the hardware schematics,
we can not specify the label. I'll add a comment about this.

> 
>> +static Property led_properties[] = {
>> +    DEFINE_PROP_STRING("color", LEDState, color),
> 
> It would appear that one can set any color via properties, including "plaid".
> So if you do want the char *color field, what's the point in the enum?

Good catch... I will either use an enum propery, or check the
property is in the allowed color names.

> 
>> +# led.c
>> +led_set_intensity(const char *color, const char *desc, uint16_t intensity) "LED desc:'%s' color:%s intensity: 0x%04"PRIx16
> 
> Is 0...65535 the best set of intensities?

You are right. I was thinking of PWM resolution (limiting to
16-bits). This is a different API to model, I mixed.

> Is that more valuable than e.g. a percentage?

Yes, the [0-100] range of integer is sufficient to represent
light intensity =)

Thanks for your review!

Phil.

Re: [PATCH v3 1/7] hw/misc: Add a LED device

[Richard Henderson]

On 6/21/20 1:35 PM, Philippe Mathieu-Daudé wrote:
>> Is color especially interesting, given that we only actually "display" the
>> color via tracing?
> 
> The idea of this device is to easily visualize events. Currently
> via tracing, but eventually an external UI could introspect the
> board for devices able to represent visual changes such LEDs, and
> automatically display them.
> To limit the list of representable object the visualizer has to
> support, I prefer to restrict this device to the existing LED
> physical colors.

Ok.  This does suggest that we do use then enum in the structure.

>> Indeed, why not insist that description is set?  If a board is forced to say
>> that the led is red, should it not also be forced to label it?
> 
> Because when we don't have access to the hardware schematics,
> we can not specify the label. I'll add a comment about this.

Well, if we don't have a hardware schematic label, then we must have the i/o
pin; we could insist that the board label the led in some way that makes some
sense.


r~

Re: [PATCH v3 4/7] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> The Witherspoon has 3 LEDs connected to a PCA9552. Add them.
> The names and reset values are taken from:
> https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
> 
> Example booting obmc-phosphor-image:
> 
>   $ qemu-system-arm -M witherspoon-bmc -trace led_change_intensity
>   1592693373.997015:led_change_intensity LED desc:'front-fault-4' color:green intensity 0x0000 -> 0xffff
>   1592693373.997632:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
>   1592693373.998239:led_change_intensity LED desc:'front-id-5' color:green intensity 0x0000 -> 0xffff
>   1592693500.291805:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
>   1592693500.312041:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
>   1592693500.821254:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
>   1592693501.331517:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
>   1592693501.841367:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
>   1592693502.350839:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
>   1592693502.861134:led_change_intensity LED desc:'front-power-3' color:green intensity 0xffff -> 0x0000
>   1592693503.371090:led_change_intensity LED desc:'front-power-3' color:green intensity 0x0000 -> 0xffff
> 
> We notice the front-power LED starts to blink.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/aspeed.c | 17 +++++++++++++++++
>  hw/arm/Kconfig  |  1 +
>  2 files changed, 18 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> +    DeviceState *led[2];

Perhaps better as LEDState?  And perhaps return that from create_led.


r~

Re: [PATCH v3 6/7] hw/misc/mps2-scc: Use the LED device

[Richard Henderson]

On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
> @@ -25,6 +25,7 @@ typedef struct {
>  
>      /*< public >*/
>      MemoryRegion iomem;
> +    DeviceState *led[8];

LEDState?

> +        for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
> +            led_set_state(LED(s->led[i]), !!extract32(value, i, 1));
> +        }

No need for !!.


r~

Re: [PATCH v3 5/7] hw/misc/mps2-fpgaio: Use the LED device

[Philippe Mathieu-Daudé]

On 6/21/20 11:00 PM, Richard Henderson wrote:
> On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
>> +    DeviceState *led[2];
> 
> Perhaps better as LEDState?  And perhaps return that from create_led.

I guess I first thought about using an opaque structure
with forward typedef declaration, but in this case I also
prefer your suggestion.

Thanks :)

Re: [PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Philippe Mathieu-Daudé]

On 6/21/20 9:23 PM, Richard Henderson wrote:
> On 6/20/20 4:07 PM, Philippe Mathieu-Daudé wrote:
>> Track the LED intensity, and emit a trace event when it changes.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/led.h | 1 +
>>  hw/misc/led.c         | 5 +++++
>>  hw/misc/trace-events  | 1 +
>>  3 files changed, 7 insertions(+)
>>
>> diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
>> index 883006bb8f..df5b32a2db 100644
>> --- a/include/hw/misc/led.h
>> +++ b/include/hw/misc/led.h
>> @@ -35,6 +35,7 @@ typedef struct LEDState {
>>      DeviceState parent_obj;
>>      /* Public */
>>  
>> +    uint16_t current_intensity;
>>      qemu_irq irq;
> 
> Why not sort this new field next to the other uint16_t and (partially) fill the
> hole?

From a reviewer point of view, I prefer to keep the state fields
separated from the properties fields, wasting few bits of RAM.

Anyway I switched to a percent value. What is better to hold
it, an 'unsigned' or 'uint8_t' type?

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

> 
> 
> r~
> 

Re: [PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Richard Henderson]

On 6/21/20 3:25 PM, Philippe Mathieu-Daudé wrote:
> Anyway I switched to a percent value. What is better to hold
> it, an 'unsigned' or 'uint8_t' type?

Might as well use unsigned; you'll not be saving space with uint8_t.


r~

Re: [PATCH v3 3/7] hw/misc/led: Emit a trace event when LED intensity has changed

[Philippe Mathieu-Daudé]

On 6/22/20 6:48 PM, Richard Henderson wrote:
> On 6/21/20 3:25 PM, Philippe Mathieu-Daudé wrote:
>> Anyway I switched to a percent value. What is better to hold
>> it, an 'unsigned' or 'uint8_t' type?
> 
> Might as well use unsigned; you'll not be saving space with uint8_t.

Bah if we want to migrate the intensity, we can't use 'unsigned',
so I'll keep uint8_t / VMSTATE_UINT8.