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

[Corey Minyard <corey@minyard.net>]

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