[RFC,09/11] zynqmp_crf: add the clock mechanism

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

This adds the pll to the zynqmp_crf and the dp_video clock output.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/misc/xilinx_zynqmp_crf.c | 440 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 440 insertions(+)

On Mon, Jun 13, 2016 at 9:27 AM,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This adds the pll to the zynqmp_crf and the dp_video clock output.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  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;
>
>  static const MemoryRegionOps crf_apb_ops = {
> @@ -325,6 +358,318 @@ static uint64_t ir_disable_prew(RegisterInfo *reg, uint64_t val64)
>      return 0;
>  }
>
> +enum clk_src {
> +    VIDEO_CLK = 4,
> +    PSS_ALT_REF_CLK = 5,
> +    AUX_REF_CLK = 6,
> +    GT_CRX_REF_CLK = 7,
> +    PSS_REF_CLK = 0
> +};
> +
> +static void apll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +
> +    qemu_clk_refresh(s->apll_to_lpd);
> +}
> +
> +static float apll_to_lpd_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    uint32_t divisor = AF_EX32(s->regs, APLL_TO_LPD_CTRL, DIVISOR0);
> +
> +    if (!divisor) {
> +        return 0.0f;
> +    } else {
> +        return input_rate / (float)divisor;
> +    }
> +}
> +
> +static void dpll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +
> +    qemu_clk_refresh(s->dpll_to_lpd);
> +}
> +
> +static float dpll_to_lpd_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    uint32_t divisor = AF_EX32(s->regs, DPLL_TO_LPD_CTRL, DIVISOR0);
> +
> +    if (!divisor) {
> +        return 0.0f;
> +    } else {
> +        return input_rate / (float)divisor;
> +    }
> +}
> +
> +static void vpll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +
> +    qemu_clk_refresh(s->vpll_to_lpd);
> +}
> +
> +static float vpll_to_lpd_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    uint32_t divisor = AF_EX32(s->regs, VPLL_TO_LPD_CTRL, DIVISOR0);
> +
> +    if (!divisor) {
> +        return 0.0f;
> +    } else {
> +        return input_rate / (float)divisor;
> +    }
> +}
> +
> +static void apll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +    uint32_t source = AF_EX32(s->regs, APLL_CTRL, BYPASS)
> +                    ? AF_EX32(s->regs, APLL_CTRL, POST_SRC)
> +                    : AF_EX32(s->regs, APLL_CTRL, PRE_SRC);
> +
> +    /*
> +     * We must ensure that only one clock is bound to the apll internal clock.
> +     */
> +    qemu_clk_unbound(s->pss_ref_clk, s->apll_clk);
> +    qemu_clk_unbound(s->video_clk, s->apll_clk);
> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->apll_clk);
> +    qemu_clk_unbound(s->aux_refclk, s->apll_clk);
> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->apll_clk);
> +
> +    switch (source) {
> +    case VIDEO_CLK:
> +        qemu_clk_bound_clock(s->video_clk, s->apll_clk);
> +        break;
> +    case PSS_ALT_REF_CLK:
> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->apll_clk);
> +        break;
> +    case AUX_REF_CLK:
> +        qemu_clk_bound_clock(s->aux_refclk, s->apll_clk);
> +        break;
> +    case GT_CRX_REF_CLK:
> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->apll_clk);
> +        break;
> +    default:
> +        qemu_clk_bound_clock(s->pss_ref_clk, s->apll_clk);
> +        break;
> +    }
> +}
> +
> +static void dpll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +    uint32_t source = AF_EX32(s->regs, DPLL_CTRL, BYPASS)
> +                    ? AF_EX32(s->regs, DPLL_CTRL, POST_SRC)
> +                    : AF_EX32(s->regs, DPLL_CTRL, PRE_SRC);
> +
> +    /*
> +     * We must ensure that only one clock is bound to the dpll internal clock.
> +     */
> +    qemu_clk_unbound(s->pss_ref_clk, s->dpll_clk);
> +    qemu_clk_unbound(s->video_clk, s->dpll_clk);
> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->dpll_clk);
> +    qemu_clk_unbound(s->aux_refclk, s->dpll_clk);
> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->dpll_clk);
> +
> +    switch (source) {
> +    case VIDEO_CLK:
> +        qemu_clk_bound_clock(s->video_clk, s->dpll_clk);
> +        break;
> +    case PSS_ALT_REF_CLK:
> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->dpll_clk);
> +        break;
> +    case AUX_REF_CLK:
> +        qemu_clk_bound_clock(s->aux_refclk, s->dpll_clk);
> +        break;
> +    case GT_CRX_REF_CLK:
> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->dpll_clk);
> +        break;
> +    default:
> +        qemu_clk_bound_clock(s->pss_ref_clk, s->dpll_clk);
> +        break;
> +    }
> +}
> +
> +static void vpll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +    uint32_t source = AF_EX32(s->regs, VPLL_CTRL, BYPASS)
> +                    ? AF_EX32(s->regs, VPLL_CTRL, POST_SRC)
> +                    : AF_EX32(s->regs, VPLL_CTRL, PRE_SRC);
> +
> +    /*
> +     * We must ensure that only one clock is bound to the vpll internal clock.
> +     */
> +    qemu_clk_unbound(s->pss_ref_clk, s->vpll_clk);
> +    qemu_clk_unbound(s->video_clk, s->vpll_clk);
> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->vpll_clk);
> +    qemu_clk_unbound(s->aux_refclk, s->vpll_clk);
> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->vpll_clk);
> +
> +    switch (source) {
> +    case VIDEO_CLK:
> +        qemu_clk_bound_clock(s->video_clk, s->vpll_clk);
> +        break;
> +    case PSS_ALT_REF_CLK:
> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->vpll_clk);
> +        break;
> +    case AUX_REF_CLK:
> +        qemu_clk_bound_clock(s->aux_refclk, s->vpll_clk);
> +        break;
> +    case GT_CRX_REF_CLK:
> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->vpll_clk);
> +        break;
> +    default:
> +        qemu_clk_bound_clock(s->pss_ref_clk, s->vpll_clk);
> +        break;
> +    }
> +}

