diff mbox

[v2,5/8] hw/timer: Add basic M41T80 emulation

Message ID 8c404c8c4ee1bfd2e4d079877d481094f797df8f.1528291908.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show

Commit Message

BALATON Zoltan June 6, 2018, 1:31 p.m. UTC
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 <balaton@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

Comments

Philippe Mathieu-Daudé June 6, 2018, 4:03 p.m. UTC | #1
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.
> 
> Signed-off-by: BALATON Zoltan <balaton@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 <balaton@eik.bme.hu>
>  L: qemu-ppc@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++;
> +    }

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?

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

> +    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)
>
BALATON Zoltan June 6, 2018, 5:35 p.m. UTC | #2
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)
>>
>
>
Cédric Le Goater June 8, 2018, 12:42 p.m. UTC | #3
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 <balaton@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 <balaton@eik.bme.hu>
>  L: qemu-ppc@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 :

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

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

Thanks,

C.
 
> +                      __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)
>
BALATON Zoltan June 8, 2018, 4:16 p.m. UTC | #4
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 <balaton@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 <balaton@eik.bme.hu>
>>  L: qemu-ppc@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.

>> +    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
Cédric Le Goater June 8, 2018, 5:49 p.m. UTC | #5
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 <balaton@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 <balaton@eik.bme.hu>
>>>  L: qemu-ppc@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
David Gibson June 13, 2018, 4:11 a.m. UTC | #6
On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> 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?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here?  You're assigning the uint8_t data to
addr - could that result in a negative value?
BALATON Zoltan June 13, 2018, 8:50 a.m. UTC | #7
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>> 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?
>
> So, I'm not particularly in favour of adding extra state variables.
>
> But is using addr < 0 safe here?  You're assigning the uint8_t data to
> addr - could that result in a negative value?

Why wouldn't it be safe with the expected values for register address 
between 0-19? If the guest sends garbage values over 127 it will either 
result in invalid register or unselected register and lead to an error 
when trying to read/write that register so I don't see what other problem 
this may cause.

The addr < 0 is to check if no address was selected before (on creating 
the device and when sending first value from host addr is set to -1. In 
this case first write will set register address, then subsequent 
reads/writes increment register address as the datasheet says).

Regards,
BALATON Zoltan
David Gibson June 13, 2018, 10:03 a.m. UTC | #8
On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > 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?
> > 
> > So, I'm not particularly in favour of adding extra state variables.
> > 
> > But is using addr < 0 safe here?  You're assigning the uint8_t data to
> > addr - could that result in a negative value?
> 
> Why wouldn't it be safe with the expected values for register address
> between 0-19? If the guest sends garbage values over 127 it will either
> result in invalid register or unselected register and lead to an error when
> trying to read/write that register so I don't see what other problem this
> may cause.

Ok, but where is that enforced?

> The addr < 0 is to check if no address was selected before (on creating the
> device and when sending first value from host addr is set to -1. In this
> case first write will set register address, then subsequent reads/writes
> increment register address as the datasheet says).
> 
> Regards,
> BALATON Zoltan
BALATON Zoltan June 13, 2018, 2:13 p.m. UTC | #9
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>>>> 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?
>>>
>>> So, I'm not particularly in favour of adding extra state variables.
>>>
>>> But is using addr < 0 safe here?  You're assigning the uint8_t data to
>>> addr - could that result in a negative value?
>>
>> Why wouldn't it be safe with the expected values for register address
>> between 0-19? If the guest sends garbage values over 127 it will either
>> result in invalid register or unselected register and lead to an error when
>> trying to read/write that register so I don't see what other problem this
>> may cause.
>
> Ok, but where is that enforced?

