Re: [PATCH 10/16] hw/i3c/aspeed_i3c: Add IBI handling

[Jeremy Kerr]

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.jk@codeconstruct.com.au/T/#u

Cheers,


Jeremy