These functions seem very repititive. Can we have one general function
that gets passed the applicable clock into it?

> +
> +/*
> + * This happen when apll get updated.
> + * As we ensure that only one clk_pin can drive apll we can just do the
> + * computation from input_rate.
> + */
> +static float apll_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    bool bypass = AF_EX32(s->regs, APLL_CTRL, BYPASS);
> +    bool reset = AF_EX32(s->regs, APLL_CTRL, RESET);
> +    float div2 = AF_EX32(s->regs, APLL_CTRL, DIV2) ? 0.5f : 1.0f;
> +    float integer = (float)(AF_EX32(s->regs, APLL_CTRL, FBDIV));
> +    float frac = AF_EX32(s->regs, APLL_FRAC_CFG, ENABLED)
> +                  ? (float)(AF_EX32(s->regs, APLL_FRAC_CFG, DATA)) / 65536.0f
> +                  : 0.0f;
> +
> +    if (bypass) {
> +        return input_rate;
> +    } else {
> +        if (reset) {
> +            /*
> +             * This is not supposed to happen user must ensure that BYPASS is
> +             * set before the PLL are reset.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "APLL is reseted but not bypassed.");
> +            return 0.0f;
> +        } else {
> +            return input_rate * div2 * (integer + frac);
> +        }
> +    }
> +}
> +
> +/*
> + * This happen when dpll get updated.
> + * As we ensure that only one clk_pin can drive dpll we can just do the
> + * computation from input_rate.
> + */
> +static float dpll_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    bool bypass = AF_EX32(s->regs, DPLL_CTRL, BYPASS);
> +    bool reset = AF_EX32(s->regs, DPLL_CTRL, RESET);
> +    float div2 = AF_EX32(s->regs, DPLL_CTRL, DIV2) ? 0.5f : 1.0f;
> +    float integer = (float)(AF_EX32(s->regs, DPLL_CTRL, FBDIV));
> +    float frac = AF_EX32(s->regs, DPLL_FRAC_CFG, ENABLED)
> +                  ? (float)(AF_EX32(s->regs, DPLL_FRAC_CFG, DATA)) / 65536.0f
> +                  : 0.0f;
> +
> +    if (bypass) {
> +        return input_rate;
> +    } else {
> +        if (reset) {
> +            /*
> +             * This is not supposed to happen user must ensure that BYPASS is
> +             * set before the PLL are reset.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "DPLL is reseted but not bypassed.");
> +            return 0.0f;
> +        } else {
> +            return input_rate * div2 * (integer + frac);
> +        }
> +    }
> +}
> +
> +/*
> + * This happen when vpll get updated.
> + * As we ensure that only one clk_pin can drive vpll we can just do the
> + * computation from input_rate.
> + */
> +static float vpll_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    bool bypass = AF_EX32(s->regs, VPLL_CTRL, BYPASS);
> +    bool reset = AF_EX32(s->regs, VPLL_CTRL, RESET);
> +    float div2 = AF_EX32(s->regs, VPLL_CTRL, DIV2) ? 0.5f : 1.0f;
> +    float integer = (float)(AF_EX32(s->regs, VPLL_CTRL, FBDIV));
> +    float frac = AF_EX32(s->regs, VPLL_FRAC_CFG, ENABLED)
> +                  ? (float)(AF_EX32(s->regs, VPLL_FRAC_CFG, DATA)) / 65536.0f
> +                  : 0.0f;
> +
> +    if (bypass) {
> +        return input_rate;
> +    } else {
> +        if (reset) {
> +            /*
> +             * This is not supposed to happen user must ensure that BYPASS is
> +             * set before the PLL are reset.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "VPLL is reseted but not bypassed.");
> +            return 0.0f;
> +        } else {
> +            return input_rate * div2 * (integer + frac);
> +        }
> +    }
> +}
> +
> +/*
> + * FIXME: Only DP video reference clock is modeled here, others are missing.
> + */
> +static float dp_video_update_rate(void *opaque, float input_rate)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(opaque);
> +    bool clock_act = AF_EX32(s->regs, DP_VIDEO_REF_CTRL, CLKACT);
> +    uint32_t divisor0 = AF_EX32(s->regs, DP_VIDEO_REF_CTRL, DIVISOR0);
> +
> +    if ((!divisor0) || (!clock_act)) {
> +        return 0.0f;
> +    } else {
> +        return input_rate / (float)(divisor0);
> +    }
> +}
> +
> +static void dp_video_ref_ctrl_postw(RegisterInfo *reg, uint64_t val64)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
> +    uint32_t source = AF_EX32(s->regs, APLL_CTRL, BYPASS)
> +                    ? AF_EX32(s->regs, APLL_CTRL, POST_SRC)
> +                    : AF_EX32(s->regs, APLL_CTRL, PRE_SRC);
> +
> +    /*
> +     * We must ensure that only one clock is bound to the dp_video_ref
> +     * internal clock, so the callback have always the right rate in it.
> +     */
> +    qemu_clk_unbound(s->vpll_clk, s->dp_video_ref);
> +    qemu_clk_unbound(s->dpll_clk, s->dp_video_ref);
> +
> +    switch (source) {
> +    case 0x00:
> +        qemu_clk_bound_clock(s->vpll_clk, s->dp_video_ref);
> +        break;
> +    case 0x02:
> +        qemu_clk_bound_clock(s->dpll_clk, s->dp_video_ref);
> +        break;
> +    default:
> +        abort();
> +        break;
> +    }
> +}

Same comment about a general function here.

> +
>  static RegisterAccessInfo crf_apb_regs_info[] = {
>      {   .name = "ERR_CTRL",  .decode.addr = A_ERR_CTRL,
>      },{ .name = "IR_STATUS",  .decode.addr = A_IR_STATUS,
> @@ -341,6 +686,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>      },{ .name = "APLL_CTRL",  .decode.addr = A_APLL_CTRL,
>          .reset = 0x2809,
>          .rsvd = 0xf88c80f6L,
> +        .post_write = apll_ctrl_postw,
>      },{ .name = "APLL_CFG",  .decode.addr = A_APLL_CFG,
>          .rsvd = 0x1801210,
>      },{ .name = "APLL_FRAC_CFG",  .decode.addr = A_APLL_FRAC_CFG,
> @@ -348,6 +694,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>      },{ .name = "DPLL_CTRL",  .decode.addr = A_DPLL_CTRL,
>          .reset = 0x2809,
>          .rsvd = 0xf88c80f6L,
> +        .post_write = dpll_ctrl_postw,
>      },{ .name = "DPLL_CFG",  .decode.addr = A_DPLL_CFG,
>          .rsvd = 0x1801210,
>      },{ .name = "DPLL_FRAC_CFG",  .decode.addr = A_DPLL_FRAC_CFG,
> @@ -355,6 +702,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>      },{ .name = "VPLL_CTRL",  .decode.addr = A_VPLL_CTRL,
>          .reset = 0x2809,
>          .rsvd = 0xf88c80f6L,
> +        .post_write = vpll_ctrl_postw,
>      },{ .name = "VPLL_CFG",  .decode.addr = A_VPLL_CFG,
>          .rsvd = 0x1801210,
>      },{ .name = "VPLL_FRAC_CFG",  .decode.addr = A_VPLL_FRAC_CFG,
> @@ -366,12 +714,15 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>      },{ .name = "APLL_TO_LPD_CTRL",  .decode.addr = A_APLL_TO_LPD_CTRL,
>          .reset = 0x400,
>          .rsvd = 0xc0ff,
> +        .post_write = apll_to_lpd_postw,
>      },{ .name = "DPLL_TO_LPD_CTRL",  .decode.addr = A_DPLL_TO_LPD_CTRL,
>          .reset = 0x400,
>          .rsvd = 0xc0ff,
> +        .post_write = dpll_to_lpd_postw,
>      },{ .name = "VPLL_TO_LPD_CTRL",  .decode.addr = A_VPLL_TO_LPD_CTRL,
>          .reset = 0x400,
>          .rsvd = 0xc0ff,
> +        .post_write = vpll_to_lpd_postw,
>      },{ .name = "CPU_A9_CTRL",  .decode.addr = A_CPU_A9_CTRL,
>          .reset = 0xf000400,
>          .rsvd = 0xf0ffc0f8L,
> @@ -384,6 +735,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>      },{ .name = "DP_VIDEO_REF_CTRL",  .decode.addr = A_DP_VIDEO_REF_CTRL,
>          .reset = 0x1002300,
>          .rsvd = 0xfeffc0f8L,
> +        .post_write = dp_video_ref_ctrl_postw,
>      },{ .name = "DP_AUDIO_REF_CTRL",  .decode.addr = A_DP_AUDIO_REF_CTRL,
>          .reset = 0x1002300,
>          .rsvd = 0xfeffc0f8L,
> @@ -479,6 +831,25 @@ static void crf_apb_reset(DeviceState *dev)
>      }
>
>      ir_update_irq(s);
> +
> +    /*
> +     * During reset, the clock selection registers bound the clock like this.
> +     */
> +    qemu_clk_bound_clock(s->pss_ref_clk, s->apll_clk);
> +    qemu_clk_bound_clock(s->pss_ref_clk, s->dpll_clk);
> +    qemu_clk_bound_clock(s->pss_ref_clk, s->vpll_clk);
> +    qemu_clk_bound_clock(s->vpll_clk, s->dp_video_ref);

What if these are already bound?

> +}
> +
> +static void crf_apb_realize(DeviceState *d, Error **errp)
> +{
> +    CRF_APB *s = XILINX_CRF_APB(d);
> +
> +    qemu_clk_bound_clock(s->apll_clk, s->apll_to_lpd);
> +    qemu_clk_bound_clock(s->dpll_clk, s->dpll_to_lpd);
> +    qemu_clk_bound_clock(s->vpll_clk, s->vpll_to_lpd);
> +
> +    crf_apb_reset(d);

Don't call the reset from realise.

>  }
>
>  static void crf_apb_init(Object *obj)
> @@ -495,6 +866,74 @@ static void crf_apb_init(Object *obj)
>                            R_RST_DDR_SS);
>      sysbus_init_mmio(sbd, &s->iomem);
>      sysbus_init_irq(sbd, &s->irq_ir);
> +
> +    /* input clocks */
> +    s->pss_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->pss_ref_clk, "pss_ref_clk");
> +    s->video_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->video_clk, "video_clk");
> +    s->pss_alt_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->pss_alt_ref_clk,
> +                              "pss_alt_ref_clk");
> +    s->aux_refclk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->aux_refclk, "aux_refclk");
> +    s->gt_crx_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->gt_crx_ref_clk,
> +                              "gt_crx_ref_clk");
> +
> +    /* internal clocks */
> +    s->apll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->apll_clk, "apll_clk");
> +    qemu_clk_set_callback(s->apll_clk, apll_update_rate, obj);
> +    s->dpll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpll_clk, "dpll_clk");
> +    qemu_clk_set_callback(s->dpll_clk, dpll_update_rate, obj);
> +    s->vpll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->vpll_clk, "vpll_clk");
> +    qemu_clk_set_callback(s->vpll_clk, vpll_update_rate, obj);
> +
> +    /* Clock output init */
> +    s->acpu_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->acpu_clk, "acpu_clk");
> +    s->dbg_trace = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_trace, "dbg_trace");
> +    s->dbg_fdp = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_fdp, "dbg_fdp");
> +    s->dp_video_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_video_ref, "dp_video_ref");
> +    qemu_clk_set_callback(s->dp_video_ref, dp_video_update_rate, obj);
> +    s->dp_audio_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_audio_ref, "dp_audio_ref");
> +    s->dp_stc_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_stc_ref, "dp_stc_ref");
> +    s->ddr = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->ddr, "ddr");
> +    s->gpu_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->gpu_ref, "gpu_ref");
> +    s->sata_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->sata_ref, "sata_ref");
> +    s->pcie_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->pcie_ref, "pcie_ref");
> +    s->gdma_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->gdma_ref, "gdma_ref");
> +    s->dpdma_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpdma_ref, "dpdma_ref");
> +    s->topsw_main = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->topsw_main, "topsw_main");
> +    s->topsw_lsbus = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->topsw_lsbus, "topsw_lsbus");
> +    s->dbg_tstmp = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_tstmp, "dbg_tstmp");
> +
> +    s->apll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->apll_to_lpd, "apll_to_lpd");
> +    qemu_clk_set_callback(s->apll_to_lpd, apll_to_lpd_update_rate, obj);
> +    s->dpll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpll_to_lpd, "dpll_to_lpd");
> +    qemu_clk_set_callback(s->dpll_to_lpd, dpll_to_lpd_update_rate, obj);
> +    s->vpll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
> +    qemu_clk_attach_to_device(DEVICE(obj), s->vpll_to_lpd, "vpll_to_lpd");
> +    qemu_clk_set_callback(s->vpll_to_lpd, vpll_to_lpd_update_rate, obj);

