Thanks for the review Peter and Cedric. On Thu, 11 Apr 2019 at 15:30, Cédric Le Goater <c...@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