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

[KONRAD Frederic <fred.konrad@greensocs.com>]

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
>>
>>