Re: [PATCH v6 03/13] hw/timer: Add NPCM7xx Timer device model

[Philippe Mathieu-Daudé]

On 7/17/20 8:02 AM, Havard Skinnemoen wrote:
> The NPCM730 and NPCM750 SoCs have three timer modules each holding five
> timers and some shared registers (e.g. interrupt status).
> 
> Each timer runs at 25 MHz divided by a prescaler, and counts down from a
> configurable initial value to zero. When zero is reached, the interrupt
> flag for the timer is set, and the timer is disabled (one-shot mode) or
> reloaded from its initial value (periodic mode).
> 
> This implementation is sufficient to boot a Linux kernel configured for
> NPCM750. Note that the kernel does not seem to actually turn on the
> interrupts.
> 
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  include/hw/timer/npcm7xx_timer.h |  96 ++++++
>  hw/timer/npcm7xx_timer.c         | 489 +++++++++++++++++++++++++++++++
>  hw/timer/Makefile.objs           |   1 +
>  hw/timer/trace-events            |   5 +
>  4 files changed, 591 insertions(+)
>  create mode 100644 include/hw/timer/npcm7xx_timer.h
>  create mode 100644 hw/timer/npcm7xx_timer.c
...

> +static hwaddr npcm7xx_tcsr_index(hwaddr reg)
> +{
> +    switch (reg) {
> +    case NPCM7XX_TIMER_TCSR0:
> +        return 0;
> +    case NPCM7XX_TIMER_TCSR1:
> +        return 1;
> +    case NPCM7XX_TIMER_TCSR2:
> +        return 2;
> +    case NPCM7XX_TIMER_TCSR3:
> +        return 3;
> +    case NPCM7XX_TIMER_TCSR4:
> +        return 4;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static hwaddr npcm7xx_ticr_index(hwaddr reg)
> +{
> +    switch (reg) {
> +    case NPCM7XX_TIMER_TICR0:
> +        return 0;
> +    case NPCM7XX_TIMER_TICR1:
> +        return 1;
> +    case NPCM7XX_TIMER_TICR2:
> +        return 2;
> +    case NPCM7XX_TIMER_TICR3:
> +        return 3;
> +    case NPCM7XX_TIMER_TICR4:
> +        return 4;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static hwaddr npcm7xx_tdr_index(hwaddr reg)
> +{
> +    switch (reg) {
> +    case NPCM7XX_TIMER_TDR0:
> +        return 0;
> +    case NPCM7XX_TIMER_TDR1:
> +        return 1;
> +    case NPCM7XX_TIMER_TDR2:
> +        return 2;
> +    case NPCM7XX_TIMER_TDR3:
> +        return 3;
> +    case NPCM7XX_TIMER_TDR4:
> +        return 4;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static uint64_t npcm7xx_timer_read(void *opaque, hwaddr offset, unsigned 
> size)
> +{
> +    NPCM7xxTimerCtrlState *s = opaque;
> +    uint64_t value = 0;
> +    hwaddr reg;
> +
> +    reg = offset / sizeof(uint32_t);
> +    switch (reg) {
> +    case NPCM7XX_TIMER_TCSR0:
> +    case NPCM7XX_TIMER_TCSR1:
> +    case NPCM7XX_TIMER_TCSR2:
> +    case NPCM7XX_TIMER_TCSR3:
> +    case NPCM7XX_TIMER_TCSR4:
> +        value = s->timer[npcm7xx_tcsr_index(reg)].tcsr;
> +        break;
> +
> +    case NPCM7XX_TIMER_TICR0:
> +    case NPCM7XX_TIMER_TICR1:
> +    case NPCM7XX_TIMER_TICR2:
> +    case NPCM7XX_TIMER_TICR3:
> +    case NPCM7XX_TIMER_TICR4:
> +        value = s->timer[npcm7xx_ticr_index(reg)].ticr;
> +        break;
> +
> +    case NPCM7XX_TIMER_TDR0:
> +    case NPCM7XX_TIMER_TDR1:
> +    case NPCM7XX_TIMER_TDR2:
> +    case NPCM7XX_TIMER_TDR3:
> +    case NPCM7XX_TIMER_TDR4:
> +        value = npcm7xx_timer_read_tdr(&s->timer[npcm7xx_tdr_index(reg)]);
> +        break;

Thanks for adding the register index getters, I'm not sure this is a
matter of taste, but I find it easier to review.

> +
> +    case NPCM7XX_TIMER_TISR:
> +        value = s->tisr;
> +        break;
> +
> +    case NPCM7XX_TIMER_WTCR:
> +        value = s->wtcr;
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid offset 0x%04" HWADDR_PRIx "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +
> +    trace_npcm7xx_timer_read(DEVICE(s)->canonical_path, offset, value);
> +
> +    return value;
> +}
...