[PATCH v1 0/2] svc-i3c-master: Reduce IBI transaction time

[PATCH v1 0/2] svc-i3c-master: Reduce IBI transaction time

[Stanley Chu]

This patchset reduces the IBI transaction time by the following
improvements.
1. Receive the request in interrupt context.
2. Emit the STOP as soon as possible.

Stanley Chu (2):
  i3c: master: svc: Receive IBI requests in interrupt context
  i3c: master: svc: Emit STOP asap in the IBI transaction

 drivers/i3c/master/svc-i3c-master.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] i3c: master: svc: Receive IBI requests in interrupt context

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

Moving the job from workqueue to ISR for two reasons.

1. Improve bus utilization.
If the requests are postponed to be received in the workqueue thread,
the SDA line remains low for a long time while the system loading is
high. During this period, the bus is not available for other targets
to raise requests.

2. Ensure prompt response to requests.
For timing-critical requests, the target may encouter a failure or the
event is missed if the request is not received in time.

IBI request is short, ISR can receive the data quickly and then queue a
work to handle it in the bottom half.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 85e16de208d3..7ceaf3ec45bb 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -201,7 +201,6 @@ struct svc_i3c_drvdata {
  * @addrs: Array containing the dynamic addresses of each attached device
  * @descs: Array of descriptors, one per attached device
  * @hj_work: Hot-join work
- * @ibi_work: IBI work
  * @irq: Main interrupt
  * @pclk: System clock
  * @fclk: Fast clock (bus)
@@ -229,7 +228,6 @@ struct svc_i3c_master {
 	u8 addrs[SVC_I3C_MAX_DEVS];
 	struct i3c_dev_desc *descs[SVC_I3C_MAX_DEVS];
 	struct work_struct hj_work;
-	struct work_struct ibi_work;
 	int irq;
 	struct clk *pclk;
 	struct clk *fclk;
@@ -487,9 +485,8 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta
 	return ret;
 }
 
-static void svc_i3c_master_ibi_work(struct work_struct *work)
+static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master)
 {
-	struct svc_i3c_master *master = container_of(work, struct svc_i3c_master, ibi_work);
 	struct svc_i3c_i2c_dev_data *data;
 	unsigned int ibitype, ibiaddr;
 	struct i3c_dev_desc *dev;
@@ -504,7 +501,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	 * schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if
 	 * any irq or schedule happen during transaction.
 	 */
-	guard(spinlock_irqsave)(&master->xferqueue.lock);
+	guard(spinlock)(&master->xferqueue.lock);
 
 	/*
 	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
@@ -530,7 +527,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	if (ret) {
 		dev_err(master->dev, "Timeout when polling for IBIWON\n");
 		svc_i3c_master_emit_stop(master);
-		goto reenable_ibis;
+		return;
 	}
 
 	status = readl(master->regs + SVC_I3C_MSTATUS);
@@ -574,7 +571,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 
 		svc_i3c_master_emit_stop(master);
 
-		goto reenable_ibis;
+		return;
 	}
 
 	/* Handle the non critical tasks */
@@ -597,9 +594,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	default:
 		break;
 	}
-
-reenable_ibis:
-	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
 }
 
 static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
@@ -618,10 +612,12 @@ static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
 	    !SVC_I3C_MSTATUS_STATE_SLVREQ(active))
 		return IRQ_HANDLED;
 
-	svc_i3c_master_disable_interrupts(master);
-
-	/* Handle the interrupt in a non atomic context */
-	queue_work(master->base.wq, &master->ibi_work);
+	/*
+	 * The SDA line remains low until the request is processed.
+	 * Receive the request in the interrupt context to respond promptly
+	 * and restore the bus to idle state.
+	 */
+	svc_i3c_master_ibi_isr(master);
 
 	return IRQ_HANDLED;
 }
@@ -1947,7 +1943,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 		return ret;
 
 	INIT_WORK(&master->hj_work, svc_i3c_master_hj_work);
