Re: [Qemu-devel] [PATCH v3 00/10] Clock framework API.

[Peter Maydell <peter.maydell@linaro.org>]

On 24 May 2017 at 08:35, KONRAD Frederic <fred.konrad@greensocs.com> wrote:
> Le 28/02/2017 à 11:02, fred.konrad@greensocs.com a écrit :
>> This is the third version of the clock framework API it contains:
>>
>>   * The first 6 patches which introduce the framework.
>>   * The 7th patch which introduces a fixed-clock model.
>>   * The rest which gives an example how to model a PLL from the existing
>>     zynqmp-crf extracted from the qemu xilinx tree.
>>
>> No specific behavior is expected yet when the CRF register set is accessed
>> but
>> the user can see for example the dp_video_ref and vpll_to_lpd rate
>> changing in
>> the monitor with the "info qtree" command when the vpll_ctrl register is
>> modified.

Some top-level review:

* I like the documentation, this is very helpful
* consider tracepoints rather than DPRINTF macro
* qemu_clk_device_add_clock() does a g_strdup, but when is this freed?
  (consider devices which are hot-unpluggable)
* similarly, what if a device with a clock with a lot of child nodes
  is destroyed? how are the ClkList structs freed?
* interaction with migration -- how is the "this clock is at this rate"
  state intended to be migrated?
* I'll leave the review of the xilinx patches to the xilinx folk
* the 'introduce zynqmp_crf' patch is missing any signoffs
  (in particular if it's from the xilinx tree it will need
  signoff from them)

thanks
-- PMM