[PATCH v1 1/2] hw/misc: Implementating dummy AST2600 I3C model
Cédric Le Goater
clg at kaod.org
Fri Dec 24 00:48:23 AEDT 2021
Hello,
On 12/22/21 10:23, Troy Lee wrote:
> Introduce a dummy AST2600 I3C model.
>
> Aspeed 2600 SDK enables I3C support by default. The I3C driver will try
> to reset the device controller and setup through device address table
> register. This dummy model response these register with default value
> listed on ast2600v10 datasheet chapter 54.2. If the device address
> table register doesn't set correctly, it will cause guest machine kernel
> panic due to reference to invalid address.
Overall looks good. Some comments,
>
> Signed-off-by: Troy Lee <troy_lee at aspeedtech.com>
> ---
> hw/misc/aspeed_i3c.c | 258 +++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 1 +
> include/hw/misc/aspeed_i3c.h | 30 ++++
> 3 files changed, 289 insertions(+)
> create mode 100644 hw/misc/aspeed_i3c.c
> create mode 100644 include/hw/misc/aspeed_i3c.h
>
> diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
> new file mode 100644
> index 0000000000..9d2bda203e
> --- /dev/null
> +++ b/hw/misc/aspeed_i3c.c
> @@ -0,0 +1,258 @@
> +/*
> + * ASPEED I3C Controller
> + *
> + * Copyright (C) 2021 ASPEED Technology Inc.
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_i3c.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +
> +/* I3C Controller Registers */
> +#define R_I3CG_REG0(x) (((x * 0x10) + 0x10) / 4)
> +#define I3CG_REG0_SDA_PULLUP_EN_MASK GENMASK(29, 28)
GENMASK() is a macro defined in the FSI model which is not upstream.
There are other ways to define bitfield masks in QEMU. Please take a
look at include/hw/registerfields.h.
> +#define I3CG_REG0_SDA_PULLUP_EN_2K BIT(28)
> +#define I3CG_REG0_SDA_PULLUP_EN_750 BIT(29)
> +#define I3CG_REG0_SDA_PULLUP_EN_545 (BIT(29) | BIT(28))
> +
> +#define R_I3CG_REG1(x) (((x * 0x10) + 0x14) / 4)
> +#define I3CG_REG1_I2C_MODE BIT(0)
> +#define I3CG_REG1_TEST_MODE BIT(1)
> +#define I3CG_REG1_ACT_MODE_MASK GENMASK(3, 2)
> +#define I3CG_REG1_ACT_MODE(x) (((x) << 2) & I3CG_REG1_ACT_MODE_MASK)
> +#define I3CG_REG1_PENDING_INT_MASK GENMASK(7, 4)
> +#define I3CG_REG1_PENDING_INT(x) (((x) << 4) & I3CG_REG1_PENDING_INT_MASK)
> +#define I3CG_REG1_SA_MASK GENMASK(14, 8)
> +#define I3CG_REG1_SA(x) (((x) << 8) & I3CG_REG1_SA_MASK)
> +#define I3CG_REG1_SA_EN BIT(15)
> +#define I3CG_REG1_INST_ID_MASK GENMASK(19, 16)
> +#define I3CG_REG1_INST_ID(x) (((x) << 16) & I3CG_REG1_INST_ID_MASK)
> +
> +/* I3C Device Registers */
> +#define R_DEVICE_CTRL (0x00 / 4)
> +#define R_DEVICE_ADDR (0x04 / 4)
> +#define R_HW_CAPABILITY (0x08 / 4)
> +#define R_COMMAND_QUEUE_PORT (0x0c / 4)
> +#define R_RESPONSE_QUEUE_PORT (0x10 / 4)
> +#define R_RX_TX_DATA_PORT (0x14 / 4)
> +#define R_IBI_QUEUE_STATUS (0x18 / 4)
> +#define R_IBI_QUEUE_DATA (0x18 / 4)
> +#define R_QUEUE_THLD_CTRL (0x1c / 4)
> +#define R_DATA_BUFFER_THLD_CTRL (0x20 / 4)
> +#define R_IBI_QUEUE_CTRL (0x24 / 4)
> +#define R_IBI_MR_REQ_REJECT (0x2c / 4)
> +#define R_IBI_SIR_REQ_REJECT (0x30 / 4)
> +#define R_RESET_CTRL (0x34 / 4)
> +#define R_SLV_EVENT_CTRL (0x38 / 4)
> +#define R_INTR_STATUS (0x3c / 4)
> +#define R_INTR_STATUS_EN (0x40 / 4)
> +#define R_INTR_SIGNAL_EN (0x44 / 4)
> +#define R_INTR_FORCE (0x48 / 4)
> +#define R_QUEUE_STATUS_LEVEL (0x4c / 4)
> +#define R_DATA_BUFFER_STATUS_LEVEL (0x50 / 4)
> +#define R_PRESENT_STATE (0x54 / 4)
> +#define R_CCC_DEVICE_STATUS (0x58 / 4)
> +#define R_DEVICE_ADDR_TABLE_POINTER (0x5c / 4)
> +#define DEVICE_ADDR_TABLE_DEPTH(x) (((x) & GENMASK(31, 16)) >> 16)
> +#define DEVICE_ADDR_TABLE_ADDR(x) ((x) & GENMASK(7, 0))
> +#define R_DEV_CHAR_TABLE_POINTER (0x60 / 4)
> +#define R_VENDOR_SPECIFIC_REG_POINTER (0x6c / 4)
> +#define R_SLV_MIPI_PID_VALUE (0x70 / 4)
> +#define R_SLV_PID_VALUE (0x74 / 4)
> +#define R_SLV_CHAR_CTRL (0x78 / 4)
> +#define R_SLV_MAX_LEN (0x7c / 4)
> +#define R_MAX_READ_TURNAROUND (0x80 / 4)
> +#define R_MAX_DATA_SPEED (0x84 / 4)
> +#define R_SLV_DEBUG_STATUS (0x88 / 4)
> +#define R_SLV_INTR_REQ (0x8c / 4)
> +#define R_DEVICE_CTRL_EXTENDED (0xb0 / 4)
> +#define R_SCL_I3C_OD_TIMING (0xb4 / 4)
> +#define R_SCL_I3C_PP_TIMING (0xb8 / 4)
> +#define R_SCL_I2C_FM_TIMING (0xbc / 4)
> +#define R_SCL_I2C_FMP_TIMING (0xc0 / 4)
> +#define R_SCL_EXT_LCNT_TIMING (0xc8 / 4)
> +#define R_SCL_EXT_TERMN_LCNT_TIMING (0xcc / 4)
> +#define R_BUS_FREE_TIMING (0xd4 / 4)
> +#define R_BUS_IDLE_TIMING (0xd8 / 4)
> +#define R_I3C_VER_ID (0xe0 / 4)
> +#define R_I3C_VER_TYPE (0xe4 / 4)
> +#define R_EXTENDED_CAPABILITY (0xe8 / 4)
> +#define R_SLAVE_CONFIG (0xec / 4)
> +
> +static uint64_t aspeed_i3c_read(void *opaque,
> + hwaddr addr,
> + unsigned int size)
You can avoid the new lines.
> +{
> + AspeedI3CState *s = ASPEED_I3C(opaque);
> + uint64_t val = 0;
> +
> + addr >>= 2;
> +
> + if (addr >= ASPEED_I3C_NR_REGS) {
ASPEED_I3C_NR_REGS is a large value (0x8000 >> 2) which doesn't really
represent the number of registers. I would do thing a bit differently.
See the comment below in realize.
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> + __func__, addr << 2);
> + } else if (addr < 0x800) {
Where does that value come from ?
> + /* I3C controller register */
> + val = s->regs[addr];
> + } else {
> + /* I3C device register */
hmm, I think we need one region per I3C device instead.
> + switch (addr & 0xff) {
> + case R_DEVICE_ADDR_TABLE_POINTER:
> + val = DEVICE_ADDR_TABLE_DEPTH(0x8) | DEVICE_ADDR_TABLE_ADDR(0x280);
> + break;
> + case R_DEV_CHAR_TABLE_POINTER:
> + val = 0x00020200;
> + break;
> + case R_VENDOR_SPECIFIC_REG_POINTER:
> + val = 0x000000b0;
> + break;
> + default:
> + val = s->regs[addr];
> + break;
> + }
> + }
> +
> + printf("I3C Read[%08lx] = [%08lx]\n", addr << 2, val);
Please use trace-events. See trace_aspeed_scu_read/write for example.
> +
> + return val;
> +}
> +
> +static void aspeed_i3c_write(void *opaque,
> + hwaddr addr,
> + uint64_t data,
> + unsigned int size)
same.
> +{
> + AspeedI3CState *s = ASPEED_I3C(opaque);
> +
> + printf("I3C Write[%08lx] = [%08lx]\n", addr, data);
same.
> +
> + addr >>= 2;
> +
> + if (addr >= ASPEED_I3C_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
> + __func__, addr << 2);
> + return;
> + }
> +
> + if (addr < 0x800) {
> + /* I3C controller register */
> + switch (addr) {
> + case R_I3CG_REG1(0):
> + case R_I3CG_REG1(1):
> + case R_I3CG_REG1(2):
> + case R_I3CG_REG1(3):
> + if (data & I3CG_REG1_I2C_MODE) {
> + qemu_log_mask(LOG_UNIMP,
> + "%s: Not support I2C mode [%08lx]=%08lx",
> + __func__, addr << 2, data);
> + break;
> + }
> + if (data & I3CG_REG1_SA_EN) {
> + qemu_log_mask(LOG_UNIMP,
> + "%s: Not support slave mode [%08lx]=%08lx",
> + __func__, addr << 2, data);
> + break;
> + }
> + s->regs[addr] = data;
> + break;
> + default:
> + s->regs[addr] = data;
> + break;
> + }
> + } else {
> + /* I3C device register */
> + switch (addr & 0xff) {
> + case R_RESET_CTRL:
> + s->regs[addr] = 0x00000000;
> + break;
> + default:
> + s->regs[addr] = data;
> + break;
> + }
> + }
> +}
> +
> +static const MemoryRegionOps aspeed_i3c_ops = {
> + .read = aspeed_i3c_read,
> + .write = aspeed_i3c_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid = {
> + .min_access_size = 1,
> + .max_access_size = 4,
> + }
> +};
> +
> +static void aspeed_i3c_reset(DeviceState *dev)
> +{
> + struct AspeedI3CState *s = ASPEED_I3C(dev);
> +
> + memset(s->regs, 0, sizeof(s->regs));
> +
> + /* Init I3C controller register */
> +
> +
> + /* Init I3C devices register */
> + for (int i = 0; i < 4; ++i) {
there are 5 devices. no ?
> + uint32_t dev_base = (0x2000 + i * 0x1000) >> 4;
> + s->regs[dev_base + R_HW_CAPABILITY] = 0x000e00bf;
> + s->regs[dev_base + R_QUEUE_THLD_CTRL] = 0x01000101;
> +
> + s->regs[dev_base + R_I3C_VER_ID] = 0x3130302a;
> + s->regs[dev_base + R_I3C_VER_TYPE] = 0x6c633033;
This is a sign that we need an AspeedI3CDevice model :)
> + }
> +}
> +
> +static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
> +{
> + AspeedI3CState *s = ASPEED_I3C(dev);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> + sysbus_init_irq(sbd, &s->irq);
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s,
> + TYPE_ASPEED_I3C, 0x8000);
I would do things differently with an AspeedI3CDevice, something like :
struct AspeedI3CDevice {
SysBusDevice parent_obj;
MemoryRegion mr;
uint32_t regs[0x300 >> 2];
};
for which you would define the realize and reset handlers like above
but relative to the device only. The read/write ops of the device
region would be simplified.
You will then to introduce an array of AspeedI3CDevice under
AspeedI3CState :
AspeedI3CDevice devices[ASPEED_I3C_NR_DEVICES];
and initialize them in a instance_init handler of AspeedI3CState :
static void aspeed_i3c_instance_init(Object *obj)
{
AspeedI3CState *s = ASPEED_I3C(obj);
int i;
for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
object_initialize_child(obj, "device[*]", &s->devices[i],
TYPE_ASPEED_I3C_DEVICE);
}
}
and realize the devices in aspeed_i3c_realize :
for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
Object *obj = OBJECT(&s->devices[i]);
if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
return;
}
/* Map the device MMIO window in the overall I3C MMIO */
memory_region_add_subregion(&s->iomem, 0x2000 + i * 0x1000;
&s->devices[i].mr);
}
I don't think we need to handle the controller registers mapped at 0x0
to start with. You could introduce a region to catch memory ops.
At some point, we would have to consider modeling the I3C bus but that's
beyond the scope of the initial model.
Thanks,
C.
> +
> + sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static const VMStateDescription vmstate_aspeed_i3c = {
> + .name = TYPE_ASPEED_I3C,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32_ARRAY(regs, AspeedI3CState, ASPEED_I3C_NR_REGS),
> + VMSTATE_END_OF_LIST(),
> + }
> +};
> +
> +static void aspeed_i3c_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = aspeed_i3c_realize;
> + dc->reset = aspeed_i3c_reset;
> + dc->desc = "Aspeed I3C Controller";
> + dc->vmsd = &vmstate_aspeed_i3c;
> +}
> +
> +static const TypeInfo aspeed_i3c_info = {
> + .name = TYPE_ASPEED_I3C,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(AspeedI3CState),
> + .class_init = aspeed_i3c_class_init,
> +};
> +
> +static void aspeed_i3c_register_types(void)
> +{
> + type_register_static(&aspeed_i3c_info);
> +}
> +
> +type_init(aspeed_i3c_register_types);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 73a92fc459..9e570131c2 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -103,6 +103,7 @@ softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
> softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> 'aspeed_hace.c',
> + 'aspeed_i3c.c',
> 'aspeed_lpc.c',
> 'aspeed_pwm.c',
> 'aspeed_scu.c',
> diff --git a/include/hw/misc/aspeed_i3c.h b/include/hw/misc/aspeed_i3c.h
> new file mode 100644
> index 0000000000..c8f8ae3791
> --- /dev/null
> +++ b/include/hw/misc/aspeed_i3c.h
> @@ -0,0 +1,30 @@
> +/*
> + * ASPEED I3C Controller
> + *
> + * Copyright (C) 2021 ASPEED Technology Inc.
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_I3C_H
> +#define ASPEED_I3C_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_I3C "aspeed.i3c"
> +#define ASPEED_I3C(obj) OBJECT_CHECK(AspeedI3CState, (obj), TYPE_ASPEED_I3C)
> +
> +#define ASPEED_I3C_NR_REGS (0x8000 >> 2)
> +
> +typedef struct AspeedI3CState {
> + /* <private> */
> + SysBusDevice parent;
> +
> + /* <public> */
> + MemoryRegion iomem;
> + qemu_irq irq;
> +
> + uint32_t regs[ASPEED_I3C_NR_REGS];
> +} AspeedI3CState;
> +#endif /* ASPEED_I3C_H */
>
More information about the openbmc
mailing list