Re: [PATCH v3 12/17] hw/arm/allwinner: add RTC device support

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Niek,

On 1/14/20 11:57 PM, Niek Linnenbank wrote:
> On Tue, Jan 14, 2020 at 11:52 PM Niek Linnenbank 
> <nieklinnenbank@gmail.com <mailto:nieklinnenbank@gmail.com>> wrote:
> 
>     Hi Philippe,
> 
>     On Mon, Jan 13, 2020 at 11:57 PM Philippe Mathieu-Daudé
>     <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>         On 1/8/20 9:00 PM, Niek Linnenbank wrote:
>          > Allwinner System-on-Chips usually contain a Real Time Clock (RTC)
>          > for non-volatile system date and time keeping. This commit
>         adds a generic
>          > Allwinner RTC device that supports the RTC devices found in
>         Allwinner SoC
>          > family sun4i (A10), sun7i (A20) and sun6i and newer (A31,
>         H2+, H3, etc).
[...]
>          > +static uint64_t allwinner_rtc_read(void *opaque, hwaddr offset,
>          > +                                   unsigned size)
>          > +{
>          > +    AwRtcState *s = AW_RTC(opaque);
>          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(s);
>          > +    uint64_t val = 0;
>          > +
>          > +    if (offset >= AW_RTC_REGS_MAXADDR) {
>          > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds
>         offset 0x%04x\n",
>          > +                      __func__, (uint32_t)offset);
>          > +        return 0;
>          > +    }
>          > +
>          > +    if (!c->regmap[offset]) {
>          > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid
>         register 0x%04x\n",
>          > +                          __func__, (uint32_t)offset);
>          > +        return 0;
>          > +    }
>          > +
>          > +    switch (c->regmap[offset]) {
>          > +    case REG_LOSC:       /* Low Oscillator Control */
>          > +        val = s->regs[REG_LOSC];
>          > +        s->regs[REG_LOSC] &= ~(REG_LOSC_YMD | REG_LOSC_HMS);
>          > +        break;
>          > +    case REG_YYMMDD:     /* RTC Year-Month-Day */
>          > +    case REG_HHMMSS:     /* RTC Hour-Minute-Second */
>          > +    case REG_GP0:        /* General Purpose Register 0 */
>          > +    case REG_GP1:        /* General Purpose Register 1 */
>          > +    case REG_GP2:        /* General Purpose Register 2 */
>          > +    case REG_GP3:        /* General Purpose Register 3 */
>          > +        val = s->regs[c->regmap[offset]];
>          > +        break;
>          > +    default:
>          > +        if (!c->read(s, offset)) {
>          > +            qemu_log_mask(LOG_UNIMP, "%s: unimplemented
>         register 0x%04x\n",
>          > +                          __func__, (uint32_t)offset);
>          > +        }
>          > +        val = s->regs[c->regmap[offset]];
>          > +        break;
>          > +    }
>          > +
>          > +    trace_allwinner_rtc_read(offset, val);
>          > +    return val;
>          > +}
>          > +
>          > +static void allwinner_rtc_write(void *opaque, hwaddr offset,
>          > +                                uint64_t val, unsigned size)
>          > +{
>          > +    AwRtcState *s = AW_RTC(opaque);
>          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(s);
>          > +
>          > +    if (offset >= AW_RTC_REGS_MAXADDR) {
>          > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds
>         offset 0x%04x\n",
>          > +                      __func__, (uint32_t)offset);
>          > +        return;
>          > +    }
>          > +
>          > +    if (!c->regmap[offset]) {
>          > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid
>         register 0x%04x\n",
>          > +                          __func__, (uint32_t)offset);
>          > +        return;
>          > +    }
>          > +
>          > +    trace_allwinner_rtc_write(offset, val);
>          > +
>          > +    switch (c->regmap[offset]) {
>          > +    case REG_YYMMDD:     /* RTC Year-Month-Day */
>          > +        s->regs[REG_YYMMDD] = val;
>          > +        s->regs[REG_LOSC]  |= REG_LOSC_YMD;
>          > +        break;
>          > +    case REG_HHMMSS:     /* RTC Hour-Minute-Second */
>          > +        s->regs[REG_HHMMSS] = val;
>          > +        s->regs[REG_LOSC]  |= REG_LOSC_HMS;
>          > +        break;
>          > +    case REG_GP0:        /* General Purpose Register 0 */
>          > +    case REG_GP1:        /* General Purpose Register 1 */
>          > +    case REG_GP2:        /* General Purpose Register 2 */
>          > +    case REG_GP3:        /* General Purpose Register 3 */
>          > +        s->regs[c->regmap[offset]] = val;
>          > +        break;
>          > +    default:
>          > +        if (!c->write(s, offset, val)) {
>          > +            qemu_log_mask(LOG_UNIMP, "%s: unimplemented
>         register 0x%04x\n",
>          > +                          __func__, (uint32_t)offset);
>          > +        }
>          > +        break;
>          > +    }
>          > +}
>          > +
>          > +static const MemoryRegionOps allwinner_rtc_ops = {
>          > +    .read = allwinner_rtc_read,
>          > +    .write = allwinner_rtc_write,
>          > +    .endianness = DEVICE_NATIVE_ENDIAN,
>          > +    .valid = {
>          > +        .min_access_size = 4,
>          > +        .max_access_size = 4,
>          > +    },
>          > +    .impl.min_access_size = 4,
>          > +};
>          > +
>          > +static void allwinner_rtc_reset(DeviceState *dev)
>          > +{
>          > +    AwRtcState *s = AW_RTC(dev);
>          > +    const AwRtcClass *c = AW_RTC_GET_CLASS(dev);
>          > +    struct tm now;
>          > +
>          > +    /* Clear registers */
>          > +    memset(s->regs, 0, sizeof(s->regs));
>          > +
>          > +    /* Get current datetime */
>          > +    qemu_get_timedate(&now, 0);
>          > +
>          > +    /* Set RTC with current datetime */
>          > +    s->regs[REG_YYMMDD] =  ((now.tm_year - c->year_offset)
>         << 16) |
>          > +                           ((now.tm_mon + 1) << 8) |
>          > +                             now.tm_mday;
>          > +    s->regs[REG_HHMMSS] = (((now.tm_wday + 6) % 7) << 29) |
>          > +                              (now.tm_hour << 16) |
>          > +                              (now.tm_min << 8) |
>          > +                               now.tm_sec;
> 
>         This doesn't look correct.
> 
>           From H3 Datasheet (Rev1.2):
>             4.8.3.4. RTC YY-MM-DD Register (Default Value: 0x00000000)
>             4.8.3.5. RTC HH-MM-SS Register (Default Value: 0x00000000)
> 
> 
>     I don't yet fully understand what you mean. Could you please explain
>     a bit more what should be changed?

I was thinking about:

- Start a VM on Monday at 12:34

- Pause the VM on Tuesday at 12:34
   rtc_time=Tuesday,12:34

- [Eventually savevm/migrate/loadvm]
   rtc_time=Tuesday,12:34

- Resume the VM on Wednesday 12:34
   (time should be Tuesday at 12:34)
   so rtc_time=Tuesday,12:34

- Check time on Thursday at 12:33
   (time should be Wednesday at 12:33)
   rtc_time=Wednesday,12:34

- Reset the VM on Thursday at 12:34
   (time was Wednesday at 12:34)

- Check time on Thursday at 12:35
   (time should be Wednesday at 12:35)

However due to reset() calling qemu_get_timedate(), the rtc_time will be 
Thursday at 12:35 instead of Wednesday.

I don't know how the hardware keep these 2*32-bit safe when powered off.

Laurent Vivier posted a patch where he uses a block backend for his 
machine NVRAM (used by RTC). This seems to me the clever way to do this:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg666837.html

I'd use a block of 8 bytes for your RTC.
If we start a machine without rtc block backend, the 2*32-bit are 
initialized as zero as the datasheet. If we provide a block, the machine 
will power-on from that stored time.

You might want to look at the global -rtc command line option:

   -rtc
   [base=utc|localtime|datetime][,clock=host|rt|vm][,driftfix=none|slew]
         Specify base as "utc" or "localtime" to let the RTC start
         at the current UTC or local time, respectively. "localtime"
         is required for correct date in MS-DOS or Windows. To start
         at a specific point in time, provide datetime in the format
         "2006-06-17T16:01:21" or "2006-06-17". The default base is
         UTC.

But it might be specific to the MC146818 RTC.

>     For filling the YYMMDD and HHMMSS register fields, I simply looked
>     at the 4.8.3.4 and 4.8.3.5 sections
>     and filled it with the time retrieved from qemu_get_timedate. The
>     shifts are done so the values are set in the proper bits.
>     If I read it with the hwclock -r command under Linux, this reads out OK.
>     On NetBSD, this works as well, except for the base year mismatch
>     which I explained above.
> 
> 
>         I'm not sure what is the proper to model this, maybe set this
>         value in
>         init()? If we suspend a machine, migrate it, and resume it, what
>         RTC are
>         we expecting?
> 
> I forgot to reply on this one.
> 
> I have not used migration very often, but I did manage to test it a 
> couple of times
> using the 'migrate' command on the Qemu monitor console before I 
> submitted each
> new version of the patch series. My expectation would be that the RTC 
> time is suspended
> along with all the other state of the machine such as memory, I/O 
> devices etc. So that would mean
> the time is 'frozen' until resumed. I think that is what we currently 
> have here.
> 
> Do you think that is correct or should it work differently?

Yes, this is the behavior I'd expect. Maybe other would disagree.