On 24 May 2017 at 08:35, KONRAD Frederic <fred.kon...@greensocs.com> wrote: > Le 28/02/2017 à 11:02, fred.kon...@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
