Hi Joe,
First up, nice work with this series! I haven't yet had a thorough look
at the series, but one item on something that caught me up on the Linux
side:
> +static void aspeed_i3c_device_ibi_queue_push(AspeedI3CDevice *s)
> +{
> + /* Stored value is in 32-bit chunks, convert it to byte chunks. */
> + uint8_t ibi_slice_size = aspeed_i3c_device_ibi_slice_size(s);
> + uint8_t num_slices = fifo8_num_used(&s->ibi_data.ibi_intermediate_queue)
> /
> + ibi_slice_size;
> + uint8_t ibi_status_count = num_slices;
> + union {
> + uint8_t b[sizeof(uint32_t)];
> + uint32_t val32;
> + } ibi_data = {
> + .val32 = 0
> + };
> +
> + /* The report was suppressed, do nothing. */
> + if (s->ibi_data.ibi_nacked && !s->ibi_data.notify_ibi_nack) {
> + ARRAY_FIELD_DP32(s->regs, PRESENT_STATE, CM_TFR_ST_STATUS,
> + ASPEED_I3C_TRANSFER_STATE_IDLE);
> + ARRAY_FIELD_DP32(s->regs, PRESENT_STATE, CM_TFR_STATUS,
> + ASPEED_I3C_TRANSFER_STATUS_IDLE);
> + return;
> + }
> +
> + /* If we don't have any slices to push, just push the status. */
> + if (num_slices == 0) {
> + s->ibi_data.ibi_queue_status =
> + FIELD_DP32(s->ibi_data.ibi_queue_status, IBI_QUEUE_STATUS,
> + LAST_STATUS, 1);
> + fifo32_push(&s->ibi_queue, s->ibi_data.ibi_queue_status);
> + ibi_status_count = 1;
> + }
> +
> + for (uint8_t i = 0; i < num_slices; i++) {
> + /* If this is the last slice, set LAST_STATUS. */
> + if (fifo8_num_used(&s->ibi_data.ibi_intermediate_queue) <
> + ibi_slice_size) {
> + s->ibi_data.ibi_queue_status =
> + FIELD_DP32(s->ibi_data.ibi_queue_status, IBI_QUEUE_STATUS,
> + IBI_DATA_LEN,
> +
> fifo8_num_used(&s->ibi_data.ibi_intermediate_queue));
> + s->ibi_data.ibi_queue_status =
> + FIELD_DP32(s->ibi_data.ibi_queue_status, IBI_QUEUE_STATUS,
> + LAST_STATUS, 1);
> + } else {
> + s->ibi_data.ibi_queue_status =
> + FIELD_DP32(s->ibi_data.ibi_queue_status, IBI_QUEUE_STATUS,
> + IBI_DATA_LEN, ibi_slice_size);
> + }
> +
> + /* Push the IBI status header. */
> + fifo32_push(&s->ibi_queue, s->ibi_data.ibi_queue_status);
> + /* Move each IBI byte into a 32-bit word and push it into the queue.
> */
> + for (uint8_t j = 0; j < ibi_slice_size; ++j) {
> + if (fifo8_is_empty(&s->ibi_data.ibi_intermediate_queue)) {
> + break;
> + }
> +
> + ibi_data.b[j & 3] =
> fifo8_pop(&s->ibi_data.ibi_intermediate_queue);
> + /* We have 32-bits, push it to the IBI FIFO. */
> + if ((j & 0x03) == 0x03) {
> + fifo32_push(&s->ibi_queue, ibi_data.val32);
> + ibi_data.val32 = 0;
> + }
> + }
You'll probably want to handle the IBI_PEC_EN DAT field when pushing the
IBI to the fifo here.
Due to a HW errata, the driver will *always* need to enable PEC_EN. In
cases where the remote isn't actually sending a PEC, this will consume
the last byte of the IBI payload (and probably cause a PEC error, which
the driver needs to ignore).
See here for the driver side, in patches 4/5 and 5/5:
https://lore.kernel.org/linux-i3c/d5d76a8d2336d2a71886537f42e71d51db184df6.1680161823.git...@codeconstruct.com.au/T/#u
Cheers,
Jeremy
