On 28/04/26 01:54PM, Philippe Mathieu-Daudé wrote:
Hi Philippe,
Thanks for the review.
Directly use .b[] instead of .pid[]?
Will drop the anonymous struct and use .b[] directly in v2.
Or (untested):
dcr = target_info.b[0];
bcr = target_info.b[1];
On BCR/DCR indices: using b[6] for BCR and b[7] for DCR per the ENTDAA frame
diagram in the I3C v1.1.1 databook.
pid = extract64(be64_to_cpu(target_info.d), 16, 48);
or:
pid = be64_to_cpu(target_info.d) >> 16;
On PID assembly and the be64_to_cpu suggestion:
The current loop (j * 8) was written assuming t->pid holds the logical
value (e.g. 0x0000AABBCCDDEEFF with AA as MSB). core.c sends using
(offset * 8) which puts the LSB on wire first, so the loop recovered the
original logical value end to end.
For spec compliance (pid[47:40] on wire first), given that core.c uses
(offset * 8), t->pid must be stored pre-reversed by the target modeller.
With that assumption be64_to_cpu >> 16 correctly recovers the logical pid
on the controller side on both LE and BE hosts. Will adopt this in v2 and add
a comment to i3c.h documenting this requirement for t->pid.
+ dw_i3c_update_char_table(s, cmd.dev_index + i, pid, target_info.bcr,
+ target_info.dcr, addr);
Also found a pre-existing bug in dw_i3c_update_char_table() - PID is
split at bit 32 but spec DCT layout (verified from databook) requires the
split at bit 16:
LOC1 = pid[47:16] (pid >> 16) & 0xffffffff
LOC2 = pid[15:0] pid & 0xffff
Will fold this fix into v2.
Will send v2 shortly.
Thanks,
Ashish