Wow! That is a lot of code

Maybe we would be better off having an array (or three for the
sections) and iterating over those. Between this init code and the
registers in the struct this is getting hard to read/manage.

Thanks,

Alistair

>  }
>
>  static const VMStateDescription vmstate_crf_apb = {
> @@ -514,6 +953,7 @@ static void crf_apb_class_init(ObjectClass *klass, void *data)
>
>      dc->reset = crf_apb_reset;
>      dc->vmsd = &vmstate_crf_apb;
> +    dc->realize = crf_apb_realize;
>  }
>
>  static const TypeInfo crf_apb_info = {
> --
> 2.5.5
>
>

On 13 June 2016 at 17:27,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This adds the pll to the zynqmp_crf and the dp_video clock output.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  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.

thanks
-- PMM

Le 02/07/2016 à 01:23, Alistair Francis a écrit :
> On Mon, Jun 13, 2016 at 9:27 AM,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This adds the pll to the zynqmp_crf and the dp_video clock output.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   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;
>>
>>   static const MemoryRegionOps crf_apb_ops = {
>> @@ -325,6 +358,318 @@ static uint64_t ir_disable_prew(RegisterInfo *reg, uint64_t val64)
>>       return 0;
>>   }
>>
>> +enum clk_src {
>> +    VIDEO_CLK = 4,
>> +    PSS_ALT_REF_CLK = 5,
>> +    AUX_REF_CLK = 6,
>> +    GT_CRX_REF_CLK = 7,
>> +    PSS_REF_CLK = 0
>> +};
>> +
>> +static void apll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +
>> +    qemu_clk_refresh(s->apll_to_lpd);
>> +}
>> +
>> +static float apll_to_lpd_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    uint32_t divisor = AF_EX32(s->regs, APLL_TO_LPD_CTRL, DIVISOR0);
>> +
>> +    if (!divisor) {
>> +        return 0.0f;
>> +    } else {
>> +        return input_rate / (float)divisor;
>> +    }
>> +}
>> +
>> +static void dpll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +
>> +    qemu_clk_refresh(s->dpll_to_lpd);
>> +}
>> +
>> +static float dpll_to_lpd_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    uint32_t divisor = AF_EX32(s->regs, DPLL_TO_LPD_CTRL, DIVISOR0);
>> +
>> +    if (!divisor) {
>> +        return 0.0f;
>> +    } else {
>> +        return input_rate / (float)divisor;
>> +    }
>> +}
>> +
>> +static void vpll_to_lpd_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +
>> +    qemu_clk_refresh(s->vpll_to_lpd);
>> +}
>> +
>> +static float vpll_to_lpd_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    uint32_t divisor = AF_EX32(s->regs, VPLL_TO_LPD_CTRL, DIVISOR0);
>> +
>> +    if (!divisor) {
>> +        return 0.0f;
>> +    } else {
>> +        return input_rate / (float)divisor;
>> +    }
>> +}
>> +
>> +static void apll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +    uint32_t source = AF_EX32(s->regs, APLL_CTRL, BYPASS)
>> +                    ? AF_EX32(s->regs, APLL_CTRL, POST_SRC)
>> +                    : AF_EX32(s->regs, APLL_CTRL, PRE_SRC);
>> +
>> +    /*
>> +     * We must ensure that only one clock is bound to the apll internal clock.
>> +     */
>> +    qemu_clk_unbound(s->pss_ref_clk, s->apll_clk);
>> +    qemu_clk_unbound(s->video_clk, s->apll_clk);
>> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->apll_clk);
>> +    qemu_clk_unbound(s->aux_refclk, s->apll_clk);
>> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->apll_clk);
>> +
>> +    switch (source) {
>> +    case VIDEO_CLK:
>> +        qemu_clk_bound_clock(s->video_clk, s->apll_clk);
>> +        break;
>> +    case PSS_ALT_REF_CLK:
>> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->apll_clk);
>> +        break;
>> +    case AUX_REF_CLK:
>> +        qemu_clk_bound_clock(s->aux_refclk, s->apll_clk);
>> +        break;
>> +    case GT_CRX_REF_CLK:
>> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->apll_clk);
>> +        break;
>> +    default:
>> +        qemu_clk_bound_clock(s->pss_ref_clk, s->apll_clk);
>> +        break;
>> +    }
>> +}
>> +
>> +static void dpll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +    uint32_t source = AF_EX32(s->regs, DPLL_CTRL, BYPASS)
>> +                    ? AF_EX32(s->regs, DPLL_CTRL, POST_SRC)
>> +                    : AF_EX32(s->regs, DPLL_CTRL, PRE_SRC);
>> +
>> +    /*
>> +     * We must ensure that only one clock is bound to the dpll internal clock.
>> +     */
>> +    qemu_clk_unbound(s->pss_ref_clk, s->dpll_clk);
>> +    qemu_clk_unbound(s->video_clk, s->dpll_clk);
>> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->dpll_clk);
>> +    qemu_clk_unbound(s->aux_refclk, s->dpll_clk);
>> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->dpll_clk);
>> +
>> +    switch (source) {
>> +    case VIDEO_CLK:
>> +        qemu_clk_bound_clock(s->video_clk, s->dpll_clk);
>> +        break;
>> +    case PSS_ALT_REF_CLK:
>> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->dpll_clk);
>> +        break;
>> +    case AUX_REF_CLK:
>> +        qemu_clk_bound_clock(s->aux_refclk, s->dpll_clk);
>> +        break;
>> +    case GT_CRX_REF_CLK:
>> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->dpll_clk);
>> +        break;
>> +    default:
>> +        qemu_clk_bound_clock(s->pss_ref_clk, s->dpll_clk);
>> +        break;
>> +    }
>> +}
>> +
>> +static void vpll_ctrl_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +    uint32_t source = AF_EX32(s->regs, VPLL_CTRL, BYPASS)
>> +                    ? AF_EX32(s->regs, VPLL_CTRL, POST_SRC)
>> +                    : AF_EX32(s->regs, VPLL_CTRL, PRE_SRC);
>> +
>> +    /*
>> +     * We must ensure that only one clock is bound to the vpll internal clock.
>> +     */
>> +    qemu_clk_unbound(s->pss_ref_clk, s->vpll_clk);
>> +    qemu_clk_unbound(s->video_clk, s->vpll_clk);
>> +    qemu_clk_unbound(s->pss_alt_ref_clk, s->vpll_clk);
>> +    qemu_clk_unbound(s->aux_refclk, s->vpll_clk);
>> +    qemu_clk_unbound(s->gt_crx_ref_clk, s->vpll_clk);
>> +
>> +    switch (source) {
>> +    case VIDEO_CLK:
>> +        qemu_clk_bound_clock(s->video_clk, s->vpll_clk);
>> +        break;
>> +    case PSS_ALT_REF_CLK:
>> +        qemu_clk_bound_clock(s->pss_alt_ref_clk, s->vpll_clk);
>> +        break;
>> +    case AUX_REF_CLK:
>> +        qemu_clk_bound_clock(s->aux_refclk, s->vpll_clk);
>> +        break;
>> +    case GT_CRX_REF_CLK:
>> +        qemu_clk_bound_clock(s->gt_crx_ref_clk, s->vpll_clk);
>> +        break;
>> +    default:
>> +        qemu_clk_bound_clock(s->pss_ref_clk, s->vpll_clk);
>> +        break;
>> +    }
>> +}
> These functions seem very repititive. Can we have one general function
> that gets passed the applicable clock into it?

