[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH rc1 12/24] hw/timer: Add limited support for Atmel 16 bit tim
From: |
Thomas Huth |
Subject: |
Re: [PATCH rc1 12/24] hw/timer: Add limited support for Atmel 16 bit timer peripheral |
Date: |
Thu, 23 Jan 2020 05:49:28 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
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?
> + 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
- [PATCH rc1 04/24] target/avr: Add instruction translation - Arithmetic and Logic Instructions, (continued)
- [PATCH rc1 04/24] target/avr: Add instruction translation - Arithmetic and Logic Instructions, Richard Henderson, 2020/01/22
- [PATCH rc1 07/24] target/avr: Add instruction translation - Bit and Bit-test Instructions, Richard Henderson, 2020/01/22
- [PATCH rc1 09/24] target/avr: Add instruction translation - CPU main translation function, Richard Henderson, 2020/01/22
- [PATCH rc1 10/24] target/avr: Add instruction disassembly function, Richard Henderson, 2020/01/22
- [PATCH rc1 14/24] target/avr: Add section about AVR into QEMU documentation, Richard Henderson, 2020/01/22
- [PATCH rc1 16/24] target/avr: Add machine none test, Richard Henderson, 2020/01/22
- [PATCH rc1 20/24] hw/avr: Add some Arduino boards, Richard Henderson, 2020/01/22
- [PATCH rc1 12/24] hw/timer: Add limited support for Atmel 16 bit timer peripheral, Richard Henderson, 2020/01/22
- Re: [PATCH rc1 12/24] hw/timer: Add limited support for Atmel 16 bit timer peripheral,
Thomas Huth <=
- [PATCH rc1 18/24] hw/avr: Introduce ATMEL_ATMEGA_MCU config, Richard Henderson, 2020/01/22
- [PATCH rc1 19/24] hw/avr: Add some ATmega microcontrollers, Richard Henderson, 2020/01/22
- [PATCH rc1 21/24] target/avr: Update build system, Richard Henderson, 2020/01/22
- [PATCH rc1 22/24] tests/boot-serial-test: Test some Arduino boards (AVR based), Richard Henderson, 2020/01/22
- [PATCH rc1 23/24] tests/acceptance: Test the Arduino MEGA2560 board, Richard Henderson, 2020/01/22
- [PATCH rc1 15/24] target/avr: Register AVR support with the rest of QEMU, Richard Henderson, 2020/01/22