[PATCH v7 0/9] Clock framework API

[PATCH v7 0/9] Clock framework API

[Damien Hedde]

This series aims to add a way to model clock distribution in qemu.
The proposed objet and qdev API 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.

There's 2 notable changes since v6:
+ The value stored in clock is now a period on 64 bits integer. The
  unit is 2^-32ns to achieve the same precision as the ptimer internal
  representation.
+ No more ClockIn/ClockOut base types, just a single Clock type. It
  makes things simplier for everything else (standalone clocks,
  possibility to read device's output clocks) and allow to chain
  several clock inputs.

Regarding the internal represention. The precision is huge so that
it is possible (in the future) to somehow connect a ptimer with a
Clock with no loss of precision.
The consequence is that we have a ~4seconds period upper bound only.
Alternatives, allowing us to keep this precision, are to use a
floating point or to extend the integer.

The added clock api is very similar the the GPIO API for devices. We
can add input and output and connect them together.

Now that ressettable API is merged, the clock tree is properly
initialized during the machine reset.
I've tested this patchset running Xilinx's Linux on the xilinx-zynq-a9
machine. 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 suggestions are welcomed.

Patches 1, 3 and 5 to 8 still need some reviews.

The patches are organised as follows:
+ Patches 1 to 4 adds the clock support in qemu
+ Patch 5 adds some documentation in docs/devel
+ Patches 6 to 8 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.
+ Patch 9 adds clock info to monitor "info qtree" command

Changes since v6:
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00599.html
- switch from frequency to period based clock state
 - single Clock type (no more ClockIn and ClockOut)
 - doc converted in rst format (Peter)
 - various fixes (Peter and Philippe)
 - better migration support for zynq devices (Peter)

Thanks to the Xilinx QEMU team who sponsored this development.

Damien Hedde (9):
  hw/core/clock: introduce clock object
  hw/core/clock-vmstate: define a vmstate entry for clock state
  qdev: add clock input&output support to devices.
  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
  qdev-monitor: print the device's clock with info qtree

 docs/devel/clocks.rst          | 356 +++++++++++++++++++++++++++++++++
 docs/devel/index.rst           |   1 +
 include/hw/char/cadence_uart.h |   1 +
 include/hw/clock.h             | 225 +++++++++++++++++++++
 include/hw/qdev-clock.h        | 160 +++++++++++++++
 include/hw/qdev-core.h         |  12 ++
 hw/arm/xilinx_zynq.c           |  57 +++++-
 hw/char/cadence_uart.c         |  73 ++++++-
 hw/core/clock-vmstate.c        |  25 +++
 hw/core/clock.c                | 131 ++++++++++++
 hw/core/qdev-clock.c           | 186 +++++++++++++++++
 hw/core/qdev.c                 |  12 ++
 hw/misc/zynq_slcr.c            | 172 +++++++++++++++-
 qdev-monitor.c                 |   9 +
 hw/char/trace-events           |   3 +
 hw/core/Makefile.objs          |   3 +
 hw/core/trace-events           |   7 +
 tests/Makefile.include         |   1 +
 18 files changed, 1412 insertions(+), 22 deletions(-)
 create mode 100644 docs/devel/clocks.rst
 create mode 100644 include/hw/clock.h
 create mode 100644 include/hw/qdev-clock.h
 create mode 100644 hw/core/clock-vmstate.c
 create mode 100644 hw/core/clock.c
 create mode 100644 hw/core/qdev-clock.c

-- 
2.24.1

[PATCH v7 1/9] hw/core/clock: introduce clock object

[Damien Hedde]

This object may be used to represent a clock inside a clock tree.

A clock may be connected to another clock so that it receives update,
through a callback, whenever the source/parent clock is updated.

Although only the root clock of a clock tree controls the values
(represented as periods) of all clocks in tree, each clock holds
a local state containing the current value so that it can be fetched
independently. It will allows us to fullfill migration requirements
by migrating each clock independently of others.

This is based on the original work of Frederic Konrad.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
--

v7:
+ merge ClockIn & ClockOut into a single type Clock
+ switch clock state to a period with 2^-32ns unit
+ add some Hz and ns helpers
+ propagate clock period when setting the source so that
  clocks with fixed period are easy to handle.
---
 include/hw/clock.h    | 216 ++++++++++++++++++++++++++++++++++++++++++
 hw/core/clock.c       | 131 +++++++++++++++++++++++++
 hw/core/Makefile.objs |   2 +
 hw/core/trace-events  |   7 ++
 4 files changed, 356 insertions(+)
 create mode 100644 include/hw/clock.h
 create mode 100644 hw/core/clock.c

diff --git a/include/hw/clock.h b/include/hw/clock.h
new file mode 100644
index 0000000000..30ac9a9946
--- /dev/null
+++ b/include/hw/clock.h
@@ -0,0 +1,216 @@
+/*
+ * Hardware Clocks
+ *
+ * Copyright GreenSocs 2016-2020
+ *
+ * Authors:
+ *  Frederic Konrad
+ *  Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HW_CLOCK_H
+#define QEMU_HW_CLOCK_H
+
+#include "qom/object.h"
+#include "qemu/queue.h"
+
+#define TYPE_CLOCK "clock"
+#define CLOCK(obj) OBJECT_CHECK(Clock, (obj), TYPE_CLOCK)
+
+typedef void ClockCallback(void *opaque);
+
+/*
+ * clock store a value representing the clock's period in 2^-32ns unit.
+ * It can represent:
+ *  + periods from 2^-32ns up to 4seconds
+ *  + frequency from ~0.25Hz 2e10Ghz
+ * Resolution of frequency representation decreases with frequency:
+ * + at 100MHz, resolution is ~2mHz
+ * + at 1Ghz,   resolution is ~0.2Hz
+ * + at 10Ghz,  resolution is ~20Hz
+ */
+#define CLOCK_SECOND (1000000000llu << 32)
+
+/*
+ * macro helpers to convert to hertz / nanosecond
+ */
+#define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_SECOND / 1000000000llu))
+#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_SECOND / 1000000000llu))
+#define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_SECOND / (hz) : 0u)
+#define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_SECOND / (per) : 0u)
+
+/**
+ * Clock:
+ * @parent_obj: parent class
+ * @period: unsigned integer representing the period of the clock
+ * @canonical_path: clock path string cache (used for trace purpose)
+ * @callback: called when clock changes
+ * @callback_opaque: argument for @callback
+ * @source: source (or parent in clock tree) of the clock
+ * @children: list of clocks connected to this one (it is their source)
+ * @sibling: structure used to form a clock list
+ */
+
+typedef struct Clock Clock;
+
+struct Clock {
+    /*< private >*/
+    Object parent_obj;
+
+    /* all fields are private and should not be modified directly */
+
+    /* fields */
+    uint64_t period;
+    char *canonical_path;
+    ClockCallback *callback;
+    void *callback_opaque;
+
+    /* Clocks are organized in a clock tree */
+    Clock *source;
+    QLIST_HEAD(, Clock) children;
+    QLIST_ENTRY(Clock) sibling;
+};
+
+/**
+ * clock_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_setup_canonical_path(Clock *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(Clock *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(Clock *clk);
+
+/**
+ * clock_set_source:
+ * @clk: the clock.
+ * @src: the source clock
+ *
+ * Setup @src as the clock source of @clk. The current @src period
+ * value is also copied to @clk and its subtree but not callback is
+ * called.
+ * Further @src update will be propagated to @clk and its subtree.
+ */
+void clock_set_source(Clock *clk, Clock *src);
+
+/**
+ * clock_set:
+ * @clk: the clock to initialize.
+ * @value: the clock's value, 0 means unclocked
+ *
+ * Set the local cached period value of @clk to @value.
+ */
+void clock_set(Clock *clk, uint64_t value);
+
+static inline void clock_set_hz(Clock *clk, unsigned hz)
+{
+    clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz));
+}
+
+static inline void clock_set_ns(Clock *clk, unsigned ns)
+{
+    clock_set(clk, CLOCK_PERIOD_FROM_NS(ns));
+}
+
+/**
+ * clock_propagate:
+ * @clk: the clock
+ *
+ * Propagate the clock period that has been previsouly configured using
+ * @clock_set(). This will update recursively all connected clocks.
+ * It is an error to call this function on a clock which has a source.
+ * Note: this function must not be called during device inititialization
+ * or migration.
+ */
+void clock_propagate(Clock *clk);
+
+/**
+ * clock_update:
+ * @clk: the clock to update.
+ * @value: the new clock's value, 0 means unclocked
+ *
+ * Update the @clk to the new @value. All connected clocks will be informed
+ * of this update. This is equivalent to call @clock_set() then
+ * @clock_propagate().
+ */
+static inline void clock_update(Clock *clk, uint64_t value)
+{
+    clock_set(clk, value);
+    clock_propagate(clk);
+}
+
+static inline void clock_update_hz(Clock *clk, unsigned hz)
+{
+    clock_update(clk, CLOCK_PERIOD_FROM_HZ(hz));
+}
+
+static inline void clock_update_ns(Clock *clk, unsigned ns)
+{
+    clock_update(clk, CLOCK_PERIOD_FROM_NS(ns));
+}
+
+/**
+ * clock_get:
+ * @clk: the clk to fetch the clock
+ *
+ * @return: the current period.
+ */
+static inline uint64_t clock_get(const Clock *clk)
+{
+    return clk->period;
+}
+
+static inline unsigned clock_get_hz(Clock *clk)
+{
+    return CLOCK_PERIOD_TO_HZ(clock_get(clk));
+}
+
+static inline unsigned clock_get_ns(Clock *clk)
+{
+    return CLOCK_PERIOD_TO_NS(clock_get(clk));
+}
+
+/**
+ * clock_is_enabled:
+ * @clk: a clock
+ *
+ * @return: true if the clock is running.
+ */
+static inline bool clock_is_enabled(const Clock *clk)
+{
+    return clock_get(clk) != 0;
+}
+
+static inline void clock_init(Clock *clk, uint64_t value)
+{
+    clock_set(clk, value);
+}
+static inline void clock_init_hz(Clock *clk, uint64_t value)
+{
+    clock_set_hz(clk, value);
+}
+static inline void clock_init_ns(Clock *clk, uint64_t value)
+{
+    clock_set_ns(clk, value);
+}
+
+#endif /* QEMU_HW_CLOCK_H */
diff --git a/hw/core/clock.c b/hw/core/clock.c
new file mode 100644
index 0000000000..4ba5765b05
--- /dev/null
+++ b/hw/core/clock.c
@@ -0,0 +1,131 @@
+/*
+ * Hardware Clocks
+ *
+ * Copyright GreenSocs 2016-2020
+ *
+ * Authors:
+ *  Frederic Konrad
+ *  Damien Hedde
+ *
+ * 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_setup_canonical_path(Clock *clk)
+{
+    g_free(clk->canonical_path);
+    clk->canonical_path = object_get_canonical_path(OBJECT(clk));
+}
+
+void clock_set_callback(Clock *clk, ClockCallback *cb, void *opaque)
+{
+    clk->callback = cb;
+    clk->callback_opaque = opaque;
+}
+
+void clock_clear_callback(Clock *clk)
+{
+    clock_set_callback(clk, NULL, NULL);
+}
+
+void clock_set(Clock *clk, uint64_t period)
+{
+    trace_clock_set(CLOCK_PATH(clk), CLOCK_PERIOD_TO_NS(clk->period),
+                    CLOCK_PERIOD_TO_NS(period));
+    clk->period = period;
+}
+
+static void clock_propagate_period(Clock *clk, bool call_callbacks)
+{
+    Clock *child;
+
+    QLIST_FOREACH(child, &clk->children, sibling) {
+        if (child->period != clk->period) {
+            child->period = clk->period;
+            trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk),
+                               CLOCK_PERIOD_TO_NS(clk->period),
+                               call_callbacks);
+            if (call_callbacks && child->callback) {
+                child->callback(child->callback_opaque);
+            }
+            clock_propagate_period(child, call_callbacks);
+        }
+    }
+}
+
+void clock_propagate(Clock *clk)
+{
+    assert(clk->source == NULL);
+    trace_clock_propagate(CLOCK_PATH(clk));
+    clock_propagate_period(clk, true);
+}
+
+void clock_set_source(Clock *clk, Clock *src)
+{
+    /* changing clock source is not supported */
+    assert(!clk->source);
+
+    trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src));
+
+    clk->period = src->period;
+    QLIST_INSERT_HEAD(&src->children, clk, sibling);
+    clk->source = src;
+    clock_propagate_period(clk, false);
+}
+
+static void clock_disconnect(Clock *clk)
+{
+    if (clk->source == NULL) {
+        return;
+    }
+
+    trace_clock_disconnect(CLOCK_PATH(clk));
+
+    clk->source = NULL;
+    QLIST_REMOVE(clk, sibling);
+}
+
+static void clock_initfn(Object *obj)
+{
+    Clock *clk = CLOCK(obj);
+
+    QLIST_INIT(&clk->children);
+}
+
+static void clock_finalizefn(Object *obj)
+{
+    Clock *clk = CLOCK(obj);
+    Clock *child, *next;
+
+    /* clear our list of children */
+    QLIST_FOREACH_SAFE(child, &clk->children, sibling, next) {
+        clock_disconnect(child);
+    }
+
+    /* remove us from source's chidlren list */
+    clock_disconnect(clk);
+
+    g_free(clk->canonical_path);
+    clk->canonical_path = NULL;
+}
+
+static const TypeInfo clock_info = {
+    .name              = TYPE_CLOCK,
+    .parent            = TYPE_OBJECT,
+    .instance_size     = sizeof(Clock),
+    .instance_init     = clock_initfn,
+    .instance_finalize = clock_finalizefn,
+};
+
+static void clock_register_types(void)
+{
+    type_register_static(&clock_info);
+}
+
+type_init(clock_register_types)
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 6215e7c208..d7080edf89 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -7,10 +7,12 @@ common-obj-y += hotplug.o
 common-obj-y += vmstate-if.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
