[v4,05/10] docs/clocks: add device's clock documentation

From: Damien Hedde <damien.hedde@greensocs.com>

Add the documentation about the clock inputs and outputs in devices.

This is based on the original work of Frederic Konrad.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 docs/devel/clock.txt | 144 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 docs/devel/clock.txt

On 17 September 2018 at 09:40,  <damien.hedde@greensocs.com> wrote:
> From: Damien Hedde <damien.hedde@greensocs.com>
>
> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

I thought I might as well review the docs changes, since they're
a good overview of the API. I have one or two semantic changes
to suggest, a few function name alterations and some minor typo
fixes below.

> ---
>  docs/devel/clock.txt | 144 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 docs/devel/clock.txt
>
> diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
> new file mode 100644
> index 0000000000..d018fbef90
> --- /dev/null
> +++ b/docs/devel/clock.txt
> @@ -0,0 +1,144 @@
> +
> +What are device's clocks
> +========================
> +
> +Clocks are ports representing input and output clocks of a device. They are QOM
> +objects developed for the purpose of modeling the distribution of clocks in
> +QEMU.
> +
> +It allows to model the clock distribution of a platform and detect configuration

"This allows us to model"

> +errors in the clock tree such as badly configured PLL, clock source selection or
> +disabled clock.
> +
> +The objects are CLOCK_IN for the input and CLOCK_OUT for the output.
> +
> +The clock value: ClockState
> +===========================
> +
> +The ClockState is the structure carried by the CLOCK_OUT and CLOCK_IN objects.
> +
> +It actually contains two fields:
> ++ an integer representing the frequency of the clock in Hertz,
> ++ and a boolean representing the clock domain reset assertion signal.
> +
> +It only simulates the clock by transmitting the frequency value and
> +doesn't model the signal itself such as pin toggle or duty cycle.
> +The special value 0 as a frequency is legal and represent the clock being
> +inactive or gated.
> +
> +Reset is also carried in the structure since both signals are closely related.
> +But they remain value-independent, the frequency can be non-zero and the reset
> +asserted as well as the opposite.
> +
> +Adding clocks to a device
> +=========================
> +
> +Adding clocks to a device must be done during the init phase of the Device
> +object.
> +
> +To add an input clock to a device, the function qdev_init_clock_in must be used.
> +It takes the name, a callback, and an opaque parameter for the clock.
> +output is more simple, only the name is required. Typically:

"Output"

> +qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +qdev_init_clock_out(DEVICE(dev), "clk-out");
> +
> +Both functions return the created CLOCK_IN/OUT pointer. For an output, it should
> +be saved in the device's state structure in order to be able to change the
> +clock. For an input, it can be saved it one need to change the callback.

I think we should just simplify this to say that "pointer, which should be
saved in the device's state structure."

(Is freeing of the allocated memory handled by the usual
QOM reference-counting ?)

> +
> +Note that it is possible to create a static array describing clock inputs and
> +outputs. The function qdev_init_clocks must be called with the array as
> +parameters to initialize the clocks: it has the same behaviour as calling the
> +qdev_init_clock/out for each clock.
> +
> +Forwarding clocks
> +=================
> +
> +Sometimes, one needs to forward, or inherit, a clock from another device.
> +Typically, when doing device composition, a device might exposes a sub-device's

"expose"

> +clock without interfering with it.
> +The function qdev_init_clock_forward can be used to achieve this behaviour.
> +Note, that it is possible to expose the clock under a different name.
> +This works for both inputs or outputs.
> +
> +For example, if device B is a child of device A, device_a_instance_init may
> +do something like this:
> +void device_a_instance_init(Object *obj)
> +{
> +    AState *A = DEVICE_A(obj);
> +    BState *B;
> +    [...] /* create B object as child of A */
> +    qdev_init_clock_forward(A, "b_clk", B, "clk");
> +    /*
> +     * Now A has a clock "b_clk" which forwards to
> +     * the "clk" of its child B.
> +     */
> +}
> +
> +This function does not returns any clock object. It is not possible to add
> +a callback on a forwarded input clock.

I suggest naming this function qdev_pass_clock():
 * it isn't doing the same thing qdev_init_clock_in/out do (which return
   a clock object)
 * this brings the name into line with the similar qdev_pass_gpios()

