[PATCH v5 0/6] I3C: master: svc: collect all patches to improve hotjoin stability

[PATCH v5 0/6] I3C: master: svc: collect all patches to improve hotjoin stability

[Frank Li]

This patches is splited from
https://lore.kernel.org/linux-i3c/ZvrAuOBLgi+HtrPD@lizhi-Precision-Tower-5810/T/#t

It needs more discussion about dt assign address issue about i3c framework.

This series is svc driver improvement and bug fixes for hotjoin. It fixes
all kinds hotjoin problem when 2 devices random hotjoin and A normal data
transfer is on going.

This patch version start from v5, which exact the same as old series's v4.

See each patches for detail issue.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (6):
      i3c: master: svc: use repeat start when IBI WIN happens
      i3c: master: svc: manually emit NACK/ACK for hotjoin
      i3c: master: svc: need check IBIWON for dynamtica address assign
      i3c: master: svc: use spin_lock_irqsave at svc_i3c_master_ibi_work()
      i3c: master: svc: wait for Manual ACK/NACK Done before next step
      i3c: master: svc: fix possible assignment of the same address to two devices

 drivers/i3c/master/svc-i3c-master.c | 122 +++++++++++++++++++++++++++---------
 1 file changed, 93 insertions(+), 29 deletions(-)
---
base-commit: 77df9e4bb2224d8ffbddec04c333a9d7965dad6c
change-id: 20241001-svc-i3c-hj-bbaba68cf1ea

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v5 1/6] i3c: master: svc: use repeat start when IBI WIN happens

[Frank Li]

There is a possibility of an IBI WIN occurring when addressing issues, even
when sending CCC commands. Most of the time, returning -EAGAIN is
acceptable, but the case below becomes highly complex.

When a Hotjoin event occurs:
- i3c_master_do_daa()
  - i3c_master_add_i3c_dev_locked()
    - A dynamic address (e.g., 0x9) is already set during DAA.
    - i3c_master_getpid_locked()
      - Another device issues HJ or IBI here. Returning -EAGAIN causes
        failure in adding the new device. However, the dynamic address(0x9)
        has already been assigned to this device. If another device issues
        HJ, it will get this address 0x9 again, causing two devices on the
        bus to use the same dynamic address 0x9.
      - Attempting to send RSTDAA when the first device fails at
        i3c_master_getpid_locked() could also fail when sending RSTDAA for
        the same reason.

According to the I3C spec, address arbitration only happens at START, never
at REPEAT start. Using repeat start when an IBI WIN occurs simplifies this
case, as i3c_master_getpid_locked() will not return an error when another
device tries to send HJ or IBI.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v3 to v4
- add miquel's ack tag
---
 drivers/i3c/master/svc-i3c-master.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index a7bfc678153e6..7cd3ce2643f1a 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1163,6 +1163,24 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		if (ret)
 			goto emit_stop;
 
+		/*
+		 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a
+		 * Frame with I3C Target Address.
+		 *
+		 * The I3C Controller normally should start a Frame, the Address may be arbitrated,
+		 * and so the Controller shall monitor to see whether an In-Band Interrupt request,
+		 * a Controller Role Request (i.e., Secondary Controller requests to become the
+		 * Active Controller), or a Hot-Join Request has been made.
+		 *
+		 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, issue
+		 * repeat start. Address arbitrate only happen at START, never happen at REPEAT
+		 * start.
+		 */
+		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+			writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+			continue;
+		}
+
 		if (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_NACK) {
 			/*
 			 * According to I3C Spec 1.1.1, 11-Jun-2021, section: 5.1.2.2.3.
@@ -1196,24 +1214,6 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		}
 	}
 
-	/*
-	 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a Frame
-	 * with I3C Target Address.
-	 *
-	 * The I3C Controller normally should start a Frame, the Address may be arbitrated, and so
-	 * the Controller shall monitor to see whether an In-Band Interrupt request, a Controller
-	 * Role Request (i.e., Secondary Controller requests to become the Active Controller), or
-	 * a Hot-Join Request has been made.
-	 *
-	 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, return failure
-	 * and yield the above events handler.
-	 */
-	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-		ret = -EAGAIN;
-		*actual_len = 0;
-		goto emit_stop;
-	}
-
 	if (rnw)
 		ret = svc_i3c_master_read(master, in, xfer_len);
 	else

