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

[Alastair D'Silva]

On Wed, 2016-12-14 at 18:02 +, Peter Maydell wrote:
> On 2 December 2016 at 05:46, Alastair D'Silva 
> wrote:
> > From: Alastair D'Silva 
> > 
> > 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 
> > Signed-off-by: Chris Smart 
> > ---
> >  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, , _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);
> > +
> 

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  wrote:
> From: Alastair D'Silva 
>
> 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 
> Signed-off-by: Chris Smart 
> ---
>  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, , _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 

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

[Alastair D'Silva]

From: Alastair D'Silva 

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 
Signed-off-by: Chris Smart 
---
 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;
 }
 
 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
diff --git a/hw/timer/rx8900.c b/hw/timer/rx8900.c
new file mode 100644
index 000..6a113e4
--- /dev/null
+++ b/hw/timer/rx8900.c
@@ -0,0 +1,912 @@
+/*
+ * Epson RX8900SA/CE Realtime Clock Module
+ *
+ * Copyright (c) 2016 IBM Corporation
+ * Authors:
+ *  Alastair D'Silva 
+ *  Chris Smart 
+ *
+ * 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=en
+ *
+ * Not implemented:
+ *  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/log.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "trace.h"
+
+#define TYPE_RX8900 "rx8900"
+#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
+
+typedef struct RX8900State {
+I2CSlave parent_obj;
+
+ptimer_state *sec_timer; /* triggered once per second */
+ptimer_state *fout_timer;
+ptimer_state *countdown_timer;
+bool fout_state;
+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 nvram_offset; /* Wrapped to stay within RX8900_NVRAM_SIZE */
+bool addr_byte;
+uint8_t last_interrupt_seconds; /* The last time the second timer ticked */
+/* the last minute the timer update interrupt was triggered (if enabled) */
+uint8_t last_update_interrupt_minutes;
+double supply_voltage;
+qemu_irq interrupt_pin;
+qemu_irq fout_pin;
+} RX8900State;
+
+static const VMStateDescription vmstate_rx8900 = {
+.name = "rx8900",
+.version_id = 1,
+.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_state, RX8900State),
+VMSTATE_INT64(offset, RX8900State),