Driver for the I3C Controller IP Core

[gastmaier]

PR Description

Creating this PR to get some preliminary feedback and because I'm sitting on it for a while without making changes.
It is divided in two commits:

• The first implements the mandatory ops
• The second implements the optional ops, all In-band interrupt related

Even though the IP Core has an Offload engine, I didn't try to implement it, because the SPI Engine Offload is being/has been reworked and I would rather wait the final implementation to settle before trying to do the same on the I3C abstraction.

Using the upstream master/slave nomenclature for coherence, our IP core uses the spec controller/peripheral.
I think we should only rename it when upstream updates the already upstream drivers.

Something that might catch the attention of reviewers is the call-back/entry point for IBIs,
That is because it is allocated per driver device, along these lines:

// my_device_i3c.c

static void my_device_i3c_ibi_handler(struct i3c_device *dev,
                                      const struct i3c_ibi_payload *payload)
{
        printk("Got interrupt payload of length %u", payload->len);
}


static int my_device_i3c_probe(struct i3c_device *i3cdev)
{
        int ret;
        struct i3c_ibi_setup *ibireq;
        const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
                                                             my_device_i3c_ids);

        if (!id)
                return PTR_ERR(id);

        ibireq = kzalloc(sizeof(struct i3c_ibi_setup), GFP_KERNEL);
        ibireq->max_payload_len = 1;
        ibireq->num_slots = 1;
        ibireq->handler = my_device_i3c_ibi_handler;
        ret = i3c_device_request_ibi(i3cdev, ibireq);
        kfree(ibireq);
        if (ret) {
                dev_err(&i3cdev->dev, "Failed to request i3c ibi %d\n",
                        ret);
        }

        ret = i3c_device_enable_ibi(i3cdev);
        if (ret) {
                dev_err(&i3cdev->dev, "Failed to enable i3c ibi %d\n",
                        ret);
        }

        return my_device_core_probe(&i3cdev->dev, ..., (uintptr_t)id->data);
}

PR Type

• Bug fix (a change that fixes an issue)
• New feature (a change that adds new functionality)
• Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

• I have conducted a self-review of my own code changes
• I have tested the changes on the relevant hardware
• I have updated the documentation outside this repo accordingly (if there is the case)

[commodo]

any chance of this going into an infinite loop?
if yes, maybe add a timeout?

[gastmaier]

No, for each command CMD sent, one command receipt CMDR is received.

[commodo]

this may be a lot of work for an IRQ handler? (not sure, just guessing here);
if it is too much work (squeezed into an IRQ handler) it may make sense to add a threaded_irq handler too;

(still open for debate on this point)

[gastmaier]

Expected number of AXI R/W per interrupt type:

• CMDR: 2 Reads (4-bytes from the CMDR plus 4-bytes of SDI data) + [2x#CMD Writes from next transfer, if any]
• IBI: 1 Read (4-bytes)
• DAA: 2 Reads + 1 Write (6-bytes R, 1-byte W)

Plus 1 Write to clear the interrupt.

2-bytes per CMD is always less than SPI Engine's commands {[REG_CONFIG;REG_CLK_DIV,REG_WORD_LEN];CS;Transfer;Sleep;CS;SYNC}

[commodo]

i'm curios if this is correct;
AFAICT, adi_i3c_master_queue_xfer() may have added this to a list, so, now it's free'd?

[commodo]

so, the idea that makes it hard (for me to visualize this, is): when wait_for_completion_timeout() finishes, is there a chance that xfer will still be part of a list?

the whole mechanism from adi_i3c_master_queue_xfer() && adi_i3c_master_unqueue_xfer() (see below example from adi_i3c_master_unqueue_xfer()), makes it hard to say if list_del_init() has been called when we get here:

	if (master->xferqueue.cur == xfer)
		master->xferqueue.cur = NULL;
	else
		list_del_init(&xfer->node)

it may be an idea to move the kfree handling in the same place where list_del_init() (and other stuff) happens;
that may make it more difficult to get the xfer->ret value and the other errors;

what about doing it the other way around?
pass xfer->cmd = cmd (at the start of the transfer) and work on it inside adi_i3c_master_unqueue_xfer() and other handlers

[gastmaier]

Design flaws TLDR:

• no FIFO flushing on unqueue_xfer (fixed by sw core reset).
• wait_for_completion_timeout is blocking (seem to be the normal).

---

current transfer: master->xferqueue.cur
queue: master->xferqueue

Pipeline:
When a xfer is queued:

• If current transfer is null, set it to xfer and start transfer.
• If not, append to xfer to queue.

When a xfer finishes, triggered by the first CMDR received and iterated through all CMDR (bus transfers are faster than AXI reads), get the next xfer from the queue and:

• if it is not null; remove xfer from queue and set it as current transfer and start transfer.
• if it is null, current transfer is set null

Finally, in normal operation wait_for_completion_timeout should never expire, but if it does:

• if xfer is current transfer, set it to null
• If not, remove the node from the queued list

A flaw of the flow is that the FIFOs are not flushed when unqueue_xfer is called; so if a CMDR from current transfer arrives (super) late it will "sit" on the FIFO and miss-belong to the next xfer.
Still, wait_for_completion_timeout should never happen since the bus never hangs (unlike I²C), unless there is a bug in the HDL.

So, overall the solution is to flush the FIFOs on timeout? As is, a core reset will flush all FIFOs

For the o.g. questions

AFAICT, adi_i3c_master_queue_xfer() may have added this to a list, so, now it's free'd?

Even if it is added to a list, the completion is only satisfied on complete(&xfer->comp) at end_xfer_locked

when wait_for_completion_timeout() finishes, is there a chance that xfer will still be part of a list?

• In normal operation (no timeout) no, it exed as current transfer before end_xfer_locked
• on timeout, no, unqueue_xfer removed it form the list.

---

At driver level, we call either i3c_device_do_priv_xfers, passing a buffer at the each xfer of the xfers array;
or even use the regmap API, e.g, regmap_raw_read; both eventually call the controller priv_xfers.

[commodo]

same comment here;
AFAICT, adi_i3c_master_queue_xfer() may have added this to a list, so, now it's free'd?`

[gastmaier]

See long comment.

[commodo]

same comment here
AFAICT, adi_i3c_master_queue_xfer() may have added this to a list, so, now it's free'd?

[gastmaier]

See long comment.

[commodo]

this ret is assigned bu never used from here-on

[gastmaier]

Done
There was also an id assigned but never used that I removed.
And a commented //sleep that I swear I had removed

[commodo]

return i3c_master_enec_locked() directly?
or will there be more code added after this later?

[gastmaier]

Sure, done

[commodo]

nitpick: null-terminators don't need trailing comma

[gastmaier]

Done