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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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