[Top][All Lists]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 em

From: BALATON Zoltan
Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
Date: Wed, 13 Jun 2018 10:50:59 +0200 (CEST)
User-agent: Alpine 2.21 (BSF 202 2017-01-01)

On Wed, 13 Jun 2018, David Gibson wrote:
On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
of day is implemented. Setting time and RTC alarm are not supported.
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
new file mode 100644
index 0000000..9dbdb1b
--- /dev/null
+++ b/hw/timer/m41t80.c
@@ -0,0 +1,117 @@
+ * M41T80 serial rtc emulation
+ *
+ * Copyright (c) 2018 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/i2c/i2c.h"
+#define TYPE_M41T80 "m41t80"
+#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
+typedef struct M41t80State {
+    I2CSlave parent_obj;
+    int8_t addr;
+} M41t80State;
+static void m41t80_realize(DeviceState *dev, Error **errp)
+    M41t80State *s = M41T80(dev);
+    s->addr = -1;
+static int m41t80_send(I2CSlave *i2c, uint8_t data)
+    M41t80State *s = M41T80(i2c);
+    if (s->addr < 0) {
+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here
rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this

Thanks for the review. I guess we could add enum for device bytes and the
special case -1 meaning no register address selected yet but this is a very
simple device with only 20 bytes and the datasheet also lists them by number
without naming them so I think we can also refer to them by number. Since
the device has only this 20 bytes the case with 127 should also not be a
problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here?  You're assigning the uint8_t data to
addr - could that result in a negative value?

Why wouldn't it be safe with the expected values for register address between 0-19? If the guest sends garbage values over 127 it will either result in invalid register or unselected register and lead to an error when trying to read/write that register so I don't see what other problem this may cause.

The addr < 0 is to check if no address was selected before (on creating the device and when sending first value from host addr is set to -1. In this case first write will set register address, then subsequent reads/writes increment register address as the datasheet says).


reply via email to

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