* [PATCH v4 1/8] hw/misc/led: Add a LED device
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 20:03 ` Luc Michel
2020-09-07 16:32 ` [PATCH v4 2/8] hw/misc/led: Allow connecting from GPIO output Philippe Mathieu-Daudé
` (6 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Philippe Mathieu-Daudé, qemu-arm, Cédric Le Goater,
Luc Michel, Joel Stanley
Add a LED device which can be connected to a GPIO output.
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.
LEDs are limited to a fixed set of colors.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/led.h | 84 +++++++++++++++++++++++++
hw/misc/led.c | 142 ++++++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 6 ++
hw/misc/Kconfig | 3 +
hw/misc/meson.build | 1 +
hw/misc/trace-events | 3 +
6 files changed, 239 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 00000000000..1aaabbebafc
--- /dev/null
+++ b/include/hw/misc/led.h
@@ -0,0 +1,84 @@
+/*
+ * 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)
+
+/**
+ * LEDColor: Color of a LED
+ *
+ * This set is restricted to physically available LED colors.
+ *
+ * LED colors from 'Table 1. Product performance of LUXEON Rebel Color
+ * Line' of the 'DS68 LUXEON Rebel Color Line' datasheet available at:
+ * https://www.lumileds.com/products/color-leds/luxeon-rebel-color/
+ */
+typedef enum { /* Coarse wavelength range */
+ LED_COLOR_VIOLET, /* 425 nm */
+ LED_COLOR_BLUE, /* 475 nm */
+ LED_COLOR_CYAN, /* 500 nm */
+ LED_COLOR_GREEN, /* 535 nm */
+ LED_COLOR_AMBER, /* 590 nm */
+ LED_COLOR_ORANGE, /* 615 nm */
+ LED_COLOR_RED, /* 630 nm */
+} LEDColor;
+
+typedef struct LEDState {
+ /* Private */
+ DeviceState parent_obj;
+ /* Public */
+
+ uint8_t intensity_percent;
+
+ /* Properties */
+ char *description;
+ char *color;
+} LEDState;
+
+/**
+ * led_set_intensity: set the intensity of a LED device
+ * @s: the LED object
+ * @intensity: intensity as percentage in range 0 to 100.
+ */
+void led_set_intensity(LEDState *s, unsigned intensity_percent);
+
+/**
+ * led_get_intensity:
+ * @s: the LED object
+ *
+ * Returns: The LED intensity as percentage in range 0 to 100.
+ */
+unsigned led_get_intensity(LEDState *s);
+
+/**
+ * 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_emitting);
+
+/**
+ * led_create_simple: Create and realize a LED device
+ * @parent: the parent object
+ * @color: color of the LED
+ * @description: description of the LED (optional)
+ *
+ * Create the device state structure, initialize it, and
+ * drop the reference to it (the device is realized).
+ */
+LEDState *led_create_simple(Object *parentobj,
+ LEDColor color,
+ const char *description);
+
+#endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
new file mode 100644
index 00000000000..f2140739b68
--- /dev/null
+++ b/hw/misc/led.c
@@ -0,0 +1,142 @@
+/*
+ * 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"
+
+#define LED_INTENSITY_PERCENT_MAX 100
+
+static const char *led_color_name[] = {
+ [LED_COLOR_VIOLET] = "violet",
+ [LED_COLOR_BLUE] = "blue",
+ [LED_COLOR_CYAN] = "cyan",
+ [LED_COLOR_GREEN] = "green",
+ [LED_COLOR_AMBER] = "amber",
+ [LED_COLOR_ORANGE] = "orange",
+ [LED_COLOR_RED] = "red",
+};
+
+static bool led_color_name_is_valid(const char *color_name)
+{
+ for (size_t i = 0; i < ARRAY_SIZE(led_color_name); i++) {
+ if (led_color_name[i] && strcmp(color_name, led_color_name[i]) == 0) {
+ return true;
+ }
+ }
+ return false;
+}
+
+void led_set_intensity(LEDState *s, unsigned intensity_percent)
+{
+ if (intensity_percent > LED_INTENSITY_PERCENT_MAX) {
+ intensity_percent = LED_INTENSITY_PERCENT_MAX;
+ }
+ trace_led_set_intensity(s->description, s->color, intensity_percent);
+ s->intensity_percent = intensity_percent;
+}
+
+unsigned led_get_intensity(LEDState *s)
+{
+ return s->intensity_percent;
+}
+
+void led_set_state(LEDState *s, bool is_emitting)
+{
+ led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
+}
+
+static void led_reset(DeviceState *dev)
+{
+ LEDState *s = LED(dev);
+
+ led_set_state(s, false);
+}
+
+static const VMStateDescription vmstate_led = {
+ .name = TYPE_LED,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(intensity_percent, LEDState),
+ 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;
+ } else if (!led_color_name_is_valid(s->color)) {
+ error_setg(errp, "property 'color' invalid or not supported");
+ return;
+ }
+ if (s->description == NULL) {
+ s->description = g_strdup("n/a");
+ }
+}
+
+static Property led_properties[] = {
+ DEFINE_PROP_STRING("color", LEDState, color),
+ DEFINE_PROP_STRING("description", LEDState, description),
+ 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)
+
+LEDState *led_create_simple(Object *parentobj,
+ LEDColor color,
+ const char *description)
+{
+ g_autofree char *name = NULL;
+ DeviceState *dev;
+
+ dev = qdev_new(TYPE_LED);
+ qdev_prop_set_string(dev, "color", led_color_name[color]);
+ if (!description) {
+ static unsigned undescribed_led_id;
+ name = g_strdup_printf("undescribed-led-#%u", undescribed_led_id++);
+ } else {
+ 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));
+ qdev_realize_and_unref(dev, NULL, &error_fatal);
+
+ return LED(dev);
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index b233da2a737..d040846b868 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1951,6 +1951,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 92c397ca07a..5c151fa3a83 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/meson.build b/hw/misc/meson.build
index e1576b81cf9..26f6dd037dc 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -11,6 +11,7 @@ softmmu_ss.add(when: 'CONFIG_TMP105', if_true: files('tmp105.c'))
softmmu_ss.add(when: 'CONFIG_TMP421', if_true: files('tmp421.c'))
softmmu_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
softmmu_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
+softmmu_ss.add(when: 'CONFIG_LED', if_true: files('led.c'))
# ARM devices
softmmu_ss.add(when: 'CONFIG_PL310', if_true: files('arm_l2x0.c'))
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 066752aa900..76c9ddb54fe 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -214,6 +214,9 @@ via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size
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, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
+
# pca9552.c
pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 1/8] hw/misc/led: Add a LED device
2020-09-07 16:32 ` [PATCH v4 1/8] hw/misc/led: Add a " Philippe Mathieu-Daudé
@ 2020-09-07 20:03 ` Luc Michel
2020-09-07 20:24 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: Luc Michel @ 2020-09-07 20:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres, qemu-arm,
Cédric Le Goater, Joel Stanley
Hi Philippe,
On 9/7/20 6:32 PM, Philippe Mathieu-Daudé wrote:
> Add a LED device which can be connected to a GPIO output.
> 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.
>
> LEDs are limited to a fixed set of colors.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/hw/misc/led.h | 84 +++++++++++++++++++++++++
> hw/misc/led.c | 142 ++++++++++++++++++++++++++++++++++++++++++
> MAINTAINERS | 6 ++
> hw/misc/Kconfig | 3 +
> hw/misc/meson.build | 1 +
> hw/misc/trace-events | 3 +
> 6 files changed, 239 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 00000000000..1aaabbebafc
> --- /dev/null
> +++ b/include/hw/misc/led.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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)
> +
> +/**
> + * LEDColor: Color of a LED
> + *
> + * This set is restricted to physically available LED colors.
> + *
> + * LED colors from 'Table 1. Product performance of LUXEON Rebel Color
> + * Line' of the 'DS68 LUXEON Rebel Color Line' datasheet available at:
> + * https://www.lumileds.com/products/color-leds/luxeon-rebel-color/
> + */
> +typedef enum { /* Coarse wavelength range */
> + LED_COLOR_VIOLET, /* 425 nm */
> + LED_COLOR_BLUE, /* 475 nm */
> + LED_COLOR_CYAN, /* 500 nm */
> + LED_COLOR_GREEN, /* 535 nm */
> + LED_COLOR_AMBER, /* 590 nm */
> + LED_COLOR_ORANGE, /* 615 nm */
> + LED_COLOR_RED, /* 630 nm */
> +} LEDColor;
> +
> +typedef struct LEDState {
> + /* Private */
> + DeviceState parent_obj;
> + /* Public */
> +
> + uint8_t intensity_percent;
> +
> + /* Properties */
> + char *description;
> + char *color;
> +} LEDState;
> +
> +/**
> + * led_set_intensity: set the intensity of a LED device
> + * @s: the LED object
> + * @intensity: intensity as percentage in range 0 to 100.
@intensity_percent
> + */
> +void led_set_intensity(LEDState *s, unsigned intensity_percent);
> +
> +/**
> + * led_get_intensity:
> + * @s: the LED object
> + *
> + * Returns: The LED intensity as percentage in range 0 to 100.
> + */
> +unsigned led_get_intensity(LEDState *s);
> +
> +/**
> + * led_set_intensity: set the state of a LED device
led_set_state
> + * @s: the LED object
> + * @is_on: boolean indicating whether the LED is emitting
@is_emitting
> + *
> + * This utility is meant for LED connected to GPIO.
> + */
> +void led_set_state(LEDState *s, bool is_emitting);
> +
> +/**
> + * led_create_simple: Create and realize a LED device
> + * @parent: the parent object
@parentobj
> + * @color: color of the LED
> + * @description: description of the LED (optional)
> + *
> + * Create the device state structure, initialize it, and
> + * drop the reference to it (the device is realized).
> + */
> +LEDState *led_create_simple(Object *parentobj,
> + LEDColor color,
> + const char *description);
> +
> +#endif /* HW_MISC_LED_H */
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> new file mode 100644
> index 00000000000..f2140739b68
> --- /dev/null
> +++ b/hw/misc/led.c
> @@ -0,0 +1,142 @@
> +/*
> + * 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"
> +
> +#define LED_INTENSITY_PERCENT_MAX 100
> +
> +static const char *led_color_name[] = {
> + [LED_COLOR_VIOLET] = "violet",
> + [LED_COLOR_BLUE] = "blue",
> + [LED_COLOR_CYAN] = "cyan",
> + [LED_COLOR_GREEN] = "green",
> + [LED_COLOR_AMBER] = "amber",
> + [LED_COLOR_ORANGE] = "orange",
> + [LED_COLOR_RED] = "red",
> +};
> +
> +static bool led_color_name_is_valid(const char *color_name)
> +{
> + for (size_t i = 0; i < ARRAY_SIZE(led_color_name); i++) {
> + if (led_color_name[i] && strcmp(color_name, led_color_name[i]) == 0) {
Why are you checking led_color_name[i] here?
Otherwise, seems good to me.
Reviewed-by: Luc Michel <luc.michel@greensocs.com>
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +void led_set_intensity(LEDState *s, unsigned intensity_percent)
> +{
> + if (intensity_percent > LED_INTENSITY_PERCENT_MAX) {
> + intensity_percent = LED_INTENSITY_PERCENT_MAX;
> + }
> + trace_led_set_intensity(s->description, s->color, intensity_percent);
> + s->intensity_percent = intensity_percent;
> +}
> +
> +unsigned led_get_intensity(LEDState *s)
> +{
> + return s->intensity_percent;
> +}
> +
> +void led_set_state(LEDState *s, bool is_emitting)
> +{
> + led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
> +}
> +
> +static void led_reset(DeviceState *dev)
> +{
> + LEDState *s = LED(dev);
> +
> + led_set_state(s, false);
> +}
> +
> +static const VMStateDescription vmstate_led = {
> + .name = TYPE_LED,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(intensity_percent, LEDState),
> + 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;
> + } else if (!led_color_name_is_valid(s->color)) {
> + error_setg(errp, "property 'color' invalid or not supported");
> + return;
> + }
> + if (s->description == NULL) {
> + s->description = g_strdup("n/a");
> + }
> +}
> +
> +static Property led_properties[] = {
> + DEFINE_PROP_STRING("color", LEDState, color),
> + DEFINE_PROP_STRING("description", LEDState, description),
> + 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)
> +
> +LEDState *led_create_simple(Object *parentobj,
> + LEDColor color,
> + const char *description)
> +{
> + g_autofree char *name = NULL;
> + DeviceState *dev;
> +
> + dev = qdev_new(TYPE_LED);
> + qdev_prop_set_string(dev, "color", led_color_name[color]);
> + if (!description) {
> + static unsigned undescribed_led_id;
> + name = g_strdup_printf("undescribed-led-#%u", undescribed_led_id++);
> + } else {
> + 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));
> + qdev_realize_and_unref(dev, NULL, &error_fatal);
> +
> + return LED(dev);
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b233da2a737..d040846b868 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1951,6 +1951,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 92c397ca07a..5c151fa3a83 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/meson.build b/hw/misc/meson.build
> index e1576b81cf9..26f6dd037dc 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -11,6 +11,7 @@ softmmu_ss.add(when: 'CONFIG_TMP105', if_true: files('tmp105.c'))
> softmmu_ss.add(when: 'CONFIG_TMP421', if_true: files('tmp421.c'))
> softmmu_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
> softmmu_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
> +softmmu_ss.add(when: 'CONFIG_LED', if_true: files('led.c'))
>
> # ARM devices
> softmmu_ss.add(when: 'CONFIG_PL310', if_true: files('arm_l2x0.c'))
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 066752aa900..76c9ddb54fe 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -214,6 +214,9 @@ via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size
> 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, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
> +
> # pca9552.c
> pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
> pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, unsigned current_state) "%s GPIO id:%u status: %u -> %u"
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 1/8] hw/misc/led: Add a LED device
2020-09-07 20:03 ` Luc Michel
@ 2020-09-07 20:24 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 20:24 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres, qemu-arm,
Cédric Le Goater, Joel Stanley
On 9/7/20 10:03 PM, Luc Michel wrote:
> Hi Philippe,
>
> On 9/7/20 6:32 PM, Philippe Mathieu-Daudé wrote:
>> Add a LED device which can be connected to a GPIO output.
>> 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.
>>
>> LEDs are limited to a fixed set of colors.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/misc/led.h | 84 +++++++++++++++++++++++++
>> hw/misc/led.c | 142 ++++++++++++++++++++++++++++++++++++++++++
>> MAINTAINERS | 6 ++
>> hw/misc/Kconfig | 3 +
>> hw/misc/meson.build | 1 +
>> hw/misc/trace-events | 3 +
>> 6 files changed, 239 insertions(+)
>> create mode 100644 include/hw/misc/led.h
>> create mode 100644 hw/misc/led.c
...
>> +/**
>> + * led_set_intensity: set the intensity of a LED device
>> + * @s: the LED object
>> + * @intensity: intensity as percentage in range 0 to 100.
> @intensity_percent
>
>> + */
>> +void led_set_intensity(LEDState *s, unsigned intensity_percent);
>> +
>> +/**
>> + * led_get_intensity:
>> + * @s: the LED object
>> + *
>> + * Returns: The LED intensity as percentage in range 0 to 100.
>> + */
>> +unsigned led_get_intensity(LEDState *s);
>> +
>> +/**
>> + * led_set_intensity: set the state of a LED device
> led_set_state
>
>> + * @s: the LED object
>> + * @is_on: boolean indicating whether the LED is emitting
> @is_emitting
>
>> + *
>> + * This utility is meant for LED connected to GPIO.
>> + */
>> +void led_set_state(LEDState *s, bool is_emitting);
>> +
>> +/**
>> + * led_create_simple: Create and realize a LED device
>> + * @parent: the parent object
> @parentobj
>
>> + * @color: color of the LED
>> + * @description: description of the LED (optional)
>> + *
>> + * Create the device state structure, initialize it, and
>> + * drop the reference to it (the device is realized).
>> + */
>> +LEDState *led_create_simple(Object *parentobj,
>> + LEDColor color,
>> + const char *description);
>> +
>> +#endif /* HW_MISC_LED_H */
>> diff --git a/hw/misc/led.c b/hw/misc/led.c
>> new file mode 100644
>> index 00000000000..f2140739b68
>> --- /dev/null
>> +++ b/hw/misc/led.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * 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"
>> +
>> +#define LED_INTENSITY_PERCENT_MAX 100
>> +
>> +static const char *led_color_name[] = {
>> + [LED_COLOR_VIOLET] = "violet",
>> + [LED_COLOR_BLUE] = "blue",
>> + [LED_COLOR_CYAN] = "cyan",
>> + [LED_COLOR_GREEN] = "green",
>> + [LED_COLOR_AMBER] = "amber",
>> + [LED_COLOR_ORANGE] = "orange",
>> + [LED_COLOR_RED] = "red",
>> +};
>> +
>> +static bool led_color_name_is_valid(const char *color_name)
>> +{
>> + for (size_t i = 0; i < ARRAY_SIZE(led_color_name); i++) {
>> + if (led_color_name[i] && strcmp(color_name,
>> led_color_name[i]) == 0) {
>
> Why are you checking led_color_name[i] here?
It could happen in v3 but not now, thanks for catching this :)
I'll address your comment and respin after few days.
>
> Otherwise, seems good to me.
>
> Reviewed-by: Luc Michel <luc.michel@greensocs.com>
Thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 2/8] hw/misc/led: Allow connecting from GPIO output
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 1/8] hw/misc/led: Add a " Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 3/8] hw/misc/led: Emit a trace event when LED intensity has changed Philippe Mathieu-Daudé
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Daniel P. Berrangé, Eduardo Habkost,
Andrew Jeffery, Joaquin de Andres, Philippe Mathieu-Daudé,
qemu-arm, Cédric Le Goater, Paolo Bonzini, Luc Michel,
Joel Stanley
Some devices expose GPIO lines.
Add a GPIO qdev input to our LED device, so we can
connect a GPIO output using qdev_connect_gpio_out().
When used with GPIOs, the intensity can only be either
minium or maximum. This depends of the polarity of the
GPIO (which can be inverted).
Declare the GpioPolarity type to model the polarity.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/misc/led.h | 8 ++++++++
include/hw/qdev-core.h | 8 ++++++++
hw/misc/led.c | 17 ++++++++++++++++-
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 1aaabbebafc..c8dd6da74d5 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -38,10 +38,16 @@ typedef struct LEDState {
/* Public */
uint8_t intensity_percent;
+ qemu_irq irq;
/* Properties */
char *description;
char *color;
+ /*
+ * When used with GPIO, the intensity at reset is related
+ * to the GPIO polarity.
+ */
+ bool inverted_polarity;
} LEDState;
/**
@@ -71,6 +77,7 @@ void led_set_state(LEDState *s, bool is_emitting);
/**
* led_create_simple: Create and realize a LED device
* @parent: the parent object
+ * @gpio_polarity: GPIO polarity
* @color: color of the LED
* @description: description of the LED (optional)
*
@@ -78,6 +85,7 @@ void led_set_state(LEDState *s, bool is_emitting);
* drop the reference to it (the device is realized).
*/
LEDState *led_create_simple(Object *parentobj,
+ GpioPolarity gpio_polarity,
LEDColor color,
const char *description);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ea3f73a282d..846354736a5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -424,6 +424,14 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
void qdev_machine_creation_done(void);
bool qdev_machine_modified(void);
+/**
+ * GpioPolarity: Polarity of a GPIO line
+ */
+typedef enum {
+ GPIO_POLARITY_ACTIVE_LOW,
+ GPIO_POLARITY_ACTIVE_HIGH
+} GpioPolarity;
+
/**
* qdev_get_gpio_in: Get one of a device's anonymous input GPIO lines
* @dev: Device whose GPIO we want
diff --git a/hw/misc/led.c b/hw/misc/led.c
index f2140739b68..1acade1d592 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"
#define LED_INTENSITY_PERCENT_MAX 100
@@ -53,11 +54,19 @@ void led_set_state(LEDState *s, bool is_emitting)
led_set_intensity(s, is_emitting ? LED_INTENSITY_PERCENT_MAX : 0);
}
+static void led_set_state_gpio_handler(void *opaque, int line, int new_state)
+{
+ LEDState *s = LED(opaque);
+
+ assert(line == 0);
+ led_set_state(s, !!new_state != s->inverted_polarity);
+}
+
static void led_reset(DeviceState *dev)
{
LEDState *s = LED(dev);
- led_set_state(s, false);
+ led_set_state(s, s->inverted_polarity);
}
static const VMStateDescription vmstate_led = {
@@ -84,11 +93,14 @@ static void led_realize(DeviceState *dev, Error **errp)
if (s->description == NULL) {
s->description = g_strdup("n/a");
}
+
+ qdev_init_gpio_in(DEVICE(s), led_set_state_gpio_handler, 1);
}
static Property led_properties[] = {
DEFINE_PROP_STRING("color", LEDState, color),
DEFINE_PROP_STRING("description", LEDState, description),
+ DEFINE_PROP_BOOL("polarity-inverted", LEDState, inverted_polarity, false),
DEFINE_PROP_END_OF_LIST(),
};
@@ -119,6 +131,7 @@ static void led_register_types(void)
type_init(led_register_types)
LEDState *led_create_simple(Object *parentobj,
+ GpioPolarity gpio_polarity,
LEDColor color,
const char *description)
{
@@ -126,6 +139,8 @@ LEDState *led_create_simple(Object *parentobj,
DeviceState *dev;
dev = qdev_new(TYPE_LED);
+ qdev_prop_set_bit(dev, "polarity-inverted",
+ gpio_polarity == GPIO_POLARITY_ACTIVE_LOW);
qdev_prop_set_string(dev, "color", led_color_name[color]);
if (!description) {
static unsigned undescribed_led_id;
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 3/8] hw/misc/led: Emit a trace event when LED intensity has changed
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 1/8] hw/misc/led: Add a " Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 2/8] hw/misc/led: Allow connecting from GPIO output Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 4/8] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1 Philippe Mathieu-Daudé
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Richard Henderson, Philippe Mathieu-Daudé, qemu-arm,
Cédric Le Goater, Luc Michel, Joel Stanley
Track the LED intensity, and emit a trace event when it changes.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/led.c | 4 ++++
hw/misc/trace-events | 1 +
2 files changed, 5 insertions(+)
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 1acade1d592..b7e9b1465bb 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -41,6 +41,10 @@ void led_set_intensity(LEDState *s, unsigned intensity_percent)
intensity_percent = LED_INTENSITY_PERCENT_MAX;
}
trace_led_set_intensity(s->description, s->color, intensity_percent);
+ if (intensity_percent != s->intensity_percent) {
+ trace_led_change_intensity(s->description, s->color,
+ s->intensity_percent, intensity_percent);
+ }
s->intensity_percent = intensity_percent;
}
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 76c9ddb54fe..89d15f05f9a 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -216,6 +216,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, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
+led_change_intensity(const char *color, const char *desc, uint8_t old_intensity_percent, uint8_t new_intensity_percent) "LED desc:'%s' color:%s intensity %u%% -> %u%%"
# pca9552.c
pca955x_gpio_status(const char *description, const char *buf) "%s GPIOs 0-15 [%s]"
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 4/8] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-09-07 16:32 ` [PATCH v4 3/8] hw/misc/led: Emit a trace event when LED intensity has changed Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 5/8] hw/misc/mps2-fpgaio: Use the LED device Philippe Mathieu-Daudé
` (3 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Richard Henderson, Philippe Mathieu-Daudé, qemu-arm,
Cédric Le Goater, Luc Michel, Joel Stanley
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 0% -> 100%
1592693373.997632:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
1592693373.998239:led_change_intensity LED desc:'front-id-5' color:green intensity 0% -> 100%
1592693500.291805:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
1592693500.312041:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
1592693500.821254:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
1592693501.331517:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
1592693501.841367:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
1592693502.350839:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
1592693502.861134:led_change_intensity LED desc:'front-power-3' color:green intensity 100% -> 0%
1592693503.371090:led_change_intensity LED desc:'front-power-3' color:green intensity 0% -> 100%
We notice the front-power LED starts to blink at a ~2Hz rate.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/arm/aspeed.c | 20 ++++++++++++++++++++
hw/arm/Kconfig | 1 +
2 files changed, 21 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8bfb1c79ddc..83e322ea983 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"
@@ -521,9 +522,20 @@ static void sonorapass_bmc_i2c_init(AspeedMachineState *bmc)
static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
{
+ static const struct {
+ unsigned gpio_id;
+ LEDColor color;
+ const char *description;
+ bool gpio_polarity;
+ } pca1_leds[] = {
+ {13, LED_COLOR_GREEN, "front-fault-4", GPIO_POLARITY_ACTIVE_LOW},
+ {14, LED_COLOR_GREEN, "front-power-3", GPIO_POLARITY_ACTIVE_LOW},
+ {15, LED_COLOR_GREEN, "front-id-5", GPIO_POLARITY_ACTIVE_LOW},
+ };
AspeedSoCState *soc = &bmc->soc;
uint8_t *eeprom_buf = g_malloc0(8 * 1024);
DeviceState *dev;
+ LEDState *led;
/* Bus 3: TODO bmp280@77 */
/* Bus 3: TODO max31785@52 */
@@ -534,6 +546,14 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
aspeed_i2c_get_bus(&soc->i2c, 3),
&error_fatal);
+ for (size_t i = 0; i < ARRAY_SIZE(pca1_leds); i++) {
+ led = led_create_simple(OBJECT(bmc),
+ pca1_leds[i].gpio_polarity,
+ pca1_leds[i].color,
+ pca1_leds[i].description);
+ qdev_connect_gpio_out(dev, pca1_leds[i].gpio_id,
+ qdev_get_gpio_in(DEVICE(led), 0));
+ }
i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), "tmp423", 0x4c);
i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), "tmp423", 0x4c);
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index bc3a423940b..06ba1c355b1 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -394,6 +394,7 @@ config ASPEED_SOC
select TMP105
select TMP421
select UNIMP
+ select LED
config MPS2
bool
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 5/8] hw/misc/mps2-fpgaio: Use the LED device
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-09-07 16:32 ` [PATCH v4 4/8] hw/arm/aspeed: Add the 3 front LEDs drived by the PCA9552 #1 Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 6/8] hw/misc/mps2-scc: " Philippe Mathieu-Daudé
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Philippe Mathieu-Daudé, qemu-arm, Cédric Le Goater,
Luc Michel, Joel Stanley
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 | 2 ++
hw/misc/mps2-fpgaio.c | 19 ++++++++++++++-----
hw/misc/Kconfig | 1 +
hw/misc/trace-events | 1 -
4 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h
index 69e265cd4b2..901880cc3a7 100644
--- a/include/hw/misc/mps2-fpgaio.h
+++ b/include/hw/misc/mps2-fpgaio.h
@@ -22,6 +22,7 @@
#define MPS2_FPGAIO_H
#include "hw/sysbus.h"
+#include "hw/misc/led.h"
#define TYPE_MPS2_FPGAIO "mps2-fpgaio"
#define MPS2_FPGAIO(obj) OBJECT_CHECK(MPS2FPGAIO, (obj), TYPE_MPS2_FPGAIO)
@@ -32,6 +33,7 @@ typedef struct {
/*< public >*/
MemoryRegion iomem;
+ LEDState *led[2];
uint32_t led0;
uint32_t prescale;
diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
index 2f3fbeef348..86ca78eb235 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(s->led[0], value & 0x01);
+ led_set_state(s->led[1], value & 0x02);
break;
case A_PRESCALE:
resync_counter(s);
@@ -251,6 +249,16 @@ static void mps2_fpgaio_init(Object *obj)
sysbus_init_mmio(sbd, &s->iomem);
}
+static void mps2_fpgaio_realize(DeviceState *dev, Error **errp)
+{
+ MPS2FPGAIO *s = MPS2_FPGAIO(dev);
+
+ s->led[0] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_GREEN, "USERLED0");
+ s->led[1] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_GREEN, "USERLED1");
+}
+
static bool mps2_fpgaio_counters_needed(void *opaque)
{
/* Currently vmstate.c insists all subsections have a 'needed' function */
@@ -299,6 +307,7 @@ static void mps2_fpgaio_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &mps2_fpgaio_vmstate;
+ dc->realize = mps2_fpgaio_realize;
dc->reset = mps2_fpgaio_reset;
device_class_set_props(dc, mps2_fpgaio_properties);
}
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 5c151fa3a83..0cecad45aad 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 89d15f05f9a..43b9e0cf250 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -93,7 +93,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.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 6/8] hw/misc/mps2-scc: Use the LED device
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-09-07 16:32 ` [PATCH v4 5/8] hw/misc/mps2-fpgaio: Use the LED device Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [PATCH v4 7/8] hw/arm/tosa: Replace fprintf() calls by LED devices Philippe Mathieu-Daudé
2020-09-07 16:32 ` [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev Philippe Mathieu-Daudé
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Philippe Mathieu-Daudé, qemu-arm, Cédric Le Goater,
Luc Michel, Joel Stanley
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 | 2 ++
hw/misc/mps2-scc.c | 25 ++++++++++++++-----------
hw/misc/Kconfig | 1 +
hw/misc/trace-events | 1 -
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
index 7045473788b..8542f384227 100644
--- a/include/hw/misc/mps2-scc.h
+++ b/include/hw/misc/mps2-scc.h
@@ -13,6 +13,7 @@
#define MPS2_SCC_H
#include "hw/sysbus.h"
+#include "hw/misc/led.h"
#define TYPE_MPS2_SCC "mps2-scc"
#define MPS2_SCC(obj) OBJECT_CHECK(MPS2SCC, (obj), TYPE_MPS2_SCC)
@@ -25,6 +26,7 @@ typedef struct {
/*< public >*/
MemoryRegion iomem;
+ LEDState *led[8];
uint32_t cfg0;
uint32_t cfg1;
diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index 9d0909e7b35..745505b849d 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(s->led[i], extract32(value, i, 1));
+ }
break;
case A_CFGDATA_OUT:
s->cfgdata_out = value;
@@ -245,10 +239,19 @@ 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);
+
}
static void mps2_scc_realize(DeviceState *dev, Error **errp)
{
+ MPS2SCC *s = MPS2_SCC(dev);
+
+ for (size_t i = 0; i < ARRAY_SIZE(s->led); i++) {
+ char *name = g_strdup_printf("SCC LED%zu", i);
+ s->led[i] = led_create_simple(OBJECT(dev), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_GREEN, name);
+ g_free(name);
+ }
}
static const VMStateDescription mps2_scc_vmstate = {
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 0cecad45aad..7557a3e7b46 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 43b9e0cf250..a620a358feb 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -85,7 +85,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.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 7/8] hw/arm/tosa: Replace fprintf() calls by LED devices
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2020-09-07 16:32 ` [PATCH v4 6/8] hw/misc/mps2-scc: " Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-07 16:32 ` [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev Philippe Mathieu-Daudé
7 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Philippe Mathieu-Daudé, qemu-arm, Cédric Le Goater,
Luc Michel, Joel Stanley
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, 16 insertions(+), 25 deletions(-)
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 90eef1f14dd..f23651fd775 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
@@ -81,26 +82,6 @@ typedef struct TosaMiscGPIOState {
SysBusDevice parent_obj;
} TosaMiscGPIOState;
-static void tosa_gpio_leds(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:
- g_assert_not_reached();
- }
-}
-
static void tosa_reset(void *opaque, int line, int level)
{
if (level) {
@@ -112,7 +93,6 @@ static void tosa_misc_gpio_init(Object *obj)
{
DeviceState *dev = DEVICE(obj);
- qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4);
qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
}
@@ -122,6 +102,7 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
TC6393xbState *tmio)
{
DeviceState *misc_gpio;
+ LEDState *led[4];
misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
@@ -143,14 +124,23 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
NULL);
+ led[0] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_BLUE, "bluetooth");
+ led[1] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_GREEN, "note");
+ led[2] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_AMBER, "charger-error");
+ led[3] = led_create_simple(OBJECT(misc_gpio), GPIO_POLARITY_ACTIVE_HIGH,
+ LED_COLOR_GREEN, "wlan");
+
qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED,
- qdev_get_gpio_in_named(misc_gpio, "leds", 0));
+ qdev_get_gpio_in(DEVICE(led[0]), 0));
qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED,
- qdev_get_gpio_in_named(misc_gpio, "leds", 1));
+ qdev_get_gpio_in(DEVICE(led[1]), 0));
qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED,
- qdev_get_gpio_in_named(misc_gpio, "leds", 2));
+ qdev_get_gpio_in(DEVICE(led[2]), 0));
qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED,
- qdev_get_gpio_in_named(misc_gpio, "leds", 3));
+ qdev_get_gpio_in(DEVICE(led[3]), 0));
qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio));
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 06ba1c355b1..bbcfa098ae2 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.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev
2020-09-07 16:32 [PATCH v4 0/8] hw/misc: Add LED device Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2020-09-07 16:32 ` [PATCH v4 7/8] hw/arm/tosa: Replace fprintf() calls by LED devices Philippe Mathieu-Daudé
@ 2020-09-07 16:32 ` Philippe Mathieu-Daudé
2020-09-08 7:54 ` Markus Armbruster
7 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-07 16:32 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jeffery, Joaquin de Andres,
Philippe Mathieu-Daudé, Markus Armbruster, qemu-arm,
Cédric Le Goater, Luc Michel, Joel Stanley
TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device,
make it a plain QDev.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because having to pass MachineState and call
object_property_add_child() simply makes things more
complex... but it seems to cleaner QOM design.
Cc: Markus Armbruster <armbru@redhat.com>
---
hw/arm/tosa.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index f23651fd775..524d5fcd10b 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -79,7 +79,7 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
typedef struct TosaMiscGPIOState {
- SysBusDevice parent_obj;
+ DeviceState parent_obj;
} TosaMiscGPIOState;
static void tosa_reset(void *opaque, int line, int level)
@@ -96,7 +96,7 @@ static void tosa_misc_gpio_init(Object *obj)
qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
}
-static void tosa_gpio_setup(PXA2xxState *cpu,
+static void tosa_gpio_setup(MachineState *machine, PXA2xxState *cpu,
DeviceState *scp0,
DeviceState *scp1,
TC6393xbState *tmio)
@@ -104,7 +104,10 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
DeviceState *misc_gpio;
LEDState *led[4];
- misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
+ misc_gpio = qdev_new(TYPE_TOSA_MISC_GPIO);
+ object_property_add_child(OBJECT(machine), "pcb-container",
+ OBJECT(misc_gpio));
+ qdev_realize_and_unref(misc_gpio, NULL, &error_fatal);
/* MMC/SD host */
pxa2xx_mmci_handlers(cpu->mmc,
@@ -253,7 +256,7 @@ static void tosa_init(MachineState *machine)
scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
- tosa_gpio_setup(mpu, scp0, scp1, tmio);
+ tosa_gpio_setup(machine, mpu, scp0, scp1, tmio);
tosa_microdrive_attach(mpu);
@@ -307,7 +310,7 @@ static const TypeInfo tosa_ssp_info = {
static const TypeInfo tosa_misc_gpio_info = {
.name = TYPE_TOSA_MISC_GPIO,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_DEVICE,
.instance_size = sizeof(TosaMiscGPIOState),
.instance_init = tosa_misc_gpio_init,
/*
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev
2020-09-07 16:32 ` [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev Philippe Mathieu-Daudé
@ 2020-09-08 7:54 ` Markus Armbruster
2020-09-08 10:04 ` Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2020-09-08 7:54 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Daniel P. Berrangé, Eduardo Habkost,
Andrew Jeffery, Joaquin de Andres, qemu-devel, qemu-arm,
Cédric Le Goater, Paolo Bonzini, Luc Michel, Joel Stanley
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device,
> make it a plain QDev.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because having to pass MachineState and call
> object_property_add_child() simply makes things more
> complex... but it seems to cleaner QOM design.
Well, what devices really *need* to be sysbus devices?
The question is trivial for "real" buses, such as PCI, USB, and so
forth: a device is a FOO device when it plugs into a FOO bus.
Sysbus is quite unlike these "real" buses. It exists because qdev
initially *required* qdevs to plug into a qbus, so we made up a qbus for
the devices that don't plug into any of our "real" buses[1].
I figure all sysbus devices could be coded as bus-less devices today.
So the answer to "what devices really *need* to be sysbus devices?" is
"none".
I think a more useful question is what devices *should* be coded as
sysbus devices vs. bus-less devices.
Sysbus is more than just a dummy qbus. It's a software interface that
provides useful stuff. To use it, the device needs to be a
SysBusDevice. This leads to a partial answer: if the device profits
from stuff we provide only to SysBusDevices, it should be one.
Perhaps the useful stuff could be separated from SysBusDevice. Then
this partial answer evaporates.
There is just one instance of TYPE_SYSTEM_BUS[2]. This leads to another
partial answer: if the device can be part of another device, it should
not be a SysBusDevice.
Sysbus also enables "dynamic" sysbus devices. Shoehorning them into
SysBusDevice may have been a mistake.
> Cc: Markus Armbruster <armbru@redhat.com>
Cc: QOM maintainers for actual QOM expertise :)
[1] sysbus.h describes itself as "Devices attached directly to the main
system bus". I think that's an (unconscious?) attempt to rationalize
away its peculiar role.
[2] Exception: TYPE_MACIO_BUS (used by g3beige and mac99) is a subtype
of TYPE_SYSTEM_BUS.
> ---
> hw/arm/tosa.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index f23651fd775..524d5fcd10b 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -79,7 +79,7 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
> OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
>
> typedef struct TosaMiscGPIOState {
> - SysBusDevice parent_obj;
> + DeviceState parent_obj;
> } TosaMiscGPIOState;
>
> static void tosa_reset(void *opaque, int line, int level)
> @@ -96,7 +96,7 @@ static void tosa_misc_gpio_init(Object *obj)
> qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
> }
>
> -static void tosa_gpio_setup(PXA2xxState *cpu,
> +static void tosa_gpio_setup(MachineState *machine, PXA2xxState *cpu,
> DeviceState *scp0,
> DeviceState *scp1,
> TC6393xbState *tmio)
> @@ -104,7 +104,10 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
> DeviceState *misc_gpio;
> LEDState *led[4];
>
> - misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
> + misc_gpio = qdev_new(TYPE_TOSA_MISC_GPIO);
> + object_property_add_child(OBJECT(machine), "pcb-container",
> + OBJECT(misc_gpio));
> + qdev_realize_and_unref(misc_gpio, NULL, &error_fatal);
>
> /* MMC/SD host */
> pxa2xx_mmci_handlers(cpu->mmc,
> @@ -253,7 +256,7 @@ static void tosa_init(MachineState *machine)
> scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
> scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
>
> - tosa_gpio_setup(mpu, scp0, scp1, tmio);
> + tosa_gpio_setup(machine, mpu, scp0, scp1, tmio);
>
> tosa_microdrive_attach(mpu);
>
> @@ -307,7 +310,7 @@ static const TypeInfo tosa_ssp_info = {
>
> static const TypeInfo tosa_misc_gpio_info = {
> .name = TYPE_TOSA_MISC_GPIO,
> - .parent = TYPE_SYS_BUS_DEVICE,
> + .parent = TYPE_DEVICE,
> .instance_size = sizeof(TosaMiscGPIOState),
> .instance_init = tosa_misc_gpio_init,
> /*
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH v4 8/8] hw/arm/tosa: Make TYPE_TOSA_MISC_GPIO a plain QDev
2020-09-08 7:54 ` Markus Armbruster
@ 2020-09-08 10:04 ` Peter Maydell
0 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2020-09-08 10:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé, Eduardo Habkost, Andrew Jeffery,
Joaquin de Andres, Philippe Mathieu-Daudé, QEMU Developers,
qemu-arm, Cédric Le Goater, Paolo Bonzini, Luc Michel,
Joel Stanley
On Tue, 8 Sep 2020 at 08:54, Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
> > TYPE_TOSA_MISC_GPIO doesn't need to be a SysBus device,
> > make it a plain QDev.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > RFC because having to pass MachineState and call
> > object_property_add_child() simply makes things more
> > complex... but it seems to cleaner QOM design.
>
> Well, what devices really *need* to be sysbus devices?
>
> The question is trivial for "real" buses, such as PCI, USB, and so
> forth: a device is a FOO device when it plugs into a FOO bus.
>
> Sysbus is quite unlike these "real" buses. It exists because qdev
> initially *required* qdevs to plug into a qbus, so we made up a qbus for
> the devices that don't plug into any of our "real" buses[1].
>
> I figure all sysbus devices could be coded as bus-less devices today.
> So the answer to "what devices really *need* to be sysbus devices?" is
> "none".
The major thing sysbus being a bus gives you is reset: devices
on buses get reset automatically, but devices not on buses don't.
So in this particular case I'm not in favour of this change --
right now the TOSA_MISC_GPIO device doesn't happen to need a reset,
but having devices floating around in the system which can't have
a reset method is a beartrap for our future selves. It's bad enough
that we have this issue today with CPU objects: I don't want us to
extend that to anything else if we can avoid it.
> I think a more useful question is what devices *should* be coded as
> sysbus devices vs. bus-less devices.
>
> Sysbus is more than just a dummy qbus. It's a software interface that
> provides useful stuff. To use it, the device needs to be a
> SysBusDevice. This leads to a partial answer: if the device profits
> from stuff we provide only to SysBusDevices, it should be one.
>
> Perhaps the useful stuff could be separated from SysBusDevice. Then
> this partial answer evaporates.
This also is true -- some pretty generic useful stuff like "I can have
MMIO regions" and "I get automatically reset" is implemented in sysbus,
and some (like "I have gpio lines") for DeviceState. I would be happy
to see this cleaned up. For the code we have at the moment I prefer
to treat SysBusDevice as the preferred parent class for devices, ie
don't directly inherit from DeviceState unless you really know what
you're doing.
The reset stuff in particular is desperately in need of a cleanup
but it's a swamp, as usual. Currently we reset along the qbus tree
(which is why non-bus-connected devices don't get their qdev reset
method called, and must fend for themselves via qemu_register_reset()).
These days I feel like "resetting a device should reset all its QOM
children" would be a more natural way to model things (with some sort
of "this device needs to override that default behaviour" for SoCs
with more complicated reset handling) but getting there from here
feels like it would be very painful.
> There is just one instance of TYPE_SYSTEM_BUS[2]. This leads to another
> partial answer: if the device can be part of another device, it should
> not be a SysBusDevice.
>
> Sysbus also enables "dynamic" sysbus devices. Shoehorning them into
> SysBusDevice may have been a mistake.
I was never much of a fan of dynamic sysbus devices at all :-)
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread