* [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC
@ 2016-10-27 2:27 alastair
2016-10-27 2:27 ` [PATCH 1/3] Add Epson RX8900 RTC support alastair
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: alastair @ 2016-10-27 2:27 UTC (permalink / raw)
To: openbmc; +Cc: clg, andrew, Alastair D'Silva
From: Alastair D'Silva <alastair@d-silva.org>
This series adds preliminary support for the Epson rx8900 RTC.
Setting and retrieving time is supported, and the chip temperature
is hardcoded.
Other features (in particular, those that require the interrupt line)
are not implmented.
Alastair D'Silva (3):
Add Epson RX8900 RTC support
Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
Add a unit test for RX8900 RTC (time functionality only)
default-configs/arm-softmmu.mak | 1 +
hw/arm/aspeed.c | 4 +
hw/arm/imx25_pdk.c | 9 +-
hw/timer/Makefile.objs | 1 +
hw/timer/rx8900.c | 426 ++++++++++++++++++++++++++++++++++++++++
tests/Makefile.include | 2 +
tests/rx8900-test.c | 82 ++++++++
7 files changed, 521 insertions(+), 4 deletions(-)
create mode 100644 hw/timer/rx8900.c
create mode 100644 tests/rx8900-test.c
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] Add Epson RX8900 RTC support
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
@ 2016-10-27 2:27 ` alastair
2016-10-27 4:19 ` Joel Stanley
2016-10-27 2:27 ` [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32 alastair
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: alastair @ 2016-10-27 2:27 UTC (permalink / raw)
To: openbmc; +Cc: clg, andrew, Alastair D'Silva
From: Alastair D'Silva <alastair@d-silva.org>
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
default-configs/arm-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/rx8900.c | 426 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 428 insertions(+)
create mode 100644 hw/timer/rx8900.c
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/timer/Makefile.objs b/hw/timer/Makefile.objs
index 7ba8c23..ea072ca 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -17,6 +17,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 0000000..4c15da8
--- /dev/null
+++ b/hw/timer/rx8900.c
@@ -0,0 +1,426 @@
+/*
+ * Epson RX8900SA/CE Realtime Clock Module
+ *
+ * Copyright (c) 2016 IBM Corporation
+ * Authors:
+ * Alastair D'Silva <alastair@d-silva.org>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ *
+ * Datasheet available at:
+ * https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE&lang=en
+ *
+ * Not implemented:
+ * Implement Alarm functions
+ * Implement Timer Counters
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/bcd.h"
+#include "qemu/error-report.h"
+
+#define TYPE_RX8900 "rx8900"
+#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
+
+#define NVRAM_SIZE 0x20
+
+typedef enum RX8900Addresses {
+ SECONDS = 0x00,
+ MINUTES = 0x01,
+ HOURS = 0x02,
+ WEEKDAY = 0x03,
+ DAY = 0x04,
+ MONTH = 0x05,
+ YEAR = 0x06,
+ RAM = 0x07,
+ ALARM_MINUTE = 0x08,
+ ALARM_HOUR = 0x09,
+ ALARM_WEEK_DAY = 0x0A,
+ TIMER_COUNTER_0 = 0x0B,
+ TIMER_COUNTER_1 = 0x0C,
+ EXTENSION_REGISTER = 0x0D,
+ FLAG_REGISTER = 0X0E,
+ CONTROL_REGISTER = 0X0F,
+ EXT_SECONDS = 0x010, /* Alias of SECONDS */
+ EXT_MINUTES = 0x11, /* Alias of MINUTES */
+ EXT_HOURS = 0x12, /* Alias of HOURS */
+ EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
+ EXT_DAY = 0x14, /* Alias of DAY */
+ EXT_MONTH = 0x15, /* Alias of MONTH */
+ EXT_YEAR = 0x16, /* Alias of YEAR */
+ TEMPERATURE = 0x17,
+ BACKUP_FUNCTION = 0x18,
+ NO_USE_1 = 0x19,
+ NO_USE_2 = 0x1A,
+ EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
+ EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
+ EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER */
+ EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
+ EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
+} RX8900Addresses;
+
+typedef enum ExtRegBits {
+ EXT_REG_TSEL0 = 0,
+ EXT_REG_TSEL1 = 1,
+ EXT_REG_FSEL0 = 2,
+ EXT_REG_FSEL1 = 3,
+ EXT_REG_TE = 4,
+ EXT_REG_USEL = 5,
+ EXT_REG_WADA = 6,
+ EXT_REG_TEST = 7
+} ExtRegBits;
+
+typedef enum CtrlRegBits {
+ CTRL_REG_RESET = 0,
+ CTRL_REG_WP0 = 1,
+ CTRL_REG_WP1 = 2,
+ CTRL_REG_AIE = 3,
+ CTRL_REG_TIE = 4,
+ CTRL_REG_UIE = 5,
+ CTRL_REG_CSEL0 = 6,
+ CTRL_REG_CSEL1 = 7
+} CtrlRegBits;
+
+typedef struct RX8900State {
+ I2CSlave parent_obj;
+
+ int64_t offset;
+ uint8_t wday_offset;
+ uint8_t nvram[NVRAM_SIZE];
+ int32_t ptr;
+ bool addr_byte;
+} RX8900State;
+
+static const VMStateDescription vmstate_rx8900 = {
+ .name = "rx8900",
+ .version_id = 2,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[] ) {
+ VMSTATE_I2C_SLAVE(parent_obj, RX8900State),
+ VMSTATE_INT64(offset, RX8900State),
+ VMSTATE_UINT8_V(wday_offset, RX8900State, 2),
+ VMSTATE_UINT8_ARRAY(nvram, RX8900State, NVRAM_SIZE),
+ VMSTATE_INT32(ptr, RX8900State),
+ VMSTATE_BOOL(addr_byte, RX8900State),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void capture_current_time(RX8900State *s)
+{
+ /* Capture the current time into the secondary registers
+ * which will be actually read by the data transfer operation.
+ */
+ struct tm now;
+ qemu_get_timedate(&now, s->offset);
+ s->nvram[SECONDS] = to_bcd(now.tm_sec);
+ s->nvram[MINUTES] = to_bcd(now.tm_min);
+ s->nvram[HOURS] = to_bcd(now.tm_hour);
+
+ s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + 1) % 7);
+ s->nvram[DAY] = to_bcd(now.tm_mday);
+ s->nvram[MONTH] = to_bcd(now.tm_mon + 1);
+ s->nvram[YEAR] = to_bcd(now.tm_year % 100);
+
+ s->nvram[EXT_SECONDS] = s->nvram[SECONDS];
+ s->nvram[EXT_MINUTES] = s->nvram[MINUTES];
+ s->nvram[EXT_HOURS] = s->nvram[HOURS];
+ s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY];
+ s->nvram[EXT_DAY] = s->nvram[DAY];
+ s->nvram[EXT_MONTH] = s->nvram[MONTH];
+ s->nvram[EXT_YEAR] = s->nvram[YEAR];
+}
+
+static void inc_regptr(RX8900State *s)
+{
+ /* The register pointer wraps around after 0x1F
+ */
+ s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
+ if (s->ptr == 0x00) {
+ capture_current_time(s);
+ }
+}
+
+static void rx8900_event(I2CSlave *i2c, enum i2c_event event)
+{
+ RX8900State *s = RX8900(i2c);
+
+ switch (event) {
+ case I2C_START_RECV:
+ /* In h/w, capture happens on any START condition, not just a
+ * START_RECV, but there is no need to actually capture on
+ * START_SEND, because the guest can't get at that data
+ * without going through a START_RECV which would overwrite it.
+ */
+ capture_current_time(s);
+ break;
+ case I2C_START_SEND:
+ s->addr_byte = true;
+ break;
+ default:
+ break;
+ }
+}
+
+static int rx8900_recv(I2CSlave *i2c)
+{
+ RX8900State *s = RX8900(i2c);
+ uint8_t res = s->nvram[s->ptr];
+ inc_regptr(s);
+ return res;
+}
+
+static void validate_extension_register(RX8900State *s, uint8_t data)
+{
+ uint8_t diffmask = data ^ s->nvram[EXTENSION_REGISTER];
+
+ if ((diffmask & 1 << EXT_REG_TSEL0) || (diffmask & 1 << EXT_REG_TSEL1)) {
+ error_report("WARNING: RX8900 - "
+ "Timer select modified but is unimplemented");
+ }
+
+ if ((diffmask & 1 << EXT_REG_FSEL0) || (diffmask & 1 << EXT_REG_FSEL1)) {
+ error_report("WARNING: RX8900 - "
+ "FOut Frequency modified but is unimplemented");
+ }
+
+ if (diffmask & 1 << EXT_REG_TE) {
+ error_report("WARNING: RX8900 - "
+ "Timer enable modified but is unimplemented");
+ }
+
+ if (diffmask & 1 << EXT_REG_USEL) {
+ error_report("WARNING: RX8900 - "
+ "Update interrupt modified but is unimplemented");
+ }
+
+ if (diffmask & 1 << EXT_REG_WADA) {
+ error_report("WARNING: RX8900 - "
+ "Week/day alarm modified but is unimplemented");
+ }
+
+ if (data & 1 << EXT_REG_TEST) {
+ error_report("WARNING: RX8900 - "
+ "Test bit is enabled but is forbidden by the manufacturer");
+ }
+}
+
+static void validate_control_register(RX8900State *s, uint8_t data)
+{
+ uint8_t diffmask = data ^ s->nvram[CONTROL_REGISTER];
+
+ if (diffmask & 1 << CTRL_REG_RESET) {
+ error_report("WARNING: RX8900 - "
+ "Reset requested but is unimplemented");
+ }
+
+ if (diffmask & 1 << CTRL_REG_WP0) {
+ error_report("WARNING: RX8900 - "
+ "Attempt to write to write protected bit %d in control register",
+ CTRL_REG_WP0);
+ }
+
+ if (diffmask & 1 << CTRL_REG_WP1) {
+ error_report("WARNING: RX8900 - "
+ "Attempt to write to write protected bit %d in control register",
+ CTRL_REG_WP1);
+ }
+
+ if (diffmask & 1 << CTRL_REG_AIE) {
+ error_report("WARNING: RX8900 - "
+ "Alarm interrupt requested but is unimplemented");
+ }
+
+ if (diffmask & 1 << CTRL_REG_TIE) {
+ error_report("WARNING: RX8900 - "
+ "Timer interrupt requested but is unimplemented");
+ }
+
+ if (diffmask & 1 << CTRL_REG_UIE) {
+ error_report("WARNING: RX8900 - "
+ "Update interrupt requested but is unimplemented");
+ }
+
+}
+
+static int rx8900_send(I2CSlave *i2c, uint8_t data)
+{
+ RX8900State *s = RX8900(i2c);
+ struct tm now;
+
+ if (s->addr_byte) {
+ s->ptr = data & (NVRAM_SIZE - 1);
+ s->addr_byte = false;
+ return 0;
+ }
+
+ qemu_get_timedate(&now, s->offset);
+ switch (s->ptr) {
+ case SECONDS:
+ case EXT_SECONDS:
+ now.tm_sec = from_bcd(data & 0x7f);
+ s->nvram[SECONDS] = data;
+ s->nvram[EXT_SECONDS] = data;
+ break;
+
+ case MINUTES:
+ case EXT_MINUTES:
+ now.tm_min = from_bcd(data & 0x7f);
+ s->nvram[MINUTES] = data;
+ s->nvram[EXT_MINUTES] = data;
+ break;
+
+ case HOURS:
+ case EXT_HOURS:
+ now.tm_hour = from_bcd(data & 0x3f);
+ s->nvram[HOURS] = data;
+ s->nvram[EXT_HOURS] = data;
+ break;
+
+ case WEEKDAY:
+ case EXT_WEEKDAY: {
+ int user_wday = 0;
+ /* The day field is supposed to contain a value in
+ * the range 0-6. Otherwise behavior is undefined.
+ */
+ switch (data) {
+ case 0x01:
+ user_wday = 0;
+ break;
+ case 0x02:
+ user_wday = 1;
+ break;
+ case 0x04:
+ user_wday = 2;
+ break;
+ case 0x08:
+ user_wday = 3;
+ break;
+ case 0x10:
+ user_wday = 4;
+ break;
+ case 0x20:
+ user_wday = 5;
+ break;
+ case 0x40:
+ user_wday = 6;
+ break;
+ default:
+ error_report("WARNING: RX8900 - weekday data '%x' is out of range,"
+ " undefined behavior will result", data);
+ break;
+ }
+ s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
+ s->nvram[WEEKDAY] = data;
+ s->nvram[EXT_WEEKDAY] = data;
+ break;
+ }
+
+ case DAY:
+ case EXT_DAY:
+ now.tm_mday = from_bcd(data & 0x3f);
+ s->nvram[DAY] = data;
+ s->nvram[EXT_DAY] = data;
+ break;
+
+ case MONTH:
+ case EXT_MONTH:
+ now.tm_mon = from_bcd(data & 0x1f) - 1;
+ s->nvram[MONTH] = data;
+ s->nvram[EXT_MONTH] = data;
+ break;
+
+ case YEAR:
+ case EXT_YEAR:
+ now.tm_year = from_bcd(data) + 100;
+ s->nvram[YEAR] = data;
+ s->nvram[EXT_YEAR] = data;
+ break;
+
+ case EXTENSION_REGISTER:
+ case EXT_EXTENSION_REGISTER:
+ validate_extension_register(s, data);
+ s->nvram[EXTENSION_REGISTER] = data;
+ s->nvram[EXT_EXTENSION_REGISTER] = data;
+ break;
+
+ case CONTROL_REGISTER:
+ case EXT_CONTROL_REGISTER:
+ validate_control_register(s, data);
+ s->nvram[EXTENSION_REGISTER] = data;
+ s->nvram[EXT_EXTENSION_REGISTER] = data;
+ break;
+
+ default:
+ s->nvram[s->ptr] = data;
+ }
+
+ inc_regptr(s);
+ return 0;
+}
+
+static int rx8900_init(I2CSlave *i2c)
+{
+ return 0;
+}
+
+static void rx8900_reset(DeviceState *dev)
+{
+ RX8900State *s = RX8900(dev);
+
+ /* The clock is running and synchronized with the host */
+ s->offset = 0;
+ memset(s->nvram, 0, NVRAM_SIZE);
+
+ /* Temperature formulation from the datasheet
+ * ( TEMP[ 7:0 ] * 2 – 187.19) / 3.218
+ *
+ * Hardcode it to 25 degrees Celcius
+ */
+ s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2 */
+
+ s->nvram[CONTROL_REGISTER] = 1 << CTRL_REG_CSEL0;
+
+ s->ptr = 0;
+ s->addr_byte = false;
+}
+
+static void rx8900_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+ k->init = rx8900_init;
+ k->event = rx8900_event;
+ k->recv = rx8900_recv;
+ k->send = rx8900_send;
+ dc->reset = rx8900_reset;
+ dc->vmsd = &vmstate_rx8900;
+}
+
+static const TypeInfo rx8900_info = {
+ .name = TYPE_RX8900,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_size = sizeof(RX8900State),
+ .class_init = rx8900_class_init,
+};
+
+static void rx8900_register_types(void)
+{
+ type_register_static(&rx8900_info);
+}
+
+type_init(rx8900_register_types)
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
2016-10-27 2:27 ` [PATCH 1/3] Add Epson RX8900 RTC support alastair
@ 2016-10-27 2:27 ` alastair
2016-10-31 6:41 ` Cédric Le Goater
2016-10-27 2:27 ` [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only) alastair
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: alastair @ 2016-10-27 2:27 UTC (permalink / raw)
To: openbmc; +Cc: clg, andrew, Alastair D'Silva
From: Alastair D'Silva <alastair@d-silva.org>
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
hw/arm/aspeed.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index c7206fd..8ccac3e 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -100,6 +100,7 @@ static void aspeed_board_init(MachineState *machine,
{
AspeedBoardState *bmc;
AspeedSoCClass *sc;
+ I2CBus *i2c12;
bmc = g_new0(AspeedBoardState, 1);
object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
@@ -137,6 +138,9 @@ static void aspeed_board_init(MachineState *machine,
aspeed_board_binfo.ram_size = ram_size;
aspeed_board_binfo.loader_start = sc->info->sdram_base;
+ i2c12 = aspeed_i2c_get_bus((DeviceState *)&bmc->soc.i2c, 11);
+ i2c_create_slave(i2c12, "rx8900", 0x32);
+
arm_load_kernel(ARM_CPU(first_cpu), &aspeed_board_binfo);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only)
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
2016-10-27 2:27 ` [PATCH 1/3] Add Epson RX8900 RTC support alastair
2016-10-27 2:27 ` [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32 alastair
@ 2016-10-27 2:27 ` alastair
2016-10-27 4:21 ` Joel Stanley
2016-10-27 4:22 ` [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC Joel Stanley
2016-10-31 6:54 ` Cédric Le Goater
4 siblings, 1 reply; 16+ messages in thread
From: alastair @ 2016-10-27 2:27 UTC (permalink / raw)
To: openbmc; +Cc: clg, andrew, Alastair D'Silva
From: Alastair D'Silva <alastair@d-silva.org>
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
hw/arm/imx25_pdk.c | 9 +++---
tests/Makefile.include | 2 ++
tests/rx8900-test.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 4 deletions(-)
create mode 100644 tests/rx8900-test.c
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index 025b608..99a819f 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -134,13 +134,14 @@ static void imx25_pdk_init(MachineState *machine)
arm_load_kernel(&s->soc.cpu, &imx25_pdk_binfo);
} else {
/*
- * This I2C device doesn't exist on the real board.
+ * These I2C devices doesn't exist on the real board.
* We add it here (only on qtest usage) to be able to do a bit
* of simple qtest. See "make check" for details.
*/
- i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
- "i2c"),
- "ds1338", 0x68);
+ I2CBus *i2c = (I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
+ "i2c");
+ i2c_create_slave(i2c, "ds1338", 0x68);
+ i2c_create_slave(i2c, "rx8900", 0x32);
}
}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index cbe38ad..3f08ba9 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -297,6 +297,7 @@ check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
check-qtest-arm-y = tests/tmp105-test$(EXESUF)
check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/rx8900-test$(EXESUF)
check-qtest-arm-y += tests/m25p80-test$(EXESUF)
gcov-files-arm-y += hw/misc/tmp105.c
check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
@@ -629,6 +630,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/rx8900-test$(EXESUF): tests/rx8900-test.o $(libqos-imx-obj-y)
tests/m25p80-test$(EXESUF): tests/m25p80-test.o
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
diff --git a/tests/rx8900-test.c b/tests/rx8900-test.c
new file mode 100644
index 0000000..8d3ecd8
--- /dev/null
+++ b/tests/rx8900-test.c
@@ -0,0 +1,82 @@
+/*
+ * QTest testcase for the DS1338 RTC
+ *
+ * Copyright (c) 2013 Jean-Christophe Dubois
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "libqos/i2c.h"
+
+#define IMX25_I2C_0_BASE 0x43F80000
+
+#define RX8900_ADDR 0x32
+
+static I2CAdapter *i2c;
+static uint8_t addr;
+
+static inline uint8_t bcd2bin(uint8_t x)
+{
+ return ((x) & 0x0f) + ((x) >> 4) * 10;
+}
+
+static void send_and_receive(void)
+{
+ uint8_t cmd[1];
+ uint8_t resp[7];
+ time_t now = time(NULL);
+ struct tm *tm_ptr;
+
+ /* reset the index in the RTC memory */
+ cmd[0] = 0;
+ i2c_send(i2c, addr, cmd, 1);
+
+ tm_ptr = gmtime(&now);
+ /* retrieve the date */
+ i2c_recv(i2c, addr, resp, 7);
+
+ /* check retrieved time against local time */
+ g_assert_cmpuint(bcd2bin(resp[0]), == , tm_ptr->tm_sec);
+ g_assert_cmpuint(bcd2bin(resp[1]), == , tm_ptr->tm_min);
+ g_assert_cmpuint(bcd2bin(resp[2]), == , tm_ptr->tm_hour);
+ g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
+ g_assert_cmpuint(bcd2bin(resp[5]), == , 1 + tm_ptr->tm_mon);
+ g_assert_cmpuint(2000 + bcd2bin(resp[6]), == , 1900 + tm_ptr->tm_year);
+}
+
+int main(int argc, char **argv)
+{
+ QTestState *s = NULL;
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+
+ /* Address must match RX8900_ADDR */
+ s = qtest_start("-display none -machine imx25-pdk");
+ i2c = imx_i2c_create(IMX25_I2C_0_BASE);
+ addr = RX8900_ADDR;
+
+ qtest_add_func("/rx8900/tx-rx", send_and_receive);
+
+ ret = g_test_run();
+
+ if (s) {
+ qtest_quit(s);
+ }
+ g_free(i2c);
+
+ return ret;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Add Epson RX8900 RTC support
2016-10-27 2:27 ` [PATCH 1/3] Add Epson RX8900 RTC support alastair
@ 2016-10-27 4:19 ` Joel Stanley
2016-10-27 5:03 ` Alastair D'Silva
0 siblings, 1 reply; 16+ messages in thread
From: Joel Stanley @ 2016-10-27 4:19 UTC (permalink / raw)
To: alastair; +Cc: OpenBMC Maillist, clg, Alastair D'Silva
Hi Alastair,
Thanks for the patches. Please run them through checkpatch.pl from the
Qemu tree before your next submission.
As we use the openbmc list for code from a few different projects,
next time use --subject-prefix to add qemu to your series.
On Thu, Oct 27, 2016 at 12:57 PM, <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/timer/Makefile.objs | 1 +
> hw/timer/rx8900.c | 426 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 428 insertions(+)
> create mode 100644 hw/timer/rx8900.c
>
> 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/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 7ba8c23..ea072ca 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -17,6 +17,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 0000000..4c15da8
> --- /dev/null
> +++ b/hw/timer/rx8900.c
> @@ -0,0 +1,426 @@
> +/*
> + * Epson RX8900SA/CE Realtime Clock Module
> + *
> + * Copyright (c) 2016 IBM Corporation
> + * Authors:
> + * Alastair D'Silva <alastair@d-silva.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
Qemu tends to use a brief copyright header. Take a look at hw/arm/aspeed_soc.c:
* Copyright 2016 IBM Corp.
*
* 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&lang=en
> + *
> + * Not implemented:
> + * Implement Alarm functions
> + * Implement Timer Counters
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "hw/i2c/i2c.h"
> +#include "qemu/bcd.h"
> +#include "qemu/error-report.h"
> +
> +#define TYPE_RX8900 "rx8900"
> +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
> +
> +#define NVRAM_SIZE 0x20
> +
> +typedef enum RX8900Addresses {
This typedef is unnecessary.
> + SECONDS = 0x00,
> + MINUTES = 0x01,
> + HOURS = 0x02,
> + WEEKDAY = 0x03,
> + DAY = 0x04,
> + MONTH = 0x05,
> + YEAR = 0x06,
> + RAM = 0x07,
> + ALARM_MINUTE = 0x08,
> + ALARM_HOUR = 0x09,
> + ALARM_WEEK_DAY = 0x0A,
> + TIMER_COUNTER_0 = 0x0B,
> + TIMER_COUNTER_1 = 0x0C,
> + EXTENSION_REGISTER = 0x0D,
> + FLAG_REGISTER = 0X0E,
> + CONTROL_REGISTER = 0X0F,
> + EXT_SECONDS = 0x010, /* Alias of SECONDS */
> + EXT_MINUTES = 0x11, /* Alias of MINUTES */
> + EXT_HOURS = 0x12, /* Alias of HOURS */
> + EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
> + EXT_DAY = 0x14, /* Alias of DAY */
> + EXT_MONTH = 0x15, /* Alias of MONTH */
> + EXT_YEAR = 0x16, /* Alias of YEAR */
> + TEMPERATURE = 0x17,
> + BACKUP_FUNCTION = 0x18,
> + NO_USE_1 = 0x19,
> + NO_USE_2 = 0x1A,
> + EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
> + EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
> + EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER */
> + EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
> + EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
Do you use all of these values? I suggest only including the
definitions for the ones you need.
Some models in Qemu chose not to have #defines (or enums) for the
registers, and instead use comments in the switch statements for the
operations they support.
> +} RX8900Addresses;
> +
> +typedef enum ExtRegBits {
> + EXT_REG_TSEL0 = 0,
> + EXT_REG_TSEL1 = 1,
> + EXT_REG_FSEL0 = 2,
> + EXT_REG_FSEL1 = 3,
> + EXT_REG_TE = 4,
> + EXT_REG_USEL = 5,
> + EXT_REG_WADA = 6,
> + EXT_REG_TEST = 7
> +} ExtRegBits;
> +
As above.
> +typedef enum CtrlRegBits {
> + CTRL_REG_RESET = 0,
> + CTRL_REG_WP0 = 1,
> + CTRL_REG_WP1 = 2,
> + CTRL_REG_AIE = 3,
> + CTRL_REG_TIE = 4,
> + CTRL_REG_UIE = 5,
> + CTRL_REG_CSEL0 = 6,
> + CTRL_REG_CSEL1 = 7
> +} CtrlRegBits;
As above.
> +
> +typedef struct RX8900State {
> + I2CSlave parent_obj;
> +
> + int64_t offset;
> + uint8_t wday_offset;
> + uint8_t nvram[NVRAM_SIZE];
> + int32_t ptr;
> + bool addr_byte;
> +} RX8900State;
> +
> +static const VMStateDescription vmstate_rx8900 = {
> + .name = "rx8900",
> + .version_id = 2,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[] ) {
> + VMSTATE_I2C_SLAVE(parent_obj, RX8900State),
> + VMSTATE_INT64(offset, RX8900State),
> + VMSTATE_UINT8_V(wday_offset, RX8900State, 2),
> + VMSTATE_UINT8_ARRAY(nvram, RX8900State, NVRAM_SIZE),
> + VMSTATE_INT32(ptr, RX8900State),
> + VMSTATE_BOOL(addr_byte, RX8900State),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void capture_current_time(RX8900State *s)
> +{
> + /* Capture the current time into the secondary registers
> + * which will be actually read by the data transfer operation.
> + */
> + struct tm now;
> + qemu_get_timedate(&now, s->offset);
> + s->nvram[SECONDS] = to_bcd(now.tm_sec);
> + s->nvram[MINUTES] = to_bcd(now.tm_min);
> + s->nvram[HOURS] = to_bcd(now.tm_hour);
> +
> + s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + 1) % 7);
> + s->nvram[DAY] = to_bcd(now.tm_mday);
> + s->nvram[MONTH] = to_bcd(now.tm_mon + 1);
> + s->nvram[YEAR] = to_bcd(now.tm_year % 100);
> +
> + s->nvram[EXT_SECONDS] = s->nvram[SECONDS];
> + s->nvram[EXT_MINUTES] = s->nvram[MINUTES];
> + s->nvram[EXT_HOURS] = s->nvram[HOURS];
> + s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY];
> + s->nvram[EXT_DAY] = s->nvram[DAY];
> + s->nvram[EXT_MONTH] = s->nvram[MONTH];
> + s->nvram[EXT_YEAR] = s->nvram[YEAR];
> +}
> +
> +static void inc_regptr(RX8900State *s)
> +{
> + /* The register pointer wraps around after 0x1F
> + */
> + s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
> + if (s->ptr == 0x00) {
> + capture_current_time(s);
> + }
> +}
> +
> +static void rx8900_event(I2CSlave *i2c, enum i2c_event event)
> +{
> + RX8900State *s = RX8900(i2c);
> +
> + switch (event) {
> + case I2C_START_RECV:
> + /* In h/w, capture happens on any START condition, not just a
> + * START_RECV, but there is no need to actually capture on
> + * START_SEND, because the guest can't get at that data
> + * without going through a START_RECV which would overwrite it.
> + */
This comment is hard to understand. Can you reword it please.
> + capture_current_time(s);
> + break;
> + case I2C_START_SEND:
> + s->addr_byte = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static int rx8900_recv(I2CSlave *i2c)
> +{
> + RX8900State *s = RX8900(i2c);
> + uint8_t res = s->nvram[s->ptr];
What happens when ->ptr is larger than your nvram array?
> + inc_regptr(s);
> + return res;
> +}
> +
> +static void validate_extension_register(RX8900State *s, uint8_t data)
> +{
> + uint8_t diffmask = data ^ s->nvram[EXTENSION_REGISTER];
> +
> + if ((diffmask & 1 << EXT_REG_TSEL0) || (diffmask & 1 << EXT_REG_TSEL1)) {
> + error_report("WARNING: RX8900 - "
> + "Timer select modified but is unimplemented");
> + }
> +
> + if ((diffmask & 1 << EXT_REG_FSEL0) || (diffmask & 1 << EXT_REG_FSEL1)) {
> + error_report("WARNING: RX8900 - "
> + "FOut Frequency modified but is unimplemented");
> + }
> +
> + if (diffmask & 1 << EXT_REG_TE) {
> + error_report("WARNING: RX8900 - "
> + "Timer enable modified but is unimplemented");
> + }
> +
> + if (diffmask & 1 << EXT_REG_USEL) {
> + error_report("WARNING: RX8900 - "
> + "Update interrupt modified but is unimplemented");
> + }
> +
> + if (diffmask & 1 << EXT_REG_WADA) {
> + error_report("WARNING: RX8900 - "
> + "Week/day alarm modified but is unimplemented");
> + }
> +
> + if (data & 1 << EXT_REG_TEST) {
> + error_report("WARNING: RX8900 - "
> + "Test bit is enabled but is forbidden by the manufacturer");
> + }
All that this function does is print "unimplemented". Why do we need it?
> +}
> +
> +static void validate_control_register(RX8900State *s, uint8_t data)
> +{
> + uint8_t diffmask = data ^ s->nvram[CONTROL_REGISTER];
> +
> + if (diffmask & 1 << CTRL_REG_RESET) {
> + error_report("WARNING: RX8900 - "
> + "Reset requested but is unimplemented");
> + }
> +
> + if (diffmask & 1 << CTRL_REG_WP0) {
> + error_report("WARNING: RX8900 - "
> + "Attempt to write to write protected bit %d in control register",
> + CTRL_REG_WP0);
> + }
> +
> + if (diffmask & 1 << CTRL_REG_WP1) {
> + error_report("WARNING: RX8900 - "
> + "Attempt to write to write protected bit %d in control register",
> + CTRL_REG_WP1);
> + }
> +
> + if (diffmask & 1 << CTRL_REG_AIE) {
> + error_report("WARNING: RX8900 - "
> + "Alarm interrupt requested but is unimplemented");
> + }
> +
> + if (diffmask & 1 << CTRL_REG_TIE) {
> + error_report("WARNING: RX8900 - "
> + "Timer interrupt requested but is unimplemented");
> + }
> +
> + if (diffmask & 1 << CTRL_REG_UIE) {
> + error_report("WARNING: RX8900 - "
> + "Update interrupt requested but is unimplemented");
> + }
> +
> +}
As above.
> +
> +static int rx8900_send(I2CSlave *i2c, uint8_t data)
> +{
> + RX8900State *s = RX8900(i2c);
> + struct tm now;
> +
> + if (s->addr_byte) {
> + s->ptr = data & (NVRAM_SIZE - 1);
> + s->addr_byte = false;
> + return 0;
> + }
> +
> + qemu_get_timedate(&now, s->offset);
> + switch (s->ptr) {
> + case SECONDS:
> + case EXT_SECONDS:
> + now.tm_sec = from_bcd(data & 0x7f);
> + s->nvram[SECONDS] = data;
> + s->nvram[EXT_SECONDS] = data;
> + break;
> +
> + case MINUTES:
> + case EXT_MINUTES:
> + now.tm_min = from_bcd(data & 0x7f);
> + s->nvram[MINUTES] = data;
> + s->nvram[EXT_MINUTES] = data;
> + break;
> +
> + case HOURS:
> + case EXT_HOURS:
> + now.tm_hour = from_bcd(data & 0x3f);
> + s->nvram[HOURS] = data;
> + s->nvram[EXT_HOURS] = data;
> + break;
> +
> + case WEEKDAY:
> + case EXT_WEEKDAY: {
> + int user_wday = 0;
> + /* The day field is supposed to contain a value in
> + * the range 0-6. Otherwise behavior is undefined.
> + */
> + switch (data) {
> + case 0x01:
> + user_wday = 0;
> + break;
> + case 0x02:
> + user_wday = 1;
> + break;
> + case 0x04:
> + user_wday = 2;
> + break;
> + case 0x08:
> + user_wday = 3;
> + break;
> + case 0x10:
> + user_wday = 4;
> + break;
> + case 0x20:
> + user_wday = 5;
> + break;
> + case 0x40:
> + user_wday = 6;
> + break;
The weekday is the bit index in data. You can use ffs or log2 to
assign this to user_wday instead of having the nested switch
statement.
> + default:
> + error_report("WARNING: RX8900 - weekday data '%x' is out of range,"
> + " undefined behavior will result", data);
> + break;
> + }
> + s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
> + s->nvram[WEEKDAY] = data;
> + s->nvram[EXT_WEEKDAY] = data;
> + break;
> + }
> +
> + case DAY:
> + case EXT_DAY:
> + now.tm_mday = from_bcd(data & 0x3f);
> + s->nvram[DAY] = data;
> + s->nvram[EXT_DAY] = data;
> + break;
> +
> + case MONTH:
> + case EXT_MONTH:
> + now.tm_mon = from_bcd(data & 0x1f) - 1;
> + s->nvram[MONTH] = data;
> + s->nvram[EXT_MONTH] = data;
> + break;
> +
> + case YEAR:
> + case EXT_YEAR:
> + now.tm_year = from_bcd(data) + 100;
> + s->nvram[YEAR] = data;
> + s->nvram[EXT_YEAR] = data;
> + break;
> +
> + case EXTENSION_REGISTER:
> + case EXT_EXTENSION_REGISTER:
> + validate_extension_register(s, data);
> + s->nvram[EXTENSION_REGISTER] = data;
> + s->nvram[EXT_EXTENSION_REGISTER] = data;
> + break;
> +
> + case CONTROL_REGISTER:
> + case EXT_CONTROL_REGISTER:
> + validate_control_register(s, data);
> + s->nvram[EXTENSION_REGISTER] = data;
> + s->nvram[EXT_EXTENSION_REGISTER] = data;
> + break;
> +
> + default:
> + s->nvram[s->ptr] = data;
> + }
> +
> + inc_regptr(s);
> + return 0;
> +}
> +
> +static int rx8900_init(I2CSlave *i2c)
> +{
> + return 0;
> +}
This does nothing. Does Qemu let you omit the callback?
> +
> +static void rx8900_reset(DeviceState *dev)
> +{
> + RX8900State *s = RX8900(dev);
> +
> + /* The clock is running and synchronized with the host */
> + s->offset = 0;
> + memset(s->nvram, 0, NVRAM_SIZE);
> +
> + /* Temperature formulation from the datasheet
> + * ( TEMP[ 7:0 ] * 2 – 187.19) / 3.218
> + *
> + * Hardcode it to 25 degrees Celcius
> + */
> + s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2 */
Instead of hardcoding, expose it as a property. This way it can be set
by the monitor at runtime.
Take a look at the temp423 model.
> +
> + s->nvram[CONTROL_REGISTER] = 1 << CTRL_REG_CSEL0;
You can use BIT(CTRL_REG_CSEL0) to make this more redable.
> +
> + s->ptr = 0;
> + s->addr_byte = false;
> +}
> +
> +static void rx8900_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> + k->init = rx8900_init;
> + k->event = rx8900_event;
> + k->recv = rx8900_recv;
> + k->send = rx8900_send;
> + dc->reset = rx8900_reset;
> + dc->vmsd = &vmstate_rx8900;
> +}
> +
> +static const TypeInfo rx8900_info = {
> + .name = TYPE_RX8900,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_size = sizeof(RX8900State),
> + .class_init = rx8900_class_init,
> +};
> +
> +static void rx8900_register_types(void)
> +{
> + type_register_static(&rx8900_info);
> +}
> +
> +type_init(rx8900_register_types)
> --
> 2.7.4
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only)
2016-10-27 2:27 ` [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only) alastair
@ 2016-10-27 4:21 ` Joel Stanley
2016-10-27 4:29 ` Alastair D'Silva
0 siblings, 1 reply; 16+ messages in thread
From: Joel Stanley @ 2016-10-27 4:21 UTC (permalink / raw)
To: alastair; +Cc: OpenBMC Maillist, clg, Alastair D'Silva
On Thu, Oct 27, 2016 at 12:57 PM, <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
You need a commit message.
I don't know much about Qemu testing, so I'll let Andrew and Cedric comment.
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> hw/arm/imx25_pdk.c | 9 +++---
> tests/Makefile.include | 2 ++
> tests/rx8900-test.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 89 insertions(+), 4 deletions(-)
> create mode 100644 tests/rx8900-test.c
>
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index 025b608..99a819f 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -134,13 +134,14 @@ static void imx25_pdk_init(MachineState *machine)
> arm_load_kernel(&s->soc.cpu, &imx25_pdk_binfo);
> } else {
> /*
> - * This I2C device doesn't exist on the real board.
> + * These I2C devices doesn't exist on the real board.
> * We add it here (only on qtest usage) to be able to do a bit
> * of simple qtest. See "make check" for details.
> */
> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
> - "i2c"),
> - "ds1338", 0x68);
> + I2CBus *i2c = (I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
> + "i2c");
> + i2c_create_slave(i2c, "ds1338", 0x68);
> + i2c_create_slave(i2c, "rx8900", 0x32);
I'm not sure how upstream will take this.
Is there a reason you didn't add them to an aspeed board instead?
> }
> }
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index cbe38ad..3f08ba9 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -297,6 +297,7 @@ check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
>
> check-qtest-arm-y = tests/tmp105-test$(EXESUF)
> check-qtest-arm-y += tests/ds1338-test$(EXESUF)
> +check-qtest-arm-y += tests/rx8900-test$(EXESUF)
> check-qtest-arm-y += tests/m25p80-test$(EXESUF)
> gcov-files-arm-y += hw/misc/tmp105.c
> check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
> @@ -629,6 +630,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
> tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
> tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
> +tests/rx8900-test$(EXESUF): tests/rx8900-test.o $(libqos-imx-obj-y)
> tests/m25p80-test$(EXESUF): tests/m25p80-test.o
> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
> tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
> diff --git a/tests/rx8900-test.c b/tests/rx8900-test.c
> new file mode 100644
> index 0000000..8d3ecd8
> --- /dev/null
> +++ b/tests/rx8900-test.c
> @@ -0,0 +1,82 @@
> +/*
> + * QTest testcase for the DS1338 RTC
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "libqos/i2c.h"
> +
> +#define IMX25_I2C_0_BASE 0x43F80000
> +
> +#define RX8900_ADDR 0x32
> +
> +static I2CAdapter *i2c;
> +static uint8_t addr;
> +
> +static inline uint8_t bcd2bin(uint8_t x)
> +{
> + return ((x) & 0x0f) + ((x) >> 4) * 10;
> +}
> +
> +static void send_and_receive(void)
> +{
> + uint8_t cmd[1];
> + uint8_t resp[7];
> + time_t now = time(NULL);
> + struct tm *tm_ptr;
> +
> + /* reset the index in the RTC memory */
> + cmd[0] = 0;
> + i2c_send(i2c, addr, cmd, 1);
> +
> + tm_ptr = gmtime(&now);
> + /* retrieve the date */
> + i2c_recv(i2c, addr, resp, 7);
> +
> + /* check retrieved time against local time */
> + g_assert_cmpuint(bcd2bin(resp[0]), == , tm_ptr->tm_sec);
> + g_assert_cmpuint(bcd2bin(resp[1]), == , tm_ptr->tm_min);
> + g_assert_cmpuint(bcd2bin(resp[2]), == , tm_ptr->tm_hour);
> + g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
> + g_assert_cmpuint(bcd2bin(resp[5]), == , 1 + tm_ptr->tm_mon);
> + g_assert_cmpuint(2000 + bcd2bin(resp[6]), == , 1900 + tm_ptr->tm_year);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + QTestState *s = NULL;
> + int ret;
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + /* Address must match RX8900_ADDR */
> + s = qtest_start("-display none -machine imx25-pdk");
> + i2c = imx_i2c_create(IMX25_I2C_0_BASE);
> + addr = RX8900_ADDR;
> +
> + qtest_add_func("/rx8900/tx-rx", send_and_receive);
> +
> + ret = g_test_run();
> +
> + if (s) {
> + qtest_quit(s);
> + }
> + g_free(i2c);
> +
> + return ret;
> +}
> --
> 2.7.4
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
` (2 preceding siblings ...)
2016-10-27 2:27 ` [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only) alastair
@ 2016-10-27 4:22 ` Joel Stanley
2016-10-27 4:33 ` Alastair D'Silva
2016-10-31 6:54 ` Cédric Le Goater
4 siblings, 1 reply; 16+ messages in thread
From: Joel Stanley @ 2016-10-27 4:22 UTC (permalink / raw)
To: alastair; +Cc: OpenBMC Maillist, clg, Alastair D'Silva
Hi Alastair,
On Thu, Oct 27, 2016 at 12:57 PM, <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This series adds preliminary support for the Epson rx8900 RTC.
>
> Setting and retrieving time is supported, and the chip temperature
> is hardcoded.
I commented in the patch about setting the temperature with a property
so we can adjust it at runtime with the monitor.
> Other features (in particular, those that require the interrupt line)
> are not implmented.
Do you intend to implement these features?
Cheers,
Joel
>
> Alastair D'Silva (3):
> Add Epson RX8900 RTC support
> Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
> Add a unit test for RX8900 RTC (time functionality only)
>
> default-configs/arm-softmmu.mak | 1 +
> hw/arm/aspeed.c | 4 +
> hw/arm/imx25_pdk.c | 9 +-
> hw/timer/Makefile.objs | 1 +
> hw/timer/rx8900.c | 426 ++++++++++++++++++++++++++++++++++++++++
> tests/Makefile.include | 2 +
> tests/rx8900-test.c | 82 ++++++++
> 7 files changed, 521 insertions(+), 4 deletions(-)
> create mode 100644 hw/timer/rx8900.c
> create mode 100644 tests/rx8900-test.c
>
> --
> 2.7.4
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only)
2016-10-27 4:21 ` Joel Stanley
@ 2016-10-27 4:29 ` Alastair D'Silva
2016-10-31 6:49 ` Cédric Le Goater
0 siblings, 1 reply; 16+ messages in thread
From: Alastair D'Silva @ 2016-10-27 4:29 UTC (permalink / raw)
To: Joel Stanley; +Cc: OpenBMC Maillist, clg
On Thu, 2016-10-27 at 14:51 +1030, Joel Stanley wrote:
> > - * This I2C device doesn't exist on the real board.
> > + * These I2C devices doesn't exist on the real board.
> > * We add it here (only on qtest usage) to be able to do a
> > bit
> > * of simple qtest. See "make check" for details.
> > */
> > - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s-
> > >soc.i2c[0]),
> > - "i2c"),
> > - "ds1338", 0x68);
> > + I2CBus *i2c = (I2CBus *)qdev_get_child_bus(DEVICE(&s-
> > >soc.i2c[0]),
> > + "i2c");
> > + i2c_create_slave(i2c, "ds1338", 0x68);
> > + i2c_create_slave(i2c, "rx8900", 0x32);
>
> I'm not sure how upstream will take this.
>
> Is there a reason you didn't add them to an aspeed board instead?
>
We are currently missing the *_i2c_create and the associated
send/receive callbacks required to access the i2c bus from the test
framework. The structure of aspeed_i2c.c was different enough from
i2c_imx.c & i2c_omap.c that I need more familiarity with the code
before I could implement it myself.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC
2016-10-27 4:22 ` [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC Joel Stanley
@ 2016-10-27 4:33 ` Alastair D'Silva
2016-10-31 0:22 ` Andrew Jeffery
0 siblings, 1 reply; 16+ messages in thread
From: Alastair D'Silva @ 2016-10-27 4:33 UTC (permalink / raw)
To: Joel Stanley; +Cc: OpenBMC Maillist, clg
On Thu, 2016-10-27 at 14:52 +1030, Joel Stanley wrote:
> Hi Alastair,
>
> On Thu, Oct 27, 2016 at 12:57 PM, <alastair@au1.ibm.com> wrote:
> >
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > This series adds preliminary support for the Epson rx8900 RTC.
> >
> > Setting and retrieving time is supported, and the chip temperature
> > is hardcoded.
>
> I commented in the patch about setting the temperature with a
> property
> so we can adjust it at runtime with the monitor.
That sounds like a good idea.
> >
> > Other features (in particular, those that require the interrupt
> > line)
> > are not implmented.
>
> Do you intend to implement these features?
Only if necessary. I need to understand how to wire the virtual non-i2c
interrupt line from the RTC to the SOC before these could be
implemented.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Add Epson RX8900 RTC support
2016-10-27 4:19 ` Joel Stanley
@ 2016-10-27 5:03 ` Alastair D'Silva
2016-10-31 6:40 ` Cédric Le Goater
0 siblings, 1 reply; 16+ messages in thread
From: Alastair D'Silva @ 2016-10-27 5:03 UTC (permalink / raw)
To: Joel Stanley; +Cc: OpenBMC Maillist, clg
On Thu, 2016-10-27 at 14:49 +1030, Joel Stanley wrote:
<snip>
> index 0000000..4c15da8
> > --- /dev/null
> > +++ b/hw/timer/rx8900.c
> > @@ -0,0 +1,426 @@
> > +/*
> > + * Epson RX8900SA/CE Realtime Clock Module
> > + *
> > + * Copyright (c) 2016 IBM Corporation
> > + * Authors:
> > + * Alastair D'Silva <alastair@d-silva.org>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later
> > version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General
> > Public
> > + * License along with this library; if not, see <http://www.gnu.or
> > g/licenses/>
>
> Qemu tends to use a brief copyright header. Take a look at
> hw/arm/aspeed_soc.c:
>
Ok.
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "qemu/bcd.h"
> > +#include "qemu/error-report.h"
> > +
> > +#define TYPE_RX8900 "rx8900"
> > +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
> > +
> > +#define NVRAM_SIZE 0x20
> > +
> > +typedef enum RX8900Addresses {
>
> This typedef is unnecessary.
>
How so? It add semantics that define the context that these values can
be used, so from a maintainability point of view, it is necessary. A
similar pattern is used in i2c-omap.c.
> >
> > + SECONDS = 0x00,
> > + MINUTES = 0x01,
> > + HOURS = 0x02,
> > + WEEKDAY = 0x03,
> > + DAY = 0x04,
> > + MONTH = 0x05,
> > + YEAR = 0x06,
> > + RAM = 0x07,
> > + ALARM_MINUTE = 0x08,
> > + ALARM_HOUR = 0x09,
> > + ALARM_WEEK_DAY = 0x0A,
> > + TIMER_COUNTER_0 = 0x0B,
> > + TIMER_COUNTER_1 = 0x0C,
> > + EXTENSION_REGISTER = 0x0D,
> > + FLAG_REGISTER = 0X0E,
> > + CONTROL_REGISTER = 0X0F,
> > + EXT_SECONDS = 0x010, /* Alias of SECONDS */
> > + EXT_MINUTES = 0x11, /* Alias of MINUTES */
> > + EXT_HOURS = 0x12, /* Alias of HOURS */
> > + EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
> > + EXT_DAY = 0x14, /* Alias of DAY */
> > + EXT_MONTH = 0x15, /* Alias of MONTH */
> > + EXT_YEAR = 0x16, /* Alias of YEAR */
> > + TEMPERATURE = 0x17,
> > + BACKUP_FUNCTION = 0x18,
> > + NO_USE_1 = 0x19,
> > + NO_USE_2 = 0x1A,
> > + EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
> > + EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
> > + EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER
> > */
> > + EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
> > + EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
>
> Do you use all of these values? I suggest only including the
> definitions for the ones you need.
>
Most are at the moment, and all will be required for a full
implementation (at which point, one would have to play "guess which
item is missing from the list").
> Some models in Qemu chose not to have #defines (or enums) for the
> registers, and instead use comments in the switch statements for the
> operations they support.
This sounds like it would lead to unmaintainable code, especially when
the same "magic numbers" are used in multiple places.
+
> > + switch (event) {
> > + case I2C_START_RECV:
> > + /* In h/w, capture happens on any START condition, not
> > just a
> > + * START_RECV, but there is no need to actually capture on
> > + * START_SEND, because the guest can't get at that data
> > + * without going through a START_RECV which would
> > overwrite it.
> > + */
>
> This comment is hard to understand. Can you reword it please.
Whoops, that was brought over from ds1338.c, I'll have a look.
> > +static int rx8900_recv(I2CSlave *i2c)
> > +{
> > + RX8900State *s = RX8900(i2c);
> > + uint8_t res = s->nvram[s->ptr];
>
> What happens when ->ptr is larger than your nvram array?
s->ptr is wrapped to stay within the array.
> > +
> > +static void validate_extension_register(RX8900State *s, uint8_t
> > data)
> > +{
> > + uint8_t diffmask = data ^ s->nvram[EXTENSION_REGISTER];
> > +
> > + if ((diffmask & 1 << EXT_REG_TSEL0) || (diffmask & 1 <<
> > EXT_REG_TSEL1)) {
> > + error_report("WARNING: RX8900 - "
> > + "Timer select modified but is unimplemented");
> > + }
> > +
> > + if ((diffmask & 1 << EXT_REG_FSEL0) || (diffmask & 1 <<
> > EXT_REG_FSEL1)) {
> > + error_report("WARNING: RX8900 - "
> > + "FOut Frequency modified but is unimplemented");
> > + }
> > +
> > + if (diffmask & 1 << EXT_REG_TE) {
> > + error_report("WARNING: RX8900 - "
> > + "Timer enable modified but is unimplemented");
> > + }
> > +
> > + if (diffmask & 1 << EXT_REG_USEL) {
> > + error_report("WARNING: RX8900 - "
> > + "Update interrupt modified but is unimplemented");
> > + }
> > +
> > + if (diffmask & 1 << EXT_REG_WADA) {
> > + error_report("WARNING: RX8900 - "
> > + "Week/day alarm modified but is unimplemented");
> > + }
> > +
> > + if (data & 1 << EXT_REG_TEST) {
> > + error_report("WARNING: RX8900 - "
> > + "Test bit is enabled but is forbidden by the
> > manufacturer");
> > + }
>
> All that this function does is print "unimplemented". Why do we need
> it?
So that when a user pokes these registers expecting something to
happen, they know what they poked and why it did not have the effect
the requested. Just printing out "unimplemented" does not give the user
enough context to diagnose the problem.
> >
> > +
> > +static int rx8900_send(I2CSlave *i2c, uint8_t data)
> > +{
> > + RX8900State *s = RX8900(i2c);
> > + struct tm now;
> > +
> > + if (s->addr_byte) {
> > + s->ptr = data & (NVRAM_SIZE - 1);
> > + s->addr_byte = false;
> > + return 0;
> > + }
> > +
> > + qemu_get_timedate(&now, s->offset);
> > + switch (s->ptr) {
> > + case SECONDS:
> > + case EXT_SECONDS:
> > + now.tm_sec = from_bcd(data & 0x7f);
> > + s->nvram[SECONDS] = data;
> > + s->nvram[EXT_SECONDS] = data;
> > + break;
> > +
> > + case MINUTES:
> > + case EXT_MINUTES:
> > + now.tm_min = from_bcd(data & 0x7f);
> > + s->nvram[MINUTES] = data;
> > + s->nvram[EXT_MINUTES] = data;
> > + break;
> > +
> > + case HOURS:
> > + case EXT_HOURS:
> > + now.tm_hour = from_bcd(data & 0x3f);
> > + s->nvram[HOURS] = data;
> > + s->nvram[EXT_HOURS] = data;
> > + break;
> > +
> > + case WEEKDAY:
> > + case EXT_WEEKDAY: {
> > + int user_wday = 0;
> > + /* The day field is supposed to contain a value in
> > + * the range 0-6. Otherwise behavior is undefined.
> > + */
> > + switch (data) {
> > + case 0x01:
> > + user_wday = 0;
> > + break;
> > + case 0x02:
> > + user_wday = 1;
> > + break;
> > + case 0x04:
> > + user_wday = 2;
> > + break;
> > + case 0x08:
> > + user_wday = 3;
> > + break;
> > + case 0x10:
> > + user_wday = 4;
> > + break;
> > + case 0x20:
> > + user_wday = 5;
> > + break;
> > + case 0x40:
> > + user_wday = 6;
> > + break;
>
> The weekday is the bit index in data. You can use ffs or log2 to
> assign this to user_wday instead of having the nested switch
> statement.
Ok, thanks, this code was quite horrible.
> > +static int rx8900_init(I2CSlave *i2c)
> > +{
> > + return 0;
> > +}
>
> This does nothing. Does Qemu let you omit the callback?
>
I'm not sure, this was taken from ds1338.c.
> >
> > +
> > +static void rx8900_reset(DeviceState *dev)
> > +{
> > + RX8900State *s = RX8900(dev);
> > +
> > + /* The clock is running and synchronized with the host */
> > + s->offset = 0;
> > + memset(s->nvram, 0, NVRAM_SIZE);
> > +
> > + /* Temperature formulation from the datasheet
> > + * ( TEMP[ 7:0 ] * 2 – 187.19) / 3.218
> > + *
> > + * Hardcode it to 25 degrees Celcius
> > + */
> > + s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2 */
>
> Instead of hardcoding, expose it as a property. This way it can be
> set
> by the monitor at runtime.
>
> Take a look at the temp423 model.
Ok, that sounds good.
> >
> > +
> > + s->nvram[CONTROL_REGISTER] = 1 << CTRL_REG_CSEL0;
>
> You can use BIT(CTRL_REG_CSEL0) to make this more redable.
Ok.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC
2016-10-27 4:33 ` Alastair D'Silva
@ 2016-10-31 0:22 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2016-10-31 0:22 UTC (permalink / raw)
To: Alastair D'Silva, Joel Stanley; +Cc: clg, OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]
On Thu, 2016-10-27 at 15:33 +1100, Alastair D'Silva wrote:
> On Thu, 2016-10-27 at 14:52 +1030, Joel Stanley wrote:
> >
> > Hi Alastair,
> >
> > On Thu, Oct 27, 2016 at 12:57 PM, <alastair@au1.ibm.com> wrote:
> > >
> > >
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > This series adds preliminary support for the Epson rx8900 RTC.
> > >
> > > Setting and retrieving time is supported, and the chip temperature
> > > is hardcoded.
> > I commented in the patch about setting the temperature with a
> > property
> > so we can adjust it at runtime with the monitor.
> That sounds like a good idea.
>
> >
> > >
> > >
> > > Other features (in particular, those that require the interrupt
> > > line)
> > > are not implmented.
> > Do you intend to implement these features?
> Only if necessary. I need to understand how to wire the virtual non-i2c
> interrupt line from the RTC to the SOC before these could be
> implemented.
There are two parts: One is to add a qemu_irq member to your device's
state struct, and initialise it in your realize() with
sysbus_init_irq().
The second part is to wire up the interrupt at the machine level: This
time in the machine's realize(), invoke:
qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->vic), irq_nr)
to grab a qemu_irq, and connect it with
sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), 0, irq)
In this case, your magic irq_nr value is 22. The interrupt controller's
default configuration matches the requirements for irq 22, so there
shouldn't be anything further.
Hope that helps,
Andrew
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Add Epson RX8900 RTC support
2016-10-27 5:03 ` Alastair D'Silva
@ 2016-10-31 6:40 ` Cédric Le Goater
0 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-10-31 6:40 UTC (permalink / raw)
To: Alastair D'Silva, Joel Stanley; +Cc: OpenBMC Maillist
On 10/27/2016 07:03 AM, Alastair D'Silva wrote:
> On Thu, 2016-10-27 at 14:49 +1030, Joel Stanley wrote:
> <snip>
>> index 0000000..4c15da8
>>> --- /dev/null
>>> +++ b/hw/timer/rx8900.c
>>> @@ -0,0 +1,426 @@
>>> +/*
>>> + * Epson RX8900SA/CE Realtime Clock Module
>>> + *
>>> + * Copyright (c) 2016 IBM Corporation
>>> + * Authors:
>>> + * Alastair D'Silva <alastair@d-silva.org>
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later
>>> version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General
>>> Public
>>> + * License along with this library; if not, see <http://www.gnu.or
>>> g/licenses/>
>>
>> Qemu tends to use a brief copyright header. Take a look at
>> hw/arm/aspeed_soc.c:
>>
>
> Ok.
>
>>> +#include "qemu/osdep.h"
>>> +#include "qemu-common.h"
>>> +#include "hw/i2c/i2c.h"
>>> +#include "qemu/bcd.h"
>>> +#include "qemu/error-report.h"
>>> +
>>> +#define TYPE_RX8900 "rx8900"
>>> +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
>>> +
>>> +#define NVRAM_SIZE 0x20
>>> +
>>> +typedef enum RX8900Addresses {
>>
>> This typedef is unnecessary.
>>
>
> How so? It add semantics that define the context that these values can
> be used, so from a maintainability point of view, it is necessary. A
> similar pattern is used in i2c-omap.c.
>
>>>
>>> + SECONDS = 0x00,
>>> + MINUTES = 0x01,
>>> + HOURS = 0x02,
>>> + WEEKDAY = 0x03,
>>> + DAY = 0x04,
>>> + MONTH = 0x05,
>>> + YEAR = 0x06,
>>> + RAM = 0x07,
>>> + ALARM_MINUTE = 0x08,
>>> + ALARM_HOUR = 0x09,
>>> + ALARM_WEEK_DAY = 0x0A,
>>> + TIMER_COUNTER_0 = 0x0B,
>>> + TIMER_COUNTER_1 = 0x0C,
>>> + EXTENSION_REGISTER = 0x0D,
>>> + FLAG_REGISTER = 0X0E,
>>> + CONTROL_REGISTER = 0X0F,
>>> + EXT_SECONDS = 0x010, /* Alias of SECONDS */
>>> + EXT_MINUTES = 0x11, /* Alias of MINUTES */
>>> + EXT_HOURS = 0x12, /* Alias of HOURS */
>>> + EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
>>> + EXT_DAY = 0x14, /* Alias of DAY */
>>> + EXT_MONTH = 0x15, /* Alias of MONTH */
>>> + EXT_YEAR = 0x16, /* Alias of YEAR */
>>> + TEMPERATURE = 0x17,
>>> + BACKUP_FUNCTION = 0x18,
>>> + NO_USE_1 = 0x19,
>>> + NO_USE_2 = 0x1A,
>>> + EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
>>> + EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
>>> + EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER
>>> */
>>> + EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
>>> + EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
>>
>> Do you use all of these values? I suggest only including the
>> definitions for the ones you need.
>>
>
> Most are at the moment, and all will be required for a full
> implementation (at which point, one would have to play "guess which
> item is missing from the list").
>
>> Some models in Qemu chose not to have #defines (or enums) for the
>> registers, and instead use comments in the switch statements for the
>> operations they support.
>
> This sounds like it would lead to unmaintainable code, especially when
> the same "magic numbers" are used in multiple places.
> +
>>> + switch (event) {
>>> + case I2C_START_RECV:
>>> + /* In h/w, capture happens on any START condition, not
>>> just a
>>> + * START_RECV, but there is no need to actually capture on
>>> + * START_SEND, because the guest can't get at that data
>>> + * without going through a START_RECV which would
>>> overwrite it.
>>> + */
>>
>> This comment is hard to understand. Can you reword it please.
>
> Whoops, that was brought over from ds1338.c, I'll have a look.
>
>>> +static int rx8900_recv(I2CSlave *i2c)
>>> +{
>>> + RX8900State *s = RX8900(i2c);
>>> + uint8_t res = s->nvram[s->ptr];
>>
>> What happens when ->ptr is larger than your nvram array?
>
> s->ptr is wrapped to stay within the array.
may be add a (s->ptr % size) ?
>
>>> +
>>> +static void validate_extension_register(RX8900State *s, uint8_t
>>> data)
>>> +{
>>> + uint8_t diffmask = data ^ s->nvram[EXTENSION_REGISTER];
>>> +
>>> + if ((diffmask & 1 << EXT_REG_TSEL0) || (diffmask & 1 <<
>>> EXT_REG_TSEL1)) {
>>> + error_report("WARNING: RX8900 - "
>>> + "Timer select modified but is unimplemented");
>>> + }
>>> +
>>> + if ((diffmask & 1 << EXT_REG_FSEL0) || (diffmask & 1 <<
>>> EXT_REG_FSEL1)) {
>>> + error_report("WARNING: RX8900 - "
>>> + "FOut Frequency modified but is unimplemented");
>>> + }
>>> +
>>> + if (diffmask & 1 << EXT_REG_TE) {
>>> + error_report("WARNING: RX8900 - "
>>> + "Timer enable modified but is unimplemented");
>>> + }
>>> +
>>> + if (diffmask & 1 << EXT_REG_USEL) {
>>> + error_report("WARNING: RX8900 - "
>>> + "Update interrupt modified but is unimplemented");
>>> + }
>>> +
>>> + if (diffmask & 1 << EXT_REG_WADA) {
>>> + error_report("WARNING: RX8900 - "
>>> + "Week/day alarm modified but is unimplemented");
>>> + }
>>> +
>>> + if (data & 1 << EXT_REG_TEST) {
>>> + error_report("WARNING: RX8900 - "
>>> + "Test bit is enabled but is forbidden by the
>>> manufacturer");
>>> + }
>>
>> All that this function does is print "unimplemented". Why do we need
>> it?
>
> So that when a user pokes these registers expecting something to
> happen, they know what they poked and why it did not have the effect
> the requested. Just printing out "unimplemented" does not give the user
> enough context to diagnose the problem.
You can have some defines for the constants : (1 << EXT_REG_*) and you should
use LOG_UNIMP instead of error_report.
>
>>>
>>> +
>>> +static int rx8900_send(I2CSlave *i2c, uint8_t data)
>>> +{
>>> + RX8900State *s = RX8900(i2c);
>>> + struct tm now;
>>> +
>>> + if (s->addr_byte) {
>>> + s->ptr = data & (NVRAM_SIZE - 1);
>>> + s->addr_byte = false;
>>> + return 0;
>>> + }
>>> +
>>> + qemu_get_timedate(&now, s->offset);
>>> + switch (s->ptr) {
>>> + case SECONDS:
>>> + case EXT_SECONDS:
>>> + now.tm_sec = from_bcd(data & 0x7f);
>>> + s->nvram[SECONDS] = data;
>>> + s->nvram[EXT_SECONDS] = data;
>>> + break;
>>> +
>>> + case MINUTES:
>>> + case EXT_MINUTES:
>>> + now.tm_min = from_bcd(data & 0x7f);
>>> + s->nvram[MINUTES] = data;
>>> + s->nvram[EXT_MINUTES] = data;
>>> + break;
>>> +
>>> + case HOURS:
>>> + case EXT_HOURS:
>>> + now.tm_hour = from_bcd(data & 0x3f);
>>> + s->nvram[HOURS] = data;
>>> + s->nvram[EXT_HOURS] = data;
>>> + break;
>>> +
>>> + case WEEKDAY:
>>> + case EXT_WEEKDAY: {
>>> + int user_wday = 0;
>>> + /* The day field is supposed to contain a value in
>>> + * the range 0-6. Otherwise behavior is undefined.
>>> + */
>>> + switch (data) {
>>> + case 0x01:
>>> + user_wday = 0;
>>> + break;
>>> + case 0x02:
>>> + user_wday = 1;
>>> + break;
>>> + case 0x04:
>>> + user_wday = 2;
>>> + break;
>>> + case 0x08:
>>> + user_wday = 3;
>>> + break;
>>> + case 0x10:
>>> + user_wday = 4;
>>> + break;
>>> + case 0x20:
>>> + user_wday = 5;
>>> + break;
>>> + case 0x40:
>>> + user_wday = 6;
>>> + break;
>>
>> The weekday is the bit index in data. You can use ffs or log2 to
>> assign this to user_wday instead of having the nested switch
>> statement.
>
> Ok, thanks, this code was quite horrible.
>
>>> +static int rx8900_init(I2CSlave *i2c)
>>> +{
>>> + return 0;
>>> +}
>>
>> This does nothing. Does Qemu let you omit the callback?
>>
>
> I'm not sure, this was taken from ds1338.c.
>
>>>
>>> +
>>> +static void rx8900_reset(DeviceState *dev)
>>> +{
>>> + RX8900State *s = RX8900(dev);
>>> +
>>> + /* The clock is running and synchronized with the host */
>>> + s->offset = 0;
>>> + memset(s->nvram, 0, NVRAM_SIZE);
>>> +
>>> + /* Temperature formulation from the datasheet
>>> + * ( TEMP[ 7:0 ] * 2 – 187.19) / 3.218
>>> + *
>>> + * Hardcode it to 25 degrees Celcius
>>> + */
>>> + s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2 */
>>
>> Instead of hardcoding, expose it as a property. This way it can be
>> set
>> by the monitor at runtime.
>>
>> Take a look at the temp423 model.
>
> Ok, that sounds good.
oh yes. If you could revive that device that would be nice also :)
Andrew had some concerns about I2C :
https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03721.html
I did not take the time to fix it yet.
>
>>>
>>> +
>>> + s->nvram[CONTROL_REGISTER] = 1 << CTRL_REG_CSEL0;
>>
>> You can use BIT(CTRL_REG_CSEL0) to make this more redable.
>
> Ok.
>
I have not looked at the specs but CTRL_REG_CSEL0 makes one think that there
could be more than one ? May be add a helper ?
Cheers,
C.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
2016-10-27 2:27 ` [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32 alastair
@ 2016-10-31 6:41 ` Cédric Le Goater
2016-11-01 4:44 ` Alastair D'Silva
0 siblings, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2016-10-31 6:41 UTC (permalink / raw)
To: alastair, openbmc; +Cc: Alastair D'Silva
On 10/27/2016 04:27 AM, alastair@au1.ibm.com wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> hw/arm/aspeed.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index c7206fd..8ccac3e 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -100,6 +100,7 @@ static void aspeed_board_init(MachineState *machine,
> {
> AspeedBoardState *bmc;
> AspeedSoCClass *sc;
> + I2CBus *i2c12;
>
> bmc = g_new0(AspeedBoardState, 1);
> object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
> @@ -137,6 +138,9 @@ static void aspeed_board_init(MachineState *machine,
> aspeed_board_binfo.ram_size = ram_size;
> aspeed_board_binfo.loader_start = sc->info->sdram_base;
>
> + i2c12 = aspeed_i2c_get_bus((DeviceState *)&bmc->soc.i2c, 11);
> + i2c_create_slave(i2c12, "rx8900", 0x32);
> +
hmm, so that is a board device ? or a SoC ?
C.
> arm_load_kernel(ARM_CPU(first_cpu), &aspeed_board_binfo);
> }
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only)
2016-10-27 4:29 ` Alastair D'Silva
@ 2016-10-31 6:49 ` Cédric Le Goater
0 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-10-31 6:49 UTC (permalink / raw)
To: Alastair D'Silva, Joel Stanley; +Cc: OpenBMC Maillist
On 10/27/2016 06:29 AM, Alastair D'Silva wrote:
> On Thu, 2016-10-27 at 14:51 +1030, Joel Stanley wrote:
>>> - * This I2C device doesn't exist on the real board.
>>> + * These I2C devices doesn't exist on the real board.
>>> * We add it here (only on qtest usage) to be able to do a
>>> bit
>>> * of simple qtest. See "make check" for details.
>>> */
>>> - i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s-
>>>> soc.i2c[0]),
>>> - "i2c"),
>>> - "ds1338", 0x68);
>>> + I2CBus *i2c = (I2CBus *)qdev_get_child_bus(DEVICE(&s-
>>>> soc.i2c[0]),
>>> + "i2c");
>>> + i2c_create_slave(i2c, "ds1338", 0x68);
>>> + i2c_create_slave(i2c, "rx8900", 0x32);
>>
>> I'm not sure how upstream will take this.
>>
>> Is there a reason you didn't add them to an aspeed board instead?
>>
>
> We are currently missing the *_i2c_create and the associated
> send/receive callbacks required to access the i2c bus from the test
> framework. The structure of aspeed_i2c.c was different enough from
> i2c_imx.c & i2c_omap.c that I need more familiarity with the code
> before I could implement it myself.
Could you extend libqos to have these routines ? or you can start an
aspeed guest which would have the device with your patchset. take a
look at the m25p80 test.
Cheers,
C.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
` (3 preceding siblings ...)
2016-10-27 4:22 ` [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC Joel Stanley
@ 2016-10-31 6:54 ` Cédric Le Goater
4 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2016-10-31 6:54 UTC (permalink / raw)
To: alastair, openbmc; +Cc: Alastair D'Silva
On 10/27/2016 04:27 AM, alastair@au1.ibm.com wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This series adds preliminary support for the Epson rx8900 RTC.
>
> Setting and retrieving time is supported, and the chip temperature
> is hardcoded.
>
> Other features (in particular, those that require the interrupt line)
> are not implmented.
That's not too complex to do but you can keep that for a followup patch.
I think you can send directly to mainline, after the few fixes.
Cheers,
C.
> Alastair D'Silva (3):
> Add Epson RX8900 RTC support
> Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
> Add a unit test for RX8900 RTC (time functionality only)
>
> default-configs/arm-softmmu.mak | 1 +
> hw/arm/aspeed.c | 4 +
> hw/arm/imx25_pdk.c | 9 +-
> hw/timer/Makefile.objs | 1 +
> hw/timer/rx8900.c | 426 ++++++++++++++++++++++++++++++++++++++++
> tests/Makefile.include | 2 +
> tests/rx8900-test.c | 82 ++++++++
> 7 files changed, 521 insertions(+), 4 deletions(-)
> create mode 100644 hw/timer/rx8900.c
> create mode 100644 tests/rx8900-test.c
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32
2016-10-31 6:41 ` Cédric Le Goater
@ 2016-11-01 4:44 ` Alastair D'Silva
0 siblings, 0 replies; 16+ messages in thread
From: Alastair D'Silva @ 2016-11-01 4:44 UTC (permalink / raw)
To: Cédric Le Goater, openbmc
On Mon, 2016-10-31 at 07:41 +0100, Cédric Le Goater wrote:
> On 10/27/2016 04:27 AM, alastair@au1.ibm.com wrote:
> >
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > hw/arm/aspeed.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index c7206fd..8ccac3e 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -100,6 +100,7 @@ static void aspeed_board_init(MachineState
> > *machine,
> > {
> > AspeedBoardState *bmc;
> > AspeedSoCClass *sc;
> > + I2CBus *i2c12;
> >
> > bmc = g_new0(AspeedBoardState, 1);
> > object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg-
> > >soc_name);
> > @@ -137,6 +138,9 @@ static void aspeed_board_init(MachineState
> > *machine,
> > aspeed_board_binfo.ram_size = ram_size;
> > aspeed_board_binfo.loader_start = sc->info->sdram_base;
> >
> > + i2c12 = aspeed_i2c_get_bus((DeviceState *)&bmc->soc.i2c, 11);
> > + i2c_create_slave(i2c12, "rx8900", 0x32);
> > +
>
> hmm, so that is a board device ? or a SoC ?
>
> C.
>
It's a board device.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-11-01 4:44 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-27 2:27 [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC alastair
2016-10-27 2:27 ` [PATCH 1/3] Add Epson RX8900 RTC support alastair
2016-10-27 4:19 ` Joel Stanley
2016-10-27 5:03 ` Alastair D'Silva
2016-10-31 6:40 ` Cédric Le Goater
2016-10-27 2:27 ` [PATCH 2/3] Connect an RX8900 RTC to i2c12 of the AST2500 SOC at address 0x32 alastair
2016-10-31 6:41 ` Cédric Le Goater
2016-11-01 4:44 ` Alastair D'Silva
2016-10-27 2:27 ` [PATCH 3/3] Add a unit test for RX8900 RTC (time functionality only) alastair
2016-10-27 4:21 ` Joel Stanley
2016-10-27 4:29 ` Alastair D'Silva
2016-10-31 6:49 ` Cédric Le Goater
2016-10-27 4:22 ` [PATCH 0/3] Add QEMU support for the Epson rx8900 RTC Joel Stanley
2016-10-27 4:33 ` Alastair D'Silva
2016-10-31 0:22 ` Andrew Jeffery
2016-10-31 6:54 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.