+common-obj-y += clock.o
 
 common-obj-$(CONFIG_SOFTMMU) += reset.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
 common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
+common-obj-$(CONFIG_SOFTMMU) += hotplug.o
 common-obj-$(CONFIG_SOFTMMU) += nmi.o
 common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
diff --git a/hw/core/trace-events b/hw/core/trace-events
index aecd8e160e..39301621ce 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -27,3 +27,10 @@ resettable_phase_exit_begin(void *obj, const char *objtype, unsigned count, int
 resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
 resettable_phase_exit_end(void *obj, const char *objtype, unsigned count) "obj=%p(%s) count=%d"
 resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
+
+# clock.c
+clock_set_source(const char *clk, const char *src) "'%s', src='%s'"
+clock_disconnect(const char *clk) "'%s'"
+clock_set(const char *clk, unsigned long long old, unsigned long long new) "'%s', ns=%llu->%llu"
+clock_propagate(const char *clk) "'%s'"
+clock_update(const char *clk, const char *src, unsigned long long val, int cb) "'%s', src='%s', ns=%llu, cb=%d"
-- 
2.24.1

Re: [PATCH v7 1/9] hw/core/clock: introduce clock object

[Alistair Francis]

On Mon, Feb 24, 2020 at 9:05 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This object may be used to represent a clock inside a clock tree.
>
> A clock may be connected to another clock so that it receives update,
> through a callback, whenever the source/parent clock is updated.
>
> Although only the root clock of a clock tree controls the values
> (represented as periods) of all clocks in tree, each clock holds
> a local state containing the current value so that it can be fetched
> independently. It will allows us to fullfill migration requirements
> by migrating each clock independently of others.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> --
>
> v7:
> + merge ClockIn & ClockOut into a single type Clock
> + switch clock state to a period with 2^-32ns unit
> + add some Hz and ns helpers
> + propagate clock period when setting the source so that
>   clocks with fixed period are easy to handle.
> ---
>  include/hw/clock.h    | 216 ++++++++++++++++++++++++++++++++++++++++++
>  hw/core/clock.c       | 131 +++++++++++++++++++++++++
>  hw/core/Makefile.objs |   2 +
>  hw/core/trace-events  |   7 ++
>  4 files changed, 356 insertions(+)
>  create mode 100644 include/hw/clock.h
>  create mode 100644 hw/core/clock.c
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> new file mode 100644
> index 0000000000..30ac9a9946
> --- /dev/null
> +++ b/include/hw/clock.h
> @@ -0,0 +1,216 @@
> +/*
> + * Hardware Clocks
> + *
> + * Copyright GreenSocs 2016-2020
> + *
> + * Authors:
> + *  Frederic Konrad
> + *  Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_HW_CLOCK_H
> +#define QEMU_HW_CLOCK_H
> +
> +#include "qom/object.h"
> +#include "qemu/queue.h"
> +
> +#define TYPE_CLOCK "clock"
> +#define CLOCK(obj) OBJECT_CHECK(Clock, (obj), TYPE_CLOCK)
> +
> +typedef void ClockCallback(void *opaque);
> +
> +/*
> + * clock store a value representing the clock's period in 2^-32ns unit.
> + * It can represent:
> + *  + periods from 2^-32ns up to 4seconds
> + *  + frequency from ~0.25Hz 2e10Ghz
> + * Resolution of frequency representation decreases with frequency:
> + * + at 100MHz, resolution is ~2mHz
> + * + at 1Ghz,   resolution is ~0.2Hz
> + * + at 10Ghz,  resolution is ~20Hz
> + */
> +#define CLOCK_SECOND (1000000000llu << 32)
> +
> +/*
> + * macro helpers to convert to hertz / nanosecond
> + */
> +#define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_SECOND / 1000000000llu))
> +#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_SECOND / 1000000000llu))
> +#define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_SECOND / (hz) : 0u)
> +#define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_SECOND / (per) : 0u)
> +
> +/**
> + * Clock:
> + * @parent_obj: parent class
> + * @period: unsigned integer representing the period of the clock
> + * @canonical_path: clock path string cache (used for trace purpose)
> + * @callback: called when clock changes
> + * @callback_opaque: argument for @callback
> + * @source: source (or parent in clock tree) of the clock
> + * @children: list of clocks connected to this one (it is their source)
> + * @sibling: structure used to form a clock list
> + */
> +
> +typedef struct Clock Clock;
> +
> +struct Clock {
> +    /*< private >*/
> +    Object parent_obj;
> +
> +    /* all fields are private and should not be modified directly */
> +
> +    /* fields */
> +    uint64_t period;
> +    char *canonical_path;
> +    ClockCallback *callback;
> +    void *callback_opaque;
> +
> +    /* Clocks are organized in a clock tree */
> +    Clock *source;
> +    QLIST_HEAD(, Clock) children;
> +    QLIST_ENTRY(Clock) sibling;
> +};
> +
> +/**
> + * clock_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_setup_canonical_path(Clock *clk);
> +
> +/**
> + * clock_add_callback:

s/clock_add_callback/clock_set_callback/g

> + * @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(Clock *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(Clock *clk);
> +
> +/**
> + * clock_set_source:
> + * @clk: the clock.
> + * @src: the source clock
> + *
> + * Setup @src as the clock source of @clk. The current @src period
> + * value is also copied to @clk and its subtree but not callback is

s/not/no/g

> + * called.
> + * Further @src update will be propagated to @clk and its subtree.
> + */
> +void clock_set_source(Clock *clk, Clock *src);
> +
> +/**
> + * clock_set:
> + * @clk: the clock to initialize.
> + * @value: the clock's value, 0 means unclocked
> + *
> + * Set the local cached period value of @clk to @value.
> + */
> +void clock_set(Clock *clk, uint64_t value);
> +
> +static inline void clock_set_hz(Clock *clk, unsigned hz)
> +{
> +    clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz));
> +}
> +
> +static inline void clock_set_ns(Clock *clk, unsigned ns)
> +{
> +    clock_set(clk, CLOCK_PERIOD_FROM_NS(ns));
> +}
> +
> +/**
> + * clock_propagate:
> + * @clk: the clock
> + *
> + * Propagate the clock period that has been previsouly configured using

s/previsouly/previously/g

> + * @clock_set(). This will update recursively all connected clocks.
> + * It is an error to call this function on a clock which has a source.
> + * Note: this function must not be called during device inititialization
> + * or migration.
> + */
> +void clock_propagate(Clock *clk);
> +
> +/**
> + * clock_update:
> + * @clk: the clock to update.
> + * @value: the new clock's value, 0 means unclocked
> + *
> + * Update the @clk to the new @value. All connected clocks will be informed
> + * of this update. This is equivalent to call @clock_set() then
> + * @clock_propagate().
> + */
> +static inline void clock_update(Clock *clk, uint64_t value)
> +{
> +    clock_set(clk, value);
> +    clock_propagate(clk);
> +}
> +
> +static inline void clock_update_hz(Clock *clk, unsigned hz)
> +{
> +    clock_update(clk, CLOCK_PERIOD_FROM_HZ(hz));
> +}
> +
> +static inline void clock_update_ns(Clock *clk, unsigned ns)
> +{
> +    clock_update(clk, CLOCK_PERIOD_FROM_NS(ns));
> +}
> +
> +/**
> + * clock_get:
> + * @clk: the clk to fetch the clock
> + *
> + * @return: the current period.
> + */
> +static inline uint64_t clock_get(const Clock *clk)
> +{
> +    return clk->period;
> +}
> +
> +static inline unsigned clock_get_hz(Clock *clk)
> +{
> +    return CLOCK_PERIOD_TO_HZ(clock_get(clk));
> +}
> +
> +static inline unsigned clock_get_ns(Clock *clk)
> +{
> +    return CLOCK_PERIOD_TO_NS(clock_get(clk));
> +}
> +
> +/**
> + * clock_is_enabled:
> + * @clk: a clock
> + *
> + * @return: true if the clock is running.
> + */
> +static inline bool clock_is_enabled(const Clock *clk)
> +{
> +    return clock_get(clk) != 0;
> +}
> +
> +static inline void clock_init(Clock *clk, uint64_t value)
> +{
> +    clock_set(clk, value);
> +}
> +static inline void clock_init_hz(Clock *clk, uint64_t value)
> +{
> +    clock_set_hz(clk, value);
> +}
> +static inline void clock_init_ns(Clock *clk, uint64_t value)
> +{
> +    clock_set_ns(clk, value);
> +}
> +
> +#endif /* QEMU_HW_CLOCK_H */
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> new file mode 100644
> index 0000000000..4ba5765b05
> --- /dev/null
> +++ b/hw/core/clock.c
> @@ -0,0 +1,131 @@
> +/*
> + * Hardware Clocks
> + *
> + * Copyright GreenSocs 2016-2020
> + *
> + * Authors:
> + *  Frederic Konrad
> + *  Damien Hedde
> + *
> + * 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_setup_canonical_path(Clock *clk)
> +{
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_set_callback(Clock *clk, ClockCallback *cb, void *opaque)
> +{
> +    clk->callback = cb;
> +    clk->callback_opaque = opaque;
> +}
> +
> +void clock_clear_callback(Clock *clk)
> +{
> +    clock_set_callback(clk, NULL, NULL);
> +}
> +
> +void clock_set(Clock *clk, uint64_t period)
> +{
> +    trace_clock_set(CLOCK_PATH(clk), CLOCK_PERIOD_TO_NS(clk->period),
> +                    CLOCK_PERIOD_TO_NS(period));
> +    clk->period = period;
> +}
> +
> +static void clock_propagate_period(Clock *clk, bool call_callbacks)
> +{
> +    Clock *child;
> +
> +    QLIST_FOREACH(child, &clk->children, sibling) {
> +        if (child->period != clk->period) {
> +            child->period = clk->period;
> +            trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk),
> +                               CLOCK_PERIOD_TO_NS(clk->period),
> +                               call_callbacks);
> +            if (call_callbacks && child->callback) {
> +                child->callback(child->callback_opaque);
> +            }
> +            clock_propagate_period(child, call_callbacks);
> +        }
> +    }
> +}
> +
> +void clock_propagate(Clock *clk)
> +{
> +    assert(clk->source == NULL);
> +    trace_clock_propagate(CLOCK_PATH(clk));
> +    clock_propagate_period(clk, true);
> +}
> +
> +void clock_set_source(Clock *clk, Clock *src)
> +{
> +    /* changing clock source is not supported */
> +    assert(!clk->source);
> +
> +    trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src));
> +
> +    clk->period = src->period;
> +    QLIST_INSERT_HEAD(&src->children, clk, sibling);
> +    clk->source = src;
> +    clock_propagate_period(clk, false);
> +}
> +
> +static void clock_disconnect(Clock *clk)
> +{
> +    if (clk->source == NULL) {
> +        return;
> +    }
> +
> +    trace_clock_disconnect(CLOCK_PATH(clk));
> +
> +    clk->source = NULL;
> +    QLIST_REMOVE(clk, sibling);
> +}
> +
> +static void clock_initfn(Object *obj)
> +{
> +    Clock *clk = CLOCK(obj);
> +
> +    QLIST_INIT(&clk->children);
> +}
> +
> +static void clock_finalizefn(Object *obj)
> +{
> +    Clock *clk = CLOCK(obj);
> +    Clock *child, *next;
> +
> +    /* clear our list of children */
> +    QLIST_FOREACH_SAFE(child, &clk->children, sibling, next) {
> +        clock_disconnect(child);
> +    }
> +
> +    /* remove us from source's chidlren list */

s/chidlren/children/g

> +    clock_disconnect(clk);
> +
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = NULL;

You shouldn't need to set this to NULL.

> +}
> +
> +static const TypeInfo clock_info = {
> +    .name              = TYPE_CLOCK,
> +    .parent            = TYPE_OBJECT,
> +    .instance_size     = sizeof(Clock),
> +    .instance_init     = clock_initfn,
> +    .instance_finalize = clock_finalizefn,
> +};
> +
> +static void clock_register_types(void)
> +{
> +    type_register_static(&clock_info);
> +}
> +
> +type_init(clock_register_types)
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 6215e7c208..d7080edf89 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,10 +7,12 @@ common-obj-y += hotplug.o
>  common-obj-y += vmstate-if.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
> +common-obj-y += clock.o
>
>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o

