On Wed, Sep 18, 2024 at 04:03:12PM -0700, Octavian Purdila wrote:
> On Wed, Sep 18, 2024 at 1:06 PM Corey Minyard <co...@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 <ta...@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
> > >
> > >