-	INIT_WORK(&master->ibi_work, svc_i3c_master_ibi_work);
 	mutex_init(&master->lock);
 
 	ret = devm_request_irq(dev, master->irq, svc_i3c_master_irq_handler,
-- 
2.34.1

[PATCH v1 2/2] i3c: master: svc: Emit STOP asap in the IBI transaction

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

Queuing the IBI request does not need to be done earlier than emitting
the STOP. Emitting STOP immediately after receiving the IBI request can
complete the IBI transaction earlier and return the bus to idle.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 7ceaf3ec45bb..9b23239ad8db 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -577,11 +577,11 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master)
 	/* Handle the non critical tasks */
 	switch (ibitype) {
 	case SVC_I3C_MSTATUS_IBITYPE_IBI:
+		svc_i3c_master_emit_stop(master);
 		if (dev) {
 			i3c_master_queue_ibi(dev, master->ibi.tbq_slot);
 			master->ibi.tbq_slot = NULL;
 		}
-		svc_i3c_master_emit_stop(master);
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
 		svc_i3c_master_emit_stop(master);
-- 
2.34.1

Re: [PATCH v1 0/2] svc-i3c-master: Reduce IBI transaction time

[Miquel Raynal]

On 15/04/2025 at 13:18:06 +08, Stanley Chu <stanley.chuys@gmail.com> wrote:

> This patchset reduces the IBI transaction time by the following
> improvements.
> 1. Receive the request in interrupt context.

I initially had a few concerns about that, especially since the wait
periods were bounded to 1s, but actually we are already in the irqsave
situation when running this code, so your series might not have
such a huge system-wide performance impact in the end.

> 2. Emit the STOP as soon as possible.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v1 1/2] i3c: master: svc: Receive IBI requests in interrupt context

[Frank Li]

