Re: [PATCH 09/17] i3c: mipi-i3c-hci: Process multiple IBIs per interrupt

[Adrian Hunter]

On 14/09/2026 19:45, Frank Li wrote:
> On Mon, Sep 14, 2026 at 02:29:55PM +0300, Adrian Hunter wrote:
>> INTR_IBI_READY indicates that one or more IBI Status Descriptors are
>> present in the IBI ring.  After clearing interrupt status, the interrupt
>> handler processes only a single IBI even though additional IBIs may
>> already be queued.
>>
>> The controller does not reassert INTR_IBI_READY solely because entries
>> remain in the ring after interrupt status has been cleared.  Consequently,
>> the remaining IBIs are not processed until some later interrupt occurs,
>> and can accumulate if IBIs arrive more often than the interrupt handler
>> runs.
>>
>> Process all IBIs that are pending when the handler runs: capture the IBI
>> enqueue pointer up front and continue until the dequeue pointer reaches
>> that position.
>>
>> Fixes: 9ad9a52cce28 ("i3c/master: introduce the mipi-i3c-hci driver")
>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> ---
>>  drivers/i3c/master/mipi-i3c-hci/dma.c | 39 +++++++++++++++++----------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> index 5b195978f376..a798b0648922 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -868,25 +868,24 @@ static void hci_dma_recycle_ibi_slot(struct i3c_hci *hci,
>>  	i3c_generic_ibi_recycle_slot(dev_ibi->pool, slot);
>>  }
>>
>> -static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>> +static bool hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh,
>> +				u32 *op1_val, unsigned int enq_ptr)
>>  {
>>  	struct hci_rings_data *rings = hci->io_data;
>>  	struct i3c_dev_desc *dev;
>>  	struct i3c_hci_dev_data *dev_data;
>>  	struct hci_dma_dev_ibi_data *dev_ibi;
>>  	struct i3c_ibi_slot *slot;
>> -	u32 op1_val, op2_val, ibi_status_error;
>> -	unsigned int ptr, enq_ptr, deq_ptr;
>> +	u32 ibi_status_error;
>> +	unsigned int ptr, deq_ptr;
>>  	unsigned int ibi_size, ibi_chunks, ibi_data_offset, first_part;
>>  	int ibi_addr, last_ptr;
>>  	void *ring_ibi_data;
>>  	dma_addr_t ring_ibi_data_dma;
>>
>> -	op1_val = rh_reg_read(RING_OPERATION1);
>> -	deq_ptr = FIELD_GET(RING_OP1_IBI_DEQ_PTR, op1_val);
>> -
>> -	op2_val = rh_reg_read(RING_OPERATION2);
>> -	enq_ptr = FIELD_GET(RING_OP2_IBI_ENQ_PTR, op2_val);
>> +	deq_ptr = FIELD_GET(RING_OP1_IBI_DEQ_PTR, *op1_val);
>> +	if (deq_ptr == enq_ptr)
>> +		return false;
>>
>>  	ibi_status_error = 0;
>>  	ibi_addr = -1;
>> @@ -936,7 +935,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>>  		dev_dbg(&hci->master.dev,
>>  			"no LAST_STATUS available (e=%d d=%d)",
>>  			enq_ptr, deq_ptr);
>> -		return;
>> +		return false;
>>  	}
>>  	deq_ptr = last_ptr + 1;
>>  	deq_ptr %= rh->ibi_status_entries;
>> @@ -1015,10 +1014,9 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>>  	i3c_master_queue_ibi(dev, slot);
>>
>>  done:
>> -	op1_val = rh_reg_read(RING_OPERATION1);
>> -	op1_val &= ~RING_OP1_IBI_DEQ_PTR;
>> -	op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
>> -	rh_reg_write(RING_OPERATION1, op1_val);
>> +	*op1_val &= ~RING_OP1_IBI_DEQ_PTR;
>> +	*op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
>> +	rh_reg_write(RING_OPERATION1, *op1_val);
>>
>>  	/* update the chunk pointer */
>>  	rh->ibi_chunk_ptr += ibi_chunks;
>> @@ -1026,6 +1024,19 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>>
>>  	/* and tell the hardware about freed chunks */
>>  	rh_reg_write(CHUNK_CONTROL, rh_reg_read(CHUNK_CONTROL) + ibi_chunks);
>> +
>> +	return true;
>> +}
>> +
>> +static void hci_dma_drain_ibi_ring(struct i3c_hci *hci, struct hci_rh_data *rh)
>> +{
>> +	u32 op1_val = rh_reg_read(RING_OPERATION1);
>> +	u32 op2_val = rh_reg_read(RING_OPERATION2);
>> +	unsigned int enq_ptr = FIELD_GET(RING_OP2_IBI_ENQ_PTR, op2_val);
>> +
>> +	/* Loop is bounded by enq_ptr. Further IBIs will re-assert INTR_IBI_READY */
>> +	while (hci_dma_process_ibi(hci, rh, &op1_val, enq_ptr))
>> +		;
> 
> You just read once RING_OPERATION1, I think you can read in RING_OPERATION1
> loop, utils hardware not pending irqs.

I think you mean RING_OPERATION2.  That would lengthen the IRQ
handling, potentially making it unbounded under continuous IBI
traffic.  It isn't necessary because a new IBI raises a new
interrupt.  I would want to see a real use-case where it makes
any difference before trying to process even more IBIs in the
same IRQ.

> 
> And I am not sure if op2_val is always mach op1_val by two 32bit read.
> is it possible op1_value changed, when read op2_val?

op1_val cannot change between the two reads.  Every field of
RING_OPERATION1 is written by software, never by the controller,
and it is always protected by hci->lock spinlock.

RING_OP2_IBI_ENQ_PTR can advance at any time, since that is the hardware
side, read-only, updated by the Host Controller, but the two values do
not need to be a consistent snapshot.  enq_ptr is used only as a bound on
how far to dequeue.  Anything the controller enqueues after we have read
it is simply left for the next interrupt: the IBI Status Descriptor is
enqueued after we cleared INTR_STATUS, so INTR_IBI_READY is asserted
again.

> 
> A new irq status may be trigger later, but it become empty ops at irq
> handle.

Yes.  If an IBI is enqueued after INTR_STATUS was cleared but before
RING_OPERATION2 is read, it is processed here and the interrupt for it
arrives afterwards.  Then it finds nothing to do, but IRQ_HANDLED is
still returned because INTR_STATUS was non-zero, so shared interrupt
handling is unaffected.