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

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

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().
[...]