Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 000..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. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 000..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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 000..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 -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 000..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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 >>> --- >>> 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 >>> 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 000..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 / 1);> + 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 --- 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 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 000..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 / 1);> +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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
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 > --- > 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 > 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 000..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 / 1);> +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) >