-- 
2.34.1

[PATCH v5 2/6] i3c: master: svc: manually emit NACK/ACK for hotjoin

[Frank Li]

When the address is arbitrated at send address, the hardware can auto-send
NACK if it is an IBI. However, manual emission of NACK/ACK is needed for
hot join or controller request events.

Add help function svc_i3c_master_handle_ibi_won() to check event type and
send out NACK if the event is not an IBI.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v3 to v4
- add Miquel review tag
---
 drivers/i3c/master/svc-i3c-master.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 7cd3ce2643f1a..c35a228f0c2f4 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -409,6 +409,24 @@ static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
 	       master->regs + SVC_I3C_MCTRL);
 }
 
+static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus)
+{
+	u32 ibitype;
+
+	ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus);
+
+	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
+	/* Hardware can't auto emit NACK for hot join and master request */
+	switch (ibitype) {
+	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
+	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
+		svc_i3c_master_nack_ibi(master);
+	}
+
+	return 0;
+}
+
 static void svc_i3c_master_ibi_work(struct work_struct *work)
 {
 	struct svc_i3c_master *master = container_of(work, struct svc_i3c_master, ibi_work);
@@ -1177,7 +1195,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		 * start.
 		 */
 		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+			svc_i3c_master_handle_ibi_won(master, reg);
 			continue;
 		}
 

-- 
2.34.1

[PATCH v5 3/6] i3c: master: svc: need check IBIWON for dynamtica address assign

[Frank Li]

When sending REQUEST_PROC_DAA, emit START and address 7E. Address
arbitration may occur at this time if other devices trigger HJ, IBI, or
CR events.

When IBIWON happen during a REQUEST_PROC_DAA, NACK the IBI request then
send a repeated start to continue current dynamtica address assign.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v3 to v4
- rework commit message
---
 drivers/i3c/master/svc-i3c-master.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index c35a228f0c2f4..5df0ec02d73ce 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -872,6 +872,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 	int ret, i;
 
 	while (true) {
+		/* clean SVC_I3C_MINT_IBIWON w1c bits */
+		writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
 		/* SVC_I3C_MCTRL_REQUEST_PROC_DAA have two mode, ENTER DAA or PROCESS DAA.
 		 *
 		 * ENTER DAA:
@@ -923,6 +926,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 			ret = svc_i3c_master_readb(master, data, 2);
 			if (ret)
 				break;
+		} else if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+			svc_i3c_master_handle_ibi_won(master, reg);
+			continue;
 		} else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) {
 			if (SVC_I3C_MSTATUS_STATE_IDLE(reg) &&
 			    SVC_I3C_MSTATUS_COMPLETE(reg)) {

-- 
2.34.1

[PATCH v5 4/6] i3c: master: svc: use spin_lock_irqsave at svc_i3c_master_ibi_work()

[Frank Li]

According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:

The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of
I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be
disabled to prevent schedule during the whole I3C transaction, otherwise,
the I3C bus timeout may happen if any irq or schedule happen during
transaction.

Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than
100us.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3-v4
- improve commit message
- needn't mutex here, other place already use spin_lock_saveirq to protent
i3c transfer.
---
 drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 5df0ec02d73ce..1ee6ce186195c 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -436,7 +436,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	u32 status, val;
 	int ret;
 
-	mutex_lock(&master->lock);
+	/*
+	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
+	 *
+	 * The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of I3C/I2C
+	 * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent
+	 * 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);
+
 	/*
 	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
 	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
@@ -456,7 +465,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	       master->regs + SVC_I3C_MCTRL);
 
 	/* Wait for IBIWON, should take approximately 100us */
-	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
+	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
 					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
 	if (ret) {
 		dev_err(master->dev, "Timeout when polling for IBIWON\n");
@@ -529,7 +538,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 
 reenable_ibis:
 	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
-	mutex_unlock(&master->lock);
 }
 
 static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)

-- 
2.34.1

[PATCH v5 5/6] i3c: master: svc: wait for Manual ACK/NACK Done before next step

[Frank Li]

