Add a simple i2c peripheral to be used for testing I2C device
models. The peripheral has a fixed number of registers that can be
read and written.

Signed-off-by: Octavian Purdila <tavip@google.com>
---
 include/hw/misc/i2c_tester.h |  30 ++++++++++
 hw/misc/i2c_tester.c         | 109 +++++++++++++++++++++++++++++++++++
 hw/misc/Kconfig              |   5 ++
 hw/misc/meson.build          |   2 +
 4 files changed, 146 insertions(+)
 create mode 100644 include/hw/misc/i2c_tester.h
 create mode 100644 hw/misc/i2c_tester.c

diff --git a/include/hw/misc/i2c_tester.h b/include/hw/misc/i2c_tester.h
new file mode 100644
index 0000000000..f6b6491008
--- /dev/null
+++ b/include/hw/misc/i2c_tester.h
@@ -0,0 +1,30 @@
+/*
+ *
+ * Copyright (c) 2024 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_I2C_TESTER_H
+#define HW_I2C_TESTER_H
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "hw/irq.h"
+
+#define I2C_TESTER_NUM_REGS    0x31
+
+#define TYPE_I2C_TESTER "i2c-tester"
+#define I2C_TESTER(obj) OBJECT_CHECK(I2cTesterState, (obj), TYPE_I2C_TESTER)
+
+typedef struct {
+    I2CSlave i2c;
+    bool set_reg_idx;
+    uint8_t reg_idx;
+    uint8_t regs[I2C_TESTER_NUM_REGS];
+} I2cTesterState;
+
+#endif /* HW_I2C_TESTER_H */
diff --git a/hw/misc/i2c_tester.c b/hw/misc/i2c_tester.c
new file mode 100644
index 0000000000..77ce8bf91a
--- /dev/null
+++ b/hw/misc/i2c_tester.c
@@ -0,0 +1,109 @@
+/*
+ * Simple I2C peripheral for testing I2C device models.
+ *
+ * Copyright (c) 2024 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/misc/i2c_tester.h"
+
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "migration/vmstate.h"
+
+static void i2c_tester_reset_enter(Object *o, ResetType type)
+{
+    I2cTesterState *s = I2C_TESTER(o);
+
+    s->set_reg_idx = false;
+    s->reg_idx     = 0;
+    memset(s->regs, 0, I2C_TESTER_NUM_REGS);
+}
+
+static int i2c_tester_event(I2CSlave *i2c, enum i2c_event event)
+{
+    I2cTesterState *s = I2C_TESTER(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->set_reg_idx = true;
+    }
+
+    return 0;
+}
+
+static uint8_t i2c_tester_rx(I2CSlave *i2c)
+{
+    I2cTesterState *s = I2C_TESTER(i2c);
+
+    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
+                      s->reg_idx);
+        return I2C_NACK;
+    }
+
+    return s->regs[s->reg_idx];
+}
+
+static int i2c_tester_tx(I2CSlave *i2c, uint8_t data)
+{
+    I2cTesterState *s = I2C_TESTER(i2c);
+
+    if (s->set_reg_idx) {
+        /* Setting the register in which the operation will be done. */
+        s->reg_idx = data;
+        s->set_reg_idx = false;
+        return 0;
+    }
+
+    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
+                      s->reg_idx);
+        return I2C_NACK;
+    }
+
+    /* Write reg data. */
+    s->regs[s->reg_idx] = data;
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_i2c_tester = {
+    .name = "i2c-tester",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_I2C_SLAVE(i2c, I2cTesterState),
+        VMSTATE_BOOL(set_reg_idx, I2cTesterState),
+        VMSTATE_UINT8(reg_idx, I2cTesterState),
+        VMSTATE_UINT8_ARRAY(regs, I2cTesterState, I2C_TESTER_NUM_REGS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void i2c_tester_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    ResettableClass *rc = RESETTABLE_CLASS(oc);
+    I2CSlaveClass *isc = I2C_SLAVE_CLASS(oc);
+
+    rc->phases.enter = i2c_tester_reset_enter;
+    dc->vmsd = &vmstate_i2c_tester;
+    isc->event = i2c_tester_event;
+    isc->recv = i2c_tester_rx;
+    isc->send = i2c_tester_tx;
+}
+
+static const TypeInfo i2c_tester_types[] = {
+    {
+        .name = TYPE_I2C_TESTER,
+        .parent = TYPE_I2C_SLAVE,
+        .instance_size = sizeof(I2cTesterState),
+        .class_init = i2c_tester_class_init
+    },
+};
+
+DEFINE_TYPES(i2c_tester_types);
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 4b688aead2..3e93c12c8e 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -213,6 +213,11 @@ config IOSB
 config XLNX_VERSAL_TRNG
     bool
 
+config I2C_TESTER
+    bool
+    default y if TEST_DEVICES
+    depends on I2C
+
 config FLEXCOMM
     bool
     select I2C
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index faaf2671ba..4f22231fa3 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -158,6 +158,8 @@ system_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c'))
 # HPPA devices
 system_ss.add(when: 'CONFIG_LASI', if_true: files('lasi.c'))
 
+system_ss.add(when: 'CONFIG_I2C_TESTER', if_true: files('i2c_tester.c'))
+
 system_ss.add(when: 'CONFIG_FLEXCOMM', if_true: files('flexcomm.c'))
 system_ss.add(when: 'CONFIG_RT500_CLKCTL', if_true: files('rt500_clkctl0.c', 'rt500_clkctl1.c'))
 system_ss.add(when: 'CONFIG_RT500_RSTCTL', if_true: files('rt500_rstctl.c'))
-- 
2.46.0.662.g92d0881bb0-goog

On Wed, Sep 18, 2024 at 12:22:47PM -0700, Octavian Purdila wrote:
> Add a simple i2c peripheral to be used for testing I2C device
> models. The peripheral has a fixed number of registers that can be
> read and written.

Why is this better than just using the eeprom device?

This has some uncommon attributes compared to most i2c devices.  For
instance, most i2c devices take their address as the first bytes of a
write operation, then auto-increment after that for reads and writes.
This seems to take one address on a write after a system reset, then use
that forever.

Anyway, unless there is a compelling reason to use this over the eeprom
device, I'm going to recommend against it.

-corey

> 
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
>  include/hw/misc/i2c_tester.h |  30 ++++++++++
>  hw/misc/i2c_tester.c         | 109 +++++++++++++++++++++++++++++++++++
>  hw/misc/Kconfig              |   5 ++
>  hw/misc/meson.build          |   2 +
>  4 files changed, 146 insertions(+)
>  create mode 100644 include/hw/misc/i2c_tester.h
>  create mode 100644 hw/misc/i2c_tester.c
> 
> diff --git a/include/hw/misc/i2c_tester.h b/include/hw/misc/i2c_tester.h
> new file mode 100644
> index 0000000000..f6b6491008
> --- /dev/null
> +++ b/include/hw/misc/i2c_tester.h
> @@ -0,0 +1,30 @@
> +/*
> + *
> + * Copyright (c) 2024 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_I2C_TESTER_H
> +#define HW_I2C_TESTER_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/irq.h"
> +
> +#define I2C_TESTER_NUM_REGS    0x31
> +
> +#define TYPE_I2C_TESTER "i2c-tester"
> +#define I2C_TESTER(obj) OBJECT_CHECK(I2cTesterState, (obj), TYPE_I2C_TESTER)
> +
> +typedef struct {
> +    I2CSlave i2c;
> +    bool set_reg_idx;
> +    uint8_t reg_idx;
> +    uint8_t regs[I2C_TESTER_NUM_REGS];
> +} I2cTesterState;
> +
> +#endif /* HW_I2C_TESTER_H */
> diff --git a/hw/misc/i2c_tester.c b/hw/misc/i2c_tester.c
> new file mode 100644
> index 0000000000..77ce8bf91a
> --- /dev/null
> +++ b/hw/misc/i2c_tester.c
> @@ -0,0 +1,109 @@
> +/*
> + * Simple I2C peripheral for testing I2C device models.
> + *
> + * Copyright (c) 2024 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/misc/i2c_tester.h"
> +
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "migration/vmstate.h"
> +
> +static void i2c_tester_reset_enter(Object *o, ResetType type)
> +{
> +    I2cTesterState *s = I2C_TESTER(o);
> +
> +    s->set_reg_idx = false;
> +    s->reg_idx     = 0;
> +    memset(s->regs, 0, I2C_TESTER_NUM_REGS);
> +}
> +
> +static int i2c_tester_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    I2cTesterState *s = I2C_TESTER(i2c);
> +
> +    if (event == I2C_START_SEND) {
> +        s->set_reg_idx = true;
> +    }
> +
> +    return 0;
> +}
> +
> +static uint8_t i2c_tester_rx(I2CSlave *i2c)
> +{
> +    I2cTesterState *s = I2C_TESTER(i2c);
> +
> +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> +                      s->reg_idx);
> +        return I2C_NACK;
> +    }
> +
> +    return s->regs[s->reg_idx];
> +}
> +
> +static int i2c_tester_tx(I2CSlave *i2c, uint8_t data)
> +{
> +    I2cTesterState *s = I2C_TESTER(i2c);
> +
> +    if (s->set_reg_idx) {
> +        /* Setting the register in which the operation will be done. */
> +        s->reg_idx = data;
> +        s->set_reg_idx = false;
> +        return 0;
> +    }
> +
> +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> +                      s->reg_idx);
> +        return I2C_NACK;
> +    }
> +
> +    /* Write reg data. */
> +    s->regs[s->reg_idx] = data;
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_i2c_tester = {
> +    .name = "i2c-tester",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(i2c, I2cTesterState),
> +        VMSTATE_BOOL(set_reg_idx, I2cTesterState),
> +        VMSTATE_UINT8(reg_idx, I2cTesterState),
> +        VMSTATE_UINT8_ARRAY(regs, I2cTesterState, I2C_TESTER_NUM_REGS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void i2c_tester_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    ResettableClass *rc = RESETTABLE_CLASS(oc);
> +    I2CSlaveClass *isc = I2C_SLAVE_CLASS(oc);
> +
> +    rc->phases.enter = i2c_tester_reset_enter;
> +    dc->vmsd = &vmstate_i2c_tester;
> +    isc->event = i2c_tester_event;
> +    isc->recv = i2c_tester_rx;
> +    isc->send = i2c_tester_tx;
> +}
> +
> +static const TypeInfo i2c_tester_types[] = {
> +    {
> +        .name = TYPE_I2C_TESTER,
> +        .parent = TYPE_I2C_SLAVE,
> +        .instance_size = sizeof(I2cTesterState),
> +        .class_init = i2c_tester_class_init
> +    },
> +};
> +
> +DEFINE_TYPES(i2c_tester_types);
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 4b688aead2..3e93c12c8e 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -213,6 +213,11 @@ config IOSB
>  config XLNX_VERSAL_TRNG
>      bool
>  
> +config I2C_TESTER
> +    bool
> +    default y if TEST_DEVICES
> +    depends on I2C
> +
>  config FLEXCOMM
>      bool
>      select I2C
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index faaf2671ba..4f22231fa3 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -158,6 +158,8 @@ system_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c'))
>  # HPPA devices
>  system_ss.add(when: 'CONFIG_LASI', if_true: files('lasi.c'))
>  
> +system_ss.add(when: 'CONFIG_I2C_TESTER', if_true: files('i2c_tester.c'))
> +
>  system_ss.add(when: 'CONFIG_FLEXCOMM', if_true: files('flexcomm.c'))
>  system_ss.add(when: 'CONFIG_RT500_CLKCTL', if_true: files('rt500_clkctl0.c', 'rt500_clkctl1.c'))
>  system_ss.add(when: 'CONFIG_RT500_RSTCTL', if_true: files('rt500_rstctl.c'))
> -- 
> 2.46.0.662.g92d0881bb0-goog
> 
>

On Wed, Sep 18, 2024 at 1:06 PM Corey Minyard <corey@minyard.net> wrote:
>
> On Wed, Sep 18, 2024 at 12:22:47PM -0700, Octavian Purdila wrote:
> > Add a simple i2c peripheral to be used for testing I2C device
> > models. The peripheral has a fixed number of registers that can be
> > read and written.
>
> Why is this better than just using the eeprom device?
>

The main reason for adding it is that, AFAICT, there is no i2c slave
device that responds with I2C_NACK during a transaction.

Also, having a dedicated device for testing purposes makes it easier
to add new features than adding it to a real device where it might not
always be possible. I tried to add the NACK functionality but I did
not find one where the datasheet would confirm the behaviour I was
looking for.

> This has some uncommon attributes compared to most i2c devices.  For
> instance, most i2c devices take their address as the first bytes of a
> write operation, then auto-increment after that for reads and writes.
> This seems to take one address on a write after a system reset, then use
> that forever.
>
> Anyway, unless there is a compelling reason to use this over the eeprom
> device, I'm going to recommend against it.
>


> -corey
>
> >
> > Signed-off-by: Octavian Purdila <tavip@google.com>
> > ---
> >  include/hw/misc/i2c_tester.h |  30 ++++++++++
> >  hw/misc/i2c_tester.c         | 109 +++++++++++++++++++++++++++++++++++
> >  hw/misc/Kconfig              |   5 ++
> >  hw/misc/meson.build          |   2 +
> >  4 files changed, 146 insertions(+)
> >  create mode 100644 include/hw/misc/i2c_tester.h
> >  create mode 100644 hw/misc/i2c_tester.c
> >
> > diff --git a/include/hw/misc/i2c_tester.h b/include/hw/misc/i2c_tester.h
> > new file mode 100644
> > index 0000000000..f6b6491008
> > --- /dev/null
> > +++ b/include/hw/misc/i2c_tester.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + *
> > + * Copyright (c) 2024 Google LLC
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef HW_I2C_TESTER_H
> > +#define HW_I2C_TESTER_H
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/irq.h"
> > +
> > +#define I2C_TESTER_NUM_REGS    0x31
> > +
> > +#define TYPE_I2C_TESTER "i2c-tester"
> > +#define I2C_TESTER(obj) OBJECT_CHECK(I2cTesterState, (obj), TYPE_I2C_TESTER)
> > +
> > +typedef struct {
> > +    I2CSlave i2c;
> > +    bool set_reg_idx;
> > +    uint8_t reg_idx;
> > +    uint8_t regs[I2C_TESTER_NUM_REGS];
> > +} I2cTesterState;
> > +
> > +#endif /* HW_I2C_TESTER_H */
> > diff --git a/hw/misc/i2c_tester.c b/hw/misc/i2c_tester.c
> > new file mode 100644
> > index 0000000000..77ce8bf91a
> > --- /dev/null
> > +++ b/hw/misc/i2c_tester.c
> > @@ -0,0 +1,109 @@
> > +/*
> > + * Simple I2C peripheral for testing I2C device models.
> > + *
> > + * Copyright (c) 2024 Google LLC
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "hw/misc/i2c_tester.h"
> > +
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "migration/vmstate.h"
> > +
> > +static void i2c_tester_reset_enter(Object *o, ResetType type)
> > +{
> > +    I2cTesterState *s = I2C_TESTER(o);
> > +
> > +    s->set_reg_idx = false;
> > +    s->reg_idx     = 0;
> > +    memset(s->regs, 0, I2C_TESTER_NUM_REGS);
> > +}
> > +
> > +static int i2c_tester_event(I2CSlave *i2c, enum i2c_event event)
> > +{
> > +    I2cTesterState *s = I2C_TESTER(i2c);
> > +
> > +    if (event == I2C_START_SEND) {
> > +        s->set_reg_idx = true;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static uint8_t i2c_tester_rx(I2CSlave *i2c)
> > +{
> > +    I2cTesterState *s = I2C_TESTER(i2c);
> > +
> > +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> > +                      s->reg_idx);
> > +        return I2C_NACK;
> > +    }
> > +
> > +    return s->regs[s->reg_idx];
> > +}
> > +
> > +static int i2c_tester_tx(I2CSlave *i2c, uint8_t data)
> > +{
> > +    I2cTesterState *s = I2C_TESTER(i2c);
> > +
> > +    if (s->set_reg_idx) {
> > +        /* Setting the register in which the operation will be done. */
> > +        s->reg_idx = data;
> > +        s->set_reg_idx = false;
> > +        return 0;
> > +    }
> > +
> > +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> > +                      s->reg_idx);
> > +        return I2C_NACK;
> > +    }
> > +
> > +    /* Write reg data. */
> > +    s->regs[s->reg_idx] = data;
> > +
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_i2c_tester = {
> > +    .name = "i2c-tester",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (const VMStateField[]) {
> > +        VMSTATE_I2C_SLAVE(i2c, I2cTesterState),
> > +        VMSTATE_BOOL(set_reg_idx, I2cTesterState),
> > +        VMSTATE_UINT8(reg_idx, I2cTesterState),
> > +        VMSTATE_UINT8_ARRAY(regs, I2cTesterState, I2C_TESTER_NUM_REGS),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void i2c_tester_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +    ResettableClass *rc = RESETTABLE_CLASS(oc);
> > +    I2CSlaveClass *isc = I2C_SLAVE_CLASS(oc);
> > +
> > +    rc->phases.enter = i2c_tester_reset_enter;
> > +    dc->vmsd = &vmstate_i2c_tester;
> > +    isc->event = i2c_tester_event;
> > +    isc->recv = i2c_tester_rx;
> > +    isc->send = i2c_tester_tx;
> > +}
> > +
> > +static const TypeInfo i2c_tester_types[] = {
> > +    {
> > +        .name = TYPE_I2C_TESTER,
> > +        .parent = TYPE_I2C_SLAVE,
> > +        .instance_size = sizeof(I2cTesterState),
> > +        .class_init = i2c_tester_class_init
> > +    },
> > +};
> > +
> > +DEFINE_TYPES(i2c_tester_types);
> > diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> > index 4b688aead2..3e93c12c8e 100644
> > --- a/hw/misc/Kconfig
> > +++ b/hw/misc/Kconfig
> > @@ -213,6 +213,11 @@ config IOSB
> >  config XLNX_VERSAL_TRNG
> >      bool
> >
> > +config I2C_TESTER
> > +    bool
> > +    default y if TEST_DEVICES
> > +    depends on I2C
> > +
> >  config FLEXCOMM
> >      bool
> >      select I2C
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index faaf2671ba..4f22231fa3 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -158,6 +158,8 @@ system_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c'))
> >  # HPPA devices
> >  system_ss.add(when: 'CONFIG_LASI', if_true: files('lasi.c'))
> >
> > +system_ss.add(when: 'CONFIG_I2C_TESTER', if_true: files('i2c_tester.c'))
> > +
> >  system_ss.add(when: 'CONFIG_FLEXCOMM', if_true: files('flexcomm.c'))
> >  system_ss.add(when: 'CONFIG_RT500_CLKCTL', if_true: files('rt500_clkctl0.c', 'rt500_clkctl1.c'))
> >  system_ss.add(when: 'CONFIG_RT500_RSTCTL', if_true: files('rt500_rstctl.c'))
> > --
> > 2.46.0.662.g92d0881bb0-goog
> >
> >

On Wed, Sep 18, 2024 at 04:03:12PM -0700, Octavian Purdila wrote:
> On Wed, Sep 18, 2024 at 1:06 PM Corey Minyard <corey@minyard.net> wrote:
> >
> > On Wed, Sep 18, 2024 at 12:22:47PM -0700, Octavian Purdila wrote:
> > > Add a simple i2c peripheral to be used for testing I2C device
> > > models. The peripheral has a fixed number of registers that can be
> > > read and written.
> >
> > Why is this better than just using the eeprom device?
> >
> 
> The main reason for adding it is that, AFAICT, there is no i2c slave
> device that responds with I2C_NACK during a transaction.
> 
> Also, having a dedicated device for testing purposes makes it easier
> to add new features than adding it to a real device where it might not
> always be possible. I tried to add the NACK functionality but I did
> not find one where the datasheet would confirm the behaviour I was
> looking for.

SMBUS devices (like the eeprom) use NACKs as part of the protocol, to
mark the end of a transfer, but that's probably not what you are looking
for.  You are using it to signal an error, which I don't think any
existing device will do.  Real devices, in general, don't NACK anything
for errors, but the protocol allows it.

Somehow in my previous look I completely missed i2c_tester_event(), so
this works like a normal I2C device, except that most of them
auto-increment the index register.  But some don't, so it's fine.

If you could document the rationale for this, it would help a lot, for
others that might want to use it.  Also, I would expect people might
add their own test code to this.  I'm not sure what you can do at the
moment about that, but just a heads up that people will probably hack on
this in the future.

So this is good.

-corey

> 
> > This has some uncommon attributes compared to most i2c devices.  For
> > instance, most i2c devices take their address as the first bytes of a
> > write operation, then auto-increment after that for reads and writes.
> > This seems to take one address on a write after a system reset, then use
> > that forever.
> >
> > Anyway, unless there is a compelling reason to use this over the eeprom
> > device, I'm going to recommend against it.
> >
> 
> 
> > -corey
> >
> > >
> > > Signed-off-by: Octavian Purdila <tavip@google.com>
> > > ---
> > >  include/hw/misc/i2c_tester.h |  30 ++++++++++
> > >  hw/misc/i2c_tester.c         | 109 +++++++++++++++++++++++++++++++++++
> > >  hw/misc/Kconfig              |   5 ++
> > >  hw/misc/meson.build          |   2 +
> > >  4 files changed, 146 insertions(+)
> > >  create mode 100644 include/hw/misc/i2c_tester.h
> > >  create mode 100644 hw/misc/i2c_tester.c
> > >
> > > diff --git a/include/hw/misc/i2c_tester.h b/include/hw/misc/i2c_tester.h
> > > new file mode 100644
> > > index 0000000000..f6b6491008
> > > --- /dev/null
> > > +++ b/include/hw/misc/i2c_tester.h
> > > @@ -0,0 +1,30 @@
> > > +/*
> > > + *
> > > + * Copyright (c) 2024 Google LLC
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#ifndef HW_I2C_TESTER_H
> > > +#define HW_I2C_TESTER_H
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "hw/i2c/i2c.h"
> > > +#include "hw/irq.h"
> > > +
> > > +#define I2C_TESTER_NUM_REGS    0x31
> > > +
> > > +#define TYPE_I2C_TESTER "i2c-tester"
> > > +#define I2C_TESTER(obj) OBJECT_CHECK(I2cTesterState, (obj), TYPE_I2C_TESTER)
> > > +
> > > +typedef struct {
> > > +    I2CSlave i2c;
> > > +    bool set_reg_idx;
> > > +    uint8_t reg_idx;
> > > +    uint8_t regs[I2C_TESTER_NUM_REGS];
> > > +} I2cTesterState;
> > > +
> > > +#endif /* HW_I2C_TESTER_H */
> > > diff --git a/hw/misc/i2c_tester.c b/hw/misc/i2c_tester.c
> > > new file mode 100644
> > > index 0000000000..77ce8bf91a
> > > --- /dev/null
> > > +++ b/hw/misc/i2c_tester.c
> > > @@ -0,0 +1,109 @@
> > > +/*
> > > + * Simple I2C peripheral for testing I2C device models.
> > > + *
> > > + * Copyright (c) 2024 Google LLC
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#include "hw/misc/i2c_tester.h"
> > > +
> > > +#include "qemu/log.h"
> > > +#include "qemu/module.h"
> > > +#include "migration/vmstate.h"
> > > +
> > > +static void i2c_tester_reset_enter(Object *o, ResetType type)
> > > +{
> > > +    I2cTesterState *s = I2C_TESTER(o);
> > > +
> > > +    s->set_reg_idx = false;
> > > +    s->reg_idx     = 0;
> > > +    memset(s->regs, 0, I2C_TESTER_NUM_REGS);
> > > +}
> > > +
> > > +static int i2c_tester_event(I2CSlave *i2c, enum i2c_event event)
> > > +{
> > > +    I2cTesterState *s = I2C_TESTER(i2c);
> > > +
> > > +    if (event == I2C_START_SEND) {
> > > +        s->set_reg_idx = true;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static uint8_t i2c_tester_rx(I2CSlave *i2c)
> > > +{
> > > +    I2cTesterState *s = I2C_TESTER(i2c);
> > > +
> > > +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> > > +                      s->reg_idx);
> > > +        return I2C_NACK;
> > > +    }
> > > +
> > > +    return s->regs[s->reg_idx];
> > > +}
> > > +
> > > +static int i2c_tester_tx(I2CSlave *i2c, uint8_t data)
> > > +{
> > > +    I2cTesterState *s = I2C_TESTER(i2c);
> > > +
> > > +    if (s->set_reg_idx) {
> > > +        /* Setting the register in which the operation will be done. */
> > > +        s->reg_idx = data;
> > > +        s->set_reg_idx = false;
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (s->reg_idx >= I2C_TESTER_NUM_REGS) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reg 0x%02x\n", __func__,
> > > +                      s->reg_idx);
> > > +        return I2C_NACK;
> > > +    }
> > > +
> > > +    /* Write reg data. */
> > > +    s->regs[s->reg_idx] = data;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_i2c_tester = {
> > > +    .name = "i2c-tester",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .fields = (const VMStateField[]) {
> > > +        VMSTATE_I2C_SLAVE(i2c, I2cTesterState),
> > > +        VMSTATE_BOOL(set_reg_idx, I2cTesterState),
> > > +        VMSTATE_UINT8(reg_idx, I2cTesterState),
> > > +        VMSTATE_UINT8_ARRAY(regs, I2cTesterState, I2C_TESTER_NUM_REGS),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static void i2c_tester_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > > +    ResettableClass *rc = RESETTABLE_CLASS(oc);
> > > +    I2CSlaveClass *isc = I2C_SLAVE_CLASS(oc);
> > > +
> > > +    rc->phases.enter = i2c_tester_reset_enter;
> > > +    dc->vmsd = &vmstate_i2c_tester;
> > > +    isc->event = i2c_tester_event;
> > > +    isc->recv = i2c_tester_rx;
> > > +    isc->send = i2c_tester_tx;
> > > +}
> > > +
> > > +static const TypeInfo i2c_tester_types[] = {
> > > +    {
> > > +        .name = TYPE_I2C_TESTER,
> > > +        .parent = TYPE_I2C_SLAVE,
> > > +        .instance_size = sizeof(I2cTesterState),
> > > +        .class_init = i2c_tester_class_init
> > > +    },
> > > +};
> > > +
> > > +DEFINE_TYPES(i2c_tester_types);
> > > diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> > > index 4b688aead2..3e93c12c8e 100644
> > > --- a/hw/misc/Kconfig
> > > +++ b/hw/misc/Kconfig
> > > @@ -213,6 +213,11 @@ config IOSB
> > >  config XLNX_VERSAL_TRNG
> > >      bool
> > >
> > > +config I2C_TESTER
> > > +    bool
> > > +    default y if TEST_DEVICES
> > > +    depends on I2C
> > > +
> > >  config FLEXCOMM
> > >      bool
> > >      select I2C
> > > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > > index faaf2671ba..4f22231fa3 100644
> > > --- a/hw/misc/meson.build
> > > +++ b/hw/misc/meson.build
> > > @@ -158,6 +158,8 @@ system_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c'))
> > >  # HPPA devices
> > >  system_ss.add(when: 'CONFIG_LASI', if_true: files('lasi.c'))
> > >
> > > +system_ss.add(when: 'CONFIG_I2C_TESTER', if_true: files('i2c_tester.c'))
> > > +
> > >  system_ss.add(when: 'CONFIG_FLEXCOMM', if_true: files('flexcomm.c'))
> > >  system_ss.add(when: 'CONFIG_RT500_CLKCTL', if_true: files('rt500_clkctl0.c', 'rt500_clkctl1.c'))
> > >  system_ss.add(when: 'CONFIG_RT500_RSTCTL', if_true: files('rt500_rstctl.c'))
> > > --
> > > 2.46.0.662.g92d0881bb0-goog
> > >
> > >