Re: [Qemu-devel] [RFC PATCH 09/11] zynqmp_crf: add the clock mechanism

[KONRAD Frederic]

Le 29/07/2016 à 15:51, Peter Maydell a écrit :
> On 13 June 2016 at 17:27,  <address@hidden> wrote:
> > From: KONRAD Frederic <address@hidden>
> >
> > This adds the pll to the zynqmp_crf and the dp_video clock output.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > ---
> >  hw/misc/xilinx_zynqmp_crf.c | 440 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 440 insertions(+)
> >
> > diff --git a/hw/misc/xilinx_zynqmp_crf.c b/hw/misc/xilinx_zynqmp_crf.c
> > index 4c670a0..2097534 100644
> > --- a/hw/misc/xilinx_zynqmp_crf.c
> > +++ b/hw/misc/xilinx_zynqmp_crf.c
> > @@ -30,6 +30,7 @@
> >  #include "hw/register.h"
> >  #include "qemu/bitops.h"
> >  #include "qemu/log.h"
> > +#include "qemu/qemu-clock.h"
> >
> >  #ifndef XILINX_CRF_APB_ERR_DEBUG
> >  #define XILINX_CRF_APB_ERR_DEBUG 0
> > @@ -281,6 +282,38 @@ typedef struct CRF_APB {
> >
> >      uint32_t regs[R_MAX];
> >      RegisterInfo regs_info[R_MAX];
> > +
> > +    /* input clocks */
> > +    qemu_clk pss_ref_clk;
> > +    qemu_clk video_clk;
> > +    qemu_clk pss_alt_ref_clk;
> > +    qemu_clk aux_refclk;
> > +    qemu_clk gt_crx_ref_clk;
> > +
> > +    /* internal clocks */
> > +    qemu_clk apll_clk;
> > +    qemu_clk dpll_clk;
> > +    qemu_clk vpll_clk;
> > +
> > +    /* output clocks */
> > +    qemu_clk acpu_clk;
> > +    qemu_clk dbg_trace;
> > +    qemu_clk dbg_fdp;
> > +    qemu_clk dp_video_ref;
> > +    qemu_clk dp_audio_ref;
> > +    qemu_clk dp_stc_ref;
> > +    qemu_clk ddr;
> > +    qemu_clk gpu_ref;
> > +    qemu_clk sata_ref;
> > +    qemu_clk pcie_ref;
> > +    qemu_clk gdma_ref;
> > +    qemu_clk dpdma_ref;
> > +    qemu_clk topsw_main;
> > +    qemu_clk topsw_lsbus;
> > +    qemu_clk dbg_tstmp;
> > +    qemu_clk apll_to_lpd;
> > +    qemu_clk dpll_to_lpd;
> > +    qemu_clk vpll_to_lpd;
> >  } CRF_APB;
>
> This looks a bit weird. Why are the input clocks and the output
> clocks the same type? I was expecting that an output clock would
> be "owned" by this device (and so a qemu_clk), whereas an input
> clock would just be a reference to a clock owned by the device
> on the other end of it.

Hi Peter,

Yes this is a choice I had to make.
Basically there is nothing different between what we call output and 
input: They both allow to set a callback before the frequency is 
transported to the next clock in the clock tree.
And I use a name to get the clock, eg: for a simple clock divider:

      ----------------
in --o| ClockDivider |o-- out
      ----------------

will be modeled like this in terms of qemu_clock:

    freq_update_cb()
      return rate/4;
          |
          |
         \_/
    ------------                ------------
 --o| qemu_clk |o-- bindings --o| qemu_clk |o--
    |    in    |                |    out   |
    ------------                ------------

Then to bind that to a fixed clock we have in the machine model:
  qemu_clk_bind_clock(
    qemu_clk_get_pin(FixedClock, "out"),
    qemu_clk_get_pin(ClockDivider, "in"));

Thanks,
Fred

> thanks
> -- PMM