Re: [PATCH 19/25] hw/timer/cmsdk-apb-dualtimer: Convert to use Clock input

[Luc Michel]

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
> 

--