Re: [PATCH rc1 12/24] hw/timer: Add limited support for Atmel 16 bit timer peripheral

[Thomas Huth <thuth@redhat.com>]

On 23/01/2020 01.02, Richard Henderson wrote:
> From: Michael Rolnik <mrolnik@gmail.com>
> 
> These were designed to facilitate testing but should provide enough
> function to be useful in other contexts.  Only a subset of the functions
> of each peripheral is implemented, mainly due to the lack of a standard
> way to handle electrical connections (like GPIO pins).
> 
> Signed-off-by: Sarah Harris <S.E.Harris@kent.ac.uk>
> Message-Id: <20200118191416.19934-13-mrolnik@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> [rth: Squash info mtree fixes and a file rename from f4bug, which was:]
> Suggested-by: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
[...]
> +static void avr_timer16_clksrc_update(AVRTimer16State *t16)
> +{
> +    uint16_t divider = 0;
> +    switch (CLKSRC(t16)) {
> +    case T16_CLKSRC_EXT_FALLING:
> +    case T16_CLKSRC_EXT_RISING:
> +        ERROR("external clock source unsupported");

Maybe this should rather be qemu_log_mask(LOG_UNIMP, ...) instead?

> +        goto end;
> +    case T16_CLKSRC_STOPPED:
> +        goto end;
> +    case T16_CLKSRC_DIV1:
> +        divider = 1;
> +        break;
> +    case T16_CLKSRC_DIV8:
> +        divider = 8;
> +        break;
> +    case T16_CLKSRC_DIV64:
> +        divider = 64;
> +        break;
> +    case T16_CLKSRC_DIV256:
> +        divider = 256;
> +        break;
> +    case T16_CLKSRC_DIV1024:
> +        divider = 1024;
> +        break;
> +    default:
> +        goto end;
> +    }
> +    t16->freq_hz = t16->cpu_freq_hz / divider;
> +    t16->period_ns = NANOSECONDS_PER_SECOND / t16->freq_hz;
> +    DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f s)",
> +             t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz);
> +end:
> +    return;
> +}

Nit: You could use an early "return" instead of "goto end" and the
superfluous return statement at the end of the function.

> +static void avr_timer16_set_alarm(AVRTimer16State *t16)
> +{
> +    if (CLKSRC(t16) == T16_CLKSRC_EXT_FALLING ||
> +        CLKSRC(t16) == T16_CLKSRC_EXT_RISING ||
> +        CLKSRC(t16) == T16_CLKSRC_STOPPED) {
> +        /* Timer is disabled or set to external clock source (unsupported) */
> +        goto end;
> +    }
> +
> +    uint64_t alarm_offset = 0xffff;
> +    enum NextInterrupt next_interrupt = OVERFLOW;
> +
> +    switch (MODE(t16)) {
> +    case T16_MODE_NORMAL:
> +        /* Normal mode */
> +        if (OCRA(t16) < alarm_offset && OCRA(t16) > CNT(t16) &&
> +            (t16->imsk & T16_INT_OCA)) {
> +            alarm_offset = OCRA(t16);
> +            next_interrupt = COMPA;
> +        }
> +        break;
> +    case T16_MODE_CTC_OCRA:
> +        /* CTC mode, top = ocra */
> +        if (OCRA(t16) < alarm_offset && OCRA(t16) > CNT(t16)) {
> +            alarm_offset = OCRA(t16);
> +            next_interrupt = COMPA;
> +        }
> +       break;
> +    case T16_MODE_CTC_ICR:
> +        /* CTC mode, top = icr */
> +        if (ICR(t16) < alarm_offset && ICR(t16) > CNT(t16)) {
> +            alarm_offset = ICR(t16);
> +            next_interrupt = CAPT;
> +        }
> +        if (OCRA(t16) < alarm_offset && OCRA(t16) > CNT(t16) &&
> +            (t16->imsk & T16_INT_OCA)) {
> +            alarm_offset = OCRA(t16);
> +            next_interrupt = COMPA;
> +        }
> +        break;
> +    default:
> +        ERROR("pwm modes are unsupported");

qemu_log_mask(LOG_UNIMP, ...) ?

> +        goto end;
> +    }
> +    if (OCRB(t16) < alarm_offset && OCRB(t16) > CNT(t16) &&
> +        (t16->imsk & T16_INT_OCB)) {
> +        alarm_offset = OCRB(t16);
> +        next_interrupt = COMPB;
> +    }
> +    if (OCRC(t16) < alarm_offset && OCRB(t16) > CNT(t16) &&
> +        (t16->imsk & T16_INT_OCC)) {
> +        alarm_offset = OCRB(t16);
> +        next_interrupt = COMPC;
> +    }
> +    alarm_offset -= CNT(t16);
> +
> +    t16->next_interrupt = next_interrupt;
> +    uint64_t alarm_ns =
> +        t16->reset_time_ns + ((CNT(t16) + alarm_offset) * t16->period_ns);
> +    timer_mod(t16->timer, alarm_ns);
> +
> +    DB_PRINT("next alarm %" PRIu64 " ns from now",
> +        alarm_offset * t16->period_ns);
> +
> +end:
> +    return;
> +}

This function could also use early returns instead of "goto end".

(no need to respin just because of these nits ... but maybe something to
consider if you respin anyway)

 Thomas