Hi Ashish > Subject: [PATCH v2] hw/i3c/dw-i3c: Fix BCR/DCR extraction and PID assembly > during ENTDAA > > The target_info union in dw_i3c_addr_assign_cmd() declares pid, bcr, and dcr > as separate union members, causing them to all alias b[0] rather than their > correct positions in the ENTDAA response buffer. > This results in dw_i3c_update_char_table() being called with BCR and DCR > both read from b[0] instead of b[6] and b[7] respectively, corrupting the > device > characteristics table on every ENTDAA operation. > Fix by replacing the broken members with uint64_t d and extracting fields per > the I3C spec ENTDAA wire format. > > Additionally, dw_i3c_update_char_table() incorrectly splits PID across > LOC1 and LOC2 at bit 32. Per the Linux kernel HCI driver > (drivers/i3c/master/mipi-i3c-hci/dct_v1.c), the DCT layout requires > LOC1 to hold pid[47:16] and LOC2 to hold pid[15:0]. Fix the split accordingly. > > Signed-off-by: Ashish Anand <[email protected]> > --- > > Changes from v1 : > 1. Replaced broken union members with uint64_t d, added wire format > comment. > 2. Replaced loop with be64_to_cpu(target_info.d) >> 16. > 3. Fixed LOC1/LOC2 split at bit 16 in dw_i3c_update_char_table. > 4. Added comment to i3c.h clarifying how core.c transmits t->pid during > ENTDAA. > > Link to v1 - > https://lore.kernel.org/qemu-devel/exdu52mug6fh3bwj7ofo3jlltyuwjfnddxobio > 4w3jhaq6uv7k@boxo3j5ocef2/T/#t > > This bug was discovered during end-to-end ENTDAA testing where BCR and > DCR values observed in the char table register did not match the target's > actual BCR and DCR values. > > hw/i3c/dw-i3c.c | 16 +++++++--------- > include/hw/i3c/i3c.h | 7 +++++++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/hw/i3c/dw-i3c.c b/hw/i3c/dw-i3c.c index d87d42be89..17ff484c5d > 100644 > --- a/hw/i3c/dw-i3c.c > +++ b/hw/i3c/dw-i3c.c > @@ -1459,11 +1459,10 @@ static void dw_i3c_update_char_table(DWI3C *s, > uint8_t offset, uint64_t pid, > > P_DEV_CHAR_TABLE_START_ADDR) / > sizeof(uint32_t)) + > (offset * sizeof(uint32_t)); > - s->regs[dev_index] = pid & 0xffffffff; > - pid >>= 32; > + s->regs[dev_index] = (pid >> 16) & 0xffffffff; > s->regs[dev_index + 1] = FIELD_DP32(s->regs[dev_index + 1], > > DEVICE_CHARACTERISTIC_TABLE_LOC2, > - MSB_PID, pid); > + MSB_PID, pid & 0xffff); > s->regs[dev_index + 2] = FIELD_DP32(s->regs[dev_index + 2], > > DEVICE_CHARACTERISTIC_TABLE_LOC3, DCR, > dcr); @@ -1507,10 +1506,9 > @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, DWI3CAddrAssignCmd > cmd) > for (i = 0; i < cmd.dev_count; i++) { > uint8_t addr = dw_i3c_target_addr(s, cmd.dev_index + i); > union { > - uint64_t pid:48; > - uint8_t bcr; > - uint8_t dcr; > + uint64_t d; > uint32_t w[2]; > + /* Per I3C spec: b[0]=PID MSB, b[5]=PID LSB, b[6]=BCR, > + b[7]=DCR */ > uint8_t b[8]; > } target_info; > > @@ -1544,9 +1542,9 @@ static void dw_i3c_addr_assign_cmd(DWI3C *s, > DWI3CAddrAssignCmd cmd) > err = DW_I3C_RESP_QUEUE_ERR_DAA_NACK; > break; > } > - dw_i3c_update_char_table(s, cmd.dev_index + i, > - target_info.pid, > target_info.bcr, > - target_info.dcr, addr); > + uint64_t pid = be64_to_cpu(target_info.d) >> 16; > + dw_i3c_update_char_table(s, cmd.dev_index + i, pid, > target_info.b[6], > + target_info.b[7], addr); > > /* Push the PID, BCR, and DCR to the RX queue. */ > dw_i3c_push_rx(s, target_info.w[0]); diff --git > a/include/hw/i3c/i3c.h b/include/hw/i3c/i3c.h index 6ba90793ad..dcf8d9b143 > 100644 > --- a/include/hw/i3c/i3c.h > +++ b/include/hw/i3c/i3c.h > @@ -138,6 +138,13 @@ struct I3CTarget { > uint8_t static_address; > uint8_t dcr; > uint8_t bcr; > + /* > + * Provisioned ID. Since core.c sends this LSB-first during ENTDAA > + * via (pid >> (offset * 8)) & 0xff, targets must store it > + * pre-reversed so that pid[47:40] goes on the wire first, as > + * required by the I3C spec. > + * e.g. for a device with pid 0xAABBCCDDEEFF, store > 0xFFEEDDCCBBAA. > + */ > uint64_t pid; > > /* CCC State tracking. */ > -- > 2.43.0
Reviewed-by: Jamin Lin <[email protected]>
