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.
> 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.
> + * @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.
> +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.
> + 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.
> +
> + /* 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.
thanks
-- PMM