I don't think this should be here.

Alistair

>  common-obj-$(CONFIG_SOFTMMU) += nmi.o
>  common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index aecd8e160e..39301621ce 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -27,3 +27,10 @@ resettable_phase_exit_begin(void *obj, const char *objtype, unsigned count, int
>  resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
>  resettable_phase_exit_end(void *obj, const char *objtype, unsigned count) "obj=%p(%s) count=%d"
>  resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
> +
> +# clock.c
> +clock_set_source(const char *clk, const char *src) "'%s', src='%s'"
> +clock_disconnect(const char *clk) "'%s'"
> +clock_set(const char *clk, unsigned long long old, unsigned long long new) "'%s', ns=%llu->%llu"
> +clock_propagate(const char *clk) "'%s'"
> +clock_update(const char *clk, const char *src, unsigned long long val, int cb) "'%s', src='%s', ns=%llu, cb=%d"
> --
> 2.24.1
>
>

Re: [PATCH v7 1/9] hw/core/clock: introduce clock object

[Damien Hedde]

On 2/25/20 1:02 AM, Alistair Francis wrote:
> On Mon, Feb 24, 2020 at 9:05 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This object may be used to represent a clock inside a clock tree.
>>
>> A clock may be connected to another clock so that it receives update,
>> through a callback, whenever the source/parent clock is updated.
>>
>> Although only the root clock of a clock tree controls the values
>> (represented as periods) of all clocks in tree, each clock holds
>> a local state containing the current value so that it can be fetched
>> independently. It will allows us to fullfill migration requirements
>> by migrating each clock independently of others.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> --
>>
>> v7:
>> + merge ClockIn & ClockOut into a single type Clock
>> + switch clock state to a period with 2^-32ns unit
>> + add some Hz and ns helpers
>> + propagate clock period when setting the source so that
>>   clocks with fixed period are easy to handle.
>> ---
>>  include/hw/clock.h    | 216 ++++++++++++++++++++++++++++++++++++++++++
>>  hw/core/clock.c       | 131 +++++++++++++++++++++++++
>>  hw/core/Makefile.objs |   2 +
>>  hw/core/trace-events  |   7 ++
>>  4 files changed, 356 insertions(+)
>>  create mode 100644 include/hw/clock.h
>>  create mode 100644 hw/core/clock.c
>>
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> new file mode 100644
>> index 0000000000..30ac9a9946
>> --- /dev/null
>> +++ b/include/hw/clock.h
>> @@ -0,0 +1,216 @@

[...]

> 
>> +    clock_disconnect(clk);
>> +
>> +    g_free(clk->canonical_path);
>> +    clk->canonical_path = NULL;
> 
> You shouldn't need to set this to NULL.

ok.

> 
>> +}
>> +
>> +static const TypeInfo clock_info = {
>> +    .name              = TYPE_CLOCK,
>> +    .parent            = TYPE_OBJECT,
>> +    .instance_size     = sizeof(Clock),
>> +    .instance_init     = clock_initfn,
>> +    .instance_finalize = clock_finalizefn,
>> +};
>> +
>> +static void clock_register_types(void)
>> +{
>> +    type_register_static(&clock_info);
>> +}
>> +
>> +type_init(clock_register_types)
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 6215e7c208..d7080edf89 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -7,10 +7,12 @@ common-obj-y += hotplug.o
>>  common-obj-y += vmstate-if.o
>>  # irq.o needed for qdev GPIO handling:
>>  common-obj-y += irq.o
>> +common-obj-y += clock.o
>>
>>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> 
> I don't think this should be here.
Oops, I missed this rebase artifact.

Thanks,
Damien

[PATCH v7 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state

[Damien Hedde]

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
--

v7: remove leading underscores in macro args
---
 include/hw/clock.h      |  9 +++++++++
 hw/core/clock-vmstate.c | 25 +++++++++++++++++++++++++
 hw/core/Makefile.objs   |  1 +
 3 files changed, 35 insertions(+)
 create mode 100644 hw/core/clock-vmstate.c

diff --git a/include/hw/clock.h b/include/hw/clock.h
index 30ac9a9946..8c191751a1 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -74,6 +74,15 @@ struct Clock {
     QLIST_ENTRY(Clock) sibling;
 };
 
+/*
+ * vmstate description entry to be added in device vmsd.
+ */
+extern const VMStateDescription vmstate_clock;
+#define VMSTATE_CLOCK(field, state) \
+    VMSTATE_CLOCK_V(field, state, 0)
+#define VMSTATE_CLOCK_V(field, state, version) \
+    VMSTATE_STRUCT_POINTER_V(field, state, version, vmstate_clock, Clock)
+
 /**
  * clock_setup_canonical_path:
  * @clk: clock
diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c
new file mode 100644
index 0000000000..260b13fc2c
--- /dev/null
+++ b/hw/core/clock-vmstate.c
@@ -0,0 +1,25 @@
+/*
+ * Clock migration structure
+ *
+ * Copyright GreenSocs 2019-2020
+ *
+ * Authors:
+ *  Damien Hedde
+ *
+ * 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_clock = {
+    .name = "clock",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(period, Clock),
+        VMSTATE_END_OF_LIST()
+    }
+};
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index d7080edf89..e3d796fdd4 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -22,6 +22,7 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += machine-hmp-cmds.o
 common-obj-$(CONFIG_SOFTMMU) += numa.o
+common-obj-$(CONFIG_SOFTMMU) += clock-vmstate.o
 obj-$(CONFIG_SOFTMMU) += machine-qmp-cmds.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
-- 
2.24.1

Re: [PATCH v7 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state

[Alistair Francis]

On Mon, Feb 24, 2020 at 9:06 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> --
>
> v7: remove leading underscores in macro args
> ---
>  include/hw/clock.h      |  9 +++++++++
>  hw/core/clock-vmstate.c | 25 +++++++++++++++++++++++++
>  hw/core/Makefile.objs   |  1 +
>  3 files changed, 35 insertions(+)
>  create mode 100644 hw/core/clock-vmstate.c
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index 30ac9a9946..8c191751a1 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -74,6 +74,15 @@ struct Clock {
>      QLIST_ENTRY(Clock) sibling;
>  };
>
> +/*
> + * vmstate description entry to be added in device vmsd.
> + */
> +extern const VMStateDescription vmstate_clock;
> +#define VMSTATE_CLOCK(field, state) \
> +    VMSTATE_CLOCK_V(field, state, 0)
> +#define VMSTATE_CLOCK_V(field, state, version) \
> +    VMSTATE_STRUCT_POINTER_V(field, state, version, vmstate_clock, Clock)
> +
>  /**
>   * clock_setup_canonical_path:
>   * @clk: clock
> diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c
> new file mode 100644
> index 0000000000..260b13fc2c
> --- /dev/null
> +++ b/hw/core/clock-vmstate.c
> @@ -0,0 +1,25 @@
> +/*
> + * Clock migration structure
> + *
> + * Copyright GreenSocs 2019-2020
> + *
> + * Authors:
> + *  Damien Hedde
> + *
> + * 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_clock = {
> +    .name = "clock",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(period, Clock),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index d7080edf89..e3d796fdd4 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -22,6 +22,7 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += machine-hmp-cmds.o
>  common-obj-$(CONFIG_SOFTMMU) += numa.o
> +common-obj-$(CONFIG_SOFTMMU) += clock-vmstate.o
>  obj-$(CONFIG_SOFTMMU) += machine-qmp-cmds.o
>
>  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> --
> 2.24.1
>
>

[PATCH v7 3/9] qdev: add clock input&output support to devices.

[Damien Hedde]

Add functions to easily handle clocks with devices.
Clock inputs and outputs should be used to handle clock propagation
between devices.
The API is very similar the GPIO API.

This is based on the original work of Frederic Konrad.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

I did not changed the constness of @name pointer field in
NamedClockList structure.
There is no obstacle to do it but the fact that we need to free the
allocated data it points to. It is possible to make it const and
hack/remove the const to call g_free but I don't know if its
allowed in qemu.

v7:
+ update ClockIn/Out types
+ qdev_connect_clock_out function removed / qdev_connect_clock_in added
  instead
+ qdev_pass_clock renamed to qdev_alias_clock
+ various small fixes (typos, comment, asserts) (Peter)
+ move device's instance_finalize code related to clock in qdev-clock.c
---
 include/hw/qdev-clock.h | 105 +++++++++++++++++++++++++
 include/hw/qdev-core.h  |  12 +++
 hw/core/qdev-clock.c    | 169 ++++++++++++++++++++++++++++++++++++++++
 hw/core/qdev.c          |  12 +++
 hw/core/Makefile.objs   |   2 +-
 tests/Makefile.include  |   1 +
 6 files changed, 300 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/qdev-clock.h
 create mode 100644 hw/core/qdev-clock.c

diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
new file mode 100644
index 0000000000..899a95ca6a
--- /dev/null
+++ b/include/hw/qdev-clock.h
@@ -0,0 +1,105 @@
+/*
+ * Device's clock input and output
+ *
+ * Copyright GreenSocs 2016-2020
+ *
+ * Authors:
+ *  Frederic Konrad
+ *  Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QDEV_CLOCK_H
+#define QDEV_CLOCK_H
+
+#include "hw/clock.h"
+
+/**
+ * qdev_init_clock_in:
+ * @dev: the device to add an input clock 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 an input clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ * The callback will be called with @opaque as opaque parameter.
+ */
+Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
+                          ClockCallback *callback, void *opaque);
+
+/**
+ * qdev_init_clock_out:
+ * @dev: the device to add an output 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 an output clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ */
+Clock *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 input clock @name from @dev or NULL if does not exist.
+ */
+Clock *qdev_get_clock_in(DeviceState *dev, const char *name);
+
+/**
+ * qdev_get_clock_out:
+ * @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 output clock @name from @dev or NULL if does not exist.
+ */
+Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
+
+/**
+ * qdev_connect_clock_in:
+ * @dev: a device
+ * @name: the name of an input clock in @dev
+ * @source: the source clock (an output clock of another device for example)
+ *
+ * Set the source clock of input clock @name of device @dev to @source.
+ * @source period update will be propagated to @name clock.
+ */
+static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
+                                         Clock *source)
+{
+    clock_set_source(qdev_get_clock_in(dev, name), source);
+}
+
+/**
+ * qdev_alias_clock:
+ * @dev: the device which has the clock
+ * @name: the name of the clock in @dev (can't be NULL)
+ * @alias_dev: the device to add the clock
+ * @alias_name: the name of the clock in @container
+ * @returns: a pointer to the clock
+ *
+ * Add a clock @alias_name in @alias_dev which is an alias of the clock @name
+ * in @dev. The direction _in_ or _out_ will the same as the original.
+ * An alias clock must not be modified or used by @alias_dev and should
+ * typically be only only for device composition purpose.
+ */
+Clock *qdev_alias_clock(DeviceState *dev, const char *name,
+                        DeviceState *alias_dev, const char *alias_name);
+
+/**
+ * qdev_finalize_clocklist:
+ * @dev: the device being finalize
+ *
+ * Clear the clocklist from @dev. Only used internally in qdev.
+ */
+void qdev_finalize_clocklist(DeviceState *dev);
+
+#endif /* QDEV_CLOCK_H */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1405b8a990..d87d989e72 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -149,6 +149,17 @@ struct NamedGPIOList {
     QLIST_ENTRY(NamedGPIOList) node;
 };
 