Yes I think we can do that.

>
>> +
>> +/*
>> + * This happen when apll get updated.
>> + * As we ensure that only one clk_pin can drive apll we can just do the
>> + * computation from input_rate.
>> + */
>> +static float apll_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    bool bypass = AF_EX32(s->regs, APLL_CTRL, BYPASS);
>> +    bool reset = AF_EX32(s->regs, APLL_CTRL, RESET);
>> +    float div2 = AF_EX32(s->regs, APLL_CTRL, DIV2) ? 0.5f : 1.0f;
>> +    float integer = (float)(AF_EX32(s->regs, APLL_CTRL, FBDIV));
>> +    float frac = AF_EX32(s->regs, APLL_FRAC_CFG, ENABLED)
>> +                  ? (float)(AF_EX32(s->regs, APLL_FRAC_CFG, DATA)) / 65536.0f
>> +                  : 0.0f;
>> +
>> +    if (bypass) {
>> +        return input_rate;
>> +    } else {
>> +        if (reset) {
>> +            /*
>> +             * This is not supposed to happen user must ensure that BYPASS is
>> +             * set before the PLL are reset.
>> +             */
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "APLL is reseted but not bypassed.");
>> +            return 0.0f;
>> +        } else {
>> +            return input_rate * div2 * (integer + frac);
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * This happen when dpll get updated.
>> + * As we ensure that only one clk_pin can drive dpll we can just do the
>> + * computation from input_rate.
>> + */
>> +static float dpll_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    bool bypass = AF_EX32(s->regs, DPLL_CTRL, BYPASS);
>> +    bool reset = AF_EX32(s->regs, DPLL_CTRL, RESET);
>> +    float div2 = AF_EX32(s->regs, DPLL_CTRL, DIV2) ? 0.5f : 1.0f;
>> +    float integer = (float)(AF_EX32(s->regs, DPLL_CTRL, FBDIV));
>> +    float frac = AF_EX32(s->regs, DPLL_FRAC_CFG, ENABLED)
>> +                  ? (float)(AF_EX32(s->regs, DPLL_FRAC_CFG, DATA)) / 65536.0f
>> +                  : 0.0f;
>> +
>> +    if (bypass) {
>> +        return input_rate;
>> +    } else {
>> +        if (reset) {
>> +            /*
>> +             * This is not supposed to happen user must ensure that BYPASS is
>> +             * set before the PLL are reset.
>> +             */
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "DPLL is reseted but not bypassed.");
>> +            return 0.0f;
>> +        } else {
>> +            return input_rate * div2 * (integer + frac);
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * This happen when vpll get updated.
>> + * As we ensure that only one clk_pin can drive vpll we can just do the
>> + * computation from input_rate.
>> + */
>> +static float vpll_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    bool bypass = AF_EX32(s->regs, VPLL_CTRL, BYPASS);
>> +    bool reset = AF_EX32(s->regs, VPLL_CTRL, RESET);
>> +    float div2 = AF_EX32(s->regs, VPLL_CTRL, DIV2) ? 0.5f : 1.0f;
>> +    float integer = (float)(AF_EX32(s->regs, VPLL_CTRL, FBDIV));
>> +    float frac = AF_EX32(s->regs, VPLL_FRAC_CFG, ENABLED)
>> +                  ? (float)(AF_EX32(s->regs, VPLL_FRAC_CFG, DATA)) / 65536.0f
>> +                  : 0.0f;
>> +
>> +    if (bypass) {
>> +        return input_rate;
>> +    } else {
>> +        if (reset) {
>> +            /*
>> +             * This is not supposed to happen user must ensure that BYPASS is
>> +             * set before the PLL are reset.
>> +             */
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "VPLL is reseted but not bypassed.");
>> +            return 0.0f;
>> +        } else {
>> +            return input_rate * div2 * (integer + frac);
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * FIXME: Only DP video reference clock is modeled here, others are missing.
>> + */
>> +static float dp_video_update_rate(void *opaque, float input_rate)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(opaque);
>> +    bool clock_act = AF_EX32(s->regs, DP_VIDEO_REF_CTRL, CLKACT);
>> +    uint32_t divisor0 = AF_EX32(s->regs, DP_VIDEO_REF_CTRL, DIVISOR0);
>> +
>> +    if ((!divisor0) || (!clock_act)) {
>> +        return 0.0f;
>> +    } else {
>> +        return input_rate / (float)(divisor0);
>> +    }
>> +}
>> +
>> +static void dp_video_ref_ctrl_postw(RegisterInfo *reg, uint64_t val64)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(reg->opaque);
>> +    uint32_t source = AF_EX32(s->regs, APLL_CTRL, BYPASS)
>> +                    ? AF_EX32(s->regs, APLL_CTRL, POST_SRC)
>> +                    : AF_EX32(s->regs, APLL_CTRL, PRE_SRC);
>> +
>> +    /*
>> +     * We must ensure that only one clock is bound to the dp_video_ref
>> +     * internal clock, so the callback have always the right rate in it.
>> +     */
>> +    qemu_clk_unbound(s->vpll_clk, s->dp_video_ref);
>> +    qemu_clk_unbound(s->dpll_clk, s->dp_video_ref);
>> +
>> +    switch (source) {
>> +    case 0x00:
>> +        qemu_clk_bound_clock(s->vpll_clk, s->dp_video_ref);
>> +        break;
>> +    case 0x02:
>> +        qemu_clk_bound_clock(s->dpll_clk, s->dp_video_ref);
>> +        break;
>> +    default:
>> +        abort();
>> +        break;
>> +    }
>> +}
> Same comment about a general function here.
>
>> +
>>   static RegisterAccessInfo crf_apb_regs_info[] = {
>>       {   .name = "ERR_CTRL",  .decode.addr = A_ERR_CTRL,
>>       },{ .name = "IR_STATUS",  .decode.addr = A_IR_STATUS,
>> @@ -341,6 +686,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>>       },{ .name = "APLL_CTRL",  .decode.addr = A_APLL_CTRL,
>>           .reset = 0x2809,
>>           .rsvd = 0xf88c80f6L,
>> +        .post_write = apll_ctrl_postw,
>>       },{ .name = "APLL_CFG",  .decode.addr = A_APLL_CFG,
>>           .rsvd = 0x1801210,
>>       },{ .name = "APLL_FRAC_CFG",  .decode.addr = A_APLL_FRAC_CFG,
>> @@ -348,6 +694,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>>       },{ .name = "DPLL_CTRL",  .decode.addr = A_DPLL_CTRL,
>>           .reset = 0x2809,
>>           .rsvd = 0xf88c80f6L,
>> +        .post_write = dpll_ctrl_postw,
>>       },{ .name = "DPLL_CFG",  .decode.addr = A_DPLL_CFG,
>>           .rsvd = 0x1801210,
>>       },{ .name = "DPLL_FRAC_CFG",  .decode.addr = A_DPLL_FRAC_CFG,
>> @@ -355,6 +702,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>>       },{ .name = "VPLL_CTRL",  .decode.addr = A_VPLL_CTRL,
>>           .reset = 0x2809,
>>           .rsvd = 0xf88c80f6L,
>> +        .post_write = vpll_ctrl_postw,
>>       },{ .name = "VPLL_CFG",  .decode.addr = A_VPLL_CFG,
>>           .rsvd = 0x1801210,
>>       },{ .name = "VPLL_FRAC_CFG",  .decode.addr = A_VPLL_FRAC_CFG,
>> @@ -366,12 +714,15 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>>       },{ .name = "APLL_TO_LPD_CTRL",  .decode.addr = A_APLL_TO_LPD_CTRL,
>>           .reset = 0x400,
>>           .rsvd = 0xc0ff,
>> +        .post_write = apll_to_lpd_postw,
>>       },{ .name = "DPLL_TO_LPD_CTRL",  .decode.addr = A_DPLL_TO_LPD_CTRL,
>>           .reset = 0x400,
>>           .rsvd = 0xc0ff,
>> +        .post_write = dpll_to_lpd_postw,
>>       },{ .name = "VPLL_TO_LPD_CTRL",  .decode.addr = A_VPLL_TO_LPD_CTRL,
>>           .reset = 0x400,
>>           .rsvd = 0xc0ff,
>> +        .post_write = vpll_to_lpd_postw,
>>       },{ .name = "CPU_A9_CTRL",  .decode.addr = A_CPU_A9_CTRL,
>>           .reset = 0xf000400,
>>           .rsvd = 0xf0ffc0f8L,
>> @@ -384,6 +735,7 @@ static RegisterAccessInfo crf_apb_regs_info[] = {
>>       },{ .name = "DP_VIDEO_REF_CTRL",  .decode.addr = A_DP_VIDEO_REF_CTRL,
>>           .reset = 0x1002300,
>>           .rsvd = 0xfeffc0f8L,
>> +        .post_write = dp_video_ref_ctrl_postw,
>>       },{ .name = "DP_AUDIO_REF_CTRL",  .decode.addr = A_DP_AUDIO_REF_CTRL,
>>           .reset = 0x1002300,
>>           .rsvd = 0xfeffc0f8L,
>> @@ -479,6 +831,25 @@ static void crf_apb_reset(DeviceState *dev)
>>       }
>>
>>       ir_update_irq(s);
>> +
>> +    /*
>> +     * During reset, the clock selection registers bound the clock like this.
>> +     */
>> +    qemu_clk_bound_clock(s->pss_ref_clk, s->apll_clk);
>> +    qemu_clk_bound_clock(s->pss_ref_clk, s->dpll_clk);
>> +    qemu_clk_bound_clock(s->pss_ref_clk, s->vpll_clk);
>> +    qemu_clk_bound_clock(s->vpll_clk, s->dp_video_ref);
> What if these are already bound?

True. I think we better do something like:
when we bind clock we drop the old connection as I said in the previous 
commented patch.

>
>> +}
>> +
>> +static void crf_apb_realize(DeviceState *d, Error **errp)
>> +{
>> +    CRF_APB *s = XILINX_CRF_APB(d);
>> +
>> +    qemu_clk_bound_clock(s->apll_clk, s->apll_to_lpd);
>> +    qemu_clk_bound_clock(s->dpll_clk, s->dpll_to_lpd);
>> +    qemu_clk_bound_clock(s->vpll_clk, s->vpll_to_lpd);
>> +
>> +    crf_apb_reset(d);
> Don't call the reset from realise.
>
>>   }
>>
>>   static void crf_apb_init(Object *obj)
>> @@ -495,6 +866,74 @@ static void crf_apb_init(Object *obj)
>>                             R_RST_DDR_SS);
>>       sysbus_init_mmio(sbd, &s->iomem);
>>       sysbus_init_irq(sbd, &s->irq_ir);
>> +
>> +    /* input clocks */
>> +    s->pss_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->pss_ref_clk, "pss_ref_clk");
>> +    s->video_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->video_clk, "video_clk");
>> +    s->pss_alt_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->pss_alt_ref_clk,
>> +                              "pss_alt_ref_clk");
>> +    s->aux_refclk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->aux_refclk, "aux_refclk");
>> +    s->gt_crx_ref_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->gt_crx_ref_clk,
>> +                              "gt_crx_ref_clk");
>> +
>> +    /* internal clocks */
>> +    s->apll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->apll_clk, "apll_clk");
>> +    qemu_clk_set_callback(s->apll_clk, apll_update_rate, obj);
>> +    s->dpll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpll_clk, "dpll_clk");
>> +    qemu_clk_set_callback(s->dpll_clk, dpll_update_rate, obj);
>> +    s->vpll_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->vpll_clk, "vpll_clk");
>> +    qemu_clk_set_callback(s->vpll_clk, vpll_update_rate, obj);
>> +
>> +    /* Clock output init */
>> +    s->acpu_clk = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->acpu_clk, "acpu_clk");
>> +    s->dbg_trace = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_trace, "dbg_trace");
>> +    s->dbg_fdp = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_fdp, "dbg_fdp");
>> +    s->dp_video_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_video_ref, "dp_video_ref");
>> +    qemu_clk_set_callback(s->dp_video_ref, dp_video_update_rate, obj);
>> +    s->dp_audio_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_audio_ref, "dp_audio_ref");
>> +    s->dp_stc_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dp_stc_ref, "dp_stc_ref");
>> +    s->ddr = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->ddr, "ddr");
>> +    s->gpu_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->gpu_ref, "gpu_ref");
>> +    s->sata_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->sata_ref, "sata_ref");
>> +    s->pcie_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->pcie_ref, "pcie_ref");
>> +    s->gdma_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->gdma_ref, "gdma_ref");
>> +    s->dpdma_ref = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpdma_ref, "dpdma_ref");
>> +    s->topsw_main = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->topsw_main, "topsw_main");
>> +    s->topsw_lsbus = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->topsw_lsbus, "topsw_lsbus");
>> +    s->dbg_tstmp = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dbg_tstmp, "dbg_tstmp");
>> +
>> +    s->apll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->apll_to_lpd, "apll_to_lpd");
>> +    qemu_clk_set_callback(s->apll_to_lpd, apll_to_lpd_update_rate, obj);
>> +    s->dpll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->dpll_to_lpd, "dpll_to_lpd");
>> +    qemu_clk_set_callback(s->dpll_to_lpd, dpll_to_lpd_update_rate, obj);
>> +    s->vpll_to_lpd = QEMU_CLOCK(object_new(TYPE_CLOCK));
>> +    qemu_clk_attach_to_device(DEVICE(obj), s->vpll_to_lpd, "vpll_to_lpd");
>> +    qemu_clk_set_callback(s->vpll_to_lpd, vpll_to_lpd_update_rate, obj);
> Wow! That is a lot of code
>
> Maybe we would be better off having an array (or three for the
> sections) and iterating over those. Between this init code and the
> registers in the struct this is getting hard to read/manage.

True, there are 26 clocks though :).

We might want an array as you suggested and add it to the framework:
struct clock_list
{
     size_t offset_of_clk;
     qemu_clk_on_rate_update_cb cb;
     const char *name;
}

then walk the array to create the clock.

Does that make sense?

Thanks,
Fred
>
> Thanks,
>
> Alistair
>
>>   }
>>
>>   static const VMStateDescription vmstate_crf_apb = {
>> @@ -514,6 +953,7 @@ static void crf_apb_class_init(ObjectClass *klass, void *data)
>>
>>       dc->reset = crf_apb_reset;
>>       dc->vmsd = &vmstate_crf_apb;
>> +    dc->realize = crf_apb_realize;
>>   }
>>
>>   static const TypeInfo crf_apb_info = {
>> --
>> 2.5.5
>>
>>

Le 29/07/2016 à 15:51, Peter Maydell a écrit :
> On 13 June 2016 at 17:27,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This adds the pll to the zynqmp_crf and the dp_video clock output.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>  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
>