| Message ID | 20180917084016.12750-2-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clock framework API. | expand |
On 17 September 2018 at 09:40, <damien.hedde@greensocs.com> wrote: > From: Damien Hedde <damien.hedde@greensocs.com> > > Introduce clock port objects: ClockIn and ClockOut. > > Theses ports may be used to distribute a clock from a object to several > other objects. They do not contain any state and serve only as intermediate > to carry a ClockState which contains 2 fields: > * an integer which represent the frequency in Hertz > * a boolean which represent the reset status of the clock domain > Both are independent: eg the reset can be asserted while the clock running and > vice versa. > > 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 | 153 ++++++++++++++++++++++++++++++++++++++++ > hw/core/clock-port.c | 145 +++++++++++++++++++++++++++++++++++++ > hw/core/Makefile.objs | 1 + > hw/core/trace-events | 6 ++ > 5 files changed, 306 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..2ab554c30e > --- /dev/null > +++ b/include/hw/clock-port.h > @@ -0,0 +1,153 @@ > +#ifndef CLOCK_PORT_H > +#define CLOCK_PORT_H > + > +#include "qom/object.h" > +#include "hw/qdev-core.h" > +#include "qemu/queue.h" > + > +#define TYPE_CLOCK_PORT "clock-port" > +#define CLOCK_PORT(obj) OBJECT_CHECK(ClockPort, (obj), TYPE_CLOCK_PORT) > +#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 struct ClockState { > + uint64_t frequency; /* frequency of the clock in Hz */ > + bool domain_reset; /* flag indicating if clock domain is under reset */ > +} ClockState; > + > +typedef void ClockCallback(void *opaque, ClockState *clk); > + > +typedef struct ClockPort { > + /*< private >*/ > + Object parent_obj; > + /*< public >*/ > + char *canonical_path; /* clock path shadow */ > +} ClockPort; Having a ClockPort which ClockIn and ClockOut are derived from seems a bit heavyweight. When would you want to operate on a generic ClockPort rather than either a ClockIn or ClockOut ? Why would you want the name (canonical_path) to be different at the input and output ends -- is it possible to simplify to just having the output end keep the name string ? > --- /dev/null > +++ b/hw/core/trace-events > @@ -0,0 +1,6 @@ > +# 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'" "driven by" > +clock_disconnect(const char *clk) "'%s'" > +clock_update(const char *clk, uint64_t freq, int reset) "'%s' frequency %" PRIu64 "Hz reset %d" > -- > 2.18.0 > thanks -- PMM
On 9/25/18 11:42 AM, Peter Maydell wrote: > On 17 September 2018 at 09:40, <damien.hedde@greensocs.com> wrote: >> From: Damien Hedde <damien.hedde@greensocs.com> >> >> Introduce clock port objects: ClockIn and ClockOut. >> >> Theses ports may be used to distribute a clock from a object to several >> other objects. They do not contain any state and serve only as intermediate >> to carry a ClockState which contains 2 fields: >> * an integer which represent the frequency in Hertz >> * a boolean which represent the reset status of the clock domain >> Both are independent: eg the reset can be asserted while the clock running and >> vice versa. >> >> 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 | 153 ++++++++++++++++++++++++++++++++++++++++ >> hw/core/clock-port.c | 145 +++++++++++++++++++++++++++++++++++++ >> hw/core/Makefile.objs | 1 + >> hw/core/trace-events | 6 ++ >> 5 files changed, 306 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..2ab554c30e >> --- /dev/null >> +++ b/include/hw/clock-port.h >> @@ -0,0 +1,153 @@ >> +#ifndef CLOCK_PORT_H >> +#define CLOCK_PORT_H >> + >> +#include "qom/object.h" >> +#include "hw/qdev-core.h" >> +#include "qemu/queue.h" >> + >> +#define TYPE_CLOCK_PORT "clock-port" >> +#define CLOCK_PORT(obj) OBJECT_CHECK(ClockPort, (obj), TYPE_CLOCK_PORT) >> +#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 struct ClockState { >> + uint64_t frequency; /* frequency of the clock in Hz */ >> + bool domain_reset; /* flag indicating if clock domain is under reset */ >> +} ClockState; >> + >> +typedef void ClockCallback(void *opaque, ClockState *clk); >> + >> +typedef struct ClockPort { >> + /*< private >*/ >> + Object parent_obj; >> + /*< public >*/ >> + char *canonical_path; /* clock path shadow */ >> +} ClockPort; > > Having a ClockPort which ClockIn and ClockOut are derived > from seems a bit heavyweight. When would you want to operate on > a generic ClockPort rather than either a ClockIn or ClockOut ? > Why would you want the name (canonical_path) to be different > at the input and output ends -- is it possible to simplify > to just having the output end keep the name string ? Here the canonical path is the full qom's canonical path. It is cached in the state to avoid recomputing it when needed. It is only used for log/trace purpose (without it, log is useless and calling object_get_canonical_path at every log seemed too costly to me). A connected ClockOut and ClockIn do not have the same (qom) parent, their canonical path is different. For example, in the zynq platform, there are 2 uarts, each one having a "refclk" input. There is also a clock controller (slcr) having 2 outputs "uart0_ref_clk" and "uart1_ref_clk". We end up with something like this concerning canonical path: + output "[...]/slcr/uart0_ref_clk" drives input "[...]/uart0/refclk" + output "[...]/slcr/uart1_ref_clk" drives input "[...]/uart1/refclk" Right now, I log a line for every end (output and input) when the clock frequency change, which lead to a kind-of-duplicate log line (names differ, but clock frequencies are obviously the same). I could only log at the output end and drop the canonical path for the input or pust the path in both objects. Anyway it is easily possible to drop the ClockPort common ancestor. A user never operates on it. > >> --- /dev/null >> +++ b/hw/core/trace-events >> @@ -0,0 +1,6 @@ >> +# 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'" > > "driven by" > >> +clock_disconnect(const char *clk) "'%s'" >> +clock_update(const char *clk, uint64_t freq, int reset) "'%s' frequency %" PRIu64 "Hz reset %d" >> -- >> 2.18.0 >> > > thanks > -- PMM >
On 25 September 2018 at 14:59, Damien Hedde <damien.hedde@greensocs.com> wrote: > > > On 9/25/18 11:42 AM, Peter Maydell wrote: >> Having a ClockPort which ClockIn and ClockOut are derived >> from seems a bit heavyweight. When would you want to operate on >> a generic ClockPort rather than either a ClockIn or ClockOut ? >> Why would you want the name (canonical_path) to be different >> at the input and output ends -- is it possible to simplify >> to just having the output end keep the name string ? > > Here the canonical path is the full qom's canonical path. It is cached > in the state to avoid recomputing it when needed. It is only used for > log/trace purpose (without it, log is useless and calling > object_get_canonical_path at every log seemed too costly to me). > A connected ClockOut and ClockIn do not have the same (qom) parent, > their canonical path is different. > For example, in the zynq platform, there are 2 uarts, each one having a > "refclk" input. There is also a clock controller (slcr) having 2 > outputs "uart0_ref_clk" and "uart1_ref_clk". We end up with something > like this concerning canonical path: > + output "[...]/slcr/uart0_ref_clk" drives input "[...]/uart0/refclk" > + output "[...]/slcr/uart1_ref_clk" drives input "[...]/uart1/refclk" > > Right now, I log a line for every end (output and input) when the clock > frequency change, which lead to a kind-of-duplicate log line (names > differ, but clock frequencies are obviously the same). I could only log > at the output end and drop the canonical path for the input or pust the > path in both objects. > > Anyway it is easily possible to drop the ClockPort common ancestor. A > user never operates on it. I think that would be preferable -- having the debug logging drive the object hierarchy setup seems like the tail wagging the dog to me. thanks -- PMM
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..2ab554c30e --- /dev/null +++ b/include/hw/clock-port.h @@ -0,0 +1,153 @@ +#ifndef CLOCK_PORT_H +#define CLOCK_PORT_H + +#include "qom/object.h" +#include "hw/qdev-core.h" +#include "qemu/queue.h" + +#define TYPE_CLOCK_PORT "clock-port" +#define CLOCK_PORT(obj) OBJECT_CHECK(ClockPort, (obj), TYPE_CLOCK_PORT) +#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 struct ClockState { + uint64_t frequency; /* frequency of the clock in Hz */ + bool domain_reset; /* flag indicating if clock domain is under reset */ +} ClockState; + +typedef void ClockCallback(void *opaque, ClockState *clk); + +typedef struct ClockPort { + /*< private >*/ + Object parent_obj; + /*< public >*/ + char *canonical_path; /* clock path shadow */ +} ClockPort; + +typedef struct ClockOut ClockOut; +typedef struct ClockIn ClockIn; + +struct ClockIn { + /*< private >*/ + ClockPort parent_obj; + /*< private >*/ + 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 >*/ + ClockPort parent_obj; + /*< private >*/ + QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */ +}; + +/* + * clock_setup_canonical_path: + * @clk: clock + * + * compute the canonical path of the clock (used by log messages) + */ +void clock_setup_canonical_path(ClockPort *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_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: + * @clk: the clock to update. + * @state: the new clock's state. + * + * Update the @clk to the new @state. + * This change will be propagated through registered clock inputs. + */ +void clock_set(ClockOut *clk, ClockState *state); + +/** + * clock_state_get_frequency: + * @clk: a clock state + * + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0. + */ +static inline uint64_t clock_state_get_frequency(const ClockState *clk) +{ + return clk ? clk->frequency : 0; +} + +/** + * clock_state_is_enabled: + * @clk: a clock state + * + * @return: true if the clock is running. If @clk is NULL return false. + */ +static inline bool clock_state_is_enabled(const ClockState *clk) +{ + return clock_state_get_frequency(clk) != 0; +} + +/** + * clock_state_get_domain_reset: + * @clk: a clock state + * + * @return: true if the domain reset is asserted. If @clk is NULL return false. + */ +static inline bool clock_state_get_domain_reset(const ClockState *clk) +{ + return clk ? clk->domain_reset : false; +} + +/** + * clock_state_is_domain_running: + * @clk: a clock state + * + * @return: true if the clock is running and reset not asserted. If @clk is + * NULL return false. + */ +static inline bool clock_state_is_domain_running(const ClockState *clk) +{ + return clock_state_is_enabled(clk) && !clock_state_get_domain_reset(clk); +} + +/** + * clock_state_copy: + * @dst the clock state destination + * @src: the clock state source + * + * Transfer the source clock state into the destination. + */ +static inline void clock_state_copy(ClockState *dst, const ClockState *src) +{ + dst->frequency = clock_state_get_frequency(src); + dst->domain_reset = clock_state_get_domain_reset(src); +} + +#endif /* CLOCK_PORT_H */ diff --git a/hw/core/clock-port.c b/hw/core/clock-port.c new file mode 100644 index 0000000000..31c344328c --- /dev/null +++ b/hw/core/clock-port.c @@ -0,0 +1,145 @@ +/* + * 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" + +#define CLOCK_PATH(_clk) (_clk->parent_obj.canonical_path) + +void clock_setup_canonical_path(ClockPort *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_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(ClockOut *clk, ClockState *state) +{ + ClockIn *follower; + trace_clock_update(CLOCK_PATH(clk), state->frequency, state->domain_reset); + + QLIST_FOREACH(follower, &clk->followers, sibling) { + trace_clock_update(CLOCK_PATH(follower), state->frequency, + state->domain_reset); + if (follower->callback) { + follower->callback(follower->callback_opaque, state); + } + } +} + +static void clock_port_finalizefn(Object *obj) +{ + ClockPort *clk = CLOCK_PORT(obj); + + g_free(clk->canonical_path); + clk->canonical_path = NULL; +} + +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); + } +} + +static void clock_in_finalizefn(Object *obj) +{ + ClockIn *clk = CLOCK_IN(obj); + + /* remove us from driver's followers list */ + clock_disconnect(clk); +} + +static const TypeInfo clock_port_info = { + .name = TYPE_CLOCK_PORT, + .parent = TYPE_OBJECT, + .abstract = true, + .instance_size = sizeof(ClockPort), + .instance_finalize = clock_port_finalizefn, +}; + +static const TypeInfo clock_out_info = { + .name = TYPE_CLOCK_OUT, + .parent = TYPE_CLOCK_PORT, + .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_CLOCK_PORT, + .instance_size = sizeof(ClockIn), + .instance_finalize = clock_in_finalizefn, +}; + +static void clock_register_types(void) +{ + type_register_static(&clock_port_info); + 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..a37fd78492 --- /dev/null +++ b/hw/core/trace-events @@ -0,0 +1,6 @@ +# 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, int reset) "'%s' frequency %" PRIu64 "Hz reset %d"