* [Qemu-devel] [PATCH v6 0/9] Clock framework API
@ 2019-09-04 12:55 Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
` (9 more replies)
0 siblings, 10 replies; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
This series aims to add a way to model clock distribution in qemu. This allows
to model the clock tree of a platform allowing us to inspect clock
configuration and detect problems such as disabled clock or bad configured
pll.
The added clock api is very similar the the gpio api for devices. We can add
input and output and connect them together.
Very few changes since v5 in the core patches: we were waiting for multi phase
ability to allow proper initialization of the clock tree. So this is almost a
simple rebase on top of the current "Multi-phase reset mechanism" series.
Based-on: <20190821163341.16309-1-damien.hedde@greensocs.com>
Changes since v5:
- drop the "-port" in file names
- new patch 2, extracted from patch 1 (to fix some problem with linux-user builds)
- patch 3, minor modification to better match gpios api and allow non device-related clock
(I've dropped the reviewed-by, see the patch message for the details of what has changed).
- patch 6, Philippe's comments and various improvement
- patches 7/8/9, multi-phase reset addition and scope reduced to uart ref clocks
The patches are organised as follows:
+ Patches 1 to 5 adds the clock support in qemu (1, 4 and 5 are already reviewed and
also a big part of the 3)
+ Patch 6 add some documentation in docs/devel
+ Patches 7 to 9 adds the uart's clocks to the xilinx_zynq platform as an
example for this framework. It updates the zynq's slcr clock controller, the
cadence_uart device, and the zynq toplevel platform.
I've tested this patchset on the xilinx-zynq-a9 machine with the buildroot's
zynq_zc706_defconfig which package the Xilinx's Linux.
Clocks are correctly updated and we ends up with a configured baudrate of 115601
on the console uart (for a theoretical 115200) which is nice. "cadence_uart*" and
"clock*" traces can be enabled to see what's going on in this platform.
Any comments and suggestion are welcomed.
Thanks to the Xilinx QEMU team who sponsored this development.
Damien Hedde (9):
hw/core/clock: introduce clock objects
hw/core/clock-vmstate: define a vmstate entry for clock state
qdev: add clock input&output support to devices.
qdev-monitor: print the device's clock with info qtree
qdev-clock: introduce an init array to ease the device construction
docs/clocks: add device's clock documentation
hw/misc/zynq_slcr: add clock generation for uarts
hw/char/cadence_uart: add clock support
hw/arm/xilinx_zynq: connect uart clocks to slcr
Makefile.objs | 1 +
docs/devel/clock.txt | 246 +++++++++++++++++++++++++++++++++
hw/arm/xilinx_zynq.c | 64 +++++++--
hw/char/cadence_uart.c | 85 ++++++++++--
hw/char/trace-events | 3 +
hw/core/Makefile.objs | 4 +-
hw/core/clock-vmstate.c | 25 ++++
hw/core/clock.c | 144 +++++++++++++++++++
hw/core/qdev-clock.c | 181 ++++++++++++++++++++++++
hw/core/qdev.c | 32 +++++
hw/core/trace-events | 6 +
hw/misc/zynq_slcr.c | 145 ++++++++++++++++++-
include/hw/char/cadence_uart.h | 1 +
include/hw/clock.h | 133 ++++++++++++++++++
include/hw/qdev-clock.h | 134 ++++++++++++++++++
include/hw/qdev-core.h | 14 ++
qdev-monitor.c | 13 ++
tests/Makefile.include | 1 +
18 files changed, 1210 insertions(+), 22 deletions(-)
create mode 100644 docs/devel/clock.txt
create mode 100644 hw/core/clock-vmstate.c
create mode 100644 hw/core/clock.c
create mode 100644 hw/core/qdev-clock.c
create mode 100644 include/hw/clock.h
create mode 100644 include/hw/qdev-clock.h
--
2.22.0
^ permalink raw reply [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-11-25 13:07 ` Philippe Mathieu-Daudé
` (2 more replies)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
` (8 subsequent siblings)
9 siblings, 3 replies; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Introduce clock objects: ClockIn and ClockOut.
These objects may be used to distribute clocks from an object to several
other objects. Each ClockIn object contains the current state of the
clock: the frequency; it allows an object to migrate its input clock state
independently of other objects.
A ClockIn may be connected to a ClockOut so that it receives update,
through a callback, whenever the Clockout is updated using the
ClockOut's set function.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Makefile.objs | 1 +
hw/core/Makefile.objs | 1 +
hw/core/clock.c | 144 ++++++++++++++++++++++++++++++++++++++++++
hw/core/trace-events | 6 ++
include/hw/clock.h | 124 ++++++++++++++++++++++++++++++++++++
5 files changed, 276 insertions(+)
create mode 100644 hw/core/clock.c
create mode 100644 include/hw/clock.h
diff --git a/Makefile.objs b/Makefile.objs
index a723a47e14..4da623c759 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -153,6 +153,7 @@ trace-events-subdirs += hw/audio
trace-events-subdirs += hw/block
trace-events-subdirs += hw/block/dataplane
trace-events-subdirs += hw/char
+trace-events-subdirs += hw/core
trace-events-subdirs += hw/dma
trace-events-subdirs += hw/hppa
trace-events-subdirs += hw/i2c
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 69b408ad1c..c66a5b2c6b 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -7,6 +7,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
# irq.o needed for qdev GPIO handling:
common-obj-y += irq.o
common-obj-y += hotplug.o
+common-obj-y += clock.o
common-obj-$(CONFIG_SOFTMMU) += nmi.o
common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
diff --git a/hw/core/clock.c b/hw/core/clock.c
new file mode 100644
index 0000000000..888f247f2a
--- /dev/null
+++ b/hw/core/clock.c
@@ -0,0 +1,144 @@
+/*
+ * Clock inputs and outputs
+ *
+ * Copyright GreenSocs 2016-2018
+ *
+ * Authors:
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ * Damien Hedde <damien.hedde@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/clock.h"
+#include "trace.h"
+
+#define CLOCK_PATH(_clk) (_clk->canonical_path)
+
+void clock_out_setup_canonical_path(ClockOut *clk)
+{
+ g_free(clk->canonical_path);
+ clk->canonical_path = object_get_canonical_path(OBJECT(clk));
+}
+
+void clock_in_setup_canonical_path(ClockIn *clk)
+{
+ g_free(clk->canonical_path);
+ clk->canonical_path = object_get_canonical_path(OBJECT(clk));
+}
+
+void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
+{
+ assert(clk);
+
+ clk->callback = cb;
+ clk->callback_opaque = opaque;
+}
+
+void clock_init_frequency(ClockIn *clk, uint64_t freq)
+{
+ assert(clk);
+
+ clk->frequency = freq;
+}
+
+void clock_clear_callback(ClockIn *clk)
+{
+ clock_set_callback(clk, NULL, NULL);
+}
+
+void clock_connect(ClockIn *clkin, ClockOut *clkout)
+{
+ assert(clkin && clkin->driver == NULL);
+ assert(clkout);
+
+ trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
+
+ QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
+ clkin->driver = clkout;
+}
+
+static void clock_disconnect(ClockIn *clk)
+{
+ if (clk->driver == NULL) {
+ return;
+ }
+
+ trace_clock_disconnect(CLOCK_PATH(clk));
+
+ clk->driver = NULL;
+ QLIST_REMOVE(clk, sibling);
+}
+
+void clock_set_frequency(ClockOut *clk, uint64_t freq)
+{
+ ClockIn *follower;
+ trace_clock_set_frequency(CLOCK_PATH(clk), freq);
+
+ QLIST_FOREACH(follower, &clk->followers, sibling) {
+ trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
+ if (follower->frequency != freq) {
+ follower->frequency = freq;
+ if (follower->callback) {
+ follower->callback(follower->callback_opaque);
+ }
+ }
+ }
+}
+
+static void clock_out_initfn(Object *obj)
+{
+ ClockOut *clk = CLOCK_OUT(obj);
+
+ QLIST_INIT(&clk->followers);
+}
+
+static void clock_out_finalizefn(Object *obj)
+{
+ ClockOut *clk = CLOCK_OUT(obj);
+ ClockIn *follower, *next;
+
+ /* clear our list of followers */
+ QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
+ clock_disconnect(follower);
+ }
+
+ g_free(clk->canonical_path);
+ clk->canonical_path = NULL;
+}
+
+static void clock_in_finalizefn(Object *obj)
+{
+ ClockIn *clk = CLOCK_IN(obj);
+
+ /* remove us from driver's followers list */
+ clock_disconnect(clk);
+
+ g_free(clk->canonical_path);
+ clk->canonical_path = NULL;
+}
+
+static const TypeInfo clock_out_info = {
+ .name = TYPE_CLOCK_OUT,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(ClockOut),
+ .instance_init = clock_out_initfn,
+ .instance_finalize = clock_out_finalizefn,
+};
+
+static const TypeInfo clock_in_info = {
+ .name = TYPE_CLOCK_IN,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(ClockIn),
+ .instance_finalize = clock_in_finalizefn,
+};
+
+static void clock_register_types(void)
+{
+ type_register_static(&clock_in_info);
+ type_register_static(&clock_out_info);
+}
+
+type_init(clock_register_types)
diff --git a/hw/core/trace-events b/hw/core/trace-events
index ecf966c314..aa940e268b 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -34,3 +34,9 @@ resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
resettable_count_underflow(void *obj) "obj=%p"
+
+# hw/core/clock-port.c
+clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
+clock_disconnect(const char *clk) "'%s'"
+clock_set_frequency(const char *clk, uint64_t freq) "'%s' freq_hz=%" PRIu64
+clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
diff --git a/include/hw/clock.h b/include/hw/clock.h
new file mode 100644
index 0000000000..fd11202ba4
--- /dev/null
+++ b/include/hw/clock.h
@@ -0,0 +1,124 @@
+#ifndef QEMU_HW_CLOCK_H
+#define QEMU_HW_CLOCK_H
+
+#include "qom/object.h"
+#include "qemu/queue.h"
+
+#define TYPE_CLOCK_IN "clock-in"
+#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
+#define TYPE_CLOCK_OUT "clock-out"
+#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
+
+typedef void ClockCallback(void *opaque);
+
+typedef struct ClockOut ClockOut;
+typedef struct ClockIn ClockIn;
+
+struct ClockIn {
+ /*< private >*/
+ Object parent_obj;
+ /*< private >*/
+ uint64_t frequency;
+ char *canonical_path; /* clock path cache */
+ ClockOut *driver; /* clock output controlling this clock */
+ ClockCallback *callback; /* local callback */
+ void *callback_opaque; /* opaque argument for the callback */
+ QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
+};
+
+struct ClockOut {
+ /*< private >*/
+ Object parent_obj;
+ /*< private >*/
+ char *canonical_path; /* clock path cache */
+ QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
+};
+
+/**
+ * clock_out_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_out_setup_canonical_path(ClockOut *clk);
+
+/**
+ * clock_in_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_in_setup_canonical_path(ClockIn *clk);
+
+/**
+ * clock_add_callback:
+ * @clk: the clock to register the callback into
+ * @cb: the callback function
+ * @opaque: the argument to the callback
+ *
+ * Register a callback called on every clock update.
+ */
+void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
+
+/**
+ * clock_clear_callback:
+ * @clk: the clock to delete the callback from
+ *
+ * Unregister the callback registered with clock_set_callback.
+ */
+void clock_clear_callback(ClockIn *clk);
+
+/**
+ * clock_init_frequency:
+ * @clk: the clock to initialize.
+ * @freq: the clock's frequency in Hz or 0 if unclocked.
+ *
+ * Initialize the local cached frequency value of @clk to @freq.
+ * Note: this function must only be called during device inititialization
+ * or migration.
+ */
+void clock_init_frequency(ClockIn *clk, uint64_t freq);
+
+/**
+ * clock_connect:
+ * @clkin: the drived clock.
+ * @clkout: the driving clock.
+ *
+ * Setup @clkout to drive @clkin: Any @clkout update will be propagated
+ * to @clkin.
+ */
+void clock_connect(ClockIn *clkin, ClockOut *clkout);
+
+/**
+ * clock_set_frequency:
+ * @clk: the clock to update.
+ * @freq: the new clock's frequency in Hz or 0 if unclocked.
+ *
+ * Update the @clk to the new @freq.
+ * This change will be propagated through registered clock inputs.
+ */
+void clock_set_frequency(ClockOut *clk, uint64_t freq);
+
+/**
+ * clock_get_frequency:
+ * @clk: the clk to fetch the clock
+ *
+ * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
+ */
+static inline uint64_t clock_get_frequency(const ClockIn *clk)
+{
+ return clk ? clk->frequency : 0;
+}
+
+/**
+ * clock_is_enabled:
+ * @clk: a clock state
+ *
+ * @return: true if the clock is running. If @clk is NULL return false.
+ */
+static inline bool clock_is_enabled(const ClockIn *clk)
+{
+ return clock_get_frequency(clk) != 0;
+}
+
+#endif /* QEMU_HW_CLOCK_H */
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-11-25 13:05 ` Philippe Mathieu-Daudé
2019-12-02 13:44 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
` (7 subsequent siblings)
9 siblings, 2 replies; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
This was in the previous reviewed commit. But it can't be in the
clock.c file in order to allow linux-user builds.
---
hw/core/Makefile.objs | 1 +
hw/core/clock-vmstate.c | 25 +++++++++++++++++++++++++
include/hw/clock.h | 9 +++++++++
3 files changed, 35 insertions(+)
create mode 100644 hw/core/clock-vmstate.c
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index c66a5b2c6b..8fcebf2e67 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
common-obj-y += resettable.o
common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
+common-obj-$(CONFIG_SOFTMMU) += clock-vmstate.o
# irq.o needed for qdev GPIO handling:
common-obj-y += irq.o
common-obj-y += hotplug.o
diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c
new file mode 100644
index 0000000000..c781369c15
--- /dev/null
+++ b/hw/core/clock-vmstate.c
@@ -0,0 +1,25 @@
+/*
+ * Clock migration structure
+ *
+ * Copyright GreenSocs 2019
+ *
+ * Authors:
+ * Damien Hedde <damien.hedde@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "migration/vmstate.h"
+#include "hw/clock.h"
+
+const VMStateDescription vmstate_clockin = {
+ .name = "clock",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(frequency, ClockIn),
+ VMSTATE_END_OF_LIST()
+ }
+};
diff --git a/include/hw/clock.h b/include/hw/clock.h
index fd11202ba4..e7efb6ad17 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -34,6 +34,15 @@ struct ClockOut {
QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
};
+/*
+ * vmstate description entry to be added in device vmsd.
+ */
+extern const VMStateDescription vmstate_clockin;
+#define VMSTATE_CLOCKIN(_field, _state) \
+ VMSTATE_CLOCKIN_V(_field, _state, 0)
+#define VMSTATE_CLOCKIN_V(_field, _state, _version) \
+ VMSTATE_STRUCT_POINTER_V(_field, _state, _version, vmstate_clockin, ClockIn)
+
/**
* clock_out_setup_canonical_path:
* @clk: clock
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-11-25 13:30 ` Philippe Mathieu-Daudé
2019-12-02 14:34 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
` (6 subsequent siblings)
9 siblings, 2 replies; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Add functions to easily add input or output clocks to a device.
A clock objects is added as a child of the device.
The api is very similar the gpio's one.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
I've removed the reviewed-by/tested-by of Philippe because I did a small
modification.
qdev_connect_clock() which allowed to connect an input to an output is
now split in 2:
+ qdev_get_clock_in() which gets a given input from a device
+ qdev_connect_clock_out() which connect a given output to a clock
(previously fetched by qdev_get_clock_in())
This part is located in (qdev-clock.[c|h]).
It better matches gpios api and also add the possibility to connect a
device's input clock to a random output clock (used in patch 9).
Also add missing qdev-clock in the test-qdev-global-props so that tests
pass.
---
hw/core/Makefile.objs | 2 +-
hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++
hw/core/qdev.c | 32 +++++++++
include/hw/qdev-clock.h | 67 +++++++++++++++++
include/hw/qdev-core.h | 14 ++++
tests/Makefile.include | 1 +
6 files changed, 270 insertions(+), 1 deletion(-)
create mode 100644 hw/core/qdev-clock.c
create mode 100644 include/hw/qdev-clock.h
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 8fcebf2e67..4523d3b5c7 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,5 +1,5 @@
# core qdev-related obj files, also used by *-user:
-common-obj-y += qdev.o qdev-properties.o
+common-obj-y += qdev.o qdev-properties.o qdev-clock.o
common-obj-y += bus.o reset.o
common-obj-y += resettable.o
common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
new file mode 100644
index 0000000000..bebdd8fa15
--- /dev/null
+++ b/hw/core/qdev-clock.c
@@ -0,0 +1,155 @@
+/*
+ * Device's clock
+ *
+ * Copyright GreenSocs 2016-2018
+ *
+ * Authors:
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ * Damien Hedde <damien.hedde@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-core.h"
+#include "qapi/error.h"
+
+static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
+ bool forward)
+{
+ NamedClockList *ncl;
+
+ /*
+ * The clock path will be computed by the device's realize function call.
+ * This is required to ensure the clock's canonical path is right and log
+ * messages are meaningfull.
+ */
+ assert(name);
+ assert(!dev->realized);
+
+ /* The ncl structure will be freed in device's finalize function call */
+ ncl = g_malloc0(sizeof(*ncl));
+ ncl->name = g_strdup(name);
+ ncl->forward = forward;
+
+ QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
+ return ncl;
+}
+
+ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+ Object *clk;
+
+ ncl = qdev_init_clocklist(dev, name, false);
+
+ clk = object_new(TYPE_CLOCK_OUT);
+
+ /* will fail if name already exists */
+ object_property_add_child(OBJECT(dev), name, clk, &error_abort);
+ object_unref(clk); /* remove the initial ref made by object_new */
+
+ ncl->out = CLOCK_OUT(clk);
+ return ncl->out;
+}
+
+ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
+ ClockCallback *callback, void *opaque)
+{
+ NamedClockList *ncl;
+ Object *clk;
+
+ ncl = qdev_init_clocklist(dev, name, false);
+
+ clk = object_new(TYPE_CLOCK_IN);
+ /*
+ * the ref initialized by object_new will be cleared during dev finalize.
+ * It allows us to safely remove the callback.
+ */
+
+ /* will fail if name already exists */
+ object_property_add_child(OBJECT(dev), name, clk, &error_abort);
+
+ ncl->in = CLOCK_IN(clk);
+ if (callback) {
+ clock_set_callback(ncl->in, callback, opaque);
+ }
+ return ncl->in;
+}
+
+static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+
+ QLIST_FOREACH(ncl, &dev->clocks, node) {
+ if (strcmp(name, ncl->name) == 0) {
+ return ncl;
+ }
+ }
+
+ return NULL;
+}
+
+void qdev_pass_clock(DeviceState *dev, const char *name,
+ DeviceState *container, const char *cont_name)
+{
+ NamedClockList *original_ncl, *ncl;
+ Object **clk;
+
+ assert(container && cont_name);
+
+ original_ncl = qdev_get_clocklist(container, cont_name);
+ assert(original_ncl); /* clock must exist in origin */
+
+ ncl = qdev_init_clocklist(dev, name, true);
+
+ if (ncl->out) {
+ clk = (Object **)&ncl->out;
+ } else {
+ clk = (Object **)&ncl->in;
+ }
+
+ /* will fail if name already exists */
+ object_property_add_link(OBJECT(dev), name, object_get_typename(*clk),
+ clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort);
+}
+
+ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+
+ assert(dev && name);
+
+ ncl = qdev_get_clocklist(dev, name);
+ return ncl ? ncl->in : NULL;
+}
+
+static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+
+ assert(dev && name);
+
+ ncl = qdev_get_clocklist(dev, name);
+ return ncl ? ncl->out : NULL;
+}
+
+void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
+ Error **errp)
+{
+ ClockOut *clkout = qdev_get_clock_out(dev, name);
+
+ if (!clk) {
+ error_setg(errp, "NULL input clock");
+ return;
+ }
+
+ if (!clkout) {
+ error_setg(errp, "no output clock '%s' in device", name);
+ return;
+ }
+
+ clock_connect(clk, clkout);
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9095f2b9c1..eae6cd3e09 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -37,6 +37,7 @@
#include "hw/qdev-properties.h"
#include "hw/boards.h"
#include "hw/sysbus.h"
+#include "hw/clock.h"
#include "migration/vmstate.h"
bool qdev_hotplug = false;
@@ -821,6 +822,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
DeviceClass *dc = DEVICE_GET_CLASS(dev);
HotplugHandler *hotplug_ctrl;
BusState *bus;
+ NamedClockList *clk;
Error *local_err = NULL;
bool unattached_parent = false;
static int unattached_count;
@@ -869,6 +871,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
*/
g_free(dev->canonical_path);
dev->canonical_path = object_get_canonical_path(OBJECT(dev));
+ QLIST_FOREACH(clk, &dev->clocks, node) {
+ if (clk->forward) {
+ continue;
+ } else if (clk->in != NULL) {
+ clock_in_setup_canonical_path(clk->in);
+ } else {
+ clock_out_setup_canonical_path(clk->out);
+ }
+ }
if (qdev_get_vmsd(dev)) {
if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
@@ -999,6 +1010,7 @@ static void device_initfn(Object *obj)
(Object **)&dev->parent_bus, NULL, 0,
&error_abort);
QLIST_INIT(&dev->gpios);
+ QLIST_INIT(&dev->clocks);
}
static void device_post_init(Object *obj)
@@ -1015,6 +1027,7 @@ static void device_post_init(Object *obj)
static void device_finalize(Object *obj)
{
NamedGPIOList *ngl, *next;
+ NamedClockList *clk, *clk_next;
DeviceState *dev = DEVICE(obj);
@@ -1028,6 +1041,25 @@ static void device_finalize(Object *obj)
*/
}
+ QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) {
+ QLIST_REMOVE(clk, node);
+ if (!clk->forward && clk->in) {
+ /*
+ * if this clock is not forwarded, clk->in is a child of dev.
+ * At this point the child property and associated reference is
+ * already deleted but we kept a ref on clk->in to ensure it lives
+ * up to this point and we can safely remove the callback.
+ * It avoids having a lost callback to a deleted device if the
+ * clk->in is still referenced somewhere else (eg: by a clock
+ * output).
+ */
+ clock_clear_callback(clk->in);
+ object_unref(OBJECT(clk->in));
+ }
+ g_free(clk->name);
+ g_free(clk);
+ }
+
/* Only send event if the device had been completely realized */
if (dev->pending_deleted_event) {
g_assert(dev->canonical_path);
diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
new file mode 100644
index 0000000000..c4ea912fdc
--- /dev/null
+++ b/include/hw/qdev-clock.h
@@ -0,0 +1,67 @@
+#ifndef QDEV_CLOCK_H
+#define QDEV_CLOCK_H
+
+#include "hw/clock.h"
+
+/**
+ * qdev_init_clock_in:
+ * @dev: the device in which to add a clock
+ * @name: the name of the clock (can't be NULL).
+ * @callback: optional callback to be called on update or NULL.
+ * @opaque: argument for the callback
+ * @returns: a pointer to the newly added clock
+ *
+ * Add a input clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ * The callback will be called with @dev as opaque parameter.
+ */
+ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
+ ClockCallback *callback, void *opaque);
+
+/**
+ * qdev_init_clock_out:
+ * @dev: the device to add a clock to
+ * @name: the name of the clock (can't be NULL).
+ * @callback: optional callback to be called on update or NULL.
+ * @returns: a pointer to the newly added clock
+ *
+ * Add a output clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ */
+ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
+
+/**
+ * qdev_get_clock_in:
+ * @dev: the device which has the clock
+ * @name: the name of the clock (can't be NULL).
+ * @returns: a pointer to the clock
+ *
+ * Get the clock @name from @dev or NULL if does not exists.
+ */
+ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name);
+
+/**
+ * qdev_connect_clock_out:
+ * @dev: the device which has the clock
+ * @name: the name of the clock (can't be NULL).
+ * @errp: error report
+ *
+ * Connect @clk to the output clock @name of @dev.
+ * Reports an error if clk is NULL or @name does not exists in @dev.
+ */
+void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
+ Error **errp);
+
+/**
+ * qdev_pass_clock:
+ * @dev: the device to forward the clock to
+ * @name: the name of the clock to be added (can't be NULL)
+ * @container: the device which already has the clock
+ * @cont_name: the name of the clock in the container device
+ *
+ * Add a clock @name to @dev which forward to the clock @cont_name in @container
+ */
+void qdev_pass_clock(DeviceState *dev, const char *name,
+ DeviceState *container, const char *cont_name);
+
+#endif /* QDEV_CLOCK_H */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index eb11f0f801..60a65f6142 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -131,6 +131,19 @@ struct NamedGPIOList {
QLIST_ENTRY(NamedGPIOList) node;
};
+typedef struct NamedClockList NamedClockList;
+
+typedef struct ClockIn ClockIn;
+typedef struct ClockOut ClockOut;
+
+struct NamedClockList {
+ char *name;
+ bool forward;
+ ClockIn *in;
+ ClockOut *out;
+ QLIST_ENTRY(NamedClockList) node;
+};
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -152,6 +165,7 @@ struct DeviceState {
int hotplugged;
BusState *parent_bus;
QLIST_HEAD(, NamedGPIOList) gpios;
+ QLIST_HEAD(, NamedClockList) clocks;
QLIST_HEAD(, BusState) child_bus;
int num_child_bus;
int instance_id_alias;
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f0b4628cc6..5c54beb29e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -566,6 +566,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
hw/core/irq.o \
hw/core/fw-path-provider.o \
hw/core/reset.o \
+ hw/core/clock.o \
$(test-qapi-obj-y)
tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (2 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 14:35 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
` (5 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
This prints the clocks attached to a DeviceState when using "info qtree" monitor
command. For every clock, it displays the direction, the name and if the
clock is forwarded. For input clock, it displays also the frequency.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
qdev-monitor.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index a0003bf2a9..d5b8be956b 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -19,6 +19,7 @@
#include "qemu/osdep.h"
#include "hw/sysbus.h"
+#include "hw/clock.h"
#include "monitor/monitor.h"
#include "monitor/qdev.h"
#include "sysemu/arch_init.h"
@@ -689,6 +690,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ObjectClass *class;
BusState *child;
NamedGPIOList *ngl;
+ NamedClockList *clk;
qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
dev->id ? dev->id : "");
@@ -703,6 +705,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ngl->num_out);
}
}
+ QLIST_FOREACH(clk, &dev->clocks, node) {
+ if (clk->out) {
+ qdev_printf("clock-out%s \"%s\"\n",
+ clk->forward ? " (fw)" : "",
+ clk->name);
+ } else {
+ qdev_printf("clock-in%s \"%s\" freq_hz=%" PRIu64"\n",
+ clk->forward ? " (fw)" : "",
+ clk->name, clock_get_frequency(clk->in));
+ }
+ }
class = object_get_class(OBJECT(dev));
do {
qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (3 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 15:13 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation Damien Hedde
` (4 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Introduce a function and macro helpers to setup several clocks
in a device from a static array description.
An element of the array describes the clock (name and direction) as
well as the related callback and an optional offset to store the
created object pointer in the device state structure.
The array must be terminated by a special element QDEV_CLOCK_END.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/core/qdev-clock.c | 26 ++++++++++++++++
include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 93 insertions(+)
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index bebdd8fa15..32ad45c061 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
clock_connect(clk, clkout);
}
+
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
+{
+ const struct ClockPortInitElem *elem;
+
+ assert(dev);
+ assert(clocks);
+
+ for (elem = &clocks[0]; elem->name != NULL; elem++) {
+ /* offset cannot be inside the DeviceState part */
+ assert(elem->offset == 0 || elem->offset > sizeof(DeviceState));
+ if (elem->is_output) {
+ ClockOut *clk;
+ clk = qdev_init_clock_out(dev, elem->name);
+ if (elem->offset) {
+ *(ClockOut **)(((void *) dev) + elem->offset) = clk;
+ }
+ } else {
+ ClockIn *clk;
+ clk = qdev_init_clock_in(dev, elem->name, elem->callback, dev);
+ if (elem->offset) {
+ *(ClockIn **)(((void *) dev) + elem->offset) = clk;
+ }
+ }
+ }
+}
diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index c4ea912fdc..b6edb9421b 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -64,4 +64,71 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
void qdev_pass_clock(DeviceState *dev, const char *name,
DeviceState *container, const char *cont_name);
+/**
+ * ClockInitElem:
+ * @name: name of the clock (can't be NULL)
+ * @is_output: indicates whether the clock is input or output
+ * @callback: for inputs, optional callback to be called on clock's update
+ * with device as opaque
+ * @offset: optional offset to store the ClockIn or ClockOut pointer in device
+ * state structure (0 means unused)
+ */
+struct ClockPortInitElem {
+ const char *name;
+ bool is_output;
+ ClockCallback *callback;
+ size_t offset;
+};
+
+#define clock_offset_value(_type, _devstate, _field) \
+ (offsetof(_devstate, _field) + \
+ type_check(_type *, typeof_field(_devstate, _field)))
+
+#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
+ .name = (stringify(_field)), \
+ .is_output = _is_output, \
+ .callback = _callback, \
+ .offset = clock_offset_value(_type, _devstate, _field), \
+}
+
+/**
+ * QDEV_CLOCK_(IN|OUT):
+ * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
+ * a pointer to that same type.
+ * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
+ * @_callback: (for input only) callback (or NULL) to be called with the device
+ * state as argument
+ *
+ * The name of the clock will be derived from @_field
+ */
+#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
+ QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
+
+#define QDEV_CLOCK_OUT(_devstate, _field) \
+ QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
+
+/**
+ * QDEV_CLOCK_IN_NOFIELD:
+ * @_name: name of the clock
+ * @_callback: callback (or NULL) to be called with the device state as argument
+ */
+#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
+ .name = _name, \
+ .is_output = false, \
+ .callback = _callback, \
+ .offset = 0, \
+}
+
+#define QDEV_CLOCK_END { .name = NULL }
+
+typedef struct ClockPortInitElem ClockPortInitArray[];
+
+/**
+ * qdev_init_clocks:
+ * @dev: the device to add clocks
+ * @clocks: a QDEV_CLOCK_END-terminated array which contains the
+ * clocks information.
+ */
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
+
#endif /* QDEV_CLOCK_H */
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (4 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 15:17 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
` (3 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Add the documentation about the clock inputs and outputs in devices.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
docs/devel/clock.txt | 246 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 246 insertions(+)
create mode 100644 docs/devel/clock.txt
diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
new file mode 100644
index 0000000000..18f79922d0
--- /dev/null
+++ b/docs/devel/clock.txt
@@ -0,0 +1,246 @@
+
+What are clocks
+===============
+
+Clocks are objects representing input and output clocks of objects. They are
+QOM objects developed for the purpose of modeling the distribution of clocks in
+QEMU.
+
+This allows us to model the clock distribution of a platform and detect
+configuration errors in the clock tree such as badly configured PLL, clock
+source selection or disabled clock.
+
+The objects are ClockIn for the input and ClockOut for the output. The QOM
+names are respectively CLOCK_IN and CLOCK_OUT.
+
+CLOCK_IN and CLOCK_OUT are typically child of some device and created in a
+similar way as gpios. ClockIn and ClockOut of different devices can be
+connected together. It is possible to create a clock which is not related
+to a device; for example to model a clock source of a machine.
+
+Here is an example of devices with clocks:
+
+ +-------------------+ +--------------------+
+ | Device B | | Device C |
+ +--------------------+ | +-----+ | | +-----+ |
+ | Device A | | |Clock|>>----->>|Clock| |
+ | +-----+ | | +-----+ |Out 3| | | |In 5| |
+ | |Clock|>>--+-->>|Clock| +-----+ | | +-----+ |
+ | |Out 1| | | | |In 2 | +-----+ | | +-----+ |
+ | +-----+ | | | +-----+ |Clock|>>----->>|Clock| |
+ +--------------------+ | | |Out 4| | | |In 6| |
+ | | +-----+ | | +-----+ |
+ | +-------------------+ +--------------------+
+ |
+ | +--------------------+
+ | | Device D |
+ | | +-----+ |
+ +-->>|Clock| |
+ | |In 7| |
+ | +-----+ |
+ +--------------------+
+
+Clocks are defined in include/hw/clock.h header and device related functions
+are defined in hw/qdev-clock.h header.
+
+The clock state
+===============
+
+The state of a clock is its frequency; it is stored as an integer representing
+it in Hertz. The special value of 0 is used to represent the clock being
+inactive or gated. The clocks do not model the signal itself (pin toggling)
+or other properties such as the duty cycle.
+
+Only the CLOCK_IN object keeps the value of a clock; this allows a device to
+fetch the current input frequency at any time. When an output is updated, the
+value is immediately propagated to all connected CLOCK_IN.
+
+Adding clocks to a device
+=========================
+
+Adding clocks to a device must be done during the init method of the Device
+instance.
+
+To add an input clock to a device, the function qdev_init_clock_in must be used.
+It takes the name, a callback, and an opaque parameter for the clock.
+Output is more simple, only the name is required. Typically:
+qdev_init_clock_in(DEVICE(dev), "clk_in", clk_in_callback, dev);
+qdev_init_clock_out(DEVICE(dev), "clk_out");
+
+Both functions return the created ClockIn/ClockOut pointer, which should be saved
+in the device's state structure for further use.
+
+These objects will be automatically deleted by the QOM reference mechanism.
+
+Note that it is possible to create a static array describing clock inputs and
+outputs. The function qdev_init_clocks() must be called with the array as
+parameter to initialize the clocks: it has the same behaviour as calling the
+qdev_init_clock_in/out() for each clock in the array. To ease the array
+construction, some macros are defined in include/hw/qdev-clock.h.
+As an example, the following creates 2 clocks to a device: 1 input and 1
+output.
+
+/* device structure containing pointer to the clock objects */
+typedef struct MyDeviceState {
+ DeviceState parent_obj;
+ ClockIn *clk_in;
+ ClockOut *clk_out;
+} MyDeviceState;
+
+/*
+ * callback for the input clock (see "Callback on input clock change" section
+ * below for more information).
+ */
+static void clk_in_callback(void *opaque);
+
+/*
+ * static array describing clocks:
+ * + a clock input named "clk_in", whose pointer is stored in clk_in
+ * field of a MyDeviceState structure with callback clk_in_callback.
+ * + a clock output named "clk_out" whose pointer is stored in clk_out
+ * field of a MyDeviceState structure.
+ */
+static const ClockPortInitArray mydev_clocks = {
+ QDEV_CLOCK_IN(MyDeviceState, clk_in, clk_in_callback),
+ QDEV_CLOCK_OUT(MyDeviceState, clk_out),
+ QDEV_CLOCK_END
+};
+
+/* device initialization function */
+static void mydev_init(Object *obj)
+{
+ /* cast to MyDeviceState */
+ MyDeviceState *mydev = MYDEVICE(obj);
+ /* create and fill the pointer fields in the MyDeviceState */
+ qdev_init_clocks(mydev, mydev_clocks);
+ [...]
+}
+
+Connecting two clocks together
+==============================
+
+Let's say we have 2 devices A and B. A has an output clock named "clk_out" and
+B has an input clock named "clk_in".
+
+The clocks are connected together using the function qdev_connect_clock:
+qdev_connect_clock(B, "clk_in", A, "clk_out", &error_abort);
+The device which has the input must be the first argument.
+
+It is possible to connect several input clocks to the same output. Every
+input callback will be called when the output changes.
+
+It is not possible to disconnect a clock or to change the clock connection
+after it is done.
+
+Unconnected input clocks
+========================
+
+A newly created input clock has a stored frequency value of 0. It means the
+clock will be considered as disabled until one sets a new frequency to the
+output clock it is connected to. If the clock remains unconnected it will
+always keep its initial value of 0.
+If this is not the wanted behaviour, clock_init_frequency should be called
+on the ClockIn object during device instance init.
+For example:
+clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
+clock_init_frequency(clk, 100 * 1000 * 1000); // default value is 100Mhz
+
+Using clock input frequency
+===========================
+
+A device can get the current frequency of an input using the
+clock_get_frequency(). It returns the last set frequency (or the init value).
+
+It is also possible to register a callback on input clock frequency changes.
+Here is an example:
+void clock_callback(void *opaque) {
+ MyDeviceState *s = (MyDeviceState *) opaque;
+ /*
+ * opaque may not be the device state pointer, but most probably it is.
+ * (It depends on what is given to the qdev_init_clock_in function)
+ */
+
+ /* do something with the new frequency */
+ fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
+ clock_get_frequency(dev->my_clk_input));
+}
+
+Changing a clock output
+=======================
+
+A device can change its outputs using the clock_set_frequency function. It
+will trigger updates on every connected inputs.
+
+For example, let's say that we have an output clock "clkout" and we have a
+pointer to it in the device state because we did the following in init phase:
+dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
+
+Then at any time (apart from the cases listed below), it is possible to
+change the clock value by doing:
+clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Ghz */
+This operation must be done while holding the qemu io lock.
+
+One can change clocks only when it is allowed to have side effects on other
+objects. In consequence, it is forbidden:
++ during migration,
++ and in the init phase of reset.
+
+Forwarding clocks
+=================
+
+Sometimes, one needs to forward, or inherit, a clock from another device.
+Typically, when doing device composition, a device might expose a sub-device's
+clock without interfering with it.
+The function qdev_pass_clock() can be used to achieve this behaviour. Note, that
+it is possible to expose the clock under a different name. This works for both
+inputs or outputs.
+
+For example, if device B is a child of device A, device_a_instance_init may
+do something like this:
+void device_a_instance_init(Object *obj)
+{
+ AState *A = DEVICE_A(obj);
+ BState *B;
+ /* create B object as child of A */
+ [...]
+ /* forward B's clock to A */
+ qdev_pass_clock(A, "b_clk", B, "clk");
+ /*
+ * Now A has a clock "b_clk" which forwards to
+ * the "clk" of its child B.
+ */
+}
+
+This function does not return any clock object. It is not possible to add
+a callback on a forwarded input clock: in the above example, only B can use
+the clock.
+
+Migration
+=========
+
+Only the ClockIn object has a state. ClockOut is not concerned by migration.
+
+In case the frequency of in input clock is needed for a device's migration,
+this state must be migrated. The VMSTATE_CLOCKIN macro defines an entry to
+be added in a vmstate description.
+
+For example, if a device has a clock input and the device state looks like:
+MyDeviceState {
+ DeviceState parent_obj;
+ ClockIn *clk;
+};
+
+Then, to add the clock frequency to the device's migrated state, the vmstate
+description is:
+VMStateDescription my_device_vmstate = {
+ .name = "my_device",
+ .fields = (VMStateField[]) {
+ VMSTATE_CLOCKIN(clk, MyDeviceState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+When adding a input clock support to an existing device, you must care about
+migration compatibility. To this end, you can use the clock_init_frequency in
+a pre_load function to setup a default value in case the source vm does not
+migrate the frequency.
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (5 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 15:20 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support Damien Hedde
` (2 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Switch the slcr to multi-phase reset and add some clocks:
+ the main input clock (ps_clk)
+ the reference clock outputs for each uart (uart0 & 1)
The clock frequencies are computed using the internal pll & uart configuration
registers and the ps_clk frequency.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
hw/misc/zynq_slcr.c | 145 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 142 insertions(+), 3 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index b9a38272d9..fa2ac608eb 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -22,6 +22,7 @@
#include "qemu/log.h"
#include "qemu/module.h"
#include "hw/registerfields.h"
+#include "hw/qdev-clock.h"
#ifndef ZYNQ_SLCR_ERR_DEBUG
#define ZYNQ_SLCR_ERR_DEBUG 0
@@ -45,6 +46,12 @@ REG32(LOCKSTA, 0x00c)
REG32(ARM_PLL_CTRL, 0x100)
REG32(DDR_PLL_CTRL, 0x104)
REG32(IO_PLL_CTRL, 0x108)
+/* fields for [ARM|DDR|IO]_PLL_CTRL registers */
+ FIELD(xxx_PLL_CTRL, PLL_RESET, 0, 1)
+ FIELD(xxx_PLL_CTRL, PLL_PWRDWN, 1, 1)
+ FIELD(xxx_PLL_CTRL, PLL_BYPASS_QUAL, 3, 1)
+ FIELD(xxx_PLL_CTRL, PLL_BYPASS_FORCE, 4, 1)
+ FIELD(xxx_PLL_CTRL, PLL_FPDIV, 12, 7)
REG32(PLL_STATUS, 0x10c)
REG32(ARM_PLL_CFG, 0x110)
REG32(DDR_PLL_CFG, 0x114)
@@ -64,6 +71,10 @@ REG32(SMC_CLK_CTRL, 0x148)
REG32(LQSPI_CLK_CTRL, 0x14c)
REG32(SDIO_CLK_CTRL, 0x150)
REG32(UART_CLK_CTRL, 0x154)
+ FIELD(UART_CLK_CTRL, CLKACT0, 0, 1)
+ FIELD(UART_CLK_CTRL, CLKACT1, 1, 1)
+ FIELD(UART_CLK_CTRL, SRCSEL, 4, 2)
+ FIELD(UART_CLK_CTRL, DIVISOR, 8, 6)
REG32(SPI_CLK_CTRL, 0x158)
REG32(CAN_CLK_CTRL, 0x15c)
REG32(CAN_MIOCLK_CTRL, 0x160)
@@ -179,11 +190,106 @@ typedef struct ZynqSLCRState {
MemoryRegion iomem;
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+ ClockIn *ps_clk;
+ ClockOut *uart0_ref_clk;
+ ClockOut *uart1_ref_clk;
} ZynqSLCRState;
-static void zynq_slcr_reset(DeviceState *d)
+/*
+ * return the output frequency of ARM/DDR/IO pll
+ * using input frequency and PLL_CTRL register
+ */
+static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
+{
+ uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
+ R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
+
+ /* first, check if pll is bypassed */
+ if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
+ return input;
+ }
+
+ /* is pll disabled ? */
+ if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
+ R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
+ return 0;
+ }
+
+ return input * mult;
+}
+
+/*
+ * return the output frequency of a clock given:
+ * + the frequencies in an array corresponding to mux's indexes
+ * + the register xxx_CLK_CTRL value
+ * + enable bit index in ctrl register
+ *
+ * This function make the assumption that ctrl_reg value is organized as follow:
+ * + bits[13:8] clock divisor
+ * + bits[5:4] clock mux selector (index in array)
+ * + bits[index] clock enable
+ */
+static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
+ uint32_t ctrl_reg,
+ unsigned index)
+{
+ uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
+ uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
+
+ /* first, check if clock is enabled */
+ if (((ctrl_reg >> index) & 1u) == 0) {
+ return 0;
+ }
+
+ /*
+ * according to the Zynq technical ref. manual UG585 v1.12.2 in
+ * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
+ * is [1;63].
+ * So divide the source while avoiding division-by-zero.
+ */
+ return mux[srcsel] / (divisor ? divisor : 1u);
+}
+
+/*
+ * macro helper around zynq_slcr_compute_clock to avoid repeating
+ * the register name.
+ */
+#define ZYNQ_COMPUTE_CLOCK(_state, _plls, _reg, _enable_field) \
+ zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
+ R_ ## _reg ## _ ## _enable_field ## _SHIFT)
+
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+{
+ uint64_t ps_clk = clock_get_frequency(s->ps_clk);
+
+ /* consider all output clocks are disabled while in reset */
+ if (device_is_resetting(DEVICE(s))) {
+ ps_clk = 0;
+ }
+
+ uint64_t io_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_IO_PLL_CTRL]);
+ uint64_t arm_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_ARM_PLL_CTRL]);
+ uint64_t ddr_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_DDR_PLL_CTRL]);
+
+ uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
+
+ /* compute uartX reference clocks */
+ clock_set_frequency(s->uart0_ref_clk,
+ ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
+ clock_set_frequency(s->uart1_ref_clk,
+ ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
+}
+
+static void zynq_slcr_ps_clk_callback(void *opaque)
+{
+ ZynqSLCRState *s = (ZynqSLCRState *) opaque;
+ zynq_slcr_compute_clocks(s);
+}
+
+static void zynq_slcr_reset_init(Object *obj, ResetType type)
{
- ZynqSLCRState *s = ZYNQ_SLCR(d);
+ ZynqSLCRState *s = ZYNQ_SLCR(obj);
int i;
DB_PRINT("RESET\n");
@@ -277,6 +383,21 @@ static void zynq_slcr_reset(DeviceState *d)
s->regs[R_DDRIOB + 12] = 0x00000021;
}
+static void zynq_slcr_reset_hold(Object *obj)
+{
+ ZynqSLCRState *s = ZYNQ_SLCR(obj);
+
+ /* will disable all output clocks */
+ zynq_slcr_compute_clocks(s);
+}
+
+static void zynq_slcr_reset_exit(Object *obj)
+{
+ ZynqSLCRState *s = ZYNQ_SLCR(obj);
+
+ /* will compute output clocks according to ps_clk and registers */
+ zynq_slcr_compute_clocks(s);
+}
static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
{
@@ -409,6 +530,12 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
}
break;
+ case R_IO_PLL_CTRL:
+ case R_ARM_PLL_CTRL:
+ case R_DDR_PLL_CTRL:
+ case R_UART_CLK_CTRL:
+ zynq_slcr_compute_clocks(s);
+ break;
}
}
@@ -418,6 +545,13 @@ static const MemoryRegionOps slcr_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
+static const ClockPortInitArray zynq_slcr_clocks = {
+ QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
+ QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
+ QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
+ QDEV_CLOCK_END
+};
+
static void zynq_slcr_init(Object *obj)
{
ZynqSLCRState *s = ZYNQ_SLCR(obj);
@@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
ZYNQ_SLCR_MMIO_SIZE);
sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+
+ qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
}
static const VMStateDescription vmstate_zynq_slcr = {
@@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
static void zynq_slcr_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
dc->vmsd = &vmstate_zynq_slcr;
- dc->reset = zynq_slcr_reset;
+ rc->phases.init = zynq_slcr_reset_init;
+ rc->phases.hold = zynq_slcr_reset_hold;
+ rc->phases.exit = zynq_slcr_reset_exit;
}
static const TypeInfo zynq_slcr_info = {
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (6 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 15:24 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
2019-12-02 16:15 ` [PATCH v6 0/9] Clock framework API Peter Maydell
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Switch the cadence uart to multi-phase reset and add the
reference clock input.
The input clock frequency is added to the migration structure.
The reference clock controls the baudrate generation. If it disabled,
any input characters and events are ignored.
If this clock remains unconnected, the uart behaves as before
(it default to a 50MHz ref clock).
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
hw/char/cadence_uart.c | 85 ++++++++++++++++++++++++++++++----
hw/char/trace-events | 3 ++
include/hw/char/cadence_uart.h | 1 +
3 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 0e315b2376..bae43a9679 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -31,6 +31,8 @@
#include "qemu/module.h"
#include "hw/char/cadence_uart.h"
#include "hw/irq.h"
+#include "hw/qdev-clock.h"
+#include "trace.h"
#ifdef CADENCE_UART_ERR_DEBUG
#define DB_PRINT(...) do { \
@@ -97,7 +99,7 @@
#define LOCAL_LOOPBACK (0x2 << UART_MR_CHMODE_SH)
#define REMOTE_LOOPBACK (0x3 << UART_MR_CHMODE_SH)
-#define UART_INPUT_CLK 50000000
+#define UART_DEFAULT_REF_CLK (50 * 1000 * 1000)
#define R_CR (0x00/4)
#define R_MR (0x04/4)
@@ -171,12 +173,15 @@ static void uart_send_breaks(CadenceUARTState *s)
static void uart_parameters_setup(CadenceUARTState *s)
{
QEMUSerialSetParams ssp;
- unsigned int baud_rate, packet_size;
+ unsigned int baud_rate, packet_size, input_clk;
+ input_clk = clock_get_frequency(s->refclk);
- baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
- UART_INPUT_CLK / 8 : UART_INPUT_CLK;
+ baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
+ baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
+ trace_cadence_uart_baudrate(baud_rate);
+
+ ssp.speed = baud_rate;
- ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
packet_size = 1;
switch (s->r[R_MR] & UART_MR_PAR) {
@@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
}
packet_size += ssp.data_bits + ssp.stop_bits;
+ if (ssp.speed == 0) {
+ /*
+ * Avoid division-by-zero below.
+ * TODO: find something better
+ */
+ ssp.speed = 1;
+ }
s->char_tx_time = (NANOSECONDS_PER_SECOND / ssp.speed) * packet_size;
qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
}
@@ -340,6 +352,11 @@ static void uart_receive(void *opaque, const uint8_t *buf, int size)
CadenceUARTState *s = opaque;
uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
+ /* ignore characters when unclocked or in reset */
+ if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+ return;
+ }
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
uart_write_rx_fifo(opaque, buf, size);
}
@@ -353,6 +370,11 @@ static void uart_event(void *opaque, int event)
CadenceUARTState *s = opaque;
uint8_t buf = '\0';
+ /* ignore characters when unclocked or in reset */
+ if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+ return;
+ }
+
if (event == CHR_EVENT_BREAK) {
uart_write_rx_fifo(opaque, &buf, 1);
}
@@ -462,9 +484,9 @@ static const MemoryRegionOps uart_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void cadence_uart_reset(DeviceState *dev)
+static void cadence_uart_reset_init(Object *obj, ResetType type)
{
- CadenceUARTState *s = CADENCE_UART(dev);
+ CadenceUARTState *s = CADENCE_UART(obj);
s->r[R_CR] = 0x00000128;
s->r[R_IMR] = 0;
@@ -473,6 +495,11 @@ static void cadence_uart_reset(DeviceState *dev)
s->r[R_BRGR] = 0x0000028B;
s->r[R_BDIV] = 0x0000000F;
s->r[R_TTRIG] = 0x00000020;
+}
+
+static void cadence_uart_reset_hold(Object *obj)
+{
+ CadenceUARTState *s = CADENCE_UART(obj);
uart_rx_reset(s);
uart_tx_reset(s);
@@ -491,6 +518,14 @@ static void cadence_uart_realize(DeviceState *dev, Error **errp)
uart_event, NULL, s, NULL, true);
}
+static void cadence_uart_refclk_update(void *opaque)
+{
+ CadenceUARTState *s = opaque;
+
+ /* recompute uart's speed on clock change */
+ uart_parameters_setup(s);
+}
+
static void cadence_uart_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
@@ -500,9 +535,23 @@ static void cadence_uart_init(Object *obj)
sysbus_init_mmio(sbd, &s->iomem);
sysbus_init_irq(sbd, &s->irq);
+ s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
+ cadence_uart_refclk_update, s);
+ /* initialize the frequency in case the clock remains unconnected */
+ clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
+
s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
}
+static int cadence_uart_pre_load(void *opaque)
+{
+ CadenceUARTState *s = opaque;
+
+ /* the frequency will be overriden if the subsection is present */
+ clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
+ return 0;
+}
+
static int cadence_uart_post_load(void *opaque, int version_id)
{
CadenceUARTState *s = opaque;
@@ -519,10 +568,21 @@ static int cadence_uart_post_load(void *opaque, int version_id)
return 0;
}
+static const VMStateDescription vmstate_cadence_uart_refclk = {
+ .name = "cadence_uart_refclk",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_CLOCKIN(refclk, CadenceUARTState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_cadence_uart = {
.name = "cadence_uart",
.version_id = 2,
.minimum_version_id = 2,
+ .pre_load = cadence_uart_pre_load,
.post_load = cadence_uart_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
@@ -535,7 +595,10 @@ static const VMStateDescription vmstate_cadence_uart = {
VMSTATE_UINT32(rx_wpos, CadenceUARTState),
VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
VMSTATE_END_OF_LIST()
- }
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_cadence_uart_refclk,
+ },
};
static Property cadence_uart_properties[] = {
@@ -546,12 +609,14 @@ static Property cadence_uart_properties[] = {
static void cadence_uart_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
dc->realize = cadence_uart_realize;
dc->vmsd = &vmstate_cadence_uart;
- dc->reset = cadence_uart_reset;
dc->props = cadence_uart_properties;
- }
+ rc->phases.init = cadence_uart_reset_init;
+ rc->phases.hold = cadence_uart_reset_hold;
+}
static const TypeInfo cadence_uart_info = {
.name = TYPE_CADENCE_UART,
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 2ce7f2f998..502a7d0507 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -77,3 +77,6 @@ cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
# nrf51_uart.c
nrf51_uart_read(uint64_t addr, uint64_t r, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
+
+# hw/char/cadence_uart.c
+cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
index 47cec956c4..a09afe4ed5 100644
--- a/include/hw/char/cadence_uart.h
+++ b/include/hw/char/cadence_uart.h
@@ -49,6 +49,7 @@ typedef struct {
CharBackend chr;
qemu_irq irq;
QEMUTimer *fifo_trigger_handle;
+ ClockIn *refclk;
} CadenceUARTState;
static inline DeviceState *cadence_uart_create(hwaddr addr,
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (7 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support Damien Hedde
@ 2019-09-04 12:55 ` Damien Hedde
2019-12-02 15:34 ` Peter Maydell
2019-12-02 16:15 ` [PATCH v6 0/9] Clock framework API Peter Maydell
9 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-09-04 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
Add the connection between the slcr's output clocks and the uarts inputs.
Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
(the default frequency). This clock is used to feed the slcr's input
clock.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
hw/arm/xilinx_zynq.c | 64 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 56 insertions(+), 8 deletions(-)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 89da34808b..4d5d214ea9 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -33,6 +33,15 @@
#include "hw/char/cadence_uart.h"
#include "hw/net/cadence_gem.h"
#include "hw/cpu/a9mpcore.h"
+#include "hw/qdev-clock.h"
+#include "sysemu/reset.h"
+
+#define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
+#define ZYNQ_MACHINE(obj) \
+ OBJECT_CHECK(ZynqMachineState, (obj), TYPE_ZYNQ_MACHINE)
+
+/* board base frequency: 33.333333 MHz */
+#define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
#define NUM_SPI_FLASHES 4
#define NUM_QSPI_FLASHES 2
@@ -73,6 +82,11 @@ static const int dma_irqs[8] = {
0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
0xe5801000 + (addr)
+typedef struct ZynqMachineState {
+ MachineState parent;
+ ClockOut *ps_clk;
+} ZynqMachineState;
+
static void zynq_write_board_setup(ARMCPU *cpu,
const struct arm_boot_info *info)
{
@@ -157,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
static void zynq_init(MachineState *machine)
{
+ ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
ram_addr_t ram_size = machine->ram_size;
const char *kernel_filename = machine->kernel_filename;
const char *kernel_cmdline = machine->kernel_cmdline;
@@ -165,7 +180,7 @@ static void zynq_init(MachineState *machine)
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
- DeviceState *dev;
+ DeviceState *dev, *slcr;
SysBusDevice *busdev;
qemu_irq pic[64];
int n;
@@ -210,9 +225,17 @@ static void zynq_init(MachineState *machine)
1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
0);
- dev = qdev_create(NULL, "xilinx,zynq_slcr");
- qdev_init_nofail(dev);
- sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
+ /* Create slcr, keep a pointer to connect clocks */
+ slcr = qdev_create(NULL, "xilinx,zynq_slcr");
+ qdev_init_nofail(slcr);
+ sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
+
+ /* Create the main clock source, and feed slcr with it */
+ zynq_machine->ps_clk = CLOCK_OUT(object_new(TYPE_CLOCK_OUT));
+ object_property_add_child(OBJECT(zynq_machine), "ps_clk",
+ OBJECT(zynq_machine->ps_clk), &error_abort);
+ object_unref(OBJECT(zynq_machine->ps_clk));
+ clock_connect(qdev_get_clock_in(slcr, "ps_clk"), zynq_machine->ps_clk);
dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -233,8 +256,12 @@ static void zynq_init(MachineState *machine)
sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
- cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
- cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+ dev = cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
+ qdev_connect_clock_out(slcr, "uart0_ref_clk",
+ qdev_get_clock_in(dev, "refclk"), &error_abort);
+ dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+ qdev_connect_clock_out(slcr, "uart1_ref_clk",
+ qdev_get_clock_in(dev, "refclk"), &error_abort);
sysbus_create_varargs("cadence_ttc", 0xF8001000,
pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
@@ -315,14 +342,35 @@ static void zynq_init(MachineState *machine)
arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
}
-static void zynq_machine_init(MachineClass *mc)
+static void zynq_reset(MachineState *machine)
+{
+ ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
+ qemu_devices_reset();
+ clock_set_frequency(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
+}
+
+static void zynq_machine_class_init(ObjectClass *oc, void *data)
{
+ MachineClass *mc = MACHINE_CLASS(oc);
mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
mc->init = zynq_init;
+ mc->reset = zynq_reset;
mc->max_cpus = 1;
mc->no_sdcard = 1;
mc->ignore_memory_transaction_failures = true;
mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
}
-DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
+static const TypeInfo zynq_machine_type = {
+ .name = TYPE_ZYNQ_MACHINE,
+ .parent = TYPE_MACHINE,
+ .class_init = zynq_machine_class_init,
+ .instance_size = sizeof(ZynqMachineState),
+};
+
+static void zynq_machine_register_types(void)
+{
+ type_register_static(&zynq_machine_type);
+}
+
+type_init(zynq_machine_register_types)
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
@ 2019-11-25 13:05 ` Philippe Mathieu-Daudé
2019-12-02 13:44 ` Peter Maydell
1 sibling, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-25 13:05 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
On 9/4/19 2:55 PM, Damien Hedde wrote:
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> This was in the previous reviewed commit. But it can't be in the
> clock.c file in order to allow linux-user builds.
> ---
> hw/core/Makefile.objs | 1 +
> hw/core/clock-vmstate.c | 25 +++++++++++++++++++++++++
> include/hw/clock.h | 9 +++++++++
> 3 files changed, 35 insertions(+)
> create mode 100644 hw/core/clock-vmstate.c
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index c66a5b2c6b..8fcebf2e67 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
> common-obj-y += resettable.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> +common-obj-$(CONFIG_SOFTMMU) += clock-vmstate.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c
> new file mode 100644
> index 0000000000..c781369c15
> --- /dev/null
> +++ b/hw/core/clock-vmstate.c
> @@ -0,0 +1,25 @@
> +/*
> + * Clock migration structure
> + *
> + * Copyright GreenSocs 2019
> + *
> + * Authors:
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "migration/vmstate.h"
> +#include "hw/clock.h"
> +
> +const VMStateDescription vmstate_clockin = {
> + .name = "clock",
> + .version_id = 0,
> + .minimum_version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(frequency, ClockIn),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index fd11202ba4..e7efb6ad17 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -34,6 +34,15 @@ struct ClockOut {
> QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
> };
>
> +/*
> + * vmstate description entry to be added in device vmsd.
> + */
> +extern const VMStateDescription vmstate_clockin;
> +#define VMSTATE_CLOCKIN(_field, _state) \
> + VMSTATE_CLOCKIN_V(_field, _state, 0)
> +#define VMSTATE_CLOCKIN_V(_field, _state, _version) \
> + VMSTATE_STRUCT_POINTER_V(_field, _state, _version, vmstate_clockin, ClockIn)
> +
> /**
> * clock_out_setup_canonical_path:
> * @clk: clock
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
@ 2019-11-25 13:07 ` Philippe Mathieu-Daudé
2019-11-25 13:37 ` Philippe Mathieu-Daudé
2019-12-02 13:42 ` Peter Maydell
2 siblings, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-25 13:07 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
On 9/4/19 2:55 PM, Damien Hedde wrote:
> Introduce clock objects: ClockIn and ClockOut.
>
> These objects may be used to distribute clocks from an object to several
> other objects. Each ClockIn object contains the current state of the
> clock: the frequency; it allows an object to migrate its input clock state
> independently of other objects.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
> through a callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Makefile.objs | 1 +
> hw/core/Makefile.objs | 1 +
> hw/core/clock.c | 144 ++++++++++++++++++++++++++++++++++++++++++
> hw/core/trace-events | 6 ++
> include/hw/clock.h | 124 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 276 insertions(+)
> create mode 100644 hw/core/clock.c
> create mode 100644 include/hw/clock.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a723a47e14..4da623c759 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/audio
> trace-events-subdirs += hw/block
> trace-events-subdirs += hw/block/dataplane
> trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
> trace-events-subdirs += hw/dma
> trace-events-subdirs += hw/hppa
> trace-events-subdirs += hw/i2c
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 69b408ad1c..c66a5b2c6b 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> +common-obj-y += clock.o
> common-obj-$(CONFIG_SOFTMMU) += nmi.o
> common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> new file mode 100644
> index 0000000000..888f247f2a
> --- /dev/null
> +++ b/hw/core/clock.c
> @@ -0,0 +1,144 @@
> +/*
> + * Clock inputs and outputs
> + *
> + * Copyright GreenSocs 2016-2018
2019 now?
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
@ 2019-11-25 13:30 ` Philippe Mathieu-Daudé
2019-12-03 15:35 ` Damien Hedde
2019-12-02 14:34 ` Peter Maydell
1 sibling, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-25 13:30 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
Nitpick: remove trailing dot in patch subject
On 9/4/19 2:55 PM, Damien Hedde wrote:
> Add functions to easily add input or output clocks to a device.
> A clock objects is added as a child of the device.
<newline>?
> The api is very similar the gpio's one.
Maybe "This API is very similar to the QDEV GPIO API."
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>
> ---
> I've removed the reviewed-by/tested-by of Philippe because I did a small
> modification.
>
> qdev_connect_clock() which allowed to connect an input to an output is
> now split in 2:
> + qdev_get_clock_in() which gets a given input from a device
> + qdev_connect_clock_out() which connect a given output to a clock
> (previously fetched by qdev_get_clock_in())
> This part is located in (qdev-clock.[c|h]).
> It better matches gpios api and also add the possibility to connect a
> device's input clock to a random output clock (used in patch 9).
>
> Also add missing qdev-clock in the test-qdev-global-props so that tests
> pass.
> ---
> hw/core/Makefile.objs | 2 +-
> hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++
> hw/core/qdev.c | 32 +++++++++
> include/hw/qdev-clock.h | 67 +++++++++++++++++
> include/hw/qdev-core.h | 14 ++++
> tests/Makefile.include | 1 +
Please setup the scripts/git.orderfile to ease reviews.
> 6 files changed, 270 insertions(+), 1 deletion(-)
> create mode 100644 hw/core/qdev-clock.c
> create mode 100644 include/hw/qdev-clock.h
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 8fcebf2e67..4523d3b5c7 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,5 +1,5 @@
> # core qdev-related obj files, also used by *-user:
> -common-obj-y += qdev.o qdev-properties.o
> +common-obj-y += qdev.o qdev-properties.o qdev-clock.o
> common-obj-y += bus.o reset.o
> common-obj-y += resettable.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> new file mode 100644
> index 0000000000..bebdd8fa15
> --- /dev/null
> +++ b/hw/core/qdev-clock.c
> @@ -0,0 +1,155 @@
> +/*
> + * Device's clock
> + *
> + * Copyright GreenSocs 2016-2018
2019
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-core.h"
> +#include "qapi/error.h"
> +
> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
> + bool forward)
Indentation off.
> +{
> + NamedClockList *ncl;
> +
> + /*
> + * The clock path will be computed by the device's realize function call.
> + * This is required to ensure the clock's canonical path is right and log
> + * messages are meaningfull.
> + */
> + assert(name);
> + assert(!dev->realized);
> +
> + /* The ncl structure will be freed in device's finalize function call */
> + ncl = g_malloc0(sizeof(*ncl));
Similar but easier to review:
ncl = g_new0(NamedClockList, 1)
> + ncl->name = g_strdup(name);
> + ncl->forward = forward;
> +
> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
> + return ncl;
> +}
> +
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_OUT);
> +
> + /* will fail if name already exists */
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> + object_unref(clk); /* remove the initial ref made by object_new */
> +
> + ncl->out = CLOCK_OUT(clk);
> + return ncl->out;
> +}
> +
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque)
Indentation off.
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_IN);
> + /*
> + * the ref initialized by object_new will be cleared during dev finalize.
> + * It allows us to safely remove the callback.
> + */
> +
> + /* will fail if name already exists */
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> +
> + ncl->in = CLOCK_IN(clk);
> + if (callback) {
> + clock_set_callback(ncl->in, callback, opaque);
> + }
> + return ncl->in;
> +}
> +
> +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + QLIST_FOREACH(ncl, &dev->clocks, node) {
> + if (strcmp(name, ncl->name) == 0) {
> + return ncl;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> + DeviceState *container, const char *cont_name)
> +{
> + NamedClockList *original_ncl, *ncl;
> + Object **clk;
Is it really a Object** or a Object*?
> +
> + assert(container && cont_name);
> +
> + original_ncl = qdev_get_clocklist(container, cont_name);
> + assert(original_ncl); /* clock must exist in origin */
> +
> + ncl = qdev_init_clocklist(dev, name, true);
> +
> + if (ncl->out) {
> + clk = (Object **)&ncl->out;
> + } else {
> + clk = (Object **)&ncl->in;
> + }
> +
> + /* will fail if name already exists */
> + object_property_add_link(OBJECT(dev), name, object_get_typename(*clk),
> + clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort);
> +}
> +
> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + assert(dev && name);
> +
> + ncl = qdev_get_clocklist(dev, name);
> + return ncl ? ncl->in : NULL;
> +}
> +
> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + assert(dev && name);
> +
> + ncl = qdev_get_clocklist(dev, name);
> + return ncl ? ncl->out : NULL;
> +}
> +
> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
> + Error **errp)
> +{
> + ClockOut *clkout = qdev_get_clock_out(dev, name);
> +
> + if (!clk) {
> + error_setg(errp, "NULL input clock");
> + return;
> + }
> +
> + if (!clkout) {
> + error_setg(errp, "no output clock '%s' in device", name);
> + return;
> + }
> +
> + clock_connect(clk, clkout);
> +}
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 9095f2b9c1..eae6cd3e09 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,6 +37,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/boards.h"
> #include "hw/sysbus.h"
> +#include "hw/clock.h"
> #include "migration/vmstate.h"
>
> bool qdev_hotplug = false;
> @@ -821,6 +822,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> DeviceClass *dc = DEVICE_GET_CLASS(dev);
> HotplugHandler *hotplug_ctrl;
> BusState *bus;
> + NamedClockList *clk;
> Error *local_err = NULL;
> bool unattached_parent = false;
> static int unattached_count;
> @@ -869,6 +871,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> */
> g_free(dev->canonical_path);
> dev->canonical_path = object_get_canonical_path(OBJECT(dev));
> + QLIST_FOREACH(clk, &dev->clocks, node) {
> + if (clk->forward) {
> + continue;
> + } else if (clk->in != NULL) {
> + clock_in_setup_canonical_path(clk->in);
> + } else {
> + clock_out_setup_canonical_path(clk->out);
> + }
> + }
>
> if (qdev_get_vmsd(dev)) {
> if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
> @@ -999,6 +1010,7 @@ static void device_initfn(Object *obj)
> (Object **)&dev->parent_bus, NULL, 0,
> &error_abort);
> QLIST_INIT(&dev->gpios);
> + QLIST_INIT(&dev->clocks);
> }
>
> static void device_post_init(Object *obj)
> @@ -1015,6 +1027,7 @@ static void device_post_init(Object *obj)
> static void device_finalize(Object *obj)
> {
> NamedGPIOList *ngl, *next;
> + NamedClockList *clk, *clk_next;
>
> DeviceState *dev = DEVICE(obj);
>
> @@ -1028,6 +1041,25 @@ static void device_finalize(Object *obj)
> */
> }
>
> + QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) {
> + QLIST_REMOVE(clk, node);
> + if (!clk->forward && clk->in) {
> + /*
> + * if this clock is not forwarded, clk->in is a child of dev.
> + * At this point the child property and associated reference is
> + * already deleted but we kept a ref on clk->in to ensure it lives
> + * up to this point and we can safely remove the callback.
> + * It avoids having a lost callback to a deleted device if the
> + * clk->in is still referenced somewhere else (eg: by a clock
> + * output).
> + */
> + clock_clear_callback(clk->in);
> + object_unref(OBJECT(clk->in));
> + }
> + g_free(clk->name);
> + g_free(clk);
> + }
> +
> /* Only send event if the device had been completely realized */
> if (dev->pending_deleted_event) {
> g_assert(dev->canonical_path);
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> new file mode 100644
> index 0000000000..c4ea912fdc
> --- /dev/null
> +++ b/include/hw/qdev-clock.h
> @@ -0,0 +1,67 @@
> +#ifndef QDEV_CLOCK_H
> +#define QDEV_CLOCK_H
> +
> +#include "hw/clock.h"
> +
> +/**
> + * qdev_init_clock_in:
> + * @dev: the device in which to add a clock
> + * @name: the name of the clock (can't be NULL).
I'd drop the trailing dot in property descriptions.
> + * @callback: optional callback to be called on update or NULL.
> + * @opaque: argument for the callback
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a input clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + * The callback will be called with @dev as opaque parameter.
> + */
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque);
> +
> +/**
> + * qdev_init_clock_out:
> + * @dev: the device to add a clock to
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a output clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + */
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_get_clock_in:
> + * @dev: the device which has the clock
> + * @name: the name of the clock (can't be NULL).
> + * @returns: a pointer to the clock
> + *
> + * Get the clock @name from @dev or NULL if does not exists.
> + */
> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_connect_clock_out:
> + * @dev: the device which has the clock
> + * @name: the name of the clock (can't be NULL).
> + * @errp: error report
> + *
> + * Connect @clk to the output clock @name of @dev.
> + * Reports an error if clk is NULL or @name does not exists in @dev.
> + */
> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
> + Error **errp);
> +
> +/**
> + * qdev_pass_clock:
> + * @dev: the device to forward the clock to
> + * @name: the name of the clock to be added (can't be NULL)
> + * @container: the device which already has the clock
> + * @cont_name: the name of the clock in the container device
> + *
> + * Add a clock @name to @dev which forward to the clock @cont_name in @container
> + */
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> + DeviceState *container, const char *cont_name);
> +
> +#endif /* QDEV_CLOCK_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index eb11f0f801..60a65f6142 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -131,6 +131,19 @@ struct NamedGPIOList {
> QLIST_ENTRY(NamedGPIOList) node;
> };
>
> +typedef struct NamedClockList NamedClockList;
> +
> +typedef struct ClockIn ClockIn;
> +typedef struct ClockOut ClockOut;
> +
> +struct NamedClockList {
> + char *name;
> + bool forward;
> + ClockIn *in;
> + ClockOut *out;
> + QLIST_ENTRY(NamedClockList) node;
> +};
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -152,6 +165,7 @@ struct DeviceState {
> int hotplugged;
> BusState *parent_bus;
> QLIST_HEAD(, NamedGPIOList) gpios;
> + QLIST_HEAD(, NamedClockList) clocks;
> QLIST_HEAD(, BusState) child_bus;
> int num_child_bus;
> int instance_id_alias;
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index f0b4628cc6..5c54beb29e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -566,6 +566,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
> hw/core/irq.o \
> hw/core/fw-path-provider.o \
> hw/core/reset.o \
> + hw/core/clock.o \
> $(test-qapi-obj-y)
> tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
> migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
>
Except the cosmetic comments:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
(Note, this series needs a rebase now).
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
2019-11-25 13:07 ` Philippe Mathieu-Daudé
@ 2019-11-25 13:37 ` Philippe Mathieu-Daudé
2019-12-03 15:14 ` Damien Hedde
2019-12-02 13:42 ` Peter Maydell
2 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-25 13:37 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
On 9/4/19 2:55 PM, Damien Hedde wrote:
> Introduce clock objects: ClockIn and ClockOut.
>
> These objects may be used to distribute clocks from an object to several
> other objects. Each ClockIn object contains the current state of the
> clock: the frequency; it allows an object to migrate its input clock state
> independently of other objects.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
> through a callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Makefile.objs | 1 +
> hw/core/Makefile.objs | 1 +
> hw/core/clock.c | 144 ++++++++++++++++++++++++++++++++++++++++++
> hw/core/trace-events | 6 ++
> include/hw/clock.h | 124 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 276 insertions(+)
> create mode 100644 hw/core/clock.c
> create mode 100644 include/hw/clock.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a723a47e14..4da623c759 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/audio
> trace-events-subdirs += hw/block
> trace-events-subdirs += hw/block/dataplane
> trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
> trace-events-subdirs += hw/dma
> trace-events-subdirs += hw/hppa
> trace-events-subdirs += hw/i2c
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 69b408ad1c..c66a5b2c6b 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> +common-obj-y += clock.o
> common-obj-$(CONFIG_SOFTMMU) += nmi.o
> common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> new file mode 100644
> index 0000000000..888f247f2a
> --- /dev/null
> +++ b/hw/core/clock.c
> @@ -0,0 +1,144 @@
> +/*
> + * Clock inputs and outputs
> + *
> + * Copyright GreenSocs 2016-2018
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/clock.h"
> +#include "trace.h"
> +
> +#define CLOCK_PATH(_clk) (_clk->canonical_path)
> +
> +void clock_out_setup_canonical_path(ClockOut *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_in_setup_canonical_path(ClockIn *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
> +{
> + assert(clk);
> +
> + clk->callback = cb;
> + clk->callback_opaque = opaque;
> +}
> +
> +void clock_init_frequency(ClockIn *clk, uint64_t freq)
> +{
> + assert(clk);
> +
> + clk->frequency = freq;
> +}
> +
> +void clock_clear_callback(ClockIn *clk)
> +{
> + clock_set_callback(clk, NULL, NULL);
> +}
> +
> +void clock_connect(ClockIn *clkin, ClockOut *clkout)
> +{
> + assert(clkin && clkin->driver == NULL);
> + assert(clkout);
> +
> + trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
> +
> + QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
> + clkin->driver = clkout;
> +}
> +
> +static void clock_disconnect(ClockIn *clk)
> +{
> + if (clk->driver == NULL) {
> + return;
> + }
> +
> + trace_clock_disconnect(CLOCK_PATH(clk));
> +
> + clk->driver = NULL;
> + QLIST_REMOVE(clk, sibling);
> +}
> +
> +void clock_set_frequency(ClockOut *clk, uint64_t freq)
> +{
> + ClockIn *follower;
> + trace_clock_set_frequency(CLOCK_PATH(clk), freq);
> +
> + QLIST_FOREACH(follower, &clk->followers, sibling) {
> + trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
> + if (follower->frequency != freq) {
> + follower->frequency = freq;
> + if (follower->callback) {
> + follower->callback(follower->callback_opaque);
> + }
> + }
> + }
> +}
> +
> +static void clock_out_initfn(Object *obj)
> +{
> + ClockOut *clk = CLOCK_OUT(obj);
> +
> + QLIST_INIT(&clk->followers);
> +}
> +
> +static void clock_out_finalizefn(Object *obj)
> +{
> + ClockOut *clk = CLOCK_OUT(obj);
> + ClockIn *follower, *next;
> +
> + /* clear our list of followers */
> + QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
> + clock_disconnect(follower);
> + }
> +
> + g_free(clk->canonical_path);
> + clk->canonical_path = NULL;
> +}
> +
> +static void clock_in_finalizefn(Object *obj)
> +{
> + ClockIn *clk = CLOCK_IN(obj);
> +
> + /* remove us from driver's followers list */
> + clock_disconnect(clk);
> +
> + g_free(clk->canonical_path);
> + clk->canonical_path = NULL;
> +}
> +
> +static const TypeInfo clock_out_info = {
> + .name = TYPE_CLOCK_OUT,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(ClockOut),
> + .instance_init = clock_out_initfn,
> + .instance_finalize = clock_out_finalizefn,
> +};
> +
> +static const TypeInfo clock_in_info = {
> + .name = TYPE_CLOCK_IN,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(ClockIn),
> + .instance_finalize = clock_in_finalizefn,
> +};
> +
> +static void clock_register_types(void)
> +{
> + type_register_static(&clock_in_info);
> + type_register_static(&clock_out_info);
> +}
> +
> +type_init(clock_register_types)
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index ecf966c314..aa940e268b 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -34,3 +34,9 @@ resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
> resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
> resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
> resettable_count_underflow(void *obj) "obj=%p"
> +
> +# hw/core/clock-port.c
"# clock.c"
> +clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
> +clock_disconnect(const char *clk) "'%s'"
> +clock_set_frequency(const char *clk, uint64_t freq) "'%s' freq_hz=%" PRIu64
> +clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> new file mode 100644
> index 0000000000..fd11202ba4
> --- /dev/null
> +++ b/include/hw/clock.h
> @@ -0,0 +1,124 @@
> +#ifndef QEMU_HW_CLOCK_H
> +#define QEMU_HW_CLOCK_H
> +
> +#include "qom/object.h"
> +#include "qemu/queue.h"
> +
> +#define TYPE_CLOCK_IN "clock-in"
> +#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
> +#define TYPE_CLOCK_OUT "clock-out"
> +#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
> +
> +typedef void ClockCallback(void *opaque);
> +
> +typedef struct ClockOut ClockOut;
> +typedef struct ClockIn ClockIn;
> +
> +struct ClockIn {
> + /*< private >*/
> + Object parent_obj;
> + /*< private >*/
> + uint64_t frequency;
> + char *canonical_path; /* clock path cache */
> + ClockOut *driver; /* clock output controlling this clock */
> + ClockCallback *callback; /* local callback */
> + void *callback_opaque; /* opaque argument for the callback */
> + QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
> +};
> +
> +struct ClockOut {
> + /*< private >*/
> + Object parent_obj;
> + /*< private >*/
> + char *canonical_path; /* clock path cache */
> + QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
> +};
Can we keep the structure definitions opaque in hw/core/clock.c?
If so, clock_get_frequency() can't be inlined anymore.
> +
> +/**
> + * clock_out_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_out_setup_canonical_path(ClockOut *clk);
> +
> +/**
> + * clock_in_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_in_setup_canonical_path(ClockIn *clk);
> +
> +/**
> + * clock_add_callback:
> + * @clk: the clock to register the callback into
> + * @cb: the callback function
> + * @opaque: the argument to the callback
> + *
> + * Register a callback called on every clock update.
> + */
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
> +
> +/**
> + * clock_clear_callback:
> + * @clk: the clock to delete the callback from
> + *
> + * Unregister the callback registered with clock_set_callback.
> + */
> +void clock_clear_callback(ClockIn *clk);
> +
> +/**
> + * clock_init_frequency:
> + * @clk: the clock to initialize.
> + * @freq: the clock's frequency in Hz or 0 if unclocked.
> + *
> + * Initialize the local cached frequency value of @clk to @freq.
> + * Note: this function must only be called during device inititialization
> + * or migration.
> + */
> +void clock_init_frequency(ClockIn *clk, uint64_t freq);
> +
> +/**
> + * clock_connect:
> + * @clkin: the drived clock.
> + * @clkout: the driving clock.
> + *
> + * Setup @clkout to drive @clkin: Any @clkout update will be propagated
> + * to @clkin.
> + */
> +void clock_connect(ClockIn *clkin, ClockOut *clkout);
> +
> +/**
> + * clock_set_frequency:
> + * @clk: the clock to update.
> + * @freq: the new clock's frequency in Hz or 0 if unclocked.
> + *
> + * Update the @clk to the new @freq.
> + * This change will be propagated through registered clock inputs.
> + */
> +void clock_set_frequency(ClockOut *clk, uint64_t freq);
> +
> +/**
> + * clock_get_frequency:
> + * @clk: the clk to fetch the clock
> + *
> + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
> + */
> +static inline uint64_t clock_get_frequency(const ClockIn *clk)
> +{
> + return clk ? clk->frequency : 0;
> +}
> +
> +/**
> + * clock_is_enabled:
> + * @clk: a clock state
> + *
> + * @return: true if the clock is running. If @clk is NULL return false.
> + */
> +static inline bool clock_is_enabled(const ClockIn *clk)
> +{
> + return clock_get_frequency(clk) != 0;
> +}
> +
> +#endif /* QEMU_HW_CLOCK_H */
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
2019-11-25 13:07 ` Philippe Mathieu-Daudé
2019-11-25 13:37 ` Philippe Mathieu-Daudé
@ 2019-12-02 13:42 ` Peter Maydell
2019-12-03 15:28 ` Damien Hedde
2 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 13:42 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Introduce clock objects: ClockIn and ClockOut.
>
> These objects may be used to distribute clocks from an object to several
> other objects. Each ClockIn object contains the current state of the
> clock: the frequency; it allows an object to migrate its input clock state
> independently of other objects.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
"updates" (or "an update")
> through a callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Makefile.objs | 1 +
> hw/core/Makefile.objs | 1 +
> hw/core/clock.c | 144 ++++++++++++++++++++++++++++++++++++++++++
> hw/core/trace-events | 6 ++
> include/hw/clock.h | 124 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 276 insertions(+)
> create mode 100644 hw/core/clock.c
> create mode 100644 include/hw/clock.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a723a47e14..4da623c759 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/audio
> trace-events-subdirs += hw/block
> trace-events-subdirs += hw/block/dataplane
> trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
> trace-events-subdirs += hw/dma
> trace-events-subdirs += hw/hppa
> trace-events-subdirs += hw/i2c
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 69b408ad1c..c66a5b2c6b 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> +common-obj-y += clock.o
> common-obj-$(CONFIG_SOFTMMU) += nmi.o
> common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> new file mode 100644
> index 0000000000..888f247f2a
> --- /dev/null
> +++ b/hw/core/clock.c
> @@ -0,0 +1,144 @@
> +/*
> + * Clock inputs and outputs
> + *
> + * Copyright GreenSocs 2016-2018
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/clock.h"
> +#include "trace.h"
> +
> +#define CLOCK_PATH(_clk) (_clk->canonical_path)
Don't use leading underscores in identifiers, please.
> +
> +void clock_out_setup_canonical_path(ClockOut *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_in_setup_canonical_path(ClockIn *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
> +{
> + assert(clk);
> +
> + clk->callback = cb;
> + clk->callback_opaque = opaque;
> +}
> +
> +void clock_init_frequency(ClockIn *clk, uint64_t freq)
> +{
> + assert(clk);
This sort of assert isn't necessary. Asserts are good
when they help to make a bug visible sooner and more
obviously -- when they avoid "something goes wrong
much later on and further from the site of the actual
error". In this case, if the assert was not present
then the code would just segfault on the next line:
> +
> + clk->frequency = freq;
which is already a very easy bug to diagnose and
where the offending caller will be in the backtrace.
If the parameter isn't supposed to be NULL, and the
method doesn't actually do anything that would
dereference it, that might be a good candidate to
assert on.
The same kind of unnecessary assert is also in some of
the other functions here (and probably in other patches).
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index ecf966c314..aa940e268b 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -34,3 +34,9 @@ resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
> resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
> resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
> resettable_count_underflow(void *obj) "obj=%p"
> +
> +# hw/core/clock-port.c
> +clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
"driven-by"
> +clock_disconnect(const char *clk) "'%s'"
> +clock_set_frequency(const char *clk, uint64_t freq) "'%s' freq_hz=%" PRIu64
> +clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> new file mode 100644
> index 0000000000..fd11202ba4
> --- /dev/null
> +++ b/include/hw/clock.h
> @@ -0,0 +1,124 @@
> +#ifndef QEMU_HW_CLOCK_H
> +#define QEMU_HW_CLOCK_H
All new files need a copyright-and-license comment header (could
you check the rest of the patchset for this, please?).
> +
> +/**
> + * clock_get_frequency:
> + * @clk: the clk to fetch the clock
> + *
> + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
> + */
> +static inline uint64_t clock_get_frequency(const ClockIn *clk)
> +{
> + return clk ? clk->frequency : 0;
> +}
Is there a use case where we want to support "pass in NULL"
rather than just making it a programming error for the caller
to try that ?
> +
> +/**
> + * clock_is_enabled:
> + * @clk: a clock state
> + *
> + * @return: true if the clock is running. If @clk is NULL return false.
> + */
> +static inline bool clock_is_enabled(const ClockIn *clk)
> +{
> + return clock_get_frequency(clk) != 0;
> +}
Ditto here.
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
2019-11-25 13:05 ` Philippe Mathieu-Daudé
@ 2019-12-02 13:44 ` Peter Maydell
1 sibling, 0 replies; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 13:44 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> This was in the previous reviewed commit. But it can't be in the
> clock.c file in order to allow linux-user builds.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
2019-11-25 13:30 ` Philippe Mathieu-Daudé
@ 2019-12-02 14:34 ` Peter Maydell
2019-12-04 9:05 ` Damien Hedde
1 sibling, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 14:34 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Add functions to easily add input or output clocks to a device.
> A clock objects is added as a child of the device.
"object"
> The api is very similar the gpio's one.
"API"; "to the GPIO API".
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>
> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
> + bool forward)
> +{
> + NamedClockList *ncl;
> +
> + /*
> + * The clock path will be computed by the device's realize function call.
> + * This is required to ensure the clock's canonical path is right and log
> + * messages are meaningfull.
"meaningful"
> + */
> + assert(name);
> + assert(!dev->realized);
> +
> + /* The ncl structure will be freed in device's finalize function call */
Do you mean "in device_finalize()", or "in the finalize method
of the device" ? If you mean a specific function, then it's
good to name it, so the reader can go and check that code if
they need to confirm that there's a matching free()/deref/etc.
> + ncl = g_malloc0(sizeof(*ncl));
Prefer g_new0(NamedClockList, 1).
> + ncl->name = g_strdup(name);
> + ncl->forward = forward;
> +
> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
> + return ncl;
> +}
> +
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_OUT);
> +
> + /* will fail if name already exists */
This is true but it would be more helpful to say
/*
* Trying to create a clock whose name clashes with some other
* clock or property is a bug in the caller and we will abort().
*/
(assuming that's what's going on here).
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> + object_unref(clk); /* remove the initial ref made by object_new */
> +
> + ncl->out = CLOCK_OUT(clk);
> + return ncl->out;
> +}
> +
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque)
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_IN);
> + /*
> + * the ref initialized by object_new will be cleared during dev finalize.
This means "in device_finalize()", I think from reading later patches ?
> + * It allows us to safely remove the callback.
> + */
> +
> + /* will fail if name already exists */
Similar remark as for earlier comment.
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> +
> + ncl->in = CLOCK_IN(clk);
> + if (callback) {
> + clock_set_callback(ncl->in, callback, opaque);
> + }
> + return ncl->in;
> +}
> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + assert(dev && name);
> +
> + ncl = qdev_get_clocklist(dev, name);
> + return ncl ? ncl->in : NULL;
> +}
Do we expect to want to be able to pass in the name of
a clock that doesn't exist ? Should that be an error
rather than returning NULL ?
> +
> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + assert(dev && name);
> +
> + ncl = qdev_get_clocklist(dev, name);
> + return ncl ? ncl->out : NULL;
Ditto.
> +}
> +
> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
> + Error **errp)
> +{
> + ClockOut *clkout = qdev_get_clock_out(dev, name);
> +
> + if (!clk) {
> + error_setg(errp, "NULL input clock");
> + return;
> + }
> +
> + if (!clkout) {
> + error_setg(errp, "no output clock '%s' in device", name);
> + return;
> + }
> +
> + clock_connect(clk, clkout);
Do we need to support returning an error here, or would it
always be a programming bug to try to connect a non-existent clock?
> --- /dev/null
> +++ b/include/hw/qdev-clock.h
> @@ -0,0 +1,67 @@
> +#ifndef QDEV_CLOCK_H
> +#define QDEV_CLOCK_H
Another missing copyright/license comment.
> +
> +#include "hw/clock.h"
> +
> +/**
> + * qdev_init_clock_in:
> + * @dev: the device in which to add a clock
"the device to add a clock input to"
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @opaque: argument for the callback
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a input clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + * The callback will be called with @dev as opaque parameter.
Isn't it called with @opaque, not @dev ?
> + */
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque);
> +
> +/**
> + * qdev_init_clock_out:
> + * @dev: the device to add a clock to
"the device to add a clock output to"
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a output clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + */
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_get_clock_in:
> + * @dev: the device which has the clock
> + * @name: the name of the clock (can't be NULL).
> + * @returns: a pointer to the clock
> + *
> + * Get the clock @name from @dev or NULL if does not exists.
"if it does not exist"
> + */
> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_connect_clock_out:
> + * @dev: the device which has the clock
> + * @name: the name of the clock (can't be NULL).
> + * @errp: error report
> + *
> + * Connect @clk to the output clock @name of @dev.
> + * Reports an error if clk is NULL or @name does not exists in @dev.
"or if @name does not exist in @dev"
> + */
> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
> + Error **errp);
> +
> +/**
> + * qdev_pass_clock:
> + * @dev: the device to forward the clock to
> + * @name: the name of the clock to be added (can't be NULL)
> + * @container: the device which already has the clock
> + * @cont_name: the name of the clock in the container device
> + *
> + * Add a clock @name to @dev which forward to the clock @cont_name in @container
> + */
'container' seems odd terminology here, because I would expect
the usual use of this function to be when a 'container' object
like an SoC wants to forward a clock to one of its components;
in that case the 'container' SoC would be @dev, wouldn't it?
We should get this to be the same way round as qdev_pass_gpios(),
which takes "DeviceState *dev, DeviceState *container", and
passes the gpios that exist on 'dev' over to 'container' so that
'container' now has gpios which it did not before.
Also, your use of 'forward to' is inconsistent: in the 'dev'
documentation you say we're forwarding the clock to 'dev',
but in the body of the documentation you say we're forwarding
the clock to the clock in 'container'.
I think the way to resolve this is to stick to the terminology
in the function name itself:
@dev: the device which has the clock
@name: the name of the clock on @dev
@container: the name of the device which the clock should
be passed to
@cont_name: the name to use for the clock on @container
Q: if you pass a clock to another device with this function,
does it still exist to be used directly on the original
device? For qdev_pass_gpios it does not (I think), but
this is more accident of implementation than anything else.
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> + DeviceState *container, const char *cont_name);
> +
> +#endif /* QDEV_CLOCK_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index eb11f0f801..60a65f6142 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -131,6 +131,19 @@ struct NamedGPIOList {
> QLIST_ENTRY(NamedGPIOList) node;
> };
>
> +typedef struct NamedClockList NamedClockList;
> +
> +typedef struct ClockIn ClockIn;
> +typedef struct ClockOut ClockOut;
> +
> +struct NamedClockList {
> + char *name;
Could this be 'const char*' ?
> + bool forward;
> + ClockIn *in;
> + ClockOut *out;
> + QLIST_ENTRY(NamedClockList) node;
> +};
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
@ 2019-12-02 14:35 ` Peter Maydell
0 siblings, 0 replies; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 14:35 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This prints the clocks attached to a DeviceState when using "info qtree" monitor
> command. For every clock, it displays the direction, the name and if the
> clock is forwarded. For input clock, it displays also the frequency.
>
> This is based on the original work of Frederic Konrad.
Providing a sample of the 'info qtree' output in the
commit message would be nice.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
@ 2019-12-02 15:13 ` Peter Maydell
2019-12-04 11:04 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 15:13 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Introduce a function and macro helpers to setup several clocks
> in a device from a static array description.
>
> An element of the array describes the clock (name and direction) as
> well as the related callback and an optional offset to store the
> created object pointer in the device state structure.
>
> The array must be terminated by a special element QDEV_CLOCK_END.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/core/qdev-clock.c | 26 ++++++++++++++++
> include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index bebdd8fa15..32ad45c061 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>
> clock_connect(clk, clkout);
> }
> +
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
> +{
> + const struct ClockPortInitElem *elem;
> +
> + assert(dev);
> + assert(clocks);
More unnecessary asserts, I think.
> +/**
> + * ClockInitElem:
> + * @name: name of the clock (can't be NULL)
> + * @is_output: indicates whether the clock is input or output
> + * @callback: for inputs, optional callback to be called on clock's update
> + * with device as opaque
> + * @offset: optional offset to store the ClockIn or ClockOut pointer in device
> + * state structure (0 means unused)
> + */
> +struct ClockPortInitElem {
> + const char *name;
> + bool is_output;
> + ClockCallback *callback;
> + size_t offset;
> +};
> +
> +#define clock_offset_value(_type, _devstate, _field) \
> + (offsetof(_devstate, _field) + \
> + type_check(_type *, typeof_field(_devstate, _field)))
Avoid leading underscores, please.
> +
> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
> + .name = (stringify(_field)), \
> + .is_output = _is_output, \
> + .callback = _callback, \
> + .offset = clock_offset_value(_type, _devstate, _field), \
> +}
> +
> +/**
> + * QDEV_CLOCK_(IN|OUT):
> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
> + * a pointer to that same type.
It's a bit unclear what "below" here is referring to. Maybe
just have this be "@devstate: name of a C struct type"
and then explain below...
> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
> + * @_callback: (for input only) callback (or NULL) to be called with the device
> + * state as argument
> + *
...here, where we can have a paragraph giving the purpose
of the macro:
"Define an entry in a ClockPortInitArray which is intended
to be passed to qdev_init_clocks(), which should be called
with an @dev argument which is a pointer to the @devstate
struct type."
> + * The name of the clock will be derived from @_field
Derived how? Guessing from the stringify(_field) above that it
will be the same as the field name ?
It makes sense to hardcode the opaque pointer for the callback to be
the device pointer.
> + */
> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
> + QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
> +
> +#define QDEV_CLOCK_OUT(_devstate, _field) \
> + QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
> +
> +/**
> + * QDEV_CLOCK_IN_NOFIELD:
> + * @_name: name of the clock
> + * @_callback: callback (or NULL) to be called with the device state as argument
> + */
> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
> + .name = _name, \
> + .is_output = false, \
> + .callback = _callback, \
> + .offset = 0, \
> +}
When would we want to use this one ?
> +
> +#define QDEV_CLOCK_END { .name = NULL }
> +
> +typedef struct ClockPortInitElem ClockPortInitArray[];
> +
> +/**
> + * qdev_init_clocks:
> + * @dev: the device to add clocks
"to add clocks to"
> + * @clocks: a QDEV_CLOCK_END-terminated array which contains the
> + * clocks information.
> + */
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
> +
> #endif /* QDEV_CLOCK_H */
> --
> 2.22.0
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 6/9] docs/clocks: add device's clock documentation
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation Damien Hedde
@ 2019-12-02 15:17 ` Peter Maydell
2019-12-04 12:11 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 15:17 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> docs/devel/clock.txt | 246 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 246 insertions(+)
> create mode 100644 docs/devel/clock.txt
Could you convert this to rst format, please?
> +Changing a clock output
> +=======================
> +
> +A device can change its outputs using the clock_set_frequency function. It
> +will trigger updates on every connected inputs.
"input"
> +
> +For example, let's say that we have an output clock "clkout" and we have a
> +pointer to it in the device state because we did the following in init phase:
> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
> +
> +Then at any time (apart from the cases listed below), it is possible to
> +change the clock value by doing:
> +clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Ghz */
> +This operation must be done while holding the qemu io lock.
> +
> +One can change clocks only when it is allowed to have side effects on other
> +objects. In consequence, it is forbidden:
> ++ during migration,
> ++ and in the init phase of reset.
> +
> +Forwarding clocks
> +=================
> +
> +Sometimes, one needs to forward, or inherit, a clock from another device.
> +Typically, when doing device composition, a device might expose a sub-device's
> +clock without interfering with it.
> +The function qdev_pass_clock() can be used to achieve this behaviour. Note, that
"Note that"
> +it is possible to expose the clock under a different name. This works for both
> +inputs or outputs.
"inputs and outputs"
> +Migration
> +=========
> +
> +Only the ClockIn object has a state. ClockOut is not concerned by migration.
"has any state".
"ClockOut has no state and does not need special handling for migration."
> +
> +In case the frequency of in input clock is needed for a device's migration,
> +this state must be migrated.
Are you trying to say that if an input clock is known to be a
fixed frequency we don't need to migrate anything? I wonder
if we need to worry about that or if we could/should just say that
input clocks should always be migrated.
> The VMSTATE_CLOCKIN macro defines an entry to
> +be added in a vmstate description.
> +
> +For example, if a device has a clock input and the device state looks like:
> +MyDeviceState {
> + DeviceState parent_obj;
> + ClockIn *clk;
> +};
> +
> +Then, to add the clock frequency to the device's migrated state, the vmstate
> +description is:
> +VMStateDescription my_device_vmstate = {
> + .name = "my_device",
> + .fields = (VMStateField[]) {
> + VMSTATE_CLOCKIN(clk, MyDeviceState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +When adding a input clock support to an existing device, you must care about
> +migration compatibility. To this end, you can use the clock_init_frequency in
> +a pre_load function to setup a default value in case the source vm does not
> +migrate the frequency.
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
@ 2019-12-02 15:20 ` Peter Maydell
2019-12-04 12:51 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 15:20 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Switch the slcr to multi-phase reset and add some clocks:
> + the main input clock (ps_clk)
> + the reference clock outputs for each uart (uart0 & 1)
>
> The clock frequencies are computed using the internal pll & uart configuration
> registers and the ps_clk frequency.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Review of this and the following two patches by some Xilinx
person would be nice. I've just looked them over for general
issues, and haven't checked against the hardware specs.
> ---
> +/*
> + * return the output frequency of a clock given:
> + * + the frequencies in an array corresponding to mux's indexes
> + * + the register xxx_CLK_CTRL value
> + * + enable bit index in ctrl register
> + *
> + * This function make the assumption that ctrl_reg value is organized as follow:
"makes"; "that the"; "follows"
> + * + bits[13:8] clock divisor
> + * + bits[5:4] clock mux selector (index in array)
> + * + bits[index] clock enable
> + */
> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
> + uint32_t ctrl_reg,
> + unsigned index)
> +{
> + uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
> + uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
> +
> + /* first, check if clock is enabled */
> + if (((ctrl_reg >> index) & 1u) == 0) {
> + return 0;
> + }
> +
> + /*
> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
> + * is [1;63].
Is this the range notation the spec doc uses?
> + * So divide the source while avoiding division-by-zero.
> + */
> + return mux[srcsel] / (divisor ? divisor : 1u);
> +}
> +
> +static const ClockPortInitArray zynq_slcr_clocks = {
> + QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
> + QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
> + QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
> + QDEV_CLOCK_END
> +};
> +
> static void zynq_slcr_init(Object *obj)
> {
> ZynqSLCRState *s = ZYNQ_SLCR(obj);
> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
> ZYNQ_SLCR_MMIO_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +
> + qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
> }
>
> static const VMStateDescription vmstate_zynq_slcr = {
> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
> static void zynq_slcr_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> + ResettableClass *rc = RESETTABLE_CLASS(klass);
>
> dc->vmsd = &vmstate_zynq_slcr;
> - dc->reset = zynq_slcr_reset;
> + rc->phases.init = zynq_slcr_reset_init;
> + rc->phases.hold = zynq_slcr_reset_hold;
> + rc->phases.exit = zynq_slcr_reset_exit;
> }
We're adding an input clock, so doesn't the migration
state struct need to be updated to migrate it ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support Damien Hedde
@ 2019-12-02 15:24 ` Peter Maydell
2019-12-04 13:35 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 15:24 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Switch the cadence uart to multi-phase reset and add the
> reference clock input.
>
> The input clock frequency is added to the migration structure.
>
> The reference clock controls the baudrate generation. If it disabled,
> any input characters and events are ignored.
>
> If this clock remains unconnected, the uart behaves as before
> (it default to a 50MHz ref clock).
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> static void uart_parameters_setup(CadenceUARTState *s)
> {
> QEMUSerialSetParams ssp;
> - unsigned int baud_rate, packet_size;
> + unsigned int baud_rate, packet_size, input_clk;
> + input_clk = clock_get_frequency(s->refclk);
>
> - baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
> - UART_INPUT_CLK / 8 : UART_INPUT_CLK;
> + baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
> + baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> + trace_cadence_uart_baudrate(baud_rate);
> +
> + ssp.speed = baud_rate;
>
> - ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> packet_size = 1;
>
> switch (s->r[R_MR] & UART_MR_PAR) {
> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
> }
>
> packet_size += ssp.data_bits + ssp.stop_bits;
> + if (ssp.speed == 0) {
> + /*
> + * Avoid division-by-zero below.
> + * TODO: find something better
> + */
Any ideas what might be better? :-)
> + ssp.speed = 1;
> + }
> s->char_tx_time = (NANOSECONDS_PER_SECOND / ssp.speed) * packet_size;
> qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
> }
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
@ 2019-12-02 15:34 ` Peter Maydell
2019-12-03 14:59 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 15:34 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Add the connection between the slcr's output clocks and the uarts inputs.
>
> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
> (the default frequency). This clock is used to feed the slcr's input
> clock.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Nothing obviously wrong in the body of the patch, but as with
7 and 8, review from a Xilinx person would be helpful.
/* board base frequency: 33.333333 MHz */
#define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
This is interesting, because it's not an integer... I'll come back
to this topic in a reply to the cover letter in a moment.
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
` (8 preceding siblings ...)
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
@ 2019-12-02 16:15 ` Peter Maydell
2019-12-04 16:40 ` Damien Hedde
9 siblings, 1 reply; 47+ messages in thread
From: Peter Maydell @ 2019-12-02 16:15 UTC (permalink / raw)
To: Damien Hedde
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This series aims to add a way to model clock distribution in qemu. This allows
> to model the clock tree of a platform allowing us to inspect clock
> configuration and detect problems such as disabled clock or bad configured
> pll.
>
> The added clock api is very similar the the gpio api for devices. We can add
> input and output and connect them together.
>
> Very few changes since v5 in the core patches: we were waiting for multi phase
> ability to allow proper initialization of the clock tree. So this is almost a
> simple rebase on top of the current "Multi-phase reset mechanism" series.
> Based-on: <20190821163341.16309-1-damien.hedde@greensocs.com>
I've now gone through and given review comments on the patchset.
I don't think there was anything particularly major -- overall
I like the structure and API (and also the documentation!).
The one topic I think we could do with discussing is whether
a simple uint64_t giving the frequency of the clock in Hz is
the right representation. In particular in your patch 9 the
board has a clock frequency that's not a nice integer number
of Hz. I think Philippe also mentioned on irc some board where
the UART clock ends up at a weird frequency. Since the
representation of the frequency is baked into the migration
format it's going to be easier to get it right first rather
than trying to change it later.
So what should the representation be? Some random thoughts:
1) ptimer internally uses a 'period plus fraction' representation:
int64_t period is the integer part of the period in nanoseconds,
uint32_t period_frac is the fractional part of the period
(if you like you can think of this as "96-bit integer
period measured in units of one-2^32nd of a nanosecond").
However its only public interfaces for setting the frequency
are (a) set the frequency in Hz (uint32_t) or (b) set
the period in nanoseconds (int64_t); the period_frac part
is used to handle frequencies which don't work out to
a nice whole number of nanoseconds per cycle.
2) I hear that SystemC uses "value plus a time unit", with
the smallest unit being a picosecond. (I think SystemC
also lets you specify the duty cycle, but we definitely
don't want to get into that!)
3) QEMUTimers are basically just nanosecond timers
4) The MAME emulator seems to work with periods of
96-bit attoseconds (represented internally by a
32-bit count of seconds plus a 64-bit count of
attoseconds). One attosecond is 1e-18 seconds.
Does anybody else have experience with other modelling
or emulator technology and how it represents clocks ?
I feel we should at least be able to represent clocks
with the same accuracy that ptimer has.
thanks
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2019-12-02 15:34 ` Peter Maydell
@ 2019-12-03 14:59 ` Damien Hedde
2019-12-03 15:29 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-12-03 14:59 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 4:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Add the connection between the slcr's output clocks and the uarts inputs.
>>
>> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
>> (the default frequency). This clock is used to feed the slcr's input
>> clock.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>
> Nothing obviously wrong in the body of the patch, but as with
> 7 and 8, review from a Xilinx person would be helpful.
>
> /* board base frequency: 33.333333 MHz */
> #define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
>
> This is interesting, because it's not an integer... I'll come back
> to this topic in a reply to the cover letter in a moment.
For this precise case, what I wanted is the resulting integer which I
got from the device trees in linux (btw I should probably add this point
in comment). Just thought it was more readable this way than "33333333".
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-11-25 13:37 ` Philippe Mathieu-Daudé
@ 2019-12-03 15:14 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-03 15:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
On 11/25/19 2:37 PM, Philippe Mathieu-Daudé wrote:
> On 9/4/19 2:55 PM, Damien Hedde wrote:
>> Introduce clock objects: ClockIn and ClockOut.
>>
>> These objects may be used to distribute clocks from an object to several
>> other objects. Each ClockIn object contains the current state of the
>> clock: the frequency; it allows an object to migrate its input clock
>> state
>> independently of other objects.
>>
>> A ClockIn may be connected to a ClockOut so that it receives update,
>> through a callback, whenever the Clockout is updated using the
>> ClockOut's set function.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Makefile.objs | 1 +
>> hw/core/Makefile.objs | 1 +
>> hw/core/clock.c | 144 ++++++++++++++++++++++++++++++++++++++++++
>> hw/core/trace-events | 6 ++
>> include/hw/clock.h | 124 ++++++++++++++++++++++++++++++++++++
>> 5 files changed, 276 insertions(+)
>> create mode 100644 hw/core/clock.c
>> create mode 100644 include/hw/clock.h
>>
>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>> index ecf966c314..aa940e268b 100644
>> --- a/hw/core/trace-events
>> +++ b/hw/core/trace-events
>> @@ -34,3 +34,9 @@ resettable_phase_hold_end(void *obj, int needed)
>> "obj=%p needed=%d"
>> resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
>> resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p
>> count=%" PRIu32
>> resettable_count_underflow(void *obj) "obj=%p"
>> +
>> +# hw/core/clock-port.c
>
> "# clock.c"
>
Oups, I missed this one in the renaming.
>> +
>> +struct ClockIn {
>> + /*< private >*/
>> + Object parent_obj;
>> + /*< private >*/
>> + uint64_t frequency;
>> + char *canonical_path; /* clock path cache */
>> + ClockOut *driver; /* clock output controlling this clock */
>> + ClockCallback *callback; /* local callback */
>> + void *callback_opaque; /* opaque argument for the callback */
>> + QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
>> +};
>> +
>> +struct ClockOut {
>> + /*< private >*/
>> + Object parent_obj;
>> + /*< private >*/
>> + char *canonical_path; /* clock path cache */
>> + QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
>> +};
>
> Can we keep the structure definitions opaque in hw/core/clock.c?
> If so, clock_get_frequency() can't be inlined anymore.
>
I think so. Apart from the monitor command (and the inline), nothing
requires the structure fields. I suppose we can add a function to access
or print the fields to be used by the monitor.
I don't have a opinion on this.
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects
2019-12-02 13:42 ` Peter Maydell
@ 2019-12-03 15:28 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-03 15:28 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 2:42 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Introduce clock objects: ClockIn and ClockOut.
>>
>> These objects may be used to distribute clocks from an object to several
>> other objects. Each ClockIn object contains the current state of the
>> clock: the frequency; it allows an object to migrate its input clock state
>> independently of other objects.
>>
>> A ClockIn may be connected to a ClockOut so that it receives update,
>
> "updates" (or "an update")
>
>> through a callback, whenever the Clockout is updated using the
>> ClockOut's set function.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> +
>> +#define CLOCK_PATH(_clk) (_clk->canonical_path)
>
> Don't use leading underscores in identifiers, please.
ok
>
>> +
>> +void clock_init_frequency(ClockIn *clk, uint64_t freq)
>> +{
>> + assert(clk);
>
> This sort of assert isn't necessary. Asserts are good
> when they help to make a bug visible sooner and more
> obviously -- when they avoid "something goes wrong
> much later on and further from the site of the actual
> error". In this case, if the assert was not present
> then the code would just segfault on the next line:
>
>> +
>> + clk->frequency = freq;
>
> which is already a very easy bug to diagnose and
> where the offending caller will be in the backtrace.
>
> If the parameter isn't supposed to be NULL, and the
> method doesn't actually do anything that would
> dereference it, that might be a good candidate to
> assert on.
>
> The same kind of unnecessary assert is also in some of
> the other functions here (and probably in other patches).
I'll take a look.
>
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> new file mode 100644
>> index 0000000000..fd11202ba4
>> --- /dev/null
>> +++ b/include/hw/clock.h
>> @@ -0,0 +1,124 @@
>> +#ifndef QEMU_HW_CLOCK_H
>> +#define QEMU_HW_CLOCK_H
>
> All new files need a copyright-and-license comment header (could
> you check the rest of the patchset for this, please?).
Sure.
>
>> +
>
>> +/**
>> + * clock_get_frequency:
>> + * @clk: the clk to fetch the clock
>> + *
>> + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
>> + */
>> +static inline uint64_t clock_get_frequency(const ClockIn *clk)
>> +{
>> + return clk ? clk->frequency : 0;
>> +}
>
> Is there a use case where we want to support "pass in NULL"
> rather than just making it a programming error for the caller
> to try that ?
No, it's probably a remnant of previous version where input and output
shared some code. I'll remove it.
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2019-12-03 14:59 ` Damien Hedde
@ 2019-12-03 15:29 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-03 15:29 UTC (permalink / raw)
To: Damien Hedde, Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/3/19 3:59 PM, Damien Hedde wrote:
> On 12/2/19 4:34 PM, Peter Maydell wrote:
>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>
>>> Add the connection between the slcr's output clocks and the uarts inputs.
>>>
>>> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
>>> (the default frequency). This clock is used to feed the slcr's input
>>> clock.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>
>> Nothing obviously wrong in the body of the patch, but as with
>> 7 and 8, review from a Xilinx person would be helpful.
>>
>> /* board base frequency: 33.333333 MHz */
>> #define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
>>
>> This is interesting, because it's not an integer... I'll come back
>> to this topic in a reply to the cover letter in a moment.
>
> For this precise case, what I wanted is the resulting integer which I
> got from the device trees in linux (btw I should probably add this point
> in comment). Just thought it was more readable this way than "33333333".
FWIW I'm auditing if it is possible to use the float type for
frequencies (before to ask on the list if this makes sense), because in
hw/core/ptimer we use timers with periods, and loose some precision
using 1/freq again.
Also we have MiB/KiB in "qemu/units.h" and I'd like to introduce MHz/KHz.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-11-25 13:30 ` Philippe Mathieu-Daudé
@ 2019-12-03 15:35 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-03 15:35 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: peter.maydell, berrange, ehabkost, alistair, mark.burton,
marcandre.lureau, qemu-arm, pbonzini, edgar.iglesias
On 11/25/19 2:30 PM, Philippe Mathieu-Daudé wrote:
> Nitpick: remove trailing dot in patch subject
>
> On 9/4/19 2:55 PM, Damien Hedde wrote:
>> Add functions to easily add input or output clocks to a device.
>> A clock objects is added as a child of the device.
>
> <newline>?
>
>> The api is very similar the gpio's one.
>
> Maybe "This API is very similar to the QDEV GPIO API."
>
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>
>> ---
>> I've removed the reviewed-by/tested-by of Philippe because I did a small
>> modification.
>>
>> qdev_connect_clock() which allowed to connect an input to an output is
>> now split in 2:
>> + qdev_get_clock_in() which gets a given input from a device
>> + qdev_connect_clock_out() which connect a given output to a clock
>> (previously fetched by qdev_get_clock_in())
>> This part is located in (qdev-clock.[c|h]).
>> It better matches gpios api and also add the possibility to connect a
>> device's input clock to a random output clock (used in patch 9).
>>
>> Also add missing qdev-clock in the test-qdev-global-props so that tests
>> pass.
>> ---
>> hw/core/Makefile.objs | 2 +-
>> hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++
>> hw/core/qdev.c | 32 +++++++++
>> include/hw/qdev-clock.h | 67 +++++++++++++++++
>> include/hw/qdev-core.h | 14 ++++
>> tests/Makefile.include | 1 +
>
> Please setup the scripts/git.orderfile to ease reviews.
>
>> 6 files changed, 270 insertions(+), 1 deletion(-)
>> create mode 100644 hw/core/qdev-clock.c
>> create mode 100644 include/hw/qdev-clock.h
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 8fcebf2e67..4523d3b5c7 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -1,5 +1,5 @@
>> # core qdev-related obj files, also used by *-user:
>> -common-obj-y += qdev.o qdev-properties.o
>> +common-obj-y += qdev.o qdev-properties.o qdev-clock.o
>> common-obj-y += bus.o reset.o
>> common-obj-y += resettable.o
>> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> new file mode 100644
>> index 0000000000..bebdd8fa15
>> --- /dev/null
>> +++ b/hw/core/qdev-clock.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Device's clock
>> + *
>> + * Copyright GreenSocs 2016-2018
>
> 2019
>
>> + *
>> + * Authors:
>> + * Frederic Konrad <fred.konrad@greensocs.com>
>> + * Damien Hedde <damien.hedde@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev-clock.h"
>> +#include "hw/qdev-core.h"
>> +#include "qapi/error.h"
>> +
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const
>> char *name,
>> + bool forward)
>
> Indentation off.
>
>> +{
>> + NamedClockList *ncl;
>> +
>> + /*
>> + * The clock path will be computed by the device's realize
>> function call.
>> + * This is required to ensure the clock's canonical path is right
>> and log
>> + * messages are meaningfull.
>> + */
>> + assert(name);
>> + assert(!dev->realized);
>> +
>> + /* The ncl structure will be freed in device's finalize function
>> call */
>> + ncl = g_malloc0(sizeof(*ncl));
>
> Similar but easier to review:
>
> ncl = g_new0(NamedClockList, 1)
>
>> + ncl->name = g_strdup(name);
>> + ncl->forward = forward;
>> +
>> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> + return ncl;
>> +}
>> +
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> + NamedClockList *ncl;
>> + Object *clk;
>> +
>> + ncl = qdev_init_clocklist(dev, name, false);
>> +
>> + clk = object_new(TYPE_CLOCK_OUT);
>> +
>> + /* will fail if name already exists */
>> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> + object_unref(clk); /* remove the initial ref made by object_new */
>> +
>> + ncl->out = CLOCK_OUT(clk);
>> + return ncl->out;
>> +}
>> +
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> + ClockCallback *callback, void *opaque)
>
> Indentation off.
>
>> +{
>> + NamedClockList *ncl;
>> + Object *clk;
>> +
>> + ncl = qdev_init_clocklist(dev, name, false);
>> +
>> + clk = object_new(TYPE_CLOCK_IN);
>> + /*
>> + * the ref initialized by object_new will be cleared during dev
>> finalize.
>> + * It allows us to safely remove the callback.
>> + */
>> +
>> + /* will fail if name already exists */
>> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +
>> + ncl->in = CLOCK_IN(clk);
>> + if (callback) {
>> + clock_set_callback(ncl->in, callback, opaque);
>> + }
>> + return ncl->in;
>> +}
>> +
>> +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const
>> char *name)
>> +{
>> + NamedClockList *ncl;
>> +
>> + QLIST_FOREACH(ncl, &dev->clocks, node) {
>> + if (strcmp(name, ncl->name) == 0) {
>> + return ncl;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +void qdev_pass_clock(DeviceState *dev, const char *name,
>> + DeviceState *container, const char *cont_name)
>> +{
>> + NamedClockList *original_ncl, *ncl;
>> + Object **clk;
>
> Is it really a Object** or a Object*?
An Object** because it tells where the Object* is stored for the link
property below.
>
>> +
>> + assert(container && cont_name);
>> +
>> + original_ncl = qdev_get_clocklist(container, cont_name);
>> + assert(original_ncl); /* clock must exist in origin */
>> +
>> + ncl = qdev_init_clocklist(dev, name, true);
>> +
>> + if (ncl->out) {
>> + clk = (Object **)&ncl->out;
>> + } else {
>> + clk = (Object **)&ncl->in;
>> + }
>> +
>> + /* will fail if name already exists */
>> + object_property_add_link(OBJECT(dev), name,
>> object_get_typename(*clk),
>> + clk, NULL, OBJ_PROP_LINK_STRONG,
>> &error_abort);
>> +}
>> +
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-12-02 14:34 ` Peter Maydell
@ 2019-12-04 9:05 ` Damien Hedde
2019-12-04 9:53 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 9:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 3:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Add functions to easily add input or output clocks to a device.
>> A clock objects is added as a child of the device.
>
> "object"
>
>> The api is very similar the gpio's one.
>
> "API"; "to the GPIO API".
>
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>
>
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
>> + bool forward)
>> +{
>> + NamedClockList *ncl;
>> +
>> + /*
>> + * The clock path will be computed by the device's realize function call.
>> + * This is required to ensure the clock's canonical path is right and log
>> + * messages are meaningfull.
>
> "meaningful"
>
>> + */
>> + assert(name);
>> + assert(!dev->realized);
>> +
>> + /* The ncl structure will be freed in device's finalize function call */
>
> Do you mean "in device_finalize()", or "in the finalize method
> of the device" ? If you mean a specific function, then it's
> good to name it, so the reader can go and check that code if
> they need to confirm that there's a matching free()/deref/etc.
Yes, it is device_finalize(). I'll update the comment.
>
>> + ncl = g_malloc0(sizeof(*ncl));
>
> Prefer g_new0(NamedClockList, 1).
>
>> + ncl->name = g_strdup(name);
>> + ncl->forward = forward;
>> +
>> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> + return ncl;
>> +}
>> +
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> + NamedClockList *ncl;
>> + Object *clk;
>> +
>> + ncl = qdev_init_clocklist(dev, name, false);
>> +
>> + clk = object_new(TYPE_CLOCK_OUT);
>> +
>> + /* will fail if name already exists */
>
> This is true but it would be more helpful to say
> /*
> * Trying to create a clock whose name clashes with some other
> * clock or property is a bug in the caller and we will abort().
> */
>
> (assuming that's what's going on here).
You're right.
>
>> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> + object_unref(clk); /* remove the initial ref made by object_new */
>> +
>> + ncl->out = CLOCK_OUT(clk);
>> + return ncl->out;
>> +}
>> +
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> + ClockCallback *callback, void *opaque)
>> +{
>> + NamedClockList *ncl;
>> + Object *clk;
>> +
>> + ncl = qdev_init_clocklist(dev, name, false);
>> +
>> + clk = object_new(TYPE_CLOCK_IN);
>> + /*
>> + * the ref initialized by object_new will be cleared during dev finalize.
>
> This means "in device_finalize()", I think from reading later patches ?
Yes. I'll update the comment too.
>
>> + * It allows us to safely remove the callback.
>> + */
>> +
>> + /* will fail if name already exists */
>
> Similar remark as for earlier comment.
>
>> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +
>> + ncl->in = CLOCK_IN(clk);
>> + if (callback) {
>> + clock_set_callback(ncl->in, callback, opaque);
>> + }
>> + return ncl->in;
>> +}
>
>> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
>> +{
>> + NamedClockList *ncl;
>> +
>> + assert(dev && name);
>> +
>> + ncl = qdev_get_clocklist(dev, name);
>> + return ncl ? ncl->in : NULL;
>> +}
>
> Do we expect to want to be able to pass in the name of
> a clock that doesn't exist ? Should that be an error
> rather than returning NULL ?
I think it can be an error and we may assert the clock exists.
>
>> +
>> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
>> +{
>> + NamedClockList *ncl;
>> +
>> + assert(dev && name);
>> +
>> + ncl = qdev_get_clocklist(dev, name);
>> + return ncl ? ncl->out : NULL;
>
> Ditto.
>
>> +}
>> +
>> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>> + Error **errp)
>> +{
>> + ClockOut *clkout = qdev_get_clock_out(dev, name);
>> +
>> + if (!clk) {
>> + error_setg(errp, "NULL input clock");
>> + return;
>> + }
>> +
>> + if (!clkout) {
>> + error_setg(errp, "no output clock '%s' in device", name);
>> + return;
>> + }
>> +
>> + clock_connect(clk, clkout);
>
> Do we need to support returning an error here, or would it
> always be a programming bug to try to connect a non-existent clock?
As for above, I think it can be considered an error. Should I remove the
errp and do an assert instead ?
>
>> --- /dev/null
>> +++ b/include/hw/qdev-clock.h
>> @@ -0,0 +1,67 @@
>> +#ifndef QDEV_CLOCK_H
>> +#define QDEV_CLOCK_H
>
> Another missing copyright/license comment.
>
>> +
>> +#include "hw/clock.h"
>> +
>> +/**
>> + * qdev_init_clock_in:
>> + * @dev: the device in which to add a clock
>
> "the device to add a clock input to"
>
>> + * @name: the name of the clock (can't be NULL).
>> + * @callback: optional callback to be called on update or NULL.
>> + * @opaque: argument for the callback
>> + * @returns: a pointer to the newly added clock
>> + *
>> + * Add a input clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + * The callback will be called with @dev as opaque parameter.
>
> Isn't it called with @opaque, not @dev ?
>
>> + */
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> + ClockCallback *callback, void *opaque);
>> +
>> +/**
>> + * qdev_init_clock_out:
>> + * @dev: the device to add a clock to
>
> "the device to add a clock output to"
>
>> + * @name: the name of the clock (can't be NULL).
>> + * @callback: optional callback to be called on update or NULL.
>> + * @returns: a pointer to the newly added clock
>> + *
>> + * Add a output clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + */
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_get_clock_in:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock (can't be NULL).
>> + * @returns: a pointer to the clock
>> + *
>> + * Get the clock @name from @dev or NULL if does not exists.
>
> "if it does not exist"
>
>> + */
>> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_connect_clock_out:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock (can't be NULL).
>> + * @errp: error report
>> + *
>> + * Connect @clk to the output clock @name of @dev.
>> + * Reports an error if clk is NULL or @name does not exists in @dev.
>
> "or if @name does not exist in @dev"
>
>> + */
>> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>> + Error **errp);
>> +
>> +/**
>> + * qdev_pass_clock:
>> + * @dev: the device to forward the clock to
>> + * @name: the name of the clock to be added (can't be NULL)
>> + * @container: the device which already has the clock
>> + * @cont_name: the name of the clock in the container device
>> + *
>> + * Add a clock @name to @dev which forward to the clock @cont_name in @container
>> + */
>
> 'container' seems odd terminology here, because I would expect
> the usual use of this function to be when a 'container' object
> like an SoC wants to forward a clock to one of its components;
> in that case the 'container' SoC would be @dev, wouldn't it?
Yes. I agree it is confusing.
This function just allow a a device 'A' to exhibit a clock from another
device 'B' (typically a composing sub-device, inside a soc like you
said). The device A is not supposed to interact with the clock itself.
The original sub-device is the true
owner/controller of the clock and is the only one which should use the
clock API: setting a callback on it (input clock); or updating the clock
frequency (output clock).
Basically, it just add the clock into the device clock namespace without
interfering with it.
> We should get this to be the same way round as qdev_pass_gpios(),
> which takes "DeviceState *dev, DeviceState *container", and
> passes the gpios that exist on 'dev' over to 'container' so that
> 'container' now has gpios which it did not before.
Ok, I'll invert the arguments.
>
> Also, your use of 'forward to' is inconsistent: in the 'dev'
> documentation you say we're forwarding the clock to 'dev',
> but in the body of the documentation you say we're forwarding
> the clock to the clock in 'container'.
I'll try to clarify this and make the documentation more consistent with
the comments here.
>
> I think the way to resolve this is to stick to the terminology
> in the function name itself:
> @dev: the device which has the clock
> @name: the name of the clock on @dev
> @container: the name of the device which the clock should
> be passed to
> @cont_name: the name to use for the clock on @container
I think container is confusing because depending on how we reason (clock
in a device; device in another device), we end up thinking the opposite.
Maybe we can use "exhibit" instead of "container" in the 2nd pair of
parameters, or prefix use "origin" or "owner" as a prefix for the first
pair of parameters.
>
> Q: if you pass a clock to another device with this function,
> does it still exist to be used directly on the original
> device? For qdev_pass_gpios it does not (I think), but
> this is more accident of implementation than anything else.
It depends what we mean by "used by".
Original device can:
+ set the callback in case it is an input
+ update the frequency in case it is an output
But since an input clock can only be connected once,
I think the logic here is that any connection should now go through the
new device. But this is not checked and using one or the other is
exactly the same.
Maybe I misunderstood the meaning of qdev_pass_gpios().
>
>> +void qdev_pass_clock(DeviceState *dev, const char *name,
>> + DeviceState *container, const char *cont_name);
>> +
>> +#endif /* QDEV_CLOCK_H */
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index eb11f0f801..60a65f6142 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -131,6 +131,19 @@ struct NamedGPIOList {
>> QLIST_ENTRY(NamedGPIOList) node;
>> };
>>
>> +typedef struct NamedClockList NamedClockList;
>> +
>> +typedef struct ClockIn ClockIn;
>> +typedef struct ClockOut ClockOut;
>> +
>> +struct NamedClockList {
>> + char *name;
>
> Could this be 'const char*' ?
Yes.
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-12-04 9:05 ` Damien Hedde
@ 2019-12-04 9:53 ` Philippe Mathieu-Daudé
2019-12-04 11:58 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-04 9:53 UTC (permalink / raw)
To: Damien Hedde, Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/4/19 10:05 AM, Damien Hedde wrote:
> On 12/2/19 3:34 PM, Peter Maydell wrote:
>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>
[...]
>>> +/**
>>> + * qdev_pass_clock:
>>> + * @dev: the device to forward the clock to
>>> + * @name: the name of the clock to be added (can't be NULL)
>>> + * @container: the device which already has the clock
>>> + * @cont_name: the name of the clock in the container device
>>> + *
>>> + * Add a clock @name to @dev which forward to the clock @cont_name in @container
>>> + */
>>
>> 'container' seems odd terminology here, because I would expect
>> the usual use of this function to be when a 'container' object
>> like an SoC wants to forward a clock to one of its components;
>> in that case the 'container' SoC would be @dev, wouldn't it?
>
> Yes. I agree it is confusing.
> This function just allow a a device 'A' to exhibit a clock from another
> device 'B' (typically a composing sub-device, inside a soc like you
> said). The device A is not supposed to interact with the clock itself.
> The original sub-device is the true
> owner/controller of the clock and is the only one which should use the
> clock API: setting a callback on it (input clock); or updating the clock
> frequency (output clock).
> Basically, it just add the clock into the device clock namespace without
> interfering with it.
>
>> We should get this to be the same way round as qdev_pass_gpios(),
>> which takes "DeviceState *dev, DeviceState *container", and
>> passes the gpios that exist on 'dev' over to 'container' so that
>> 'container' now has gpios which it did not before.
>
> Ok, I'll invert the arguments.
>
>>
>> Also, your use of 'forward to' is inconsistent: in the 'dev'
>> documentation you say we're forwarding the clock to 'dev',
>> but in the body of the documentation you say we're forwarding
>> the clock to the clock in 'container'.
>
> I'll try to clarify this and make the documentation more consistent with
> the comments here.
>
>>
>> I think the way to resolve this is to stick to the terminology
>> in the function name itself:
>> @dev: the device which has the clock
>> @name: the name of the clock on @dev
>> @container: the name of the device which the clock should
>> be passed to
>> @cont_name: the name to use for the clock on @container
>
> I think container is confusing because depending on how we reason (clock
> in a device; device in another device), we end up thinking the opposite.
>
> Maybe we can use "exhibit" instead of "container" in the 2nd pair of
> parameters, or prefix use "origin" or "owner" as a prefix for the first
> pair of parameters.
@sink vs @source?
>> Q: if you pass a clock to another device with this function,
>> does it still exist to be used directly on the original
>> device? For qdev_pass_gpios it does not (I think), but
>> this is more accident of implementation than anything else.
>
> It depends what we mean by "used by".
> Original device can:
> + set the callback in case it is an input
> + update the frequency in case it is an output
Hmm here you use @input vs @output...
> But since an input clock can only be connected once,
> I think the logic here is that any connection should now go through the
> new device. But this is not checked and using one or the other is
> exactly the same.
>
> Maybe I misunderstood the meaning of qdev_pass_gpios().
[...]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction
2019-12-02 15:13 ` Peter Maydell
@ 2019-12-04 11:04 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 11:04 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 4:13 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Introduce a function and macro helpers to setup several clocks
>> in a device from a static array description.
>>
>> An element of the array describes the clock (name and direction) as
>> well as the related callback and an optional offset to store the
>> created object pointer in the device state structure.
>>
>> The array must be terminated by a special element QDEV_CLOCK_END.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> hw/core/qdev-clock.c | 26 ++++++++++++++++
>> include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 93 insertions(+)
>>
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> index bebdd8fa15..32ad45c061 100644
>> --- a/hw/core/qdev-clock.c
>> +++ b/hw/core/qdev-clock.c
>> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk,
>>
>> clock_connect(clk, clkout);
>> }
>> +
>> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
>> +{
>> + const struct ClockPortInitElem *elem;
>> +
>> + assert(dev);
>> + assert(clocks);
>
> More unnecessary asserts, I think.
>
>
>
>> +/**
>> + * ClockInitElem:
>> + * @name: name of the clock (can't be NULL)
>> + * @is_output: indicates whether the clock is input or output
>> + * @callback: for inputs, optional callback to be called on clock's update
>> + * with device as opaque
>> + * @offset: optional offset to store the ClockIn or ClockOut pointer in device
>> + * state structure (0 means unused)
>> + */
>> +struct ClockPortInitElem {
>> + const char *name;
>> + bool is_output;
>> + ClockCallback *callback;
>> + size_t offset;
>> +};
>> +
>> +#define clock_offset_value(_type, _devstate, _field) \
>> + (offsetof(_devstate, _field) + \
>> + type_check(_type *, typeof_field(_devstate, _field)))
>
> Avoid leading underscores, please.
>
>> +
>> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
>> + .name = (stringify(_field)), \
>> + .is_output = _is_output, \
>> + .callback = _callback, \
>> + .offset = clock_offset_value(_type, _devstate, _field), \
>> +}
>> +
>> +/**
>> + * QDEV_CLOCK_(IN|OUT):
>> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
>> + * a pointer to that same type.
>
> It's a bit unclear what "below" here is referring to. Maybe
> just have this be "@devstate: name of a C struct type"
> and then explain below...
>
>> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
>> + * @_callback: (for input only) callback (or NULL) to be called with the device
>> + * state as argument
>> + *
>
> ...here, where we can have a paragraph giving the purpose
> of the macro:
>
> "Define an entry in a ClockPortInitArray which is intended
> to be passed to qdev_init_clocks(), which should be called
> with an @dev argument which is a pointer to the @devstate
> struct type."
Sounds good.
>
>> + * The name of the clock will be derived from @_field
>
> Derived how? Guessing from the stringify(_field) above that it
> will be the same as the field name ?
yes.
>
> It makes sense to hardcode the opaque pointer for the callback to be
> the device pointer.
>
>
>> + */
>> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
>> + QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
>> +
>> +#define QDEV_CLOCK_OUT(_devstate, _field) \
>> + QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
>> +
>> +/**
>> + * QDEV_CLOCK_IN_NOFIELD:
>> + * @_name: name of the clock
>> + * @_callback: callback (or NULL) to be called with the device state as argument
>> + */
>> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
>> + .name = _name, \
>> + .is_output = false, \
>> + .callback = _callback, \
>> + .offset = 0, \
>> +}
>
> When would we want to use this one ?
If the callback interaction is enough, we don't need to access the clock
object directly. So we don't need the field in the device state
structure. I can remove this macro for sake of simplicity.
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.
2019-12-04 9:53 ` Philippe Mathieu-Daudé
@ 2019-12-04 11:58 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 11:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/4/19 10:53 AM, Philippe Mathieu-Daudé wrote:
> On 12/4/19 10:05 AM, Damien Hedde wrote:
>> On 12/2/19 3:34 PM, Peter Maydell wrote:
>>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde
>>> <damien.hedde@greensocs.com> wrote:
>>>>
> [...]
>>>> +/**
>>>> + * qdev_pass_clock:
>>>> + * @dev: the device to forward the clock to
>>>> + * @name: the name of the clock to be added (can't be NULL)
>>>> + * @container: the device which already has the clock
>>>> + * @cont_name: the name of the clock in the container device
>>>> + *
>>>> + * Add a clock @name to @dev which forward to the clock @cont_name
>>>> in @container
>>>> + */
>>>
>>> 'container' seems odd terminology here, because I would expect
>>> the usual use of this function to be when a 'container' object
>>> like an SoC wants to forward a clock to one of its components;
>>> in that case the 'container' SoC would be @dev, wouldn't it?
>>
>> Yes. I agree it is confusing.
>> This function just allow a a device 'A' to exhibit a clock from another
>> device 'B' (typically a composing sub-device, inside a soc like you
>> said). The device A is not supposed to interact with the clock itself.
>> The original sub-device is the true
>> owner/controller of the clock and is the only one which should use the
>> clock API: setting a callback on it (input clock); or updating the clock
>> frequency (output clock).
>> Basically, it just add the clock into the device clock namespace without
>> interfering with it.
>>
>>> We should get this to be the same way round as qdev_pass_gpios(),
>>> which takes "DeviceState *dev, DeviceState *container", and
>>> passes the gpios that exist on 'dev' over to 'container' so that
>>> 'container' now has gpios which it did not before.
>>
>> Ok, I'll invert the arguments.
>>
>>>
>>> Also, your use of 'forward to' is inconsistent: in the 'dev'
>>> documentation you say we're forwarding the clock to 'dev',
>>> but in the body of the documentation you say we're forwarding
>>> the clock to the clock in 'container'.
>>
>> I'll try to clarify this and make the documentation more consistent with
>> the comments here.
>>
>>>
>>> I think the way to resolve this is to stick to the terminology
>>> in the function name itself:
>>> @dev: the device which has the clock
>>> @name: the name of the clock on @dev
>>> @container: the name of the device which the clock should
>>> be passed to
>>> @cont_name: the name to use for the clock on @container
>>
>> I think container is confusing because depending on how we reason (clock
>> in a device; device in another device), we end up thinking the opposite.
>>
>> Maybe we can use "exhibit" instead of "container" in the 2nd pair of
>> parameters, or prefix use "origin" or "owner" as a prefix for the first
>> pair of parameters.
>
> @sink vs @source?
>
>>> Q: if you pass a clock to another device with this function,
>>> does it still exist to be used directly on the original
>>> device? For qdev_pass_gpios it does not (I think), but
>>> this is more accident of implementation than anything else.
>>
>> It depends what we mean by "used by".
>> Original device can:
>> + set the callback in case it is an input
>> + update the frequency in case it is an output
>
> Hmm here you use @input vs @output...
Not really. What I meant here is that the device which originally
creates the clock is the only device which can interact with the clock.
And it will never gives this right to another device even if
qdev_pass_clock() is used.
There are 2 interactions, depending on the clock direction (input or
output). If it is an input clock, it can register a callback on
frequency changes; and if it is an output clock, it can updates the
frequency of the clock.
>
>> But since an input clock can only be connected once,
>> I think the logic here is that any connection should now go through the
>> new device. But this is not checked and using one or the other is
>> exactly the same.
>>
>> Maybe I misunderstood the meaning of qdev_pass_gpios().
> [...]
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 6/9] docs/clocks: add device's clock documentation
2019-12-02 15:17 ` Peter Maydell
@ 2019-12-04 12:11 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 12:11 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 4:17 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Add the documentation about the clock inputs and outputs in devices.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> docs/devel/clock.txt | 246 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 246 insertions(+)
>> create mode 100644 docs/devel/clock.txt
>
> Could you convert this to rst format, please?Yes.
>
>
>
>> +Changing a clock output
>> +=======================
>> +
>> +A device can change its outputs using the clock_set_frequency function. It
>> +will trigger updates on every connected inputs.
>
> "input"
>
>> +
>> +For example, let's say that we have an output clock "clkout" and we have a
>> +pointer to it in the device state because we did the following in init phase:
>> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
>> +
>> +Then at any time (apart from the cases listed below), it is possible to
>> +change the clock value by doing:
>> +clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Ghz */
>> +This operation must be done while holding the qemu io lock.
>> +
>> +One can change clocks only when it is allowed to have side effects on other
>> +objects. In consequence, it is forbidden:
>> ++ during migration,
>> ++ and in the init phase of reset.
>> +
>> +Forwarding clocks
>> +=================
>> +
>> +Sometimes, one needs to forward, or inherit, a clock from another device.
>> +Typically, when doing device composition, a device might expose a sub-device's
>> +clock without interfering with it.
>> +The function qdev_pass_clock() can be used to achieve this behaviour. Note, that
>
> "Note that"
>
>> +it is possible to expose the clock under a different name. This works for both
>> +inputs or outputs.
>
> "inputs and outputs"
>
>
>> +Migration
>> +=========
>> +
>> +Only the ClockIn object has a state. ClockOut is not concerned by migration.
>
> "has any state".
>
> "ClockOut has no state and does not need special handling for migration."
>
>> +
>> +In case the frequency of in input clock is needed for a device's migration,
>> +this state must be migrated.
>
> Are you trying to say that if an input clock is known to be a
> fixed frequency we don't need to migrate anything? I wonder
> if we need to worry about that or if we could/should just say that
> input clocks should always be migrated.
What I wanted to say is that there are indeed probably cases where
migrating the frequency is unnecessary. For example if we only use the
callback and never fetch the frequency outside it: if the frequency is
only used to compute something which is already saved/loaded during
migration.
But yes we could just do as you say. It's probably less confusing.
>
>> The VMSTATE_CLOCKIN macro defines an entry to
>> +be added in a vmstate description.
>> +
>> +For example, if a device has a clock input and the device state looks like:
>> +MyDeviceState {
>> + DeviceState parent_obj;
>> + ClockIn *clk;
>> +};
>> +
>> +Then, to add the clock frequency to the device's migrated state, the vmstate
>> +description is:
>> +VMStateDescription my_device_vmstate = {
>> + .name = "my_device",
>> + .fields = (VMStateField[]) {
>> + VMSTATE_CLOCKIN(clk, MyDeviceState),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +When adding a input clock support to an existing device, you must care about
>> +migration compatibility. To this end, you can use the clock_init_frequency in
>> +a pre_load function to setup a default value in case the source vm does not
>> +migrate the frequency.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2019-12-02 15:20 ` Peter Maydell
@ 2019-12-04 12:51 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 12:51 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 4:20 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Switch the slcr to multi-phase reset and add some clocks:
>> + the main input clock (ps_clk)
>> + the reference clock outputs for each uart (uart0 & 1)
>>
>> The clock frequencies are computed using the internal pll & uart configuration
>> registers and the ps_clk frequency.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>
> Review of this and the following two patches by some Xilinx
> person would be nice. I've just looked them over for general
> issues, and haven't checked against the hardware specs.
>
>> ---
>
>
>> +/*
>> + * return the output frequency of a clock given:
>> + * + the frequencies in an array corresponding to mux's indexes
>> + * + the register xxx_CLK_CTRL value
>> + * + enable bit index in ctrl register
>> + *
>> + * This function make the assumption that ctrl_reg value is organized as follow:
>
> "makes"; "that the"; "follows"
>
>> + * + bits[13:8] clock divisor
>> + * + bits[5:4] clock mux selector (index in array)
>> + * + bits[index] clock enable
>> + */
>> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
>> + uint32_t ctrl_reg,
>> + unsigned index)
>> +{
>> + uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
>> + uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
>> +
>> + /* first, check if clock is enabled */
>> + if (((ctrl_reg >> index) & 1u) == 0) {
>> + return 0;
>> + }
>> +
>> + /*
>> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
>> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
>> + * is [1;63].
>
> Is this the range notation the spec doc uses?
The exact terms is:
"The 6-bit divider provides a divide range of 1 to 63"
At the time, I checked also the kernel sources, and this is the behavior
implemented in the driver as well (1 based timer and allowing 0 special
value for bypass). The bypass is undocumented as far as I can tell.
>
>> + * So divide the source while avoiding division-by-zero.
>> + */
>> + return mux[srcsel] / (divisor ? divisor : 1u);
>> +}
>> +
>
>> +static const ClockPortInitArray zynq_slcr_clocks = {
>> + QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
>> + QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
>> + QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
>> + QDEV_CLOCK_END
>> +};
>> +
>> static void zynq_slcr_init(Object *obj)
>> {
>> ZynqSLCRState *s = ZYNQ_SLCR(obj);
>> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
>> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
>> ZYNQ_SLCR_MMIO_SIZE);
>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +
>> + qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
>> }
>>
>> static const VMStateDescription vmstate_zynq_slcr = {
>> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
>> static void zynq_slcr_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> + ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>> dc->vmsd = &vmstate_zynq_slcr;
>> - dc->reset = zynq_slcr_reset;
>> + rc->phases.init = zynq_slcr_reset_init;
>> + rc->phases.hold = zynq_slcr_reset_hold;
>> + rc->phases.exit = zynq_slcr_reset_exit;
>> }
>
> We're adding an input clock, so doesn't the migration
> state struct need to be updated to migrate it ?
Yes, we can. Although this input clock is really not expected to change.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support
2019-12-02 15:24 ` Peter Maydell
@ 2019-12-04 13:35 ` Damien Hedde
0 siblings, 0 replies; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 13:35 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 4:24 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Switch the cadence uart to multi-phase reset and add the
>> reference clock input.
>>
>> The input clock frequency is added to the migration structure.
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored.
>>
>> If this clock remains unconnected, the uart behaves as before
>> (it default to a 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>
>> static void uart_parameters_setup(CadenceUARTState *s)
>> {
>> QEMUSerialSetParams ssp;
>> - unsigned int baud_rate, packet_size;
>> + unsigned int baud_rate, packet_size, input_clk;
>> + input_clk = clock_get_frequency(s->refclk);
>>
>> - baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> - UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> + baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
>> + baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> + trace_cadence_uart_baudrate(baud_rate);
>> +
>> + ssp.speed = baud_rate;
>>
>> - ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> packet_size = 1;
>>
>> switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
>> }
>>
>> packet_size += ssp.data_bits + ssp.stop_bits;
>> + if (ssp.speed == 0) {
>> + /*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
>
> Any ideas what might be better? :-)
Well maybe the comment is misplaced. Because it is probably a good thing
to round up the ssp.speed in case it becomes 0 (which is very unlikely
apart from the case where the input clock is 0/disabled).
The problem is what should we do when the clock is disabled ?
Right now we:
+ set a minimal baudrate
+ ignore input characters/events
+ still forward output characters... (I just checked)
I suppose we could at least fix the last point: we can drop any output
characters. But if this happen, there is definitely a problem somewhere
(a firmware should not try to send characters to an unclocked uart). Is
there a qemu way of reporting this kind of situation ?
It would be best to somehow tell the backend we're not handling anything
anymore. So I could put that in the comment instead.
I really don't know if/how we can do that. When I looked I did not see
any way to do the opposite of qemu_chr_fe_accept_input() which is done
to start receiving stuff.
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-02 16:15 ` [PATCH v6 0/9] Clock framework API Peter Maydell
@ 2019-12-04 16:40 ` Damien Hedde
2019-12-04 20:34 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-12-04 16:40 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias, Philippe Mathieu-Daudé
On 12/2/19 5:15 PM, Peter Maydell wrote:
>
> The one topic I think we could do with discussing is whether
> a simple uint64_t giving the frequency of the clock in Hz is
> the right representation. In particular in your patch 9 the
> board has a clock frequency that's not a nice integer number
> of Hz. I think Philippe also mentioned on irc some board where
> the UART clock ends up at a weird frequency. Since the
> representation of the frequency is baked into the migration
> format it's going to be easier to get it right first rather
> than trying to change it later.
>
> So what should the representation be? Some random thoughts:
>
> 1) ptimer internally uses a 'period plus fraction' representation:
> int64_t period is the integer part of the period in nanoseconds,
> uint32_t period_frac is the fractional part of the period
> (if you like you can think of this as "96-bit integer
> period measured in units of one-2^32nd of a nanosecond").
> However its only public interfaces for setting the frequency
> are (a) set the frequency in Hz (uint32_t) or (b) set
> the period in nanoseconds (int64_t); the period_frac part
> is used to handle frequencies which don't work out to
> a nice whole number of nanoseconds per cycle.
>
> 2) I hear that SystemC uses "value plus a time unit", with
> the smallest unit being a picosecond. (I think SystemC
> also lets you specify the duty cycle, but we definitely
> don't want to get into that!)
The "value" is internally stored in a 64bits unsigned integer.
>
> 3) QEMUTimers are basically just nanosecond timers
>
> 4) The MAME emulator seems to work with periods of
> 96-bit attoseconds (represented internally by a
> 32-bit count of seconds plus a 64-bit count of
> attoseconds). One attosecond is 1e-18 seconds.
>
> Does anybody else have experience with other modelling
> or emulator technology and how it represents clocks ?
5) In linux, a clock rate is an "unsigned long" representing Hz.
>
> I feel we should at least be able to represent clocks
> with the same accuracy that ptimer has.
Then is a maybe a good idea to store the period and not the frequency in
clocks so that we don't loose anything when we switch from a clock to a
ptimer ?
Regarding the clock, I don't see any strong obstacle to switch
internally to a period based value.
The only things we have to choose is how to represent a disabled clock.
Since putting a "0" period to a ptimer will disable the timer in
ptimer_reload(). We can choose that (and it's a good value because we
can multiply or divide it, it stays the same).
We could use the same representation as a ptimer. But if we don't keep a
C number representation, then computation of frequencies/periods will be
complicated at best and error prone.
From that point of view, if we could stick to a 64bits integer (or
floating point number) it would be great. Can we use a sub nanosecond
unit that fit our needs ?
I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
the unit of the ptimer fractional part ?) and if I'm not mistaken
+ we have a frequency range from ~0.2Hz up to 10^18Hz
+ the resolution is decreasing with the frequency (but at 100Mhz we have
a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
resolution). We hit 1Hz resolution around 2GHz.
So it sounds to me we have largely enough resolution to model clocks in
the range of frequencies we will have to handle. What do you think ?
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-04 16:40 ` Damien Hedde
@ 2019-12-04 20:34 ` Philippe Mathieu-Daudé
2019-12-05 9:36 ` Damien Hedde
0 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-04 20:34 UTC (permalink / raw)
To: Damien Hedde, Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/4/19 5:40 PM, Damien Hedde wrote:
> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>
>> The one topic I think we could do with discussing is whether
>> a simple uint64_t giving the frequency of the clock in Hz is
>> the right representation. In particular in your patch 9 the
>> board has a clock frequency that's not a nice integer number
>> of Hz. I think Philippe also mentioned on irc some board where
>> the UART clock ends up at a weird frequency. Since the
>> representation of the frequency is baked into the migration
>> format it's going to be easier to get it right first rather
>> than trying to change it later.
Important precision for Damien, IIUC we can not migrate float/double types.
>> So what should the representation be? Some random thoughts:
>>
>> 1) ptimer internally uses a 'period plus fraction' representation:
>> int64_t period is the integer part of the period in nanoseconds,
>> uint32_t period_frac is the fractional part of the period
>> (if you like you can think of this as "96-bit integer
>> period measured in units of one-2^32nd of a nanosecond").
>> However its only public interfaces for setting the frequency
>> are (a) set the frequency in Hz (uint32_t) or (b) set
>> the period in nanoseconds (int64_t); the period_frac part
>> is used to handle frequencies which don't work out to
>> a nice whole number of nanoseconds per cycle.
This is very clear, thanks Peter!
The period+period_frac split allow us to migrate the 96 bits:
VMSTATE_UINT32(period_frac, ptimer_state),
VMSTATE_INT64(period, ptimer_state),
>> 2) I hear that SystemC uses "value plus a time unit", with
>> the smallest unit being a picosecond. (I think SystemC
>> also lets you specify the duty cycle, but we definitely
>> don't want to get into that!)
>
> The "value" is internally stored in a 64bits unsigned integer.
>
>>
>> 3) QEMUTimers are basically just nanosecond timers
Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
#define SCALE_MS 1000000
#define SCALE_US 1000
#define SCALE_NS 1
>>
>> 4) The MAME emulator seems to work with periods of
>> 96-bit attoseconds (represented internally by a
>> 32-bit count of seconds plus a 64-bit count of
>> attoseconds). One attosecond is 1e-18 seconds.
>>
>> Does anybody else have experience with other modelling
>> or emulator technology and how it represents clocks ?
>
> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>
>>
>> I feel we should at least be able to represent clocks
>> with the same accuracy that ptimer has.
>
> Then is a maybe a good idea to store the period and not the frequency in
> clocks so that we don't loose anything when we switch from a clock to a
> ptimer ?
I think storing the period as an integer type is a good idea.
However if we store the period in nanoseconds, we get at most 1GHz
frequency.
The attosecond granularity feels overkill.
If we use a 96-bit integer to store picoseconds and use similar SCALE
macros we get to 1THz.
Regardless the unit chosen, as long it is integer, we can migrate it.
If can migrate the period, we don't need to migrate the frequency.
We can then use the float type in with the timer API to pass frequencies
(which in the modeled hardware are ratios, likely not integers).
So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> Regarding the clock, I don't see any strong obstacle to switch
> internally to a period based value.
> The only things we have to choose is how to represent a disabled clock.
> Since putting a "0" period to a ptimer will disable the timer in
> ptimer_reload(). We can choose that (and it's a good value because we
> can multiply or divide it, it stays the same).
>
> We could use the same representation as a ptimer. But if we don't keep a
> C number representation, then computation of frequencies/periods will be
> complicated at best and error prone.
>
> From that point of view, if we could stick to a 64bits integer (or
> floating point number) it would be great. Can we use a sub nanosecond
> unit that fit our needs ?
>
> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> the unit of the ptimer fractional part ?) and if I'm not mistaken
> + we have a frequency range from ~0.2Hz up to 10^18Hz
> + the resolution is decreasing with the frequency (but at 100Mhz we have
> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> resolution). We hit 1Hz resolution around 2GHz.
>
> So it sounds to me we have largely enough resolution to model clocks in
> the range of frequencies we will have to handle. What do you think ?
Back to your series, I wonder why you want to store the frequency in
ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
What matters is ClockOut, and each device exposing ClockOuts has a
(migrated) state of the output frequencies (rather in fields, or encoded
in registers). Once migrated, after the state is loaded back into the
device, we call post_load(). Isn't it a good place to call
clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
frequency.
IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-04 20:34 ` Philippe Mathieu-Daudé
@ 2019-12-05 9:36 ` Damien Hedde
2019-12-05 9:59 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Damien Hedde @ 2019-12-05 9:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> On 12/4/19 5:40 PM, Damien Hedde wrote:
>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>
>>> The one topic I think we could do with discussing is whether
>>> a simple uint64_t giving the frequency of the clock in Hz is
>>> the right representation. In particular in your patch 9 the
>>> board has a clock frequency that's not a nice integer number
>>> of Hz. I think Philippe also mentioned on irc some board where
>>> the UART clock ends up at a weird frequency. Since the
>>> representation of the frequency is baked into the migration
>>> format it's going to be easier to get it right first rather
>>> than trying to change it later.
>
> Important precision for Damien, IIUC we can not migrate float/double types.
>
>>> So what should the representation be? Some random thoughts:
>>>
>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>> int64_t period is the integer part of the period in nanoseconds,
>>> uint32_t period_frac is the fractional part of the period
>>> (if you like you can think of this as "96-bit integer
>>> period measured in units of one-2^32nd of a nanosecond").
>>> However its only public interfaces for setting the frequency
>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>> the period in nanoseconds (int64_t); the period_frac part
>>> is used to handle frequencies which don't work out to
>>> a nice whole number of nanoseconds per cycle.
>
> This is very clear, thanks Peter!
>
> The period+period_frac split allow us to migrate the 96 bits:
>
> VMSTATE_UINT32(period_frac, ptimer_state),
> VMSTATE_INT64(period, ptimer_state),
>
>>> 2) I hear that SystemC uses "value plus a time unit", with
>>> the smallest unit being a picosecond. (I think SystemC
>>> also lets you specify the duty cycle, but we definitely
>>> don't want to get into that!)
>>
>> The "value" is internally stored in a 64bits unsigned integer.
>>
>>>
>>> 3) QEMUTimers are basically just nanosecond timers
>
> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>
> #define SCALE_MS 1000000
> #define SCALE_US 1000
> #define SCALE_NS 1
>
>>>
>>> 4) The MAME emulator seems to work with periods of
>>> 96-bit attoseconds (represented internally by a
>>> 32-bit count of seconds plus a 64-bit count of
>>> attoseconds). One attosecond is 1e-18 seconds.
>>>
>>> Does anybody else have experience with other modelling
>>> or emulator technology and how it represents clocks ?
>>
>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>
>>>
>>> I feel we should at least be able to represent clocks
>>> with the same accuracy that ptimer has.
>>
>> Then is a maybe a good idea to store the period and not the frequency in
>> clocks so that we don't loose anything when we switch from a clock to a
>> ptimer ?
>
> I think storing the period as an integer type is a good idea.
>
> However if we store the period in nanoseconds, we get at most 1GHz
> frequency.
>
> The attosecond granularity feels overkill.
>
> If we use a 96-bit integer to store picoseconds and use similar SCALE
> macros we get to 1THz.
>
> Regardless the unit chosen, as long it is integer, we can migrate it.
> If can migrate the period, we don't need to migrate the frequency.
> We can then use the float type in with the timer API to pass frequencies
> (which in the modeled hardware are ratios, likely not integers).
>
> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>
>> Regarding the clock, I don't see any strong obstacle to switch
>> internally to a period based value.
>> The only things we have to choose is how to represent a disabled clock.
>> Since putting a "0" period to a ptimer will disable the timer in
>> ptimer_reload(). We can choose that (and it's a good value because we
>> can multiply or divide it, it stays the same).
>>
>> We could use the same representation as a ptimer. But if we don't keep a
>> C number representation, then computation of frequencies/periods will be
>> complicated at best and error prone.
>>
>> From that point of view, if we could stick to a 64bits integer (or
>> floating point number) it would be great. Can we use a sub nanosecond
>> unit that fit our needs ?
>>
>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>> resolution). We hit 1Hz resolution around 2GHz.
>>
>> So it sounds to me we have largely enough resolution to model clocks in
>> the range of frequencies we will have to handle. What do you think ?
>
> Back to your series, I wonder why you want to store the frequency in
> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> What matters is ClockOut, and each device exposing ClockOuts has a
> (migrated) state of the output frequencies (rather in fields, or encoded
> in registers). Once migrated, after the state is loaded back into the
> device, we call post_load(). Isn't it a good place to call
> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> frequency.
>
> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>
I agree it is more logical to store the frequency in clock out. But,
regarding migration constraints, we have no choice I think because a
device cannot rely on values that are migrated by another device for
restoring its state. (when I checked, I add the impression that
post_load()s are called on a per device migration basis not all at the
end of migration).
So we could store the frequency in clock out and migrate things there.
But since we have no way to ensure all clock out states are migrated
before some device fetch a ClockIn: we'll have to say "don't fetch one
of your ClockIn frequency during migration and migrate the value
yourself if you need it", pretty much like gpios.
So we will probably migrate all ClockOut and almost all ClockIn.
It would nice if we had a way to ensure clocks are migrated before
devices try to use them. But I don't think this is possible.
--
Damien
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 9:36 ` Damien Hedde
@ 2019-12-05 9:59 ` Philippe Mathieu-Daudé
2019-12-05 10:21 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-05 9:59 UTC (permalink / raw)
To: Damien Hedde, Peter Maydell, Dr. David Alan Gilbert
Cc: Daniel P. Berrange, Eduardo Habkost, Alistair Francis,
Mark Burton, QEMU Developers, Marc-André Lureau, qemu-arm,
Paolo Bonzini, Edgar E. Iglesias
On 12/5/19 10:36 AM, Damien Hedde wrote:
> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>
>>>> The one topic I think we could do with discussing is whether
>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>> the right representation. In particular in your patch 9 the
>>>> board has a clock frequency that's not a nice integer number
>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>> the UART clock ends up at a weird frequency. Since the
>>>> representation of the frequency is baked into the migration
>>>> format it's going to be easier to get it right first rather
>>>> than trying to change it later.
>>
>> Important precision for Damien, IIUC we can not migrate float/double types.
>>
>>>> So what should the representation be? Some random thoughts:
>>>>
>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>> int64_t period is the integer part of the period in nanoseconds,
>>>> uint32_t period_frac is the fractional part of the period
>>>> (if you like you can think of this as "96-bit integer
>>>> period measured in units of one-2^32nd of a nanosecond").
>>>> However its only public interfaces for setting the frequency
>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>> the period in nanoseconds (int64_t); the period_frac part
>>>> is used to handle frequencies which don't work out to
>>>> a nice whole number of nanoseconds per cycle.
>>
>> This is very clear, thanks Peter!
>>
>> The period+period_frac split allow us to migrate the 96 bits:
>>
>> VMSTATE_UINT32(period_frac, ptimer_state),
>> VMSTATE_INT64(period, ptimer_state),
>>
>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>> the smallest unit being a picosecond. (I think SystemC
>>>> also lets you specify the duty cycle, but we definitely
>>>> don't want to get into that!)
>>>
>>> The "value" is internally stored in a 64bits unsigned integer.
>>>
>>>>
>>>> 3) QEMUTimers are basically just nanosecond timers
>>
>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>
>> #define SCALE_MS 1000000
>> #define SCALE_US 1000
>> #define SCALE_NS 1
>>
>>>>
>>>> 4) The MAME emulator seems to work with periods of
>>>> 96-bit attoseconds (represented internally by a
>>>> 32-bit count of seconds plus a 64-bit count of
>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>
>>>> Does anybody else have experience with other modelling
>>>> or emulator technology and how it represents clocks ?
>>>
>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>
>>>>
>>>> I feel we should at least be able to represent clocks
>>>> with the same accuracy that ptimer has.
>>>
>>> Then is a maybe a good idea to store the period and not the frequency in
>>> clocks so that we don't loose anything when we switch from a clock to a
>>> ptimer ?
>>
>> I think storing the period as an integer type is a good idea.
>>
>> However if we store the period in nanoseconds, we get at most 1GHz
>> frequency.
>>
>> The attosecond granularity feels overkill.
>>
>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>> macros we get to 1THz.
>>
>> Regardless the unit chosen, as long it is integer, we can migrate it.
>> If can migrate the period, we don't need to migrate the frequency.
>> We can then use the float type in with the timer API to pass frequencies
>> (which in the modeled hardware are ratios, likely not integers).
>>
>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>
>>> Regarding the clock, I don't see any strong obstacle to switch
>>> internally to a period based value.
>>> The only things we have to choose is how to represent a disabled clock.
>>> Since putting a "0" period to a ptimer will disable the timer in
>>> ptimer_reload(). We can choose that (and it's a good value because we
>>> can multiply or divide it, it stays the same).
>>>
>>> We could use the same representation as a ptimer. But if we don't keep a
>>> C number representation, then computation of frequencies/periods will be
>>> complicated at best and error prone.
>>>
>>> From that point of view, if we could stick to a 64bits integer (or
>>> floating point number) it would be great. Can we use a sub nanosecond
>>> unit that fit our needs ?
>>>
>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>> resolution). We hit 1Hz resolution around 2GHz.
>>>
>>> So it sounds to me we have largely enough resolution to model clocks in
>>> the range of frequencies we will have to handle. What do you think ?
>>
>> Back to your series, I wonder why you want to store the frequency in
>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>> What matters is ClockOut, and each device exposing ClockOuts has a
>> (migrated) state of the output frequencies (rather in fields, or encoded
>> in registers). Once migrated, after the state is loaded back into the
>> device, we call post_load(). Isn't it a good place to call
>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>> frequency.
>>
>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>
>
> I agree it is more logical to store the frequency in clock out. But,
> regarding migration constraints, we have no choice I think because a
> device cannot rely on values that are migrated by another device for
> restoring its state. (when I checked, I add the impression that
> post_load()s are called on a per device migration basis not all at the
> end of migration).
Cc'ing David to clear that out.
> So we could store the frequency in clock out and migrate things there.
> But since we have no way to ensure all clock out states are migrated
> before some device fetch a ClockIn: we'll have to say "don't fetch one
> of your ClockIn frequency during migration and migrate the value
> yourself if you need it", pretty much like gpios.
>
> So we will probably migrate all ClockOut and almost all ClockIn.
>
> It would nice if we had a way to ensure clocks are migrated before
> devices try to use them. But I don't think this is possible.
>
> --
> Damien
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 9:59 ` Philippe Mathieu-Daudé
@ 2019-12-05 10:21 ` Dr. David Alan Gilbert
2019-12-05 10:44 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-05 10:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Damien Hedde, Peter Maydell, Daniel P. Berrange, Eduardo Habkost,
Alistair Francis, Mark Burton, QEMU Developers, Paolo Bonzini,
qemu-arm, Marc-André Lureau, Edgar E. Iglesias
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 12/5/19 10:36 AM, Damien Hedde wrote:
> > On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> > > On 12/4/19 5:40 PM, Damien Hedde wrote:
> > > > On 12/2/19 5:15 PM, Peter Maydell wrote:
> > > > >
> > > > > The one topic I think we could do with discussing is whether
> > > > > a simple uint64_t giving the frequency of the clock in Hz is
> > > > > the right representation. In particular in your patch 9 the
> > > > > board has a clock frequency that's not a nice integer number
> > > > > of Hz. I think Philippe also mentioned on irc some board where
> > > > > the UART clock ends up at a weird frequency. Since the
> > > > > representation of the frequency is baked into the migration
> > > > > format it's going to be easier to get it right first rather
> > > > > than trying to change it later.
> > >
> > > Important precision for Damien, IIUC we can not migrate float/double types.
> > >
> > > > > So what should the representation be? Some random thoughts:
> > > > >
> > > > > 1) ptimer internally uses a 'period plus fraction' representation:
> > > > > int64_t period is the integer part of the period in nanoseconds,
> > > > > uint32_t period_frac is the fractional part of the period
> > > > > (if you like you can think of this as "96-bit integer
> > > > > period measured in units of one-2^32nd of a nanosecond").
> > > > > However its only public interfaces for setting the frequency
> > > > > are (a) set the frequency in Hz (uint32_t) or (b) set
> > > > > the period in nanoseconds (int64_t); the period_frac part
> > > > > is used to handle frequencies which don't work out to
> > > > > a nice whole number of nanoseconds per cycle.
> > >
> > > This is very clear, thanks Peter!
> > >
> > > The period+period_frac split allow us to migrate the 96 bits:
> > >
> > > VMSTATE_UINT32(period_frac, ptimer_state),
> > > VMSTATE_INT64(period, ptimer_state),
> > >
> > > > > 2) I hear that SystemC uses "value plus a time unit", with
> > > > > the smallest unit being a picosecond. (I think SystemC
> > > > > also lets you specify the duty cycle, but we definitely
> > > > > don't want to get into that!)
> > > >
> > > > The "value" is internally stored in a 64bits unsigned integer.
> > > >
> > > > >
> > > > > 3) QEMUTimers are basically just nanosecond timers
> > >
> > > Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
> > >
> > > #define SCALE_MS 1000000
> > > #define SCALE_US 1000
> > > #define SCALE_NS 1
> > >
> > > > >
> > > > > 4) The MAME emulator seems to work with periods of
> > > > > 96-bit attoseconds (represented internally by a
> > > > > 32-bit count of seconds plus a 64-bit count of
> > > > > attoseconds). One attosecond is 1e-18 seconds.
> > > > >
> > > > > Does anybody else have experience with other modelling
> > > > > or emulator technology and how it represents clocks ?
> > > >
> > > > 5) In linux, a clock rate is an "unsigned long" representing Hz.
> > > >
> > > > >
> > > > > I feel we should at least be able to represent clocks
> > > > > with the same accuracy that ptimer has.
> > > >
> > > > Then is a maybe a good idea to store the period and not the frequency in
> > > > clocks so that we don't loose anything when we switch from a clock to a
> > > > ptimer ?
> > >
> > > I think storing the period as an integer type is a good idea.
> > >
> > > However if we store the period in nanoseconds, we get at most 1GHz
> > > frequency.
> > >
> > > The attosecond granularity feels overkill.
> > >
> > > If we use a 96-bit integer to store picoseconds and use similar SCALE
> > > macros we get to 1THz.
> > >
> > > Regardless the unit chosen, as long it is integer, we can migrate it.
> > > If can migrate the period, we don't need to migrate the frequency.
> > > We can then use the float type in with the timer API to pass frequencies
> > > (which in the modeled hardware are ratios, likely not integers).
> > >
> > > So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> > >
> > > > Regarding the clock, I don't see any strong obstacle to switch
> > > > internally to a period based value.
> > > > The only things we have to choose is how to represent a disabled clock.
> > > > Since putting a "0" period to a ptimer will disable the timer in
> > > > ptimer_reload(). We can choose that (and it's a good value because we
> > > > can multiply or divide it, it stays the same).
> > > >
> > > > We could use the same representation as a ptimer. But if we don't keep a
> > > > C number representation, then computation of frequencies/periods will be
> > > > complicated at best and error prone.
> > > >
> > > > From that point of view, if we could stick to a 64bits integer (or
> > > > floating point number) it would be great. Can we use a sub nanosecond
> > > > unit that fit our needs ?
> > > >
> > > > I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> > > > the unit of the ptimer fractional part ?) and if I'm not mistaken
> > > > + we have a frequency range from ~0.2Hz up to 10^18Hz
> > > > + the resolution is decreasing with the frequency (but at 100Mhz we have
> > > > a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> > > > resolution). We hit 1Hz resolution around 2GHz.
> > > >
> > > > So it sounds to me we have largely enough resolution to model clocks in
> > > > the range of frequencies we will have to handle. What do you think ?
> > >
> > > Back to your series, I wonder why you want to store the frequency in
> > > ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> > > What matters is ClockOut, and each device exposing ClockOuts has a
> > > (migrated) state of the output frequencies (rather in fields, or encoded
> > > in registers). Once migrated, after the state is loaded back into the
> > > device, we call post_load(). Isn't it a good place to call
> > > clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> > > frequency.
> > >
> > > IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
> > >
> >
> > I agree it is more logical to store the frequency in clock out. But,
> > regarding migration constraints, we have no choice I think because a
> > device cannot rely on values that are migrated by another device for
> > restoring its state. (when I checked, I add the impression that
> > post_load()s are called on a per device migration basis not all at the
> > end of migration).
>
> Cc'ing David to clear that out.
That's pretty much right; the 'post_load' is called on a structure at the end
of loading the data for that structure.
You can do things at the end of migration; one way is to register a
vm change state handler (search for qemu_add_vm_change_state_handler)
and that means you get a kick when the VM starts running or a timer set
in virtual time (not wall clock time because that becomes sensitive
to the speed of the host).
Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
anywhere.
Dave
> > So we could store the frequency in clock out and migrate things there.
> > But since we have no way to ensure all clock out states are migrated
> > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > of your ClockIn frequency during migration and migrate the value
> > yourself if you need it", pretty much like gpios.
> >
> > So we will probably migrate all ClockOut and almost all ClockIn.
> >
> > It would nice if we had a way to ensure clocks are migrated before
> > devices try to use them. But I don't think this is possible.
> >
> > --
> > Damien
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 10:21 ` Dr. David Alan Gilbert
@ 2019-12-05 10:44 ` Philippe Mathieu-Daudé
2019-12-05 10:56 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-05 10:44 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Peter Maydell, Alex Bennée,
Richard Henderson
Cc: Damien Hedde, Daniel P. Berrange, Eduardo Habkost,
Alistair Francis, Mark Burton, QEMU Developers, Paolo Bonzini,
qemu-arm, Marc-André Lureau, Edgar E. Iglesias
On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> On 12/5/19 10:36 AM, Damien Hedde wrote:
>>> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>>>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>>>
>>>>>> The one topic I think we could do with discussing is whether
>>>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>>>> the right representation. In particular in your patch 9 the
>>>>>> board has a clock frequency that's not a nice integer number
>>>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>>>> the UART clock ends up at a weird frequency. Since the
>>>>>> representation of the frequency is baked into the migration
>>>>>> format it's going to be easier to get it right first rather
>>>>>> than trying to change it later.
>>>>
>>>> Important precision for Damien, IIUC we can not migrate float/double types.
>>>>
>>>>>> So what should the representation be? Some random thoughts:
>>>>>>
>>>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>>> int64_t period is the integer part of the period in nanoseconds,
>>>>>> uint32_t period_frac is the fractional part of the period
>>>>>> (if you like you can think of this as "96-bit integer
>>>>>> period measured in units of one-2^32nd of a nanosecond").
>>>>>> However its only public interfaces for setting the frequency
>>>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>>>> the period in nanoseconds (int64_t); the period_frac part
>>>>>> is used to handle frequencies which don't work out to
>>>>>> a nice whole number of nanoseconds per cycle.
>>>>
>>>> This is very clear, thanks Peter!
>>>>
>>>> The period+period_frac split allow us to migrate the 96 bits:
>>>>
>>>> VMSTATE_UINT32(period_frac, ptimer_state),
>>>> VMSTATE_INT64(period, ptimer_state),
>>>>
>>>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>>>> the smallest unit being a picosecond. (I think SystemC
>>>>>> also lets you specify the duty cycle, but we definitely
>>>>>> don't want to get into that!)
>>>>>
>>>>> The "value" is internally stored in a 64bits unsigned integer.
>>>>>
>>>>>>
>>>>>> 3) QEMUTimers are basically just nanosecond timers
>>>>
>>>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>>>
>>>> #define SCALE_MS 1000000
>>>> #define SCALE_US 1000
>>>> #define SCALE_NS 1
>>>>
>>>>>>
>>>>>> 4) The MAME emulator seems to work with periods of
>>>>>> 96-bit attoseconds (represented internally by a
>>>>>> 32-bit count of seconds plus a 64-bit count of
>>>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>>>
>>>>>> Does anybody else have experience with other modelling
>>>>>> or emulator technology and how it represents clocks ?
>>>>>
>>>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>>>
>>>>>>
>>>>>> I feel we should at least be able to represent clocks
>>>>>> with the same accuracy that ptimer has.
>>>>>
>>>>> Then is a maybe a good idea to store the period and not the frequency in
>>>>> clocks so that we don't loose anything when we switch from a clock to a
>>>>> ptimer ?
>>>>
>>>> I think storing the period as an integer type is a good idea.
>>>>
>>>> However if we store the period in nanoseconds, we get at most 1GHz
>>>> frequency.
>>>>
>>>> The attosecond granularity feels overkill.
>>>>
>>>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>>>> macros we get to 1THz.
>>>>
>>>> Regardless the unit chosen, as long it is integer, we can migrate it.
>>>> If can migrate the period, we don't need to migrate the frequency.
>>>> We can then use the float type in with the timer API to pass frequencies
>>>> (which in the modeled hardware are ratios, likely not integers).
>>>>
>>>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>>>
>>>>> Regarding the clock, I don't see any strong obstacle to switch
>>>>> internally to a period based value.
>>>>> The only things we have to choose is how to represent a disabled clock.
>>>>> Since putting a "0" period to a ptimer will disable the timer in
>>>>> ptimer_reload(). We can choose that (and it's a good value because we
>>>>> can multiply or divide it, it stays the same).
>>>>>
>>>>> We could use the same representation as a ptimer. But if we don't keep a
>>>>> C number representation, then computation of frequencies/periods will be
>>>>> complicated at best and error prone.
>>>>>
>>>>> From that point of view, if we could stick to a 64bits integer (or
>>>>> floating point number) it would be great. Can we use a sub nanosecond
>>>>> unit that fit our needs ?
>>>>>
>>>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>>>> resolution). We hit 1Hz resolution around 2GHz.
>>>>>
>>>>> So it sounds to me we have largely enough resolution to model clocks in
>>>>> the range of frequencies we will have to handle. What do you think ?
>>>>
>>>> Back to your series, I wonder why you want to store the frequency in
>>>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>>>> What matters is ClockOut, and each device exposing ClockOuts has a
>>>> (migrated) state of the output frequencies (rather in fields, or encoded
>>>> in registers). Once migrated, after the state is loaded back into the
>>>> device, we call post_load(). Isn't it a good place to call
>>>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>>>> frequency.
>>>>
>>>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>>>
>>>
>>> I agree it is more logical to store the frequency in clock out. But,
>>> regarding migration constraints, we have no choice I think because a
>>> device cannot rely on values that are migrated by another device for
>>> restoring its state. (when I checked, I add the impression that
>>> post_load()s are called on a per device migration basis not all at the
>>> end of migration).
>>
>> Cc'ing David to clear that out.
>
>
> That's pretty much right; the 'post_load' is called on a structure at the end
> of loading the data for that structure.
>
> You can do things at the end of migration; one way is to register a
> vm change state handler (search for qemu_add_vm_change_state_handler)
> and that means you get a kick when the VM starts running or a timer set
> in virtual time (not wall clock time because that becomes sensitive
> to the speed of the host).
>
> Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
> true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
> anywhere.
OK, Cc'ing Alex & Richard now, because I guess remember a discussion
about "we can not migrate floats because this is a architectural
implementation, so cross-architecture migration is likely to fail". But
I can't find trace of a such discussion on the list or IRC logs.
Maybe this was instead about whether we can use the host FPU registers.
I hope I'm wrong and confuse, this is a great news for me to know we
can migrate floats!
> Dave
>
>>> So we could store the frequency in clock out and migrate things there.
>>> But since we have no way to ensure all clock out states are migrated
>>> before some device fetch a ClockIn: we'll have to say "don't fetch one
>>> of your ClockIn frequency during migration and migrate the value
>>> yourself if you need it", pretty much like gpios.
>>>
>>> So we will probably migrate all ClockOut and almost all ClockIn.
>>>
>>> It would nice if we had a way to ensure clocks are migrated before
>>> devices try to use them. But I don't think this is possible.
>>>
>>> --
>>> Damien
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 10:44 ` Philippe Mathieu-Daudé
@ 2019-12-05 10:56 ` Dr. David Alan Gilbert
2019-12-05 11:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 47+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-05 10:56 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Damien Hedde, Peter Maydell, Daniel P. Berrange, Eduardo Habkost,
Mark Burton, Alistair Francis, Richard Henderson,
QEMU Developers, Paolo Bonzini, qemu-arm, Marc-André Lureau,
Edgar E. Iglesias, Alex Bennée
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> > > On 12/5/19 10:36 AM, Damien Hedde wrote:
> > > > On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> > > > > On 12/4/19 5:40 PM, Damien Hedde wrote:
> > > > > > On 12/2/19 5:15 PM, Peter Maydell wrote:
> > > > > > >
> > > > > > > The one topic I think we could do with discussing is whether
> > > > > > > a simple uint64_t giving the frequency of the clock in Hz is
> > > > > > > the right representation. In particular in your patch 9 the
> > > > > > > board has a clock frequency that's not a nice integer number
> > > > > > > of Hz. I think Philippe also mentioned on irc some board where
> > > > > > > the UART clock ends up at a weird frequency. Since the
> > > > > > > representation of the frequency is baked into the migration
> > > > > > > format it's going to be easier to get it right first rather
> > > > > > > than trying to change it later.
> > > > >
> > > > > Important precision for Damien, IIUC we can not migrate float/double types.
> > > > >
> > > > > > > So what should the representation be? Some random thoughts:
> > > > > > >
> > > > > > > 1) ptimer internally uses a 'period plus fraction' representation:
> > > > > > > int64_t period is the integer part of the period in nanoseconds,
> > > > > > > uint32_t period_frac is the fractional part of the period
> > > > > > > (if you like you can think of this as "96-bit integer
> > > > > > > period measured in units of one-2^32nd of a nanosecond").
> > > > > > > However its only public interfaces for setting the frequency
> > > > > > > are (a) set the frequency in Hz (uint32_t) or (b) set
> > > > > > > the period in nanoseconds (int64_t); the period_frac part
> > > > > > > is used to handle frequencies which don't work out to
> > > > > > > a nice whole number of nanoseconds per cycle.
> > > > >
> > > > > This is very clear, thanks Peter!
> > > > >
> > > > > The period+period_frac split allow us to migrate the 96 bits:
> > > > >
> > > > > VMSTATE_UINT32(period_frac, ptimer_state),
> > > > > VMSTATE_INT64(period, ptimer_state),
> > > > >
> > > > > > > 2) I hear that SystemC uses "value plus a time unit", with
> > > > > > > the smallest unit being a picosecond. (I think SystemC
> > > > > > > also lets you specify the duty cycle, but we definitely
> > > > > > > don't want to get into that!)
> > > > > >
> > > > > > The "value" is internally stored in a 64bits unsigned integer.
> > > > > >
> > > > > > >
> > > > > > > 3) QEMUTimers are basically just nanosecond timers
> > > > >
> > > > > Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
> > > > >
> > > > > #define SCALE_MS 1000000
> > > > > #define SCALE_US 1000
> > > > > #define SCALE_NS 1
> > > > >
> > > > > > >
> > > > > > > 4) The MAME emulator seems to work with periods of
> > > > > > > 96-bit attoseconds (represented internally by a
> > > > > > > 32-bit count of seconds plus a 64-bit count of
> > > > > > > attoseconds). One attosecond is 1e-18 seconds.
> > > > > > >
> > > > > > > Does anybody else have experience with other modelling
> > > > > > > or emulator technology and how it represents clocks ?
> > > > > >
> > > > > > 5) In linux, a clock rate is an "unsigned long" representing Hz.
> > > > > >
> > > > > > >
> > > > > > > I feel we should at least be able to represent clocks
> > > > > > > with the same accuracy that ptimer has.
> > > > > >
> > > > > > Then is a maybe a good idea to store the period and not the frequency in
> > > > > > clocks so that we don't loose anything when we switch from a clock to a
> > > > > > ptimer ?
> > > > >
> > > > > I think storing the period as an integer type is a good idea.
> > > > >
> > > > > However if we store the period in nanoseconds, we get at most 1GHz
> > > > > frequency.
> > > > >
> > > > > The attosecond granularity feels overkill.
> > > > >
> > > > > If we use a 96-bit integer to store picoseconds and use similar SCALE
> > > > > macros we get to 1THz.
> > > > >
> > > > > Regardless the unit chosen, as long it is integer, we can migrate it.
> > > > > If can migrate the period, we don't need to migrate the frequency.
> > > > > We can then use the float type in with the timer API to pass frequencies
> > > > > (which in the modeled hardware are ratios, likely not integers).
> > > > >
> > > > > So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> > > > >
> > > > > > Regarding the clock, I don't see any strong obstacle to switch
> > > > > > internally to a period based value.
> > > > > > The only things we have to choose is how to represent a disabled clock.
> > > > > > Since putting a "0" period to a ptimer will disable the timer in
> > > > > > ptimer_reload(). We can choose that (and it's a good value because we
> > > > > > can multiply or divide it, it stays the same).
> > > > > >
> > > > > > We could use the same representation as a ptimer. But if we don't keep a
> > > > > > C number representation, then computation of frequencies/periods will be
> > > > > > complicated at best and error prone.
> > > > > >
> > > > > > From that point of view, if we could stick to a 64bits integer (or
> > > > > > floating point number) it would be great. Can we use a sub nanosecond
> > > > > > unit that fit our needs ?
> > > > > >
> > > > > > I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> > > > > > the unit of the ptimer fractional part ?) and if I'm not mistaken
> > > > > > + we have a frequency range from ~0.2Hz up to 10^18Hz
> > > > > > + the resolution is decreasing with the frequency (but at 100Mhz we have
> > > > > > a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> > > > > > resolution). We hit 1Hz resolution around 2GHz.
> > > > > >
> > > > > > So it sounds to me we have largely enough resolution to model clocks in
> > > > > > the range of frequencies we will have to handle. What do you think ?
> > > > >
> > > > > Back to your series, I wonder why you want to store the frequency in
> > > > > ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> > > > > What matters is ClockOut, and each device exposing ClockOuts has a
> > > > > (migrated) state of the output frequencies (rather in fields, or encoded
> > > > > in registers). Once migrated, after the state is loaded back into the
> > > > > device, we call post_load(). Isn't it a good place to call
> > > > > clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> > > > > frequency.
> > > > >
> > > > > IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
> > > > >
> > > >
> > > > I agree it is more logical to store the frequency in clock out. But,
> > > > regarding migration constraints, we have no choice I think because a
> > > > device cannot rely on values that are migrated by another device for
> > > > restoring its state. (when I checked, I add the impression that
> > > > post_load()s are called on a per device migration basis not all at the
> > > > end of migration).
> > >
> > > Cc'ing David to clear that out.
> >
> >
> > That's pretty much right; the 'post_load' is called on a structure at the end
> > of loading the data for that structure.
> >
> > You can do things at the end of migration; one way is to register a
> > vm change state handler (search for qemu_add_vm_change_state_handler)
> > and that means you get a kick when the VM starts running or a timer set
> > in virtual time (not wall clock time because that becomes sensitive
> > to the speed of the host).
> >
> > Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
> > true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
> > anywhere.
>
> OK, Cc'ing Alex & Richard now, because I guess remember a discussion about
> "we can not migrate floats because this is a architectural implementation,
> so cross-architecture migration is likely to fail". But I can't find trace
> of a such discussion on the list or IRC logs.
> Maybe this was instead about whether we can use the host FPU registers.
We have to be careful when migrating the FP registers within a CPU,
since they can have crazy values that are not valid/weird corners of
standard FP encodings (e.g. if the guest just uses the FP registers as
a spare 64bit register - which is perfectly valid on some
architectures). However, migrating an actual floating point
real world measurement should be fine. I'm assuming we can rely on
64 bit IEEE FP format on the wire being portable.
Dave
> I hope I'm wrong and confuse, this is a great news for me to know we
> can migrate floats!
>
> > Dave
> >
> > > > So we could store the frequency in clock out and migrate things there.
> > > > But since we have no way to ensure all clock out states are migrated
> > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > of your ClockIn frequency during migration and migrate the value
> > > > yourself if you need it", pretty much like gpios.
> > > >
> > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > >
> > > > It would nice if we had a way to ensure clocks are migrated before
> > > > devices try to use them. But I don't think this is possible.
> > > >
> > > > --
> > > > Damien
> > > >
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 10:56 ` Dr. David Alan Gilbert
@ 2019-12-05 11:01 ` Philippe Mathieu-Daudé
2019-12-06 12:46 ` Cleber Rosa
0 siblings, 1 reply; 47+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-05 11:01 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Cleber Rosa
Cc: Damien Hedde, Peter Maydell, Daniel P. Berrange, Eduardo Habkost,
Mark Burton, Alistair Francis, Richard Henderson,
QEMU Developers, Paolo Bonzini, qemu-arm, Marc-André Lureau,
Edgar E. Iglesias, Alex Bennée
On 12/5/19 11:56 AM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
>>> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>>>> On 12/5/19 10:36 AM, Damien Hedde wrote:
>>>>> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>>>>>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>>>>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>>>>>
>>>>>>>> The one topic I think we could do with discussing is whether
>>>>>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>>>>>> the right representation. In particular in your patch 9 the
>>>>>>>> board has a clock frequency that's not a nice integer number
>>>>>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>>>>>> the UART clock ends up at a weird frequency. Since the
>>>>>>>> representation of the frequency is baked into the migration
>>>>>>>> format it's going to be easier to get it right first rather
>>>>>>>> than trying to change it later.
>>>>>>
>>>>>> Important precision for Damien, IIUC we can not migrate float/double types.
>>>>>>
>>>>>>>> So what should the representation be? Some random thoughts:
>>>>>>>>
>>>>>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>>>>> int64_t period is the integer part of the period in nanoseconds,
>>>>>>>> uint32_t period_frac is the fractional part of the period
>>>>>>>> (if you like you can think of this as "96-bit integer
>>>>>>>> period measured in units of one-2^32nd of a nanosecond").
>>>>>>>> However its only public interfaces for setting the frequency
>>>>>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>>>>>> the period in nanoseconds (int64_t); the period_frac part
>>>>>>>> is used to handle frequencies which don't work out to
>>>>>>>> a nice whole number of nanoseconds per cycle.
>>>>>>
>>>>>> This is very clear, thanks Peter!
>>>>>>
>>>>>> The period+period_frac split allow us to migrate the 96 bits:
>>>>>>
>>>>>> VMSTATE_UINT32(period_frac, ptimer_state),
>>>>>> VMSTATE_INT64(period, ptimer_state),
>>>>>>
>>>>>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>>>>>> the smallest unit being a picosecond. (I think SystemC
>>>>>>>> also lets you specify the duty cycle, but we definitely
>>>>>>>> don't want to get into that!)
>>>>>>>
>>>>>>> The "value" is internally stored in a 64bits unsigned integer.
>>>>>>>
>>>>>>>>
>>>>>>>> 3) QEMUTimers are basically just nanosecond timers
>>>>>>
>>>>>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>>>>>
>>>>>> #define SCALE_MS 1000000
>>>>>> #define SCALE_US 1000
>>>>>> #define SCALE_NS 1
>>>>>>
>>>>>>>>
>>>>>>>> 4) The MAME emulator seems to work with periods of
>>>>>>>> 96-bit attoseconds (represented internally by a
>>>>>>>> 32-bit count of seconds plus a 64-bit count of
>>>>>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>>>>>
>>>>>>>> Does anybody else have experience with other modelling
>>>>>>>> or emulator technology and how it represents clocks ?
>>>>>>>
>>>>>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>>>>>
>>>>>>>>
>>>>>>>> I feel we should at least be able to represent clocks
>>>>>>>> with the same accuracy that ptimer has.
>>>>>>>
>>>>>>> Then is a maybe a good idea to store the period and not the frequency in
>>>>>>> clocks so that we don't loose anything when we switch from a clock to a
>>>>>>> ptimer ?
>>>>>>
>>>>>> I think storing the period as an integer type is a good idea.
>>>>>>
>>>>>> However if we store the period in nanoseconds, we get at most 1GHz
>>>>>> frequency.
>>>>>>
>>>>>> The attosecond granularity feels overkill.
>>>>>>
>>>>>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>>>>>> macros we get to 1THz.
>>>>>>
>>>>>> Regardless the unit chosen, as long it is integer, we can migrate it.
>>>>>> If can migrate the period, we don't need to migrate the frequency.
>>>>>> We can then use the float type in with the timer API to pass frequencies
>>>>>> (which in the modeled hardware are ratios, likely not integers).
>>>>>>
>>>>>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>>>>>
>>>>>>> Regarding the clock, I don't see any strong obstacle to switch
>>>>>>> internally to a period based value.
>>>>>>> The only things we have to choose is how to represent a disabled clock.
>>>>>>> Since putting a "0" period to a ptimer will disable the timer in
>>>>>>> ptimer_reload(). We can choose that (and it's a good value because we
>>>>>>> can multiply or divide it, it stays the same).
>>>>>>>
>>>>>>> We could use the same representation as a ptimer. But if we don't keep a
>>>>>>> C number representation, then computation of frequencies/periods will be
>>>>>>> complicated at best and error prone.
>>>>>>>
>>>>>>> From that point of view, if we could stick to a 64bits integer (or
>>>>>>> floating point number) it would be great. Can we use a sub nanosecond
>>>>>>> unit that fit our needs ?
>>>>>>>
>>>>>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>>>>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>>>>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>>>>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>>>>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>>>>>> resolution). We hit 1Hz resolution around 2GHz.
>>>>>>>
>>>>>>> So it sounds to me we have largely enough resolution to model clocks in
>>>>>>> the range of frequencies we will have to handle. What do you think ?
>>>>>>
>>>>>> Back to your series, I wonder why you want to store the frequency in
>>>>>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>>>>>> What matters is ClockOut, and each device exposing ClockOuts has a
>>>>>> (migrated) state of the output frequencies (rather in fields, or encoded
>>>>>> in registers). Once migrated, after the state is loaded back into the
>>>>>> device, we call post_load(). Isn't it a good place to call
>>>>>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>>>>>> frequency.
>>>>>>
>>>>>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>>>>>
>>>>>
>>>>> I agree it is more logical to store the frequency in clock out. But,
>>>>> regarding migration constraints, we have no choice I think because a
>>>>> device cannot rely on values that are migrated by another device for
>>>>> restoring its state. (when I checked, I add the impression that
>>>>> post_load()s are called on a per device migration basis not all at the
>>>>> end of migration).
>>>>
>>>> Cc'ing David to clear that out.
>>>
>>>
>>> That's pretty much right; the 'post_load' is called on a structure at the end
>>> of loading the data for that structure.
>>>
>>> You can do things at the end of migration; one way is to register a
>>> vm change state handler (search for qemu_add_vm_change_state_handler)
>>> and that means you get a kick when the VM starts running or a timer set
>>> in virtual time (not wall clock time because that becomes sensitive
>>> to the speed of the host).
>>>
>>> Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
>>> true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
>>> anywhere.
>>
>> OK, Cc'ing Alex & Richard now, because I guess remember a discussion about
>> "we can not migrate floats because this is a architectural implementation,
>> so cross-architecture migration is likely to fail". But I can't find trace
>> of a such discussion on the list or IRC logs.
>> Maybe this was instead about whether we can use the host FPU registers.
>
> We have to be careful when migrating the FP registers within a CPU,
> since they can have crazy values that are not valid/weird corners of
> standard FP encodings (e.g. if the guest just uses the FP registers as
> a spare 64bit register - which is perfectly valid on some
> architectures). However, migrating an actual floating point
> real world measurement should be fine. I'm assuming we can rely on
> 64 bit IEEE FP format on the wire being portable.
Understood, thanks for clearing this out!
Side note, we don't do cross-arch migration testing, but we talked about
having a 'different QEMU version' migration test. When we get a such
test setup, it shouldn't be too difficult to evolve to some cross-arch
migration test.
>> I hope I'm wrong and confuse, this is a great news for me to know we
>> can migrate floats!
>>
>>> Dave
>>>
>>>>> So we could store the frequency in clock out and migrate things there.
>>>>> But since we have no way to ensure all clock out states are migrated
>>>>> before some device fetch a ClockIn: we'll have to say "don't fetch one
>>>>> of your ClockIn frequency during migration and migrate the value
>>>>> yourself if you need it", pretty much like gpios.
>>>>>
>>>>> So we will probably migrate all ClockOut and almost all ClockIn.
>>>>>
>>>>> It would nice if we had a way to ensure clocks are migrated before
>>>>> devices try to use them. But I don't think this is possible.
>>>>>
>>>>> --
>>>>> Damien
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-05 11:01 ` Philippe Mathieu-Daudé
@ 2019-12-06 12:46 ` Cleber Rosa
2019-12-06 13:48 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 47+ messages in thread
From: Cleber Rosa @ 2019-12-06 12:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Damien Hedde, Peter Maydell, Daniel P. Berrange, Eduardo Habkost,
Mark Burton, Alistair Francis, Richard Henderson,
Dr. David Alan Gilbert, QEMU Developers, Paolo Bonzini, qemu-arm,
Marc-André Lureau, Edgar E. Iglesias, Oksana Voshchana,
Alex Bennée
On Thu, Dec 05, 2019 at 12:01:56PM +0100, Philippe Mathieu-Daudé wrote:
>
> Understood, thanks for clearing this out!
>
> Side note, we don't do cross-arch migration testing, but we talked about
> having a 'different QEMU version' migration test. When we get a such test
> setup, it shouldn't be too difficult to evolve to some cross-arch migration
> test.
>
+Oksana,
Avocado-VT has had this as a core concept as far as I can remember, and
it's exposed even as a command line argument:
avocado run --help
...
[--vt-qemu-dst-bin VT_DST_QEMU_BIN]
...
Oksana is currently working on new migration test cases, and may consider
looking into adding "different QEMU version" support too.
PS: I have to say, though, that I'm trying to get my mind around
cross-arch migration being real.
- Cleber.
> > > I hope I'm wrong and confuse, this is a great news for me to know we
> > > can migrate floats!
> > >
> > > > Dave
> > > >
> > > > > > So we could store the frequency in clock out and migrate things there.
> > > > > > But since we have no way to ensure all clock out states are migrated
> > > > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > > > of your ClockIn frequency during migration and migrate the value
> > > > > > yourself if you need it", pretty much like gpios.
> > > > > >
> > > > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > > > >
> > > > > > It would nice if we had a way to ensure clocks are migrated before
> > > > > > devices try to use them. But I don't think this is possible.
> > > > > >
> > > > > > --
> > > > > > Damien
> > > > > >
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > >
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v6 0/9] Clock framework API
2019-12-06 12:46 ` Cleber Rosa
@ 2019-12-06 13:48 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 47+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-06 13:48 UTC (permalink / raw)
To: Cleber Rosa
Cc: Damien Hedde, Peter Maydell, Daniel P. Berrange, Eduardo Habkost,
Mark Burton, Philippe Mathieu-Daudé,
Alistair Francis, Richard Henderson, QEMU Developers,
Paolo Bonzini, qemu-arm, Marc-André Lureau,
Edgar E. Iglesias, Oksana Voshchana, Alex Bennée
* Cleber Rosa (crosa@redhat.com) wrote:
> On Thu, Dec 05, 2019 at 12:01:56PM +0100, Philippe Mathieu-Daudé wrote:
> >
> > Understood, thanks for clearing this out!
> >
> > Side note, we don't do cross-arch migration testing, but we talked about
> > having a 'different QEMU version' migration test. When we get a such test
> > setup, it shouldn't be too difficult to evolve to some cross-arch migration
> > test.
> >
>
> +Oksana,
>
> Avocado-VT has had this as a core concept as far as I can remember, and
> it's exposed even as a command line argument:
>
> avocado run --help
> ...
> [--vt-qemu-dst-bin VT_DST_QEMU_BIN]
> ...
>
Yeh, I've run that in the past - it works OK.
Dave
> Oksana is currently working on new migration test cases, and may consider
> looking into adding "different QEMU version" support too.
>
> PS: I have to say, though, that I'm trying to get my mind around
> cross-arch migration being real.
>
> - Cleber.
>
> > > > I hope I'm wrong and confuse, this is a great news for me to know we
> > > > can migrate floats!
> > > >
> > > > > Dave
> > > > >
> > > > > > > So we could store the frequency in clock out and migrate things there.
> > > > > > > But since we have no way to ensure all clock out states are migrated
> > > > > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > > > > of your ClockIn frequency during migration and migrate the value
> > > > > > > yourself if you need it", pretty much like gpios.
> > > > > > >
> > > > > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > > > > >
> > > > > > > It would nice if we had a way to ensure clocks are migrated before
> > > > > > > devices try to use them. But I don't think this is possible.
> > > > > > >
> > > > > > > --
> > > > > > > Damien
> > > > > > >
> > > > > >
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > >
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> >
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree
2019-09-04 9:38 [Qemu-devel] " damien.hedde
@ 2019-09-04 9:38 ` damien.hedde
0 siblings, 0 replies; 47+ messages in thread
From: damien.hedde @ 2019-09-04 9:38 UTC (permalink / raw)
To: qemu-devel
Cc: Damien Hedde, peter.maydell, berrange, ehabkost, alistair,
mark.burton, pbonzini, qemu-arm, marcandre.lureau,
edgar.iglesias, philmd
From: Damien Hedde <damien.hedde@greensocs.com>
This prints the clocks attached to a DeviceState when using "info qtree" monitor
command. For every clock, it displays the direction, the name and if the
clock is forwarded. For input clock, it displays also the frequency.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
qdev-monitor.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index a0003bf2a9..d5b8be956b 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -19,6 +19,7 @@
#include "qemu/osdep.h"
#include "hw/sysbus.h"
+#include "hw/clock.h"
#include "monitor/monitor.h"
#include "monitor/qdev.h"
#include "sysemu/arch_init.h"
@@ -689,6 +690,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ObjectClass *class;
BusState *child;
NamedGPIOList *ngl;
+ NamedClockList *clk;
qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
dev->id ? dev->id : "");
@@ -703,6 +705,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ngl->num_out);
}
}
+ QLIST_FOREACH(clk, &dev->clocks, node) {
+ if (clk->out) {
+ qdev_printf("clock-out%s \"%s\"\n",
+ clk->forward ? " (fw)" : "",
+ clk->name);
+ } else {
+ qdev_printf("clock-in%s \"%s\" freq_hz=%" PRIu64"\n",
+ clk->forward ? " (fw)" : "",
+ clk->name, clock_get_frequency(clk->in));
+ }
+ }
class = object_get_class(OBJECT(dev));
do {
qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
--
2.22.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
end of thread, other threads:[~2019-12-06 15:51 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 12:55 [Qemu-devel] [PATCH v6 0/9] Clock framework API Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
2019-11-25 13:07 ` Philippe Mathieu-Daudé
2019-11-25 13:37 ` Philippe Mathieu-Daudé
2019-12-03 15:14 ` Damien Hedde
2019-12-02 13:42 ` Peter Maydell
2019-12-03 15:28 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
2019-11-25 13:05 ` Philippe Mathieu-Daudé
2019-12-02 13:44 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
2019-11-25 13:30 ` Philippe Mathieu-Daudé
2019-12-03 15:35 ` Damien Hedde
2019-12-02 14:34 ` Peter Maydell
2019-12-04 9:05 ` Damien Hedde
2019-12-04 9:53 ` Philippe Mathieu-Daudé
2019-12-04 11:58 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
2019-12-02 14:35 ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
2019-12-02 15:13 ` Peter Maydell
2019-12-04 11:04 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation Damien Hedde
2019-12-02 15:17 ` Peter Maydell
2019-12-04 12:11 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
2019-12-02 15:20 ` Peter Maydell
2019-12-04 12:51 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support Damien Hedde
2019-12-02 15:24 ` Peter Maydell
2019-12-04 13:35 ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
2019-12-02 15:34 ` Peter Maydell
2019-12-03 14:59 ` Damien Hedde
2019-12-03 15:29 ` Philippe Mathieu-Daudé
2019-12-02 16:15 ` [PATCH v6 0/9] Clock framework API Peter Maydell
2019-12-04 16:40 ` Damien Hedde
2019-12-04 20:34 ` Philippe Mathieu-Daudé
2019-12-05 9:36 ` Damien Hedde
2019-12-05 9:59 ` Philippe Mathieu-Daudé
2019-12-05 10:21 ` Dr. David Alan Gilbert
2019-12-05 10:44 ` Philippe Mathieu-Daudé
2019-12-05 10:56 ` Dr. David Alan Gilbert
2019-12-05 11:01 ` Philippe Mathieu-Daudé
2019-12-06 12:46 ` Cleber Rosa
2019-12-06 13:48 ` Dr. David Alan Gilbert
-- strict thread matches above, loose matches on Subject: below --
2019-09-04 9:38 [Qemu-devel] " damien.hedde
2019-09-04 9:38 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree damien.hedde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).