qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/6] hw/timer: Add Epson RX8900 RTC support


From: Alastair D'Silva
Subject: Re: [Qemu-devel] [PATCH v2 4/6] hw/timer: Add Epson RX8900 RTC support
Date: Fri, 02 Dec 2016 14:30:25 +1100

On Fri, 2016-12-02 at 13:48 +1100, Alexey Kardashevskiy wrote:
> On 02/12/16 11:19, Alastair D'Silva wrote:
> > On Thu, 2016-12-01 at 16:53 +1100, Alexey Kardashevskiy wrote:
> > 
> > > On 30/11/16 16:36, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <address@hidden>
> > > > 
> > > > This patch adds support for the Epson RX8900 I2C RTC.
> > > > 
> > > > The following chip features are implemented:
> > > >  - RTC (wallclock based, ptimer 10x oversampling to pick up
> > > >         wallclock transitions)
> > > >  - Time update interrupt (per second/minute, wallclock based)
> > > >  - Alarms (wallclock based)
> > > >  - Temperature (set via a property)
> > > >  - Countdown timer (emulated clock via ptimer)
> > > >  - FOUT via GPIO (emulated clock via ptimer)
> > > > 
> > > > The following chip features are unimplemented:
> > > >  - Low voltage detection
> > > >  - i2c timeout
> > > > 
> > > > The implementation exports the following named GPIOs:
> > > > rx8900-interrupt-out
> > > > rx8900-fout-enable
> > > > rx8900-fout
> > > > 
> > > > Signed-off-by: Alastair D'Silva <address@hidden>
> > > > Signed-off-by: Chris Smart <address@hidden>
> > > > ---
> > > >  default-configs/arm-softmmu.mak |   1 +
> > > >  hw/timer/Makefile.objs          |   2 +
> > > >  hw/timer/rx8900.c               | 890
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  hw/timer/rx8900_regs.h          | 139 +++++++
> > > >  hw/timer/trace-events           |  31 ++
> > > >  5 files changed, 1063 insertions(+)
> > > >  create mode 100644 hw/timer/rx8900.c
> > > >  create mode 100644 hw/timer/rx8900_regs.h
> > > > 
> > > > diff --git a/default-configs/arm-softmmu.mak b/default-
> > > > configs/arm-
> > > > softmmu.mak
> > > > index 6de3e16..adb600e 100644
> > > > --- a/default-configs/arm-softmmu.mak
> > > > +++ b/default-configs/arm-softmmu.mak
> > > > @@ -29,6 +29,7 @@ CONFIG_SMC91C111=y
> > > >  CONFIG_ALLWINNER_EMAC=y
> > > >  CONFIG_IMX_FEC=y
> > > >  CONFIG_DS1338=y
> > > > +CONFIG_RX8900=y
> > > >  CONFIG_PFLASH_CFI01=y
> > > >  CONFIG_PFLASH_CFI02=y
> > > >  CONFIG_MICRODRIVE=y
> > > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> > > > index 7ba8c23..fa028ac 100644
> > > > --- a/hw/timer/Makefile.objs
> > > > +++ b/hw/timer/Makefile.objs
> > > > @@ -3,6 +3,7 @@ common-obj-$(CONFIG_ARM_MPTIMER) +=
> > > > arm_mptimer.o
> > > >  common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
> > > >  common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
> > > >  common-obj-$(CONFIG_DS1338) += ds1338.o
> > > > +common-obj-$(CONFIG_RX8900) += rx8900.o
> > > >  common-obj-$(CONFIG_HPET) += hpet.o
> > > >  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> > > >  common-obj-$(CONFIG_M48T59) += m48t59.o
> > > > @@ -17,6 +18,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
> > > >  common-obj-$(CONFIG_IMX) += imx_gpt.o
> > > >  common-obj-$(CONFIG_LM32) += lm32_timer.o
> > > >  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> > > > +common-obj-$(CONFIG_RX8900) += rx8900.o
> > > >  
> > > >  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
> > > >  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> > > > diff --git a/hw/timer/rx8900.c b/hw/timer/rx8900.c
> > > > new file mode 100644
> > > > index 0000000..e634819
> > > > --- /dev/null
> > > > +++ b/hw/timer/rx8900.c
> > > > @@ -0,0 +1,890 @@
> > > > +/*
> > > > + * Epson RX8900SA/CE Realtime Clock Module
> > > > + *
> > > > + * Copyright (c) 2016 IBM Corporation
> > > > + * Authors:
> > > > + *  Alastair D'Silva <address@hidden>
> > > > + *  Chris Smart <address@hidden>
> > > > + *
> > > > + * This code is licensed under the GPL version 2 or
> > > > later.  See
> > > > + * the COPYING file in the top-level directory.
> > > > + *
> > > > + * Datasheet available at:
> > > > + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX89
> > > > 00CE
> > > > &lang=en
> > > > + *
> > > > + * Not implemented:
> > > > + *  Implement i2c timeout
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu-common.h"
> > > > +#include "hw/i2c/i2c.h"
> > > > +#include "hw/timer/rx8900_regs.h"
> > > > +#include "hw/ptimer.h"
> > > > +#include "qemu/main-loop.h"
> > > > +#include "qemu/bcd.h"
> > > > +#include "qemu/log.h"
> > > > +#include "qapi/error.h"
> > > > +#include "qapi/visitor.h"
> > > > +#include "trace.h"
> > > > +
> > > > + #include <sys/time.h>
> > > > +
> > > > + #include <execinfo.h>
> > > 
> > > Not needed empty lines and spaces before "#include".
> > > 
> > 
> > Ok, these were leftovers and don't belong there anyway.
> > 
> > > 
> > > > +
> > > > +#define TYPE_RX8900 "rx8900"
> > > > +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj),
> > > > TYPE_RX8900)
> > > > +
> > > > +typedef struct RX8900State {
> > > > +    I2CSlave parent_obj;
> > > > +
> > > > +    ptimer_state *sec_timer; /* triggered once per second */
> > > > +    ptimer_state *fout_timer;
> > > > +    ptimer_state *countdown_timer;
> > > > +    bool fout;
> > > 
> > > Is this "FOE" on the chip?
> > > 
> > 
> > No, it tracks the state of the fout waveform. I'll rename it to
> > fout_state.
> 
> Since it is bool, fout_enabled makes more sense.
> 

No it does't, it's not an enabled control, but the phase of the
waveform.

> 
> > 
> > > 
> > > > +    int64_t offset;
> > > > +    uint8_t weekday; /* Saved for deferred offset calculation,
> > > > 0-6 
> > > > */
> > > > +    uint8_t wday_offset;
> > > > +    uint8_t nvram[RX8900_NVRAM_SIZE];
> > > > +    int32_t ptr; /* Wrapped to stay within RX8900_NVRAM_SIZE
> > > > */
> > > 
> > > It is rather "nvram_offset" than some pointer.
> > > 
> > 
> > Ok
> > 
> > > 
> > > > +    bool addr_byte;
> > > > +    uint8_t last_interrupt_seconds;
> > > 
> > > s/last_interrupt_seconds/last_update_interrupt_seconds/
> > > 
> > 
> > No, this is more generic than the update interrupt.
> 
> 
> last_update_interrupt_minutes is updated in update_control_register()
> and
> rx8900_timer_tick()
> 
> last_interrupt_seconds is updated in update_control_register() and
> rx8900_timer_tick() as well.
> 
> Yes, the conditions are sometime different but it is still quite hard
> to
> tell how one is more generic than the other. All I am saying here is
> that
> the names are not clear and there is no comment about them either.

I'll add some comments.

> 
> > > 
> > > > +    uint8_t last_update_interrupt_minutes;
> > > > +    double supply_voltage;
> > > > +    qemu_irq interrupt_pin;
> > > 
> > > Is this "INT" on the chip?
> > > 
> > 
> > Yes
> 
> I'd suggest adding a short comment /* INT pin */ at the end of the
> line.

I don't think it's a huge logical leap here to associate
"interrupt_pin" with the interrupt pin of the device.