Wait for the controller to complete emitting ACK/NACK, otherwise the next
command may be omitted by the hardware.

A "command done" check in svc_i3c_master_nack(ack)_ibi() and change the
return type to int to flag possible timeouts.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3 to v4
- rework commit message
- add miquel's review tag
- directly return readl(...)
---
 drivers/i3c/master/svc-i3c-master.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 1ee6ce186195c..3388c9af63fcc 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -388,10 +388,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
 	return 0;
 }
 
-static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
+static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
 				   bool mandatory_byte)
 {
 	unsigned int ibi_ack_nack;
+	u32 reg;
 
 	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
 	if (mandatory_byte)
@@ -400,18 +401,30 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
 		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
 
 	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
+
+	return readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
+					 SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
+
 }
 
-static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
+static int svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
 {
+	int ret;
+	u32 reg;
+
 	writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK |
 	       SVC_I3C_MCTRL_IBIRESP_NACK,
 	       master->regs + SVC_I3C_MCTRL);
+
+	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
+					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
+	return ret;
 }
 
 static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus)
 {
 	u32 ibitype;
+	int ret = 0;
 
 	ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus);
 
@@ -421,10 +434,10 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta
 	switch (ibitype) {
 	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
-		svc_i3c_master_nack_ibi(master);
+		ret = svc_i3c_master_nack_ibi(master);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void svc_i3c_master_ibi_work(struct work_struct *work)
@@ -935,7 +948,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 			if (ret)
 				break;
 		} else if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			svc_i3c_master_handle_ibi_won(master, reg);
