On 5/16/21 12:09 AM, Michael Rolnik wrote: > Signed-off-by: Michael Rolnik <[email protected]> > --- > MAINTAINERS | 2 + > hw/avr/Kconfig | 1 + > hw/avr/atmega.c | 15 +- > hw/avr/atmega.h | 2 +
I'd split this patch in 4: ^ patch #2, wire wdt in atmega (This patch is OK) > hw/watchdog/Kconfig | 3 + > hw/watchdog/avr_wdt.c | 279 ++++++++++++++++++++++++++++++++++ > hw/watchdog/meson.build | 2 + > hw/watchdog/trace-events | 5 + > include/hw/watchdog/avr_wdt.h | 47 ++++++ ^ patch #1, add wdt model > target/avr/cpu.c | 3 + > target/avr/cpu.h | 1 + > target/avr/helper.c | 7 +- ^ patch #4, wire opcode > target/avr/translate.c | 58 ++++++- ^ patch #3, adding gen_io() (I'd like review from Alex / Pavel Dovgalyuk) > 13 files changed, 419 insertions(+), 6 deletions(-) > create mode 100644 hw/watchdog/avr_wdt.c > create mode 100644 include/hw/watchdog/avr_wdt.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 78561a223f..e53bccfa9a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1036,6 +1036,8 @@ F: include/hw/timer/avr_timer16.h > F: hw/timer/avr_timer16.c > F: include/hw/misc/avr_power.h > F: hw/misc/avr_power.c > +F: include/hw/watchdog/avr_wdt.h > +F: hw/watchdog/avr_wdt.c > > Arduino > M: Philippe Mathieu-Daudé <[email protected]> > diff --git a/hw/watchdog/avr_wdt.c b/hw/watchdog/avr_wdt.c > new file mode 100644 > index 0000000000..4e14f734cd > --- /dev/null > +++ b/hw/watchdog/avr_wdt.c > @@ -0,0 +1,279 @@ > +/* > + * AVR watchdog > + * > + * Copyright (c) 2021 Michael Rolnik > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see > + * <http://www.gnu.org/licenses/lgpl-2.1.html> Why not use GPL-2.0-or-later? > + > +#define DB_PRINT(fmt, args...) /* Nothing */ If you don't use it, drop it? > + > +#define MS2NS(n) ((n) * 1000000ull) Please drop this macro ... > +static void avr_wdt_reset_alarm(AVRWatchdogState *wdt) > +{ > + uint32_t csr = wdt->csr; > + int wdp = WDP(csr); > + > + if (WDIE(csr) == 0 && WDE(csr) == 0) { > + /* watchdog is stopped */ > + return; > + } > + > + timer_mod_ns(&wdt->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > + (MS2NS(15) << wdp)); ... and use 15 * SCALE_MS. Even better, add a avr_wdt_timeout_value() where you check the prescaler, and here: timer_mod_ns(&wdt->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + avr_wdt_timeout_value(wdt)) What is this '15' magic number anyway? > +} > + > +static void avr_wdt_interrupt(void *opaque) Maybe name that avr_wdt_expire/timeout? > +{ > + AVRWatchdogState *wdt = opaque; > + int8_t csr = wdt->csr; > + trace_avr_wdt_expired(csr); > + if (WDIE(csr) == 1) { > + /* Interrupt Mode */ > + set_bits(&wdt->csr, R_CSR_WDIF_MASK); > + qemu_set_irq(wdt->irq, 1); > + rst_bits(&wdt->csr, R_CSR_WDIE_MASK); > + trace_avr_wdt_interrupt(); replaced by trace_avr_wdt_expired()? > + } else > + if (WDE(csr) == 1) { Can we have definitions instead of '1' & comment? > + /* System Reset Mode */ > + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > + } What about the watchdog_perform_action() case? > + > + avr_wdt_reset_alarm(wdt); This call doesn't seem correct, maybe add it in each case? Anyway this function body doesn't look like table 12-1 "Watchdog Timer Configuration". What about: if (!WDTON) { goto system_reset; } else { if (WDIE) { // interrupt if (WDE) { return; } else { goto system_reset; } } if (WDE) { goto system_reset; } g_assert_not_reached(); } system_reset: ... > +} > + > +static void avr_wdt_reset(DeviceState *dev) > +{ > + AVRWatchdogState *wdt = AVR_WDT(dev); > + > + wdt->csr = 0; What about MCUSR flags? > + qemu_set_irq(wdt->irq, 0); > + avr_wdt_reset_alarm(wdt); > +} > + > +static void avr_wdt_write(void *opaque, hwaddr offset, > + uint64_t val64, unsigned size) > +{ > + assert(size == 1); > + AVRWatchdogState *wdt = opaque; > + uint8_t val = (uint8_t)val64; > + uint8_t set1 = val; /* bits that should be set to 1 */ > + uint8_t set0 = ~val;/* bits that should be set to 0 */ > + uint8_t mcusr = 0; > + > + /* > + * Bit 7 - WDIF: Watchdog Interrupt Flag > + * This bit is set when a time-out occurs in the Watchdog Timer and the > + * Watchdog Timer is configured for interrupt. WDIF is cleared by > hardware > + * when executing the corresponding interrupt handling vector. > + * Alternatively, WDIF is cleared by writing a logic one to the flag. > + * When the I-bit in SREG and WDIE are set, the Watchdog Time-out > Interrupt > + * is executed. > + */ > + if (val & R_CSR_WDIF_MASK) { > + rst_bits(&set1, R_CSR_WDIF_MASK); /* don't set 1 */ > + set_bits(&set0, R_CSR_WDIF_MASK); /* set 0 */ > + } else { > + rst_bits(&set1, R_CSR_WDIF_MASK); /* don't set 1 */ > + rst_bits(&set0, R_CSR_WDIF_MASK); /* don't set 0 */ > + } > + > + /* > + * Bit 4 - WDCE: Watchdog Change Enable > + * This bit is used in timed sequences for changing WDE and prescaler > + * bits. To clear the WDE bit, and/or change the prescaler bits, > + * WDCE must be set. > + * Once written to one, hardware will clear WDCE after four clock > cycles. > + */ > + if (!(val & R_CSR_WDCE_MASK)) { > + uint8_t bits = R_CSR_WDE_MASK | R_CSR_WDP0_MASK | R_CSR_WDP1_MASK | > + R_CSR_WDP2_MASK | R_CSR_WDP3_MASK; > + rst_bits(&set1, bits); > + rst_bits(&set0, bits); > + } > + > + /* > + * Bit 3 - WDE: Watchdog System Reset Enable > + * WDE is overridden by WDRF in MCUSR. This means that WDE is always set > + * when WDRF is set. To clear WDE, WDRF must be cleared first. This > + * feature ensures multiple resets during conditions causing failure, > and > + * a safe start-up after the failure. > + */ > + cpu_physical_memory_read(A_MCUSR, &mcusr, sizeof(mcusr)); > + if (mcusr & R_MCUSR_WDRF_MASK) { > + set_bits(&set1, R_CSR_WDE_MASK); > + rst_bits(&set0, R_CSR_WDE_MASK); > + } > + > + /* update CSR value */ > + assert((set0 & set1) == 0); > + > + val = wdt->csr; > + set_bits(&val, set1); > + rst_bits(&val, set0); > + wdt->csr = val; > + trace_avr_wdt_write(offset, val); > + avr_wdt_reset_alarm(wdt); > + > + /* > + * Bit 6 - WDIE: Watchdog Interrupt Enable > + * When this bit is written to one and the I-bit in the Status Register > is > + * set, the Watchdog Interrupt is enabled. If WDE is cleared in > + * combination with this setting, the Watchdog Timer is in Interrupt > Mode, > + * and the corresponding interrupt is executed if time-out in the > Watchdog > + * Timer occurs. > + * If WDE is set, the Watchdog Timer is in Interrupt and System Reset > Mode. > + * The first time-out in the Watchdog Timer will set WDIF. Executing the > + * corresponding interrupt vector will clear WDIE and WDIF > automatically by > + * hardware (the Watchdog goes to System Reset Mode). This is useful for > + * keeping the Watchdog Timer security while using the interrupt. To > stay > + * in Interrupt and System Reset Mode, WDIE must be set after each > + * interrupt. This should however not be done within the interrupt > service > + * routine itself, as this might compromise the safety-function of the > + * Watchdog System Reset mode. If the interrupt is not executed before > the > + * next time-out, a System Reset will be applied. > + */ > + if ((val & R_CSR_WDIE_MASK) && (wdt->csr & R_CSR_WDIF_MASK)) { > + avr_wdt_interrupt(opaque); > + } This function is too complex, I'm skipping it. > diff --git a/include/hw/watchdog/avr_wdt.h b/include/hw/watchdog/avr_wdt.h > new file mode 100644 > index 0000000000..006f9496fb > --- /dev/null > +++ b/include/hw/watchdog/avr_wdt.h > @@ -0,0 +1,47 @@ > +/* > + * AVR watchdog > + * > + * Copyright (c) 2021 Michael Rolnik > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see > + * <http://www.gnu.org/licenses/lgpl-2.1.html> > + */ > + > +#ifndef HW_WATCHDOG_AVR_WDT_H > +#define HW_WATCHDOG_AVR_WDT_H > + > +#include "hw/sysbus.h" > +#include "qemu/timer.h" > +#include "hw/hw.h" Probably not needed. > +#include "qom/object.h" > + > +#define TYPE_AVR_WDT "avr-wdt" > +OBJECT_DECLARE_SIMPLE_TYPE(AVRWatchdogState, AVR_WDT) > + > +struct AVRWatchdogState { > + /* <private> */ > + SysBusDevice parent_obj; > + > + /* <public> */ > + MemoryRegion iomem; > + MemoryRegion imsk_iomem; > + MemoryRegion ifr_iomem; > + QEMUTimer timer; > + qemu_irq irq; > + > + /* registers */ > + uint8_t csr; > +}; > + > +#endif /* HW_WATCHDOG_AVR_WDT_H */