+typedef struct Clock Clock;
+typedef struct NamedClockList NamedClockList;
+
+struct NamedClockList {
+    char *name;
+    Clock *clock;
+    bool output;
+    bool alias;
+    QLIST_ENTRY(NamedClockList) node;
+};
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
@@ -171,6 +182,7 @@ struct DeviceState {
     bool allow_unplug_during_migration;
     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/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
new file mode 100644
index 0000000000..9af0159517
--- /dev/null
+++ b/hw/core/qdev-clock.c
@@ -0,0 +1,169 @@
+/*
+ * Device's clock input and output
+ *
+ * Copyright GreenSocs 2016-2020
+ *
+ * Authors:
+ *  Frederic Konrad
+ *  Damien Hedde
+ *
+ * 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"
+
+/*
+ * qdev_init_clocklist:
+ * Add a new clock in a device
+ */
+static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
+                                           bool output, Clock *clk)
+{
+    NamedClockList *ncl;
+
+    /*
+     * Clock must be added before realize() so that we can compute the
+     * clock's canonical path durint device_realize().
+     */
+    assert(!dev->realized);
+
+    /*
+     * The ncl structure is freed by qdev_finalize_clocklist() which will
+     * be called during @dev's device_finalize().
+     */
+    ncl = g_new0(NamedClockList, 1);
+    ncl->name = g_strdup(name);
+    ncl->output = output;
+    ncl->alias = (clk != NULL);
+
+    /*
+     * Trying to create a clock whose name clashes with some other
+     * clock or property is a bug in the caller and we will abort().
+     */
+    if (clk == NULL) {
+        clk = CLOCK(object_new(TYPE_CLOCK));
+        object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort);
+        if (output) {
+            /*
+             * Remove object_new()'s initial reference.
+             * Note that for inputs, the reference created by object_new()
+             * will be deleted in qdev_finalize_clocklist().
+             */
+            object_unref(OBJECT(clk));
+        }
+    } else {
+        object_property_add_link(OBJECT(dev), name,
+                                 object_get_typename(OBJECT(clk)),
+                                 (Object **) &ncl->clock,
+                                 NULL, OBJ_PROP_LINK_STRONG, &error_abort);
+    }
+
+    ncl->clock = clk;
+
+    QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
+    return ncl;
+}
+
+void qdev_finalize_clocklist(DeviceState *dev)
+{
+    /* called by @dev's device_finalize() */
+    NamedClockList *ncl, *ncl_next;
+
+    QLIST_FOREACH_SAFE(ncl, &dev->clocks, node, ncl_next) {
+        QLIST_REMOVE(ncl, node);
+        if (!ncl->output && !ncl->alias) {
+            /*
+             * We kept a reference on the input clock to ensure it lives up to
+             * this point so we can safely remove the callback.
+             * It avoids having a callback to a deleted object if ncl->clock
+             * is still referenced somewhere else (eg: by a clock output).
+             */
+            clock_clear_callback(ncl->clock);
+            object_unref(OBJECT(ncl->clock));
+        }
+        g_free(ncl->name);
+        g_free(ncl);
+    }
+}
+
+Clock *qdev_init_clock_out(DeviceState *dev, const char *name)
+{
+    NamedClockList *ncl;
+
+    assert(name);
+
+    ncl = qdev_init_clocklist(dev, name, true, NULL);
+
+    return ncl->clock;
+}
+
+Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
+                            ClockCallback *callback, void *opaque)
+{
+    NamedClockList *ncl;
+
+    assert(name);
+
+    ncl = qdev_init_clocklist(dev, name, false, NULL);
+
+    if (callback) {
+        clock_set_callback(ncl->clock, callback, opaque);
+    }
+    return ncl->clock;
+}
+
+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;
+        }
+    }
+
+    assert(false);
+    return NULL;
+}
+
+Clock *qdev_get_clock_in(DeviceState *dev, const char *name)
+{
+    NamedClockList *ncl;
+
+    assert(name);
+
+    ncl = qdev_get_clocklist(dev, name);
+    assert(!ncl->output);
+
+    return ncl->clock;
+}
+
+Clock *qdev_get_clock_out(DeviceState *dev, const char *name)
+{
+    NamedClockList *ncl;
+
+    assert(name);
+
+    ncl = qdev_get_clocklist(dev, name);
+    assert(ncl->output);
+
+    return ncl->clock;
+}
+
+Clock *qdev_alias_clock(DeviceState *dev, const char *name,
+                        DeviceState *alias_dev, const char *alias_name)
+{
+    NamedClockList *ncl;
+
+    assert(name && alias_name);
+
+    ncl = qdev_get_clocklist(dev, name);
+
+    qdev_init_clocklist(alias_dev, alias_name, ncl->output, ncl->clock);
+
+    return ncl->clock;
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3937d1eb1a..f390697348 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/qdev-clock.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
@@ -855,6 +856,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     HotplugHandler *hotplug_ctrl;
     BusState *bus;
+    NamedClockList *ncl;
     Error *local_err = NULL;
     bool unattached_parent = false;
     static int unattached_count;
@@ -902,6 +904,13 @@ 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(ncl, &dev->clocks, node) {
+            if (ncl->alias) {
+                continue;
+            } else {
+                clock_setup_canonical_path(ncl->clock);
+            }
+        }
 
         if (qdev_get_vmsd(dev)) {
             if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
@@ -1025,6 +1034,7 @@ static void device_initfn(Object *obj)
     dev->allow_unplug_during_migration = false;
 
     QLIST_INIT(&dev->gpios);
+    QLIST_INIT(&dev->clocks);
 }
 
 static void device_post_init(Object *obj)
@@ -1054,6 +1064,8 @@ static void device_finalize(Object *obj)
          */
     }
 