> > 
> > > > +    qemu_irq fout_pin;
> > > 
> > > Is this "FOUT" on the chip?
> > > 
> > 
> > Yes
> 
> 
> Same here.

Ditto

> > 
> > > 
> > > > +} RX8900State;
> > > > +
> > > > +static const VMStateDescription vmstate_rx8900 = {
> > > > +    .name = "rx8900",
> > > > +    .version_id = 2,
> > > 
> > > 
> > > vmstate version is 2 for a brand new device means that there is
> > > another
> > > device which can migrate to it? I think you want version_id=1 and
> > > get
> > > rid
> > > of _V below.
> > > 
> > 
> > Ok
> > 
> > > 
> > > 
> > > > +    .minimum_version_id = 1,
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_I2C_SLAVE(parent_obj, RX8900State),
> > > > +        VMSTATE_PTIMER(sec_timer, RX8900State),
> > > > +        VMSTATE_PTIMER(fout_timer, RX8900State),
> > > > +        VMSTATE_PTIMER(countdown_timer, RX8900State),
> > > > +        VMSTATE_BOOL(fout, RX8900State),
> > > > +        VMSTATE_INT64(offset, RX8900State),
> > > > +        VMSTATE_UINT8_V(weekday, RX8900State, 2),
> > > > +        VMSTATE_UINT8_V(wday_offset, RX8900State, 2),
> > > > +        VMSTATE_UINT8_ARRAY(nvram, RX8900State,
> > > > RX8900_NVRAM_SIZE),
> > > > +        VMSTATE_INT32(ptr, RX8900State),
> > > > +        VMSTATE_BOOL(addr_byte, RX8900State),
> > > > +        VMSTATE_UINT8_V(last_interrupt_seconds, RX8900State,
> > > > 2),
> > > > +        VMSTATE_UINT8_V(last_update_interrupt_minutes,
> > > > RX8900State, 2),
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    }
> > > > +};
> > > > +
> > > > +static void rx8900_reset(DeviceState *dev);
> > > > +static void disable_countdown_timer(RX8900State *s);
> > > 
> > > This one not needed.
> > > 
> > 
> > Ok
> > 
> > > 
> > > > +static void enable_countdown_timer(RX8900State *s);
> > > > +static void disable_timer(RX8900State *s);
> > > > +static void enable_timer(RX8900State *s);
> > > > From a quick look, all these functions could be moved right
> > > > here,
> > > > cannot they?
> > 
> > Ok, except for reset, which I prefer to leave next to the other
> > init
> > code so that associated code is kept togther.
> > > 
> > > > +
> > > > +static void capture_current_time(RX8900State *s)
> > > > +{
> > > > +    /* Capture the current time into the secondary registers
> > > > +     * which will be actually read by the data transfer
> > > > operation.
> > > > +     */
> > > > +    struct tm now;
> > > > +    qemu_get_timedate(&now, s->offset);
> > > > +    s->nvram[SECONDS] = to_bcd(now.tm_sec);
> > > > +    s->nvram[MINUTES] = to_bcd(now.tm_min);
> > > > +    s->nvram[HOURS] = to_bcd(now.tm_hour);
> > > > +
> > > > +    s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + s-
> > > > >wday_offset) %
> > > > 7);
> > > 
> > > s/0x01/1/ ?
> > 
> > Weekday is a walking bit, I think hex notation is clearer.
> 
> 
> "1" is a pure one. 0x01 suggests a bit mask so it may be possible to
> shift
> some other value. I cannot think how 0x01 is clearer, I asked Paul,
> he
> cannot either ;)
> 

From the datasheet:
The day data values are counted as follows: Day 01h, Day 02h, Day 04h,
Day 08h, Day 10h, Day 20h, Day 40h, Day 01h, Day 02h, etc.   

