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

[Philippe Mathieu-Daudé]

On 1/23/20 5:49 AM, Thomas Huth wrote:
> On 23/01/2020 01.02, Richard Henderson wrote:
> > From: Michael Rolnik <address@hidden>
> >
> > 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 <address@hidden>
> > Message-Id: <address@hidden>
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > [rth: Squash info mtree fixes and a file rename from f4bug, which was:]
> > Suggested-by: Aleksandar Markovic <address@hidden>
> > Signed-off-by: Richard Henderson <address@hidden>
> > ---
> [...]
> > +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?

Yes.

> > +        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.

I'd prefer use a 'break' statement, and check if divider != then { set 
values and log trace event}.

> > +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