diff mbox series

[v2,03/21] hw: timer: Add ASPEED RTC device

Message ID 20190618165311.27066-4-clg@kaod.org
State New
Headers show
Series None | expand

Commit Message

Cédric Le Goater June 18, 2019, 4:52 p.m. UTC
From: Joel Stanley <joel@jms.id.au>


The RTC is modeled to provide time and date functionality. It is
initialised at zero to match the hardware.

There is no modelling of the alarm functionality, which includes the IRQ
line. As there is no guest code to exercise this function that is
acceptable for now.

Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

---
 include/hw/timer/aspeed_rtc.h |  31 ++++++
 hw/timer/aspeed_rtc.c         | 180 ++++++++++++++++++++++++++++++++++
 hw/timer/Makefile.objs        |   2 +-
 hw/timer/trace-events         |   4 +
 4 files changed, 216 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/timer/aspeed_rtc.h
 create mode 100644 hw/timer/aspeed_rtc.c

-- 
2.21.0

Comments

Peter Maydell July 2, 2019, 7:19 p.m. UTC | #1
On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:
>

> From: Joel Stanley <joel@jms.id.au>

>

> The RTC is modeled to provide time and date functionality. It is

> initialised at zero to match the hardware.

>

> There is no modelling of the alarm functionality, which includes the IRQ

> line. As there is no guest code to exercise this function that is

> acceptable for now.

>

> Signed-off-by: Joel Stanley <joel@jms.id.au>

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


Hi; Coverity complains about this function (CID 1402782):

> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

> +{

> +    struct tm tm;

> +    uint32_t year, cent;

> +    uint32_t reg1 = rtc->reg[COUNTER1];

> +    uint32_t reg2 = rtc->reg[COUNTER2];

> +

> +    tm.tm_mday = (reg1 >> 24) & 0x1f;

> +    tm.tm_hour = (reg1 >> 16) & 0x1f;

> +    tm.tm_min = (reg1 >> 8) & 0x3f;

> +    tm.tm_sec = (reg1 >> 0) & 0x3f;

> +

> +    cent = (reg2 >> 16) & 0x1f;

> +    year = (reg2 >> 8) & 0x7f;

> +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

> +    tm.tm_year = year + (cent * 100) - 1900;

> +

> +    rtc->offset = qemu_timedate_diff(&tm);

> +}


because the tm_wday field of 'struct tm tm' is not initialized
before we call qemu_timedate_diff(). This is a false
positive because the "read" of this field is just the place
in qemu_timedate_diff() that does "struct tm tmp = *tm;"
before calling mktime(), and mktime() ignores tm_wday.
We could make Coverity happy by using a struct initializer
on 'tm' here; on the other hand we don't do that anywhere else
which calls qemu_timedate_diff(), so maybe I should just mark
this a false positive?

thanks
-- PMM
Joel Stanley July 4, 2019, 7:49 a.m. UTC | #2
On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:

> >

> > From: Joel Stanley <joel@jms.id.au>

> >

> > The RTC is modeled to provide time and date functionality. It is

> > initialised at zero to match the hardware.

> >

> > There is no modelling of the alarm functionality, which includes the IRQ

> > line. As there is no guest code to exercise this function that is

> > acceptable for now.

> >

> > Signed-off-by: Joel Stanley <joel@jms.id.au>

> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

>

> Hi; Coverity complains about this function (CID 1402782):

>

> > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

> > +{

> > +    struct tm tm;

> > +    uint32_t year, cent;

> > +    uint32_t reg1 = rtc->reg[COUNTER1];

> > +    uint32_t reg2 = rtc->reg[COUNTER2];

> > +

> > +    tm.tm_mday = (reg1 >> 24) & 0x1f;

> > +    tm.tm_hour = (reg1 >> 16) & 0x1f;

> > +    tm.tm_min = (reg1 >> 8) & 0x3f;

> > +    tm.tm_sec = (reg1 >> 0) & 0x3f;

> > +

> > +    cent = (reg2 >> 16) & 0x1f;

> > +    year = (reg2 >> 8) & 0x7f;

> > +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

> > +    tm.tm_year = year + (cent * 100) - 1900;

> > +

> > +    rtc->offset = qemu_timedate_diff(&tm);

> > +}

>

