Re: [PATCH v2 1/2] hw/misc: Implementating dummy AST2600 I3C model

2022-01-11 Thread Cédric Le Goater 

Hello Troy,



+
+memory_region_init_io(>iomem, OBJECT(s), _i3c_ops, s,
+TYPE_ASPEED_I3C, ASPEED_I3C_NR_REGS << 2);
+
+sysbus_init_mmio(sbd, >iomem);


I would add a container region containing all the regions :


  memory_region_init(>iomem_container, OBJECT(s),
 TYPE_ASPEED_I3C ".container", 0x8000);

  sysbus_init_mmio(sbd, >iomem_container);

  memory_region_init_io(>iomem, OBJECT(s), _i3c_ops, s,
  TYPE_ASPEED_I3C ".regs", 0x70);

  memory_region_add_subregion(>iomem_container, 0x0, >iomem);



The goal is to have a stricter layout so that you can catch errors :

  1e7a-1e7a7fff (prio 0, i/o): aspeed.i3c.container
1e7a-1e7a006f (prio 0, i/o): aspeed.i3c.regs
1e7a2000-1e7a22ff (prio 0, i/o): aspeed.i3c.device.0
1e7a3000-1e7a32ff (prio 0, i/o): aspeed.i3c.device.1
1e7a4000-1e7a42ff (prio 0, i/o): aspeed.i3c.device.2
1e7a5000-1e7a52ff (prio 0, i/o): aspeed.i3c.device.3
1e7a6000-1e7a62ff (prio 0, i/o): aspeed.i3c.device.4
1e7a7000-1e7a72ff (prio 0, i/o): aspeed.i3c.device.5

and if under U-Boot, you peek into unimplemented regs, you get a warning :

  ast# md 1e7a
  1e7a:    
  1e7a0010:    
  1e7a0020:    
  1e7a0030:    
  1e7a0040:    
  1e7a0050:    
  1e7a0060:    
  1e7a0070:aspeed_soc.io: unimplemented device read  (size 4, offset 
0x1a0070)
   aspeed_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.


That said, not all Aspeed models follow this pattern. We learned along
the way and we could improve some older implementations.

btw, I tried your series with ast2600-default-obmc.tar.gz. It boots fine.

Thanks,

C.

Re: [PATCH v2 1/2] hw/misc: Implementating dummy AST2600 I3C model

2022-01-10 Thread Troy Lee 
Hi Cedric,
On Mon, Jan 10, 2022 at 10:25 PM Cédric Le Goater  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 
> > ---
> >   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 00..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,

Re: [PATCH v2 1/2] hw/misc: Implementating dummy AST2600 I3C model

2022-01-10 Thread Cédric Le Goater 

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

[PATCH v2 1/2] hw/misc: Implementating dummy AST2600 I3C model

2022-01-09 Thread Troy Lee 
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 
---
 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 00..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