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

[Damien Hedde <damien.hedde@greensocs.com>]

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