* [Qemu-devel] [PATCH v5 0/9] Clock framework API.
@ 2018-10-02 14:24 Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects Damien Hedde
` (10 more replies)
0 siblings, 11 replies; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
This series aims to add a way to model clocks in qemu between devices.
This allows to model the clock tree of a platform allowing us to inspect clock
configuration and detect problems such as disabled clock or bad configured
pll.
This series is a reroll of the v4 sent recently without the reset feature as
requested by Peter. I also took into account the reviews about migration and
other suggestions.
This framework was originally discussed in 2017, here:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07218.html
For the user, the framework is now very similar to the device's gpio API.
Clocks inputs and outputs can be added in devices during initialization phase.
Then an input can be connected to an output: it means every time the output
clock changes, a callback in the input is triggered allowing any action to be
taken. A difference with gpios is that several inputs can be connected to a
single output without doing any glue.
Behind the scene, there is 2 objects: a clock input which is a placeholder for
a callback, and a clock output which is a list of inputs. The value transferred
between an output and an input is an integer representing the clock frequency.
The input clock callback is called every time the clock frequency changes.
The input side holds a cached value of the frequency (the output does not need
one). This allows a device to fetch its input clock frequency at any time
without caching it itself.
This internal state is added to handle migration in order not to update and
propagate clocks during it (it adds cross-device and order-specific effects).
Each device handles its input clock migration by adding the clock frequency in
its own vmstate description.
Regarding the migration strategy, discussion started in the v4 patches.
The problem is that we add some kind of wire between different devices and
we face propagation issue.
The chosen solution allows migration compatibility from a platform version
with no implemented clocks to a platform with clocks. A migrated device that
have a new input clock is responsible to initialize its frequency during
migration. Each input clock having its own state, such initialization will not
have any side-effect on other input clock connected to the same output.
Output clocks, having no state, are not set during migration: If a clock output
frequency changes due to migration (eg: clock computation bug-fix), the effects
will be delayed. Eventually the clock output will be updated after migration if
some (software) event trigger a clock update, but it can not be guaranteed.
There is also the problem of initialization which is very much like the
migration. Currently, in the zynq example, clocks outputs are initialized in
the clock controller device_reset. According to Peter, outputs should not be
set in device_reset, so we need a better way.
It is not simple, since we can have complicated cases with several propagation
step.
We can't initialize outputs (without propagating) during device init and uses
inputs value in device_reset to complete the initialization.
Consider the case where we have a device generating a frequency, another device
doing a clock division, then a device using this clock.
[DevClockGenerator] -> clk1 -> [DevClockDiv] -> clk2 -> [Dev]
I don't see how we can handle such initialization without doing some
propagation phase at some point.
Regarding clock gating. The 0 frequency value means the clock is gated.
If need be, a gate device can be built taking an input gpio and clock and
generating an output clock.
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.
We are considering switching to a generic payload evolution of this API.
For example by specifying the qom carried type when adding an input/output to
a device. This would allow us, for example, to add a power input port to handle
power gating or others ports without modifying the device state structure.
Any comments and suggestion are welcomed.
The patches are organised as follows:
+ Patches 1 to 4 adds the clock support in qemu.
+ Patch 5 add some documentation in docs/devel
+ Patches 6 to 9 adds the uart's clocks to the xilinx_zynq platform as an
example for this framework. It updates the zynq's slcr clock controller, the
cadence_uart device, and the zynq toplevel platform.
Compared to v4, the changes are:
- no more reset flag, we are talking frequency only here
- name changes to better match gpio api
- migration strategy change
- sysbus patch removed (was becoming complicated due too migration changes)
Compared to v3, the following notable changes happen:
- Bindings are now fixed during machine realisation,
- Input clock objects have been removed from the point of view of the
user, i.e. nothing need to be added in the device state. A device can
now declare a input clock by calling qdev_init_clock_in() (similarly of
what is done for GPIOs),
- Callbacks called on clock change now return void. The input owner is
responsible of making necessary updates accordingly (e.g. update another
clock output, or modify its internal state) (again, mostly like GPIOs).
Thanks to the Xilinx QEMU team who sponsored this development.
Damien Hedde (9):
hw/core/clock-port: introduce clock port objects
qdev: add clock input&output support to devices.
qdev-monitor: print the device's clock with info qtree
qdev-clock: introduce an init array to ease the device construction
docs/clocks: add device's clock documentation
hw/misc/zynq_slcr: use standard register definition
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
docs/devel/clock.txt | 163 +++++++++
Makefile.objs | 1 +
include/hw/char/cadence_uart.h | 3 +
include/hw/clock-port.h | 136 ++++++++
include/hw/qdev-clock.h | 129 +++++++
include/hw/qdev-core.h | 14 +
include/hw/qdev.h | 1 +
hw/arm/xilinx_zynq.c | 17 +-
hw/char/cadence_uart.c | 92 ++++-
hw/core/clock-port.c | 159 +++++++++
hw/core/qdev-clock.c | 166 +++++++++
hw/core/qdev.c | 29 ++
hw/misc/zynq_slcr.c | 607 ++++++++++++++++++++-------------
qdev-monitor.c | 12 +
hw/char/trace-events | 3 +
hw/core/Makefile.objs | 3 +-
hw/core/trace-events | 7 +
17 files changed, 1289 insertions(+), 253 deletions(-)
create mode 100644 docs/devel/clock.txt
create mode 100644 include/hw/clock-port.h
create mode 100644 include/hw/qdev-clock.h
create mode 100644 hw/core/clock-port.c
create mode 100644 hw/core/qdev-clock.c
create mode 100644 hw/core/trace-events
--
2.19.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:53 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices Damien Hedde
` (9 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Introduce clock port objects: ClockIn and ClockOut.
Theses ports may be used to distribute a clock from a object to several
other objects. The ClockIn object contains the current state of the
clock: the frequency.
A ClockIn may be connected to a ClockOut so that it receives update,
through the callback, whenever the Clockout is updated using the
ClockOut's set function.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Makefile.objs | 1 +
include/hw/clock-port.h | 136 ++++++++++++++++++++++++++++++++++
hw/core/clock-port.c | 159 ++++++++++++++++++++++++++++++++++++++++
hw/core/Makefile.objs | 1 +
hw/core/trace-events | 7 ++
5 files changed, 304 insertions(+)
create mode 100644 include/hw/clock-port.h
create mode 100644 hw/core/clock-port.c
create mode 100644 hw/core/trace-events
diff --git a/Makefile.objs b/Makefile.objs
index ce9c79235e..b29747075f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -210,6 +210,7 @@ trace-events-subdirs += hw/audio
trace-events-subdirs += hw/block
trace-events-subdirs += hw/block/dataplane
trace-events-subdirs += hw/char
+trace-events-subdirs += hw/core
trace-events-subdirs += hw/display
trace-events-subdirs += hw/dma
trace-events-subdirs += hw/hppa
diff --git a/include/hw/clock-port.h b/include/hw/clock-port.h
new file mode 100644
index 0000000000..8266549350
--- /dev/null
+++ b/include/hw/clock-port.h
@@ -0,0 +1,136 @@
+#ifndef CLOCK_PORT_H
+#define CLOCK_PORT_H
+
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "qemu/queue.h"
+#include "migration/vmstate.h"
+
+#define TYPE_CLOCK_IN "clock-in"
+#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
+#define TYPE_CLOCK_OUT "clock-out"
+#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
+
+typedef void ClockCallback(void *opaque);
+
+typedef struct ClockOut ClockOut;
+typedef struct ClockIn ClockIn;
+
+struct ClockIn {
+ /*< private >*/
+ Object parent_obj;
+ /*< private >*/
+ uint64_t frequency;
+ char *canonical_path; /* clock path cache */
+ ClockOut *driver; /* clock output controlling this clock */
+ ClockCallback *callback; /* local callback */
+ void *callback_opaque; /* opaque argument for the callback */
+ QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
+};
+
+struct ClockOut {
+ /*< private >*/
+ Object parent_obj;
+ /*< private >*/
+ char *canonical_path; /* clock path cache */
+ QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
+};
+
+extern const VMStateDescription vmstate_clockin;
+
+/*
+ * vmstate description entry to be added in device vmsd.
+ */
+#define VMSTATE_CLOCKIN(_field, _state) \
+ VMSTATE_CLOCKIN_V(_field, _state, 0)
+#define VMSTATE_CLOCKIN_V(_field, _state, _version) \
+ VMSTATE_STRUCT_POINTER_V(_field, _state, _version, vmstate_clockin, ClockIn)
+
+/**
+ * clock_out_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_out_setup_canonical_path(ClockOut *clk);
+
+/**
+ * clock_in_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_in_setup_canonical_path(ClockIn *clk);
+
+/**
+ * clock_add_callback:
+ * @clk: the clock to register the callback into
+ * @cb: the callback function
+ * @opaque: the argument to the callback
+ *
+ * Register a callback called on every clock update.
+ */
+void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
+
+/**
+ * clock_clear_callback:
+ * @clk: the clock to delete the callback from
+ *
+ * Unregister the callback registered with clock_set_callback.
+ */
+void clock_clear_callback(ClockIn *clk);
+
+/**
+ * clock_init_frequency:
+ * @clk: the clock to initialize.
+ * @freq: the clock's frequency in Hz or 0 if unclocked.
+ *
+ * Initialize the local cached frequency value of @clk to @freq.
+ * Note: this function must only be called during device inititialization
+ * or migration.
+ */
+void clock_init_frequency(ClockIn *clk, uint64_t freq);
+
+/**
+ * clock_connect:
+ * @clkin: the drived clock.
+ * @clkout: the driving clock.
+ *
+ * Setup @clkout to drive @clkin: Any @clkout update will be propagated
+ * to @clkin.
+ */
+void clock_connect(ClockIn *clkin, ClockOut *clkout);
+
+/**
+ * clock_set_frequency:
+ * @clk: the clock to update.
+ * @freq: the new clock's frequency in Hz or 0 if unclocked.
+ *
+ * Update the @clk to the new @freq.
+ * This change will be propagated through registered clock inputs.
+ */
+void clock_set_frequency(ClockOut *clk, uint64_t freq);
+
+/**
+ * clock_get_frequency:
+ * @clk: the clk to fetch the clock
+ *
+ * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
+ */
+static inline uint64_t clock_get_frequency(const ClockIn *clk)
+{
+ return clk ? clk->frequency : 0;
+}
+
+/**
+ * clock_is_enabled:
+ * @clk: a clock state
+ *
+ * @return: true if the clock is running. If @clk is NULL return false.
+ */
+static inline bool clock_is_enabled(const ClockIn *clk)
+{
+ return clock_get_frequency(clk) != 0;
+}
+
+#endif /* CLOCK_PORT_H */
diff --git a/hw/core/clock-port.c b/hw/core/clock-port.c
new file mode 100644
index 0000000000..25bab0fbed
--- /dev/null
+++ b/hw/core/clock-port.c
@@ -0,0 +1,159 @@
+/*
+ * Clock inputs and outputs
+ *
+ * Copyright GreenSocs 2016-2018
+ *
+ * Authors:
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ * Damien Hedde <damien.hedde@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "hw/clock-port.h"
+#include "hw/qdev-core.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+const VMStateDescription vmstate_clockin = {
+ .name = "clockin",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(frequency, ClockIn),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+#define CLOCK_PATH(_clk) (_clk->canonical_path)
+
+void clock_out_setup_canonical_path(ClockOut *clk)
+{
+ g_free(clk->canonical_path);
+ clk->canonical_path = object_get_canonical_path(OBJECT(clk));
+}
+
+void clock_in_setup_canonical_path(ClockIn *clk)
+{
+ g_free(clk->canonical_path);
+ clk->canonical_path = object_get_canonical_path(OBJECT(clk));
+}
+
+void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
+{
+ assert(clk);
+
+ clk->callback = cb;
+ clk->callback_opaque = opaque;
+}
+
+void clock_init_frequency(ClockIn *clk, uint64_t freq)
+{
+ assert(clk);
+
+ clk->frequency = freq;
+}
+
+void clock_clear_callback(ClockIn *clk)
+{
+ clock_set_callback(clk, NULL, NULL);
+}
+
+void clock_connect(ClockIn *clkin, ClockOut *clkout)
+{
+ assert(clkin && clkin->driver == NULL);
+ assert(clkout);
+
+ trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
+
+ QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
+ clkin->driver = clkout;
+}
+
+static void clock_disconnect(ClockIn *clk)
+{
+ if (clk->driver == NULL) {
+ return;
+ }
+
+ trace_clock_disconnect(CLOCK_PATH(clk));
+
+ clk->driver = NULL;
+ QLIST_REMOVE(clk, sibling);
+}
+
+void clock_set_frequency(ClockOut *clk, uint64_t freq)
+{
+ ClockIn *follower;
+ trace_clock_update(CLOCK_PATH(clk), freq);
+
+ QLIST_FOREACH(follower, &clk->followers, sibling) {
+ trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
+ if (follower->frequency != freq) {
+ follower->frequency = freq;
+ if (follower->callback) {
+ follower->callback(follower->callback_opaque);
+ }
+ }
+ }
+}
+
+static void clock_out_initfn(Object *obj)
+{
+ ClockOut *clk = CLOCK_OUT(obj);
+
+ QLIST_INIT(&clk->followers);
+}
+
+static void clock_out_finalizefn(Object *obj)
+{
+ ClockOut *clk = CLOCK_OUT(obj);
+ ClockIn *follower, *next;
+
+ /* clear our list of followers */
+ QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
+ clock_disconnect(follower);
+ }
+
+ g_free(clk->canonical_path);
+ clk->canonical_path = NULL;
+}
+
+static void clock_in_finalizefn(Object *obj)
+{
+ ClockIn *clk = CLOCK_IN(obj);
+
+ /* remove us from driver's followers list */
+ clock_disconnect(clk);
+
+ g_free(clk->canonical_path);
+ clk->canonical_path = NULL;
+}
+
+static const TypeInfo clock_out_info = {
+ .name = TYPE_CLOCK_OUT,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(ClockOut),
+ .instance_init = clock_out_initfn,
+ .instance_finalize = clock_out_finalizefn,
+};
+
+static const TypeInfo clock_in_info = {
+ .name = TYPE_CLOCK_IN,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(ClockIn),
+ .instance_finalize = clock_in_finalizefn,
+};
+
+static void clock_register_types(void)
+{
+ type_register_static(&clock_in_info);
+ type_register_static(&clock_out_info);
+}
+
+type_init(clock_register_types)
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index eb88ca979e..f7102121f4 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
# irq.o needed for qdev GPIO handling:
common-obj-y += irq.o
common-obj-y += hotplug.o
+common-obj-y += clock-port.o
common-obj-$(CONFIG_SOFTMMU) += nmi.o
common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
diff --git a/hw/core/trace-events b/hw/core/trace-events
new file mode 100644
index 0000000000..d4880ec138
--- /dev/null
+++ b/hw/core/trace-events
@@ -0,0 +1,7 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# hw/core/clock-port.c
+clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
+clock_disconnect(const char *clk) "'%s'"
+clock_update(const char *clk, uint64_t freq) "'%s' frequency %" PRIu64 "Hz"
+clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices.
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:36 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
` (8 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Add functions to easily add input or output clocks to a device.
The clock port objects are added as children of the device.
A function allows to connect two clocks together.
It should be called by some toplevel to make a connection between
2 (sub-)devices.
Also add a function which forwards a port to another device. This
function allows, in the case of device composition, to expose a
sub-device clock port as its own clock port.
This is really an alias: when forwarding an input, only one callback can
be registered on it since there is only one Clockin object behind all
aliases.
This is based on the original work of Frederic Konrad.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
include/hw/qdev-clock.h | 62 ++++++++++++++++++
include/hw/qdev-core.h | 14 ++++
include/hw/qdev.h | 1 +
hw/core/qdev-clock.c | 140 ++++++++++++++++++++++++++++++++++++++++
hw/core/qdev.c | 29 +++++++++
hw/core/Makefile.objs | 2 +-
6 files changed, 247 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..d76aa9f479
--- /dev/null
+++ b/include/hw/qdev-clock.h
@@ -0,0 +1,62 @@
+#ifndef QDEV_CLOCK_H
+#define QDEV_CLOCK_H
+
+#include "hw/clock-port.h"
+#include "hw/qdev-core.h"
+#include "qapi/error.h"
+
+/**
+ * qdev_init_clock_in:
+ * @dev: the device in which to add a clock
+ * @name: the name of the clock (can't be NULL).
+ * @callback: optional callback to be called on update or NULL.
+ * @opaque: argument for the callback
+ * @returns: a pointer to the newly added clock
+ *
+ * Add a input clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ * The callback will be called with @dev as opaque parameter.
+ */
+ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
+ ClockCallback *callback, void *opaque);
+
+/**
+ * qdev_init_clock_out:
+ * @dev: the device to add a clock to
+ * @name: the name of the clock (can't be NULL).
+ * @callback: optional callback to be called on update or NULL.
+ * @returns: a pointer to the newly added clock
+ *
+ * Add a output clock to device @dev as a clock named @name.
+ * This adds a child<> property.
+ */
+ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
+
+/**
+ * qdev_pass_clock:
+ * @dev: the device to forward the clock to
+ * @name: the name of the clock to be added (can't be NULL)
+ * @container: the device which already has the clock
+ * @cont_name: the name of the clock in the container device
+ *
+ * Add a clock @name to @dev which forward to the clock @cont_name in @container
+ */
+void qdev_pass_clock(DeviceState *dev, const char *name,
+ DeviceState *container, const char *cont_name);
+
+/**
+ * qdev_connect_clock:
+ * @dev: the drived clock device.
+ * @name: the drived clock name.
+ * @driver: the driving clock device.
+ * @driver_name: the driving clock name.
+ * @errp: error report
+ *
+ * Setup @driver_name output clock of @driver to drive @name input clock of
+ * @dev. Errors are trigerred if clock does not exists
+ */
+void qdev_connect_clock(DeviceState *dev, const char *name,
+ DeviceState *driver, const char *driver_name,
+ Error **errp);
+
+#endif /* QDEV_CLOCK_H */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f1fd0f8736..e6014d3a41 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -127,6 +127,19 @@ struct NamedGPIOList {
QLIST_ENTRY(NamedGPIOList) node;
};
+typedef struct NamedClockList NamedClockList;
+
+typedef struct ClockIn ClockIn;
+typedef struct ClockOut ClockOut;
+
+struct NamedClockList {
+ char *name;
+ bool forward;
+ ClockIn *in;
+ ClockOut *out;
+ QLIST_ENTRY(NamedClockList) node;
+};
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -147,6 +160,7 @@ struct DeviceState {
int hotplugged;
BusState *parent_bus;
QLIST_HEAD(, NamedGPIOList) gpios;
+ QLIST_HEAD(, NamedClockList) clocks;
QLIST_HEAD(, BusState) child_bus;
int num_child_bus;
int instance_id_alias;
diff --git a/include/hw/qdev.h b/include/hw/qdev.h
index 5cb8b080a6..b031da7b41 100644
--- a/include/hw/qdev.h
+++ b/include/hw/qdev.h
@@ -4,5 +4,6 @@
#include "hw/hw.h"
#include "hw/qdev-core.h"
#include "hw/qdev-properties.h"
+#include "hw/qdev-clock.h"
#endif
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
new file mode 100644
index 0000000000..f0e4839aed
--- /dev/null
+++ b/hw/core/qdev-clock.c
@@ -0,0 +1,140 @@
+/*
+ * Device's clock
+ *
+ * Copyright GreenSocs 2016-2018
+ *
+ * Authors:
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ * Damien Hedde <damien.hedde@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/object.h"
+#include "hw/qdev-clock.h"
+#include "qapi/error.h"
+
+static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
+ bool forward)
+{
+ NamedClockList *ncl;
+
+ /*
+ * The clock path will be computed by the device's realize function call.
+ * This is required to ensure the clock's canonical path is right and log
+ * messages are meaningfull.
+ */
+ assert(name);
+ assert(!dev->realized);
+
+ ncl = g_malloc0(sizeof(*ncl));
+ ncl->name = g_strdup(name);
+ ncl->forward = forward;
+
+ QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
+ return ncl;
+}
+
+ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+ Object *clk;
+
+ ncl = qdev_init_clocklist(dev, name, false);
+
+ clk = object_new(TYPE_CLOCK_OUT);
+
+ /* will fail if name already exists */
+ object_property_add_child(OBJECT(dev), name, clk, &error_abort);
+ object_unref(clk); /* remove the initial ref made by object_new */
+
+ ncl->out = CLOCK_OUT(clk);
+ return ncl->out;
+}
+
+ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
+ ClockCallback *callback, void *opaque)
+{
+ NamedClockList *ncl;
+ Object *clk;
+
+ ncl = qdev_init_clocklist(dev, name, false);
+
+ clk = object_new(TYPE_CLOCK_IN);
+ /*
+ * the ref initialized by object_new will be cleared during dev finalize.
+ * It allows us to safely remove the callback.
+ */
+
+ /* will fail if name already exists */
+ object_property_add_child(OBJECT(dev), name, clk, &error_abort);
+
+ ncl->in = CLOCK_IN(clk);
+ if (callback) {
+ clock_set_callback(ncl->in, callback, opaque);
+ }
+ return ncl->in;
+}
+
+static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name)
+{
+ NamedClockList *ncl;
+
+ QLIST_FOREACH(ncl, &dev->clocks, node) {
+ if (strcmp(name, ncl->name) == 0) {
+ return ncl;
+ }
+ }
+
+ return NULL;
+}
+
+void qdev_pass_clock(DeviceState *dev, const char *name,
+ DeviceState *container, const char *cont_name)
+{
+ NamedClockList *original_ncl, *ncl;
+ Object **clk;
+
+ assert(container && cont_name);
+
+ original_ncl = qdev_get_clocklist(container, cont_name);
+ assert(original_ncl); /* clock must exist in origin */
+
+ ncl = qdev_init_clocklist(dev, name, true);
+
+ if (ncl->out) {
+ clk = (Object **)&ncl->out;
+ } else {
+ clk = (Object **)&ncl->in;
+ }
+
+ /* will fail if name already exists */
+ object_property_add_link(OBJECT(dev), name, object_get_typename(*clk),
+ clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort);
+}
+
+void qdev_connect_clock(DeviceState *dev, const char *name,
+ DeviceState *driver, const char *driver_name,
+ Error **errp)
+{
+ NamedClockList *ncl, *drv_ncl;
+
+ assert(dev && name);
+ assert(driver && driver_name);
+
+ ncl = qdev_get_clocklist(dev, name);
+ if (!ncl || !ncl->in) {
+ error_setg(errp, "no input clock '%s' in device", name);
+ return;
+ }
+
+ drv_ncl = qdev_get_clocklist(driver, driver_name);
+ if (!drv_ncl || !drv_ncl->out) {
+ error_setg(errp, "no output clock '%s' in driver", driver_name);
+ return;
+ }
+
+ clock_connect(ncl->in , drv_ncl->out);
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 529b82de18..c48edf180f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -790,6 +790,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
DeviceClass *dc = DEVICE_GET_CLASS(dev);
HotplugHandler *hotplug_ctrl;
BusState *bus;
+ NamedClockList *clk;
Error *local_err = NULL;
bool unattached_parent = false;
static int unattached_count;
@@ -846,6 +847,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
*/
g_free(dev->canonical_path);
dev->canonical_path = object_get_canonical_path(OBJECT(dev));
+ QLIST_FOREACH(clk, &dev->clocks, node) {
+ if (clk->forward) {
+ continue;
+ } else if (clk->in != NULL) {
+ clock_in_setup_canonical_path(clk->in);
+ } else {
+ clock_out_setup_canonical_path(clk->out);
+ }
+ }
if (qdev_get_vmsd(dev)) {
if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
@@ -972,6 +982,7 @@ static void device_initfn(Object *obj)
(Object **)&dev->parent_bus, NULL, 0,
&error_abort);
QLIST_INIT(&dev->gpios);
+ QLIST_INIT(&dev->clocks);
}
static void device_post_init(Object *obj)
@@ -983,6 +994,7 @@ static void device_post_init(Object *obj)
static void device_finalize(Object *obj)
{
NamedGPIOList *ngl, *next;
+ NamedClockList *clk, *clk_next;
DeviceState *dev = DEVICE(obj);
@@ -996,6 +1008,23 @@ static void device_finalize(Object *obj)
*/
}
+ QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) {
+ QLIST_REMOVE(clk, node);
+ if (!clk->forward && clk->in) {
+ /*
+ * if this clock is not forwarded, clk->in & clk->out are child of
+ * dev.
+ * At this point the properties and associated reference are
+ * already deleted but we kept a ref on clk->in to ensure we
+ * don't have a lost callback to a deleted device somewhere.
+ */
+ clock_clear_callback(clk->in);
+ object_unref(OBJECT(clk->in));
+ }
+ g_free(clk->name);
+ g_free(clk);
+ }
+
/* Only send event if the device had been completely realized */
if (dev->pending_deleted_event) {
g_assert(dev->canonical_path);
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index f7102121f4..fc0505e716 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,5 +1,5 @@
# core qdev-related obj files, also used by *-user:
-common-obj-y += qdev.o qdev-properties.o
+common-obj-y += qdev.o qdev-properties.o qdev-clock.o
common-obj-y += bus.o reset.o
common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 22:42 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
` (7 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, 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.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
qdev-monitor.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 61e0300991..8c39a3a65b 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -682,6 +682,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ObjectClass *class;
BusState *child;
NamedGPIOList *ngl;
+ NamedClockList *clk;
qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
dev->id ? dev->id : "");
@@ -696,6 +697,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
ngl->num_out);
}
}
+ QLIST_FOREACH(clk, &dev->clocks, node) {
+ if (clk->out) {
+ qdev_printf("clock-out%s \"%s\"\n",
+ clk->forward ? " (fw)" : "",
+ clk->name);
+ } else {
+ qdev_printf("clock-in%s \"%s\" freq=%" PRIu64 "Hz\n",
+ clk->forward ? " (fw)" : "",
+ clk->name, clock_get_frequency(clk->in));
+ }
+ }
class = object_get_class(OBJECT(dev));
do {
qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (2 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-03 8:23 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation Damien Hedde
` (6 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, 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>
---
include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
hw/core/qdev-clock.c | 26 ++++++++++++++++
2 files changed, 93 insertions(+)
diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index d76aa9f479..fba907dee2 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -59,4 +59,71 @@ void qdev_connect_clock(DeviceState *dev, const char *name,
DeviceState *driver, const char *driver_name,
Error **errp);
+/**
+ * ClockInitElem:
+ * @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 clock pointer in device'state
+ * structure (0 means unused)
+ */
+struct ClockPortInitElem {
+ const char *name;
+ bool output;
+ ClockCallback *callback;
+ size_t offset;
+};
+
+#define clock_offset_value(_type, _devstate, _field) \
+ (offsetof(_devstate, _field) + \
+ type_check(_type *, typeof_field(_devstate, _field)))
+
+#define QDEV_CLOCK(_output, _type, _devstate, _field, _callback) { \
+ .name = (stringify(_field)), \
+ .output = _output, \
+ .callback = _callback, \
+ .offset = clock_offset_value(_type, _devstate, _field), \
+}
+
+/**
+ * QDEV_CLOCK_(IN|OUT):
+ * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
+ * a pointer to that same type.
+ * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
+ * @_callback: (for input only) callback (or NULL) to be called with the device
+ * state as argument
+ *
+ * The name of the clock will be derived from @_field
+ */
+#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
+ QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
+
+#define QDEV_CLOCK_OUT(_devstate, _field) \
+ QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
+
+/**
+ * QDEV_CLOCK_IN_NOFIELD:
+ * @_name: name of the clock
+ * @_callback: callback (or NULL) to be called with the device state as argument
+ */
+#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
+ .name = _name, \
+ .output = false, \
+ .callback = _callback, \
+ .offset = 0, \
+}
+
+#define QDEV_CLOCK_END { .name = NULL }
+
+typedef struct ClockPortInitElem ClockPortInitArray[];
+
+/**
+ * qdev_init_clocks:
+ * @dev: the device to add clocks
+ * @clocks: a QDEV_CLOCK_END-terminated array which contains the
+ * clocks information.
+ */
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
+
#endif /* QDEV_CLOCK_H */
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index f0e4839aed..08afe3983d 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -138,3 +138,29 @@ void qdev_connect_clock(DeviceState *dev, const char *name,
clock_connect(ncl->in , drv_ncl->out);
}
+
+void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
+{
+ const struct ClockPortInitElem *elem;
+
+ assert(dev);
+ assert(clocks);
+
+ for (elem = &clocks[0]; elem->name != NULL; elem++) {
+ /* offset cannot be inside the DeviceState part */
+ assert(elem->offset == 0 || elem->offset > sizeof(DeviceState));
+ if (elem->output) {
+ ClockOut *clk;
+ clk = qdev_init_clock_out(dev, elem->name);
+ if (elem->offset) {
+ *(ClockOut **)(((void *) dev) + elem->offset) = clk;
+ }
+ } else {
+ ClockIn *clk;
+ clk = qdev_init_clock_in(dev, elem->name, elem->callback, dev);
+ if (elem->offset) {
+ *(ClockIn **)(((void *) dev) + elem->offset) = clk;
+ }
+ }
+ }
+}
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (3 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:48 ` Philippe Mathieu-Daudé
2018-10-03 8:18 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition Damien Hedde
` (5 subsequent siblings)
10 siblings, 2 replies; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, 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>
---
docs/devel/clock.txt | 163 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 163 insertions(+)
create mode 100644 docs/devel/clock.txt
diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
new file mode 100644
index 0000000000..6dd8abdee6
--- /dev/null
+++ b/docs/devel/clock.txt
@@ -0,0 +1,163 @@
+
+What are device's clocks
+========================
+
+Clocks are ports representing input and output clocks of a device. They are QOM
+objects developed for the purpose of modeling the distribution of clocks in
+QEMU.
+
+This allows us to model the clock distribution of a platform and detect
+configuration errors in the clock tree such as badly configured PLL, clock
+source selection or disabled clock.
+
+The objects are CLOCK_IN for the input and CLOCK_OUT for the output.
+
+The clock value: ClockState
+===========================
+
+The ClockState is the structure carried by the CLOCK_OUT and CLOCK_IN objects.
+It contains one integer field representing the frequency of the clock in Hertz.
+
+It only simulates the clock by transmitting the frequency value and
+doesn't model the signal itself such as pin toggle or duty cycle.
+The special value 0 as a frequency is legal and represent the clock being
+inactive or gated.
+
+Adding clocks to a device
+=========================
+
+Adding clocks to a device must be done during the init phase of the Device
+object.
+
+To add an input clock to a device, the function qdev_init_clock_in must be used.
+It takes the name, a callback, and an opaque parameter for the clock.
+Output is more simple, only the name is required. Typically:
+qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
+qdev_init_clock_out(DEVICE(dev), "clk-out");
+
+Both functions return the created CLOCK_IN/OUT pointer, which should be saved
+in the device's state structure.
+
+Theses 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
+parameters to initialize the clocks: it has the same behaviour as calling the
+qdev_init_clock/out for each clock in the array.
+
+Unconnected input clocks
+========================
+
+Unconnected input clocks have a default frequency value of 0. It means the
+clock will be considered as disabled. If this is not the wanted behaviour,
+clock_init_frequency should be called on the ClockIn object during device init.
+For example:
+clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
+clock_init_frequency(clk, 100 * 1000 * 1000); // init value is 100Mhz
+
+Forwarding clocks
+=================
+
+Sometimes, one needs to forward, or inherit, a clock from another device.
+Typically, when doing device composition, a device might expose a sub-device's
+clock without interfering with it.
+The function qdev_pass_clock can be used to achieve this behaviour. Note, that
+it is possible to expose the clock under a different name. This works for both
+inputs or outputs.
+
+For example, if device B is a child of device A, device_a_instance_init may
+do something like this:
+void device_a_instance_init(Object *obj)
+{
+ AState *A = DEVICE_A(obj);
+ BState *B;
+ [...] /* create B object as child of A */
+ qdev_pass_clock(A, "b_clk", B, "clk");
+ /*
+ * Now A has a clock "b_clk" which forwards to
+ * the "clk" of its child B.
+ */
+}
+
+This function does not returns any clock object. It is not possible to add
+a callback on a forwarded input clock.
+
+Connecting two clocks together
+==============================
+
+Let's say we have 2 devices A and B. A has an output clock named "clkout" and B
+has an input clock named "clkin".
+
+The clocks are connected together using the function qdev_connect_clock:
+qdev_connect_clock(B, "clkin", A, "clkout", &error_abort);
+The device which has the input must be the first argument.
+
+It is possible to connect several input clocks to the same output. Every
+input callback will be called when the output changes.
+
+It is not possible to disconnect a clock or to change the clock connection
+after it is done.
+
+Changing a clock output
+=======================
+
+A device can change its outputs using the clock_set function. It will trigger
+updates on any connected inputs.
+
+For example, let's say that we have an output clock "clkout" and we have a
+pointer to it in the device state because we did the following in init phase:
+dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
+
+Then at any time, it is possible to change the clock value by doing:
+clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Mhz */
+
+Callback on input clock change
+==============================
+
+Here is an example of an input callback:
+void clock_callback(void *opaque) {
+ MyDeviceState *s = (MyDeviceState *) opaque;
+ /*
+ * opaque may not be the device state pointer, but most probably it is.
+ * (It depends on what is given to the qdev_init_clock_in function)
+ */
+
+ /* do something with the new frequency */
+ fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
+ clock_get_frequency(dev->my_clk_input));
+}
+
+The state argument needs only to be copied if the device needs to use the value
+later: the state pointer argument of the pointer will not be valid anymore
+after the end of the function.
+
+Migration
+=========
+
+Only the CLOCK_IN object has a state. CLOCK_OUT frequency should not be set
+in migration post_load.
+
+In case the frequency of in input clock is needed for a device's migration,
+this state must be migrated. The VMSTATE_CLOCKIN macro defines an entry to
+be added in a vmstate description.
+
+For example, if a device has a clock input and the device state looks like:
+MyDeviceState {
+ DeviceState parent_obj;
+ ClockIn *clk;
+};
+
+Then, to add the clock frequency to the device's migrated state, the vmstate
+description is:
+VMStateDescription my_device_vmstate = {
+ .name = "my_device",
+ .fields = (VMStateField[]) {
+ VMSTATE_CLOCKIN(clk, MyDeviceState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+When adding a input clock support to an existing device, you must care about
+migration compatibility. To this end, you can use the clock_init_frequency in
+a pre_load function to setup a default value in case the source vm does not
+migrate the frequency.
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (4 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-04 17:24 ` Alistair Francis
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
` (4 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Replace the zynq_slcr registers enum and macros using the
hw/registerfields.h macros.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/zynq_slcr.c | 468 ++++++++++++++++++++++----------------------
1 file changed, 234 insertions(+), 234 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index d6bdd027ef..baa13d1316 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -20,6 +20,7 @@
#include "hw/sysbus.h"
#include "sysemu/sysemu.h"
#include "qemu/log.h"
+#include "hw/registerfields.h"
#ifndef ZYNQ_SLCR_ERR_DEBUG
#define ZYNQ_SLCR_ERR_DEBUG 0
@@ -35,138 +36,135 @@
#define XILINX_LOCK_KEY 0x767b
#define XILINX_UNLOCK_KEY 0xdf0d
-#define R_PSS_RST_CTRL_SOFT_RST 0x1
+REG32(SCL, 0x000)
+REG32(LOCK, 0x004)
+REG32(UNLOCK, 0x008)
+REG32(LOCKSTA, 0x00c)
-enum {
- SCL = 0x000 / 4,
- LOCK,
- UNLOCK,
- LOCKSTA,
+REG32(ARM_PLL_CTRL, 0x100)
+REG32(DDR_PLL_CTRL, 0x104)
+REG32(IO_PLL_CTRL, 0x108)
+REG32(PLL_STATUS, 0x10c)
+REG32(ARM_PLL_CFG, 0x110)
+REG32(DDR_PLL_CFG, 0x114)
+REG32(IO_PLL_CFG, 0x118)
- ARM_PLL_CTRL = 0x100 / 4,
- DDR_PLL_CTRL,
- IO_PLL_CTRL,
- PLL_STATUS,
- ARM_PLL_CFG,
- DDR_PLL_CFG,
- IO_PLL_CFG,
-
- ARM_CLK_CTRL = 0x120 / 4,
- DDR_CLK_CTRL,
- DCI_CLK_CTRL,
- APER_CLK_CTRL,
- USB0_CLK_CTRL,
- USB1_CLK_CTRL,
- GEM0_RCLK_CTRL,
- GEM1_RCLK_CTRL,
- GEM0_CLK_CTRL,
- GEM1_CLK_CTRL,
- SMC_CLK_CTRL,
- LQSPI_CLK_CTRL,
- SDIO_CLK_CTRL,
- UART_CLK_CTRL,
- SPI_CLK_CTRL,
- CAN_CLK_CTRL,
- CAN_MIOCLK_CTRL,
- DBG_CLK_CTRL,
- PCAP_CLK_CTRL,
- TOPSW_CLK_CTRL,
+REG32(ARM_CLK_CTRL, 0x120)
+REG32(DDR_CLK_CTRL, 0x124)
+REG32(DCI_CLK_CTRL, 0x128)
+REG32(APER_CLK_CTRL, 0x12c)
+REG32(USB0_CLK_CTRL, 0x130)
+REG32(USB1_CLK_CTRL, 0x134)
+REG32(GEM0_RCLK_CTRL, 0x138)
+REG32(GEM1_RCLK_CTRL, 0x13c)
+REG32(GEM0_CLK_CTRL, 0x140)
+REG32(GEM1_CLK_CTRL, 0x144)
+REG32(SMC_CLK_CTRL, 0x148)
+REG32(LQSPI_CLK_CTRL, 0x14c)
+REG32(SDIO_CLK_CTRL, 0x150)
+REG32(UART_CLK_CTRL, 0x154)
+REG32(SPI_CLK_CTRL, 0x158)
+REG32(CAN_CLK_CTRL, 0x15c)
+REG32(CAN_MIOCLK_CTRL, 0x160)
+REG32(DBG_CLK_CTRL, 0x164)
+REG32(PCAP_CLK_CTRL, 0x168)
+REG32(TOPSW_CLK_CTRL, 0x16c)
#define FPGA_CTRL_REGS(n, start) \
- FPGA ## n ## _CLK_CTRL = (start) / 4, \
- FPGA ## n ## _THR_CTRL, \
- FPGA ## n ## _THR_CNT, \
- FPGA ## n ## _THR_STA,
- FPGA_CTRL_REGS(0, 0x170)
- FPGA_CTRL_REGS(1, 0x180)
- FPGA_CTRL_REGS(2, 0x190)
- FPGA_CTRL_REGS(3, 0x1a0)
-
- BANDGAP_TRIP = 0x1b8 / 4,
- PLL_PREDIVISOR = 0x1c0 / 4,
- CLK_621_TRUE,
-
- PSS_RST_CTRL = 0x200 / 4,
- DDR_RST_CTRL,
- TOPSW_RESET_CTRL,
- DMAC_RST_CTRL,
- USB_RST_CTRL,
- GEM_RST_CTRL,
- SDIO_RST_CTRL,
- SPI_RST_CTRL,
- CAN_RST_CTRL,
- I2C_RST_CTRL,
- UART_RST_CTRL,
- GPIO_RST_CTRL,
- LQSPI_RST_CTRL,
- SMC_RST_CTRL,
- OCM_RST_CTRL,
- FPGA_RST_CTRL = 0x240 / 4,
- A9_CPU_RST_CTRL,
-
- RS_AWDT_CTRL = 0x24c / 4,
- RST_REASON,
-
- REBOOT_STATUS = 0x258 / 4,
- BOOT_MODE,
-
- APU_CTRL = 0x300 / 4,
- WDT_CLK_SEL,
-
- TZ_DMA_NS = 0x440 / 4,
- TZ_DMA_IRQ_NS,
- TZ_DMA_PERIPH_NS,
-
- PSS_IDCODE = 0x530 / 4,
-
- DDR_URGENT = 0x600 / 4,
- DDR_CAL_START = 0x60c / 4,
- DDR_REF_START = 0x614 / 4,
- DDR_CMD_STA,
- DDR_URGENT_SEL,
- DDR_DFI_STATUS,
-
- MIO = 0x700 / 4,
+ REG32(FPGA ## n ## _CLK_CTRL, (start)) \
+ REG32(FPGA ## n ## _THR_CTRL, (start) + 0x4)\
+ REG32(FPGA ## n ## _THR_CNT, (start) + 0x8)\
+ REG32(FPGA ## n ## _THR_STA, (start) + 0xc)
+FPGA_CTRL_REGS(0, 0x170)
+FPGA_CTRL_REGS(1, 0x180)
+FPGA_CTRL_REGS(2, 0x190)
+FPGA_CTRL_REGS(3, 0x1a0)
+
+REG32(BANDGAP_TRIP, 0x1b8)
+REG32(PLL_PREDIVISOR, 0x1c0)
+REG32(CLK_621_TRUE, 0x1c4)
+
+REG32(PSS_RST_CTRL, 0x200)
+ FIELD(PSS_RST_CTRL, SOFT_RST, 0, 1)
+REG32(DDR_RST_CTRL, 0x204)
+REG32(TOPSW_RESET_CTRL, 0x208)
+REG32(DMAC_RST_CTRL, 0x20c)
+REG32(USB_RST_CTRL, 0x210)
+REG32(GEM_RST_CTRL, 0x214)
+REG32(SDIO_RST_CTRL, 0x218)
+REG32(SPI_RST_CTRL, 0x21c)
+REG32(CAN_RST_CTRL, 0x220)
+REG32(I2C_RST_CTRL, 0x224)
+REG32(UART_RST_CTRL, 0x228)
+REG32(GPIO_RST_CTRL, 0x22c)
+REG32(LQSPI_RST_CTRL, 0x230)
+REG32(SMC_RST_CTRL, 0x234)
+REG32(OCM_RST_CTRL, 0x238)
+REG32(FPGA_RST_CTRL, 0x240)
+REG32(A9_CPU_RST_CTRL, 0x244)
+
+REG32(RS_AWDT_CTRL, 0x24c)
+REG32(RST_REASON, 0x250)
+
+REG32(REBOOT_STATUS, 0x258)
+REG32(BOOT_MODE, 0x25c)
+
+REG32(APU_CTRL, 0x300)
+REG32(WDT_CLK_SEL, 0x304)
+
+REG32(TZ_DMA_NS, 0x440)
+REG32(TZ_DMA_IRQ_NS, 0x444)
+REG32(TZ_DMA_PERIPH_NS, 0x448)
+
+REG32(PSS_IDCODE, 0x530)
+
+REG32(DDR_URGENT, 0x600)
+REG32(DDR_CAL_START, 0x60c)
+REG32(DDR_REF_START, 0x614)
+REG32(DDR_CMD_STA, 0x618)
+REG32(DDR_URGENT_SEL, 0x61c)
+REG32(DDR_DFI_STATUS, 0x620)
+
+REG32(MIO, 0x700)
#define MIO_LENGTH 54
- MIO_LOOPBACK = 0x804 / 4,
- MIO_MST_TRI0,
- MIO_MST_TRI1,
+REG32(MIO_LOOPBACK, 0x804)
+REG32(MIO_MST_TRI0, 0x808)
+REG32(MIO_MST_TRI1, 0x80c)
- SD0_WP_CD_SEL = 0x830 / 4,
- SD1_WP_CD_SEL,
+REG32(SD0_WP_CD_SEL, 0x830)
+REG32(SD1_WP_CD_SEL, 0x834)
- LVL_SHFTR_EN = 0x900 / 4,
- OCM_CFG = 0x910 / 4,
+REG32(LVL_SHFTR_EN, 0x900)
+REG32(OCM_CFG, 0x910)
- CPU_RAM = 0xa00 / 4,
+REG32(CPU_RAM, 0xa00)
- IOU = 0xa30 / 4,
+REG32(IOU, 0xa30)
- DMAC_RAM = 0xa50 / 4,
+REG32(DMAC_RAM, 0xa50)
- AFI0 = 0xa60 / 4,
- AFI1 = AFI0 + 3,
- AFI2 = AFI1 + 3,
- AFI3 = AFI2 + 3,
+REG32(AFI0, 0xa60)
+REG32(AFI1, 0xa6c)
+REG32(AFI2, 0xa78)
+REG32(AFI3, 0xa84)
#define AFI_LENGTH 3
- OCM = 0xa90 / 4,
+REG32(OCM, 0xa90)
- DEVCI_RAM = 0xaa0 / 4,
+REG32(DEVCI_RAM, 0xaa0)
- CSG_RAM = 0xab0 / 4,
+REG32(CSG_RAM, 0xab0)
- GPIOB_CTRL = 0xb00 / 4,
- GPIOB_CFG_CMOS18,
- GPIOB_CFG_CMOS25,
- GPIOB_CFG_CMOS33,
- GPIOB_CFG_HSTL = 0xb14 / 4,
- GPIOB_DRVR_BIAS_CTRL,
+REG32(GPIOB_CTRL, 0xb00)
+REG32(GPIOB_CFG_CMOS18, 0xb04)
+REG32(GPIOB_CFG_CMOS25, 0xb08)
+REG32(GPIOB_CFG_CMOS33, 0xb0c)
+REG32(GPIOB_CFG_HSTL, 0xb14)
+REG32(GPIOB_DRVR_BIAS_CTRL, 0xb18)
- DDRIOB = 0xb40 / 4,
+REG32(DDRIOB, 0xb40)
#define DDRIOB_LENGTH 14
-};
#define ZYNQ_SLCR_MMIO_SIZE 0x1000
#define ZYNQ_SLCR_NUM_REGS (ZYNQ_SLCR_MMIO_SIZE / 4)
@@ -189,150 +187,152 @@ static void zynq_slcr_reset(DeviceState *d)
DB_PRINT("RESET\n");
- s->regs[LOCKSTA] = 1;
+ s->regs[R_LOCKSTA] = 1;
/* 0x100 - 0x11C */
- s->regs[ARM_PLL_CTRL] = 0x0001A008;
- s->regs[DDR_PLL_CTRL] = 0x0001A008;
- s->regs[IO_PLL_CTRL] = 0x0001A008;
- s->regs[PLL_STATUS] = 0x0000003F;
- s->regs[ARM_PLL_CFG] = 0x00014000;
- s->regs[DDR_PLL_CFG] = 0x00014000;
- s->regs[IO_PLL_CFG] = 0x00014000;
+ s->regs[R_ARM_PLL_CTRL] = 0x0001A008;
+ s->regs[R_DDR_PLL_CTRL] = 0x0001A008;
+ s->regs[R_IO_PLL_CTRL] = 0x0001A008;
+ s->regs[R_PLL_STATUS] = 0x0000003F;
+ s->regs[R_ARM_PLL_CFG] = 0x00014000;
+ s->regs[R_DDR_PLL_CFG] = 0x00014000;
+ s->regs[R_IO_PLL_CFG] = 0x00014000;
/* 0x120 - 0x16C */
- s->regs[ARM_CLK_CTRL] = 0x1F000400;
- s->regs[DDR_CLK_CTRL] = 0x18400003;
- s->regs[DCI_CLK_CTRL] = 0x01E03201;
- s->regs[APER_CLK_CTRL] = 0x01FFCCCD;
- s->regs[USB0_CLK_CTRL] = s->regs[USB1_CLK_CTRL] = 0x00101941;
- s->regs[GEM0_RCLK_CTRL] = s->regs[GEM1_RCLK_CTRL] = 0x00000001;
- s->regs[GEM0_CLK_CTRL] = s->regs[GEM1_CLK_CTRL] = 0x00003C01;
- s->regs[SMC_CLK_CTRL] = 0x00003C01;
- s->regs[LQSPI_CLK_CTRL] = 0x00002821;
- s->regs[SDIO_CLK_CTRL] = 0x00001E03;
- s->regs[UART_CLK_CTRL] = 0x00003F03;
- s->regs[SPI_CLK_CTRL] = 0x00003F03;
- s->regs[CAN_CLK_CTRL] = 0x00501903;
- s->regs[DBG_CLK_CTRL] = 0x00000F03;
- s->regs[PCAP_CLK_CTRL] = 0x00000F01;
+ s->regs[R_ARM_CLK_CTRL] = 0x1F000400;
+ s->regs[R_DDR_CLK_CTRL] = 0x18400003;
+ s->regs[R_DCI_CLK_CTRL] = 0x01E03201;
+ s->regs[R_APER_CLK_CTRL] = 0x01FFCCCD;
+ s->regs[R_USB0_CLK_CTRL] = s->regs[R_USB1_CLK_CTRL] = 0x00101941;
+ s->regs[R_GEM0_RCLK_CTRL] = s->regs[R_GEM1_RCLK_CTRL] = 0x00000001;
+ s->regs[R_GEM0_CLK_CTRL] = s->regs[R_GEM1_CLK_CTRL] = 0x00003C01;
+ s->regs[R_SMC_CLK_CTRL] = 0x00003C01;
+ s->regs[R_LQSPI_CLK_CTRL] = 0x00002821;
+ s->regs[R_SDIO_CLK_CTRL] = 0x00001E03;
+ s->regs[R_UART_CLK_CTRL] = 0x00003F03;
+ s->regs[R_SPI_CLK_CTRL] = 0x00003F03;
+ s->regs[R_CAN_CLK_CTRL] = 0x00501903;
+ s->regs[R_DBG_CLK_CTRL] = 0x00000F03;
+ s->regs[R_PCAP_CLK_CTRL] = 0x00000F01;
/* 0x170 - 0x1AC */
- s->regs[FPGA0_CLK_CTRL] = s->regs[FPGA1_CLK_CTRL] = s->regs[FPGA2_CLK_CTRL]
- = s->regs[FPGA3_CLK_CTRL] = 0x00101800;
- s->regs[FPGA0_THR_STA] = s->regs[FPGA1_THR_STA] = s->regs[FPGA2_THR_STA]
- = s->regs[FPGA3_THR_STA] = 0x00010000;
+ s->regs[R_FPGA0_CLK_CTRL] = s->regs[R_FPGA1_CLK_CTRL]
+ = s->regs[R_FPGA2_CLK_CTRL]
+ = s->regs[R_FPGA3_CLK_CTRL] = 0x00101800;
+ s->regs[R_FPGA0_THR_STA] = s->regs[R_FPGA1_THR_STA]
+ = s->regs[R_FPGA2_THR_STA]
+ = s->regs[R_FPGA3_THR_STA] = 0x00010000;
/* 0x1B0 - 0x1D8 */
- s->regs[BANDGAP_TRIP] = 0x0000001F;
- s->regs[PLL_PREDIVISOR] = 0x00000001;
- s->regs[CLK_621_TRUE] = 0x00000001;
+ s->regs[R_BANDGAP_TRIP] = 0x0000001F;
+ s->regs[R_PLL_PREDIVISOR] = 0x00000001;
+ s->regs[R_CLK_621_TRUE] = 0x00000001;
/* 0x200 - 0x25C */
- s->regs[FPGA_RST_CTRL] = 0x01F33F0F;
- s->regs[RST_REASON] = 0x00000040;
+ s->regs[R_FPGA_RST_CTRL] = 0x01F33F0F;
+ s->regs[R_RST_REASON] = 0x00000040;
- s->regs[BOOT_MODE] = 0x00000001;
+ s->regs[R_BOOT_MODE] = 0x00000001;
/* 0x700 - 0x7D4 */
for (i = 0; i < 54; i++) {
- s->regs[MIO + i] = 0x00001601;
+ s->regs[R_MIO + i] = 0x00001601;
}
for (i = 2; i <= 8; i++) {
- s->regs[MIO + i] = 0x00000601;
+ s->regs[R_MIO + i] = 0x00000601;
}
- s->regs[MIO_MST_TRI0] = s->regs[MIO_MST_TRI1] = 0xFFFFFFFF;
+ s->regs[R_MIO_MST_TRI0] = s->regs[R_MIO_MST_TRI1] = 0xFFFFFFFF;
- s->regs[CPU_RAM + 0] = s->regs[CPU_RAM + 1] = s->regs[CPU_RAM + 3]
- = s->regs[CPU_RAM + 4] = s->regs[CPU_RAM + 7]
- = 0x00010101;
- s->regs[CPU_RAM + 2] = s->regs[CPU_RAM + 5] = 0x01010101;
- s->regs[CPU_RAM + 6] = 0x00000001;
+ s->regs[R_CPU_RAM + 0] = s->regs[R_CPU_RAM + 1] = s->regs[R_CPU_RAM + 3]
+ = s->regs[R_CPU_RAM + 4] = s->regs[R_CPU_RAM + 7]
+ = 0x00010101;
+ s->regs[R_CPU_RAM + 2] = s->regs[R_CPU_RAM + 5] = 0x01010101;
+ s->regs[R_CPU_RAM + 6] = 0x00000001;
- s->regs[IOU + 0] = s->regs[IOU + 1] = s->regs[IOU + 2] = s->regs[IOU + 3]
- = 0x09090909;
- s->regs[IOU + 4] = s->regs[IOU + 5] = 0x00090909;
- s->regs[IOU + 6] = 0x00000909;
+ s->regs[R_IOU + 0] = s->regs[R_IOU + 1] = s->regs[R_IOU + 2]
+ = s->regs[R_IOU + 3] = 0x09090909;
+ s->regs[R_IOU + 4] = s->regs[R_IOU + 5] = 0x00090909;
+ s->regs[R_IOU + 6] = 0x00000909;
- s->regs[DMAC_RAM] = 0x00000009;
+ s->regs[R_DMAC_RAM] = 0x00000009;
- s->regs[AFI0 + 0] = s->regs[AFI0 + 1] = 0x09090909;
- s->regs[AFI1 + 0] = s->regs[AFI1 + 1] = 0x09090909;
- s->regs[AFI2 + 0] = s->regs[AFI2 + 1] = 0x09090909;
- s->regs[AFI3 + 0] = s->regs[AFI3 + 1] = 0x09090909;
- s->regs[AFI0 + 2] = s->regs[AFI1 + 2] = s->regs[AFI2 + 2]
- = s->regs[AFI3 + 2] = 0x00000909;
+ s->regs[R_AFI0 + 0] = s->regs[R_AFI0 + 1] = 0x09090909;
+ s->regs[R_AFI1 + 0] = s->regs[R_AFI1 + 1] = 0x09090909;
+ s->regs[R_AFI2 + 0] = s->regs[R_AFI2 + 1] = 0x09090909;
+ s->regs[R_AFI3 + 0] = s->regs[R_AFI3 + 1] = 0x09090909;
+ s->regs[R_AFI0 + 2] = s->regs[R_AFI1 + 2] = s->regs[R_AFI2 + 2]
+ = s->regs[R_AFI3 + 2] = 0x00000909;
- s->regs[OCM + 0] = 0x01010101;
- s->regs[OCM + 1] = s->regs[OCM + 2] = 0x09090909;
+ s->regs[R_OCM + 0] = 0x01010101;
+ s->regs[R_OCM + 1] = s->regs[R_OCM + 2] = 0x09090909;
- s->regs[DEVCI_RAM] = 0x00000909;
- s->regs[CSG_RAM] = 0x00000001;
+ s->regs[R_DEVCI_RAM] = 0x00000909;
+ s->regs[R_CSG_RAM] = 0x00000001;
- s->regs[DDRIOB + 0] = s->regs[DDRIOB + 1] = s->regs[DDRIOB + 2]
- = s->regs[DDRIOB + 3] = 0x00000e00;
- s->regs[DDRIOB + 4] = s->regs[DDRIOB + 5] = s->regs[DDRIOB + 6]
- = 0x00000e00;
- s->regs[DDRIOB + 12] = 0x00000021;
+ s->regs[R_DDRIOB + 0] = s->regs[R_DDRIOB + 1] = s->regs[R_DDRIOB + 2]
+ = s->regs[R_DDRIOB + 3] = 0x00000e00;
+ s->regs[R_DDRIOB + 4] = s->regs[R_DDRIOB + 5] = s->regs[R_DDRIOB + 6]
+ = 0x00000e00;
+ s->regs[R_DDRIOB + 12] = 0x00000021;
}
static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
{
switch (offset) {
- case LOCK:
- case UNLOCK:
- case DDR_CAL_START:
- case DDR_REF_START:
+ case R_LOCK:
+ case R_UNLOCK:
+ case R_DDR_CAL_START:
+ case R_DDR_REF_START:
return !rnw; /* Write only */
- case LOCKSTA:
- case FPGA0_THR_STA:
- case FPGA1_THR_STA:
- case FPGA2_THR_STA:
- case FPGA3_THR_STA:
- case BOOT_MODE:
- case PSS_IDCODE:
- case DDR_CMD_STA:
- case DDR_DFI_STATUS:
- case PLL_STATUS:
+ case R_LOCKSTA:
+ case R_FPGA0_THR_STA:
+ case R_FPGA1_THR_STA:
+ case R_FPGA2_THR_STA:
+ case R_FPGA3_THR_STA:
+ case R_BOOT_MODE:
+ case R_PSS_IDCODE:
+ case R_DDR_CMD_STA:
+ case R_DDR_DFI_STATUS:
+ case R_PLL_STATUS:
return rnw;/* read only */
- case SCL:
- case ARM_PLL_CTRL ... IO_PLL_CTRL:
- case ARM_PLL_CFG ... IO_PLL_CFG:
- case ARM_CLK_CTRL ... TOPSW_CLK_CTRL:
- case FPGA0_CLK_CTRL ... FPGA0_THR_CNT:
- case FPGA1_CLK_CTRL ... FPGA1_THR_CNT:
- case FPGA2_CLK_CTRL ... FPGA2_THR_CNT:
- case FPGA3_CLK_CTRL ... FPGA3_THR_CNT:
- case BANDGAP_TRIP:
- case PLL_PREDIVISOR:
- case CLK_621_TRUE:
- case PSS_RST_CTRL ... A9_CPU_RST_CTRL:
- case RS_AWDT_CTRL:
- case RST_REASON:
- case REBOOT_STATUS:
- case APU_CTRL:
- case WDT_CLK_SEL:
- case TZ_DMA_NS ... TZ_DMA_PERIPH_NS:
- case DDR_URGENT:
- case DDR_URGENT_SEL:
- case MIO ... MIO + MIO_LENGTH - 1:
- case MIO_LOOPBACK ... MIO_MST_TRI1:
- case SD0_WP_CD_SEL:
- case SD1_WP_CD_SEL:
- case LVL_SHFTR_EN:
- case OCM_CFG:
- case CPU_RAM:
- case IOU:
- case DMAC_RAM:
- case AFI0 ... AFI3 + AFI_LENGTH - 1:
- case OCM:
- case DEVCI_RAM:
- case CSG_RAM:
- case GPIOB_CTRL ... GPIOB_CFG_CMOS33:
- case GPIOB_CFG_HSTL:
- case GPIOB_DRVR_BIAS_CTRL:
- case DDRIOB ... DDRIOB + DDRIOB_LENGTH - 1:
+ case R_SCL:
+ case R_ARM_PLL_CTRL ... R_IO_PLL_CTRL:
+ case R_ARM_PLL_CFG ... R_IO_PLL_CFG:
+ case R_ARM_CLK_CTRL ... R_TOPSW_CLK_CTRL:
+ case R_FPGA0_CLK_CTRL ... R_FPGA0_THR_CNT:
+ case R_FPGA1_CLK_CTRL ... R_FPGA1_THR_CNT:
+ case R_FPGA2_CLK_CTRL ... R_FPGA2_THR_CNT:
+ case R_FPGA3_CLK_CTRL ... R_FPGA3_THR_CNT:
+ case R_BANDGAP_TRIP:
+ case R_PLL_PREDIVISOR:
+ case R_CLK_621_TRUE:
+ case R_PSS_RST_CTRL ... R_A9_CPU_RST_CTRL:
+ case R_RS_AWDT_CTRL:
+ case R_RST_REASON:
+ case R_REBOOT_STATUS:
+ case R_APU_CTRL:
+ case R_WDT_CLK_SEL:
+ case R_TZ_DMA_NS ... R_TZ_DMA_PERIPH_NS:
+ case R_DDR_URGENT:
+ case R_DDR_URGENT_SEL:
+ case R_MIO ... R_MIO + MIO_LENGTH - 1:
+ case R_MIO_LOOPBACK ... R_MIO_MST_TRI1:
+ case R_SD0_WP_CD_SEL:
+ case R_SD1_WP_CD_SEL:
+ case R_LVL_SHFTR_EN:
+ case R_OCM_CFG:
+ case R_CPU_RAM:
+ case R_IOU:
+ case R_DMAC_RAM:
+ case R_AFI0 ... R_AFI3 + AFI_LENGTH - 1:
+ case R_OCM:
+ case R_DEVCI_RAM:
+ case R_CSG_RAM:
+ case R_GPIOB_CTRL ... R_GPIOB_CFG_CMOS33:
+ case R_GPIOB_CFG_HSTL:
+ case R_GPIOB_DRVR_BIAS_CTRL:
+ case R_DDRIOB ... R_DDRIOB + DDRIOB_LENGTH - 1:
return true;
default:
return false;
@@ -370,24 +370,24 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
}
switch (offset) {
- case SCL:
- s->regs[SCL] = val & 0x1;
+ case R_SCL:
+ s->regs[R_SCL] = val & 0x1;
return;
- case LOCK:
+ case R_LOCK:
if ((val & 0xFFFF) == XILINX_LOCK_KEY) {
DB_PRINT("XILINX LOCK 0xF8000000 + 0x%x <= 0x%x\n", (int)offset,
(unsigned)val & 0xFFFF);
- s->regs[LOCKSTA] = 1;
+ s->regs[R_LOCKSTA] = 1;
} else {
DB_PRINT("WRONG XILINX LOCK KEY 0xF8000000 + 0x%x <= 0x%x\n",
(int)offset, (unsigned)val & 0xFFFF);
}
return;
- case UNLOCK:
+ case R_UNLOCK:
if ((val & 0xFFFF) == XILINX_UNLOCK_KEY) {
DB_PRINT("XILINX UNLOCK 0xF8000000 + 0x%x <= 0x%x\n", (int)offset,
(unsigned)val & 0xFFFF);
- s->regs[LOCKSTA] = 0;
+ s->regs[R_LOCKSTA] = 0;
} else {
DB_PRINT("WRONG XILINX UNLOCK KEY 0xF8000000 + 0x%x <= 0x%x\n",
(int)offset, (unsigned)val & 0xFFFF);
@@ -395,7 +395,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
return;
}
- if (s->regs[LOCKSTA]) {
+ if (s->regs[R_LOCKSTA]) {
qemu_log_mask(LOG_GUEST_ERROR,
"SCLR registers are locked. Unlock them first\n");
return;
@@ -403,8 +403,8 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
s->regs[offset] = val;
switch (offset) {
- case PSS_RST_CTRL:
- if (val & R_PSS_RST_CTRL_SOFT_RST) {
+ case R_PSS_RST_CTRL:
+ if (FIELD_EX32(val, PSS_RST_CTRL, SOFT_RST)) {
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
}
break;
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (5 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:10 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support Damien Hedde
` (3 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Add 2 clock outputs for each uart (uart0 & 1):
+ the reference clock
+ the bus interface clock
The clock frequencies are computed using the internal pll & uart configuration
registers.
All clocks depend on the main input clock (ps_clk), which is hard-coded
to 33.33MHz (zcu102 evaluation board frequency and frequency specified
in Xilinx's device tree file)
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
hw/misc/zynq_slcr.c | 125 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 125 insertions(+)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index baa13d1316..0b599ebd97 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -33,6 +33,8 @@
} \
} while (0)
+#define INPUT_PS_REF_CLK_FREQUENCY (33333333)
+
#define XILINX_LOCK_KEY 0x767b
#define XILINX_UNLOCK_KEY 0xdf0d
@@ -44,15 +46,27 @@ 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)
REG32(IO_PLL_CFG, 0x118)
REG32(ARM_CLK_CTRL, 0x120)
+ FIELD(ARM_CLK_CTRL, SRCSEL, 4, 2)
+ FIELD(ARM_CLK_CTRL, DIVISOR, 8, 6)
+ FIELD(ARM_CLK_CTRL, CPU_1XCLKACT, 27, 1)
+ FIELD(ARM_CLK_CTRL, CPU_PERI_CLKACT, 28, 1)
REG32(DDR_CLK_CTRL, 0x124)
REG32(DCI_CLK_CTRL, 0x128)
REG32(APER_CLK_CTRL, 0x12c)
+ FIELD(APER_CLK_CTRL, UART0_CPU1XCLKACT, 20, 1)
+ FIELD(APER_CLK_CTRL, UART1_CPU1XCLKACT, 21, 1)
REG32(USB0_CLK_CTRL, 0x130)
REG32(USB1_CLK_CTRL, 0x134)
REG32(GEM0_RCLK_CTRL, 0x138)
@@ -63,12 +77,19 @@ 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)
REG32(DBG_CLK_CTRL, 0x164)
REG32(PCAP_CLK_CTRL, 0x168)
REG32(TOPSW_CLK_CTRL, 0x16c)
+/* common fields to lots of *_CLK_CTRL registers */
+ FIELD(xxx_CLK_CTRL, SRCSEL, 4, 2)
+ FIELD(xxx_CLK_CTRL, DIVISOR, 8, 6)
#define FPGA_CTRL_REGS(n, start) \
REG32(FPGA ## n ## _CLK_CTRL, (start)) \
@@ -178,8 +199,92 @@ typedef struct ZynqSLCRState {
MemoryRegion iomem;
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+ ClockOut *uart0_amba_clk;
+ ClockOut *uart1_amba_clk;
+ ClockOut *uart0_ref_clk;
+ ClockOut *uart1_ref_clk;
} ZynqSLCRState;
+/*
+ * return the output frequency of ARM/DDR/IO pll
+ * using input frequency and PLL_CTRL register
+ */
+static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
+{
+ uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
+ R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
+
+ /* first, check if pll is bypassed */
+ if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
+ return input;
+ }
+
+ /* is pll disabled ? */
+ if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
+ R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
+ return 0;
+ }
+
+ return input * mult;
+}
+
+/*
+ * return the output frequency of a clock given:
+ * + the pll's frequencies in an array corresponding to mux's indexes
+ * + the register xxx_CLK_CTRL value
+ * + enable bit index in ctrl register
+ *
+ * This function make the assumption that ctrl_reg value is organized as follow:
+ * + bits[13:8] clock divisor
+ * + bits[5:4] clock mux selector (index in array)
+ * + bits[index] clock enable
+ */
+static uint64_t zynq_slcr_compute_clock(const uint64_t plls[],
+ uint32_t ctrl_reg,
+ unsigned index)
+{
+ uint32_t divisor = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, DIVISOR);
+ uint32_t srcsel = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, SRCSEL);
+
+ if ((ctrl_reg & (1u << index)) == 0) {
+ return 0;
+ }
+
+ return plls[srcsel] / (divisor ? divisor : 1u);
+}
+
+#define ZYNQ_CLOCK(_state, _plls, _reg, _enable_field) \
+ zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
+ R_ ## _reg ## _ ## _enable_field ## _SHIFT)
+#define ZYNQ_GATE(_state, _clk, _reg, _field) \
+ (ARRAY_FIELD_EX32((_state)->regs, _reg, _field) ? (_clk) : 0)
+
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+{
+ uint64_t ps_clk = INPUT_PS_REF_CLK_FREQUENCY;
+ 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 cpu_mux[4] = {arm_pll, arm_pll, ddr_pll, io_pll};
+ uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
+ uint64_t cpu1x_clk;
+
+ /* compute uartX amba clocks */
+ cpu1x_clk = ZYNQ_CLOCK(s, cpu_mux, ARM_CLK_CTRL, CPU_PERI_CLKACT);
+ cpu1x_clk = ZYNQ_GATE(s, cpu1x_clk, ARM_CLK_CTRL, CPU_1XCLKACT);
+ clock_set_frequency(s->uart0_amba_clk,
+ ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART0_CPU1XCLKACT));
+ clock_set_frequency(s->uart1_amba_clk,
+ ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART1_CPU1XCLKACT));
+
+ /* compute uartX ref clocks */
+ clock_set_frequency(s->uart0_ref_clk,
+ ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
+ clock_set_frequency(s->uart1_ref_clk,
+ ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
+}
+
static void zynq_slcr_reset(DeviceState *d)
{
ZynqSLCRState *s = ZYNQ_SLCR(d);
@@ -274,6 +379,8 @@ static void zynq_slcr_reset(DeviceState *d)
s->regs[R_DDRIOB + 4] = s->regs[R_DDRIOB + 5] = s->regs[R_DDRIOB + 6]
= 0x00000e00;
s->regs[R_DDRIOB + 12] = 0x00000021;
+
+ zynq_slcr_compute_clocks(s);
}
@@ -408,6 +515,14 @@ 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_ARM_CLK_CTRL:
+ case R_APER_CLK_CTRL:
+ case R_UART_CLK_CTRL:
+ zynq_slcr_compute_clocks(s);
+ break;
}
}
@@ -417,6 +532,14 @@ static const MemoryRegionOps slcr_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
+static const ClockPortInitArray zynq_slcr_clocks = {
+ QDEV_CLOCK_OUT(ZynqSLCRState, uart0_amba_clk),
+ QDEV_CLOCK_OUT(ZynqSLCRState, uart1_amba_clk),
+ 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);
@@ -424,6 +547,8 @@ static void zynq_slcr_init(Object *obj)
memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
ZYNQ_SLCR_MMIO_SIZE);
sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+
+ qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
}
static const VMStateDescription vmstate_zynq_slcr = {
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (6 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:26 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
` (2 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Add bus interface and uart reference clock inputs.
Note: it is hard to find out from the doc what is the behavior when only one
of the clock is disabled.
The implemented behaviour is that register access needs both clock being active.
The bus interface control the mmios visibility
The reference clock controls the baudrate generation. If it disabled,
any input characters and events are ignored. Also register accesses are
conditioned to the clock being enabled (but is it really the case in
reality ?) and a guest error is triggerred if that is not the case.
If theses clocks remains unconnected, the uart behaves as before
(default to 50MHz ref clock).
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
include/hw/char/cadence_uart.h | 3 ++
hw/char/cadence_uart.c | 92 ++++++++++++++++++++++++++++++++--
hw/char/trace-events | 3 ++
3 files changed, 93 insertions(+), 5 deletions(-)
diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
index 118e3f10de..fd1d4725f4 100644
--- a/include/hw/char/cadence_uart.h
+++ b/include/hw/char/cadence_uart.h
@@ -21,6 +21,7 @@
#include "hw/sysbus.h"
#include "chardev/char-fe.h"
#include "qemu/timer.h"
+#include "hw/clock-port.h"
#define CADENCE_UART_RX_FIFO_SIZE 16
#define CADENCE_UART_TX_FIFO_SIZE 16
@@ -47,6 +48,8 @@ typedef struct {
CharBackend chr;
qemu_irq irq;
QEMUTimer *fifo_trigger_handle;
+ ClockIn *refclk;
+ ClockIn *busclk;
} CadenceUARTState;
static inline DeviceState *cadence_uart_create(hwaddr addr,
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index fbdbd463bb..feb5cee4d7 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -28,6 +28,7 @@
#include "qemu/timer.h"
#include "qemu/log.h"
#include "hw/char/cadence_uart.h"
+#include "trace.h"
#ifdef CADENCE_UART_ERR_DEBUG
#define DB_PRINT(...) do { \
@@ -94,7 +95,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)
@@ -165,15 +166,30 @@ static void uart_send_breaks(CadenceUARTState *s)
&break_enabled);
}
+static unsigned int uart_input_clk(CadenceUARTState *s)
+{
+ return clock_get_frequency(s->refclk);
+}
+
static void uart_parameters_setup(CadenceUARTState *s)
{
QEMUSerialSetParams ssp;
unsigned int baud_rate, packet_size;
baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
- UART_INPUT_CLK / 8 : UART_INPUT_CLK;
+ uart_input_clk(s) / 8 : uart_input_clk(s);
+ baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
+ trace_cadence_uart_baudrate(baud_rate);
+
+ ssp.speed = baud_rate;
+ if (ssp.speed == 0) {
+ /*
+ * Avoid division-by-zero below.
+ * TODO: find something better
+ */
+ ssp.speed = 1;
+ }
- ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
packet_size = 1;
switch (s->r[R_MR] & UART_MR_PAR) {
@@ -337,6 +353,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 */
+ if (!clock_is_enabled(s->refclk)) {
+ return;
+ }
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
uart_write_rx_fifo(opaque, buf, size);
}
@@ -350,6 +371,11 @@ static void uart_event(void *opaque, int event)
CadenceUARTState *s = opaque;
uint8_t buf = '\0';
+ /* ignore events when unclocked */
+ if (!clock_is_enabled(s->refclk)) {
+ return;
+ }
+
if (event == CHR_EVENT_BREAK) {
uart_write_rx_fifo(opaque, &buf, 1);
}
@@ -382,6 +408,14 @@ static void uart_write(void *opaque, hwaddr offset,
{
CadenceUARTState *s = opaque;
+ /* ignore accesses when bus or ref clock is disabled */
+ if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "cadence_uart: Trying to write register 0x%x"
+ " while clock is disabled\n", (unsigned) offset);
+ return;
+ }
+
DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
offset >>= 2;
if (offset >= CADENCE_UART_R_MAX) {
@@ -440,6 +474,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
CadenceUARTState *s = opaque;
uint32_t c = 0;
+ /* ignore accesses when bus or ref clock is disabled */
+ if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "cadence_uart: Trying to read register 0x%x"
+ " while clock is disabled\n", (unsigned) offset);
+ return 0;
+ }
+
offset >>= 2;
if (offset >= CADENCE_UART_R_MAX) {
c = 0;
@@ -488,6 +530,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);
@@ -497,9 +547,26 @@ static void cadence_uart_init(Object *obj)
sysbus_init_mmio(sbd, &s->iomem);
sysbus_init_irq(sbd, &s->irq);
+ s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
+ cadence_uart_refclk_update, s);
+ /* initialize the frequency in case the clock remains unconnected */
+ clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
+ s->busclk = qdev_init_clock_in(DEVICE(obj), "busclk", NULL, NULL);
+ /* initialize the frequency to non-zero in case it remains unconnected */
+ clock_init_frequency(s->busclk, 100 * 1000 * 1000);
+
s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
}
+static int cadence_uart_pre_load(void *opaque)
+{
+ CadenceUARTState *s = opaque;
+
+ clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
+ clock_init_frequency(s->busclk, 100 * 1000 * 1000);
+ return 0;
+}
+
static int cadence_uart_post_load(void *opaque, int version_id)
{
CadenceUARTState *s = opaque;
@@ -516,10 +583,22 @@ static int cadence_uart_post_load(void *opaque, int version_id)
return 0;
}
+static const VMStateDescription vmstate_cadence_uart_clocks = {
+ .name = "cadence_uart_clocks",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_CLOCKIN(refclk, CadenceUARTState),
+ VMSTATE_CLOCKIN(busclk, CadenceUARTState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_cadence_uart = {
.name = "cadence_uart",
.version_id = 2,
.minimum_version_id = 2,
+ .pre_load = cadence_uart_pre_load,
.post_load = cadence_uart_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
@@ -532,7 +611,10 @@ static const VMStateDescription vmstate_cadence_uart = {
VMSTATE_UINT32(rx_wpos, CadenceUARTState),
VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
VMSTATE_END_OF_LIST()
- }
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_cadence_uart_clocks,
+ },
};
static Property cadence_uart_properties[] = {
@@ -548,7 +630,7 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_cadence_uart;
dc->reset = cadence_uart_reset;
dc->props = cadence_uart_properties;
- }
+}
static const TypeInfo cadence_uart_info = {
.name = TYPE_CADENCE_UART,
diff --git a/hw/char/trace-events b/hw/char/trace-events
index b64213d4dd..2ea25d1ea1 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -73,3 +73,6 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got character 0x%x from backe
cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend pending"
cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to backend"
cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
+
+# hw/char/cadence_uart.c
+cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (7 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support Damien Hedde
@ 2018-10-02 14:24 ` Damien Hedde
2018-10-02 23:28 ` Philippe Mathieu-Daudé
2018-10-04 16:13 ` [Qemu-devel] [PATCH v5 0/9] Clock framework API Philippe Mathieu-Daudé
2018-10-12 15:26 ` Damien Hedde
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-02 14:24 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, pbonzini, peter.maydell, alistair, saipava,
mark.burton, luc.michel, konrad, edgar.iglesias, Damien Hedde
Add the connection between the slcr's output clocks and the uarts inputs.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
hw/arm/xilinx_zynq.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index f1496d2927..88f61c6a18 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -166,7 +166,7 @@ static void zynq_init(MachineState *machine)
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
- DeviceState *dev;
+ DeviceState *dev, *slcr;
SysBusDevice *busdev;
qemu_irq pic[64];
int n;
@@ -212,9 +212,10 @@ 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);
dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -235,8 +236,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(dev, "busclk", slcr, "uart0_amba_clk", &error_abort);
+ qdev_connect_clock(dev, "refclk", slcr, "uart0_ref_clk", &error_abort);
+ dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+ qdev_connect_clock(dev, "busclk", slcr, "uart1_amba_clk", &error_abort);
+ qdev_connect_clock(dev, "refclk", slcr, "uart1_ref_clk", &error_abort);
sysbus_create_varargs("cadence_ttc", 0xF8001000,
pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
--
2.19.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
@ 2018-10-02 22:42 ` Philippe Mathieu-Daudé
2018-10-12 10:20 ` Damien Hedde
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 22:42 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> This prints the clocks attached to a DeviceState when using "info qtree" monitor
> command. For every clock, it displays the direction, the name and if the
> clock is forwarded. For input clock, it displays also the frequency.
What would also be really useful (during development mostly)
is a "info clktree" monitor command.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> qdev-monitor.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 61e0300991..8c39a3a65b 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -682,6 +682,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
> ObjectClass *class;
> BusState *child;
> NamedGPIOList *ngl;
> + NamedClockList *clk;
>
> qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
> dev->id ? dev->id : "");
> @@ -696,6 +697,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
> ngl->num_out);
> }
> }
> + QLIST_FOREACH(clk, &dev->clocks, node) {
> + if (clk->out) {
> + qdev_printf("clock-out%s \"%s\"\n",
> + clk->forward ? " (fw)" : "",
> + clk->name);
> + } else {
> + qdev_printf("clock-in%s \"%s\" freq=%" PRIu64 "Hz\n",
IMHO 'freq_hz=%" PRIu64 "\n"' is easier to read.
However if we plan to add/use a qemu_strtohz() similar to
qemu_strtosz_metric(), that would be fine.
> + clk->forward ? " (fw)" : "",
> + clk->name, clock_get_frequency(clk->in));
> + }
> + }
> class = object_get_class(OBJECT(dev));
> do {
> qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
@ 2018-10-02 23:10 ` Philippe Mathieu-Daudé
2018-10-12 13:24 ` Damien Hedde
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:10 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add 2 clock outputs for each uart (uart0 & 1):
> + the reference clock
> + the bus interface clock
>
> The clock frequencies are computed using the internal pll & uart configuration
> registers.
>
> All clocks depend on the main input clock (ps_clk), which is hard-coded
> to 33.33MHz (zcu102 evaluation board frequency and frequency specified
> in Xilinx's device tree file)
What about adding in zynq_init:
ClockOut *ps_clk = qdev_init_clock_out(DEVICE(machine), "ps_clk");
clock_set_frequency(ps_clk, PS_REF_CLK_FREQUENCY);
and later:
qdev_connect_clock(slcr, "ps_clk",
DEVICE(machine), "ps_clk_in", &error_abort);
That would be a more complete example of clock tree.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> hw/misc/zynq_slcr.c | 125 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index baa13d1316..0b599ebd97 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -33,6 +33,8 @@
> } \
> } while (0)
>
> +#define INPUT_PS_REF_CLK_FREQUENCY (33333333)
> +
> #define XILINX_LOCK_KEY 0x767b
> #define XILINX_UNLOCK_KEY 0xdf0d
>
> @@ -44,15 +46,27 @@ 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)
> REG32(IO_PLL_CFG, 0x118)
>
> REG32(ARM_CLK_CTRL, 0x120)
> + FIELD(ARM_CLK_CTRL, SRCSEL, 4, 2)
> + FIELD(ARM_CLK_CTRL, DIVISOR, 8, 6)
> + FIELD(ARM_CLK_CTRL, CPU_1XCLKACT, 27, 1)
> + FIELD(ARM_CLK_CTRL, CPU_PERI_CLKACT, 28, 1)
> REG32(DDR_CLK_CTRL, 0x124)
> REG32(DCI_CLK_CTRL, 0x128)
> REG32(APER_CLK_CTRL, 0x12c)
> + FIELD(APER_CLK_CTRL, UART0_CPU1XCLKACT, 20, 1)
> + FIELD(APER_CLK_CTRL, UART1_CPU1XCLKACT, 21, 1)
> REG32(USB0_CLK_CTRL, 0x130)
> REG32(USB1_CLK_CTRL, 0x134)
> REG32(GEM0_RCLK_CTRL, 0x138)
> @@ -63,12 +77,19 @@ 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)
> REG32(DBG_CLK_CTRL, 0x164)
> REG32(PCAP_CLK_CTRL, 0x168)
> REG32(TOPSW_CLK_CTRL, 0x16c)
> +/* common fields to lots of *_CLK_CTRL registers */
> + FIELD(xxx_CLK_CTRL, SRCSEL, 4, 2)
> + FIELD(xxx_CLK_CTRL, DIVISOR, 8, 6)
>
> #define FPGA_CTRL_REGS(n, start) \
> REG32(FPGA ## n ## _CLK_CTRL, (start)) \
> @@ -178,8 +199,92 @@ typedef struct ZynqSLCRState {
> MemoryRegion iomem;
>
> uint32_t regs[ZYNQ_SLCR_NUM_REGS];
> +
> + ClockOut *uart0_amba_clk;
> + ClockOut *uart1_amba_clk;
> + ClockOut *uart0_ref_clk;
> + ClockOut *uart1_ref_clk;
> } ZynqSLCRState;
>
> +/*
> + * return the output frequency of ARM/DDR/IO pll
> + * using input frequency and PLL_CTRL register
> + */
> +static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
> +{
> + uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
> + R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
> +
> + /* first, check if pll is bypassed */
> + if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
> + return input;
> + }
> +
> + /* is pll disabled ? */
> + if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
> + R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
> + return 0;
> + }
> +
> + return input * mult;
> +}
> +
> +/*
> + * return the output frequency of a clock given:
> + * + the pll's frequencies in an array corresponding to mux's indexes
> + * + the register xxx_CLK_CTRL value
> + * + enable bit index in ctrl register
> + *
> + * This function make the assumption that ctrl_reg value is organized as follow:
> + * + bits[13:8] clock divisor
> + * + bits[5:4] clock mux selector (index in array)
> + * + bits[index] clock enable
> + */
> +static uint64_t zynq_slcr_compute_clock(const uint64_t plls[],
> + uint32_t ctrl_reg,
> + unsigned index)
> +{
> + uint32_t divisor = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, DIVISOR);
> + uint32_t srcsel = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, SRCSEL);
> +
> + if ((ctrl_reg & (1u << index)) == 0) {
> + return 0;
> + }
> +
> + return plls[srcsel] / (divisor ? divisor : 1u);
> +}
> +
> +#define ZYNQ_CLOCK(_state, _plls, _reg, _enable_field) \
> + zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
> + R_ ## _reg ## _ ## _enable_field ## _SHIFT)
> +#define ZYNQ_GATE(_state, _clk, _reg, _field) \
> + (ARRAY_FIELD_EX32((_state)->regs, _reg, _field) ? (_clk) : 0)
> +
> +static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
> +{
> + uint64_t ps_clk = INPUT_PS_REF_CLK_FREQUENCY;
> + 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 cpu_mux[4] = {arm_pll, arm_pll, ddr_pll, io_pll};
> + uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
> + uint64_t cpu1x_clk;
> +
> + /* compute uartX amba clocks */
> + cpu1x_clk = ZYNQ_CLOCK(s, cpu_mux, ARM_CLK_CTRL, CPU_PERI_CLKACT);
> + cpu1x_clk = ZYNQ_GATE(s, cpu1x_clk, ARM_CLK_CTRL, CPU_1XCLKACT);
> + clock_set_frequency(s->uart0_amba_clk,
> + ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART0_CPU1XCLKACT));
> + clock_set_frequency(s->uart1_amba_clk,
> + ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART1_CPU1XCLKACT));
> +
> + /* compute uartX ref clocks */
> + clock_set_frequency(s->uart0_ref_clk,
> + ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
> + clock_set_frequency(s->uart1_ref_clk,
> + ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
> +}
> +
> static void zynq_slcr_reset(DeviceState *d)
> {
> ZynqSLCRState *s = ZYNQ_SLCR(d);
> @@ -274,6 +379,8 @@ static void zynq_slcr_reset(DeviceState *d)
> s->regs[R_DDRIOB + 4] = s->regs[R_DDRIOB + 5] = s->regs[R_DDRIOB + 6]
> = 0x00000e00;
> s->regs[R_DDRIOB + 12] = 0x00000021;
> +
> + zynq_slcr_compute_clocks(s);
> }
>
>
> @@ -408,6 +515,14 @@ 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_ARM_CLK_CTRL:
> + case R_APER_CLK_CTRL:
> + case R_UART_CLK_CTRL:
> + zynq_slcr_compute_clocks(s);
> + break;
> }
> }
>
> @@ -417,6 +532,14 @@ static const MemoryRegionOps slcr_ops = {
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> +static const ClockPortInitArray zynq_slcr_clocks = {
> + QDEV_CLOCK_OUT(ZynqSLCRState, uart0_amba_clk),
> + QDEV_CLOCK_OUT(ZynqSLCRState, uart1_amba_clk),
> + 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);
> @@ -424,6 +547,8 @@ static void zynq_slcr_init(Object *obj)
> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
> ZYNQ_SLCR_MMIO_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +
> + qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
> }
>
> static const VMStateDescription vmstate_zynq_slcr = {
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support Damien Hedde
@ 2018-10-02 23:26 ` Philippe Mathieu-Daudé
2018-10-12 13:42 ` Damien Hedde
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:26 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add bus interface and uart reference clock inputs.
>
> Note: it is hard to find out from the doc what is the behavior when only one
> of the clock is disabled.
>
> The implemented behaviour is that register access needs both clock being active.
>
> The bus interface control the mmios visibility
This sentence sounds odd.
>
> The reference clock controls the baudrate generation. If it disabled,
> any input characters and events are ignored. Also register accesses are
> conditioned to the clock being enabled (but is it really the case in
> reality ?) and a guest error is triggerred if that is not the case.
>
> If theses clocks remains unconnected, the uart behaves as before
> (default to 50MHz ref clock).
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> include/hw/char/cadence_uart.h | 3 ++
> hw/char/cadence_uart.c | 92 ++++++++++++++++++++++++++++++++--
> hw/char/trace-events | 3 ++
> 3 files changed, 93 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
> index 118e3f10de..fd1d4725f4 100644
> --- a/include/hw/char/cadence_uart.h
> +++ b/include/hw/char/cadence_uart.h
> @@ -21,6 +21,7 @@
> #include "hw/sysbus.h"
> #include "chardev/char-fe.h"
> #include "qemu/timer.h"
> +#include "hw/clock-port.h"
>
> #define CADENCE_UART_RX_FIFO_SIZE 16
> #define CADENCE_UART_TX_FIFO_SIZE 16
> @@ -47,6 +48,8 @@ typedef struct {
> CharBackend chr;
> qemu_irq irq;
> QEMUTimer *fifo_trigger_handle;
> + ClockIn *refclk;
> + ClockIn *busclk;
> } CadenceUARTState;
>
> static inline DeviceState *cadence_uart_create(hwaddr addr,
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index fbdbd463bb..feb5cee4d7 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -28,6 +28,7 @@
> #include "qemu/timer.h"
> #include "qemu/log.h"
> #include "hw/char/cadence_uart.h"
> +#include "trace.h"
>
> #ifdef CADENCE_UART_ERR_DEBUG
> #define DB_PRINT(...) do { \
> @@ -94,7 +95,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)
> @@ -165,15 +166,30 @@ static void uart_send_breaks(CadenceUARTState *s)
> &break_enabled);
> }
>
> +static unsigned int uart_input_clk(CadenceUARTState *s)
> +{
> + return clock_get_frequency(s->refclk);
> +}
> +
> static void uart_parameters_setup(CadenceUARTState *s)
> {
> QEMUSerialSetParams ssp;
> unsigned int baud_rate, packet_size;
>
> baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
> - UART_INPUT_CLK / 8 : UART_INPUT_CLK;
> + uart_input_clk(s) / 8 : uart_input_clk(s);
> + baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
Safe because s->r[R_BRGR] >= 1, OK.
> + trace_cadence_uart_baudrate(baud_rate);
> +
> + ssp.speed = baud_rate;
> + if (ssp.speed == 0) {
> + /*
> + * Avoid division-by-zero below.
> + * TODO: find something better
> + */
> + ssp.speed = 1;
> + }
>
> - ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> packet_size = 1;
>
> switch (s->r[R_MR] & UART_MR_PAR) {
> @@ -337,6 +353,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 */
> + if (!clock_is_enabled(s->refclk)) {
> + return;
> + }
> +
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> uart_write_rx_fifo(opaque, buf, size);
> }
> @@ -350,6 +371,11 @@ static void uart_event(void *opaque, int event)
> CadenceUARTState *s = opaque;
> uint8_t buf = '\0';
>
> + /* ignore events when unclocked */
> + if (!clock_is_enabled(s->refclk)) {
> + return;
> + }
> +
> if (event == CHR_EVENT_BREAK) {
> uart_write_rx_fifo(opaque, &buf, 1);
> }
> @@ -382,6 +408,14 @@ static void uart_write(void *opaque, hwaddr offset,
> {
> CadenceUARTState *s = opaque;
>
> + /* ignore accesses when bus or ref clock is disabled */
> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "cadence_uart: Trying to write register 0x%x"
> + " while clock is disabled\n", (unsigned) offset);
> + return;
> + }
> +
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> @@ -440,6 +474,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
> CadenceUARTState *s = opaque;
> uint32_t c = 0;
>
> + /* ignore accesses when bus or ref clock is disabled */
> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "cadence_uart: Trying to read register 0x%x"
> + " while clock is disabled\n", (unsigned) offset);
> + return 0;
> + }
> +
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> c = 0;
> @@ -488,6 +530,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);
> @@ -497,9 +547,26 @@ static void cadence_uart_init(Object *obj)
> sysbus_init_mmio(sbd, &s->iomem);
> sysbus_init_irq(sbd, &s->irq);
>
> + s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
> + cadence_uart_refclk_update, s);
> + /* initialize the frequency in case the clock remains unconnected */
> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
> + s->busclk = qdev_init_clock_in(DEVICE(obj), "busclk", NULL, NULL);
> + /* initialize the frequency to non-zero in case it remains unconnected */
> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
#define INPUT_BUS_CLK_FREQUENCY (100 * 1000 * 1000)
> +
> s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
> }
>
> +static int cadence_uart_pre_load(void *opaque)
> +{
> + CadenceUARTState *s = opaque;
> +
> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
INPUT_BUS_CLK_FREQUENCY
> + return 0;
> +}
> +
> static int cadence_uart_post_load(void *opaque, int version_id)
> {
> CadenceUARTState *s = opaque;
> @@ -516,10 +583,22 @@ static int cadence_uart_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static const VMStateDescription vmstate_cadence_uart_clocks = {
> + .name = "cadence_uart_clocks",
> + .version_id = 0,
> + .minimum_version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_CLOCKIN(refclk, CadenceUARTState),
> + VMSTATE_CLOCKIN(busclk, CadenceUARTState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_cadence_uart = {
> .name = "cadence_uart",
> .version_id = 2,
> .minimum_version_id = 2,
> + .pre_load = cadence_uart_pre_load,
> .post_load = cadence_uart_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
> @@ -532,7 +611,10 @@ static const VMStateDescription vmstate_cadence_uart = {
> VMSTATE_UINT32(rx_wpos, CadenceUARTState),
> VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
> VMSTATE_END_OF_LIST()
> - }
> + },
> + .subsections = (const VMStateDescription * []) {
> + &vmstate_cadence_uart_clocks,
> + },
> };
>
> static Property cadence_uart_properties[] = {
> @@ -548,7 +630,7 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_cadence_uart;
> dc->reset = cadence_uart_reset;
> dc->props = cadence_uart_properties;
> - }
> +}
>
> static const TypeInfo cadence_uart_info = {
> .name = TYPE_CADENCE_UART,
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index b64213d4dd..2ea25d1ea1 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -73,3 +73,6 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got character 0x%x from backe
> cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend pending"
> cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to backend"
> cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
> +
> +# hw/char/cadence_uart.c
> +cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Except migration:
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
@ 2018-10-02 23:28 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:28 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add the connection between the slcr's output clocks and the uarts inputs.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> hw/arm/xilinx_zynq.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index f1496d2927..88f61c6a18 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -166,7 +166,7 @@ static void zynq_init(MachineState *machine)
> MemoryRegion *address_space_mem = get_system_memory();
> MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
> MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
> - DeviceState *dev;
> + DeviceState *dev, *slcr;
> SysBusDevice *busdev;
> qemu_irq pic[64];
> int n;
> @@ -212,9 +212,10 @@ 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);
>
> dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
> qdev_prop_set_uint32(dev, "num-cpu", 1);
> @@ -235,8 +236,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(dev, "busclk", slcr, "uart0_amba_clk", &error_abort);
> + qdev_connect_clock(dev, "refclk", slcr, "uart0_ref_clk", &error_abort);
> + dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
> + qdev_connect_clock(dev, "busclk", slcr, "uart1_amba_clk", &error_abort);
> + qdev_connect_clock(dev, "refclk", slcr, "uart1_ref_clk", &error_abort);
>
> sysbus_create_varargs("cadence_ttc", 0xF8001000,
> pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices.
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices Damien Hedde
@ 2018-10-02 23:36 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:36 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add functions to easily add input or output clocks to a device.
> The clock port objects are added as children of the device.
>
> A function allows to connect two clocks together.
> It should be called by some toplevel to make a connection between
> 2 (sub-)devices.
>
> Also add a function which forwards a port to another device. This
> function allows, in the case of device composition, to expose a
> sub-device clock port as its own clock port.
> This is really an alias: when forwarding an input, only one callback can
> be registered on it since there is only one Clockin object behind all
> aliases.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> include/hw/qdev-clock.h | 62 ++++++++++++++++++
> include/hw/qdev-core.h | 14 ++++
> include/hw/qdev.h | 1 +
> hw/core/qdev-clock.c | 140 ++++++++++++++++++++++++++++++++++++++++
> hw/core/qdev.c | 29 +++++++++
> hw/core/Makefile.objs | 2 +-
> 6 files changed, 247 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..d76aa9f479
> --- /dev/null
> +++ b/include/hw/qdev-clock.h
> @@ -0,0 +1,62 @@
> +#ifndef QDEV_CLOCK_H
> +#define QDEV_CLOCK_H
> +
> +#include "hw/clock-port.h"
> +#include "hw/qdev-core.h"
> +#include "qapi/error.h"
> +
> +/**
> + * qdev_init_clock_in:
> + * @dev: the device in which to add a clock
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @opaque: argument for the callback
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a input clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + * The callback will be called with @dev as opaque parameter.
> + */
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque);
> +
> +/**
> + * qdev_init_clock_out:
> + * @dev: the device to add a clock to
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a output clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + */
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_pass_clock:
> + * @dev: the device to forward the clock to
> + * @name: the name of the clock to be added (can't be NULL)
> + * @container: the device which already has the clock
> + * @cont_name: the name of the clock in the container device
> + *
> + * Add a clock @name to @dev which forward to the clock @cont_name in @container
> + */
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> + DeviceState *container, const char *cont_name);
Indent ;)
> +
> +/**
> + * qdev_connect_clock:
> + * @dev: the drived clock device.
> + * @name: the drived clock name.
> + * @driver: the driving clock device.
> + * @driver_name: the driving clock name.
> + * @errp: error report
> + *
> + * Setup @driver_name output clock of @driver to drive @name input clock of
> + * @dev. Errors are trigerred if clock does not exists
> + */
> +void qdev_connect_clock(DeviceState *dev, const char *name,
> + DeviceState *driver, const char *driver_name,
> + Error **errp);
> +
> +#endif /* QDEV_CLOCK_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index f1fd0f8736..e6014d3a41 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -127,6 +127,19 @@ struct NamedGPIOList {
> QLIST_ENTRY(NamedGPIOList) node;
> };
>
> +typedef struct NamedClockList NamedClockList;
> +
> +typedef struct ClockIn ClockIn;
> +typedef struct ClockOut ClockOut;
> +
> +struct NamedClockList {
> + char *name;
> + bool forward;
> + ClockIn *in;
> + ClockOut *out;
> + QLIST_ENTRY(NamedClockList) node;
> +};
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -147,6 +160,7 @@ struct DeviceState {
> int hotplugged;
> BusState *parent_bus;
> QLIST_HEAD(, NamedGPIOList) gpios;
> + QLIST_HEAD(, NamedClockList) clocks;
> QLIST_HEAD(, BusState) child_bus;
> int num_child_bus;
> int instance_id_alias;
> diff --git a/include/hw/qdev.h b/include/hw/qdev.h
> index 5cb8b080a6..b031da7b41 100644
> --- a/include/hw/qdev.h
> +++ b/include/hw/qdev.h
> @@ -4,5 +4,6 @@
> #include "hw/hw.h"
> #include "hw/qdev-core.h"
> #include "hw/qdev-properties.h"
> +#include "hw/qdev-clock.h"
>
> #endif
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> new file mode 100644
> index 0000000000..f0e4839aed
> --- /dev/null
> +++ b/hw/core/qdev-clock.c
> @@ -0,0 +1,140 @@
> +/*
> + * Device's clock
> + *
> + * Copyright GreenSocs 2016-2018
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qom/object.h"
> +#include "hw/qdev-clock.h"
> +#include "qapi/error.h"
> +
> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,
> + bool forward)
> +{
> + NamedClockList *ncl;
> +
> + /*
> + * The clock path will be computed by the device's realize function call.
> + * This is required to ensure the clock's canonical path is right and log
> + * messages are meaningfull.
> + */
> + assert(name);
> + assert(!dev->realized);
> +
Maybe add a comment "This will be free'd in device_finalize()".
> + ncl = g_malloc0(sizeof(*ncl));
> + ncl->name = g_strdup(name);
> + ncl->forward = forward;
> +
> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
> + return ncl;
> +}
> +
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_OUT);
> +
> + /* will fail if name already exists */
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> + object_unref(clk); /* remove the initial ref made by object_new */
> +
> + ncl->out = CLOCK_OUT(clk);
> + return ncl->out;
> +}
> +
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> + ClockCallback *callback, void *opaque)
> +{
> + NamedClockList *ncl;
> + Object *clk;
> +
> + ncl = qdev_init_clocklist(dev, name, false);
> +
> + clk = object_new(TYPE_CLOCK_IN);
> + /*
> + * the ref initialized by object_new will be cleared during dev finalize.
> + * It allows us to safely remove the callback.
> + */
> +
> + /* will fail if name already exists */
> + object_property_add_child(OBJECT(dev), name, clk, &error_abort);
> +
> + ncl->in = CLOCK_IN(clk);
> + if (callback) {
> + clock_set_callback(ncl->in, callback, opaque);
> + }
> + return ncl->in;
> +}
> +
> +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name)
> +{
> + NamedClockList *ncl;
> +
> + QLIST_FOREACH(ncl, &dev->clocks, node) {
> + if (strcmp(name, ncl->name) == 0) {
> + return ncl;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> + DeviceState *container, const char *cont_name)
> +{
> + NamedClockList *original_ncl, *ncl;
> + Object **clk;
> +
> + assert(container && cont_name);
> +
> + original_ncl = qdev_get_clocklist(container, cont_name);
> + assert(original_ncl); /* clock must exist in origin */
> +
> + ncl = qdev_init_clocklist(dev, name, true);
> +
> + if (ncl->out) {
> + clk = (Object **)&ncl->out;
> + } else {
> + clk = (Object **)&ncl->in;
> + }
> +
> + /* will fail if name already exists */
> + object_property_add_link(OBJECT(dev), name, object_get_typename(*clk),
> + clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort);
Indent.
> +}
> +
> +void qdev_connect_clock(DeviceState *dev, const char *name,
> + DeviceState *driver, const char *driver_name,
> + Error **errp)
> +{
> + NamedClockList *ncl, *drv_ncl;
> +
> + assert(dev && name);
> + assert(driver && driver_name);
> +
> + ncl = qdev_get_clocklist(dev, name);
> + if (!ncl || !ncl->in) {
> + error_setg(errp, "no input clock '%s' in device", name);
> + return;
> + }
> +
> + drv_ncl = qdev_get_clocklist(driver, driver_name);
> + if (!drv_ncl || !drv_ncl->out) {
> + error_setg(errp, "no output clock '%s' in driver", driver_name);
> + return;
> + }
> +
> + clock_connect(ncl->in , drv_ncl->out);
> +}
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 529b82de18..c48edf180f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -790,6 +790,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> DeviceClass *dc = DEVICE_GET_CLASS(dev);
> HotplugHandler *hotplug_ctrl;
> BusState *bus;
> + NamedClockList *clk;
> Error *local_err = NULL;
> bool unattached_parent = false;
> static int unattached_count;
> @@ -846,6 +847,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> */
> g_free(dev->canonical_path);
> dev->canonical_path = object_get_canonical_path(OBJECT(dev));
> + QLIST_FOREACH(clk, &dev->clocks, node) {
> + if (clk->forward) {
> + continue;
> + } else if (clk->in != NULL) {
> + clock_in_setup_canonical_path(clk->in);
> + } else {
> + clock_out_setup_canonical_path(clk->out);
> + }
> + }
>
> if (qdev_get_vmsd(dev)) {
> if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
> @@ -972,6 +982,7 @@ static void device_initfn(Object *obj)
> (Object **)&dev->parent_bus, NULL, 0,
> &error_abort);
> QLIST_INIT(&dev->gpios);
> + QLIST_INIT(&dev->clocks);
> }
>
> static void device_post_init(Object *obj)
> @@ -983,6 +994,7 @@ static void device_post_init(Object *obj)
> static void device_finalize(Object *obj)
> {
> NamedGPIOList *ngl, *next;
> + NamedClockList *clk, *clk_next;
>
> DeviceState *dev = DEVICE(obj);
>
> @@ -996,6 +1008,23 @@ static void device_finalize(Object *obj)
> */
> }
>
> + QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) {
> + QLIST_REMOVE(clk, node);
> + if (!clk->forward && clk->in) {
> + /*
> + * if this clock is not forwarded, clk->in & clk->out are child of
> + * dev.
> + * At this point the properties and associated reference are
> + * already deleted but we kept a ref on clk->in to ensure we
> + * don't have a lost callback to a deleted device somewhere.
> + */
> + clock_clear_callback(clk->in);
> + object_unref(OBJECT(clk->in));
> + }
> + g_free(clk->name);
> + g_free(clk);
OK.
> + }
> +
> /* 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 f7102121f4..fc0505e716 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,5 +1,5 @@
> # core qdev-related obj files, also used by *-user:
> -common-obj-y += qdev.o qdev-properties.o
> +common-obj-y += qdev.o qdev-properties.o qdev-clock.o
> common-obj-y += bus.o reset.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation Damien Hedde
@ 2018-10-02 23:48 ` Philippe Mathieu-Daudé
2018-10-03 8:18 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:48 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> docs/devel/clock.txt | 163 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 163 insertions(+)
> create mode 100644 docs/devel/clock.txt
>
> diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
> new file mode 100644
> index 0000000000..6dd8abdee6
> --- /dev/null
> +++ b/docs/devel/clock.txt
> @@ -0,0 +1,163 @@
> +
> +What are device's clocks
> +========================
> +
> +Clocks are ports representing input and output clocks of a device. They are QOM
> +objects developed for the purpose of modeling the distribution of clocks in
> +QEMU.
> +
> +This allows us to model the clock distribution of a platform and detect
> +configuration errors in the clock tree such as badly configured PLL, clock
> +source selection or disabled clock.
> +
> +The objects are CLOCK_IN for the input and CLOCK_OUT for the output.
A simple ASCII diagram would help to make this clearer.
> +
> +The clock value: ClockState
> +===========================
> +
> +The ClockState is the structure carried by the CLOCK_OUT and CLOCK_IN objects.
This is only true for ClockIn, right?
> +It contains one integer field representing the frequency of the clock in Hertz.
unsigned
> +
> +It only simulates the clock by transmitting the frequency value and
> +doesn't model the signal itself such as pin toggle or duty cycle.
> +The special value 0 as a frequency is legal and represent the clock being
> +inactive or gated.
OK.
> +
> +Adding clocks to a device
> +=========================
> +
> +Adding clocks to a device must be done during the init phase of the Device
> +object.
> +
> +To add an input clock to a device, the function qdev_init_clock_in must be used.
> +It takes the name, a callback, and an opaque parameter for the clock.
> +Output is more simple, only the name is required. Typically:
> +qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +qdev_init_clock_out(DEVICE(dev), "clk-out");
> +
> +Both functions return the created CLOCK_IN/OUT pointer, which should be saved
ClockIn/ClockOut
> +in the device's state structure.
> +
> +Theses objects will be automatically deleted by the qom reference mechanism.
QOM
> +
> +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
> +parameters to initialize the clocks: it has the same behaviour as calling the
> +qdev_init_clock/out for each clock in the array.
> +
> +Unconnected input clocks
> +========================
> +
> +Unconnected input clocks have a default frequency value of 0. It means the
> +clock will be considered as disabled. If this is not the wanted behaviour,
> +clock_init_frequency should be called on the ClockIn object during device init.
> +For example:
> +clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +clock_init_frequency(clk, 100 * 1000 * 1000); // init value is 100Mhz
> +
> +Forwarding clocks
> +=================
> +
> +Sometimes, one needs to forward, or inherit, a clock from another device.
> +Typically, when doing device composition, a device might expose a sub-device's
> +clock without interfering with it.
> +The function qdev_pass_clock can be used to achieve this behaviour. Note, that
> +it is possible to expose the clock under a different name. This works for both
> +inputs or outputs.
> +
> +For example, if device B is a child of device A, device_a_instance_init may
> +do something like this:
> +void device_a_instance_init(Object *obj)
> +{
> + AState *A = DEVICE_A(obj);
> + BState *B;
> + [...] /* create B object as child of A */
> + qdev_pass_clock(A, "b_clk", B, "clk");
> + /*
> + * Now A has a clock "b_clk" which forwards to
> + * the "clk" of its child B.
> + */
> +}
> +
> +This function does not returns any clock object. It is not possible to add
> +a callback on a forwarded input clock.
> +
> +Connecting two clocks together
> +==============================
> +
> +Let's say we have 2 devices A and B. A has an output clock named "clkout" and B
> +has an input clock named "clkin".
Maybe "clk_in" "clk_out" to avoid confusion with ClkIn and ClkOut.
> +
> +The clocks are connected together using the function qdev_connect_clock:
> +qdev_connect_clock(B, "clkin", A, "clkout", &error_abort);
> +The device which has the input must be the first argument.
> +
> +It is possible to connect several input clocks to the same output. Every
> +input callback will be called when the output changes.
> +
> +It is not possible to disconnect a clock or to change the clock connection
> +after it is done.
> +
> +Changing a clock output
> +=======================
> +
> +A device can change its outputs using the clock_set function. It will trigger
> +updates on any connected inputs.
> +
> +For example, let's say that we have an output clock "clkout" and we have a
> +pointer to it in the device state because we did the following in init phase:
> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
> +
> +Then at any time, it is possible to change the clock value by doing:
> +clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Mhz */
1GHz
> +
> +Callback on input clock change
> +==============================
> +
> +Here is an example of an input callback:
> +void clock_callback(void *opaque) {
> + MyDeviceState *s = (MyDeviceState *) opaque;
> + /*
> + * opaque may not be the device state pointer, but most probably it is.
> + * (It depends on what is given to the qdev_init_clock_in function)
> + */
> +
> + /* do something with the new frequency */
> + fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
> + clock_get_frequency(dev->my_clk_input));
Yes.
> +}
> +
> +The state argument needs only to be copied if the device needs to use the value
> +later: the state pointer argument of the pointer will not be valid anymore
> +after the end of the function.
> +
> +Migration
> +=========
> +
> +Only the CLOCK_IN object has a state. CLOCK_OUT frequency should not be set
ClkIn ClkOut
> +in migration post_load.
> +
> +In case the frequency of in input clock is needed for a device's migration,
> +this state must be migrated. The VMSTATE_CLOCKIN macro defines an entry to
> +be added in a vmstate description.
> +
> +For example, if a device has a clock input and the device state looks like:
> +MyDeviceState {
> + DeviceState parent_obj;
> + ClockIn *clk;
> +};
> +
> +Then, to add the clock frequency to the device's migrated state, the vmstate
> +description is:
> +VMStateDescription my_device_vmstate = {
> + .name = "my_device",
> + .fields = (VMStateField[]) {
> + VMSTATE_CLOCKIN(clk, MyDeviceState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +When adding a input clock support to an existing device, you must care about
> +migration compatibility. To this end, you can use the clock_init_frequency in
> +a pre_load function to setup a default value in case the source vm does not
> +migrate the frequency.
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects Damien Hedde
@ 2018-10-02 23:53 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 23:53 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Introduce clock port objects: ClockIn and ClockOut.
>
> Theses ports may be used to distribute a clock from a object to several
> other objects. The ClockIn object contains the current state of the
> clock: the frequency.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
> through the callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Makefile.objs | 1 +
> include/hw/clock-port.h | 136 ++++++++++++++++++++++++++++++++++
> hw/core/clock-port.c | 159 ++++++++++++++++++++++++++++++++++++++++
> hw/core/Makefile.objs | 1 +
> hw/core/trace-events | 7 ++
> 5 files changed, 304 insertions(+)
> create mode 100644 include/hw/clock-port.h
> create mode 100644 hw/core/clock-port.c
> create mode 100644 hw/core/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ce9c79235e..b29747075f 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -210,6 +210,7 @@ trace-events-subdirs += hw/audio
> trace-events-subdirs += hw/block
> trace-events-subdirs += hw/block/dataplane
> trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
> trace-events-subdirs += hw/display
> trace-events-subdirs += hw/dma
> trace-events-subdirs += hw/hppa
> diff --git a/include/hw/clock-port.h b/include/hw/clock-port.h
> new file mode 100644
> index 0000000000..8266549350
> --- /dev/null
> +++ b/include/hw/clock-port.h
> @@ -0,0 +1,136 @@
> +#ifndef CLOCK_PORT_H
> +#define CLOCK_PORT_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "qemu/queue.h"
> +#include "migration/vmstate.h"
> +
> +#define TYPE_CLOCK_IN "clock-in"
> +#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
> +#define TYPE_CLOCK_OUT "clock-out"
> +#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
> +
> +typedef void ClockCallback(void *opaque);
> +
> +typedef struct ClockOut ClockOut;
> +typedef struct ClockIn ClockIn;
> +
> +struct ClockIn {
> + /*< private >*/
> + Object parent_obj;
> + /*< private >*/
> + uint64_t frequency;
> + char *canonical_path; /* clock path cache */
> + ClockOut *driver; /* clock output controlling this clock */
> + ClockCallback *callback; /* local callback */
> + void *callback_opaque; /* opaque argument for the callback */
> + QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
> +};
> +
> +struct ClockOut {
> + /*< private >*/
> + Object parent_obj;
> + /*< private >*/
> + char *canonical_path; /* clock path cache */
> + QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
> +};
> +
> +extern const VMStateDescription vmstate_clockin;
> +
> +/*
> + * vmstate description entry to be added in device vmsd.
> + */
> +#define VMSTATE_CLOCKIN(_field, _state) \
> + VMSTATE_CLOCKIN_V(_field, _state, 0)
> +#define VMSTATE_CLOCKIN_V(_field, _state, _version) \
> + VMSTATE_STRUCT_POINTER_V(_field, _state, _version, vmstate_clockin, ClockIn)
> +
> +/**
> + * clock_out_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_out_setup_canonical_path(ClockOut *clk);
> +
> +/**
> + * clock_in_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_in_setup_canonical_path(ClockIn *clk);
> +
> +/**
> + * clock_add_callback:
> + * @clk: the clock to register the callback into
> + * @cb: the callback function
> + * @opaque: the argument to the callback
> + *
> + * Register a callback called on every clock update.
> + */
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
> +
> +/**
> + * clock_clear_callback:
> + * @clk: the clock to delete the callback from
> + *
> + * Unregister the callback registered with clock_set_callback.
> + */
> +void clock_clear_callback(ClockIn *clk);
> +
> +/**
> + * clock_init_frequency:
> + * @clk: the clock to initialize.
> + * @freq: the clock's frequency in Hz or 0 if unclocked.
> + *
> + * Initialize the local cached frequency value of @clk to @freq.
> + * Note: this function must only be called during device inititialization
> + * or migration.
> + */
> +void clock_init_frequency(ClockIn *clk, uint64_t freq);
> +
> +/**
> + * clock_connect:
> + * @clkin: the drived clock.
> + * @clkout: the driving clock.
> + *
> + * Setup @clkout to drive @clkin: Any @clkout update will be propagated
> + * to @clkin.
> + */
> +void clock_connect(ClockIn *clkin, ClockOut *clkout);
> +
> +/**
> + * clock_set_frequency:
> + * @clk: the clock to update.
> + * @freq: the new clock's frequency in Hz or 0 if unclocked.
> + *
> + * Update the @clk to the new @freq.
> + * This change will be propagated through registered clock inputs.
> + */
> +void clock_set_frequency(ClockOut *clk, uint64_t freq);
> +
> +/**
> + * clock_get_frequency:
> + * @clk: the clk to fetch the clock
> + *
> + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
> + */
> +static inline uint64_t clock_get_frequency(const ClockIn *clk)
> +{
> + return clk ? clk->frequency : 0;
> +}
> +
> +/**
> + * clock_is_enabled:
> + * @clk: a clock state
> + *
> + * @return: true if the clock is running. If @clk is NULL return false.
> + */
> +static inline bool clock_is_enabled(const ClockIn *clk)
> +{
> + return clock_get_frequency(clk) != 0;
> +}
> +
> +#endif /* CLOCK_PORT_H */
> diff --git a/hw/core/clock-port.c b/hw/core/clock-port.c
> new file mode 100644
> index 0000000000..25bab0fbed
> --- /dev/null
> +++ b/hw/core/clock-port.c
> @@ -0,0 +1,159 @@
> +/*
> + * Clock inputs and outputs
> + *
> + * Copyright GreenSocs 2016-2018
> + *
> + * Authors:
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + * Damien Hedde <damien.hedde@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "hw/clock-port.h"
> +#include "hw/qdev-core.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +const VMStateDescription vmstate_clockin = {
> + .name = "clockin",
> + .version_id = 0,
> + .minimum_version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(frequency, ClockIn),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +#define CLOCK_PATH(_clk) (_clk->canonical_path)
> +
> +void clock_out_setup_canonical_path(ClockOut *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_in_setup_canonical_path(ClockIn *clk)
> +{
> + g_free(clk->canonical_path);
> + clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
> +{
> + assert(clk);
> +
> + clk->callback = cb;
> + clk->callback_opaque = opaque;
> +}
> +
> +void clock_init_frequency(ClockIn *clk, uint64_t freq)
> +{
> + assert(clk);
> +
> + clk->frequency = freq;
> +}
> +
> +void clock_clear_callback(ClockIn *clk)
> +{
> + clock_set_callback(clk, NULL, NULL);
> +}
> +
> +void clock_connect(ClockIn *clkin, ClockOut *clkout)
> +{
> + assert(clkin && clkin->driver == NULL);
> + assert(clkout);
> +
> + trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
> +
> + QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
> + clkin->driver = clkout;
> +}
> +
> +static void clock_disconnect(ClockIn *clk)
> +{
> + if (clk->driver == NULL) {
> + return;
> + }
> +
> + trace_clock_disconnect(CLOCK_PATH(clk));
> +
> + clk->driver = NULL;
> + QLIST_REMOVE(clk, sibling);
> +}
> +
> +void clock_set_frequency(ClockOut *clk, uint64_t freq)
> +{
> + ClockIn *follower;
> + trace_clock_update(CLOCK_PATH(clk), freq);
Maybe the function name makes sens: trace_clock_set_frequency()
> +
> + QLIST_FOREACH(follower, &clk->followers, sibling) {
> + trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
> + if (follower->frequency != freq) {
> + follower->frequency = freq;
> + if (follower->callback) {
> + follower->callback(follower->callback_opaque);
> + }
> + }
> + }
> +}
> +
> +static void clock_out_initfn(Object *obj)
> +{
> + ClockOut *clk = CLOCK_OUT(obj);
> +
> + QLIST_INIT(&clk->followers);
> +}
> +
> +static void clock_out_finalizefn(Object *obj)
> +{
> + ClockOut *clk = CLOCK_OUT(obj);
> + ClockIn *follower, *next;
> +
> + /* clear our list of followers */
> + QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
> + clock_disconnect(follower);
> + }
> +
> + g_free(clk->canonical_path);
> + clk->canonical_path = NULL;
> +}
> +
> +static void clock_in_finalizefn(Object *obj)
> +{
> + ClockIn *clk = CLOCK_IN(obj);
> +
> + /* remove us from driver's followers list */
> + clock_disconnect(clk);
> +
> + g_free(clk->canonical_path);
> + clk->canonical_path = NULL;
> +}
> +
> +static const TypeInfo clock_out_info = {
> + .name = TYPE_CLOCK_OUT,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(ClockOut),
> + .instance_init = clock_out_initfn,
> + .instance_finalize = clock_out_finalizefn,
> +};
> +
> +static const TypeInfo clock_in_info = {
> + .name = TYPE_CLOCK_IN,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(ClockIn),
> + .instance_finalize = clock_in_finalizefn,
> +};
> +
> +static void clock_register_types(void)
> +{
> + type_register_static(&clock_in_info);
> + type_register_static(&clock_out_info);
> +}
> +
> +type_init(clock_register_types)
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index eb88ca979e..f7102121f4 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> +common-obj-y += clock-port.o
> common-obj-$(CONFIG_SOFTMMU) += nmi.o
>
> common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> new file mode 100644
> index 0000000000..d4880ec138
> --- /dev/null
> +++ b/hw/core/trace-events
> @@ -0,0 +1,7 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# hw/core/clock-port.c
> +clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
> +clock_disconnect(const char *clk) "'%s'"
> +clock_update(const char *clk, uint64_t freq) "'%s' frequency %" PRIu64 "Hz"
> +clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation Damien Hedde
2018-10-02 23:48 ` Philippe Mathieu-Daudé
@ 2018-10-03 8:18 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-03 8:18 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 02/10/2018 16:24, Damien Hedde wrote:
> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> docs/devel/clock.txt | 163 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 163 insertions(+)
> create mode 100644 docs/devel/clock.txt
>
> diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
> new file mode 100644
> index 0000000000..6dd8abdee6
> --- /dev/null
> +++ b/docs/devel/clock.txt
> @@ -0,0 +1,163 @@
> +
> +What are device's clocks
> +========================
> +
> +Clocks are ports representing input and output clocks of a device. They are QOM
> +objects developed for the purpose of modeling the distribution of clocks in
> +QEMU.
> +
> +This allows us to model the clock distribution of a platform and detect
> +configuration errors in the clock tree such as badly configured PLL, clock
> +source selection or disabled clock.
> +
> +The objects are CLOCK_IN for the input and CLOCK_OUT for the output.
> +
> +The clock value: ClockState
> +===========================
> +
> +The ClockState is the structure carried by the CLOCK_OUT and CLOCK_IN objects.
> +It contains one integer field representing the frequency of the clock in Hertz.
> +
> +It only simulates the clock by transmitting the frequency value and
> +doesn't model the signal itself such as pin toggle or duty cycle.
> +The special value 0 as a frequency is legal and represent the clock being
> +inactive or gated.
> +
> +Adding clocks to a device
> +=========================
> +
> +Adding clocks to a device must be done during the init phase of the Device
> +object.
> +
> +To add an input clock to a device, the function qdev_init_clock_in must be used.
> +It takes the name, a callback, and an opaque parameter for the clock.
> +Output is more simple, only the name is required. Typically:
> +qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +qdev_init_clock_out(DEVICE(dev), "clk-out");
> +
> +Both functions return the created CLOCK_IN/OUT pointer, which should be saved
> +in the device's state structure.
> +
> +Theses 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
> +parameters to initialize the clocks: it has the same behaviour as calling the
> +qdev_init_clock/out for each clock in the array.
Can you add a simple example here?
> +
> +Unconnected input clocks
> +========================
> +
> +Unconnected input clocks have a default frequency value of 0. It means the
> +clock will be considered as disabled. If this is not the wanted behaviour,
> +clock_init_frequency should be called on the ClockIn object during device init.
> +For example:
> +clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +clock_init_frequency(clk, 100 * 1000 * 1000); // init value is 100Mhz
> +
> +Forwarding clocks
> +=================
> +
> +Sometimes, one needs to forward, or inherit, a clock from another device.
> +Typically, when doing device composition, a device might expose a sub-device's
> +clock without interfering with it.
> +The function qdev_pass_clock can be used to achieve this behaviour. Note, that
> +it is possible to expose the clock under a different name. This works for both
> +inputs or outputs.
> +
> +For example, if device B is a child of device A, device_a_instance_init may
> +do something like this:
> +void device_a_instance_init(Object *obj)
> +{
> + AState *A = DEVICE_A(obj);
> + BState *B;
> + [...] /* create B object as child of A */
> + qdev_pass_clock(A, "b_clk", B, "clk");
> + /*
> + * Now A has a clock "b_clk" which forwards to
> + * the "clk" of its child B.
> + */
> +}
> +
> +This function does not returns any clock object. It is not possible to add
> +a callback on a forwarded input clock.
> +
> +Connecting two clocks together
> +==============================
> +
> +Let's say we have 2 devices A and B. A has an output clock named "clkout" and B
> +has an input clock named "clkin".
> +
> +The clocks are connected together using the function qdev_connect_clock:
> +qdev_connect_clock(B, "clkin", A, "clkout", &error_abort);
> +The device which has the input must be the first argument.
> +
> +It is possible to connect several input clocks to the same output. Every
> +input callback will be called when the output changes.
> +
> +It is not possible to disconnect a clock or to change the clock connection
> +after it is done.
> +
> +Changing a clock output
> +=======================
> +
> +A device can change its outputs using the clock_set function. It will trigger
> +updates on any connected inputs.
> +
> +For example, let's say that we have an output clock "clkout" and we have a
> +pointer to it in the device state because we did the following in init phase:
> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
> +
> +Then at any time, it is possible to change the clock value by doing:
> +clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Mhz */
> +
> +Callback on input clock change
> +==============================
> +
> +Here is an example of an input callback:
> +void clock_callback(void *opaque) {
> + MyDeviceState *s = (MyDeviceState *) opaque;
> + /*
> + * opaque may not be the device state pointer, but most probably it is.
> + * (It depends on what is given to the qdev_init_clock_in function)
> + */
> +
> + /* do something with the new frequency */
> + fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
> + clock_get_frequency(dev->my_clk_input));
> +}
> +
> +The state argument needs only to be copied if the device needs to use the value
> +later: the state pointer argument of the pointer will not be valid anymore
> +after the end of the function.
> +
> +Migration
> +=========
> +
> +Only the CLOCK_IN object has a state. CLOCK_OUT frequency should not be set
> +in migration post_load.
> +
> +In case the frequency of in input clock is needed for a device's migration,
> +this state must be migrated. The VMSTATE_CLOCKIN macro defines an entry to
> +be added in a vmstate description.
> +
> +For example, if a device has a clock input and the device state looks like:
> +MyDeviceState {
> + DeviceState parent_obj;
> + ClockIn *clk;
> +};
> +
> +Then, to add the clock frequency to the device's migrated state, the vmstate
> +description is:
> +VMStateDescription my_device_vmstate = {
> + .name = "my_device",
> + .fields = (VMStateField[]) {
> + VMSTATE_CLOCKIN(clk, MyDeviceState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +When adding a input clock support to an existing device, you must care about
> +migration compatibility. To this end, you can use the clock_init_frequency in
> +a pre_load function to setup a default value in case the source vm does not
> +migrate the frequency.
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
@ 2018-10-03 8:23 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-03 8:23 UTC (permalink / raw)
To: Damien Hedde, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
On 02/10/2018 16:24, Damien Hedde wrote:
> Introduce a function and macro helpers to setup several clocks
> in a device from a static array description.
>
> An element of the array describes the clock (name and direction) as
> well as the related callback and an optional offset to store the
> created object pointer in the device state structure.
>
> The array must be terminated by a special element QDEV_CLOCK_END.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> include/hw/qdev-clock.h | 67 +++++++++++++++++++++++++++++++++++++++++
> hw/core/qdev-clock.c | 26 ++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> index d76aa9f479..fba907dee2 100644
> --- a/include/hw/qdev-clock.h
> +++ b/include/hw/qdev-clock.h
> @@ -59,4 +59,71 @@ void qdev_connect_clock(DeviceState *dev, const char *name,
> DeviceState *driver, const char *driver_name,
> Error **errp);
>
> +/**
> + * ClockInitElem:
> + * @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 clock pointer in device'state
... to store the the ClockIn or ClockOut pointer in device state
> + * structure (0 means unused)
> + */
> +struct ClockPortInitElem {
> + const char *name;
> + bool output;
Maybe name this 'is_output'
> + ClockCallback *callback;
> + size_t offset;
> +};
> +
> +#define clock_offset_value(_type, _devstate, _field) \
> + (offsetof(_devstate, _field) + \
> + type_check(_type *, typeof_field(_devstate, _field)))
> +
> +#define QDEV_CLOCK(_output, _type, _devstate, _field, _callback) { \
Ditto.
> + .name = (stringify(_field)), \
> + .output = _output, \
> + .callback = _callback, \
> + .offset = clock_offset_value(_type, _devstate, _field), \
> +}
> +
> +/**
> + * QDEV_CLOCK_(IN|OUT):
> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must be
> + * a pointer to that same type.
> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
> + * @_callback: (for input only) callback (or NULL) to be called with the device
> + * state as argument
> + *
> + * The name of the clock will be derived from @_field
> + */
> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
> + QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
> +
> +#define QDEV_CLOCK_OUT(_devstate, _field) \
> + QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
> +
> +/**
> + * QDEV_CLOCK_IN_NOFIELD:
> + * @_name: name of the clock
> + * @_callback: callback (or NULL) to be called with the device state as argument
> + */
> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
> + .name = _name, \
> + .output = false, \
> + .callback = _callback, \
> + .offset = 0, \
> +}
> +
> +#define QDEV_CLOCK_END { .name = NULL }
> +
> +typedef struct ClockPortInitElem ClockPortInitArray[];
> +
> +/**
> + * qdev_init_clocks:
> + * @dev: the device to add clocks
> + * @clocks: a QDEV_CLOCK_END-terminated array which contains the
> + * clocks information.
> + */
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
> +
> #endif /* QDEV_CLOCK_H */
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index f0e4839aed..08afe3983d 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -138,3 +138,29 @@ void qdev_connect_clock(DeviceState *dev, const char *name,
>
> clock_connect(ncl->in , drv_ncl->out);
> }
> +
> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
> +{
> + const struct ClockPortInitElem *elem;
> +
> + assert(dev);
> + assert(clocks);
> +
> + for (elem = &clocks[0]; elem->name != NULL; elem++) {
> + /* offset cannot be inside the DeviceState part */
> + assert(elem->offset == 0 || elem->offset > sizeof(DeviceState));
> + if (elem->output) {
> + ClockOut *clk;
> + clk = qdev_init_clock_out(dev, elem->name);
> + if (elem->offset) {
> + *(ClockOut **)(((void *) dev) + elem->offset) = clk;
> + }
> + } else {
> + ClockIn *clk;
> + clk = qdev_init_clock_in(dev, elem->name, elem->callback, dev);
> + if (elem->offset) {
> + *(ClockIn **)(((void *) dev) + elem->offset) = clk;
> + }
> + }
> + }
> +}
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (8 preceding siblings ...)
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
@ 2018-10-04 16:13 ` Philippe Mathieu-Daudé
2018-10-11 16:20 ` Damien Hedde
2018-10-12 15:26 ` Damien Hedde
10 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-04 16:13 UTC (permalink / raw)
To: Damien Hedde, qemu-devel, Thomas Huth
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Damien,
On 02/10/2018 16:24, Damien Hedde wrote:
> This series aims to add a way to model clocks in qemu between devices.
> This allows to model the clock tree of a platform allowing us to inspect clock
> configuration and detect problems such as disabled clock or bad configured
> pll.
>
> This series is a reroll of the v4 sent recently without the reset feature as
> requested by Peter. I also took into account the reviews about migration and
> other suggestions.
>
> This framework was originally discussed in 2017, here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07218.html
>
> For the user, the framework is now very similar to the device's gpio API.
> Clocks inputs and outputs can be added in devices during initialization phase.
> Then an input can be connected to an output: it means every time the output
> clock changes, a callback in the input is triggered allowing any action to be
> taken. A difference with gpios is that several inputs can be connected to a
> single output without doing any glue.
>
> Behind the scene, there is 2 objects: a clock input which is a placeholder for
> a callback, and a clock output which is a list of inputs. The value transferred
> between an output and an input is an integer representing the clock frequency.
> The input clock callback is called every time the clock frequency changes.
> The input side holds a cached value of the frequency (the output does not need
> one). This allows a device to fetch its input clock frequency at any time
> without caching it itself.
>
> This internal state is added to handle migration in order not to update and
> propagate clocks during it (it adds cross-device and order-specific effects).
> Each device handles its input clock migration by adding the clock frequency in
> its own vmstate description.
>
> Regarding the migration strategy, discussion started in the v4 patches.
> The problem is that we add some kind of wire between different devices and
> we face propagation issue.
> The chosen solution allows migration compatibility from a platform version
> with no implemented clocks to a platform with clocks. A migrated device that
> have a new input clock is responsible to initialize its frequency during
> migration. Each input clock having its own state, such initialization will not
> have any side-effect on other input clock connected to the same output.
> Output clocks, having no state, are not set during migration: If a clock output
> frequency changes due to migration (eg: clock computation bug-fix), the effects
> will be delayed. Eventually the clock output will be updated after migration if
> some (software) event trigger a clock update, but it can not be guaranteed.
>
> There is also the problem of initialization which is very much like the
> migration. Currently, in the zynq example, clocks outputs are initialized in
> the clock controller device_reset. According to Peter, outputs should not be
> set in device_reset, so we need a better way.
>
> It is not simple, since we can have complicated cases with several propagation
> step.
> We can't initialize outputs (without propagating) during device init and uses
> inputs value in device_reset to complete the initialization.
> Consider the case where we have a device generating a frequency, another device
> doing a clock division, then a device using this clock.
> [DevClockGenerator] -> clk1 -> [DevClockDiv] -> clk2 -> [Dev]
> I don't see how we can handle such initialization without doing some
> propagation phase at some point.
>
> Regarding clock gating. The 0 frequency value means the clock is gated.
> If need be, a gate device can be built taking an input gpio and clock and
> generating an output clock.
>
> 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.
>
> We are considering switching to a generic payload evolution of this API.
> For example by specifying the qom carried type when adding an input/output to
> a device. This would allow us, for example, to add a power input port to handle
> power gating or others ports without modifying the device state structure.
>
> Any comments and suggestion are welcomed.
How would you instanciate devices and connect their clocks from the
command line (with the -device option)?
Should clocked devices have DeviceClass::user_creatable = false by default?
Thanks,
Phil.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition Damien Hedde
@ 2018-10-04 17:24 ` Alistair Francis
0 siblings, 0 replies; 33+ messages in thread
From: Alistair Francis @ 2018-10-04 17:24 UTC (permalink / raw)
To: damien.hedde
Cc: qemu-devel@nongnu.org Developers, Edgar Iglesias, Peter Maydell,
Alistair Francis, Mark Burton, Sai Pavan Boddu, qemu-arm,
Paolo Bonzini, konrad, Luc Michel
On Tue, Oct 2, 2018 at 7:36 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Replace the zynq_slcr registers enum and macros using the
> hw/registerfields.h macros.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/misc/zynq_slcr.c | 468 ++++++++++++++++++++++----------------------
> 1 file changed, 234 insertions(+), 234 deletions(-)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index d6bdd027ef..baa13d1316 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -20,6 +20,7 @@
> #include "hw/sysbus.h"
> #include "sysemu/sysemu.h"
> #include "qemu/log.h"
> +#include "hw/registerfields.h"
>
> #ifndef ZYNQ_SLCR_ERR_DEBUG
> #define ZYNQ_SLCR_ERR_DEBUG 0
> @@ -35,138 +36,135 @@
> #define XILINX_LOCK_KEY 0x767b
> #define XILINX_UNLOCK_KEY 0xdf0d
>
> -#define R_PSS_RST_CTRL_SOFT_RST 0x1
> +REG32(SCL, 0x000)
> +REG32(LOCK, 0x004)
> +REG32(UNLOCK, 0x008)
> +REG32(LOCKSTA, 0x00c)
>
> -enum {
> - SCL = 0x000 / 4,
> - LOCK,
> - UNLOCK,
> - LOCKSTA,
> +REG32(ARM_PLL_CTRL, 0x100)
> +REG32(DDR_PLL_CTRL, 0x104)
> +REG32(IO_PLL_CTRL, 0x108)
> +REG32(PLL_STATUS, 0x10c)
> +REG32(ARM_PLL_CFG, 0x110)
> +REG32(DDR_PLL_CFG, 0x114)
> +REG32(IO_PLL_CFG, 0x118)
>
> - ARM_PLL_CTRL = 0x100 / 4,
> - DDR_PLL_CTRL,
> - IO_PLL_CTRL,
> - PLL_STATUS,
> - ARM_PLL_CFG,
> - DDR_PLL_CFG,
> - IO_PLL_CFG,
> -
> - ARM_CLK_CTRL = 0x120 / 4,
> - DDR_CLK_CTRL,
> - DCI_CLK_CTRL,
> - APER_CLK_CTRL,
> - USB0_CLK_CTRL,
> - USB1_CLK_CTRL,
> - GEM0_RCLK_CTRL,
> - GEM1_RCLK_CTRL,
> - GEM0_CLK_CTRL,
> - GEM1_CLK_CTRL,
> - SMC_CLK_CTRL,
> - LQSPI_CLK_CTRL,
> - SDIO_CLK_CTRL,
> - UART_CLK_CTRL,
> - SPI_CLK_CTRL,
> - CAN_CLK_CTRL,
> - CAN_MIOCLK_CTRL,
> - DBG_CLK_CTRL,
> - PCAP_CLK_CTRL,
> - TOPSW_CLK_CTRL,
> +REG32(ARM_CLK_CTRL, 0x120)
> +REG32(DDR_CLK_CTRL, 0x124)
> +REG32(DCI_CLK_CTRL, 0x128)
> +REG32(APER_CLK_CTRL, 0x12c)
> +REG32(USB0_CLK_CTRL, 0x130)
> +REG32(USB1_CLK_CTRL, 0x134)
> +REG32(GEM0_RCLK_CTRL, 0x138)
> +REG32(GEM1_RCLK_CTRL, 0x13c)
> +REG32(GEM0_CLK_CTRL, 0x140)
> +REG32(GEM1_CLK_CTRL, 0x144)
> +REG32(SMC_CLK_CTRL, 0x148)
> +REG32(LQSPI_CLK_CTRL, 0x14c)
> +REG32(SDIO_CLK_CTRL, 0x150)
> +REG32(UART_CLK_CTRL, 0x154)
> +REG32(SPI_CLK_CTRL, 0x158)
> +REG32(CAN_CLK_CTRL, 0x15c)
> +REG32(CAN_MIOCLK_CTRL, 0x160)
> +REG32(DBG_CLK_CTRL, 0x164)
> +REG32(PCAP_CLK_CTRL, 0x168)
> +REG32(TOPSW_CLK_CTRL, 0x16c)
>
> #define FPGA_CTRL_REGS(n, start) \
> - FPGA ## n ## _CLK_CTRL = (start) / 4, \
> - FPGA ## n ## _THR_CTRL, \
> - FPGA ## n ## _THR_CNT, \
> - FPGA ## n ## _THR_STA,
> - FPGA_CTRL_REGS(0, 0x170)
> - FPGA_CTRL_REGS(1, 0x180)
> - FPGA_CTRL_REGS(2, 0x190)
> - FPGA_CTRL_REGS(3, 0x1a0)
> -
> - BANDGAP_TRIP = 0x1b8 / 4,
> - PLL_PREDIVISOR = 0x1c0 / 4,
> - CLK_621_TRUE,
> -
> - PSS_RST_CTRL = 0x200 / 4,
> - DDR_RST_CTRL,
> - TOPSW_RESET_CTRL,
> - DMAC_RST_CTRL,
> - USB_RST_CTRL,
> - GEM_RST_CTRL,
> - SDIO_RST_CTRL,
> - SPI_RST_CTRL,
> - CAN_RST_CTRL,
> - I2C_RST_CTRL,
> - UART_RST_CTRL,
> - GPIO_RST_CTRL,
> - LQSPI_RST_CTRL,
> - SMC_RST_CTRL,
> - OCM_RST_CTRL,
> - FPGA_RST_CTRL = 0x240 / 4,
> - A9_CPU_RST_CTRL,
> -
> - RS_AWDT_CTRL = 0x24c / 4,
> - RST_REASON,
> -
> - REBOOT_STATUS = 0x258 / 4,
> - BOOT_MODE,
> -
> - APU_CTRL = 0x300 / 4,
> - WDT_CLK_SEL,
> -
> - TZ_DMA_NS = 0x440 / 4,
> - TZ_DMA_IRQ_NS,
> - TZ_DMA_PERIPH_NS,
> -
> - PSS_IDCODE = 0x530 / 4,
> -
> - DDR_URGENT = 0x600 / 4,
> - DDR_CAL_START = 0x60c / 4,
> - DDR_REF_START = 0x614 / 4,
> - DDR_CMD_STA,
> - DDR_URGENT_SEL,
> - DDR_DFI_STATUS,
> -
> - MIO = 0x700 / 4,
> + REG32(FPGA ## n ## _CLK_CTRL, (start)) \
> + REG32(FPGA ## n ## _THR_CTRL, (start) + 0x4)\
> + REG32(FPGA ## n ## _THR_CNT, (start) + 0x8)\
> + REG32(FPGA ## n ## _THR_STA, (start) + 0xc)
> +FPGA_CTRL_REGS(0, 0x170)
> +FPGA_CTRL_REGS(1, 0x180)
> +FPGA_CTRL_REGS(2, 0x190)
> +FPGA_CTRL_REGS(3, 0x1a0)
> +
> +REG32(BANDGAP_TRIP, 0x1b8)
> +REG32(PLL_PREDIVISOR, 0x1c0)
> +REG32(CLK_621_TRUE, 0x1c4)
> +
> +REG32(PSS_RST_CTRL, 0x200)
> + FIELD(PSS_RST_CTRL, SOFT_RST, 0, 1)
> +REG32(DDR_RST_CTRL, 0x204)
> +REG32(TOPSW_RESET_CTRL, 0x208)
> +REG32(DMAC_RST_CTRL, 0x20c)
> +REG32(USB_RST_CTRL, 0x210)
> +REG32(GEM_RST_CTRL, 0x214)
> +REG32(SDIO_RST_CTRL, 0x218)
> +REG32(SPI_RST_CTRL, 0x21c)
> +REG32(CAN_RST_CTRL, 0x220)
> +REG32(I2C_RST_CTRL, 0x224)
> +REG32(UART_RST_CTRL, 0x228)
> +REG32(GPIO_RST_CTRL, 0x22c)
> +REG32(LQSPI_RST_CTRL, 0x230)
> +REG32(SMC_RST_CTRL, 0x234)
> +REG32(OCM_RST_CTRL, 0x238)
> +REG32(FPGA_RST_CTRL, 0x240)
> +REG32(A9_CPU_RST_CTRL, 0x244)
> +
> +REG32(RS_AWDT_CTRL, 0x24c)
> +REG32(RST_REASON, 0x250)
> +
> +REG32(REBOOT_STATUS, 0x258)
> +REG32(BOOT_MODE, 0x25c)
> +
> +REG32(APU_CTRL, 0x300)
> +REG32(WDT_CLK_SEL, 0x304)
> +
> +REG32(TZ_DMA_NS, 0x440)
> +REG32(TZ_DMA_IRQ_NS, 0x444)
> +REG32(TZ_DMA_PERIPH_NS, 0x448)
> +
> +REG32(PSS_IDCODE, 0x530)
> +
> +REG32(DDR_URGENT, 0x600)
> +REG32(DDR_CAL_START, 0x60c)
> +REG32(DDR_REF_START, 0x614)
> +REG32(DDR_CMD_STA, 0x618)
> +REG32(DDR_URGENT_SEL, 0x61c)
> +REG32(DDR_DFI_STATUS, 0x620)
> +
> +REG32(MIO, 0x700)
> #define MIO_LENGTH 54
>
> - MIO_LOOPBACK = 0x804 / 4,
> - MIO_MST_TRI0,
> - MIO_MST_TRI1,
> +REG32(MIO_LOOPBACK, 0x804)
> +REG32(MIO_MST_TRI0, 0x808)
> +REG32(MIO_MST_TRI1, 0x80c)
>
> - SD0_WP_CD_SEL = 0x830 / 4,
> - SD1_WP_CD_SEL,
> +REG32(SD0_WP_CD_SEL, 0x830)
> +REG32(SD1_WP_CD_SEL, 0x834)
>
> - LVL_SHFTR_EN = 0x900 / 4,
> - OCM_CFG = 0x910 / 4,
> +REG32(LVL_SHFTR_EN, 0x900)
> +REG32(OCM_CFG, 0x910)
>
> - CPU_RAM = 0xa00 / 4,
> +REG32(CPU_RAM, 0xa00)
>
> - IOU = 0xa30 / 4,
> +REG32(IOU, 0xa30)
>
> - DMAC_RAM = 0xa50 / 4,
> +REG32(DMAC_RAM, 0xa50)
>
> - AFI0 = 0xa60 / 4,
> - AFI1 = AFI0 + 3,
> - AFI2 = AFI1 + 3,
> - AFI3 = AFI2 + 3,
> +REG32(AFI0, 0xa60)
> +REG32(AFI1, 0xa6c)
> +REG32(AFI2, 0xa78)
> +REG32(AFI3, 0xa84)
> #define AFI_LENGTH 3
>
> - OCM = 0xa90 / 4,
> +REG32(OCM, 0xa90)
>
> - DEVCI_RAM = 0xaa0 / 4,
> +REG32(DEVCI_RAM, 0xaa0)
>
> - CSG_RAM = 0xab0 / 4,
> +REG32(CSG_RAM, 0xab0)
>
> - GPIOB_CTRL = 0xb00 / 4,
> - GPIOB_CFG_CMOS18,
> - GPIOB_CFG_CMOS25,
> - GPIOB_CFG_CMOS33,
> - GPIOB_CFG_HSTL = 0xb14 / 4,
> - GPIOB_DRVR_BIAS_CTRL,
> +REG32(GPIOB_CTRL, 0xb00)
> +REG32(GPIOB_CFG_CMOS18, 0xb04)
> +REG32(GPIOB_CFG_CMOS25, 0xb08)
> +REG32(GPIOB_CFG_CMOS33, 0xb0c)
> +REG32(GPIOB_CFG_HSTL, 0xb14)
> +REG32(GPIOB_DRVR_BIAS_CTRL, 0xb18)
>
> - DDRIOB = 0xb40 / 4,
> +REG32(DDRIOB, 0xb40)
> #define DDRIOB_LENGTH 14
> -};
>
> #define ZYNQ_SLCR_MMIO_SIZE 0x1000
> #define ZYNQ_SLCR_NUM_REGS (ZYNQ_SLCR_MMIO_SIZE / 4)
> @@ -189,150 +187,152 @@ static void zynq_slcr_reset(DeviceState *d)
>
> DB_PRINT("RESET\n");
>
> - s->regs[LOCKSTA] = 1;
> + s->regs[R_LOCKSTA] = 1;
> /* 0x100 - 0x11C */
> - s->regs[ARM_PLL_CTRL] = 0x0001A008;
> - s->regs[DDR_PLL_CTRL] = 0x0001A008;
> - s->regs[IO_PLL_CTRL] = 0x0001A008;
> - s->regs[PLL_STATUS] = 0x0000003F;
> - s->regs[ARM_PLL_CFG] = 0x00014000;
> - s->regs[DDR_PLL_CFG] = 0x00014000;
> - s->regs[IO_PLL_CFG] = 0x00014000;
> + s->regs[R_ARM_PLL_CTRL] = 0x0001A008;
> + s->regs[R_DDR_PLL_CTRL] = 0x0001A008;
> + s->regs[R_IO_PLL_CTRL] = 0x0001A008;
> + s->regs[R_PLL_STATUS] = 0x0000003F;
> + s->regs[R_ARM_PLL_CFG] = 0x00014000;
> + s->regs[R_DDR_PLL_CFG] = 0x00014000;
> + s->regs[R_IO_PLL_CFG] = 0x00014000;
>
> /* 0x120 - 0x16C */
> - s->regs[ARM_CLK_CTRL] = 0x1F000400;
> - s->regs[DDR_CLK_CTRL] = 0x18400003;
> - s->regs[DCI_CLK_CTRL] = 0x01E03201;
> - s->regs[APER_CLK_CTRL] = 0x01FFCCCD;
> - s->regs[USB0_CLK_CTRL] = s->regs[USB1_CLK_CTRL] = 0x00101941;
> - s->regs[GEM0_RCLK_CTRL] = s->regs[GEM1_RCLK_CTRL] = 0x00000001;
> - s->regs[GEM0_CLK_CTRL] = s->regs[GEM1_CLK_CTRL] = 0x00003C01;
> - s->regs[SMC_CLK_CTRL] = 0x00003C01;
> - s->regs[LQSPI_CLK_CTRL] = 0x00002821;
> - s->regs[SDIO_CLK_CTRL] = 0x00001E03;
> - s->regs[UART_CLK_CTRL] = 0x00003F03;
> - s->regs[SPI_CLK_CTRL] = 0x00003F03;
> - s->regs[CAN_CLK_CTRL] = 0x00501903;
> - s->regs[DBG_CLK_CTRL] = 0x00000F03;
> - s->regs[PCAP_CLK_CTRL] = 0x00000F01;
> + s->regs[R_ARM_CLK_CTRL] = 0x1F000400;
> + s->regs[R_DDR_CLK_CTRL] = 0x18400003;
> + s->regs[R_DCI_CLK_CTRL] = 0x01E03201;
> + s->regs[R_APER_CLK_CTRL] = 0x01FFCCCD;
> + s->regs[R_USB0_CLK_CTRL] = s->regs[R_USB1_CLK_CTRL] = 0x00101941;
> + s->regs[R_GEM0_RCLK_CTRL] = s->regs[R_GEM1_RCLK_CTRL] = 0x00000001;
> + s->regs[R_GEM0_CLK_CTRL] = s->regs[R_GEM1_CLK_CTRL] = 0x00003C01;
> + s->regs[R_SMC_CLK_CTRL] = 0x00003C01;
> + s->regs[R_LQSPI_CLK_CTRL] = 0x00002821;
> + s->regs[R_SDIO_CLK_CTRL] = 0x00001E03;
> + s->regs[R_UART_CLK_CTRL] = 0x00003F03;
> + s->regs[R_SPI_CLK_CTRL] = 0x00003F03;
> + s->regs[R_CAN_CLK_CTRL] = 0x00501903;
> + s->regs[R_DBG_CLK_CTRL] = 0x00000F03;
> + s->regs[R_PCAP_CLK_CTRL] = 0x00000F01;
>
> /* 0x170 - 0x1AC */
> - s->regs[FPGA0_CLK_CTRL] = s->regs[FPGA1_CLK_CTRL] = s->regs[FPGA2_CLK_CTRL]
> - = s->regs[FPGA3_CLK_CTRL] = 0x00101800;
> - s->regs[FPGA0_THR_STA] = s->regs[FPGA1_THR_STA] = s->regs[FPGA2_THR_STA]
> - = s->regs[FPGA3_THR_STA] = 0x00010000;
> + s->regs[R_FPGA0_CLK_CTRL] = s->regs[R_FPGA1_CLK_CTRL]
> + = s->regs[R_FPGA2_CLK_CTRL]
> + = s->regs[R_FPGA3_CLK_CTRL] = 0x00101800;
> + s->regs[R_FPGA0_THR_STA] = s->regs[R_FPGA1_THR_STA]
> + = s->regs[R_FPGA2_THR_STA]
> + = s->regs[R_FPGA3_THR_STA] = 0x00010000;
>
> /* 0x1B0 - 0x1D8 */
> - s->regs[BANDGAP_TRIP] = 0x0000001F;
> - s->regs[PLL_PREDIVISOR] = 0x00000001;
> - s->regs[CLK_621_TRUE] = 0x00000001;
> + s->regs[R_BANDGAP_TRIP] = 0x0000001F;
> + s->regs[R_PLL_PREDIVISOR] = 0x00000001;
> + s->regs[R_CLK_621_TRUE] = 0x00000001;
>
> /* 0x200 - 0x25C */
> - s->regs[FPGA_RST_CTRL] = 0x01F33F0F;
> - s->regs[RST_REASON] = 0x00000040;
> + s->regs[R_FPGA_RST_CTRL] = 0x01F33F0F;
> + s->regs[R_RST_REASON] = 0x00000040;
>
> - s->regs[BOOT_MODE] = 0x00000001;
> + s->regs[R_BOOT_MODE] = 0x00000001;
>
> /* 0x700 - 0x7D4 */
> for (i = 0; i < 54; i++) {
> - s->regs[MIO + i] = 0x00001601;
> + s->regs[R_MIO + i] = 0x00001601;
> }
> for (i = 2; i <= 8; i++) {
> - s->regs[MIO + i] = 0x00000601;
> + s->regs[R_MIO + i] = 0x00000601;
> }
>
> - s->regs[MIO_MST_TRI0] = s->regs[MIO_MST_TRI1] = 0xFFFFFFFF;
> + s->regs[R_MIO_MST_TRI0] = s->regs[R_MIO_MST_TRI1] = 0xFFFFFFFF;
>
> - s->regs[CPU_RAM + 0] = s->regs[CPU_RAM + 1] = s->regs[CPU_RAM + 3]
> - = s->regs[CPU_RAM + 4] = s->regs[CPU_RAM + 7]
> - = 0x00010101;
> - s->regs[CPU_RAM + 2] = s->regs[CPU_RAM + 5] = 0x01010101;
> - s->regs[CPU_RAM + 6] = 0x00000001;
> + s->regs[R_CPU_RAM + 0] = s->regs[R_CPU_RAM + 1] = s->regs[R_CPU_RAM + 3]
> + = s->regs[R_CPU_RAM + 4] = s->regs[R_CPU_RAM + 7]
> + = 0x00010101;
> + s->regs[R_CPU_RAM + 2] = s->regs[R_CPU_RAM + 5] = 0x01010101;
> + s->regs[R_CPU_RAM + 6] = 0x00000001;
>
> - s->regs[IOU + 0] = s->regs[IOU + 1] = s->regs[IOU + 2] = s->regs[IOU + 3]
> - = 0x09090909;
> - s->regs[IOU + 4] = s->regs[IOU + 5] = 0x00090909;
> - s->regs[IOU + 6] = 0x00000909;
> + s->regs[R_IOU + 0] = s->regs[R_IOU + 1] = s->regs[R_IOU + 2]
> + = s->regs[R_IOU + 3] = 0x09090909;
> + s->regs[R_IOU + 4] = s->regs[R_IOU + 5] = 0x00090909;
> + s->regs[R_IOU + 6] = 0x00000909;
>
> - s->regs[DMAC_RAM] = 0x00000009;
> + s->regs[R_DMAC_RAM] = 0x00000009;
>
> - s->regs[AFI0 + 0] = s->regs[AFI0 + 1] = 0x09090909;
> - s->regs[AFI1 + 0] = s->regs[AFI1 + 1] = 0x09090909;
> - s->regs[AFI2 + 0] = s->regs[AFI2 + 1] = 0x09090909;
> - s->regs[AFI3 + 0] = s->regs[AFI3 + 1] = 0x09090909;
> - s->regs[AFI0 + 2] = s->regs[AFI1 + 2] = s->regs[AFI2 + 2]
> - = s->regs[AFI3 + 2] = 0x00000909;
> + s->regs[R_AFI0 + 0] = s->regs[R_AFI0 + 1] = 0x09090909;
> + s->regs[R_AFI1 + 0] = s->regs[R_AFI1 + 1] = 0x09090909;
> + s->regs[R_AFI2 + 0] = s->regs[R_AFI2 + 1] = 0x09090909;
> + s->regs[R_AFI3 + 0] = s->regs[R_AFI3 + 1] = 0x09090909;
> + s->regs[R_AFI0 + 2] = s->regs[R_AFI1 + 2] = s->regs[R_AFI2 + 2]
> + = s->regs[R_AFI3 + 2] = 0x00000909;
>
> - s->regs[OCM + 0] = 0x01010101;
> - s->regs[OCM + 1] = s->regs[OCM + 2] = 0x09090909;
> + s->regs[R_OCM + 0] = 0x01010101;
> + s->regs[R_OCM + 1] = s->regs[R_OCM + 2] = 0x09090909;
>
> - s->regs[DEVCI_RAM] = 0x00000909;
> - s->regs[CSG_RAM] = 0x00000001;
> + s->regs[R_DEVCI_RAM] = 0x00000909;
> + s->regs[R_CSG_RAM] = 0x00000001;
>
> - s->regs[DDRIOB + 0] = s->regs[DDRIOB + 1] = s->regs[DDRIOB + 2]
> - = s->regs[DDRIOB + 3] = 0x00000e00;
> - s->regs[DDRIOB + 4] = s->regs[DDRIOB + 5] = s->regs[DDRIOB + 6]
> - = 0x00000e00;
> - s->regs[DDRIOB + 12] = 0x00000021;
> + s->regs[R_DDRIOB + 0] = s->regs[R_DDRIOB + 1] = s->regs[R_DDRIOB + 2]
> + = s->regs[R_DDRIOB + 3] = 0x00000e00;
> + s->regs[R_DDRIOB + 4] = s->regs[R_DDRIOB + 5] = s->regs[R_DDRIOB + 6]
> + = 0x00000e00;
> + s->regs[R_DDRIOB + 12] = 0x00000021;
> }
>
>
> static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
> {
> switch (offset) {
> - case LOCK:
> - case UNLOCK:
> - case DDR_CAL_START:
> - case DDR_REF_START:
> + case R_LOCK:
> + case R_UNLOCK:
> + case R_DDR_CAL_START:
> + case R_DDR_REF_START:
> return !rnw; /* Write only */
> - case LOCKSTA:
> - case FPGA0_THR_STA:
> - case FPGA1_THR_STA:
> - case FPGA2_THR_STA:
> - case FPGA3_THR_STA:
> - case BOOT_MODE:
> - case PSS_IDCODE:
> - case DDR_CMD_STA:
> - case DDR_DFI_STATUS:
> - case PLL_STATUS:
> + case R_LOCKSTA:
> + case R_FPGA0_THR_STA:
> + case R_FPGA1_THR_STA:
> + case R_FPGA2_THR_STA:
> + case R_FPGA3_THR_STA:
> + case R_BOOT_MODE:
> + case R_PSS_IDCODE:
> + case R_DDR_CMD_STA:
> + case R_DDR_DFI_STATUS:
> + case R_PLL_STATUS:
> return rnw;/* read only */
> - case SCL:
> - case ARM_PLL_CTRL ... IO_PLL_CTRL:
> - case ARM_PLL_CFG ... IO_PLL_CFG:
> - case ARM_CLK_CTRL ... TOPSW_CLK_CTRL:
> - case FPGA0_CLK_CTRL ... FPGA0_THR_CNT:
> - case FPGA1_CLK_CTRL ... FPGA1_THR_CNT:
> - case FPGA2_CLK_CTRL ... FPGA2_THR_CNT:
> - case FPGA3_CLK_CTRL ... FPGA3_THR_CNT:
> - case BANDGAP_TRIP:
> - case PLL_PREDIVISOR:
> - case CLK_621_TRUE:
> - case PSS_RST_CTRL ... A9_CPU_RST_CTRL:
> - case RS_AWDT_CTRL:
> - case RST_REASON:
> - case REBOOT_STATUS:
> - case APU_CTRL:
> - case WDT_CLK_SEL:
> - case TZ_DMA_NS ... TZ_DMA_PERIPH_NS:
> - case DDR_URGENT:
> - case DDR_URGENT_SEL:
> - case MIO ... MIO + MIO_LENGTH - 1:
> - case MIO_LOOPBACK ... MIO_MST_TRI1:
> - case SD0_WP_CD_SEL:
> - case SD1_WP_CD_SEL:
> - case LVL_SHFTR_EN:
> - case OCM_CFG:
> - case CPU_RAM:
> - case IOU:
> - case DMAC_RAM:
> - case AFI0 ... AFI3 + AFI_LENGTH - 1:
> - case OCM:
> - case DEVCI_RAM:
> - case CSG_RAM:
> - case GPIOB_CTRL ... GPIOB_CFG_CMOS33:
> - case GPIOB_CFG_HSTL:
> - case GPIOB_DRVR_BIAS_CTRL:
> - case DDRIOB ... DDRIOB + DDRIOB_LENGTH - 1:
> + case R_SCL:
> + case R_ARM_PLL_CTRL ... R_IO_PLL_CTRL:
> + case R_ARM_PLL_CFG ... R_IO_PLL_CFG:
> + case R_ARM_CLK_CTRL ... R_TOPSW_CLK_CTRL:
> + case R_FPGA0_CLK_CTRL ... R_FPGA0_THR_CNT:
> + case R_FPGA1_CLK_CTRL ... R_FPGA1_THR_CNT:
> + case R_FPGA2_CLK_CTRL ... R_FPGA2_THR_CNT:
> + case R_FPGA3_CLK_CTRL ... R_FPGA3_THR_CNT:
> + case R_BANDGAP_TRIP:
> + case R_PLL_PREDIVISOR:
> + case R_CLK_621_TRUE:
> + case R_PSS_RST_CTRL ... R_A9_CPU_RST_CTRL:
> + case R_RS_AWDT_CTRL:
> + case R_RST_REASON:
> + case R_REBOOT_STATUS:
> + case R_APU_CTRL:
> + case R_WDT_CLK_SEL:
> + case R_TZ_DMA_NS ... R_TZ_DMA_PERIPH_NS:
> + case R_DDR_URGENT:
> + case R_DDR_URGENT_SEL:
> + case R_MIO ... R_MIO + MIO_LENGTH - 1:
> + case R_MIO_LOOPBACK ... R_MIO_MST_TRI1:
> + case R_SD0_WP_CD_SEL:
> + case R_SD1_WP_CD_SEL:
> + case R_LVL_SHFTR_EN:
> + case R_OCM_CFG:
> + case R_CPU_RAM:
> + case R_IOU:
> + case R_DMAC_RAM:
> + case R_AFI0 ... R_AFI3 + AFI_LENGTH - 1:
> + case R_OCM:
> + case R_DEVCI_RAM:
> + case R_CSG_RAM:
> + case R_GPIOB_CTRL ... R_GPIOB_CFG_CMOS33:
> + case R_GPIOB_CFG_HSTL:
> + case R_GPIOB_DRVR_BIAS_CTRL:
> + case R_DDRIOB ... R_DDRIOB + DDRIOB_LENGTH - 1:
> return true;
> default:
> return false;
> @@ -370,24 +370,24 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
> }
>
> switch (offset) {
> - case SCL:
> - s->regs[SCL] = val & 0x1;
> + case R_SCL:
> + s->regs[R_SCL] = val & 0x1;
> return;
> - case LOCK:
> + case R_LOCK:
> if ((val & 0xFFFF) == XILINX_LOCK_KEY) {
> DB_PRINT("XILINX LOCK 0xF8000000 + 0x%x <= 0x%x\n", (int)offset,
> (unsigned)val & 0xFFFF);
> - s->regs[LOCKSTA] = 1;
> + s->regs[R_LOCKSTA] = 1;
> } else {
> DB_PRINT("WRONG XILINX LOCK KEY 0xF8000000 + 0x%x <= 0x%x\n",
> (int)offset, (unsigned)val & 0xFFFF);
> }
> return;
> - case UNLOCK:
> + case R_UNLOCK:
> if ((val & 0xFFFF) == XILINX_UNLOCK_KEY) {
> DB_PRINT("XILINX UNLOCK 0xF8000000 + 0x%x <= 0x%x\n", (int)offset,
> (unsigned)val & 0xFFFF);
> - s->regs[LOCKSTA] = 0;
> + s->regs[R_LOCKSTA] = 0;
> } else {
> DB_PRINT("WRONG XILINX UNLOCK KEY 0xF8000000 + 0x%x <= 0x%x\n",
> (int)offset, (unsigned)val & 0xFFFF);
> @@ -395,7 +395,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
> return;
> }
>
> - if (s->regs[LOCKSTA]) {
> + if (s->regs[R_LOCKSTA]) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "SCLR registers are locked. Unlock them first\n");
> return;
> @@ -403,8 +403,8 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
> s->regs[offset] = val;
>
> switch (offset) {
> - case PSS_RST_CTRL:
> - if (val & R_PSS_RST_CTRL_SOFT_RST) {
> + case R_PSS_RST_CTRL:
> + if (FIELD_EX32(val, PSS_RST_CTRL, SOFT_RST)) {
> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> }
> break;
> --
> 2.19.0
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-04 16:13 ` [Qemu-devel] [PATCH v5 0/9] Clock framework API Philippe Mathieu-Daudé
@ 2018-10-11 16:20 ` Damien Hedde
2018-10-11 16:23 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-11 16:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Thomas Huth, edgar.iglesias, peter.maydell, alistair,
mark.burton, saipava, qemu-arm, pbonzini, konrad, luc.michel
Hi Philippe,
On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
>
> On 02/10/2018 16:24, Damien Hedde wrote:
>> This series aims to add a way to model clocks in qemu between devices.
>> This allows to model the clock tree of a platform allowing us to inspect clock
>> configuration and detect problems such as disabled clock or bad configured
>> pll.
>>
>> [...]
>>
>> Any comments and suggestion are welcomed.
>
> How would you instanciate devices and connect their clocks from the
> command line (with the -device option)?
I didn't not thought about that. I'm not sure to understand how this is
done for a gpio for example. Is this done by setting the link property
manually ?
If that's the case, I suppose we can somehow add a link property on the
input side to the output side to achieve that. The other way seem not
possible due to the fact the output side has a dynamically allocated
list of inputs.
>
> Should clocked devices have DeviceClass::user_creatable = false by default?
It would be a solution, but I don't think we want this ?
>
> Thanks,
>
> Phil.
>
Thanks,
Damien
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-11 16:20 ` Damien Hedde
@ 2018-10-11 16:23 ` Peter Maydell
2018-10-11 17:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2018-10-11 16:23 UTC (permalink / raw)
To: Damien Hedde
Cc: Philippe Mathieu-Daudé,
QEMU Developers, Thomas Huth, Edgar Iglesias, Alistair Francis,
Mark Burton, Sai Pavan Boddu, qemu-arm, Paolo Bonzini, konrad,
Luc Michel
On 11 October 2018 at 17:20, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Hi Philippe,
>
> On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote:
>> Hi Damien,
>>
>> On 02/10/2018 16:24, Damien Hedde wrote:
>>> This series aims to add a way to model clocks in qemu between devices.
>>> This allows to model the clock tree of a platform allowing us to inspect clock
>>> configuration and detect problems such as disabled clock or bad configured
>>> pll.
>>>
>>> [...]
>>>
>>> Any comments and suggestion are welcomed.
>>
>> How would you instanciate devices and connect their clocks from the
>> command line (with the -device option)?
> I didn't not thought about that. I'm not sure to understand how this is
> done for a gpio for example. Is this done by setting the link property
> manually ?
You can't wire up GPIOs on the command line. I don't think we
really need to be able to wire up clocks on the command line either,
do we?
> Should clocked devices have DeviceClass::user_creatable = false by default?
How many devices have a clock and nothing else that would cause
them to be non-user-creatable (ie no GPIOs, no IRQ lines, no
memory-mapped memory regions) ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-11 16:23 ` Peter Maydell
@ 2018-10-11 17:00 ` Philippe Mathieu-Daudé
2018-10-11 17:12 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-11 17:00 UTC (permalink / raw)
To: Peter Maydell, Damien Hedde
Cc: QEMU Developers, Thomas Huth, Edgar Iglesias, Alistair Francis,
Mark Burton, Sai Pavan Boddu, qemu-arm, Paolo Bonzini, konrad,
Luc Michel
On 11/10/2018 18:23, Peter Maydell wrote:
> On 11 October 2018 at 17:20, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Hi Philippe,
>>
>> On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Damien,
>>>
>>> On 02/10/2018 16:24, Damien Hedde wrote:
>>>> This series aims to add a way to model clocks in qemu between devices.
>>>> This allows to model the clock tree of a platform allowing us to inspect clock
>>>> configuration and detect problems such as disabled clock or bad configured
>>>> pll.
>>>>
>>>> [...]
>>>>
>>>> Any comments and suggestion are welcomed.
>>>
>>> How would you instanciate devices and connect their clocks from the
>>> command line (with the -device option)?
>> I didn't not thought about that. I'm not sure to understand how this is
>> done for a gpio for example. Is this done by setting the link property
>> manually ?
>
> You can't wire up GPIOs on the command line. I don't think we
> really need to be able to wire up clocks on the command line either,
> do we?
This would be a big mess.
>
>> Should clocked devices have DeviceClass::user_creatable = false by default?
>
> How many devices have a clock and nothing else that would cause
> them to be non-user-creatable (ie no GPIOs, no IRQ lines, no
> memory-mapped memory regions) ?
I'm not sure I understood your question.
But I understand than as devices consuming GPIO/IRQ, clocked device
can't be instantiate from command line, and I am happy with that.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-11 17:00 ` Philippe Mathieu-Daudé
@ 2018-10-11 17:12 ` Peter Maydell
2018-10-11 17:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2018-10-11 17:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Damien Hedde, QEMU Developers, Thomas Huth, Edgar Iglesias,
Alistair Francis, Mark Burton, Sai Pavan Boddu, qemu-arm,
Paolo Bonzini, konrad, Luc Michel
On 11 October 2018 at 18:00, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 11/10/2018 18:23, Peter Maydell wrote:
>> How many devices have a clock and nothing else that would cause
>> them to be non-user-creatable (ie no GPIOs, no IRQ lines, no
>> memory-mapped memory regions) ?
>
> I'm not sure I understood your question.
Sorry, let me try rephrasing.
We started with the question of whether devices with clocks should be
marked not user creatable. It's already the case that devices with any
of IRQs, GPIOs or memory-mapped registers can't be user created.
(In particular, any sysbus device is not user-creatable by default.)
So the question was intended to ask how often it matters whether
we can't wire up devices with clocks on the command line.
Are there any devices which would have clocks, but aren't *already*
ruled out of being user creatable for other reasons? If there
aren't any, it doesn't really matter much that we don't have
a mechanism for command line clock wiring.
So I think the answer is:
* almost all users of this API framework are going to be
sysbus devices; those are already not user-creatable
* any device that uses this framework, and which is not
a sysbus device (but instead a plain old qdev device)
will need to mark itself as not-user-creatable by hand,
the same as if it uses the qdev gpio APIs
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-11 17:12 ` Peter Maydell
@ 2018-10-11 17:16 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-11 17:16 UTC (permalink / raw)
To: Peter Maydell
Cc: Damien Hedde, QEMU Developers, Thomas Huth, Edgar Iglesias,
Alistair Francis, Mark Burton, Sai Pavan Boddu, qemu-arm,
Paolo Bonzini, konrad, Luc Michel
On 11/10/2018 19:12, Peter Maydell wrote:
> On 11 October 2018 at 18:00, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 11/10/2018 18:23, Peter Maydell wrote:
>>> How many devices have a clock and nothing else that would cause
>>> them to be non-user-creatable (ie no GPIOs, no IRQ lines, no
>>> memory-mapped memory regions) ?
>>
>> I'm not sure I understood your question.
>
> Sorry, let me try rephrasing.
>
> We started with the question of whether devices with clocks should be
> marked not user creatable. It's already the case that devices with any
> of IRQs, GPIOs or memory-mapped registers can't be user created.
> (In particular, any sysbus device is not user-creatable by default.)
> So the question was intended to ask how often it matters whether
> we can't wire up devices with clocks on the command line.
> Are there any devices which would have clocks, but aren't *already*
> ruled out of being user creatable for other reasons? If there
> aren't any, it doesn't really matter much that we don't have
> a mechanism for command line clock wiring.
Thank you for clearing this out.
>
> So I think the answer is:
> * almost all users of this API framework are going to be
> sysbus devices; those are already not user-creatable
Yes.
> * any device that uses this framework, and which is not
> a sysbus device (but instead a plain old qdev device)
> will need to mark itself as not-user-creatable by hand,
> the same as if it uses the qdev gpio APIs
OK, you answered my first question, it is now clearer to me :)
Thank you!
Phil.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree
2018-10-02 22:42 ` Philippe Mathieu-Daudé
@ 2018-10-12 10:20 ` Damien Hedde
0 siblings, 0 replies; 33+ messages in thread
From: Damien Hedde @ 2018-10-12 10:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Philippe,
On 10/3/18 12:42 AM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
>
> On 10/2/18 4:24 PM, Damien Hedde wrote:
>> This prints the clocks attached to a DeviceState when using "info qtree" monitor
>> command. For every clock, it displays the direction, the name and if the
>> clock is forwarded. For input clock, it displays also the frequency.
>
> What would also be really useful (during development mostly)
> is a "info clktree" monitor command.
What would be the output ? A list of all clock related devices and their
clocks ? Or something more like a clock tree starting from the source(s)
clock up to the last ones ? (or the opposite starting from a
clocked-device down to the sources)
Concerning development, it might also be useful to have access to the
frequency of a clock output. Maybe I should add the value in the object
for that purpose ?
>
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> qdev-monitor.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 61e0300991..8c39a3a65b 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -682,6 +682,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>> ObjectClass *class;
>> BusState *child;
>> NamedGPIOList *ngl;
>> + NamedClockList *clk;
>>
>> qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
>> dev->id ? dev->id : "");
>> @@ -696,6 +697,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>> ngl->num_out);
>> }
>> }
>> + QLIST_FOREACH(clk, &dev->clocks, node) {
>> + if (clk->out) {
>> + qdev_printf("clock-out%s \"%s\"\n",
>> + clk->forward ? " (fw)" : "",
>> + clk->name);
>> + } else {
>> + qdev_printf("clock-in%s \"%s\" freq=%" PRIu64 "Hz\n",
>
> IMHO 'freq_hz=%" PRIu64 "\n"' is easier to read.
The frequency value in hertz is not easy to read in general (eg:
33333333Hz). Maybe we should print it with an apropriate unit (eg:
33.333333MHz). We can also use the "'" printf flag for thousand
separator but it's locale-dependent (and in my case it prints nothing).
>
> However if we plan to add/use a qemu_strtohz() similar to
> qemu_strtosz_metric(), that would be fine.
You mean for test purpose ?
>
>> + clk->forward ? " (fw)" : "",
>> + clk->name, clock_get_frequency(clk->in));
>> + }
>> + }
>> class = object_get_class(OBJECT(dev));
>> do {
>> qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2018-10-02 23:10 ` Philippe Mathieu-Daudé
@ 2018-10-12 13:24 ` Damien Hedde
2018-10-12 13:27 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-12 13:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Philippe,
On 10/3/18 1:10 AM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
>
> On 10/2/18 4:24 PM, Damien Hedde wrote:
>> Add 2 clock outputs for each uart (uart0 & 1):
>> + the reference clock
>> + the bus interface clock
>>
>> The clock frequencies are computed using the internal pll & uart configuration
>> registers.
>>
>> All clocks depend on the main input clock (ps_clk), which is hard-coded
>> to 33.33MHz (zcu102 evaluation board frequency and frequency specified
>> in Xilinx's device tree file)
>
> What about adding in zynq_init:
>
> ClockOut *ps_clk = qdev_init_clock_out(DEVICE(machine), "ps_clk");
> clock_set_frequency(ps_clk, PS_REF_CLK_FREQUENCY);
>
> and later:
>
> qdev_connect_clock(slcr, "ps_clk",
> DEVICE(machine), "ps_clk_in", &error_abort);
>
> That would be a more complete example of clock tree.>
You're right.
I'm wondering if in such a case (a fixed frequency base clock), we
should have a way to change the frequency through command line ? (In
order to start the machine with, for example, a 50MHz base clock).
But I don't know what's the right way to do it and if it needs some kind
of special support in the clock object or in the machine object, like a
frequency property.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> hw/misc/zynq_slcr.c | 125 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 125 insertions(+)
>>
>> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
>> index baa13d1316..0b599ebd97 100644
>> --- a/hw/misc/zynq_slcr.c
>> +++ b/hw/misc/zynq_slcr.c
>> @@ -33,6 +33,8 @@
>> } \
>> } while (0)
>>
>> +#define INPUT_PS_REF_CLK_FREQUENCY (33333333)
>> +
>> #define XILINX_LOCK_KEY 0x767b
>> #define XILINX_UNLOCK_KEY 0xdf0d
>>
>> @@ -44,15 +46,27 @@ 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)
>> REG32(IO_PLL_CFG, 0x118)
>>
>> REG32(ARM_CLK_CTRL, 0x120)
>> + FIELD(ARM_CLK_CTRL, SRCSEL, 4, 2)
>> + FIELD(ARM_CLK_CTRL, DIVISOR, 8, 6)
>> + FIELD(ARM_CLK_CTRL, CPU_1XCLKACT, 27, 1)
>> + FIELD(ARM_CLK_CTRL, CPU_PERI_CLKACT, 28, 1)
>> REG32(DDR_CLK_CTRL, 0x124)
>> REG32(DCI_CLK_CTRL, 0x128)
>> REG32(APER_CLK_CTRL, 0x12c)
>> + FIELD(APER_CLK_CTRL, UART0_CPU1XCLKACT, 20, 1)
>> + FIELD(APER_CLK_CTRL, UART1_CPU1XCLKACT, 21, 1)
>> REG32(USB0_CLK_CTRL, 0x130)
>> REG32(USB1_CLK_CTRL, 0x134)
>> REG32(GEM0_RCLK_CTRL, 0x138)
>> @@ -63,12 +77,19 @@ 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)
>> REG32(DBG_CLK_CTRL, 0x164)
>> REG32(PCAP_CLK_CTRL, 0x168)
>> REG32(TOPSW_CLK_CTRL, 0x16c)
>> +/* common fields to lots of *_CLK_CTRL registers */
>> + FIELD(xxx_CLK_CTRL, SRCSEL, 4, 2)
>> + FIELD(xxx_CLK_CTRL, DIVISOR, 8, 6)
>>
>> #define FPGA_CTRL_REGS(n, start) \
>> REG32(FPGA ## n ## _CLK_CTRL, (start)) \
>> @@ -178,8 +199,92 @@ typedef struct ZynqSLCRState {
>> MemoryRegion iomem;
>>
>> uint32_t regs[ZYNQ_SLCR_NUM_REGS];
>> +
>> + ClockOut *uart0_amba_clk;
>> + ClockOut *uart1_amba_clk;
>> + ClockOut *uart0_ref_clk;
>> + ClockOut *uart1_ref_clk;
>> } ZynqSLCRState;
>>
>> +/*
>> + * return the output frequency of ARM/DDR/IO pll
>> + * using input frequency and PLL_CTRL register
>> + */
>> +static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
>> +{
>> + uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
>> + R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
>> +
>> + /* first, check if pll is bypassed */
>> + if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
>> + return input;
>> + }
>> +
>> + /* is pll disabled ? */
>> + if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
>> + R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
>> + return 0;
>> + }
>> +
>> + return input * mult;
>> +}
>> +
>> +/*
>> + * return the output frequency of a clock given:
>> + * + the pll's frequencies in an array corresponding to mux's indexes
>> + * + the register xxx_CLK_CTRL value
>> + * + enable bit index in ctrl register
>> + *
>> + * This function make the assumption that ctrl_reg value is organized as follow:
>> + * + bits[13:8] clock divisor
>> + * + bits[5:4] clock mux selector (index in array)
>> + * + bits[index] clock enable
>> + */
>> +static uint64_t zynq_slcr_compute_clock(const uint64_t plls[],
>> + uint32_t ctrl_reg,
>> + unsigned index)
>> +{
>> + uint32_t divisor = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, DIVISOR);
>> + uint32_t srcsel = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, SRCSEL);
>> +
>> + if ((ctrl_reg & (1u << index)) == 0) {
>> + return 0;
>> + }
>> +
>> + return plls[srcsel] / (divisor ? divisor : 1u);
>> +}
>> +
>> +#define ZYNQ_CLOCK(_state, _plls, _reg, _enable_field) \
>> + zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
>> + R_ ## _reg ## _ ## _enable_field ## _SHIFT)
>> +#define ZYNQ_GATE(_state, _clk, _reg, _field) \
>> + (ARRAY_FIELD_EX32((_state)->regs, _reg, _field) ? (_clk) : 0)
>> +
>> +static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
>> +{
>> + uint64_t ps_clk = INPUT_PS_REF_CLK_FREQUENCY;
>> + 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 cpu_mux[4] = {arm_pll, arm_pll, ddr_pll, io_pll};
>> + uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
>> + uint64_t cpu1x_clk;
>> +
>> + /* compute uartX amba clocks */
>> + cpu1x_clk = ZYNQ_CLOCK(s, cpu_mux, ARM_CLK_CTRL, CPU_PERI_CLKACT);
>> + cpu1x_clk = ZYNQ_GATE(s, cpu1x_clk, ARM_CLK_CTRL, CPU_1XCLKACT);
>> + clock_set_frequency(s->uart0_amba_clk,
>> + ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART0_CPU1XCLKACT));
>> + clock_set_frequency(s->uart1_amba_clk,
>> + ZYNQ_GATE(s, cpu1x_clk, APER_CLK_CTRL, UART1_CPU1XCLKACT));
>> +
>> + /* compute uartX ref clocks */
>> + clock_set_frequency(s->uart0_ref_clk,
>> + ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
>> + clock_set_frequency(s->uart1_ref_clk,
>> + ZYNQ_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
>> +}
>> +
>> static void zynq_slcr_reset(DeviceState *d)
>> {
>> ZynqSLCRState *s = ZYNQ_SLCR(d);
>> @@ -274,6 +379,8 @@ static void zynq_slcr_reset(DeviceState *d)
>> s->regs[R_DDRIOB + 4] = s->regs[R_DDRIOB + 5] = s->regs[R_DDRIOB + 6]
>> = 0x00000e00;
>> s->regs[R_DDRIOB + 12] = 0x00000021;
>> +
>> + zynq_slcr_compute_clocks(s);
>> }
>>
>>
>> @@ -408,6 +515,14 @@ 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_ARM_CLK_CTRL:
>> + case R_APER_CLK_CTRL:
>> + case R_UART_CLK_CTRL:
>> + zynq_slcr_compute_clocks(s);
>> + break;
>> }
>> }
>>
>> @@ -417,6 +532,14 @@ static const MemoryRegionOps slcr_ops = {
>> .endianness = DEVICE_NATIVE_ENDIAN,
>> };
>>
>> +static const ClockPortInitArray zynq_slcr_clocks = {
>> + QDEV_CLOCK_OUT(ZynqSLCRState, uart0_amba_clk),
>> + QDEV_CLOCK_OUT(ZynqSLCRState, uart1_amba_clk),
>> + 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);
>> @@ -424,6 +547,8 @@ static void zynq_slcr_init(Object *obj)
>> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
>> ZYNQ_SLCR_MMIO_SIZE);
>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +
>> + qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
>> }
>>
>> static const VMStateDescription vmstate_zynq_slcr = {
>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts
2018-10-12 13:24 ` Damien Hedde
@ 2018-10-12 13:27 ` Peter Maydell
0 siblings, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2018-10-12 13:27 UTC (permalink / raw)
To: Damien Hedde
Cc: Philippe Mathieu-Daudé,
QEMU Developers, Edgar Iglesias, Alistair Francis, Mark Burton,
Sai Pavan Boddu, qemu-arm, Paolo Bonzini, konrad, Luc Michel
On 12 October 2018 at 14:24, Damien Hedde <damien.hedde@greensocs.com> wrote:
> I'm wondering if in such a case (a fixed frequency base clock), we
> should have a way to change the frequency through command line ? (In
> order to start the machine with, for example, a 50MHz base clock).
> But I don't know what's the right way to do it and if it needs some kind
> of special support in the clock object or in the machine object, like a
> frequency property.
New command line arguments are a complexity you don't want to get in to.
Just have the board model the frequency at whatever it typically is.
It seems vanishingly rare that a user would really care to model the
board frequency as something different.
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support
2018-10-02 23:26 ` Philippe Mathieu-Daudé
@ 2018-10-12 13:42 ` Damien Hedde
0 siblings, 0 replies; 33+ messages in thread
From: Damien Hedde @ 2018-10-12 13:42 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: edgar.iglesias, peter.maydell, alistair, mark.burton, saipava,
qemu-arm, pbonzini, konrad, luc.michel
Hi Philippe,
On 10/3/18 1:26 AM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
>
> On 10/2/18 4:24 PM, Damien Hedde wrote:
>> Add bus interface and uart reference clock inputs.
>>
>> Note: it is hard to find out from the doc what is the behavior when only one
>> of the clock is disabled.
>>
>> The implemented behaviour is that register access needs both clock being active.
>>
>> The bus interface control the mmios visibility
>
> This sentence sounds odd.
Indeed, and also outdated.
I removed the part switching on/off the mmio visibility according to the
bus clock : accesses were silently ignored in that case, which would
probably have made some user mad at some point.
I replaced it by a check on every access and a LOG_GUEST_ERROR in case
the access is dropped.
>
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored. Also register accesses are
>> conditioned to the clock being enabled (but is it really the case in
>> reality ?) and a guest error is triggerred if that is not the case.
>>
>> If theses clocks remains unconnected, the uart behaves as before
>> (default to 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> include/hw/char/cadence_uart.h | 3 ++
>> hw/char/cadence_uart.c | 92 ++++++++++++++++++++++++++++++++--
>> hw/char/trace-events | 3 ++
>> 3 files changed, 93 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
>> index 118e3f10de..fd1d4725f4 100644
>> --- a/include/hw/char/cadence_uart.h
>> +++ b/include/hw/char/cadence_uart.h
>> @@ -21,6 +21,7 @@
>> #include "hw/sysbus.h"
>> #include "chardev/char-fe.h"
>> #include "qemu/timer.h"
>> +#include "hw/clock-port.h"
>>
>> #define CADENCE_UART_RX_FIFO_SIZE 16
>> #define CADENCE_UART_TX_FIFO_SIZE 16
>> @@ -47,6 +48,8 @@ typedef struct {
>> CharBackend chr;
>> qemu_irq irq;
>> QEMUTimer *fifo_trigger_handle;
>> + ClockIn *refclk;
>> + ClockIn *busclk;
>> } CadenceUARTState;
>>
>> static inline DeviceState *cadence_uart_create(hwaddr addr,
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index fbdbd463bb..feb5cee4d7 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -28,6 +28,7 @@
>> #include "qemu/timer.h"
>> #include "qemu/log.h"
>> #include "hw/char/cadence_uart.h"
>> +#include "trace.h"
>>
>> #ifdef CADENCE_UART_ERR_DEBUG
>> #define DB_PRINT(...) do { \
>> @@ -94,7 +95,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)
>> @@ -165,15 +166,30 @@ static void uart_send_breaks(CadenceUARTState *s)
>> &break_enabled);
>> }
>>
>> +static unsigned int uart_input_clk(CadenceUARTState *s)
>> +{
>> + return clock_get_frequency(s->refclk);
>> +}
>> +
>> static void uart_parameters_setup(CadenceUARTState *s)
>> {
>> QEMUSerialSetParams ssp;
>> unsigned int baud_rate, packet_size;
>>
>> baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> - UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> + uart_input_clk(s) / 8 : uart_input_clk(s);
>> + baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>
> Safe because s->r[R_BRGR] >= 1, OK.
>
>> + trace_cadence_uart_baudrate(baud_rate);
>> +
>> + ssp.speed = baud_rate;
>> + if (ssp.speed == 0) {
>> + /*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
>> + ssp.speed = 1;
>> + }
>>
>> - ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> packet_size = 1;
>>
>> switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -337,6 +353,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 */
>> + if (!clock_is_enabled(s->refclk)) {
>> + return;
>> + }
>> +
>> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
>> uart_write_rx_fifo(opaque, buf, size);
>> }
>> @@ -350,6 +371,11 @@ static void uart_event(void *opaque, int event)
>> CadenceUARTState *s = opaque;
>> uint8_t buf = '\0';
>>
>> + /* ignore events when unclocked */
>> + if (!clock_is_enabled(s->refclk)) {
>> + return;
>> + }
>> +
>> if (event == CHR_EVENT_BREAK) {
>> uart_write_rx_fifo(opaque, &buf, 1);
>> }
>> @@ -382,6 +408,14 @@ static void uart_write(void *opaque, hwaddr offset,
>> {
>> CadenceUARTState *s = opaque;
>>
>> + /* ignore accesses when bus or ref clock is disabled */
>> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "cadence_uart: Trying to write register 0x%x"
>> + " while clock is disabled\n", (unsigned) offset);
>> + return;
>> + }
>> +
>> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
>> offset >>= 2;
>> if (offset >= CADENCE_UART_R_MAX) {
>> @@ -440,6 +474,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
>> CadenceUARTState *s = opaque;
>> uint32_t c = 0;
>>
>> + /* ignore accesses when bus or ref clock is disabled */
>> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "cadence_uart: Trying to read register 0x%x"
>> + " while clock is disabled\n", (unsigned) offset);
>> + return 0;
>> + }
>> +
>> offset >>= 2;
>> if (offset >= CADENCE_UART_R_MAX) {
>> c = 0;
>> @@ -488,6 +530,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);
>> @@ -497,9 +547,26 @@ static void cadence_uart_init(Object *obj)
>> sysbus_init_mmio(sbd, &s->iomem);
>> sysbus_init_irq(sbd, &s->irq);
>>
>> + s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
>> + cadence_uart_refclk_update, s);
>> + /* initialize the frequency in case the clock remains unconnected */
>> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
>> + s->busclk = qdev_init_clock_in(DEVICE(obj), "busclk", NULL, NULL);
>> + /* initialize the frequency to non-zero in case it remains unconnected */
>> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
>
> #define INPUT_BUS_CLK_FREQUENCY (100 * 1000 * 1000)
>
>> +
>> s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
>> }
>>
>> +static int cadence_uart_pre_load(void *opaque)
>> +{
>> + CadenceUARTState *s = opaque;
>> +
>> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
>> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
>
> INPUT_BUS_CLK_FREQUENCY
>
>> + return 0;
>> +}
>> +
>> static int cadence_uart_post_load(void *opaque, int version_id)
>> {
>> CadenceUARTState *s = opaque;
>> @@ -516,10 +583,22 @@ static int cadence_uart_post_load(void *opaque, int version_id)
>> return 0;
>> }
>>
>> +static const VMStateDescription vmstate_cadence_uart_clocks = {
>> + .name = "cadence_uart_clocks",
>> + .version_id = 0,
>> + .minimum_version_id = 0,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_CLOCKIN(refclk, CadenceUARTState),
>> + VMSTATE_CLOCKIN(busclk, CadenceUARTState),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> static const VMStateDescription vmstate_cadence_uart = {
>> .name = "cadence_uart",
>> .version_id = 2,
>> .minimum_version_id = 2,
>> + .pre_load = cadence_uart_pre_load,
>> .post_load = cadence_uart_post_load,
>> .fields = (VMStateField[]) {
>> VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
>> @@ -532,7 +611,10 @@ static const VMStateDescription vmstate_cadence_uart = {
>> VMSTATE_UINT32(rx_wpos, CadenceUARTState),
>> VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
>> VMSTATE_END_OF_LIST()
>> - }
>> + },
>> + .subsections = (const VMStateDescription * []) {
>> + &vmstate_cadence_uart_clocks,
>> + },
>> };
>>
>> static Property cadence_uart_properties[] = {
>> @@ -548,7 +630,7 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
>> dc->vmsd = &vmstate_cadence_uart;
>> dc->reset = cadence_uart_reset;
>> dc->props = cadence_uart_properties;
>> - }
>> +}
>>
>> static const TypeInfo cadence_uart_info = {
>> .name = TYPE_CADENCE_UART,
>> diff --git a/hw/char/trace-events b/hw/char/trace-events
>> index b64213d4dd..2ea25d1ea1 100644
>> --- a/hw/char/trace-events
>> +++ b/hw/char/trace-events
>> @@ -73,3 +73,6 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got character 0x%x from backe
>> cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend pending"
>> cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to backend"
>> cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
>> +
>> +# hw/char/cadence_uart.c
>> +cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Except migration:
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
` (9 preceding siblings ...)
2018-10-04 16:13 ` [Qemu-devel] [PATCH v5 0/9] Clock framework API Philippe Mathieu-Daudé
@ 2018-10-12 15:26 ` Damien Hedde
2018-10-16 15:48 ` Peter Maydell
10 siblings, 1 reply; 33+ messages in thread
From: Damien Hedde @ 2018-10-12 15:26 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: qemu-arm, pbonzini, alistair, saipava, mark.burton, luc.michel,
konrad, edgar.iglesias
Hi Peter,
Sorry to bother you with this, but you said some time ago you would
write something about reset.
On 10/2/18 4:24 PM, Damien Hedde wrote:
> There is also the problem of initialization which is very much like the
> migration. Currently, in the zynq example, clocks outputs are initialized in
> the clock controller device_reset. According to Peter, outputs should not be
> set in device_reset, so we need a better way.
To illustrate the problem, lets take an example with the zynq case in
the patchset. We have a 2-stage clock tree:
+--------+ +----------------+ +------+
| Machine|>>base_clk>>|Clock controller|>>uart_clock>>| UART |
+--------+ +----------------+ +------+
At the end of the tree, the uart uses the clock to setup the backend
baudrate. The problem is that we need the whole clock path initialized
before having the right final frequency.
I've had some thought about it and I see several solutions
1. Set clock outputs in device_reset. But It can trigger side effects in
yet-non-reseted devices.
2. Have some kind of global 2nd stage reset to do all the output
propagation.
3. Try to propagate init values at platform building when doing the
clock connection.
4. (1) and (2) mixed. Have a per device 2nd stage reset "clock_reset"
(or any better name) method called when device_reset has been called and
all clock inputs have been initialized (by other device "clock_reset").
At the end of reset phase everything should have been propagated.
(1) being not-an-option and also (2) I think.
(3) seems complicated at best due to the unknown clock connection order.
And I'm not sure clock connection is the right moment to do this. In the
general case, a clock init value can depend on on some user-set/config
properties which will be applied later on I suppose (But I don't have a
clue at which point this operation -- the "realize" thing -- cab be done
in the platform startup sequence)
Do you think (4) is sensible ? It means any device requiring clock input
value will need to implement this new method. Basically this
"clock_reset" is just a part of the actual device_reset being delayed.
Obviously if there is a clock loop, the method will never be called.
--
Damien
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-12 15:26 ` Damien Hedde
@ 2018-10-16 15:48 ` Peter Maydell
2018-12-18 15:24 ` Damien Hedde
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2018-10-16 15:48 UTC (permalink / raw)
To: Damien Hedde
Cc: QEMU Developers, qemu-arm, Paolo Bonzini, Alistair Francis,
Sai Pavan Boddu, Mark Burton, Luc Michel, konrad, Edgar Iglesias
On 12 October 2018 at 16:26, Damien Hedde <damien.hedde@greensocs.com> wrote:
> Hi Peter,
>
> Sorry to bother you with this, but you said some time ago you would
> write something about reset.
Yeah, sorry about that. I haven't found (made) the time to think
the issues through yet. Let me just dump to email the notes I had.
This is a very "raw" set of notes, so it's not very organised and
has a lot of 'todo' bits in it...
Overall question -- does anybody know of other device modelling
systems that do a good job of modelling reset, and how they do it?
If we can borrow a proven design that might be better tha
thrashing one out ourseles...
My idea was roughly that we need a multi-phase reset, something like:
PHASE 1:
Devices reset internal state. No calls out to other devices
(ie no setting IRQ line state, etc)
Some common bit of code (dev base class?) marks the device
reset as being in-progress by setting dev->resetting = true.
PHASE 2:
Devices update their output lines (calls to irq line setting
functions, clock setting, etc)
In this phase devices may be the recipients of callbacks for
IRQ line level changes, etc, and must cope with this. A
callback can tell it's being called for a level change during
reset because dev->resetting will be true.
(They can update their own outputs in response to input changes.)
PHASE 3:
Common code (dev base class) sets dev->resetting = false.
(Individual devices don't get to do anything here, unless
there's some painful case that requires it.)
The theory is that by decoupling reset into multiple phases
we can avoid most of the reset order issues, by having the
two ends do things in the right phase and/or by having
the destination end be able to identify when a change is
happening in the middle of reset.
Compatibility with existing device reset methods:
essentially the current rules are the same as the "phase 1"
rules, so we can just say that the current reset code in
all devices is phase 1 reset. Then we can adjust anything
that needs a phase 2 (which is probably not all that many
devices).
Places where reset order currently matters:
(this is just a list of things we should make sure our design
copes with)
* wired-high IRQ lines, and device outputs that are asserted
as the device comes out of reset
* setting PC in the generic-loader device vs CPU reset
* image load (rom image reset) vs Arm M-profile reset wanting
to read the reset PC/SP from memory
* Arm M-profile CPUs should have INITVTOR as a config signal input
* x86 CPU vs APIC reset order ??
* anything else?
We should have some sort of "reset domain" object which can
contain devices and also other reset domains. A reset domain's
reset is simple: its phase 1 reset is "for each child, do
phase 1 reset for that child", similarly for phases 2 and 3.
A reset domain has an overall "reset" method that says
"do phase 1 for all children, then phase 2, then phase 3".
This allows things like "this SoC has a reset line which
resets all its internal devices". (Do we want to give
reset domains input gpio lines to trigger reset, or is
being able to call a method on it sufficient?)
At the moment we have reset methods on devices:
* each bus handles reset for the things on the bus
* the (one and only) sysbus resets all the sysbus devices
* devices on no bus don't get reset at all unless something
takes special measures! (notably the CPUs are reset in
a very ad-hoc way at the moment)
I guess this should be cleaned up by making each bus be
a 'reset domain', and having a "system" reset domain which gets
anything which doesn't have a parent reset domain (and which
we then use for whole-board power-on reset). Calling the
system reset domain's overall reset method will then do
the 3-phase reset for everything.
How would direct qemu_register_reset() calls fit into this
scheme? (Some of them are devices that could get QOM reset
methods instead. Perhaps we could get away with dumping the
rest into the 'master reset' domain, or leaving them as-is?
Some are for CPU reset which we definitely should clean up.)
We need to handle different types of reset somehow, in
particular "cold/power on reset" versus "warm reset".
Can we do this just with reset domain objects, or do we
need/want to pass each device's reset method a "type" argument?
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/9] Clock framework API.
2018-10-16 15:48 ` Peter Maydell
@ 2018-12-18 15:24 ` Damien Hedde
0 siblings, 0 replies; 33+ messages in thread
From: Damien Hedde @ 2018-12-18 15:24 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, qemu-arm, Paolo Bonzini, Alistair Francis,
Sai Pavan Boddu, Mark Burton, Luc Michel, Edgar Iglesias
On 10/16/18 5:48 PM, Peter Maydell wrote:
> On 12 October 2018 at 16:26, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> Hi Peter,
>>
>> Sorry to bother you with this, but you said some time ago you would
>> write something about reset.
>
> Yeah, sorry about that. I haven't found (made) the time to think
> the issues through yet. Let me just dump to email the notes I had.
> This is a very "raw" set of notes, so it's not very organised and
> has a lot of 'todo' bits in it...
>
>
> Overall question -- does anybody know of other device modelling
> systems that do a good job of modelling reset, and how they do it?
> If we can borrow a proven design that might be better tha
> thrashing one out ourseles...
>
>
> My idea was roughly that we need a multi-phase reset, something like:
>
> PHASE 1:
> Devices reset internal state. No calls out to other devices
> (ie no setting IRQ line state, etc)
> Some common bit of code (dev base class?) marks the device
> reset as being in-progress by setting dev->resetting = true.
>
> PHASE 2:
> Devices update their output lines (calls to irq line setting
> functions, clock setting, etc)
> In this phase devices may be the recipients of callbacks for
> IRQ line level changes, etc, and must cope with this. A
> callback can tell it's being called for a level change during
> reset because dev->resetting will be true.
> (They can update their own outputs in response to input changes.)
>
> PHASE 3:
> Common code (dev base class) sets dev->resetting = false.
> (Individual devices don't get to do anything here, unless
> there's some painful case that requires it.)
>
> The theory is that by decoupling reset into multiple phases
> we can avoid most of the reset order issues, by having the
> two ends do things in the right phase and/or by having
> the destination end be able to identify when a change is
> happening in the middle of reset.
This strategy seems fine to me.
>
> Compatibility with existing device reset methods:
> essentially the current rules are the same as the "phase 1"
> rules, so we can just say that the current reset code in
> all devices is phase 1 reset. Then we can adjust anything
> that needs a phase 2 (which is probably not all that many
> devices).
>
> Places where reset order currently matters:
> (this is just a list of things we should make sure our design
> copes with)
> * wired-high IRQ lines, and device outputs that are asserted
> as the device comes out of reset
> * setting PC in the generic-loader device vs CPU reset
> * image load (rom image reset) vs Arm M-profile reset wanting
> to read the reset PC/SP from memory
> * Arm M-profile CPUs should have INITVTOR as a config signal input
> * x86 CPU vs APIC reset order ??
> * anything else?
>
> We should have some sort of "reset domain" object which can
> contain devices and also other reset domains. A reset domain's
> reset is simple: its phase 1 reset is "for each child, do
> phase 1 reset for that child", similarly for phases 2 and 3.
> A reset domain has an overall "reset" method that says
> "do phase 1 for all children, then phase 2, then phase 3".
> This allows things like "this SoC has a reset line which
> resets all its internal devices". (Do we want to give
> reset domains input gpio lines to trigger reset, or is
> being able to call a method on it sufficient?)
Consider the following use case: a platform with a controller that holds
a reset line on another device (starting with reset held).
At the beginning, the main (cold) reset will be triggered to start the
platform. During phase 2, the controller will set the reset line of the
other device (a gpio). Then we have phase 3. How do we handle this ?
Do we need to "delay" the phase 3 of the device until the controller
releases its reset line or do we consider the internal gpio reset line
to be outside of this reset scope ?
If we do that, we have 2 reset "sources": the main reset and the
internal gpio and there is some kind of OR operation to do between these
2 sources.
>
> At the moment we have reset methods on devices:
> * each bus handles reset for the things on the bus
> * the (one and only) sysbus resets all the sysbus devices
> * devices on no bus don't get reset at all unless something
> takes special measures! (notably the CPUs are reset in
> a very ad-hoc way at the moment)
> I guess this should be cleaned up by making each bus be
> a 'reset domain', and having a "system" reset domain which gets
> anything which doesn't have a parent reset domain (and which
> we then use for whole-board power-on reset). Calling the
> system reset domain's overall reset method will then do
> the 3-phase reset for everything>
> How would direct qemu_register_reset() calls fit into this
> scheme? (Some of them are devices that could get QOM reset
> methods instead. Perhaps we could get away with dumping the
> rest into the 'master reset' domain, or leaving them as-is?
> Some are for CPU reset which we definitely should clean up.)
>
> We need to handle different types of reset somehow, in
> particular "cold/power on reset" versus "warm reset".
> Can we do this just with reset domain objects, or do we
> need/want to pass each device's reset method a "type" argument?
Maybe phase 1 only needs the type, phase 2 and 3 can only depends on the
device state.
Anyway, this reset problem seems outside of the scope of this patch set.
I think I'll do a reroll with the remark I've got so far, but it will
still miss the reset part to be well integrated. I can also propose
something in another rfc about the 3-phase reset to advance on that part.
--
Damien
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-12-18 15:25 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-02 14:24 [Qemu-devel] [PATCH v5 0/9] Clock framework API Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects Damien Hedde
2018-10-02 23:53 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 2/9] qdev: add clock input&output support to devices Damien Hedde
2018-10-02 23:36 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
2018-10-02 22:42 ` Philippe Mathieu-Daudé
2018-10-12 10:20 ` Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 4/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
2018-10-03 8:23 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation Damien Hedde
2018-10-02 23:48 ` Philippe Mathieu-Daudé
2018-10-03 8:18 ` Philippe Mathieu-Daudé
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 6/9] hw/misc/zynq_slcr: use standard register definition Damien Hedde
2018-10-04 17:24 ` Alistair Francis
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
2018-10-02 23:10 ` Philippe Mathieu-Daudé
2018-10-12 13:24 ` Damien Hedde
2018-10-12 13:27 ` Peter Maydell
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support Damien Hedde
2018-10-02 23:26 ` Philippe Mathieu-Daudé
2018-10-12 13:42 ` Damien Hedde
2018-10-02 14:24 ` [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
2018-10-02 23:28 ` Philippe Mathieu-Daudé
2018-10-04 16:13 ` [Qemu-devel] [PATCH v5 0/9] Clock framework API Philippe Mathieu-Daudé
2018-10-11 16:20 ` Damien Hedde
2018-10-11 16:23 ` Peter Maydell
2018-10-11 17:00 ` Philippe Mathieu-Daudé
2018-10-11 17:12 ` Peter Maydell
2018-10-11 17:16 ` Philippe Mathieu-Daudé
2018-10-12 15:26 ` Damien Hedde
2018-10-16 15:48 ` Peter Maydell
2018-12-18 15:24 ` Damien Hedde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.