> +
> +Connecting two clocks together
> +==============================
> +
> +Let's say we have 2 devices A and B. A has an output clock named "clkout" and B
> +has an input clock named "clkin".
> +
> +The clocks are connected together using the function qdev_clock_connect:
> +qdev_clock_connect(B, "clkin", A, "clkout", &error_abort);

I suggest calling this qdev_connect_clock() (which then parallels
the existing qdev_connect_gpio_out*())

> +The device which has the input must be the first argument.
> +
> +It is possible to connect several input clock to the same output. Every

"clocks"

> +input callback will be called during the output changes.

"when"

> +
> +It is not possible to disconnect a clock neither to change the clock connection

"or", not "neither"

> +after it is done.
> +
> +Changing a clock output
> +=======================
> +
> +A device can change its outputs using the clock_set function. It will trigger
> +updates on any connected inputs.
> +
> +For example, let's say that we have an output clock "clkout" and we have pointer

"a pointer to it"

> +on it in the device state because we did the following in init phase:
> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
> +
> +Then at any time, it is possible to change the clock value by doing:
> +ClockState state;
> +state.frequency = 1000 * 1000 * 1000; /* 1MHz */
> +clock_set(dev->clkout, &state);


This seems a bit indirect. Why doesn't clock_set() just take the
frequency as a direct argument ?

> +
> +Outputs must be initialized in the device_reset method to ensure every connected
> +inputs is updated at machine startup.

device_reset should not set outputs.

> +
> +Callback on input clock change
> +==============================
> +
> +Here is an example of an input callback:
> +void clock_callback(void *opaque, ClockState *state) {
> +    MyDeviceState *s = (MyDeviceState *) opaque;
> +    /*
> +     * opaque may not be the device state pointer, but most probably it is.
> +     * (It depends on what is given to the qdev_init_clock_in function)
> +     */
> +
> +    /* do something with the state */
> +    fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
> +                    clock_state_get_frequency(state));
> +
> +    /* optionally store the value for later use in our state */
> +    clock_state_copy(&s->clkin_shadow, state);


This seems odd to me. Why do we have a clock_state_copy() at all?
The input end of a device has a pointer to the input end of the
clock, and there should just be a function for "what is the current
frequency of this CLOCK_IN ?".

The callback function should probably be passed a parameter
which is the CLOCK_IN it corresponds to, not an arbitrary
ClockState. (I'm not sure how useful the ClockState struct is.)

> +}
> +
> +The state argument needs only to be copied if the device needs to use the value
> +later: the state pointer argument of the pointer will not be valid anymore
> +after the end of the function.
> +
> +Migration
> +=========
> +
> +State is not stored in clock input or output. A device is responsible for
> +setting its output clock correctly in the post_load function (using clock_set)
> +after its state has been loaded. This way, during the migration process, all
> +clocks will be correctly restored.
> --
> 2.18.0
>

thanks
-- PMM

On 9/25/18 11:36 AM, Peter Maydell wrote:
> On 17 September 2018 at 09:40,  <damien.hedde@greensocs.com> wrote:
>> From: Damien Hedde <damien.hedde@greensocs.com>
>>
>> Add the documentation about the clock inputs and outputs in devices.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> I thought I might as well review the docs changes, since they're
> a good overview of the API. I have one or two semantic changes
> to suggest, a few function name alterations and some minor typo
> fixes below.

I've only kept the parts I'm responding to. I'll do the suggested name
changes.

> 
>> ---
>>  docs/devel/clock.txt | 144 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 144 insertions(+)
>>  create mode 100644 docs/devel/clock.txt
>>
>> diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
>> new file mode 100644
>> index 0000000000..d018fbef90
>> --- /dev/null
>> +++ b/docs/devel/clock.txt
>> @@ -0,0 +1,144 @@
>> +
>> +Adding clocks to a device
>> +=========================
>> +
>> +Adding clocks to a device must be done during the init phase of the Device
>> +object.
>> +
>> +To add an input clock to a device, the function qdev_init_clock_in must be used.
>> +It takes the name, a callback, and an opaque parameter for the clock.
>> +output is more simple, only the name is required. Typically:
> 
> "Output"
> 
>> +qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
>> +qdev_init_clock_out(DEVICE(dev), "clk-out");
>> +
>> +Both functions return the created CLOCK_IN/OUT pointer. For an output, it should
>> +be saved in the device's state structure in order to be able to change the
>> +clock. For an input, it can be saved it one need to change the callback.
> 
> I think we should just simplify this to say that "pointer, which should be
> saved in the device's state structure."
> 
> (Is freeing of the allocated memory handled by the usual
> QOM reference-counting ?)

