From: KONRAD Frederic <fred.konrad@greensocs.com>

Hi,

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.

bus: main-system-bus
  type System
  dev: xlnx.zynqmp_crf, id ""
    gpio-out "sysbus-irq" 1
    qemu-clk "dbg_trace" 0
    qemu-clk "dp_stc_ref" 0
    qemu-clk "dpll_to_lpd" 12500000
    qemu-clk "acpu_clk" 0
    qemu-clk "pcie_ref" 0
    qemu-clk "topsw_main" 0
    qemu-clk "topsw_lsbus" 0
    qemu-clk "dp_audio_ref" 0
    qemu-clk "sata_ref" 0
    qemu-clk "dp_video_ref" 1428571
    qemu-clk "vpll_clk" 50000000
    qemu-clk "apll_to_lpd" 12500000
    qemu-clk "dpll_clk" 50000000
    qemu-clk "gpu_ref" 0
    qemu-clk "aux_refclk" 0
    qemu-clk "video_clk" 27000000
    qemu-clk "gdma_ref" 0
    qemu-clk "gt_crx_ref_clk" 0
    qemu-clk "dbg_fdp" 0
    qemu-clk "apll_clk" 50000000
    qemu-clk "pss_alt_ref_clk" 0
    qemu-clk "ddr" 0
    qemu-clk "dbg_tstmp" 0
    qemu-clk "pss_ref_clk" 50000000
    qemu-clk "dpdma_ref" 0
    qemu-clk "vpll_to_lpd" 12500000
    mmio 00000000fd1a0000/000000000000010c
    
This series is based on the current master
(d992f2f1368ceb92e6bfd8efece174110f4236ff).

Thanks,
Fred

V2 -> V3:
  * Rebased on current master.
  * Renamed qemu_clk / QEMUClock as suggested by Cédric.
  * Renamed in_rate to ref_rate and out_rate to rate.
  * Renamed qemu_clk_bind_clock to qemu_clk_bind.
  * Example added to the documentation as suggested by Peter.

V1 -> V2:
  * Rebased on current master.
  * Some function renamed and documentation fixed.

RFC -> V1:
  * Rebased on current master.
  * The docs has been fixed.
  * qemu_clk_init_device helper has been provided to ease the initialization
    of the devices.

KONRAD Frederic (10):
  qemu-clk: introduce qemu-clk qom object
  qemu-clk: allow to add a clock to a device
  qemu-clk: allow to bind two clocks together
  qemu-clk: introduce an init array to help the device construction
  qdev-monitor: print the device's clock with info qtree
  docs: add qemu-clock documentation
  introduce fixed-clock
  introduce zynqmp_crf
  zynqmp: add the zynqmp_crf to the platform
  zynqmp: add reference clock

 Makefile.objs                 |   1 +
 docs/clock.txt                | 278 ++++++++++++
 hw/arm/xlnx-zynqmp.c          |  56 +++
 hw/misc/Makefile.objs         |   2 +
 hw/misc/fixed-clock.c         |  88 ++++
 hw/misc/xilinx_zynqmp_crf.c   | 968 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h  |   8 +
 include/hw/misc/fixed-clock.h |  30 ++
 include/qemu/qemu-clock.h     | 161 +++++++
 qdev-monitor.c                |   2 +
 qemu-clock.c                  | 176 ++++++++
 11 files changed, 1770 insertions(+)
 create mode 100644 docs/clock.txt
 create mode 100644 hw/misc/fixed-clock.c
 create mode 100644 hw/misc/xilinx_zynqmp_crf.c
 create mode 100644 include/hw/misc/fixed-clock.h
 create mode 100644 include/qemu/qemu-clock.h
 create mode 100644 qemu-clock.c

-- 
1.8.3.1

Ping.

Thanks,
Fred

