Hi Cédric > > Jamin, > > Does that look good ? > > Thanks, >
Looks good to me. Added review tag Thanks, Jamin > C. > > On 5/5/26 15:40, Ashish Anand wrote: > > 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/exdu52mug6fh3bwj7ofo3jlltyuwjfnddxo > > bio4w3jhaq6uv7k@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. */
