Re: [Qemu-arm] [PATCH 1/2] hw: timer: Add ASPEED RTC device

[Joel Stanley]

Thanks for the review Peter and Cedric.

On Thu, 11 Apr 2019 at 15:30, Cédric Le Goater <address@hidden> 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