+    qdev_finalize_clocklist(dev);
+
     /* Only send event if the device had been completely realized */
     if (dev->pending_deleted_event) {
         g_assert(dev->canonical_path);
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index e3d796fdd4..2fdcb7dd00 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -7,7 +7,7 @@ common-obj-y += hotplug.o
 common-obj-y += vmstate-if.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
-common-obj-y += clock.o
+common-obj-y += clock.o qdev-clock.o
 
 common-obj-$(CONFIG_SOFTMMU) += reset.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
diff --git a/tests/Makefile.include b/tests/Makefile.include
index edcbd475aa..5a4511a86f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -436,6 +436,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
 	hw/core/fw-path-provider.o \
 	hw/core/reset.o \
 	hw/core/vmstate-if.o \
+	hw/core/clock.o hw/core/qdev-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.24.1

Re: [PATCH v7 3/9] qdev: add clock input&output support to devices.

[Alistair Francis]

/On Mon, Feb 24, 2020 at 9:12 AM Damien Hedde <damien.hedde@greensocs.com> wrote
>
> Add functions to easily handle clocks with devices.
> Clock inputs and outputs should be used to handle clock propagation
> between devices.
> The API is very similar the GPIO API.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> I did not changed the constness of @name pointer field in
> NamedClockList structure.
> There is no obstacle to do it but the fact that we need to free the
> allocated data it points to. It is possible to make it const and
> hack/remove the const to call g_free but I don't know if its
> allowed in qemu.
>
> v7:
> + update ClockIn/Out types
> + qdev_connect_clock_out function removed / qdev_connect_clock_in added
>   instead
> + qdev_pass_clock renamed to qdev_alias_clock
> + various small fixes (typos, comment, asserts) (Peter)
> + move device's instance_finalize code related to clock in qdev-clock.c
> ---
>  include/hw/qdev-clock.h | 105 +++++++++++++++++++++++++
>  include/hw/qdev-core.h  |  12 +++
>  hw/core/qdev-clock.c    | 169 ++++++++++++++++++++++++++++++++++++++++
>  hw/core/qdev.c          |  12 +++
>  hw/core/Makefile.objs   |   2 +-
>  tests/Makefile.include  |   1 +
>  6 files changed, 300 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/qdev-clock.h
>  create mode 100644 hw/core/qdev-clock.c
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> new file mode 100644
> index 0000000000..899a95ca6a
> --- /dev/null
> +++ b/include/hw/qdev-clock.h
> @@ -0,0 +1,105 @@
> +/*
> + * Device's clock input and output
> + *
> + * Copyright GreenSocs 2016-2020
> + *
> + * Authors:
> + *  Frederic Konrad
> + *  Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QDEV_CLOCK_H
> +#define QDEV_CLOCK_H
> +
> +#include "hw/clock.h"
> +
> +/**
> + * qdev_init_clock_in:
> + * @dev: the device to add an input clock 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 an input clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + * The callback will be called with @opaque as opaque parameter.
> + */
> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
> +                          ClockCallback *callback, void *opaque);
> +
> +/**
> + * qdev_init_clock_out:
> + * @dev: the device to add an output clock to
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.

qdev_init_clock_out() doesn't have a callback.

> + * @returns: a pointer to the newly added clock

> + *
> + * Add an output clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + */
> +Clock *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 input clock @name from @dev or NULL if does not exist.
> + */
> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_get_clock_out:
> + * @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 output clock @name from @dev or NULL if does not exist.
> + */
> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_connect_clock_in:
> + * @dev: a device
> + * @name: the name of an input clock in @dev
> + * @source: the source clock (an output clock of another device for example)
> + *
> + * Set the source clock of input clock @name of device @dev to @source.
> + * @source period update will be propagated to @name clock.
> + */
> +static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
> +                                         Clock *source)
> +{
> +    clock_set_source(qdev_get_clock_in(dev, name), source);
> +}
> +
> +/**
> + * qdev_alias_clock:
> + * @dev: the device which has the clock
> + * @name: the name of the clock in @dev (can't be NULL)
> + * @alias_dev: the device to add the clock
> + * @alias_name: the name of the clock in @container
> + * @returns: a pointer to the clock
> + *
> + * Add a clock @alias_name in @alias_dev which is an alias of the clock @name
> + * in @dev. The direction _in_ or _out_ will the same as the original.
> + * An alias clock must not be modified or used by @alias_dev and should
> + * typically be only only for device composition purpose.
> + */
> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
> +                        DeviceState *alias_dev, const char *alias_name);
> +
> +/**
> + * qdev_finalize_clocklist:
> + * @dev: the device being finalize

It probably should be:

@dev: the device being finalized

> + *
> + * Clear the clocklist from @dev. Only used internally in qdev.
> + */
> +void qdev_finalize_clocklist(DeviceState *dev);
> +
> +#endif /* QDEV_CLOCK_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1405b8a990..d87d989e72 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -149,6 +149,17 @@ struct NamedGPIOList {
>      QLIST_ENTRY(NamedGPIOList) node;
>  };
>
> +typedef struct Clock Clock;
> +typedef struct NamedClockList NamedClockList;
> +
> +struct NamedClockList {
> +    char *name;
> +    Clock *clock;
> +    bool output;
> +    bool alias;
> +    QLIST_ENTRY(NamedClockList) node;
> +};
> +
>  /**
>   * DeviceState:
>   * @realized: Indicates whether the device has been fully constructed.
> @@ -171,6 +182,7 @@ struct DeviceState {
>      bool allow_unplug_during_migration;
>      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/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> new file mode 100644
> index 0000000000..9af0159517
> --- /dev/null
> +++ b/hw/core/qdev-clock.c
> @@ -0,0 +1,169 @@
> +/*
> + * Device's clock input and output
> + *
> + * Copyright GreenSocs 2016-2020
> + *
> + * Authors:
> + *  Frederic Konrad
> + *  Damien Hedde
> + *
> + * 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"
> +
> +/*
> + * qdev_init_clocklist:
> + * Add a new clock in a device
> + */
> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
> +                                           bool output, Clock *clk)
> +{
> +    NamedClockList *ncl;
> +
> +    /*
> +     * Clock must be added before realize() so that we can compute the
> +     * clock's canonical path durint device_realize().

s/durint/during/g

> +     */
> +    assert(!dev->realized);
> +
> +    /*
> +     * The ncl structure is freed by qdev_finalize_clocklist() which will
> +     * be called during @dev's device_finalize().
> +     */
> +    ncl = g_new0(NamedClockList, 1);
> +    ncl->name = g_strdup(name);
> +    ncl->output = output;
> +    ncl->alias = (clk != NULL);
> +
> +    /*
> +     * Trying to create a clock whose name clashes with some other
> +     * clock or property is a bug in the caller and we will abort().
> +     */
> +    if (clk == NULL) {
> +        clk = CLOCK(object_new(TYPE_CLOCK));
> +        object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort);
> +        if (output) {
> +            /*
> +             * Remove object_new()'s initial reference.
> +             * Note that for inputs, the reference created by object_new()
> +             * will be deleted in qdev_finalize_clocklist().
> +             */
> +            object_unref(OBJECT(clk));
> +        }
> +    } else {
> +        object_property_add_link(OBJECT(dev), name,
> +                                 object_get_typename(OBJECT(clk)),
> +                                 (Object **) &ncl->clock,
> +                                 NULL, OBJ_PROP_LINK_STRONG, &error_abort);
> +    }
> +
> +    ncl->clock = clk;
> +
> +    QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
> +    return ncl;
> +}
> +
> +void qdev_finalize_clocklist(DeviceState *dev)
> +{
> +    /* called by @dev's device_finalize() */
> +    NamedClockList *ncl, *ncl_next;
> +
> +    QLIST_FOREACH_SAFE(ncl, &dev->clocks, node, ncl_next) {
> +        QLIST_REMOVE(ncl, node);
> +        if (!ncl->output && !ncl->alias) {
> +            /*
> +             * We kept a reference on the input clock to ensure it lives up to
> +             * this point so we can safely remove the callback.
> +             * It avoids having a callback to a deleted object if ncl->clock
> +             * is still referenced somewhere else (eg: by a clock output).
> +             */
> +            clock_clear_callback(ncl->clock);
> +            object_unref(OBJECT(ncl->clock));
> +        }
> +        g_free(ncl->name);
> +        g_free(ncl);
> +    }
> +}
> +
> +Clock *qdev_init_clock_out(DeviceState *dev, const char *name)
> +{
> +    NamedClockList *ncl;
> +
> +    assert(name);
> +
> +    ncl = qdev_init_clocklist(dev, name, true, NULL);
> +
> +    return ncl->clock;
> +}
> +
> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
> +                            ClockCallback *callback, void *opaque)
> +{
> +    NamedClockList *ncl;
> +
> +    assert(name);
> +
> +    ncl = qdev_init_clocklist(dev, name, false, NULL);
> +
> +    if (callback) {
> +        clock_set_callback(ncl->clock, callback, opaque);
> +    }
> +    return ncl->clock;
> +}
> +
> +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;
> +        }
> +    }
> +
> +    assert(false);

Remove this.

Otherwise it looks good.

Alistair