> 
> > 
> > > 
> > > > +    s->nvram[DAY] = to_bcd(now.tm_mday);
> > > > +    s->nvram[MONTH] = to_bcd(now.tm_mon + 1);
> > > > +    s->nvram[YEAR] = to_bcd(now.tm_year % 100);
> > > > +
> > > > +    s->nvram[EXT_SECONDS] = s->nvram[SECONDS];
> > > > +    s->nvram[EXT_MINUTES] = s->nvram[MINUTES];
> > > > +    s->nvram[EXT_HOURS] = s->nvram[HOURS];
> > > > +    s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY];
> > > > +    s->nvram[EXT_DAY] = s->nvram[DAY];
> > > > +    s->nvram[EXT_MONTH] = s->nvram[MONTH];
> > > > +    s->nvram[EXT_YEAR] = s->nvram[YEAR];
> > > > +
> > > > +    trace_rx8900_capture_current_time(now.tm_hour, now.tm_min,
> > > > now.tm_sec,
> > > > +            (now.tm_wday + s->wday_offset) % 7,
> > > > +            now.tm_mday, now.tm_mon, now.tm_year + 1900,
> > > > +            s->nvram[HOURS], s->nvram[MINUTES], s-
> > > > >nvram[SECONDS],
> > > > +            s->nvram[WEEKDAY], s->nvram[DAY], s->nvram[MONTH], 
> > > > s-
> > > > > nvram[YEAR]);
> > > > 
> > > > +}
> > > > +
> > > > +/**
> > > > + * Increment the internal register pointer, dealing with
> > > > wrapping
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void inc_regptr(RX8900State *s)
> > > > +{
> > > > +    /* The register pointer wraps around after 0x1F
> > > > +     */
> > > > +    s->ptr = (s->ptr + 1) & (RX8900_NVRAM_SIZE - 1);
> > > > +    trace_rx8900_regptr_update(s->ptr);
> > > > +
> > > > +    if (s->ptr == 0x00) {
> > > 
> > > 
> > > Magic constant 0x00. Is this offset in nvram? Then make it just
> > > 0,
> > > otherwise it looks like a mask.
> > 
> > Replaced with START_ADDRESS from RX8900Addresses.
> > 
> > > 
> > > > +        trace_rx8900_regptr_overflow();
> > > > +        capture_current_time(s);
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Receive an I2C Event
> > > > + * @param i2c the i2c device instance
> > > > + * @param event the event to handle
> > > > + */
> > > > +static void rx8900_event(I2CSlave *i2c, enum i2c_event event)
> > > > +{
> > > > +    RX8900State *s = RX8900(i2c);
> > > > +
> > > > +    switch (event) {
> > > > +    case I2C_START_RECV:
> > > > +        /* In h/w, time capture happens on any START
> > > > condition,
> > > > not just a
> > > > +         * START_RECV. For the emulation, it doesn't actually
> > > > matter,
> > > > +         * since a START_RECV has to occur before the data can
> > > > be
> > > > read.
> > > > +         */
> > > > +        capture_current_time(s);
> > > > +        break;
> > > > +    case I2C_START_SEND:
> > > > +        s->addr_byte = true;
> > > > +        break;
> > > > +    case I2C_FINISH:
> > > > +        if (s->weekday < 7) {
> > > > +            /* We defer the weekday calculation as it is
> > > > handed to
> > > > us before
> > > > +             * the date has been updated. If we calculate the
> > > > weekday offset
> > > > +             * when it is passed to us, we will incorrectly
> > > > determine it
> > > > +             * based on the current emulated date, rather than
> > > > the
> > > > date that
> > > > +             * has been written.
> > > > +             */
> > > 
> > > The RX8900 spec does not use word "offset" at all so I cannot
> > > make
> > > sense
> > > from the paragraph above - why exactly do you need 2 offsets
> > > ("offset" and
> > > "wday_offset") and why weekday cannot be calculated when it is
> > > needed
> > > from
> > > the current time + "offset"?
> > > 
> > 
> > The offsets refer to the difference between the host clock & the
> > emulated clock. The weekday cannot be calculated as the RX8900 does
> > not
> > validate the weekday - the user can set the weekday to anything.
> 
> 
> Ufff. Now I am totally confused. You say "the weekday cannot be
> calculated"
> but the comment says you defer its calculation. The comment needs an
> update.

Sorry, my bad, the weekday cannot be calculated without the weekday
offset, as the weekday is not guaranteed to be correct for that date.

> 
> > 
> > > 
> > > > +            struct tm now;
> > > > +            qemu_get_timedate(&now, s->offset);
> > > > +
> > > > +            s->wday_offset = (s->weekday - now.tm_wday + 7) %
> > > > 7;
> > > > +
> > > > +            trace_rx8900_event_weekday(s->weekday, BIT(s-
> > > > > weekday),
> > > > 
> > > > +                    s->wday_offset);
> > > > +
> > > > +            s->weekday = 7;
> > > 
> > > I'd rather use 0xff (defined in a macro) as an invalid weekday.
> > > 
> > 
> > Ok
> > 
> > > 
> > > > +        }
> > > > +        break;
> > > > +
> > > > +    default:
> > > > +        break;
> > > 
> > > 
> > > Not needed "default" case.
> > > 
> > 
> > This is a hint to static analysers (and developers) that the other
> > enumeration cases were not forgotten, but intentionally have no
> > action.
> 
> 
> Out of curiosity - what static analyzer does complain about this? I
> looked
> at the kernel and QEMU trees and seems that it is not common thing to
> leave
> an empty "default" case.

Eclipse Codan will flag missing enum elements in a switch.

> 
> > 
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Perform an i2c receive action
> > > > + * @param i2c the i2c device instance
> > > > + * @return the value of the current register
> > > > + * @post the internal register pointer is incremented
> > > > + */
> > > > +static int rx8900_recv(I2CSlave *i2c)
> > > > +{
> > > > +    RX8900State *s = RX8900(i2c);
> > > > +    uint8_t res = s->nvram[s->ptr];
> > > > +    trace_rx8900_read_register(s->ptr, res);
> > > > +    inc_regptr(s);
> > > > +    return res;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Validate the extension register and perform actions based
> > > > on
> > > > the bits
> > > > + * @param s the RTC to operate on
> > > > + * @param data the new data for the extension register
> > > > + */
> > > > +static void update_extension_register(RX8900State *s, uint8_t
> > > > data)
> > > > +{
> > > > +    if (data & EXT_MASK_TEST) {
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Test bit is enabled but is forbidden by the
> > > > manufacturer");
> > > 
> > > QEMU uses indents under opening bracket.
> > > 
> > 
> > Debatable, it's not prescribed in the coding style, and existing
> > code
> > varies. Can you point me at where that is stated?
> 
> Nope but it is all over the place. You may see occasinally different
> styles
> but not in main files such as vl.c or hw/pci/pci.c.
> 
> 
> > 
> > > In general, I use vim with
> > > set expandtab
> > > set tabstop=4
> > > set shiftwidth=4
> > > set cino=:0,(0
> > > 
> > > 
> > > The coding style also says about 80 characters limit (and some of
> > > your
> > > patched failed this) and makes no exception, however people
> > > often 
> > 
> > Actually, it does:
> 
> 
> Ok, good to know. When I studied the coding style last time, it just
> was
> not there.
> 
> 
> > "Sometimes it is hard to do, especially when dealing with QEMU
> > subsystems that use long function or symbol names.  Even in that
> > case,
> > do not make lines much longer than 80 characters."
> > 
> > > follow
> > > the kernel rule not to split strings even if they do not fit 80
> > > characters
> > > limit.
> > > 
> > > 
> > > And please run ./scripts/checkpatch.pl on patches before posting,
> > > 3/6
> > > and
> > > 5/6 failed because of too long lines.
> > 
> > I have this set up as a git commit hook, so I'm not sure how this
> > slipped in, but sure, I can rerun it before submitting.
> > 
> > > 
> > > > +    }
> > > > +
> > > > +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> > > > +            (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> > > > +        uint8_t fsel = (data & (EXT_MASK_FSEL0 |
> > > > EXT_MASK_FSEL1))
> > > > +                >> EXT_REG_FSEL0;
> > > > +        /* FSELx has changed */
> > > > +        switch (fsel) {
> > > > +        case 0x01:
> > > 
> > > Magic value of 0x01, 0x02 here and below.
> > > 
> > 
> > I think this use case is reasonable, we are matching against the 2
> > FSEL
> > bits.
> 
> I'd literally do this:
> 
> switch (data & (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> case EXT_MASK_FSEL0:
>       trace_rx8900_set_fout(1024);
>       ptimer_set_limit(s->fout_timer, 32, 1);
>       break;
> case EXT_MASK_FSEL1:
>       trace_rx8900_set_fout(1);
>       ptimer_set_limit(s->fout_timer, 32768, 1);
>       break;
> case 0:
> case EXT_MASK_FSEL0 | EXT_MASK_FSEL0:
>       trace_rx8900_set_fout(32768);
>       ptimer_set_limit(s->fout_timer, 1, 1);
>       break;
> }
> 
> Easier to read and you can be sure that nothing is missed and all
> bits
> combinations are covered.

Ok

> 
> > 
> > > 
> > > > +            trace_rx8900_set_fout(1024);
> > > > +            ptimer_set_limit(s->fout_timer, 32, 1);
> > > > +            break;
> > > > +        case 0x02:
> > > > +            trace_rx8900_set_fout(1);
> > > > +            ptimer_set_limit(s->fout_timer, 32768, 1);
> > > > +            break;
> > > > +        default:
> > > > +            trace_rx8900_set_fout(32768);
> > > > +            ptimer_set_limit(s->fout_timer, 1, 1);
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> > > > +            (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) {
> > > > +        uint8_t tsel = (data & (EXT_MASK_TSEL0 |
> > > > EXT_MASK_TSEL1))
> > > > +                >> EXT_REG_TSEL0;
> > > > +        /* TSELx has changed */
> > > > +        switch (tsel) {
> > > > +        case 0x00:
> > > > +            trace_rx8900_set_countdown_timer(64);
> > > > +            ptimer_set_limit(s->countdown_timer, 4096 / 64,
> > > > 1);
> > > > +            break;
> > > > +        case 0x01:
> > > > +            trace_rx8900_set_countdown_timer(1);
> > > > +            ptimer_set_limit(s->countdown_timer, 4096, 1);
> > > > +            break;
> > > > +        case 0x02:
> > > > +            trace_rx8900_set_countdown_timer_per_minute();
> > > > +            ptimer_set_limit(s->countdown_timer, 4069 * 60,
> > > > 1);
> > > 
> > > s/4069/4096/ ?
> > > And why 4096? Please define it in a macro.
> > 
> > Ok
> > 
> > > 
> > > > +            break;
> > > > +        case 0x03:
> > > > +            trace_rx8900_set_countdown_timer(4096);
> > > > +            ptimer_set_limit(s->countdown_timer, 1, 1);
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (data & EXT_MASK_TE) {
> > > > +        enable_countdown_timer(s);
> > > > +    }
> > > > +
> > > > +    s->nvram[EXTENSION_REGISTER] = data;
> > > > +    s->nvram[EXT_EXTENSION_REGISTER] = data;
> > > > +
> > > > +}
> > > > +/**
> > > > + * Validate the control register and perform actions based on
> > > > the
> > > > bits
> > > > + * @param s the RTC to operate on
> > > > + * @param data the new value for the control register
> > > > + */
> > > > +
> > > > +static void update_control_register(RX8900State *s, uint8_t
> > > > data)
> > > > +{
> > > > +    uint8_t diffmask = ~s->nvram[CONTROL_REGISTER] & data;
> > > > +
> > > > +    if (diffmask & CTRL_MASK_WP0) {
> > > > +        data &= ~CTRL_MASK_WP0;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Attempt to write to write protected bit %d in
> > > > control
> > > > register",
> > > > +            CTRL_REG_WP0);
> > > > +    }
> > > > +
> > > > +    if (diffmask & CTRL_MASK_WP1) {
> > > > +        data &= ~CTRL_MASK_WP1;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Attempt to write to write protected bit %d in
> > > > control
> > > > register",
> > > > +            CTRL_REG_WP1);
> > > > +    }
> > > > +
> > > > +    if (data & CTRL_MASK_RESET) {
> > > > +        data &= ~CTRL_MASK_RESET;
> > > > +        rx8900_reset(DEVICE(s));
> > > > +    }
> > > > +
> > > > +    if (diffmask & CTRL_MASK_UIE) {
> > > > +        /* Update interrupts were off and are now on */
> > > > +        struct tm now;
> > > > +
> > > > +        trace_rx8900_enable_update_timer();
> > > > +
> > > > +        qemu_get_timedate(&now, s->offset);
> > > > +
> > > > +        s->last_update_interrupt_minutes = now.tm_min;
> > > > +        s->last_interrupt_seconds = now.tm_sec;
> > > > +        enable_timer(s);
> > > > +    }
> > > > +
> > > > +    if (diffmask & CTRL_MASK_AIE) {
> > > > +        /* Alarm interrupts were off and are now on */
> > > > +        struct tm now;
> > > > +
> > > > +        trace_rx8900_enable_alarm();
> > > > +
> > > > +        qemu_get_timedate(&now, s->offset);
> > > > +
> > > > +        s->last_interrupt_seconds = now.tm_sec;
> > > 
> > > 
> > > s->last_update_interrupt_minutes is skipped here for a reason?
> > > 
> > 
> > Yes, that pertains to the update interrupts, and we are dealing
> > with
> > the alarm here.
> > 
> > > 
> > > > +        enable_timer(s);
> > > > +    }
> > > > +
> > > > +    if (!(data & (CTRL_MASK_UIE | CTRL_MASK_AIE))) {
> > > > +        disable_timer(s);
> > > > +    }
> > > 
> > > 
> > > Can UIE and AIE be both set? If not, "else" could be used in two
> > > "if"
> > > above
> > > to document this.
> > > 
> > 
> > Yes.
> > 
> > > 
> > > > +
> > > > +    s->nvram[CONTROL_REGISTER] = data;
> > > > +    s->nvram[EXT_CONTROL_REGISTER] = data;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Validate the flag register
> > > > + * @param s the RTC to operate on
> > > > + * @param data the new value for the flag register
> > > > + */
> > > > +static void validate_flag_register(RX8900State *s, uint8_t
> > > > *data)
> > > > +{
> > > > +    uint8_t diffmask = ~s->nvram[FLAG_REGISTER] & *data;
> > > > +
> > > > +    if (diffmask & FLAG_MASK_VDET) {
> > > > +        *data &= ~FLAG_MASK_VDET;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Only 0 can be written to VDET bit %d in the flag
> > > > register",
> > > > +            FLAG_REG_VDET);
> > > > +    }
> > > > +
> > > > +    if (diffmask & FLAG_MASK_VLF) {
> > > > +        *data &= ~FLAG_MASK_VLF;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Only 0 can be written to VLF bit %d in the flag
> > > > register",
> > > > +            FLAG_REG_VLF);
> > > > +    }
> > > > +
> > > > +    if (diffmask & FLAG_MASK_UNUSED_2) {
> > > > +        *data &= ~FLAG_MASK_UNUSED_2;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Only 0 can be written to unused bit %d in the
> > > > flag
> > > > register",
> > > > +            FLAG_REG_UNUSED_2);
> > > > +    }
> > > > +
> > > > +    if (diffmask & FLAG_MASK_UNUSED_6) {
> > > > +        *data &= ~FLAG_MASK_UNUSED_6;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Only 0 can be written to unused bit %d in the
> > > > flag
> > > > register",
> > > > +            FLAG_REG_UNUSED_6);
> > > > +    }
> > > > +
> > > > +    if (diffmask & FLAG_MASK_UNUSED_7) {
> > > > +        *data &= ~FLAG_MASK_UNUSED_7;
> > > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > > +            "Only 0 can be written to unused bit %d in the
> > > > flag
> > > > register",
> > > > +            FLAG_REG_UNUSED_7);
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Tick the per second timer (can be called more frequently as
> > > > it
> > > > early exits
> > > > + * if the wall clock has not progressed)
> > > > + * @param opaque the RTC to tick
> > > > + */
> > > > +static void rx8900_timer_tick(void *opaque)
> > > > +{
> > > > +    RX8900State *s = (RX8900State *)opaque;
> > > > +    struct tm now;
> > > > +    bool fire_interrupt = false;
> > > > +    bool alarm_week_day_matches;
> > > > +
> > > > +    qemu_get_timedate(&now, s->offset);
> > > > +
> > > > +    if (now.tm_sec == s->last_interrupt_seconds) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s->last_interrupt_seconds = now.tm_sec;
> > > > +
> > > > +    trace_rx8900_tick();
> > > > +
> > > > +    /* Update timer interrupt */
> > > > +    if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_UIE) {
> > > > +        if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL) &&
> > > > +                now.tm_min != s-
> > > > >last_update_interrupt_minutes) {
> > > > +            s->last_update_interrupt_minutes = now.tm_min;
> > > > +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> > > > +            fire_interrupt = true;
> > > > +        } else if (!(s->nvram[EXTENSION_REGISTER] &
> > > > EXT_MASK_USEL)) {
> > > > +            /* per second update interrupt */
> > > > +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> > > > +            fire_interrupt = true;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    /* Alarm interrupt */
> > > > +    alarm_week_day_matches = s->nvram[ALARM_WEEK_DAY] ==
> > > > +            ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_WADA) ?
> > > > +                    to_bcd(now.tm_mday) :
> > > > +                    0x01 << ((now.tm_wday + s->wday_offset) %
> > > > 7));
> > > 
> > > 
> > > 0x01 is a mask or enum or just "1" which needs to be shifted?
> > > 
> > 
> > Weekday is a walking bit, I think hex is the most appropriate
> > notation.
> > 
> > > Also, it is hard to read an expression with "=", "==" and "?",
> > > "if"
> > > would
> > > be better here imho.
> > > 
> > 
> > Ok (it read a lot better before it was wrapped).
> > 
> > > 
> > > > +
> > > > +    if ((s->nvram[CONTROL_REGISTER] & CTRL_MASK_AIE) &&
> > > > now.tm_sec
> > > > == 0) {
> > > > +        if (s->nvram[ALARM_MINUTE] == to_bcd(now.tm_min) &&
> > > > +                s->nvram[ALARM_HOUR] == to_bcd(now.tm_hour) &&
> > > > +                alarm_week_day_matches) {
> > > 
> > > It should be one "if", not two.
> > > 
> > 
> > Ok
> > > 
> > > > +            trace_rx8900_trigger_alarm();
> > > > +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_AF;
> > > > +            fire_interrupt = true;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (fire_interrupt) {
> > > > +        trace_rx8900_fire_interrupt();
> > > > +        qemu_irq_pulse(s->interrupt_pin);
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Disable the per second timer
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void disable_timer(RX8900State *s)
> > > > +{
> > > > +    trace_rx8900_disable_timer();
> > > > +    ptimer_stop(s->sec_timer);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Enable the per second timer
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void enable_timer(RX8900State *s)
> > > > +{
> > > > +    trace_rx8900_enable_timer();
> > > > +    ptimer_run(s->sec_timer, 0);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Handle FOUT_ENABLE (FOE) line
> > > > + * Enables/disables the FOUT line
> > > > + * @param opaque the device instance
> > > > + * @param n the IRQ number
> > > > + * @param level true if the line has been raised
> > > > + */
> > > > +static void rx8900_fout_enable_handler(void *opaque, int n,
> > > > int
> > > > level)
> > > > +{
> > > > +    RX8900State *s = RX8900(opaque);
> > > > +
> > > > +    if (level) {
> > > > +        trace_rx8900_enable_fout();
> > > > +        ptimer_run(s->fout_timer, 0);
> > > > +    } else {
> > > > +        /* disable fout */
> > > > +        trace_rx8900_disable_fout();
> > > > +        ptimer_stop(s->fout_timer);
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Tick the FOUT timer
> > > > + * @param opaque the device instance
> > > > + */
> > > > +static void rx8900_fout_tick(void *opaque)
> > > > +{
> > > > +    RX8900State *s = (RX8900State *)opaque;
> > > > +
> > > > +    trace_rx8900_fout_toggle();
> > > > +    s->fout = !s->fout;
> > > > +
> > > > +    if (s->fout) {
> > > > +        qemu_irq_raise(s->fout_pin);
> > > > +    } else {
> > > > +        qemu_irq_lower(s->fout_pin);
> > > > +    }
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + * Disable the countdown timer
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void disable_countdown_timer(RX8900State *s)
> > > > +{
> > > > +    trace_rx8900_disable_countdown();
> > > > +    ptimer_stop(s->countdown_timer);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Enable the countdown timer
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void enable_countdown_timer(RX8900State *s)
> > > > +{
> > > > +    trace_rx8900_enable_countdown();
> > > > +    ptimer_run(s->countdown_timer, 0);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Tick the countdown timer
> > > > + * @param opaque the device instance
> > > > + */
> > > > +static void rx8900_countdown_tick(void *opaque)
> > > > +{
> > > > +    RX8900State *s = (RX8900State *)opaque;
> > > > +
> > > > +    uint16_t count = s->nvram[TIMER_COUNTER_0] +
> > > 
> > > Nit: in cases like this it is usually "|", not "+".
> > > 
> > 
> > Ok
> > 
> > > 
> > > > +            ((s->nvram[TIMER_COUNTER_1] & 0x0F) << 8);
> > > > +    trace_rx8900_countdown_tick(count);
> > > > +    count--;
> > > > +
> > > > +    s->nvram[TIMER_COUNTER_0] = (uint8_t)(count & 0x00ff);
> > > > +    s->nvram[TIMER_COUNTER_1] = (uint8_t)((count & 0x0f00) >>
> > > > 8);
> > > > +
> > > > +    if (count == 0) {
> > > > +        trace_rx8900_countdown_elapsed();
> > > > +
> > > > +        disable_countdown_timer(s);
> > > > +
> > > > +        s->nvram[FLAG_REGISTER] |= FLAG_MASK_TF;
> > > > +
> > > > +        if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_TIE) {
> > > > +            trace_rx8900_fire_interrupt();
> > > > +            qemu_irq_pulse(s->interrupt_pin);
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Verify the current voltage and raise flags if it is low
> > > > + * @param s the RTC to operate on
> > > > + */
> > > > +static void check_voltage(RX8900State *s)
> > > > +{
> > > > +    if (!(s->nvram[BACKUP_FUNCTION] & BACKUP_MASK_VDETOFF)) {
> > > > +        if (s->supply_voltage < 2.0f) {
> > > > +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VDET;
> > > > +        }
> > > > +
> > > > +        if (s->supply_voltage < 1.6f) {
> > > > +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VLF;
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > > +/**
> > > > + * Receive a byte of data from i2c
> > > > + * @param i2c the i2c device that is receiving data
> > > > + * @param data the data that was received
> > > > + */
> > > > +static int rx8900_send(I2CSlave *i2c, uint8_t data)
> > > > +{
> > > > +    RX8900State *s = RX8900(i2c);
> > > > +    struct tm now;
> > > > +
> > > > +    trace_rx8900_i2c_data_receive(data);
> > > > +
> > > > +    if (s->addr_byte) {
> > > > +        s->ptr = data & (RX8900_NVRAM_SIZE - 1);
> > > > +        trace_rx8900_regptr_update(s->ptr);
> > > > +        s->addr_byte = false;
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    trace_rx8900_set_register(s->ptr, data);
> > > > +
> > > > +    qemu_get_timedate(&now, s->offset);
> > > > +    switch (s->ptr) {
> > > > +    case SECONDS:
> > > > +    case EXT_SECONDS:
> > > > +        now.tm_sec = from_bcd(data & 0x7f);
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case MINUTES:
> > > > +    case EXT_MINUTES:
> > > > +        now.tm_min = from_bcd(data & 0x7f);
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case HOURS:
> > > > +    case EXT_HOURS:
> > > > +        now.tm_hour = from_bcd(data & 0x3f);
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case WEEKDAY:
> > > > +    case EXT_WEEKDAY: {
> > > > +        int user_wday = ctz32(data);
> > > > +        /* The day field is supposed to contain a value in
> > > > +         * the range 0-6. Otherwise behavior is undefined.
> > > > +         */
> > > > +        switch (data) {
> > > > +        case 0x01:
> > > > +        case 0x02:
> > > > +        case 0x04:
> > > > +        case 0x08:
> > > > +        case 0x10:
> > > > +        case 0x20:
> > > > +        case 0x40:
> > > > +            break;
> > > > +        default:
> > > 
> > > Instead of the switch:
> > > 
> > > if (data & 0x80) {
> > 
> > Weekday is a walking bit, the proposed check allows invalid values.
> 
> 
> Ah, right, should have been if(data==0x80).
> 
> Actually you want to test that
> ((1<<ctz32(data)) == data) && (user_wday < 7)

It's short and clever, but the problem with clever code is it requires
clever people to understand it. I had to stop and think about what that
was doing.

> 
> > 
> > > 
> > > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > > +                "RX8900 - weekday data '%x' is out of range, "
> > > > +                        "undefined behavior will result",
> > > > data);
> > > 
> > > 
> > > btw other cases of switch (s->ptr) do not do such a check, just
> > > "&0x7f" or
> > > "&0x3f", why is the weekday case so special?
> > > 
> > 
> > Weekday is a walking bit, not a number.
> 
> It is still not clear why you check data validity for a weekday but
> not
> other things which you just mask. Has "hours == 0x3f" defined
> behaviour,
> for example?
> 

The values which are masked are BCD encoded, the mask is sufficient for
full validation of value.

> 
> > 
> > > 
> > > 
> > > > +            break;
> > > > +        }
> > > > +        s->weekday = user_wday;
> > > > +        break;
> > > > +    }
> > > > +
> > > > +    case DAY:
> > > > +    case EXT_DAY:
> > > > +        now.tm_mday = from_bcd(data & 0x3f);
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case MONTH:
> > > > +    case EXT_MONTH:
> > > > +        now.tm_mon = from_bcd(data & 0x1f) - 1;
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case YEAR:
> > > > +    case EXT_YEAR:
> > > > +        now.tm_year = from_bcd(data) + 100;
> > > > +        s->offset = qemu_timedate_diff(&now);
> > > > +        break;
> > > > +
> > > > +    case EXTENSION_REGISTER:
> > > > +    case EXT_EXTENSION_REGISTER:
> > > > +        update_extension_register(s, data);
> > > > +        break;
> > > > +
> > > > +    case FLAG_REGISTER:
> > > > +    case EXT_FLAG_REGISTER:
> > > > +        validate_flag_register(s, &data);
> > > > +
> > > > +        s->nvram[FLAG_REGISTER] = data;
> > > > +        s->nvram[EXT_FLAG_REGISTER] = data;
> > > > +
> > > > +        check_voltage(s);
> > > > +        break;
> > > > +
> > > > +    case CONTROL_REGISTER:
> > > > +    case EXT_CONTROL_REGISTER:
> > > > +        update_control_register(s, data);
> > > > +        break;
> > > > +
> > > > +    default:
> > > > +        s->nvram[s->ptr] = data;
> > > > +    }
> > > > +
> > > > +    inc_regptr(s);
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Get the device temperature in Celcius as a property
> > > > + * @param obj the device
> > > > + * @param v
> > > > + * @param name the property name
> > > > + * @param opaque
> > > > + * @param errp an error object to populate on failure
> > > > + */
> > > > +static void rx8900_get_temperature(Object *obj, Visitor *v,
> > > > const
> > > > char *name,
> > > > +                                   void *opaque, Error **errp)
> > > > +{
> > > > +    RX8900State *s = RX8900(obj);
> > > > +    double value = (s->nvram[TEMPERATURE] * 2.0f - 187.1f) /
> > > > 3.218f;
> > > > +
> > > > +    trace_rx8900_read_temperature(s->nvram[TEMPERATURE],
> > > > value);
> > > 
> > > Nit: s/read/get/
> > > 
> > 
> > Ok
> > 
> > > 
> > > > +
> > > > +    visit_type_number(v, name, &value, errp);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Set the device temperature in Celcius as a property
> > > > + * @param obj the device
> > > > + * @param v
> > > > + * @param name the property name
> > > > + * @param opaque
> > > > + * @param errp an error object to populate on failure
> > > > + */
> > > > +static void rx8900_set_temperature(Object *obj, Visitor *v,
> > > > const
> > > > char *name,
> > > > +                                   void *opaque, Error **errp)
> > > > +{
> > > > +    RX8900State *s = RX8900(obj);
> > > > +    Error *local_err = NULL;
> > > > +    double temp; /* degrees Celcius */
> > > > +    visit_type_number(v, name, &temp, &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +    if (temp >= 100 || temp < -58) {
> > > > +        error_setg(errp, "value %f°C is out of range", temp);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s->nvram[TEMPERATURE] = (uint8_t) ((temp * 3.218f +
> > > > 187.19f) /
> > > > 2);
> > > > +
> > > > +    trace_rx8900_set_temperature(s->nvram[TEMPERATURE], temp);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Get the device supply voltage as a property
> > > > + * @param obj the device
> > > > + * @param v
> > > > + * @param name the property name
> > > > + * @param opaque
> > > > + * @param errp an error object to populate on failure
> > > > + */
> > > > +static void rx8900_get_voltage(Object *obj, Visitor *v, const
> > > > char
> > > > *name,
> > > > +                                   void *opaque, Error **errp)
> > > > +{
> > > > +    RX8900State *s = RX8900(obj);
> > > > +
> > > > +    visit_type_number(v, name, &s->supply_voltage, errp);
> > > 
> > > 
> > > rx8900_get_temperature() got a trace point, this one did not ;)
> > > 
> > 
> > This did not perform a transformation on the data.
> > 
> > > > +}
> > > > +
> > > > +/**
> > > > + * Set the device supply voltage as a property
> > > > + * @param obj the device
> > > > + * @param v
> > > > + * @param name the property name
> > > > + * @param opaque
> > > > + * @param errp an error object to populate on failure
> > > > + */
> > > > +static void rx8900_set_voltage(Object *obj, Visitor *v, const
> > > > char
> > > > *name,
> > > > +                                   void *opaque, Error **errp)
> > > > +{
> > > > +    RX8900State *s = RX8900(obj);
> > > > +    Error *local_err = NULL;
> > > > +    double temp;
> > > > +    visit_type_number(v, name, &temp, &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s->supply_voltage = temp;
> > > > +    trace_rx8900_set_voltage(s->supply_voltage);
> > > > +
> > > > +    check_voltage(s);
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + * Configure device properties
> > > > + * @param obj the device
> > > > + */
> > > > +static void rx8900_initfn(Object *obj)
> > > > +{
> > > > +    object_property_add(obj, "temperature", "number",
> > > > +                        rx8900_get_temperature,
> > > > +                        rx8900_set_temperature, NULL, NULL,
> > > > NULL);
> > > > +
> > > > +    object_property_add(obj, "supply voltage", "number",
> > > 
> > > s/supply voltage/voltage/
> > > As having spaces in a property name makes it harder to use in the
> > > QEMU cli
> > > and there is just one voltage so "supply" is not really needed.
> > > 
> > 
> > Ok
> > 
> > > 
> > > 
> > > > +                        rx8900_get_voltage,
> > > > +                        rx8900_set_voltage, NULL, NULL, NULL);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Reset the device
> > > > + * @param dev the RX8900 device to reset
> > > > + */
> > > > +static void rx8900_reset(DeviceState *dev)
> > > > +{
> > > > +    RX8900State *s = RX8900(dev);
> > > > +
> > > > +    trace_rx8900_reset();
> > > > +
> > > > +    /* The clock is running and synchronized with the host */
> > > > +    s->offset = 0;
> > > > +    s->weekday = 7; /* Set to an invalid value */
> > > > +
> > > > +    s->nvram[EXTENSION_REGISTER] = EXT_MASK_TSEL1;
> > > > +    s->nvram[CONTROL_REGISTER] = CTRL_MASK_CSEL0;
> > > > +    s->nvram[FLAG_REGISTER] &= FLAG_MASK_VDET | FLAG_MASK_VLF;
> > > > +
> > > > +    s->ptr = 0;
> > > > +
> > > > +    trace_rx8900_regptr_update(s->ptr);
> > > > +
> > > > +    s->addr_byte = false;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Realize an RX8900 device instance
> > > > + * Set up timers
> > > > + * Configure GPIO lines
> > > > + * @param dev the device instance to realize
> > > > + * @param errp an error object to populate on error
> > > > + */
> > > > +static void rx8900_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    RX8900State *s = RX8900(dev);
> > > > +    I2CSlave *i2c = I2C_SLAVE(dev);
> > > > +    QEMUBH *bh;
> > > > +    char name[64];
> > > > +
> > > > +    s->fout = false;
> > > > +
> > > > +    memset(s->nvram, 0, RX8900_NVRAM_SIZE);
> > > > +    /* Temperature formulation from the datasheet
> > > > +     * ( TEMP[ 7:0 ] * 2 - 187.19) / 3.218
> > > > +     *
> > > > +     * Set the initial state to 25 degrees Celcius
> > > > +     */
> > > > +    s->nvram[TEMPERATURE] = 135; /* (25 * 3.218 + 187.19) / 2
> > > > */
> > > 
> > > 
> > > May be
> > > #define RX8900_C_TO_TEMP(c)       (((c) * 3.218 + 187.19) / 2)
> > > ?
> > > 
> > > You do this math in few places.
> > 
> > Ok
> > 
> > > 
> > > > +
> > > > +    bh = qemu_bh_new(rx8900_timer_tick, s);
> > > > +    s->sec_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> > > > +    /* we trigger the timer at 10Hz and check for rollover, as
> > > > the
> > > > qemu
> > > > +     * clock does not advance in realtime in the test
> > > > environment,
> > > > +     * leading to unstable test results
> > > > +     */
> > > > +    ptimer_set_freq(s->sec_timer, 10);
> > > > +    ptimer_set_limit(s->sec_timer, 1, 1);
> > > > +
> > > > +    bh = qemu_bh_new(rx8900_fout_tick, s);
> > > > +    s->fout_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> > > > +    /* frequency doubled to generate 50% duty cycle square
> > > > wave */
> > > > +    ptimer_set_freq(s->fout_timer, 32768 * 2);
> > > > +    ptimer_set_limit(s->fout_timer, 1, 1);
> > > > +
> > > > +    bh = qemu_bh_new(rx8900_countdown_tick, s);
> > > > +    s->countdown_timer = ptimer_init(bh,
> > > > PTIMER_POLICY_DEFAULT);
> > > > +    ptimer_set_freq(s->countdown_timer, 4096);
> > > > +    ptimer_set_limit(s->countdown_timer, 4096, 1);
> > > > +
> > > > +
> > > 
> > > 
> > > An extra empty line.
> > > 
> > 
> > This is intentional, to separate the different operations.
> 
> One is enough though. If you think the next block is so separate,
> then give
> it a comment.

True

> 
> > 
> > > > +    snprintf(name, sizeof(name), "rx8900-interrupt-out");
> > > > +    qdev_init_gpio_out_named(&i2c->qdev, &s->interrupt_pin,
> > > > name,
> > > > 1);
> > > > +    trace_rx8900_pin_name("Interrupt", name);
> > > 
> > > I would just pass a string to qdev_init_gpio_out_named() and
> > > ditch
> > > @name
> > > from tracepoints as they use unique strings anyway. And I'd also
> > 
> > The same trace is reused
> 
> The first parameter of reused trace is unique anyway.
> 
Yes, but the name value-adds.

> 
> > 
> > > ditch
> > > trace_rx8900_pin_name tracepoints as they do not seem very useful
> > > -
> > > they do
> > > not report an error or a success.
> > 
> > It makes it easier when debugging to validate that your idea of the
> > named interrupt matches the implementation.
> 
> I am missing the point here. If the trace printed a string from i2c-
> >qdev,
> then yes, the trace could be used to tell if the actual name matches
> what
> you passed to qdev_init_gpio_in_named() but in this code the trace
> will
> always print the string you snprintf() 2 lines above. Quite useless.
> 

It was useful to me in development, it may be useful in the future.
Think of it as an announcement of "this is where you can find me".

> > > > +
> > > > +    snprintf(name, sizeof(name), "rx8900-fout-enable");
> > > > +    qdev_init_gpio_in_named(&i2c->qdev,
> > > > rx8900_fout_enable_handler, name, 1);
> > > > +    trace_rx8900_pin_name("Fout-enable", name);
> > > > +
> > > > +    snprintf(name, sizeof(name), "rx8900-fout");
> > > > +    qdev_init_gpio_out_named(&i2c->qdev, &s->fout_pin, name,
> > > > 1);
> > > > +    trace_rx8900_pin_name("Fout", name);
> > > > +
> > > > +    s->supply_voltage = 3.3f;
> > > > +    trace_rx8900_set_voltage(s->supply_voltage);
> > > > +}
> > > > +
> > > > +/**
> > > > + * Set up the device callbacks
> > > > + * @param klass the device class
> > > > + * @param data
> > > > + */
> > > > +static void rx8900_class_init(ObjectClass *klass, void *data)
> > > > +{
> > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> > > > +
> > > > +    k->event = rx8900_event;
> > > > +    k->recv = rx8900_recv;
> > > > +    k->send = rx8900_send;
> > > > +    dc->realize = rx8900_realize;
> > > > +    dc->reset = rx8900_reset;
> > > > +    dc->vmsd = &vmstate_rx8900;
> > > > +}
> > > > +
> > > > +static const TypeInfo rx8900_info = {
> > > > +    .name = TYPE_RX8900,
> > > > +    .parent = TYPE_I2C_SLAVE,
> > > > +    .instance_size = sizeof(RX8900State),
> > > > +    .instance_init = rx8900_initfn,
> > > > +    .class_init = rx8900_class_init,
> > > > +};
> > > > +
> > > > +/**
> > > > + * Register the device with QEMU
> > > > + */
> > > > +static void rx8900_register_types(void)
> > > > +{
> > > > +    type_register_static(&rx8900_info);
> > > > +}
> > > > +
> > > > +type_init(rx8900_register_types)
> > > > diff --git a/hw/timer/rx8900_regs.h b/hw/timer/rx8900_regs.h
> > > > new file mode 100644
> > > > index 0000000..cfec535
> > > > --- /dev/null
> > > > +++ b/hw/timer/rx8900_regs.h
> > > 
> > > Why cannot the content of this header go to hw/timer/rx8900.c ?
> > > Do
> > > you
> > > expect some future code to use these definitions? If so, please
> > > add a
> > > note
> > > to the commit log.
> > > 
> > 
> > These are shared with the test code.
> 
> Ah, my bad, I did grep and somehow missed tests/rx8900-test.c.
> 
> 
> > 
> > > 
> > > > @@ -0,0 +1,139 @@
> > > > +/*
> > > > + * Epson RX8900SA/CE Realtime Clock Module
> > > > + *
> > > > + * Copyright (c) 2016 IBM Corporation
> > > > + * Authors:
> > > > + *  Alastair D'Silva <address@hidden>
> > > > + *
> > > > + * This code is licensed under the GPL version 2 or
> > > > later.  See
> > > > + * the COPYING file in the top-level directory.
> > > > + *
> > > > + * Datasheet available at:
> > > > + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX89
> > > > 00CE
> > > > &lang=en
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef RX8900_REGS_H
> > > > +#define RX8900_REGS_H
> > > > +
> > > > +#include "qemu/bitops.h"
> > > > +
> > > > +#define RX8900_NVRAM_SIZE 0x20
> > > > +
> > > > +typedef enum RX8900Addresses {
> > > > +    SECONDS = 0x00,
> > > > +    MINUTES = 0x01,
> > > > +    HOURS = 0x02,
> > > > +    WEEKDAY = 0x03,
> > > > +    DAY = 0x04,
> > > > +    MONTH = 0x05,
> > > > +    YEAR = 0x06,
> > > > +    RAM = 0x07,
> > > > +    ALARM_MINUTE = 0x08,
> > > > +    ALARM_HOUR = 0x09,
> > > > +    ALARM_WEEK_DAY = 0x0A,
> > > > +    TIMER_COUNTER_0 = 0x0B,
> > > > +    TIMER_COUNTER_1 = 0x0C,
> > > > +    EXTENSION_REGISTER = 0x0D,
> > > > +    FLAG_REGISTER = 0X0E,
> > > > +    CONTROL_REGISTER = 0X0F,
> > > > +    EXT_SECONDS = 0x010, /* Alias of SECONDS */
> > > > +    EXT_MINUTES = 0x11, /* Alias of MINUTES */
> > > > +    EXT_HOURS = 0x12, /* Alias of HOURS */
> > > > +    EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
> > > > +    EXT_DAY = 0x14, /* Alias of DAY */
> > > > +    EXT_MONTH = 0x15, /* Alias of MONTH */
> > > > +    EXT_YEAR = 0x16, /* Alias of YEAR */
> > > > +    TEMPERATURE = 0x17,
> > > > +    BACKUP_FUNCTION = 0x18,
> > > > +    NO_USE_1 = 0x19,
> > > > +    NO_USE_2 = 0x1A,
> > > > +    EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
> > > > +    EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
> > > > +    EXT_EXTENSION_REGISTER = 0x1D, /* Alias of
> > > > EXTENSION_REGISTER
> > > > */
> > > > +    EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
> > > > +    EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER
> > > > */
> > > > +} RX8900Addresses;
> > > > +
> > > > +typedef enum ExtRegBits {
> > > > +    EXT_REG_TSEL0 = 0,
> > > > +    EXT_REG_TSEL1 = 1,
> > > > +    EXT_REG_FSEL0 = 2,
> > > > +    EXT_REG_FSEL1 = 3,
> > > > +    EXT_REG_TE = 4,
> > > > +    EXT_REG_USEL = 5,
> > > > +    EXT_REG_WADA = 6,
> > > > +    EXT_REG_TEST = 7
> > > > +} ExtRegBits;
> > > > +
> > > > +typedef enum ExtRegMasks {
> > > > +    EXT_MASK_TSEL0 = BIT(0),
> > > > +    EXT_MASK_TSEL1 = BIT(1),
> > > > +    EXT_MASK_FSEL0 = BIT(2),
> > > > +    EXT_MASK_FSEL1 = BIT(3),
> > > > +    EXT_MASK_TE = BIT(4),
> > > > +    EXT_MASK_USEL = BIT(5),
> > > > +    EXT_MASK_WADA = BIT(6),
> > > > +    EXT_MASK_TEST = BIT(7)
> > > > +} ExtRegMasks;
> > > > +
> > > > +typedef enum CtrlRegBits {
> > > > +    CTRL_REG_RESET = 0,
> > > > +    CTRL_REG_WP0 = 1,
> > > > +    CTRL_REG_WP1 = 2,
> > > > +    CTRL_REG_AIE = 3,
> > > > +    CTRL_REG_TIE = 4,
> > > > +    CTRL_REG_UIE = 5,
> > > > +    CTRL_REG_CSEL0 = 6,
> > > > +    CTRL_REG_CSEL1 = 7
> > > > +} CtrlRegBits;
> > > > +
> > > > +typedef enum CtrlRegMask {
> > > > +    CTRL_MASK_RESET = BIT(0),
> > > > +    CTRL_MASK_WP0 = BIT(1),
> > > > +    CTRL_MASK_WP1 = BIT(2),
> > > > +    CTRL_MASK_AIE = BIT(3),
> > > > +    CTRL_MASK_TIE = BIT(4),
> > > > +    CTRL_MASK_UIE = BIT(5),
> > > > +    CTRL_MASK_CSEL0 = BIT(6),
> > > > +    CTRL_MASK_CSEL1 = BIT(7)
> > > > +} CtrlRegMask;
> > > > +
> > > > +typedef enum FlagRegBits {
> > > > +    FLAG_REG_VDET = 0,
> > > > +    FLAG_REG_VLF = 1,
> > > > +    FLAG_REG_UNUSED_2 = 2,
> > > > +    FLAG_REG_AF = 3,
> > > > +    FLAG_REG_TF = 4,
> > > > +    FLAG_REG_UF = 5,
> > > > +    FLAG_REG_UNUSED_6 = 6,
> > > > +    FLAG_REG_UNUSED_7 = 7
> > > > +} FlagRegBits;
> > > > +
> > > > +#define RX8900_INTERRUPT_SOURCES 6
> > > > +typedef enum FlagRegMask {
> > > > +    FLAG_MASK_VDET = BIT(0),
> > > > +    FLAG_MASK_VLF = BIT(1),
> > > > +    FLAG_MASK_UNUSED_2 = BIT(2),
> > > > +    FLAG_MASK_AF = BIT(3),
> > > > +    FLAG_MASK_TF = BIT(4),
> > > > +    FLAG_MASK_UF = BIT(5),
> > > > +    FLAG_MASK_UNUSED_6 = BIT(6),
> > > > +    FLAG_MASK_UNUSED_7 = BIT(7)
> > > > +} FlagRegMask;
> > > > +
> > > > +typedef enum BackupRegBits {
> > > > +    BACKUP_REG_BKSMP0 = 0,
> > > > +    BACKUP_REG_BKSMP1 = 1,
> > > > +    BACKUP_REG_SWOFF = 2,
> > > > +    BACKUP_REG_VDETOFF = 3
> > > > +} BackupRegBits;
> > > > +
> > > > +typedef enum BackupRegMask {
> > > > +    BACKUP_MASK_BKSMP0 = BIT(0),
> > > > +    BACKUP_MASK_BKSMP1 = BIT(1),
> > > > +    BACKUP_MASK_SWOFF = BIT(2),
> > > > +    BACKUP_MASK_VDETOFF = BIT(3)
> > > > +} BackupRegMask;
> > > > +
> > > > +#endif
> > > > diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> > > > index 3495c41..057e414 100644
> > > > --- a/hw/timer/trace-events
> > > > +++ b/hw/timer/trace-events
> > > > @@ -49,3 +49,34 @@ aspeed_timer_ctrl_pulse_enable(uint8_t i,
> > > > bool
> > > > enable) "Timer %" PRIu8 ": %d"
> > > >  aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32
> > > >  aspeed_timer_set_value(int timer, int reg, uint32_t value)
> > > > "Timer
> > > > %d register %d: 0x%" PRIx32
> > > >  aspeed_timer_read(uint64_t offset, unsigned size, uint64_t
> > > > value)
> > > > "From 0x%" PRIx64 ": of size %u: 0x%" PRIx64
> > > > +
> > > > +# hw/timer/rx8900.c
> > > > +rx8900_capture_current_time(int hour, int minute, int second,
> > > > int
> > > > weekday, int mday, int month, int year, int raw_hours, int
> > > > raw_minutes, int raw_seconds, int raw_weekday, int raw_day, int
> > > > raw_month, int raw_year) "Update current time to %02d:%02d:%02d
> > > > %d
> > > > %d/%d/%d (0x%02x%02x%02x%02x%02x%02x%02x)"
> > > > +rx8900_regptr_update(uint32_t ptr) "Operating on register
> > > > 0x%02x"
> > > > +rx8900_regptr_overflow(void) "Register pointer has overflowed,
> > > > wrapping to 0"
> > > > +rx8900_event_weekday(int weekday, int weekmask, int
> > > > weekday_offset) "Set weekday to %d (0x%02x), wday_offset=%d"
> > > > +rx8900_read_register(int address, int val) "Read register 0x%x
> > > > =
> > > > 0x%x"
> > > > +rx8900_set_fout(int hz) "Setting fout to %dHz"
> > > > +rx8900_set_countdown_timer(int hz) "Setting countdown timer to
> > > > %d
> > > > Hz"
> > > > +rx8900_set_countdown_timer_per_minute(void) "Setting countdown
> > > > timer to per minute updates"
> > > > +rx8900_enable_update_timer(void) "Enabling update timer"
> > > > +rx8900_enable_alarm(void) "Enabling alarm"
> > > > +rx8900_trigger_alarm(void) "Triggering alarm"
> > > > +rx8900_tick(void) "Tick"
> > > 
> > > When traces are printed, the whole name is printed as well so you
> > > can
> > > easily drop "Tick" and just make it an empty string:
> > > 
> > > +rx8900_tick(void) ""
> > > 
> > > This can be done to more than a half of traces.
> > 
> > This would make it harder to interpret, as one would then not be
> > able
> > to skim down the message column, but would instead have to jump
> > between
> > the location & message to determine what happened.
> 
> Jump between columns? These are printed in stderr like:
> 
> address@hidden:spapr_pci_msi_setup dev"nec-usb-xhci" vector
> 15,
> addr=40000000000
> address@hidden:spapr_pci_rtas_ibm_change_msi cfgaddr 0 func
> 4,
> requested 16, first irq 4104
> 
> I configure traces as:
> --enable-trace-backend=log
> 
> or (for older QEMUs)
> --enable-trace-backend=stderr
> 
Ok

> 
> > > > +rx8900_fire_interrupt(void) "Pulsing interrupt"
> > > > +rx8900_disable_timer(void) "Disabling timer"
> > > > +rx8900_enable_timer(void) "Enabling timer"
> > > > +rx8900_disable_fout(void) "Disabling fout"
> > > > +rx8900_enable_fout(void) "Enabling fout"
> > > > +rx8900_fout_toggle(void) "Toggling fout"
> > > > +rx8900_disable_countdown(void) "Disabling countdown timer"
> > > > +rx8900_enable_countdown(void) "Enabling countdown timer"
> > > > +rx8900_countdown_tick(int count) "Countdown tick, count=%d"
> > > > +rx8900_countdown_elapsed(void) "Countdown elapsed"
> > > > +rx8900_i2c_data_receive(uint8_t data) "Received I2C data
> > > > 0x%02x"
> > > > +rx8900_set_register(uint32_t addr, uint8_t data) "Set data
> > > > 0x%02x=0x%02x"
> > > > +rx8900_read_temperature(uint8_t raw, double val) "Read
> > > > temperature
> > > > property, 0x%x = %f°C"
> > > 
> > > Can be just "0x%x %f°C"
> > > 
> > 
> > I don't see that as an improvement.
> 
> Words "read" and "temperature" do not appear twice, the word
> "property" is
> useless here, the line gets shorter which matters when you run many
> terminals next to each other. This is an improvement.

Due to the wrapping, all I saw was that the '=' was missing, Ok.

> 
> Look at block/trace-events or ./trace-events or net/trace-events -
> people
> usually try avoiding duplications.
> 
> 
> > 
> > > 
> > > > +rx8900_set_temperature(uint8_t raw, double val) "Set
> > > > temperature
> > > > property, 0x%x = %f°C"
> > > > +rx8900_reset(void) "Reset"
> > > > +rx8900_pin_name(const char *type, const char *name) "'%s' pin
> > > > is
> > > > '%s'"
> > > > +rx8900_set_voltage(double voltage) "Device voltage set to %f"
> > > > 
> > 
> > Thanks for the review, it's quite detailed (which is great). I've
> > actioned the non-contended recommendations and will resubmit.
> 
> 
> 


reply via email to

[Prev in Thread] Current Thread [Next in Thread]