Le 28/02/2017 à 11:02, fred.konrad@greensocs.com a écrit :
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Hi,
>
> 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.
>
> bus: main-system-bus
>   type System
>   dev: xlnx.zynqmp_crf, id ""
>     gpio-out "sysbus-irq" 1
>     qemu-clk "dbg_trace" 0
>     qemu-clk "dp_stc_ref" 0
>     qemu-clk "dpll_to_lpd" 12500000
>     qemu-clk "acpu_clk" 0
>     qemu-clk "pcie_ref" 0
>     qemu-clk "topsw_main" 0
>     qemu-clk "topsw_lsbus" 0
>     qemu-clk "dp_audio_ref" 0
>     qemu-clk "sata_ref" 0
>     qemu-clk "dp_video_ref" 1428571
>     qemu-clk "vpll_clk" 50000000
>     qemu-clk "apll_to_lpd" 12500000
>     qemu-clk "dpll_clk" 50000000
>     qemu-clk "gpu_ref" 0
>     qemu-clk "aux_refclk" 0
>     qemu-clk "video_clk" 27000000
>     qemu-clk "gdma_ref" 0
>     qemu-clk "gt_crx_ref_clk" 0
>     qemu-clk "dbg_fdp" 0
>     qemu-clk "apll_clk" 50000000
>     qemu-clk "pss_alt_ref_clk" 0
>     qemu-clk "ddr" 0
>     qemu-clk "dbg_tstmp" 0
>     qemu-clk "pss_ref_clk" 50000000
>     qemu-clk "dpdma_ref" 0
>     qemu-clk "vpll_to_lpd" 12500000
>     mmio 00000000fd1a0000/000000000000010c
>
> This series is based on the current master
> (d992f2f1368ceb92e6bfd8efece174110f4236ff).
>
> Thanks,
> Fred
>
> V2 -> V3:
>   * Rebased on current master.
>   * Renamed qemu_clk / QEMUClock as suggested by Cédric.
>   * Renamed in_rate to ref_rate and out_rate to rate.
>   * Renamed qemu_clk_bind_clock to qemu_clk_bind.
>   * Example added to the documentation as suggested by Peter.
>
> V1 -> V2:
>   * Rebased on current master.
>   * Some function renamed and documentation fixed.
>
> RFC -> V1:
>   * Rebased on current master.
>   * The docs has been fixed.
>   * qemu_clk_init_device helper has been provided to ease the initialization
>     of the devices.
>
> KONRAD Frederic (10):
>   qemu-clk: introduce qemu-clk qom object
>   qemu-clk: allow to add a clock to a device
>   qemu-clk: allow to bind two clocks together
>   qemu-clk: introduce an init array to help the device construction
>   qdev-monitor: print the device's clock with info qtree
>   docs: add qemu-clock documentation
>   introduce fixed-clock
>   introduce zynqmp_crf
>   zynqmp: add the zynqmp_crf to the platform
>   zynqmp: add reference clock
>
>  Makefile.objs                 |   1 +
>  docs/clock.txt                | 278 ++++++++++++
>  hw/arm/xlnx-zynqmp.c          |  56 +++
>  hw/misc/Makefile.objs         |   2 +
>  hw/misc/fixed-clock.c         |  88 ++++
>  hw/misc/xilinx_zynqmp_crf.c   | 968 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h  |   8 +
>  include/hw/misc/fixed-clock.h |  30 ++
>  include/qemu/qemu-clock.h     | 161 +++++++
>  qdev-monitor.c                |   2 +
>  qemu-clock.c                  | 176 ++++++++
>  11 files changed, 1770 insertions(+)
>  create mode 100644 docs/clock.txt
>  create mode 100644 hw/misc/fixed-clock.c
>  create mode 100644 hw/misc/xilinx_zynqmp_crf.c
>  create mode 100644 include/hw/misc/fixed-clock.h
>  create mode 100644 include/qemu/qemu-clock.h
>  create mode 100644 qemu-clock.c
>

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

Le 06/06/2017 à 17:18, Peter Maydell a écrit :
> 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
>

Nice thanks for the feedback!

Actually the hot unpluggable is a good question what can we do for that?
Is that reasonable for a device which produce a clock to be hot
unpluggable?

For the migration maybe we can refresh the whole clock tree at the end
of the migration. Is that a good idea?

Fred