On Wed, 2016-12-14 at 18:02 +0000, Peter Maydell wrote: > On 2 December 2016 at 05:46, Alastair D'Silva <alast...@au1.ibm.com> > wrote: > > From: Alastair D'Silva <alast...@d-silva.org> > > > > This patch adds support for the Epson RX8900 I2C RTC. > > > > The following chip features are implemented: > > - RTC (wallclock based, ptimer 10x oversampling to pick up > > wallclock transitions) > > - Time update interrupt (per second/minute, wallclock based) > > - Alarms (wallclock based) > > - Temperature (set via a property) > > - Countdown timer (emulated clock via ptimer) > > - FOUT via GPIO (emulated clock via ptimer) > > > > The following chip features are unimplemented: > > - Low voltage detection > > - i2c timeout > > > > The implementation exports the following named GPIOs: > > rx8900-interrupt-out > > rx8900-fout-enable > > rx8900-fout > > > > Signed-off-by: Alastair D'Silva <alast...@d-silva.org> > > Signed-off-by: Chris Smart <ch...@distroguy.com> > > --- > > default-configs/arm-softmmu.mak | 1 + > > hw/i2c/core.c | 4 +- > > hw/timer/Makefile.objs | 2 + > > hw/timer/rx8900.c | 912 > > ++++++++++++++++++++++++++++++++++++++++ > > hw/timer/rx8900_regs.h | 141 +++++++ > > hw/timer/trace-events | 31 ++ > > 6 files changed, 1089 insertions(+), 2 deletions(-) > > create mode 100644 hw/timer/rx8900.c > > create mode 100644 hw/timer/rx8900_regs.h > > > > 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/i2c/core.c b/hw/i2c/core.c > > index ae3ca94..e40781e 100644 > > --- a/hw/i2c/core.c > > +++ b/hw/i2c/core.c > > @@ -262,9 +262,9 @@ static int i2c_slave_qdev_init(DeviceState > > *dev) > > > > if (sc->init) { > > return sc->init(s); > > - } else { > > - return 0; > > } > > + > > + return 0; > > } > > > > This change shouldn't be in this patch.
Ok > > > DeviceState *i2c_create_slave(I2CBus *bus, const char *name, > > uint8_t addr) > > 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 > > +/** > > + * Get the device temperature in Celcius as a property > > It's spelt "Celsius" -- please fix through the whole file. > Whoops, thanks for that :) > > + * @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; > > Why use 'double' for the variable when you're doing all your > arithmetic at 'float' precision because of those 'f' suffixes? > Unless there's a good reason I'd suggest dropping the 'f's and > just doing all the arithmetic at double precision. Ok > > +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); > > Not sure that UTF-8 characters directly in error strings > is a great idea. I'd stick to ASCII. It is valid 8 bit ASCII, but is code-page dependent, Ok, I'll remove it. > > + return; > > + } > > + > > + s->nvram[TEMPERATURE] = encode_temperature(temp); > > + > > + trace_rx8900_set_temperature(s->nvram[TEMPERATURE], temp); > > +} > > + > > +/** > > + * Get the device supply voltage 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_voltage(Object *obj, Visitor *v, const char > > *name, > > + void *opaque, Error **errp) > > +{ > > + RX8900State *s = RX8900(obj); > > + > > + visit_type_number(v, name, &s->supply_voltage, errp); > > +} > > + > > +/** > > + * Set the device supply voltage 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_voltage(Object *obj, Visitor *v, const char > > *name, > > + void *opaque, Error **errp) > > +{ > > + RX8900State *s = RX8900(obj); > > + Error *local_err = NULL; > > + double temp; > > + visit_type_number(v, name, &temp, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + s->supply_voltage = temp; > > + trace_rx8900_set_voltage(s->supply_voltage); > > + > > + check_voltage(s); > > +} > > + > > + > > +/** > > + * 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); > > + > > + object_property_add(obj, "voltage", "number", > > + rx8900_get_voltage, > > + rx8900_set_voltage, NULL, NULL, NULL); > > +} > > + > > +/** > > + * Reset the device > > + * @param dev the RX8900 device to reset > > + */ > > +static void rx8900_reset(DeviceState *dev) > > +{ > > + RX8900State *s = RX8900(dev); > > + > > + trace_rx8900_reset(); > > + > > + /* The clock is running and synchronized with the host */ > > + s->offset = 0; > > + s->weekday = 7; /* Set to an invalid value */ > > + > > + s->nvram[EXTENSION_REGISTER] = EXT_MASK_TSEL1; > > + s->nvram[CONTROL_REGISTER] = CTRL_MASK_CSEL0; > > + s->nvram[FLAG_REGISTER] &= FLAG_MASK_VDET | FLAG_MASK_VLF; > > + > > + s->nvram_offset = 0; > > + > > + trace_rx8900_regptr_update(s->nvram_offset); > > + > > + s->addr_byte = false; > > +} > > + > > +/** > > + * Realize an RX8900 device instance > > + * Set up timers > > + * Configure GPIO lines > > + * @param dev the device instance to realize > > + * @param errp an error object to populate on error > > + */ > > +static void rx8900_realize(DeviceState *dev, Error **errp) > > +{ > > + RX8900State *s = RX8900(dev); > > + I2CSlave *i2c = I2C_SLAVE(dev); > > + QEMUBH *bh; > > + char name[64]; > > + > > + s->fout_state = false; > > + > > + memset(s->nvram, 0, RX8900_NVRAM_SIZE); > > This looks like something you should be doing in reset. > > > + /* Set the initial state to 25 degrees Celcius */ > > + s->nvram[TEMPERATURE] = encode_temperature(25.0f); > > ...and this. > I reuse the reset code for the soft-reset, which prevents me from doing these operations there. > > + > > + /* Set up timers */ > > + bh = qemu_bh_new(rx8900_timer_tick, s); > > + s->sec_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT); > > + /* we trigger the timer at 10Hz and check for rollover, as the > > qemu > > + * clock does not advance in realtime in the test environment, > > + * leading to unstable test results > > + */ > > + ptimer_set_freq(s->sec_timer, 10); > > + ptimer_set_limit(s->sec_timer, 1, 1); > > + > > + bh = qemu_bh_new(rx8900_fout_tick, s); > > + s->fout_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT); > > + /* frequency doubled to generate 50% duty cycle square wave */ > > + ptimer_set_freq(s->fout_timer, 32768 * 2); > > + ptimer_set_limit(s->fout_timer, 1, 1); > > + > > + bh = qemu_bh_new(rx8900_countdown_tick, s); > > + s->countdown_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT); > > + ptimer_set_freq(s->countdown_timer, COUNTDOWN_TIMER_FREQ); > > + ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ, 1); > > + > > + > > + /* set up GPIO */ > > + snprintf(name, sizeof(name), "rx8900-interrupt-out"); > > + qdev_init_gpio_out_named(&i2c->qdev, &s->interrupt_pin, name, > > 1); > > + trace_rx8900_pin_name("Interrupt", name); > > + > > + snprintf(name, sizeof(name), "rx8900-fout-enable"); > > + qdev_init_gpio_in_named(&i2c->qdev, > > rx8900_fout_enable_handler, name, 1); > > + trace_rx8900_pin_name("Fout-enable", name); > > + > > + snprintf(name, sizeof(name), "rx8900-fout"); > > + qdev_init_gpio_out_named(&i2c->qdev, &s->fout_pin, name, 1); > > + trace_rx8900_pin_name("Fout", name); > > + > > + /* Set up default voltage */ > > + s->supply_voltage = 3.3f; > > + trace_rx8900_set_voltage(s->supply_voltage); > > +} > > +rx8900_get_temperature(uint8_t raw, double val) "0x%x = %f°C" > > +rx8900_set_temperature(uint8_t raw, double val) "0x%x = %f°C" > > Stick to ASCII here too. > Ok > thanks > -- PMM >