Re: [PATCH v2 18/18] hw/rtc: Add Ricoh RS5C372 RTC emulation

[Corey Minyard <cminyard@mvista.com>]

[-- Attachment #1: Type: text/plain, Size: 16209 bytes --]

On Tue, Mar 11, 2025 at 10:20:03AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/3/25 08:34, Bernhard Beschow wrote:
> > 
> > 
> > Am 7. März 2025 19:18:34 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > > 
> > > 
> > > Am 4. März 2025 18:53:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > > > 
> > > > 
> > > > Am 23. Februar 2025 11:47:08 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > > > > The implementation just allows Linux to determine date and time.
> > > > > 
> > > > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > > > ---
> > > > > MAINTAINERS                |   2 +
> > > > > hw/rtc/rs5c372.c           | 236 +++++++++++++++++++++++++++++++++++++
> > > > > tests/qtest/rs5c372-test.c |  43 +++++++
> > > > > hw/rtc/Kconfig             |   5 +
> > > > > hw/rtc/meson.build         |   1 +
> > > > > hw/rtc/trace-events        |   4 +
> > > > > tests/qtest/meson.build    |   1 +
> > > > > 7 files changed, 292 insertions(+)
> > > > > create mode 100644 hw/rtc/rs5c372.c
> > > > > create mode 100644 tests/qtest/rs5c372-test.c
> > > > 
> > > > Ping for just this patch. I'd like to have it merged for 10.0.
> > > 
> > > Ping^2 -- just few days left before soft freeze.
> > 
> > Last ping before the freeze
> > 
> > It would really be nice to have this device model in 10.0 since this would allow me to use upstream QEMU.
> 
> Apparently I2C maintainer wasn't Cc'ed (now is):
> 
>   Corey Minyard <cminyard@mvista.com> (maintainer:I2C and SMBus)
> 
> At a glance patch LGTM, so:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Well, it's fairly hard to read this way :-).  But this looks good.  My
only comment is on:

>>>> +#define NVRAM_SIZE 0x10
>>>> +
>>>> +/* Flags definitions */
>>>> +#define SECONDS_CH 0x80
>>>> +#define HOURS_PM   0x20
>>>> +#define CTRL2_24   0x20

Those are fairly generic names; when I see things like that I worry
about conflicting with other generic names that might come into an
include later.  Not a huge deal, though.

Acked-by: Corey Minyard <cminyard@mvista.com>

> 
> > 
> > Thanks,
> > Bernhard
> > 
> > > 
> > > AFAICS no open issues and I'd really like to have this RTC merged for 10.0. What is holding it back?
> > > 
> > > Best regards,
> > > Bernhard
> > > 
> > > > 
> > > > Thanks,
> > > > Bernhard
> > > > 
> > > > > 
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index 489e426d85..2552cfd65c 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -828,10 +828,12 @@ F: hw/arm/imx8mp-evk.c
> > > > > F: hw/arm/fsl-imx8mp.c
> > > > > F: hw/misc/imx8mp_*.c
> > > > > F: hw/pci-host/fsl_imx8m_phy.c
> > > > > +F: hw/rtc/rs5c372.c
> > > > > F: include/hw/arm/fsl-imx8mp.h
> > > > > F: include/hw/misc/imx8mp_*.h
> > > > > F: include/hw/pci-host/fsl_imx8m_phy.h
> > > > > F: pc-bios/imx8mp*
> > > > > +F: tests/qtest/rs5c372-test.c
> > > > > F: docs/system/arm/imx8mp-evk.rst
> > > > > 
> > > > > MPS2 / MPS3
> > > > > diff --git a/hw/rtc/rs5c372.c b/hw/rtc/rs5c372.c
> > > > > new file mode 100644
> > > > > index 0000000000..5542f74085
> > > > > --- /dev/null
> > > > > +++ b/hw/rtc/rs5c372.c
> > > > > @@ -0,0 +1,236 @@
> > > > > +/*
> > > > > + * Ricoh RS5C372, R222x I2C RTC
> > > > > + *
> > > > > + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
> > > > > + *
> > > > > + * Based on hw/rtc/ds1338.c
> > > > > + *
> > > > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "hw/i2c/i2c.h"
> > > > > +#include "hw/qdev-properties.h"
> > > > > +#include "hw/resettable.h"
> > > > > +#include "migration/vmstate.h"
> > > > > +#include "qemu/bcd.h"
> > > > > +#include "qom/object.h"
> > > > > +#include "system/rtc.h"
> > > > > +#include "trace.h"
> > > > > +
> > > > > +#define NVRAM_SIZE 0x10
> > > > > +
> > > > > +/* Flags definitions */
> > > > > +#define SECONDS_CH 0x80
> > > > > +#define HOURS_PM   0x20
> > > > > +#define CTRL2_24   0x20
> > > > > +
> > > > > +#define TYPE_RS5C372 "rs5c372"
> > > > > +OBJECT_DECLARE_SIMPLE_TYPE(RS5C372State, RS5C372)
> > > > > +
> > > > > +struct RS5C372State {
> > > > > +    I2CSlave parent_obj;
> > > > > +
> > > > > +    int64_t offset;
> > > > > +    uint8_t wday_offset;
> > > > > +    uint8_t nvram[NVRAM_SIZE];
> > > > > +    uint8_t ptr;
> > > > > +    uint8_t tx_format;
> > > > > +    bool addr_byte;
> > > > > +};
> > > > > +
> > > > > +static void capture_current_time(RS5C372State *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[0] = to_bcd(now.tm_sec);
> > > > > +    s->nvram[1] = to_bcd(now.tm_min);
> > > > > +    if (s->nvram[0xf] & CTRL2_24) {
> > > > > +        s->nvram[2] = to_bcd(now.tm_hour);
> > > > > +    } else {
> > > > > +        int tmp = now.tm_hour;
> > > > > +        if (tmp % 12 == 0) {
> > > > > +            tmp += 12;
> > > > > +        }
> > > > > +        if (tmp <= 12) {
> > > > > +            s->nvram[2] = to_bcd(tmp);
> > > > > +        } else {
> > > > > +            s->nvram[2] = HOURS_PM | to_bcd(tmp - 12);
> > > > > +        }
> > > > > +    }
> > > > > +    s->nvram[3] = (now.tm_wday + s->wday_offset) % 7 + 1;
> > > > > +    s->nvram[4] = to_bcd(now.tm_mday);
> > > > > +    s->nvram[5] = to_bcd(now.tm_mon + 1);
> > > > > +    s->nvram[6] = to_bcd(now.tm_year - 100);
> > > > > +}
> > > > > +
> > > > > +static void inc_regptr(RS5C372State *s)
> > > > > +{
> > > > > +    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
> > > > > +}
> > > > > +
> > > > > +static int rs5c372_event(I2CSlave *i2c, enum i2c_event event)
> > > > > +{
> > > > > +    RS5C372State *s = RS5C372(i2c);
> > > > > +
> > > > > +    switch (event) {
> > > > > +    case I2C_START_RECV:
> > > > > +        /*
> > > > > +         * In h/w, capture happens on any START condition, not just a
> > > > > +         * START_RECV, but there is no need to actually capture on
> > > > > +         * START_SEND, because the guest can't get at that data
> > > > > +         * without going through a START_RECV which would overwrite it.
> > > > > +         */
> > > > > +        capture_current_time(s);
> > > > > +        s->ptr = 0xf;
> > > > > +        break;
> > > > > +    case I2C_START_SEND:
> > > > > +        s->addr_byte = true;
> > > > > +        break;
> > > > > +    default:
> > > > > +        break;
> > > > > +    }
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +static uint8_t rs5c372_recv(I2CSlave *i2c)
> > > > > +{
> > > > > +    RS5C372State *s = RS5C372(i2c);
> > > > > +    uint8_t res;
> > > > > +
> > > > > +    res  = s->nvram[s->ptr];
> > > > > +
> > > > > +    trace_rs5c372_recv(s->ptr, res);
> > > > > +
> > > > > +    inc_regptr(s);
> > > > > +    return res;
> > > > > +}
> > > > > +
> > > > > +static int rs5c372_send(I2CSlave *i2c, uint8_t data)
> > > > > +{
> > > > > +    RS5C372State *s = RS5C372(i2c);
> > > > > +
> > > > > +    if (s->addr_byte) {
> > > > > +        s->ptr = data >> 4;
> > > > > +        s->tx_format = data & 0xf;
> > > > > +        s->addr_byte = false;
> > > > > +        return 0;
> > > > > +    }
> > > > > +
> > > > > +    trace_rs5c372_send(s->ptr, data);
> > > > > +
> > > > > +    if (s->ptr < 7) {
> > > > > +        /* Time register. */
> > > > > +        struct tm now;
> > > > > +        qemu_get_timedate(&now, s->offset);
> > > > > +        switch (s->ptr) {
> > > > > +        case 0:
> > > > > +            now.tm_sec = from_bcd(data & 0x7f);
> > > > > +            break;
> > > > > +        case 1:
> > > > > +            now.tm_min = from_bcd(data & 0x7f);
> > > > > +            break;
> > > > > +        case 2:
> > > > > +            if (s->nvram[0xf] & CTRL2_24) {
> > > > > +                now.tm_hour = from_bcd(data & 0x3f);
> > > > > +            } else {
> > > > > +                int tmp = from_bcd(data & (HOURS_PM - 1));
> > > > > +                if (data & HOURS_PM) {
> > > > > +                    tmp += 12;
> > > > > +                }
> > > > > +                if (tmp % 12 == 0) {
> > > > > +                    tmp -= 12;
> > > > > +                }
> > > > > +                now.tm_hour = tmp;
> > > > > +            }
> > > > > +            break;
> > > > > +        case 3:
> > > > > +            {
> > > > > +                /*
> > > > > +                 * The day field is supposed to contain a value in the range
> > > > > +                 * 1-7. Otherwise behavior is undefined.
> > > > > +                 */
> > > > > +                int user_wday = (data & 7) - 1;
> > > > > +                s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
> > > > > +            }
> > > > > +            break;
> > > > > +        case 4:
> > > > > +            now.tm_mday = from_bcd(data & 0x3f);
> > > > > +            break;
> > > > > +        case 5:
> > > > > +            now.tm_mon = from_bcd(data & 0x1f) - 1;
> > > > > +            break;
> > > > > +        case 6:
> > > > > +            now.tm_year = from_bcd(data) + 100;
> > > > > +            break;
> > > > > +        }
> > > > > +        s->offset = qemu_timedate_diff(&now);
> > > > > +    } else {
> > > > > +        s->nvram[s->ptr] = data;
> > > > > +    }
> > > > > +    inc_regptr(s);
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +static void rs5c372_reset_hold(Object *obj, ResetType type)
> > > > > +{
> > > > > +    RS5C372State *s = RS5C372(obj);
> > > > > +
> > > > > +    /* The clock is running and synchronized with the host */
> > > > > +    s->offset = 0;
> > > > > +    s->wday_offset = 0;
> > > > > +    memset(s->nvram, 0, NVRAM_SIZE);
> > > > > +    s->ptr = 0;
> > > > > +    s->addr_byte = false;
> > > > > +}
> > > > > +
> > > > > +static const VMStateDescription rs5c372_vmstate = {
> > > > > +    .name = "rs5c372",
> > > > > +    .version_id = 1,
> > > > > +    .minimum_version_id = 1,
> > > > > +    .fields = (const VMStateField[]) {
> > > > > +        VMSTATE_I2C_SLAVE(parent_obj, RS5C372State),
> > > > > +        VMSTATE_INT64(offset, RS5C372State),
> > > > > +        VMSTATE_UINT8_V(wday_offset, RS5C372State, 2),
> > > > > +        VMSTATE_UINT8_ARRAY(nvram, RS5C372State, NVRAM_SIZE),
> > > > > +        VMSTATE_UINT8(ptr, RS5C372State),
> > > > > +        VMSTATE_UINT8(tx_format, RS5C372State),
> > > > > +        VMSTATE_BOOL(addr_byte, RS5C372State),
> > > > > +        VMSTATE_END_OF_LIST()
> > > > > +    }
> > > > > +};
> > > > > +
> > > > > +static void rs5c372_init(Object *obj)
> > > > > +{
> > > > > +    qdev_prop_set_uint8(DEVICE(obj), "address", 0x32);
> > > > > +}
> > > > > +
> > > > > +static void rs5c372_class_init(ObjectClass *klass, void *data)
> > > > > +{
> > > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > > +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> > > > > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > > > > +
> > > > > +    k->event = rs5c372_event;
> > > > > +    k->recv = rs5c372_recv;
> > > > > +    k->send = rs5c372_send;
> > > > > +    dc->vmsd = &rs5c372_vmstate;
> > > > > +    rc->phases.hold = rs5c372_reset_hold;
> > > > > +}
> > > > > +
> > > > > +static const TypeInfo rs5c372_types[] = {
> > > > > +    {
> > > > > +        .name          = TYPE_RS5C372,
> > > > > +        .parent        = TYPE_I2C_SLAVE,
> > > > > +        .instance_size = sizeof(RS5C372State),
> > > > > +        .instance_init = rs5c372_init,
> > > > > +        .class_init    = rs5c372_class_init,
> > > > > +    },
> > > > > +};
> > > > > +
> > > > > +DEFINE_TYPES(rs5c372_types)
> > > > > diff --git a/tests/qtest/rs5c372-test.c b/tests/qtest/rs5c372-test.c
> > > > > new file mode 100644
> > > > > index 0000000000..0f6a9b68b9
> > > > > --- /dev/null
> > > > > +++ b/tests/qtest/rs5c372-test.c
> > > > > @@ -0,0 +1,43 @@
> > > > > +/*
> > > > > + * QTest testcase for the RS5C372 RTC
> > > > > + *
> > > > > + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
> > > > > + *
> > > > > + * Based on ds1338-test.c
> > > > > + *
> > > > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "qemu/bcd.h"
> > > > > +#include "libqos/i2c.h"
> > > > > +
> > > > > +#define RS5C372_ADDR 0x32
> > > > > +
> > > > > +static void rs5c372_read_date(void *obj, void *data, QGuestAllocator *alloc)
> > > > > +{
> > > > > +    QI2CDevice *i2cdev = obj;
> > > > > +
> > > > > +    uint8_t resp[0x10];
> > > > > +    time_t now = time(NULL);
> > > > > +    struct tm *utc = gmtime(&now);
> > > > > +
> > > > > +    i2c_read_block(i2cdev, 0, resp, sizeof(resp));
> > > > > +
> > > > > +    /* check retrieved time against local time */
> > > > > +    g_assert_cmpuint(from_bcd(resp[5]), == , utc->tm_mday);
> > > > > +    g_assert_cmpuint(from_bcd(resp[6]), == , 1 + utc->tm_mon);
> > > > > +    g_assert_cmpuint(2000 + from_bcd(resp[7]), == , 1900 + utc->tm_year);
> > > > > +}
> > > > > +
> > > > > +static void rs5c372_register_nodes(void)
> > > > > +{
> > > > > +    QOSGraphEdgeOptions opts = { };
> > > > > +    add_qi2c_address(&opts, &(QI2CAddress) { RS5C372_ADDR });
> > > > > +
> > > > > +    qos_node_create_driver("rs5c372", i2c_device_create);
> > > > > +    qos_node_consumes("rs5c372", "i2c-bus", &opts);
> > > > > +    qos_add_test("read_date", "rs5c372", rs5c372_read_date, NULL);
> > > > > +}
> > > > > +
> > > > > +libqos_init(rs5c372_register_nodes);
> > > > > diff --git a/hw/rtc/Kconfig b/hw/rtc/Kconfig
> > > > > index 2fe04ec1d0..315b0e4ecc 100644
> > > > > --- a/hw/rtc/Kconfig
> > > > > +++ b/hw/rtc/Kconfig
> > > > > @@ -26,3 +26,8 @@ config GOLDFISH_RTC
> > > > > 
> > > > > config LS7A_RTC
> > > > >      bool
> > > > > +
> > > > > +config RS5C372_RTC
> > > > > +    bool
> > > > > +    depends on I2C
> > > > > +    default y if I2C_DEVICES
> > > > > diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
> > > > > index 8ecc2d792c..6c87864dc0 100644
> > > > > --- a/hw/rtc/meson.build
> > > > > +++ b/hw/rtc/meson.build
> > > > > @@ -13,3 +13,4 @@ system_ss.add(when: 'CONFIG_GOLDFISH_RTC', if_true: files('goldfish_rtc.c'))
> > > > > system_ss.add(when: 'CONFIG_LS7A_RTC', if_true: files('ls7a_rtc.c'))
> > > > > system_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-rtc.c'))
> > > > > system_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))
> > > > > +system_ss.add(when: 'CONFIG_RS5C372_RTC', if_true: files('rs5c372.c'))
> > > > > diff --git a/hw/rtc/trace-events b/hw/rtc/trace-events
> > > > > index 8012afe102..b9f2852d35 100644
> > > > > --- a/hw/rtc/trace-events
> > > > > +++ b/hw/rtc/trace-events
> > > > > @@ -35,3 +35,7 @@ m48txx_nvram_mem_write(uint32_t addr, uint32_t value) "mem write addr:0x%04x val
> > > > > # goldfish_rtc.c
> > > > > goldfish_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
> > > > > goldfish_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
> > > > > +
> > > > > +# rs5c372.c
> > > > > +rs5c372_recv(uint32_t addr, uint8_t value) "[0x%" PRIx32 "] -> 0x%02" PRIx8
> > > > > +rs5c372_send(uint32_t addr, uint8_t value) "[0x%" PRIx32 "] <- 0x%02" PRIx8
> > > > > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > > > > index 8a6243382a..9e5380ba7a 100644
> > > > > --- a/tests/qtest/meson.build
> > > > > +++ b/tests/qtest/meson.build
> > > > > @@ -298,6 +298,7 @@ qos_test_ss.add(
> > > > >    'pca9552-test.c',
> > > > >    'pci-test.c',
> > > > >    'pcnet-test.c',
> > > > > +  'rs5c372-test.c',
> > > > >    'sdhci-test.c',
> > > > >    'spapr-phb-test.c',
> > > > >    'tmp105-test.c',
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3365 bytes --]