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

[Cédric Le Goater]

On 06/08/2018 06:16 PM, BALATON Zoltan wrote:
> 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.

OK. It's fine with me but you might get some inspiration from Linux 
for the names :)

>>> +    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.

yes. It seems that Linux simply ignores it. Let's forget it.

Thanks,
C.

>>> +    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