Re: [Qemu-ppc] [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > of day is implemented. Setting time and RTC alarm are not supported.
[...]
> > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > new file mode 100644
> > index 0000000..9dbdb1b
> > --- /dev/null
> > +++ b/hw/timer/m41t80.c
> > @@ -0,0 +1,117 @@
> > +/*
> > + * M41T80 serial rtc emulation
> > + *
> > + * Copyright (c) 2018 BALATON Zoltan
> > + *
> > + * This work is licensed under the GNU GPL license version 2 or later.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "qemu/timer.h"
> > +#include "qemu/bcd.h"
> > +#include "hw/i2c/i2c.h"
> > +
> > +#define TYPE_M41T80 "m41t80"
> > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > +
> > +typedef struct M41t80State {
> > +    I2CSlave parent_obj;
> > +    int8_t addr;
> > +} M41t80State;
> > +
> > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > +{
> > +    M41t80State *s = M41T80(dev);
> > +
> > +    s->addr = -1;
> > +}
> > +
> > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > +{
> > +    M41t80State *s = M41T80(i2c);
> > +
> > +    if (s->addr < 0) {
> > +        s->addr = data;
> > +    } else {
> > +        s->addr++;
> > +    }
>
> What about adding enum i2c_event in M41t80State and use the enum here
> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> expected?

Thanks for the review. I guess we could add enum for device bytes and the 
special case -1 meaning no register address selected yet but this is a 
very simple device with only 20 bytes and the datasheet also lists them by 
number without naming them so I think we can also refer to them by number. 
Since the device has only this 20 bytes the case with 127 should also not 
be a problem as that's invalid address anyway. Or did you mean something 
else?

> > +    return 0;
> > +}
> > +
> > +static int m41t80_recv(I2CSlave *i2c)
> > +{
> > +    M41t80State *s = M41T80(i2c);
> > +    struct tm now;
> > +    qemu_timeval tv;
>
>       int8_t addr;
>
> > +
> > +    if (s->addr < 0) {
> > +        s->addr = 0;
> > +    }
>
>       addr = s->addr++;
>
> So you can simply use 'addr' bellow.

But that would mean numbers in switch cases below would be +1 compared to 
how they are listed in the register map in the datasheet but now they 
match so I'd leave it like this. (In case someone wants to check this with 
the datasheet, the one I could find on-line says the range of register 
number 4 (holding day of week) is 1-7 but U-Boot and clients expect 0-6 so 
I think it's a bug in the datasheet and clients were tested on hardware.)

Regards,
BALATON Zoltan

> > +    if (s->addr >= 1 && s->addr <= 7) {
> > +        qemu_get_timedate(&now, -1);
> > +    }
> > +    switch (s->addr++) {
> > +    case 0:
> > +        qemu_gettimeofday(&tv);
> > +        return to_bcd(tv.tv_usec / 10000);
> > +    case 1:
> > +        return to_bcd(now.tm_sec);
> > +    case 2:
> > +        return to_bcd(now.tm_min);
> > +    case 3:
> > +        return to_bcd(now.tm_hour);
> > +    case 4:
> > +        return to_bcd(now.tm_wday);
> > +    case 5:
> > +        return to_bcd(now.tm_mday);
> > +    case 6:
> > +        return to_bcd(now.tm_mon + 1);
> > +    case 7:
> > +        return to_bcd(now.tm_year % 100);
> > +    case 8 ... 19:
> > +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
> > +                      __func__, s->addr - 1);
> > +        return 0;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
> > +                      __func__, s->addr - 1);
> > +        return 0;
> > +    }
> > +}
> > +
> > +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
> > +{
> > +    M41t80State *s = M41T80(i2c);
> > +
> > +    if (event == I2C_START_SEND) {
> > +        s->addr = -1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void m41t80_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > +
> > +    dc->realize = m41t80_realize;
> > +    sc->send = m41t80_send;
> > +    sc->recv = m41t80_recv;
> > +    sc->event = m41t80_event;
> > +}
> > +
> > +static const TypeInfo m41t80_info = {
> > +    .name          = TYPE_M41T80,
> > +    .parent        = TYPE_I2C_SLAVE,
> > +    .instance_size = sizeof(M41t80State),
> > +    .class_init    = m41t80_class_init,
> > +};
> > +
> > +static void m41t80_register_types(void)
> > +{
> > +    type_register_static(&m41t80_info);
> > +}
> > +
> > +type_init(m41t80_register_types)