Yes. I will add a note to signal not to worry about that.

> 
>> +
>> +Changing a clock output
>> +=======================
>> +
>> +A device can change its outputs using the clock_set function. It will trigger
>> +updates on any connected inputs.
>> +
>> +For example, let's say that we have an output clock "clkout" and we have pointer
> 
> "a pointer to it"
> 
>> +on it in the device state because we did the following in init phase:
>> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
>> +
>> +Then at any time, it is possible to change the clock value by doing:
>> +ClockState state;
>> +state.frequency = 1000 * 1000 * 1000; /* 1MHz */
>> +clock_set(dev->clkout, &state);
> 
> 
> This seems a bit indirect. Why doesn't clock_set() just take the
> frequency as a direct argument ?

The structure ClockState is here because it contains 2 fields (frequency
value and reset flag).
Since I am removing the reset, I can replace ClockState pointer by the
frequency integer as you say.

> 
>> +
>> +Outputs must be initialized in the device_reset method to ensure every connected
>> +inputs is updated at machine startup.
> 
> device_reset should not set outputs.

How do the initialization then ?
Only alternative I see is to initialize a clock input when it is
connected to an output.

If device_reset does not set outputs (which are a direct consequence of
a internal device state being reset). It means calling device_reset at
any other point than simulation startup will break things. Is that the
semantic behind device_reset ?

> 
>> +
>> +Callback on input clock change
>> +==============================
>> +
>> +Here is an example of an input callback:
>> +void clock_callback(void *opaque, ClockState *state) {
>> +    MyDeviceState *s = (MyDeviceState *) opaque;
>> +    /*
>> +     * opaque may not be the device state pointer, but most probably it is.
>> +     * (It depends on what is given to the qdev_init_clock_in function)
>> +     */
>> +
>> +    /* do something with the state */
>> +    fprintf(stdout, "device new frequency is %" PRIu64 "Hz\n",
>> +                    clock_state_get_frequency(state));
>> +
>> +    /* optionally store the value for later use in our state */
>> +    clock_state_copy(&s->clkin_shadow, state);
> 
> 
> This seems odd to me. Why do we have a clock_state_copy() at all?
> The input end of a device has a pointer to the input end of the
> clock, and there should just be a function for "what is the current
> frequency of this CLOCK_IN ?".
> 
> The callback function should probably be passed a parameter
> which is the CLOCK_IN it corresponds to, not an arbitrary
> ClockState. (I'm not sure how useful the ClockState struct is.)
> 

Right now, there is no clock state in the input end and in consequence
no way to have current frequency. Hence, if one wants to access the
frequency later on, he has to save it himself.

Since we're talking to add the state in the clock object to handle the
migration, the copy function become useless too and we may also drop the
ClockState pointer argument because we will be able to request the
current frequency.

Thanks
--
Damien

On 25 September 2018 at 12:47, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
> On 9/25/18 11:36 AM, Peter Maydell wrote:
>> On 17 September 2018 at 09:40,  <damien.hedde@greensocs.com> wrote:
>>> +Outputs must be initialized in the device_reset method to ensure every connected
>>> +inputs is updated at machine startup.
>>
>> device_reset should not set outputs.
>
> How do the initialization then ?
> Only alternative I see is to initialize a clock input when it is
> connected to an output.
>
> If device_reset does not set outputs (which are a direct consequence of
> a internal device state being reset). It means calling device_reset at
> any other point than simulation startup will break things. Is that the
> semantic behind device_reset ?

device_reset's semantics are not great, but basically they are
"power on reset", equivalent to a complete simulation restart.
If a device has some other kind of warm reset triggered by
eg a register write it needs to implement that itself, perhaps
sharing code with the device reset method. A warm reset triggered
by a register write can obviously assert output signals.

I have some thoughts about trying to clean up our reset
semantics which I need to write up later this week.

thanks
-- PMM