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 <bala...@eik.bme.hu>
>>> ---
>>> 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 <bala...@eik.bme.hu>
>>> L: qemu-...@nongnu.org
>>> 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
