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

[BALATON Zoltan]

On Fri, 8 Jun 2018, Cédric Le Goater wrote:
> On 06/06/2018 03:31 PM, 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.
> >
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >  MAINTAINERS                     |   1 +
> >  default-configs/ppc-softmmu.mak |   1 +
> >  hw/timer/Makefile.objs          |   1 +
> >  hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 120 insertions(+)
> >  create mode 100644 hw/timer/m41t80.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 41cd373..9e13bc1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -826,6 +826,7 @@ M: BALATON Zoltan <address@hidden>
> >  L: address@hidden
> >  S: Maintained
> >  F: hw/ide/sii3112.c
> > +F: hw/timer/m41t80.c
> >
> >  SH4 Machines
> >  ------------
> > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > index 7d0dc2f..9fbaadc 100644
> > --- a/default-configs/ppc-softmmu.mak
> > +++ b/default-configs/ppc-softmmu.mak
> > @@ -27,6 +27,7 @@ CONFIG_SM501=y
> >  CONFIG_IDE_SII3112=y
> >  CONFIG_I2C=y
> >  CONFIG_BITBANG_I2C=y
> > +CONFIG_M41T80=y
> >
> >  # For Macs
> >  CONFIG_MAC=y
> > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> > index 8b27a4b..e16b2b9 100644
> > --- a/hw/timer/Makefile.objs
> > +++ b/hw/timer/Makefile.objs
> > @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
> >  common-obj-$(CONFIG_DS1338) += ds1338.o
> >  common-obj-$(CONFIG_HPET) += hpet.o
> >  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> > +common-obj-$(CONFIG_M41T80) += m41t80.o
> >  common-obj-$(CONFIG_M48T59) += m48t59.o
> >  ifeq ($(CONFIG_ISA_BUS),y)
> >  common-obj-$(CONFIG_M48T59) += m48t59-isa.o
> > 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++;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int m41t80_recv(I2CSlave *i2c)
> > +{
> > +    M41t80State *s = M41T80(i2c);
> > +    struct tm now;
> > +    qemu_timeval tv;
> > +
> > +    if (s->addr < 0) {
> > +        s->addr = 0;
> > +    }
> > +    if (s->addr >= 1 && s->addr <= 7) {
> > +        qemu_get_timedate(&now, -1);
> > +    }
> > +    switch (s->addr++) {
>
> you could use some define to name the registers :

This was also suggested by Philippe Mathieu-Daudé and my answer to that 
was that I don't feel like I want to come up with names the datasheet does 
not have either. I think this device is simple enough with just 20 
consecutively numbered registers that appear only in these switch cases by 
number as in the datasheet table so that I don't want to make it more 
difficult to read by encrypting these numbers behind some arbitrary 
defines without a good reason. They are also so simple that it's clear 
from the usually one line implementation what they do so that's also not a 
good reason to name them.

> > +    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);
>
> There is an interesting century bit in specs.

Which I could not figure out how should work and guests seem to be happy 
without it so I did not try to implement it.

> > +    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",
>
> is the beginning \n needed ?

Probably not, maybe left there due to previous debug logs I've removed. 
I'll drop the beginning \n-s in next version.

> Thanks,
>
> C.

Thanks for the review,
BALATON Zoltan