> because the tm_wday field of 'struct tm tm' is not initialized

> before we call qemu_timedate_diff(). This is a false

> positive because the "read" of this field is just the place

> in qemu_timedate_diff() that does "struct tm tmp = *tm;"

> before calling mktime(), and mktime() ignores tm_wday.

> We could make Coverity happy by using a struct initializer

> on 'tm' here; on the other hand we don't do that anywhere else

> which calls qemu_timedate_diff(), so maybe I should just mark

> this a false positive?


I don't have an opinion on which option to take. Perhaps mark it as a
false positive?

Cheers,

Joel
Peter Maydell July 4, 2019, 1:08 p.m. UTC | #3
On Thu, 4 Jul 2019 at 08:49, Joel Stanley <joel@jms.id.au> wrote:
>

> On Tue, 2 Jul 2019 at 19:19, Peter Maydell <peter.maydell@linaro.org> wrote:

> >

> > On Tue, 18 Jun 2019 at 17:53, Cédric Le Goater <clg@kaod.org> wrote:

> > >

> > > From: Joel Stanley <joel@jms.id.au>

> > >

> > > The RTC is modeled to provide time and date functionality. It is

> > > initialised at zero to match the hardware.

> > >

> > > There is no modelling of the alarm functionality, which includes the IRQ

> > > line. As there is no guest code to exercise this function that is

> > > acceptable for now.

> > >

> > > Signed-off-by: Joel Stanley <joel@jms.id.au>

> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> >

> > Hi; Coverity complains about this function (CID 1402782):

> >

> > > +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

> > > +{

> > > +    struct tm tm;

> > > +    uint32_t year, cent;

> > > +    uint32_t reg1 = rtc->reg[COUNTER1];

> > > +    uint32_t reg2 = rtc->reg[COUNTER2];

> > > +

> > > +    tm.tm_mday = (reg1 >> 24) & 0x1f;

> > > +    tm.tm_hour = (reg1 >> 16) & 0x1f;

> > > +    tm.tm_min = (reg1 >> 8) & 0x3f;

> > > +    tm.tm_sec = (reg1 >> 0) & 0x3f;

> > > +

> > > +    cent = (reg2 >> 16) & 0x1f;

> > > +    year = (reg2 >> 8) & 0x7f;

> > > +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

> > > +    tm.tm_year = year + (cent * 100) - 1900;

> > > +

> > > +    rtc->offset = qemu_timedate_diff(&tm);

> > > +}

> >

> > because the tm_wday field of 'struct tm tm' is not initialized

> > before we call qemu_timedate_diff(). This is a false

> > positive because the "read" of this field is just the place

> > in qemu_timedate_diff() that does "struct tm tmp = *tm;"

> > before calling mktime(), and mktime() ignores tm_wday.

> > We could make Coverity happy by using a struct initializer

> > on 'tm' here; on the other hand we don't do that anywhere else

> > which calls qemu_timedate_diff(), so maybe I should just mark

> > this a false positive?

>

> I don't have an opinion on which option to take. Perhaps mark it as a

> false positive?


Yeah, seems reasonable.

-- PMM
diff mbox series

Patch

