From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUYly-0000Il-Tv for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:26:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUYlq-0006Jk-VC for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:26:13 -0400 Received: from greensocs.com ([193.104.36.180]:35792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUYlq-0006JF-D2 for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:26:06 -0400 References: <1465835259-21449-1-git-send-email-fred.konrad@greensocs.com> <1465835259-21449-10-git-send-email-fred.konrad@greensocs.com> From: KONRAD Frederic Message-ID: <916f9e5b-20e5-a35e-a91b-3d7ff6fc5ce0@greensocs.com> Date: Tue, 2 Aug 2016 14:26:45 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH 09/11] zynqmp_crf: add the clock mechanism List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: "qemu-devel@nongnu.org Developers" , Edgar Iglesias , Peter Maydell , Mark Burton Le 02/07/2016 =C3=A0 01:23, Alistair Francis a =C3=A9crit : > On Mon, Jun 13, 2016 at 9:27 AM, wrote: >> From: KONRAD Frederic >> >> This adds the pll to the zynqmp_crf and the dp_video clock output. >> >> Signed-off-by: KONRAD Frederic >> --- >> 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 =3D { >> @@ -325,6 +358,318 @@ static uint64_t ir_disable_prew(RegisterInfo *re= g, uint64_t val64) >> return 0; >> } >> >> +enum clk_src { >> + VIDEO_CLK =3D 4, >> + PSS_ALT_REF_CLK =3D 5, >> + AUX_REF_CLK =3D 6, >> + GT_CRX_REF_CLK =3D 7, >> + PSS_REF_CLK =3D 0 >> +}; >> + >> +static void apll_to_lpd_postw(RegisterInfo *reg, uint64_t val64) >> +{ >> + CRF_APB *s =3D 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 =3D XILINX_CRF_APB(opaque); >> + uint32_t divisor =3D 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 =3D 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 =3D XILINX_CRF_APB(opaque); >> + uint32_t divisor =3D 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 =3D 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 =3D XILINX_CRF_APB(opaque); >> + uint32_t divisor =3D 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 =3D XILINX_CRF_APB(reg->opaque); >> + uint32_t source =3D 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 intern= al 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 =3D XILINX_CRF_APB(reg->opaque); >> + uint32_t source =3D 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 intern= al 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 =3D XILINX_CRF_APB(reg->opaque); >> + uint32_t source =3D 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 intern= al 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 t= he >> + * computation from input_rate. >> + */ >> +static float apll_update_rate(void *opaque, float input_rate) >> +{ >> + CRF_APB *s =3D XILINX_CRF_APB(opaque); >> + bool bypass =3D AF_EX32(s->regs, APLL_CTRL, BYPASS); >> + bool reset =3D AF_EX32(s->regs, APLL_CTRL, RESET); >> + float div2 =3D AF_EX32(s->regs, APLL_CTRL, DIV2) ? 0.5f : 1.0f; >> + float integer =3D (float)(AF_EX32(s->regs, APLL_CTRL, FBDIV)); >> + float frac =3D 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 B= YPASS 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 t= he >> + * computation from input_rate. >> + */ >> +static float dpll_update_rate(void *opaque, float input_rate) >> +{ >> + CRF_APB *s =3D XILINX_CRF_APB(opaque); >> + bool bypass =3D AF_EX32(s->regs, DPLL_CTRL, BYPASS); >> + bool reset =3D AF_EX32(s->regs, DPLL_CTRL, RESET); >> + float div2 =3D AF_EX32(s->regs, DPLL_CTRL, DIV2) ? 0.5f : 1.0f; >> + float integer =3D (float)(AF_EX32(s->regs, DPLL_CTRL, FBDIV)); >> + float frac =3D 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 B= YPASS 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 t= he >> + * computation from input_rate. >> + */ >> +static float vpll_update_rate(void *opaque, float input_rate) >> +{ >> + CRF_APB *s =3D XILINX_CRF_APB(opaque); >> + bool bypass =3D AF_EX32(s->regs, VPLL_CTRL, BYPASS); >> + bool reset =3D AF_EX32(s->regs, VPLL_CTRL, RESET); >> + float div2 =3D AF_EX32(s->regs, VPLL_CTRL, DIV2) ? 0.5f : 1.0f; >> + float integer =3D (float)(AF_EX32(s->regs, VPLL_CTRL, FBDIV)); >> + float frac =3D 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 B= YPASS 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 m= issing. >> + */ >> +static float dp_video_update_rate(void *opaque, float input_rate) >> +{ >> + CRF_APB *s =3D XILINX_CRF_APB(opaque); >> + bool clock_act =3D AF_EX32(s->regs, DP_VIDEO_REF_CTRL, CLKACT); >> + uint32_t divisor0 =3D AF_EX32(s->regs, DP_VIDEO_REF_CTRL, DIVISOR= 0); >> + >> + 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 =3D XILINX_CRF_APB(reg->opaque); >> + uint32_t source =3D 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_re= f >> + * 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[] =3D { >> { .name =3D "ERR_CTRL", .decode.addr =3D A_ERR_CTRL, >> },{ .name =3D "IR_STATUS", .decode.addr =3D A_IR_STATUS, >> @@ -341,6 +686,7 @@ static RegisterAccessInfo crf_apb_regs_info[] =3D = { >> },{ .name =3D "APLL_CTRL", .decode.addr =3D A_APLL_CTRL, >> .reset =3D 0x2809, >> .rsvd =3D 0xf88c80f6L, >> + .post_write =3D apll_ctrl_postw, >> },{ .name =3D "APLL_CFG", .decode.addr =3D A_APLL_CFG, >> .rsvd =3D 0x1801210, >> },{ .name =3D "APLL_FRAC_CFG", .decode.addr =3D A_APLL_FRAC_CFG= , >> @@ -348,6 +694,7 @@ static RegisterAccessInfo crf_apb_regs_info[] =3D = { >> },{ .name =3D "DPLL_CTRL", .decode.addr =3D A_DPLL_CTRL, >> .reset =3D 0x2809, >> .rsvd =3D 0xf88c80f6L, >> + .post_write =3D dpll_ctrl_postw, >> },{ .name =3D "DPLL_CFG", .decode.addr =3D A_DPLL_CFG, >> .rsvd =3D 0x1801210, >> },{ .name =3D "DPLL_FRAC_CFG", .decode.addr =3D A_DPLL_FRAC_CFG= , >> @@ -355,6 +702,7 @@ static RegisterAccessInfo crf_apb_regs_info[] =3D = { >> },{ .name =3D "VPLL_CTRL", .decode.addr =3D A_VPLL_CTRL, >> .reset =3D 0x2809, >> .rsvd =3D 0xf88c80f6L, >> + .post_write =3D vpll_ctrl_postw, >> },{ .name =3D "VPLL_CFG", .decode.addr =3D A_VPLL_CFG, >> .rsvd =3D 0x1801210, >> },{ .name =3D "VPLL_FRAC_CFG", .decode.addr =3D A_VPLL_FRAC_CFG= , >> @@ -366,12 +714,15 @@ static RegisterAccessInfo crf_apb_regs_info[] =3D= { >> },{ .name =3D "APLL_TO_LPD_CTRL", .decode.addr =3D A_APLL_TO_LP= D_CTRL, >> .reset =3D 0x400, >> .rsvd =3D 0xc0ff, >> + .post_write =3D apll_to_lpd_postw, >> },{ .name =3D "DPLL_TO_LPD_CTRL", .decode.addr =3D A_DPLL_TO_LP= D_CTRL, >> .reset =3D 0x400, >> .rsvd =3D 0xc0ff, >> + .post_write =3D dpll_to_lpd_postw, >> },{ .name =3D "VPLL_TO_LPD_CTRL", .decode.addr =3D A_VPLL_TO_LP= D_CTRL, >> .reset =3D 0x400, >> .rsvd =3D 0xc0ff, >> + .post_write =3D vpll_to_lpd_postw, >> },{ .name =3D "CPU_A9_CTRL", .decode.addr =3D A_CPU_A9_CTRL, >> .reset =3D 0xf000400, >> .rsvd =3D 0xf0ffc0f8L, >> @@ -384,6 +735,7 @@ static RegisterAccessInfo crf_apb_regs_info[] =3D = { >> },{ .name =3D "DP_VIDEO_REF_CTRL", .decode.addr =3D A_DP_VIDEO_= REF_CTRL, >> .reset =3D 0x1002300, >> .rsvd =3D 0xfeffc0f8L, >> + .post_write =3D dp_video_ref_ctrl_postw, >> },{ .name =3D "DP_AUDIO_REF_CTRL", .decode.addr =3D A_DP_AUDIO_= REF_CTRL, >> .reset =3D 0x1002300, >> .rsvd =3D 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 li= ke 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=20 commented patch. > >> +} >> + >> +static void crf_apb_realize(DeviceState *d, Error **errp) >> +{ >> + CRF_APB *s =3D 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 =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->pss_ref_clk, "pss_ref_c= lk"); >> + s->video_clk =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->video_clk, "video_clk")= ; >> + s->pss_alt_ref_clk =3D 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 =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->aux_refclk, "aux_refclk= "); >> + s->gt_crx_ref_clk =3D 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 =3D 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 =3D 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 =3D 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 =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->acpu_clk, "acpu_clk"); >> + s->dbg_trace =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dbg_trace, "dbg_trace")= ; >> + s->dbg_fdp =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dbg_fdp, "dbg_fdp"); >> + s->dp_video_ref =3D 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 =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dp_audio_ref, "dp_audio= _ref"); >> + s->dp_stc_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dp_stc_ref, "dp_stc_ref= "); >> + s->ddr =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->ddr, "ddr"); >> + s->gpu_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->gpu_ref, "gpu_ref"); >> + s->sata_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->sata_ref, "sata_ref"); >> + s->pcie_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->pcie_ref, "pcie_ref"); >> + s->gdma_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->gdma_ref, "gdma_ref"); >> + s->dpdma_ref =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dpdma_ref, "dpdma_ref")= ; >> + s->topsw_main =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->topsw_main, "topsw_main= "); >> + s->topsw_lsbus =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->topsw_lsbus, "topsw_lsb= us"); >> + s->dbg_tstmp =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dbg_tstmp, "dbg_tstmp")= ; >> + >> + s->apll_to_lpd =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->apll_to_lpd, "apll_to_l= pd"); >> + qemu_clk_set_callback(s->apll_to_lpd, apll_to_lpd_update_rate, ob= j); >> + s->dpll_to_lpd =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->dpll_to_lpd, "dpll_to_l= pd"); >> + qemu_clk_set_callback(s->dpll_to_lpd, dpll_to_lpd_update_rate, ob= j); >> + s->vpll_to_lpd =3D QEMU_CLOCK(object_new(TYPE_CLOCK)); >> + qemu_clk_attach_to_device(DEVICE(obj), s->vpll_to_lpd, "vpll_to_l= pd"); >> + qemu_clk_set_callback(s->vpll_to_lpd, vpll_to_lpd_update_rate, ob= j); > 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 =3D { >> @@ -514,6 +953,7 @@ static void crf_apb_class_init(ObjectClass *klass,= void *data) >> >> dc->reset =3D crf_apb_reset; >> dc->vmsd =3D &vmstate_crf_apb; >> + dc->realize =3D crf_apb_realize; >> } >> >> static const TypeInfo crf_apb_info =3D { >> -- >> 2.5.5 >> >>