> +    return NULL;
> +}
> +
> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name)
> +{
> +    NamedClockList *ncl;
> +
> +    assert(name);
> +
> +    ncl = qdev_get_clocklist(dev, name);
> +    assert(!ncl->output);
> +
> +    return ncl->clock;
> +}
> +
> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name)
> +{
> +    NamedClockList *ncl;
> +
> +    assert(name);
> +
> +    ncl = qdev_get_clocklist(dev, name);
> +    assert(ncl->output);
> +
> +    return ncl->clock;
> +}
> +
> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
> +                        DeviceState *alias_dev, const char *alias_name)
> +{
> +    NamedClockList *ncl;
> +
> +    assert(name && alias_name);
> +
> +    ncl = qdev_get_clocklist(dev, name);
> +
> +    qdev_init_clocklist(alias_dev, alias_name, ncl->output, ncl->clock);
> +
> +    return ncl->clock;
> +}
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3937d1eb1a..f390697348 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/qdev-clock.h"
>  #include "migration/vmstate.h"
>  #include "trace.h"
>
> @@ -855,6 +856,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      HotplugHandler *hotplug_ctrl;
>      BusState *bus;
> +    NamedClockList *ncl;
>      Error *local_err = NULL;
>      bool unattached_parent = false;
>      static int unattached_count;
> @@ -902,6 +904,13 @@ 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(ncl, &dev->clocks, node) {
> +            if (ncl->alias) {
> +                continue;
> +            } else {
> +                clock_setup_canonical_path(ncl->clock);
> +            }
> +        }
>
>          if (qdev_get_vmsd(dev)) {
>              if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
> @@ -1025,6 +1034,7 @@ static void device_initfn(Object *obj)
>      dev->allow_unplug_during_migration = false;
>
>      QLIST_INIT(&dev->gpios);
> +    QLIST_INIT(&dev->clocks);
>  }
>
>  static void device_post_init(Object *obj)
> @@ -1054,6 +1064,8 @@ static void device_finalize(Object *obj)
>           */
>      }
>
> +    qdev_finalize_clocklist(dev);
> +
>      /* Only send event if the device had been completely realized */
>      if (dev->pending_deleted_event) {
>          g_assert(dev->canonical_path);
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index e3d796fdd4..2fdcb7dd00 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,7 +7,7 @@ common-obj-y += hotplug.o
>  common-obj-y += vmstate-if.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
> -common-obj-y += clock.o
> +common-obj-y += clock.o qdev-clock.o
>
>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index edcbd475aa..5a4511a86f 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -436,6 +436,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
>         hw/core/fw-path-provider.o \
>         hw/core/reset.o \
>         hw/core/vmstate-if.o \
> +       hw/core/clock.o hw/core/qdev-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.24.1
>
>

Re: [PATCH v7 3/9] qdev: add clock input&output support to devices.

[Damien Hedde]

On 2/25/20 2:27 AM, Alistair Francis wrote:
> /On Mon, Feb 24, 2020 at 9:12 AM Damien Hedde <damien.hedde@greensocs.com> wrote
>>
>> Add functions to easily handle clocks with devices.
>> Clock inputs and outputs should be used to handle clock propagation
>> between devices.
>> The API is very similar the GPIO API.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> I did not changed the constness of @name pointer field in
>> NamedClockList structure.
>> There is no obstacle to do it but the fact that we need to free the
>> allocated data it points to. It is possible to make it const and
>> hack/remove the const to call g_free but I don't know if its
>> allowed in qemu.
>>
>> v7:
>> + update ClockIn/Out types
>> + qdev_connect_clock_out function removed / qdev_connect_clock_in added
>>   instead
>> + qdev_pass_clock renamed to qdev_alias_clock
>> + various small fixes (typos, comment, asserts) (Peter)
>> + move device's instance_finalize code related to clock in qdev-clock.c
>> ---
>>  include/hw/qdev-clock.h | 105 +++++++++++++++++++++++++
>>  include/hw/qdev-core.h  |  12 +++
>>  hw/core/qdev-clock.c    | 169 ++++++++++++++++++++++++++++++++++++++++
>>  hw/core/qdev.c          |  12 +++
>>  hw/core/Makefile.objs   |   2 +-
>>  tests/Makefile.include  |   1 +
>>  6 files changed, 300 insertions(+), 1 deletion(-)
>>  create mode 100644 include/hw/qdev-clock.h
>>  create mode 100644 hw/core/qdev-clock.c
>>
>> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
>> new file mode 100644
>> index 0000000000..899a95ca6a
>> --- /dev/null
>> +++ b/include/hw/qdev-clock.h
>> @@ -0,0 +1,105 @@
>> +/*
>> + * Device's clock input and output
>> + *
>> + * Copyright GreenSocs 2016-2020
>> + *
>> + * Authors:
>> + *  Frederic Konrad
>> + *  Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QDEV_CLOCK_H
>> +#define QDEV_CLOCK_H
>> +
>> +#include "hw/clock.h"
>> +
>> +/**
>> + * qdev_init_clock_in:
>> + * @dev: the device to add an input clock 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 an input clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + * The callback will be called with @opaque as opaque parameter.
>> + */
>> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +                          ClockCallback *callback, void *opaque);
>> +
>> +/**
>> + * qdev_init_clock_out:
>> + * @dev: the device to add an output clock to
>> + * @name: the name of the clock (can't be NULL).
>> + * @callback: optional callback to be called on update or NULL.
> 
> qdev_init_clock_out() doesn't have a callback.
> 
>> + * @returns: a pointer to the newly added clock
> 
>> + *
>> + * Add an output clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + */
>> +Clock *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 input clock @name from @dev or NULL if does not exist.
>> + */
>> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_get_clock_out:
>> + * @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 output clock @name from @dev or NULL if does not exist.
>> + */
>> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_connect_clock_in:
>> + * @dev: a device
>> + * @name: the name of an input clock in @dev
>> + * @source: the source clock (an output clock of another device for example)
>> + *
>> + * Set the source clock of input clock @name of device @dev to @source.
>> + * @source period update will be propagated to @name clock.
>> + */
>> +static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
>> +                                         Clock *source)
>> +{
>> +    clock_set_source(qdev_get_clock_in(dev, name), source);
>> +}
>> +
>> +/**
>> + * qdev_alias_clock:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock in @dev (can't be NULL)
>> + * @alias_dev: the device to add the clock
>> + * @alias_name: the name of the clock in @container
>> + * @returns: a pointer to the clock
>> + *
>> + * Add a clock @alias_name in @alias_dev which is an alias of the clock @name
>> + * in @dev. The direction _in_ or _out_ will the same as the original.
>> + * An alias clock must not be modified or used by @alias_dev and should
>> + * typically be only only for device composition purpose.
>> + */
>> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>> +                        DeviceState *alias_dev, const char *alias_name);
>> +
>> +/**
>> + * qdev_finalize_clocklist:
>> + * @dev: the device being finalize
> 
> It probably should be:
> 
> @dev: the device being finalized
> 
>> + *
>> + * Clear the clocklist from @dev. Only used internally in qdev.
>> + */
>> +void qdev_finalize_clocklist(DeviceState *dev);
>> +
>> +#endif /* QDEV_CLOCK_H */
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 1405b8a990..d87d989e72 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -149,6 +149,17 @@ struct NamedGPIOList {
>>      QLIST_ENTRY(NamedGPIOList) node;
>>  };
>>
>> +typedef struct Clock Clock;
>> +typedef struct NamedClockList NamedClockList;
>> +
>> +struct NamedClockList {
>> +    char *name;
>> +    Clock *clock;
>> +    bool output;
>> +    bool alias;
>> +    QLIST_ENTRY(NamedClockList) node;
>> +};
>> +
>>  /**
>>   * DeviceState:
>>   * @realized: Indicates whether the device has been fully constructed.
>> @@ -171,6 +182,7 @@ struct DeviceState {
>>      bool allow_unplug_during_migration;
>>      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/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> new file mode 100644
>> index 0000000000..9af0159517
>> --- /dev/null
>> +++ b/hw/core/qdev-clock.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Device's clock input and output
>> + *
>> + * Copyright GreenSocs 2016-2020
>> + *
>> + * Authors:
>> + *  Frederic Konrad
>> + *  Damien Hedde
>> + *
>> + * 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"
>> +
>> +/*
>> + * qdev_init_clocklist:
>> + * Add a new clock in a device
>> + */
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
>> +                                           bool output, Clock *clk)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    /*
>> +     * Clock must be added before realize() so that we can compute the
>> +     * clock's canonical path durint device_realize().
> 
> s/durint/during/g
> 
>> +     */
>> +    assert(!dev->realized);
>> +
>> +    /*
>> +     * The ncl structure is freed by qdev_finalize_clocklist() which will
>> +     * be called during @dev's device_finalize().
>> +     */
>> +    ncl = g_new0(NamedClockList, 1);
>> +    ncl->name = g_strdup(name);
>> +    ncl->output = output;
>> +    ncl->alias = (clk != NULL);
>> +
>> +    /*
>> +     * Trying to create a clock whose name clashes with some other
>> +     * clock or property is a bug in the caller and we will abort().
>> +     */
>> +    if (clk == NULL) {
>> +        clk = CLOCK(object_new(TYPE_CLOCK));
>> +        object_property_add_child(OBJECT(dev), name, OBJECT(clk), &error_abort);
>> +        if (output) {
>> +            /*
>> +             * Remove object_new()'s initial reference.
>> +             * Note that for inputs, the reference created by object_new()
>> +             * will be deleted in qdev_finalize_clocklist().
>> +             */
>> +            object_unref(OBJECT(clk));
>> +        }
>> +    } else {
>> +        object_property_add_link(OBJECT(dev), name,
>> +                                 object_get_typename(OBJECT(clk)),
>> +                                 (Object **) &ncl->clock,
>> +                                 NULL, OBJ_PROP_LINK_STRONG, &error_abort);
>> +    }
>> +
>> +    ncl->clock = clk;
>> +
>> +    QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> +    return ncl;
>> +}
>> +
>> +void qdev_finalize_clocklist(DeviceState *dev)
>> +{
>> +    /* called by @dev's device_finalize() */
>> +    NamedClockList *ncl, *ncl_next;
>> +>> +    return NULL;
>> +}
>> +
>> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +    assert(!ncl->output);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +    assert(ncl->output);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>> +                        DeviceState *alias_dev, const char *alias_name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name && alias_name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +
>> +    qdev_init_clocklist(alias_dev, alias_name, ncl->output, ncl->clock);
>> +
>> +    return ncl->clock;
>> +}
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 3937d1eb1a..f390697348 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/qdev-clock.h"
>>  #include "migration/vmstate.h"
>>  #include "trace.h"
>>
>> @@ -855,6 +856,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>      HotplugHandler *hotplug_ctrl;
>>      BusState *bus;
>> +    NamedClockList *ncl;
>>      Error *local_err = NULL;
>>      bool unattached_parent = false;
>>      static int unattached_count;
>> @@ -902,6 +904,13 @@ 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(ncl, &dev->clocks, node) {
>> +            if (ncl->alias) {
>> +                continue;
>> +            } else {
>> +                clock_setup_canonical_path(ncl->clock);
>> +            }
>> +        }
>>
>>          if (qdev_get_vmsd(dev)) {
>>              if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
>> @@ -1025,6 +1034,7 @@ static void device_initfn(Object *obj)
>>      dev->allow_unplug_during_migration = false;
>>
>>      QLIST_INIT(&dev->gpios);
>> +    QLIST_INIT(&dev->clocks);
>>  }
>>
>>  static void device_post_init(Object *obj)
>> @@ -1054,6 +1064,8 @@ static void device_finalize(Object *obj)
>>           */
>>      }
>>
>> +    qdev_finalize_clocklist(dev);
>> +
>>      /* Only send event if the device had been completely realized */
>>      if (dev->pending_deleted_event) {
>>          g_assert(dev->canonical_path);
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index e3d796fdd4..2fdcb7dd00 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -7,7 +7,7 @@ common-obj-y += hotplug.o
>>  common-obj-y += vmstate-if.o
>>  # irq.o needed for qdev GPIO handling:
>>  common-obj-y += irq.o
>> -common-obj-y += clock.o
>> +common-obj-y += clock.o qdev-clock.o
>>
>>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index edcbd475aa..5a4511a86f 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -436,6 +436,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
>>         hw/core/fw-path-provider.o \
>>         hw/core/reset.o \
>>         hw/core/vmstate-if.o \
>> +       hw/core/clock.o hw/core/qdev-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.24.1
>>
>>
>> +    QLIST_FOREACH_SAFE(ncl, &dev->clocks, node, ncl_next) {
>> +        QLIST_REMOVE(ncl, node);
>> +        if (!ncl->output && !ncl->alias) {
>> +            /*
>> +             * We kept a reference on the input clock to ensure it lives up to
>> +             * this point so we can safely remove the callback.
>> +             * It avoids having a callback to a deleted object if ncl->clock
>> +             * is still referenced somewhere else (eg: by a clock output).
>> +             */
>> +            clock_clear_callback(ncl->clock);
>> +            object_unref(OBJECT(ncl->clock));
>> +        }
>> +        g_free(ncl->name);
>> +        g_free(ncl);
>> +    }
>> +}
>> +
>> +Clock *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_init_clocklist(dev, name, true, NULL);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +                            ClockCallback *callback, void *opaque)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_init_clocklist(dev, name, false, NULL);
>> +
>> +    if (callback) {
>> +        clock_set_callback(ncl->clock, callback, opaque);
>> +    }
>> +    return ncl->clock;
>> +}
>> +
>> +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;
>> +        }
>> +    }
>> +
>> +    assert(false);
> 
> Remove this.
You're right, It will fail right after anyway if we get there.

> 
> Otherwise it looks good.
> 
> Alistair
> 

Thanks,
Damien

[PATCH v7 4/9] qdev-clock: introduce an init array to ease the device construction

[Damien Hedde]

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>
--

v7:
 + update ClockIn/Out types
 + remove the QDEV_CLOCK_IN_NOFIELD macro
 + remove leading underscores in macro arguments (Peter)
 + updated some comments (Peter)
 + removed trivial asserts (Peter)
