Driver for the I3C Controller IP Core

[gastmaier]

PR Description

Even though the IP Core has an Offload engine, I didn't implement it here to not make things harder upstream; As is, matches the current abstraction methods.

Using the upstream master/slave nomenclature for coherence, our IP core uses the spec controller/peripheral.

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

[gastmaier]

Forced pushed to resolve conflict at MAINTAINERS file due to rebase on the main branch + some I2C rework.

[gastmaier]

Known design issue under fix elaboration:
Each cmd raises one cmdr, but as is the linux driver expects one cmdr per xfer.
I'm looking into changing the linux driver to receive one cmdr per cmd, hence one irq per cmd.
Since the protocol is fairly slow, we shouldn't miss irq, but I'm testing to ensure.

[gastmaier]

V0->V1

Changes:

• Read one CMDR per CMD instead of XFER.
• Clear CMDR pending only after reading the CMDR FIFO.
• Clean-up unused defines.
• Add CMDR_UDA_ERROR to error check.
• Rempve ret from i3c_master_unregister

Why:

• The controller yields one CMDR per CMD, and not per XFER.
• The CMDR pending irq is only deserted if the CMDR FIFO is empty.
• -
• To propagate the error, but should never occur.
• Changed to void on upstream

[gastmaier]

Will upstream the driver, no reason to merge this to main