Hi Joe,
> Subject: RE: [PATCH 08/19] hw/i3c/dw-i3c: Add register RO field masks
>
> Hi Joe,
>
> > Subject: [PATCH 08/19] hw/i3c/dw-i3c: Add register RO field masks
> >
> > Adds read-only register masks for the DwC I3C controller.
> >
> > Signed-off-by: Joe Komlodi <[email protected]>
> >
> > Reviewed-by: Patrick Venture <[email protected]>
> > ---
> > hw/i3c/dw-i3c.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/hw/i3c/dw-i3c.c b/hw/i3c/dw-i3c.c index
> > 845fbd5efd..4f40f999f4
> > 100644
> > --- a/hw/i3c/dw-i3c.c
> > +++ b/hw/i3c/dw-i3c.c
> > @@ -313,6 +313,45 @@ static const uint32_t
> > dw_i3c_resets[DW_I3C_NR_REGS] = {
> > [R_SLAVE_CONFIG] = 0x00000023,
> > };
> >
> > +static const uint32_t dw_i3c_ro[DW_I3C_NR_REGS] = {
> > + [R_DEVICE_CTRL] = 0x04fffe00,
> > + [R_DEVICE_ADDR] = 0x7f807f80,
> > + [R_HW_CAPABILITY] = 0xffffffff,
> > + [R_IBI_QUEUE_STATUS] = 0xffffffff,
> > + [R_DATA_BUFFER_THLD_CTRL] = 0xf8f8f8f8,
> > + [R_IBI_QUEUE_CTRL] = 0xfffffff0,
> > + [R_RESET_CTRL] = 0xffffffc0,
> > + [R_SLV_EVENT_CTRL] = 0xffffff3f,
> > + [R_INTR_STATUS] = 0xffff809f,
> > + [R_INTR_STATUS_EN] = 0xffff8080,
> > + [R_INTR_SIGNAL_EN] = 0xffff8080,
> > + [R_INTR_FORCE] = 0xffff8000,
> > + [R_QUEUE_STATUS_LEVEL] = 0xffffffff,
> > + [R_DATA_BUFFER_STATUS_LEVEL] = 0xffffffff,
> > + [R_PRESENT_STATE] = 0xffffffff,
> > + [R_CCC_DEVICE_STATUS] = 0xffffffff,
> > + [R_I3C_VER_ID] = 0xffffffff,
> > + [R_I3C_VER_TYPE] = 0xffffffff,
> > + [R_DEVICE_ADDR_TABLE_POINTER] = 0xffffffff,
> > + [R_DEV_CHAR_TABLE_POINTER] = 0xffcbffff,
> > + [R_SLV_PID_VALUE] = 0xffff0fff,
> > + [R_SLV_CHAR_CTRL] = 0xffffffff,
> > + [A_VENDOR_SPECIFIC_REG_POINTER] = 0xffffffff,
> > + [R_SLV_MAX_LEN] = 0xffffffff,
> > + [R_MAX_READ_TURNAROUND] = 0xffffffff,
> > + [R_MAX_DATA_SPEED] = 0xffffffff,
> > + [R_SLV_INTR_REQ] = 0xfffffff0,
> > + [R_SLV_TSX_SYMBL_TIMING] = 0xffffffc0,
I don’t see this register defined in the datasheet.
Which document are you referencing for this member?
> > + [R_DEVICE_CTRL_EXTENDED] = 0xfffffff8,
> > + [R_SCL_I3C_OD_TIMING] = 0xff00ff00,
> > + [R_SCL_I3C_PP_TIMING] = 0xff00ff00,
> > + [R_SCL_I2C_FMP_TIMING] = 0xff000000,
> > + [R_SCL_EXT_TERMN_LCNT_TIMING] = 0x0000fff0,
> > + [R_BUS_IDLE_TIMING] = 0xfff00000,
> > + [R_EXTENDED_CAPABILITY] = 0xffffffff,
I don’t see this register defined in the datasheet.
Which document are you referencing for this member?
> > + [R_SLAVE_CONFIG] = 0xffffffff,
I don’t see this register defined in the datasheet.
Which document are you referencing for this member?
> > +};
> > +
> > static uint64_t dw_i3c_read(void *opaque, hwaddr offset, unsigned size)
> {
> > DWI3C *s = DW_I3C(opaque);
> > @@ -341,6 +380,7 @@ static void dw_i3c_write(void *opaque, hwaddr
> > offset, uint64_t value,
> >
> > trace_dw_i3c_write(s->id, offset, value);
> >
> > + value &= ~dw_i3c_ro[addr];
> > switch (addr) {
>
> The following registers are read-only, and the code already checks their
> access
> based solely on the register address.
> Why do we still need to define them in dw_i3c_ro?
>
> 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:
>
> Thanks-Jamin
>
> > case R_HW_CAPABILITY:
> > case R_RESPONSE_QUEUE_PORT:
> > --
> > 2.50.0.rc1.591.g9c95f17f64-goog