+			ret = svc_i3c_master_handle_ibi_won(master, reg);
+			if (ret)
+				break;
 			continue;
 		} else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) {
 			if (SVC_I3C_MSTATUS_STATE_IDLE(reg) &&
@@ -1209,7 +1224,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		 * start.
 		 */
 		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			svc_i3c_master_handle_ibi_won(master, reg);
+			ret = svc_i3c_master_handle_ibi_won(master, reg);
+			if (ret)
+				goto emit_stop;
 			continue;
 		}
 

-- 
2.34.1

[PATCH v5 6/6] i3c: master: svc: fix possible assignment of the same address to two devices

[Frank Li]

svc_i3c_master_do_daa() {
    ...
    for (i = 0; i < dev_nb; i++) {
        ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
        if (ret)
            goto rpm_out;
    }
}

If two devices (A and B) are detected in DAA and address 0xa is assigned to
device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked()
for device A (addr: 0xa) could prevent device B (addr: 0xb) from being
registered on the bus. The I3C stack might still consider 0xb a free
address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A,
causing both devices A and B to use the same address 0xb, violating the I3C
specification.

The return value for i3c_master_add_i3c_dev_locked() should not be checked
because subsequent steps will scan the entire I3C bus, independent of
whether i3c_master_add_i3c_dev_locked() returns success.

If device A registration fails, there is still a chance to register device
B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while
retrieving device information.

Cc: stable@kernel.org
Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v3 to v4
- add comments about not check return value
- add miquel's review tag
---
 drivers/i3c/master/svc-i3c-master.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 3388c9af63fcc..7dc52111ee0f1 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1103,12 +1103,27 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
 	if (ret)
 		goto rpm_out;
 
-	/* Register all devices who participated to the core */
-	for (i = 0; i < dev_nb; i++) {
-		ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
-		if (ret)
-			goto rpm_out;
-	}
+	/*
+	 * Register all devices who participated to the core
+	 *
+	 * If two devices (A and B) are detected in DAA and address 0xa is assigned to
+	 * device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked()
+	 * for device A (addr: 0xa) could prevent device B (addr: 0xb) from being
+	 * registered on the bus. The I3C stack might still consider 0xb a free
+	 * address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A,
+	 * causing both devices A and B to use the same address 0xb, violating the I3C
+	 * specification.
+	 *
+	 * The return value for i3c_master_add_i3c_dev_locked() should not be checked
+	 * because subsequent steps will scan the entire I3C bus, independent of
+	 * whether i3c_master_add_i3c_dev_locked() returns success.
+	 *
+	 * If device A registration fails, there is still a chance to register device
+	 * B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while
+	 * retrieving device information.
+	 */
+	for (i = 0; i < dev_nb; i++)
+		i3c_master_add_i3c_dev_locked(m, addrs[i]);
 
 	/* Configure IBI auto-rules */
 	ret = svc_i3c_update_ibirules(master);

-- 
2.34.1

Re: [PATCH v5 0/6] I3C: master: svc: collect all patches to improve hotjoin stability

[Frank Li]

On Tue, Oct 01, 2024 at 12:02:49PM -0400, Frank Li wrote:
> This patches is splited from
> https://lore.kernel.org/linux-i3c/ZvrAuOBLgi+HtrPD@lizhi-Precision-Tower-5810/T/#t
>
> It needs more discussion about dt assign address issue about i3c framework.
>
> This series is svc driver improvement and bug fixes for hotjoin. It fixes
> all kinds hotjoin problem when 2 devices random hotjoin and A normal data
> transfer is on going.
>
> This patch version start from v5, which exact the same as old series's v4.
>
> See each patches for detail issue.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Frank Li (6):
>       i3c: master: svc: use repeat start when IBI WIN happens
>       i3c: master: svc: manually emit NACK/ACK for hotjoin
>       i3c: master: svc: need check IBIWON for dynamtica address assign

Alex:
	I am not sure why linux-i3c mail list truncate above 3 patches
https://lore.kernel.org/linux-i3c/
	but other mail list include all patches.
https://lore.kernel.org/imx/20241001-svc-i3c-hj-v5-0-480ab8aed849@nxp.com/T/#t

Frank

>       i3c: master: svc: use spin_lock_irqsave at svc_i3c_master_ibi_work()
>       i3c: master: svc: wait for Manual ACK/NACK Done before next step
>       i3c: master: svc: fix possible assignment of the same address to two devices
>
>  drivers/i3c/master/svc-i3c-master.c | 122 +++++++++++++++++++++++++++---------
>  1 file changed, 93 insertions(+), 29 deletions(-)
> ---
> base-commit: 77df9e4bb2224d8ffbddec04c333a9d7965dad6c
> change-id: 20241001-svc-i3c-hj-bbaba68cf1ea
>
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
>

Re: [PATCH v5 3/6] i3c: master: svc: need check IBIWON for dynamtica address assign

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:52 -0400:

> When sending REQUEST_PROC_DAA, emit START and address 7E. Address
> arbitration may occur at this time if other devices trigger HJ, IBI, or
> CR events.
> 
> When IBIWON happen during a REQUEST_PROC_DAA, NACK the IBI request then
> send a repeated start to continue current dynamtica address assign.

					dynamic address assignment?

With this fixed,

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

Thanks,
Miquèl

Re: [PATCH v5 4/6] i3c: master: svc: use spin_lock_irqsave at svc_i3c_master_ibi_work()

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:53 -0400:

> According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> 
> The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of
> I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be
> disabled to prevent schedule during the whole I3C transaction, otherwise,
> the I3C bus timeout may happen if any irq or schedule happen during
> transaction.
> 
> Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than
> 100us.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v3-v4
> - improve commit message
> - needn't mutex here, other place already use spin_lock_saveirq to protent
> i3c transfer.
> ---
>  drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 5df0ec02d73ce..1ee6ce186195c 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -436,7 +436,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	u32 status, val;
>  	int ret;
>  
> -	mutex_lock(&master->lock);
> +	/*
> +	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> +	 *
> +	 * The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of I3C/I2C
> +	 * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent
> +	 * 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);
> +
>  	/*
>  	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
>  	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
> @@ -456,7 +465,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	       master->regs + SVC_I3C_MCTRL);
>  
>  	/* Wait for IBIWON, should take approximately 100us */
> -	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> +	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
>  					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);

If we now are holding a spinlock and expect this to happen within
100us, then I guess the timeout should be reduced?

>  	if (ret) {
>  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
> @@ -529,7 +538,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  
>  reenable_ibis:
>  	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
> -	mutex_unlock(&master->lock);
>  }
>  
>  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> 

Otherwise,

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


Thanks,
Miquèl

Re: [PATCH v5 5/6] i3c: master: svc: wait for Manual ACK/NACK Done before next step

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:54 -0400:

