[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 3/4] hw/timer: Add Epson RX8900 RTC support
|
From: |
Alastair D'Silva |
|
Subject: |
Re: [Qemu-arm] [PATCH 3/4] hw/timer: Add Epson RX8900 RTC support |
|
Date: |
Fri, 18 Nov 2016 11:19:18 +1100 |
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 <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> > 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 <address@hidden>
> > + *
> > + * 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 address@hidden: %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
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, (continued)
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Cédric Le Goater, 2016/11/22
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Paolo Bonzini, 2016/11/22
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Alastair D'Silva, 2016/11/22
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Paolo Bonzini, 2016/11/22
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Alastair D'Silva, 2016/11/22
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Paolo Bonzini, 2016/11/23
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Edgar E. Iglesias, 2016/11/23
- Re: [Qemu-arm] [PATCH 2/4] qtest: Support named interrupts, Paolo Bonzini, 2016/11/23
[Qemu-arm] [PATCH 3/4] hw/timer: Add Epson RX8900 RTC support, Alastair D'Silva, 2016/11/17
Re: [Qemu-arm] [Qemu-devel] [PATCH 0/4] Add support for the Epson RX8900 RTC to the aspeed board, no-reply, 2016/11/17