* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-11 15:25 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 15:25 UTC (permalink / raw) To: Joel Stanley Cc: Cédric Le Goater, Andrew Jeffery, qemu-arm, QEMU Developers On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: > > Signed-off-by: Joel Stanley <joel@jms.id.au> > Signed-off-by: Cédric Le Goater <clg@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 <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) 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. > + > +#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. > + > + 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. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-11 15:25 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 15:25 UTC (permalink / raw) To: Joel Stanley Cc: Andrew Jeffery, qemu-arm, Cédric Le Goater, QEMU Developers On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: > > Signed-off-by: Joel Stanley <joel@jms.id.au> > Signed-off-by: Cédric Le Goater <clg@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 <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) 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. > + > +#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. > + > + 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. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device 2019-04-11 15:25 ` Peter Maydell (?) @ 2019-04-11 15:30 ` Cédric Le Goater 2019-04-11 16:36 ` Philippe Mathieu-Daudé 2019-04-12 3:17 ` Joel Stanley -1 siblings, 2 replies; 14+ messages in thread From: Cédric Le Goater @ 2019-04-11 15:30 UTC (permalink / raw) To: Peter Maydell, Joel Stanley; +Cc: Andrew Jeffery, qemu-arm, QEMU Developers On 4/11/19 5:25 PM, Peter Maydell wrote: > On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> Signed-off-by: Cédric Le Goater <clg@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 <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) > > 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. > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device 2019-04-11 15:30 ` Cédric Le Goater @ 2019-04-11 16:36 ` Philippe Mathieu-Daudé 2019-04-11 16:55 ` Peter Maydell 2019-04-12 3:17 ` Joel Stanley 1 sibling, 1 reply; 14+ messages in thread From: Philippe Mathieu-Daudé @ 2019-04-11 16:36 UTC (permalink / raw) To: Cédric Le Goater, Peter Maydell, Joel Stanley Cc: Andrew Jeffery, qemu-arm, QEMU Developers 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 <joel@jms.id.au> wrote: >>> >>> Signed-off-by: Joel Stanley <joel@jms.id.au> >>> Signed-off-by: Cédric Le Goater <clg@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 <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) >> >> 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 >> > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-11 16:55 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 16:55 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Cédric Le Goater, Joel Stanley, Andrew Jeffery, qemu-arm, QEMU Developers On Thu, 11 Apr 2019 at 17:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Peter, > > On 4/11/19 5:30 PM, Cédric Le Goater wrote: > > On 4/11/19 5:25 PM, Peter Maydell wrote: > >> This is missing a reset function and vmstate. > > Is vmstate mandatory for new devices. Yes, I think so. You can have vmstate that says "this can't be migrated" if you absolutely must, but it's generally easy enough to make it properly migratable. If the device generally has no state to migrate it should at least provide a comment saying so. (I think we should add a way to say this in a vmstate, so that we can then assert that every device sets its vmsd pointer to something.) Failing to provide a vmstate means you have a machine which will silently break if the user tries to do savevm/loadvm, which is not very nice. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-11 16:55 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 16:55 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Andrew Jeffery, QEMU Developers, qemu-arm, Cédric Le Goater, Joel Stanley On Thu, 11 Apr 2019 at 17:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Hi Peter, > > On 4/11/19 5:30 PM, Cédric Le Goater wrote: > > On 4/11/19 5:25 PM, Peter Maydell wrote: > >> This is missing a reset function and vmstate. > > Is vmstate mandatory for new devices. Yes, I think so. You can have vmstate that says "this can't be migrated" if you absolutely must, but it's generally easy enough to make it properly migratable. If the device generally has no state to migrate it should at least provide a comment saying so. (I think we should add a way to say this in a vmstate, so that we can then assert that every device sets its vmsd pointer to something.) Failing to provide a vmstate means you have a machine which will silently break if the user tries to do savevm/loadvm, which is not very nice. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-12 3:17 ` Joel Stanley 0 siblings, 0 replies; 14+ messages in thread From: Joel Stanley @ 2019-04-12 3:17 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Maydell, Andrew Jeffery, qemu-arm, QEMU Developers Thanks for the review Peter and Cedric. On Thu, 11 Apr 2019 at 15:30, Cédric Le Goater <clg@kaod.org> wrote: > > On 4/11/19 5:25 PM, Peter Maydell wrote: > >> + > >> +#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. I don't like them as you can no longer jump between the definition and the use of the defines. > > > > >> + > >> +#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. I looked at these and found them more confusing than writing what was actually happening. This code came from the Linux kernel driver, which I wrote, so I know it's correct. The shift by zero is there to follow the pattern of the code proceeding it, again to stop mistakes. If we require using these helper macros then I can make the change. If it's optional then I would prefer to leave it as is. > > What about the FIELD*() macros in hw/registerfields.h ? > > >> +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. Ok, I can add those. vmstate and migration is a foreign concept for the small ARM machines. As well as being a developer, I am an end user of Qemu for them (the aspeeds, microbit), and the use case is to boot up a firmware and check that it does correct thing. I've never considered saving the state and resming it, and can't think of a situation where that would be done. If we're adding it for consistency then I understand. I don't think it sees any testing though. Cheers, Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-12 3:17 ` Joel Stanley 0 siblings, 0 replies; 14+ messages in thread From: Joel Stanley @ 2019-04-12 3:17 UTC (permalink / raw) To: Cédric Le Goater Cc: Andrew Jeffery, Peter Maydell, qemu-arm, QEMU Developers Thanks for the review Peter and Cedric. On Thu, 11 Apr 2019 at 15:30, Cédric Le Goater <clg@kaod.org> wrote: > > On 4/11/19 5:25 PM, Peter Maydell wrote: > >> + > >> +#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. I don't like them as you can no longer jump between the definition and the use of the defines. > > > > >> + > >> +#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. I looked at these and found them more confusing than writing what was actually happening. This code came from the Linux kernel driver, which I wrote, so I know it's correct. The shift by zero is there to follow the pattern of the code proceeding it, again to stop mistakes. If we require using these helper macros then I can make the change. If it's optional then I would prefer to leave it as is. > > What about the FIELD*() macros in hw/registerfields.h ? > > >> +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. Ok, I can add those. vmstate and migration is a foreign concept for the small ARM machines. As well as being a developer, I am an end user of Qemu for them (the aspeeds, microbit), and the use case is to boot up a firmware and check that it does correct thing. I've never considered saving the state and resming it, and can't think of a situation where that would be done. If we're adding it for consistency then I understand. I don't think it sees any testing though. Cheers, Joel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-12 11:55 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-12 11:55 UTC (permalink / raw) To: Joel Stanley Cc: Cédric Le Goater, Peter Maydell, Andrew Jeffery, qemu-arm, QEMU Developers On Fri, 12 Apr 2019 at 04:17, Joel Stanley <joel@jms.id.au> wrote: > vmstate and migration is a foreign concept for the small ARM machines. > As well as being a developer, I am an end user of Qemu for them (the > aspeeds, microbit), and the use case is to boot up a firmware and > check that it does correct thing. I've never considered saving the > state and resming it, and can't think of a situation where that would > be done. It's a really useful debugging tool: https://translatedcode.wordpress.com/2015/07/06/tricks-for-debugging-qemu-savevm-snapshots/ thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device @ 2019-04-12 11:55 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-12 11:55 UTC (permalink / raw) To: Joel Stanley Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Cédric Le Goater, QEMU Developers On Fri, 12 Apr 2019 at 04:17, Joel Stanley <joel@jms.id.au> wrote: > vmstate and migration is a foreign concept for the small ARM machines. > As well as being a developer, I am an end user of Qemu for them (the > aspeeds, microbit), and the use case is to boot up a firmware and > check that it does correct thing. I've never considered saving the > state and resming it, and can't think of a situation where that would > be done. It's a really useful debugging tool: https://translatedcode.wordpress.com/2015/07/06/tricks-for-debugging-qemu-savevm-snapshots/ thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device 2019-04-12 11:55 ` Peter Maydell (?) @ 2019-04-15 6:13 ` Cédric Le Goater -1 siblings, 0 replies; 14+ messages in thread From: Cédric Le Goater @ 2019-04-15 6:13 UTC (permalink / raw) To: Peter Maydell, Joel Stanley; +Cc: Andrew Jeffery, qemu-arm, QEMU Developers On 4/12/19 1:55 PM, Peter Maydell wrote: > On Fri, 12 Apr 2019 at 04:17, Joel Stanley <joel@jms.id.au> wrote: >> vmstate and migration is a foreign concept for the small ARM machines. >> As well as being a developer, I am an end user of Qemu for them (the >> aspeeds, microbit), and the use case is to boot up a firmware and >> check that it does correct thing. I've never considered saving the >> state and resming it, and can't think of a situation where that would >> be done. > > It's a really useful debugging tool: > https://translatedcode.wordpress.com/2015/07/06/tricks-for-debugging-qemu-savevm-snapshots/ yes. Our Aspeed machines migrate quite well today and, who knows, we might want one day to run Aspeed VMs on the "cloud". I doubt it but, anyway, it's really not complex to do either. Thanks, C. ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20190328062222.8409-3-joel@jms.id.au>]
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/aspeed: Add RTC to SoC @ 2019-04-11 15:28 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 15:28 UTC (permalink / raw) To: Joel Stanley Cc: Cédric Le Goater, Andrew Jeffery, qemu-arm, QEMU Developers On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: > > Signed-off-by: Joel Stanley <joel@jms.id.au> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/arm/aspeed_soc.c | 15 +++++++++++++++ > include/hw/arm/aspeed_soc.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index a27233d4876b..628ec633c91a 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -32,6 +32,7 @@ > #define ASPEED_SOC_SDMC_BASE 0x1E6E0000 > #define ASPEED_SOC_SCU_BASE 0x1E6E2000 > #define ASPEED_SOC_SRAM_BASE 0x1E720000 > +#define ASPEED_SOC_RTC_BASE 0x1E781000 > #define ASPEED_SOC_TIMER_BASE 0x1E782000 > #define ASPEED_SOC_WDT_BASE 0x1E785000 > #define ASPEED_SOC_I2C_BASE 0x1E78A000 > @@ -135,6 +136,10 @@ static void aspeed_soc_init(Object *obj) > object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL); > qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default()); > > + object_initialize(&s->rtc, sizeof(s->rtc), TYPE_ASPEED_RTC); > + object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL); > + qdev_set_parent_bus(DEVICE(&s->rtc), sysbus_get_default()); These three lines should be written sysbus_init_child_obj(obj, "rtc", &s->rtc, sizeof(s->rtc), TYPE_ASPEED_RTC); which is both shorter and also avoids leaking a reference to the child object. (We should also at some point fix the existing uses of this pattern in this file.) thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/aspeed: Add RTC to SoC @ 2019-04-11 15:28 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-04-11 15:28 UTC (permalink / raw) To: Joel Stanley Cc: Andrew Jeffery, qemu-arm, Cédric Le Goater, QEMU Developers On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: > > Signed-off-by: Joel Stanley <joel@jms.id.au> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/arm/aspeed_soc.c | 15 +++++++++++++++ > include/hw/arm/aspeed_soc.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index a27233d4876b..628ec633c91a 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -32,6 +32,7 @@ > #define ASPEED_SOC_SDMC_BASE 0x1E6E0000 > #define ASPEED_SOC_SCU_BASE 0x1E6E2000 > #define ASPEED_SOC_SRAM_BASE 0x1E720000 > +#define ASPEED_SOC_RTC_BASE 0x1E781000 > #define ASPEED_SOC_TIMER_BASE 0x1E782000 > #define ASPEED_SOC_WDT_BASE 0x1E785000 > #define ASPEED_SOC_I2C_BASE 0x1E78A000 > @@ -135,6 +136,10 @@ static void aspeed_soc_init(Object *obj) > object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL); > qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default()); > > + object_initialize(&s->rtc, sizeof(s->rtc), TYPE_ASPEED_RTC); > + object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL); > + qdev_set_parent_bus(DEVICE(&s->rtc), sysbus_get_default()); These three lines should be written sysbus_init_child_obj(obj, "rtc", &s->rtc, sizeof(s->rtc), TYPE_ASPEED_RTC); which is both shorter and also avoids leaking a reference to the child object. (We should also at some point fix the existing uses of this pattern in this file.) thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/aspeed: Add RTC to SoC 2019-04-11 15:28 ` Peter Maydell (?) @ 2019-04-11 15:31 ` Cédric Le Goater -1 siblings, 0 replies; 14+ messages in thread From: Cédric Le Goater @ 2019-04-11 15:31 UTC (permalink / raw) To: Peter Maydell, Joel Stanley; +Cc: Andrew Jeffery, qemu-arm, QEMU Developers On 4/11/19 5:28 PM, Peter Maydell wrote: > On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote: >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/arm/aspeed_soc.c | 15 +++++++++++++++ >> include/hw/arm/aspeed_soc.h | 2 ++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >> index a27233d4876b..628ec633c91a 100644 >> --- a/hw/arm/aspeed_soc.c >> +++ b/hw/arm/aspeed_soc.c >> @@ -32,6 +32,7 @@ >> #define ASPEED_SOC_SDMC_BASE 0x1E6E0000 >> #define ASPEED_SOC_SCU_BASE 0x1E6E2000 >> #define ASPEED_SOC_SRAM_BASE 0x1E720000 >> +#define ASPEED_SOC_RTC_BASE 0x1E781000 >> #define ASPEED_SOC_TIMER_BASE 0x1E782000 >> #define ASPEED_SOC_WDT_BASE 0x1E785000 >> #define ASPEED_SOC_I2C_BASE 0x1E78A000 >> @@ -135,6 +136,10 @@ static void aspeed_soc_init(Object *obj) >> object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL); >> qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default()); >> >> + object_initialize(&s->rtc, sizeof(s->rtc), TYPE_ASPEED_RTC); >> + object_property_add_child(obj, "rtc", OBJECT(&s->rtc), NULL); >> + qdev_set_parent_bus(DEVICE(&s->rtc), sysbus_get_default()); > > These three lines should be written > sysbus_init_child_obj(obj, "rtc", &s->rtc, sizeof(s->rtc), > TYPE_ASPEED_RTC); > > which is both shorter and also avoids leaking a reference to > the child object. > > (We should also at some point fix the existing uses of this > pattern in this file.) yes. and there are a few. I will do that. Thanks, C. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-04-15 6:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190328062222.8409-1-joel@jms.id.au> [not found] ` <20190328062222.8409-2-joel@jms.id.au> 2019-04-11 15:25 ` [Qemu-devel] [PATCH 1/2] hw: timer: Add ASPEED RTC device Peter Maydell 2019-04-11 15:25 ` Peter Maydell 2019-04-11 15:30 ` Cédric Le Goater 2019-04-11 16:36 ` Philippe Mathieu-Daudé 2019-04-11 16:55 ` Peter Maydell 2019-04-11 16:55 ` Peter Maydell 2019-04-12 3:17 ` Joel Stanley 2019-04-12 3:17 ` Joel Stanley 2019-04-12 11:55 ` Peter Maydell 2019-04-12 11:55 ` Peter Maydell 2019-04-15 6:13 ` Cédric Le Goater [not found] ` <20190328062222.8409-3-joel@jms.id.au> 2019-04-11 15:28 ` [Qemu-devel] [PATCH 2/2] hw/arm/aspeed: Add RTC to SoC Peter Maydell 2019-04-11 15:28 ` Peter Maydell 2019-04-11 15:31 ` 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.