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

[David Gibson]

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


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