Hi Cedric, On Mon, Jan 10, 2022 at 10:25 PM Cédric Le Goater <c...@kaod.org> wrote: > > Hello Troy, > > On 1/10/22 08:21, 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. > > > > v2: > > - Split i3c model into i3c and i3c_device > > - Create 6x i3c devices > > - Using register fields macro > > > > Signed-off-by: Troy Lee <troy_...@aspeedtech.com> > > --- > > hw/misc/aspeed_i3c.c | 410 +++++++++++++++++++++++++++++++++++ > > hw/misc/meson.build | 1 + > > hw/misc/trace-events | 6 + > > include/hw/misc/aspeed_i3c.h | 57 +++++ > > 4 files changed, 474 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..16a4f2d4e4 > > --- /dev/null > > +++ b/hw/misc/aspeed_i3c.c > > @@ -0,0 +1,410 @@ > > +/* > > + * 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 "hw/registerfields.h" > > +#include "hw/qdev-properties.h" > > +#include "qapi/error.h" > > +#include "migration/vmstate.h" > > +#include "trace.h" > > + > > +/* I3C Controller Registers */ > > +REG32(I3C1_REG0, 0x10) > > +REG32(I3C1_REG1, 0x14) > > + FIELD(I3C1_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C1_REG1, SA_EN, 15, 1) > > +REG32(I3C2_REG0, 0x20) > > +REG32(I3C2_REG1, 0x24) > > + FIELD(I3C2_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C2_REG1, SA_EN, 15, 1) > > +REG32(I3C3_REG0, 0x30) > > +REG32(I3C3_REG1, 0x34) > > + FIELD(I3C3_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C3_REG1, SA_EN, 15, 1) > > +REG32(I3C4_REG0, 0x40) > > +REG32(I3C4_REG1, 0x44) > > + FIELD(I3C4_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C4_REG1, SA_EN, 15, 1) > > +REG32(I3C5_REG0, 0x50) > > +REG32(I3C5_REG1, 0x54) > > + FIELD(I3C5_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C5_REG1, SA_EN, 15, 1) > > +REG32(I3C6_REG0, 0x60) > > +REG32(I3C6_REG1, 0x64) > > + FIELD(I3C6_REG1, I2C_MODE, 0, 1) > > + FIELD(I3C6_REG1, SA_EN, 15, 1) > > + > > +/* I3C Device Registers */ > > +REG32(DEVICE_CTRL, 0x00) > > +REG32(DEVICE_ADDR, 0x04) > > +REG32(HW_CAPABILITY, 0x08) > > +REG32(COMMAND_QUEUE_PORT, 0x0c) > > +REG32(RESPONSE_QUEUE_PORT, 0x10) > > +REG32(RX_TX_DATA_PORT, 0x14) > > +REG32(IBI_QUEUE_STATUS, 0x18) > > +REG32(IBI_QUEUE_DATA, 0x18) > > +REG32(QUEUE_THLD_CTRL, 0x1c) > > +REG32(DATA_BUFFER_THLD_CTRL, 0x20) > > +REG32(IBI_QUEUE_CTRL, 0x24) > > +REG32(IBI_MR_REQ_REJECT, 0x2c) > > +REG32(IBI_SIR_REQ_REJECT, 0x30) > > +REG32(RESET_CTRL, 0x34) > > +REG32(SLV_EVENT_CTRL, 0x38) > > +REG32(INTR_STATUS, 0x3c) > > +REG32(INTR_STATUS_EN, 0x40) > > +REG32(INTR_SIGNAL_EN, 0x44) > > +REG32(INTR_FORCE, 0x48) > > +REG32(QUEUE_STATUS_LEVEL, 0x4c) > > +REG32(DATA_BUFFER_STATUS_LEVEL, 0x50) > > +REG32(PRESENT_STATE, 0x54) > > +REG32(CCC_DEVICE_STATUS, 0x58) > > +REG32(DEVICE_ADDR_TABLE_POINTER, 0x5c) > > + FIELD(DEVICE_ADDR_TABLE_POINTER, DEPTH, 16, 16) > > + FIELD(DEVICE_ADDR_TABLE_POINTER, ADDR, 0, 16) > > +REG32(DEV_CHAR_TABLE_POINTER, 0x60) > > +REG32(VENDOR_SPECIFIC_REG_POINTER, 0x6c) > > +REG32(SLV_MIPI_PID_VALUE, 0x70) > > +REG32(SLV_PID_VALUE, 0x74) > > +REG32(SLV_CHAR_CTRL, 0x78) > > +REG32(SLV_MAX_LEN, 0x7c) > > +REG32(MAX_READ_TURNAROUND, 0x80) > > +REG32(MAX_DATA_SPEED, 0x84) > > +REG32(SLV_DEBUG_STATUS, 0x88) > > +REG32(SLV_INTR_REQ, 0x8c) > > +REG32(DEVICE_CTRL_EXTENDED, 0xb0) > > +REG32(SCL_I3C_OD_TIMING, 0xb4) > > +REG32(SCL_I3C_PP_TIMING, 0xb8) > > +REG32(SCL_I2C_FM_TIMING, 0xbc) > > +REG32(SCL_I2C_FMP_TIMING, 0xc0) > > +REG32(SCL_EXT_LCNT_TIMING, 0xc8) > > +REG32(SCL_EXT_TERMN_LCNT_TIMING, 0xcc) > > +REG32(BUS_FREE_TIMING, 0xd4) > > +REG32(BUS_IDLE_TIMING, 0xd8) > > +REG32(I3C_VER_ID, 0xe0) > > +REG32(I3C_VER_TYPE, 0xe4) > > +REG32(EXTENDED_CAPABILITY, 0xe8) > > +REG32(SLAVE_CONFIG, 0xec) > > + > > +static uint64_t aspeed_i3c_device_read(void *opaque, hwaddr offset, > > + unsigned size) > > +{ > > + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(opaque); > > + uint32_t addr = offset >> 2; > > + uint64_t value; > > + > > + switch (addr) { > > + case R_COMMAND_QUEUE_PORT: > > + value = 0; > > + break; > > + default: > > + value = s->regs[addr]; > > + break; > > + } > > + > > + trace_aspeed_i3c_device_read(s->id, offset, value); > > + > > + return value; > > +} > > + > > +static void aspeed_i3c_device_write(void *opaque, hwaddr offset, > > + uint64_t value, unsigned size) > > +{ > > + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(opaque); > > + uint32_t addr = offset >> 2; > > + > > + trace_aspeed_i3c_device_write(s->id, offset, value); > > + > > + switch (addr) { > > + case R_HW_CAPABILITY: > > + case R_RESPONSE_QUEUE_PORT: > > + case R_IBI_QUEUE_DATA: > > + case R_QUEUE_STATUS_LEVEL: > > + case R_PRESENT_STATE: > > + case R_CCC_DEVICE_STATUS: > > + case R_DEVICE_ADDR_TABLE_POINTER: > > + case R_VENDOR_SPECIFIC_REG_POINTER: > > + case R_SLV_CHAR_CTRL: > > + case R_SLV_MAX_LEN: > > + case R_MAX_READ_TURNAROUND: > > + case R_I3C_VER_ID: > > + case R_I3C_VER_TYPE: > > + case R_EXTENDED_CAPABILITY: > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: write to readonly register[%02lx] = %08lx\n", > > + __func__, offset, value); > > + break; > > + case R_RX_TX_DATA_PORT: > > + break; > > + case R_RESET_CTRL: > > + break; > > + default: > > + s->regs[addr] = value; > > + break; > > + } > > +} > > + > > +static const VMStateDescription aspeed_i3c_device_vmstate = { > > + .name = TYPE_ASPEED_I3C, > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .fields = (VMStateField[]){ > > + VMSTATE_UINT32_ARRAY(regs, AspeedI3CDevice, > > ASPEED_I3C_DEVICE_NR_REGS), > > + VMSTATE_END_OF_LIST(), > > + } > > +}; > > + > > +static const MemoryRegionOps aspeed_i3c_device_ops = { > > + .read = aspeed_i3c_device_read, > > + .write = aspeed_i3c_device_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > +}; > > + > > +static void aspeed_i3c_device_reset(DeviceState *dev) > > +{ > > + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(dev); > > + > > + memset(s->regs, 0, sizeof(s->regs)); > > + > > + s->regs[R_HW_CAPABILITY] = 0x000e00bf; > > + s->regs[R_QUEUE_THLD_CTRL] = 0x01000101; > > + s->regs[R_I3C_VER_ID] = 0x3130302a; > > + s->regs[R_I3C_VER_TYPE] = 0x6c633033; > > + s->regs[R_DEVICE_ADDR_TABLE_POINTER] = > > + (0x08 << R_DEVICE_ADDR_TABLE_POINTER_DEPTH_SHIFT) | > > + (0x280 << R_DEVICE_ADDR_TABLE_POINTER_ADDR_SHIFT); > > + s->regs[R_DEV_CHAR_TABLE_POINTER] = 0x00020200; > > + s->regs[A_VENDOR_SPECIFIC_REG_POINTER] = 0x000000b0; > > + s->regs[R_SLV_MAX_LEN] = (0xff << 16) | (0xff); > > +} > > > Some models store the reset definitions in an array and simply > memset() the values in s->regs. See SCU. No need need to resend > just for that.
This will be improved in v3. > > + > > +static void aspeed_i3c_device_realize(DeviceState *dev, Error **errp) > > +{ > > + AspeedI3CDevice *s = ASPEED_I3C_DEVICE(dev); > > + g_autofree char *name = g_strdup_printf(TYPE_ASPEED_I3C_DEVICE ".%d", > > + s->id); > > + > > + if (!s->controller) { > > + error_setg(errp, TYPE_ASPEED_I3C_DEVICE ": 'controller' link not > > set"); > > + return; > > + } > > AspeedI3CDevice does not use ->controller. Do you have plans for it ? Nope, removed in v3. > > + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > > + > > + memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i3c_device_ops, > > + s, name, 0x1000); > > I would initialize the register window for the exact number of regs because > it's a good way to catch out of bounds accesses. 0x300 in this case. > Updated. > > + > > + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr); > > You don't need to "sysbus-declare" the region. It will be mapped in > the overall region of the I3C controller, which itself is mapped at > 0x1e7a0000 > Removed. > > +} > > + > > +static uint64_t aspeed_i3c_read(void *opaque, > > + hwaddr addr, > > + unsigned int size) > > This prototype fits on one line. > Updated. > > +{ > > + AspeedI3CState *s = ASPEED_I3C(opaque); > > + uint64_t val = 0; > > + > > + if (addr >= (ASPEED_I3C_NR_REGS << 2)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx > > "\n", > > + __func__, addr << 2); > > + } else if (addr < 0x800) { > > The controller only has 0x70 << 2 registers > > > + /* I3C controller register */ > > + val = s->regs[addr >> 2]; > > + } else { > > + /* I3C device register */ > > + } > > hmm, this read op looks a little weird. > After I applied the container/subregion design, the boundary check can be removed. The weird code snippets are removed as well. It looks much cleaner. > > + trace_aspeed_i3c_read(addr, val); > > + > > + return val; > > +} > > + > > +static void aspeed_i3c_write(void *opaque, > > + hwaddr addr, > > + uint64_t data, > > + unsigned int size) > > +{ > > + AspeedI3CState *s = ASPEED_I3C(opaque); > > + > > + trace_aspeed_i3c_write(addr, data); > > + > > + 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 the window is correctly sized, you don't need this check. > Updated. > > + /* I3C controller register */ > > + switch (addr) { > > + case R_I3C1_REG1: > > + case R_I3C2_REG1: > > + case R_I3C3_REG1: > > + case R_I3C4_REG1: > > + case R_I3C5_REG1: > > + case R_I3C6_REG1: > > + if (data & R_I3C1_REG1_I2C_MODE_MASK) { > > + qemu_log_mask(LOG_UNIMP, > > + "%s: Not support I2C mode [%08lx]=%08lx", > > + __func__, addr << 2, data); > > + break; > > + } > > + if (data & R_I3C1_REG1_SA_EN_MASK) { > > + 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; > > + } > > +} > > + > > +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); > > Remove 'struct' > Updated. > > + memset(s->regs, 0, sizeof(s->regs)); > > +} > > + > > +static void aspeed_i3c_realize(DeviceState *dev, Error **errp) > > +{ > > + int i; > > + AspeedI3CState *s = ASPEED_I3C(dev); > > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > + > > + sysbus_init_irq(sbd, &s->irq); > > I don't think the I3C controller has an IRQ line. > Removed. > > + > > + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s, > > + TYPE_ASPEED_I3C, ASPEED_I3C_NR_REGS << 2); > > + > > + sysbus_init_mmio(sbd, &s->iomem); > > I would add a container region containing all the regions : > > > memory_region_init(&s->iomem_container, OBJECT(s), > TYPE_ASPEED_I3C ".container", 0x8000); > > sysbus_init_mmio(sbd, &s->iomem_container); > > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i3c_ops, s, > TYPE_ASPEED_I3C ".regs", 0x70); > > memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem); > > > > The goal is to have a stricter layout so that you can catch errors : > > 000000001e7a0000-000000001e7a7fff (prio 0, i/o): aspeed.i3c.container > 000000001e7a0000-000000001e7a006f (prio 0, i/o): aspeed.i3c.regs > 000000001e7a2000-000000001e7a22ff (prio 0, i/o): aspeed.i3c.device.0 > 000000001e7a3000-000000001e7a32ff (prio 0, i/o): aspeed.i3c.device.1 > 000000001e7a4000-000000001e7a42ff (prio 0, i/o): aspeed.i3c.device.2 > 000000001e7a5000-000000001e7a52ff (prio 0, i/o): aspeed.i3c.device.3 > 000000001e7a6000-000000001e7a62ff (prio 0, i/o): aspeed.i3c.device.4 > 000000001e7a7000-000000001e7a72ff (prio 0, i/o): aspeed.i3c.device.5 > > and if under U-Boot, you peek into unimplemented regs, you get a warning : > > ast# md 1e7a0000 > 1e7a0000: 00000000 00000000 00000000 00000000 ................ > 1e7a0010: 00000000 00000000 00000000 00000000 ................ > 1e7a0020: 00000000 00000000 00000000 00000000 ................ > 1e7a0030: 00000000 00000000 00000000 00000000 ................ > 1e7a0040: 00000000 00000000 00000000 00000000 ................ > 1e7a0050: 00000000 00000000 00000000 00000000 ................ > 1e7a0060: 00000000 00000000 00000000 00000000 ................ > 1e7a0070:aspeed_soc.io: unimplemented device read (size 4, offset > 0x1a0070) > 00000000aspeed_soc.io: unimplemented device read (size 4, offset > 0x1a0074) Thanks for the code snippet, learnt and applied. Yes, it would be easier to catch driver problems. > > + sysbus_init_mmio(sbd, &s->iomem); > > + > > + for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) { > > + Object *dev = OBJECT(&s->devices[i]); > > + > > + if (!object_property_set_link(dev, "controller", OBJECT(s), errp)) > > { > > + return; > > + } > > This might not be needed. > Removed. > > + if (!object_property_set_uint(dev, "device-id", i, errp)) { > > + return; > > + } > > + > > + if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) { > > + return; > > + } > > + > > + /* > > + * Register Address of I3CX Device = > > + * (Base Address of Global Register) + (Offset of I3CX) + > > Offset > > + * X = 0, 1, 2, 3, 4, 5 > > + * Offset of I3C0 = 0x2000 > > + * Offset of I3C1 = 0x3000 > > + * Offset of I3C2 = 0x4000 > > + * Offset of I3C3 = 0x5000 > > + * Offset of I3C4 = 0x6000 > > + * Offset of I3C5 = 0x7000 > > + */ > > + memory_region_add_subregion(&s->iomem, > > and map in &s->iomem_container with the example above. > > > + 0x2000 + (i * (ASPEED_I3C_DEVICE_NR_REGS << 2)), > > Just use : 0x2000 + i * 0x1000, > Updated. > > + &s->devices[i].mr); > > + } > > + > > +} > > + > > +static Property aspeed_i3c_device_properties[] = { > > + DEFINE_PROP_UINT8("device-id", AspeedI3CDevice, id, 0), > > + DEFINE_PROP_LINK("controller", AspeedI3CDevice, controller, > > TYPE_ASPEED_I3C, > > + AspeedI3CState *), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void aspeed_i3c_device_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->desc = "Aspeed I3C Device"; > > + dc->realize = aspeed_i3c_device_realize; > > + dc->reset = aspeed_i3c_device_reset; > > + device_class_set_props(dc, aspeed_i3c_device_properties); > > +} > > + > > +static const TypeInfo aspeed_i3c_device_info = { > > + .name = TYPE_ASPEED_I3C_DEVICE, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(AspeedI3CDevice), > > + .class_init = aspeed_i3c_device_class_init, > > +}; > > + > > +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_STRUCT_ARRAY(devices, AspeedI3CState, > > ASPEED_I3C_NR_DEVICES, 1, > > + aspeed_i3c_device_vmstate, AspeedI3CDevice), > > + 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 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); > > + } > > +} > > Please put this aspeed_i3c_instance_init() routine above > aspeed_i3c_realize(). It's cleaner. > Updated. > > + > > +static const TypeInfo aspeed_i3c_info = { > > + .name = TYPE_ASPEED_I3C, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_init = aspeed_i3c_instance_init, > > + .instance_size = sizeof(AspeedI3CState), > > + .class_init = aspeed_i3c_class_init, > > +}; > > + > > +static void aspeed_i3c_register_types(void) > > +{ > > + type_register_static(&aspeed_i3c_device_info); > > + 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 3f41a3a5b2..d1a1169108 100644 > > --- a/hw/misc/meson.build > > +++ b/hw/misc/meson.build > > @@ -105,6 +105,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_scu.c', > > 'aspeed_sdmc.c', > > diff --git a/hw/misc/trace-events b/hw/misc/trace-events > > index 2da96d167a..1c373dd0a4 100644 > > --- a/hw/misc/trace-events > > +++ b/hw/misc/trace-events > > @@ -199,6 +199,12 @@ armsse_mhu_write(uint64_t offset, uint64_t data, > > unsigned size) "SSE-200 MHU wri > > # aspeed_xdma.c > > aspeed_xdma_write(uint64_t offset, uint64_t data) "XDMA write: offset > > 0x%" PRIx64 " data 0x%" PRIx64 > > > > +# aspeed_i3c.c > > +aspeed_i3c_read(uint64_t offset, uint64_t data) "I3C read: offset 0x%" > > PRIx64 " data 0x%" PRIx64 > > +aspeed_i3c_write(uint64_t offset, uint64_t data) "I3C write: offset 0x%" > > PRIx64 " data 0x%" PRIx64 > > +aspeed_i3c_device_read(uint32_t deviceid, uint64_t offset, uint64_t data) > > "I3C Dev[%u] read: offset 0x%" PRIx64 " data 0x%" PRIx64 > > +aspeed_i3c_device_write(uint32_t deviceid, uint64_t offset, uint64_t data) > > "I3C Dev[%u] write: offset 0x%" PRIx64 " data 0x%" PRIx64 > > + > > # bcm2835_property.c > > bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) > > "mbox property tag:0x%08x in_sz:%u out_sz:%zu" > > > > diff --git a/include/hw/misc/aspeed_i3c.h b/include/hw/misc/aspeed_i3c.h > > new file mode 100644 > > index 0000000000..276f70b001 > > --- /dev/null > > +++ b/include/hw/misc/aspeed_i3c.h > > @@ -0,0 +1,57 @@ > > +/* > > + * 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 TYPE_ASPEED_I3C_DEVICE "aspeed.i3c.device" > > +OBJECT_DECLARE_TYPE(AspeedI3CState, AspeedI3CClass, ASPEED_I3C) > > + > > +#define ASPEED_I3C_NR_REGS (0x8000 >> 2) > > +#define ASPEED_I3C_DEVICE_NR_REGS (0x1000 >> 2) > > There are less registers. > Updated in v3. - I3C_REG to 0x80 >> 2 - I3C_DEVICE to 0x300 >> 2 > > +#define ASPEED_I3C_NR_DEVICES 6 > > + > > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI3CDevice, ASPEED_I3C_DEVICE) > > +typedef struct AspeedI3CDevice { > > + /* <private> */ > > + SysBusDevice parent; > > + struct AspeedI3CState *controller; > > + > > + /* <public> */ > > + MemoryRegion mr; > > + qemu_irq irq; > > + > > + uint8_t id; > > + uint32_t regs[ASPEED_I3C_DEVICE_NR_REGS]; > > +} AspeedI3CDevice; > > + > > +typedef struct AspeedI3CClass { > > + SysBusDeviceClass parent_class; > > + > > + uint8_t num_devices; > > + uint8_t reg_size; > > + > > + qemu_irq (*bus_get_irq)(AspeedI3CDevice *); > > +} AspeedI3CClass; > > The class is unused. Do you have plans for other SoCs with different > layouts ? > Correct, removed in v3. Thanks for the review, I've learnt a lot in this series. Troy Lee > Thanks, > > C. > > > > +typedef struct AspeedI3CState { > > + /* <private> */ > > + SysBusDevice parent; > > + > > + /* <public> */ > > + MemoryRegion iomem; > > + qemu_irq irq; > > + > > + uint32_t regs[ASPEED_I3C_NR_REGS]; > > + AspeedI3CDevice devices[ASPEED_I3C_NR_DEVICES]; > > +} AspeedI3CState; > > +#endif /* ASPEED_I3C_H */ > > >