Hi Peter, On 4/11/19 5:30 PM, Cédric Le Goater wrote: > On 4/11/19 5:25 PM, Peter Maydell wrote: >> On Thu, 28 Mar 2019 at 06:22, Joel Stanley <j...@jms.id.au> wrote: >>> >>> Signed-off-by: Joel Stanley <j...@jms.id.au> >>> Signed-off-by: Cédric Le Goater <c...@kaod.org> >>> --- >>> hw/timer/Makefile.objs | 2 +- >>> hw/timer/aspeed_rtc.c | 157 ++++++++++++++++++++++++++++++++++ >>> hw/timer/trace-events | 4 + >>> include/hw/timer/aspeed_rtc.h | 31 +++++++ >>> 4 files changed, 193 insertions(+), 1 deletion(-) >>> create mode 100644 hw/timer/aspeed_rtc.c >>> create mode 100644 include/hw/timer/aspeed_rtc.h >>> >>> 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/aspeed_rtc.c b/hw/timer/aspeed_rtc.c >>> new file mode 100644 >>> index 000000000000..daccf00eccdc >>> --- /dev/null >>> +++ b/hw/timer/aspeed_rtc.c >>> @@ -0,0 +1,157 @@ >>> +/* >>> + * ASPEED Real Time Clock >>> + * Joel Stanley <j...@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) >> >> Not mandatory, but you might like the REG32() macro in >> hw/registerfields.h which defines A_FOO and R_FOO >> constants for you for the addresses and the indexes. > > Yes. May be we should start using these macros in all our models. > >> >>> + >>> +#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; >> >> Shift by zero ? >> >> Consider using extract32() rather than by-hand shift and mask. > > What about the FIELD*() macros in hw/registerfields.h ? > > Thanks, > > C. > >> >>> + >>> + 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: >>> + abort(); >> >> More usually written 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); >>> + } >> >> If this is deliberately going to fall through to the next >> case then it should have a /* fall through */ comment >> (ditto in the write fn). >> >>> + case ALARM: >>> + case CONTROL: >>> + case ALARM_STATUS: >>> + val = rtc->reg[r]; >>> + break; >>> + 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_class_init(ObjectClass *klass, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + dc->realize = aspeed_rtc_realize; >> >> This is missing a reset function and vmstate.
Is vmstate mandatory for new devices? >> >> thanks >> -- PMM >> > >