Each patch is indepedents. See commit message for detail.
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 | 50 ++++++++++++++++++++++++++++-
1 file changed, 49 insertions(+), 1 deletion(-)
--
2.34.1
svc_i3c_master_irq_handler() wrong check register SVC_I3C_MINTMASKED. It
should be SVC_I3C_MSTATUS.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[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 c252446b2bc5..5ab68d6e439d 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -475,7 +475,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
┌─────┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┌─────
SCL: ┘ └─────┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┘
───┐ ┌─────┐ ┌─────┐ ┌───────────┐
SDA: └───────────────────────┘ └─────┘ └─────┘ └─────
xxx╱ ╲╱ ╲╱ ╲╱ ╲╱ ╲
: xxx╲IBI ╱╲ Addr(0x0a) ╱╲ RW ╱╲NACK╱╲ S ╱
In-Band Interrupt (IBI) occurred and IBI work thread may not to be
scheduled. When svc_i3c_master_priv_xfers() initiates the I3C transfer
frame 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.
IBIWON check has been added in svc_i3c_master_xfer(). In case this
situation occurs, a failure is now returned to the driver.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
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 ebdb3ea1af9d..0f57a5f75e39 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1009,6 +1009,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 |
@@ -1027,6 +1030,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 falure
+ * and yeild 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
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 reach checking
point. As a result, mandatory data are being missed.
Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
in FIFO.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
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 0f57a5f75e39..c252446b2bc5 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -331,6 +331,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);
@@ -340,6 +341,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
Need call svc_i3c_master_emit_stop() release bus.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
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 5ab68d6e439d..5bca369d6912 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -403,6 +403,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 10 KHz or 100 μs.
This is a just warning. System irq thread schedule latency is possible
bigger than 100us. Just omit this waring.
Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
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 5bca369d6912..18bc277edc8a 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)
@@ -225,6 +226,11 @@ 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)
+ return false;
+
dev_err(master->dev,
"Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
mstatus, merrwarn);
--
2.34.1
Hi Frank,
[email protected] wrote on Mon, 16 Oct 2023 11:32:28 -0400:
> ┌─────┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┏──┐ ┌─────
> SCL: ┘ └─────┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┛ └──┘
> ───┐ ┌─────┐ ┌─────┐ ┌───────────┐
> SDA: └───────────────────────┘ └─────┘ └─────┘ └─────
> xxx╱ ╲╱ ╲╱ ╲╱ ╲╱ ╲
> : xxx╲IBI ╱╲ Addr(0x0a) ╱╲ RW ╱╲NACK╱╲ S ╱
>
> In-Band Interrupt (IBI) occurred and IBI work thread may not to be
If an In-Band... occurs and the IBI work thread is not
immediately scheduled, when svc... initiates an I3C transfer and
attempts...
> scheduled. When svc_i3c_master_priv_xfers() initiates the I3C transfer
> frame 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.
>
> IBIWON check has been added in svc_i3c_master_xfer(). In case this
Add IBIWON check in svc_...
> situation occurs, a failure is now returned to the driver.
return a failure...
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[email protected]>
> ---
> 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 ebdb3ea1af9d..0f57a5f75e39 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1009,6 +1009,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 |
> @@ -1027,6 +1030,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 falure
> + * and yeild the above events handler.
Typos: yeild and falure
> + */
> + if (SVC_I3C_MSTATUS_IBIWON(reg)) {
> + ret = -ENXIO;
> + goto emit_stop;
> + }
> +
> if (rnw)
> ret = svc_i3c_master_read(master, in, xfer_len);
> else
With all the typos fixed:
Reviewed-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
Hi Frank,
[email protected] wrote on Mon, 16 Oct 2023 11:32:29 -0400:
> 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 reach checking
> point. As a result, mandatory data are being missed.
can be missed.
> Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
> in FIFO.
is already in the FIFO
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[email protected]>
> ---
> 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 0f57a5f75e39..c252446b2bc5 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -331,6 +331,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);
> @@ -340,6 +341,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);
Are you sure !MSTATUS_COMPLETE(val) is never a valid condition?
Especially with non-mandatory bytes?
Also, are you sure of the indentation here?
> + 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);
Thanks,
Miquèl
Hi Frank,
[email protected] wrote on Mon, 16 Oct 2023 11:32:30 -0400:
> svc_i3c_master_irq_handler() wrong check register SVC_I3C_MINTMASKED. It
wrongly checks
> should be SVC_I3C_MSTATUS.
Ah right, good catch.
Reviewed-by: Miquel Raynal <[email protected]>
>
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[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 c252446b2bc5..5ab68d6e439d 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -475,7 +475,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;
Thanks,
Miquèl
Hi Frank,
[email protected] wrote on Mon, 16 Oct 2023 11:32:31 -0400:
> Need call svc_i3c_master_emit_stop() release bus.
Can you please elaborate this commit message please?
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[email protected]>
> ---
> 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 5ab68d6e439d..5bca369d6912 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -403,6 +403,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;
> }
>
Thanks,
Miquèl
Hi Frank,
[email protected] wrote on Mon, 16 Oct 2023 11:32:32 -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 10 KHz or 100 μs.
>
> This is a just warning. System irq thread schedule latency is possible
can be bigger
> bigger than 100us. Just omit this waring.
I'm not sure this is the correct approach. It's a real issue but there
is not much we can do about it. Perhaps dev_err is too high, but I
would not entirely drop this message. Maybe a comment and turning the
message into a dbg printk would be more appropriate?
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Cc: [email protected]
> Signed-off-by: Frank Li <[email protected]>
> ---
> 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 5bca369d6912..18bc277edc8a 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)
> @@ -225,6 +226,11 @@ 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)
> + return false;
> +
> dev_err(master->dev,
> "Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
> mstatus, merrwarn);
Thanks,
Miquèl
On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> [email protected] wrote on Mon, 16 Oct 2023 11:32:32 -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 10 KHz or 100 μs.
> >
> > This is a just warning. System irq thread schedule latency is possible
>
> can be bigger
> > bigger than 100us. Just omit this waring.
>
> I'm not sure this is the correct approach. It's a real issue but there
> is not much we can do about it. Perhaps dev_err is too high, but I
> would not entirely drop this message. Maybe a comment and turning the
> message into a dbg printk would be more appropriate?
The key is not message. It return true, means IBI/HJ thread will not run.
Frank
>
> > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> > Cc: [email protected]
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> > 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 5bca369d6912..18bc277edc8a 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)
> > @@ -225,6 +226,11 @@ 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)
> > + return false;
> > +
> > dev_err(master->dev,
> > "Error condition: MSTATUS 0x%08x, MERRWARN 0x%08x\n",
> > mstatus, merrwarn);
>
>
> Thanks,
> Miquèl
Hi Frank,
[email protected] wrote on Tue, 17 Oct 2023 10:45:14 -0400:
> On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >
> > [email protected] wrote on Mon, 16 Oct 2023 11:32:32 -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 10 KHz or 100 μs.
> > >
> > > This is a just warning. System irq thread schedule latency is possible
> >
> > can be bigger
> > > bigger than 100us. Just omit this waring.
> >
> > I'm not sure this is the correct approach. It's a real issue but there
> > is not much we can do about it. Perhaps dev_err is too high, but I
> > would not entirely drop this message. Maybe a comment and turning the
> > message into a dbg printk would be more appropriate?
>
> The key is not message. It return true, means IBI/HJ thread will not run.
But why should the workers run if it's too late?
Thanks,
Miquèl
On Tue, Oct 17, 2023 at 05:06:03PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> [email protected] wrote on Tue, 17 Oct 2023 10:45:14 -0400:
>
> > On Tue, Oct 17, 2023 at 04:33:35PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > >
> > > [email protected] wrote on Mon, 16 Oct 2023 11:32:32 -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 10 KHz or 100 μs.
> > > >
> > > > This is a just warning. System irq thread schedule latency is possible
> > >
> > > can be bigger
> > > > bigger than 100us. Just omit this waring.
> > >
> > > I'm not sure this is the correct approach. It's a real issue but there
> > > is not much we can do about it. Perhaps dev_err is too high, but I
> > > would not entirely drop this message. Maybe a comment and turning the
> > > message into a dbg printk would be more appropriate?
> >
> > The key is not message. It return true, means IBI/HJ thread will not run.
>
> But why should the workers run if it's too late?
IBI ACK already sent, target think master already accepted IBI. then master
driver check TIMEOUT,
If without run IBI thread, target's driver will wait target sent IBI. And
target wait for driver handle IBI.
So the whole system may lock or wait for long time out.
Hardware check TIMEOUT and software send ACK is totally async.
>
> Thanks,
> Miquèl
On Tue, Oct 17, 2023 at 04:27:07PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> [email protected] wrote on Mon, 16 Oct 2023 11:32:29 -0400:
>
> > 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 reach checking
> > point. As a result, mandatory data are being missed.
>
> can be missed.
>
> > Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already
> > in FIFO.
>
> is already in the FIFO
>
> >
> > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> > Cc: [email protected]
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> > 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 0f57a5f75e39..c252446b2bc5 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -331,6 +331,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);
> > @@ -340,6 +341,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);
>
> Are you sure !MSTATUS_COMPLETE(val) is never a valid condition?
> Especially with non-mandatory bytes?
>
> Also, are you sure of the indentation here?
If no-mandatory data, it is equal RDTERAM is 0. It takes some times to
change my slave code to create such test case.
Frank Li
>
> > + 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);
>
>
> Thanks,
> Miqu?l