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

[Peter Maydell <peter.maydell@linaro.org>]

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