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

[Octavian Purdila]

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