Message ID | 20210121190622.22000-20-peter.maydell@linaro.org |
---|---|
State | New |
Headers | show |
Series | Convert CMSDK timer, watchdog, dualtimer to Clock framework | expand |
On 19:06 Thu 21 Jan , Peter Maydell wrote: > Switch the CMSDK APB dualtimer device over to using its Clock input; > the pclk-frq property is now ignored. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/cmsdk-apb-dualtimer.c | 42 ++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 5 deletions(-) > > diff --git a/hw/timer/cmsdk-apb-dualtimer.c b/hw/timer/cmsdk-apb-dualtimer.c > index 781b496037b..828127b366f 100644 > --- a/hw/timer/cmsdk-apb-dualtimer.c > +++ b/hw/timer/cmsdk-apb-dualtimer.c > @@ -106,6 +106,22 @@ static void cmsdk_apb_dualtimer_update(CMSDKAPBDualTimer *s) > qemu_set_irq(s->timerintc, timintc); > } > > +static int cmsdk_dualtimermod_divisor(CMSDKAPBDualTimerModule *m) > +{ > + /* Return the divisor set by the current CONTROL.PRESCALE value */ > + switch (FIELD_EX32(m->control, CONTROL, PRESCALE)) { > + case 0: > + return 1; > + case 1: > + return 16; > + case 2: > + case 3: /* UNDEFINED, we treat like 2 (and complained when it was set) */ > + return 256; > + default: > + g_assert_not_reached(); > + } > +} > + > static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > uint32_t newctrl) > { > @@ -146,7 +162,7 @@ static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > default: > g_assert_not_reached(); > } > - ptimer_set_freq(m->timer, m->parent->pclk_frq / divisor); > + ptimer_set_period_from_clock(m->timer, m->parent->timclk, divisor); Just a small cosmetic note, maybe you can use your new cmsdk_dualtimermod_divisor function to factor out the switch above? Something like: if (changed & R_CONTROL_PRESCALE_MASK) { if (FIELD_EX32(newctrl, CONTROL, PRESCALE) == 3) { qemu_log_mask(LOG_GUEST_ERROR, "CMSDK APB dual-timer: CONTROL.PRESCALE==0b11" " is undefined behaviour\n"); } ptimer_set_period_from_clock(m->timer, m->parent->timclk, cmsdk_dualtimermod_divisor(m)); } With or without this modification: Reviewed-by: Luc Michel <luc@lmichel.fr> Luc > } > > if (changed & R_CONTROL_MODE_MASK) { > @@ -414,7 +430,8 @@ static void cmsdk_dualtimermod_reset(CMSDKAPBDualTimerModule *m) > * limit must both be set to 0xffff, so we wrap at 16 bits. > */ > ptimer_set_limit(m->timer, 0xffff, 1); > - ptimer_set_freq(m->timer, m->parent->pclk_frq); > + ptimer_set_period_from_clock(m->timer, m->parent->timclk, > + cmsdk_dualtimermod_divisor(m)); > ptimer_transaction_commit(m->timer); > } > > @@ -432,6 +449,20 @@ static void cmsdk_apb_dualtimer_reset(DeviceState *dev) > s->timeritop = 0; > } > > +static void cmsdk_apb_dualtimer_clk_update(void *opaque) > +{ > + CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(opaque); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(s->timermod); i++) { > + CMSDKAPBDualTimerModule *m = &s->timermod[i]; > + ptimer_transaction_begin(m->timer); > + ptimer_set_period_from_clock(m->timer, m->parent->timclk, > + cmsdk_dualtimermod_divisor(m)); > + ptimer_transaction_commit(m->timer); > + } > +} > + > static void cmsdk_apb_dualtimer_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > @@ -446,7 +477,8 @@ static void cmsdk_apb_dualtimer_init(Object *obj) > for (i = 0; i < ARRAY_SIZE(s->timermod); i++) { > sysbus_init_irq(sbd, &s->timermod[i].timerint); > } > - s->timclk = qdev_init_clock_in(DEVICE(s), "TIMCLK", NULL, NULL); > + s->timclk = qdev_init_clock_in(DEVICE(s), "TIMCLK", > + cmsdk_apb_dualtimer_clk_update, s); > } > > static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp) > @@ -454,8 +486,8 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp) > CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(dev); > int i; > > - if (s->pclk_frq == 0) { > - error_setg(errp, "CMSDK APB timer: pclk-frq property must be set"); > + if (!clock_has_source(s->timclk)) { > + error_setg(errp, "CMSDK APB dualtimer: TIMCLK clock must be connected"); > return; > } > > -- > 2.20.1 > --
On Fri, 22 Jan 2021 at 20:48, Luc Michel <luc@lmichel.fr> wrote: > > On 19:06 Thu 21 Jan , Peter Maydell wrote: > > Switch the CMSDK APB dualtimer device over to using its Clock input; > > the pclk-frq property is now ignored. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/timer/cmsdk-apb-dualtimer.c | 42 ++++++++++++++++++++++++++++++---- > > 1 file changed, 37 insertions(+), 5 deletions(-) > > > > diff --git a/hw/timer/cmsdk-apb-dualtimer.c b/hw/timer/cmsdk-apb-dualtimer.c > > index 781b496037b..828127b366f 100644 > > --- a/hw/timer/cmsdk-apb-dualtimer.c > > +++ b/hw/timer/cmsdk-apb-dualtimer.c > > @@ -106,6 +106,22 @@ static void cmsdk_apb_dualtimer_update(CMSDKAPBDualTimer *s) > > qemu_set_irq(s->timerintc, timintc); > > } > > > > +static int cmsdk_dualtimermod_divisor(CMSDKAPBDualTimerModule *m) > > +{ > > + /* Return the divisor set by the current CONTROL.PRESCALE value */ > > + switch (FIELD_EX32(m->control, CONTROL, PRESCALE)) { > > + case 0: > > + return 1; > > + case 1: > > + return 16; > > + case 2: > > + case 3: /* UNDEFINED, we treat like 2 (and complained when it was set) */ > > + return 256; > > + default: > > + g_assert_not_reached(); > > + } > > +} > > + > > static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > > uint32_t newctrl) > > { > > @@ -146,7 +162,7 @@ static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > > default: > > g_assert_not_reached(); > > } > > - ptimer_set_freq(m->timer, m->parent->pclk_frq / divisor); > > + ptimer_set_period_from_clock(m->timer, m->parent->timclk, divisor); > > Just a small cosmetic note, maybe you can use your new > cmsdk_dualtimermod_divisor function to factor out the switch above? > Something like: > > if (changed & R_CONTROL_PRESCALE_MASK) { > if (FIELD_EX32(newctrl, CONTROL, PRESCALE) == 3) { > qemu_log_mask(LOG_GUEST_ERROR, > "CMSDK APB dual-timer: CONTROL.PRESCALE==0b11" > " is undefined behaviour\n"); > } > > ptimer_set_period_from_clock(m->timer, m->parent->timclk, > cmsdk_dualtimermod_divisor(m)); > } Nope, because cmsdk_dualtimermod_divisor() uses the current m->control value, and at this point in the code we need the divisor from the new control value which isn't in m->control yet. I liked the slight duplication better than either having to pass m->control in in all the other callsites or trying to refactor the control write handling so that m->control is updated before this point in the code. thanks -- PMM
On 21:41 Fri 22 Jan , Peter Maydell wrote: > On Fri, 22 Jan 2021 at 20:48, Luc Michel <luc@lmichel.fr> wrote: > > > > On 19:06 Thu 21 Jan , Peter Maydell wrote: > > > Switch the CMSDK APB dualtimer device over to using its Clock input; > > > the pclk-frq property is now ignored. > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > --- > > > hw/timer/cmsdk-apb-dualtimer.c | 42 ++++++++++++++++++++++++++++++---- > > > 1 file changed, 37 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/timer/cmsdk-apb-dualtimer.c b/hw/timer/cmsdk-apb-dualtimer.c > > > index 781b496037b..828127b366f 100644 > > > --- a/hw/timer/cmsdk-apb-dualtimer.c > > > +++ b/hw/timer/cmsdk-apb-dualtimer.c > > > @@ -106,6 +106,22 @@ static void cmsdk_apb_dualtimer_update(CMSDKAPBDualTimer *s) > > > qemu_set_irq(s->timerintc, timintc); > > > } > > > > > > +static int cmsdk_dualtimermod_divisor(CMSDKAPBDualTimerModule *m) > > > +{ > > > + /* Return the divisor set by the current CONTROL.PRESCALE value */ > > > + switch (FIELD_EX32(m->control, CONTROL, PRESCALE)) { > > > + case 0: > > > + return 1; > > > + case 1: > > > + return 16; > > > + case 2: > > > + case 3: /* UNDEFINED, we treat like 2 (and complained when it was set) */ > > > + return 256; > > > + default: > > > + g_assert_not_reached(); > > > + } > > > +} > > > + > > > static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > > > uint32_t newctrl) > > > { > > > @@ -146,7 +162,7 @@ static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, > > > default: > > > g_assert_not_reached(); > > > } > > > - ptimer_set_freq(m->timer, m->parent->pclk_frq / divisor); > > > + ptimer_set_period_from_clock(m->timer, m->parent->timclk, divisor); > > > > Just a small cosmetic note, maybe you can use your new > > cmsdk_dualtimermod_divisor function to factor out the switch above? > > Something like: > > > > if (changed & R_CONTROL_PRESCALE_MASK) { > > if (FIELD_EX32(newctrl, CONTROL, PRESCALE) == 3) { > > qemu_log_mask(LOG_GUEST_ERROR, > > "CMSDK APB dual-timer: CONTROL.PRESCALE==0b11" > > " is undefined behaviour\n"); > > } > > > > ptimer_set_period_from_clock(m->timer, m->parent->timclk, > > cmsdk_dualtimermod_divisor(m)); > > } > > Nope, because cmsdk_dualtimermod_divisor() uses the current > m->control value, and at this point in the code we need the > divisor from the new control value which isn't in m->control yet. > I liked the slight duplication better than either having to > pass m->control in in all the other callsites or trying to > refactor the control write handling so that m->control is > updated before this point in the code. Oops yes I missed that. Sure make sense, forget what I said. > > thanks > -- PMM --
On 1/21/21 8:06 PM, Peter Maydell wrote: > Switch the CMSDK APB dualtimer device over to using its Clock input; > the pclk-frq property is now ignored. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/cmsdk-apb-dualtimer.c | 42 ++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 5 deletions(-) ... > @@ -454,8 +486,8 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp) > CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(dev); > int i; > > - if (s->pclk_frq == 0) { > - error_setg(errp, "CMSDK APB timer: pclk-frq property must be set"); > + if (!clock_has_source(s->timclk)) { > + error_setg(errp, "CMSDK APB dualtimer: TIMCLK clock must be connected"); > return; > } Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/timer/cmsdk-apb-dualtimer.c b/hw/timer/cmsdk-apb-dualtimer.c index 781b496037b..828127b366f 100644 --- a/hw/timer/cmsdk-apb-dualtimer.c +++ b/hw/timer/cmsdk-apb-dualtimer.c @@ -106,6 +106,22 @@ static void cmsdk_apb_dualtimer_update(CMSDKAPBDualTimer *s) qemu_set_irq(s->timerintc, timintc); } +static int cmsdk_dualtimermod_divisor(CMSDKAPBDualTimerModule *m) +{ + /* Return the divisor set by the current CONTROL.PRESCALE value */ + switch (FIELD_EX32(m->control, CONTROL, PRESCALE)) { + case 0: + return 1; + case 1: + return 16; + case 2: + case 3: /* UNDEFINED, we treat like 2 (and complained when it was set) */ + return 256; + default: + g_assert_not_reached(); + } +} + static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, uint32_t newctrl) { @@ -146,7 +162,7 @@ static void cmsdk_dualtimermod_write_control(CMSDKAPBDualTimerModule *m, default: g_assert_not_reached(); } - ptimer_set_freq(m->timer, m->parent->pclk_frq / divisor); + ptimer_set_period_from_clock(m->timer, m->parent->timclk, divisor); } if (changed & R_CONTROL_MODE_MASK) { @@ -414,7 +430,8 @@ static void cmsdk_dualtimermod_reset(CMSDKAPBDualTimerModule *m) * limit must both be set to 0xffff, so we wrap at 16 bits. */ ptimer_set_limit(m->timer, 0xffff, 1); - ptimer_set_freq(m->timer, m->parent->pclk_frq); + ptimer_set_period_from_clock(m->timer, m->parent->timclk, + cmsdk_dualtimermod_divisor(m)); ptimer_transaction_commit(m->timer); } @@ -432,6 +449,20 @@ static void cmsdk_apb_dualtimer_reset(DeviceState *dev) s->timeritop = 0; } +static void cmsdk_apb_dualtimer_clk_update(void *opaque) +{ + CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(opaque); + int i; + + for (i = 0; i < ARRAY_SIZE(s->timermod); i++) { + CMSDKAPBDualTimerModule *m = &s->timermod[i]; + ptimer_transaction_begin(m->timer); + ptimer_set_period_from_clock(m->timer, m->parent->timclk, + cmsdk_dualtimermod_divisor(m)); + ptimer_transaction_commit(m->timer); + } +} + static void cmsdk_apb_dualtimer_init(Object *obj) { SysBusDevice *sbd = SYS_BUS_DEVICE(obj); @@ -446,7 +477,8 @@ static void cmsdk_apb_dualtimer_init(Object *obj) for (i = 0; i < ARRAY_SIZE(s->timermod); i++) { sysbus_init_irq(sbd, &s->timermod[i].timerint); } - s->timclk = qdev_init_clock_in(DEVICE(s), "TIMCLK", NULL, NULL); + s->timclk = qdev_init_clock_in(DEVICE(s), "TIMCLK", + cmsdk_apb_dualtimer_clk_update, s); } static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp) @@ -454,8 +486,8 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev, Error **errp) CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(dev); int i; - if (s->pclk_frq == 0) { - error_setg(errp, "CMSDK APB timer: pclk-frq property must be set"); + if (!clock_has_source(s->timclk)) { + error_setg(errp, "CMSDK APB dualtimer: TIMCLK clock must be connected"); return; }
Switch the CMSDK APB dualtimer device over to using its Clock input; the pclk-frq property is now ignored. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/timer/cmsdk-apb-dualtimer.c | 42 ++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 5 deletions(-)