| Message ID | 20190904125531.27545-4-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clock framework API | expand |
Nitpick: remove trailing dot in patch subject On 9/4/19 2:55 PM, Damien Hedde wrote: > Add functions to easily add input or output clocks to a device. > A clock objects is added as a child of the device. <newline>? > The api is very similar the gpio's one. Maybe "This API is very similar to the QDEV GPIO API." > > This is based on the original work of Frederic Konrad. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > > --- > I've removed the reviewed-by/tested-by of Philippe because I did a small > modification. > > qdev_connect_clock() which allowed to connect an input to an output is > now split in 2: > + qdev_get_clock_in() which gets a given input from a device > + qdev_connect_clock_out() which connect a given output to a clock > (previously fetched by qdev_get_clock_in()) > This part is located in (qdev-clock.[c|h]). > It better matches gpios api and also add the possibility to connect a > device's input clock to a random output clock (used in patch 9). > > Also add missing qdev-clock in the test-qdev-global-props so that tests > pass. > --- > hw/core/Makefile.objs | 2 +- > hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++ > hw/core/qdev.c | 32 +++++++++ > include/hw/qdev-clock.h | 67 +++++++++++++++++ > include/hw/qdev-core.h | 14 ++++ > tests/Makefile.include | 1 + Please setup the scripts/git.orderfile to ease reviews. > 6 files changed, 270 insertions(+), 1 deletion(-) > create mode 100644 hw/core/qdev-clock.c > create mode 100644 include/hw/qdev-clock.h > > diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs > index 8fcebf2e67..4523d3b5c7 100644 > --- a/hw/core/Makefile.objs > +++ b/hw/core/Makefile.objs > @@ -1,5 +1,5 @@ > # core qdev-related obj files, also used by *-user: > -common-obj-y += qdev.o qdev-properties.o > +common-obj-y += qdev.o qdev-properties.o qdev-clock.o > common-obj-y += bus.o reset.o > common-obj-y += resettable.o > common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o > diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c > new file mode 100644 > index 0000000000..bebdd8fa15 > --- /dev/null > +++ b/hw/core/qdev-clock.c > @@ -0,0 +1,155 @@ > +/* > + * Device's clock > + * > + * Copyright GreenSocs 2016-2018 2019 > + * > + * Authors: > + * Frederic Konrad <fred.konrad@greensocs.com> > + * Damien Hedde <damien.hedde@greensocs.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > +#include "hw/qdev-clock.h" > +#include "hw/qdev-core.h" > +#include "qapi/error.h" > + > +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, > + bool forward) Indentation off. > +{ > + NamedClockList *ncl; > + > + /* > + * The clock path will be computed by the device's realize function call. > + * This is required to ensure the clock's canonical path is right and log > + * messages are meaningfull. > + */ > + assert(name); > + assert(!dev->realized); > + > + /* The ncl structure will be freed in device's finalize function call */ > + ncl = g_malloc0(sizeof(*ncl)); Similar but easier to review: ncl = g_new0(NamedClockList, 1) > + ncl->name = g_strdup(name); > + ncl->forward = forward; > + > + QLIST_INSERT_HEAD(&dev->clocks, ncl, node); > + return ncl; > +} > + > +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + Object *clk; > + > + ncl = qdev_init_clocklist(dev, name, false); > + > + clk = object_new(TYPE_CLOCK_OUT); > + > + /* will fail if name already exists */ > + object_property_add_child(OBJECT(dev), name, clk, &error_abort); > + object_unref(clk); /* remove the initial ref made by object_new */ > + > + ncl->out = CLOCK_OUT(clk); > + return ncl->out; > +} > + > +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, > + ClockCallback *callback, void *opaque) Indentation off. > +{ > + NamedClockList *ncl; > + Object *clk; > + > + ncl = qdev_init_clocklist(dev, name, false); > + > + clk = object_new(TYPE_CLOCK_IN); > + /* > + * the ref initialized by object_new will be cleared during dev finalize. > + * It allows us to safely remove the callback. > + */ > + > + /* will fail if name already exists */ > + object_property_add_child(OBJECT(dev), name, clk, &error_abort); > + > + ncl->in = CLOCK_IN(clk); > + if (callback) { > + clock_set_callback(ncl->in, callback, opaque); > + } > + return ncl->in; > +} > + > +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + > + QLIST_FOREACH(ncl, &dev->clocks, node) { > + if (strcmp(name, ncl->name) == 0) { > + return ncl; > + } > + } > + > + return NULL; > +} > + > +void qdev_pass_clock(DeviceState *dev, const char *name, > + DeviceState *container, const char *cont_name) > +{ > + NamedClockList *original_ncl, *ncl; > + Object **clk; Is it really a Object** or a Object*? > + > + assert(container && cont_name); > + > + original_ncl = qdev_get_clocklist(container, cont_name); > + assert(original_ncl); /* clock must exist in origin */ > + > + ncl = qdev_init_clocklist(dev, name, true); > + > + if (ncl->out) { > + clk = (Object **)&ncl->out; > + } else { > + clk = (Object **)&ncl->in; > + } > + > + /* will fail if name already exists */ > + object_property_add_link(OBJECT(dev), name, object_get_typename(*clk), > + clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort); > +} > + > +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + > + assert(dev && name); > + > + ncl = qdev_get_clocklist(dev, name); > + return ncl ? ncl->in : NULL; > +} > + > +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + > + assert(dev && name); > + > + ncl = qdev_get_clocklist(dev, name); > + return ncl ? ncl->out : NULL; > +} > + > +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, > + Error **errp) > +{ > + ClockOut *clkout = qdev_get_clock_out(dev, name); > + > + if (!clk) { > + error_setg(errp, "NULL input clock"); > + return; > + } > + > + if (!clkout) { > + error_setg(errp, "no output clock '%s' in device", name); > + return; > + } > + > + clock_connect(clk, clkout); > +} > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 9095f2b9c1..eae6cd3e09 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -37,6 +37,7 @@ > #include "hw/qdev-properties.h" > #include "hw/boards.h" > #include "hw/sysbus.h" > +#include "hw/clock.h" > #include "migration/vmstate.h" > > bool qdev_hotplug = false; > @@ -821,6 +822,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > DeviceClass *dc = DEVICE_GET_CLASS(dev); > HotplugHandler *hotplug_ctrl; > BusState *bus; > + NamedClockList *clk; > Error *local_err = NULL; > bool unattached_parent = false; > static int unattached_count; > @@ -869,6 +871,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > */ > g_free(dev->canonical_path); > dev->canonical_path = object_get_canonical_path(OBJECT(dev)); > + QLIST_FOREACH(clk, &dev->clocks, node) { > + if (clk->forward) { > + continue; > + } else if (clk->in != NULL) { > + clock_in_setup_canonical_path(clk->in); > + } else { > + clock_out_setup_canonical_path(clk->out); > + } > + } > > if (qdev_get_vmsd(dev)) { > if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, > @@ -999,6 +1010,7 @@ static void device_initfn(Object *obj) > (Object **)&dev->parent_bus, NULL, 0, > &error_abort); > QLIST_INIT(&dev->gpios); > + QLIST_INIT(&dev->clocks); > } > > static void device_post_init(Object *obj) > @@ -1015,6 +1027,7 @@ static void device_post_init(Object *obj) > static void device_finalize(Object *obj) > { > NamedGPIOList *ngl, *next; > + NamedClockList *clk, *clk_next; > > DeviceState *dev = DEVICE(obj); > > @@ -1028,6 +1041,25 @@ static void device_finalize(Object *obj) > */ > } > > + QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) { > + QLIST_REMOVE(clk, node); > + if (!clk->forward && clk->in) { > + /* > + * if this clock is not forwarded, clk->in is a child of dev. > + * At this point the child property and associated reference is > + * already deleted but we kept a ref on clk->in to ensure it lives > + * up to this point and we can safely remove the callback. > + * It avoids having a lost callback to a deleted device if the > + * clk->in is still referenced somewhere else (eg: by a clock > + * output). > + */ > + clock_clear_callback(clk->in); > + object_unref(OBJECT(clk->in)); > + } > + g_free(clk->name); > + g_free(clk); > + } > + > /* Only send event if the device had been completely realized */ > if (dev->pending_deleted_event) { > g_assert(dev->canonical_path); > diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h > new file mode 100644 > index 0000000000..c4ea912fdc > --- /dev/null > +++ b/include/hw/qdev-clock.h > @@ -0,0 +1,67 @@ > +#ifndef QDEV_CLOCK_H > +#define QDEV_CLOCK_H > + > +#include "hw/clock.h" > + > +/** > + * qdev_init_clock_in: > + * @dev: the device in which to add a clock > + * @name: the name of the clock (can't be NULL). I'd drop the trailing dot in property descriptions. > + * @callback: optional callback to be called on update or NULL. > + * @opaque: argument for the callback > + * @returns: a pointer to the newly added clock > + * > + * Add a input clock to device @dev as a clock named @name. > + * This adds a child<> property. > + * The callback will be called with @dev as opaque parameter. > + */ > +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, > + ClockCallback *callback, void *opaque); > + > +/** > + * qdev_init_clock_out: > + * @dev: the device to add a clock to > + * @name: the name of the clock (can't be NULL). > + * @callback: optional callback to be called on update or NULL. > + * @returns: a pointer to the newly added clock > + * > + * Add a output clock to device @dev as a clock named @name. > + * This adds a child<> property. > + */ > +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name); > + > +/** > + * qdev_get_clock_in: > + * @dev: the device which has the clock > + * @name: the name of the clock (can't be NULL). > + * @returns: a pointer to the clock > + * > + * Get the clock @name from @dev or NULL if does not exists. > + */ > +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name); > + > +/** > + * qdev_connect_clock_out: > + * @dev: the device which has the clock > + * @name: the name of the clock (can't be NULL). > + * @errp: error report > + * > + * Connect @clk to the output clock @name of @dev. > + * Reports an error if clk is NULL or @name does not exists in @dev. > + */ > +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, > + Error **errp); > + > +/** > + * qdev_pass_clock: > + * @dev: the device to forward the clock to > + * @name: the name of the clock to be added (can't be NULL) > + * @container: the device which already has the clock > + * @cont_name: the name of the clock in the container device > + * > + * Add a clock @name to @dev which forward to the clock @cont_name in @container > + */ > +void qdev_pass_clock(DeviceState *dev, const char *name, > + DeviceState *container, const char *cont_name); > + > +#endif /* QDEV_CLOCK_H */ > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index eb11f0f801..60a65f6142 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -131,6 +131,19 @@ struct NamedGPIOList { > QLIST_ENTRY(NamedGPIOList) node; > }; > > +typedef struct NamedClockList NamedClockList; > + > +typedef struct ClockIn ClockIn; > +typedef struct ClockOut ClockOut; > + > +struct NamedClockList { > + char *name; > + bool forward; > + ClockIn *in; > + ClockOut *out; > + QLIST_ENTRY(NamedClockList) node; > +}; > + > /** > * DeviceState: > * @realized: Indicates whether the device has been fully constructed. > @@ -152,6 +165,7 @@ struct DeviceState { > int hotplugged; > BusState *parent_bus; > QLIST_HEAD(, NamedGPIOList) gpios; > + QLIST_HEAD(, NamedClockList) clocks; > QLIST_HEAD(, BusState) child_bus; > int num_child_bus; > int instance_id_alias; > diff --git a/tests/Makefile.include b/tests/Makefile.include > index f0b4628cc6..5c54beb29e 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -566,6 +566,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \ > hw/core/irq.o \ > hw/core/fw-path-provider.o \ > hw/core/reset.o \ > + hw/core/clock.o \ > $(test-qapi-obj-y) > tests/test-vmstate$(EXESUF): tests/test-vmstate.o \ > migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \ > Except the cosmetic comments: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> (Note, this series needs a rebase now).
On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Add functions to easily add input or output clocks to a device. > A clock objects is added as a child of the device. "object" > The api is very similar the gpio's one. "API"; "to the GPIO API". > > This is based on the original work of Frederic Konrad. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > > +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, > + bool forward) > +{ > + NamedClockList *ncl; > + > + /* > + * The clock path will be computed by the device's realize function call. > + * This is required to ensure the clock's canonical path is right and log > + * messages are meaningfull. "meaningful" > + */ > + assert(name); > + assert(!dev->realized); > + > + /* The ncl structure will be freed in device's finalize function call */ Do you mean "in device_finalize()", or "in the finalize method of the device" ? If you mean a specific function, then it's good to name it, so the reader can go and check that code if they need to confirm that there's a matching free()/deref/etc. > + ncl = g_malloc0(sizeof(*ncl)); Prefer g_new0(NamedClockList, 1). > + ncl->name = g_strdup(name); > + ncl->forward = forward; > + > + QLIST_INSERT_HEAD(&dev->clocks, ncl, node); > + return ncl; > +} > + > +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + Object *clk; > + > + ncl = qdev_init_clocklist(dev, name, false); > + > + clk = object_new(TYPE_CLOCK_OUT); > + > + /* will fail if name already exists */ This is true but it would be more helpful to say /* * Trying to create a clock whose name clashes with some other * clock or property is a bug in the caller and we will abort(). */ (assuming that's what's going on here). > + object_property_add_child(OBJECT(dev), name, clk, &error_abort); > + object_unref(clk); /* remove the initial ref made by object_new */ > + > + ncl->out = CLOCK_OUT(clk); > + return ncl->out; > +} > + > +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, > + ClockCallback *callback, void *opaque) > +{ > + NamedClockList *ncl; > + Object *clk; > + > + ncl = qdev_init_clocklist(dev, name, false); > + > + clk = object_new(TYPE_CLOCK_IN); > + /* > + * the ref initialized by object_new will be cleared during dev finalize. This means "in device_finalize()", I think from reading later patches ? > + * It allows us to safely remove the callback. > + */ > + > + /* will fail if name already exists */ Similar remark as for earlier comment. > + object_property_add_child(OBJECT(dev), name, clk, &error_abort); > + > + ncl->in = CLOCK_IN(clk); > + if (callback) { > + clock_set_callback(ncl->in, callback, opaque); > + } > + return ncl->in; > +} > +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + > + assert(dev && name); > + > + ncl = qdev_get_clocklist(dev, name); > + return ncl ? ncl->in : NULL; > +} Do we expect to want to be able to pass in the name of a clock that doesn't exist ? Should that be an error rather than returning NULL ? > + > +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name) > +{ > + NamedClockList *ncl; > + > + assert(dev && name); > + > + ncl = qdev_get_clocklist(dev, name); > + return ncl ? ncl->out : NULL; Ditto. > +} > + > +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, > + Error **errp) > +{ > + ClockOut *clkout = qdev_get_clock_out(dev, name); > + > + if (!clk) { > + error_setg(errp, "NULL input clock"); > + return; > + } > + > + if (!clkout) { > + error_setg(errp, "no output clock '%s' in device", name); > + return; > + } > + > + clock_connect(clk, clkout); Do we need to support returning an error here, or would it always be a programming bug to try to connect a non-existent clock? > --- /dev/null > +++ b/include/hw/qdev-clock.h > @@ -0,0 +1,67 @@ > +#ifndef QDEV_CLOCK_H > +#define QDEV_CLOCK_H Another missing copyright/license comment. > + > +#include "hw/clock.h" > + > +/** > + * qdev_init_clock_in: > + * @dev: the device in which to add a clock "the device to add a clock input to" > + * @name: the name of the clock (can't be NULL). > + * @callback: optional callback to be called on update or NULL. > + * @opaque: argument for the callback > + * @returns: a pointer to the newly added clock > + * > + * Add a input clock to device @dev as a clock named @name. > + * This adds a child<> property. > + * The callback will be called with @dev as opaque parameter. Isn't it called with @opaque, not @dev ? > + */ > +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, > + ClockCallback *callback, void *opaque); > + > +/** > + * qdev_init_clock_out: > + * @dev: the device to add a clock to "the device to add a clock output to" > + * @name: the name of the clock (can't be NULL). > + * @callback: optional callback to be called on update or NULL. > + * @returns: a pointer to the newly added clock > + * > + * Add a output clock to device @dev as a clock named @name. > + * This adds a child<> property. > + */ > +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name); > + > +/** > + * qdev_get_clock_in: > + * @dev: the device which has the clock > + * @name: the name of the clock (can't be NULL). > + * @returns: a pointer to the clock > + * > + * Get the clock @name from @dev or NULL if does not exists. "if it does not exist" > + */ > +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name); > + > +/** > + * qdev_connect_clock_out: > + * @dev: the device which has the clock > + * @name: the name of the clock (can't be NULL). > + * @errp: error report > + * > + * Connect @clk to the output clock @name of @dev. > + * Reports an error if clk is NULL or @name does not exists in @dev. "or if @name does not exist in @dev" > + */ > +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, > + Error **errp); > + > +/** > + * qdev_pass_clock: > + * @dev: the device to forward the clock to > + * @name: the name of the clock to be added (can't be NULL) > + * @container: the device which already has the clock > + * @cont_name: the name of the clock in the container device > + * > + * Add a clock @name to @dev which forward to the clock @cont_name in @container > + */ 'container' seems odd terminology here, because I would expect the usual use of this function to be when a 'container' object like an SoC wants to forward a clock to one of its components; in that case the 'container' SoC would be @dev, wouldn't it? We should get this to be the same way round as qdev_pass_gpios(), which takes "DeviceState *dev, DeviceState *container", and passes the gpios that exist on 'dev' over to 'container' so that 'container' now has gpios which it did not before. Also, your use of 'forward to' is inconsistent: in the 'dev' documentation you say we're forwarding the clock to 'dev', but in the body of the documentation you say we're forwarding the clock to the clock in 'container'. I think the way to resolve this is to stick to the terminology in the function name itself: @dev: the device which has the clock @name: the name of the clock on @dev @container: the name of the device which the clock should be passed to @cont_name: the name to use for the clock on @container Q: if you pass a clock to another device with this function, does it still exist to be used directly on the original device? For qdev_pass_gpios it does not (I think), but this is more accident of implementation than anything else. > +void qdev_pass_clock(DeviceState *dev, const char *name, > + DeviceState *container, const char *cont_name); > + > +#endif /* QDEV_CLOCK_H */ > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index eb11f0f801..60a65f6142 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -131,6 +131,19 @@ struct NamedGPIOList { > QLIST_ENTRY(NamedGPIOList) node; > }; > > +typedef struct NamedClockList NamedClockList; > + > +typedef struct ClockIn ClockIn; > +typedef struct ClockOut ClockOut; > + > +struct NamedClockList { > + char *name; Could this be 'const char*' ? > + bool forward; > + ClockIn *in; > + ClockOut *out; > + QLIST_ENTRY(NamedClockList) node; > +}; thanks -- PMM
On 11/25/19 2:30 PM, Philippe Mathieu-Daudé wrote: > Nitpick: remove trailing dot in patch subject > > On 9/4/19 2:55 PM, Damien Hedde wrote: >> Add functions to easily add input or output clocks to a device. >> A clock objects is added as a child of the device. > > <newline>? > >> The api is very similar the gpio's one. > > Maybe "This API is very similar to the QDEV GPIO API." > >> >> This is based on the original work of Frederic Konrad. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> >> --- >> I've removed the reviewed-by/tested-by of Philippe because I did a small >> modification. >> >> qdev_connect_clock() which allowed to connect an input to an output is >> now split in 2: >> + qdev_get_clock_in() which gets a given input from a device >> + qdev_connect_clock_out() which connect a given output to a clock >> (previously fetched by qdev_get_clock_in()) >> This part is located in (qdev-clock.[c|h]). >> It better matches gpios api and also add the possibility to connect a >> device's input clock to a random output clock (used in patch 9). >> >> Also add missing qdev-clock in the test-qdev-global-props so that tests >> pass. >> --- >> hw/core/Makefile.objs | 2 +- >> hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++ >> hw/core/qdev.c | 32 +++++++++ >> include/hw/qdev-clock.h | 67 +++++++++++++++++ >> include/hw/qdev-core.h | 14 ++++ >> tests/Makefile.include | 1 + > > Please setup the scripts/git.orderfile to ease reviews. > >> 6 files changed, 270 insertions(+), 1 deletion(-) >> create mode 100644 hw/core/qdev-clock.c >> create mode 100644 include/hw/qdev-clock.h >> >> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs >> index 8fcebf2e67..4523d3b5c7 100644 >> --- a/hw/core/Makefile.objs >> +++ b/hw/core/Makefile.objs >> @@ -1,5 +1,5 @@ >> # core qdev-related obj files, also used by *-user: >> -common-obj-y += qdev.o qdev-properties.o >> +common-obj-y += qdev.o qdev-properties.o qdev-clock.o >> common-obj-y += bus.o reset.o >> common-obj-y += resettable.o >> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o >> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c >> new file mode 100644 >> index 0000000000..bebdd8fa15 >> --- /dev/null >> +++ b/hw/core/qdev-clock.c >> @@ -0,0 +1,155 @@ >> +/* >> + * Device's clock >> + * >> + * Copyright GreenSocs 2016-2018 > > 2019 > >> + * >> + * Authors: >> + * Frederic Konrad <fred.konrad@greensocs.com> >> + * Damien Hedde <damien.hedde@greensocs.com> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "hw/qdev-clock.h" >> +#include "hw/qdev-core.h" >> +#include "qapi/error.h" >> + >> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const >> char *name, >> + bool forward) > > Indentation off. > >> +{ >> + NamedClockList *ncl; >> + >> + /* >> + * The clock path will be computed by the device's realize >> function call. >> + * This is required to ensure the clock's canonical path is right >> and log >> + * messages are meaningfull. >> + */ >> + assert(name); >> + assert(!dev->realized); >> + >> + /* The ncl structure will be freed in device's finalize function >> call */ >> + ncl = g_malloc0(sizeof(*ncl)); > > Similar but easier to review: > > ncl = g_new0(NamedClockList, 1) > >> + ncl->name = g_strdup(name); >> + ncl->forward = forward; >> + >> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node); >> + return ncl; >> +} >> + >> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name) >> +{ >> + NamedClockList *ncl; >> + Object *clk; >> + >> + ncl = qdev_init_clocklist(dev, name, false); >> + >> + clk = object_new(TYPE_CLOCK_OUT); >> + >> + /* will fail if name already exists */ >> + object_property_add_child(OBJECT(dev), name, clk, &error_abort); >> + object_unref(clk); /* remove the initial ref made by object_new */ >> + >> + ncl->out = CLOCK_OUT(clk); >> + return ncl->out; >> +} >> + >> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, >> + ClockCallback *callback, void *opaque) > > Indentation off. > >> +{ >> + NamedClockList *ncl; >> + Object *clk; >> + >> + ncl = qdev_init_clocklist(dev, name, false); >> + >> + clk = object_new(TYPE_CLOCK_IN); >> + /* >> + * the ref initialized by object_new will be cleared during dev >> finalize. >> + * It allows us to safely remove the callback. >> + */ >> + >> + /* will fail if name already exists */ >> + object_property_add_child(OBJECT(dev), name, clk, &error_abort); >> + >> + ncl->in = CLOCK_IN(clk); >> + if (callback) { >> + clock_set_callback(ncl->in, callback, opaque); >> + } >> + return ncl->in; >> +} >> + >> +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const >> char *name) >> +{ >> + NamedClockList *ncl; >> + >> + QLIST_FOREACH(ncl, &dev->clocks, node) { >> + if (strcmp(name, ncl->name) == 0) { >> + return ncl; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +void qdev_pass_clock(DeviceState *dev, const char *name, >> + DeviceState *container, const char *cont_name) >> +{ >> + NamedClockList *original_ncl, *ncl; >> + Object **clk; > > Is it really a Object** or a Object*? An Object** because it tells where the Object* is stored for the link property below. > >> + >> + assert(container && cont_name); >> + >> + original_ncl = qdev_get_clocklist(container, cont_name); >> + assert(original_ncl); /* clock must exist in origin */ >> + >> + ncl = qdev_init_clocklist(dev, name, true); >> + >> + if (ncl->out) { >> + clk = (Object **)&ncl->out; >> + } else { >> + clk = (Object **)&ncl->in; >> + } >> + >> + /* will fail if name already exists */ >> + object_property_add_link(OBJECT(dev), name, >> object_get_typename(*clk), >> + clk, NULL, OBJ_PROP_LINK_STRONG, >> &error_abort); >> +} >> + -- Damien
On 12/2/19 3:34 PM, Peter Maydell wrote: > On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote: >> >> Add functions to easily add input or output clocks to a device. >> A clock objects is added as a child of the device. > > "object" > >> The api is very similar the gpio's one. > > "API"; "to the GPIO API". > >> >> This is based on the original work of Frederic Konrad. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> > >> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, >> + bool forward) >> +{ >> + NamedClockList *ncl; >> + >> + /* >> + * The clock path will be computed by the device's realize function call. >> + * This is required to ensure the clock's canonical path is right and log >> + * messages are meaningfull. > > "meaningful" > >> + */ >> + assert(name); >> + assert(!dev->realized); >> + >> + /* The ncl structure will be freed in device's finalize function call */ > > Do you mean "in device_finalize()", or "in the finalize method > of the device" ? If you mean a specific function, then it's > good to name it, so the reader can go and check that code if > they need to confirm that there's a matching free()/deref/etc. Yes, it is device_finalize(). I'll update the comment. > >> + ncl = g_malloc0(sizeof(*ncl)); > > Prefer g_new0(NamedClockList, 1). > >> + ncl->name = g_strdup(name); >> + ncl->forward = forward; >> + >> + QLIST_INSERT_HEAD(&dev->clocks, ncl, node); >> + return ncl; >> +} >> + >> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name) >> +{ >> + NamedClockList *ncl; >> + Object *clk; >> + >> + ncl = qdev_init_clocklist(dev, name, false); >> + >> + clk = object_new(TYPE_CLOCK_OUT); >> + >> + /* will fail if name already exists */ > > This is true but it would be more helpful to say > /* > * Trying to create a clock whose name clashes with some other > * clock or property is a bug in the caller and we will abort(). > */ > > (assuming that's what's going on here). You're right. > >> + object_property_add_child(OBJECT(dev), name, clk, &error_abort); >> + object_unref(clk); /* remove the initial ref made by object_new */ >> + >> + ncl->out = CLOCK_OUT(clk); >> + return ncl->out; >> +} >> + >> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, >> + ClockCallback *callback, void *opaque) >> +{ >> + NamedClockList *ncl; >> + Object *clk; >> + >> + ncl = qdev_init_clocklist(dev, name, false); >> + >> + clk = object_new(TYPE_CLOCK_IN); >> + /* >> + * the ref initialized by object_new will be cleared during dev finalize. > > This means "in device_finalize()", I think from reading later patches ? Yes. I'll update the comment too. > >> + * It allows us to safely remove the callback. >> + */ >> + >> + /* will fail if name already exists */ > > Similar remark as for earlier comment. > >> + object_property_add_child(OBJECT(dev), name, clk, &error_abort); >> + >> + ncl->in = CLOCK_IN(clk); >> + if (callback) { >> + clock_set_callback(ncl->in, callback, opaque); >> + } >> + return ncl->in; >> +} > >> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name) >> +{ >> + NamedClockList *ncl; >> + >> + assert(dev && name); >> + >> + ncl = qdev_get_clocklist(dev, name); >> + return ncl ? ncl->in : NULL; >> +} > > Do we expect to want to be able to pass in the name of > a clock that doesn't exist ? Should that be an error > rather than returning NULL ? I think it can be an error and we may assert the clock exists. > >> + >> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name) >> +{ >> + NamedClockList *ncl; >> + >> + assert(dev && name); >> + >> + ncl = qdev_get_clocklist(dev, name); >> + return ncl ? ncl->out : NULL; > > Ditto. > >> +} >> + >> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, >> + Error **errp) >> +{ >> + ClockOut *clkout = qdev_get_clock_out(dev, name); >> + >> + if (!clk) { >> + error_setg(errp, "NULL input clock"); >> + return; >> + } >> + >> + if (!clkout) { >> + error_setg(errp, "no output clock '%s' in device", name); >> + return; >> + } >> + >> + clock_connect(clk, clkout); > > Do we need to support returning an error here, or would it > always be a programming bug to try to connect a non-existent clock? As for above, I think it can be considered an error. Should I remove the errp and do an assert instead ? > >> --- /dev/null >> +++ b/include/hw/qdev-clock.h >> @@ -0,0 +1,67 @@ >> +#ifndef QDEV_CLOCK_H >> +#define QDEV_CLOCK_H > > Another missing copyright/license comment. > >> + >> +#include "hw/clock.h" >> + >> +/** >> + * qdev_init_clock_in: >> + * @dev: the device in which to add a clock > > "the device to add a clock input to" > >> + * @name: the name of the clock (can't be NULL). >> + * @callback: optional callback to be called on update or NULL. >> + * @opaque: argument for the callback >> + * @returns: a pointer to the newly added clock >> + * >> + * Add a input clock to device @dev as a clock named @name. >> + * This adds a child<> property. >> + * The callback will be called with @dev as opaque parameter. > > Isn't it called with @opaque, not @dev ? > >> + */ >> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, >> + ClockCallback *callback, void *opaque); >> + >> +/** >> + * qdev_init_clock_out: >> + * @dev: the device to add a clock to > > "the device to add a clock output to" > >> + * @name: the name of the clock (can't be NULL). >> + * @callback: optional callback to be called on update or NULL. >> + * @returns: a pointer to the newly added clock >> + * >> + * Add a output clock to device @dev as a clock named @name. >> + * This adds a child<> property. >> + */ >> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name); >> + >> +/** >> + * qdev_get_clock_in: >> + * @dev: the device which has the clock >> + * @name: the name of the clock (can't be NULL). >> + * @returns: a pointer to the clock >> + * >> + * Get the clock @name from @dev or NULL if does not exists. > > "if it does not exist" > >> + */ >> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name); >> + >> +/** >> + * qdev_connect_clock_out: >> + * @dev: the device which has the clock >> + * @name: the name of the clock (can't be NULL). >> + * @errp: error report >> + * >> + * Connect @clk to the output clock @name of @dev. >> + * Reports an error if clk is NULL or @name does not exists in @dev. > > "or if @name does not exist in @dev" > >> + */ >> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, >> + Error **errp); >> + >> +/** >> + * qdev_pass_clock: >> + * @dev: the device to forward the clock to >> + * @name: the name of the clock to be added (can't be NULL) >> + * @container: the device which already has the clock >> + * @cont_name: the name of the clock in the container device >> + * >> + * Add a clock @name to @dev which forward to the clock @cont_name in @container >> + */ > > 'container' seems odd terminology here, because I would expect > the usual use of this function to be when a 'container' object > like an SoC wants to forward a clock to one of its components; > in that case the 'container' SoC would be @dev, wouldn't it? Yes. I agree it is confusing. This function just allow a a device 'A' to exhibit a clock from another device 'B' (typically a composing sub-device, inside a soc like you said). The device A is not supposed to interact with the clock itself. The original sub-device is the true owner/controller of the clock and is the only one which should use the clock API: setting a callback on it (input clock); or updating the clock frequency (output clock). Basically, it just add the clock into the device clock namespace without interfering with it. > We should get this to be the same way round as qdev_pass_gpios(), > which takes "DeviceState *dev, DeviceState *container", and > passes the gpios that exist on 'dev' over to 'container' so that > 'container' now has gpios which it did not before. Ok, I'll invert the arguments. > > Also, your use of 'forward to' is inconsistent: in the 'dev' > documentation you say we're forwarding the clock to 'dev', > but in the body of the documentation you say we're forwarding > the clock to the clock in 'container'. I'll try to clarify this and make the documentation more consistent with the comments here. > > I think the way to resolve this is to stick to the terminology > in the function name itself: > @dev: the device which has the clock > @name: the name of the clock on @dev > @container: the name of the device which the clock should > be passed to > @cont_name: the name to use for the clock on @container I think container is confusing because depending on how we reason (clock in a device; device in another device), we end up thinking the opposite. Maybe we can use "exhibit" instead of "container" in the 2nd pair of parameters, or prefix use "origin" or "owner" as a prefix for the first pair of parameters. > > Q: if you pass a clock to another device with this function, > does it still exist to be used directly on the original > device? For qdev_pass_gpios it does not (I think), but > this is more accident of implementation than anything else. It depends what we mean by "used by". Original device can: + set the callback in case it is an input + update the frequency in case it is an output But since an input clock can only be connected once, I think the logic here is that any connection should now go through the new device. But this is not checked and using one or the other is exactly the same. Maybe I misunderstood the meaning of qdev_pass_gpios(). > >> +void qdev_pass_clock(DeviceState *dev, const char *name, >> + DeviceState *container, const char *cont_name); >> + >> +#endif /* QDEV_CLOCK_H */ >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index eb11f0f801..60a65f6142 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -131,6 +131,19 @@ struct NamedGPIOList { >> QLIST_ENTRY(NamedGPIOList) node; >> }; >> >> +typedef struct NamedClockList NamedClockList; >> + >> +typedef struct ClockIn ClockIn; >> +typedef struct ClockOut ClockOut; >> + >> +struct NamedClockList { >> + char *name; > > Could this be 'const char*' ? Yes. -- Damien
On 12/4/19 10:05 AM, Damien Hedde wrote: > On 12/2/19 3:34 PM, Peter Maydell wrote: >> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote: >>> [...] >>> +/** >>> + * qdev_pass_clock: >>> + * @dev: the device to forward the clock to >>> + * @name: the name of the clock to be added (can't be NULL) >>> + * @container: the device which already has the clock >>> + * @cont_name: the name of the clock in the container device >>> + * >>> + * Add a clock @name to @dev which forward to the clock @cont_name in @container >>> + */ >> >> 'container' seems odd terminology here, because I would expect >> the usual use of this function to be when a 'container' object >> like an SoC wants to forward a clock to one of its components; >> in that case the 'container' SoC would be @dev, wouldn't it? > > Yes. I agree it is confusing. > This function just allow a a device 'A' to exhibit a clock from another > device 'B' (typically a composing sub-device, inside a soc like you > said). The device A is not supposed to interact with the clock itself. > The original sub-device is the true > owner/controller of the clock and is the only one which should use the > clock API: setting a callback on it (input clock); or updating the clock > frequency (output clock). > Basically, it just add the clock into the device clock namespace without > interfering with it. > >> We should get this to be the same way round as qdev_pass_gpios(), >> which takes "DeviceState *dev, DeviceState *container", and >> passes the gpios that exist on 'dev' over to 'container' so that >> 'container' now has gpios which it did not before. > > Ok, I'll invert the arguments. > >> >> Also, your use of 'forward to' is inconsistent: in the 'dev' >> documentation you say we're forwarding the clock to 'dev', >> but in the body of the documentation you say we're forwarding >> the clock to the clock in 'container'. > > I'll try to clarify this and make the documentation more consistent with > the comments here. > >> >> I think the way to resolve this is to stick to the terminology >> in the function name itself: >> @dev: the device which has the clock >> @name: the name of the clock on @dev >> @container: the name of the device which the clock should >> be passed to >> @cont_name: the name to use for the clock on @container > > I think container is confusing because depending on how we reason (clock > in a device; device in another device), we end up thinking the opposite. > > Maybe we can use "exhibit" instead of "container" in the 2nd pair of > parameters, or prefix use "origin" or "owner" as a prefix for the first > pair of parameters. @sink vs @source? >> Q: if you pass a clock to another device with this function, >> does it still exist to be used directly on the original >> device? For qdev_pass_gpios it does not (I think), but >> this is more accident of implementation than anything else. > > It depends what we mean by "used by". > Original device can: > + set the callback in case it is an input > + update the frequency in case it is an output Hmm here you use @input vs @output... > But since an input clock can only be connected once, > I think the logic here is that any connection should now go through the > new device. But this is not checked and using one or the other is > exactly the same. > > Maybe I misunderstood the meaning of qdev_pass_gpios(). [...]
On 12/4/19 10:53 AM, Philippe Mathieu-Daudé wrote: > On 12/4/19 10:05 AM, Damien Hedde wrote: >> On 12/2/19 3:34 PM, Peter Maydell wrote: >>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde >>> <damien.hedde@greensocs.com> wrote: >>>> > [...] >>>> +/** >>>> + * qdev_pass_clock: >>>> + * @dev: the device to forward the clock to >>>> + * @name: the name of the clock to be added (can't be NULL) >>>> + * @container: the device which already has the clock >>>> + * @cont_name: the name of the clock in the container device >>>> + * >>>> + * Add a clock @name to @dev which forward to the clock @cont_name >>>> in @container >>>> + */ >>> >>> 'container' seems odd terminology here, because I would expect >>> the usual use of this function to be when a 'container' object >>> like an SoC wants to forward a clock to one of its components; >>> in that case the 'container' SoC would be @dev, wouldn't it? >> >> Yes. I agree it is confusing. >> This function just allow a a device 'A' to exhibit a clock from another >> device 'B' (typically a composing sub-device, inside a soc like you >> said). The device A is not supposed to interact with the clock itself. >> The original sub-device is the true >> owner/controller of the clock and is the only one which should use the >> clock API: setting a callback on it (input clock); or updating the clock >> frequency (output clock). >> Basically, it just add the clock into the device clock namespace without >> interfering with it. >> >>> We should get this to be the same way round as qdev_pass_gpios(), >>> which takes "DeviceState *dev, DeviceState *container", and >>> passes the gpios that exist on 'dev' over to 'container' so that >>> 'container' now has gpios which it did not before. >> >> Ok, I'll invert the arguments. >> >>> >>> Also, your use of 'forward to' is inconsistent: in the 'dev' >>> documentation you say we're forwarding the clock to 'dev', >>> but in the body of the documentation you say we're forwarding >>> the clock to the clock in 'container'. >> >> I'll try to clarify this and make the documentation more consistent with >> the comments here. >> >>> >>> I think the way to resolve this is to stick to the terminology >>> in the function name itself: >>> @dev: the device which has the clock >>> @name: the name of the clock on @dev >>> @container: the name of the device which the clock should >>> be passed to >>> @cont_name: the name to use for the clock on @container >> >> I think container is confusing because depending on how we reason (clock >> in a device; device in another device), we end up thinking the opposite. >> >> Maybe we can use "exhibit" instead of "container" in the 2nd pair of >> parameters, or prefix use "origin" or "owner" as a prefix for the first >> pair of parameters. > > @sink vs @source? > >>> Q: if you pass a clock to another device with this function, >>> does it still exist to be used directly on the original >>> device? For qdev_pass_gpios it does not (I think), but >>> this is more accident of implementation than anything else. >> >> It depends what we mean by "used by". >> Original device can: >> + set the callback in case it is an input >> + update the frequency in case it is an output > > Hmm here you use @input vs @output... Not really. What I meant here is that the device which originally creates the clock is the only device which can interact with the clock. And it will never gives this right to another device even if qdev_pass_clock() is used. There are 2 interactions, depending on the clock direction (input or output). If it is an input clock, it can register a callback on frequency changes; and if it is an output clock, it can updates the frequency of the clock. > >> But since an input clock can only be connected once, >> I think the logic here is that any connection should now go through the >> new device. But this is not checked and using one or the other is >> exactly the same. >> >> Maybe I misunderstood the meaning of qdev_pass_gpios(). > [...] >
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs index 8fcebf2e67..4523d3b5c7 100644 --- a/hw/core/Makefile.objs +++ b/hw/core/Makefile.objs @@ -1,5 +1,5 @@ # core qdev-related obj files, also used by *-user: -common-obj-y += qdev.o qdev-properties.o +common-obj-y += qdev.o qdev-properties.o qdev-clock.o common-obj-y += bus.o reset.o common-obj-y += resettable.o common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c new file mode 100644 index 0000000000..bebdd8fa15 --- /dev/null +++ b/hw/core/qdev-clock.c @@ -0,0 +1,155 @@ +/* + * Device's clock + * + * Copyright GreenSocs 2016-2018 + * + * Authors: + * Frederic Konrad <fred.konrad@greensocs.com> + * Damien Hedde <damien.hedde@greensocs.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "hw/qdev-clock.h" +#include "hw/qdev-core.h" +#include "qapi/error.h" + +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, + bool forward) +{ + NamedClockList *ncl; + + /* + * The clock path will be computed by the device's realize function call. + * This is required to ensure the clock's canonical path is right and log + * messages are meaningfull. + */ + assert(name); + assert(!dev->realized); + + /* The ncl structure will be freed in device's finalize function call */ + ncl = g_malloc0(sizeof(*ncl)); + ncl->name = g_strdup(name); + ncl->forward = forward; + + QLIST_INSERT_HEAD(&dev->clocks, ncl, node); + return ncl; +} + +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name) +{ + NamedClockList *ncl; + Object *clk; + + ncl = qdev_init_clocklist(dev, name, false); + + clk = object_new(TYPE_CLOCK_OUT); + + /* will fail if name already exists */ + object_property_add_child(OBJECT(dev), name, clk, &error_abort); + object_unref(clk); /* remove the initial ref made by object_new */ + + ncl->out = CLOCK_OUT(clk); + return ncl->out; +} + +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, + ClockCallback *callback, void *opaque) +{ + NamedClockList *ncl; + Object *clk; + + ncl = qdev_init_clocklist(dev, name, false); + + clk = object_new(TYPE_CLOCK_IN); + /* + * the ref initialized by object_new will be cleared during dev finalize. + * It allows us to safely remove the callback. + */ + + /* will fail if name already exists */ + object_property_add_child(OBJECT(dev), name, clk, &error_abort); + + ncl->in = CLOCK_IN(clk); + if (callback) { + clock_set_callback(ncl->in, callback, opaque); + } + return ncl->in; +} + +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const char *name) +{ + NamedClockList *ncl; + + QLIST_FOREACH(ncl, &dev->clocks, node) { + if (strcmp(name, ncl->name) == 0) { + return ncl; + } + } + + return NULL; +} + +void qdev_pass_clock(DeviceState *dev, const char *name, + DeviceState *container, const char *cont_name) +{ + NamedClockList *original_ncl, *ncl; + Object **clk; + + assert(container && cont_name); + + original_ncl = qdev_get_clocklist(container, cont_name); + assert(original_ncl); /* clock must exist in origin */ + + ncl = qdev_init_clocklist(dev, name, true); + + if (ncl->out) { + clk = (Object **)&ncl->out; + } else { + clk = (Object **)&ncl->in; + } + + /* will fail if name already exists */ + object_property_add_link(OBJECT(dev), name, object_get_typename(*clk), + clk, NULL, OBJ_PROP_LINK_STRONG, &error_abort); +} + +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name) +{ + NamedClockList *ncl; + + assert(dev && name); + + ncl = qdev_get_clocklist(dev, name); + return ncl ? ncl->in : NULL; +} + +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name) +{ + NamedClockList *ncl; + + assert(dev && name); + + ncl = qdev_get_clocklist(dev, name); + return ncl ? ncl->out : NULL; +} + +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, + Error **errp) +{ + ClockOut *clkout = qdev_get_clock_out(dev, name); + + if (!clk) { + error_setg(errp, "NULL input clock"); + return; + } + + if (!clkout) { + error_setg(errp, "no output clock '%s' in device", name); + return; + } + + clock_connect(clk, clkout); +} diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 9095f2b9c1..eae6cd3e09 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -37,6 +37,7 @@ #include "hw/qdev-properties.h" #include "hw/boards.h" #include "hw/sysbus.h" +#include "hw/clock.h" #include "migration/vmstate.h" bool qdev_hotplug = false; @@ -821,6 +822,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) DeviceClass *dc = DEVICE_GET_CLASS(dev); HotplugHandler *hotplug_ctrl; BusState *bus; + NamedClockList *clk; Error *local_err = NULL; bool unattached_parent = false; static int unattached_count; @@ -869,6 +871,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp) */ g_free(dev->canonical_path); dev->canonical_path = object_get_canonical_path(OBJECT(dev)); + QLIST_FOREACH(clk, &dev->clocks, node) { + if (clk->forward) { + continue; + } else if (clk->in != NULL) { + clock_in_setup_canonical_path(clk->in); + } else { + clock_out_setup_canonical_path(clk->out); + } + } if (qdev_get_vmsd(dev)) { if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, @@ -999,6 +1010,7 @@ static void device_initfn(Object *obj) (Object **)&dev->parent_bus, NULL, 0, &error_abort); QLIST_INIT(&dev->gpios); + QLIST_INIT(&dev->clocks); } static void device_post_init(Object *obj) @@ -1015,6 +1027,7 @@ static void device_post_init(Object *obj) static void device_finalize(Object *obj) { NamedGPIOList *ngl, *next; + NamedClockList *clk, *clk_next; DeviceState *dev = DEVICE(obj); @@ -1028,6 +1041,25 @@ static void device_finalize(Object *obj) */ } + QLIST_FOREACH_SAFE(clk, &dev->clocks, node, clk_next) { + QLIST_REMOVE(clk, node); + if (!clk->forward && clk->in) { + /* + * if this clock is not forwarded, clk->in is a child of dev. + * At this point the child property and associated reference is + * already deleted but we kept a ref on clk->in to ensure it lives + * up to this point and we can safely remove the callback. + * It avoids having a lost callback to a deleted device if the + * clk->in is still referenced somewhere else (eg: by a clock + * output). + */ + clock_clear_callback(clk->in); + object_unref(OBJECT(clk->in)); + } + g_free(clk->name); + g_free(clk); + } + /* Only send event if the device had been completely realized */ if (dev->pending_deleted_event) { g_assert(dev->canonical_path); diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h new file mode 100644 index 0000000000..c4ea912fdc --- /dev/null +++ b/include/hw/qdev-clock.h @@ -0,0 +1,67 @@ +#ifndef QDEV_CLOCK_H +#define QDEV_CLOCK_H + +#include "hw/clock.h" + +/** + * qdev_init_clock_in: + * @dev: the device in which to add a clock + * @name: the name of the clock (can't be NULL). + * @callback: optional callback to be called on update or NULL. + * @opaque: argument for the callback + * @returns: a pointer to the newly added clock + * + * Add a input clock to device @dev as a clock named @name. + * This adds a child<> property. + * The callback will be called with @dev as opaque parameter. + */ +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name, + ClockCallback *callback, void *opaque); + +/** + * qdev_init_clock_out: + * @dev: the device to add a clock to + * @name: the name of the clock (can't be NULL). + * @callback: optional callback to be called on update or NULL. + * @returns: a pointer to the newly added clock + * + * Add a output clock to device @dev as a clock named @name. + * This adds a child<> property. + */ +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name); + +/** + * qdev_get_clock_in: + * @dev: the device which has the clock + * @name: the name of the clock (can't be NULL). + * @returns: a pointer to the clock + * + * Get the clock @name from @dev or NULL if does not exists. + */ +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name); + +/** + * qdev_connect_clock_out: + * @dev: the device which has the clock + * @name: the name of the clock (can't be NULL). + * @errp: error report + * + * Connect @clk to the output clock @name of @dev. + * Reports an error if clk is NULL or @name does not exists in @dev. + */ +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn *clk, + Error **errp); + +/** + * qdev_pass_clock: + * @dev: the device to forward the clock to + * @name: the name of the clock to be added (can't be NULL) + * @container: the device which already has the clock + * @cont_name: the name of the clock in the container device + * + * Add a clock @name to @dev which forward to the clock @cont_name in @container + */ +void qdev_pass_clock(DeviceState *dev, const char *name, + DeviceState *container, const char *cont_name); + +#endif /* QDEV_CLOCK_H */ diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index eb11f0f801..60a65f6142 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -131,6 +131,19 @@ struct NamedGPIOList { QLIST_ENTRY(NamedGPIOList) node; }; +typedef struct NamedClockList NamedClockList; + +typedef struct ClockIn ClockIn; +typedef struct ClockOut ClockOut; + +struct NamedClockList { + char *name; + bool forward; + ClockIn *in; + ClockOut *out; + QLIST_ENTRY(NamedClockList) node; +}; + /** * DeviceState: * @realized: Indicates whether the device has been fully constructed. @@ -152,6 +165,7 @@ struct DeviceState { int hotplugged; BusState *parent_bus; QLIST_HEAD(, NamedGPIOList) gpios; + QLIST_HEAD(, NamedClockList) clocks; QLIST_HEAD(, BusState) child_bus; int num_child_bus; int instance_id_alias; diff --git a/tests/Makefile.include b/tests/Makefile.include index f0b4628cc6..5c54beb29e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -566,6 +566,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \ hw/core/irq.o \ hw/core/fw-path-provider.o \ hw/core/reset.o \ + hw/core/clock.o \ $(test-qapi-obj-y) tests/test-vmstate$(EXESUF): tests/test-vmstate.o \ migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
Add functions to easily add input or output clocks to a device. A clock objects is added as a child of the device. The api is very similar the gpio's one. This is based on the original work of Frederic Konrad. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- I've removed the reviewed-by/tested-by of Philippe because I did a small modification. qdev_connect_clock() which allowed to connect an input to an output is now split in 2: + qdev_get_clock_in() which gets a given input from a device + qdev_connect_clock_out() which connect a given output to a clock (previously fetched by qdev_get_clock_in()) This part is located in (qdev-clock.[c|h]). It better matches gpios api and also add the possibility to connect a device's input clock to a random output clock (used in patch 9). Also add missing qdev-clock in the test-qdev-global-props so that tests pass. --- hw/core/Makefile.objs | 2 +- hw/core/qdev-clock.c | 155 ++++++++++++++++++++++++++++++++++++++++ hw/core/qdev.c | 32 +++++++++ include/hw/qdev-clock.h | 67 +++++++++++++++++ include/hw/qdev-core.h | 14 ++++ tests/Makefile.include | 1 + 6 files changed, 270 insertions(+), 1 deletion(-) create mode 100644 hw/core/qdev-clock.c create mode 100644 include/hw/qdev-clock.h