diff --git a/include/hw/timer/aspeed_rtc.h b/include/hw/timer/aspeed_rtc.h
new file mode 100644
index 000000000000..1f1155a676c1
--- /dev/null
+++ b/include/hw/timer/aspeed_rtc.h
@@ -0,0 +1,31 @@ 
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef ASPEED_RTC_H
+#define ASPEED_RTC_H
+
+#include <stdint.h>
+
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+typedef struct AspeedRtcState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t reg[0x18];
+    int offset;
+
+} AspeedRtcState;
+
+#define TYPE_ASPEED_RTC "aspeed.rtc"
+#define ASPEED_RTC(obj) OBJECT_CHECK(AspeedRtcState, (obj), TYPE_ASPEED_RTC)
+
+#endif /* ASPEED_RTC_H */
diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c
new file mode 100644
index 000000000000..19f061c846e8
--- /dev/null
+++ b/hw/timer/aspeed_rtc.c
@@ -0,0 +1,180 @@ 
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/timer/aspeed_rtc.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+
+#include "trace.h"
+
+#define COUNTER1        (0x00 / 4)
+#define COUNTER2        (0x04 / 4)
+#define ALARM           (0x08 / 4)
+#define CONTROL         (0x10 / 4)
+#define ALARM_STATUS    (0x14 / 4)
+
+#define RTC_UNLOCKED    BIT(1)
+#define RTC_ENABLED     BIT(0)
+
+static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
+{
+    struct tm tm;
+    uint32_t year, cent;
+    uint32_t reg1 = rtc->reg[COUNTER1];
+    uint32_t reg2 = rtc->reg[COUNTER2];
+
+    tm.tm_mday = (reg1 >> 24) & 0x1f;
+    tm.tm_hour = (reg1 >> 16) & 0x1f;
+    tm.tm_min = (reg1 >> 8) & 0x3f;
+    tm.tm_sec = (reg1 >> 0) & 0x3f;
+
+    cent = (reg2 >> 16) & 0x1f;
+    year = (reg2 >> 8) & 0x7f;
+    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
+    tm.tm_year = year + (cent * 100) - 1900;
+
+    rtc->offset = qemu_timedate_diff(&tm);
+}
+
+static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)
+{
+    uint32_t year, cent;
+    struct tm now;
+
+    qemu_get_timedate(&now, rtc->offset);
+
+    switch (r) {
+    case COUNTER1:
+        return (now.tm_mday << 24) | (now.tm_hour << 16) |
+            (now.tm_min << 8) | now.tm_sec;
+    case COUNTER2:
+        cent = (now.tm_year + 1900) / 100;
+        year = now.tm_year % 100;
+        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |
+            ((now.tm_mon + 1) & 0xf);
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,
+                                unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint64_t val;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (rtc->reg[CONTROL] & RTC_ENABLED) {
+            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);
+        }
+        /* fall through */
+    case CONTROL:
+        val = rtc->reg[r];
+        break;
+    case ALARM:
+    case ALARM_STATUS:
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        return 0;
+    }
+
+    trace_aspeed_rtc_read(addr, val);
+
+    return val;
+}
+
+static void aspeed_rtc_write(void *opaque, hwaddr addr,
+                             uint64_t val, unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (!(rtc->reg[CONTROL] & RTC_UNLOCKED)) {
+            break;
+        }
+        /* fall through */
+    case CONTROL:
+        rtc->reg[r] = val;
+        aspeed_rtc_calc_offset(rtc);
+        break;
+    case ALARM:
+    case ALARM_STATUS:
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        break;
+    }
+    trace_aspeed_rtc_write(addr, val);
+}
+
+static void aspeed_rtc_reset(DeviceState *d)
+{
+    AspeedRtcState *rtc = ASPEED_RTC(d);
+
+    rtc->offset = 0;
+    memset(rtc->reg, 0, sizeof(rtc->reg));
+}
+
+static const MemoryRegionOps aspeed_rtc_ops = {
+    .read = aspeed_rtc_read,
+    .write = aspeed_rtc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription vmstate_aspeed_rtc = {
+    .name = TYPE_ASPEED_RTC,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, AspeedRtcState, 0x18),
+        VMSTATE_INT32(offset, AspeedRtcState),
+        VMSTATE_INT32(offset, AspeedRtcState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void aspeed_rtc_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedRtcState *s = ASPEED_RTC(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_rtc_ops, s,
+                          "aspeed-rtc", 0x18ULL);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_rtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_rtc_realize;
+    dc->vmsd = &vmstate_aspeed_rtc;
+    dc->reset = aspeed_rtc_reset;
+}
+
+static const TypeInfo aspeed_rtc_info = {
+    .name          = TYPE_ASPEED_RTC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedRtcState),
+    .class_init    = aspeed_rtc_class_init,
+};
+
+static void aspeed_rtc_register_types(void)
+{
+    type_register_static(&aspeed_rtc_info);
+}
+
+type_init(aspeed_rtc_register_types)
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 0e9a4530f848..123d92c9692c 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -41,7 +41,7 @@  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
 obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
 
 common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
-common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o
 
 common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o
 common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index dcaf3d6da6c8..db02a9142cda 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -66,6 +66,10 @@  cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK A
 cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 
+# hw/timer/aspeed-rtc.c
+aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+
 # sun4v-rtc.c
 sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 " value 0x%" PRIx64
 sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 " value 0x%" PRIx64