Re: [PATCH 19/25] hw/misc: add i2c-tester

[Corey Minyard <corey@minyard.net>]

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