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

[Damien Hedde]

On 9/25/18 11:36 AM, Peter Maydell wrote:
> 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.

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