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 <shen...@gmail.com>: > > > > > > > > > Am 4. März 2025 18:53:10 UTC schrieb Bernhard Beschow <shen...@gmail.com>: > > > > > > > > > > > > Am 23. Februar 2025 11:47:08 UTC schrieb Bernhard Beschow > > > > <shen...@gmail.com>: > > > > > The implementation just allows Linux to determine date and time. > > > > > > > > > > Signed-off-by: Bernhard Beschow <shen...@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 <cminy...@mvista.com> (maintainer:I2C and SMBus) > > At a glance patch LGTM, so: > Reviewed-by: Philippe Mathieu-Daudé <phi...@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 <cminy...@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 <shen...@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 <shen...@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',
>
smime.p7s
Description: S/MIME cryptographic signature
