On Thu, 2016-11-17 at 09:29 +0100, Cédric Le Goater wrote: On 11/17/2016 05:36 AM, Alastair D'Silva wrote: > > > > From: Alastair D'Silva <alast...@d-silva.org> > > > > This patch adds support for the Epson RX8900 RTC chip. > > It would be nice to have a short list of the features this > chip has and also the main point of the design. I see you > are using a BH. >
Ok > > > > > Signed-off-by: Alastair D'Silva <alast...@d-silva.org> > > --- > > default-configs/arm-softmmu.mak | 1 + > > hw/timer/Makefile.objs | 2 + > > hw/timer/rx8900.c | 891 > > ++++++++++++++++++++++++++++++++++++++++ > > hw/timer/rx8900_regs.h | 125 ++++++ > > tests/Makefile.include | 2 + > > tests/rx8900-test.c | 800 > > ++++++++++++++++++++++++++++++++++++ > > Nice test ! But Why aren't you using the aspeed machine in > qtest ? > > The reason I am asking is because the I2C controller model > is a little too simplistic in the way it handles the irq > status and we would need a test for it. > The aspeed machine is missing a bunch of I2C infrastructure (imx_i2c_create & friends) required to access it from the test harness, and I don't have the knowledge required to implement it. I am working on the premise that the RX8900 is an independent device, and so can be tested independently of the aspeed model. > > Also I would put the test case in another patch, after the > model and after patch 2 also which introduces named > interrupts for qtest. > Ok > > > > > 6 files changed, 1821 insertions(+) > > create mode 100644 hw/timer/rx8900.c > > create mode 100644 hw/timer/rx8900_regs.h > > create mode 100644 tests/rx8900-test.c > > > > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm- > > softmmu.mak > > index 6de3e16..adb600e 100644 > > --- a/default-configs/arm-softmmu.mak > > +++ b/default-configs/arm-softmmu.mak > > @@ -29,6 +29,7 @@ CONFIG_SMC91C111=y > > CONFIG_ALLWINNER_EMAC=y > > CONFIG_IMX_FEC=y > > CONFIG_DS1338=y > > +CONFIG_RX8900=y > > CONFIG_PFLASH_CFI01=y > > CONFIG_PFLASH_CFI02=y > > CONFIG_MICRODRIVE=y > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > > index 7ba8c23..fa028ac 100644 > > --- a/hw/timer/Makefile.objs > > +++ b/hw/timer/Makefile.objs > > @@ -3,6 +3,7 @@ common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o > > common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o > > common-obj-$(CONFIG_CADENCE) += cadence_ttc.o > > common-obj-$(CONFIG_DS1338) += ds1338.o > > +common-obj-$(CONFIG_RX8900) += rx8900.o > > common-obj-$(CONFIG_HPET) += hpet.o > > common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o > > common-obj-$(CONFIG_M48T59) += m48t59.o > > @@ -17,6 +18,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o > > common-obj-$(CONFIG_IMX) += imx_gpt.o > > common-obj-$(CONFIG_LM32) += lm32_timer.o > > common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o > > +common-obj-$(CONFIG_RX8900) += rx8900.o > > > > obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o > > obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o > > diff --git a/hw/timer/rx8900.c b/hw/timer/rx8900.c > > new file mode 100644 > > index 0000000..208a31b > > --- /dev/null > > +++ b/hw/timer/rx8900.c > > @@ -0,0 +1,891 @@ > > +/* > > + * Epson RX8900SA/CE Realtime Clock Module > > + * > > + * Copyright (c) 2016 IBM Corporation > > + * Authors: > > + * Alastair D'Silva <alast...@d-silva.org> > > + * > > + * This code is licensed under the GPL version 2 or later. See > > + * the COPYING file in the top-level directory. > > + * > > + * Datasheet available at: > > + * https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE > > &lang=en > > + * > > + * Not implemented: > > + * Implement Timer Counters > > + * Implement i2c timeout > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "qemu-common.h" > > +#include "hw/i2c/i2c.h" > > +#include "hw/timer/rx8900_regs.h" > > +#include "hw/ptimer.h" > > +#include "qemu/main-loop.h" > > +#include "qemu/bcd.h" > > +#include "qemu/error-report.h" > > +#include "qemu/log.h" > > +#include "qapi/error.h" > > +#include "qapi/visitor.h" > > + > > + #include <sys/time.h> > > + > > + #include <execinfo.h> > > + > > +#define TYPE_RX8900 "rx8900" > > +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900) > > + > > +static bool log; > > + > > +typedef struct RX8900State { > > + I2CSlave parent_obj; > > + > > + ptimer_state *sec_timer; /* triggered once per second */ > > + ptimer_state *fout_timer; > > + ptimer_state *countdown_timer; > > + bool fout; > > + int64_t offset; > > + uint8_t weekday; /* Saved for deferred offset calculation, 0-6 > > */ > > + uint8_t wday_offset; > > + uint8_t nvram[RX8900_NVRAM_SIZE]; > > + int32_t ptr; /* Wrapped to stay within RX8900_NVRAM_SIZE */ > > + bool addr_byte; > > + uint8_t last_interrupt_seconds; > > + uint8_t last_update_interrupt_minutes; > > + qemu_irq interrupt_pin; > > + qemu_irq fout_pin; > > +} RX8900State; > > + > > +static const VMStateDescription vmstate_rx8900 = { > > + .name = "rx8900", > > + .version_id = 2, > > + .minimum_version_id = 1, > > + .fields = (VMStateField[]) { > > + VMSTATE_I2C_SLAVE(parent_obj, RX8900State), > > + VMSTATE_PTIMER(sec_timer, RX8900State), > > + VMSTATE_PTIMER(fout_timer, RX8900State), > > + VMSTATE_PTIMER(countdown_timer, RX8900State), > > + VMSTATE_BOOL(fout, RX8900State), > > + VMSTATE_INT64(offset, RX8900State), > > + VMSTATE_UINT8_V(weekday, RX8900State, 2), > > + VMSTATE_UINT8_V(wday_offset, RX8900State, 2), > > + VMSTATE_UINT8_ARRAY(nvram, RX8900State, > > RX8900_NVRAM_SIZE), > > + VMSTATE_INT32(ptr, RX8900State), > > + VMSTATE_BOOL(addr_byte, RX8900State), > > + VMSTATE_UINT8_V(last_interrupt_seconds, RX8900State, 2), > > + VMSTATE_UINT8_V(last_update_interrupt_minutes, > > RX8900State, 2), > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > +static void rx8900_reset(DeviceState *dev); > > +static void disable_countdown_timer(RX8900State *s); > > +static void enable_countdown_timer(RX8900State *s); > > +static void disable_timer(RX8900State *s); > > +static void enable_timer(RX8900State *s); > > + > > +#ifdef RX8900_TRACE > > +#define RX8900_TRACE_BUF_SIZE 256 > > +/** > > + * Emit a trace message > > + * @param file the source filename > > + * @param line the line number the message was emitted from > > + * @param dev the RX8900 device > > + * @param fmt a printf style format > > + */ > > +static void trace(const char *file, int line, const char *func, > > + I2CSlave *dev, const char *fmt, ...) > > +{ > > + va_list ap; > > + char buf[RX8900_TRACE_BUF_SIZE]; > > + char timestamp[32]; > > + int len; > > + struct timeval now; > > + struct tm *now2; > > + > > + gettimeofday(&now, NULL); > > + now2 = localtime(&now.tv_sec); > > + > > + strftime(timestamp, sizeof(timestamp), "%F %T", now2); > > + > > + len = snprintf(buf, sizeof(buf), "\n\t%s.%03ld %s:%s:%d: > > RX8900 %s %s@0x%x: %s", > > + timestamp, now.tv_usec / 1000, > > + file, func, line, dev->qdev.id, dev->qdev.parent_bus- > > >name, > > + dev->address, fmt); > > + if (len >= RX8900_TRACE_BUF_SIZE) { > > + error_report("%s:%d: Trace buffer overflow", file, line); > > + } > > + > > + va_start(ap, fmt); > > + error_vreport(buf, ap); > > + va_end(ap); > > +} > > + > > +/** > > + * Emit a trace message > > + * @param dev the RX8900 device > > + * @param fmt a printf format > > + */ > > +#define TRACE(dev, fmt, ...) \ > > + do { \ > > + if (log) { \ > > + trace(__FILE__, __LINE__, __func__, &dev, fmt, ## > > __VA_ARGS__); \ > > + } \ > > + } while (0) > > +#else > > +#define TRACE(dev, fmt, ...) > > +#endif > > > Although this is very pratical and nicely written, I don't > think you can keep these TRACE calls in the code. You should > use the qemu trace subsystem for it. > Ok. Would it be worth expanding this to include the file & line, or do you expect resistance? > > > > > +static void capture_current_time(RX8900State *s) > > +{ > > + /* Capture the current time into the secondary registers > > + * which will be actually read by the data transfer operation. > > + */ > > + struct tm now; > > + qemu_get_timedate(&now, s->offset); > > + s->nvram[SECONDS] = to_bcd(now.tm_sec); > > + s->nvram[MINUTES] = to_bcd(now.tm_min); > > + s->nvram[HOURS] = to_bcd(now.tm_hour); > > + > > + s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + s->wday_offset) % > > 7); > > + s->nvram[DAY] = to_bcd(now.tm_mday); > > + s->nvram[MONTH] = to_bcd(now.tm_mon + 1); > > + s->nvram[YEAR] = to_bcd(now.tm_year % 100); > > + > > + s->nvram[EXT_SECONDS] = s->nvram[SECONDS]; > > + s->nvram[EXT_MINUTES] = s->nvram[MINUTES]; > > + s->nvram[EXT_HOURS] = s->nvram[HOURS]; > > + s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY]; > > + s->nvram[EXT_DAY] = s->nvram[DAY]; > > + s->nvram[EXT_MONTH] = s->nvram[MONTH]; > > + s->nvram[EXT_YEAR] = s->nvram[YEAR]; > > + > > + TRACE(s->parent_obj, "Update current time to %02d:%02d:%02d %d > > %d/%d/%d " > > + "(0x%02x%02x%02x%02x%02x%02x%02x)", > > + now.tm_hour, now.tm_min, now.tm_sec, > > + (now.tm_wday + s->wday_offset) % 7, > > + now.tm_mday, now.tm_mon, now.tm_year + 1900, > > + s->nvram[HOURS], s->nvram[MINUTES], s->nvram[SECONDS], > > + s->nvram[WEEKDAY], > > + s->nvram[DAY], s->nvram[MONTH], s->nvram[YEAR]); > > +} > > + > > +/** > > + * Increment the internal register pointer, dealing with wrapping > > + * @param s the RTC to operate on > > + */ > > +static void inc_regptr(RX8900State *s) > > +{ > > + /* The register pointer wraps around after 0x1F > > + */ > > + s->ptr = (s->ptr + 1) & (RX8900_NVRAM_SIZE - 1); > > + TRACE(s->parent_obj, "Operating on register 0x%02x", s->ptr); > > + > > + if (s->ptr == 0x00) { > > + TRACE(s->parent_obj, "Register pointer has overflowed, > > wrapping to 0"); > > + capture_current_time(s); > > + } > > +} > > + > > +/** > > + * Receive an I2C Event > > + * @param i2c the i2c device instance > > + * @param event the event to handle > > + */ > > +static void rx8900_event(I2CSlave *i2c, enum i2c_event event) > > +{ > > + RX8900State *s = RX8900(i2c); > > + > > + switch (event) { > > + case I2C_START_RECV: > > + /* In h/w, time capture happens on any START condition, > > not just a > > + * START_RECV. For the emulation, it doesn't actually > > matter, > > + * since a START_RECV has to occur before the data can be > > read. > > + */ > > + capture_current_time(s); > > + break; > > + case I2C_START_SEND: > > + s->addr_byte = true; > > + break; > > + case I2C_FINISH: > > + if (s->weekday < 7) { > > + /* We defer the weekday calculation as it is handed to > > us before > > + * the date has been updated. If we calculate the > > weekday offset > > + * when it is passed to us, we will incorrectly > > determine it > > + * based on the current emulated date, rather than the > > date that > > + * has been written. > > + */ > > + struct tm now; > > + qemu_get_timedate(&now, s->offset); > > + > > + s->wday_offset = (s->weekday - now.tm_wday + 7) % 7; > > + > > + TRACE(s->parent_obj, "Set weekday to %d (0x%02x), > > wday_offset=%d", > > + s->weekday, BIT(s->weekday), s->wday_offset); > > + > > + s->weekday = 7; > > + } > > + break; > > + > > + default: > > + break; > > + } > > +} > > + > > +/** > > + * Perform an i2c receive action > > + * @param i2c the i2c device instance > > + * @return the value of the current register > > + * @post the internal register pointer is incremented > > + */ > > +static int rx8900_recv(I2CSlave *i2c) > > +{ > > + RX8900State *s = RX8900(i2c); > > + uint8_t res = s->nvram[s->ptr]; > > + TRACE(s->parent_obj, "Read register 0x%x = 0x%x", s->ptr, > > res); > > + inc_regptr(s); > > + return res; > > +} > > + > > +/** > > + * Validate the extension register and perform actions based on > > the bits > > + * @param s the RTC to operate on > > + * @param data the new data for the extension register > > + */ > > +static void update_extension_register(RX8900State *s, uint8_t > > data) > > +{ > > + if (data & EXT_MASK_TEST) { > > + error_report("WARNING: RX8900 - " > > + "Test bit is enabled but is forbidden by the > > manufacturer"); > > may be use instead : > > qemu_log_mask(LOG_GUEST_ERROR, > OK > > > > + } > > + > > + if ((data ^ s->nvram[EXTENSION_REGISTER]) & > > + (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) { > > + uint8_t fsel = (data & (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) > > + >> EXT_REG_FSEL0; > > + /* FSELx has changed */ > > + switch (fsel) { > > + case 0x01: > > + TRACE(s->parent_obj, "Setting fout to 1024Hz"); > > + ptimer_set_limit(s->fout_timer, 32, 1); > > + break; > > + case 0x02: > > + TRACE(s->parent_obj, "Setting fout to 1Hz"); > > + ptimer_set_limit(s->fout_timer, 32768, 1); > > + break; > > + default: > > + TRACE(s->parent_obj, "Setting fout to 32768Hz"); > > + ptimer_set_limit(s->fout_timer, 1, 1); > > + break; > > + } > > + } > > + > > + if ((data ^ s->nvram[EXTENSION_REGISTER]) & > > + (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) { > > + uint8_t tsel = (data & (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) > > + >> EXT_REG_TSEL0; > > + /* TSELx has changed */ > > + switch (tsel) { > > + case 0x00: > > + TRACE(s->parent_obj, "Setting countdown timer to 64 > > Hz"); > > + ptimer_set_limit(s->countdown_timer, 4096 / 64, 1); > > + break; > > + case 0x01: > > + TRACE(s->parent_obj, "Setting countdown timer to 1 > > Hz"); > > + ptimer_set_limit(s->countdown_timer, 4096, 1); > > + break; > > + case 0x02: > > + TRACE(s->parent_obj, > > + "Setting countdown timer to per minute > > updates"); > > + ptimer_set_limit(s->countdown_timer, 4069 * 60, 1); > > + break; > > + case 0x03: > > + TRACE(s->parent_obj, "Setting countdown timer to > > 4096Hz"); > > + ptimer_set_limit(s->countdown_timer, 1, 1); > > + break; > > + } > > + } > > + > > + if (data & EXT_MASK_TE) { > > + enable_countdown_timer(s); > > + } > > + > > + s->nvram[EXTENSION_REGISTER] = data; > > + s->nvram[EXT_EXTENSION_REGISTER] = data; > > + > > +} > > +/** > > + * Validate the control register and perform actions based on the > > bits > > + * @param s the RTC to operate on > > + * @param data the new value for the control register > > + */ > > + > > +static void update_control_register(RX8900State *s, uint8_t data) > > +{ > > + uint8_t diffmask = ~s->nvram[CONTROL_REGISTER] & data; > > + > > + if (diffmask & CTRL_MASK_WP0) { > > + data &= ~CTRL_MASK_WP0; > > + error_report("WARNING: RX8900 - " > > + "Attempt to write to write protected bit %d in control > > register", > > + CTRL_REG_WP0); > > > may be use instead : > > qemu_log_mask(LOG_GUEST_ERROR, > > > > > + } > > + > > + if (diffmask & CTRL_MASK_WP1) { > > + data &= ~CTRL_MASK_WP1; > > + error_report("WARNING: RX8900 - " > > + "Attempt to write to write protected bit %d in control > > register", > > + CTRL_REG_WP1); > > ditto for all in fact. > > > > > + } > > + > > + if (data & CTRL_MASK_RESET) { > > + data &= ~CTRL_MASK_RESET; > > + rx8900_reset(DEVICE(s)); > > + } > > + > > + if (diffmask & CTRL_MASK_UIE) { > > + /* Update interrupts were off and are now on */ > > + struct tm now; > > + > > + TRACE(s->parent_obj, "Enabling update timer"); > > + > > + qemu_get_timedate(&now, s->offset); > > + > > + s->last_update_interrupt_minutes = now.tm_min; > > + s->last_interrupt_seconds = now.tm_sec; > > + enable_timer(s); > > + } > > + > > + if (diffmask & CTRL_MASK_AIE) { > > + /* Alarm interrupts were off and are now on */ > > + struct tm now; > > + > > + TRACE(s->parent_obj, "Enabling alarm"); > > + > > + qemu_get_timedate(&now, s->offset); > > + > > + s->last_interrupt_seconds = now.tm_sec; > > + enable_timer(s); > > + } > > + > > + if (!(data & (CTRL_MASK_UIE | CTRL_MASK_AIE))) { > > + disable_timer(s); > > + } > > + > > + if (data & CTRL_MASK_TIE) { > > + enable_countdown_timer(s); > > + } > > + > > + s->nvram[CONTROL_REGISTER] = data; > > + s->nvram[EXT_CONTROL_REGISTER] = data; > > +} > > + > > +/** > > + * Validate the flag register > > + * @param s the RTC to operate on > > + * @param data the new value for the flag register > > + */ > > +static void validate_flag_register(RX8900State *s, uint8_t *data) > > +{ > > + uint8_t diffmask = ~s->nvram[FLAG_REGISTER] & *data; > > + > > + if (diffmask & FLAG_MASK_VDET) { > > + *data &= ~FLAG_MASK_VDET; > > + error_report("WARNING: RX8900 - " > > + "Only 0 can be written to VDET bit %d in the flag > > register", > > + FLAG_REG_VDET); > > + } > > + > > + if (diffmask & FLAG_MASK_VLF) { > > + *data &= ~FLAG_MASK_VLF; > > + error_report("WARNING: RX8900 - " > > + "Only 0 can be written to VLF bit %d in the flag > > register", > > + FLAG_REG_VLF); > > + } > > + > > + if (diffmask & FLAG_MASK_UNUSED_2) { > > + *data &= ~FLAG_MASK_UNUSED_2; > > + error_report("WARNING: RX8900 - " > > + "Only 0 can be written to unused bit %d in the flag > > register", > > + FLAG_REG_UNUSED_2); > > + } > > + > > + if (diffmask & FLAG_MASK_UNUSED_6) { > > + *data &= ~FLAG_MASK_UNUSED_6; > > + error_report("WARNING: RX8900 - " > > + "Only 0 can be written to unused bit %d in the flag > > register", > > + FLAG_REG_UNUSED_6); > > + } > > + > > + if (diffmask & FLAG_MASK_UNUSED_7) { > > + *data &= ~FLAG_MASK_UNUSED_7; > > + error_report("WARNING: RX8900 - " > > + "Only 0 can be written to unused bit %d in the flag > > register", > > + FLAG_REG_UNUSED_7); > > + } > > +} > > + > > +/** > > + * Tick the per second timer (can be called more frequently as it > > early exits > > + * if the wall clock has not progressed) > > + * @param opaque the RTC to tick > > + */ > > +static void rx8900_timer_tick(void *opaque) > > +{ > > + RX8900State *s = (RX8900State *)opaque; > > + struct tm now; > > + bool fire_interrupt = false; > > + > > + qemu_get_timedate(&now, s->offset); > > + > > + if (now.tm_sec == s->last_interrupt_seconds) { > > + return; > > + } > > + > > + s->last_interrupt_seconds = now.tm_sec; > > + > > + TRACE(s->parent_obj, "Tick"); > > + > > + /* Update timer interrupt */ > > + if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_UIE) { > > + if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL) && > > + now.tm_min != s->last_update_interrupt_minutes) { > > + s->last_update_interrupt_minutes = now.tm_min; > > + s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF; > > + fire_interrupt = true; > > + } else if (!(s->nvram[EXTENSION_REGISTER] & > > EXT_MASK_USEL)) { > > + /* per second update interrupt */ > > + s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF; > > + fire_interrupt = true; > > + } > > + } > > + > > + /* Alarm interrupt */ > > + if ((s->nvram[CONTROL_REGISTER] & CTRL_MASK_AIE) && now.tm_sec > > == 0) { > > + if (s->nvram[ALARM_MINUTE] == to_bcd(now.tm_min) && > > + s->nvram[ALARM_HOUR] == to_bcd(now.tm_hour) && > > + s->nvram[ALARM_WEEK_DAY] == > > + ((s->nvram[EXTENSION_REGISTER] & > > EXT_MASK_WADA) ? > > + to_bcd(now.tm_mday) : > > + 0x01 << ((now.tm_wday + s- > > >wday_offset) % 7))) { > > > that's a nice if condition :) May be we could use a temp variable for > the last one. Ok, it was more readable with longer lines :) > > > > > + TRACE(s->parent_obj, "Triggering alarm"); > > + s->nvram[FLAG_REGISTER] |= FLAG_MASK_AF; > > + fire_interrupt = true; > > + } > > + } > > + > > + if (fire_interrupt) { > > + TRACE(s->parent_obj, "Pulsing interrupt"); > > + qemu_irq_pulse(s->interrupt_pin); > > + } > > +} > > + > > +/** > > + * Disable the per second timer > > + * @param s the RTC to operate on > > + */ > > +static void disable_timer(RX8900State *s) > > +{ > > + TRACE(s->parent_obj, "Disabling timer"); > > + ptimer_stop(s->sec_timer); > > +} > > + > > +/** > > + * Enable the per second timer > > + * @param s the RTC to operate on > > + */ > > +static void enable_timer(RX8900State *s) > > +{ > > + TRACE(s->parent_obj, "Enabling timer"); > > + ptimer_run(s->sec_timer, 0); > > +} > > + > > +/** > > + * Handle FOUT_ENABLE (FOE) line > > + * Enables/disables the FOUT line > > + * @param opaque the device instance > > + * @param n the IRQ number > > + * @param level true if the line has been raised > > + */ > > +static void rx8900_fout_enable_handler(void *opaque, int n, int > > level) > > +{ > > + RX8900State *s = RX8900(opaque); > > + > > + if (level) { > > + TRACE(s->parent_obj, "Enabling fout"); > > + ptimer_run(s->fout_timer, 0); > > + } else { > > + /* disable fout */ > > + TRACE(s->parent_obj, "Disabling fout"); > > + ptimer_stop(s->fout_timer); > > + } > > +} > > + > > +/** > > + * Tick the FOUT timer > > + * @param opaque the device instance > > + */ > > +static void rx8900_fout_tick(void *opaque) > > +{ > > + RX8900State *s = (RX8900State *)opaque; > > + > > + TRACE(s->parent_obj, "fout toggle"); > > + s->fout = !s->fout; > > + > > + if (s->fout) { > > + qemu_irq_raise(s->fout_pin); > > + } else { > > + qemu_irq_lower(s->fout_pin); > > + } > > +} > > + > > + > > +/** > > + * Disable the countdown timer > > + * @param s the RTC to operate on > > + */ > > +static void disable_countdown_timer(RX8900State *s) > > +{ > > + TRACE(s->parent_obj, "Disabling countdown timer"); > > + ptimer_stop(s->countdown_timer); > > +} > > + > > +/** > > + * Enable the per second timer > > + * @param s the RTC to operate on > > + */ > > +static void enable_countdown_timer(RX8900State *s) > > +{ > > + TRACE(s->parent_obj, "Enabling countdown timer"); > > + ptimer_run(s->countdown_timer, 0); > > +} > > These helpers don't add much I think. They are called in multiple places and add tracing. > > > > +/** > > + * Tick the countdown timer > > + * @param opaque the device instance > > + */ > > +static void rx8900_countdown_tick(void *opaque) > > +{ > > + RX8900State *s = (RX8900State *)opaque; > > + > > + uint16_t count = s->nvram[TIMER_COUNTER_0] + > > + ((s->nvram[TIMER_COUNTER_1] & 0x0F) << 8); > > + TRACE(s->parent_obj, "countdown tick, count=%d", count); > > + count--; > > + > > + s->nvram[TIMER_COUNTER_0] = (uint8_t)(count & 0x00ff); > > + s->nvram[TIMER_COUNTER_1] = (uint8_t)((count & 0x0f00) >> 8); > > + > > + if (count == 0) { > > + TRACE(s->parent_obj, "Countdown has elapsed, pulsing > > interrupt"); > > + > > + disable_countdown_timer(s); > > + > > + s->nvram[FLAG_REGISTER] |= FLAG_MASK_TF; > > + qemu_irq_pulse(s->interrupt_pin); > > + } > > +} > > + > > + > > +/** > > + * Receive a byte of data from i2c > > + * @param i2c the i2c device that is receiving data > > + * @param data the data that was received > > + */ > > +static int rx8900_send(I2CSlave *i2c, uint8_t data) > > +{ > > + RX8900State *s = RX8900(i2c); > > + struct tm now; > > + > > + TRACE(s->parent_obj, "Received I2C data 0x%02x", data); > > + > > + if (s->addr_byte) { > > + s->ptr = data & (RX8900_NVRAM_SIZE - 1); > > + TRACE(s->parent_obj, "Operating on register 0x%02x", s- > > >ptr); > > + s->addr_byte = false; > > + return 0; > > + } > > + > > + TRACE(s->parent_obj, "Set data 0x%02x=0x%02x", s->ptr, data); > > + > > + qemu_get_timedate(&now, s->offset); > > + switch (s->ptr) { > > + case SECONDS: > > + case EXT_SECONDS: > > + now.tm_sec = from_bcd(data & 0x7f); > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case MINUTES: > > + case EXT_MINUTES: > > + now.tm_min = from_bcd(data & 0x7f); > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case HOURS: > > + case EXT_HOURS: > > + now.tm_hour = from_bcd(data & 0x3f); > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case WEEKDAY: > > + case EXT_WEEKDAY: { > > + int user_wday = ctz32(data); > > + /* The day field is supposed to contain a value in > > + * the range 0-6. Otherwise behavior is undefined. > > + */ > > + switch (data) { > > + case 0x01: > > + case 0x02: > > + case 0x04: > > + case 0x08: > > + case 0x10: > > + case 0x20: > > + case 0x40: > > + break; > > + default: > > + error_report("WARNING: RX8900 - weekday data '%x' is > > out of range," > > + " undefined behavior will result", data); > > + break; > > + } > > + s->weekday = user_wday; > > + break; > > + } > > + > > + case DAY: > > + case EXT_DAY: > > + now.tm_mday = from_bcd(data & 0x3f); > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case MONTH: > > + case EXT_MONTH: > > + now.tm_mon = from_bcd(data & 0x1f) - 1; > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case YEAR: > > + case EXT_YEAR: > > + now.tm_year = from_bcd(data) + 100; > > + s->offset = qemu_timedate_diff(&now); > > + break; > > + > > + case EXTENSION_REGISTER: > > + case EXT_EXTENSION_REGISTER: > > + update_extension_register(s, data); > > + break; > > + > > + case FLAG_REGISTER: > > + case EXT_FLAG_REGISTER: > > + validate_flag_register(s, &data); > > + > > + s->nvram[FLAG_REGISTER] = data; > > + s->nvram[EXT_FLAG_REGISTER] = data; > > + break; > > + > > + case CONTROL_REGISTER: > > + case EXT_CONTROL_REGISTER: > > + update_control_register(s, data); > > + break; > > + > > + default: > > + s->nvram[s->ptr] = data; > > + } > > + > > + inc_regptr(s); > > + return 0; > > +} > > + > > +/** > > + * Get the device temperature in Celcius as a property > > + * @param obj the device > > + * @param v > > + * @param name the property name > > + * @param opaque > > + * @param errp an error object to populate on failure > > + */ > > +static void rx8900_get_temperature(Object *obj, Visitor *v, const > > char *name, > > + void *opaque, Error **errp) > > +{ > > + RX8900State *s = RX8900(obj); > > + double value = (s->nvram[TEMPERATURE] * 2.0f - 187.1f) / > > 3.218f; > > + > > + TRACE(s->parent_obj, "Read temperature property, 0x%x = %f°C", > > + s->nvram[TEMPERATURE], value); > > + > > + visit_type_number(v, name, &value, errp); > > +} > > + > > +/** > > + * Set the device temperature in Celcius as a property > > + * @param obj the device > > + * @param v > > + * @param name the property name > > + * @param opaque > > + * @param errp an error object to populate on failure > > + */ > > +static void rx8900_set_temperature(Object *obj, Visitor *v, const > > char *name, > > + void *opaque, Error **errp) > > +{ > > + RX8900State *s = RX8900(obj); > > + Error *local_err = NULL; > > + double temp; /* degrees Celcius */ > > + visit_type_number(v, name, &temp, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + if (temp >= 100 || temp < -58) { > > + error_setg(errp, "value %f°C is out of range", temp); > > + return; > > + } > > + > > + s->nvram[TEMPERATURE] = (uint8_t) ((temp * 3.218f + 187.19f) / > > 2); > > + > > + TRACE(s->parent_obj, "Set temperature property, 0x%x = %f°C", > > + s->nvram[TEMPERATURE], temp); > > +} > > + > > + > > +/** > > + * Initialize the device > > + * @param i2c the i2c device instance > > + */ > > +static int rx8900_init(I2CSlave *i2c) > > +{ > > + TRACE(*i2c, "Initialized"); > > + > > + return 0; > > +} > > you can remove this routine. > I don't think I can, core.c:i2c_slave_qdev_init() calls it without a guard. > > > > +/** > > + * Configure device properties > > + * @param obj the device > > + */ > > +static void rx8900_initfn(Object *obj) > > +{ > > + object_property_add(obj, "temperature", "number", > > + rx8900_get_temperature, > > + rx8900_set_temperature, NULL, NULL, NULL); > > +} > > + > > +/** > > + * Reset the device > > + * @param dev the RX8900 device to reset > > + */ > > +static void rx8900_reset(DeviceState *dev) > > +{ > > + RX8900State *s = RX8900(dev); > > + > > + TRACE(s->parent_obj, "Reset"); > > + > > + /* The clock is running and synchronized with the host */ > > + s->offset = 0; > > + s->weekday = 7; /* Set to an invalid value */ > > + > > + /* Temperature formulation from the datasheet > > + * ( TEMP[ 7:0 ] * 2 - 187.19) / 3.218 > > + * > > + * Set the initial state to 25 degrees Celcius > > + */ > > + s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2 */ > > + > > + s->nvram[EXTENSION_REGISTER] = EXT_MASK_TSEL1; > > + s->nvram[CONTROL_REGISTER] = CTRL_MASK_CSEL0; > > + s->nvram[FLAG_REGISTER] = FLAG_MASK_VLF | FLAG_MASK_VDET; > > Just asking : why not fully memset(0) the nvram and then set > the values ? I also use this function for a soft-reset, which does not clear the nvram. Actually, I should move the temperature out of there... Cheers, -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
