Re: [Qemu-devel] [PATCH v3 5/7] hw/timer: Add Epson RX8900 RTC support

[Peter Maydell]

On 2 December 2016 at 05:46, Alastair D'Silva <address@hidden> wrote:
> From: Alastair D'Silva <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Chris Smart <address@hidden>
> ---
>  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