Hi Philippe. Thanks for you review. Please explain how to link a device.
Regards, Michael On Wed, Jul 10, 2019 at 1:06 PM Philippe Mathieu-Daudé <phi...@redhat.com> wrote: > Hi Michael, Sarah. > > Sorry for reviewing this series that late. > > On 7/9/19 11:36 PM, Michael Rolnik wrote: > > From: Sarah Harris <s.e.har...@kent.ac.uk> > > > > 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: Michael Rolnik <mrol...@gmail.com> > > --- > > hw/char/Kconfig | 3 + > > hw/char/Makefile.objs | 1 + > > hw/char/avr_usart.c | 316 ++++++++++++++++++ > > hw/timer/Kconfig | 3 + > > hw/timer/Makefile.objs | 1 + > > hw/timer/avr_timer16.c | 587 +++++++++++++++++++++++++++++++++ > > include/hw/char/avr_usart.h | 99 ++++++ > > include/hw/timer/avr_timer16.h | 99 ++++++ > > 8 files changed, 1109 insertions(+) > > create mode 100644 hw/char/avr_usart.c > > create mode 100644 hw/timer/avr_timer16.c > > create mode 100644 include/hw/char/avr_usart.h > > create mode 100644 include/hw/timer/avr_timer16.h > > > > diff --git a/hw/char/Kconfig b/hw/char/Kconfig > > index 40e7a8b8bb..331b20983f 100644 > > --- a/hw/char/Kconfig > > +++ b/hw/char/Kconfig > > @@ -46,3 +46,6 @@ config SCLPCONSOLE > > > > config TERMINAL3270 > > bool > > + > > +config AVR_USART > > + bool > > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs > > index 02d8a66925..09ed50f1d0 100644 > > --- a/hw/char/Makefile.objs > > +++ b/hw/char/Makefile.objs > > @@ -21,6 +21,7 @@ obj-$(CONFIG_PSERIES) += spapr_vty.o > > obj-$(CONFIG_DIGIC) += digic-uart.o > > obj-$(CONFIG_STM32F2XX_USART) += stm32f2xx_usart.o > > obj-$(CONFIG_RASPI) += bcm2835_aux.o > > +obj-$(CONFIG_AVR_USART) += avr_usart.o > > > > common-obj-$(CONFIG_CMSDK_APB_UART) += cmsdk-apb-uart.o > > common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o > > diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c > > new file mode 100644 > > index 0000000000..26c711336b > > --- /dev/null > > +++ b/hw/char/avr_usart.c > > @@ -0,0 +1,316 @@ > > +/* > > + * AVR USART > > + * > > + * Copyright (c) 2018 University of Kent > > + * Author: Sarah Harris > > + * > > + * Permission is hereby granted, free of charge, to any person > obtaining a copy > > + * of this software and associated documentation files (the > "Software"), to deal > > + * in the Software without restriction, including without limitation > the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN > > + * THE SOFTWARE. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "hw/char/avr_usart.h" > > +#include "qemu/log.h" > > + > > +static int avr_usart_can_receive(void *opaque) > > +{ > > + AVRUsartState *usart = opaque; > > + > > + if (usart->data_valid || !(usart->csrb & USART_CSRB_RXEN)) { > > + return 0; > > + } > > + return 1; > > +} > > + > > +static void avr_usart_receive(void *opaque, const uint8_t *buffer, int > size) > > +{ > > + AVRUsartState *usart = opaque; > > + assert(size == 1); > > + assert(!usart->data_valid); > > + usart->data = buffer[0]; > > + usart->data_valid = true; > > + usart->csra |= USART_CSRA_RXC; > > + if (usart->csrb & USART_CSRB_RXCIE) { > > + qemu_set_irq(usart->rxc_irq, 1); > > + } > > +} > > + > > +static void update_char_mask(AVRUsartState *usart) > > +{ > > + uint8_t mode = ((usart->csrc & USART_CSRC_CSZ0) ? 1 : 0) | > > + ((usart->csrc & USART_CSRC_CSZ1) ? 2 : 0) | > > + ((usart->csrb & USART_CSRB_CSZ2) ? 4 : 0); > > + switch (mode) { > > + case 0: > > + usart->char_mask = 0b11111; > > + break; > > + case 1: > > + usart->char_mask = 0b111111; > > + break; > > + case 2: > > + usart->char_mask = 0b1111111; > > + break; > > + case 3: > > + usart->char_mask = 0b11111111; > > + break; > > + case 4: > > + /* Fallthrough. */ > > + case 5: > > + /* Fallthrough. */ > > + case 6: > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: Reserved character size 0x%x\n", > > + __func__, > > + mode); > > + break; > > + case 7: > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: Nine bit character size not supported (forcing > eight)\n", > > + __func__); > > + usart->char_mask = 0b11111111; > > + break; > > + default: > > + assert(0); > > + } > > +} > > + > > +static void avr_usart_reset(DeviceState *dev) > > +{ > > + AVRUsartState *usart = AVR_USART(dev); > > + usart->data_valid = false; > > + usart->csra = 0b00100000; > > + usart->csrb = 0b00000000; > > + usart->csrc = 0b00000110; > > + usart->brrl = 0; > > + usart->brrh = 0; > > + update_char_mask(usart); > > + qemu_set_irq(usart->rxc_irq, 0); > > + qemu_set_irq(usart->txc_irq, 0); > > + qemu_set_irq(usart->dre_irq, 0); > > +} > > + > > +static uint64_t avr_usart_read(void *opaque, hwaddr addr, unsigned int > size) > > +{ > > + AVRUsartState *usart = opaque; > > + uint8_t prr; > > + uint8_t data; > > + assert(size == 1); > > + > > + cpu_physical_memory_read(usart->prr_address, &prr, 1); > > This looks wrong... > > You should implement the PowerManagement block as a device, map it, link > it to this device, then access directly usart->prr_link->regs[PRR0]. > How do I link devices? example please > > > + if (prr & usart->prr_mask) { > > + /* USART disabled, ignore. */ > > + avr_usart_reset(DEVICE(usart)); > > + return 0; > > + } > > + > > + switch (addr) { > > + case USART_DR: > > + if (!(usart->csrb & USART_CSRB_RXEN)) { > > + /* Receiver disabled, ignore. */ > > + return 0; > > + } > > + if (usart->data_valid) { > > + data = usart->data & usart->char_mask; > > + usart->data_valid = false; > > + } else { > > + data = 0; > > + } > > + usart->csra &= 0xff ^ USART_CSRA_RXC; > > + qemu_set_irq(usart->rxc_irq, 0); > > + qemu_chr_fe_accept_input(&usart->chr); > > + return data; > > + case USART_CSRA: > > + return usart->csra; > > + case USART_CSRB: > > + return usart->csrb; > > + case USART_CSRC: > > + return usart->csrc; > > + case USART_BRRL: > > + return usart->brrl; > > + case USART_BRRH: > > + return usart->brrh; > > + default: > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: Bad offset 0x%"HWADDR_PRIx"\n", > > + __func__, > > + addr); > > + } > > + return 0; > > +} > > + > > +static void avr_usart_write(void *opaque, hwaddr addr, uint64_t value, > > + unsigned int size) > > +{ > > + AVRUsartState *usart = opaque; > > + uint8_t mask; > > + uint8_t data; > > + assert((value & 0xff) == value); > > + assert(size == 1); > > + > > + uint8_t prr; > > + cpu_physical_memory_read(usart->prr_address, &prr, 1); > > + if (prr & usart->prr_mask) { > > + /* USART disabled, ignore. */ > > + avr_usart_reset(DEVICE(usart)); > > + return; > > + } > > + > > + switch (addr) { > > + case USART_DR: > > + if (!(usart->csrb & USART_CSRB_TXEN)) { > > + /* Transmitter disabled, ignore. */ > > + return; > > + } > > + usart->csra |= USART_CSRA_TXC; > > + usart->csra |= USART_CSRA_DRE; > > + if (usart->csrb & USART_CSRB_TXCIE) { > > + qemu_set_irq(usart->txc_irq, 1); > > + usart->csra &= 0xff ^ USART_CSRA_TXC; > > + } > > + if (usart->csrb & USART_CSRB_DREIE) { > > + qemu_set_irq(usart->dre_irq, 1); > > + } > > + data = value; > > + qemu_chr_fe_write_all(&usart->chr, &data, 1); > > + break; > > + case USART_CSRA: > > + mask = 0b01000011; > > + /* Mask read-only bits. */ > > + value = (value & mask) | (usart->csra & (0xff ^ mask)); > > + usart->csra = value; > > + if (value & USART_CSRA_TXC) { > > + usart->csra ^= USART_CSRA_TXC; > > + qemu_set_irq(usart->txc_irq, 0); > > + } > > + if (value & USART_CSRA_MPCM) { > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: MPCM not supported by USART\n", > > + __func__); > > + } > > + break; > > + case USART_CSRB: > > + mask = 0b11111101; > > + /* Mask read-only bits. */ > > + value = (value & mask) | (usart->csrb & (0xff ^ mask)); > > + usart->csrb = value; > > + if (!(value & USART_CSRB_RXEN)) { > > + /* Receiver disabled, flush input buffer. */ > > + usart->data_valid = false; > > + } > > + qemu_set_irq(usart->rxc_irq, > > + ((value & USART_CSRB_RXCIE) && > > + (usart->csra & USART_CSRA_RXC)) ? 1 : 0); > > + qemu_set_irq(usart->txc_irq, > > + ((value & USART_CSRB_TXCIE) && > > + (usart->csra & USART_CSRA_TXC)) ? 1 : 0); > > + qemu_set_irq(usart->dre_irq, > > + ((value & USART_CSRB_DREIE) && > > + (usart->csra & USART_CSRA_DRE)) ? 1 : 0); > > + update_char_mask(usart); > > + break; > > + case USART_CSRC: > > + usart->csrc = value; > > + if ((value & USART_CSRC_MSEL1) && (value & USART_CSRC_MSEL0)) { > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: SPI mode not supported by USART\n", > > + __func__); > > + } > > + if ((value & USART_CSRC_MSEL1) && !(value & USART_CSRC_MSEL0)) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad USART mode\n", > __func__); > > + } > > + if (!(value & USART_CSRC_PM1) && (value & USART_CSRC_PM0)) { > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: Bad USART parity mode\n", > > + __func__); > > + } > > + update_char_mask(usart); > > + break; > > + case USART_BRRL: > > + usart->brrl = value; > > + break; > > + case USART_BRRH: > > + usart->brrh = value & 0b00001111; > > + break; > > + default: > > + qemu_log_mask( > > + LOG_GUEST_ERROR, > > + "%s: Bad offset 0x%"HWADDR_PRIx"\n", > > + __func__, > > + addr); > > + } > > +} > > + > > +static const MemoryRegionOps avr_usart_ops = { > > + .read = avr_usart_read, > > + .write = avr_usart_write, > > This misses: > > .impl.max = 1, > > > + .endianness = DEVICE_NATIVE_ENDIAN, > > +}; > > + > > +static Property avr_usart_properties[] = { > > + DEFINE_PROP_CHR("chardev", AVRUsartState, chr), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void avr_usart_init(Object *obj) > > +{ > > + AVRUsartState *s = AVR_USART(obj); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rxc_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->dre_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->txc_irq); > > + memory_region_init_io(&s->mmio, obj, &avr_usart_ops, s, > TYPE_AVR_USART, 7); > > USART is 8 byte wide. > > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio); > > +} > > + > > +static void avr_usart_realize(DeviceState *dev, Error **errp) > > +{ > > + AVRUsartState *s = AVR_USART(dev); > > + qemu_chr_fe_set_handlers(&s->chr, avr_usart_can_receive, > > + avr_usart_receive, NULL, NULL, > > + s, NULL, true); > > + avr_usart_reset(dev); > > +} > > + > > +static void avr_usart_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->reset = avr_usart_reset; > > + dc->props = avr_usart_properties; > > + dc->realize = avr_usart_realize; > > +} > > + > > +static const TypeInfo avr_usart_info = { > > + .name = TYPE_AVR_USART, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(AVRUsartState), > > + .instance_init = avr_usart_init, > > + .class_init = avr_usart_class_init, > > +}; > > + > > +static void avr_usart_register_types(void) > > +{ > > + type_register_static(&avr_usart_info); > > +} > > + > > +type_init(avr_usart_register_types) > > diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig > > index 51921eb63f..ad754df750 100644 > > --- a/hw/timer/Kconfig > > +++ b/hw/timer/Kconfig > > @@ -61,3 +61,6 @@ config CMSDK_APB_TIMER > > config CMSDK_APB_DUALTIMER > > bool > > select PTIMER > > + > > +config AVR_TIMER16 > > + bool > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > > index 123d92c969..4f853e265d 100644 > > --- a/hw/timer/Makefile.objs > > +++ b/hw/timer/Makefile.objs > > @@ -35,6 +35,7 @@ obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o > > obj-$(CONFIG_SH4) += sh_timer.o > > obj-$(CONFIG_DIGIC) += digic-timer.o > > obj-$(CONFIG_MIPS_CPS) += mips_gictimer.o > > +obj-$(CONFIG_AVR_TIMER16) += avr_timer16.o > > > > obj-$(CONFIG_MC146818RTC) += mc146818rtc.o > > > > diff --git a/hw/timer/avr_timer16.c b/hw/timer/avr_timer16.c > > new file mode 100644 > > index 0000000000..89f8f55eaa > > --- /dev/null > > +++ b/hw/timer/avr_timer16.c > > @@ -0,0 +1,587 @@ > > +/* > > + * AVR 16 bit timer > > + * > > + * Copyright (c) 2018 University of Kent > > + * Author: Ed Robbins > > + * > > + * Permission is hereby granted, free of charge, to any person > obtaining a copy > > + * of this software and associated documentation files (the > "Software"), to deal > > + * in the Software without restriction, including without limitation > the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN > > + * THE SOFTWARE. > > + */ > > + > > +/* > > + * Driver for 16 bit timers on 8 bit AVR devices. > > + * Note: > > + * ATmega640/V-1280/V-1281/V-2560/V-2561/V timers 1, 3, 4 and 5 are 16 > bit > > + */ > > + > > +/* > > + * XXX TODO: Power Reduction Register support > > + * prescaler pause support > > + * PWM modes, GPIO, output capture pins, input compare pin > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "hw/timer/avr_timer16.h" > > +#include "qemu/log.h" > > + > > +/* Register offsets */ > > +#define T16_CRA 0x0 > > +#define T16_CRB 0x1 > > +#define T16_CRC 0x2 > > +#define T16_CNTL 0x4 > > +#define T16_CNTH 0x5 > > +#define T16_ICRL 0x6 > > +#define T16_ICRH 0x7 > > +#define T16_OCRAL 0x8 > > +#define T16_OCRAH 0x9 > > +#define T16_OCRBL 0xa > > +#define T16_OCRBH 0xb > > +#define T16_OCRCL 0xc > > +#define T16_OCRCH 0xd > > + > > +/* Field masks */ > > +#define T16_CRA_WGM01 0x3 > > +#define T16_CRA_COMC 0xc > > +#define T16_CRA_COMB 0x30 > > +#define T16_CRA_COMA 0xc0 > > +#define T16_CRA_OC_CONF \ > > + (T16_CRA_COMA | T16_CRA_COMB | T16_CRA_COMC) > > + > > +#define T16_CRB_CS 0x7 > > +#define T16_CRB_WGM23 0x18 > > +#define T16_CRB_ICES 0x40 > > +#define T16_CRB_ICNC 0x80 > > + > > +#define T16_CRC_FOCC 0x20 > > +#define T16_CRC_FOCB 0x40 > > +#define T16_CRC_FOCA 0x80 > > + > > +/* Fields masks both TIMSK and TIFR (interrupt mask/flag registers) */ > > +#define T16_INT_TOV 0x1 /* Timer overflow */ > > +#define T16_INT_OCA 0x2 /* Output compare A */ > > +#define T16_INT_OCB 0x4 /* Output compare B */ > > +#define T16_INT_OCC 0x8 /* Output compare C */ > > +#define T16_INT_IC 0x20 /* Input capture */ > > + > > +/* Clock source values */ > > +#define T16_CLKSRC_STOPPED 0 > > +#define T16_CLKSRC_DIV1 1 > > +#define T16_CLKSRC_DIV8 2 > > +#define T16_CLKSRC_DIV64 3 > > +#define T16_CLKSRC_DIV256 4 > > +#define T16_CLKSRC_DIV1024 5 > > +#define T16_CLKSRC_EXT_FALLING 6 > > +#define T16_CLKSRC_EXT_RISING 7 > > + > > +/* Timer mode values (not including PWM modes) */ > > +#define T16_MODE_NORMAL 0 > > +#define T16_MODE_CTC_OCRA 4 > > +#define T16_MODE_CTC_ICR 12 > > + > > +/* Accessors */ > > +#define CLKSRC(t16) (t16->crb & T16_CRB_CS) > > +#define MODE(t16) (((t16->crb & T16_CRB_WGM23) >> 1) | \ > > + (t16->cra & T16_CRA_WGM01)) > > +#define CNT(t16) VAL16(t16->cntl, t16->cnth) > > +#define OCRA(t16) VAL16(t16->ocral, t16->ocrah) > > +#define OCRB(t16) VAL16(t16->ocrbl, t16->ocrbh) > > +#define OCRC(t16) VAL16(t16->ocrcl, t16->ocrch) > > +#define ICR(t16) VAL16(t16->icrl, t16->icrh) > > + > > +/* Helper macros */ > > +#define VAL16(l, h) ((h << 8) | l) > > +#define ERROR(fmt, args...) \ > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__, ## args) > > +#define DB_PRINT(fmt, args...) /* Nothing */ > > +/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## > args)*/ > > + > > +static inline int64_t avr_timer16_ns_to_ticks(AVRTimer16State *t16, > int64_t t) > > +{ > > + if (t16->period_ns == 0) { > > + return 0; > > + } > > + return t / t16->period_ns; > > +} > > + > > +static void avr_timer16_update_cnt(AVRTimer16State *t16) > > +{ > > + uint16_t cnt; > > + cnt = avr_timer16_ns_to_ticks(t16, > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - > > + t16->reset_time_ns); > > + t16->cntl = (uint8_t)(cnt & 0xff); > > + t16->cnth = (uint8_t)((cnt & 0xff00) >> 8); > > +} > > + > > +static inline void avr_timer16_recalc_reset_time(AVRTimer16State *t16) > > +{ > > + t16->reset_time_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - > > + CNT(t16) * t16->period_ns; > > +} > > + > > +static void avr_timer16_clock_reset(AVRTimer16State *t16) > > +{ > > + t16->cntl = 0; > > + t16->cnth = 0; > > + t16->reset_time_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > +} > > + > > +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; > > + DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f > s)", > > + t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz); > > +end: > > + return; > > +} > > + > > +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"); > > + 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; > > +} > > + > > +static void avr_timer16_interrupt(void *opaque) > > +{ > > + AVRTimer16State *t16 = opaque; > > + uint8_t mode = MODE(t16); > > + > > + avr_timer16_update_cnt(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) */ > > + return; > > + } > > + > > + DB_PRINT("interrupt, cnt = %d", CNT(t16)); > > + > > + /* Counter overflow */ > > + if (t16->next_interrupt == OVERFLOW) { > > + DB_PRINT("0xffff overflow"); > > + avr_timer16_clock_reset(t16); > > + if (t16->imsk & T16_INT_TOV) { > > + t16->ifr |= T16_INT_TOV; > > + qemu_set_irq(t16->ovf_irq, 1); > > + } > > + } > > + /* Check for ocra overflow in CTC mode */ > > + if (mode == T16_MODE_CTC_OCRA && t16->next_interrupt == COMPA) { > > + DB_PRINT("CTC OCRA overflow"); > > + avr_timer16_clock_reset(t16); > > + } > > + /* Check for icr overflow in CTC mode */ > > + if (mode == T16_MODE_CTC_ICR && t16->next_interrupt == CAPT) { > > + DB_PRINT("CTC ICR overflow"); > > + avr_timer16_clock_reset(t16); > > + if (t16->imsk & T16_INT_IC) { > > + t16->ifr |= T16_INT_IC; > > + qemu_set_irq(t16->capt_irq, 1); > > + } > > + } > > + /* Check for output compare interrupts */ > > + if (t16->imsk & T16_INT_OCA && t16->next_interrupt == COMPA) { > > + t16->ifr |= T16_INT_OCA; > > + qemu_set_irq(t16->compa_irq, 1); > > + } > > + if (t16->imsk & T16_INT_OCB && t16->next_interrupt == COMPB) { > > + t16->ifr |= T16_INT_OCB; > > + qemu_set_irq(t16->compb_irq, 1); > > + } > > + if (t16->imsk & T16_INT_OCC && t16->next_interrupt == COMPC) { > > + t16->ifr |= T16_INT_OCC; > > + qemu_set_irq(t16->compc_irq, 1); > > + } > > + avr_timer16_set_alarm(t16); > > +} > > + > > +static void avr_timer16_reset(DeviceState *dev) > > +{ > > + AVRTimer16State *t16 = AVR_TIMER16(dev); > > + > > + avr_timer16_clock_reset(t16); > > + avr_timer16_clksrc_update(t16); > > + avr_timer16_set_alarm(t16); > > + > > + qemu_set_irq(t16->capt_irq, 0); > > + qemu_set_irq(t16->compa_irq, 0); > > + qemu_set_irq(t16->compb_irq, 0); > > + qemu_set_irq(t16->compc_irq, 0); > > + qemu_set_irq(t16->ovf_irq, 0); > > +} > > + > > +static uint64_t avr_timer16_read(void *opaque, hwaddr offset, unsigned > size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + uint8_t retval = 0; > > + > > + switch (offset) { > > + case T16_CRA: > > + retval = t16->cra; > > + break; > > + case T16_CRB: > > + retval = t16->crb; > > + break; > > + case T16_CRC: > > + retval = t16->crc; > > + break; > > + case T16_CNTL: > > + avr_timer16_update_cnt(t16); > > + t16->rtmp = t16->cnth; > > + retval = t16->cntl; > > + break; > > + case T16_CNTH: > > + retval = t16->rtmp; > > + break; > > + case T16_ICRL: > > + /* > > + * The timer copies cnt to icr when the input capture pin > changes > > + * state or when the analog comparator has a match. We don't > > + * emulate this behaviour. We do support it's use for defining a > > + * TOP value in T16_MODE_CTC_ICR > > + */ > > + t16->rtmp = t16->icrh; > > + retval = t16->icrl; > > + break; > > + case T16_ICRH: > > + retval = t16->rtmp; > > + break; > > + case T16_OCRAL: > > + retval = t16->ocral; > > + break; > > + case T16_OCRAH: > > + retval = t16->ocrah; > > + break; > > + case T16_OCRBL: > > + retval = t16->ocrbl; > > + break; > > + case T16_OCRBH: > > + retval = t16->ocrbh; > > + break; > > + case T16_OCRCL: > > + retval = t16->ocrcl; > > + break; > > + case T16_OCRCH: > > + retval = t16->ocrch; > > + break; > > + default: > > + break; > > + } > > + return (uint64_t)retval; > > +} > > + > > +static void avr_timer16_write(void *opaque, hwaddr offset, > > + uint64_t val64, unsigned size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + uint8_t val8 = (uint8_t)val64; > > + uint8_t prev_clk_src = CLKSRC(t16); > > + > > + DB_PRINT("write %d to offset %d", val8, (uint8_t)offset); > > + > > + switch (offset) { > > + case T16_CRA: > > + t16->cra = val8; > > + if (t16->cra & T16_CRA_OC_CONF) { > > + ERROR("output compare pins unsupported"); > > + } > > + break; > > + case T16_CRB: > > + t16->crb = val8; > > + if (t16->crb & T16_CRB_ICNC) { > > + ERROR("input capture noise canceller unsupported"); > > + } > > + if (t16->crb & T16_CRB_ICES) { > > + ERROR("input capture unsupported"); > > + } > > + if (CLKSRC(t16) != prev_clk_src) { > > + avr_timer16_clksrc_update(t16); > > + if (prev_clk_src == T16_CLKSRC_STOPPED) { > > + t16->reset_time_ns = > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > + } > > + } > > + break; > > + case T16_CRC: > > + t16->crc = val8; > > + ERROR("output compare pins unsupported"); > > + break; > > + case T16_CNTL: > > + /* > > + * CNT is the 16-bit counter value, it must be read/written via > > + * a temporary register (rtmp) to make the read/write atomic. > > + */ > > + /* ICR also has this behaviour, and shares rtmp */ > > + /* > > + * Writing CNT blocks compare matches for one clock cycle. > > + * Writing CNT to TOP or to an OCR value (if in use) will > > + * skip the relevant interrupt > > + */ > > + t16->cntl = val8; > > + t16->cnth = t16->rtmp; > > + avr_timer16_recalc_reset_time(t16); > > + break; > > + case T16_CNTH: > > + t16->rtmp = val8; > > + break; > > + case T16_ICRL: > > + /* ICR can only be written in mode T16_MODE_CTC_ICR */ > > + if (MODE(t16) == T16_MODE_CTC_ICR) { > > + t16->icrl = val8; > > + t16->icrh = t16->rtmp; > > + } > > + break; > > + case T16_ICRH: > > + if (MODE(t16) == T16_MODE_CTC_ICR) { > > + t16->rtmp = val8; > > + } > > + break; > > + case T16_OCRAL: > > + /* > > + * OCRn cause the relevant output compare flag to be raised, and > > + * trigger an interrupt, when CNT is equal to the value here > > + */ > > + t16->ocral = val8; > > + break; > > + case T16_OCRAH: > > + t16->ocrah = val8; > > + break; > > + case T16_OCRBL: > > + t16->ocrbl = val8; > > + break; > > + case T16_OCRBH: > > + t16->ocrbh = val8; > > + break; > > + case T16_OCRCL: > > + t16->ocrcl = val8; > > + break; > > + case T16_OCRCH: > > + t16->ocrch = val8; > > + break; > > + default: > > + break; > > + } > > + avr_timer16_set_alarm(t16); > > +} > > + > > +static uint64_t avr_timer16_imsk_read(void *opaque, > > + hwaddr offset, > > + unsigned size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + if (offset != 0) { > > + return 0; > > + } > > + return t16->imsk; > > +} > > + > > +static void avr_timer16_imsk_write(void *opaque, hwaddr offset, > > + uint64_t val64, unsigned size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + if (offset != 0) { > > + return; > > + } > > + t16->imsk = (uint8_t)val64; > > +} > > + > > +static uint64_t avr_timer16_ifr_read(void *opaque, > > + hwaddr offset, > > + unsigned size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + if (offset != 0) { > > + return 0; > > + } > > + return t16->ifr; > > +} > > + > > +static void avr_timer16_ifr_write(void *opaque, hwaddr offset, > > + uint64_t val64, unsigned size) > > +{ > > + assert(size == 1); > > + AVRTimer16State *t16 = opaque; > > + if (offset != 0) { > > + return; > > + } > > + t16->ifr = (uint8_t)val64; > > +} > > + > > +static const MemoryRegionOps avr_timer16_ops = { > > + .read = avr_timer16_read, > > + .write = avr_timer16_write > if you use .impl { .min = 2 } you simplify your design. > > > + .endianness = DEVICE_NATIVE_ENDIAN > > +}; > > + > > +static const MemoryRegionOps avr_timer16_imsk_ops = { > > + .read = avr_timer16_imsk_read, > > + .write = avr_timer16_imsk_write, > > + .endianness = DEVICE_NATIVE_ENDIAN > > +}; > > + > > +static const MemoryRegionOps avr_timer16_ifr_ops = { > > + .read = avr_timer16_ifr_read, > > + .write = avr_timer16_ifr_write, > > + .endianness = DEVICE_NATIVE_ENDIAN > > +}; > > + > > +static Property avr_timer16_properties[] = { > > + DEFINE_PROP_UINT64("cpu-frequency-hz", struct AVRTimer16State, > > + cpu_freq_hz, 20000000), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void avr_timer16_init(Object *obj) > > +{ > > + AVRTimer16State *s = AVR_TIMER16(obj); > > + > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->capt_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->compa_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->compb_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->compc_irq); > > + sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->ovf_irq); > > + > > + memory_region_init_io(&s->iomem, obj, &avr_timer16_ops, > > + s, TYPE_AVR_TIMER16, 0xe); > > A timer region is 0x10 wide, Ok. > > > + memory_region_init_io(&s->imsk_iomem, obj, &avr_timer16_imsk_ops, > > + s, TYPE_AVR_TIMER16, 0x1); > > + memory_region_init_io(&s->ifr_iomem, obj, &avr_timer16_ifr_ops, > > + s, TYPE_AVR_TIMER16, 0x1); > > These 2 registers seem part of the Interrupt Controller, and don't > belong here. You might want to use a LINK_PROPERTY to link a timer > device to the INTC. > How do I do it? > > > + > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem); > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->imsk_iomem); > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->ifr_iomem); > > + > > + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, avr_timer16_interrupt, > s); > > +} > > + > > +static void avr_timer16_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->reset = avr_timer16_reset; > > + dc->props = avr_timer16_properties; > > +} > > + > > +static const TypeInfo avr_timer16_info = { > > + .name = TYPE_AVR_TIMER16, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(AVRTimer16State), > > + .instance_init = avr_timer16_init, > > + .class_init = avr_timer16_class_init, > > +}; > > + > > +static void avr_timer16_register_types(void) > > +{ > > + type_register_static(&avr_timer16_info); > > +} > > + > > +type_init(avr_timer16_register_types) > > diff --git a/include/hw/char/avr_usart.h b/include/hw/char/avr_usart.h > > new file mode 100644 > > index 0000000000..ba28fbe5d1 > > --- /dev/null > > +++ b/include/hw/char/avr_usart.h > > @@ -0,0 +1,99 @@ > > +/* > > + * AVR USART > > + * > > + * Copyright (c) 2018 University of Kent > > + * Author: Sarah Harris > > + * > > + * Permission is hereby granted, free of charge, to any person > obtaining a copy > > + * of this software and associated documentation files (the > "Software"), to deal > > + * in the Software without restriction, including without limitation > the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN > > + * THE SOFTWARE. > > + */ > > + > > +#ifndef HW_AVR_USART_H > > +#define HW_AVR_USART_H > > + > > +#include "hw/sysbus.h" > > +#include "chardev/char-fe.h" > > +#include "hw/hw.h" > > + > > +/* Offsets of registers. */ > > +#define USART_DR 0x06 > > +#define USART_CSRA 0x00 > > +#define USART_CSRB 0x01 > > +#define USART_CSRC 0x02 > > +#define USART_BRRH 0x05 > > +#define USART_BRRL 0x04 > > + > > +/* Relevant bits in regiters. */ > > +#define USART_CSRA_RXC (1 << 7) > > +#define USART_CSRA_TXC (1 << 6) > > +#define USART_CSRA_DRE (1 << 5) > > +#define USART_CSRA_MPCM (1 << 0) > > + > > +#define USART_CSRB_RXCIE (1 << 7) > > +#define USART_CSRB_TXCIE (1 << 6) > > +#define USART_CSRB_DREIE (1 << 5) > > +#define USART_CSRB_RXEN (1 << 4) > > +#define USART_CSRB_TXEN (1 << 3) > > +#define USART_CSRB_CSZ2 (1 << 2) > > +#define USART_CSRB_RXB8 (1 << 1) > > +#define USART_CSRB_TXB8 (1 << 0) > > + > > +#define USART_CSRC_MSEL1 (1 << 7) > > +#define USART_CSRC_MSEL0 (1 << 6) > > +#define USART_CSRC_PM1 (1 << 5) > > +#define USART_CSRC_PM0 (1 << 4) > > +#define USART_CSRC_CSZ1 (1 << 2) > > +#define USART_CSRC_CSZ0 (1 << 1) > > + > > +#define TYPE_AVR_USART "avr-usart" > > +#define AVR_USART(obj) \ > > + OBJECT_CHECK(AVRUsartState, (obj), TYPE_AVR_USART) > > + > > +typedef struct { > > + /* <private> */ > > + SysBusDevice parent_obj; > > + > > + /* <public> */ > > + MemoryRegion mmio; > > + > > + CharBackend chr; > > + > > + /* Address of Power Reduction Register and bit that controls this > UART */ > > + hwaddr prr_address; > > + uint8_t prr_mask; > > So ideally you'd use a link to the PowerManagement block here. > Please explain. > > > + > > + uint8_t data; > > + bool data_valid; > > + uint8_t char_mask; > > + /* Control and Status Registers */ > > + uint8_t csra; > > + uint8_t csrb; > > + uint8_t csrc; > > + /* Baud Rate Registers (low/high byte) */ > > + uint8_t brrh; > > + uint8_t brrl; > > + > > + /* Receive Complete */ > > + qemu_irq rxc_irq; > > + /* Transmit Complete */ > > + qemu_irq txc_irq; > > + /* Data Register Empty */ > > + qemu_irq dre_irq; > > +} AVRUsartState; > > + > > +#endif /* HW_AVR_USART_H */ > > diff --git a/include/hw/timer/avr_timer16.h > b/include/hw/timer/avr_timer16.h > > new file mode 100644 > > index 0000000000..301e36a154 > > --- /dev/null > > +++ b/include/hw/timer/avr_timer16.h > > @@ -0,0 +1,99 @@ > > +/* > > + * AVR 16 bit timer > > + * > > + * Copyright (c) 2018 University of Kent > > + * Author: Ed Robbins > > + * > > + * Permission is hereby granted, free of charge, to any person > obtaining a copy > > + * of this software and associated documentation files (the > "Software"), to deal > > + * in the Software without restriction, including without limitation > the rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN > > + * THE SOFTWARE. > > + */ > > + > > +/* > > + * Driver for 16 bit timers on 8 bit AVR devices. > > + * Note: > > + * On ATmega640/V-1280/V-1281/V-2560/V-2561/V timers 1, 3, 4 and 5 are > 16 bit > > + */ > > + > > +#ifndef AVR_TIMER16_H > > +#define AVR_TIMER16_H > > + > > +#include "hw/sysbus.h" > > +#include "qemu/timer.h" > > +#include "hw/hw.h" > > + > > +enum NextInterrupt { > > + OVERFLOW, > > + COMPA, > > + COMPB, > > + COMPC, > > + CAPT > > +}; > > + > > +#define TYPE_AVR_TIMER16 "avr-timer16" > > +#define AVR_TIMER16(obj) \ > > + OBJECT_CHECK(AVRTimer16State, (obj), TYPE_AVR_TIMER16) > > + > > +typedef struct AVRTimer16State { > > + /* <private> */ > > + SysBusDevice parent_obj; > > + > > + /* <public> */ > > + MemoryRegion iomem; > > + MemoryRegion imsk_iomem; > > + MemoryRegion ifr_iomem; > > + QEMUTimer *timer; > > + qemu_irq capt_irq; > > + qemu_irq compa_irq; > > + qemu_irq compb_irq; > > + qemu_irq compc_irq; > > + qemu_irq ovf_irq; > > + > > + /* Address of Power Reduction Register and bit that controls this > timer */ > > + hwaddr prr_address; > > + uint8_t prr_mask; > > Again, you should use a link to the PowerManagement block here. > > > + > > + /* registers */ > > + uint8_t cra; > > + uint8_t crb; > > + uint8_t crc; > > Using 16-bit for following... > > > + uint8_t cntl; > > + uint8_t cnth; > > + uint8_t icrl; > > + uint8_t icrh; > > + uint8_t ocral; > > + uint8_t ocrah; > > + uint8_t ocrbl; > > + uint8_t ocrbh; > > + uint8_t ocrcl; > > + uint8_t ocrch; > > ... until here would simplify your model. > > > + /* > > + * Reads and writes to CNT and ICR utilise a bizarre temporary > > + * register, which we emulate > > + */ > > + uint8_t rtmp; > > + uint8_t imsk; > > + uint8_t ifr; > > + > > + uint64_t cpu_freq_hz; > > + uint64_t freq_hz; > > + uint64_t period_ns; > > + uint64_t reset_time_ns; > > + enum NextInterrupt next_interrupt; > > +} AVRTimer16State; > > + > > +#endif /* AVR_TIMER16_H */ > > > > If the maintainer taking your series don't think my comments are > blockers, you might consider them as later cleanup. > > Regards, > > Phil. > -- Best Regards, Michael Rolnik