I don't see the problem. The addr register selects the register to read or 
write. It is set by the first write when the device is accessed the first 
time (this is denoted by addr == -1 (or really any negative value). The 
device has 20 registers and trying to read any register outside addr 
between 0-19 will result in returning 0 and logging a warning about 
invalid register in m41t80_recv. What could fail here when guest sends 
garbage? It will set addr to invalid value and try to read non-exitent 
register and get an error just like for any other nonexistent value of 
addr (or start to read from register 0 if it manages to set a negative 
value). All writes of registers are ignored currently (except setting addr 
by the first write). What should be enforced here?

Regards,
BALATON Zoltan
David Gibson June 14, 2018, 1:27 a.m. UTC | #10
On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > > > 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?
> > > > 
> > > > So, I'm not particularly in favour of adding extra state variables.
> > > > 
> > > > But is using addr < 0 safe here?  You're assigning the uint8_t data to
> > > > addr - could that result in a negative value?
> > > 
> > > Why wouldn't it be safe with the expected values for register address
> > > between 0-19? If the guest sends garbage values over 127 it will either
> > > result in invalid register or unselected register and lead to an error when
> > > trying to read/write that register so I don't see what other problem this
> > > may cause.
> > 
> > Ok, but where is that enforced?
> 
> I don't see the problem. The addr register selects the register to read or
> write. It is set by the first write when the device is accessed the first
> time (this is denoted by addr == -1 (or really any negative value). The
> device has 20 registers and trying to read any register outside addr between
> 0-19 will result in returning 0 and logging a warning about invalid register
> in m41t80_recv. What could fail here when guest sends garbage? It will set
> addr to invalid value and try to read non-exitent register and get an error
> just like for any other nonexistent value of addr (or start to read from
> register 0 if it manages to set a negative value). All writes of registers
> are ignored currently (except setting addr by the first write). What should
> be enforced here?

Ah, I see your point.  I mean strictly we should match the hardware
behaviour if you write garbage addresses here, but really I don't
think it matters much.

Ok, it should be fine.
BALATON Zoltan June 14, 2018, 7:54 a.m. UTC | #11
On Thu, 14 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
>> I don't see the problem. The addr register selects the register to read or
>> write. It is set by the first write when the device is accessed the first
>> time (this is denoted by addr == -1 (or really any negative value). The
>> device has 20 registers and trying to read any register outside addr between
>> 0-19 will result in returning 0 and logging a warning about invalid register
>> in m41t80_recv. What could fail here when guest sends garbage? It will set
>> addr to invalid value and try to read non-exitent register and get an error
>> just like for any other nonexistent value of addr (or start to read from
>> register 0 if it manages to set a negative value). All writes of registers
>> are ignored currently (except setting addr by the first write). What should
>> be enforced here?
>
> Ah, I see your point.  I mean strictly we should match the hardware
> behaviour if you write garbage addresses here, but really I don't
> think it matters much.

Problem is like usual I have no idea what the real hardware does. I've 
only seen the datasheet, never seen real device and have no way to test. 
All the clients I've tested seem to be OK with the current emulation so 
unless someone has more info on how this should work I think we can live 
with this version and then fix it later if found to be needed.

Regards,
BALATON Zoltan
David Gibson June 15, 2018, 4:08 a.m. UTC | #12
On Thu, Jun 14, 2018 at 09:54:41AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> > > I don't see the problem. The addr register selects the register to read or
> > > write. It is set by the first write when the device is accessed the first
> > > time (this is denoted by addr == -1 (or really any negative value). The
> > > device has 20 registers and trying to read any register outside addr between
> > > 0-19 will result in returning 0 and logging a warning about invalid register
> > > in m41t80_recv. What could fail here when guest sends garbage? It will set
> > > addr to invalid value and try to read non-exitent register and get an error
> > > just like for any other nonexistent value of addr (or start to read from
> > > register 0 if it manages to set a negative value). All writes of registers
> > > are ignored currently (except setting addr by the first write). What should
> > > be enforced here?
> > 
> > Ah, I see your point.  I mean strictly we should match the hardware
> > behaviour if you write garbage addresses here, but really I don't
> > think it matters much.
> 
> Problem is like usual I have no idea what the real hardware does. I've only
> seen the datasheet, never seen real device and have no way to test. All the
> clients I've tested seem to be OK with the current emulation so unless
> someone has more info on how this should work I think we can live with this
> version and then fix it later if found to be needed.

Ok, fair enough.
diff mbox

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 41cd373..9e13bc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -826,6 +826,7 @@  M: BALATON Zoltan <balaton@eik.bme.hu>
 L: qemu-ppc@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++) {
+    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)