Re: [PATCH v35 10/13] target/avr: Add limited support for USART and 16 bit timer peripherals

[Philippe Mathieu-Daudé]

Hi Michael,

On 10/29/19 10:24 PM, Michael Rolnik wrote:
> From: Sarah Harris <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>
> ---
>   hw/char/Kconfig                |   3 +
>   hw/char/Makefile.objs          |   1 +
>   hw/char/avr_usart.c            | 324 ++++++++++++++++++
>   hw/misc/Kconfig                |   3 +
>   hw/misc/Makefile.objs          |   2 +
>   hw/misc/avr_mask.c             | 112 ++++++
>   hw/timer/Kconfig               |   3 +
>   hw/timer/Makefile.objs         |   2 +
>   hw/timer/avr_timer16.c         | 605 +++++++++++++++++++++++++++++++++
>   include/hw/char/avr_usart.h    |  97 ++++++
>   include/hw/misc/avr_mask.h     |  47 +++
>   include/hw/timer/avr_timer16.h |  97 ++++++
>   12 files changed, 1296 insertions(+)
>   create mode 100644 hw/char/avr_usart.c
>   create mode 100644 hw/misc/avr_mask.c
>   create mode 100644 hw/timer/avr_timer16.c
>   create mode 100644 include/hw/char/avr_usart.h
>   create mode 100644 include/hw/misc/avr_mask.h
>   create mode 100644 include/hw/timer/avr_timer16.h

I haven't read all the other review comments yet, so I'm not sure you 
need to respin a new version of this series.

In another review I suggested to rename 'avr' -> 'avr8'.

If you have to send another version, can you split this patch in 3?
IRQ/TMR/UART.

[...]
> +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");
> +        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 = 1000000000ULL / t16->freq_hz;

Please use NANOSECONDS_PER_SECOND here.

> +    DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f s)",
> +             t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz);
> +end:
> +    return;
> +}

While trying this patch it looks the timer fires too fast (or maybe 
doesn't use the correct clock rate).

When looking at the instructions in GDB render the debugging of timing 
issues pointless (which is why I wanted to compare the asm executed with 
the IRQ timed events on stdout).

I should have some time to continue investigating Sunday evening.

Regards,

Phil.