---
 include/hw/qdev-clock.h | 55 +++++++++++++++++++++++++++++++++++++++++
 hw/core/qdev-clock.c    | 17 +++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index 899a95ca6a..604b25fb85 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -102,4 +102,59 @@ Clock *qdev_alias_clock(DeviceState *dev, const char *name,
  */
 void qdev_finalize_clocklist(DeviceState *dev);
 
+/**
+ * ClockPortInitElem:
+ * @name: name of the clock (can't be NULL)
+ * @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(devstate, field) \
+    (offsetof(devstate, field) + \
+     type_check(Clock *, typeof_field(devstate, field)))
+
+#define QDEV_CLOCK(out_not_in, devstate, field, cb) { \
+    .name = (stringify(field)), \
+    .is_output = out_not_in, \
+    .callback = cb, \
+    .offset = clock_offset_value(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 Clock*)
+ * @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, devstate, field, callback)
+
+#define QDEV_CLOCK_OUT(devstate, field) \
+    QDEV_CLOCK(true, devstate, field, NULL)
+
+#define QDEV_CLOCK_END { .name = NULL }
+
+typedef struct ClockPortInitElem ClockPortInitArray[];
+
+/**
+ * qdev_init_clocks:
+ * @dev: the device 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 */
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index 9af0159517..9b0ddca056 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -116,6 +116,23 @@ Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
     return ncl->clock;
 }
 
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
+{
+    const struct ClockPortInitElem *elem;
+
+    for (elem = &clocks[0]; elem->name != NULL; elem++) {
+        Clock **clkp;
+        /* offset cannot be inside the DeviceState part */
+        assert(elem->offset > sizeof(DeviceState));
+        clkp = (Clock **)(((void *) dev) + elem->offset);
+        if (elem->is_output) {
+            *clkp = qdev_init_clock_out(dev, elem->name);
+        } else {
+            *clkp = qdev_init_clock_in(dev, elem->name, elem->callback, dev);
+        }
+    }
+}
+
 static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name)
 {
     NamedClockList *ncl;
-- 
2.24.1

[PATCH v7 5/9] docs/clocks: add device's clock documentation

[Damien Hedde]

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>
---

v7:
 + update ClockIn/Out types
---
 docs/devel/clocks.rst | 356 ++++++++++++++++++++++++++++++++++++++++++
 docs/devel/index.rst  |   1 +
 2 files changed, 357 insertions(+)
 create mode 100644 docs/devel/clocks.rst

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
new file mode 100644
index 0000000000..cc5563691d
--- /dev/null
+++ b/docs/devel/clocks.rst
@@ -0,0 +1,356 @@
+
+What are clocks
+===============
+
+Clocks are QOM objects developed for the purpose of modeling the
+distribution of clocks in QEMU.
+
+They allow 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 object is *Clock* and its QOM name is ``CLOCK``.
+
+Clocks are typically used with devices where they are used to model inputs
+and outputs. They are created in a similar way as gpios. Inputs and outputs
+of different devices can be connect together.
+
+In these cases a Clock object is a child of a Device object but this is not
+a requirement. Clocks can be independent of devices. For example it is possible
+to create a clock outside of any device to model the main clock source of a
+machine.
+
+Here is an example of clocks::
+
+    +---------+      +----------------------+   +--------------+
+    | Clock 1 |      |       Device B       |   |   Device C   |
+    |         |      | +-------+  +-------+ |   | +-------+    |
+    |         |>>-+-->>|Clock 2|  |Clock 3|>>--->>|Clock 6|    |
+    +---------+   |  | | (in)  |  | (out) | |   | | (in)  |    |
+                  |  | +-------+  +-------+ |   | +-------+    |
+                  |  |            +-------+ |   +--------------+
+                  |  |            |Clock 4|>>
+                  |  |            | (out) | |   +--------------+
+                  |  |            +-------+ |   |   Device D   |
+                  |  |            +-------+ |   | +-------+    |
+                  |  |            |Clock 5|>>--->>|Clock 7|    |
+                  |  |            | (out) | |   | | (in)  |    |
+                  |  |            +-------+ |   | +-------+    |
+                  |  +----------------------+   |              |
+                  |                             | +-------+    |
+                  +----------------------------->>|Clock 8|    |
+                                                | | (in)  |    |
+                                                | +-------+    |
+                                                +--------------+
+
+Clocks are defined in include/hw/clock.h header and device related functions
+are defined in include/hw/qdev-clock.h header.
+
+The clock state
+===============
+
+The state of a clock is its period; it is stored as an integer representing
+it in 2^-32ns unit. 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.
+
+All clocks contain this state: outputs as well as inputs. It allows to fetch
+the current period of a clock at any time. When a clock is updated, the
+value is immediately propagated to all connected clocks in the tree.
+
+To ease interaction with clocks. Helpers with a unit suffix are defined for
+every clock state setter or getter. They are:
+  + ``_ns`` for handling periods in nanosecond, and
+  + ``_hz`` for handling frequencies in hertz.
+The 0 period value is converted to 0 in hertz and vice versa. 0 always means
+that the clock is disabled.
+
+Adding a new a clock
+====================
+
+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 callback (this will
+be explained in a following section below).
+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 Clock 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: one input and one
+output.
+
+::
+
+    /* device structure containing pointer to the clock objects */
+    typedef struct MyDeviceState {
+        DeviceState parent_obj;
+        Clock *clk_in;
+        Clock *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);
+        [...]
+    }
+
+An alternative way to create a clock is to simply call
+``object_new(TYPE_CLOCK)``. In that case the clock will neither be an input nor
+an output of a device. After the whole QOM hieracrhy of the clock has been set
+``clock_setup_canonical_path()`` should be called.
+
+At creation, the period of the clock is 0: the clock is disabled. You can
+change it using ``clock_set[_ns|_hz]()``.
+
+Note that if you are creating a clock with a fixed period which will never
+change (for example the main clock source of a board), then you'll have
+nothing else to do. This value will be propagated to other clocks when
+connecting the clocks together and devices will fetch the right value during
+the first reset.
+
+Retrieving clocks from a device
+===============================
+
+``qdev_get_clock_in()`` and ``dev_get_clock_out()`` are available to get the clock inputs or outputs of a device. For example::
+
+    Clock *clk = qdev_get_clock_in(DEVICE(mydev), "clk_in");
+
+or::
+
+    Clock *clk = qdev_get_clock_out(DEVICE(mydev), "clk_out");
+
+Connecting two clocks together
+==============================
+
+To connect two clocks together, use the ``clock_set_source()`` function.
+Given two clocks ``clk1``, and ``clk2``, ``clock_set_source(clk2, clk1);``
+configure ``clk2`` to follow the ``clk1`` period changes. Every time ``clk1``
+is updated, ``clk2`` will be updated too.
+
+When connecting clock between devices, prefer using the
+``qdev_connect_clock_in()`` function set the source of an input device clock.
+For example, to connect the input clock ``clk2`` of ``devB`` to the output
+clock ``clk1`` of ``devA``, do::
+
+    qdev_connect_clock_in(devB, "clk2", qdev_get_clock_out(devA, "clk1"))
+
+We used ``qdev_get_clock_out()`` above, but any clock can drive an input clock,
+even another input clock. The following diagram shows some
+examples of connections. Note also that a clock can drive several other clocks.
+
+::
+
+  +------------+  +--------------------------------------------------+
+  |  Device A  |  |                   Device B                       |
+  |            |  |               +---------------------+            |
+  |            |  |               |       Device C      |            |
+  |  +-------+ |  | +-------+     | +-------+ +-------+ |  +-------+ |
+  |  |Clock 1|>>-->>|Clock 2|>>+-->>|Clock 3| |Clock 5|>>>>|Clock 6|>>
+  |  | (out) | |  | | (in)  |  |  | | (in)  | | (out) | |  | (out) | |
+  |  +-------+ |  | +-------+  |  | +-------+ +-------+ |  +-------+ |
+  +------------+  |            |  +---------------------+            |
+                  |            |                                     |
+                  |            |  +--------------+                   |
+                  |            |  |   Device D   |                   |
+                  |            |  | +-------+    |                   |
+                  |            +-->>|Clock 4|    |                   |
+                  |               | | (in)  |    |                   |
+                  |               | +-------+    |                   |
+                  |               +--------------+                   |
+                  +--------------------------------------------------+
+
+In the above example, when *Clock 1* is updated by *Device A*, three clocks gets the new clock period value: *Clock 2*, Clock 3* and *Clock 4*.
+
+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 is disabled (period of 0). It means the clock will
+be considered as disabled until the period is updated. If the clock remains
+unconnected it will always keep its initial value of 0. If this is not the
+wanted behaviour, ``clock_set()``, ``clock_set_ns()`` or ``clock_set_hz()``
+should be called on the Clock object during device instance init. For example::
+
+    clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback,
+                             dev);
+    /* set initial value to 10ns / 100MHz */
+    clock_set_ns(clk, 10);
+
+Fetching clock frequency/period
+===============================
+
+To get the current state of a clock, the function ``clock_get()``,
+``clock_get_ns()`` or ``clock_get_hz()`` must be used.
+
+It is also possible to register a callback on 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 period */
+        fprintf(stdout, "device new period is %" PRIu64 "ns\n",
+                        clock_get_ns(dev->my_clk_input));
+    }
+
+Changing a clock period
+=======================
+
+A device can change its outputs using the ``clock_update()``,
+``clock_update_ns()`` or ``clock_update_hz()`` function. It will trigger
+updates on every connected 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_update_hz(dev->clkout, 1000 * 1000 * 1000); /* 1Ghz */
+
+Because updating a clock may trigger any side effects through connected clocks
+and their callbacks, this operation must be done while holding the qemu io lock.
+
+For the same reason, one can updates clocks only when it is allowed to have
+side effects on other objects. In consequence, it is forbidden:
++ during migration,
++ and in the enter phase of reset.
+
+Note that calling ``clock_update[_ns|_hz]()`` is equivalent to call
+``clock_set[_ns|_hz]()`` (with the same arguments) then ``clock_propagate()`` on
+the clock. Thus, setting the clock value can separated from triggering the
+side-effects. This is often required to factorize code to handle reset and
+migration in devices.
+
+Aliasing 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_alias_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 and 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 */
+        [...]
+        qdev_alias_clock(B, "clk", A, "b_clk");
+        /*
+         * Now A has a clock "b_clk" which is an alias to
+         * the clock "clk" of its child B.
+         */
+    }
+
+This function does not return any clock object. The new clock has the same
+direction (input or output) as the original one. This function only adds a link
+to the existing clock. In the above example, B object remains the only object
+allowed to use the clock and device A must not try to change the clock period
+or set a callback to the clock. Here follows a diagram describing the example
+with an input clock::
+
+    +--------------------------+
+    |        Device A          |
+    |         +--------------+ |
+    |         |   Device B   | |
+    |         | +-------+    | |
+    >>"b_clk">>>| "clk" |    | |
+    |  (in)   | |  (in) |    | |
+    |         | +-------+    | |
+    |         +--------------+ |
+    +--------------------------+
+
+Migration
+=========
+
+Clock state are not migrated automatically. Every device must handle its
+clock migration. Alias clocks must not be migrated.
+
+To ensure clock states are restored correctly during migration, there is two
+solutions.
+
+Clocks states can be migrated by adding an entry into the device
+vmstate description. To this purpose, the ``VMSTATE_CLOCK`` macro defines
+such an entry and should be used. This is typically used to migrate an input
+clock state. The following example describes it::
+
+    MyDeviceState {
+        DeviceState parent_obj;
+        [...] /* some fields */
+        Clock *clk;
+    };
+
+    VMStateDescription my_device_vmstate = {
+        .name = "my_device",
+        .fields = (VMStateField[]) {
+            [...], /* other migrated fields */
+            VMSTATE_CLOCK(clk, MyDeviceState),
+            VMSTATE_END_OF_LIST()
+        }
+    };
+
+The second solution is to restore the clock state using information already
+at our disposal. This can be used to restore output clocks states using the
+device state. The functions ``clock_set[_ns|_hz]()`` can be used during
+``post_load()`` migration callback.
+
+When adding a clock support to an existing device, if you care about migration
+compatibility. To this end, you can use ``clock_set()`` in a ``pre_load()``
+function to setup a default value in case the source virtual machine does not
+send the clock state. You may also need to use a vmstate subsection.
+
+Care should be taken not to use ``clock_update[_ns|_hz]()`` or
+``clock_propagate()`` during the whole migration procedure because it will
+trigger side effects to other devices in an unknown state.
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 4dc2ca8d71..f9c8c668ee 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -25,3 +25,4 @@ Contents:
    tcg-plugins
    bitops
    reset
+   clocks
-- 
2.24.1

[PATCH v7 6/9] hw/misc/zynq_slcr: add clock generation for uarts

[Damien Hedde]

Add some clocks to zynq_slcr
+ the main input clock (ps_clk)
+ the reference clock outputs for each uart (uart0 & 1)

This commit also transitional the slcr to multi-phase reset as it is
required to initialize the clocks correctly.

The clock frequencies are computed using the internal pll & uart configuration
registers and the input ps_clk frequency.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

v7:
  + handle migration of input clock
  + update ClockIn/ClockOut types
  + comments correction/precision (Peter)