> Wait for the controller to complete emitting ACK/NACK, otherwise the next
> command may be omitted by the hardware.
> 
> A "command done" check in svc_i3c_master_nack(ack)_ibi() and change the

Add a?

> return type to int to flag possible timeouts.
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v3 to v4
> - rework commit message
> - add miquel's review tag
> - directly return readl(...)
> ---
>  drivers/i3c/master/svc-i3c-master.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 1ee6ce186195c..3388c9af63fcc 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -388,10 +388,11 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
>  	return 0;
>  }
>  
> -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
>  				   bool mandatory_byte)
>  {
>  	unsigned int ibi_ack_nack;
> +	u32 reg;
>  
>  	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
>  	if (mandatory_byte)
> @@ -400,18 +401,30 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
>  		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
>  
>  	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
> +
> +	return readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
> +					 SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
> +
>  }
>  
> -static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
> +static int svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
>  {
> +	int ret;
> +	u32 reg;
> +
>  	writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK |
>  	       SVC_I3C_MCTRL_IBIRESP_NACK,
>  	       master->regs + SVC_I3C_MCTRL);
> +
> +	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
> +					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
> +	return ret;
>  }
>  
>  static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus)
>  {
>  	u32 ibitype;
> +	int ret = 0;
>  
>  	ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus);
>  
> @@ -421,10 +434,10 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta
>  	switch (ibitype) {
>  	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
>  	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
> -		svc_i3c_master_nack_ibi(master);
> +		ret = svc_i3c_master_nack_ibi(master);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void svc_i3c_master_ibi_work(struct work_struct *work)
> @@ -935,7 +948,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  			if (ret)
>  				break;
>  		} else if (SVC_I3C_MSTATUS_IBIWON(reg)) {
> -			svc_i3c_master_handle_ibi_won(master, reg);
> +			ret = svc_i3c_master_handle_ibi_won(master, reg);
> +			if (ret)
> +				break;
>  			continue;
>  		} else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) {
>  			if (SVC_I3C_MSTATUS_STATE_IDLE(reg) &&
> @@ -1209,7 +1224,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  		 * start.
>  		 */
>  		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
> -			svc_i3c_master_handle_ibi_won(master, reg);
> +			ret = svc_i3c_master_handle_ibi_won(master, reg);
> +			if (ret)
> +				goto emit_stop;
>  			continue;
>  		}
>  
> 


Thanks,
Miquèl

Re: [PATCH v5 4/6] i3c: master: svc: use spin_lock_irqsave at svc_i3c_master_ibi_work()

[Frank Li]

On Tue, Oct 01, 2024 at 10:15:09PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:53 -0400:
>
> > According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> >
> > The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of
> > I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be
> > disabled to prevent schedule during the whole I3C transaction, otherwise,
> > the I3C bus timeout may happen if any irq or schedule happen during
> > transaction.
> >
> > Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than
> > 100us.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v3-v4
> > - improve commit message
> > - needn't mutex here, other place already use spin_lock_saveirq to protent
> > i3c transfer.
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 5df0ec02d73ce..1ee6ce186195c 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -436,7 +436,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >  	u32 status, val;
> >  	int ret;
> >
> > -	mutex_lock(&master->lock);
> > +	/*
> > +	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> > +	 *
> > +	 * The I3C Controller shall hold SCL low while the Bus is in ACK/NACK Phase of I3C/I2C
> > +	 * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent
> > +	 * 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);
> > +
> >  	/*
> >  	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
> >  	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
> > @@ -456,7 +465,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >  	       master->regs + SVC_I3C_MCTRL);
> >
> >  	/* Wait for IBIWON, should take approximately 100us */
> > -	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> > +	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
> >  					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
>
> If we now are holding a spinlock and expect this to happen within
> 100us, then I guess the timeout should be reduced?

yes, 100 should be enough for timeout. Normal it should be set at 9th SCL.

Frank

>
> >  	if (ret) {
> >  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
> > @@ -529,7 +538,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >
> >  reenable_ibis:
> >  	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
> > -	mutex_unlock(&master->lock);
> >  }
> >
> >  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> >
>
> Otherwise,
>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
>
> Thanks,
> Miquèl
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c