On Tue, Apr 15, 2025 at 01:18:07PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> Moving the job from workqueue to ISR for two reasons.
>
> 1. Improve bus utilization.
> If the requests are postponed to be received in the workqueue thread,
> the SDA line remains low for a long time while the system loading is
> high. During this period, the bus is not available for other targets
> to raise requests.
>
> 2. Ensure prompt response to requests.
> For timing-critical requests, the target may encouter a failure or the
> event is missed if the request is not received in time.
>
> IBI request is short, ISR can receive the data quickly and then queue a
> work to handle it in the bottom half.
>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master/svc-i3c-master.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 85e16de208d3..7ceaf3ec45bb 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -201,7 +201,6 @@ struct svc_i3c_drvdata {
>   * @addrs: Array containing the dynamic addresses of each attached device
>   * @descs: Array of descriptors, one per attached device
>   * @hj_work: Hot-join work
> - * @ibi_work: IBI work
>   * @irq: Main interrupt
>   * @pclk: System clock
>   * @fclk: Fast clock (bus)
> @@ -229,7 +228,6 @@ struct svc_i3c_master {
>  	u8 addrs[SVC_I3C_MAX_DEVS];
>  	struct i3c_dev_desc *descs[SVC_I3C_MAX_DEVS];
>  	struct work_struct hj_work;
> -	struct work_struct ibi_work;
>  	int irq;
>  	struct clk *pclk;
>  	struct clk *fclk;
> @@ -487,9 +485,8 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta
>  	return ret;
>  }
>
> -static void svc_i3c_master_ibi_work(struct work_struct *work)
> +static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master)
>  {
> -	struct svc_i3c_master *master = container_of(work, struct svc_i3c_master, ibi_work);
>  	struct svc_i3c_i2c_dev_data *data;
>  	unsigned int ibitype, ibiaddr;
>  	struct i3c_dev_desc *dev;
> @@ -504,7 +501,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	 * schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if
>  	 * any irq or schedule happen during transaction.
>  	 */
> -	guard(spinlock_irqsave)(&master->xferqueue.lock);
> +	guard(spinlock)(&master->xferqueue.lock);
>
>  	/*
>  	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
> @@ -530,7 +527,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	if (ret) {
>  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
>  		svc_i3c_master_emit_stop(master);
> -		goto reenable_ibis;
> +		return;
>  	}
>
>  	status = readl(master->regs + SVC_I3C_MSTATUS);
> @@ -574,7 +571,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>
>  		svc_i3c_master_emit_stop(master);
>
> -		goto reenable_ibis;
> +		return;
>  	}
>
>  	/* Handle the non critical tasks */
> @@ -597,9 +594,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	default:
>  		break;
>  	}
> -
> -reenable_ibis:
> -	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
>  }
>
>  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> @@ -618,10 +612,12 @@ static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
>  	    !SVC_I3C_MSTATUS_STATE_SLVREQ(active))
>  		return IRQ_HANDLED;
>
> -	svc_i3c_master_disable_interrupts(master);
> -
> -	/* Handle the interrupt in a non atomic context */
> -	queue_work(master->base.wq, &master->ibi_work);
> +	/*
> +	 * The SDA line remains low until the request is processed.
> +	 * Receive the request in the interrupt context to respond promptly
> +	 * and restore the bus to idle state.
> +	 */
> +	svc_i3c_master_ibi_isr(master);
>
>  	return IRQ_HANDLED;
>  }
> @@ -1947,7 +1943,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  		return ret;
>
>  	INIT_WORK(&master->hj_work, svc_i3c_master_hj_work);
> -	INIT_WORK(&master->ibi_work, svc_i3c_master_ibi_work);
>  	mutex_init(&master->lock);
>
>  	ret = devm_request_irq(dev, master->irq, svc_i3c_master_irq_handler,
> --
> 2.34.1
>

Re: [PATCH v1 2/2] i3c: master: svc: Emit STOP asap in the IBI transaction

[Frank Li]

On Tue, Apr 15, 2025 at 01:18:08PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> Queuing the IBI request does not need to be done earlier than emitting
> the STOP. Emitting STOP immediately after receiving the IBI request can
> complete the IBI transaction earlier and return the bus to idle.
>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i3c/master/svc-i3c-master.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 7ceaf3ec45bb..9b23239ad8db 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -577,11 +577,11 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master)
>  	/* Handle the non critical tasks */
>  	switch (ibitype) {
>  	case SVC_I3C_MSTATUS_IBITYPE_IBI:
> +		svc_i3c_master_emit_stop(master);
>  		if (dev) {
>  			i3c_master_queue_ibi(dev, master->ibi.tbq_slot);
>  			master->ibi.tbq_slot = NULL;
>  		}
> -		svc_i3c_master_emit_stop(master);
>  		break;
>  	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
>  		svc_i3c_master_emit_stop(master);
> --
> 2.34.1
>

Re: [PATCH v1 0/2] svc-i3c-master: Reduce IBI transaction time

[Stanley Chu]

On Tue, Apr 15, 2025 at 4:31 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> On 15/04/2025 at 13:18:06 +08, Stanley Chu <stanley.chuys@gmail.com> wrote:
>
> > This patchset reduces the IBI transaction time by the following
> > improvements.
> > 1. Receive the request in interrupt context.
>
> I initially had a few concerns about that, especially since the wait
> periods were bounded to 1s, but actually we are already in the irqsave

Hi Miquel,
The poll timeout is 1 ms. Normally, the IBI can be completed in a few us.

Thanks,
Stanley

> situation when running this code, so your series might not have
> such a huge system-wide performance impact in the end.
>
> > 2. Emit the STOP as soon as possible.
>
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Thanks,
> Miquèl

Re: [PATCH v1 0/2] svc-i3c-master: Reduce IBI transaction time

[Alexandre Belloni]

On Tue, 15 Apr 2025 13:18:06 +0800, Stanley Chu wrote:
> This patchset reduces the IBI transaction time by the following
> improvements.
> 1. Receive the request in interrupt context.
> 2. Emit the STOP as soon as possible.
> 
> Stanley Chu (2):
>   i3c: master: svc: Receive IBI requests in interrupt context
>   i3c: master: svc: Emit STOP asap in the IBI transaction
> 
> [...]

Applied, thanks!

[1/2] i3c: master: svc: Receive IBI requests in interrupt context
      https://git.kernel.org/abelloni/c/8d29fa6d921c
[2/2] i3c: master: svc: Emit STOP asap in the IBI transaction
      https://git.kernel.org/abelloni/c/81f2a9af9821

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com