Re: [Qemu-devel] [PATCH v4 05/10] docs/clocks: add device's clock documentation

[Peter Maydell]

On 17 September 2018 at 09:40,  <address@hidden> wrote:
> From: Damien Hedde <address@hidden>
>
> 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 <address@hidden>

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