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

[Damien Hedde]

On 12/2/19 3:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <address@hidden> 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 <address@hidden>
>>
> 
>> +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