qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alastair D'Silva
Subject: Re: [Qemu-devel] [PATCH v3 5/7] hw/timer: Add Epson RX8900 RTC support
Date: Thu, 15 Dec 2016 11:48:52 +1100

On Wed, 2016-12-14 at 18:02 +0000, Peter Maydell wrote:
> 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.

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
> 


reply via email to

[Prev in Thread] Current Thread [Next in Thread]