Each patch is indepedents. See commit message for detail.
Change from v1 to v4.
See each patch notes
If there are not notes between v2 to v4, that's means not change.
Frank Li (6):
i3c: master: svc: fix race condition in ibi work thread
i3c: master: svc: fix wrong data return when IBI happen during start
frame
i3c: master: svc: fix ibi may not return mandatory data byte
i3c: master: svc: fix check wrong status register in irq handler
i3c: master: svc: fix SDA keep low when polling IBIWON timeout happen
i3c: master: svc: fix random hot join failure since timeout error
drivers/i3c/master/svc-i3c-master.c | 54 ++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
--
2.34.1
The ibi work thread operates asynchronously with other transfers, such as
svc_i3c_master_priv_xfers(). Introduce mutex protection to ensure the
completion of the entire i3c/i2c transaction.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Reviewed-by: Miquel Raynal <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v1 to v2
- update commit message
- Add Reviewed-by: Miquel Raynal <[email protected]>
drivers/i3c/master/svc-i3c-master.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index d29de5fe533e6..8cd708c965085 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -174,6 +174,7 @@ struct svc_i3c_regs_save {
* @ibi.slots: Available IBI slots
* @ibi.tbq_slot: To be queued IBI slot
* @ibi.lock: IBI lock
+ * @lock: Transfer lock, protect between IBI work thread and callbacks from master
*/
struct svc_i3c_master {
struct i3c_master_controller base;
@@ -202,6 +203,7 @@ struct svc_i3c_master {
/* Prevent races within IBI handlers */
spinlock_t lock;
} ibi;
+ struct mutex lock;
};
/**
@@ -383,6 +385,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
u32 status, val;
int ret;
+ mutex_lock(&master->lock);
/* Acknowledge the incoming interrupt with the AUTOIBI mechanism */
writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI |
SVC_I3C_MCTRL_IBIRESP_AUTO,
@@ -459,6 +462,7 @@ 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)
@@ -1203,9 +1207,11 @@ static int svc_i3c_master_send_bdcast_ccc_cmd(struct svc_i3c_master *master,
cmd->read_len = 0;
cmd->continued = false;
+ mutex_lock(&master->lock);
svc_i3c_master_enqueue_xfer(master, xfer);
if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
svc_i3c_master_dequeue_xfer(master, xfer);
+ mutex_unlock(&master->lock);
ret = xfer->ret;
kfree(buf);
@@ -1249,9 +1255,11 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
cmd->read_len = read_len;
cmd->continued = false;
+ mutex_lock(&master->lock);
svc_i3c_master_enqueue_xfer(master, xfer);
if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
svc_i3c_master_dequeue_xfer(master, xfer);
+ mutex_unlock(&master->lock);
if (cmd->read_len != xfer_len)
ccc->dests[0].payload.len = cmd->read_len;
@@ -1308,9 +1316,11 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
cmd->continued = (i + 1) < nxfers;
}
+ mutex_lock(&master->lock);
svc_i3c_master_enqueue_xfer(master, xfer);
if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
svc_i3c_master_dequeue_xfer(master, xfer);
+ mutex_unlock(&master->lock);
ret = xfer->ret;
svc_i3c_master_free_xfer(xfer);
@@ -1346,9 +1356,11 @@ static int svc_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
cmd->continued = (i + 1 < nxfers);
}
+ mutex_lock(&master->lock);
svc_i3c_master_enqueue_xfer(master, xfer);
if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
svc_i3c_master_dequeue_xfer(master, xfer);
+ mutex_unlock(&master->lock);
ret = xfer->ret;
svc_i3c_master_free_xfer(xfer);
@@ -1539,6 +1551,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
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,
IRQF_NO_SUSPEND, "svc-i3c-irq", master);
if (ret)
--
2.34.1
┌─────┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┌─────
SCL: ┘ └─────┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┘
───┐ ┌─────┐ ┌─────┐ ┌───────────┐
SDA: └───────────────────────┘ └─────┘ └─────┘ └─────
xxx╱ ╲╱ ╲╱ ╲╱ ╲╱ ╲
: xxx╲IBI ╱╲ Addr(0x0a) ╱╲ RW ╱╲NACK╱╲ S ╱
If an In-Band Interrupt (IBI) occurs and IBI work thread is not immediately
scheduled, when svc_i3c_master_priv_xfers() initiates the I3C transfer and
attempts to send address 0x7e, the target interprets it as an
IBI handler and returns the target address 0x0a.
However, svc_i3c_master_priv_xfers() does not handle this case and proceeds
with other transfers, resulting in incorrect data being returned.
Add IBIWON check in svc_i3c_master_xfer(). In case this situation occurs,
return a failure to the driver.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Reviewed-by: Miquel Raynal <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- When -> when
Change from v1 to v2
- update commit message
- fix typo yeild and falure
Change from v1 to v2
- update commit message
- fix typo yeild and falure
drivers/i3c/master/svc-i3c-master.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 8cd708c965085..abebef666b2bb 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1010,6 +1010,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
u32 reg;
int ret;
+ /* clean SVC_I3C_MINT_IBIWON w1c bits */
+ writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
xfer_type |
SVC_I3C_MCTRL_IBIRESP_NACK |
@@ -1028,6 +1031,23 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
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, return failure
+ * and yield the above events handler.
+ */
+ if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+ ret = -ENXIO;
+ goto emit_stop;
+ }
+
if (rnw)
ret = svc_i3c_master_read(master, in, xfer_len);
else
--
2.34.1
svc_i3c_master_irq_handler() wrongly checks register SVC_I3C_MINTMASKED. It
should be SVC_I3C_MSTATUS.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Reviewed-by: Miquel Raynal <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v2 to v3
- 'wrong check' -> 'wrongly checks'
Change from v1 to v2
- Add Reviewed-by: Miquel Raynal <[email protected]>
Change from v1 to v2
- Add Reviewed-by: Miquel Raynal <[email protected]>
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 dd06b7c9333f1..b113460f059c3 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -476,7 +476,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
{
struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
- u32 active = readl(master->regs + SVC_I3C_MINTMASKED);
+ u32 active = readl(master->regs + SVC_I3C_MSTATUS);
if (!SVC_I3C_MSTATUS_SLVSTART(active))
return IRQ_NONE;
--
2.34.1
Upon IBIWON timeout, the SDA line will always be kept low if we don't emit
a stop. Calling svc_i3c_master_emit_stop() there will let the bus return to
idle state.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Reviewed-by: Miquel Raynal <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- remove duplidate in commit message
Call svc_i3c_master_emit_stop() to let i3c bus come back to idle statue
when IBIWON timeout happen.
Change from v2 to v3
- Update commit message and add review tag
Change from v1 to v2
- Update commite message
Change from v1 to v2
- Update commite message
Change from v2 to v3
- Update commit message and add review tag
Change from v1 to v2
- Update commite message
Change from v1 to v2
- Update commite message
drivers/i3c/master/svc-i3c-master.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index b113460f059c3..1a57fdebaa26d 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -404,6 +404,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
if (ret) {
dev_err(master->dev, "Timeout when polling for IBIWON\n");
+ svc_i3c_master_emit_stop(master);
goto reenable_ibis;
}
--
2.34.1
master side report:
silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
BIT 20: TIMEOUT error
The module has stalled too long in a frame. This happens when:
- The TX FIFO or RX FIFO is not handled and the bus is stuck in the
middle of a message,
- No STOP was issued and between messages,
- IBI manual is used and no decision was made.
The maximum stall period is 100 μs.
This can be considered as being just a warning as the system IRQ latency
can easily be greater than 100us.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- fixed missed update commit message
Add dev_dbg for TIMEOUT case
This can be considered as being just a warning as the system IRQ latency
can easily be greater than 100us.
Change from v2 to v3
- remove 10k at commit message
Change from v1 to v2
-none
Change from v1 to v2
-none
Change from v2 to v3
- remove 10k at commit message
Change from v1 to v2
-none
Change from v1 to v2
-none
drivers/i3c/master/svc-i3c-master.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 1a57fdebaa26d..6b6bdd163af4f 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -93,6 +93,7 @@
#define SVC_I3C_MINTMASKED 0x098
#define SVC_I3C_MERRWARN 0x09C
#define SVC_I3C_MERRWARN_NACK BIT(2)
+#define SVC_I3C_MERRWARN_TIMEOUT BIT(20)
#define SVC_I3C_MDMACTRL 0x0A0
#define SVC_I3C_MDATACTRL 0x0AC
#define SVC_I3C_MDATACTRL_FLUSHTB BIT(0)
@@ -226,6 +227,14 @@ static bool svc_i3c_master_error(struct svc_i3c_master *master)
if (SVC_I3C_MSTATUS_ERRWARN(mstatus)) {
merrwarn = readl(master->regs + SVC_I3C_MERRWARN);
writel(merrwarn, master->regs + SVC_I3C_MERRWARN);
+
+ /* Ignore timeout error */
+ if (merrwarn & SVC_I3C_MERRWARN_TIMEOUT) {
+ dev_dbg(master->dev, "Warning condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
+ mstatus, merrwarn);
+ return false;
+ }
+
dev_err(master->dev,
"Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
mstatus, merrwarn);
--
2.34.1
MSTATUS[RXPEND] is only updated after the data transfer cycle started. This
creates an issue when the I3C clock is slow, and the CPU is running fast
enough that MSTATUS[RXPEND] may not be updated when the code reaches
checking point. As a result, mandatory data can be missed.
Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data is
already in FIFO. It also works without mandatory data.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Reviewed-by: Miquel Raynal <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- add review tag Miquel
Change from v2 to v3
- update commit message and add no mandatory data in commits message
Change from v1 to v2
- update commit message
it also works without mandatory bytes
Change from v1 to v2
- update commit message
it also works without mandatory bytes
Change from v2 to v3
- update commit message and add no mandatory data in commits message
Change from v1 to v2
- update commit message
it also works without mandatory bytes
Change from v1 to v2
- update commit message
it also works without mandatory bytes
drivers/i3c/master/svc-i3c-master.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index abebef666b2bb..dd06b7c9333f1 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -332,6 +332,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
struct i3c_ibi_slot *slot;
unsigned int count;
u32 mdatactrl;
+ int ret, val;
u8 *buf;
slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
@@ -341,6 +342,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
slot->len = 0;
buf = slot->data;
+ ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
+ SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000);
+ if (ret) {
+ dev_err(master->dev, "Timeout when polling for COMPLETE\n");
+ return ret;
+ }
+
while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
slot->len < SVC_I3C_FIFO_SIZE) {
mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
--
2.34.1
Hi Frank,
[email protected] wrote on Mon, 23 Oct 2023 12:16:58 -0400:
> master side report:
> silvaco-i3c-master 44330000.i3c-master: Error condition: MSTATUS 0x020090c7, MERRWARN 0x00100000
>
> BIT 20: TIMEOUT error
> The module has stalled too long in a frame. This happens when:
> - The TX FIFO or RX FIFO is not handled and the bus is stuck in the
> middle of a message,
> - No STOP was issued and between messages,
> - IBI manual is used and no decision was made.
> The maximum stall period is 100 μs.
>
> This can be considered as being just a warning as the system IRQ latency
> can easily be greater than 100us.
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[email protected]>
Reviewed-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
On Mon, 23 Oct 2023 12:16:52 -0400, Frank Li wrote:
> Each patch is indepedents. See commit message for detail.
>
> Change from v1 to v4.
> See each patch notes
>
> If there are not notes between v2 to v4, that's means not change.
>
> [...]
Applied, thanks!
[1/6] i3c: master: svc: fix race condition in ibi work thread
commit: 6bf3fc268183816856c96b8794cd66146bc27b35
[2/6] i3c: master: svc: fix wrong data return when IBI happen during start frame
commit: 5e5e3c92e748a6d859190e123b9193cf4911fcca
[3/6] i3c: master: svc: fix ibi may not return mandatory data byte
commit: c85e209b799f12d18a90ae6353b997b1bb1274a5
[4/6] i3c: master: svc: fix check wrong status register in irq handler
commit: 225d5ef048c4ed01a475c95d94833bd7dd61072d
[5/6] i3c: master: svc: fix SDA keep low when polling IBIWON timeout happen
commit: dfd7cd6aafdb1f5ba93828e97e56b38304b23a05
[6/6] i3c: master: svc: fix random hot join failure since timeout error
commit: 9aaeef113c55248ecf3ab941c2e4460aaa8b8b9a
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com