---
 hw/misc/zynq_slcr.c | 172 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 168 insertions(+), 4 deletions(-)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index b9a38272d9..f7472d1f3c 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,127 @@ typedef struct ZynqSLCRState {
     MemoryRegion iomem;
 
     uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+    Clock *ps_clk;
+    Clock *uart0_ref_clk;
+    Clock *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;
+    }
+
+    /* frequency multiplier -> period division */
+    return input / mult;
+}
+
+/*
+ * return the output period of a clock given:
+ * + the periods in an array corresponding to input mux selector
+ * + the register xxx_CLK_CTRL value
+ * + enable bit index in ctrl register
+ *
+ * This function makes the assumption that the ctrl_reg value is organized as
+ * follows:
+ * + bits[13:8]  clock frequency divisor
+ * + bits[5:4]   clock mux selector (index in array)
+ * + bits[index] clock enable
+ */
+static uint64_t zynq_slcr_compute_clock(const uint64_t periods[],
+                                        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 disabled */
+    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 6-bit divider provides a divide range of 1 to 63"
+     * We follow here what is implemented in linux kernel and consider
+     * the 0 value as a bypass (no division).
+     */
+    /* frequency divisor -> period multiplication */
+    return periods[srcsel] * (divisor ? divisor : 1u);
+}
+
+/*
+ * macro helper around zynq_slcr_compute_clock to avoid repeating
+ * the register name.
+ */
+#define ZYNQ_COMPUTE_CLK(state, plls, reg, enable_field) \
+    zynq_slcr_compute_clock((plls), (state)->regs[reg], \
+                            reg ## _ ## enable_field ## _SHIFT)
+
+/**
+ * Compute and set the ouputs clocks periods.
+ * But do not propagate them further. Connected clocks
+ * will not receive any updates (See zynq_slcr_compute_clocks())
+ */
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+{
+    uint64_t ps_clk = clock_get(s->ps_clk);
+
+    /* consider outputs clocks are disabled while in reset */
+    if (device_is_in_reset(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(s->uart0_ref_clk,
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0));
+    clock_set(s->uart1_ref_clk,
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1));
+}
+
+/**
+ * Propagate the outputs clocks.
+ * zynq_slcr_compute_clocks() should have been called before
+ * to configure them.
+ */
+static void zynq_slcr_propagate_clocks(ZynqSLCRState *s)
 {
-    ZynqSLCRState *s = ZYNQ_SLCR(d);
+    clock_propagate(s->uart0_ref_clk);
+    clock_propagate(s->uart1_ref_clk);
+}
+
+static void zynq_slcr_ps_clk_callback(void *opaque)
+{
+    ZynqSLCRState *s = (ZynqSLCRState *) opaque;
+    zynq_slcr_compute_clocks(s);
+    zynq_slcr_propagate_clocks(s);
+}
+
+static void zynq_slcr_reset_init(Object *obj, ResetType type)
+{
+    ZynqSLCRState *s = ZYNQ_SLCR(obj);
     int i;
 
     DB_PRINT("RESET\n");
@@ -277,6 +404,23 @@ 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);
+    zynq_slcr_propagate_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);
+    zynq_slcr_propagate_clocks(s);
+}
 
 static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
 {
@@ -409,6 +553,13 @@ 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);
+        zynq_slcr_propagate_clocks(s);
+        break;
     }
 }
 
@@ -418,6 +569,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,14 +583,17 @@ 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 = {
     .name = "zynq_slcr",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, ZynqSLCRState, ZYNQ_SLCR_NUM_REGS),
+        VMSTATE_CLOCK_V(ps_clk, ZynqSLCRState, 3),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -440,9 +601,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.enter = 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.24.1

[PATCH v7 7/9] hw/char/cadence_uart: add clock support

[Damien Hedde]

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>

---

v7:
 + update ClockIn/ClockOut types
 + update due to resettable changes
 + use a versioned field instead subsection in vmstate
---
 include/hw/char/cadence_uart.h |  1 +
 hw/char/cadence_uart.c         | 73 +++++++++++++++++++++++++++++-----
 hw/char/trace-events           |  3 ++
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
index 47cec956c4..2a179a572f 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;
+    Clock *refclk;
 } CadenceUARTState;
 
 static inline DeviceState *cadence_uart_create(hwaddr addr,
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 22e47972f1..e196906c92 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_hz(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_in_reset(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, QEMUChrEvent event)
     CadenceUARTState *s = opaque;
     uint8_t buf = '\0';
 
+    /* ignore characters when unclocked or in reset */
+    if (!clock_is_enabled(s->refclk) || device_is_in_reset(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_set_hz(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 refclk field is present */
+    clock_set_hz(s->refclk, UART_DEFAULT_REF_CLK);
+    return 0;
+}
+
 static int cadence_uart_post_load(void *opaque, int version_id)
 {
     CadenceUARTState *s = opaque;
@@ -521,8 +570,9 @@ static int cadence_uart_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_cadence_uart = {
     .name = "cadence_uart",
-    .version_id = 2,
+    .version_id = 3,
     .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),
@@ -534,8 +584,9 @@ static const VMStateDescription vmstate_cadence_uart = {
         VMSTATE_UINT32(tx_count, CadenceUARTState),
         VMSTATE_UINT32(rx_wpos, CadenceUARTState),
         VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
+        VMSTATE_CLOCK_V(refclk, CadenceUARTState, 3),
         VMSTATE_END_OF_LIST()
-    }
+    },
 };
 
 static Property cadence_uart_properties[] = {
@@ -546,10 +597,12 @@ 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;
+    rc->phases.enter = cadence_uart_reset_init;
+    rc->phases.hold  = cadence_uart_reset_hold;
     device_class_set_props(dc, cadence_uart_properties);
   }
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 6f938301d9..d20eafd56f 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -97,3 +97,6 @@ exynos_uart_wo_read(uint32_t channel, const char *name, uint32_t reg) "UART%d: T
 exynos_uart_rxsize(uint32_t channel, uint32_t size) "UART%d: Rx FIFO size: %d"
 exynos_uart_channel_error(uint32_t channel) "Wrong UART channel number: %d"
 exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d: Rx timeout stat=0x%x intsp=0x%x"
+
+# hw/char/cadence_uart.c
+cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
-- 
2.24.1

[PATCH v7 8/9] hw/arm/xilinx_zynq: connect uart clocks to slcr

[Damien Hedde]

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>

---
v7
 + update ClockIn/ClockOut types
 + simplify the ps_clk frequency init
---
 hw/arm/xilinx_zynq.c | 57 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3a0fa5b23f..261a1690a8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -34,6 +34,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
@@ -74,6 +83,11 @@ static const int dma_irqs[8] = {
     0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
     0xe5801000 + (addr)
 
+typedef struct ZynqMachineState {
+    MachineState parent;
+    Clock *ps_clk;
+} ZynqMachineState;
+
 static void zynq_write_board_setup(ARMCPU *cpu,
                                    const struct arm_boot_info *info)
 {
@@ -158,12 +172,13 @@ 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;
     ARMCPU *cpu;
     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;
@@ -208,9 +223,18 @@ 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(object_new(TYPE_CLOCK));
+    object_property_add_child(OBJECT(zynq_machine), "ps_clk",
+                              OBJECT(zynq_machine->ps_clk), &error_abort);
+    object_unref(OBJECT(zynq_machine->ps_clk));
+    clock_set_hz(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
+    qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
 
     dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -231,8 +255,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_in(dev, "refclk",
+                          qdev_get_clock_out(slcr, "uart0_ref_clk"));
+    dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    qdev_connect_clock_in(dev, "refclk",
+                          qdev_get_clock_out(slcr, "uart1_ref_clk"));
 
     sysbus_create_varargs("cadence_ttc", 0xF8001000,
             pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
@@ -310,8 +338,9 @@ static void zynq_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &zynq_binfo);
 }
 
-static void zynq_machine_init(MachineClass *mc)
+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->max_cpus = 1;
@@ -320,4 +349,16 @@ static void zynq_machine_init(MachineClass *mc)
     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.24.1

[PATCH v7 9/9] qdev-monitor: print the device's clock with info qtree

[Damien Hedde]

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.

Here follows a sample of `info qtree` output on xilinx_zynq machine
after linux boot with only one uart clocked:
> bus: main-system-bus
>  type System
>  [...]
>  dev: cadence_uart, id ""
>    gpio-out "sysbus-irq" 1
>    clock-in "refclk" freq_hz=0.000000e+00
>    chardev = ""
>    mmio 00000000e0001000/0000000000001000
>  dev: cadence_uart, id ""
>    gpio-out "sysbus-irq" 1
>    clock-in "refclk" freq_hz=1.375661e+07
>    chardev = "serial0"
>    mmio 00000000e0000000/0000000000001000
>  [...]
>  dev: xilinx,zynq_slcr, id ""
>    clock-out "uart1_ref_clk" freq_hz=0.000000e+00
>    clock-out "uart0_ref_clk" freq_hz=1.375661e+07
>    clock-in "ps_clk" freq_hz=3.333333e+07
>    mmio 00000000f8000000/0000000000001000

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

v7:
 + print output clocks frequencies too
 + add sample of qtree message above
 + display frequencies in floating-point
---
 qdev-monitor.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 8ce71a206b..1d84b4e416 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -38,6 +38,7 @@
 #include "migration/misc.h"
 #include "migration/migration.h"
 #include "qemu/cutils.h"
+#include "hw/clock.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -736,6 +737,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
     ObjectClass *class;
     BusState *child;
     NamedGPIOList *ngl;
+    NamedClockList *ncl;
 
     qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
                 dev->id ? dev->id : "");
@@ -750,6 +752,13 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
                         ngl->num_out);
         }
     }
+    QLIST_FOREACH(ncl, &dev->clocks, node) {
+        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
+                    ncl->output ? "out" : "in",
+                    ncl->alias ? " (alias)" : "",
+                    ncl->name,
+                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
+    }
     class = object_get_class(OBJECT(dev));
     do {
         qdev_print_props(mon, dev, DEVICE_CLASS(class)->props_, indent);
-- 
2.24.1

Re: [PATCH v7 0/9] Clock framework API

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200224170301.246623-1-damien.hedde@greensocs.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/blkdebug.o
  CC      block/blkverify.o

Warning, treated as error:
/tmp/qemu-test/src/docs/devel/clocks.rst:63:Unexpected indentation.
  CC      block/blkreplay.o
  CC      block/parallels.o
---
  CC      block/block-backend.o
  CC      block/snapshot.o
  CC      block/qapi.o
make: *** [Makefile:1061: docs/devel/index.html] Error 2
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=fbce66ee9f48427cbd21cf2fec369d49', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-u8hmmz2e/src/docker-src.2020-02-24-13.03.13.16369:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=fbce66ee9f48427cbd21cf2fec369d49
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-u8hmmz2e/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m5.646s
user    0m8.502s


The full log is available at
http://patchew.org/logs/20200224170301.246623-1-damien.hedde@greensocs.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v7 0/9] Clock framework API

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200224170301.246623-1-damien.hedde@greensocs.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      net/trace.o
  CC      ui/trace.o

Warning, treated as error:
/tmp/qemu-test/src/docs/devel/clocks.rst:63:Unexpected indentation.
  CC      hw/core/trace.o
  CC      hw/display/trace.o
---
  CC      stubs/dump.o
  CC      stubs/error-printf.o
  CC      stubs/fdset.o
make: *** [Makefile:1061: docs/devel/index.html] Error 2
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=89dbd9a708664da584fcef20d091e8cb', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-2vh2fz33/src/docker-src.2020-02-24-14.14.10.890:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=89dbd9a708664da584fcef20d091e8cb
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2vh2fz33/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m36.883s
user    0m8.075s


The full log is available at
http://patchew.org/logs/20200224170301.246623-1-damien.hedde@greensocs.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com